]>
Commit | Line | Data |
---|---|---|
e01b8519 VD |
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 | |
0a4f051f | 22 | the latest "What's cooking in git.git" email |
e01b8519 VD |
23 | (https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[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 | |
f22fdf33 | 129 | or misalignment of conditions in an `if()` statement. |
e01b8519 VD |
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] |