Re: [PATCH] Documentation: add ReviewingGuidelines

[Junio C Hamano <gitster@pobox.com>]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> +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.

> +- 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.

> +- 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.

> +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".

> +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.