From: Jakub Narebski <jnareb@gmail.com>
To: git@vger.kernel.org
Subject: [RFH] How to review patches: Documentation/ReviewingPatches?
Date: Fri, 13 Feb 2009 00:45:57 +0100 [thread overview]
Message-ID: <200902130045.59395.jnareb@gmail.com> (raw)
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
next reply other threads:[~2009-02-12 23:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-02-12 23:45 Jakub Narebski [this message]
2009-02-13 0:08 ` [RFH] How to review patches: Documentation/ReviewingPatches? 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200902130045.59395.jnareb@gmail.com \
--to=jnareb@gmail.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).