freshlybakedca.ke
Git fork
1Reviewing Patches in the Git Project
2====================================
3
4Introduction
5------------
6The Git development community is a widely distributed, diverse, ever-changing
7group of individuals. Asynchronous communication via the Git mailing list poses
8unique challenges when reviewing or discussing patches. This document contains
9some guiding principles and helpful tools you can use to make your reviews both
10more efficient for yourself and more effective for other contributors.
11
12Note that none of the recommendations here are binding or in any way a
13requirement of participation in the Git community. They are provided as a
14resource to supplement your skills as a contributor.
15
16Principles
17----------
18
19Selecting patch(es) to review
20~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
21If you are looking for a patch series in need of review, start by checking
22the latest "What's cooking in git.git" email
23(https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/[example]). The "What's
24cooking" emails & replies can be found using the query `s:"What's cooking"` on
25the https://lore.kernel.org/git/[`lore.kernel.org` mailing list archive];
26alternatively, you can find the contents of the "What's cooking" email tracked
27in `whats-cooking.txt` on the `todo` branch of Git. Topics tagged with "Needs
28review" and those in the "[New Topics]" section are typically those that would
29benefit the most from additional review.
30
31Patches can also be searched manually in the mailing list archive using a query
32like `s:"PATCH" -s:"Re:"`. You can browse these results for topics relevant to
33your expertise or interest.
34
35If you've already contributed to Git, you may also be CC'd in another
36contributor's patch series. These are topics where the author feels that your
37attention is warranted. This may be because their patch changes something you
38wrote previously (making you a good judge of whether the new approach does or
39doesn't work), or because you have the expertise to provide an exceptionally
40helpful review. There is no requirement to review these patches but, in the
41spirit of open source collaboration, you should strongly consider doing so.
42
43Reviewing patches
44~~~~~~~~~~~~~~~~~
45While every contributor takes their own approach to reviewing patches, here are
46some general pieces of advice to make your reviews as clear and helpful as
47possible. The advice is broken into two rough categories: high-level reviewing
48guidance, 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. Positive
76 reviews indicate that it is not only the original author of the
77 patches who care about the issue the patches address, and are
78 highly encouraged.
79
80- Do not hesitate to give positive reviews on a series from your
81 work colleague. If your positive review is written well, it will
82 not make you look as if you two are representing corporate
83 interest on a series that is otherwise uninteresting to other
84 community members and shoving it down their throat.
85
86- Write a positive review in such a way that others can understand
87 why you support the goal, the approach, and the implementation the
88 patches took. Make sure to demonstrate that you did thoroughly read
89 the series and understood problem area well enough to be able to
90 say that the patches are written well. Feel free to "think out
91 loud" in your review: describe how you read & understood a complex section of
92 a patch, ask a question about something that confused you, point out something
93 you found exceptionally well-written, etc.
94
95- In particular, uplifting feedback goes a long way towards
96 encouraging contributors to participate more actively in the Git
97 community.
98
99==== Performing your review
100- Provide your review comments per-patch in a plaintext "Reply-All" email to the
101 relevant patch. Comments should be made inline, immediately below the relevant
102 section(s).
103
104- You may find that the limited context provided in the patch diff is sometimes
105 insufficient for a thorough review. In such cases, you can review patches in
106 your local tree by either applying patches with linkgit:git-am[1] or checking
107 out the associated branch from https://github.com/gitster/git once the series
108 is tracked there.
109
110- Large, complicated patch diffs are sometimes unavoidable, such as when they
111 refactor existing code. If you find such a patch difficult to parse, try
112 reviewing the diff produced with the `--color-moved` and/or
113 `--ignore-space-change` options.
114
115- If a patch is long, you are encouraged to delete parts of it that are
116 unrelated to your review from the email reply. Make sure to leave enough
117 context for readers to understand your comments!
118
119- If you cannot complete a full review of a series all at once, consider letting
120 the author know (on- or off-list) if/when you plan to review the rest of the
121 series.
122
123Completing a review
124~~~~~~~~~~~~~~~~~~~
125Once each patch of a series is reviewed, the author (and/or other contributors)
126may discuss the review(s). This may result in no changes being applied, or the
127author will send a new version of their patch(es).
128
129After a series is rerolled in response to your or others' review, make sure to
130re-review the updates. If you are happy with the state of the patch series,
131explicitly indicate your approval (typically with a reply to the latest
132version's cover letter). Optionally, you can let the author know that they can
133add a "Reviewed-by: <you>" trailer if they resubmit the reviewed patch verbatim
134in a later iteration of the series.
135
136Finally, subsequent "What's cooking" emails may explicitly ask whether a
137reviewed topic is ready for merging to the `next` branch (typically phrased
138"Will merge to \'next\'?"). You can help the maintainer and author by responding
139with a short description of the state of your (and others', if applicable)
140review, including the links to the relevant thread(s).
141
142Terminology
143-----------
144nit: ::
145 Denotes a small issue that should be fixed, such as a typographical error
146 or misalignment of conditions in an `if()` statement.
147
148aside: ::
149optional: ::
150non-blocking: ::
151 Indicates to the reader that the following comment should not block the
152 acceptance of the patch or series. These are typically recommendations
153 related to code organization & style, or musings about topics related to
154 the patch in question, but beyond its scope.
155
156s/<before>/<after>/::
157 Shorthand for "you wrote <before>, but I think you meant <after>," usually
158 for misspellings or other typographical errors. The syntax is a reference
159 to "substitute" command commonly found in Unix tools such as `ed`, `sed`,
160 `vim`, and `perl`.
161
162cover letter::
163 The "Patch 0" of a multi-patch series. This email describes the
164 high-level intent and structure of the patch series to readers on the
165 Git mailing list. It is also where the changelog notes and range-diff of
166 subsequent versions are provided by the author.
167+
168On single-patch submissions, cover letter content is typically not sent as a
169separate email. Instead, it is inserted between the end of the patch's commit
170message (after the `---`) and the beginning of the diff.
171
172#leftoverbits::
173 Used by either an author or a reviewer to describe features or suggested
174 changes that are out-of-scope of a given patch or series, but are relevant
175 to the topic for the sake of discussion.
176
177See Also
178--------
179link:MyFirstContribution.html[MyFirstContribution]