diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4420058beefd..84160a07e3de 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -245,6 +245,13 @@ Every file type uses a variation on the same license text ("Apache-2.0 WITH LLVM-exception") with similar formatting. If you're not sure what text to use, please ask on Discourse Forums. +## Workflow + +Carbon repositories all follow a common +[pull-request workflow](docs/project/pull_request_workflow.md) for landing +changes. It is a trunk-based development model that emphasizes small, +incremental changes and preserves a simple linear history. + ## Acknowledgements Carbon's Contributing guidelines are based on diff --git a/docs/project/pull_request_workflow.md b/docs/project/pull_request_workflow.md new file mode 100644 index 000000000000..a87f0ee79dd8 --- /dev/null +++ b/docs/project/pull_request_workflow.md @@ -0,0 +1,116 @@ +# Trunk-based pull-request GitHub workflow + + + +Carbon repositories follow a few basic principles: + +- Development directly on the `trunk` branch and + [revert to green](#green-tests). +- Always use pull requests, rather than pushing directly. +- Changes should be small, incremental, and review-optimized. +- Preserve linear history by + [rebasing](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges#rebase-and-merge-your-pull-request-commits) + or + [squashing](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges#squash-and-merge-your-pull-request-commits) + pull requests rather than using unsquashed merge commits. + +These principles try to optimize for several different uses or activities with +version control: + +- Continuous integration and bisection to identify failures and revert to green. +- Code review both at the time of commit and follow-up review after commit. +- Understanding how things evolve over time, which can manifest in different + ways: + - When were things introduced? + - How does the main branch and project evolve over time? + - How was a bug or surprising thing introduced? + +Note that this isn't a complete guide to doing code reviews, and just focuses on +the mechanical workflow and branch management. TODO: Add an explicit link to +more detailed guidance on managing pull request based code reviews when it is +developed. + +## Trunk based development + +We work in a simple +[trunk-based development](https://trunkbaseddevelopment.com/) model. This means +all development activity takes place on a single common `trunk` branch in the +repository (our default branch). We focus on +[small, incremental changes](#small_incremental_changes) rather than feature +branches or the "scaled" variations of this workflow. + +### Green tests + +The `trunk` branch should always stay "green". That means that if tests fail or +if we discover bugs or errors, we revert to a "green" state by default, where +the failure or bug is no longer present. Fixing forward is fine if that will be +comparably fast and efficient. The goal isn't to dogmatically avoid fixing +forward, but to prioritize getting back to green quickly. We hope to eventually +tool this through automatic continuous-integration powered submit queues, but +even those can fail and the principle remains. + +## Always use pull requests (with review) rather than pushing directly + +We want to ensure that changes to Carbon are always reviewed, and the simplest +way to do this is to consistently follow a pull request workflow. Even if the +change seems trivial, still go through a pull request -- it'll likely be trivial +to review. Always wait for someone else to review your pull request rather than +just merging it, even if you have permission to do so. + +Our GitHub repos are configured to require pull requests and review before they +are merged, so this rule is enforced automatically. + +## Small, incremental changes + +Developing in small, incremental changes improves code review time, continuous +integration, and bisection. This means we typically squash pull requests into a +single commit when landing. We use two fundamental guides for deciding how to +split up pull requests: + +1. Ensure that each pull request builds and passes any tests cleanly when you + request review and when it lands. This will ensure bisection and continuous + integration can effectively process them. + +2. Without violating the first point, try to get each pull request to be "just + right": not too big, not too small. You don't want to separate a pattern of + tightly related changes into separate requests when they're easier to review + as a set or batch, and you don't want to bundle unrelated changes together. + Typically you should try to keep the pull request as small as you can without + breaking apart tightly coupled changes. However, listen to your code reviewer + if they ask to split things up or combine them. + +While the default is to squash pull requests into a single commit, _during_ the +review you typically want to leave the development history undisturbed until the +end so that comments on any particular increment aren't lost. We typically use +the GitHub squash-and-merge functionality to land things. + +### Managing pull requests with multiple commits + +Sometimes, it will make sense to _land_ a series of separate commits for a +single pull request through rebasing. This can happen when there is important +overarching context that should feed into the review, but the changes can be +usefully decomposed when landing them. When following this model, each commit +you intend to end up on the `trunk` branch needs to follow the same fundamental +rules as the pull request above: they should each build and pass tests when +landed in order, and they should have well written, cohesive commit messages. + +Prior to landing the pull request, you are expected to rebase it to produce this +final commit sequence, either interactively or not. This kind of rebase rewrites +the history in Git, which can make it hard to track the resolution of code +review comments. Typically, only do this as a cleanup step when the review has +finished, or when it won't otherwise disrupt code review. It is healthy and +expected to add "addressing review comments" commits during the review and then +squashing them away before the pull request is merged. + +## Linear history + +We want the history of the `trunk` branch of each repository to be as simple and +easy to understand as possible. While Git has strong support for managing +complex history and merge patterns, we find understanding and reasoning about +the history -- especially for humans -- to be at least somewhat simplified by +sticking to a linear progression. As a consequence, we either squash pull +requests or rebase them when merging them. diff --git a/proposals/README.md b/proposals/README.md index 4f8bfe92a460..5697c87d61bc 100644 --- a/proposals/README.md +++ b/proposals/README.md @@ -23,6 +23,7 @@ request: +- [0029 - Linear, rebase, and pull-request GitHub workflow](p0029.md) - [0044 - Proposal tracking](p0044.md) - [Decision](p0044-decision.md) - [0074 - Change comment/decision timelines in proposal process](p0074.md) diff --git a/proposals/p0029.md b/proposals/p0029.md new file mode 100644 index 000000000000..a3809a28b0f1 --- /dev/null +++ b/proposals/p0029.md @@ -0,0 +1,157 @@ +# Linear, rebase, and pull-request GitHub workflow + + + +[Pull request](https://github.com/carbon-language/carbon-lang/pull/29) + +## Problem + +There are a variety of workflows that can be effectively used with both Git +version control and GitHub. We should have a common and consistent workflow +across as much of Carbon as possible. While some specific areas may end up +needing specialized flows to suit their needs, these should be the exceptions +and can be handled on a case-by-case basis when they arise. + +## Background + +- Chapter 16 "Version Control and Branch Management" in the SWE book + (_[Software Engineering at Google](https://www.amazon.com/Software-Engineering-Google-Lessons-Programming/dp/1492082791)_) +- [Trunk Based Development](https://trunkbaseddevelopment.com/) +- GitHub documentation on + "[pull requests](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-requests)" + - [Configuration of their merges](https://help.github.com/en/github/administering-a-repository/configuring-pull-request-merges) + - [Protected branches](https://help.github.com/en/github/administering-a-repository/about-protected-branches) + +## Proposal + +Carbon repositories should all follow a common +[workflow](/docs/project/pull_request_workflow.md) for managing and landing +changes. The document outlines the specifics of the proposed workflow as well as +the core motivation. + +### Rename the default git branch to `trunk` + +This achieves two goals: + +1. Replaces the term `master`. This term, while only used in isolation in Git, + [was used](https://mail.gnome.org/archives/desktop-devel-list/2019-May/msg00066.html) + in immediately preceding and related systems as part of extremely problematic + "master/slave" terminology. That background associates the term with + unacceptable historical and cultural meanings. The intent of those using or + adopting the term isn't relevant to this association. The less overtly + problematic term being isolated from the rest doesn't erase its history, and + doesn't completely avoid painful associations. + +2. It directly anchors and reinforces contributors on the trunk-based workflow. + +### Longer discussion of linear history + +While Git can effectively bisect through complex history graphs, this is +significantly harder than bisecting across linear history. Especially for any +part of Carbon involving code, we expect bisection to be a pervasive tool and we +can make it simpler and more effective by forcing a linear history. + +A linear history also makes it much easier to ask questions about whether a +particular change has landed yet, or when a bug was introduced. For some people, +releases are more simply understood as branching from a specific snapshot of the +linear history. While tools like `git log` can provide similar functionality, it +is less trivially understood. + +Continuous integration is simplified for many of the same reasons as bisection: +the set of potential deltas is reduced to a linear sequence. Reverting to green +becomes easier to understand, and testing each incremental commit has a single +obvious interpretation. + +Requiring linear history also incentivises incremental development that is +committed early to the main branch. This, in essence, ensures a single source of +truth model even with the distributed version control system. Because +works-in-progress are required to be rebased, they tend to merge early and often +rather than forming long-lived development branches. This helps reduce the total +merge resolution and testing costs as a project scales. For more details about +the advantages of using a single source of truth, see the full text of the +"Version Control and Branch Management" chapter in the SWE book. + +One concern with linear history when rebasing a sequence of changes and merging +them is that the pull request associated with that sequence might not be obvious +from the main branch commit log. However, there is enough information in the +repository to establish the relationship, and GitHub's UI surfaces the pull +request on each commit in the series. + +## Alternatives considered + +### LLVM model + +LLVM allows directly pushing/submitting to their "trunk" with post-commit +review. LLVM enforces linear history for day-to-day development. Merge commits +are allowed for rare events like contributing a new subproject. + +Pros: + +- Still has linear history. +- Incentivizes squashing for continuous integration and bisection. +- Very low overhead for fixing trivial mistakes. + +Cons: + +- Creates extremely bad incentives around code review. + - Lots of patches don't get pre-commit review, even if they would benefit from + it. + - Very experienced contributors are much better at avoiding pre-commit review, + so are rarely blocked waiting on review. + - Leads to the most experienced members of the community not doing enough code + reviews, or being timely enough in code reviews. + - Lots of patches submitted with post-commit review are never reviewed in + practice unless they break something. +- UI and basic support for code reviews entirely focused on pull requests. + +### Fork and merge model + +Classically, Git and GitHub support merging entire complex graphs of +development. + +Pros: + +- Mostly supported by pull requests, so still able to use much of that + functionality. +- Supports a model in which contributors do not communicate and can each develop + a local, decentralized fork while still achieving overall reconciliation. +- Can model much more complex history of code evolution faithfully in the + tooling. + - Most of the time these aren't so complex that they create problems for + humans. + +Cons: + +- History is harder for humans to understand and reason about in complex cases. +- Bisection and continuous integration are more complex. + - May create difficulty for continuous integration against mainline, because + unclear what "order" they should be applied / explored. While there are + technical approaches to address this, the don't seem to eliminate the + complexity, merely provide a clear set of mechanics for handling it. +- Reduces incentives to land small, incremental changes by allowing + non-linearity to reduce the effort required for large and/or complex merges. +- Makes review of the main branch's history harder due to non-linearity. + +### Fork and merge, but branches can only have linear history + +Imagine a fork and merge model, but PRs can only have linear history. That is, +branching and merging within a PR, or merging `trunk` into PR is not allowed. In +this model, the only merge commits are merges of PRs into `trunk`. PRs +themselves don’t contain merge commits. + +Pros: + +- Mostly supported by GitHub pull requests, so still able to use much of that + functionality. +- Restricts non-linearity of history. The only non-linearity that is left is + merge commits on the trunk branch. PRs themselves can’t contain merge commits. + +Cons: + +- Requires a custom presubmit on GitHub that checks linearity of a PR. +- The cons of the fork and merge strategy regarding the complexity of history + remain, but to a smaller degree, since non-linearity is restricted.