Re: [PATCH] Documentation: add ReviewingGuidelines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 9, 2022, 19:42 UTC
- Message-ID
- <xmqqwnacibbm.fsf@gitster.g>
- In-Reply-To
- <pull.1348.git.1662747205235.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> From: Victoria Dye <vdye@github.com> > > Add a reviewing guidelines document including advice and common terminology > used in Git mailing list reviews. The document is included in the > 'TECH_DOCS' list in order to include it in Git's published documentation. > > Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> > Helped-by: Derrick Stolee <derrickstolee@github.com> > Signed-off-by: Victoria Dye <vdye@github.com>
Thanks, all, for starting this.
> +Patches can also be searched manually in the mailing list archive using a query > +like `s:"PATCH" -s:"Re:"`. You can browse these results for topics relevant to > +your expertise or interest.
It probably is a good idea to say "the 'lore.kernel.org' mailing list archive" somewhere here, as the queries may not work on other archives like marc.info/?l=git archive.
Show 6 quoted lines
> +If you've already contributed to Git, you may also be CC'd in another > +contributor's patch series. These are usually topics where the author feels that > +your attention is warranted; this may be due to prior contributions, > +demonstrated expertise, and/or interest in related topics. There is no > +requirement to review these series, but you may find them easier to review as a > +result of your preexisting background knowledge on the topic.
I think "your attention is warranted" is a good way to summarize, but the readers may want to know that the reason for Cc'ing ranges from "you may want to know that I am about to butcher the code you wrote earlier, which you might care about (so I am giving a notice so that you can stop me and offer a better alternative)" to "your reviewing would really help making this patch better".
It is true that there is no requirement to review anything, but scratching each other's back, especially the reason for Cc'ing is to request help, is in the spirit of working on a piece of open source software. That may be more important than knowing that they may be easier to review than others.
> +- If a patch is long, you can delete parts of it that are unrelated to your > + review from the email reply. Make sure to leave enough context for readers to > + understand your comments!
"you can" -> "it is encouraged to"
> +- When pointing out an issue, try to include suggestions for how the author > + could fix it. This not only helps the author to understand and fix the issue, > + it also deepens and improves your understanding of the topic.
Thanks for saying "try to". Sometimes reviewers can tell something is wrong without being able to say what the alternative is that is right, but at least they should try before saying "that one is wrong" and stopping at it.
Show 6 quoted lines
> +- Reviews do not need to exclusively point out problems. Feel free to "think out > + loud" in your review: describe how you read & understood a complex section of > + a patch, ask a question about something that confused you, point out something > + you found exceptionally well-written, etc. In particular, uplifting feedback > + goes a long way towards encouraging contributors to participate more actively > + in the Git community.
Good piece of advice. It also helps if the authors understood the above. A review response to a patch may not necessarily point out the problems and they do not need to become defensive.
Show 7 quoted lines
> +- When providing a recommendation, be as clear as possible about whether you
> + consider it "blocking" (the code would be broken or otherwise made worse if an
> + issue isn't fixed) or "non-blocking" (the patch could be made better by taking
> + the recommendation, but acceptance of the series does not require it).
> + Non-blocking recommendations can be particularly ambiguous when they are
> + related to - but outside the scope of - a series ("nice-to-have"s), or when
> + they represent only stylistic differences between the author and reviewer.I hear that some communities take the "reviewer wins" approach to tiebreak the last one. In any case, it is a good piece of advice to make sure which parts are "just thinking out aloud" and which parts are pointing out problems in the patch that need to be corrected in the next iteration. The former may observe an existing problem in the code in context that may need to be addressed in the longer term but can be left out of the scope of the topic, and being clear about that is helpful to the author.
> +- If you read and review a series but find nothing that warrants inline > + commentary, reply to the series' cover letter to indicate that you've reviewed > + the changes.
I would prefer to see folks avoid doing this on the initial iteration of a topic, until/unless the reviewer has demonstrated proficiency in the affected area. If everybody considers that you are one of the authorities of revision traversal and the patch series is about updating revision.c, then such a "I've reviewed and everything is so clean there is nothing to say" may be helpful, but it is hard to put a proper weight on such a statement by somebody whose understanding of the area is not well known.
It is a different story to give such a "looks good" on a second and subsequent iteration by a reviewer who commented on an earlier round, of course.
Show 11 quoted lines
> +Completing a review > +~~~~~~~~~~~~~~~~~~~ > +Once each patch of a series is reviewed, the author (and/or other contributors) > +may discuss the review(s). This may result in no changes being applied, or the > +author will send a new version of their patch(es). > + > +After a series is rerolled in response to your or others' review, make sure to > +re-review the updates. If you are happy with the state of the patch series, > +explicitly indicate your approval (typically with a reply to the latest > +version's cover letter). Optionally, you can let the author know that they can > +add a "Reviewed-by: <you>" trailer to subsequent versions of their series.
"to subsequent versions" -> "if they resubmit the reviewed patch verbatim".
Show 5 quoted lines
> +Finally, subsequent "What's cooking" emails may explicitly ask whether a > +reviewed topic is ready for merging to the `next` branch (typically phrased > +"Will merge to 'next'?"). You can help the maintainer and author by responding > +with a short description of the state of your (and others', if applicable) > +review.
Thanks for mentioning this. "We have reached the agreement that, while this and that have room for improvement, the patch is a strict improvement over the status quo, and should move forward, at <URL>" that points into the lore archive would be the most easy and clear.
Everything else I did not quote from your patch looked good to me without any need to comment.
Thanks.