{"thread":{"id":"58411","subject":"[PATCH] Documentation: add ReviewingGuidelines","startedAt":"2022-09-09T18:13:41Z","lastAt":"2022-09-22T13:29:32Z","messageCount":15,"participants":["Victoria Dye via GitGitGadget","Junio C Hamano","Victoria Dye","Josh Steadmon","Derrick Stolee","Johannes Schindelin","Glen Choo","Elijah Newren","Konstantin Ryabitsev","Shaoxuan Yuan","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"462880","messageId":"pull.1348.git.1662747205235.gitgitgadget@gmail.com","threadId":"58411","inReplyTo":null,"subject":"[PATCH] Documentation: add ReviewingGuidelines","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-09T18:13:25Z","receivedAt":"2022-09-09T18:13:41Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a reviewing guidelines document including advice and common terminology\nused in Git mailing list reviews. The document is included in the\n'TECH_DOCS' list in order to include it in Git's published documentation.\n\nHelped-by: Johannes Schindelin \nHelped-by: Derrick Stolee \nSigned-off-by: Victoria Dye \n---\n Documentation: add ReviewingGuidelines\n \n This patch follows up on a discussion a few weeks ago in the Git IRC\n standup [1], where it was mentioned that it would be nice to have\n consistent definitions for common review terminology (like 'nit:'). The\n \"ReviewingGuidelines\" document created here builds on that idea, as well\n as past discussions around the idea of advice for reviewers (similar to\n the guidelines for new contributors in MyFirstContribution [2]).\n \n The goal of this document is to clarify & standardize some of the more\n niche concepts important to the Git project (\"What's cooking\" emails,\n terminology), as well as provide general reviewing advice based on my\n observations of effective reviews from others on the mailing list.\n \n One thing that's particularly important to me here is that the advice\n presented here does not gatekeep or otherwise denigrate the personal\n preferences or style of reviewers. With that in mind, one of the things\n I'm looking for in reviews of this document is making sure that the tone\n & content reflect that more positive/encouraging intent. And, of course,\n I'm happy to hear what other tips & terminology people think would be\n helpful to include!\n \n Thanks!\n \n * Victoria\n \n [1]\n https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53\n [2] https://git-scm.com/docs/MyFirstContribution\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1348\n\n Documentation/Makefile | 1 +\n Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++\n 2 files changed, 161 insertions(+)\n create mode 100644 Documentation/ReviewingGuidelines.txt\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex bd6b6fcb930..d3a19df8bed 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases\n API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))\n SP_ARTICLES += $(API_DOCS)\n \n+TECH_DOCS += ReviewingGuidelines\n TECH_DOCS += MyFirstContribution\n TECH_DOCS += MyFirstObjectWalk\n TECH_DOCS += SubmittingPatches\ndiff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt\nnew file mode 100644\nindex 00000000000..bcc59baf863\n--- /dev/null\n+++ b/Documentation/ReviewingGuidelines.txt\n@@ -0,0 +1,160 @@\n+Reviewing Patches in the Git Project\n+====================================\n+\n+Introduction\n+------------\n+The Git development community is a widely distributed, diverse, ever-changing\n+group of individuals. Asynchronous communication via the Git mailing list poses\n+unique challenges when reviewing or discussing patches. This document contains\n+some guiding principles and helpful tools you can use to make your reviews both\n+more efficient for yourself and more effective for other contributors.\n+\n+Note that none of the recommendations here are binding or in any way a\n+requirement of participation in the Git community. They are provided as a\n+resource to supplement your skills as a contributor.\n+\n+Principles\n+----------\n+\n+Selecting patch(es) to review\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n+If you are looking for a patch series in need of review, start by checking\n+latest \"What's cooking in git.git\" email\n+(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The \"What's\n+cooking\" emails & replies can be found using the query `s:\"What's cooking\"` on\n+the mailing list archive; alternatively, you can find the contents of the\n+\"What's cooking\" email tracked in `whats-cooking.txt` on the `todo` branch of\n+Git. Topics tagged with \"Needs review\" and those in the \"[New Topics]\" section\n+are typically those that would benefit the most from additional review.\n+\n+Patches can also be searched manually in the mailing list archive using a query\n+like `s:\"PATCH\" -s:\"Re:\"`. You can browse these results for topics relevant to\n+your expertise or interest.\n+\n+If you've already contributed to Git, you may also be CC'd in another\n+contributor's patch series. These are usually topics where the author feels that\n+your attention is warranted; this may be due to prior contributions,\n+demonstrated expertise, and/or interest in related topics. There is no\n+requirement to review these series, but you may find them easier to review as a\n+result of your preexisting background knowledge on the topic.\n+\n+Reviewing patches\n+~~~~~~~~~~~~~~~~~\n+While every contributor takes their own approach to reviewing patches, here are\n+some general pieces of advice to make your reviews to be as clear and helpful as\n+possible.\n+\n+- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n+ relevant patch. Comments should be made inline, immediately below the relevant\n+ section(s).\n+\n+- Remember to review the content of commit messages for correctness and clarity,\n+ in addition to the code change in the patch's diff. The commit message of a\n+ patch should accurately and fully explain the code change being made in the\n+ diff.\n+\n+- You may find that the limited context provided in the patch diff is sometimes\n+ insufficient for a thorough review. In such cases, you can review patches in\n+ your local tree by either applying patches with linkgit:git-am[1] or checking\n+ out the associated branch from https://github.com/gitster/git once the series\n+ is tracked there.\n+\n+- Large, complicated patch diffs are sometimes unavoidable, such as when they\n+ refactor existing code. If you find such a patch difficult to parse, try\n+ reviewing the diff produced with the `--color-moved` and/or\n+ `--ignore-space-change` options.\n+\n+- Reviewing test coverage is an important - but easy to overlook - component of\n+ reviews. A patch's changes may be covered by existing tests, or new tests may\n+ be introduced to exercise new behavior. Checking out a patch or series locally\n+ allows you to manually mutate lines of new & existing tests to verify expected\n+ pass/fail behavior. You can use this information to verify proper coverage or\n+ to suggest additional tests the author could add.\n+\n+- If a patch is long, you can delete parts of it that are unrelated to your\n+ review from the email reply. Make sure to leave enough context for readers to\n+ understand your comments!\n+\n+- When pointing out an issue, try to include suggestions for how the author\n+ could fix it. This not only helps the author to understand and fix the issue,\n+ it also deepens and improves your understanding of the topic.\n+\n+- Reviews do not need to exclusively point out problems. Feel free to \"think out\n+ loud\" in your review: describe how you read & understood a complex section of\n+ a patch, ask a question about something that confused you, point out something\n+ you found exceptionally well-written, etc. In particular, uplifting feedback\n+ goes a long way towards encouraging contributors to participate more actively\n+ in the Git community.\n+\n+- When providing a recommendation, be as clear as possible about whether you\n+ consider it \"blocking\" (the code would be broken or otherwise made worse if an\n+ issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n+ the recommendation, but acceptance of the series does not require it).\n+ Non-blocking recommendations can be particularly ambiguous when they are\n+ related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n+ they represent only stylistic differences between the author and reviewer.\n+\n+- If you cannot complete a full review of a series all at once, consider letting\n+ the author know (on- or off-list) if/when you plan to review the rest of the\n+ series.\n+\n+- If you read and review a series but find nothing that warrants inline\n+ commentary, reply to the series' cover letter to indicate that you've reviewed\n+ the changes.\n+\n+Completing a review\n+~~~~~~~~~~~~~~~~~~~\n+Once each patch of a series is reviewed, the author (and/or other contributors)\n+may discuss the review(s). This may result in no changes being applied, or the\n+author will send a new version of their patch(es).\n+\n+After a series is rerolled in response to your or others' review, make sure to\n+re-review the updates. If you are happy with the state of the patch series,\n+explicitly indicate your approval (typically with a reply to the latest\n+version's cover letter). Optionally, you can let the author know that they can\n+add a \"Reviewed-by: \" trailer to subsequent versions of their series.\n+\n+Finally, subsequent \"What's cooking\" emails may explicitly ask whether a\n+reviewed topic is ready for merging to the `next` branch (typically phrased\n+\"Will merge to 'next'?\"). You can help the maintainer and author by responding\n+with a short description of the state of your (and others', if applicable)\n+review.\n+\n+Terminology\n+-----------\n+nit: ::\n+\tDenotes a small issue that should be fixed, such as a typographical error\n+\tor mis-alignment of conditions in an `if()` statement.\n+\n+aside: ::\n+optional: ::\n+non-blocking: ::\n+\tIndicates to the reader that the following comment should not block the\n+\tacceptance of the patch or series. These are typically recommendations\n+\trelated to code organization & style, or musings about topics related to\n+\tthe patch in question, but beyond its scope.\n+\n+s///::\n+\tShorthand for \"you wrote , but I think you meant ,\" usually\n+\tfor misspellings or other typographical errors. The syntax is a reference\n+\tto \"substitute\" command commonly found in Unix tools such as `ed`, `sed`,\n+\t`vim`, and `perl`.\n+\n+cover letter::\n+\tThe \"Patch 0\" of a multi-patch series. This email describes the\n+\thigh-level intent and structure of the patch series to readers on the\n+\tGit mailing list. It is also where the changelog notes and range-diff of\n+\tsubsequent versions are provided by the author.\n++\n+On single-patch submissions, cover letter content is typically not sent as a\n+separate email. Instead, it is inserted between the end of the patch's commit\n+message (after the `---`) and the beginning of the diff.\n+\n+#leftoverbits::\n+ Used by either an author or a reviewer to describe features or suggested\n+ changes that are out-of-scope of a given patch or series, but are relevant\n+ to the topic for the sake of discussion.\n+\n+See Also\n+--------\n+link:MyFirstContribution.html[MyFirstContribution]\n\nbase-commit: 79f2338b3746d23454308648b2491e5beba4beff\n-- \ngitgitgadget\n"},{"id":"462884","messageId":"xmqqwnacibbm.fsf@gitster.g","threadId":"58411","inReplyTo":"pull.1348.git.1662747205235.gitgitgadget@gmail.com","subject":"Re: [PATCH] Documentation: add ReviewingGuidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-09T19:42:37Z","receivedAt":"2022-09-09T19:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Add a reviewing guidelines document including advice and common terminology\n> used in Git mailing list reviews. The document is included in the\n> 'TECH_DOCS' list in order to include it in Git's published documentation.\n>\n> Helped-by: Johannes Schindelin \n> Helped-by: Derrick Stolee \n> Signed-off-by: Victoria Dye \n\nThanks, all, for starting this.\n\n> +Patches can also be searched manually in the mailing list archive using a query\n> +like `s:\"PATCH\" -s:\"Re:\"`. You can browse these results for topics relevant to\n> +your expertise or interest.\n\nIt probably is a good idea to say \"the 'lore.kernel.org' mailing\nlist archive\" somewhere here, as the queries may not work on other\narchives like marc.info/?l=git archive.\n\n> +If you've already contributed to Git, you may also be CC'd in another\n> +contributor's patch series. These are usually topics where the author feels that\n> +your attention is warranted; this may be due to prior contributions,\n> +demonstrated expertise, and/or interest in related topics. There is no\n> +requirement to review these series, but you may find them easier to review as a\n> +result of your preexisting background knowledge on the topic.\n\nI think \"your attention is warranted\" is a good way to summarize,\nbut the readers may want to know that the reason for Cc'ing ranges\nfrom \"you may want to know that I am about to butcher the code you\nwrote earlier, which you might care about (so I am giving a notice\nso that you can stop me and offer a better alternative)\" to \"your\nreviewing would really help making this patch better\".\n\nIt is true that there is no requirement to review anything, but\nscratching each other's back, especially the reason for Cc'ing is to\nrequest help, is in the spirit of working on a piece of open source\nsoftware. That may be more important than knowing that they may be\neasier to review than others.\n\n\n> +- If a patch is long, you can delete parts of it that are unrelated to your\n> + review from the email reply. Make sure to leave enough context for readers to\n> + understand your comments!\n\n\"you can\" -> \"it is encouraged to\"\n\n> +- When pointing out an issue, try to include suggestions for how the author\n> + could fix it. This not only helps the author to understand and fix the issue,\n> + it also deepens and improves your understanding of the topic.\n\nThanks for saying \"try to\". Sometimes reviewers can tell something\nis wrong without being able to say what the alternative is that is\nright, but at least they should try before saying \"that one is wrong\"\nand stopping at it.\n\n> +- Reviews do not need to exclusively point out problems. Feel free to \"think out\n> + loud\" in your review: describe how you read & understood a complex section of\n> + a patch, ask a question about something that confused you, point out something\n> + you found exceptionally well-written, etc. In particular, uplifting feedback\n> + goes a long way towards encouraging contributors to participate more actively\n> + in the Git community.\n\nGood piece of advice. It also helps if the authors understood the\nabove. A review response to a patch may not necessarily point out\nthe problems and they do not need to become defensive.\n\n> +- When providing a recommendation, be as clear as possible about whether you\n> + consider it \"blocking\" (the code would be broken or otherwise made worse if an\n> + issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n> + the recommendation, but acceptance of the series does not require it).\n> + Non-blocking recommendations can be particularly ambiguous when they are\n> + related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n> + they represent only stylistic differences between the author and reviewer.\n\nI hear that some communities take the \"reviewer wins\" approach to\ntiebreak the last one. In any case, it is a good piece of advice to\nmake sure which parts are \"just thinking out aloud\" and which parts\nare pointing out problems in the patch that need to be corrected in\nthe next iteration. The former may observe an existing problem in\nthe code in context that may need to be addressed in the longer term\nbut can be left out of the scope of the topic, and being clear about\nthat is helpful to the author.\n\n> +- If you read and review a series but find nothing that warrants inline\n> + commentary, reply to the series' cover letter to indicate that you've reviewed\n> + the changes.\n\nI would prefer to see folks avoid doing this on the initial\niteration of a topic, until/unless the reviewer has demonstrated\nproficiency in the affected area. If everybody considers that you\nare one of the authorities of revision traversal and the patch\nseries is about updating revision.c, then such a \"I've reviewed and\neverything is so clean there is nothing to say\" may be helpful, but\nit is hard to put a proper weight on such a statement by somebody\nwhose understanding of the area is not well known. \n\nIt is a different story to give such a \"looks good\" on a second and\nsubsequent iteration by a reviewer who commented on an earlier\nround, of course.\n\n> +Completing a review\n> +~~~~~~~~~~~~~~~~~~~\n> +Once each patch of a series is reviewed, the author (and/or other contributors)\n> +may discuss the review(s). This may result in no changes being applied, or the\n> +author will send a new version of their patch(es).\n> +\n> +After a series is rerolled in response to your or others' review, make sure to\n> +re-review the updates. If you are happy with the state of the patch series,\n> +explicitly indicate your approval (typically with a reply to the latest\n> +version's cover letter). Optionally, you can let the author know that they can\n> +add a \"Reviewed-by: \" trailer to subsequent versions of their series.\n\n\"to subsequent versions\" -> \"if they resubmit the reviewed patch verbatim\".\n\n> +Finally, subsequent \"What's cooking\" emails may explicitly ask whether a\n> +reviewed topic is ready for merging to the `next` branch (typically phrased\n> +\"Will merge to 'next'?\"). You can help the maintainer and author by responding\n> +with a short description of the state of your (and others', if applicable)\n> +review.\n\nThanks for mentioning this. \"We have reached the agreement that,\nwhile this and that have room for improvement, the patch is a strict\nimprovement over the status quo, and should move forward, at \"\nthat points into the lore archive would be the most easy and clear.\n\nEverything else I did not quote from your patch looked good to me\nwithout any need to comment.\n\nThanks.\n"},{"id":"462981","messageId":"xmqqr10f88jm.fsf@gitster.g","threadId":"58411","inReplyTo":"pull.1348.git.1662747205235.gitgitgadget@gmail.com","subject":"Re: [PATCH] Documentation: add ReviewingGuidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-13T17:54:05Z","receivedAt":"2022-09-13T18:35:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Add a reviewing guidelines document including advice and common terminology\n> used in Git mailing list reviews. The document is included in the\n> 'TECH_DOCS' list in order to include it in Git's published documentation.\n>\n> Helped-by: Johannes Schindelin \n> Helped-by: Derrick Stolee \n> Signed-off-by: Victoria Dye \n> ---\n\nI've commented on the text but haven't seen anybody else reviewing.\nNo interest? Everybody silently happy?\n\n"},{"id":"463007","messageId":"5685773e-db83-6b92-ff42-0d51e6e6a22e@github.com","threadId":"58411","inReplyTo":"xmqqr10f88jm.fsf@gitster.g","subject":"Re: [PATCH] Documentation: add ReviewingGuidelines","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-13T23:11:00Z","receivedAt":"2022-09-13T23:11:06Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> From: Victoria Dye \n>>\n>> Add a reviewing guidelines document including advice and common terminology\n>> used in Git mailing list reviews. The document is included in the\n>> 'TECH_DOCS' list in order to include it in Git's published documentation.\n>>\n>> Helped-by: Johannes Schindelin \n>> Helped-by: Derrick Stolee \n>> Signed-off-by: Victoria Dye \n>> ---\n> \n> I've commented on the text but haven't seen anybody else reviewing.\n> No interest? Everybody silently happy?\n\nMy guess is that there aren't as many eyes on this as there might typically\nbe because of Git Merge. In any case, I plan to re-roll based on your\nfeedback [1] (ideally) by the end of the week if other reviews aren't sent\nin the meantime. \n\nI'm hoping there's a bit more interest after Git Merge. With reviewing being\nsuch an integral part of contribution to Git, I'm really interested in\nhearing people's thoughts on what should/shouldn't be in this document.\n\n[1] https://lore.kernel.org/git/xmqqwnacibbm.fsf@gitster.g/\n"},{"id":"463063","messageId":"YyO0U+52vJuTlfo7@google.com","threadId":"58411","inReplyTo":"pull.1348.git.1662747205235.gitgitgadget@gmail.com","subject":"Re: [PATCH] Documentation: add ReviewingGuidelines","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-09-15T23:25:07Z","receivedAt":"2022-09-15T23:25:20Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"I like all the advice here, thanks for the patch! My only quibble is\nthat the items in the \"Reviewing patches\" section might be worth\nre-ordering based on topic / importance. For example, I think\n\"philosophy\" items like \"patches should include test coverage\" is more\nimportant than tips on how to use diff flags, and so it would make sense\nto be listed earlier. But this is subjective, and the doc is short so\nin practice it probably doesn't matter.\n\nThanks again!\n\nReviewed-by: Josh Steadmon \n\n\nOn 2022.09.09 18:13, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Add a reviewing guidelines document including advice and common terminology\n> used in Git mailing list reviews. The document is included in the\n> 'TECH_DOCS' list in order to include it in Git's published documentation.\n> \n> Helped-by: Johannes Schindelin \n> Helped-by: Derrick Stolee \n> Signed-off-by: Victoria Dye \n> ---\n> Documentation: add ReviewingGuidelines\n> \n> This patch follows up on a discussion a few weeks ago in the Git IRC\n> standup [1], where it was mentioned that it would be nice to have\n> consistent definitions for common review terminology (like 'nit:'). The\n> \"ReviewingGuidelines\" document created here builds on that idea, as well\n> as past discussions around the idea of advice for reviewers (similar to\n> the guidelines for new contributors in MyFirstContribution [2]).\n> \n> The goal of this document is to clarify & standardize some of the more\n> niche concepts important to the Git project (\"What's cooking\" emails,\n> terminology), as well as provide general reviewing advice based on my\n> observations of effective reviews from others on the mailing list.\n> \n> One thing that's particularly important to me here is that the advice\n> presented here does not gatekeep or otherwise denigrate the personal\n> preferences or style of reviewers. With that in mind, one of the things\n> I'm looking for in reviews of this document is making sure that the tone\n> & content reflect that more positive/encouraging intent. And, of course,\n> I'm happy to hear what other tips & terminology people think would be\n> helpful to include!\n> \n> Thanks!\n> \n> * Victoria\n> \n> [1]\n> https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53\n> [2] https://git-scm.com/docs/MyFirstContribution\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1348\n> \n> Documentation/Makefile | 1 +\n> Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++\n> 2 files changed, 161 insertions(+)\n> create mode 100644 Documentation/ReviewingGuidelines.txt\n> \n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index bd6b6fcb930..d3a19df8bed 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases\n> API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))\n> SP_ARTICLES += $(API_DOCS)\n> \n> +TECH_DOCS += ReviewingGuidelines\n> TECH_DOCS += MyFirstContribution\n> TECH_DOCS += MyFirstObjectWalk\n> TECH_DOCS += SubmittingPatches\n> diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt\n> new file mode 100644\n> index 00000000000..bcc59baf863\n> --- /dev/null\n> +++ b/Documentation/ReviewingGuidelines.txt\n> @@ -0,0 +1,160 @@\n> +Reviewing Patches in the Git Project\n> +====================================\n> +\n> +Introduction\n> +------------\n> +The Git development community is a widely distributed, diverse, ever-changing\n> +group of individuals. Asynchronous communication via the Git mailing list poses\n> +unique challenges when reviewing or discussing patches. This document contains\n> +some guiding principles and helpful tools you can use to make your reviews both\n> +more efficient for yourself and more effective for other contributors.\n> +\n> +Note that none of the recommendations here are binding or in any way a\n> +requirement of participation in the Git community. They are provided as a\n> +resource to supplement your skills as a contributor.\n> +\n> +Principles\n> +----------\n> +\n> +Selecting patch(es) to review\n> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> +If you are looking for a patch series in need of review, start by checking\n> +latest \"What's cooking in git.git\" email\n> +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The \"What's\n> +cooking\" emails & replies can be found using the query `s:\"What's cooking\"` on\n> +the mailing list archive; alternatively, you can find the contents of the\n> +\"What's cooking\" email tracked in `whats-cooking.txt` on the `todo` branch of\n> +Git. Topics tagged with \"Needs review\" and those in the \"[New Topics]\" section\n> +are typically those that would benefit the most from additional review.\n> +\n> +Patches can also be searched manually in the mailing list archive using a query\n> +like `s:\"PATCH\" -s:\"Re:\"`. You can browse these results for topics relevant to\n> +your expertise or interest.\n> +\n> +If you've already contributed to Git, you may also be CC'd in another\n> +contributor's patch series. These are usually topics where the author feels that\n> +your attention is warranted; this may be due to prior contributions,\n> +demonstrated expertise, and/or interest in related topics. There is no\n> +requirement to review these series, but you may find them easier to review as a\n> +result of your preexisting background knowledge on the topic.\n> +\n> +Reviewing patches\n> +~~~~~~~~~~~~~~~~~\n> +While every contributor takes their own approach to reviewing patches, here are\n> +some general pieces of advice to make your reviews to be as clear and helpful as\n> +possible.\n> +\n> +- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n> + relevant patch. Comments should be made inline, immediately below the relevant\n> + section(s).\n> +\n> +- Remember to review the content of commit messages for correctness and clarity,\n> + in addition to the code change in the patch's diff. The commit message of a\n> + patch should accurately and fully explain the code change being made in the\n> + diff.\n> +\n> +- You may find that the limited context provided in the patch diff is sometimes\n> + insufficient for a thorough review. In such cases, you can review patches in\n> + your local tree by either applying patches with linkgit:git-am[1] or checking\n> + out the associated branch from https://github.com/gitster/git once the series\n> + is tracked there.\n> +\n> +- Large, complicated patch diffs are sometimes unavoidable, such as when they\n> + refactor existing code. If you find such a patch difficult to parse, try\n> + reviewing the diff produced with the `--color-moved` and/or\n> + `--ignore-space-change` options.\n> +\n> +- Reviewing test coverage is an important - but easy to overlook - component of\n> + reviews. A patch's changes may be covered by existing tests, or new tests may\n> + be introduced to exercise new behavior. Checking out a patch or series locally\n> + allows you to manually mutate lines of new & existing tests to verify expected\n> + pass/fail behavior. You can use this information to verify proper coverage or\n> + to suggest additional tests the author could add.\n> +\n> +- If a patch is long, you can delete parts of it that are unrelated to your\n> + review from the email reply. Make sure to leave enough context for readers to\n> + understand your comments!\n> +\n> +- When pointing out an issue, try to include suggestions for how the author\n> + could fix it. This not only helps the author to understand and fix the issue,\n> + it also deepens and improves your understanding of the topic.\n> +\n> +- Reviews do not need to exclusively point out problems. Feel free to \"think out\n> + loud\" in your review: describe how you read & understood a complex section of\n> + a patch, ask a question about something that confused you, point out something\n> + you found exceptionally well-written, etc. In particular, uplifting feedback\n> + goes a long way towards encouraging contributors to participate more actively\n> + in the Git community.\n> +\n> +- When providing a recommendation, be as clear as possible about whether you\n> + consider it \"blocking\" (the code would be broken or otherwise made worse if an\n> + issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n> + the recommendation, but acceptance of the series does not require it).\n> + Non-blocking recommendations can be particularly ambiguous when they are\n> + related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n> + they represent only stylistic differences between the author and reviewer.\n> +\n> +- If you cannot complete a full review of a series all at once, consider letting\n> + the author know (on- or off-list) if/when you plan to review the rest of the\n> + series.\n> +\n> +- If you read and review a series but find nothing that warrants inline\n> + commentary, reply to the series' cover letter to indicate that you've reviewed\n> + the changes.\n> +\n> +Completing a review\n> +~~~~~~~~~~~~~~~~~~~\n> +Once each patch of a series is reviewed, the author (and/or other contributors)\n> +may discuss the review(s). This may result in no changes being applied, or the\n> +author will send a new version of their patch(es).\n> +\n> +After a series is rerolled in response to your or others' review, make sure to\n> +re-review the updates. If you are happy with the state of the patch series,\n> +explicitly indicate your approval (typically with a reply to the latest\n> +version's cover letter). Optionally, you can let the author know that they can\n> +add a \"Reviewed-by: \" trailer to subsequent versions of their series.\n> +\n> +Finally, subsequent \"What's cooking\" emails may explicitly ask whether a\n> +reviewed topic is ready for merging to the `next` branch (typically phrased\n> +\"Will merge to 'next'?\"). You can help the maintainer and author by responding\n> +with a short description of the state of your (and others', if applicable)\n> +review.\n> +\n> +Terminology\n> +-----------\n> +nit: ::\n> +\tDenotes a small issue that should be fixed, such as a typographical error\n> +\tor mis-alignment of conditions in an `if()` statement.\n> +\n> +aside: ::\n> +optional: ::\n> +non-blocking: ::\n> +\tIndicates to the reader that the following comment should not block the\n> +\tacceptance of the patch or series. These are typically recommendations\n> +\trelated to code organization & style, or musings about topics related to\n> +\tthe patch in question, but beyond its scope.\n> +\n> +s///::\n> +\tShorthand for \"you wrote , but I think you meant ,\" usually\n> +\tfor misspellings or other typographical errors. The syntax is a reference\n> +\tto \"substitute\" command commonly found in Unix tools such as `ed`, `sed`,\n> +\t`vim`, and `perl`.\n> +\n> +cover letter::\n> +\tThe \"Patch 0\" of a multi-patch series. This email describes the\n> +\thigh-level intent and structure of the patch series to readers on the\n> +\tGit mailing list. It is also where the changelog notes and range-diff of\n> +\tsubsequent versions are provided by the author.\n> ++\n> +On single-patch submissions, cover letter content is typically not sent as a\n> +separate email. Instead, it is inserted between the end of the patch's commit\n> +message (after the `---`) and the beginning of the diff.\n> +\n> +#leftoverbits::\n> + Used by either an author or a reviewer to describe features or suggested\n> + changes that are out-of-scope of a given patch or series, but are relevant\n> + to the topic for the sake of discussion.\n> +\n> +See Also\n> +--------\n> +link:MyFirstContribution.html[MyFirstContribution]\n> \n> base-commit: 79f2338b3746d23454308648b2491e5beba4beff\n> -- \n> gitgitgadget\n"},{"id":"463181","messageId":"925b1e37-17aa-9168-9246-ac48e043c0d4@github.com","threadId":"58411","inReplyTo":"5685773e-db83-6b92-ff42-0d51e6e6a22e@github.com","subject":"Re: [PATCH] Documentation: add ReviewingGuidelines","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-19T15:08:17Z","receivedAt":"2022-09-19T15:08:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/13/2022 7:11 PM, Victoria Dye wrote:\n> Junio C Hamano wrote:\n>> \"Victoria Dye via GitGitGadget\" writes:\n>>\n>>> From: Victoria Dye \n>>>\n>>> Add a reviewing guidelines document including advice and common terminology\n>>> used in Git mailing list reviews. The document is included in the\n>>> 'TECH_DOCS' list in order to include it in Git's published documentation.\n>>>\n>>> Helped-by: Johannes Schindelin \n>>> Helped-by: Derrick Stolee \n>>> Signed-off-by: Victoria Dye \n>>> ---\n>>\n>> I've commented on the text but haven't seen anybody else reviewing.\n>> No interest? Everybody silently happy?\n> \n> My guess is that there aren't as many eyes on this as there might typically\n> be because of Git Merge.\n\nYes, Git Merge took all of my attention in the past week, so I couldn't\nchime in at all here.\n\nMy \"Helped-by\" includes some small suggestions from me, but mostly I\nfully support having this kind of document. This one is an excellent\nbase to start from for future augmentation as we discover ideas that\ncould avoid sticky situations.\n\nI particularly like how this document assumes good intent from all\nparties, but recommends over-communicating to be sure that intent is\nclear to everyone.\n\nThanks,\n-Stolee\n"},{"id":"463183","messageId":"ono01655-2r33-9081-q542-p2sp8r5n65s3@tzk.qr","threadId":"58411","inReplyTo":"925b1e37-17aa-9168-9246-ac48e043c0d4@github.com","subject":"Re: [PATCH] Documentation: add ReviewingGuidelines","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-19T15:48:46Z","receivedAt":"2022-09-19T15:49:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 19 Sep 2022, Derrick Stolee wrote:\n\n> On 9/13/2022 7:11 PM, Victoria Dye wrote:\n> > Junio C Hamano wrote:\n> >> \"Victoria Dye via GitGitGadget\" writes:\n> >>\n> >>> From: Victoria Dye \n> >>>\n> >>> Add a reviewing guidelines document including advice and common terminology\n> >>> used in Git mailing list reviews. The document is included in the\n> >>> 'TECH_DOCS' list in order to include it in Git's published documentation.\n> >>>\n> >>> Helped-by: Johannes Schindelin \n> >>> Helped-by: Derrick Stolee \n> >>> Signed-off-by: Victoria Dye \n> >>> ---\n> >>\n> >> I've commented on the text but haven't seen anybody else reviewing.\n> >> No interest? Everybody silently happy?\n> >\n> > My guess is that there aren't as many eyes on this as there might typically\n> > be because of Git Merge.\n>\n> Yes, Git Merge took all of my attention in the past week, so I couldn't\n> chime in at all here.\n>\n> My \"Helped-by\" includes some small suggestions from me, but mostly I\n> fully support having this kind of document. This one is an excellent\n> base to start from for future augmentation as we discover ideas that\n> could avoid sticky situations.\n>\n> I particularly like how this document assumes good intent from all\n> parties, but recommends over-communicating to be sure that intent is\n> clear to everyone.\n\nWhat Stolee said.\n\nI really like how this document serves as a great inspiration to align\nactions with intentions (answering the question \"How do I craft my review\nin a way that the reader _sees_ my good intention, too?\"; Sometimes there\nis a disconnect between intent and impact).\n\nThank you for putting this together, Victoria!\nDscho\n"},{"id":"463217","messageId":"kl6lbkrbcjqb.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58411","inReplyTo":"YyO0U+52vJuTlfo7@google.com","subject":"Re: [PATCH] Documentation: add ReviewingGuidelines","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-09-19T18:17:00Z","receivedAt":"2022-09-19T18:17:10Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Josh Steadmon writes:\n\n> I like all the advice here, thanks for the patch!\n\nI agree, I think this doc is a great step forward. Thanks!\n\n> My only quibble is\n> that the items in the \"Reviewing patches\" section might be worth\n> re-ordering based on topic / importance.\n\nI share this preference for organizing around the topic, especially\nsince it make it easier for us to think about what topics we haven't\nadequately addressed. For example, I'm interested in how reviewers can\nsustainably review and avoid burnout while still providing the reviews\nthat the project needs. (Perhaps we could help reviewers figure out when\nthey've hit the point of diminishing returns in their review?)\n\nNevertheless, I don't think this should block this patch from getting\nmerged; I find the doc very helpful as-is.\n\n>\n> On 2022.09.09 18:13, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye \n>> \n>> Add a reviewing guidelines document including advice and common terminology\n>> used in Git mailing list reviews. The document is included in the\n>> 'TECH_DOCS' list in order to include it in Git's published documentation.\n>> \n>> Helped-by: Johannes Schindelin \n>> Helped-by: Derrick Stolee \n>> Signed-off-by: Victoria Dye \n>> ---\n>> Documentation: add ReviewingGuidelines\n>> \n>> This patch follows up on a discussion a few weeks ago in the Git IRC\n>> standup [1], where it was mentioned that it would be nice to have\n>> consistent definitions for common review terminology (like 'nit:'). The\n>> \"ReviewingGuidelines\" document created here builds on that idea, as well\n>> as past discussions around the idea of advice for reviewers (similar to\n>> the guidelines for new contributors in MyFirstContribution [2]).\n>> \n>> The goal of this document is to clarify & standardize some of the more\n>> niche concepts important to the Git project (\"What's cooking\" emails,\n>> terminology), as well as provide general reviewing advice based on my\n>> observations of effective reviews from others on the mailing list.\n>> \n>> One thing that's particularly important to me here is that the advice\n>> presented here does not gatekeep or otherwise denigrate the personal\n>> preferences or style of reviewers. With that in mind, one of the things\n>> I'm looking for in reviews of this document is making sure that the tone\n>> & content reflect that more positive/encouraging intent. And, of course,\n>> I'm happy to hear what other tips & terminology people think would be\n>> helpful to include!\n>> \n>> Thanks!\n>> \n>> * Victoria\n>> \n>> [1]\n>> https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53\n>> [2] https://git-scm.com/docs/MyFirstContribution\n>> \n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v1\n>> Pull-Request: https://github.com/gitgitgadget/git/pull/1348\n>> \n>> Documentation/Makefile | 1 +\n>> Documentation/ReviewingGuidelines.txt | 160 ++++++++++++++++++++++++++\n>> 2 files changed, 161 insertions(+)\n>> create mode 100644 Documentation/ReviewingGuidelines.txt\n>> \n>> diff --git a/Documentation/Makefile b/Documentation/Makefile\n>> index bd6b6fcb930..d3a19df8bed 100644\n>> --- a/Documentation/Makefile\n>> +++ b/Documentation/Makefile\n>> @@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases\n>> API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))\n>> SP_ARTICLES += $(API_DOCS)\n>> \n>> +TECH_DOCS += ReviewingGuidelines\n>> TECH_DOCS += MyFirstContribution\n>> TECH_DOCS += MyFirstObjectWalk\n>> TECH_DOCS += SubmittingPatches\n>> diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt\n>> new file mode 100644\n>> index 00000000000..bcc59baf863\n>> --- /dev/null\n>> +++ b/Documentation/ReviewingGuidelines.txt\n>> @@ -0,0 +1,160 @@\n>> +Reviewing Patches in the Git Project\n>> +====================================\n>> +\n>> +Introduction\n>> +------------\n>> +The Git development community is a widely distributed, diverse, ever-changing\n>> +group of individuals. Asynchronous communication via the Git mailing list poses\n>> +unique challenges when reviewing or discussing patches. This document contains\n>> +some guiding principles and helpful tools you can use to make your reviews both\n>> +more efficient for yourself and more effective for other contributors.\n>> +\n>> +Note that none of the recommendations here are binding or in any way a\n>> +requirement of participation in the Git community. They are provided as a\n>> +resource to supplement your skills as a contributor.\n>> +\n>> +Principles\n>> +----------\n>> +\n>> +Selecting patch(es) to review\n>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>> +If you are looking for a patch series in need of review, start by checking\n>> +latest \"What's cooking in git.git\" email\n>> +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The \"What's\n>> +cooking\" emails & replies can be found using the query `s:\"What's cooking\"` on\n>> +the mailing list archive; alternatively, you can find the contents of the\n>> +\"What's cooking\" email tracked in `whats-cooking.txt` on the `todo` branch of\n>> +Git. Topics tagged with \"Needs review\" and those in the \"[New Topics]\" section\n>> +are typically those that would benefit the most from additional review.\n>> +\n>> +Patches can also be searched manually in the mailing list archive using a query\n>> +like `s:\"PATCH\" -s:\"Re:\"`. You can browse these results for topics relevant to\n>> +your expertise or interest.\n>> +\n>> +If you've already contributed to Git, you may also be CC'd in another\n>> +contributor's patch series. These are usually topics where the author feels that\n>> +your attention is warranted; this may be due to prior contributions,\n>> +demonstrated expertise, and/or interest in related topics. There is no\n>> +requirement to review these series, but you may find them easier to review as a\n>> +result of your preexisting background knowledge on the topic.\n>> +\n>> +Reviewing patches\n>> +~~~~~~~~~~~~~~~~~\n>> +While every contributor takes their own approach to reviewing patches, here are\n>> +some general pieces of advice to make your reviews to be as clear and helpful as\n>> +possible.\n>> +\n>> +- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n>> + relevant patch. Comments should be made inline, immediately below the relevant\n>> + section(s).\n>> +\n>> +- Remember to review the content of commit messages for correctness and clarity,\n>> + in addition to the code change in the patch's diff. The commit message of a\n>> + patch should accurately and fully explain the code change being made in the\n>> + diff.\n>> +\n>> +- You may find that the limited context provided in the patch diff is sometimes\n>> + insufficient for a thorough review. In such cases, you can review patches in\n>> + your local tree by either applying patches with linkgit:git-am[1] or checking\n>> + out the associated branch from https://github.com/gitster/git once the series\n>> + is tracked there.\n>> +\n>> +- Large, complicated patch diffs are sometimes unavoidable, such as when they\n>> + refactor existing code. If you find such a patch difficult to parse, try\n>> + reviewing the diff produced with the `--color-moved` and/or\n>> + `--ignore-space-change` options.\n>> +\n>> +- Reviewing test coverage is an important - but easy to overlook - component of\n>> + reviews. A patch's changes may be covered by existing tests, or new tests may\n>> + be introduced to exercise new behavior. Checking out a patch or series locally\n>> + allows you to manually mutate lines of new & existing tests to verify expected\n>> + pass/fail behavior. You can use this information to verify proper coverage or\n>> + to suggest additional tests the author could add.\n>> +\n>> +- If a patch is long, you can delete parts of it that are unrelated to your\n>> + review from the email reply. Make sure to leave enough context for readers to\n>> + understand your comments!\n>> +\n>> +- When pointing out an issue, try to include suggestions for how the author\n>> + could fix it. This not only helps the author to understand and fix the issue,\n>> + it also deepens and improves your understanding of the topic.\n>> +\n>> +- Reviews do not need to exclusively point out problems. Feel free to \"think out\n>> + loud\" in your review: describe how you read & understood a complex section of\n>> + a patch, ask a question about something that confused you, point out something\n>> + you found exceptionally well-written, etc. In particular, uplifting feedback\n>> + goes a long way towards encouraging contributors to participate more actively\n>> + in the Git community.\n>> +\n>> +- When providing a recommendation, be as clear as possible about whether you\n>> + consider it \"blocking\" (the code would be broken or otherwise made worse if an\n>> + issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n>> + the recommendation, but acceptance of the series does not require it).\n>> + Non-blocking recommendations can be particularly ambiguous when they are\n>> + related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n>> + they represent only stylistic differences between the author and reviewer.\n>> +\n>> +- If you cannot complete a full review of a series all at once, consider letting\n>> + the author know (on- or off-list) if/when you plan to review the rest of the\n>> + series.\n>> +\n>> +- If you read and review a series but find nothing that warrants inline\n>> + commentary, reply to the series' cover letter to indicate that you've reviewed\n>> + the changes.\n>> +\n>> +Completing a review\n>> +~~~~~~~~~~~~~~~~~~~\n>> +Once each patch of a series is reviewed, the author (and/or other contributors)\n>> +may discuss the review(s). This may result in no changes being applied, or the\n>> +author will send a new version of their patch(es).\n>> +\n>> +After a series is rerolled in response to your or others' review, make sure to\n>> +re-review the updates. If you are happy with the state of the patch series,\n>> +explicitly indicate your approval (typically with a reply to the latest\n>> +version's cover letter). Optionally, you can let the author know that they can\n>> +add a \"Reviewed-by: \" trailer to subsequent versions of their series.\n>> +\n>> +Finally, subsequent \"What's cooking\" emails may explicitly ask whether a\n>> +reviewed topic is ready for merging to the `next` branch (typically phrased\n>> +\"Will merge to 'next'?\"). You can help the maintainer and author by responding\n>> +with a short description of the state of your (and others', if applicable)\n>> +review.\n>> +\n>> +Terminology\n>> +-----------\n>> +nit: ::\n>> +\tDenotes a small issue that should be fixed, such as a typographical error\n>> +\tor mis-alignment of conditions in an `if()` statement.\n>> +\n>> +aside: ::\n>> +optional: ::\n>> +non-blocking: ::\n>> +\tIndicates to the reader that the following comment should not block the\n>> +\tacceptance of the patch or series. These are typically recommendations\n>> +\trelated to code organization & style, or musings about topics related to\n>> +\tthe patch in question, but beyond its scope.\n>> +\n>> +s///::\n>> +\tShorthand for \"you wrote , but I think you meant ,\" usually\n>> +\tfor misspellings or other typographical errors. The syntax is a reference\n>> +\tto \"substitute\" command commonly found in Unix tools such as `ed`, `sed`,\n>> +\t`vim`, and `perl`.\n>> +\n>> +cover letter::\n>> +\tThe \"Patch 0\" of a multi-patch series. This email describes the\n>> +\thigh-level intent and structure of the patch series to readers on the\n>> +\tGit mailing list. It is also where the changelog notes and range-diff of\n>> +\tsubsequent versions are provided by the author.\n>> ++\n>> +On single-patch submissions, cover letter content is typically not sent as a\n>> +separate email. Instead, it is inserted between the end of the patch's commit\n>> +message (after the `---`) and the beginning of the diff.\n>> +\n>> +#leftoverbits::\n>> + Used by either an author or a reviewer to describe features or suggested\n>> + changes that are out-of-scope of a given patch or series, but are relevant\n>> + to the topic for the sake of discussion.\n>> +\n>> +See Also\n>> +--------\n>> +link:MyFirstContribution.html[MyFirstContribution]\n>> \n>> base-commit: 79f2338b3746d23454308648b2491e5beba4beff\n>> -- \n>> gitgitgadget\n"},{"id":"463225","messageId":"pull.1348.v2.git.1663614767058.gitgitgadget@gmail.com","threadId":"58411","inReplyTo":"pull.1348.git.1662747205235.gitgitgadget@gmail.com","subject":"[PATCH v2] Documentation: add ReviewingGuidelines","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-19T19:12:46Z","receivedAt":"2022-09-19T19:12:56Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a reviewing guidelines document including advice and common terminology\nused in Git mailing list reviews. The document is included in the\n'TECH_DOCS' list in order to include it in Git's published documentation.\n\nHelped-by: Johannes Schindelin \nHelped-by: Derrick Stolee \nHelped-by: Junio C Hamano \nHelped-by: Josh Steadmon \nSigned-off-by: Victoria Dye \n---\n Documentation: add ReviewingGuidelines\n \n This patch follows up on a discussion a few weeks ago in the Git IRC\n standup [1], where it was mentioned that it would be nice to have\n consistent definitions for common review terminology (like 'nit:'). The\n \"ReviewingGuidelines\" document created here builds on that idea, as well\n as past discussions around the idea of advice for reviewers (similar to\n the guidelines for new contributors in MyFirstContribution [2]).\n \n The goal of this document is to clarify & standardize some of the more\n niche concepts important to the Git project (\"What's cooking\" emails,\n terminology), as well as provide general reviewing advice based on my\n observations of effective reviews from others on the mailing list.\n \n One thing that's particularly important to me here is that the advice\n presented here does not gatekeep or otherwise denigrate the personal\n preferences or style of reviewers. With that in mind, one of the things\n I'm looking for in reviews of this document is making sure that the tone\n & content reflect that more positive/encouraging intent. And, of course,\n I'm happy to hear what other tips & terminology people think would be\n helpful to include!\n \n \n Changes since V1\n ================\n \n * Reorganized \"Principles\" section advice into \"High-level guidance\"\n and \"Performing your review\" subsections.\n * Dropped recommendation to comment on cover letter with \"LGTM\" if you\n have no other recommendations (somewhat redundant with the\n \"Completing a review\" section, and such comments don't tend to add\n value except when coming from highly-experienced reviewers anyway).\n * Added clarity & modified reasoning for why reviewing CC'd patches is\n helpful.\n * Miscellaneous other revisions recommended by [3].\n \n Thanks!\n \n * Victoria\n \n [1]\n https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-29#l53\n [2] https://git-scm.com/docs/MyFirstContribution [3]\n https://lore.kernel.org/git/xmqqwnacibbm.fsf@gitster.g/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1348%2Fvdye%2Ffeature%2Freviewing-guidelines-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1348/vdye/feature/reviewing-guidelines-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1348\n\nRange-diff vs v1:\n\n 1: b2ed5641c24 ! 1: 7326058b23a Documentation: add ReviewingGuidelines\n @@ Commit message\n \n Helped-by: Johannes Schindelin \n Helped-by: Derrick Stolee \n + Helped-by: Junio C Hamano \n + Helped-by: Josh Steadmon \n Signed-off-by: Victoria Dye \n \n ## Documentation/Makefile ##\n @@ Documentation/ReviewingGuidelines.txt (new)\n +latest \"What's cooking in git.git\" email\n +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The \"What's\n +cooking\" emails & replies can be found using the query `s:\"What's cooking\"` on\n -+the mailing list archive; alternatively, you can find the contents of the\n -+\"What's cooking\" email tracked in `whats-cooking.txt` on the `todo` branch of\n -+Git. Topics tagged with \"Needs review\" and those in the \"[New Topics]\" section\n -+are typically those that would benefit the most from additional review.\n ++the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive];\n ++alternatively, you can find the contents of the \"What's cooking\" email tracked\n ++in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with \"Needs\n ++review\" and those in the \"[New Topics]\" section are typically those that would\n ++benefit the most from additional review.\n +\n +Patches can also be searched manually in the mailing list archive using a query\n +like `s:\"PATCH\" -s:\"Re:\"`. You can browse these results for topics relevant to\n +your expertise or interest.\n +\n +If you've already contributed to Git, you may also be CC'd in another\n -+contributor's patch series. These are usually topics where the author feels that\n -+your attention is warranted; this may be due to prior contributions,\n -+demonstrated expertise, and/or interest in related topics. There is no\n -+requirement to review these series, but you may find them easier to review as a\n -+result of your preexisting background knowledge on the topic.\n ++contributor's patch series. These are topics where the author feels that your\n ++attention is warranted. This may be because their patch changes something you\n ++wrote previously (making you a good judge of whether the new approach does or\n ++doesn't work), or because you have the expertise to provide an exceptionally\n ++helpful review. There is no requirement to review these patches but, in the\n ++spirit of open source collaboration, you should strongly consider doing so.\n +\n +Reviewing patches\n +~~~~~~~~~~~~~~~~~\n +While every contributor takes their own approach to reviewing patches, here are\n -+some general pieces of advice to make your reviews to be as clear and helpful as\n -+possible.\n -+\n -+- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n -+ relevant patch. Comments should be made inline, immediately below the relevant\n -+ section(s).\n ++some general pieces of advice to make your reviews as clear and helpful as\n ++possible. The advice is broken into two rough categories: high-level reviewing\n ++guidance, and concrete tips for interacting with patches on the mailing list.\n +\n ++==== High-level guidance\n +- Remember to review the content of commit messages for correctness and clarity,\n + in addition to the code change in the patch's diff. The commit message of a\n + patch should accurately and fully explain the code change being made in the\n + diff.\n +\n -+- You may find that the limited context provided in the patch diff is sometimes\n -+ insufficient for a thorough review. In such cases, you can review patches in\n -+ your local tree by either applying patches with linkgit:git-am[1] or checking\n -+ out the associated branch from https://github.com/gitster/git once the series\n -+ is tracked there.\n -+\n -+- Large, complicated patch diffs are sometimes unavoidable, such as when they\n -+ refactor existing code. If you find such a patch difficult to parse, try\n -+ reviewing the diff produced with the `--color-moved` and/or\n -+ `--ignore-space-change` options.\n -+\n +- Reviewing test coverage is an important - but easy to overlook - component of\n + reviews. A patch's changes may be covered by existing tests, or new tests may\n + be introduced to exercise new behavior. Checking out a patch or series locally\n @@ Documentation/ReviewingGuidelines.txt (new)\n + pass/fail behavior. You can use this information to verify proper coverage or\n + to suggest additional tests the author could add.\n +\n -+- If a patch is long, you can delete parts of it that are unrelated to your\n -+ review from the email reply. Make sure to leave enough context for readers to\n -+ understand your comments!\n ++- When providing a recommendation, be as clear as possible about whether you\n ++ consider it \"blocking\" (the code would be broken or otherwise made worse if an\n ++ issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n ++ the recommendation, but acceptance of the series does not require it).\n ++ Non-blocking recommendations can be particularly ambiguous when they are\n ++ related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n ++ they represent only stylistic differences between the author and reviewer.\n +\n -+- When pointing out an issue, try to include suggestions for how the author\n ++- When commenting on an issue, try to include suggestions for how the author\n + could fix it. This not only helps the author to understand and fix the issue,\n + it also deepens and improves your understanding of the topic.\n +\n @@ Documentation/ReviewingGuidelines.txt (new)\n + goes a long way towards encouraging contributors to participate more actively\n + in the Git community.\n +\n -+- When providing a recommendation, be as clear as possible about whether you\n -+ consider it \"blocking\" (the code would be broken or otherwise made worse if an\n -+ issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n -+ the recommendation, but acceptance of the series does not require it).\n -+ Non-blocking recommendations can be particularly ambiguous when they are\n -+ related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n -+ they represent only stylistic differences between the author and reviewer.\n ++==== Performing your review\n ++- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n ++ relevant patch. Comments should be made inline, immediately below the relevant\n ++ section(s).\n ++\n ++- You may find that the limited context provided in the patch diff is sometimes\n ++ insufficient for a thorough review. In such cases, you can review patches in\n ++ your local tree by either applying patches with linkgit:git-am[1] or checking\n ++ out the associated branch from https://github.com/gitster/git once the series\n ++ is tracked there.\n ++\n ++- Large, complicated patch diffs are sometimes unavoidable, such as when they\n ++ refactor existing code. If you find such a patch difficult to parse, try\n ++ reviewing the diff produced with the `--color-moved` and/or\n ++ `--ignore-space-change` options.\n ++\n ++- If a patch is long, you are encouraged to delete parts of it that are\n ++ unrelated to your review from the email reply. Make sure to leave enough\n ++ context for readers to understand your comments!\n +\n +- If you cannot complete a full review of a series all at once, consider letting\n + the author know (on- or off-list) if/when you plan to review the rest of the\n + series.\n +\n -+- If you read and review a series but find nothing that warrants inline\n -+ commentary, reply to the series' cover letter to indicate that you've reviewed\n -+ the changes.\n -+\n +Completing a review\n +~~~~~~~~~~~~~~~~~~~\n +Once each patch of a series is reviewed, the author (and/or other contributors)\n @@ Documentation/ReviewingGuidelines.txt (new)\n +re-review the updates. If you are happy with the state of the patch series,\n +explicitly indicate your approval (typically with a reply to the latest\n +version's cover letter). Optionally, you can let the author know that they can\n -+add a \"Reviewed-by: \" trailer to subsequent versions of their series.\n ++add a \"Reviewed-by: \" trailer if they resubmit the reviewed patch verbatim\n ++in a later iteration of the series.\n +\n +Finally, subsequent \"What's cooking\" emails may explicitly ask whether a\n +reviewed topic is ready for merging to the `next` branch (typically phrased\n -+\"Will merge to 'next'?\"). You can help the maintainer and author by responding\n ++\"Will merge to \\'next\\'?\"). You can help the maintainer and author by responding\n +with a short description of the state of your (and others', if applicable)\n -+review.\n ++review, including the links to the relevant thread(s).\n +\n +Terminology\n +-----------\n\n\n Documentation/Makefile | 1 +\n Documentation/ReviewingGuidelines.txt | 162 ++++++++++++++++++++++++++\n 2 files changed, 163 insertions(+)\n create mode 100644 Documentation/ReviewingGuidelines.txt\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex bd6b6fcb930..d3a19df8bed 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -101,6 +101,7 @@ SP_ARTICLES += howto/coordinate-embargoed-releases\n API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))\n SP_ARTICLES += $(API_DOCS)\n \n+TECH_DOCS += ReviewingGuidelines\n TECH_DOCS += MyFirstContribution\n TECH_DOCS += MyFirstObjectWalk\n TECH_DOCS += SubmittingPatches\ndiff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt\nnew file mode 100644\nindex 00000000000..0e323d54779\n--- /dev/null\n+++ b/Documentation/ReviewingGuidelines.txt\n@@ -0,0 +1,162 @@\n+Reviewing Patches in the Git Project\n+====================================\n+\n+Introduction\n+------------\n+The Git development community is a widely distributed, diverse, ever-changing\n+group of individuals. Asynchronous communication via the Git mailing list poses\n+unique challenges when reviewing or discussing patches. This document contains\n+some guiding principles and helpful tools you can use to make your reviews both\n+more efficient for yourself and more effective for other contributors.\n+\n+Note that none of the recommendations here are binding or in any way a\n+requirement of participation in the Git community. They are provided as a\n+resource to supplement your skills as a contributor.\n+\n+Principles\n+----------\n+\n+Selecting patch(es) to review\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n+If you are looking for a patch series in need of review, start by checking\n+latest \"What's cooking in git.git\" email\n+(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The \"What's\n+cooking\" emails & replies can be found using the query `s:\"What's cooking\"` on\n+the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive];\n+alternatively, you can find the contents of the \"What's cooking\" email tracked\n+in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with \"Needs\n+review\" and those in the \"[New Topics]\" section are typically those that would\n+benefit the most from additional review.\n+\n+Patches can also be searched manually in the mailing list archive using a query\n+like `s:\"PATCH\" -s:\"Re:\"`. You can browse these results for topics relevant to\n+your expertise or interest.\n+\n+If you've already contributed to Git, you may also be CC'd in another\n+contributor's patch series. These are topics where the author feels that your\n+attention is warranted. This may be because their patch changes something you\n+wrote previously (making you a good judge of whether the new approach does or\n+doesn't work), or because you have the expertise to provide an exceptionally\n+helpful review. There is no requirement to review these patches but, in the\n+spirit of open source collaboration, you should strongly consider doing so.\n+\n+Reviewing patches\n+~~~~~~~~~~~~~~~~~\n+While every contributor takes their own approach to reviewing patches, here are\n+some general pieces of advice to make your reviews as clear and helpful as\n+possible. The advice is broken into two rough categories: high-level reviewing\n+guidance, and concrete tips for interacting with patches on the mailing list.\n+\n+==== High-level guidance\n+- Remember to review the content of commit messages for correctness and clarity,\n+ in addition to the code change in the patch's diff. The commit message of a\n+ patch should accurately and fully explain the code change being made in the\n+ diff.\n+\n+- Reviewing test coverage is an important - but easy to overlook - component of\n+ reviews. A patch's changes may be covered by existing tests, or new tests may\n+ be introduced to exercise new behavior. Checking out a patch or series locally\n+ allows you to manually mutate lines of new & existing tests to verify expected\n+ pass/fail behavior. You can use this information to verify proper coverage or\n+ to suggest additional tests the author could add.\n+\n+- When providing a recommendation, be as clear as possible about whether you\n+ consider it \"blocking\" (the code would be broken or otherwise made worse if an\n+ issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n+ the recommendation, but acceptance of the series does not require it).\n+ Non-blocking recommendations can be particularly ambiguous when they are\n+ related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n+ they represent only stylistic differences between the author and reviewer.\n+\n+- When commenting on an issue, try to include suggestions for how the author\n+ could fix it. This not only helps the author to understand and fix the issue,\n+ it also deepens and improves your understanding of the topic.\n+\n+- Reviews do not need to exclusively point out problems. Feel free to \"think out\n+ loud\" in your review: describe how you read & understood a complex section of\n+ a patch, ask a question about something that confused you, point out something\n+ you found exceptionally well-written, etc. In particular, uplifting feedback\n+ goes a long way towards encouraging contributors to participate more actively\n+ in the Git community.\n+\n+==== Performing your review\n+- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n+ relevant patch. Comments should be made inline, immediately below the relevant\n+ section(s).\n+\n+- You may find that the limited context provided in the patch diff is sometimes\n+ insufficient for a thorough review. In such cases, you can review patches in\n+ your local tree by either applying patches with linkgit:git-am[1] or checking\n+ out the associated branch from https://github.com/gitster/git once the series\n+ is tracked there.\n+\n+- Large, complicated patch diffs are sometimes unavoidable, such as when they\n+ refactor existing code. If you find such a patch difficult to parse, try\n+ reviewing the diff produced with the `--color-moved` and/or\n+ `--ignore-space-change` options.\n+\n+- If a patch is long, you are encouraged to delete parts of it that are\n+ unrelated to your review from the email reply. Make sure to leave enough\n+ context for readers to understand your comments!\n+\n+- If you cannot complete a full review of a series all at once, consider letting\n+ the author know (on- or off-list) if/when you plan to review the rest of the\n+ series.\n+\n+Completing a review\n+~~~~~~~~~~~~~~~~~~~\n+Once each patch of a series is reviewed, the author (and/or other contributors)\n+may discuss the review(s). This may result in no changes being applied, or the\n+author will send a new version of their patch(es).\n+\n+After a series is rerolled in response to your or others' review, make sure to\n+re-review the updates. If you are happy with the state of the patch series,\n+explicitly indicate your approval (typically with a reply to the latest\n+version's cover letter). Optionally, you can let the author know that they can\n+add a \"Reviewed-by: \" trailer if they resubmit the reviewed patch verbatim\n+in a later iteration of the series.\n+\n+Finally, subsequent \"What's cooking\" emails may explicitly ask whether a\n+reviewed topic is ready for merging to the `next` branch (typically phrased\n+\"Will merge to \\'next\\'?\"). You can help the maintainer and author by responding\n+with a short description of the state of your (and others', if applicable)\n+review, including the links to the relevant thread(s).\n+\n+Terminology\n+-----------\n+nit: ::\n+\tDenotes a small issue that should be fixed, such as a typographical error\n+\tor mis-alignment of conditions in an `if()` statement.\n+\n+aside: ::\n+optional: ::\n+non-blocking: ::\n+\tIndicates to the reader that the following comment should not block the\n+\tacceptance of the patch or series. These are typically recommendations\n+\trelated to code organization & style, or musings about topics related to\n+\tthe patch in question, but beyond its scope.\n+\n+s///::\n+\tShorthand for \"you wrote , but I think you meant ,\" usually\n+\tfor misspellings or other typographical errors. The syntax is a reference\n+\tto \"substitute\" command commonly found in Unix tools such as `ed`, `sed`,\n+\t`vim`, and `perl`.\n+\n+cover letter::\n+\tThe \"Patch 0\" of a multi-patch series. This email describes the\n+\thigh-level intent and structure of the patch series to readers on the\n+\tGit mailing list. It is also where the changelog notes and range-diff of\n+\tsubsequent versions are provided by the author.\n++\n+On single-patch submissions, cover letter content is typically not sent as a\n+separate email. Instead, it is inserted between the end of the patch's commit\n+message (after the `---`) and the beginning of the diff.\n+\n+#leftoverbits::\n+ Used by either an author or a reviewer to describe features or suggested\n+ changes that are out-of-scope of a given patch or series, but are relevant\n+ to the topic for the sake of discussion.\n+\n+See Also\n+--------\n+link:MyFirstContribution.html[MyFirstContribution]\n\nbase-commit: 79f2338b3746d23454308648b2491e5beba4beff\n-- \ngitgitgadget\n"},{"id":"463234","messageId":"YyjNxlJEZ2WESP3C@google.com","threadId":"58411","inReplyTo":"pull.1348.v2.git.1663614767058.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Documentation: add ReviewingGuidelines","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-09-19T20:15:02Z","receivedAt":"2022-09-19T20:15:16Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.09.19 19:12, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Add a reviewing guidelines document including advice and common terminology\n> used in Git mailing list reviews. The document is included in the\n> 'TECH_DOCS' list in order to include it in Git's published documentation.\n> \n> Helped-by: Johannes Schindelin \n> Helped-by: Derrick Stolee \n> Helped-by: Junio C Hamano \n> Helped-by: Josh Steadmon \n> Signed-off-by: Victoria Dye \n> ---\n\nLooks great, thanks!\n\nReviewed-by: Josh Steadmon \n"},{"id":"463235","messageId":"xmqq4jx39hb6.fsf@gitster.g","threadId":"58411","inReplyTo":"pull.1348.v2.git.1663614767058.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Documentation: add ReviewingGuidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-19T21:37:33Z","receivedAt":"2022-09-19T21:38:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Add a reviewing guidelines document including advice and common terminology\n> used in Git mailing list reviews. The document is included in the\n> 'TECH_DOCS' list in order to include it in Git's published documentation.\n>\n> Helped-by: Johannes Schindelin \n> Helped-by: Derrick Stolee \n> Helped-by: Junio C Hamano \n> Helped-by: Josh Steadmon \n> Signed-off-by: Victoria Dye \n> ---\n> Documentation: add ReviewingGuidelines\n> \n> This patch follows up on a discussion a few weeks ago in the Git IRC\n> standup [1], where it was mentioned that it would be nice to have\n> consistent definitions for common review terminology (like 'nit:'). The\n> \"ReviewingGuidelines\" document created here builds on that idea, as well\n> as past discussions around the idea of advice for reviewers (similar to\n> the guidelines for new contributors in MyFirstContribution [2]).\n\nThanks. Will queue.\n\nI think this is ready for 'next' and then to 'master' during this\ncycle. Thanks for writing it, and thanks all for reviewing it.\n"},{"id":"463249","messageId":"CABPp-BEB_+YoKZ=U6NPc8J+KZyMSYRsom34CeqjxUCyw0=LEyg@mail.gmail.com","threadId":"58411","inReplyTo":"pull.1348.v2.git.1663614767058.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Documentation: add ReviewingGuidelines","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-20T00:43:15Z","receivedAt":"2022-09-20T00:43:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 19, 2022 at 12:21 PM Victoria Dye via GitGitGadget\n wrote:\n>\n[...]\n> +==== Performing your review\n> +- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n> + relevant patch. Comments should be made inline, immediately below the relevant\n> + section(s).\n> +\n> +- You may find that the limited context provided in the patch diff is sometimes\n> + insufficient for a thorough review. In such cases, you can review patches in\n> + your local tree by either applying patches with linkgit:git-am[1] or checking\n> + out the associated branch from https://github.com/gitster/git once the series\n> + is tracked there.\n\nLots of reviews also come with \"Fetch-It-Via\" instructions in the\ncover letter, making it really easy to grab. Might be worth\nmentioning?\n\nAlso, would it make sense for us to replace \"applying\" with\n\"downloading and applying\", perhaps mentioning `b4 am` for the\ndownloading half?\n\n(I tend to use the Fetch-It-Via or wait for it to show up in\ngitster/git, but b4 is really nice for the other cases.)\n\n> +- Large, complicated patch diffs are sometimes unavoidable, such as when they\n> + refactor existing code. If you find such a patch difficult to parse, try\n> + reviewing the diff produced with the `--color-moved` and/or\n> + `--ignore-space-change` options.\n\nSimilarly, Documentation refactorings or significant rewordings are\nsometimes easier to view with --color-words or --color-words=.\n\n[...]\n> +See Also\n> +--------\n> +link:MyFirstContribution.html[MyFirstContribution]\n>\n> base-commit: 79f2338b3746d23454308648b2491e5beba4beff\n\nI like this document! I had a couple ideas that might or might not\nmake sense to include in the document; it looks good to me either way.\n"},{"id":"463348","messageId":"20220920212350.f5do44qqduhyp46u@meerkat.local","threadId":"58411","inReplyTo":"CABPp-BEB_+YoKZ=U6NPc8J+KZyMSYRsom34CeqjxUCyw0=LEyg@mail.gmail.com","subject":"Re: [PATCH v2] Documentation: add ReviewingGuidelines","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2022-09-20T21:23:50Z","receivedAt":"2022-09-20T21:23:57Z","isPatch":true,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On Mon, Sep 19, 2022 at 05:43:15PM -0700, Elijah Newren wrote:\n> > +- You may find that the limited context provided in the patch diff is sometimes\n> > + insufficient for a thorough review. In such cases, you can review patches in\n> > + your local tree by either applying patches with linkgit:git-am[1] or checking\n> > + out the associated branch from https://github.com/gitster/git once the series\n> > + is tracked there.\n> \n> Lots of reviews also come with \"Fetch-It-Via\" instructions in the\n> cover letter, making it really easy to grab. Might be worth\n> mentioning?\n> \n> Also, would it make sense for us to replace \"applying\" with\n> \"downloading and applying\", perhaps mentioning `b4 am` for the\n> downloading half?\n\nB4 can also \"convert\" a patch series into a pull request using \"shazam\".\nE.g.:\n\n b4 shazam -H \n\nThis will do some behind-the-scenes magic and give you a FETCH_HEAD that you\ncan review, check out into a new branch, merge, etc.\n\nYou can try this with this very thread, if you are inside the git's own repo:\n\n\t$ b4 shazam -H pull.1348.git.1662747205235.gitgitgadget@gmail.com\n\tGrabbing thread from lore.kernel.org/all/pull.1348.git.1662747205235.gitgitgadget%40gmail.com/t.mbox.gz\n\tChecking for newer revisions on https://lore.kernel.org/all/\n\tAnalyzing 12 messages in the thread\n\tWill use the latest revision: v2\n\tYou can pick other revisions using the -vN flag\n\tChecking attestation on all messages, may take a moment...\n\t---\n\t ✓ [PATCH v2] Documentation: add ReviewingGuidelines\n\t\t+ Reviewed-by: Josh Steadmon (✓ DKIM/google.com)\n\t ---\n\t ✓ Signed: DKIM/gmail.com\n\t---\n\tTotal patches: 1\n\t---\n\tMagic: Preparing a sparse worktree\n\t---\n\tApplying: Documentation: add ReviewingGuidelines\n\t---\n\tFetching into FETCH_HEAD\n\tYou can now merge or checkout FETCH_HEAD\n\t e.g.: git merge --no-ff -F /home/user/work/git/git/.git/b4-cover --edit FETCH_HEAD --signoff\n\n> (I tend to use the Fetch-It-Via or wait for it to show up in\n> gitster/git, but b4 is really nice for the other cases.)\n\nGreat to hear! :)\n\n-Konstantin\n"},{"id":"463416","messageId":"df9b1022-fc96-f0fe-8652-78e8891e3c99@gmail.com","threadId":"58411","inReplyTo":"CABPp-BEB_+YoKZ=U6NPc8J+KZyMSYRsom34CeqjxUCyw0=LEyg@mail.gmail.com","subject":"Re: [PATCH v2] Documentation: add ReviewingGuidelines","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-09-22T04:24:54Z","receivedAt":"2022-09-22T04:25:02Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On 9/19/2022 5:43 PM, Elijah Newren wrote:\n> On Mon, Sep 19, 2022 at 12:21 PM Victoria Dye via GitGitGadget\n> wrote:\n>>\n> [...]\n>> +==== Performing your review\n>> +- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n>> + relevant patch. Comments should be made inline, immediately below the relevant\n>> + section(s).\n>> +\n>> +- You may find that the limited context provided in the patch diff is sometimes\n>> + insufficient for a thorough review. In such cases, you can review patches in\n>> + your local tree by either applying patches with linkgit:git-am[1] or checking\n>> + out the associated branch from https://github.com/gitster/git once the series\n>> + is tracked there.\n> \n> Lots of reviews also come with \"Fetch-It-Via\" instructions in the\n> cover letter, making it really easy to grab. Might be worth\n> mentioning?\n> \n> Also, would it make sense for us to replace \"applying\" with\n> \"downloading and applying\", perhaps mentioning `b4 am` for the\n> downloading half?\n> \n> (I tend to use the Fetch-It-Via or wait for it to show up in\n> gitster/git, but b4 is really nice for the other cases.)\n\nThanks, I did not know about b4, it looks quite helpful!\n\nI think it is worth mentioning some recommended practices to operate\n'git-am'. 'git-am' was a bit confusing the first time I tried to grab\npeople's patches from the mailing list without using \"Fetch-It-Via\",\ne.g. what is mailbox and how to convert emails into mailbox.\n\nb4 sounds like a good start to add these practices, and probably some\nother recommendations (I don't know much here)?\n\nPlease note that these are just some thoughts, the document itself looks\ngood without adding these practices (maybe we can add them later) :-)\n\n[...]\n\nThanks,\nShaoxuan\n"},{"id":"463440","messageId":"93d4683a-81db-2d9a-edd9-a3790c16a5db@gmail.com","threadId":"58411","inReplyTo":"pull.1348.v2.git.1663614767058.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Documentation: add ReviewingGuidelines","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-22T13:29:22Z","receivedAt":"2022-09-22T13:29:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\nThanks for working on this, sorry it has taken me a while to get round \nto looking at it. I think it makes a really useful addition to our \ndocumentation. I've left a few comments below, the only one I feel \nstrongly about is adding something to say what the purpose of the review \nshould be and emphasizing positive comments earlier in the document.\n\nOn 19/09/2022 20:12, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n\n> diff --git a/Documentation/ReviewingGuidelines.txt b/Documentation/ReviewingGuidelines.txt\n> new file mode 100644\n> index 00000000000..0e323d54779\n> --- /dev/null\n> +++ b/Documentation/ReviewingGuidelines.txt\n> @@ -0,0 +1,162 @@\n> +Reviewing Patches in the Git Project\n> +====================================\n> +\n> +Introduction\n> +------------\n> +The Git development community is a widely distributed, diverse, ever-changing\n> +group of individuals. Asynchronous communication via the Git mailing list poses\n> +unique challenges when reviewing or discussing patches. This document contains\n> +some guiding principles and helpful tools you can use to make your reviews both\n> +more efficient for yourself and more effective for other contributors.\n> +\n> +Note that none of the recommendations here are binding or in any way a\n> +requirement of participation in the Git community. They are provided as a\n> +resource to supplement your skills as a contributor.\n> +\n> +Principles\n> +----------\n> +\n> +Selecting patch(es) to review\n> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> +If you are looking for a patch series in need of review, start by checking\n> +latest \"What's cooking in git.git\" email\n> +(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The \"What's\n> +cooking\" emails & replies can be found using the query `s:\"What's cooking\"` on\n> +the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive];\n> +alternatively, you can find the contents of the \"What's cooking\" email tracked\n> +in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with \"Needs\n> +review\" and those in the \"[New Topics]\" section are typically those that would\n> +benefit the most from additional review.\n> +\n> +Patches can also be searched manually in the mailing list archive using a query\n> +like `s:\"PATCH\" -s:\"Re:\"`. You can browse these results for topics relevant to\n> +your expertise or interest.\n> +\n> +If you've already contributed to Git, you may also be CC'd in another\n> +contributor's patch series. These are topics where the author feels that your\n> +attention is warranted. This may be because their patch changes something you\n> +wrote previously (making you a good judge of whether the new approach does or\n> +doesn't work), or because you have the expertise to provide an exceptionally\n\n[optional] Maybe this says something about me, but the use of \n\"exceptionally\" here feels like it is setting quite high expectations \nfor the review. Perhaps we could say \"particularly\" instead\n\n> +helpful review. There is no requirement to review these patches but, in the\n> +spirit of open source collaboration, you should strongly consider doing so.\n> +\n> +Reviewing patches\n> +~~~~~~~~~~~~~~~~~\n> +While every contributor takes their own approach to reviewing patches, here are\n> +some general pieces of advice to make your reviews as clear and helpful as\n> +possible. The advice is broken into two rough categories: high-level reviewing\n> +guidance, and concrete tips for interacting with patches on the mailing list.\n> +\n> +==== High-level guidance\n\nI think it would be worth adding something here (or maybe extending the \nprevious paragraph) to state the purpose of the review and emphasize the \nimportance of positive comments. Maybe something like this (which is a \nreworked version of your final list item)\n\nThe purpose of the review is to provide constructive feedback explaining \nhow you think the patch could be improved or in some cases why the patch \nis not suitable for inclusion. As well as pointing out any problems it \nis helpful to provide positive feedback on aspects that you particularly \nliked. You should also feel free to \"think outloud\" in your review \ndescribing how you read & understood a complex section of a patch. It is \nalso useful to ask questions about anything that confused you\n\n> +- Remember to review the content of commit messages for correctness and clarity,\n> + in addition to the code change in the patch's diff. The commit message of a\n> + patch should accurately and fully explain the code change being made in the\n> + diff.\n> +\n> +- Reviewing test coverage is an important - but easy to overlook - component of\n> + reviews. A patch's changes may be covered by existing tests, or new tests may\n> + be introduced to exercise new behavior. Checking out a patch or series locally\n> + allows you to manually mutate lines of new & existing tests to verify expected\n> + pass/fail behavior. You can use this information to verify proper coverage or\n> + to suggest additional tests the author could add.\n> +\n> +- When providing a recommendation, be as clear as possible about whether you\n> + consider it \"blocking\" (the code would be broken or otherwise made worse if an\n> + issue isn't fixed) or \"non-blocking\" (the patch could be made better by taking\n> + the recommendation, but acceptance of the series does not require it).\n> + Non-blocking recommendations can be particularly ambiguous when they are\n> + related to - but outside the scope of - a series (\"nice-to-have\"s), or when\n> + they represent only stylistic differences between the author and reviewer.\n> +\n> +- When commenting on an issue, try to include suggestions for how the author\n> + could fix it. This not only helps the author to understand and fix the issue,\n> + it also deepens and improves your understanding of the topic.\n> +\n> +- Reviews do not need to exclusively point out problems. Feel free to \"think out\n> + loud\" in your review: describe how you read & understood a complex section of\n> + a patch, ask a question about something that confused you, point out something\n> + you found exceptionally well-written, etc. In particular, uplifting feedback\n> + goes a long way towards encouraging contributors to participate more actively\n> + in the Git community.\n\nAs I said above I'd like to see this come higher up the list\n\n> +==== Performing your review\n> +- Provide your review comments per-patch in a plaintext \"Reply-All\" email to the\n> + relevant patch. Comments should be made inline, immediately below the relevant\n> + section(s).\n> +\n> +- You may find that the limited context provided in the patch diff is sometimes\n> + insufficient for a thorough review. In such cases, you can review patches in\n> + your local tree by either applying patches with linkgit:git-am[1] or checking\n> + out the associated branch from https://github.com/gitster/git once the series\n> + is tracked there.\n> +\n> +- Large, complicated patch diffs are sometimes unavoidable, such as when they\n> + refactor existing code. If you find such a patch difficult to parse, try\n> + reviewing the diff produced with the `--color-moved` and/or\n> + `--ignore-space-change` options.\n\n[optional] --ignore-space-change is quite a blunt instrument, perhaps we \ncould suggest using --color-moved-ws=allow-indentation-change which I \nfind particularly useful for refactorings. I'd also second Elijah's \nsuggestion to mention --color-words for documentation changes.\n\n> +- If a patch is long, you are encouraged to delete parts of it that are\n> + unrelated to your review from the email reply. Make sure to leave enough\n> + context for readers to understand your comments!\n> +\n> +- If you cannot complete a full review of a series all at once, consider letting\n> + the author know (on- or off-list) if/when you plan to review the rest of the\n> + series.\n> +\n> +Completing a review\n> +~~~~~~~~~~~~~~~~~~~\n> +Once each patch of a series is reviewed, the author (and/or other contributors)\n> +may discuss the review(s). This may result in no changes being applied, or the\n> +author will send a new version of their patch(es).\n> +\n> +After a series is rerolled in response to your or others' review, make sure to\n> +re-review the updates. \n\n[optional] Maybe add\n\nWhen re-reviewing the series it is helpful to inspect the range diff to \nsee what the author has changed since your last review.\n\n> If you are happy with the state of the patch series,\n> +explicitly indicate your approval (typically with a reply to the latest\n> +version's cover letter). Optionally, you can let the author know that they can\n> +add a \"Reviewed-by: \" trailer if they resubmit the reviewed patch verbatim\n> +in a later iteration of the series.\n> +\n> +Finally, subsequent \"What's cooking\" emails may explicitly ask whether a\n> +reviewed topic is ready for merging to the `next` branch (typically phrased\n> +\"Will merge to \\'next\\'?\"). You can help the maintainer and author by responding\n> +with a short description of the state of your (and others', if applicable)\n> +review, including the links to the relevant thread(s).\n> +\n> +Terminology\n> +-----------\n> +nit: ::\n> +\tDenotes a small issue that should be fixed, such as a typographical error\n> +\tor mis-alignment of conditions in an `if()` statement.\n> +\n> +aside: ::\n> +optional: ::\n> +non-blocking: ::\n> +\tIndicates to the reader that the following comment should not block the\n> +\tacceptance of the patch or series. These are typically recommendations\n> +\trelated to code organization & style, or musings about topics related to\n> +\tthe patch in question, but beyond its scope.\n\n[optional] The reference to code style made me wonder if we should add \nsomething recommending that reviewers refer patch authors to our coding \nguidelines where appropriate.\n\nThanks again for working on this\n\nPhillip\n\n> +\n> +s///::\n> +\tShorthand for \"you wrote , but I think you meant ,\" usually\n> +\tfor misspellings or other typographical errors. The syntax is a reference\n> +\tto \"substitute\" command commonly found in Unix tools such as `ed`, `sed`,\n> +\t`vim`, and `perl`.\n> +\n> +cover letter::\n> +\tThe \"Patch 0\" of a multi-patch series. This email describes the\n> +\thigh-level intent and structure of the patch series to readers on the\n> +\tGit mailing list. It is also where the changelog notes and range-diff of\n> +\tsubsequent versions are provided by the author.\n> ++\n> +On single-patch submissions, cover letter content is typically not sent as a\n> +separate email. Instead, it is inserted between the end of the patch's commit\n> +message (after the `---`) and the beginning of the diff.\n> +\n> +#leftoverbits::\n> + Used by either an author or a reviewer to describe features or suggested\n> + changes that are out-of-scope of a given patch or series, but are relevant\n> + to the topic for the sake of discussion.\n> +\n> +See Also\n> +--------\n> +link:MyFirstContribution.html[MyFirstContribution]\n> \n> base-commit: 79f2338b3746d23454308648b2491e5beba4beff\n\n"}]}