Re: [RFH] How to review patches: Documentation/ReviewingPatches?

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2009, 08:44 UTC
Message-ID: <7vocx6bu9r.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <49952728.2080404@trolltech.com>

Marius Storm-Olsen <marius@trolltech.com> writes:

Show 12 quoted lines

> One thing I've wondered about though when sending patches, is how to
> send the fixups. Lets say I have a patch serie with 8 patches, do I
> send the whole serie each time, or do I just send an update to each
> individual patch? Do I attach it to the previous thread, or start a
> new one?
>
> I couldn't really draw any conclusion by watching the list, since all
> methods are used. However, I'd like to do what's easiest for the
> reviewers and maintainers. Probably a new series each time is easiest
> for Junio to parse and apply, without single updates deep in a
> thread. However, that might also be considered a tad 'spamming' of the
> list?

People work at different paces, especially because we are mostly volunteers and hobbists who work on git not on full-time basis [*1*].

Although I obviously appreciate if people make it easy for _me_ to process patches, and it may become necessary to optimize the rules to remove the maintainer bottleneck if/when the amount of useful patches in the overall list traffic starts to exceed my bandwidth [*2*], I do not think it is a healthy thing to implement rules to make contributors' life more difficult to make _my_ life easier.

So please do not take this message as me setting a rule. Take it just as a datapoint from me. Other reviewers may have different preference, and I am interested in hearing from them, too, especially their preference is different from mine.

 * Marking the second and the third iterations as [PATCH v2], [PATCH v3]
   really helps, especially if you are a busy contributor whose throughput
   exceeds reviewers' throughput.

 * Resending the whole series would help, especially if their earlier
   round did not hit 'pu'.  If an earlier round did not land on 'pu', it
   is a sign that I either did not read them carefully to judge if they
   are 'pu' worthy, I did not even look at it beyond their commit log
   messages, I thought they were outright wrong, or I saw objections from
   others that were reasonable.

 * Once you have an earlier round in 'pu', it is Ok to resend only the
   updated ones, with a cover letter that says "the second and the third
   ones are the same as the previous round, so I am sending the updates
   for the first one and the fourth one, and this round additionally has
   the fifth one."

   But I suspect resending the whole series may help reviewers who missed
   the previous round in this case, too.

 * If you are resending the same patch as the previous round, I'd really
   appreciate a single line comment "This is unchanged from the last
   round" after the three-dash marker.  I often end up saving two messages
   to temporary files and run diff on them to see if they are the same
   without such indication.

 * If you are sending an updated patch, unless the whole series has been
   re-split and there is no one-to-one correspondence with the previous
   round, it is appreciated if you list the changes from the previous
   round below the three-dash marker.  Many people already do this, and it
   helps when reading the interdiff with the previous version.

[Footnotes]

*1* I am allowed to work on git for 20% of my day-job time budget by my employer and NEC, so I am not a 100% full-time hobbist.

*2* At some point, I suspect we would have a problem similar to the one pre-BK Linux kernel project had, the "maintainer does not scale" problem.

Subsytem maintainers like Paulus for gitk, Shawn for git-gui and bash completion, Eric for git-svn, and Alexandre for emacs really have helped, as I can choose to either ignore or simply kibitz on patches in these areas, without having to worry about dropping patches in these areas.