diff --git a/docs/project/code_review.md b/docs/project/code_review.md index d8a2e0164dae..53afcafa2242 100644 --- a/docs/project/code_review.md +++ b/docs/project/code_review.md @@ -216,11 +216,6 @@ far. It is still possible to shrink a change so much that it becomes nonsensical in isolation. For example, a change without appropriate tests is not self-contained. -You may want to use a set of -[stacked pull requests](pull_request_workflow.md#stacking-dependent-pull-requests) -rather than a single, larger pull request in order to keep changes easy to -review. - ### Responding to review comments Many comments have easy and simple responses. The easiest is **"Done"**. When diff --git a/docs/project/pull_request_workflow.md b/docs/project/pull_request_workflow.md index e04788b50738..845f6a8a32f3 100644 --- a/docs/project/pull_request_workflow.md +++ b/docs/project/pull_request_workflow.md @@ -14,7 +14,6 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - [Green tests](#green-tests) - [Always use pull requests (with review) rather than pushing directly](#always-use-pull-requests-with-review-rather-than-pushing-directly) - [Small, incremental changes](#small-incremental-changes) - - [Stacking dependent pull requests](#stacking-dependent-pull-requests) - [Managing pull requests with multiple commits](#managing-pull-requests-with-multiple-commits) - [Linear history](#linear-history) @@ -102,170 +101,6 @@ 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. -### Stacking dependent pull requests - -Carbon uses pull requests in the common, distributed GitHub model where you -first fork the repository, typically into your own private GitHub fork, and then -develop on feature branches in that fork. When a branch is ready for review, it -is turned into a pull request against the official repository. This flow should -always be where you start when contributing to Carbon, and it scales well even -with many independent changes in flight. - -However, a common limitation to hit is when you want to create a _stack_ of -_dependent_, small, and incremental changes and allow them to be reviewed in -parallel. Each of these should be its own pull request to facilitate our desire -for small and incremental changes and review. Unfortunately, GitHub has very -poor support for managing the _review_ of these stacked pull requests. -Specifically, one pull request cannot serve as the _base_ for another pull -request, so each pull request will include all of the commits and diffs of the -preceding pull requests in the stack. - -We suggest a specific workflow to address this (note, commit access is -required): - -1. Create your initial pull request from a branch of your fork, nothing special - is needed at this step. Let's say you have a branch `feature-basic` in your - clone of your fork, and that the `origin` remote is your fork. - - Push the branch to your fork: - - ```shell - git checkout feature-basic - git push origin - ``` - - And create a pull request for it using the - [`gh`](/docs/project/contribution_tools.md#gh-cli) tool: - - ```shell - gh pr create - ``` - - Let's imagine this creates a pull request `N` in the upstream repository. - -2. _If_ you end up needing to create a subsequent pull request based on the - first one, we need to create a _branch_ in the upstream repository that - tracks the first pull request and serves as the base for the subsequent pull - request. Assuming your fork `$USER/carbon-lang` is remote `origin` and - `carbon-language/carbon-lang` is remote `upstream` in your repository: - - ```shell - git checkout feature-basic - git push upstream HEAD:pull-N-feature-basic - ``` - - Everyone marked as a contributor to Carbon is allowed to push branches if - the name matches `pull-*`, skipping pull request review processes. They can - be force pushed as necessary and deleted. These branch names should only be - used for this ephemeral purpose. All other branch names are protected. - -3. Create your stacked branch on your fork: - - ```shell - git checkout -b next-feature-extension - git commit -a -m 'Some initial work on the next feature.' - git push origin - ``` - -4. Create the pull request using the upstream branch tracking your prior pull - request as the base: - - ```shell - gh pr create --base pull-N-feature-basic - ``` - - This creates a baseline for the new, stacked pull request that you have - manually synced to your prior pull request. - -5. Each time you update the original pull request by pushing more commits to - the `feature-basic` branch on your `origin`, you'll want to re-push to the - upstream tracking branch as well: - - ```shell - git checkout feature-basic - git commit -a -m 'Address some code review feedback...' - git push - git push upstream HEAD:pull-N-feature-basic - ``` - - Then _merge_ those changes into your subsequent pull request: - - ```shell - git checkout next-feature-extension - git merge feature-basic - git push - ``` - - The merge will prevent disrupting the history of `next-feature-extension` - where you may have code review comments on specific commits, while still - allowing the pull request diff view to show the new delta after - incorporating the new baseline. - -6. Follow a similar process as in 5 above for merging updates from the main - branch of `upstream`: - - ```shell - git checkout trunk - git pull --rebase upstream - - # Update your fork (optional). - git push - - # Merge changes from upstream into your branch without disrupting history. - git checkout feature-basic - git merge trunk - # Push to the first PR on your fork. - git push - # Synchronize the upstream tracking branch for the first PR. - git push upstream HEAD:pull-N-feature-basic - - # Merge changes from the first PR (now including changes from trunk) - # without disrupting history. - git checkout next-feature-extension - git merge feature-basic - # And push to the second PR on your fork. - git push - ``` - -7. When the first pull request lands in the main upstream branch, merge those - changes from upstream trunk into the stacked branch: - - ```shell - # Pick up the first PR's changes from upstream trunk. - git checkout trunk - git pull --rebase upstream - - # Merge those changes into the stacked PR branch. - git checkout next-feature-extension - git merge trunk - git push - ``` - - Then update the stacked PR's base branch to be `carbon-language:trunk` - rather than the upstream tracking branch. To do this, go to the page for the - PR on GitHub, click the "Edit" button to the right of the PR title, and then - select `trunk` from the "base" drop-down box below the PR title. - - Once that's done, delete the upstream tracking branch: - - ```shell - git push upstream --delete pull-N-feature-basic - ``` - -8. When landing the second, stacked pull request, it will require actively - rebasing or squashing due to the complex merge history used while updating. - -Additional notes: - -- If you need to create a third or more stacked pull requests, simply repeat - the steps starting from #2 above for each pull request in the stack, but - starting from the prior pull request's branch. - -- If you want to split the two pull requests so they become independent, you - can explicitly edit the base branch of a pull request in the GitHub UI. The - result will be two pull requests with an overlapping initial sequence of - commits. You can then restructure each one to make sense independently. - ### Managing pull requests with multiple commits Sometimes, it will make sense to _land_ a series of separate commits for a diff --git a/proposals/p1367.md b/proposals/p1367.md index b4d6143c24b8..02c8af9cb730 100644 --- a/proposals/p1367.md +++ b/proposals/p1367.md @@ -116,11 +116,9 @@ fallback for other PRs that don't have explicit assignment. ### Stacked PRs -Advice for -[stacked PRs](/docs/project/pull_request_workflow.md#stacking-dependent-pull-requests) -is no longer "just ask an admin"; it is "commit access required". This is -because removing CODEOWNERS removes boundary between merge access _to a feature -branch_ and merge access _to trunk_. +Advice for stacked PRs is no longer "just ask an admin"; it is "commit access +required". This is because removing CODEOWNERS removes boundary between merge +access _to a feature branch_ and merge access _to trunk_. ## Rationale @@ -150,9 +148,7 @@ Advantages: - Relies on built-in GitHub CODEOWNERS featureset, intended for this purpose. - Can use built-in features for review assignment. -- Works with current - [stacked PR](/docs/project/pull_request_workflow.md#stacking-dependent-pull-requests) - advice. +- Works with current stacked PR advice. Disadvantages: