Review pull requests and respond to feedback

To review a pull request is to read its diff, run it when reading is not enough, leave comments that help the author improve it, and then submit one of three verdicts: Comment, Approve or Request changes. A useful review checks, in this order, whether the change works and the tests prove it, whether it is safe, whether it is readable, and whether it is documented. Good comments are specific, explain why, and say whether they block the merge. The author answers with new commits, often fixup commits that are folded into the commit they correct just before the merge, and splits a change too large to review into a stack of small pull requests.

This lesson reviews one pull request with real Git from both sides. It adds a formatter that looks right and passes its own tests, and it is wrong for one number in two. You will see how a reviewer finds that, how the comment is written, how the author answers with a fixup commit, and how a stack of three pull requests absorbs a fix to the bottom one.

What a review is on GitHub

Anyone with read access to a repository can review a pull request and comment on it; asking someone for a review takes write access to the repository, for the pull request’s author too. On the Files changed tab you click the blue + beside a line, or drag across several lines, write a comment and choose Start a review. Until you submit the review, those comments are pending and only you can see them, so you can read the whole change and edit what you wrote before the author is notified. Then Review changes offers the three verdicts:

That last column surprises people. Request changes is informational on a branch without a ruleset or branch protection rule; on a protected branch that requires pull request reviews, a change request from someone with write access blocks the merge until that same person approves. Such a branch can also require a number of approvals from reviewers with write access, dismiss approvals when new commits change the diff, require that the most recent push is approved by someone other than the person who pushed it, and require a code owner’s approval. An author can never approve their own pull request. From the terminal, gh pr review 21 --approve, --comment or --request-changes with -b for the text submits the same verdicts.

The exercise near the end of this lesson turns these rules into a function that decides whether a set of reviews lets a pull request merge.

Read the diff in an order that finds bugs

A review that starts at the first line of the diff and comments on spacing is the most common kind and the least useful. Read in this order instead:

1. The design. Is this the right change, in the right place? Google’s public review guide calls the overall design the most important thing to cover, and a design problem found after line-by-line comments wastes both people’s time.
2. Does it work? Read the behaviour, not just the code: inputs at the edges, errors, empty and very large values. Ask whether the tests would fail if the code were broken.
3. Is it safe? Secrets, tokens and personal data in the diff; input that reaches a query, a shell or a page without validation; new permissions.
4. Is it readable? Names that say what things are, comments that explain why rather than what, nothing more complex than it needs to be.
5. Is it documented? A changed behaviour needs its README, help text or changelog updated in the same pull request.

The pull request in this lesson adds a function that formats amounts for a fest budget page with Indian digit grouping, where the last three digits form one group and the digits in front of them are grouped in twos.

The script below plays both sides with real Git and Node.js. It reads the diff the way the Files changed tab shows it, against the point where the branch left main, then runs the pull request’s own tests, then tests the sizes at which the grouping changes, comparing with Intl.NumberFormat('en-IN').

// A review, the way a careful reviewer does it: read the pull request's diff, run its own tests, then test the edges
// the tests left out. The pull request adds Indian digit grouping (12,34,567) to a budget page.
import { spawnSync } from 'node:child_process';
import { join } from 'node:path';
import { pathToFileURL } from 'node:url';
import { asha, meera, freshFolder, newRepo, git, show, write, commit } from './git-kit.mjs';

const repo = newRepo(freshFolder('reviewing'), meera, 'budget');
commit(repo, meera, 'budget.mjs', "export const total = (items) => items.reduce((sum, x) => sum + x, 0);\n", 'Add the budget total');

git(repo, asha, 'switch', '-q', '-c', 'indian-grouping');
write(repo, 'format.mjs', `// Formats a whole number with Indian digit grouping: 1234567 -> "12,34,567".
export function formatIndian(n) {
  const digits = String(n);
  if (digits.length <= 3) return digits;
  let head = digits.slice(0, -3);
  const groups = [digits.slice(-3)];
  while (head.length >= 2) {
    groups.unshift(head.slice(-2));
    head = head.slice(0, -2);
  }
  return [head, ...groups].join(',');
}
`);
write(repo, 'format.test.mjs', `import { formatIndian } from './format.mjs';

const cases = [[999, '999'], [1000, '1,000'], [100000, '1,00,000'], [10000000, '1,00,00,000']];
let failed = 0;
for (const [n, want] of cases) {
  const got = formatIndian(n);
  if (got !== want) failed++;
  console.log(\`\${got === want ? 'ok  ' : 'FAIL'} formatIndian(\${n}) = \${got}\`);
}
console.log(\`\${cases.length - failed} passed, \${failed} failed\`);
process.exitCode = failed ? 1 : 0;
`);
git(repo, asha, 'add', '.');
git(repo, asha, 'commit', '-q', '-m', 'Format amounts with Indian digit grouping');

console.log(git(repo, meera, 'version').out);
console.log('\n# 1. Read what the pull request changes, against the point where it left main');
show(repo, meera, 'diff', '--stat', 'main...indian-grouping');
show(repo, meera, 'diff', 'main...indian-grouping', '--', 'format.mjs');

console.log("\n# 2. Run the pull request's own tests");
git(repo, meera, 'switch', '-q', 'indian-grouping');
const tests = spawnSync(process.execPath, ['format.test.mjs'], { cwd: repo, encoding: 'utf8' });
console.log('$ node format.test.mjs');
console.log(`${tests.stdout}${tests.stderr}`.trimEnd());

console.log('\n# 3. Test the edges: where the number of digits before the last three changes');
const { formatIndian } = await import(pathToFileURL(join(repo, 'format.mjs')).href);
const intl = new Intl.NumberFormat('en-IN');
console.log('number      the pull request   Intl.NumberFormat(en-IN)');
for (const n of [1000, 10000, 100000, 1000000, 1234567, 10000000, -2500]) {
  const ours = formatIndian(n);
  const ref = intl.format(n);
  console.log(`${String(n).padEnd(12)}${ours.padEnd(19)}${ref}${ours === ref ? '' : '   <- differs'}`);
}

git version 2.54.0 (Apple Git-157)

# 1. Read what the pull request changes, against the point where it left main
$ git diff --stat main...indian-grouping
 format.mjs      | 12 ++++++++++++
 format.test.mjs | 11 +++++++++++
 2 files changed, 23 insertions(+)
$ git diff main...indian-grouping -- format.mjs
diff --git a/format.mjs b/format.mjs
new file mode 100644
index 0000000..33ed249
--- /dev/null
+++ b/format.mjs
@@ -0,0 +1,12 @@
+// Formats a whole number with Indian digit grouping: 1234567 -> "12,34,567".
+export function formatIndian(n) {
+  const digits = String(n);
+  if (digits.length <= 3) return digits;
+  let head = digits.slice(0, -3);
+  const groups = [digits.slice(-3)];
+  while (head.length >= 2) {
+    groups.unshift(head.slice(-2));
+    head = head.slice(0, -2);
+  }
+  return [head, ...groups].join(',');
+}

# 2. Run the pull request's own tests
$ node format.test.mjs
ok   formatIndian(999) = 999
ok   formatIndian(1000) = 1,000
ok   formatIndian(100000) = 1,00,000
ok   formatIndian(10000000) = 1,00,00,000
4 passed, 0 failed

# 3. Test the edges: where the number of digits before the last three changes
number      the pull request   Intl.NumberFormat(en-IN)
1000        1,000              1,000
10000       ,10,000            10,000   <- differs
100000      1,00,000           1,00,000
1000000     ,10,00,000         10,00,000   <- differs
1234567     ,12,34,567         12,34,567   <- differs
10000000    1,00,00,000        1,00,00,000
-2500       ,-2,500            -2,500   <- differs

// The helpers this lesson's examples share. Each example builds throwaway repositories in a folder of its own and
// runs real Git on them, with Git's system and personal settings switched off, fixed names and e-mail addresses, and
// a fixed clock that moves one minute per command, so the commit IDs it prints are the ones you get when you run it.
import { spawnSync } from 'node:child_process';
import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs';
import { join } from 'node:path';

// On macOS, /usr/bin/git is a small launcher that looks Git up again on every call. Ask once where Git keeps its own
// programs and run the git found there (anywhere else that is the same git as on your PATH).
const own = spawnSync('git', ['--exec-path'], { encoding: 'utf8' }).stdout?.trim() ?? '';
const GIT = [join(own, 'git'), join(own, 'git.exe')].find((file) => own && existsSync(file)) ?? 'git';

export const asha = { name: 'Asha Rao', email: 'asha@example.com' };
export const ravi = { name: 'Ravi Menon', email: 'ravi@example.com' };
export const meera = { name: 'Meera Iyer', email: 'meera@example.com' };

let clock = 1767600000; // seconds since 1970; Git's own date format is "<seconds> <time zone>"
let emptyConfig = '';

/**
 * A new, empty folder for this run's repositories, next to the script (made again on every run). It also holds the
 * empty settings file that stands in for your own, so the repositories go in folders inside it (newRepo, git clone).
 */
export function freshFolder(name) {
  const dir = join(process.cwd(), name);
  rmSync(dir, { recursive: true, force: true });
  mkdirSync(dir, { recursive: true });
  emptyConfig = join(dir, 'empty.gitconfig');
  writeFileSync(emptyConfig, '');
  return dir;
}

/** Creates a repository with the branch main in a new folder inside `folder`, and returns its path. */
export function newRepo(folder, who, name) {
  git(folder, who, 'init', '-q', '-b', 'main', name);
  return join(folder, name);
}

/** Runs git in `dir` as `who` and returns its exit code and what it printed (both streams, as a terminal shows them). */
export function git(dir, who, ...args) {
  clock += 60;
  const when = `${clock} +0530`;
  const r = spawnSync(GIT, args, {
    cwd: dir,
    encoding: 'utf8',
    env: {
      PATH: process.env.PATH ?? '',
      HOME: process.env.HOME ?? dir,
      LC_ALL: 'C',
      GIT_CONFIG_NOSYSTEM: '1',
      GIT_CONFIG_GLOBAL: emptyConfig,
      GIT_AUTHOR_NAME: who.name,
      GIT_AUTHOR_EMAIL: who.email,
      GIT_COMMITTER_NAME: who.name,
      GIT_COMMITTER_EMAIL: who.email,
      GIT_AUTHOR_DATE: when,
      GIT_COMMITTER_DATE: when,
    },
  });
  return { code: r.status, out: `${r.stdout}${r.stderr}`.trimEnd() };
}

/** An argument as you would type it in a shell ("--format='%h %s'"). */
function quote(arg) {
  if (/^[\w@%+=:,./^~-]+$/.test(arg)) return arg;
  const option = /^(--[\w-]+=)(.*)$/.exec(arg);
  return option ? `${option[1]}'${option[2]}'` : `'${arg}'`;
}

/** Like git(), and prints the command and its output the way a terminal session reads. */
export function show(dir, who, ...args) {
  const r = git(dir, who, ...args);
  console.log(`$ git ${args.map(quote).join(' ')}`);
  // A progress line ends in a carriage return and is overwritten by the next one: keep what a terminal ends up showing.
  const shown = r.out.split('\n').map((line) => line.split('\r').filter(Boolean).at(-1) ?? '').join('\n');
  if (shown) console.log(shown);
  return r;
}

/** Writes a file inside a repository. */
export function write(dir, file, text) {
  writeFileSync(join(dir, file), text);
}

/** Writes a file and commits it as `who`. */
export function commit(dir, who, file, text, message) {
  write(dir, file, text);
  git(dir, who, 'add', file);
  return git(dir, who, 'commit', '-q', '-m', message);
}

All four of the pull request’s tests pass, and the function is still wrong for 10000, 1000000, 1234567 and -2500, which come out with a stray comma in front. Every tested number leaves an odd count of digits in front of the last three (0, 1, 3 and 5), and the loop’s head.length >= 2 goes wrong only when that count is even: with exactly two digits left it takes them as a group and leaves an empty string, which join turns into a leading comma. Round numbers made the tests look thorough; the sizes where the grouping changes are what a reviewer should try. The three-dot range main...indian-grouping matters too: it compares with the commit where the branch started, which is what a pull request page shows, so changes that landed on main meanwhile do not appear as the author’s.

Two habits help on larger diffs. Mark each file Viewed once you have read it; GitHub collapses it, counts your progress, and unmarks it if the file changes again. And when you are sent a patch rather than a pull request, you can still read it with the same care.

Comment so the author can act

Each comment should leave the author knowing what is wrong, why it matters, and whether the merge waits for it. Google’s guide for its reviewers adds a few rules that travel well:

• Comment on the code, not the person. “This loop drops the last group” reads very differently from “you broke the loop”.
• Explain why, so the author learns the principle and not just the fix.
• Point out the problem and let the author choose the fix, unless a direct suggestion is clearly faster.
• Label how much each comment matters: Nit: for a small polish, Optional: for an idea, FYI: for something to know for later. Without labels, authors tend to treat every comment as mandatory.
• Say what is good, too.

For the formatter, four comments cover what the drill found. The first blocks the merge and carries a suggested change: a block fenced with suggestion that holds the replacement for the lines the comment is on, which the author can commit with one click.

**Blocking:** this loop also takes the last two digits when exactly two are left, so `formatIndian(1000000)`
returns `,10,00,000` with a leading comma, and so does every number with 5, 7 or 9 digits. Stopping while more
than two digits are left fixes it:

```suggestion
  while (head.length > 2) {
```

Could you add 10000, 1000000 and 1234567 to the tests? Those are the sizes where the grouping changes.

The other three: “Could this use Intl.NumberFormat('en-IN')? It groups the same way, handles negative amounts and decimals, and needs no code of ours to maintain.” (a design question, asked as a question); “Can a total be negative, for a refund? Today -2500 prints ,-2,500; either handle it or say in the comment that the function takes non-negative whole numbers.” (behaviour); and “Nit: head could be leading to say what it holds.” Then the verdict is Request changes, because the first comment blocks.

Suggestions are applied from the pull request page with Commit suggestion, or collected with Add suggestion to batch and committed together. Either way GitHub makes one commit on the pull request’s branch, with everyone whose suggestion is in it as a co-author and the person applying them as the committer. You need write access to the repository to apply them; on a pull request from a fork, maintainers can apply them when the author allowed edits from maintainers.

Automated reviewers are worth a look as a second opinion, and that is all they are. GitHub’s own documentation for Copilot code review says it is not guaranteed to spot every problem, that it sometimes makes mistakes, and that its feedback should be checked and supplemented with a human review; by default its reviews do not count toward required approvals. Whoever approves owns the approval.

A reviewer’s speed matters as much as their care: Google’s guide sets one business day as the longest a review request should wait for a first response, and suggests asking for a large change to be split rather than leaving it unreviewed.

Run it before you approve

Reading finds most problems; running finds the rest. gh pr checkout 21 fetches pull request 21 into a local branch and switches to it, whether it came from a branch of the same repository or from a fork; it also takes a pull request’s URL or branch name, --detach checks it out without a branch, and --force resets an existing local branch to the pull request’s latest state.

Without gh, Git can do it alone, because GitHub keeps every pull request’s commits in the base repository under a ref of its own, even before the merge: git fetch origin pull/21/head:pr-21 creates the branch pr-21 from it. A normal clone does not fetch those refs, and they are read-only: a push to refs/pull/ is refused as an attempt to update a hidden ref. In the script below a bare repository plays GitHub, and the script creates refs/pull/21/head the way GitHub does when the pull request is opened.

// Checking out a pull request to run it. GitHub keeps every pull request's commits under refs/pull/<number>/head in
// the base repository, even when they came from a fork. Here a bare repository plays GitHub, and the script creates
// that ref the way GitHub does when pull request #21 is opened.
import { join } from 'node:path';
import { asha, meera, freshFolder, git, show, commit } from './git-kit.mjs';

const root = freshFolder('pull-refs');
const at = (name) => join(root, name);
git(root, meera, 'init', '-q', '--bare', '-b', 'main', 'origin.git');
git(root, meera, 'clone', '-q', 'origin.git', 'seed');
commit(at('seed'), meera, 'budget.mjs', 'export const total = (items) => items.reduce((a, b) => a + b, 0);\n', 'Add the budget total');
git(at('seed'), meera, 'push', '-q', 'origin', 'main');

// Asha works in her fork; opening the pull request makes GitHub store her branch's tip as refs/pull/21/head.
git(root, asha, 'clone', '-q', 'origin.git', 'asha-fork');
git(at('asha-fork'), asha, 'switch', '-q', '-c', 'indian-grouping');
commit(at('asha-fork'), asha, 'format.mjs', '// Indian digit grouping\nexport const formatIndian = (n) => String(n);\n', 'Format amounts with Indian digit grouping');
git(at('asha-fork'), asha, 'push', '-q', 'origin', 'indian-grouping:refs/pull/21/head');

// Meera reviews from her own clone.
git(root, meera, 'clone', '-q', 'origin.git', 'meera');
const repo = at('meera');
console.log(git(repo, meera, 'version').out);
console.log('\n# The pull request is in the repository, but a normal clone does not fetch it');
show(repo, meera, 'ls-remote', 'origin');
show(repo, meera, 'branch', '--all');

console.log('\n# Fetch pull request 21 into a local branch and look at it');
show(repo, meera, 'fetch', 'origin', 'pull/21/head:pr-21');
show(repo, meera, 'switch', 'pr-21');
show(repo, meera, 'log', '--oneline', 'main..pr-21');

console.log('\n# Or fetch every pull request, as origin/pr/<number>, from now on');
show(repo, meera, 'config', '--add', 'remote.origin.fetch', '+refs/pull/*/head:refs/remotes/origin/pr/*');
show(repo, meera, 'fetch', 'origin');

git version 2.54.0 (Apple Git-157)

# The pull request is in the repository, but a normal clone does not fetch it
$ git ls-remote origin
4fe3af02406fea6f318def13ee5ecedb7adfd32d	HEAD
4fe3af02406fea6f318def13ee5ecedb7adfd32d	refs/heads/main
27c7a0d769028a11ddabe9c4b86c944e34b66787	refs/pull/21/head
$ git branch --all
* main
  remotes/origin/HEAD -> origin/main
  remotes/origin/main

# Fetch pull request 21 into a local branch and look at it
$ git fetch origin pull/21/head:pr-21
From pull-refs/origin
 * [new ref]         refs/pull/21/head -> pr-21
$ git switch pr-21
Switched to branch 'pr-21'
$ git log --oneline main..pr-21
27c7a0d Format amounts with Indian digit grouping

# Or fetch every pull request, as origin/pr/<number>, from now on
$ git config --add remote.origin.fetch '+refs/pull/*/head:refs/remotes/origin/pr/*'
$ git fetch origin
From pull-refs/origin
 * [new ref]         refs/pull/21/head -> origin/pr/21

git ls-remote shows the ref on the remote, and git branch --all shows that the clone did not fetch it. After the fetch, main..pr-21 lists exactly the pull request’s commit. The last two commands add a second fetch rule to the remote’s configuration and fetch with it: from then on every git fetch brings each pull request in as origin/pr/<number>. That is handy for a maintainer who checks out several pull requests a day, and noisy in a busy repository with thousands of them.

Who is asked to review: code owners

A CODEOWNERS file names the people or teams responsible for parts of a repository, and GitHub requests their review automatically when a pull request changes their files. Put it in .github/, in the repository’s root or in docs/; if there are several, GitHub uses the first it finds, in that order. The file that counts is the one on the pull request’s base branch, so each branch can have its own.

Each line is a pattern followed by its owners: a user (@meera), a team (@fest-org/web-team) or an e-mail address. Patterns follow most gitignore rules, with three exceptions: ! does not negate a pattern, [ ] does not define a character range, and \# does not turn a line that starts with # into a pattern. Paths are case sensitive, even on a computer whose file system is not. And the rule that decides everything: for each file, the last matching line wins.

# Code owners of the fest website. For each changed file, the last line that matches decides who is asked to review.
*                         @fest-org/web-team
*.css                     @meera
/docs/                    @ravi
/src/payments/            @fest-org/treasurers
/src/payments/README.md   @asha

Swap lines 3 and 4, and docs/schedule.css goes to @meera instead: in this file the order of the lines is part of their meaning, so put broad patterns first and the exceptions after them. One more difference from gitignore is worth knowing: in CODEOWNERS, docs/* matches the files directly in docs but not files in folders below it, while in a .gitignore file the same pattern also matches those folders, and with them everything inside.

A few rules keep a CODEOWNERS file from quietly doing nothing. A line with invalid syntax is skipped, and GitHub flags errors when you open the file on GitHub. An owner who does not exist or lacks write access to the repository is not assigned. A file of 3 MB or more is ignored completely. Owners are not asked to review a draft pull request until it is marked ready for review. And a protected branch can require a code owner’s approval, which is what turns “asked to review” into “must approve”.

Answer review with commits

On the author’s side, the simplest answer to a review is another commit, pushed to the same branch, and a reply to each comment saying what you changed or why you did not. Once the changes are substantial, ask for another look: the sync icon next to each reviewer’s name, on the right of the Conversation tab, requests their review again.

When a comment points at a mistake in an earlier commit of the branch, a fixup commit keeps both the review and the history clean. git commit --fixup=<commit> records the change as a commit of its own, titled fixup! followed by the title of the commit it corrects, so the reviewer can see exactly what the fix was. Before the merge, git rebase --autosquash main moves each fixup right after its target and squashes it in; the target keeps its log message and its author. (--fixup=amend:<commit> also replaces the target’s message, and --fixup=reword:<commit> changes only the message.) Since Git 2.44, --autosquash works without --interactive; add -i to look at the plan first. If the project squash-merges every pull request, you can skip the folding: the squash does it.

// Answering review with a fixup commit: the reviewer sees exactly what changed, and before the merge the fix is
// folded into the commit it corrects. git range-diff then compares the branch before and after.
import { join } from 'node:path';
import { asha, meera, freshFolder, git, show, write, commit } from './git-kit.mjs';

const BUGGY = `// Formats a whole number with Indian digit grouping: 1234567 -> "12,34,567".
export function formatIndian(n) {
  const digits = String(n);
  if (digits.length <= 3) return digits;
  let head = digits.slice(0, -3);
  const groups = [digits.slice(-3)];
  while (head.length >= 2) {
    groups.unshift(head.slice(-2));
    head = head.slice(0, -2);
  }
  return [head, ...groups].join(',');
}
`;
const TESTS = (cases) => `import { formatIndian } from './format.mjs';

for (const [n, want] of ${cases}) {
  if (formatIndian(n) !== want) throw new Error(\`formatIndian(\${n}) is \${formatIndian(n)}, not \${want}\`);
}
`;

const root = freshFolder('fixups');
const at = (name) => join(root, name);
git(root, meera, 'init', '-q', '--bare', '-b', 'main', 'origin.git');
git(root, meera, 'clone', '-q', 'origin.git', 'seed');
commit(at('seed'), meera, 'budget.mjs', "export const total = (items) => items.reduce((a, b) => a + b, 0);\n", 'Add the budget total');
git(at('seed'), meera, 'push', '-q', 'origin', 'main');

git(root, asha, 'clone', '-q', 'origin.git', 'asha');
const repo = at('asha');
git(repo, asha, 'switch', '-q', '-c', 'indian-grouping');
write(repo, 'format.mjs', BUGGY);
write(repo, 'format.test.mjs', TESTS("[[999, '999'], [1000, '1,000'], [100000, '1,00,000']]"));
git(repo, asha, 'add', '.');
git(repo, asha, 'commit', '-q', '-m', 'Format amounts with Indian digit grouping');
commit(repo, asha, 'budget.mjs', "import { formatIndian } from './format.mjs';\n\nexport const total = (items) => formatIndian(items.reduce((a, b) => a + b, 0));\n", 'Show budget totals with digit grouping');
git(repo, asha, 'push', '-q', '-u', 'origin', 'indian-grouping');
console.log(git(repo, asha, 'version').out);

console.log('\n# Review asked for a fix in the first commit: Asha commits it as a fixup and pushes');
write(repo, 'format.mjs', BUGGY.replace('head.length >= 2', 'head.length > 2'));
write(repo, 'format.test.mjs', TESTS("[[999, '999'], [1000, '1,000'], [10000, '10,000'], [100000, '1,00,000'], [1000000, '10,00,000'], [1234567, '12,34,567']]"));
git(repo, asha, 'add', 'format.mjs', 'format.test.mjs');
show(repo, asha, 'commit', '--fixup=HEAD~1');
show(repo, asha, 'log', '--oneline', 'main..');
show(repo, asha, 'push');

console.log('\n# Approved. Before the merge, fold the fixup into the commit it fixes');
show(repo, asha, 'rebase', '--autosquash', 'main');
show(repo, asha, 'log', '--oneline', 'main..');

console.log('\n# What changed between the branch on the remote and the rewritten one');
show(repo, asha, 'range-diff', 'main', 'origin/indian-grouping', 'indian-grouping');
show(repo, asha, 'push', '--force-with-lease');

git version 2.54.0 (Apple Git-157)

# Review asked for a fix in the first commit: Asha commits it as a fixup and pushes
$ git commit --fixup=HEAD~1
[indian-grouping e0d30da] fixup! Format amounts with Indian digit grouping
 2 files changed, 2 insertions(+), 2 deletions(-)
$ git log --oneline main..
e0d30da fixup! Format amounts with Indian digit grouping
066c956 Show budget totals with digit grouping
7554d1f Format amounts with Indian digit grouping
$ git push
To fixups/origin.git
   066c956..e0d30da  indian-grouping -> indian-grouping

# Approved. Before the merge, fold the fixup into the commit it fixes
$ git rebase --autosquash main
Successfully rebased and updated refs/heads/indian-grouping.
$ git log --oneline main..
142161c Show budget totals with digit grouping
332a11c Format amounts with Indian digit grouping

# What changed between the branch on the remote and the rewritten one
$ git range-diff main origin/indian-grouping indian-grouping
1:  7554d1f ! 1:  332a11c Format amounts with Indian digit grouping
    @@ format.mjs (new)
     +  if (digits.length <= 3) return digits;
     +  let head = digits.slice(0, -3);
     +  const groups = [digits.slice(-3)];
    -+  while (head.length >= 2) {
    ++  while (head.length > 2) {
     +    groups.unshift(head.slice(-2));
     +    head = head.slice(0, -2);
     +  }
    @@ format.test.mjs (new)
     @@
     +import { formatIndian } from './format.mjs';
     +
    -+for (const [n, want] of [[999, '999'], [1000, '1,000'], [100000, '1,00,000']]) {
    ++for (const [n, want] of [[999, '999'], [1000, '1,000'], [10000, '10,000'], [100000, '1,00,000'], [1000000, '10,00,000'], [1234567, '12,34,567']]) {
     +  if (formatIndian(n) !== want) throw new Error(`formatIndian(${n}) is ${formatIndian(n)}, not ${want}`);
     +}
2:  066c956 = 2:  142161c Show budget totals with digit grouping
3:  e0d30da < -:  ------- fixup! Format amounts with Indian digit grouping
$ git push --force-with-lease
To fixups/origin.git
 + e0d30da...142161c indian-grouping -> indian-grouping (forced update)

The push of the fixup is an ordinary fast-forward (066c956..e0d30da), so the reviewer looks at one small commit instead of rereading the branch. After the approval, the rebase leaves two commits, and git range-diff explains the rewrite commit by commit. 1: 7554d1f ! 1: 332a11c says the first commit’s patch changed, and the indented diff of diffs shows how: the loop condition and three new test cases. 2: 066c956 = 2: 142161c says the second commit’s patch is identical and only its ID changed, because the commit below it changed. 3: e0d30da < -: ------- says the fixup exists only in the old version. That one command is the honest answer to “what changed since I approved?”, and its output is worth pasting into the pull request when you rewrite a reviewed branch. The rewrite then needs --force-with-lease, as the lesson on pull requests showed, and on a branch that dismisses stale approvals, a fresh approval: fold fixups in before the final approval, not after it.

Split a big change into a stack

Google’s guide calls about 100 changed lines a reasonable size for one change and 1,000 lines usually too large, and its advice for big features is to stack: write one small change, send it for review, and start the next one on top of it. On GitHub, a stack is a chain of pull requests whose bases are each other’s branches: part-1 into main, part-2 into part-1, part-3 into part-2. Each one has a small diff that one reviewer can read in one sitting.

The difficulty comes when review asks for a change at the bottom. Fixing part-1 rewrites its commit, and every branch above it still sits on the old one. git rebase --update-refs (Git 2.38 and later) solves that: while it rebases the top branch, it also moves every branch that points at one of the commits it rewrites. Set rebase.updateRefs to true to make it the default. The script below makes the same fix to the same stack twice, without and with it.

// A stack of three pull requests: part-1 into main, part-2 into part-1, part-3 into part-2. Review asks for a change
// in part-1. Folding it in from the top of the stack rewrites part-1's commit, so the branches above it must move too:
// without --update-refs they are left on the old commits; with it, one rebase moves all three.
import { asha, freshFolder, newRepo, git, show, write, commit } from './git-kit.mjs';

const FORMAT = `export function formatIndian(n) {
  const digits = String(n);
  if (digits.length <= 3) return digits;
  let head = digits.slice(0, -3);
  const groups = [digits.slice(-3)];
  while (head.length > 2) {
    groups.unshift(head.slice(-2));
    head = head.slice(0, -2);
  }
  return [head, ...groups].join(',');
}
`;

function buildStack(folder, name) {
  const repo = newRepo(folder, asha, name);
  commit(repo, asha, 'budget.mjs', 'export const total = (items) => items.reduce((a, b) => a + b, 0);\n', 'Add the budget total');
  git(repo, asha, 'switch', '-q', '-c', 'part-1');
  commit(repo, asha, 'format.mjs', FORMAT, 'Add a formatter for Indian digit grouping');
  git(repo, asha, 'switch', '-q', '-c', 'part-2');
  commit(repo, asha, 'budget.mjs', "import { formatIndian } from './format.mjs';\n\nexport const total = (items) => formatIndian(items.reduce((a, b) => a + b, 0));\n", 'Show budget totals with digit grouping');
  git(repo, asha, 'switch', '-q', '-c', 'part-3');
  commit(repo, asha, 'report.mjs', "import { total } from './budget.mjs';\n\nexport const report = (items) => `Total: Rs ${total(items)}`;\n", 'Add a printable budget report');
  // The review of part-1 asks for negative amounts (refunds); Asha fixes it from the top of the stack.
  write(repo, 'format.mjs', FORMAT.replace('export function formatIndian(n) {\n', "export function formatIndian(n) {\n  if (n < 0) return '-' + formatIndian(-n);\n"));
  git(repo, asha, 'add', 'format.mjs');
  git(repo, asha, 'commit', '-q', '--fixup=part-1');
  return repo;
}

const folder = freshFolder('stack');
const before = buildStack(folder, 'without-update-refs');
console.log(git(before, asha, 'version').out);
console.log('\n# The stack, with the fixup for part-1 on top of part-3');
show(before, asha, 'log', '--oneline', '--decorate', 'main..part-3');

console.log('\n# Without --update-refs: only part-3 moves');
show(before, asha, 'rebase', '--autosquash', 'main');
show(before, asha, 'log', '--graph', '--oneline', '--decorate', 'part-1', 'part-2', 'part-3');

console.log('\n# With --update-refs: one rebase moves the whole stack');
const after = buildStack(folder, 'with-update-refs');
show(after, asha, 'rebase', '--autosquash', '--update-refs', 'main');
show(after, asha, 'log', '--graph', '--oneline', '--decorate', 'part-1', 'part-2', 'part-3');

git version 2.54.0 (Apple Git-157)

# The stack, with the fixup for part-1 on top of part-3
$ git log --oneline --decorate main..part-3
0fc321e (HEAD -> part-3) fixup! Add a formatter for Indian digit grouping
930135e Add a printable budget report
8a1a345 (part-2) Show budget totals with digit grouping
bab0cc8 (part-1) Add a formatter for Indian digit grouping

# Without --update-refs: only part-3 moves
$ git rebase --autosquash main
Successfully rebased and updated refs/heads/part-3.
$ git log --graph --oneline --decorate part-1 part-2 part-3
* e3e92bc (HEAD -> part-3) Add a printable budget report
* f32fb80 Show budget totals with digit grouping
* 2d3126b Add a formatter for Indian digit grouping
| * 8a1a345 (part-2) Show budget totals with digit grouping
| * bab0cc8 (part-1) Add a formatter for Indian digit grouping
|/  
* 96ca742 (main) Add the budget total

# With --update-refs: one rebase moves the whole stack
$ git rebase --autosquash --update-refs main
Successfully rebased and updated refs/heads/part-3.
Updated the following refs with --update-refs:
	refs/heads/part-1
	refs/heads/part-2
$ git log --graph --oneline --decorate part-1 part-2 part-3
* e2a6e4b (HEAD -> part-3) Add a printable budget report
* d3ac4b7 (part-2) Show budget totals with digit grouping
* 4e69692 (part-1) Add a formatter for Indian digit grouping
* 007041b (main) Add the budget total

Without --update-refs, only part-3 moved: the graph forks, with part-1 and part-2 still on the commits without the fix, so their pull requests would go on showing the old code. With it, the same rebase reported Updated the following refs with --update-refs and the three branches line up again; one git push --force-with-lease origin part-1 part-2 part-3 then updates all three pull requests.

When the bottom pull request merges, change the next one’s base to main (edit the pull request and pick the new base); GitHub warns that some commits may leave the timeline and some review comments may become outdated. If part-1 was squash-merged, its original commits never reached main, so replay only the commits of part-2: git rebase --onto main part-1 part-2.

Practise: can this pull request be merged?

The review rules of a protected branch are easy to state and easy to misapply, especially stale approvals and change requests from people without write access. Write the decision as a function and check it against the sample tests.

/** The first reason the pull request cannot be merged yet, or null when its reviews allow the merge. */
export function mergeBlocker(pr, reviews, rules) {
  // Replace this line with your code.
  return null;
}

import { test, assert } from 'mysmartcopilot:test';
import { mergeBlocker } from './merge.mjs';

const pr = { author: 'asha', head: 'c2', owners: [] };
const one = { approvals: 1, dismissStale: false, requireCodeOwner: false };
const review = (state) => (user, commit = 'c2', canWrite = true) => ({ user, state, commit, canWrite });
const approve = review('approved');
const changes = review('changes_requested');
const comment = review('commented');

test('one approval from someone with write access is enough', () => {
  assert.equal(mergeBlocker(pr, [approve('meera')], one), null);
  assert.equal(mergeBlocker(pr, [], one), 'needs-approvals');
});

test('the author cannot approve their own pull request', () => {
  assert.equal(mergeBlocker(pr, [approve('asha')], one), 'needs-approvals');
});

test('reviews without write access do not count', () => {
  assert.equal(mergeBlocker(pr, [approve('guest', 'c2', false)], one), 'needs-approvals');
  assert.equal(mergeBlocker(pr, [changes('guest', 'c2', false), approve('meera')], one), null);
});

test('requested changes block until the same reviewer approves', () => {
  assert.equal(mergeBlocker(pr, [changes('ravi'), approve('meera')], one), 'changes-requested');
  assert.equal(mergeBlocker(pr, [changes('ravi'), approve('meera'), approve('ravi')], one), null);
});

test('a later comment changes nothing', () => {
  assert.equal(mergeBlocker(pr, [approve('meera'), comment('meera')], one), null);
  assert.equal(mergeBlocker(pr, [changes('ravi'), comment('ravi'), approve('meera')], one), 'changes-requested');
});

test('stale approvals stop counting when the rule is on', () => {
  assert.equal(mergeBlocker(pr, [approve('meera', 'c1')], one), null);
  assert.equal(mergeBlocker(pr, [approve('meera', 'c1')], { ...one, dismissStale: true }), 'needs-approvals');
  assert.equal(mergeBlocker(pr, [approve('meera', 'c1'), approve('meera', 'c2')], { ...one, dismissStale: true }), null);
});

test('two approvals means two reviewers', () => {
  const two = { ...one, approvals: 2 };
  assert.equal(mergeBlocker(pr, [approve('meera'), approve('meera')], two), 'needs-approvals');
  assert.equal(mergeBlocker(pr, [approve('meera'), approve('ravi')], two), null);
});

test('a code owner must approve when the rule is on', () => {
  const owned = { ...pr, owners: ['ravi'] };
  const rule = { ...one, requireCodeOwner: true };
  assert.equal(mergeBlocker(owned, [approve('meera')], rule), 'needs-code-owner');
  assert.equal(mergeBlocker(owned, [approve('ravi')], rule), null);
  assert.equal(mergeBlocker(pr, [approve('meera')], rule), null);
});

Interview questions

Fresher · What is the difference between Comment, Approve and Request changes on a pull request review?

All three submit a review: the comments you wrote on lines of the diff, plus an overall message. Comment gives feedback without a verdict and never blocks anything. Approve says the change is ready to merge and counts toward the approvals a protected branch may require, as long as the reviewer has write access and is not the author. Request changes says the feedback must be addressed first. On a branch without a ruleset or branch protection rule it is only informational; when the branch requires pull request reviews, a change request from someone with write access blocks the merge until that same reviewer approves. Use Comment for questions and small notes, and Request changes only for problems that really must be fixed before merging.

Key takeaways

• Read in order: design, then behaviour and tests, then safety, readability and documentation.
• Tests that pass prove only what they test; try the sizes and values where the behaviour changes.
• Comments are specific, explain why, and say whether they block; label the rest Nit:, Optional: or FYI:.
• Request changes blocks a merge only on a branch whose rules require reviews, and then until the same reviewer approves.
• Run a pull request before approving it: gh pr checkout 21, or git fetch origin pull/21/head:pr-21.
• In CODEOWNERS the last matching line wins, paths are case sensitive, and the base branch’s file counts.
• Answer review with git commit --fixup, fold fixups in with git rebase --autosquash, and show the rewrite with git range-diff.
• Keep stacked pull requests aligned with git rebase --update-refs.