Conference Presentation
How Jane Street Does Code Review
- Jane Street developers anticipate that concepts presented may inspire external tool development after the speaker's departure and anticipate initial resistance described as "positive insanity" that could lead to alternative implementations.
- The workflow treats branches as the atomic unit of review rather than individual commits, utilizing merge operations colloquially called rebasing instead of true rebasing due to Mercurial's constraints, which simplifies reviews but complicates bisecting.
- Future plans involve using "traits" to tag buggy commits so ancestors are recognized as infected, reverting features via reverse diffs or new commits rather than history rewriting, and handling nested branches through shell scripts or naming conventions.
- Developers expect that focusing on small branches enables frequent, unpolished commits without concern for message quality or squashing, while locking features prevents merging critical code until locks are removed to avoid unverified releases.
- Code review obligations extend to non-code artifacts like configuration files, blog posts, and trading limits, with a centralized version control system enforcing actions such as withholding releases with pending comments.
- A "to-do" dashboard is expected to provide a flat listing of features and review obligations to prevent forgotten changes, while the "CR soon" mechanism assigns lingering items to file owners after merging to ensure eventual resolution.
- Inline commenting systems are predicted to initially cause physical pain but will eventually foster a shared ownership environment by allowing direct fixes for typos or variable names without back-and-forth exchanges.
- The "brain" mechanism ensures reviewers do not re-read commits after rebasing by displaying only changes since their last review state, while the "diff of diffs" approach is expected to be trivial for humans to read except during merge conflicts.
- Developers plan to use "workspaces" to maintain multiple branch checkouts simultaneously without interrupting build servers and suggest integrating future Markdown talks into the code review system.
- The "file owner" role is primarily defined by managing "CR soon" items for finished reviews with lingering comments, and merging branches is the standard practice despite colloquial references to rebasing.