Re: Checkin Policy

[Craig Burley <burley at gnu dot org>]

>I've noticed that changes to reload, especially, have more than once
>resulted in seriously broken back-ends.  That's because working on
>reload is notoriously hard. [...]
>
>That said, I'd like to suggest, rather strongly this time, that we
>require people to run the regression tests before submitting patches,
>and certainly before comitting them. [...]
>
>Just as we require ChangeLog entries for submissions, I suggest that
>we also require the submitter to run the full set of regression tests
>for any back-end change, and the language-specific set for front-end
>changes.  I further suggest that middle-end changes be tested on an
>x86 target, except where inconvenient (no such machine is available)
>or illogical (the change only affects RISC platforms, say).  I don't
>think the submitter should have to *prove* that s/he ran the tests, or
>even assert so explicitly, but it should still be required.  And, if
>someone tends to forget to run the tests, the powers that be might
>encourage that person to explicitly state that they have done so
>before check-in.

I think there's lots of good sense in the above, but in a bazaar-style
development project like egcs, it could tend to slow down the pace
of sharing lots of "ordinary" patches.

My suggestion would be to apply the above advice to patches that
are not "clearly correct" as viewed by a programmer who thoroughly
understands the module in question and its interactions.

So, for example, that rules out pretty much any changes I'd make to
reload, but rules in most of what I do to the g77 front end on a
day-to-day basis.

(As far as what kind of testing to do: simply extend "clearly correct"
to specific areas.  E.g. changing the g77 front end code -- not
configury stuff, though -- in pretty much any way is "clearly correct"
for the gcc, g++, and other compilers, so *their* test suites need not
be run regardless.)

>Had this policy been in effect, Ulrich and I would not have had to
>track down the bug, and that time could have gone to making GCC, and
>the GNU C library, better.

This ultimately gets down to why I think "clearly correct" is a
reasonable basis for a checkin.  It's a balance between rapidly
sharing *improvements* to the code base versus rapidly delegating
responsibility for finding bugs in one's own changes to the rest of
the community.

By "improvements to the code base" I mean changes that, to someone
acquainted with the code "in that area", the patch appears to simplify
the design or implementation of the code and/or extend it to support
cases it previously would have failed for (a bug, or similar).  In
a product as complicated as egcs, such improvements can easily result
in overall bugs due to complicated interactions with other modules,
including less-than-ideally-documented work-arounds that depend on
the fixed module *not* having been fixed.  But, that doesn't mean
the patch isn't an improvement to the code base itself.

Looking at the extreme cases: a "clearly correct" patch might well
break every single test case, perhaps even produce a compiler that
cannot bootstrap itself on any target.  Nevertheless, the sharing of
that patch allows the rest of the community to assess whether it *is*
clearly correct and, if so, how to properly fix the compiler assuming
it is accepted.  (Of course, it might be ultimately rejected because
fixing the rest of the compiler is deemed "too difficult for now", but
the community as a whole might make that decision more appropriately
than the original programmer.  One or two such cases a year might seem
annoying, but many more can provide evidence that the code base is
due for some kind of scheduled overhaul -- which is why I prefer people
to feel some freedom to share such clear improvements, generally.)

On the other hand, if a programmer feels his "clearly correct" patch
cannot be checked in until he gets all the previously passing tests to
pass, he might feel unwilling or unable to share it, or even come up
with it in the first place.

But, I do agree that pretty much anyone making a change to something
known to be as crucial and inscrutable as reload should seriously
consider running the tests before checking it in.  This wouldn't
include things like fixing obvious typos.

And, it's important that people understand that "clearly correct" *never*
means "seems to fix a test case I happen to be working on right now".
That's more of an "I kicked it here and the engine stopped sputtering"
kind of fix, but it is emphatically *not* a proof that the patch itself
is "clearly correct".

In fact, a general rule of thumb about "clearly correct" patches is that
their "clearly correct" status, in a programmer's view, is almost entirely
impossible to change by the results of comparing test runs (between the
patched and unpatched product) of any sort.  As soon as one must point
to test cases, one is no longer in the realm of "clearly correct".

Fortunately, egcs-patches works pretty well as a forum for discussing
patches, and their possible worst-case effects, *before* they get
checked in.  Even people with write privileges should consider using
that, and taking advice to run the tests, prior to checking in patches
that aren't "clearly correct".

        tq vm, (burley)