ReviewingGuidelines.txt

   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 the latest "What's cooking in git.git" email
  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
 129         or misalignment 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]