[PATCH] Documentation: add ReviewingGuidelines

[PATCH] Documentation: add ReviewingGuidelines

[Victoria Dye via GitGitGadget]

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>
---
    Documentation: add ReviewingGuidelines
    
    This patch follows up on a discussion a few weeks ago in the Git IRC
    standup [1], where it was mentioned that it would be nice to have
    consistent definitions for common review terminology (like 'nit:'). The
    "ReviewingGuidelines" document created here builds on that idea, as well
    as past discussions around the idea of advice for reviewers (similar to
    the guidelines for new contributors in MyFirstContribution [2]).
    
    The goal of this document is to clarify & standardize some of the more
    niche concepts important to the Git project ("What's cooking" emails,
    terminology), as well as provide general reviewing advice based on my
    observations of effective reviews from others on the mailing list.
    
    One thing that's particularly important to me here is that the advice
    presented here does not gatekeep or otherwise denigrate the personal
    preferences or style of reviewers. With that in mind, one of the things
    I'm looking for in reviews of this document is making sure that the tone
    & content reflect that more positive/encouraging intent. And, of course,
    I'm happy to hear what other tips & terminology people think would be
    helpful to include!
    
    Thanks!
    
     * Victoria
    
    [1]
    https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53
    [2] https://git-scm.com/docs/MyFirstContribution

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1348

 Documentation/Makefile                |   1 +
 Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++
 2 files changed, 161 insertions(+)
 create mode 100644 Documentation/ReviewingGuidelines.txt

diff --git a/Documentation/Makefile b/Documentation/Makefile
index bd6b6fcb930..d3a19df8bed 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases
 API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))
 SP_ARTICLES += $(API_DOCS)
 
+TECH_DOCS += ReviewingGuidelines
 TECH_DOCS += MyFirstContribution
 TECH_DOCS += MyFirstObjectWalk
 TECH_DOCS += SubmittingPatches
diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt
new file mode 100644
index 00000000000..bcc59baf863
--- /dev/null
+++ b/Documentation/ReviewingGuidelines.txt
@@ -0,0 +1,160 @@
+Reviewing Patches in the Git Project
+====================================
+
+Introduction
+------------
+The Git development community is a widely distributed, diverse, ever-changing
+group of individuals. Asynchronous communication via the Git mailing list poses
+unique challenges when reviewing or discussing patches. This document contains
+some guiding principles and helpful tools you can use to make your reviews both
+more efficient for yourself and more effective for other contributors.
+
+Note that none of the recommendations here are binding or in any way a
+requirement of participation in the Git community. They are provided as a
+resource to supplement your skills as a contributor.
+
+Principles
+----------
+
+Selecting patch(es) to review
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If you are looking for a patch series in need of review, start by checking
+latest "What's cooking in git.git" email
+(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's
+cooking" emails & replies can be found using the query `s:"What's cooking"` on
+the mailing list archive; alternatively, you can find the contents of the
+"What's cooking" email tracked in `whats-cooking.txt` on the `todo` branch of
+Git. Topics tagged with "Needs review" and those in the "[New Topics]" section
+are typically those that would benefit the most from additional review.
+
+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.
+
+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.
+
+Reviewing patches
+~~~~~~~~~~~~~~~~~
+While every contributor takes their own approach to reviewing patches, here are
+some general pieces of advice to make your reviews to be as clear and helpful as
+possible.
+
+- Provide your review comments per-patch in a plaintext "Reply-All" email to the
+  relevant patch. Comments should be made inline, immediately below the relevant
+  section(s).
+
+- Remember to review the content of commit messages for correctness and clarity,
+  in addition to the code change in the patch's diff. The commit message of a
+  patch should accurately and fully explain the code change being made in the
+  diff.
+
+- You may find that the limited context provided in the patch diff is sometimes
+  insufficient for a thorough review. In such cases, you can review patches in
+  your local tree by either applying patches with linkgit:git-am[1] or checking
+  out the associated branch from https://github.com/gitster/git once the series
+  is tracked there.
+
+- Large, complicated patch diffs are sometimes unavoidable, such as when they
+  refactor existing code. If you find such a patch difficult to parse, try
+  reviewing the diff produced with the `--color-moved` and/or
+  `--ignore-space-change` options.
+
+- Reviewing test coverage is an important - but easy to overlook - component of
+  reviews. A patch's changes may be covered by existing tests, or new tests may
+  be introduced to exercise new behavior. Checking out a patch or series locally
+  allows you to manually mutate lines of new & existing tests to verify expected
+  pass/fail behavior. You can use this information to verify proper coverage or
+  to suggest additional tests the author could add.
+
+- 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!
+
+- 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.
+
+- 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.
+
+- 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.
+
+- If you cannot complete a full review of a series all at once, consider letting
+  the author know (on- or off-list) if/when you plan to review the rest of the
+  series.
+
+- 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.
+
+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.
+
+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.
+
+Terminology
+-----------
+nit: ::
+	Denotes a small issue that should be fixed, such as a typographical error
+	or mis-alignment of conditions in an `if()` statement.
+
+aside: ::
+optional: ::
+non-blocking: ::
+	Indicates to the reader that the following comment should not block the
+	acceptance of the patch or series. These are typically recommendations
+	related to code organization & style, or musings about topics related to
+	the patch in question, but beyond its scope.
+
+s/<before>/<after>/::
+	Shorthand for "you wrote <before>, but I think you meant <after>," usually
+	for misspellings or other typographical errors. The syntax is a reference
+	to "substitute" command commonly found in Unix tools such as `ed`, `sed`,
+	`vim`, and `perl`.
+
+cover letter::
+	The "Patch 0" of a multi-patch series. This email describes the
+	high-level intent and structure of the patch series to readers on the
+	Git mailing list. It is also where the changelog notes and range-diff of
+	subsequent versions are provided by the author.
++
+On single-patch submissions, cover letter content is typically not sent as a
+separate email. Instead, it is inserted between the end of the patch's commit
+message (after the `---`) and the beginning of the diff.
+
+#leftoverbits::
+  Used by either an author or a reviewer to describe features or suggested
+  changes that are out-of-scope of a given patch or series, but are relevant
+  to the topic for the sake of discussion.
+
+See Also
+--------
+link:MyFirstContribution.html[MyFirstContribution]

base-commit: 79f2338b3746d23454308648b2491e5beba4beff
-- 
gitgitgadget

Re: [PATCH] Documentation: add ReviewingGuidelines

[Junio C Hamano]

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

Re: [PATCH] Documentation: add ReviewingGuidelines

[Junio C Hamano]

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

I've commented on the text but haven't seen anybody else reviewing.
No interest?  Everybody silently happy?

Re: [PATCH] Documentation: add ReviewingGuidelines

[Victoria Dye]

Junio C Hamano wrote:
> "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>
>> ---
> 
> I've commented on the text but haven't seen anybody else reviewing.
> No interest?  Everybody silently happy?

My guess is that there aren't as many eyes on this as there might typically
be because of Git Merge. In any case, I plan to re-roll based on your
feedback [1] (ideally) by the end of the week if other reviews aren't sent
in the meantime. 

I'm hoping there's a bit more interest after Git Merge. With reviewing being
such an integral part of contribution to Git, I'm really interested in
hearing people's thoughts on what should/shouldn't be in this document.

[1] https://lore.kernel.org/git/xmqqwnacibbm.fsf@gitster.g/

Re: [PATCH] Documentation: add ReviewingGuidelines

[Josh Steadmon]

I like all the advice here, thanks for the patch! My only quibble is
that the items in the "Reviewing patches" section might be worth
re-ordering based on topic / importance. For example, I think
"philosophy" items like "patches should include test coverage" is more
important than tips on how to use diff flags, and so it would make sense
to be listed earlier. But this is subjective, and the doc is short so
in practice it probably doesn't matter.

Thanks again!

Reviewed-by: Josh Steadmon <steadmon@google.com>


On 2022.09.09 18:13, Victoria Dye via GitGitGadget wrote:
> 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>
> ---
>     Documentation: add ReviewingGuidelines
>     
>     This patch follows up on a discussion a few weeks ago in the Git IRC
>     standup [1], where it was mentioned that it would be nice to have
>     consistent definitions for common review terminology (like 'nit:'). The
>     "ReviewingGuidelines" document created here builds on that idea, as well
>     as past discussions around the idea of advice for reviewers (similar to
>     the guidelines for new contributors in MyFirstContribution [2]).
>     
>     The goal of this document is to clarify & standardize some of the more
>     niche concepts important to the Git project ("What's cooking" emails,
>     terminology), as well as provide general reviewing advice based on my
>     observations of effective reviews from others on the mailing list.
>     
>     One thing that's particularly important to me here is that the advice
>     presented here does not gatekeep or otherwise denigrate the personal
>     preferences or style of reviewers. With that in mind, one of the things
>     I'm looking for in reviews of this document is making sure that the tone
>     & content reflect that more positive/encouraging intent. And, of course,
>     I'm happy to hear what other tips & terminology people think would be
>     helpful to include!
>     
>     Thanks!
>     
>      * Victoria
>     
>     [1]
>     https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53
>     [2] https://git-scm.com/docs/MyFirstContribution
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1348
> 
>  Documentation/Makefile                |   1 +
>  Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++
>  2 files changed, 161 insertions(+)
>  create mode 100644 Documentation/ReviewingGuidelines.txt
> 
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index bd6b6fcb930..d3a19df8bed 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases
>  API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))
>  SP_ARTICLES += $(API_DOCS)
>  
> +TECH_DOCS += ReviewingGuidelines
>  TECH_DOCS += MyFirstContribution
>  TECH_DOCS += MyFirstObjectWalk
>  TECH_DOCS += SubmittingPatches
> diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt
> new file mode 100644
> index 00000000000..bcc59baf863
> --- /dev/null
> +++ b/Documentation/ReviewingGuidelines.txt
> @@ -0,0 +1,160 @@
> +Reviewing Patches in the Git Project
> +====================================
> +
> +Introduction
> +------------
> +The Git development community is a widely distributed, diverse, ever-changing
> +group of individuals. Asynchronous communication via the Git mailing list poses
> +unique challenges when reviewing or discussing patches. This document contains
> +some guiding principles and helpful tools you can use to make your reviews both
> +more efficient for yourself and more effective for other contributors.
> +
> +Note that none of the recommendations here are binding or in any way a
> +requirement of participation in the Git community. They are provided as a
> +resource to supplement your skills as a contributor.
> +
> +Principles
> +----------
> +
> +Selecting patch(es) to review
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +If you are looking for a patch series in need of review, start by checking
> +latest "What's cooking in git.git" email
> +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's
> +cooking" emails & replies can be found using the query `s:"What's cooking"` on
> +the mailing list archive; alternatively, you can find the contents of the
> +"What's cooking" email tracked in `whats-cooking.txt` on the `todo` branch of
> +Git. Topics tagged with "Needs review" and those in the "[New Topics]" section
> +are typically those that would benefit the most from additional review.
> +
> +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.
> +
> +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.
> +
> +Reviewing patches
> +~~~~~~~~~~~~~~~~~
> +While every contributor takes their own approach to reviewing patches, here are
> +some general pieces of advice to make your reviews to be as clear and helpful as
> +possible.
> +
> +- Provide your review comments per-patch in a plaintext "Reply-All" email to the
> +  relevant patch. Comments should be made inline, immediately below the relevant
> +  section(s).
> +
> +- Remember to review the content of commit messages for correctness and clarity,
> +  in addition to the code change in the patch's diff. The commit message of a
> +  patch should accurately and fully explain the code change being made in the
> +  diff.
> +
> +- You may find that the limited context provided in the patch diff is sometimes
> +  insufficient for a thorough review. In such cases, you can review patches in
> +  your local tree by either applying patches with linkgit:git-am[1] or checking
> +  out the associated branch from https://github.com/gitster/git once the series
> +  is tracked there.
> +
> +- Large, complicated patch diffs are sometimes unavoidable, such as when they
> +  refactor existing code. If you find such a patch difficult to parse, try
> +  reviewing the diff produced with the `--color-moved` and/or
> +  `--ignore-space-change` options.
> +
> +- Reviewing test coverage is an important - but easy to overlook - component of
> +  reviews. A patch's changes may be covered by existing tests, or new tests may
> +  be introduced to exercise new behavior. Checking out a patch or series locally
> +  allows you to manually mutate lines of new & existing tests to verify expected
> +  pass/fail behavior. You can use this information to verify proper coverage or
> +  to suggest additional tests the author could add.
> +
> +- 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!
> +
> +- 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.
> +
> +- 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.
> +
> +- 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.
> +
> +- If you cannot complete a full review of a series all at once, consider letting
> +  the author know (on- or off-list) if/when you plan to review the rest of the
> +  series.
> +
> +- 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.
> +
> +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.
> +
> +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.
> +
> +Terminology
> +-----------
> +nit: ::
> +	Denotes a small issue that should be fixed, such as a typographical error
> +	or mis-alignment of conditions in an `if()` statement.
> +
> +aside: ::
> +optional: ::
> +non-blocking: ::
> +	Indicates to the reader that the following comment should not block the
> +	acceptance of the patch or series. These are typically recommendations
> +	related to code organization & style, or musings about topics related to
> +	the patch in question, but beyond its scope.
> +
> +s/<before>/<after>/::
> +	Shorthand for "you wrote <before>, but I think you meant <after>," usually
> +	for misspellings or other typographical errors. The syntax is a reference
> +	to "substitute" command commonly found in Unix tools such as `ed`, `sed`,
> +	`vim`, and `perl`.
> +
> +cover letter::
> +	The "Patch 0" of a multi-patch series. This email describes the
> +	high-level intent and structure of the patch series to readers on the
> +	Git mailing list. It is also where the changelog notes and range-diff of
> +	subsequent versions are provided by the author.
> ++
> +On single-patch submissions, cover letter content is typically not sent as a
> +separate email. Instead, it is inserted between the end of the patch's commit
> +message (after the `---`) and the beginning of the diff.
> +
> +#leftoverbits::
> +  Used by either an author or a reviewer to describe features or suggested
> +  changes that are out-of-scope of a given patch or series, but are relevant
> +  to the topic for the sake of discussion.
> +
> +See Also
> +--------
> +link:MyFirstContribution.html[MyFirstContribution]
> 
> base-commit: 79f2338b3746d23454308648b2491e5beba4beff
> -- 
> gitgitgadget

Re: [PATCH] Documentation: add ReviewingGuidelines

[Derrick Stolee]

On 9/13/2022 7:11 PM, Victoria Dye wrote:
> Junio C Hamano wrote:
>> "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>
>>> ---
>>
>> I've commented on the text but haven't seen anybody else reviewing.
>> No interest?  Everybody silently happy?
> 
> My guess is that there aren't as many eyes on this as there might typically
> be because of Git Merge.

Yes, Git Merge took all of my attention in the past week, so I couldn't
chime in at all here.

My "Helped-by" includes some small suggestions from me, but mostly I
fully support having this kind of document. This one is an excellent
base to start from for future augmentation as we discover ideas that
could avoid sticky situations.

I particularly like how this document assumes good intent from all
parties, but recommends over-communicating to be sure that intent is
clear to everyone.

Thanks,
-Stolee

Re: [PATCH] Documentation: add ReviewingGuidelines

[Johannes Schindelin]

Hi,

On Mon, 19 Sep 2022, Derrick Stolee wrote:

> On 9/13/2022 7:11 PM, Victoria Dye wrote:
> > Junio C Hamano wrote:
> >> "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>
> >>> ---
> >>
> >> I've commented on the text but haven't seen anybody else reviewing.
> >> No interest?  Everybody silently happy?
> >
> > My guess is that there aren't as many eyes on this as there might typically
> > be because of Git Merge.
>
> Yes, Git Merge took all of my attention in the past week, so I couldn't
> chime in at all here.
>
> My "Helped-by" includes some small suggestions from me, but mostly I
> fully support having this kind of document. This one is an excellent
> base to start from for future augmentation as we discover ideas that
> could avoid sticky situations.
>
> I particularly like how this document assumes good intent from all
> parties, but recommends over-communicating to be sure that intent is
> clear to everyone.

What Stolee said.

I really like how this document serves as a great inspiration to align
actions with intentions (answering the question "How do I craft my review
in a way that the reader _sees_ my good intention, too?"; Sometimes there
is a disconnect between intent and impact).

Thank you for putting this together, Victoria!
Dscho

Re: [PATCH] Documentation: add ReviewingGuidelines

[Glen Choo]

Josh Steadmon <steadmon@google.com> writes:

> I like all the advice here, thanks for the patch!

I agree, I think this doc is a great step forward. Thanks!

>                                                   My only quibble is
> that the items in the "Reviewing patches" section might be worth
> re-ordering based on topic / importance.

I share this preference for organizing around the topic, especially
since it make it easier for us to think about what topics we haven't
adequately addressed. For example, I'm interested in how reviewers can
sustainably review and avoid burnout while still providing the reviews
that the project needs. (Perhaps we could help reviewers figure out when
they've hit the point of diminishing returns in their review?)

Nevertheless, I don't think this should block this patch from getting
merged; I find the doc very helpful as-is.

>
> On 2022.09.09 18:13, Victoria Dye via GitGitGadget wrote:
>> 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>
>> ---
>>     Documentation: add ReviewingGuidelines
>>     
>>     This patch follows up on a discussion a few weeks ago in the Git IRC
>>     standup [1], where it was mentioned that it would be nice to have
>>     consistent definitions for common review terminology (like 'nit:'). The
>>     "ReviewingGuidelines" document created here builds on that idea, as well
>>     as past discussions around the idea of advice for reviewers (similar to
>>     the guidelines for new contributors in MyFirstContribution [2]).
>>     
>>     The goal of this document is to clarify & standardize some of the more
>>     niche concepts important to the Git project ("What's cooking" emails,
>>     terminology), as well as provide general reviewing advice based on my
>>     observations of effective reviews from others on the mailing list.
>>     
>>     One thing that's particularly important to me here is that the advice
>>     presented here does not gatekeep or otherwise denigrate the personal
>>     preferences or style of reviewers. With that in mind, one of the things
>>     I'm looking for in reviews of this document is making sure that the tone
>>     & content reflect that more positive/encouraging intent. And, of course,
>>     I'm happy to hear what other tips & terminology people think would be
>>     helpful to include!
>>     
>>     Thanks!
>>     
>>      * Victoria
>>     
>>     [1]
>>     https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53
>>     [2] https://git-scm.com/docs/MyFirstContribution
>> 
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1348
>> 
>>  Documentation/Makefile                |   1 +
>>  Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++
>>  2 files changed, 161 insertions(+)
>>  create mode 100644 Documentation/ReviewingGuidelines.txt
>> 
>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>> index bd6b6fcb930..d3a19df8bed 100644
>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> @@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases
>>  API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))
>>  SP_ARTICLES += $(API_DOCS)
>>  
>> +TECH_DOCS += ReviewingGuidelines
>>  TECH_DOCS += MyFirstContribution
>>  TECH_DOCS += MyFirstObjectWalk
>>  TECH_DOCS += SubmittingPatches
>> diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt
>> new file mode 100644
>> index 00000000000..bcc59baf863
>> --- /dev/null
>> +++ b/Documentation/ReviewingGuidelines.txt
>> @@ -0,0 +1,160 @@
>> +Reviewing Patches in the Git Project
>> +====================================
>> +
>> +Introduction
>> +------------
>> +The Git development community is a widely distributed, diverse, ever-changing
>> +group of individuals. Asynchronous communication via the Git mailing list poses
>> +unique challenges when reviewing or discussing patches. This document contains
>> +some guiding principles and helpful tools you can use to make your reviews both
>> +more efficient for yourself and more effective for other contributors.
>> +
>> +Note that none of the recommendations here are binding or in any way a
>> +requirement of participation in the Git community. They are provided as a
>> +resource to supplement your skills as a contributor.
>> +
>> +Principles
>> +----------
>> +
>> +Selecting patch(es) to review
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +If you are looking for a patch series in need of review, start by checking
>> +latest "What's cooking in git.git" email
>> +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's
>> +cooking" emails & replies can be found using the query `s:"What's cooking"` on
>> +the mailing list archive; alternatively, you can find the contents of the
>> +"What's cooking" email tracked in `whats-cooking.txt` on the `todo` branch of
>> +Git. Topics tagged with "Needs review" and those in the "[New Topics]" section
>> +are typically those that would benefit the most from additional review.
>> +
>> +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.
>> +
>> +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.
>> +
>> +Reviewing patches
>> +~~~~~~~~~~~~~~~~~
>> +While every contributor takes their own approach to reviewing patches, here are
>> +some general pieces of advice to make your reviews to be as clear and helpful as
>> +possible.
>> +
>> +- Provide your review comments per-patch in a plaintext "Reply-All" email to the
>> +  relevant patch. Comments should be made inline, immediately below the relevant
>> +  section(s).
>> +
>> +- Remember to review the content of commit messages for correctness and clarity,
>> +  in addition to the code change in the patch's diff. The commit message of a
>> +  patch should accurately and fully explain the code change being made in the
>> +  diff.
>> +
>> +- You may find that the limited context provided in the patch diff is sometimes
>> +  insufficient for a thorough review. In such cases, you can review patches in
>> +  your local tree by either applying patches with linkgit:git-am[1] or checking
>> +  out the associated branch from https://github.com/gitster/git once the series
>> +  is tracked there.
>> +
>> +- Large, complicated patch diffs are sometimes unavoidable, such as when they
>> +  refactor existing code. If you find such a patch difficult to parse, try
>> +  reviewing the diff produced with the `--color-moved` and/or
>> +  `--ignore-space-change` options.
>> +
>> +- Reviewing test coverage is an important - but easy to overlook - component of
>> +  reviews. A patch's changes may be covered by existing tests, or new tests may
>> +  be introduced to exercise new behavior. Checking out a patch or series locally
>> +  allows you to manually mutate lines of new & existing tests to verify expected
>> +  pass/fail behavior. You can use this information to verify proper coverage or
>> +  to suggest additional tests the author could add.
>> +
>> +- 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!
>> +
>> +- 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.
>> +
>> +- 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.
>> +
>> +- 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.
>> +
>> +- If you cannot complete a full review of a series all at once, consider letting
>> +  the author know (on- or off-list) if/when you plan to review the rest of the
>> +  series.
>> +
>> +- 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.
>> +
>> +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.
>> +
>> +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.
>> +
>> +Terminology
>> +-----------
>> +nit: ::
>> +	Denotes a small issue that should be fixed, such as a typographical error
>> +	or mis-alignment of conditions in an `if()` statement.
>> +
>> +aside: ::
>> +optional: ::
>> +non-blocking: ::
>> +	Indicates to the reader that the following comment should not block the
>> +	acceptance of the patch or series. These are typically recommendations
>> +	related to code organization & style, or musings about topics related to
>> +	the patch in question, but beyond its scope.
>> +
>> +s/<before>/<after>/::
>> +	Shorthand for "you wrote <before>, but I think you meant <after>," usually
>> +	for misspellings or other typographical errors. The syntax is a reference
>> +	to "substitute" command commonly found in Unix tools such as `ed`, `sed`,
>> +	`vim`, and `perl`.
>> +
>> +cover letter::
>> +	The "Patch 0" of a multi-patch series. This email describes the
>> +	high-level intent and structure of the patch series to readers on the
>> +	Git mailing list. It is also where the changelog notes and range-diff of
>> +	subsequent versions are provided by the author.
>> ++
>> +On single-patch submissions, cover letter content is typically not sent as a
>> +separate email. Instead, it is inserted between the end of the patch's commit
>> +message (after the `---`) and the beginning of the diff.
>> +
>> +#leftoverbits::
>> +  Used by either an author or a reviewer to describe features or suggested
>> +  changes that are out-of-scope of a given patch or series, but are relevant
>> +  to the topic for the sake of discussion.
>> +
>> +See Also
>> +--------
>> +link:MyFirstContribution.html[MyFirstContribution]
>> 
>> base-commit: 79f2338b3746d23454308648b2491e5beba4beff
>> -- 
>> gitgitgadget

[PATCH v2] Documentation: add ReviewingGuidelines

[Victoria Dye via GitGitGadget]

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>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
    Documentation: add ReviewingGuidelines
    
    This patch follows up on a discussion a few weeks ago in the Git IRC
    standup [1], where it was mentioned that it would be nice to have
    consistent definitions for common review terminology (like 'nit:'). The
    "ReviewingGuidelines" document created here builds on that idea, as well
    as past discussions around the idea of advice for reviewers (similar to
    the guidelines for new contributors in MyFirstContribution [2]).
    
    The goal of this document is to clarify & standardize some of the more
    niche concepts important to the Git project ("What's cooking" emails,
    terminology), as well as provide general reviewing advice based on my
    observations of effective reviews from others on the mailing list.
    
    One thing that's particularly important to me here is that the advice
    presented here does not gatekeep or otherwise denigrate the personal
    preferences or style of reviewers. With that in mind, one of the things
    I'm looking for in reviews of this document is making sure that the tone
    & content reflect that more positive/encouraging intent. And, of course,
    I'm happy to hear what other tips & terminology people think would be
    helpful to include!
    
    
    Changes since V1
    ================
    
     * Reorganized "Principles" section advice into "High-level guidance"
       and "Performing your review" subsections.
     * Dropped recommendation to comment on cover letter with "LGTM" if you
       have no other recommendations (somewhat redundant with the
       "Completing a review" section, and such comments don't tend to add
       value except when coming from highly-experienced reviewers anyway).
     * Added clarity & modified reasoning for why reviewing CC'd patches is
       helpful.
     * Miscellaneous other revisions recommended by [3].
    
    Thanks!
    
     * Victoria
    
    [1]
    https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53
    [2] https://git-scm.com/docs/MyFirstContribution [3]
    https://lore.kernel.org/git/xmqqwnacibbm.fsf@gitster.g/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1348

Range-diff vs v1:

 1:  b2ed5641c24 ! 1:  7326058b23a Documentation: add ReviewingGuidelines
     @@ Commit message
      
          Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
          Helped-by: Derrick Stolee <derrickstolee@github.com>
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
     +    Helped-by: Josh Steadmon <steadmon@google.com>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## Documentation/Makefile ##
     @@ Documentation/ReviewingGuidelines.txt (new)
      +latest "What's cooking in git.git" email
      +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's
      +cooking" emails & replies can be found using the query `s:"What's cooking"` on
     -+the mailing list archive; alternatively, you can find the contents of the
     -+"What's cooking" email tracked in `whats-cooking.txt` on the `todo` branch of
     -+Git. Topics tagged with "Needs review" and those in the "[New Topics]" section
     -+are typically those that would benefit the most from additional review.
     ++the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive];
     ++alternatively, you can find the contents of the "What's cooking" email tracked
     ++in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with "Needs
     ++review" and those in the "[New Topics]" section are typically those that would
     ++benefit the most from additional review.
      +
      +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.
      +
      +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.
     ++contributor's patch series. These are topics where the author feels that your
     ++attention is warranted. This may be because their patch changes something you
     ++wrote previously (making you a good judge of whether the new approach does or
     ++doesn't work), or because you have the expertise to provide an exceptionally
     ++helpful review. There is no requirement to review these patches but, in the
     ++spirit of open source collaboration, you should strongly consider doing so.
      +
      +Reviewing patches
      +~~~~~~~~~~~~~~~~~
      +While every contributor takes their own approach to reviewing patches, here are
     -+some general pieces of advice to make your reviews to be as clear and helpful as
     -+possible.
     -+
     -+- Provide your review comments per-patch in a plaintext "Reply-All" email to the
     -+  relevant patch. Comments should be made inline, immediately below the relevant
     -+  section(s).
     ++some general pieces of advice to make your reviews as clear and helpful as
     ++possible. The advice is broken into two rough categories: high-level reviewing
     ++guidance, and concrete tips for interacting with patches on the mailing list.
      +
     ++==== High-level guidance
      +- Remember to review the content of commit messages for correctness and clarity,
      +  in addition to the code change in the patch's diff. The commit message of a
      +  patch should accurately and fully explain the code change being made in the
      +  diff.
      +
     -+- You may find that the limited context provided in the patch diff is sometimes
     -+  insufficient for a thorough review. In such cases, you can review patches in
     -+  your local tree by either applying patches with linkgit:git-am[1] or checking
     -+  out the associated branch from https://github.com/gitster/git once the series
     -+  is tracked there.
     -+
     -+- Large, complicated patch diffs are sometimes unavoidable, such as when they
     -+  refactor existing code. If you find such a patch difficult to parse, try
     -+  reviewing the diff produced with the `--color-moved` and/or
     -+  `--ignore-space-change` options.
     -+
      +- Reviewing test coverage is an important - but easy to overlook - component of
      +  reviews. A patch's changes may be covered by existing tests, or new tests may
      +  be introduced to exercise new behavior. Checking out a patch or series locally
     @@ Documentation/ReviewingGuidelines.txt (new)
      +  pass/fail behavior. You can use this information to verify proper coverage or
      +  to suggest additional tests the author could add.
      +
     -+- 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!
     ++- 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.
      +
     -+- When pointing out an issue, try to include suggestions for how the author
     ++- When commenting on 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.
      +
     @@ Documentation/ReviewingGuidelines.txt (new)
      +  goes a long way towards encouraging contributors to participate more actively
      +  in the Git community.
      +
     -+- 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.
     ++==== Performing your review
     ++- Provide your review comments per-patch in a plaintext "Reply-All" email to the
     ++  relevant patch. Comments should be made inline, immediately below the relevant
     ++  section(s).
     ++
     ++- You may find that the limited context provided in the patch diff is sometimes
     ++  insufficient for a thorough review. In such cases, you can review patches in
     ++  your local tree by either applying patches with linkgit:git-am[1] or checking
     ++  out the associated branch from https://github.com/gitster/git once the series
     ++  is tracked there.
     ++
     ++- Large, complicated patch diffs are sometimes unavoidable, such as when they
     ++  refactor existing code. If you find such a patch difficult to parse, try
     ++  reviewing the diff produced with the `--color-moved` and/or
     ++  `--ignore-space-change` options.
     ++
     ++- If a patch is long, you are encouraged to 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!
      +
      +- If you cannot complete a full review of a series all at once, consider letting
      +  the author know (on- or off-list) if/when you plan to review the rest of the
      +  series.
      +
     -+- 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.
     -+
      +Completing a review
      +~~~~~~~~~~~~~~~~~~~
      +Once each patch of a series is reviewed, the author (and/or other contributors)
     @@ Documentation/ReviewingGuidelines.txt (new)
      +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.
     ++add a "Reviewed-by: <you>" trailer if they resubmit the reviewed patch verbatim
     ++in a later iteration of the series.
      +
      +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
     ++"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.
     ++review, including the links to the relevant thread(s).
      +
      +Terminology
      +-----------


 Documentation/Makefile                |   1 +
 Documentation/ReviewingGuidelines.txt | 162 ++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/ReviewingGuidelines.txt

diff --git a/Documentation/Makefile b/Documentation/Makefile
index bd6b6fcb930..d3a19df8bed 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases
 API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))
 SP_ARTICLES += $(API_DOCS)
 
+TECH_DOCS += ReviewingGuidelines
 TECH_DOCS += MyFirstContribution
 TECH_DOCS += MyFirstObjectWalk
 TECH_DOCS += SubmittingPatches
diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt
new file mode 100644
index 00000000000..0e323d54779
--- /dev/null
+++ b/Documentation/ReviewingGuidelines.txt
@@ -0,0 +1,162 @@
+Reviewing Patches in the Git Project
+====================================
+
+Introduction
+------------
+The Git development community is a widely distributed, diverse, ever-changing
+group of individuals. Asynchronous communication via the Git mailing list poses
+unique challenges when reviewing or discussing patches. This document contains
+some guiding principles and helpful tools you can use to make your reviews both
+more efficient for yourself and more effective for other contributors.
+
+Note that none of the recommendations here are binding or in any way a
+requirement of participation in the Git community. They are provided as a
+resource to supplement your skills as a contributor.
+
+Principles
+----------
+
+Selecting patch(es) to review
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If you are looking for a patch series in need of review, start by checking
+latest "What's cooking in git.git" email
+(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's
+cooking" emails & replies can be found using the query `s:"What's cooking"` on
+the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive];
+alternatively, you can find the contents of the "What's cooking" email tracked
+in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with "Needs
+review" and those in the "[New Topics]" section are typically those that would
+benefit the most from additional review.
+
+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.
+
+If you've already contributed to Git, you may also be CC'd in another
+contributor's patch series. These are topics where the author feels that your
+attention is warranted. This may be because their patch changes something you
+wrote previously (making you a good judge of whether the new approach does or
+doesn't work), or because you have the expertise to provide an exceptionally
+helpful review. There is no requirement to review these patches but, in the
+spirit of open source collaboration, you should strongly consider doing so.
+
+Reviewing patches
+~~~~~~~~~~~~~~~~~
+While every contributor takes their own approach to reviewing patches, here are
+some general pieces of advice to make your reviews as clear and helpful as
+possible. The advice is broken into two rough categories: high-level reviewing
+guidance, and concrete tips for interacting with patches on the mailing list.
+
+==== High-level guidance
+- Remember to review the content of commit messages for correctness and clarity,
+  in addition to the code change in the patch's diff. The commit message of a
+  patch should accurately and fully explain the code change being made in the
+  diff.
+
+- Reviewing test coverage is an important - but easy to overlook - component of
+  reviews. A patch's changes may be covered by existing tests, or new tests may
+  be introduced to exercise new behavior. Checking out a patch or series locally
+  allows you to manually mutate lines of new & existing tests to verify expected
+  pass/fail behavior. You can use this information to verify proper coverage or
+  to suggest additional tests the author could add.
+
+- 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.
+
+- When commenting on 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.
+
+- 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.
+
+==== Performing your review
+- Provide your review comments per-patch in a plaintext "Reply-All" email to the
+  relevant patch. Comments should be made inline, immediately below the relevant
+  section(s).
+
+- You may find that the limited context provided in the patch diff is sometimes
+  insufficient for a thorough review. In such cases, you can review patches in
+  your local tree by either applying patches with linkgit:git-am[1] or checking
+  out the associated branch from https://github.com/gitster/git once the series
+  is tracked there.
+
+- Large, complicated patch diffs are sometimes unavoidable, such as when they
+  refactor existing code. If you find such a patch difficult to parse, try
+  reviewing the diff produced with the `--color-moved` and/or
+  `--ignore-space-change` options.
+
+- If a patch is long, you are encouraged to 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!
+
+- If you cannot complete a full review of a series all at once, consider letting
+  the author know (on- or off-list) if/when you plan to review the rest of the
+  series.
+
+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 if they resubmit the reviewed patch verbatim
+in a later iteration of the series.
+
+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, including the links to the relevant thread(s).
+
+Terminology
+-----------
+nit: ::
+	Denotes a small issue that should be fixed, such as a typographical error
+	or mis-alignment of conditions in an `if()` statement.
+
+aside: ::
+optional: ::
+non-blocking: ::
+	Indicates to the reader that the following comment should not block the
+	acceptance of the patch or series. These are typically recommendations
+	related to code organization & style, or musings about topics related to
+	the patch in question, but beyond its scope.
+
+s/<before>/<after>/::
+	Shorthand for "you wrote <before>, but I think you meant <after>," usually
+	for misspellings or other typographical errors. The syntax is a reference
+	to "substitute" command commonly found in Unix tools such as `ed`, `sed`,
+	`vim`, and `perl`.
+
+cover letter::
+	The "Patch 0" of a multi-patch series. This email describes the
+	high-level intent and structure of the patch series to readers on the
+	Git mailing list. It is also where the changelog notes and range-diff of
+	subsequent versions are provided by the author.
++
+On single-patch submissions, cover letter content is typically not sent as a
+separate email. Instead, it is inserted between the end of the patch's commit
+message (after the `---`) and the beginning of the diff.
+
+#leftoverbits::
+  Used by either an author or a reviewer to describe features or suggested
+  changes that are out-of-scope of a given patch or series, but are relevant
+  to the topic for the sake of discussion.
+
+See Also
+--------
+link:MyFirstContribution.html[MyFirstContribution]

base-commit: 79f2338b3746d23454308648b2491e5beba4beff
-- 
gitgitgadget

Re: [PATCH v2] Documentation: add ReviewingGuidelines

[Josh Steadmon]

On 2022.09.19 19:12, Victoria Dye via GitGitGadget wrote:
> 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>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Josh Steadmon <steadmon@google.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---

Looks great, thanks!

Reviewed-by: Josh Steadmon <steadmon@google.com>

Re: [PATCH v2] Documentation: add ReviewingGuidelines

[Junio C Hamano]

"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>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Josh Steadmon <steadmon@google.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>     Documentation: add ReviewingGuidelines
>     
>     This patch follows up on a discussion a few weeks ago in the Git IRC
>     standup [1], where it was mentioned that it would be nice to have
>     consistent definitions for common review terminology (like 'nit:'). The
>     "ReviewingGuidelines" document created here builds on that idea, as well
>     as past discussions around the idea of advice for reviewers (similar to
>     the guidelines for new contributors in MyFirstContribution [2]).

Thanks.  Will queue.

I think this is ready for 'next' and then to 'master' during this
cycle.  Thanks for writing it, and thanks all for reviewing it.

Re: [PATCH v2] Documentation: add ReviewingGuidelines

[Elijah Newren]

On Mon, Sep 19, 2022 at 12:21 PM Victoria Dye via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
[...]
> +==== Performing your review
> +- Provide your review comments per-patch in a plaintext "Reply-All" email to the
> +  relevant patch. Comments should be made inline, immediately below the relevant
> +  section(s).
> +
> +- You may find that the limited context provided in the patch diff is sometimes
> +  insufficient for a thorough review. In such cases, you can review patches in
> +  your local tree by either applying patches with linkgit:git-am[1] or checking
> +  out the associated branch from https://github.com/gitster/git once the series
> +  is tracked there.

Lots of reviews also come with "Fetch-It-Via" instructions in the
cover letter, making it really easy to grab.  Might be worth
mentioning?

Also, would it make sense for us to replace "applying" with
"downloading and applying", perhaps mentioning `b4 am` for the
downloading half?

(I tend to use the Fetch-It-Via or wait for it to show up in
gitster/git, but b4 is really nice for the other cases.)

> +- Large, complicated patch diffs are sometimes unavoidable, such as when they
> +  refactor existing code. If you find such a patch difficult to parse, try
> +  reviewing the diff produced with the `--color-moved` and/or
> +  `--ignore-space-change` options.

Similarly, Documentation refactorings or significant rewordings are
sometimes easier to view with --color-words or --color-words=.

[...]
> +See Also
> +--------
> +link:MyFirstContribution.html[MyFirstContribution]
>
> base-commit: 79f2338b3746d23454308648b2491e5beba4beff

I like this document!  I had a couple ideas that might or might not
make sense to include in the document; it looks good to me either way.

Re: [PATCH v2] Documentation: add ReviewingGuidelines

[Konstantin Ryabitsev]

On Mon, Sep 19, 2022 at 05:43:15PM -0700, Elijah Newren wrote:
> > +- You may find that the limited context provided in the patch diff is sometimes
> > +  insufficient for a thorough review. In such cases, you can review patches in
> > +  your local tree by either applying patches with linkgit:git-am[1] or checking
> > +  out the associated branch from https://github.com/gitster/git once the series
> > +  is tracked there.
> 
> Lots of reviews also come with "Fetch-It-Via" instructions in the
> cover letter, making it really easy to grab.  Might be worth
> mentioning?
> 
> Also, would it make sense for us to replace "applying" with
> "downloading and applying", perhaps mentioning `b4 am` for the
> downloading half?

B4 can also "convert" a patch series into a pull request using "shazam".
E.g.:

    b4 shazam -H <msgid>

This will do some behind-the-scenes magic and give you a FETCH_HEAD that you
can review, check out into a new branch, merge, etc.

You can try this with this very thread, if you are inside the git's own repo:

	$ b4 shazam -H pull.1348.git.1662747205235.gitgitgadget@gmail.com
	Grabbing thread from lore.kernel.org/all/pull.1348.git.1662747205235.gitgitgadget%40gmail.com/t.mbox.gz
	Checking for newer revisions on https://lore.kernel.org/all/
	Analyzing 12 messages in the thread
	Will use the latest revision: v2
	You can pick other revisions using the -vN flag
	Checking attestation on all messages, may take a moment...
	---
	  ✓ [PATCH v2] Documentation: add ReviewingGuidelines
		+ Reviewed-by: Josh Steadmon <steadmon@google.com> (✓ DKIM/google.com)
	  ---
	  ✓ Signed: DKIM/gmail.com
	---
	Total patches: 1
	---
	Magic: Preparing a sparse worktree
	---
	Applying: Documentation: add ReviewingGuidelines
	---
	Fetching into FETCH_HEAD
	You can now merge or checkout FETCH_HEAD
	  e.g.: git merge --no-ff -F /home/user/work/git/git/.git/b4-cover --edit FETCH_HEAD --signoff

> (I tend to use the Fetch-It-Via or wait for it to show up in
> gitster/git, but b4 is really nice for the other cases.)

Great to hear! :)

-Konstantin

Re: [PATCH v2] Documentation: add ReviewingGuidelines

[Shaoxuan Yuan]

On 9/19/2022 5:43 PM, Elijah Newren wrote:
> On Mon, Sep 19, 2022 at 12:21 PM Victoria Dye via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
> [...]
>> +==== Performing your review
>> +- Provide your review comments per-patch in a plaintext "Reply-All" email to the
>> +  relevant patch. Comments should be made inline, immediately below the relevant
>> +  section(s).
>> +
>> +- You may find that the limited context provided in the patch diff is sometimes
>> +  insufficient for a thorough review. In such cases, you can review patches in
>> +  your local tree by either applying patches with linkgit:git-am[1] or checking
>> +  out the associated branch from https://github.com/gitster/git once the series
>> +  is tracked there.
> 
> Lots of reviews also come with "Fetch-It-Via" instructions in the
> cover letter, making it really easy to grab.  Might be worth
> mentioning?
> 
> Also, would it make sense for us to replace "applying" with
> "downloading and applying", perhaps mentioning `b4 am` for the
> downloading half?
> 
> (I tend to use the Fetch-It-Via or wait for it to show up in
> gitster/git, but b4 is really nice for the other cases.)

Thanks, I did not know about b4, it looks quite helpful!

I think it is worth mentioning some recommended practices to operate
'git-am'. 'git-am' was a bit confusing the first time I tried to grab
people's patches from the mailing list without using "Fetch-It-Via",
e.g. what is mailbox and how to convert emails into mailbox.

b4 sounds like a good start to add these practices, and probably some
other recommendations (I don't know much here)?

Please note that these are just some thoughts, the document itself looks
good without adding these practices (maybe we can add them later) :-)

[...]

Thanks,
Shaoxuan

Re: [PATCH v2] Documentation: add ReviewingGuidelines

[Phillip Wood]

Hi Victoria

Thanks for working on this, sorry it has taken me a while to get round 
to looking at it. I think it makes a really useful addition to our 
documentation. I've left a few comments below, the only one I feel 
strongly about is adding something to say what the purpose of the review 
should be and emphasizing positive comments earlier in the document.

On 19/09/2022 20:12, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>

> diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt
> new file mode 100644
> index 00000000000..0e323d54779
> --- /dev/null
> +++ b/Documentation/ReviewingGuidelines.txt
> @@ -0,0 +1,162 @@
> +Reviewing Patches in the Git Project
> +====================================
> +
> +Introduction
> +------------
> +The Git development community is a widely distributed, diverse, ever-changing
> +group of individuals. Asynchronous communication via the Git mailing list poses
> +unique challenges when reviewing or discussing patches. This document contains
> +some guiding principles and helpful tools you can use to make your reviews both
> +more efficient for yourself and more effective for other contributors.
> +
> +Note that none of the recommendations here are binding or in any way a
> +requirement of participation in the Git community. They are provided as a
> +resource to supplement your skills as a contributor.
> +
> +Principles
> +----------
> +
> +Selecting patch(es) to review
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +If you are looking for a patch series in need of review, start by checking
> +latest "What's cooking in git.git" email
> +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's
> +cooking" emails & replies can be found using the query `s:"What's cooking"` on
> +the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive];
> +alternatively, you can find the contents of the "What's cooking" email tracked
> +in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with "Needs
> +review" and those in the "[New Topics]" section are typically those that would
> +benefit the most from additional review.
> +
> +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.
> +
> +If you've already contributed to Git, you may also be CC'd in another
> +contributor's patch series. These are topics where the author feels that your
> +attention is warranted. This may be because their patch changes something you
> +wrote previously (making you a good judge of whether the new approach does or
> +doesn't work), or because you have the expertise to provide an exceptionally

[optional] Maybe this says something about me, but the use of 
"exceptionally" here feels like it is setting quite high expectations 
for the review. Perhaps we could say "particularly" instead

> +helpful review. There is no requirement to review these patches but, in the
> +spirit of open source collaboration, you should strongly consider doing so.
> +
> +Reviewing patches
> +~~~~~~~~~~~~~~~~~
> +While every contributor takes their own approach to reviewing patches, here are
> +some general pieces of advice to make your reviews as clear and helpful as
> +possible. The advice is broken into two rough categories: high-level reviewing
> +guidance, and concrete tips for interacting with patches on the mailing list.
> +
> +==== High-level guidance

I think it would be worth adding something here (or maybe extending the 
previous paragraph) to state the purpose of the review and emphasize the 
importance of positive comments. Maybe something like this (which is a 
reworked version of your final list item)

The purpose of the review is to provide constructive feedback explaining 
how you think the patch could be improved or in some cases why the patch 
is not suitable for inclusion. As well as pointing out any problems it 
is helpful to provide positive feedback on aspects that you particularly 
liked. You should also feel free to "think outloud" in your review 
describing how you read & understood a complex section of a patch. It is 
also useful to ask questions about anything that confused you

> +- Remember to review the content of commit messages for correctness and clarity,
> +  in addition to the code change in the patch's diff. The commit message of a
> +  patch should accurately and fully explain the code change being made in the
> +  diff.
> +
> +- Reviewing test coverage is an important - but easy to overlook - component of
> +  reviews. A patch's changes may be covered by existing tests, or new tests may
> +  be introduced to exercise new behavior. Checking out a patch or series locally
> +  allows you to manually mutate lines of new & existing tests to verify expected
> +  pass/fail behavior. You can use this information to verify proper coverage or
> +  to suggest additional tests the author could add.
> +
> +- 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.
> +
> +- When commenting on 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.
> +
> +- 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.

As I said above I'd like to see this come higher up the list

> +==== Performing your review
> +- Provide your review comments per-patch in a plaintext "Reply-All" email to the
> +  relevant patch. Comments should be made inline, immediately below the relevant
> +  section(s).
> +
> +- You may find that the limited context provided in the patch diff is sometimes
> +  insufficient for a thorough review. In such cases, you can review patches in
> +  your local tree by either applying patches with linkgit:git-am[1] or checking
> +  out the associated branch from https://github.com/gitster/git once the series
> +  is tracked there.
> +
> +- Large, complicated patch diffs are sometimes unavoidable, such as when they
> +  refactor existing code. If you find such a patch difficult to parse, try
> +  reviewing the diff produced with the `--color-moved` and/or
> +  `--ignore-space-change` options.

[optional] --ignore-space-change is quite a blunt instrument, perhaps we 
could suggest using --color-moved-ws=allow-indentation-change which I 
find particularly useful for refactorings. I'd also second Elijah's 
suggestion to mention --color-words for documentation changes.

> +- If a patch is long, you are encouraged to 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!
> +
> +- If you cannot complete a full review of a series all at once, consider letting
> +  the author know (on- or off-list) if/when you plan to review the rest of the
> +  series.
> +
> +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. 

[optional] Maybe add

When re-reviewing the series it is helpful to inspect the range diff to 
see what the author has changed since your last review.

> 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 if they resubmit the reviewed patch verbatim
> +in a later iteration of the series.
> +
> +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, including the links to the relevant thread(s).
> +
> +Terminology
> +-----------
> +nit: ::
> +	Denotes a small issue that should be fixed, such as a typographical error
> +	or mis-alignment of conditions in an `if()` statement.
> +
> +aside: ::
> +optional: ::
> +non-blocking: ::
> +	Indicates to the reader that the following comment should not block the
> +	acceptance of the patch or series. These are typically recommendations
> +	related to code organization & style, or musings about topics related to
> +	the patch in question, but beyond its scope.

[optional] The reference to code style made me wonder if we should add 
something recommending that reviewers refer patch authors to our coding 
guidelines where appropriate.

Thanks again for working on this

Phillip

> +
> +s/<before>/<after>/::
> +	Shorthand for "you wrote <before>, but I think you meant <after>," usually
> +	for misspellings or other typographical errors. The syntax is a reference
> +	to "substitute" command commonly found in Unix tools such as `ed`, `sed`,
> +	`vim`, and `perl`.
> +
> +cover letter::
> +	The "Patch 0" of a multi-patch series. This email describes the
> +	high-level intent and structure of the patch series to readers on the
> +	Git mailing list. It is also where the changelog notes and range-diff of
> +	subsequent versions are provided by the author.
> ++
> +On single-patch submissions, cover letter content is typically not sent as a
> +separate email. Instead, it is inserted between the end of the patch's commit
> +message (after the `---`) and the beginning of the diff.
> +
> +#leftoverbits::
> +  Used by either an author or a reviewer to describe features or suggested
> +  changes that are out-of-scope of a given patch or series, but are relevant
> +  to the topic for the sake of discussion.
> +
> +See Also
> +--------
> +link:MyFirstContribution.html[MyFirstContribution]
> 
> base-commit: 79f2338b3746d23454308648b2491e5beba4beff