carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-09-24 21:30:12 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Remove stacked PR notes (#2205)

Browse Source 
chandlerc had originally added these and previously expressed being okay with removing them. They've just proven too complex to be applied; it needs to be easier for people to want to do it.
This commit is contained in:
Jon Ross-Perkins
2022-09-21 16:10:36 -07:00
committed by GitHub
parent 804a09bccd
commit 1dcd15380c
3 changed files with 4 additions and 178 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/project/code_review.md
-5
View File
@@ -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
docs/project/pull_request_workflow.md
-165
View File
@@ -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
proposals/p1367.md
+4 -8
View File
@@ -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: