Documentation: add ReviewingGuidelines
PATCH 1/1 Documentation: add ReviewingGuidelines
From: Victoria Dye <[email protected]>
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.
---
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
Documentation/Makefile +1 -0
@@ -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
Documentation/ReviewingGuidelines.txt new file +160 -0
160 lines not on this page · raw message
base-commit: 79f2338b3746d23454308648b2491e5beba4beff
Signature · 1 line
gitgitgadget
Quoted 9 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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 <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Victoria Dye <[email protected]>
Thanks, all, for starting this.
Documentation/ReviewingGuidelines.txt line 32
Quoted 3 lines
Documentation/ReviewingGuidelines.txt new file +3
-0+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.
Documentation/ReviewingGuidelines.txt line 39
Quoted 6 lines
Documentation/ReviewingGuidelines.txt new file +6
-0+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.
Documentation/ReviewingGuidelines.txt line 76
Quoted 3 lines
+- If a patch is long, you can delete parts of it that are unrelated to your
Documentation/ReviewingGuidelines.txt new file +2
-0+ review from the email reply. Make sure to leave enough context for readers to + understand your comments!
"you can" -> "it is encouraged to"
Documentation/ReviewingGuidelines.txt line 80
Quoted 3 lines
+- When pointing out an issue, try to include suggestions for how the author
Documentation/ReviewingGuidelines.txt new file +2
-0+ 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.
Documentation/ReviewingGuidelines.txt line 87
Quoted 6 lines
+- Reviews do not need to exclusively point out problems. Feel free to "think out
Documentation/ReviewingGuidelines.txt new file +5
-0+ 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.
Documentation/ReviewingGuidelines.txt line 95
Quoted 7 lines
+- When providing a recommendation, be as clear as possible about whether you
Documentation/ReviewingGuidelines.txt new file +6
-0+ 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.
Documentation/ReviewingGuidelines.txt line 103
Quoted 3 lines
+- If you read and review a series but find nothing that warrants inline
Documentation/ReviewingGuidelines.txt new file +2
-0+ 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.
Documentation/ReviewingGuidelines.txt line 115
Quoted 11 lines
Documentation/ReviewingGuidelines.txt new file +11
-0+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".
Documentation/ReviewingGuidelines.txt line 121
Quoted 5 lines
Documentation/ReviewingGuidelines.txt new file +5
-0+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.
Quoted 10 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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 <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
I've commented on the text but haven't seen anybody else reviewing. No interest? Everybody silently happy?
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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.
Quoted 7 lines
Quoted 4 lines
Helped-by Johannes Schindelin <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
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.
Quoted 14 lines · Victoria Dye
Quoted 13 lines · Junio C Hamano
Quoted 10 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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 <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
Quoted 5 lines
Quoted 2 lines
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
Hi,
Quoted 24 lines · Derrick Stolee
Quoted 14 lines · Victoria Dye
Quoted 13 lines · Junio C Hamano
Quoted 10 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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 <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
Quoted 5 lines
Quoted 2 lines
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.
Quoted 8 lines
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
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!
Quoted 231 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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 <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
Documentation: add ReviewingGuidelinesThis 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/MyFirstContributionPublished-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/1348Documentation/Makefile | 1 + Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 Documentation/ReviewingGuidelines.txtDocumentation/Makefile +1
-0@@ -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 += SubmittingPatchesDocumentation/ReviewingGuidelines.txt new file +160
-0@@ -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/[email protected]/[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:
79f2338b3746d23454308648b2491e5beba4beffSignature · 1 line
gitgitgadget
Quoted 1 line · Josh Steadmon
I like all the advice here, thanks for the patch!
I agree, I think this doc is a great step forward. Thanks!
Quoted 3 lines
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.
Quoted 233 lines · Victoria Dye via GitGitGadget
Quoted 231 lines
From: Victoria Dye <[email protected]>
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 <[email protected]>Helped-by Derrick Stolee <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
Documentation: add ReviewingGuidelinesThis 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/MyFirstContributionPublished-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/1348Documentation/Makefile | 1 + Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 Documentation/ReviewingGuidelines.txtDocumentation/Makefile +1
-0@@ -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 += SubmittingPatchesDocumentation/ReviewingGuidelines.txt new file +160
-0@@ -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/[email protected]/[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:
79f2338b3746d23454308648b2491e5beba4beffSignature · 1 line
gitgitgadget
PATCH v2 1/1 Documentation: add ReviewingGuidelines
In reply to Victoria Dye via GitGitGadgetFrom: Victoria Dye <[email protected]>
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.
---
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/[email protected]/
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: Junio C Hamano <[email protected]>
+ Helped-by: Josh Steadmon <[email protected]>
## Documentation/Makefile ##
@@ Documentation/ReviewingGuidelines.txt (new)
+latest "What's cooking in git.git" email +(https://lore.kernel.org/git/[email protected]/[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
Documentation/Makefile +1 -0
@@ -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
Documentation/ReviewingGuidelines.txt new file +162 -0
162 lines not on this page · raw message
base-commit: 79f2338b3746d23454308648b2491e5beba4beff
Signature · 1 line
gitgitgadget
Quoted 6 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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.
Quoted 6 lines
Helped-by Johannes Schindelin <[email protected]>Helped-by Derrick Stolee <[email protected]>Helped-by Junio C Hamano <[email protected]>Helped-by Josh Steadmon <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
Looks great, thanks!
Quoted 14 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
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 <[email protected]>Helped-by Derrick Stolee <[email protected]>Helped-by Junio C Hamano <[email protected]>Helped-by Josh Steadmon <[email protected]>Signed-off-by Victoria Dye <[email protected]>---
Documentation: add ReviewingGuidelines
Quoted 6 lines
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.
Quoted 1 line · Victoria Dye via GitGitGadget
[...]
Documentation/ReviewingGuidelines.txt line 91
Quoted 10 lines
+==== Performing your review +- Provide your review comments per-patch in a plaintext "Reply-All" email to theDocumentation/ReviewingGuidelines.txt new file +8
-0+ 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.)
Documentation/ReviewingGuidelines.txt line 96
Quoted 4 lines
+- Large, complicated patch diffs are sometimes unavoidable, such as when they
Documentation/ReviewingGuidelines.txt new file +3
-0+ 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=.
[...]
Quoted 5 lines
Documentation/ReviewingGuidelines.txt new file +3
-0+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.
Quoted 6 lines · Elijah Newren
Documentation/ReviewingGuidelines.txt line 91
Quoted 5 lines
+- You may find that the limited context provided in the patch diff is sometimes
Documentation/ReviewingGuidelines.txt new file +4
-0+ 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.
Quoted 7 lines
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 amfor 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 [email protected] 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 <[email protected]> (✓ 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
Quoted 2 lines
(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
Quoted 19 lines · Elijah Newren
Quoted 1 line · Victoria Dye via GitGitGadget
[...]
Documentation/ReviewingGuidelines.txt line 91
Quoted 10 lines
+==== Performing your review +- Provide your review comments per-patch in a plaintext "Reply-All" email to theDocumentation/ReviewingGuidelines.txt new file +8
-0+ 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?
Quoted 6 lines
Also, would it make sense for us to replace "applying" with "downloading and applying", perhaps mentioning
b4 amfor 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
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.
Quoted 37 lines · Victoria Dye via GitGitGadget
From: Victoria Dye <[email protected]>
Documentation/ReviewingGuidelines.txt new file +29
-0@@ -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/[email protected]/[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.
Documentation/ReviewingGuidelines.txt line 39
Quoted 10 lines
Documentation/ReviewingGuidelines.txt new file +10
-0+ +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
Documentation/ReviewingGuidelines.txt line 50
Quoted 11 lines
Documentation/ReviewingGuidelines.txt new file +11
-0+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
Quoted 20 lines
Documentation/ReviewingGuidelines.txt new file +20
-0+- 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. +
Documentation/ReviewingGuidelines.txt line 80
Quoted 10 lines
Documentation/ReviewingGuidelines.txt new file +10
-0+- 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
Quoted 5 lines
Documentation/ReviewingGuidelines.txt new file +5
-0+==== 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). +
Documentation/ReviewingGuidelines.txt line 96
Quoted 10 lines
Documentation/ReviewingGuidelines.txt new file +10
-0+- 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.
Quoted 6 lines
Documentation/ReviewingGuidelines.txt new file +6
-0+- 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
Documentation/ReviewingGuidelines.txt
Quoted 10 lines
Documentation/ReviewingGuidelines.txt new file +10
-0+ 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.
Quoted 15 lines
If you are happy with the state of the patch series,
Documentation/ReviewingGuidelines.txt new file +14
-0+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: ::
Documentation/ReviewingGuidelines.txt line 137
Quoted 10 lines
Documentation/ReviewingGuidelines.txt new file +10
-0+ 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
Quoted 27 lines
Documentation/ReviewingGuidelines.txt new file +25
-0+ +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