Skip to content

[docs][GitHub] Document alternative approach to stacked PRs #132424

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 4 commits into from
Apr 2, 2025
Merged

[docs][GitHub] Document alternative approach to stacked PRs #132424

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be8759d
[docs][GitHub] Document alternative approach to stacked PRs
banach-space Mar 21, 2025
ba20d05
fixup! [docs][GitHub] Document alternative approach to stacked PRs
banach-space Mar 22, 2025
41d32b7
fixup! fixup! [docs][GitHub] Document alternative approach to stacked…
banach-space Mar 25, 2025
5591fe0
fixup! fixup! fixup! [docs][GitHub] Document alternative approach to …
banach-space Mar 26, 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
44 changes: 43 additions & 1 deletion llvm/docs/GitHub.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ Before your first PR
Please ensure that you have set a valid email address in your GitHub account,
see :ref:`github-email-address`.

.. _github_branches:

Branches
========

Expand All @@ -29,8 +31,48 @@ intended to be able to support "stacked" pull-request. Do not create any branche
llvm/llvm-project repository otherwise, please use a fork (see below). User branches that
aren't associated with a pull-request **will be deleted**.

Stacked Pull Requests
=====================

GitHub does not natively support stacked pull requests. However, there are
several common alternatives.

To illustrate, let's assume you're working on two branches in your fork of the
``llvm/llvm-project`` repository, and you want to eventually merge both into ``main``:

* `feature_1`, which contains commit `feature_commit_1`,
* `feature_2`, which contains commit `feature_commit_2` and depends on
`feature_1` (so it also includes `feature_commit_1`).

Your options are as follows:

#. Two PRs with dependency note

Create PR_1 for branch feature_1 and PR_2 for branch feature_2. In PR_2, add a
note in the PR summary indicating that it is part of a series or depends on
another PR (e.g., “Depends on #PR_1”). It's also helpful to highlight which
commits belong to the base PR, so reviewers can focus on the new changes.

#. User branches in `llvm/llvm-project`

Create user branches in the main repository, as described
:ref:`above<github_branches>`. Then:

* Create a pull request from `users/<username>/feature_1` → `main`
* Create another from `users/<username>/feature_2` →
`users/<username>/feature_1` Once `feature_1` is merged, you can update the
base of the second PR to target main.

#. Use a stacked PR tool

Use a tool like SPR or Graphite (described below) to manage stacked PRs more
easily.

Each of these approaches can help streamline the review process. Choose the one
Copy link
Member

@ftynse ftynse Mar 25, 2025

Choose a reason for hiding this comment

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

I'd suggest calling out that the entire point of the exercise is to make the review process smoother, both for the reviewer and, to some extent, for the author by providing enough context and not making them waste cycles on changes that can be reviewed separately.

Copy link
Collaborator

@statham-arm statham-arm Mar 25, 2025

Choose a reason for hiding this comment

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

I think, more than that, perhaps explain why anyone wants to stack multiple PRs in the first place! If you open the whole section by saying "Github doesn't support them" then you assume that people already knew that they wanted them (not to mention what they are).

This certainly wasn't obvious to me. To my way of thinking, one of the good things about a Github PR compared to a Phabricator patch is that a PR doesn't have a 1–1 correspondence between patches and reviews. If you have a patch series containing logically separate sub-changes, the obvious thing is to put it all in a single PR, so that it can be reviewed as a whole, but the logical sub-changes remain separate once it's landed. So if you're coming to LLVM from a Github project that lets you do that (because it doesn't insist on "squash and merge"), you wouldn't think of stacked PRs in the first place.

So perhaps it's worth starting off with some introductory text that explains

  1. that we use "squash and merge", so you can't do the more obvious thing you might be used to from elsewhere
  2. why we do that (I'm not sure myself but I think it probably has to do with using the multiple commits in the PR to show the evolution of the patch as review progresses)
  3. that therefore, if you do want to keep your logical sub-changes separate, you're going to have to put each one in its own PR and stack them in some way, and here are some ways to do it.

Copy link
Contributor Author

@banach-space banach-space Mar 25, 2025

Choose a reason for hiding this comment

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

Added at the top.

Copy link
Member

@mstorsjo mstorsjo Mar 25, 2025

Choose a reason for hiding this comment

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

I'd still prefer if you explicitly spell out that LLVM uses "squash and merge" - this is the root cause why we can't do it like many other projects do it. This leads to the need for separate branches.

Copy link
Contributor Author

@banach-space banach-space Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Added at the top.

@statham-arm , I posted that "before" you posted your comment (more specifically, "before" GitHub displayed your comment to me). So my reply was specifically to Alex. I have expanded that section a bit, but will add a a note on "squash and merge" as well. Thanks!

I'd still prefer if you explicitly spell out that LLVM uses "squash and merge"

Sure. I just need to make sure I am not duplicating info. "squash and merge" is already covered here:

🤔

Copy link
Collaborator

@statham-arm statham-arm Mar 25, 2025

Choose a reason for hiding this comment

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

(Yes, given what you changed, it was clear to me that you were replying to the comment before mine – I didn't misunderstand!)

Copy link
Member

@mstorsjo mstorsjo Mar 25, 2025

Choose a reason for hiding this comment

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

Sure. I just need to make sure I am not duplicating info. "squash and merge" is already covered here:

https://llvm.org/docs/GitHub.html#landing-your-change

Right, I see - I haven't checked the full context (and I did indeed think your reply was to Simon).

IMO, even if it duplicates info a little bit, it may still be good to at least briefly mention that this is due to us using "squash and merge".

Also now from looking at the full document, it feels like the order of info is a bit weird, when it starts out with how to create branches in the repo and using graphite, and only then giving the high level info about using PRs for contributions. But that's of course a separate discussion.

Copy link
Contributor Author

@banach-space banach-space Mar 26, 2025

Choose a reason for hiding this comment

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

it feels like the order of info is a bit weird

Yes, it's probably about time for somebody to review the whole thing and to unify this document a bit. Moving sections should be easy, I can do it in a follow-up PR. I will also try to review the document as a whole, but no promises just yet :)

@statham-arm , the latest commit incorporates most of your suggestions. I didn't add any justification for LLVM using "squash and merge" - that probably belongs to some other, more general section (which we should add). I am also slightly concerned that I miss some nuance of the rationale behind "squash and merge", so would rather stick to "we use squash and merge, therefore ...". WDYT?

Copy link
Collaborator

@statham-arm statham-arm Mar 26, 2025

Choose a reason for hiding this comment

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

I don't have a strong opinion where we explain the reasoning for "squash and merge", but I think it would be good to have it explained somewhere, if only so that people used to the normal way of doing things don't assume the answer is "because we hate you and want you to have to do everything the most complicated way possible" 🙂

(After all, the fact that we have to list several different workarounds here, including two that depend on an extra tool, shows that there clearly is a downside to "squash and merge", so it's reasonable for people to want to know what benefit LLVM thinks is worth all that inconvenience!)

But if you personally don't know the answer (and I can't blame you, because I don't either) then fair enough!

Copy link
Member

@mstorsjo mstorsjo Mar 26, 2025

Choose a reason for hiding this comment

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

@statham-arm AFAIK the reason for mandating "squash and merge" comes from the issue, that github is (supposedly) 1) bad at showing what has changed between one iteration of a PR and the next, if you squash/rebase your commits during review, and 2) bad at keeping inline comments visible and in sync with the surrounding code, during reviews, if you squash/rebase your commits during review. This requires doing PR updates by incremental commits on top, and hence mandating "squash and merge".

Whether it really is so bad at these things to warrant all this extra complexity, is definitely a topic worth bringing up for discussion somewhere - I totally agree with your sentiment here.

(If we could assume that people with merge privileges are git-savvy enough to look at a PR and determine if it is a set of multiple independent commits that should be kept as is, merged via "rebase and merge", or if it is a series of incremental tweaks on what's supposed to be one change and should be squashed - then we could consider to allow both merge styles. But I guess that may be asking a bit much, and creates a pretty big risk for ugly broken intermediate commits ending up in the repo.)

that works best for your workflow.
Copy link
Collaborator

@joker-eph joker-eph Mar 25, 2025

Choose a reason for hiding this comment

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

We should describe that there are different behavior between the two, in particular the first one (from a fork) won't display a proper diff for the subsequent PR in the stack, while the second one displays the expected diff and allows independent review of the PRs.

Copy link
Contributor Author

@banach-space banach-space Mar 25, 2025

Choose a reason for hiding this comment

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

I expanded the current description and added a note at the end.


Using Graphite for stacked Pull Requests
========================================
----------------------------------------

`Graphite <https://app.graphite.dev/>`_ is a stacked pull request tool supported
by the LLVM repo (the other being `reviewable.io <https://reviewable.io>`_).
Expand Down