|
| 1 | +Reviewing Patches in the Git Project |
| 2 | +==================================== |
| 3 | + |
| 4 | +Introduction |
| 5 | +------------ |
| 6 | +The Git development community is a widely distributed, diverse, ever-changing |
| 7 | +group of individuals. Asynchronous communication via the Git mailing list poses |
| 8 | +unique challenges when reviewing or discussing patches. This document contains |
| 9 | +some guiding principles and helpful tools you can use to make your reviews both |
| 10 | +more efficient for yourself and more effective for other contributors. |
| 11 | + |
| 12 | +Note that none of the recommendations here are binding or in any way a |
| 13 | +requirement of participation in the Git community. They are provided as a |
| 14 | +resource to supplement your skills as a contributor. |
| 15 | + |
| 16 | +Principles |
| 17 | +---------- |
| 18 | + |
| 19 | +Selecting patch(es) to review |
| 20 | +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
| 21 | +If you are looking for a patch series in need of review, start by checking |
| 22 | +latest "What's cooking in git.git" email |
| 23 | +(https://lore.kernel.org/git/ [email protected]/[example]). The "What's |
| 24 | +cooking" emails & replies can be found using the query `s:"What's cooking"` on |
| 25 | +the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive]; |
| 26 | +alternatively, you can find the contents of the "What's cooking" email tracked |
| 27 | +in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with "Needs |
| 28 | +review" and those in the "[New Topics]" section are typically those that would |
| 29 | +benefit the most from additional review. |
| 30 | + |
| 31 | +Patches can also be searched manually in the mailing list archive using a query |
| 32 | +like `s:"PATCH" -s:"Re:"`. You can browse these results for topics relevant to |
| 33 | +your expertise or interest. |
| 34 | + |
| 35 | +If you've already contributed to Git, you may also be CC'd in another |
| 36 | +contributor's patch series. These are topics where the author feels that your |
| 37 | +attention is warranted. This may be because their patch changes something you |
| 38 | +wrote previously (making you a good judge of whether the new approach does or |
| 39 | +doesn't work), or because you have the expertise to provide an exceptionally |
| 40 | +helpful review. There is no requirement to review these patches but, in the |
| 41 | +spirit of open source collaboration, you should strongly consider doing so. |
| 42 | + |
| 43 | +Reviewing patches |
| 44 | +~~~~~~~~~~~~~~~~~ |
| 45 | +While every contributor takes their own approach to reviewing patches, here are |
| 46 | +some general pieces of advice to make your reviews as clear and helpful as |
| 47 | +possible. The advice is broken into two rough categories: high-level reviewing |
| 48 | +guidance, and concrete tips for interacting with patches on the mailing list. |
| 49 | + |
| 50 | +==== High-level guidance |
| 51 | +- Remember to review the content of commit messages for correctness and clarity, |
| 52 | + in addition to the code change in the patch's diff. The commit message of a |
| 53 | + patch should accurately and fully explain the code change being made in the |
| 54 | + diff. |
| 55 | + |
| 56 | +- Reviewing test coverage is an important - but easy to overlook - component of |
| 57 | + reviews. A patch's changes may be covered by existing tests, or new tests may |
| 58 | + be introduced to exercise new behavior. Checking out a patch or series locally |
| 59 | + allows you to manually mutate lines of new & existing tests to verify expected |
| 60 | + pass/fail behavior. You can use this information to verify proper coverage or |
| 61 | + to suggest additional tests the author could add. |
| 62 | + |
| 63 | +- When providing a recommendation, be as clear as possible about whether you |
| 64 | + consider it "blocking" (the code would be broken or otherwise made worse if an |
| 65 | + issue isn't fixed) or "non-blocking" (the patch could be made better by taking |
| 66 | + the recommendation, but acceptance of the series does not require it). |
| 67 | + Non-blocking recommendations can be particularly ambiguous when they are |
| 68 | + related to - but outside the scope of - a series ("nice-to-have"s), or when |
| 69 | + they represent only stylistic differences between the author and reviewer. |
| 70 | + |
| 71 | +- When commenting on an issue, try to include suggestions for how the author |
| 72 | + could fix it. This not only helps the author to understand and fix the issue, |
| 73 | + it also deepens and improves your understanding of the topic. |
| 74 | + |
| 75 | +- Reviews do not need to exclusively point out problems. Feel free to "think out |
| 76 | + loud" in your review: describe how you read & understood a complex section of |
| 77 | + a patch, ask a question about something that confused you, point out something |
| 78 | + you found exceptionally well-written, etc. In particular, uplifting feedback |
| 79 | + goes a long way towards encouraging contributors to participate more actively |
| 80 | + in the Git community. |
| 81 | + |
| 82 | +==== Performing your review |
| 83 | +- Provide your review comments per-patch in a plaintext "Reply-All" email to the |
| 84 | + relevant patch. Comments should be made inline, immediately below the relevant |
| 85 | + section(s). |
| 86 | + |
| 87 | +- You may find that the limited context provided in the patch diff is sometimes |
| 88 | + insufficient for a thorough review. In such cases, you can review patches in |
| 89 | + your local tree by either applying patches with linkgit:git-am[1] or checking |
| 90 | + out the associated branch from https://github.com/gitster/git once the series |
| 91 | + is tracked there. |
| 92 | + |
| 93 | +- Large, complicated patch diffs are sometimes unavoidable, such as when they |
| 94 | + refactor existing code. If you find such a patch difficult to parse, try |
| 95 | + reviewing the diff produced with the `--color-moved` and/or |
| 96 | + `--ignore-space-change` options. |
| 97 | + |
| 98 | +- If a patch is long, you are encouraged to delete parts of it that are |
| 99 | + unrelated to your review from the email reply. Make sure to leave enough |
| 100 | + context for readers to understand your comments! |
| 101 | + |
| 102 | +- If you cannot complete a full review of a series all at once, consider letting |
| 103 | + the author know (on- or off-list) if/when you plan to review the rest of the |
| 104 | + series. |
| 105 | + |
| 106 | +Completing a review |
| 107 | +~~~~~~~~~~~~~~~~~~~ |
| 108 | +Once each patch of a series is reviewed, the author (and/or other contributors) |
| 109 | +may discuss the review(s). This may result in no changes being applied, or the |
| 110 | +author will send a new version of their patch(es). |
| 111 | + |
| 112 | +After a series is rerolled in response to your or others' review, make sure to |
| 113 | +re-review the updates. If you are happy with the state of the patch series, |
| 114 | +explicitly indicate your approval (typically with a reply to the latest |
| 115 | +version's cover letter). Optionally, you can let the author know that they can |
| 116 | +add a "Reviewed-by: <you>" trailer if they resubmit the reviewed patch verbatim |
| 117 | +in a later iteration of the series. |
| 118 | + |
| 119 | +Finally, subsequent "What's cooking" emails may explicitly ask whether a |
| 120 | +reviewed topic is ready for merging to the `next` branch (typically phrased |
| 121 | +"Will merge to \'next\'?"). You can help the maintainer and author by responding |
| 122 | +with a short description of the state of your (and others', if applicable) |
| 123 | +review, including the links to the relevant thread(s). |
| 124 | + |
| 125 | +Terminology |
| 126 | +----------- |
| 127 | +nit: :: |
| 128 | + Denotes a small issue that should be fixed, such as a typographical error |
| 129 | + or mis-alignment of conditions in an `if()` statement. |
| 130 | + |
| 131 | +aside: :: |
| 132 | +optional: :: |
| 133 | +non-blocking: :: |
| 134 | + Indicates to the reader that the following comment should not block the |
| 135 | + acceptance of the patch or series. These are typically recommendations |
| 136 | + related to code organization & style, or musings about topics related to |
| 137 | + the patch in question, but beyond its scope. |
| 138 | + |
| 139 | +s/<before>/<after>/:: |
| 140 | + Shorthand for "you wrote <before>, but I think you meant <after>," usually |
| 141 | + for misspellings or other typographical errors. The syntax is a reference |
| 142 | + to "substitute" command commonly found in Unix tools such as `ed`, `sed`, |
| 143 | + `vim`, and `perl`. |
| 144 | + |
| 145 | +cover letter:: |
| 146 | + The "Patch 0" of a multi-patch series. This email describes the |
| 147 | + high-level intent and structure of the patch series to readers on the |
| 148 | + Git mailing list. It is also where the changelog notes and range-diff of |
| 149 | + subsequent versions are provided by the author. |
| 150 | ++ |
| 151 | +On single-patch submissions, cover letter content is typically not sent as a |
| 152 | +separate email. Instead, it is inserted between the end of the patch's commit |
| 153 | +message (after the `---`) and the beginning of the diff. |
| 154 | + |
| 155 | +#leftoverbits:: |
| 156 | + Used by either an author or a reviewer to describe features or suggested |
| 157 | + changes that are out-of-scope of a given patch or series, but are relevant |
| 158 | + to the topic for the sake of discussion. |
| 159 | + |
| 160 | +See Also |
| 161 | +-------- |
| 162 | +link:MyFirstContribution.html[MyFirstContribution] |
0 commit comments