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

[banach-space]

No description provided.

[banach-space]

This is an attempt to encourage folks to specify which PRs their contributions depend on. Recent examples:

• [flang-rt] Added IsContiguousUpTo runtime function. #131048
• [LinkerWrapper] Pass all files to the device linker #97573
• [mlir][Transforms] Dialect conversion: Fix missing source materialization #97903
• [llvm][ARM][AArch64] Add attributes to synthetic functions. #83153

[boomanaiden154]

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.

[banach-space]

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:

• [docs] Stress out the branch naming scheme for Graphite. #104499

Not quite what I thought.

[boomanaiden154]

Interesting. We should probably add something.

[boomanaiden154]

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.

[banach-space]

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:

• [mlir][vector] Allow multi dim vectors in vector.scatter #132217

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

[boomanaiden154]

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?

[banach-space]

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.

[boomanaiden154]

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

[banach-space]

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?

[boomanaiden154]

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.

[banach-space]

Hey @boomanaiden154 — thanks again for all the thoughtful discussion and suggestions so far!

I’ve refactored this to align with your feedback. The current version outlines three different ways of handling stacked PRs, with the goal of documenting the approaches currently in use.

I understand you’re not particularly fond of the first approach:

That's not a very good way to do it, at least in my opinion.

Before recommending one method over the others, I’d like to give each some visibility and see if clear preferences emerge. If so, I’m happy to reflect that in the docs — possibly in a follow-up PR.

Let me know what you think!

[boomanaiden154]

I’ve refactored this to align with your feedback. The current version outlines three different ways of handling stacked PRs, with the goal of documenting the approaches currently in use.

Thanks for the update. I think you've captured the current state of how things work reasonably well.

Before recommending one method over the others, I’d like to give each some visibility and see if clear preferences emerge. If so, I’m happy to reflect that in the docs — possibly in a follow-up PR.

Seems reasonable enough to me. I think of what is in these docs as more of recommendations rather than just documentation so I don't think it would be bad if we could clarify here what is preferred, but if people are already using all three techniques and reviewers are generally happy, it should be fine to leave it for a follow up.

[boomanaiden154]

Tentatively LGTM.

Please wait for someone else to approve though and comment on how they want to deal with documenting current state versus making reccomendations.

[banach-space]

Thanks! I've posted https://discourse.llvm.org/t/rfc-documenting-stacked-pr-practices/ to help broadcast this discussion.

[joker-eph]

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.

[banach-space]

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

[ftynse]

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.

[statham-arm]

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.

[banach-space]

Added at the top.

[mstorsjo]

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.

[banach-space]

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:

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

🤔

[statham-arm]

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

[mstorsjo]

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.

[banach-space]

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?

[statham-arm]

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!

[mstorsjo]

@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.)

[joker-eph]

LGTM, please wait for approval from others who commented (and a couple of days at least)

[banach-space]

This commit should cover all the comments from David, Martin and Simon. Thanks for the suggestions!

[statham-arm]

LGTM – this is definitely an improvement on the docs before this commit, and also an improvement on the first version I saw in this review.

[mstorsjo]

LGTM, the details I requested are very nicely explained now - thanks!

[banach-space]

@ftynse Could you take another look? I’ll leave this open until next week - if there are no further comments, I plan to land it around Tuesday.

[banach-space]

@ftynse Could you take another look? I’ll leave this open until next week - if there are no further comments, I plan to land it around Tuesday.

I haven't heard back from Alex, but I believe all comments have been addressed. I'll go ahead and land this now and, if needed, iterate on it in-tree.

Thanks again, everyone, for the reviews! 🙏🏻