Skip to content

[llvm][docs] Reorder Stacked PR approaches in GitHub.rst #138126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 6, 2025
Merged

[llvm][docs] Reorder Stacked PR approaches in GitHub.rst #138126

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4faad8
[llvm][docs] Reorder Stacked PR approaches in GitHub.rst
paschalis-mpeis May 1, 2025
071536a
Addressed reviewers + nit
paschalis-mpeis May 2, 2025
c0b23f9
Addressing reviewers (2)
paschalis-mpeis May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions llvm/docs/GitHub.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,16 +254,6 @@ To illustrate, assume that you are working on two branches in your fork of the

Your options are as follows:

#. Two PRs with a dependency note

Create PR_1 for `feature_1` and PR_2 for `feature_2`. In PR_2, include a
note in the PR summary indicating that it depends on PR_1 (e.g.,
“Depends on #PR_1”).

To make review easier, make it clear which commits are part of the base PR
and which are new, e.g. "The first N commits are from the base PR". This
helps reviewers focus only on the incremental changes.

#. Use user branches in ``llvm/llvm-project``

Create user branches in the main repository, as described
Expand All @@ -274,8 +264,24 @@ Your options are as follows:

This approach allows GitHub to display clean, incremental diffs for each PR
in the stack, making it much easier for reviewers to see what has changed at
each step. Once `feature_1` is merged, you can rebase and re-target
`feature_2` to `main`.
each step. Once `feature_1` is merged, GitHub will automatically rebase and
re-target your branch `feature_2` to `main`. For more complex stacks, you can
perform this step using the web interface.

This approach requires
`Commit Access <https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access>`_.
However, if you are involved with a Stacked PR, there's a good chance you'll
be granted access.
Copy link
Contributor

@banach-space banach-space May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The paragraph you linked discusses the process of obtaining commit access, rather than providing a general definition of what "commit access" means. I suggest updating the link title to reflect that more accurately.

Also, regarding the sentence:

However, if you are involved with a Stacked PR, there's a good chance you'll be granted access.

This phrasing could be misleading. In the LLVM project, needing stacked PRs is not itself a sufficient criterion for being granted commit access. We should avoid suggesting a causal link where there isn’t one.

I suggest keeping this simple and factual: "This approach requires commit access: ".

I hope that this makes sense.

Copy link
Member Author

@paschalis-mpeis paschalis-mpeis May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Andrzej,

Good points, and thanks for your review!

I've kept the link and now it refers on how to obtain commit access. – or do you think it's better to remove it?


#. Two PRs with a dependency note

Create PR_1 for `feature_1` and PR_2 for `feature_2`. In PR_2, include a
note in the PR summary indicating that it depends on PR_1 (e.g.,
“Depends on #PR_1”).

To make review easier, make it clear which commits are part of the base PR
and which are new, e.g. "The first N commits are from the base PR". This
helps reviewers focus only on the incremental changes.

#. Use a stacked PR tool

Expand Down
Loading