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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux