Pull Requests and Code Review
Before you start
You need: branching and conflicts (Articles 03–04).
Time: about 50 minutes, plus reviewing someone else's work.
Learning objective
Raise a reviewable pull request and give review feedback that is specific, useful and kind.
Topics
- What a pull request is for
- Preparing a branch
- Writing the description
- Size and scope
- Reviewing someone else's code
- Responding to feedback
- Merge strategies
- Protected branches
What a pull request is for
A pull request proposes merging one branch into another and opens it for discussion first.
It does four things:
| Purpose | Value |
|---|---|
| Catches defects | A second reader finds what the author cannot see |
| Spreads knowledge | Two people now understand the change |
| Records the decision | The discussion is searchable in eighteen months |
| Enforces standards | CI, tests and required approvals run before merge |
The second one is often the most valuable long-term. A team where only one person understands the fee module has a problem that review quietly fixes.
Terminology: GitHub and Azure DevOps say pull request; GitLab says merge request. Same thing.
Preparing the branch
Before you open it:
git fetch origin
git rebase origin/main # or: git merge origin/main
dotnet build
dotnet test
git log --oneline main..HEAD # read your own commits
git diff main...HEAD # read your own diff
Read your own diff first. You will find a debug line, a commented-out block, or a file you did not mean to include. Finding it yourself costs nothing; a reviewer finding it costs a round trip.
Updating from main before opening matters because a PR that conflicts cannot be merged, and the reviewer cannot see clean changes.
git push -u origin feature/student-search
The description
## What
Adds student search by name and roll number to the students list page.
## Why
The office staff currently scroll through 800 students to find one.
Reported in #142.
## How
- New GET /api/students/search endpoint, filtered by SchoolId from the token
- Debounced search input on the students page (300 ms)
- Search covers Name and RollNumber, case-insensitive
## Testing
- Unit tests for the repository query, including empty and partial matches
- Manual: searched "Ravi", "NCA-2024", and a string with no matches
- Verified a user from School A cannot find School B's students
## Notes
Search is not paginated yet — the result set is capped at 50.
Pagination is #148.
Fixes #142
The reviewer sees the diff. They cannot see the reasoning. "Why" is the section that makes review fast — without it, the reviewer reconstructs your intent from the code and reviews against a guess.
"Testing" changes the review's nature. "I checked cross-school isolation" tells the reviewer that the multi-tenant risk was considered; its absence means they must check it themselves.
"Notes" pre-empts the obvious question. Saying pagination is deliberately deferred saves a comment, a reply and a re-review.
Size
| Lines changed | Review quality |
|---|---|
| Under 200 | Careful, line by line |
| 200–400 | Reasonable |
| 400–1000 | Skimmed |
| Over 1000 | Approved without real review |
A 2,000-line pull request does not get reviewed. It gets approved. Everyone knows this and it happens anyway.
Splitting up:
- Refactoring in one PR, behaviour change in another
- Formatting in its own PR, always
- Database migration, then the code using it
- Backend endpoint, then frontend consumption
Separating refactor from behaviour is the highest-value split. Mixed together, the reviewer cannot tell which of 600 changed lines actually alters what the program does.
Reviewing
What to look for, in order:
Correctness. Does it do what the description says? Edge cases — null, empty, zero, absent, the maximum? For the School system specifically: is an absent student handled before the marks comparison, and does SchoolId come from the token rather than the request?
Security. Parameterised queries? Authorisation checked server-side, not just hidden in the UI? Any secret in the diff?
Tests. Do they cover the new behaviour, or only the happy path? Would they fail if the change were reverted?
Readability. Will this be understandable in a year? Are the names accurate?
Consistency. Does it match how the rest of the codebase does this?
Comments that work
Bad: This is wrong.
Good: If MarksObtained is null for an absent student, this comparison
throws. Should we check IsAbsent before comparing?
Bad: Bad naming.
Good: `d` is hard to follow three lines down — would `dueDate` work?
Bad: Why didn't you use a stored procedure?
Good: Have you considered a stored procedure here? The same join
appears in FeeReportService, so it might be worth sharing.
Bad: You forgot the SchoolId filter.
Good: This query doesn't filter by SchoolId — I think a user from
School A could read School B's students here. Worth confirming?
Ask questions rather than issuing verdicts. You may have missed context, and a question lets the author explain without either of you losing ground. When you are right, the question still gets the fix.
Comment on the code, not the person. "This method does X" not "you always do X".
Distinguish blocking from optional:
Blocking: SQL injection risk — this concatenates user input into the query.
Nit: Extra blank line here.
Question: Is the 50-result cap deliberate?
Praise: Nice — this handles the absent case properly.
Prefix nits explicitly. An unprefixed style preference reads as a required change, and the author spends an hour on something you did not care about.
Say what is good. Review that only ever finds fault makes people defensive, and defensive authors argue instead of fixing.
Review promptly. A PR sitting for three days blocks the author, goes stale against main, and gets worse to merge. Same day, ideally.
Responding
# make the changes
git add -p
git commit -m "Filter students by SchoolId from the token"
git push
Reply to every comment. Fixed, or why not. Silence looks like the comment was ignored.
Good: Fixed in 8c2e4f1.
Good: Good catch — added a test for the absent case too.
Good: I kept the loop here because the LINQ version needs two passes
over 800 rows. Happy to change it if you'd rather.
Bad: (resolves the thread with no reply)
Disagreement is legitimate. Explain the reasoning; if it is still contested, ask a third person rather than trading comments.
Push new commits rather than amending during review. A reviewer can then read just what changed since their last look. Squash on merge if the team prefers a clean history — that is a merge-time decision, not a review-time one.
Do not take it personally. Review criticises code. Everyone's code gets comments, including the most senior person on the team.
Merge strategies
| Strategy | Result | Use when |
|---|---|---|
| Merge commit | Full history plus a merge commit | You want the branch visible as a unit |
| Squash and merge | One commit on main | The branch has messy work-in-progress commits |
| Rebase and merge | Commits replayed, linear, no merge commit | Every commit is clean and worth keeping |
Squash is the common default because most feature branches contain commits like "wip", "fix typo" and "actually fix it" that nobody needs on main.
Squash discards the individual commits. If they were carefully separated — refactor, then behaviour, then tests — squashing throws away work that would have helped a future git bisect.
After merging:
git switch main
git pull
git branch -d feature/student-search
git push origin --delete feature/student-search
git fetch --prune
Protected branches
Typical settings on main:
- Require a pull request — no direct pushes
- Require at least one approval
- Require CI to pass
- Require the branch to be up to date
- Block force pushes and deletion
- Dismiss approvals when new commits are pushed
These rules exist because "just this once" is how untested code reaches production. They apply to everyone including whoever configured them, and the inconvenience is the point.
Diagnosing
| Symptom | Cause |
|---|---|
| PR shows unrelated files | Branched from the wrong place, or main not merged in |
| "This branch has conflicts" | main moved — merge or rebase, then push |
| CI fails but it works locally | Environment difference, or an uncommitted file |
| Every file shows as changed | Line endings or a reformat |
| The reviewer cannot follow it | Too large — split it |
| Approvals were dismissed | New commits pushed after approval, by policy |
Errors you will hit
| What you see | Cause | Fix |
|---|---|---|
| PR shows unrelated files | Branched from the wrong place | Rebase onto the target branch |
| "This branch has conflicts" | The target moved | Merge or rebase, then push |
| CI fails but it works locally | Environment difference, or an uncommitted file | Check what CI ran |
| Approvals disappeared | New commits pushed, by policy | Expected |
| The reviewer cannot follow it | Too large | Split it |
A 2,000-line pull request does not get reviewed — it gets approved. Everyone knows this, and it happens anyway.
Common mistakes
- Opening a PR without reading your own diff
- No "why" in the description
- 2,000-line pull requests
- Refactoring and behaviour changes in one PR
- Reformatting mixed with real changes
- Review comments as verdicts rather than questions
- Not marking nits as nits
- Resolving comments without replying
- Squashing away carefully separated commits
- Merging without CI passing
- Leaving PRs unreviewed for days
Practice
The course exercise is raise and review a pull request.
- Create a branch, make a small change, and open a PR with all five description sections.
- Read
git diff main...HEADbefore opening. Note anything you would not want a reviewer to see. - Deliberately include a
Console.WriteLineand catch it in your own review. - Open a PR mixing a reformat with a one-line fix. Try to review it. Then split it and compare.
- Review a colleague's PR — or an open-source one — and write one blocking comment, one question and one nit, each labelled.
- Rewrite three harsh comments as questions.
- Respond to review feedback with new commits, and reply to every thread.
- Push a commit after approval on a repository that dismisses stale approvals. Observe what happens.
- Merge the same branch three ways in three copies — merge commit, squash, rebase — and compare
git log --oneline --graph. - Configure branch protection on a test repository and try to push directly to
main. - Let a branch go stale, then update it from
mainand push. - Review a PR against the checklist: absent handled first,
SchoolIdfrom the token, parameterised queries, tests covering the new path.
Exercise 12 is the one worth turning into a written checklist for your own team.
You can now
- Prepare a branch and read your own diff first
- Write a description that explains why
- Keep a pull request small enough to review
- Give feedback that is specific and labelled
- Respond to every comment
Review questions
- Why does pull request size affect review quality so sharply?
- What does the "why" section give a reviewer that the diff cannot?
- Why should nits be labelled as nits?
- What does squash merging discard, and when does that matter?
Next: Team workflows