By way of background, note that we currently have documentation on the use of GitHub for reviews which includes guidance on how to update a PR. This guidance recommends against rebasing except for in specific circumstances, and makes no mention of merge commits.
In #76704 I attempted to make it more explicit that rebasing when the PR was made using a commit that fails CI as a base is useful (as the pre-commit CI results then show the pass/fail of the PR in question). @jyknight provided a great description of the potential use of merge commits instead, which it seems is already being used in a number of reviews. Apparently GitHubâs UI is more compatible with this approach than rebases.
Iâm creating this topic for two reasons:
To get input on whether we should encourage and document the use of merges for updated PRs. Iâm not the person to lead the charge on this, but hopefully this thread can at least start the conversation.
To try to unblock that PR, especially if deciding on 1) is going to be a lengthy process.
If the use of merge commits means that we can encourage merging in upstream changes much more frequently during review Iâd definitely be in favour (provided there arenât big usability barriers). For some types of patches, applying locally and kicking the tires is very helpful and also a good way to give better review feedback (e.g. exhibiting a missed optimisation). Now that rebasing patches is uncommon, youâre stuck either doing such experimentation on an older version of LLVM (meaning observations may no longer hold) or manually doing the rebase yourself which seems like wasted effort.
CC @jyknight@mehdi_amini from that thread (and âitrofimowâ who it seems isnât on Discourse).
This is about updating the PR branch when something changes in the main branch. The proposal is to merge main into the PR branch instead of rebasing the PR branch on top of main. When the PR is ready to merge, using squash-and-commit will remove the merge commits.
We have used that in another project that I worked on and it worked quite well.
Edit: Maybe I misunderstood your commentâwhen it comes to locally applying PRs, I usually fetch the PR branch from GH: pull/NNNNN/head.
+1 for updating PR branches via merge commits.
Weâve been using it on a project I previously worked on, and it worked out for us well.
Iâd like to note that recommending merge commits over rebase should help with recurring issue of people doing rebase wrong, generating a spam notifications to people who recently committed anything to main. Recent example is https://github.com/llvm/llvm-project/pull/76680, but I guess it has been happening daily ever since we opened GitHub PRs. (I donât commit to main too often, so corrections from those who have been exposed to this more are welcome)
I would highly recommend git fetch origin pull/74275/head instead of all that tedious mucking about with patch files. Then you can git checkout FETCH_HEAD or git cherry-pick FETCH_HEAD or whatever you want.
Ultimately, it depends on what youâre using PRs for.
If youâre working with a mindset of â1 PR leads to 1 commit on mainâ, then merge commits are ugly but donât cause any real problems.
If you have a PR that is really multiple commits (which is often useful!), then merge commits are obviously a very bad thing and you have to rebase instead.
I donât think one-size-fits-all is the right approach here.
Canât you merge while the PR is in review, and then do one rebase at the end? Wouldnât that remove the merge commits? I havenât tried it, just wondering.