hi,
Usually we need precommit a test to review, so after the PR reviewed, there are more than 1 commit.
But I only have the permission to choose Squash and merge, need I apply for additional permission to enable other options ?
This is intended: a pull-request should contain a single change, so a single commit.
We review the pull-request title and description as the expect final commit message.
Unfortunately, the only way to merge multi-commit PR is currently the following process:
git checkout your-branch
git fetch upstream
git rebase upstream/main
git push your-fork your-branch -f
git push upstream your-branch:main
The important part is that you force push the branch on your fork and then push the same ref to upstream. This way the PR will be marked as merged, and GitHub will add back-links from the commits to the PR.
If additional commits land between force-pushing to your fork and pushing to upstream, you need to repeat the whole process again until it succeeds.
It would of course be much easier and less error-prone if LLVM just enabled the “rebase” option to automate this. Currently we are losing the commits ↔ PR association on many pull requests because people fail to use the above process and just directly push their commits to upstream, and then manually close the PR.
Thanks very much @nikic @mehdi_amini
Should we add @nikic’s solution to LLVM GitHub User Guide — LLVM 18.0.0git documentation ?
SGTM although I’m unclear what the upstream part refers to.
Typically a review would want to squash several commits together, which I guess you could construct on a separate branch and then rename things back before doing the force-push. As the PR is looking at branches based on name, that should work?
I guess you should add the (#12345) thing to first line of commit message, in your local commit when doing it like this?
I assume that github won’t add it automatically when just pushing to main. Right?
(The thing with how github isn’t using the commit message from the commits in the pull request, but rather having the default behavior of gathering pieces from the first comment in the pull-request, combined with the title, plus adding “(#12345)”, but not until merging the commit is a bit annoying. It’s more or less impossible to “review” the commit message. And IIUC, even when using “gh pr merge” the merged commit message won’t be the same as I’m seeing locally. Maybe those things should be documented somewhere as well as long as we recommend using “gh”.)
While it is not part of the commit message, GitHub retains the link to the pull request when it is merged. You can see it below the commit message here:
This happens as long as the PR counts as “merged”, which is why it’s important that the same ref gets pushed to the PR branch and main.
Ok, but 99.9% of the time I’m not in the github world. So when I for example do “git log” locally and find some commit, and it lacks a reference to some code review, then I usually assume that there is no associated code review. And then I don’t need to waste time on trying to look up that commit in github.
In the past, with phabricator, I even got the full URL in the commit message. So it was “one click away” to get to the review. Now I need to open github, paste some SHA-1, and then maybe I’ll find an associated pull-request, and then I can follow that link. IMHO that is alot more work, specially just a waste of time if the commit was pushed without review.
Uh… This is quite the opposite IMO: when I look at a PR in the UI, I know immediately what the post-merge commit message will be (when GitHub squash/merge the PR): WYSIWIG.
This is the easiest way for me to review it!
It also isn’t really “gathering pieces from the first comment in the pull-request”: it takes the title and the description (it’s not a comment), which is exactly how git commit convention is defined.
https://llvm.org/docs/GitHub.html#updating-pull-requests
you must use the pull request title and description as the commit message. You can do this manually with an interactive git rebase or with GitHub’s built-in tool. See the section about landing your fix below.
What you see locally does not matter, since the reviewer wouldn’t even see it in the first place.
Well. What did not work for me was this:
-
I opened a pull request with one commit (used “gh pr create” to upload it).
Made some fixes, and added a second commit to the branch in the fork. -
Then, when it was time to land the patch, I squashed commits locally. And updated the commit message to cleanup given the squash. This included fixing some spelling in the first line of the commit message. I updated the fork by force pushing my branch, that now had a single commit.
-
Looking at my pull requst in github, it now had a single commit, with my updated commit message. But then when selecting “Squash and merge”, it gave me a commit messages that looked like the original commit messages, used several weeks earlier when originally creating the pull request. This is what I found annoying.
I mean, when you review the pull-request I would assume that reviewing the commit message actually would involve looking at the commit message for the commits on the branch that is going to be merged (given by https://github.com/llvm/llvm-project/pull/xxxxx/commits). Assume that we would allow merging without squashing. Then there will be multiple commit messages, and if one can’t assume that it is the commit messages on the branch that will be used then github just, in my opinion, “complicates things”.
The thing that github uses text from the “Conversation” for the commit message, instead of the text seen in the “Commits” view, is just unintuitive to me at last.
GitHub uses whatever it’s configured to use. LLVM has configured it to use the PR’s description, rather than a Frankenstein amalgamation of each commit’s message, because (a) that mirrors what happens in Phabricator where arc patch takes the review’s summary (b) it avoids having a flood of commits of the form “[Foo] Do X\nfixup! [Foo] Do X\nbug fix\nreview feedback\n…” which is inevitable if you use GitHub’s other option for Squash and Merge (I’ve seen that happen, it devolves into a mess very quickly, and even if people are careful it’s easily done accidentally).
2 Likes
You could have just skipped this, you don’t need to do anything like that locally: no squash, not commit message editing. It all happens in the PR itself.
No: this isn’t needed, we can just ignore this entirely.
Why would we have reviewers jumps through hoops when the information is just perfectly displayed in the landing page?
I’d add that this is exactly the feature! Why should it use something you just edited locally when the review happened and the reviewer saw the PR description and not what you were just doing?