Pull Requests and Code Review: Git Collaboration Best Practices
Master pull request workflows and code review — writing effective PR descriptions, review best practices, collaboration patterns, and team workflows.
Introduction
Pull requests (or merge requests in GitLab) are the primary mechanism for collaborative code integration in modern development. They transform Git from a version control system into a collaboration platform, enabling discussion, review, and quality gates before code reaches the main branch.
A well-crafted pull request respects the reviewer’s time, clearly communicates intent, and makes review effortless. A poorly crafted one creates friction, delays integration, and breeds resentment. The difference lies in discipline, empathy, and understanding that code review is a communication tool as much as a quality gate.
This guide covers the full PR lifecycle — from writing descriptions that reviewers appreciate, through effective review practices, to team workflows that make collaboration smooth and productive.
When to Use / When Not to Use
When to Use Pull Requests
- Feature integration — any significant change deserves review before merging
- Bug fixes — even small fixes benefit from a second pair of eyes
- Refactoring — structural changes need careful review to avoid regressions
- Knowledge sharing — PRs are a primary mechanism for team learning
- Compliance — regulated environments require documented review trails
When Not to Use Pull Requests
- Trivial typo fixes — commit directly if your team agrees on the threshold
- Emergency hotfixes — fix first, PR after (but still create the PR)
- Automated changes — bot-generated PRs should be minimal and auto-mergeable
- Personal experiments — keep exploratory work in local branches
Core Concepts
A pull request is a request to merge changes from one branch into another. It includes a diff, a description, discussion threads, review approvals, and CI/CD status checks. The PR is the unit of collaborative code change.
graph TD
Dev["Developer"] -->|Push branch| Remote["Remote Repository"]
Remote -->|Create| PR["Pull Request"]
PR -->|Notify| Reviewers["Reviewers"]
Reviewers -->|Review| PR
PR -->|CI Checks| CI["CI/CD Pipeline"]
CI -->|Pass/Fail| PR
PR -->|Approve| Approved["Approved"]
Approved -->|Merge| Main["Main Branch"]
PR -->|Request Changes| Dev
Dev -->|Update| PR
Architecture or Flow Diagram
flowchart TD
A["Complete feature branch"] --> Rebase["Rebase onto latest main"]
Rebase --> Tests["Run tests locally"]
Tests --> Push["git push origin feature-branch"]
Push --> Create["Create Pull Request"]
Create --> Describe["Write description:\nWhat, Why, How"]
Describe --> Link["Link to issue/ticket"]
Link --> Request["Request reviewers"]
Request --> CI["CI runs automatically"]
CI --> Check{"All checks pass?"}
Check -->|No| Fix["Fix failures,\nupdate PR"]
Check -->|Yes| Review["Team reviews code"]
Fix --> CI
Review --> Feedback{"Feedback?"}
Feedback -->|Changes requested| DevFix["Address feedback,\npush updates"]
Feedback -->|Approved| Merge["Merge PR"]
DevFix --> Review
Merge --> Cleanup["Delete branch"]
Cleanup --> Done["Feature integrated"]
Step-by-Step Guide / Deep Dive
Creating an Effective PR
# Before creating the PR
git switch main
git pull origin main
git switch feature-branch
git rebase main # Get current
git push --force-with-lease # Update remote branch
# Create PR via CLI (GitHub)
gh pr create \
--base main \
--head feature-branch \
--title "feat: add user authentication with JWT" \
--body-file PR_DESCRIPTION.md
PR Description Template
## What
Brief summary of what this PR does.
## Why
Business or technical rationale for the change.
Links to issue: #123
## How
Key implementation decisions and approach.
## Testing
- [ ] Unit tests pass
- [ ] Integration tests pass
- [ ] Manual testing completed
- [ ] Edge cases considered
## Screenshots (if UI)
Before/after comparison.
## Risks
Any potential risks or areas needing extra review attention.
Code Review Checklist
## For Reviewers
### Reference: Correctness
### Reference: Maintainability
#### Correctness
- [ ] Does the code do what it claims?
- [ ] Are edge cases handled?
- [ ] Are there off-by-one errors?
#### Security
- [ ] Is user input validated/sanitized?
- [ ] Are secrets handled properly?
- [ ] Is authentication/authorization correct?
#### Performance
- [ ] Are there N+1 queries?
- [ ] Is caching used appropriately?
- [ ] Are there memory leaks?
#### Maintainability
- [ ] Is the code readable?
- [ ] Are functions/methods focused?
- [ ] Is there adequate documentation?
#### Testing
- [ ] Are there tests for new functionality?
- [ ] Do tests cover edge cases?
- [ ] Are tests readable and maintainable?
Review Response Workflow
# After receiving review feedback
git switch feature-branch
# Make changes
git add .
git commit -m "fix: address review comments - validate email format"
git push origin feature-branch
# Or amend if the PR hasn't been reviewed yet
git commit --amend
git push --force-with-lease origin feature-branch
Production Failure Scenarios
| Scenario | Impact | Mitigation |
|---|---|---|
| Merging without review | Bugs reach production | Require minimum approvals in branch protection |
| Rubber-stamp reviews | Defects slip through | Rotate reviewers; require substantive comments |
| Large PRs (500+ lines) | Reviewers skim, miss issues | Limit PRs to 400 lines; split large changes |
| Stale PRs (weeks old) | Conflicts accumulate, context lost | Review within 24 hours; rebase regularly |
| Merging failing CI | Broken main branch | Block merge on CI status checks |
Branch Protection Rules
# GitHub CLI: Set up branch protection
gh api repos/{owner}/{repo}/branches/main/protection \
--method PUT \
--field required_pull_request_reviews='{"required_approving_review_count":2}' \
--field required_status_checks='{"strict":true,"contexts":["ci/test"]}' \
--field enforce_admins=true \
--field restrictions=null
Trade-off Analysis
| Approach | Pros | Cons |
|---|---|---|
| Required reviews | Quality gate, knowledge sharing | Slower integration, bottlenecks |
| Single approval | Fast, lightweight | Less thorough review |
| Self-merge | Fastest, trusted teams | No quality gate |
| Squash merge | Clean history | Loses individual commit context |
| Merge commit | Preserves full history | Noisier git log |
| Rebase merge | Linear history | Can hide integration complexity |
Implementation Snippets
# GitHub: Create PR with labels and assignees
gh pr create \
--title "feat: add rate limiting to API" \
--body "Implements token bucket algorithm for API rate limiting" \
--label "enhancement" \
--assignee @me \
--reviewer senior-dev,security-team
# GitLab: Create merge request
glab mr create \
--source-branch feature/rate-limiting \
--target-branch main \
--title "feat: add rate limiting" \
--description "Implements rate limiting using token bucket" \
--assignee @me \
--reviewer @team-lead
# Review PR locally
gh pr checkout 123
# Review code, then:
gh pr review 123 --approve
# or:
gh pr review 123 --comment "Please add tests for edge case"
Observability Checklist
- Logs: Record PR creation, review, and merge events
- Metrics: Track PR size, review time, and approval rate
- Alerts: Alert on stale PRs (>7 days without review)
- Traces: Link PRs to deployed versions and incidents
- Dashboards: Display team review throughput and bottlenecks
Security & Compliance Considerations
- Require minimum number of approvals for production branches
- Block merges when CI security scans fail
- Document review decisions for audit compliance
- Use CODEOWNERS files to auto-assign security-sensitive reviews
- Never bypass branch protection rules, even for emergencies
Common Pitfalls / Anti-Patterns
- Mega PRs — changesets over 400 lines get superficial reviews
- Description-less PRs — reviewers shouldn’t have to read code to understand intent
- Ignoring review feedback — address every comment, even to explain why you disagree
- Reviewing your own PR — get a second pair of eyes, always
- Merging without waiting for CI — automated checks catch what humans miss
- Leaving branches after merge — delete merged branches to keep the repo clean
Quick Recap Checklist
- Rebase onto main before creating PR
- Write a clear PR description with What, Why, How
- Link PR to issue/ticket
- Request appropriate reviewers
- Wait for CI checks to pass
- Address all review feedback
- Require minimum approvals before merge
- Delete branch after merge
PR Lifecycle Architecture
flowchart TD
Dev["Developer pushes branch"] --> Create["Create PR/MR"]
Create --> AutoChecks["Automated Checks\nCI, lint, tests, security scan"]
AutoChecks --> Pass{"All checks pass?"}
Pass -->|No| Fix["Fix failures,\nupdate PR"]
Pass -->|Yes| Review["Human Code Review"]
Fix --> AutoChecks
Review --> Feedback{"Feedback?"}
Feedback -->|Changes requested| DevFix["Address feedback,\npush updates"]
Feedback -->|Approved| Approve["Approved (min reviewers)"]
DevFix --> Review
Approve --> MergeType{"Merge strategy?"}
MergeType -->|Squash| Squash["Squash merge\nSingle commit"]
MergeType -->|Rebase| RbMerge["Rebase merge\nLinear history"]
MergeType -->|Merge commit| McMerge["Merge commit\nPreserves history"]
Squash --> Cleanup["Delete source branch"]
RbMerge --> Cleanup
McMerge --> Cleanup
Cleanup --> Deploy["Deploy to production"]
classDef stage color:#00fff9
class Dev,Create,AutoChecks,Review,Approve,Squash,RbMerge,McMerge,Cleanup,Deploy stage
Production Failure: Merging Unreviewed PRs
Scenario: A team member marks their own PR as approved to bypass the review requirement. The PR contains a subtle SQL injection vulnerability that a reviewer would have caught. CI passes because the test suite doesn’t cover this input path. The PR is merged and deployed.
Impact: Security vulnerability in production. The review process — designed as a quality gate — was bypassed, allowing unvetted code to reach users.
Mitigation:
- Require minimum approvals from different people (not the author)
- Enable CODEOWNERS to auto-assign appropriate reviewers
- Block self-approval in platform settings
- Require CI to pass before merge is allowed
- Prevent merging stale branches — require rebase onto latest main
# GitHub: Enforce review requirements
gh api repos/{owner}/{repo}/branches/main/protection \
--method PUT \
--field required_pull_request_reviews='{
"required_approving_review_count": 2,
"dismiss_stale_reviews": true,
"require_code_owner_reviews": true,
"require_last_push_approval": true
}'
# CODEOWNERS file (.github/CODEOWNERS)
# Auto-assign reviewers based on file paths
*.py @backend-team
*.ts @frontend-team
src/auth/ @security-team
Trade-offs: PR Merge Strategies
| Strategy | History Impact | Attribution | Revert Ease | Best For |
|---|---|---|---|---|
| Squash merge | Single commit on main, PR branch history lost | Original author preserved, individual commits lost | Easy — single commit revert | Small PRs, feature branches with WIP commits |
| Rebase merge | Linear history, all commits replayed | Individual commits preserved with original authors | Easy — revert individual commits | Teams wanting clean history with full attribution |
| Merge commit | Non-linear, merge commit records integration point | Full history preserved, including all commits | Easy — revert merge commit (all changes at once) | Large features, audit compliance, release branches |
Observability Checklist: PR Metrics
- Review time: Average time from PR creation to merge (target: <24 hours)
- Comment density: Comments per 100 lines of code (healthy: 2-5 per 100 lines)
- CI pass rate: Percentage of PRs passing CI on first attempt (target: >80%)
- PR size distribution: Track lines changed per PR (healthy: <400 lines)
- Stale PR count: PRs open >7 days without activity (target: <5% of total)
- Review turnaround: Time from review request to first comment (target: <4 hours)
- Revert rate: Percentage of merged PRs that are later reverted (target: <2%)
# GitHub CLI: PR metrics
gh pr list --state merged --json mergedAt,createdAt,additions,deletions \
| jq '[.[] | {
review_time: ((.mergedAt | fromdateiso8601) - (.createdAt | fromdateiso8601)) / 3600,
lines_changed: (.additions + .deletions)
}]'
Interview Questions
A good PR description answers what changed, why it changed, and how it was implemented. It links to the relevant issue, describes testing performed, notes any risks, and provides context that isn't obvious from the diff alone. The goal is to make the reviewer's job as easy as possible.
Research shows that review effectiveness drops significantly after 200-400 lines. Reviewers start skimming, missing defects, and providing superficial feedback. Small PRs get faster, more thorough reviews and are easier to revert if problems are discovered after merge.
Squash merge combines all PR commits into one, creating a clean history but losing individual commit context. Regular merge creates a merge commit preserving all commits and the branch structure. Rebase merge replays commits onto the target branch for a linear history. Choose based on your team's history preferences.
Respond respectfully with reasoning. Explain your perspective with evidence — benchmarks, documentation, or design principles. If consensus isn't reached, escalate to a tech lead or team discussion. Never ignore feedback or merge over objections. The goal is the best code, not winning an argument.
Force-with-lease (--force-with-lease) is a safer variant of force push that refuses to overwrite changes if there are commits you haven't seen. It checks that the remote ref matches your local ref before pushing — if someone else pushed changes, the push fails. This prevents accidentally overwriting work others have pushed. Always prefer this over standard --force.
An effective PR title follows conventional commit format (feat:, fix:, docs:, refactor:, etc.), briefly describes the change, and stays under 72 characters. It should make sense when read in the git log. Bad: "fix bug". Good: "fix: resolve null pointer in user authentication flow". The title should stand alone — reviewers often browse PR titles to find specific changes.
Merge creates a merge commit that preserves the complete branch history and shows when feature work diverged and merged. Rebase replays your commits on top of the target branch, creating a linear history. Rebase is preferred for keeping git history clean, but it rewrites commit history — never rebase commits that have been pushed and shared with others.
Structure comments by starting with what you understood (to confirm alignment), then state your concern clearly, and suggest an alternative. Use prefixes like "nit:" for minor style issues, "question:" for seeking understanding, and "blocker:" for must-fix issues. Keep comments actionable — "this could cause issues" is less useful than "this may cause N+1 queries on large datasets — consider adding an index or batch fetch."
CODEOWNERS is a file that maps file paths or patterns to specific teams or individuals. When a PR touches those files, the owners are automatically requested for review. This ensures that security-sensitive code (auth/, payment/), critical infrastructure, or domain-specific files always get reviewed by the right people — even if the author forgets to request them.
Key PR health metrics include: review time (time from PR open to merge — target under 24 hours), PR size (lines changed — keep under 400), CI pass rate (first-attempt pass percentage — target above 80%), comment density (comments per 100 lines — healthy range 2-5), stale PR percentage (PRs over 7 days old — keep under 5%), and revert rate (percentage of merged PRs later reverted — target under 2%).
Use a draft PR when your changes are work-in-progress and not ready for formal review. Draft PRs don't require reviewers, can't be merged (unless explicitly enabled), and signal that the code is not production-ready. Use regular PRs when you're asking for review and think the code is ready to merge. Some teams use draft PRs for early feedback, architectural discussion, or CI status sharing before the work is complete.
Requiring rebase before merge ensures the PR is based on the latest code in the target branch, preventing "merge hell" where the PR is days or weeks stale. It enforces that the PR author has incorporated recent changes, resolved any conflicts locally, and verified the code works with the latest main. This reduces integration surprises and makes the git history cleaner.
Break it into smaller PRs using one of these strategies: vertical slices (split by feature end-to-end rather than layer-by-layer), stacked PRs (create dependent PRs where each builds on the previous), or changesets (split into refactor-only PR and feature-only PR). If it can't be split (e.g., a large migration), add a detailed review guide, record a walkthrough video, or designate a "focused reviewer" for specific sections.
Blocking comments indicate issues that must be resolved before approval — bugs, security vulnerabilities, broken logic. Non-blocking comments (often prefixed with "nit:") suggest improvements that are nice-to-have but don't prevent merge — style preferences, naming suggestions, minor improvements. Reviewers should clearly distinguish between them so authors don't spend time on optional changes or feel compelled to address every comment.
A stacked PR is a sequence of dependent PRs where each PR builds on the previous one. For example: PR1 refactors the database layer, PR2 adds new API endpoints using that layer, PR3 implements the feature using those endpoints. Each PR can be reviewed and merged independently. Use stacked PRs for large features that span multiple subsystems — they keep each PR small and reviewable while maintaining a logical progression.
Use the platform's resolution system — GitHub/GitLab marks threads as resolved when the author addresses them, and reviewers must re-open if not satisfied. Set a team norm that unresolved threads block merge. For high-priority comments, request re-review explicitly. Track outstanding comments in the PR description or use a shared review dashboard. Some teams add a bot that summarizes unresolved comments before merge.
For changes spanning multiple repositories, use stacked PRs or dependency PRs. Create PRs in dependency order — merge the library PR first, then update the dependent service. Use feature flags or configuration to switch between old and new implementations during transition. Alternatively, coordinate a simultaneous merge across all repos and communicate the timeline to the team.
Approved means the reviewer has signed off and the PR can be merged once other conditions (CI, reviews) are met. Changes requested indicates the reviewer has blocking concerns that must be addressed before merging. Some platforms distinguish between "request changes" (blocking) and "comment only" (non-blocking). Always resolve all requested changes before merging.
Optimize for fast reviews by: keeping PRs small (under 400 lines), writing clear descriptions with context, adding screenshots or recordings for UI changes, including test instructions, linking related issues, responding to feedback promptly, and requesting reviewers strategically based on code ownership. Draft PRs for early feedback reduce later-stage review cycles.
Best practices include: requiring minimum approvals before merge, enabling branch protection on main branches, squashing or rebasing feature branches before merge for clean history, running CI checks before merge, deleting branches after merge to reduce clutter, using merge queues for high-volume repos, and establishing team norms for review response time (e.g., first response within 4 hours).
Further Reading
Additional Resources
- GitHub Pull Request Documentation — Official PR workflow guide
- Google Engineering Practices: Code Review — Google’s code review guidelines
- Thoughtbot Code Review Guide — Team-based review patterns
- GitLab Merge Request Guidelines — MR workflow best practices
Related Topics
- Atomic Commits — Writing commits that tell a story
- Git Branching Strategies — GitFlow, trunk-based development, and release flows
- Effective Commit Messages — Subject lines, body text, and versioning
Tools
- GitHub CLI — Command-line PR management
- GitLab CLI (glab) — GitLab MR from terminal
- Husky — Git hooks for CI integration
- Commitlint — Enforce conventional commit formats
Conclusion
PRs are where version control meets human collaboration. The technical mechanics — feature branches, rebasing, merge strategies — exist to support the social process of reviewing and discussing code before it ships. A great PR workflow combines solid Git practices with clear communication and constructive feedback.
Category
Related Posts
Git Blame and Annotate: Line-by-Line Code Attribution
Master git blame for line-by-line code attribution, understanding code history, finding when code changed, and using blame effectively for code comprehension.
Git Merge and Merge Strategies Explained
Deep dive into Git merge strategies — fast-forward, three-way, recursive, ours, subtree. Learn when each strategy applies and how to control merge behavior.
Git Remote Management: Adding, Removing, and Configuring Remotes
Master git remote operations — adding, removing, renaming remotes, managing multiple remotes, and configuring remote URLs for effective collaboration.