| <?xml version="1.0" encoding="UTF-8"?> | |
| <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" | |
| "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd"> | |
| <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en"> | |
| <head> | |
| <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=UTF-8" /> | |
| <meta name="generator" content="AsciiDoc 10.2.0" /> | |
| <title>Reviewing Patches in the Git Project</title> | |
| <style type="text/css"> | |
| /* Shared CSS for AsciiDoc xhtml11 and html5 backends */ | |
| /* Default font. */ | |
| body { | |
| font-family: Georgia,serif; | |
| } | |
| /* Title font. */ | |
| h1, h2, h3, h4, h5, h6, | |
| div.title, caption.title, | |
| thead, p.table.header, | |
| #toctitle, | |
| #author, #revnumber, #revdate, #revremark, | |
| #footer { | |
| font-family: Arial,Helvetica,sans-serif; | |
| } | |
| body { | |
| margin: 1em 5% 1em 5%; | |
| } | |
| a { | |
| color: blue; | |
| text-decoration: underline; | |
| } | |
| a:visited { | |
| color: fuchsia; | |
| } | |
| em { | |
| font-style: italic; | |
| color: navy; | |
| } | |
| strong { | |
| font-weight: bold; | |
| color: #083194; | |
| } | |
| h1, h2, h3, h4, h5, h6 { | |
| color: #527bbd; | |
| margin-top: 1.2em; | |
| margin-bottom: 0.5em; | |
| line-height: 1.3; | |
| } | |
| h1, h2, h3 { | |
| border-bottom: 2px solid silver; | |
| } | |
| h2 { | |
| padding-top: 0.5em; | |
| } | |
| h3 { | |
| float: left; | |
| } | |
| h3 + * { | |
| clear: left; | |
| } | |
| h5 { | |
| font-size: 1.0em; | |
| } | |
| div.sectionbody { | |
| margin-left: 0; | |
| } | |
| hr { | |
| border: 1px solid silver; | |
| } | |
| p { | |
| margin-top: 0.5em; | |
| margin-bottom: 0.5em; | |
| } | |
| ul, ol, li > p { | |
| margin-top: 0; | |
| } | |
| ul > li { color: #aaa; } | |
| ul > li > * { color: black; } | |
| .monospaced, code, pre { | |
| font-family: "Courier New", Courier, monospace; | |
| font-size: inherit; | |
| color: navy; | |
| padding: 0; | |
| margin: 0; | |
| } | |
| pre { | |
| white-space: pre-wrap; | |
| } | |
| #author { | |
| color: #527bbd; | |
| font-weight: bold; | |
| font-size: 1.1em; | |
| } | |
| #email { | |
| } | |
| #revnumber, #revdate, #revremark { | |
| } | |
| #footer { | |
| font-size: small; | |
| border-top: 2px solid silver; | |
| padding-top: 0.5em; | |
| margin-top: 4.0em; | |
| } | |
| #footer-text { | |
| float: left; | |
| padding-bottom: 0.5em; | |
| } | |
| #footer-badges { | |
| float: right; | |
| padding-bottom: 0.5em; | |
| } | |
| #preamble { | |
| margin-top: 1.5em; | |
| margin-bottom: 1.5em; | |
| } | |
| div.imageblock, div.exampleblock, div.verseblock, | |
| div.quoteblock, div.literalblock, div.listingblock, div.sidebarblock, | |
| div.admonitionblock { | |
| margin-top: 1.0em; | |
| margin-bottom: 1.5em; | |
| } | |
| div.admonitionblock { | |
| margin-top: 2.0em; | |
| margin-bottom: 2.0em; | |
| margin-right: 10%; | |
| color: #606060; | |
| } | |
| div.content { /* Block element content. */ | |
| padding: 0; | |
| } | |
| /* Block element titles. */ | |
| div.title, caption.title { | |
| color: #527bbd; | |
| font-weight: bold; | |
| text-align: left; | |
| margin-top: 1.0em; | |
| margin-bottom: 0.5em; | |
| } | |
| div.title + * { | |
| margin-top: 0; | |
| } | |
| td div.title:first-child { | |
| margin-top: 0.0em; | |
| } | |
| div.content div.title:first-child { | |
| margin-top: 0.0em; | |
| } | |
| div.content + div.title { | |
| margin-top: 0.0em; | |
| } | |
| div.sidebarblock > div.content { | |
| background: #ffffee; | |
| border: 1px solid #dddddd; | |
| border-left: 4px solid #f0f0f0; | |
| padding: 0.5em; | |
| } | |
| div.listingblock > div.content { | |
| border: 1px solid #dddddd; | |
| border-left: 5px solid #f0f0f0; | |
| background: #f8f8f8; | |
| padding: 0.5em; | |
| } | |
| div.quoteblock, div.verseblock { | |
| padding-left: 1.0em; | |
| margin-left: 1.0em; | |
| margin-right: 10%; | |
| border-left: 5px solid #f0f0f0; | |
| color: #888; | |
| } | |
| div.quoteblock > div.attribution { | |
| padding-top: 0.5em; | |
| text-align: right; | |
| } | |
| div.verseblock > pre.content { | |
| font-family: inherit; | |
| font-size: inherit; | |
| } | |
| div.verseblock > div.attribution { | |
| padding-top: 0.75em; | |
| text-align: left; | |
| } | |
| /* DEPRECATED: Pre version 8.2.7 verse style literal block. */ | |
| div.verseblock + div.attribution { | |
| text-align: left; | |
| } | |
| div.admonitionblock .icon { | |
| vertical-align: top; | |
| font-size: 1.1em; | |
| font-weight: bold; | |
| text-decoration: underline; | |
| color: #527bbd; | |
| padding-right: 0.5em; | |
| } | |
| div.admonitionblock td.content { | |
| padding-left: 0.5em; | |
| border-left: 3px solid #dddddd; | |
| } | |
| div.exampleblock > div.content { | |
| border-left: 3px solid #dddddd; | |
| padding-left: 0.5em; | |
| } | |
| div.imageblock div.content { padding-left: 0; } | |
| span.image img { border-style: none; vertical-align: text-bottom; } | |
| a.image:visited { color: white; } | |
| dl { | |
| margin-top: 0.8em; | |
| margin-bottom: 0.8em; | |
| } | |
| dt { | |
| margin-top: 0.5em; | |
| margin-bottom: 0; | |
| font-style: normal; | |
| color: navy; | |
| } | |
| dd > *:first-child { | |
| margin-top: 0.1em; | |
| } | |
| ul, ol { | |
| list-style-position: outside; | |
| } | |
| ol.arabic { | |
| list-style-type: decimal; | |
| } | |
| ol.loweralpha { | |
| list-style-type: lower-alpha; | |
| } | |
| ol.upperalpha { | |
| list-style-type: upper-alpha; | |
| } | |
| ol.lowerroman { | |
| list-style-type: lower-roman; | |
| } | |
| ol.upperroman { | |
| list-style-type: upper-roman; | |
| } | |
| div.compact ul, div.compact ol, | |
| div.compact p, div.compact p, | |
| div.compact div, div.compact div { | |
| margin-top: 0.1em; | |
| margin-bottom: 0.1em; | |
| } | |
| tfoot { | |
| font-weight: bold; | |
| } | |
| td > div.verse { | |
| white-space: pre; | |
| } | |
| div.hdlist { | |
| margin-top: 0.8em; | |
| margin-bottom: 0.8em; | |
| } | |
| div.hdlist tr { | |
| padding-bottom: 15px; | |
| } | |
| dt.hdlist1.strong, td.hdlist1.strong { | |
| font-weight: bold; | |
| } | |
| td.hdlist1 { | |
| vertical-align: top; | |
| font-style: normal; | |
| padding-right: 0.8em; | |
| color: navy; | |
| } | |
| td.hdlist2 { | |
| vertical-align: top; | |
| } | |
| div.hdlist.compact tr { | |
| margin: 0; | |
| padding-bottom: 0; | |
| } | |
| .comment { | |
| background: yellow; | |
| } | |
| .footnote, .footnoteref { | |
| font-size: 0.8em; | |
| } | |
| span.footnote, span.footnoteref { | |
| vertical-align: super; | |
| } | |
| #footnotes { | |
| margin: 20px 0 20px 0; | |
| padding: 7px 0 0 0; | |
| } | |
| #footnotes div.footnote { | |
| margin: 0 0 5px 0; | |
| } | |
| #footnotes hr { | |
| border: none; | |
| border-top: 1px solid silver; | |
| height: 1px; | |
| text-align: left; | |
| margin-left: 0; | |
| width: 20%; | |
| min-width: 100px; | |
| } | |
| div.colist td { | |
| padding-right: 0.5em; | |
| padding-bottom: 0.3em; | |
| vertical-align: top; | |
| } | |
| div.colist td img { | |
| margin-top: 0.3em; | |
| } | |
| @media print { | |
| #footer-badges { display: none; } | |
| } | |
| #toc { | |
| margin-bottom: 2.5em; | |
| } | |
| #toctitle { | |
| color: #527bbd; | |
| font-size: 1.1em; | |
| font-weight: bold; | |
| margin-top: 1.0em; | |
| margin-bottom: 0.1em; | |
| } | |
| div.toclevel0, div.toclevel1, div.toclevel2, div.toclevel3, div.toclevel4 { | |
| margin-top: 0; | |
| margin-bottom: 0; | |
| } | |
| div.toclevel2 { | |
| margin-left: 2em; | |
| font-size: 0.9em; | |
| } | |
| div.toclevel3 { | |
| margin-left: 4em; | |
| font-size: 0.9em; | |
| } | |
| div.toclevel4 { | |
| margin-left: 6em; | |
| font-size: 0.9em; | |
| } | |
| span.aqua { color: aqua; } | |
| span.black { color: black; } | |
| span.blue { color: blue; } | |
| span.fuchsia { color: fuchsia; } | |
| span.gray { color: gray; } | |
| span.green { color: green; } | |
| span.lime { color: lime; } | |
| span.maroon { color: maroon; } | |
| span.navy { color: navy; } | |
| span.olive { color: olive; } | |
| span.purple { color: purple; } | |
| span.red { color: red; } | |
| span.silver { color: silver; } | |
| span.teal { color: teal; } | |
| span.white { color: white; } | |
| span.yellow { color: yellow; } | |
| span.aqua-background { background: aqua; } | |
| span.black-background { background: black; } | |
| span.blue-background { background: blue; } | |
| span.fuchsia-background { background: fuchsia; } | |
| span.gray-background { background: gray; } | |
| span.green-background { background: green; } | |
| span.lime-background { background: lime; } | |
| span.maroon-background { background: maroon; } | |
| span.navy-background { background: navy; } | |
| span.olive-background { background: olive; } | |
| span.purple-background { background: purple; } | |
| span.red-background { background: red; } | |
| span.silver-background { background: silver; } | |
| span.teal-background { background: teal; } | |
| span.white-background { background: white; } | |
| span.yellow-background { background: yellow; } | |
| span.big { font-size: 2em; } | |
| span.small { font-size: 0.6em; } | |
| span.underline { text-decoration: underline; } | |
| span.overline { text-decoration: overline; } | |
| span.line-through { text-decoration: line-through; } | |
| div.unbreakable { page-break-inside: avoid; } | |
| /* | |
| * xhtml11 specific | |
| * | |
| * */ | |
| div.tableblock { | |
| margin-top: 1.0em; | |
| margin-bottom: 1.5em; | |
| } | |
| div.tableblock > table { | |
| border: 3px solid #527bbd; | |
| } | |
| thead, p.table.header { | |
| font-weight: bold; | |
| color: #527bbd; | |
| } | |
| p.table { | |
| margin-top: 0; | |
| } | |
| /* Because the table frame attribute is overridden by CSS in most browsers. */ | |
| div.tableblock > table[frame="void"] { | |
| border-style: none; | |
| } | |
| div.tableblock > table[frame="hsides"] { | |
| border-left-style: none; | |
| border-right-style: none; | |
| } | |
| div.tableblock > table[frame="vsides"] { | |
| border-top-style: none; | |
| border-bottom-style: none; | |
| } | |
| /* | |
| * html5 specific | |
| * | |
| * */ | |
| table.tableblock { | |
| margin-top: 1.0em; | |
| margin-bottom: 1.5em; | |
| } | |
| thead, p.tableblock.header { | |
| font-weight: bold; | |
| color: #527bbd; | |
| } | |
| p.tableblock { | |
| margin-top: 0; | |
| } | |
| table.tableblock { | |
| border-width: 3px; | |
| border-spacing: 0px; | |
| border-style: solid; | |
| border-color: #527bbd; | |
| border-collapse: collapse; | |
| } | |
| th.tableblock, td.tableblock { | |
| border-width: 1px; | |
| padding: 4px; | |
| border-style: solid; | |
| border-color: #527bbd; | |
| } | |
| table.tableblock.frame-topbot { | |
| border-left-style: hidden; | |
| border-right-style: hidden; | |
| } | |
| table.tableblock.frame-sides { | |
| border-top-style: hidden; | |
| border-bottom-style: hidden; | |
| } | |
| table.tableblock.frame-none { | |
| border-style: hidden; | |
| } | |
| th.tableblock.halign-left, td.tableblock.halign-left { | |
| text-align: left; | |
| } | |
| th.tableblock.halign-center, td.tableblock.halign-center { | |
| text-align: center; | |
| } | |
| th.tableblock.halign-right, td.tableblock.halign-right { | |
| text-align: right; | |
| } | |
| th.tableblock.valign-top, td.tableblock.valign-top { | |
| vertical-align: top; | |
| } | |
| th.tableblock.valign-middle, td.tableblock.valign-middle { | |
| vertical-align: middle; | |
| } | |
| th.tableblock.valign-bottom, td.tableblock.valign-bottom { | |
| vertical-align: bottom; | |
| } | |
| /* | |
| * manpage specific | |
| * | |
| * */ | |
| body.manpage h1 { | |
| padding-top: 0.5em; | |
| padding-bottom: 0.5em; | |
| border-top: 2px solid silver; | |
| border-bottom: 2px solid silver; | |
| } | |
| body.manpage h2 { | |
| border-style: none; | |
| } | |
| body.manpage div.sectionbody { | |
| margin-left: 3em; | |
| } | |
| @media print { | |
| body.manpage div#toc { display: none; } | |
| } | |
| </style> | |
| <script type="text/javascript"> | |
| /*<+'])'); | |
| // Function that scans the DOM tree for header elements (the DOM2 | |
| // nodeIterator API would be a better technique but not supported by all | |
| // browsers). | |
| var iterate = function (el) { | |
| for (var i = el.firstChild; i != null; i = i.nextSibling) { | |
| if (i.nodeType == 1 /* Node.ELEMENT_NODE */) { | |
| var mo = re.exec(i.tagName); | |
| if (mo && (i.getAttribute("class") || i.getAttribute("className")) != "float") { | |
| result[result.length] = new TocEntry(i, getText(i), mo[1]-1); | |
| } | |
| iterate(i); | |
| } | |
| } | |
| } | |
| iterate(el); | |
| return result; | |
| } | |
| var toc = document.getElementById("toc"); | |
| if (!toc) { | |
| return; | |
| } | |
| // Delete existing TOC entries in case we're reloading the TOC. | |
| var tocEntriesToRemove = []; | |
| var i; | |
| for (i = 0; i < toc.childNodes.length; i++) { | |
| var entry = toc.childNodes[i]; | |
| if (entry.nodeName.toLowerCase() == 'div' | |
| && entry.getAttribute("class") | |
| && entry.getAttribute("class").match(/^toclevel/)) | |
| tocEntriesToRemove.push(entry); | |
| } | |
| for (i = 0; i < tocEntriesToRemove.length; i++) { | |
| toc.removeChild(tocEntriesToRemove[i]); | |
| } | |
| // Rebuild TOC entries. | |
| var entries = tocEntries(document.getElementById("content"), toclevels); | |
| for (var i = 0; i < entries.length; ++i) { | |
| var entry = entries[i]; | |
| if (entry.element.id == "") | |
| entry.element.id = "_toc_" + i; | |
| var a = document.createElement("a"); | |
| a.href = "#" + entry.element.id; | |
| a.appendChild(document.createTextNode(entry.text)); | |
| var div = document.createElement("div"); | |
| div.appendChild(a); | |
| div.className = "toclevel" + entry.toclevel; | |
| toc.appendChild(div); | |
| } | |
| if (entries.length == 0) | |
| toc.parentNode.removeChild(toc); | |
| }, | |
| ///////////////////////////////////////////////////////////////////// | |
| // Footnotes generator | |
| ///////////////////////////////////////////////////////////////////// | |
| /* Based on footnote generation code from: | |
| * http://www.brandspankingnew.net/archive/2005/07/format_footnote.html | |
| */ | |
| footnotes: function () { | |
| // Delete existing footnote entries in case we're reloading the footnodes. | |
| var i; | |
| var noteholder = document.getElementById("footnotes"); | |
| if (!noteholder) { | |
| return; | |
| } | |
| var entriesToRemove = []; | |
| for (i = 0; i < noteholder.childNodes.length; i++) { | |
| var entry = noteholder.childNodes[i]; | |
| if (entry.nodeName.toLowerCase() == 'div' && entry.getAttribute("class") == "footnote") | |
| entriesToRemove.push(entry); | |
| } | |
| for (i = 0; i < entriesToRemove.length; i++) { | |
| noteholder.removeChild(entriesToRemove[i]); | |
| } | |
| // Rebuild footnote entries. | |
| var cont = document.getElementById("content"); | |
| var spans = cont.getElementsByTagName("span"); | |
| var refs = {}; | |
| var n = 0; | |
| for (i=0; i<spans.length; i++) { | |
| if (spans[i].className == "footnote") { | |
| n++; | |
| var note = spans[i].getAttribute("data-note"); | |
| if (!note) { | |
| // Use [\s\S] in place of . so multi-line matches work. | |
| // Because JavaScript has no s (dotall) regex flag. | |
| note = spans[i].innerHTML.match(/\s*\[([\s\S]*)]\s*/)[1]; | |
| spans[i].innerHTML = | |
| "[<a id='_footnoteref_" + n + "' href='#_footnote_" + n + | |
| "' title='View footnote' class='footnote'>" + n + "</a>]"; | |
| spans[i].setAttribute("data-note", note); | |
| } | |
| noteholder.innerHTML += | |
| "<div class='footnote' id='_footnote_" + n + "'>" + | |
| "<a href='#_footnoteref_" + n + "' title='Return to text'>" + | |
| n + "</a>. " + note + "</div>"; | |
| var id =spans[i].getAttribute("id"); | |
| if (id != null) refs["#"+id] = n; | |
| } | |
| } | |
| if (n == 0) | |
| noteholder.parentNode.removeChild(noteholder); | |
| else { | |
| // Process footnoterefs. | |
| for (i=0; i<spans.length; i++) { | |
| if (spans[i].className == "footnoteref") { | |
| var href = spans[i].getElementsByTagName("a")[0].getAttribute("href"); | |
| href = href.match(/#.*/)[0]; // Because IE return full URL. | |
| n = refs[href]; | |
| spans[i].innerHTML = | |
| "[<a href='#_footnote_" + n + | |
| "' title='View footnote' class='footnote'>" + n + "</a>]"; | |
| } | |
| } | |
| } | |
| }, | |
| install: function(toclevels) { | |
| var timerId; | |
| function reinstall() { | |
| asciidoc.footnotes(); | |
| if (toclevels) { | |
| asciidoc.toc(toclevels); | |
| } | |
| } | |
| function reinstallAndRemoveTimer() { | |
| clearInterval(timerId); | |
| reinstall(); | |
| } | |
| timerId = setInterval(reinstall, 500); | |
| if (document.addEventListener) | |
| document.addEventListener("DOMContentLoaded", reinstallAndRemoveTimer, false); | |
| else | |
| window.onload = reinstallAndRemoveTimer; | |
| } | |
| } | |
| asciidoc.install(); | |
| /*]]>*/ | |
| </script> | |
| </head> | |
| <body class="article"> | |
| <div id="header"> | |
| <h1>Reviewing Patches in the Git Project</h1> | |
| <span id="revdate">2024-04-25</span> | |
| </div> | |
| <div id="content"> | |
| <div class="sect1"> | |
| <h2 id="_introduction">Introduction</h2> | |
| <div class="sectionbody"> | |
| <div class="paragraph"><p>The Git development community is a widely distributed, diverse, ever-changing | |
| group of individuals. Asynchronous communication via the Git mailing list poses | |
| unique challenges when reviewing or discussing patches. This document contains | |
| some guiding principles and helpful tools you can use to make your reviews both | |
| more efficient for yourself and more effective for other contributors.</p></div> | |
| <div class="paragraph"><p>Note that none of the recommendations here are binding or in any way a | |
| requirement of participation in the Git community. They are provided as a | |
| resource to supplement your skills as a contributor.</p></div> | |
| </div> | |
| </div> | |
| <div class="sect1"> | |
| <h2 id="_principles">Principles</h2> | |
| <div class="sectionbody"> | |
| <div class="sect2"> | |
| <h3 id="_selecting_patch_es_to_review">Selecting patch(es) to review</h3> | |
| <div class="paragraph"><p>If you are looking for a patch series in need of review, start by checking | |
| the latest "What’s cooking in git.git" email | |
| (<a href="https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/">example</a>). The "What’s | |
| cooking" emails & replies can be found using the query <code>s:"What's cooking"</code> on | |
| the <a href="https://lore.kernel.org/git/"><code>lore.kernel.org</code> mailing list archive</a>; | |
| alternatively, you can find the contents of the "What’s cooking" email tracked | |
| in <code>whats-cooking.txt</code> on the <code>todo</code> branch of Git. Topics tagged with "Needs | |
| review" and those in the "[New Topics]" section are typically those that would | |
| benefit the most from additional review.</p></div> | |
| <div class="paragraph"><p>Patches can also be searched manually in the mailing list archive using a query | |
| like <code>s:"PATCH" -s:"Re:"</code>. You can browse these results for topics relevant to | |
| your expertise or interest.</p></div> | |
| <div class="paragraph"><p>If you’ve already contributed to Git, you may also be CC’d in another | |
| contributor’s patch series. These are topics where the author feels that your | |
| attention is warranted. This may be because their patch changes something you | |
| wrote previously (making you a good judge of whether the new approach does or | |
| doesn’t work), or because you have the expertise to provide an exceptionally | |
| helpful review. There is no requirement to review these patches but, in the | |
| spirit of open source collaboration, you should strongly consider doing so.</p></div> | |
| </div> | |
| <div class="sect2"> | |
| <h3 id="_reviewing_patches">Reviewing patches</h3> | |
| <div class="paragraph"><p>While every contributor takes their own approach to reviewing patches, here are | |
| some general pieces of advice to make your reviews as clear and helpful as | |
| possible. The advice is broken into two rough categories: high-level reviewing | |
| guidance, and concrete tips for interacting with patches on the mailing list.</p></div> | |
| <div class="sect3"> | |
| <h4 id="_high_level_guidance">High-level guidance</h4> | |
| <div class="ulist"><ul> | |
| <li> | |
| <p> | |
| Remember to review the content of commit messages for correctness and clarity, | |
| in addition to the code change in the patch’s diff. The commit message of a | |
| patch should accurately and fully explain the code change being made in the | |
| diff. | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| Reviewing test coverage is an important - but easy to overlook - component of | |
| reviews. A patch’s changes may be covered by existing tests, or new tests may | |
| be introduced to exercise new behavior. Checking out a patch or series locally | |
| allows you to manually mutate lines of new & existing tests to verify expected | |
| pass/fail behavior. You can use this information to verify proper coverage or | |
| to suggest additional tests the author could add. | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| When providing a recommendation, be as clear as possible about whether you | |
| consider it "blocking" (the code would be broken or otherwise made worse if an | |
| issue isn’t fixed) or "non-blocking" (the patch could be made better by taking | |
| the recommendation, but acceptance of the series does not require it). | |
| Non-blocking recommendations can be particularly ambiguous when they are | |
| related to - but outside the scope of - a series ("nice-to-have"s), or when | |
| they represent only stylistic differences between the author and reviewer. | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| When commenting on an issue, try to include suggestions for how the author | |
| could fix it. This not only helps the author to understand and fix the issue, | |
| it also deepens and improves your understanding of the topic. | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| Reviews do not need to exclusively point out problems. Feel free to "think out | |
| loud" in your review: describe how you read & understood a complex section of | |
| a patch, ask a question about something that confused you, point out something | |
| you found exceptionally well-written, etc. In particular, uplifting feedback | |
| goes a long way towards encouraging contributors to participate more actively | |
| in the Git community. | |
| </p> | |
| </li> | |
| </ul></div> | |
| </div> | |
| <div class="sect3"> | |
| <h4 id="_performing_your_review">Performing your review</h4> | |
| <div class="ulist"><ul> | |
| <li> | |
| <p> | |
| Provide your review comments per-patch in a plaintext "Reply-All" email to the | |
| relevant patch. Comments should be made inline, immediately below the relevant | |
| section(s). | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| You may find that the limited context provided in the patch diff is sometimes | |
| insufficient for a thorough review. In such cases, you can review patches in | |
| your local tree by either applying patches with <a href="git-am.html">git-am(1)</a> or checking | |
| out the associated branch from <a href="https://github.com/gitster/git">https://github.com/gitster/git</a> once the series | |
| is tracked there. | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| Large, complicated patch diffs are sometimes unavoidable, such as when they | |
| refactor existing code. If you find such a patch difficult to parse, try | |
| reviewing the diff produced with the <code>--color-moved</code> and/or | |
| <code>--ignore-space-change</code> options. | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| If a patch is long, you are encouraged to delete parts of it that are | |
| unrelated to your review from the email reply. Make sure to leave enough | |
| context for readers to understand your comments! | |
| </p> | |
| </li> | |
| <li> | |
| <p> | |
| If you cannot complete a full review of a series all at once, consider letting | |
| the author know (on- or off-list) if/when you plan to review the rest of the | |
| series. | |
| </p> | |
| </li> | |
| </ul></div> | |
| </div> | |
| </div> | |
| <div class="sect2"> | |
| <h3 id="_completing_a_review">Completing a review</h3> | |
| <div class="paragraph"><p>Once each patch of a series is reviewed, the author (and/or other contributors) | |
| may discuss the review(s). This may result in no changes being applied, or the | |
| author will send a new version of their patch(es).</p></div> | |
| <div class="paragraph"><p>After a series is rerolled in response to your or others' review, make sure to | |
| re-review the updates. If you are happy with the state of the patch series, | |
| explicitly indicate your approval (typically with a reply to the latest | |
| version’s cover letter). Optionally, you can let the author know that they can | |
| add a "Reviewed-by: <you>" trailer if they resubmit the reviewed patch verbatim | |
| in a later iteration of the series.</p></div> | |
| <div class="paragraph"><p>Finally, subsequent "What’s cooking" emails may explicitly ask whether a | |
| reviewed topic is ready for merging to the ‘next` branch (typically phrased | |
| "Will merge to 'next’?"). You can help the maintainer and author by responding | |
| with a short description of the state of your (and others', if applicable) | |
| review, including the links to the relevant thread(s).</p></div> | |
| </div> | |
| </div> | |
| </div> | |
| <div class="sect1"> | |
| <h2 id="_terminology">Terminology</h2> | |
| <div class="sectionbody"> | |
| <div class="dlist"><dl> | |
| <dt class="hdlist1"> | |
| nit: | |
| </dt> | |
| <dd> | |
| <p> | |
| Denotes a small issue that should be fixed, such as a typographical error | |
| or misalignment of conditions in an <code>if()</code> statement. | |
| </p> | |
| </dd> | |
| <dt class="hdlist1"> | |
| aside: | |
| </dt> | |
| <dt class="hdlist1"> | |
| optional: | |
| </dt> | |
| <dt class="hdlist1"> | |
| non-blocking: | |
| </dt> | |
| <dd> | |
| <p> | |
| Indicates to the reader that the following comment should not block the | |
| acceptance of the patch or series. These are typically recommendations | |
| related to code organization & style, or musings about topics related to | |
| the patch in question, but beyond its scope. | |
| </p> | |
| </dd> | |
| <dt class="hdlist1"> | |
| s/<before>/<after>/ | |
| </dt> | |
| <dd> | |
| <p> | |
| Shorthand for "you wrote <before>, but I think you meant <after>," usually | |
| for misspellings or other typographical errors. The syntax is a reference | |
| to "substitute" command commonly found in Unix tools such as <code>ed</code>, <code>sed</code>, | |
| <code>vim</code>, and <code>perl</code>. | |
| </p> | |
| </dd> | |
| <dt class="hdlist1"> | |
| cover letter | |
| </dt> | |
| <dd> | |
| <p> | |
| The "Patch 0" of a multi-patch series. This email describes the | |
| high-level intent and structure of the patch series to readers on the | |
| Git mailing list. It is also where the changelog notes and range-diff of | |
| subsequent versions are provided by the author. | |
| </p> | |
| <div class="paragraph"><p>On single-patch submissions, cover letter content is typically not sent as a | |
| separate email. Instead, it is inserted between the end of the patch’s commit | |
| message (after the <code>---</code>) and the beginning of the diff.</p></div> | |
| </dd> | |
| <dt class="hdlist1"> | |
| #leftoverbits | |
| </dt> | |
| <dd> | |
| <p> | |
| Used by either an author or a reviewer to describe features or suggested | |
| changes that are out-of-scope of a given patch or series, but are relevant | |
| to the topic for the sake of discussion. | |
| </p> | |
| </dd> | |
| </dl></div> | |
| </div> | |
| </div> | |
| <div class="sect1"> | |
| <h2 id="_see_also">See Also</h2> | |
| <div class="sectionbody"> | |
| <div class="paragraph"><p><a href="MyFirstContribution.html">MyFirstContribution</a></p></div> | |
| </div> | |
| </div> | |
| </div> | |
| <div id="footnotes"><hr /></div> | |
| <div id="footer"> | |
| <div id="footer-text"> | |
| Last updated | |
| 2023-10-23 14:43:46 PDT | |
| </div> | |
| </div> | |
| </body> | |
| </html> |