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 1 commit
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
17 changes: 16 additions & 1 deletion llvm/docs/GitHub.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,23 @@ 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. There are two common
Copy link
Contributor

@boomanaiden154 boomanaiden154 Mar 21, 2025

Choose a reason for hiding this comment

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

With user branches we sort of have stacked pull requests. The supper still isn't great though.

Copy link
Contributor Author

@banach-space banach-space Mar 21, 2025

Choose a reason for hiding this comment

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

Are user branches alone sufficient for stacked PRs? If yes, then I am behind and time to learn something new 😅

In general, I was under the impression that user branches were discouraged?

Copy link
Contributor

@boomanaiden154 boomanaiden154 Mar 22, 2025

Choose a reason for hiding this comment

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

Yes, user branches will give you stacked PRs. You can do them manually, but SPR will set them up for you automatically.

User branches aren't great, but I wouldn't say they're discouraged. The official (maybe just de facto?) policy is that they are perfectly fine to be used for stacked PRs, but should be avoided for PRs that are not stacked.

alternatives:

* Add a note in your PR summary indicating that your patch is part of a
series or depends on another PR (e.g., “Depends on #123456”). It also helps
to highlight which commits belong to other PRs, so reviewers can focus only
Copy link
Contributor

@boomanaiden154 boomanaiden154 Mar 21, 2025

Choose a reason for hiding this comment

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

Is anyone actually using the workflow where multiple PRs are opened where PR x has its commit and all the commits it depends on? User branches should be used for that and it makes the review much easier.

When using SPR though, it is good to add a note summarizing everything.

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

I haven't noticed anyone using SPR or Graphite in MLIR. Most people just include multiple commits (myself included). In fact, it's this PR specifically that prompted me to write this section:

So yes, this is quite common actually. At least from my experience.

Copy link
Contributor

@boomanaiden154 boomanaiden154 Mar 22, 2025

Choose a reason for hiding this comment

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

Interesting. That's not a very good way to do it, at least in my opinion. SPR is pretty easy to use and makes things a lot nicer. Is it just that people don't know about spr? Is there a reason they're not using SPR/Graphite?

Copy link
Contributor Author

@banach-space banach-space Mar 23, 2025

Choose a reason for hiding this comment

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

Is it just that people don't know about spr?

Yup, I think that we need to do a bit of "advertising". So, once this is approved, I will post something on Discourse to draw people's attention to the available options.

on the relevant changes.
* Use Graphite (described below), a tool that supports stacked PR workflows.
Copy link
Contributor

@boomanaiden154 boomanaiden154 Mar 21, 2025

Choose a reason for hiding this comment

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

SPR (https://github.com/spacedentist/spr) also works and I think is documented somewhere in this file. It is used reasonably commonly by llvm developers.

Copy link
Contributor Author

@banach-space banach-space Mar 21, 2025

Choose a reason for hiding this comment

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

I actually don't see any references to SPR 🤔

I was also expecting to find them here. In fact, I assumed that whenever the section on Graphite was introduced, folks would have removed references to SPR:

Not quite what I thought.

Copy link
Contributor

@boomanaiden154 boomanaiden154 Mar 22, 2025

Choose a reason for hiding this comment

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

Interesting. We should probably add something.


Both approaches help streamline the review process. Choose the one that works
best for you.

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