Server IP : 184.154.167.98 / Your IP : 18.118.128.17 Web Server : Apache System : Linux pink.dnsnetservice.com 4.18.0-553.22.1.lve.1.el8.x86_64 #1 SMP Tue Oct 8 15:52:54 UTC 2024 x86_64 User : puertode ( 1767) PHP Version : 7.2.34 Disable Function : NONE MySQL : OFF | cURL : ON | WGET : ON | Perl : ON | Python : ON | Sudo : ON | Pkexec : ON Directory : /usr/share/doc/git/ |
Upload File : |
<?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 8.6.10" /> <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 overriden 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"> /*<![CDATA[*/ var asciidoc = { // Namespace. ///////////////////////////////////////////////////////////////////// // Table Of Contents generator ///////////////////////////////////////////////////////////////////// /* Author: Mihai Bazon, September 2002 * http://students.infoiasi.ro/~mishoo * * Table Of Content generator * Version: 0.4 * * Feel free to use this script under the terms of the GNU General Public * License, as long as you do not remove or alter this notice. */ /* modified by Troy D. Hanson, September 2006. License: GPL */ /* modified by Stuart Rackham, 2006, 2009. License: GPL */ // toclevels = 1..4. toc: function (toclevels) { function getText(el) { var text = ""; for (var i = el.firstChild; i != null; i = i.nextSibling) { if (i.nodeType == 3 /* Node.TEXT_NODE */) // IE doesn't speak constants. text += i.data; else if (i.firstChild != null) text += getText(i); } return text; } function TocEntry(el, text, toclevel) { this.element = el; this.text = text; this.toclevel = toclevel; } function tocEntries(el, toclevels) { var result = new Array; var re = new RegExp('[hH]([1-'+(toclevels+1)+'])'); // 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"></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 2024-05-31 00:41:06 UTC </div> </div> </body> </html>