Conference Presentation
How Jane Street Does Code Review
- Core Philosophy: Jane Street's code review tool, "Iron," is a custom in-house system based on Mercurial designed to prioritize mechanical reading of code and branch-level state over traditional commit-centric workflows.
- Design Constraints: The tool was built to satisfy specific constraints that existing web-based, Git-like tools fail to handle seamlessly:
- Reviewing merge commits without re-reading upstream changes.
- Maintaining review state and comments through rebases (even though Jane Street practices "merge-as-rebase" to avoid history rewriting).
- Ensuring reviewers never need to re-read code that hasn't actually changed since their last review.
- Allowing changes to a branch during review without invalidating previous reviewer consensus.
- The "Brain" Mechanism:
- A "brain" is a mutable diff state associated with a specific person-branch pair, tracking exactly what code a reviewer has processed.
- Reviewing a commit applies that commit's diff to the reviewer's brain; the system only presents the "diff of diffs" (the delta between the reviewer's current brain and the branch state) when updates occur.
- This approach naturally handles rebases and merges by showing only the new changes relative to the reviewer's known state, eliminating the need to re-review entire branches.
- Inline Comments as Code:
- Feedback is added by checking out the branch, inserting special syntax comments directly into the source code (e.g.,
CR <author> <target>), and pushing them back to the branch. - This preserves context across rebases because comments are part of the branch history, avoiding the "lost comment" problem common in web-based tools.
- The Emacs client minimizes context switching, allowing reviewers to jump from a diff view directly to the code buffer to add comments or fixes.
- Reviewers can directly modify the branch to fix typos or improvements, fostering a collaborative environment where code is treated as a shared asset rather than a static deliverable.
- Feedback is added by checking out the branch, inserting special syntax comments directly into the source code (e.g.,
- Review Obligations and Ownership:
- Feature Reviewers: Review the entire branch before signing off.
- File Reviewers/Owners: Have permanent obligations to review changes to specific files or directories, declared via metadata files (S-expressions) in the repository.
- CR Soon: Unresolved comments on merged code are automatically reassigned to file owners, who are responsible for addressing them eventually.
- Nested Branches:
- Jane Street frequently uses nested branches (e.g.,
monad-trans->testing-dsl->dsl-features) where dependent features branch off one another rather than a single trunk. - These are managed as atomic units of review; a dependent feature is reviewed only after its parent branch is accepted.
- This approach blurs the line between commits and branches, treating the entire branch sequence as the reviewable unit.
- Jane Street frequently uses nested branches (e.g.,
- Workspaces and Client Features:
- Workspaces: Unlike Git worktrees, Jane Street maintains multiple dirty checkouts of different branches simultaneously in a centralized environment, allowing parallel builds and eliminating the need to stash changes when switching context.
- To-Do Dashboard: An Emacs-based interface aggregates review status, dirty workspaces, pending rebases, and obligations into a single view, acting as a personal task manager for development workflows.
- Asynchronous Operations: Rebases and other complex operations run in the background, with failures or conflicts surfaced in the to-do list rather than blocking the user's terminal.
- Centralized Safety and Locks:
- The system is centralized, allowing for enforced policies that prevent merging or releasing features before all review obligations are met.
- "Locks" are used to prevent specific operations (like releasing a feature or rebasing) to avoid introducing critical bugs or breaking review states for other developers.
- Commit Philosophy:
- Commits are treated as ephemeral implementation steps rather than a permanent historical record; developers frequently commit small, messy changes without squashing or writing descriptive messages.
- The repository history is intentionally "ugly" with many merge commits, but this is irrelevant to the review process which operates on branch states.
- Extension of Review to Non-Code:
- The Iron tool is used to review configuration files, trading limits, and blog posts, applying the same obligation and review workflows to ensure high-fidelity changes across the organization.
- Q&A Insights:
- Mercurial Influence: The tool's "merge-only" philosophy was partly influenced by Mercurial's historical stance against rewriting history, though the tool reimplements missing features like worktrees.
- Debugging Bisect: Due to the lack of clean history (constant merging), standard
git bisectis difficult; the team relies on rolling back deployed executables and using "traits" (tags marking bugs) to track which revisions are affected. - File Ownership vs. Collaborative Editing: File ownership does not restrict editing but ensures that unresolved comments ("CR Soon") are assigned to the person responsible for the file's long-term integrity.