The previous lesson left Diego with his pull request open and Ana about to review it. Now comes what happens inside that channel: code review.

Almost everybody reviews code the same way: they open the changes tab in the browser, scroll, leave three comments about variable names and approve. That is not reviewing; that is skimming. A real review requires understanding the problem, understanding the solution and, very often, running the code. And for that you need Git, not a browser.

This lesson has two halves. The first is technical: which commands to use in order to review properly, including one — git range-diff — that solves a problem everybody suffers from and almost nobody knows has a solution. The second is human: what to look at, in what order, how to comment without demoralising anybody and what to do when two people cannot agree. Both matter equally.

1. What a review is really for (and what it is not)
2. The right diff for reviewing: why three dots
3. Fetching the branch and examining it with Git
4. Reviewing commit by commit
5. git range-diff: what has changed since the last review
6. Review by email: request-pull, format-patch and am
7. What to look at, in order of priority
8. How to comment well
9. Size matters: the effect of PR size
10. Approving, requesting changes and handling disagreements

Before the commands, the purpose. A code review pursues three goals, and they are not on the same level:

1. Finding defects. The obvious one. A second pair of eyes finds the edge case you did not consider, the inverted condition, the variable read before being assigned. It is real, but — and this surprises a lot of people — it is not the main benefit. Automated tests and CI (lesson 07-06) catch more defects than any human review, and they do so without getting tired.

2. Spreading knowledge. This one really is the big one. When Ana reviews Carla's change in app.js, two people end up understanding that part of the code instead of one. When Diego, who comes from outside, reads Ana's comments, he learns how things are done in this project. Review is the mechanism by which a team stops having islands of knowledge where only one person knows how something works. That "only Bruno understands the sync module" is an operational risk, and review is the cheap cure.

3. Maintaining consistency. Code where every file looks as if it were written by a different person is more expensive to maintain than uniform code, even if the individual decisions are worse. Review is where that uniformity gets negotiated.

And now, what it is not for:

The rule is devastating in its simplicity: if a machine can check it, let the machine check it. Every human comment about a whitespace character is a comment that has not been spent on the logic, and it breeds resentment on top of that. If your team argues about single or double quotes in reviews, the problem is not the quotes: it is that a formatter is missing from the pre-commit.

This idea has a direct consequence for how the process is designed: the automated tools go first, in the local hook or in CI, and the human review starts where they finish. If CI is red, do not review: there is no point spending human attention on something that does not even pass the tests.

This section is the technical heart of the lesson, and it deserves to be read slowly.

Ana wants to see "what Diego has done". It sounds trivial. It is not.

Let us recall the situation. Diego started from main at commit C3. While he was working, the team integrated two more commits into main. The graph (lesson 03-01) looks like this:

Now the two options:

The two dots, in git diff, are decorative: git diff A..B is exactly the same as git diff A B. It compares the two tips: the tree of C5 against the tree of D2.

What does that show? The sum of two different things:

• Diego's changes (D1, D2), the right way round.
• The changes of C4 and C5, the wrong way round — because from C5's point of view, Diego's branch has "undone" what Ana and Carla did.

The result is a contaminated diff. Ana sees deleted lines that she herself wrote yesterday and that Diego has never touched. It is pure noise.

The three dots in git diff mean something very specific:

git diff A...B is equivalent to git diff $(git merge-base A B) B

That is: it compares from the common ancestor. In our graph, the common ancestor of main and Diego's branch is C3. So git diff main...fix/reorder compares C3 with D2.

And that is exactly what Diego has done, and nothing else. Commits C4 and C5 do not appear, because C3 comes before them.

Check it for yourself:

The summary table, worth memorising:

Important warning: in git log the dots mean something else. This is one of Git's most tiresome traps, and we already mentioned it in lesson 06-04. In git log, A..B is "the commits in B that are not in A" (what you want for reviewing) and A...B is the symmetric difference: the commits exclusive to each side. In other words, for reviewing you use three dots in diff and two dots in log. It is not intuitive. It is how it is.

The underlying reason is that diff operates on trees (two snapshots to compare) and log operates on sets of commits. They are different operations that reuse the same notation with different semantics. An unfortunate historical decision, but it is the one we have.

And here is the important connection: when the platform shows you a pull request's diff, it is showing you the three-dot one. That is why you sometimes see a clean diff on the web and then, running git diff main branch locally, changes that are not the author's turn up. It is not a bug in the platform: it is that you were using the wrong command.

Ana starts by bringing in Diego's work with the PR reference from the previous lesson:

With that she can already work without switching branches, because every inspection command accepts references:

Two dots, because it is log. Read it as: "the branch's commits that are not in main".

This is always the first command to run. In ten seconds you know whether you are facing a five-minute review or an hour-long one, and whether the scope matches what the description promised. If the PR says "fixes the reordering" and it touches eighteen files, you already have your first comment.

The -U10 option (lesson 02-05) shows ten lines of context instead of three. In a review it is almost always what you want: the defect is usually in the interaction between what changed and what did not.

If somebody re-indented a function while modifying it, -w (equivalent to --ignore-all-space) reveals the real change under the noise. Combined with --word-diff, it is very useful for documentation changes:

--color-moved paints in a different colour the blocks that have simply been moved elsewhere, distinguishing them from those that have been written afresh. In a refactoring PR, the difference between "they moved 200 lines" and "they rewrote 200 lines" completely changes what needs reviewing.

And of course, running it. This is where git worktree (lesson 06-06) earns its keep:

Ana checks the original bug, applies the change, checks that it goes away. None of this is visible in a diff.

A well-built PR (lesson 07-01) has commits that tell a story. Take advantage of that: reviewing three commits of fifty lines each is enormously easier than reviewing one of a hundred and fifty.

• --reverse presents them from oldest to newest, which is the order in which they were thought through.
• -p adds each one's diff.

To jump between them without leaving the terminal:

Another very useful view: who has touched these lines before. If Diego's change modifies a function that Bruno wrote two weeks ago for a specific reason, it is worth knowing before approving:

And git blame with the options from module 6:

Note the main at the end: blame on the version before the change, to understand the context Diego found.

A methodological tip. Read the PR description first, then the --stat, then the commit messages, and only then the code. Arriving at the diff knowing what you expect to find turns the review into a verification of hypotheses rather than a blind read. It is several times faster and it catches more.

This section is the gem of the lesson.

The situation: Ana reviewed Diego's PR and asked for three changes. Diego applied them and, on top of that, ran git rebase upstream/main because main had moved on. He now pushes with --force-with-lease and the PR updates.

Ana comes back. And she runs into a real problem: all the commits have new hashes. The rebase rewrote them (lesson 05-01). The platform offers her "see the changes since your last review", but it often gives up with a message along the lines of "the author has force-pushed, the comparison cannot be shown". Ana does not know whether Diego only applied her three suggestions or whether he changed other things along the way. Her only apparent option is to review the whole thing again.

git range-diff solves precisely this:

git range-diff compares two series of commits and tells you, for each one, whether it has stayed the same, whether it has disappeared, whether it is new or what has changed inside it. It is a diff of diffs.

Using it requires having saved the previous version. Ana, being forward-thinking, did so before finishing her first review:

Now, after Diego's update:

The three-dot syntax is a shortcut. The full form takes three arguments and is sometimes clearer:

The output:

And now the key part: how to read that output.

In the example, Ana reads it in five seconds: the first commit has not changed (no need to reread it), the second has changed exactly in the condition she asked to have corrected, and there is a new third one that adds a test. Review done. Without range-diff, she would have reread a hundred and fifty lines.

Practical details that make it work better:

--creation-factor (60 by default) controls how alike two commits have to be for them to count as "the same commit, modified" rather than "one deleted and another new". If range-diff shows you a pile of < and > when you expected !, raise the value.

This use is just as valuable: before force-pushing after a rebase, check that you have not broken anything by accident. @{u} is the branch's upstream (lesson 04-06), that is, what is already published; HEAD is what you are about to publish. If everything comes out as = except what you meant to change, go ahead with confidence. If a modified commit turns up that you were not expecting, you have just spotted a conflict badly resolved during the rebase.

And a third use: comparing how a series of commits was applied on two different branches, for example after a mass cherry-pick to a maintenance branch (lesson 05-03).

git range-diff has existed since Git 2.19 (2018) and is still little known. It is probably the command with the best ratio between what it solves and how much it is used. If you take away a single technical thing from this lesson, let it be this one.

Before web platforms existed, and still today in projects such as the Linux kernel, Git, PostgreSQL or Buildroot, review happens on a mailing list. It is worth knowing the mechanism for three reasons: it explains where the expression "pull request" comes from, it works without depending on any company, and it turns up in important projects you might want to contribute to.

The three arguments are: the starting point (a tag or commit the recipient already has), the URL from which they can fetch the work, and the branch.

The output is text ready to paste into an email:

This is literally a "pull request": a message that says "pull from here". The platforms' buttons automate this email. Now the name makes sense.

When the recipient cannot or will not fetch from your server, the changes are sent as patches in the email itself:

Each file is a complete email: headers, author, date, commit message and the diff. Common options:

The cover letter is the equivalent of the PR description, and the -v2 is how you indicate that this is the second version of the series after the comments received. In these projects, git range-diff is routinely included in the v2 cover letter, so that reviewers can see what changed with respect to v1. It is precisely the workflow the command was designed for.

On the other side, whoever receives the patches applies them with git am (apply mailbox):

git am creates one commit per patch, preserving the original author, their date and their message. That is the crucial difference from git apply, which only applies the changes to the working directory without creating anything. If a patch does not apply cleanly:

To send them by email there is git send-email, which talks directly to an SMTP server. We shall not go into it: if you ever collaborate with a project of this kind, its documentation will explain its specific configuration.

Here the human half begins. The most common mistake of a novice reviewer is starting with the small stuff: they comment on a variable name on line 3, get bogged down, and never realise that the entire solution is wrongly framed.

Review in this order, and do not drop a level until you have closed the previous one:

• Does it really solve the problem described? Have you reproduced it?
• Edge cases: empty list, null values, very long text, odd characters?
• Race conditions, shared state, event ordering?
• Error handling, or is the catch empty?
• Are there tests? Do they test the behaviour or the implementation?
• Security: unvalidated input, user data inserted without escaping, secrets in the code? (module 8)

If something fails here, stop and comment on it. Do not carry on reviewing variable names in code that is about to disappear.

• Does it fit the existing architecture or does it introduce a new pattern without justification?
• Is it in the right place? Should it live in ui-components rather than in app.js?
• Does it duplicate something that already exists?
• Is it more complicated than it needs to be? Or too generic "just in case"?
• Does it break any interface that other code depends on?

This level is the most valuable and the most neglected, because it demands understanding the system. It is also the one that costs most to correct later: a design problem caught in review is two hours; the same problem caught six months later is a refactoring job.

• Do the names say what the things are?
• Are there comments where they are needed — the why, not the what — and an absence of obvious ones?
• Do the functions do one single thing?
• Is the complexity justified, or is there an if nested four levels deep?
• Do the commit messages explain the why? (lesson 08-01)

And here, remember section 1: if the linter can check it, do not comment on it. This level should be practically empty in a project with good tooling. If it is not, the right conclusion is not "we need to comment more", but "we need to configure the formatter".

A badly written review does more harm than no review at all. Four rules.

It is not a matter of softening things out of politeness: it is that the code-focused formulation contains the information needed to fix it, and the person-focused one does not. "It's wrong" cannot be acted on; "it fails with quotes in the title" can.

The reviewer knows which of their comments are essential and which are preferences. The author does not know unless you tell them. Marking it explicitly saves an enormous amount of friction, and there is a fairly widespread convention:

An example on Diego's PR:

That last point is not padding. Commenting on what is good is not empty courtesy: it tells the author what to keep doing, and in a team where only the bad gets commented on, review feels like a punishment.

Platforms let you propose a specific change that the author accepts with one click. If your comment is going to run to three paragraphs explaining how to rewrite four lines, write the four lines.

A PR that waits two days blocks its author, accumulates conflicts with main and loses context (the author has already forgotten why they did what they did). Many teams agree an explicit commitment along the lines of "every PR gets a first response within 24 hours". The commitment is to respond, not necessarily to approve: an "I'll look at it tomorrow morning" already unblocks the other person's planning.

If only one thing from this lesson were applied in the team, it should be this one.

A human reviewer's capacity does not scale with the size of the change: it collapses. Studies on code review have been pointing in the same direction for decades, and the experience of anybody who has reviewed a lot confirms it:

There is a cruel paradox hidden in there: the bigger a change is, the riskier it is and the less it gets reviewed. A fifteen-hundred-line PR, which is precisely the one that would most need attention, is the one that gets an approval in four minutes because nobody has the time or the energy to do it properly.

And there is a second, less obvious effect: the number of comments does not grow with size, it shrinks. A reviewer leaves ten comments on a hundred-line PR and three on a thousand-line one, because in the second they give up. The author reads the silence as approval.

Some changes are large by nature. Strategies, in order of preference:

1. Split it into several chained PRs. Each one complete, coherent and reviewable on its own; each one starting from the previous one. It is more work for the author and enormously better for the team.
2. Separate the mechanical from the substantial. One PR with the mass renaming or the reformatting (reviewable in two minutes with git diff -w) and another with the behaviour change. And record the reformatting commit in .git-blame-ignore-revs (lesson 06-03).
3. Review commit by commit, if the author built them well (section 4).
4. Review in pairs, live. For large architectural changes, half an hour of conversation yields more than two hundred asynchronous comments.

And a recommendation for the author: if your PR is going to be unavoidably large, say so in the description and suggest a reading order. "Start with app.js lines 40-90, which is the real change; the rest is mechanical propagation" saves the reviewer half an hour.

Two nuances about "approve" that avoid a lot of friction:

• Approving does not mean "it is perfect", it means "this improves the current state of the project and does not introduce problems". Chasing perfection in every PR paralyses the team and burns people out.
• You can approve with minor comments. It is the healthy way out for nit:s and suggestion:s: you approve, the author decides whether to apply them, and nobody waits another review round over a variable name. Many teams call it LGTM with nits.

Technical disagreements are normal and healthy. What has to be avoided is letting them fester in the PR thread for days.

A recommended protocol, in order:

1. Bring data, not opinions. "This is slow" gets nowhere; "I measured it with a thousand tasks: 340 ms against 12 ms" gets somewhere.
2. Separate the important from the aesthetic. If you cannot explain what real problem the thing you are criticising causes, it is probably a preference. Mark it as nit: and move on.
3. If after two rounds you still disagree, get out of the text. A ten-minute call resolves what twenty comments do not. Afterwards, write the conclusion in the PR so there is a record.
4. If it still is not resolved, escalate using a criterion agreed in advance: the person responsible for that part of the code decides, or a third party breaks the tie. What matters is that the rule exists before the conflict, not that it gets improvised during it.
5. Document the decision so as not to repeat the discussion in three months' time. If it is a general convention, it goes in CONTRIBUTING.md; if it is an architectural decision, into a decision record.

And a useful asymmetry that many teams adopt: whoever proposes a change to the status quo carries the burden of proof. If the project already does things one way and the reviewer prefers another, the reviewer should justify the change, not the author defend what already exists. It stops every PR turning into a referendum on the project's style.

One type of comment turns up in almost every review: the commit message does not explain the why, does not follow the agreed format, or says "various fixes". It is a legitimate comment and it is worth making — the history is permanent documentation — but the specific rules for writing them are the content of lesson 08-01. There we shall see what format to use, how to structure the body and how to link the GT-NNN tickets that the module 6 hooks already validate.

Mistake 1: using git diff main branch (or main..branch) to review. It mixes the author's work with what main has moved on by since they branched off. Three dots: git diff main...branch.

Mistake 2: confusing the semantics of the dots between diff and log. In diff you want ...; in log you want ... It is counter-intuitive and it is how it is.

Mistake 3: reviewing only in the browser. Without running the code, a review catches typos and little else. Fetch the branch with the PR reference and try it out.

Mistake 4: starting with style. All the attention gets spent on the trivial and a wrong design gets approved. Correctness, design, readability, style. In that order.

Mistake 5: commenting on what the linter already checks. It is noise, it breeds resentment and it reveals a gap in the project's tooling, not in the author's code.

Mistake 6: not distinguishing the blocking from the optional. The author is left not knowing what they have to change in order to merge. Use explicit prefixes.

Mistake 7: rereading the whole PR after the author rebases. That is what git range-diff exists for. Save a branch holding what you reviewed and compare.

Mistake 8: approving a thousand-line PR in five minutes. It is worse than not reviewing it, because it creates a false sense of control. Ask for it to be split up.

Mistake 9: turning the review into a display of technical superiority. It destroys the team's willingness to propose changes, which is the very asset review was meant to protect.

Tip 1: git diff --stat main...branch always first. Ten seconds that orient the whole review.

Tip 2: save a marker branch after reviewing. git branch review/pr-42-v1 review/pr-42. Your future self will thank you when v2 arrives.

Tip 3: git range-diff @{u}...HEAD before every push --force-with-lease. It verifies that the rebase has not broken anything. Thirty seconds well spent.

Tip 4: -U10 and --color-moved by default in reviews. More context, and a distinction between moved and rewritten code.

Tip 5: a worktree dedicated to reviewing. git worktree add ../review <branch>. Reviewing stops costing a context switch.

Tip 6: an alias for whatever you repeat. For example git config --global alias.review '!f() { git fetch origin pull/$1/head:review/pr-$1 && git diff --stat main...review/pr-$1; }; f' (lesson 06-04).

1. Create a repository with app.js and styles.css and three commits on main.
2. Create feature/filters from there and make two commits that only touch app.js.
3. Go back to main and make two more commits that touch only styles.css.
4. Run git diff main feature/filters, git diff main..feature/filters and git diff main...feature/filters. Explain what each one shows and why the third is the right one for reviewing.
5. Demonstrate the equivalence of the three-dot form using git merge-base.
6. Run git log --oneline main..feature/filters and git log --oneline main...feature/filters --left-right. Explain the difference with the diff case.

On the previous repository:

1. Get the summary of files and lines in the change.
2. List the branch's commits with their full messages.
3. Walk through them one by one from oldest to newest, with their diffs.
4. Show the full diff with ten lines of context and ignoring whitespace.
5. Create a worktree at /tmp/review pointing at the branch, without leaving main.
6. Find out who last wrote the lines the branch modifies, before the change.

1. Save a reference to the branch as it stands: git branch review/v1 feature/filters.
2. Simulate the author's response to a review: rebase feature/filters onto main, modify one line of one of the commits with an interactive rebase (edit) and add a new commit at the end.
3. Run git range-diff main...review/v1 main...feature/filters.
4. Identify in the output which commit is equivalent, which one has changed and which one is new.
5. Try raising --creation-factor and observe how the pairing changes.
6. Generate the branch's patches with git format-patch -v2 --cover-letter and apply them on a new branch with git am. Check that the original author is preserved.

Solution 1:

The first two forms are identical and show styles.css with deleted lines that the branch never touched: they are main's commits seen the wrong way round. The third shows only app.js, which is what the branch actually did.

In log, .. gives the commits exclusive to the branch (what you want) and ... gives those on both sides. In diff it is the other way round. The reason is that diff compares trees and log selects sets of commits.

Solution 2:

Solution 3:

The original author and date are preserved: that is what distinguishes git am from git apply.

Reviewing code well is a skill, and half of that skill is technical. The essentials:

• A review is above all for spreading knowledge and maintaining consistency; finding defects matters, but it is where it adds least compared with automated tests. Whatever a machine can check, let the machine check.
• The right diff for reviewing is git diff main...branch, with three dots: it compares from the common ancestor, not from the tip of main, and therefore shows only what the author did. It is the same one the platforms show.
• Watch out for the asymmetry: three dots in diff, two dots in log. git log --oneline main..branch to see the proposed commits.
• The inspection repertoire: --stat always first, -U10 for more context, -w to ignore whitespace, --color-moved to tell moved code from rewritten code, --reverse -p to walk commit by commit, and a worktree to actually run it.
• git range-diff compares two versions of the same series of commits and marks each one as unchanged (=), modified (!), removed (<) or new (>). It turns a second review after a rebase into five seconds of work. Use it on yourself too, before every push --force-with-lease.
• git request-pull generates the email that gave the "pull request" its name, and format-patch/am are the email review model of the Linux kernel, where range-diff in the v2 cover letter is standard practice.
• Review in order: correctness → design → readability → style, and do not drop a level without closing the previous one.
• Comment about the code, not about the person; explicitly mark the blocking: as against suggestion: and nit:; and comment on what is good, too.
• PR size is the factor that most determines the quality of a review. Between 50 and 200 lines is the sweet spot; above a thousand, the review is practically fictional. Split it up, separate the mechanical from the substantial, or review in pairs.
• Approving means "this improves the project", not "it is perfect". And have it agreed in advance how disagreements get settled.

The team now knows how to propose changes and review them. But the piece above is still missing: which branches exist, what each one is for and where the proposals go? Diego opens his PR against main… but what if the project had a develop branch? And what if an urgent bug had to be fixed in an old version that is already in production? Those are the workflows, and we start with the most structured and classic of them all in lesson 07-03: The Git Flow Workflow.