Pull Requests

Intuition

A formal code review process: Pull requests are like submitting a proposal to a committee — you present your changes, colleagues review them, discuss improvements, and only then does the change get approved and merged. It turns solo coding into a team activity.

Why it matters: PRs are the gatekeeper of code quality. They catch bugs before they reach production, spread knowledge across the team, and create a permanent record of why changes were made.

The key insight: Good PR descriptions save hours of review time — explaining what changed, why it changed, and how to test it transforms a review from guesswork into a focused discussion.

What is a Pull Request

A pull request (PR) — called a merge request in GitLab — is a proposal to merge a branch into another branch, accompanied by a code review discussion. While git merge is a local operation, a pull request is a platform feature (GitHub, GitLab, Gitea) that adds:

A web interface for reviewing diffs.
Discussion threads on specific lines of code.
Automated CI checks.
Approval workflows.
Audit trail of who reviewed and when.

Anatomy of a Good Pull Request

Title

The title should be a concise summary of the change:

feat(auth): add JWT token refresh mechanism
fix(parser): handle null input without segfault
refactor(api): extract validation into separate module
docs(readme): update installation instructions for macOS

Description

A good PR description answers:

What does this PR change?
Why is this change needed? (Context, motivation, issue reference)
How was it implemented? (Design decisions, alternatives considered)
How to test? (Steps for the reviewer to verify)

Template:

## Summary

Brief description of the change.

## Motivation

Why this change is needed. Reference to issue: Closes #42.

## Changes

- Added JWT token refresh middleware
- Updated token expiry from 1h to 15min
- Added refresh token rotation

## Testing

1. Login with valid credentials
2. Wait for access token to expire (or reduce expiry for testing)
3. Verify that requests succeed (token was automatically refreshed)
4. Verify that refresh token was rotated

## Screenshots

(If applicable)

## Checklist

- [ ] Tests pass
- [ ] No linting errors
- [ ] Documentation updated
- [ ] Breaking changes documented

Size

Small PRs are easier to review, faster to merge, and safer to revert. Aim for:

Metric	Target	Maximum
Lines changed	< 200	< 400
Files changed	< 10	< 20
Review time	< 30 min	< 1 hour

If a PR exceeds these limits, consider splitting it into multiple smaller PRs.

Code Review

Reviewing PRs

When reviewing a PR, focus on:

Category	What to Check
Correctness	Does the code do what the PR claims? Are there edge cases?
Design	Does the change fit the existing architecture? Is the abstraction appropriate?
Readability	Is the code understandable? Are variable/function names clear?
Testing	Are there tests for the new behavior? Do existing tests still pass?
Security	Does the change introduce any vulnerabilities? (Input validation, authentication, etc.)
Performance	Does the change introduce any regressions? (N+1 queries, unnecessary copies, etc.)

Writing Review Comments

Be constructive and specific:

Bad	Good
”This is wrong"	"This condition is inverted — `x < 0` should be `x >= 0`"
"Use a better algorithm"	"A binary search would reduce this from $O(n)$ to $O(\log n)$ "
"This is hard to read"	"Extract this into a named function `is_valid_email` with a docstring”

Distinguish blocking from non-blocking comments:

[REQUIRED] This must be fixed before merging.

- The null check is missing -- this will crash if `user` is null.

[SUGGESTION] Consider this improvement (non-blocking).

- You could use `String::from_utf8_lossy` instead of `unsafe { String::from_utf8_unchecked }`.

[QUESTION] I"m not sure about this -- please clarify.

- Why is the timeout set to 30 seconds? Is this documented somewhere?

CI Integration

Required Status Checks

Configure GitHub to require certain checks before a PR can be merged:

Tests: npm test``cargo test``pytestEtc.
Linting: eslint``clang-tidy``flake8Etc.
Type checking: tsc --noEmit``mypyEtc.
Build: npm run build``cargo buildEtc.
Security: npm audit``snyk testEtc.

Branch Protection Rules

For the main branch, configure:

Rule	Purpose
Require PR before merging	All changes must be reviewed
Require approvals (1+)	At least one person must approve
Require status checks	All CI checks must pass
Require up-to-date branch	PR must be rebased on latest `main` before merging
Require signed commits	Commits must be GPG/SSH signed
Require linear history	No merge commits (force rebase)
Restrict who can push	Only authorized users can bypass PRs

Auto-Merge

After all checks pass and approvals are granted, enable auto-merge:

## Via GitHub CLI
$ gh pr merge 42 --auto --squash

Merging Strategies on Platforms

Strategy	Git Command	When to Use
Create a merge commit	`git merge --no-ff`	Preserves full history; good for large teams
Squash and merge	`git merge --squash`	Linear history; one commit per PR
Rebase and merge	`git rebase` + `git merge --ff-only`	Linear history; preserves individual commits

Squash and Merge: Implications

Squash and merge creates a single commit from all commits in the PR. This has consequences:

Pros: Clean, linear history; easy to git bisect; PR is a single unit.
Cons: Individual commit messages are lost; cannot revert a specific commit within the PR.

the only commit message for the entire change.

Reverting a PR

After a Merge Commit

$ git revert -m 1 <merge-commit-hash>

The -m 1 flag tells Git to keep the first parent (the branch you merged into), effectively undoing the entire feature.

After a Squash and Merge

$ git revert <squash-commit-hash>

Single commit — straightforward revert.

PR Etiquette

Self-review before requesting review. Fix obvious issues yourself.
Keep PRs small. Review under 400 lines of changes.
Respond promptly to review comments.
Push fixes to the same branch. Don’t open a new PR.
Don’t force-push after someone has reviewed your code.
Thank reviewers for their time and feedback.

Common Pitfalls

Forgetting edge cases in algorithm design (e.g., empty input, single element, already sorted data).
Neglecting to normalise database designs, leading to data redundancy and update anomalies.
Forgetting that $O(n \log n)$ average-case for quicksort becomes $O(n^2)$ worst-case on already sorted input.
Confusing an algorithm with a program. An algorithm is a step-by-step procedure, not its implementation in code.

Worked Examples

Worked examples demonstrating the application of key concepts are covered in the detailed sub-pages linked above.

---

Cross-References

Remote Operations provides the push and fetch mechanics that underpin pull request creation and merging.
Workflows defines the team conventions within which pull requests operate as the review mechanism.
Merging covers the merge strategies used when integrating pull request branches.

Mark this page as reviewed