1008 lines
27 KiB
HTML
1008 lines
27 KiB
HTML
<?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"></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:</code>"What's <code>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:</code>"PATCH" <code>-s:</code>"Re:". 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. Positive
|
|
reviews indicate that it is not only the original author of the
|
|
patches who care about the issue the patches address, and are
|
|
highly encouraged.
|
|
</p>
|
|
</li>
|
|
<li>
|
|
<p>
|
|
Do not hesitate to give positive reviews on a series from your
|
|
work colleague. If your positive review is written well, it will
|
|
not make you look as if you two are representing corporate
|
|
interest on a series that is otherwise uninteresting to other
|
|
community members and shoving it down their throat.
|
|
</p>
|
|
</li>
|
|
<li>
|
|
<p>
|
|
Write a positive review in such a way that others can understand
|
|
why you support the goal, the approach, and the implementation the
|
|
patches took. Make sure to demonstrate that you did thoroughly read
|
|
the series and understood problem area well enough to be able to
|
|
say that the patches are written well. 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.
|
|
</p>
|
|
</li>
|
|
<li>
|
|
<p>
|
|
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
|
|
2025-08-18 02:18:23 CEST
|
|
</div>
|
|
</div>
|
|
</body>
|
|
</html>
|