* [RFH] How to review patches: Documentation/ReviewingPatches?
@ 2009-02-12 23:45 Jakub Narebski
2009-02-13 0:08 ` Johannes Schindelin
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Narebski @ 2009-02-12 23:45 UTC (permalink / raw)
To: git
Thanks to Documentation/SubmittingPatches we have gathered in one place
information and checklist on how to write good patches that have chance
to be accepted. Thanks to Documentation/CodingGuidelines we have
gathered in one place guidelines to keep to the code (with the most
important one "imitate the existing code" ;-)). (And thanks to
todo:MaintNotes we know how maintainer works...).
What I'd like to have is Documentation/ReviewingPatches to help with (at
least technical side of) reviewing patches, which is very important but
a hard work, c.f. http://www.kroah.com/log/linux/ols_2006_keynote.html
A few questions that it would be nice to have answered in proposed
future document:
* who can add 'Acked-by:', and when it could be added
* when one can (and should) add 'Tested-by:'.
* when to resend patches, how long to wait for review,
and when to send reminder (ping or resend, and when use which).
There is for example question whether (or when) to quote whole patch;
I think that for shorter patches it is better to quote them verbatim
wholesame, even if you refer only to parts of it, or only have comments
to the commit message. But for longer patches I think it makes sense
to quote selectively only the parts you are commenting on.
What about patch series? In my opinion if patch series has cover letter,
and doesn't use chained replies (i.e. all patches are replies to cover
letter), it leads to much more readable review discussion, but this
might be just me. Should one (if applicable) reply to cover letter
first with the impressions on the patch series as whole?
In my last review I have put summary of status for each patch in series
as reply to cover letter, in the shortlog for series (a summary of
comments). I think it is a good idea, and helps maintainer who doesn't
have then to read individual responses (subthreads) carefully... so I
guess it should be in Documentation/ReviewingPatches to make this
practice more widespread.
The other side is advice for patch authors how to respond to reviewers
comments... and warn them that they might be grumpy. To give
guidelines how to decide when reviewer is putting a request, giving an
alternate solution (perhaps better, perhaps worse), and when he/she
have doubts... or just makes idle discussion.
But perhaps such document (Documentation/ReviewingPatches) is not
needed? Reviewers should know the code in question well, so they
perhaps all are long-time contributors, and know all those rules by
heart...
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFH] How to review patches: Documentation/ReviewingPatches?
2009-02-12 23:45 [RFH] How to review patches: Documentation/ReviewingPatches? Jakub Narebski
@ 2009-02-13 0:08 ` Johannes Schindelin
2009-02-13 7:54 ` Marius Storm-Olsen
2009-02-15 1:14 ` Jakub Narebski
0 siblings, 2 replies; 6+ messages in thread
From: Johannes Schindelin @ 2009-02-13 0:08 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
Hi,
On Fri, 13 Feb 2009, Jakub Narebski wrote:
> What I'd like to have is Documentation/ReviewingPatches to help with (at
> least technical side of) reviewing patches, which is very important but
> a hard work, c.f. http://www.kroah.com/log/linux/ols_2006_keynote.html
As I found out quite painfully recently, reviewing is a matter of trust,
much more so than a matter of form or style.
I mean, it does not really hurt when somebody says ACK not understanding
that an ACK is expected to come from people who wrote that particular
code, or are at least very familiar with it. We know what the guy means,
and that's it.
However, it does hurt when somebody says "I tested this extensively, and
it works, I also reviewed the code, and it is correct" and you find out
much later that the review consisted of a run through "indent" and seeing
that there were no differences. Unsurprisingly, the patch had to be
reverted entirely.
It's much better to have somebody prove her capability as an excellent
reviewer, for example by offering comments that result in a better patch,
earning respect by others in the process.
Speaking for myself, it is also quite possible that you find out that your
reviewing fu is leaving to be desired. Concretely, it is widely known
that patches I reviewed still contain several issues after I find no more.
But at least I can serve as an early filter as long as I have the time.
There is another reason why I do not want any ReviewingPatches: reviewing
is already such a tedious process; let's not make it harder by forcing a
potential reviewer to sift through a document (the same could be said
about SubmittingPatches; IMHO it just repeats what common sense would do
anyway when imitating existing code).
I'd rather suggest to patch submitters to make such a good case that all
the world is interested in their patch, throwing a lot of eyeballs (AKA
review) at it.
BTW this is a pretty valuable skill, also (maybe especially) outside of
this mailing list, to get people interested in your work.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFH] How to review patches: Documentation/ReviewingPatches?
2009-02-13 0:08 ` Johannes Schindelin
@ 2009-02-13 7:54 ` Marius Storm-Olsen
2009-02-13 8:44 ` Junio C Hamano
2009-02-15 1:14 ` Jakub Narebski
1 sibling, 1 reply; 6+ messages in thread
From: Marius Storm-Olsen @ 2009-02-13 7:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narebski, git
[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]
Johannes Schindelin said the following on 13.02.2009 01:08:
> There is another reason why I do not want any ReviewingPatches:
> reviewing is already such a tedious process; let's not make it
> harder by forcing a potential reviewer to sift through a document
> (the same could be said about SubmittingPatches; IMHO it just
> repeats what common sense would do anyway when imitating existing
> code).
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?
Though, ignoring mail threads is fairly trivial with threading MUAs
;-) (I've used "Mark thread as read" quite a bit lately :-)
Any opinions, preferably from those that review a lot, and apply
patches directly from the mailing list? Maybe this could qualify as a
section in Documentation/SubmittingPatches?
--
.marius [@trolltech.com]
'if you know what you're doing, it's not research'
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFH] How to review patches: Documentation/ReviewingPatches?
2009-02-13 7:54 ` Marius Storm-Olsen
@ 2009-02-13 8:44 ` Junio C Hamano
2009-02-13 11:05 ` Johannes Schindelin
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2009-02-13 8:44 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Johannes Schindelin, Jakub Narebski, git
Marius Storm-Olsen <marius@trolltech.com> writes:
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFH] How to review patches: Documentation/ReviewingPatches?
2009-02-13 8:44 ` Junio C Hamano
@ 2009-02-13 11:05 ` Johannes Schindelin
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Schindelin @ 2009-02-13 11:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Marius Storm-Olsen, Jakub Narebski, git
Hi,
On Fri, 13 Feb 2009, Junio C Hamano wrote:
> Marius Storm-Olsen <marius@trolltech.com> writes:
>
> > 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?
>
> * Resending the whole series would help, especially if their earlier
> round did not hit 'pu'.
Note that I chose to do it differently quite a number of times. When I
feel that a particular part of the patch series is in deep discussion
mode, I will reply to the discussions with updates to that particular
patch, often only as an interdiff.
When I feel that the result is in a shape that could be applied, or when I
feel that people are substantially confused as to what is the current
state, I send out a whole updated series. This is to avoid sending
v1..v99 of an 18-strong patch series, and basically dominate the volume of
the list.
> Subsytem maintainers like Paulus for gitk, Shawn for git-gui and bash
> completion, Eric for git-svn, and Alexandre for emacs really have helped,
... and Jakub for gitweb, Simon for git-p4, Hannes for mingw.git, the New
Zealand gang for cvsserver/cvsimport, not to forget Shawn for
fast-import... It is really great to see all that development going on!
Ciao,
Dscho
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFH] How to review patches: Documentation/ReviewingPatches?
2009-02-13 0:08 ` Johannes Schindelin
2009-02-13 7:54 ` Marius Storm-Olsen
@ 2009-02-15 1:14 ` Jakub Narebski
1 sibling, 0 replies; 6+ messages in thread
From: Jakub Narebski @ 2009-02-15 1:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Fri, 13 Feb 2009, Johannes Schindelin wrote:
> There is another reason why I do not want any ReviewingPatches: reviewing
> is already such a tedious process; let's not make it harder by forcing a
> potential reviewer to sift through a document (the same could be said
> about SubmittingPatches; IMHO it just repeats what common sense would do
> anyway when imitating existing code).
>
> I'd rather suggest to patch submitters to make such a good case that all
> the world is interested in their patch, throwing a lot of eyeballs (AKA
> review) at it.
Well, I thought of ReviewingPatches less as of listing set of rules to
follow, as in the case of SubmittingPatches (because there output is
processed by tools, and preserved), but rather as set of guidelines and
hints. Something like "rules" of programming :-).
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-02-15 1:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-02-12 23:45 [RFH] How to review patches: Documentation/ReviewingPatches? Jakub Narebski
2009-02-13 0:08 ` Johannes Schindelin
2009-02-13 7:54 ` Marius Storm-Olsen
2009-02-13 8:44 ` Junio C Hamano
2009-02-13 11:05 ` Johannes Schindelin
2009-02-15 1:14 ` Jakub Narebski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.