ReviewingGuidelines.html

   1 <?xml version="1.0" encoding="UTF-8"?>
   2 <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
   3     "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
   4 <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
   5 <head>
   6 <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=UTF-8" />
   7 <meta name="generator" content="AsciiDoc 10.2.0" />
   8 <title>Reviewing Patches in the Git Project</title>
   9 <style type="text/css">
  10 /* Shared CSS for AsciiDoc xhtml11 and html5 backends */
  11
  12 /* Default font. */
  13 body {
  14   font-family: Georgia,serif;
  15 }
  16
  17 /* Title font. */
  18 h1, h2, h3, h4, h5, h6,
  19 div.title, caption.title,
  20 thead, p.table.header,
  21 #toctitle,
  22 #author, #revnumber, #revdate, #revremark,
  23 #footer {
  24   font-family: Arial,Helvetica,sans-serif;
  25 }
  26
  27 body {
  28   margin: 1em 5% 1em 5%;
  29 }
  30
  31 a {
  32   color: blue;
  33   text-decoration: underline;
  34 }
  35 a:visited {
  36   color: fuchsia;
  37 }
  38
  39 em {
  40   font-style: italic;
  41   color: navy;
  42 }
  43
  44 strong {
  45   font-weight: bold;
  46   color: #083194;
  47 }
  48
  49 h1, h2, h3, h4, h5, h6 {
  50   color: #527bbd;
  51   margin-top: 1.2em;
  52   margin-bottom: 0.5em;
  53   line-height: 1.3;
  54 }
  55
  56 h1, h2, h3 {
  57   border-bottom: 2px solid silver;
  58 }
  59 h2 {
  60   padding-top: 0.5em;
  61 }
  62 h3 {
  63   float: left;
  64 }
  65 h3 + * {
  66   clear: left;
  67 }
  68 h5 {
  69   font-size: 1.0em;
  70 }
  71
  72 div.sectionbody {
  73   margin-left: 0;
  74 }
  75
  76 hr {
  77   border: 1px solid silver;
  78 }
  79
  80 p {
  81   margin-top: 0.5em;
  82   margin-bottom: 0.5em;
  83 }
  84
  85 ul, ol, li > p {
  86   margin-top: 0;
  87 }
  88 ul > li     { color: #aaa; }
  89 ul > li > * { color: black; }
  90
  91 .monospaced, code, pre {
  92   font-family: "Courier New", Courier, monospace;
  93   font-size: inherit;
  94   color: navy;
  95   padding: 0;
  96   margin: 0;
  97 }
  98 pre {
  99   white-space: pre-wrap;
 100 }
 101
 102 #author {
 103   color: #527bbd;
 104   font-weight: bold;
 105   font-size: 1.1em;
 106 }
 107 #email {
 108 }
 109 #revnumber, #revdate, #revremark {
 110 }
 111
 112 #footer {
 113   font-size: small;
 114   border-top: 2px solid silver;
 115   padding-top: 0.5em;
 116   margin-top: 4.0em;
 117 }
 118 #footer-text {
 119   float: left;
 120   padding-bottom: 0.5em;
 121 }
 122 #footer-badges {
 123   float: right;
 124   padding-bottom: 0.5em;
 125 }
 126
 127 #preamble {
 128   margin-top: 1.5em;
 129   margin-bottom: 1.5em;
 130 }
 131 div.imageblock, div.exampleblock, div.verseblock,
 132 div.quoteblock, div.literalblock, div.listingblock, div.sidebarblock,
 133 div.admonitionblock {
 134   margin-top: 1.0em;
 135   margin-bottom: 1.5em;
 136 }
 137 div.admonitionblock {
 138   margin-top: 2.0em;
 139   margin-bottom: 2.0em;
 140   margin-right: 10%;
 141   color: #606060;
 142 }
 143
 144 div.content { /* Block element content. */
 145   padding: 0;
 146 }
 147
 148 /* Block element titles. */
 149 div.title, caption.title {
 150   color: #527bbd;
 151   font-weight: bold;
 152   text-align: left;
 153   margin-top: 1.0em;
 154   margin-bottom: 0.5em;
 155 }
 156 div.title + * {
 157   margin-top: 0;
 158 }
 159
 160 td div.title:first-child {
 161   margin-top: 0.0em;
 162 }
 163 div.content div.title:first-child {
 164   margin-top: 0.0em;
 165 }
 166 div.content + div.title {
 167   margin-top: 0.0em;
 168 }
 169
 170 div.sidebarblock > div.content {
 171   background: #ffffee;
 172   border: 1px solid #dddddd;
 173   border-left: 4px solid #f0f0f0;
 174   padding: 0.5em;
 175 }
 176
 177 div.listingblock > div.content {
 178   border: 1px solid #dddddd;
 179   border-left: 5px solid #f0f0f0;
 180   background: #f8f8f8;
 181   padding: 0.5em;
 182 }
 183
 184 div.quoteblock, div.verseblock {
 185   padding-left: 1.0em;
 186   margin-left: 1.0em;
 187   margin-right: 10%;
 188   border-left: 5px solid #f0f0f0;
 189   color: #888;
 190 }
 191
 192 div.quoteblock > div.attribution {
 193   padding-top: 0.5em;
 194   text-align: right;
 195 }
 196
 197 div.verseblock > pre.content {
 198   font-family: inherit;
 199   font-size: inherit;
 200 }
 201 div.verseblock > div.attribution {
 202   padding-top: 0.75em;
 203   text-align: left;
 204 }
 205 /* DEPRECATED: Pre version 8.2.7 verse style literal block. */
 206 div.verseblock + div.attribution {
 207   text-align: left;
 208 }
 209
 210 div.admonitionblock .icon {
 211   vertical-align: top;
 212   font-size: 1.1em;
 213   font-weight: bold;
 214   text-decoration: underline;
 215   color: #527bbd;
 216   padding-right: 0.5em;
 217 }
 218 div.admonitionblock td.content {
 219   padding-left: 0.5em;
 220   border-left: 3px solid #dddddd;
 221 }
 222
 223 div.exampleblock > div.content {
 224   border-left: 3px solid #dddddd;
 225   padding-left: 0.5em;
 226 }
 227
 228 div.imageblock div.content { padding-left: 0; }
 229 span.image img { border-style: none; vertical-align: text-bottom; }
 230 a.image:visited { color: white; }
 231
 232 dl {
 233   margin-top: 0.8em;
 234   margin-bottom: 0.8em;
 235 }
 236 dt {
 237   margin-top: 0.5em;
 238   margin-bottom: 0;
 239   font-style: normal;
 240   color: navy;
 241 }
 242 dd > *:first-child {
 243   margin-top: 0.1em;
 244 }
 245
 246 ul, ol {
 247     list-style-position: outside;
 248 }
 249 ol.arabic {
 250   list-style-type: decimal;
 251 }
 252 ol.loweralpha {
 253   list-style-type: lower-alpha;
 254 }
 255 ol.upperalpha {
 256   list-style-type: upper-alpha;
 257 }
 258 ol.lowerroman {
 259   list-style-type: lower-roman;
 260 }
 261 ol.upperroman {
 262   list-style-type: upper-roman;
 263 }
 264
 265 div.compact ul, div.compact ol,
 266 div.compact p, div.compact p,
 267 div.compact div, div.compact div {
 268   margin-top: 0.1em;
 269   margin-bottom: 0.1em;
 270 }
 271
 272 tfoot {
 273   font-weight: bold;
 274 }
 275 td > div.verse {
 276   white-space: pre;
 277 }
 278
 279 div.hdlist {
 280   margin-top: 0.8em;
 281   margin-bottom: 0.8em;
 282 }
 283 div.hdlist tr {
 284   padding-bottom: 15px;
 285 }
 286 dt.hdlist1.strong, td.hdlist1.strong {
 287   font-weight: bold;
 288 }
 289 td.hdlist1 {
 290   vertical-align: top;
 291   font-style: normal;
 292   padding-right: 0.8em;
 293   color: navy;
 294 }
 295 td.hdlist2 {
 296   vertical-align: top;
 297 }
 298 div.hdlist.compact tr {
 299   margin: 0;
 300   padding-bottom: 0;
 301 }
 302
 303 .comment {
 304   background: yellow;
 305 }
 306
 307 .footnote, .footnoteref {
 308   font-size: 0.8em;
 309 }
 310
 311 span.footnote, span.footnoteref {
 312   vertical-align: super;
 313 }
 314
 315 #footnotes {
 316   margin: 20px 0 20px 0;
 317   padding: 7px 0 0 0;
 318 }
 319
 320 #footnotes div.footnote {
 321   margin: 0 0 5px 0;
 322 }
 323
 324 #footnotes hr {
 325   border: none;
 326   border-top: 1px solid silver;
 327   height: 1px;
 328   text-align: left;
 329   margin-left: 0;
 330   width: 20%;
 331   min-width: 100px;
 332 }
 333
 334 div.colist td {
 335   padding-right: 0.5em;
 336   padding-bottom: 0.3em;
 337   vertical-align: top;
 338 }
 339 div.colist td img {
 340   margin-top: 0.3em;
 341 }
 342
 343 @media print {
 344   #footer-badges { display: none; }
 345 }
 346
 347 #toc {
 348   margin-bottom: 2.5em;
 349 }
 350
 351 #toctitle {
 352   color: #527bbd;
 353   font-size: 1.1em;
 354   font-weight: bold;
 355   margin-top: 1.0em;
 356   margin-bottom: 0.1em;
 357 }
 358
 359 div.toclevel0, div.toclevel1, div.toclevel2, div.toclevel3, div.toclevel4 {
 360   margin-top: 0;
 361   margin-bottom: 0;
 362 }
 363 div.toclevel2 {
 364   margin-left: 2em;
 365   font-size: 0.9em;
 366 }
 367 div.toclevel3 {
 368   margin-left: 4em;
 369   font-size: 0.9em;
 370 }
 371 div.toclevel4 {
 372   margin-left: 6em;
 373   font-size: 0.9em;
 374 }
 375
 376 span.aqua { color: aqua; }
 377 span.black { color: black; }
 378 span.blue { color: blue; }
 379 span.fuchsia { color: fuchsia; }
 380 span.gray { color: gray; }
 381 span.green { color: green; }
 382 span.lime { color: lime; }
 383 span.maroon { color: maroon; }
 384 span.navy { color: navy; }
 385 span.olive { color: olive; }
 386 span.purple { color: purple; }
 387 span.red { color: red; }
 388 span.silver { color: silver; }
 389 span.teal { color: teal; }
 390 span.white { color: white; }
 391 span.yellow { color: yellow; }
 392
 393 span.aqua-background { background: aqua; }
 394 span.black-background { background: black; }
 395 span.blue-background { background: blue; }
 396 span.fuchsia-background { background: fuchsia; }
 397 span.gray-background { background: gray; }
 398 span.green-background { background: green; }
 399 span.lime-background { background: lime; }
 400 span.maroon-background { background: maroon; }
 401 span.navy-background { background: navy; }
 402 span.olive-background { background: olive; }
 403 span.purple-background { background: purple; }
 404 span.red-background { background: red; }
 405 span.silver-background { background: silver; }
 406 span.teal-background { background: teal; }
 407 span.white-background { background: white; }
 408 span.yellow-background { background: yellow; }
 409
 410 span.big { font-size: 2em; }
 411 span.small { font-size: 0.6em; }
 412
 413 span.underline { text-decoration: underline; }
 414 span.overline { text-decoration: overline; }
 415 span.line-through { text-decoration: line-through; }
 416
 417 div.unbreakable { page-break-inside: avoid; }
 418
 419
 420 /*
 421  * xhtml11 specific
 422  *
 423  * */
 424
 425 div.tableblock {
 426   margin-top: 1.0em;
 427   margin-bottom: 1.5em;
 428 }
 429 div.tableblock > table {
 430   border: 3px solid #527bbd;
 431 }
 432 thead, p.table.header {
 433   font-weight: bold;
 434   color: #527bbd;
 435 }
 436 p.table {
 437   margin-top: 0;
 438 }
 439 /* Because the table frame attribute is overridden by CSS in most browsers. */
 440 div.tableblock > table[frame="void"] {
 441   border-style: none;
 442 }
 443 div.tableblock > table[frame="hsides"] {
 444   border-left-style: none;
 445   border-right-style: none;
 446 }
 447 div.tableblock > table[frame="vsides"] {
 448   border-top-style: none;
 449   border-bottom-style: none;
 450 }
 451
 452
 453 /*
 454  * html5 specific
 455  *
 456  * */
 457
 458 table.tableblock {
 459   margin-top: 1.0em;
 460   margin-bottom: 1.5em;
 461 }
 462 thead, p.tableblock.header {
 463   font-weight: bold;
 464   color: #527bbd;
 465 }
 466 p.tableblock {
 467   margin-top: 0;
 468 }
 469 table.tableblock {
 470   border-width: 3px;
 471   border-spacing: 0px;
 472   border-style: solid;
 473   border-color: #527bbd;
 474   border-collapse: collapse;
 475 }
 476 th.tableblock, td.tableblock {
 477   border-width: 1px;
 478   padding: 4px;
 479   border-style: solid;
 480   border-color: #527bbd;
 481 }
 482
 483 table.tableblock.frame-topbot {
 484   border-left-style: hidden;
 485   border-right-style: hidden;
 486 }
 487 table.tableblock.frame-sides {
 488   border-top-style: hidden;
 489   border-bottom-style: hidden;
 490 }
 491 table.tableblock.frame-none {
 492   border-style: hidden;
 493 }
 494
 495 th.tableblock.halign-left, td.tableblock.halign-left {
 496   text-align: left;
 497 }
 498 th.tableblock.halign-center, td.tableblock.halign-center {
 499   text-align: center;
 500 }
 501 th.tableblock.halign-right, td.tableblock.halign-right {
 502   text-align: right;
 503 }
 504
 505 th.tableblock.valign-top, td.tableblock.valign-top {
 506   vertical-align: top;
 507 }
 508 th.tableblock.valign-middle, td.tableblock.valign-middle {
 509   vertical-align: middle;
 510 }
 511 th.tableblock.valign-bottom, td.tableblock.valign-bottom {
 512   vertical-align: bottom;
 513 }
 514
 515
 516 /*
 517  * manpage specific
 518  *
 519  * */
 520
 521 body.manpage h1 {
 522   padding-top: 0.5em;
 523   padding-bottom: 0.5em;
 524   border-top: 2px solid silver;
 525   border-bottom: 2px solid silver;
 526 }
 527 body.manpage h2 {
 528   border-style: none;
 529 }
 530 body.manpage div.sectionbody {
 531   margin-left: 3em;
 532 }
 533
 534 @media print {
 535   body.manpage div#toc { display: none; }
 536 }
 537
 538
 539 </style>
 540 <script type="text/javascript">
 541 /*<![CDATA[*/
 542 var asciidoc = {  // Namespace.
 543
 544 /////////////////////////////////////////////////////////////////////
 545 // Table Of Contents generator
 546 /////////////////////////////////////////////////////////////////////
 547
 548 /* Author: Mihai Bazon, September 2002
 549  * http://students.infoiasi.ro/~mishoo
 550  *
 551  * Table Of Content generator
 552  * Version: 0.4
 553  *
 554  * Feel free to use this script under the terms of the GNU General Public
 555  * License, as long as you do not remove or alter this notice.
 556  */
 557
 558  /* modified by Troy D. Hanson, September 2006. License: GPL */
 559  /* modified by Stuart Rackham, 2006, 2009. License: GPL */
 560
 561 // toclevels = 1..4.
 562 toc: function (toclevels) {
 563
 564   function getText(el) {
 565     var text = "";
 566     for (var i = el.firstChild; i != null; i = i.nextSibling) {
 567       if (i.nodeType == 3 /* Node.TEXT_NODE */) // IE doesn't speak constants.
 568         text += i.data;
 569       else if (i.firstChild != null)
 570         text += getText(i);
 571     }
 572     return text;
 573   }
 574
 575   function TocEntry(el, text, toclevel) {
 576     this.element = el;
 577     this.text = text;
 578     this.toclevel = toclevel;
 579   }
 580
 581   function tocEntries(el, toclevels) {
 582     var result = new Array;
 583     var re = new RegExp('[hH]([1-'+(toclevels+1)+'])');
 584     // Function that scans the DOM tree for header elements (the DOM2
 585     // nodeIterator API would be a better technique but not supported by all
 586     // browsers).
 587     var iterate = function (el) {
 588       for (var i = el.firstChild; i != null; i = i.nextSibling) {
 589         if (i.nodeType == 1 /* Node.ELEMENT_NODE */) {
 590           var mo = re.exec(i.tagName);
 591           if (mo && (i.getAttribute("class") || i.getAttribute("className")) != "float") {
 592             result[result.length] = new TocEntry(i, getText(i), mo[1]-1);
 593           }
 594           iterate(i);
 595         }
 596       }
 597     }
 598     iterate(el);
 599     return result;
 600   }
 601
 602   var toc = document.getElementById("toc");
 603   if (!toc) {
 604     return;
 605   }
 606
 607   // Delete existing TOC entries in case we're reloading the TOC.
 608   var tocEntriesToRemove = [];
 609   var i;
 610   for (i = 0; i < toc.childNodes.length; i++) {
 611     var entry = toc.childNodes[i];
 612     if (entry.nodeName.toLowerCase() == 'div'
 613      && entry.getAttribute("class")
 614      && entry.getAttribute("class").match(/^toclevel/))
 615       tocEntriesToRemove.push(entry);
 616   }
 617   for (i = 0; i < tocEntriesToRemove.length; i++) {
 618     toc.removeChild(tocEntriesToRemove[i]);
 619   }
 620
 621   // Rebuild TOC entries.
 622   var entries = tocEntries(document.getElementById("content"), toclevels);
 623   for (var i = 0; i < entries.length; ++i) {
 624     var entry = entries[i];
 625     if (entry.element.id == "")
 626       entry.element.id = "_toc_" + i;
 627     var a = document.createElement("a");
 628     a.href = "#" + entry.element.id;
 629     a.appendChild(document.createTextNode(entry.text));
 630     var div = document.createElement("div");
 631     div.appendChild(a);
 632     div.className = "toclevel" + entry.toclevel;
 633     toc.appendChild(div);
 634   }
 635   if (entries.length == 0)
 636     toc.parentNode.removeChild(toc);
 637 },
 638
 639
 640 /////////////////////////////////////////////////////////////////////
 641 // Footnotes generator
 642 /////////////////////////////////////////////////////////////////////
 643
 644 /* Based on footnote generation code from:
 645  * http://www.brandspankingnew.net/archive/2005/07/format_footnote.html
 646  */
 647
 648 footnotes: function () {
 649   // Delete existing footnote entries in case we're reloading the footnodes.
 650   var i;
 651   var noteholder = document.getElementById("footnotes");
 652   if (!noteholder) {
 653     return;
 654   }
 655   var entriesToRemove = [];
 656   for (i = 0; i < noteholder.childNodes.length; i++) {
 657     var entry = noteholder.childNodes[i];
 658     if (entry.nodeName.toLowerCase() == 'div' && entry.getAttribute("class") == "footnote")
 659       entriesToRemove.push(entry);
 660   }
 661   for (i = 0; i < entriesToRemove.length; i++) {
 662     noteholder.removeChild(entriesToRemove[i]);
 663   }
 664
 665   // Rebuild footnote entries.
 666   var cont = document.getElementById("content");
 667   var spans = cont.getElementsByTagName("span");
 668   var refs = {};
 669   var n = 0;
 670   for (i=0; i<spans.length; i++) {
 671     if (spans[i].className == "footnote") {
 672       n++;
 673       var note = spans[i].getAttribute("data-note");
 674       if (!note) {
 675         // Use [\s\S] in place of . so multi-line matches work.
 676         // Because JavaScript has no s (dotall) regex flag.
 677         note = spans[i].innerHTML.match(/\s*\[([\s\S]*)]\s*/)[1];
 678         spans[i].innerHTML =
 679           "[<a id='_footnoteref_" + n + "' href='#_footnote_" + n +
 680           "' title='View footnote' class='footnote'>" + n + "</a>]";
 681         spans[i].setAttribute("data-note", note);
 682       }
 683       noteholder.innerHTML +=
 684         "<div class='footnote' id='_footnote_" + n + "'>" +
 685         "<a href='#_footnoteref_" + n + "' title='Return to text'>" +
 686         n + "</a>. " + note + "</div>";
 687       var id =spans[i].getAttribute("id");
 688       if (id != null) refs["#"+id] = n;
 689     }
 690   }
 691   if (n == 0)
 692     noteholder.parentNode.removeChild(noteholder);
 693   else {
 694     // Process footnoterefs.
 695     for (i=0; i<spans.length; i++) {
 696       if (spans[i].className == "footnoteref") {
 697         var href = spans[i].getElementsByTagName("a")[0].getAttribute("href");
 698         href = href.match(/#.*/)[0];  // Because IE return full URL.
 699         n = refs[href];
 700         spans[i].innerHTML =
 701           "[<a href='#_footnote_" + n +
 702           "' title='View footnote' class='footnote'>" + n + "</a>]";
 703       }
 704     }
 705   }
 706 },
 707
 708 install: function(toclevels) {
 709   var timerId;
 710
 711   function reinstall() {
 712     asciidoc.footnotes();
 713     if (toclevels) {
 714       asciidoc.toc(toclevels);
 715     }
 716   }
 717
 718   function reinstallAndRemoveTimer() {
 719     clearInterval(timerId);
 720     reinstall();
 721   }
 722
 723   timerId = setInterval(reinstall, 500);
 724   if (document.addEventListener)
 725     document.addEventListener("DOMContentLoaded", reinstallAndRemoveTimer, false);
 726   else
 727     window.onload = reinstallAndRemoveTimer;
 728 }
 729
 730 }
 731 asciidoc.install();
 732 /*]]>*/
 733 </script>
 734 </head>
 735 <body class="article">
 736 <div id="header">
 737 <h1>Reviewing Patches in the Git Project</h1>
 738 </div>
 739 <div id="content">
 740 <div class="sect1">
 741 <h2 id="_introduction">Introduction</h2>
 742 <div class="sectionbody">
 743 <div class="paragraph"><p>The Git development community is a widely distributed, diverse, ever-changing
 744 group of individuals. Asynchronous communication via the Git mailing list poses
 745 unique challenges when reviewing or discussing patches. This document contains
 746 some guiding principles and helpful tools you can use to make your reviews both
 747 more efficient for yourself and more effective for other contributors.</p></div>
 748 <div class="paragraph"><p>Note that none of the recommendations here are binding or in any way a
 749 requirement of participation in the Git community. They are provided as a
 750 resource to supplement your skills as a contributor.</p></div>
 751 </div>
 752 </div>
 753 <div class="sect1">
 754 <h2 id="_principles">Principles</h2>
 755 <div class="sectionbody">
 756 <div class="sect2">
 757 <h3 id="_selecting_patch_es_to_review">Selecting patch(es) to review</h3>
 758 <div class="paragraph"><p>If you are looking for a patch series in need of review, start by checking
 759 latest "What&#8217;s cooking in git.git" email
 760 (<a href="https://lore.kernel.org/git/xmqqilm1yp3m.fsf@gitster.g/">example</a>). The "What&#8217;s
 761 cooking" emails &amp; replies can be found using the query <code>s:"What's cooking"</code> on
 762 the <a href="https://lore.kernel.org/git/"><code>lore.kernel.org</code> mailing list archive</a>;
 763 alternatively, you can find the contents of the "What&#8217;s cooking" email tracked
 764 in <code>whats-cooking.txt</code> on the <code>todo</code> branch of Git. Topics tagged with "Needs
 765 review" and those in the "[New Topics]" section are typically those that would
 766 benefit the most from additional review.</p></div>
 767 <div class="paragraph"><p>Patches can also be searched manually in the mailing list archive using a query
 768 like <code>s:"PATCH" -s:"Re:"</code>. You can browse these results for topics relevant to
 769 your expertise or interest.</p></div>
 770 <div class="paragraph"><p>If you&#8217;ve already contributed to Git, you may also be CC&#8217;d in another
 771 contributor&#8217;s patch series. These are topics where the author feels that your
 772 attention is warranted. This may be because their patch changes something you
 773 wrote previously (making you a good judge of whether the new approach does or
 774 doesn&#8217;t work), or because you have the expertise to provide an exceptionally
 775 helpful review. There is no requirement to review these patches but, in the
 776 spirit of open source collaboration, you should strongly consider doing so.</p></div>
 777 </div>
 778 <div class="sect2">
 779 <h3 id="_reviewing_patches">Reviewing patches</h3>
 780 <div class="paragraph"><p>While every contributor takes their own approach to reviewing patches, here are
 781 some general pieces of advice to make your reviews as clear and helpful as
 782 possible. The advice is broken into two rough categories: high-level reviewing
 783 guidance, and concrete tips for interacting with patches on the mailing list.</p></div>
 784 <div class="sect3">
 785 <h4 id="_high_level_guidance">High-level guidance</h4>
 786 <div class="ulist"><ul>
 787 <li>
 788 <p>
 789 Remember to review the content of commit messages for correctness and clarity,
 790   in addition to the code change in the patch&#8217;s diff. The commit message of a
 791   patch should accurately and fully explain the code change being made in the
 792   diff.
 793 </p>
 794 </li>
 795 <li>
 796 <p>
 797 Reviewing test coverage is an important - but easy to overlook - component of
 798   reviews. A patch&#8217;s changes may be covered by existing tests, or new tests may
 799   be introduced to exercise new behavior. Checking out a patch or series locally
 800   allows you to manually mutate lines of new &amp; existing tests to verify expected
 801   pass/fail behavior. You can use this information to verify proper coverage or
 802   to suggest additional tests the author could add.
 803 </p>
 804 </li>
 805 <li>
 806 <p>
 807 When providing a recommendation, be as clear as possible about whether you
 808   consider it "blocking" (the code would be broken or otherwise made worse if an
 809   issue isn&#8217;t fixed) or "non-blocking" (the patch could be made better by taking
 810   the recommendation, but acceptance of the series does not require it).
 811   Non-blocking recommendations can be particularly ambiguous when they are
 812   related to - but outside the scope of - a series ("nice-to-have"s), or when
 813   they represent only stylistic differences between the author and reviewer.
 814 </p>
 815 </li>
 816 <li>
 817 <p>
 818 When commenting on an issue, try to include suggestions for how the author
 819   could fix it. This not only helps the author to understand and fix the issue,
 820   it also deepens and improves your understanding of the topic.
 821 </p>
 822 </li>
 823 <li>
 824 <p>
 825 Reviews do not need to exclusively point out problems. Feel free to "think out
 826   loud" in your review: describe how you read &amp; understood a complex section of
 827   a patch, ask a question about something that confused you, point out something
 828   you found exceptionally well-written, etc. In particular, uplifting feedback
 829   goes a long way towards encouraging contributors to participate more actively
 830   in the Git community.
 831 </p>
 832 </li>
 833 </ul></div>
 834 </div>
 835 <div class="sect3">
 836 <h4 id="_performing_your_review">Performing your review</h4>
 837 <div class="ulist"><ul>
 838 <li>
 839 <p>
 840 Provide your review comments per-patch in a plaintext "Reply-All" email to the
 841   relevant patch. Comments should be made inline, immediately below the relevant
 842   section(s).
 843 </p>
 844 </li>
 845 <li>
 846 <p>
 847 You may find that the limited context provided in the patch diff is sometimes
 848   insufficient for a thorough review. In such cases, you can review patches in
 849   your local tree by either applying patches with <a href="git-am.html">git-am(1)</a> or checking
 850   out the associated branch from <a href="https://github.com/gitster/git">https://github.com/gitster/git</a> once the series
 851   is tracked there.
 852 </p>
 853 </li>
 854 <li>
 855 <p>
 856 Large, complicated patch diffs are sometimes unavoidable, such as when they
 857   refactor existing code. If you find such a patch difficult to parse, try
 858   reviewing the diff produced with the <code>--color-moved</code> and/or
 859   <code>--ignore-space-change</code> options.
 860 </p>
 861 </li>
 862 <li>
 863 <p>
 864 If a patch is long, you are encouraged to delete parts of it that are
 865   unrelated to your review from the email reply. Make sure to leave enough
 866   context for readers to understand your comments!
 867 </p>
 868 </li>
 869 <li>
 870 <p>
 871 If you cannot complete a full review of a series all at once, consider letting
 872   the author know (on- or off-list) if/when you plan to review the rest of the
 873   series.
 874 </p>
 875 </li>
 876 </ul></div>
 877 </div>
 878 </div>
 879 <div class="sect2">
 880 <h3 id="_completing_a_review">Completing a review</h3>
 881 <div class="paragraph"><p>Once each patch of a series is reviewed, the author (and/or other contributors)
 882 may discuss the review(s). This may result in no changes being applied, or the
 883 author will send a new version of their patch(es).</p></div>
 884 <div class="paragraph"><p>After a series is rerolled in response to your or others' review, make sure to
 885 re-review the updates. If you are happy with the state of the patch series,
 886 explicitly indicate your approval (typically with a reply to the latest
 887 version&#8217;s cover letter). Optionally, you can let the author know that they can
 888 add a "Reviewed-by: &lt;you&gt;" trailer if they resubmit the reviewed patch verbatim
 889 in a later iteration of the series.</p></div>
 890 <div class="paragraph"><p>Finally, subsequent "What&#8217;s cooking" emails may explicitly ask whether a
 891 reviewed topic is ready for merging to the &#8216;next` branch (typically phrased
 892 "Will merge to 'next&#8217;?"). You can help the maintainer and author by responding
 893 with a short description of the state of your (and others', if applicable)
 894 review, including the links to the relevant thread(s).</p></div>
 895 </div>
 896 </div>
 897 </div>
 898 <div class="sect1">
 899 <h2 id="_terminology">Terminology</h2>
 900 <div class="sectionbody">
 901 <div class="dlist"><dl>
 902 <dt class="hdlist1">
 903 nit:
 904 </dt>
 905 <dd>
 906 <p>
 907         Denotes a small issue that should be fixed, such as a typographical error
 908         or mis-alignment of conditions in an <code>if()</code> statement.
 909 </p>
 910 </dd>
 911 <dt class="hdlist1">
 912 aside:
 913 </dt>
 914 <dt class="hdlist1">
 915 optional:
 916 </dt>
 917 <dt class="hdlist1">
 918 non-blocking:
 919 </dt>
 920 <dd>
 921 <p>
 922         Indicates to the reader that the following comment should not block the
 923         acceptance of the patch or series. These are typically recommendations
 924         related to code organization &amp; style, or musings about topics related to
 925         the patch in question, but beyond its scope.
 926 </p>
 927 </dd>
 928 <dt class="hdlist1">
 929 s/&lt;before&gt;/&lt;after&gt;/
 930 </dt>
 931 <dd>
 932 <p>
 933         Shorthand for "you wrote &lt;before&gt;, but I think you meant &lt;after&gt;," usually
 934         for misspellings or other typographical errors. The syntax is a reference
 935         to "substitute" command commonly found in Unix tools such as <code>ed</code>, <code>sed</code>,
 936         <code>vim</code>, and <code>perl</code>.
 937 </p>
 938 </dd>
 939 <dt class="hdlist1">
 940 cover letter
 941 </dt>
 942 <dd>
 943 <p>
 944         The "Patch 0" of a multi-patch series. This email describes the
 945         high-level intent and structure of the patch series to readers on the
 946         Git mailing list. It is also where the changelog notes and range-diff of
 947         subsequent versions are provided by the author.
 948 </p>
 949 <div class="paragraph"><p>On single-patch submissions, cover letter content is typically not sent as a
 950 separate email. Instead, it is inserted between the end of the patch&#8217;s commit
 951 message (after the <code>---</code>) and the beginning of the diff.</p></div>
 952 </dd>
 953 <dt class="hdlist1">
 954 #leftoverbits
 955 </dt>
 956 <dd>
 957 <p>
 958   Used by either an author or a reviewer to describe features or suggested
 959   changes that are out-of-scope of a given patch or series, but are relevant
 960   to the topic for the sake of discussion.
 961 </p>
 962 </dd>
 963 </dl></div>
 964 </div>
 965 </div>
 966 <div class="sect1">
 967 <h2 id="_see_also">See Also</h2>
 968 <div class="sectionbody">
 969 <div class="paragraph"><p><a href="MyFirstContribution.html">MyFirstContribution</a></p></div>
 970 </div>
 971 </div>
 972 </div>
 973 <div id="footnotes"><hr /></div>
 974 <div id="footer">
 975 <div id="footer-text">
 976 Last updated
 977  2022-09-21 15:44:34 PDT
 978 </div>
 979 </div>
 980 </body>
 981 </html>