Define a policy for merging Pull Requests with multiple commits #56637
Comments
|
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:
Additional context:
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. |
|
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). |
EugeneZelenko
added
infrastructure
|
@llvm/issue-subscribers-infrastructure |
Not doing force pushes means:
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) |
|
I feel like there shouldn't be one single policy for a given PR, because different cases have different needs. Outlining my personal requirements:
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. |
|
@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 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:
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. |
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...
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 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". |
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.] |
|
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:
I don't see how the GitHub UI can show "a minimal scoped change making it easy to review" without user branches. |
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. |
|
You wrote earlier:
with:
I feel I am missing something because I read a direct contradiction. |
|
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. |
|
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:
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? |
|
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. |
|
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.
|
|
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. |
See discussion in #56637. Reviewed By: ldionne, jhenderson Differential Revision: https://reviews.llvm.org/D147284
|
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. |
|
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. |
|
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. |
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? |
|
@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. |
|
@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. |
|
The reason for these separate commits is to ease the review isn’t it? If so then squashing shouldn’t be a problem. |
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.
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.)
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.
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. |
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)
Can you elaborate on the part that breaks with squash merge? It’s not clear to me right now. |
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.
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.)
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.
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. |
|
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". |
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.
I dispute this: it has to be a very minor part of the reason, I hope.
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. |
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).
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...)
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 81ec494#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. |
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?). |
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. |
I agree this does not scale: but does it need to? How many new LLVM developers a submitting a new target every year? |
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. |
|
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. |
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
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. |
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. |
Discussing this with @joker-eph offline, it seems possible to create a set of groups with write access to 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 An idea of the "tiers" would be:
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". |
|
Instead of restricting merge rights to Core tier, I'd rather require PRs to have an approval from someone in Core tier instead. 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, |
I may be wrong, but IIUC, the "merge right" comes with the "right to merge into 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. |
While I'm sympathetic to your efforts, I find "restricting rights to merge into |
If both commits are in the same pull request then they will not get tested separately by the CI builders, is that OK? |
|
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 |
|
Proposal: We disable "Allow rebase merging" and for the workflow @nikic is describing with the pre-committed tests we use this process:
|
|
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 We can be quite flexible here, even with strict rules on what's allowed on I'd also propose a similar rule to stacked PRs:
No shenanigans with branches, no need for special groups in the Github admin interface, no special tools on top of Github. |
|
@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 A disadvantage of merging by hand is that the PR won't get marked as merged. You can amend
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. |
The text was updated successfully, but these errors were encountered: