I’m confused and I feel like I’m missing something.
It sounds like many of the people who do not like this RFC are complaining that having to create a PR adds an extra burden and a lot of work? But creating a PR is extremely quick and easy: Go to the URL that the ‘git push’ command gives you and click a button. That takes just a few seconds. If I’m not interested in waiting for a review or premerge tests, then I can immediately click another button to merge (commit) the PR. All told that’s one cut & paste & two button clicks, taking ~30 seconds.
So where is all the extra time and work that people are discussing? What am I missing?
It seems like the benefits of this proposal (easier workflow for releases; being able to see & monitor post-commit reviews for the first time; automatic running of premerge testing, if desired; etc.) far outweigh the extra 30 seconds it takes to create a PR. Don’t they?
This will be a massive improvement, since we will be able to to do a rebase merge right from the UI, and it will make it much easier to enforce passing CI. Without this we have to manually merge each PR using a script or by hand. It’s very error prone and tedious.
I’m unsure on this, so have not weighed in so far, but perhaps an illustrative example of when I tend to make direct commits is useful.
I go to my local checkout and either create a new branch off the latest main in order to write a new fix or update to the latest main to test something, and when I come to do my initial ninja or ninja check before possibly modifying the tree, it doesn’t build / pass the tests. If it’s an obvious fix I can just make it, git push (after a git pull --rebase because you’re all too productive), and get on with my work, all within the terminal. But in this world I have to create a new branch, push that, file a new PR and merge it. Often these are particularly mundane fixes (lately it’s repeatedly been “DXIL doesn’t build when enabling the split shared libraries build”, because that’s my build config and I enable experimental targets to be a good citizen) and I’m already annoyed that I’m having to waste my time fixing someone else’s mistakes rather than whatever I was actually trying to do, so more friction in pushing that change will just add to that. Now, is that a lot more friction? No, not really. But it is non-zero, so I don’t think it should just be discounted, and the benefit for something like that kind of fix is truly zero (nobody is interested in reviewing me adding a missed dependency to DXIL’s CMakeLists.txt, and no pre-commit CI is going to turn up anything other than me making a spelling mistake that will also break my local build, if there’s even pre-commit CI for DXIL at all), so it does feel like jumping through hoops in the name of policy. But I also recognise that, unless you have a blanket policy like that, people will disagree on what a “reasonable” change to make is. I suspect nobody here actually believes there’s benefit to having such changes go through a PR; rather they don’t want people being able to judge certain commits as ok that aren’t?
+1 I’m strongly supportive of this effort, and some of the future improvements. I’ve found LLVM to be more of an outlier w.r.t. its contribution model than other open source projects.
Pull requests have some benefits due to the way the GitHub works: they provide a centralized place for discussion, automatic tagging, auditing, and other workflow features. That’s probably enough incentive to push for this.
Command-line tooling for interacting with pull requests exists, but we don’t have any official tooling or documentation for it. If we have a command that automatically makes a pull request and merges it/marks it mergable, that would be helpful.
Do stacked pull requests work without access to direct pushes? Presumably you can make tools that work without direct push access, but I’m not sure how the tools currently work.
Some of the claimed benefits make a bunch of assumptions about how we expect people to interact with the project in the future. Like, this is asserting we want mandatory reviews and mandatory CI, but those impose significant overhead on maintainers, and it’s not clear who is pushing for this.
I don’t follow, how is this related to this RFC? Isn’t the problem with merging release PRs that we disable rebase merge? I don’t think this RFC proposes to enable rebase merges?
This RFC shouldn’t really impact the release backport workflow at all, as that one already always goes through pull requests.
You can specify per-branch merge strategies (e.g. squash and merge for main, rebase and merge for release/*), but only if you have enabled the “Require Pull Requests” ruleset for the branch.
It sounds to me like we get a meaningful number of benefits and added consistency from requiring PRs. I particularly like David’s notes on how this can make post-commit review more transparent, efficient, and accessible.
I agree it’s not great that this will add drag to some flows, but Mehdi’s script gives me hope that that can be brought down to an acceptable overhead (I’m guessing a dozen or so seconds per PR that otherwise would’ve been a direct git push to LLVM?).
I see this change as being mechanical and incremental, and I’m supportive. I think standardizing on pull requests as the mechanism to land changes will help us reduce friction and keep the project running smoothly.
Many folks have raised concerns about the possibility of future mandatory two-party (2P, the author counts as one) code review, but we should absolutely have that conversation when that’s what’s on the table.
I 100% share this concern! Hence the effort (stop and go) that I’ve put into the dylib RFC, to focus specifically on reducing LLVM build compute resource requirements. I think it’s critical to the long term health of the project that it remains accessible to students and researchers.
However, I think using PRs as the mechanism to directly land changes (not imposing two-human review requirements) has the potential to make the project more accessible to hobbyists and beginners. If we make that the main pathway for landing changes, hobbyists don’t have to worry as much about platform specific tests, CI requirements, etc. Having your patch reverted because it broke ARM or Windows or some platform you don’t have access to has always been a huge pain point, and make self-approved PRs the preferred way to land changes means you aren’t obligated to have as much hardware or think as much about portability.
Requiring PRs also has the benefit that the build and tests will work more often when updating LLVM. I assume this is another major stumbling block for new contributors: you check out LLVM, and things don’t all pass.
Ability to revert PRs from GUI without modifying git tree in my dev environment saves me from rebuilding my local context. Given the proposal is narrowly scoped to main branch alone and that it does not add any review requirements to PRs, I am struggling to understand the concerns against this proposal. FWIW +1 from me.
I went through discussion about impact of this proposal on release process here and on Discord, and here’s my understanding. Hopefully it’s going to help others understand the situation better.
I believe the end goal is to enable “Rebase and merge” option for PRs targeting release branches. It seems that we already have a “ruleset” for release branches that require PRs, restricting direct pushing. Additional options for requiring PRs can restrict which merge methods are allowed. To illustrate:
One would think that you can tick “Rebase” here and be done, but merge methods in this list do not work unless they are enabled globally for the repository. Which leaves us with the following options:
Status quo. Release managers have to emulate “Rebase and merge” via a script, which Tom described as tedious and error-prone.
Enable “Rebase and merge” without accepting this proposal. This merge option will become available for both release andmain branches. I believe enabling it for main will help people who maintain stacks as commits in a single PR, but certainly warrants its own RFC.
Enable “Rebase and merge” without accepting this proposal and disable “Rebase and merge” by requiring PRs for main branch, but adding all the people with commit access to the list that can bypass the PR requirement. The side effect is that every direct push to main will be greeted with some kind of warning message. Tom reported that when he enabled it, there was backlash from the community, so he reverted the change. (Kudos for listening to the community!)
Enable “Rebase and merge” while accepting this proposal follows Option 3, but without the bypass list.
My opinion here is that there are viable options to improve the release manager workflow without accepting this proposal. I think that we should go for Option 3, because there’s nothing wrong in a warning telling people to go through PR workflow (exact wording of the warning is to be confirmed, because I can’t find previous discussion on this).
Maybe in couple of years, when individual subprojects would be pushing their contributors towards leveraging robust and useful CI via PRs for some time and seeing success in the dwindling number of direct pushes, we’ll be in a much better position to go forward with this proposal.
One thing worth noting here is that if this configuration is used (require pull requests + bypass + only squash allowed) then actually both the squash and the rebase options become available in the UI – but the rebase option works like all other bypasses, i.e. requires you to tick a red checkbox to indicate that you know what you’re doing. I think this would actually be a pretty nice feature, because it would make rebase merge available for the cases where it makes sense, while still preventing people from using it by accident.
FWIW I’d also be much more supportive of enabling this, but allowing to bypass it indefinitely (or until there is a separate RFC about actually enforcing it).
you don’t need permission to push to main to create a stacked PR. All you need is permission to create a branch like users/<user-name>/<branch-name>. Afaiu this wouldn’t be affected by this RFC.
I think it would be great to add a tool for this upstream (as in, in the LLVM git utilities). This will help people to evaluate now how the workflow is going to look like once this RFC is accepted. And even if the RFC is not accepted, it will still be useful for people who want to use it voluntarily.
For this kind of script, the feature’s I’d like to see supported are:
Support both immediately merging the PR, and enabling auto-merge instead. I think having a low-effort option to push something only after pre-merge checks pass would be quite valuable.
Allow pushing multiple commits. Under the new model that would require opening up PRs and merging them in sequence.
I think the latter case is something especially important to support. If, as part of some work, I accumulate a sequence of 5 NFC commits, replacing a single git push upstream main with five minutes of fiddling with git commands and GitHub UI would be a massive workflow regression. We need scripting support to still make this viable in the new world order.
I agree this tool should most likely be part of the llvm repo - but I would like to make a plea that we write scripts like this in Python so that they can be used on Windows as well without having to resort to WSL or similar thing.
