patch tracking

patch tracking

[Joe Buck]

The biggest problem facing GCC seems to be that patches aren't getting
reviewed.  I think it's a process problem; we don't have a system for
keeping track of patches, and the load on the people qualified to
approve patches is too high.  As a result, it's very difficult for
people without blanket write privs to effectively contribute to the
project; the turnaround for patch review can be months.  This means
we're not getting the problems fixed and the frustration level is high.

In thinking about what the ideal system should look like, I have the
following ideas (feel free to discard and come up with something better):

First, we want to decrease the number of patches that the overworked
experts must handle, by having a pre-screening process.  So, we have
a team of volunteers who have some experience but are not necessarily
gurus.  These folks would be the first screen for incoming patches.  They
would ask the following questions:

1) Are there papers on file for the patch submitter?  (The volunteers
would need to have a copy of the list to verify this).

(If not, ask submitter to file papers and resubmit the patch when
complete)

2) Does the patch follow the GNU coding guidelines?  If not, ask submitter
to revise.  (not worth nitpicking over extremely tiny violations here,
since this is not the final review)

3) Has the submitter indicated that the patch has been adequately
tested, and that the rules on http://gcc.gnu.org/contribute.html
have been followed?  If not, confirm, ask to resubmit.

4) Does the patch contain obvious bogosities?  If so, kick it back.

Note that these questions apply to code changes, not documentation
changes.  We may want to split off proposed patches to the manual
(that do not accompany code changes) or to the web site onto a separate
list that would run the way we do now, since we don't have the same
problems there. (gcc-doc-patches, say).

Next, we want to carefully track patches that pass the pre-screening
process to make sure that they get a timely review and are added if
appropriate.

If these tests are all passed, then the patch would enter a tracking
system.  Perhaps GNATS can be used, even though the patch is not exactly
a bug.  A new category proposed-patch can be created with the patch
as an attachment.  We can come up with conventions as to how to use
the severity and priority fields, we can keep track of what has been
analyzed, etc.

I realize that GNATS is not ideal for patch-tracking, but it's already
in place.

Thoughts?

Joe

Re: patch tracking

[Diego Novillo]

On Fri, 14 Sep 2001, Joe Buck wrote:

> I realize that GNATS is not ideal for patch-tracking, but it's already
> in place.
> 
> Thoughts?
> 
I second the idea.  We use a similar system internally.  We have
GNATS categories for each major component of the compiler.  Each
category  has a primary and a secondary reviewer.

The system is setup so that primary reviewers are not necessarily
experts, but wish to gain more experience in that area.  If a
patch is too elaborate, the secondary reviewer provides
assistance.

Patches submitted to the internal GNATS database are copied to
the developer mailing list as well, so that anybody can jump in
if they want to contribute.

The patches have these states:

open::Default state for a new patch.
assigned::Patch assigned to a reviewer.
feedback::Patch is awaiting further explanation from submitter.
applied:closed:Patch has been applied. No further work on it is necessary.
rejected:closed:Patch has been rejected by reviewer or withdrawn by submitter.
closed:closed:This patch no longer active; it is resolved or otherwise defunct.


Diego.

Re: patch tracking

[egcs]

On Fri, Sep 14, 2001 at 12:02:07PM -0700, Joe Buck wrote:
> The biggest problem facing GCC seems to be that patches aren't getting
> reviewed.  I think it's a process problem; we don't have a system for
> keeping track of patches, and the load on the people qualified to
> approve patches is too high.  As a result, it's very difficult for
> people without blanket write privs to effectively contribute to the
> project; the turnaround for patch review can be months.  This means
> we're not getting the problems fixed and the frustration level is high.

savannah.gnu.org is running sourceforge which contains a patch
manager. Maybe this could be an option.

-- 
albert chin (china@thewrittenword.com)

Re: patch tracking

[Joseph S. Myers]

On Fri, 14 Sep 2001, Joe Buck wrote:

> First, we want to decrease the number of patches that the overworked
> experts must handle, by having a pre-screening process.  So, we have
> a team of volunteers who have some experience but are not necessarily
> gurus.  These folks would be the first screen for incoming patches.  They
> would ask the following questions:

We *really* need this for bugs as well.  Volunteers for bug checking don't
need great expertise (though they do need some familiarity with common
pitfalls of the programming language): they should

	* Check that all relevant information about version, target, etc., 
	is included.  If enough information to reproduce isn't present, 
	put the report in feedback state.

	* Test the bug against the current release *and* mainline CVS, if 
	possible on the same target.  (Also using -Wall to detect possible 
	bugs in the code, etc..)  If the bug report is a --enable-checking 
	one, make sure they don't just check against a release with 
	enable-checking turned off.  If the bug report claims that a 
	release branch fix (2.95 or 3.0) is warranted, e.g. because of a 
	regression, don't automatically close it because a fix is on the 
	mainline.

	* If the bug is no longer present, close as fixed; if present and
	seems valid, mark analysed, detailing the tested version (and
	maybe adjust category).  If they don't have the platform to test,
	request help with this.  If the bug report is bogus, explain why 
	and close it.  If the bug report title is unhelpful, make it 
	clearer.  If a valid, reproducable bug was just sent to gcc-bugs, 
	enter it in GNATS.

This would help make GNATS more usable for finding problems to work on.  
Perhaps some of the volunteers who helped with bug review for 3.0 would be
willing to help with this on an ongoing basis?  Presumably GNATS-only
accounts can be made available for people volunteering for bug review.

> Note that these questions apply to code changes, not documentation
> changes.  We may want to split off proposed patches to the manual
> (that do not accompany code changes) or to the web site onto a separate
> list that would run the way we do now, since we don't have the same
> problems there. (gcc-doc-patches, say).

Every one of those questions applies to documentation changes as well
(though I do attempt to review them for Texinfo problems, but have no
authority to approve most of them).  The patch review problems apply to
them as well; they can wait months for someone to look at them.

> If these tests are all passed, then the patch would enter a tracking
> system.  Perhaps GNATS can be used, even though the patch is not exactly
> a bug.  A new category proposed-patch can be created with the patch
> as an attachment.  We can come up with conventions as to how to use
> the severity and priority fields, we can keep track of what has been
> analyzed, etc.
> 
> I realize that GNATS is not ideal for patch-tracking, but it's already
> in place.

Get a way to go from a web archive URL to a plain text version of a
message (tabs intact, etc.).  (For example, perhaps it could be arranged
for archived messages to include the number of the message in ezmlm's
archives (or the actual *get* email address), so people could mail ezmlm
to retrieve the message, or perhaps every message could have corresponding
files with the plain text of the message and the plain text after MIME
decoding.)  Then patches don't need duplicating in GNATS.

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: patch tracking

[Phil Edwards]

On Fri, Sep 14, 2001 at 08:28:18PM +0100, Joseph S. Myers wrote:
> 	* Check that all relevant information about version, target, etc., 
> 	is included.  If enough information to reproduce isn't present, 
> 	put the report in feedback state.

I have occasionally tossed around a GNATS patch to disallow initial
submissions of high-priority PRs; a PR must be submitted as low or medium,
and then a maintainer must push it to high.

This and other GCC-only changes might be made to GNATS in order to achieve
these goals.  While local changes are admittedly a pain, GNATS doesn't
change very often, and we're not obligated by holy writ to use stock
GNU source.  :-)

The patch is utterly untested because I don't know how to run a GNATS
server, and don't have the time (or interest, really) to teach myself.
If someone else wants to start modifying GNATS to meet these goals, I'll
happily turn the patch over.


Phil

-- 
Would I had phrases that are not known, utterances that are strange, in
new language that has not been used, free from repetition, not an utterance
which has grown stale, which men of old have spoken.
                                     - anonymous Egyptian scribe, c.1700 BC

Re: patch tracking

[Zack Weinberg]

On Fri, Sep 14, 2001 at 12:02:07PM -0700, Joe Buck wrote:
> 
> Next, we want to carefully track patches that pass the pre-screening
> process to make sure that they get a timely review and are added if
> appropriate.
> 
> If these tests are all passed, then the patch would enter a tracking
> system.  Perhaps GNATS can be used, even though the patch is not exactly
> a bug.  A new category proposed-patch can be created with the patch
> as an attachment.  We can come up with conventions as to how to use
> the severity and priority fields, we can keep track of what has been
> analyzed, etc.

I have been considering a very simple patch tracker which would work
by monitoring the gcc-patches and gcc-cvs mailing lists.  The
principle would be: every time a patch is posted to gcc-patches, it
records the subject line and change log.  (If there is no change log,
it sends an automatic note to the submitter reminding them that they
need to provide change logs.)  When it sees a checkin go by on gcc-cvs
with (nearly) the same change log, it drops the patch from its
database.

Follow-ups to any given patch are detected by subject line, possibly
also References: and In-Reply-To: headers.  As long as a patch is
under active discussion, the tracker does nothing.  However, if a week
goes by with neither discussion nor check-in of any given patch, the
tracker will generate a nag message reminding people to review the
patch.

I have not decided whether the tracker should generate one big nag
message per week, or follow up to each individual thread.  The former
would be less obnoxious, but then obnoxiousness is kind of the point.

There needs to be a way to tell the tracker that a patch has been
dropped or rejected, but I don't know what is appropriate.  I want
it to be hard to do that, because far too often people get discouraged
and withdraw patches that are necessary - they may not have submitted
exactly the right patch for their bug, but we forget that there really
was a bug that needed to get fixed.

There also needs to be a way to indicate that a patch supersedes a
previous one, probably by writing "This patch supersedes <message-id>"
in the text of the new message.

The program needs a way to get the URL of an archived message to
gcc-patches.  Ideally it would be possible to derive that directly
from the message-id; less ideally, the web server could be queried
somehow; even less ideally, the program could run on gcc.gnu.org and
dig through the archive.  (I'd like to run the bot on a machine under
my direct control at least at first.)

zw

Re: patch tracking

[Phil Edwards]

On Fri, Sep 14, 2001 at 02:02:54PM -0700, Zack Weinberg wrote:
> I have not decided whether the tracker should generate one big nag
> message per week, or follow up to each individual thread.  The former
> would be less obnoxious, but then obnoxiousness is kind of the point.

How about individual messages to the people privately, and one big summary
to the list?  If I were to receive N nag messages, it would be easier to
delete each message as I review the patch.  With a big summary, I must
remember which ones I have since reviewed; with individual nags, I can
simply look at which messages remain in my mailbox.


Although I must point out that the problem of patch queuing has been solved
before.  I strongly recommend adopting something used on sourceforge or
mozilla or whatever for GCC's purposes.  Else we're right back where we
started:  people needing to be freed up for patch review (Zack) are getting
stuck with other duties (writing patch queuing nagging programs).  Granted,
Zack's doing this already on his own, but it made a good example. *grin*


Phil

-- 
Would I had phrases that are not known, utterances that are strange, in
new language that has not been used, free from repetition, not an utterance
which has grown stale, which men of old have spoken.
                                     - anonymous Egyptian scribe, c.1700 BC

Re: patch tracking

[Joe Buck]

Phil writes:

> Although I must point out that the problem of patch queuing has been solved
> before.  I strongly recommend adopting something used on sourceforge or
> mozilla or whatever for GCC's purposes.

savannah.gnu.org is the GNU project's SourceForge machine, so it seems
if we want to try SourceForge facilities it's the one to use.

I'm not familiar with SourceForge facilities for patch tracking though.

Re: patch tracking

[Zack Weinberg]

On Fri, Sep 14, 2001 at 05:16:48PM -0400, Phil Edwards wrote:
> On Fri, Sep 14, 2001 at 02:02:54PM -0700, Zack Weinberg wrote:
> > I have not decided whether the tracker should generate one big nag
> > message per week, or follow up to each individual thread.  The former
> > would be less obnoxious, but then obnoxiousness is kind of the point.
> 
> How about individual messages to the people privately, and one big summary
> to the list?  If I were to receive N nag messages, it would be easier to
> delete each message as I review the patch.  With a big summary, I must
> remember which ones I have since reviewed; with individual nags, I can
> simply look at which messages remain in my mailbox.

Good idea, but which people do you mean?  One of the contributing
factors to this problem is that for large parts of the compiler there
is no one person who should be reminded to review patches.  Either
there is a group, and which one do you pick? or there is no one at
all, and you're left flooding the blanket write people, who are
already overloaded.

Better just to post nags to the list, I think.

> Although I must point out that the problem of patch queuing has been solved
> before.  I strongly recommend adopting something used on sourceforge or
> mozilla or whatever for GCC's purposes.  Else we're right back where we
> started:  people needing to be freed up for patch review (Zack) are getting
> stuck with other duties (writing patch queuing nagging programs).  Granted,
> Zack's doing this already on his own, but it made a good example. *grin*

I don't know what sourceforge or mozilla or whatever use, but I'm
willing to bet it's web-centric.  Maybe I'm strange but I infinitely
prefer systems which are email-centric.

Also, I'm not usually the person reviewing patches.  I'm usually the
person waiting for patches to be reviewed.  (We don't get that many
patches for cpplib over the transom, y'know.)  If I can use some of
that waiting time to avoid having to wait so much in the future, well,
why not?

zw

Re: patch tracking

[Joe Buck]

Zack writes:

> I have been considering a very simple patch tracker which would work
> by monitoring the gcc-patches and gcc-cvs mailing lists.  The
> principle would be: every time a patch is posted to gcc-patches, it
> records the subject line and change log...

The problem is that there are too many things on gcc-patches that are
not patches but rather are discussion of patches, or are completely
untested hacks that don't deserve tracking.  That's why I want the
first-level filter.  We would try to set a deadline of no more than
N days before any new message that is a patch is either bounced or
goes into the system to be tracked.

> The program needs a way to get the URL of an archived message to
> gcc-patches.

Maybe the way to combine the ideas is that when the pre-screeners
approve a patch for further processing, they insert the URL into
your patch-tracker program.

Joe

Re: patch tracking

[Zack Weinberg]

On Fri, Sep 14, 2001 at 02:52:34PM -0700, Joe Buck wrote:
> Zack writes:
> 
> > I have been considering a very simple patch tracker which would work
> > by monitoring the gcc-patches and gcc-cvs mailing lists.  The
> > principle would be: every time a patch is posted to gcc-patches, it
> > records the subject line and change log...
> 
> The problem is that there are too many things on gcc-patches that are
> not patches but rather are discussion of patches, 

It is trivial to detect patches in messages programmatically.

> or are completely untested hacks that don't deserve tracking.

I don't think it's the patch tracker's business to be making such
judgements, nor do I think that any approach that involves more human
effort on the front end will be successful.  The overwhelming problem,
as I see it, is patches that come in and never see a single follow-up
message.  Even if they are wrong.

The goal of my design is to fit as seamlessly as possible into the way
we work now.  We carry on discussing patches as we always have, but
now there is a safety net which prevents patches from being
forgotten.  

zw

Re: patch tracking

[Gerald Pfeifer]

On Fri, 14 Sep 2001, Joe Buck wrote:
> First, we want to decrease the number of patches that the overworked
> experts must handle, by having a pre-screening process.

Also, we should abondon the "Bootstrap running, okay if it passes? --
Please forget the previous patch, here is a new one!" scenario we have
been seeing increasingly often.

> I realize that GNATS is not ideal for patch-tracking, but it's already
> in place.

Some projects like FreeBSD are using GNATS for tracking patches; it's
not perfect, but it works relatively well AFAIK.

Gerald

(In the past I twice suggested to use GNATS to avoid patches falling
through the cracks, but in both cases there wasn't a single reply. OTOH,
my proposal was by far less elaborate than the current one. ;) )

Re: patch tracking

[Bernd Schmidt]

On Fri, 14 Sep 2001, Joe Buck wrote:

> The biggest problem facing GCC seems to be that patches aren't getting
> reviewed.

Even if I'm alone on this, let me just say that I also think there's a
problem that patches are sometimes installed without sufficient review.
Most people seem to think the constant breakage in the mainline is caused
by insufficient testing and can be avoided by making the check-in criteria
more painful and by resorting to automatic reversion if a patch turns out
to be a dud.  I'm of the opinion that sometime, patch review isn't done
as well as it should be done (testing cannot prove correctness, but
understanding can).

The real risk for quality is not getting too few patches reviewed, it's
getting too many patches installed.

When I to review a bugfix patch, I try to make sure I fully understand
the problem - that may involve debugging it myself.  For larger patches,
I still try to make sure that I understand it well enough so that I could
quickly fix a bug in it if it gets installed.  My opinioin is that if a
patch I reviewed causes breakage, I made a serious mistake.  In practice,
this means that I don't get to review very many patches, since anything
nontrivial takes hours to review, and there's a limited supply of those.

Now people may claim that egcs was formed to address this bottleneck.  I
know how frustrating it can be not to have a patch reviewed; I was
suffering from it for a long time as well.  But we have a lot more
contributors now than in 1997, and the number of experienced developers
doesn't grow as quickly as the number of submissions.  I don't think
there's a good solution to this problem.


Bernd

Re: patch tracking

[Joe Buck]

I wrote:
> > The biggest problem facing GCC seems to be that patches aren't getting
> > reviewed.

Berndt writes:
> Even if I'm alone on this, let me just say that I also think there's a
> problem that patches are sometimes installed without sufficient review.

Note that the two-level process I proposed is not intended to reduce the
amount of review any patch gets; quite the opposite.  Every patch has to
get through two rounds.  The difference is that we keep track so that
patches aren't lost.  It may still take a long time to get patches
installed, but at least we'll be able to see where each patch is in the
process.

We could perhaps increase the analysis requirements.  There are a few
developers who seem to be in the habit of contributing lots of patches
without much explanation of what problem is being fixed.  Such patches
could be punted back in the first round.  The final reviewer still must
verify that the analysis and patch are correct, of course.

There is one particular category of patch that I am especially eager
to speed up: patches that fix severe breakage (like "won't bootstrap")
on minority platforms.  If we can't find a way to do that, as time
goes on gcc will work on fewer and fewer platforms.  We may need to
have a priority-setting process, and the SC might have to get involved,
meaning that the SC would request developers to treat certain patches
as higher priority (not as pressure to include a possibly bogus patch,
but to actively work with contributors to fix certain problems).  Of
course, we are all volunteers so this prioritization would only be
a strong request.

Re: patch tracking

[Marc Espie]

In article < 200109162145.OAA02357@atrus.synopsys.com > you write:
>There is one particular category of patch that I am especially eager
>to speed up: patches that fix severe breakage (like "won't bootstrap")
>on minority platforms.  If we can't find a way to do that, as time
>goes on gcc will work on fewer and fewer platforms.  We may need to

Putting on my `opressed OpenBSD minority' hat...
I concur. 
I don't count the number of times I had to become completely obnoxious
just to get a nagging minor problem (and often complete with patches)
reviewed and in the mainline/the stable tree.

Many thanks to the very few fellow developers who can find the time to
help me (rth specifically, as he's helped me more often than not)

Re: patch tracking

[dewar]

<<The real risk for quality is not getting too few patches reviewed, it's
getting too many patches installed.
>>

I strongly agree with this assessment.