Define a policy for merging Pull Requests with multiple commits

[tstellar]

• Should we require only one commit per Pull Request?
• Should we require squashing pull requests before merging?
• If we do require squashing how do we ensure that the commit message looks nice?
• Other things to consider?

[rengolin]

This is a really hard one, and we can only solve this with policy. If we mandate squash, it's an easy GH policy. If we allow rebase, it's a "documentation" kind of policy. In small teams, both work well.

In large disconnected teams, with so many diverse people working on it, we're bound to have problems with rebase. We'll also have problems with forced squash, if we can't solve the stacked PRs well for those who need it.

To me, the main issues are:

• Squashing each PR:
  • Pro: Easy to bisect, revert, identify where the review comments are
  • Con: Bundle work that may have meant to be separate by original author
• Allowing rebase:
  • Pro: No fiddling with commit messages, nor having giant list of commits in one
  • Con: Allows lots of "review comments" and "fix" commits to bleed through main (strong bisect & revert issues)

Additional context:

• The monorepo has many sub-projects working together, people that already work very differently
• Consensus may be reached on sub-communities but not overall
• Difference in approach may already be causing grief (rapid fire of small project vs strong need to bisect of stable targets)

This will also depend on how we do stacked PRs. If people feel strongly about their commit history, they can move a single PR to a stacked one, but only if that's easy to do and manage the reviews, otherwise, we'd be creating unnecessary bureaucracy on the review process.

For commit messages, my personal opinion is that they need to be informative before looking nice. If the PR has an overall description and each commit has their own, GH already bundles them one after the other, so the information is there, in the right order. Looking nice rapidly loses importance, IMO, after the first paragraph.

[kparzysz-quic]

We shouldn't require PRs to only have a single commit. Doing it would mandate using forced pushes, and that may be disruptive (I think it damages GitHub's understanding of where prior review comments were attached).

[llvmbot]

@llvm/issue-subscribers-infrastructure

[joker-eph]

We shouldn't require PRs to only have a single commit. Doing it would mandate using forced pushes, and that may be disruptive (I think it damages GitHub's understanding of where prior review comments were attached).

Not doing force pushes means:

• it is hard to do stacked PR
• you can't rebase on main (do you merge main back into your branch?)

Not having a single commit means you can't review the commit message that will be merged. (Edit: => not exactly true: we can configure now GitHub to take the PR description as commit message)

[jh7370]

I feel like there shouldn't be one single policy for a given PR, because different cases have different needs. Outlining my personal requirements:

1. I don't want to see fixup commits appearing as separate commits in the main branch, or really even in the commit message. This requires some form of squashing.
2. If we have any of the following cases, I want to be able to have mutliple commits resulting from the same PRs (and therefore need Rebase & Merge):
   1. Mandating that every change requires a PR (i.e. preventing direct pushes to main) - this is because simple typos or clean-ups that I'd currently push without review will need a new PR and potentially someone to review them, which is too heavyweight and will result in not bothering to make these changes. By having them in the same PR as the follow-on work that is likely related, I don't need to worry about a separate PR, although I also don't want to pollute the "main" commit.
   2. Stacked commits that end up in a single PR (if that's the approach we choose to take) - we want these to end up as individual commits (but not their fixups in between!).
3. I want a way to be able to identify the PR that resulted in a commit landing (currently resolved with our "Differential Revision" label). This is automatically done by squashed PRs (the PR link gets added to the commit summary), but not for rebases (as far as I know anyway). If allowing rebases, we'd have to insist that PR links are added somewhere in the commit message.
4. I want to be able to review final proposed commit messages. From experience, Squash & Merge routinely ends up with messed up messages containing something along the lines of:

   
   Some nice commit message summary

• Some nice commit message summary

A nice commit message description.

• Fixed a typo

• Addressed review comments

Added more tests

• Addressed review comments

Renamed some variables

When working within my company, most of my work uses Squash & Merge, with the occasional Rebase & Merge as needed, usually because I have a separate commit that doesn't really need its own review, in the middle of a series of linked PRs. However, before clicking the merge button, I usually end up doing a manual interactive rebase to squash commits and reword commit messages, before clicking "Squash & Merge" to get the added PR link. However, I consider myself disciplined enough to not need my commit messages reviewing. I don't hold that same trust in LLVM developers...

Aside: I assume we're going to disable the regular merge option, since that means a non-linear history, which we don't want.

[rengolin]

@jh7370, to most of your points, a simple answer could be "policy" and "culture". With Phab, we don't have a clear way to review the final commit message (people can change before commit) or how many patches go in (people can have a rich git history to merge and do git diff main to update Phab). But it's ok, because of our culture and policies.

On the other hand, if we require people to create branches on the main repo, we'll have to give everyone write access to the repo before they show code and behaviour quality, which goes against the developer's policy. It's also very dangerous.

Having said that, I totally agree with all your points, and I think the solutions you found on your own projects are pretty much the same I found on mine. I believe those are generally accepted as good and we can just write that into policy and adapt to that culture, like we did with Phab.

I think there's a lot of "How do I do X in GH like I did in Phab" and that's not helpful. What we need to be asking is just "How do I do X in GH" and if that changes our policies/culture a bit, without breaking how we work, there's no harm.

A few ideas:

• We should encourage squash & merge unless there's a reason not to (ex. stacked reviews)
• On GH squash, developers must clean up the extra commit messages into a single review
  • We can request that the commit message be only what's in the GH description, so that people do that beforehand and really delete everything else upon squash
  • Developers will need to "squash" the commit message on creation and not add more stuff on fixup patches
• On GH rebase, people need to make sure their history is sane and each commit has a decent message
  • GH allows us to see each commit and their messages, so we can already review each message before approval
• As currently, we rely on people:
  • Not radically changing the commit message(s) after approval / before commit
  • Not adding local revert/fixup commits when merging PRs

The key thing here is that accidentally having fixup trail commit messages or an additional fixup commit is not the end of the world, and it's easy to fix with policy and education, but giving everyone in Github write access to our repo is bound to bring us serious problems as soon as "some people" find out. Clearing that out may end up as a technical and social nightmare.

Force push isn't a problem if it's done on other people forks. Sure, GH will have trouble with the review, but it's a thing that sometimes is necessary. It'd be up to each developer (and reviewers) to learn and cope what's the best strategy. As @jh7370 said, each case is different and having strict rules wouldn't make sense.

Luckily, the LLVM policies are written in a soft manner because we all know edge cases exist and we don't want to push people to do weird things for silly reasons. We also rely on people being generally nice and well intended, and that has mostly worked for the past many years, so I think we'll be fine.

Finally, we'll probably learn how to work with GH as we go, and change some of our ways in the process. I'm wary of trying to regulate too much what we do and how we work beforehand, but I'm also trying to make sure we don't fall into a situation we can't get out of. I personally think we should be fine, though.

[joker-eph]

@jh7370, to most of your points, a simple answer could be "policy" and "culture". With Phab, we don't have a clear way to review the final commit message (people can change before commit) or how many patches go in (people can have a rich git history to merge and do git diff main to update Phab). But it's ok, because of our culture and policies.

There is a difference though between you "can" do it and the tool "encourages it" or does not favor the opposite. While Phab does not technically prevent it and you "can" push many commits for a review, you'd do it once and likely get notified by the reviewer about the expectation or pushing something that matches the review. It's not clear to me that GitHub makes it as clear since you're presenting a history with many commits, and after all, the reviewer approved it as is...

On the other hand, if we require people to create branches on the main repo, we'll have to give everyone write access to the repo before they show code and behaviour quality, which goes against the developer's policy. It's also very dangerous.

Stacked PR requires to allow user branches, but it does not mean we should require these for every PRs including the non stacked one. Also you can give write access to the repo and restrict a branch to a given group (I think?), so you wouldn't have open main to everyone necessarily.

I agree with you that it can be made manageable but a few things you mentioned "developers must do X" and "developers should do Y" are not necessarily encouraged by the GitHub UI. That is, the UI plays against us by not being "easy to use, hard to misuse" with respect to these "good practices". Nothing is a blocker, but I'm skeptical that documentation solves everything (there is too much documentation and most folks can't read it all anyway), so I'm interested in what can be done in a more integrated way. For example for GitHub issues you can have templates that guides you and describe the expectation for filing bugs, are there similar things that can be done with PR? Can we customize the message that show up when you click "squash and merge" for example?

[rengolin]

Stacked PR requires to allow user branches

No it doesn't.

The community may have converged into a solution that does, but that's by far not the only solution. It's not even the most natural solution in Gihutb.

[Edited to add: "Stacked PRs", as in one PR on top of another, does need branches, but "Stacked Reviews", which is what we actually need, doesn't. Sorry, nomenclature is hard.]

[joker-eph]

You're playing with words, and I can't understand the difference you're trying to make between "Stacked PRs" and "Stacked Reviews" really. When I Google "stacked pull request" I find https://github.com/ejoffe/spr or https://github.com/marketplace/stacked-pull-requests which both rely on this description of the concept:

The Pull Requests are managed as a Stack where each Pull Request is based on the branch of the previous. Each PR contains a minimal scoped change making it easy to review.

I don't see how the GitHub UI can show "a minimal scoped change making it easy to review" without user branches.

[rengolin]

You're playing with words

I'm not sure what that means to you, but to me it sounds accusative.

Stacked PR is a stack of Github PRs (which can only currently be done with branches on the main repo). This only works when the developer pushing the changes has write access to the repository (or at least branch creation permission).

Stacked reviews is a review of a stack of commits, which currently we do in Phab via different reviews, and Github does as a single PR with multiple commits.

[joker-eph]

You wrote earlier:

Stacked PR requires to allow user branches No it doesn't.

with:

Stacked PR is a stack of Github PRs (which can only currently be done with branches on the main repo).

I feel I am missing something because I read a direct contradiction.

[joker-eph]

Carbon has a doc about stacked PR: https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/pull_request_workflow.md#stacking-dependent-pull-requests

They have branch protection in general, but allow only user branches that start with a specific prefix to end up in their upstream repo temporarily.

[tstellar]

It seems like most people agree that a pull request with one functional commit plus several "Fix up" commits should be squashed before it is pushed. It seems the remaining question for this scenario is how should this be implemented. From what I can tell there are 2 options:

1. Require that people squash locally and then force push to the pull request so that the commit message can be reviewed.
2. Allow people to use the "Squash and Merge" button and edit the commit message in the UI before pushing.

Is there a way to enforce either of these options? Should we recommend one over the other or just say it's up to the pull request author and the reviewer?

[jh7370]

I don't know of a specific way of enforcing this (though there may well be one). I do think we should have a requirement for newer contributors to do the local squash and force push (as long as this doesn't lose the comment history at least, which force pushes can cause issues with), but I think this can be relaxed for those more experienced who have shown they can create correct squashed commit messages. Either way, I think the reviewer can request it explicitly, and the committer should be expected to comply with the request.

Relatedly, I also think that the initial description of the PR should mirror the final intended commit message (or at least contain the message), when it's first uploaded. I believe this is the default for a PR created with a single commit, but it doesn't hurt to have this documented somewhere. We should also ban PRs being first created with multiple commits too, though I could be persuaded of some corner cases otherwise.

[joker-eph]

I believe you can configure the squash message to default to the PR description. This seems safer to me instead of the default which pulls all the commits in the pull-request together in the message. With this setting the reviewer can also comment on the PR description itself and reasonably expect it to be reflected in the squashed commit:

[tstellar]

I wrote some preliminary documentation for updating pull requests based on this conversation: https://reviews.llvm.org/D147284

I think we still need something in there about when it is OK to use multiple commits in a pull request, but I wasn't sure if we had consensus on that.

[tstellar]

I just enabled the "Allow squash merging" option for the repo and set the default commit message to "Default to pull request title and description" since we have documented this now as our repo policy.

We still have "Allow rebase merging" enabled and we need to decide if we want to keep this enabled or disable it.

[tstellar]

Based on the discussion in #63295 it seems like there is a preference for disabling "Allow rebase merging" which would prevent contributors from pushing multiple commits at once via a pull request. You would still be able to have a pull request with multiple commits, but they would need to be squashed before merging.

[nikic]

I don't think we can disable merge via rebase, as many middle-end LLVM changes need two commits: One that adds baseline tests and one that implements the functional change on top of that.

Contributors with commit access can directly land the first commit, but external contributors cannot. Currently they have to create a stacked review on Phabricator, but on GitHub they can simply create a PR with two commits. It's important that these two commits do not get squashed when merging though.

[joker-eph]

One that adds baseline tests and one that implements the functional change on top of that.

I don’t quite get what you’re describing? Why wouldn’t the tests and the change be in the same commit? What happens if you checkout the first commit alone and run the tests?

[nikic]

@joker-eph See https://llvm.org/docs/TestingGuide.html#precommit-workflow-for-tests.

The tests on the first commit show the behavior without the functional change. The second commit only shows the diff introduced by the functional change.

[jh7370]

@nikic, I don't quite get why there's a hard requirement to have the initial test and behaviour change in different commits? Every change I've ever made (admittedly not in the middle-end), and practically everything that I've reviewed lands the commit and functionality in one commit. If they DO need to be separate commits for whatever reason, then we're into the "stacked pull request" territory, it seems to me, and this is covered in #56636.

---

Another reason to use squash & merge is that the "#NNN" suffix added to commit messages when landing a PR only happens in that case (possibly also in plain "merge" cases, but I have no experience with that mode and think we should be avoiding it for reasons others have articulated already). This is the equivalent of the Differential Revision links people currently manually add to commit messages, which are really useful for code archaeology.

[joker-eph]

The reason for these separate commits is to ease the review isn’t it? If so then squashing shouldn’t be a problem. The author can still push the baseline test (the doc calls out it does not need review), otherwise indeed it become a « stacked review » question as @jh7370 mentioned.

[nikic]

@nikic, I don't quite get why there's a hard requirement to have the initial test and behaviour change in different commits?

Because we need to see a codegen diff to make sure tests are testing what they are supposed to. Otherwise we get tests that have the same behavior with and without the patch, or tests that have different input IR but end up being the same due to canonicalization, and thus no longer covering the code path they are intended to cover.

Every change I've ever made (admittedly not in the middle-end), and practically everything that I've reviewed lands the commit and functionality in one commit.

Pre-committed tests are required for most changes in most parts of LLVM. Basically any change where pre-commit is possible and auto-generated check lines work. (For example, if you work on debuginfo you might never encounter this workflow, because debuginfo tests tend to be hand-written to this day.)

If they DO need to be separate commits for whatever reason, then we're into the "stacked pull request" territory, it seems to me, and this is covered in #56636.

For 3rd-party contributors, stacked reviews is what we use right now, and I think it's a pretty bad experience for them. When we move to GitHub, we certainly do not want to replicate that (esp. considering that GitHub has no good support for stacked reviews).

Rather, the functionality GitHub offers in a single PR with two commits is perfect here, both from a contributor and reviewer perspective. We don't want or need the two commits to go through separate reviews, they can and should be part of one PR.

So yes, you could say that this just a "stacked review" question, but it's actually a case where GitHub has a very good answer to that question, as long as you don't artificially hobble it by disabling the merge via rebase option.

Another reason to use squash & merge is that the "#NNN" suffix added to commit messages when landing a PR only happens in that case (possibly also in plain "merge" cases, but I have no experience with that mode and think we should be avoiding it for reasons others have articulated already). This is the equivalent of the Differential Revision links people currently manually add to commit messages, which are really useful for code archaeology.

GitHub provides back-links to the PR no matter which merge option has been used, though I believe they are only available via the web interface.

---

Worth noting that the pre-commit workflow is just the most common example of the pattern "NFC commits followed by functional commit". The same also applies to e.g. doing some non-functional refactoring first and then doing a functional change, which we encourage over doing both in one commit. The pull request model is great for that, as long as pull requests with multiple commits are properly supported.

[joker-eph]

Because we need to see a codegen diff to make sure tests are testing what they are supposed to. Otherwise we get tests that have the same behavior with and without the patch

It’s not clear to me how having multiple commits in a PR will enforce this, you need multiple PR for this, or instead encourage people to push NFC ahead (like most folks with write access should)

The pull request model is great for that, as long as pull requests with multiple commits are properly supported

Can you elaborate on the part that breaks with squash merge? It’s not clear to me right now.

[nikic]

Because we need to see a codegen diff to make sure tests are testing what they are supposed to. Otherwise we get tests that have the same behavior with and without the patch

It’s not clear to me how having multiple commits in a PR will enforce this, you need multiple PR for this

Why do we need multiple PRs for this? GitHub's review interface allows you to review individual commits in the PR, so in that case we would (primarily) only review the second one, which contains the test diffs.

or instead encourage people to push NFC ahead

This is what we currently do, but it does have some disadvantages. In particular, it is pretty common that during the course of the review additional test coverage or changes to existing tests are requested. Ideally, we would commit the baseline test coverage in its final form, and not have multiple fixup commits for the tests.

What many people currently do is to keep the baseline tests locally and only submit the functional patch. They only commit the final baseline tests once accepted. This has the disadvantage that the patch can no longer be easily applied by other people and pre-merge checks don't work. (With GitHub this wouldn't work at all.)

(like most folks with write access should)

The migration to GitHub PRs is done for the benefit of people without write access, so surely we want to have a process that works for people without commit access as well.

The pull request model is great for that, as long as pull requests with multiple commits are properly supported

Can you elaborate on the part that breaks with squash merge? It’s not clear to me right now.

Uh, we don't want to lose history? Like, if you split off an NFC refactoring in a separate commit from a functional change and then you squash merge both commits -- that kind of defeats the point? It's not like we want to have a separation between NFC and FC only during the review, it should also remain the history long-term.

[rengolin]

I agree with @joker-eph and @jh7370 here. This seems to be a discussion about a particular method of development that is not necessarily the only one or even the main one in LLVM.

I don't think we should code the policy of the entire repo based on anecdotal evidence, but on general consensus. We have historically taken the slow-and-steady approach towards policy to make sure we don't break people's stuff just because we missed their use-case.

My proposal here is to allow both rebase and squash merges depending on the commits. In other projects I have successfully used this strategy by merely stating intent on the PR description: "Please don't squash this PR".

I wish Github could add the PR ID to the commits (or at least the last commit) as it merges, like it does on squashes, for the reasons @jh7370 stated above, but that's a small grudge, and largely irrelevant.

As we start using more and more PRs, we'll have more experience on what people really want/need, and then we can change the policy at the time it's clear what the requirements are.

After all, LLVM policy has never been about "what we should do because...", but about "what we have been doing and is working versus the other thing that isn't".

[joker-eph]

Because we need to see a codegen diff to make sure tests are testing what they are supposed to. Otherwise we get tests that have the same behavior with and without the patch It’s not clear to me how having multiple commits in a PR will enforce this, you need multiple PR for this Why do we need multiple PRs for this

Because I assume CI won’t build and test every individual commits in the PR, so we don’t « make sure » of anything unless we do exactly this.

The migration to GitHub PRs is done for the benefit of people without write access

I dispute this: it has to be a very minor part of the reason, I hope. Processes should be optimized for the majority of the contributions IMO.

Uh, we don't want to lose history? Like, if you split off an NFC refactoring in a separate commit from a functional change and then you squash merge both commits -- that kind of defeats the point? It's not like we want to have a separation between NFC and FC only during the review, it should also remain the history long-term

Baseline tests is what you brought up before: do you believe that history requires to have these in a separate commit? I don’t see why really: the baseline tests don’t make sense outside of the review context as far I as I understand.

Other kind of NFC are an entirely different story: this is stacked PR again.

[nikic]

Because we need to see a codegen diff to make sure tests are testing what they are supposed to. Otherwise we get tests that have the same behavior with and without the patch It’s not clear to me how having multiple commits in a PR will enforce this, you need multiple PR for this Why do we need multiple PRs for this

Because I assume CI won’t build and test every individual commits in the PR, so we don’t « make sure » of anything unless we do exactly this.

Thanks, I see what you mean now. There's certainly some risk there relative to directly committing the change, but unless the contributor is actively malicious, mistakes will manifest in rather obvious ways (e.g. the second commit has no test diff, because baseline checks were generated with the wrong build).

The migration to GitHub PRs is done for the benefit of people without write access

I dispute this: it has to be a very minor part of the reason, I hope. Processes should be optimized for the majority of the contributions IMO.

I do agree with the latter statement, but I thought one of the main motivations for using GitHub PRs is the better onboarding experience for new contributors, who will usually be familiar with the GitHub PR workflow and never heard of Phabricator before. If we wanted to optimize just for existing and experienced contributors, we could probably stay on Phabricator. (Though I think this discussion is a bit off-topic for this issue...)

Uh, we don't want to lose history? Like, if you split off an NFC refactoring in a separate commit from a functional change and then you squash merge both commits -- that kind of defeats the point? It's not like we want to have a separation between NFC and FC only during the review, it should also remain the history long-term

Baseline tests is what you brought up before: do you believe that history requires to have these in a separate commit? I don’t see why really: the baseline tests don’t make sense outside of the review context as far I as I understand.

Other kind of NFC are an entirely different story: this is stacked PR again.

I view baseline tests as one instance of the general "NFC followed by FC" pattern. Having commits show a codegen diff rather than just a test addition is as useful when you look at history years later as it is during initial review.

To give an example, if you have a test diff like https://github.com/llvm/llvm-project/commit/81ec494c363d4934e692e8b35e0b3fbbc3de1c2b#diff-22cc8d7b93b88098dc7ebb0bbbd6d133df4bf356c44b2d6664963b43b851f0f7 the only change is in the order of an instruction. Seeing it like that, it's clear what the previous codegen was and how the fix changed it. If this would just add a test case in the same squashed commit, you could see what the new codegen is, but it would be pretty hard to figure out what the old codegen was (you could go to godbolt, find a reasonably close version of the compiler and compare the output or something).

---

FWIW, this is of course caught up in the stacked review discussion, but I don't think these are orthogonal discussions that can be separated. The fact is that only allowing squash merges effectively disallows the best alternative to stacked reviews that GitHub currently offers, which is PRs with multiple commits.

I guess you could take my comments here to say that: For many of the stacked reviews we currently use in LLVM, PRs with multiple commits are an adequate (even better) replacement. They don't work well for all current uses of stacked reviews, but they do work well for many of them.

I believe we should leverage that (by allowing PRs with multiple commits and allowing rebase merges), and only go for more complex ways to approximate stacked reviews (e.g. multiple PRs of different commits in the same branch with "only review the last commit" comments) in cases that need that complexity.

[joker-eph]

The fact is that only allowing squash merges effectively disallows the best alternative to stacked reviews that GitHub currently offers, which is PRs with multiple commits. I guess you could take my comments here to say that: For many of the stacked reviews we currently use in LLVM, PRs with multiple commits are an adequate (even better) replacement. They don't work well for all current uses of stacked reviews, but they do work well for many of them.

Actually with the way I'm developing stacked PR locally, my best alternative to stacked PR is instead: multiple individual chained PRs where the branches are in the repo and not in forks. One new commit per branch (and so a single commit per PR): this is what is the closest to what we have today, the only regression is how GitHub UI sometimes mis-behaves when we amend+force-push. And this is even scriptable to make it work somehow automatically.

I don't see multiple-commits per PR as a suitable replacement for anything else than trivial review (how do you update a review that spans multiple commits over multiple rounds?).

[rengolin]

Actually with the way I'm developing stacked PR locally, my best alternative to stacked PR is instead: multiple individual chained PRs where the branches are in the repo and not in forks.

This only works if the developer has the right to create branches on the main repo, which requires write permissions.

Currently, most new targets start from people who do not have permission and are only granted after a successful number of merges and acceptance of the target. This isn't dissimilar to other efforts that need stacked PRs.

For this to work, we need to give write permissions to any external developer that wants to do a stacked PR against LLVM before they have the change to show minimum quality. This does not scale.

[joker-eph]

For this to work, we need to give write permissions to any external developer that wants to do a stacked PR against LLVM before they have the change to show minimum quality. This does not scale.

I agree this does not scale: but does it need to? How many new LLVM developers a submitting a new target every year?

[kparzysz-quic]

Actually with the way I'm developing stacked PR locally, my best alternative to stacked PR is instead: multiple individual chained PRs where the branches are in the repo and not in forks. One new commit per branch (and so a single commit per PR): this is what is the closest to what we have today, the only regression is how GitHub UI sometimes mis-behaves when we amend+force-push.

This was considered a while back, but the concern at the time was having a multitude of "private" branches in the public repo. We could set github up to auto-delete a branch after commit, but there may still be stale/abandoned branches left around. IMO this is not a blocker, but I'm not sure what other people think.

[Endilll]

I agree with the comment above. As a minor point, I really like the clean state of our git repo from branch and tag standpoint.

[rengolin]

We could set github up to auto-delete a branch after commit

This works if the branch is merged, not if it's abandoned (open) or closed.

Another point is branch naming. In the past, I've used user/feature/seq as naming to avoid clashes. But there's no guarantee people will read the docs, so in practice people will create whatever they want, and we get to be the annoying folks who ask them to rename all branches, etc.

How many new LLVM developers a submitting a new target every year?

Not many, admittedly, which I think makes it worse. We're opening a potential supply-chain violation that is hard to track for a case that isn't going to be widely used. If we restrict stacked PRs to existing writers, then this is perfectly fine. Also, we don't have a policy of removing committers, so in practice, this would increase the number of inactive committers.

Using Github PRs can make it easier to have less people with write permissions, by allowing auto-merge after approval and tests passing, which is the state that I'd like to see LLVM at. This means active reviewers and developers continue with write access and the rest just rely on their reviews and approvals.

This adds little to no overhead to the current process and considerably simplifies supply-chain analysis.

[felipepiovezan]

Using Github PRs can make it easier to have less people with write permissions, by allowing auto-merge after approval and tests passing, which is the state that I'd like to see LLVM at.

This breaks when there are multiple parties interested in a particular part of the code base. It is very common for a reviewer to quickly approve something and, about 24h later, a separate reviewer provides good feedback indicating the patch should not yet be merged.

[rengolin]

This breaks when there are multiple parties interested in a particular part of the code base. It is very common for a reviewer to quickly approve something and, about 24h later, a separate reviewer provides good feedback indicating the patch should not yet be merged.

Auto-merge doesn't have to be enabled by default. I think there's a way to "approve with auto-merge" and just "approve". That was what I was proposing, not always-auto-merge.

[rengolin]

Using Github PRs can make it easier to have less people with write permissions, by allowing auto-merge after approval and tests passing, which is the state that I'd like to see LLVM at. This means active reviewers and developers continue with write access and the rest just rely on their reviews and approvals.

Discussing this with @joker-eph offline, it seems possible to create a set of groups with write access to main while others writers are not able to merge, so a PR can only be merged by someone in that list. This is where the auto-merge comes in handy but isn't a requirement.

This seems to be strictly equal or better than our current process for people without commit access. Now, we approve and commit for others, then we'll approve and press "merge". It's strictly better between people with merge access: one approves, the other merges.

If we reduce the number of people that can merge to main, however, it will be strictly worse for people who used to be able to merge to main but lost the permission. But this can be a matter of permission removal policy and thus reduces considerably the cost, for example, based on last interaction (commit, review, release, etc).

An idea of the "tiers" would be:	Tier	Write (other)	Write (main)	Single PRs	Stacked PRs	Approval	Merge
Core	Yes	Yes	Yes	Yes	Yes	Yes
Active	Yes	No	Yes	Yes	Yes	No
Others	No	No	Yes	No	No	No

With "others" being new as well as inactive developers. With "core" and "active" having some yet-to-be-defined distribution.

I'm not claiming this is a good idea, or that the management is easy, or that it will be well received. I'm just saying it's "possible".

[Endilll]

Instead of restricting merge rights to Core tier, I'd rather require PRs to have an approval from someone in Core tier instead. That would reduce the load on maintainers, and prevent mistakes, because I'd expect the author to know best when to land a patch (merge a PR).

As an example, I've been working on improving test infrastructure for the sake of C++ DR testing, which resulted in 4 patches (D151426) to different placed (Clang, Clang test suite, split-file, lit) I've been working on for over a month. I've been holding off approved ones for a while (and it paid off), but we can't expect maintainers to remember the context to avoid making mistakes.

[rengolin]

Instead of restricting merge rights to Core tier, I'd rather require PRs to have an approval from someone in Core tier instead.

I may be wrong, but IIUC, the "merge right" comes with the "right to merge into main" which is what we're trying to restrict. But otherwise, I agree with you, whatever reduces the load on "core" people.

We don't really want this to be a hierarchical model like Linux or GCC. Nothing wrong with that model, just isn't what we've done so far and I'm not proposing we change it.

[Endilll]

"right to merge into main" which is what we're trying to restrict

While I'm sympathetic to your efforts, I find "restricting rights to merge into main" and "avoiding hierarchical model" goals very much conflicting, unless restrictions are applied more or less technically (like commit access we have today).

[tstellar]

@joker-eph See https://llvm.org/docs/TestingGuide.html#precommit-workflow-for-tests.

The tests on the first commit show the behavior without the functional change. The second commit only shows the diff introduced by the functional change.

If both commits are in the same pull request then they will not get tested separately by the CI builders, is that OK?

[kwk]

Hello. I feel like I'm jumping in a discussion and my point might be out of scope but somehow the stacked PRs "issue" that has come up often ties into this discussion of multiple commits or squashed ones. I've briefly looked at https://github.com/ejoffe/spr and it seems to address the stacked PRs problem quite nicely with github. I can imagine that github itself will someday build something similar to this into their gh client. Does anyone have experience with spr?

[tstellar]

@kwk There is a different ticket for stacked PRs: https://github.com/llvm/llvm-project/issues/56636

[tstellar]

Proposal:

We disable "Allow rebase merging" and for the workflow @nikic is describing with the pre-committed tests we use this process:

1. Create a pull request with both the test commit and the fix commit.
2. Get approval to commit the test commit.
3. Manually push the test commit only to main (i.e Using git push not the PR interface).
4. Click the "Update Branch" button on the Pull Request. This will rebase the branch and drop the test commit from the PR.
5. Get approval to merge the fix commit.
6. Squash and merge the pull request.

[rengolin]

This also works well for fixups before merge, not just tests. Separate PRs can also be used to avoid confusing the review and the approvals (if we require Github approval - which I recommend), then rebase the PR on main after merge the fix PR, rinse, repeat.

We can be quite flexible here, even with strict rules on what's allowed on main.

I'd also propose a similar rule to stacked PRs:

1. Create a PR from a single branch with all commits (no need to number them).
2. Review comments, update the branch, mark comments as resolved, recycle.
3. Once all comments for "this round" have been addressed, rebase manually, force-push, reset PR, new round.
4. Once approved, the author can do a squash-merge (Github button), or ask a committer to git push the branch instead (rebase-push).

No shenanigans with branches, no need for special groups in the Github admin interface, no special tools on top of Github.

[nikic]

@tstellar I think at that point it would make more sense to merge the whole PR manually. Merging just the first commit manually and the second one via the interface makes things more complicated.

Effectively this means that PRs by third-party contributors need to be merged via git am of the PR .patch file, which is close to what I would currently do on Phabricator.

A disadvantage of merging by hand is that the PR won't get marked as merged. You can amend Closes #N to the commit message to close the PR, but it will show as closed rather than merged then.

I'd also propose a similar rule to stacked PRs:

1. Create a PR from a single branch with all commits (no need to number them).
2. Review comments, update the branch, mark comments as resolved, recycle.
3. Once all comments for "this round" have been addressed, rebase manually, force-push, reset PR, new round.
4. Once approved, the author can do a squash-merge (Github button), or ask a committer to git push the branch instead (rebase-push).

I don't really understand the process you have in mind here. What is a "round" and what does "reset PR" mean?

[rengolin]

I don't really understand the process you have in mind here. What is a "round" and what does "reset PR" mean?

The biggest problem of Github with force pushes is that it gets confused with previous comments. Phab does that too, but to a less degree. Here "reset" means the previous comments will probably be lost due to a force push and "round" means between resets. It may not "reset" but it can, so it should be done when all of the comments have been marked as "done".

One suggestion was to not force-push, just add more commits to the end, but in a stacked PR, you want the history to reflect the work you've done, so eventually, you need to rebase squashing the fixups.

Or not, it's up to you. But if you do, then you should wait until you fixed all comments, so that they don't get lost, thus, "reset".

I think the main point here is that we should be stuck in how we do things today in Phab and blame Github because it doesn't work. We should evolve how we work and move on.