How's it going with pull requests?

In general: the UI is painful, just as many people with direct GitHub experience warned about. There is many clicks to do the same thing as what was straightforward on Phabricator, on top of what others complained about, I feel the fact that you need to reload a page to go from the main page to the diff is sluggish, not being able to approve a PR from the main page annoys me all the time (I review the diff, go back to the main page to review the description/title and check the build-status before approving, but now I need to go back to the diff!).

On the topic of force-pushes, here are two aspects that are rough edges:

  1. When the author force-pushes, the email I get from GitHub that says “@author pushed 1 commit” has a "View it on GitHub " link that is always leading to “We went looking everywhere, but couldn’t find those commits.” That is supposed to be the link that shows the incremental diff from this push.

  1. Next to each “conversation” (inline review) in the main page for a PR, there is at the top-right a “view reviewed changes” link which allows to see the review comment in the context of the diff that was actually reviewed (before the push):

This link does not work with force-pushes. For example, here is a “conversation” on a PR without force-push: [mlir] Add Python bindings for DenseResourceElementsAttr. by stellaraccident · Pull Request #66319 · llvm/llvm-project · GitHub
The “view reviewed change” link leads to:
Note the suffix in the link, and how if you go this page it says “changes from 2 commits” (the PR has 7 now).

This is a PR with force-push:
The “view reviewed change” link is ; note how there is no suffix in this link, it shows the current full diff of the PR after the force-push, the original context is entirely lost.


Agree. I have tried today. It requires a setting (@tstellar @endill):

Failed to publish: GitHub error 403 on POST Although you appear to have the correct authorization credentials, the llvm organization has enabled OAuth App access restrictions, meaning that data access to third-parties is limited. For more information on these restrictions, including how to enable this app, visit Managing OAuth access to your organization's data - GitHub Docs

We should create another thread to discuss reviewable. I think it 's worth trying, but we probably need to understand what will happen when we turn it on. It needs certain Org permissions, for one, and it may make using GitHub for review more difficult.


I believe there were also references to, not sure if someone made a test-drive.

1 Like

Does that work for you? When I force-push multiple times, github just displays “force-pushed N times, most recently from commit X to Y”, and one can only see the diff for the last part. So if I do two fixups in a row this way, the first one is lost and reviewers have no good way of covering it.

An approach I have seen is to discourage force pushes during review (so that we get these “fixup” commits), but then once review is done, ask the author to squash the commits without doing any changes to the actual diff. This avoids having those fixup commits in the permanent git history.

Btw, do you all have some sort of contact with Github to inform them of what the main pain points are? On the Rust side I know there are some contacts between people in the infra team and people at Github, and while Github is huge and slow-moving, it could still be worth giving them some sort of feedback. If they hear the same complaints from multiple big projects, they might consider doing something about it. In particular the issues with force-pushes (like not being able to properly diff them) are definitely shared with Rust. I’m also frequently frustrated that going from a line comment in the discussion view to the same comment in the diff view is completely manual – there is a link “view reviewed changes”, but it often doesn’t work at all (just jumps to the diff view, not any particular part of it), even when it works it just jumps to the file containing this line, which is pretty useless for large files with many comments. It should really jump to exactly the same comment in the other page. Stacked PRs are much less of a concern for us, maybe because we’ve always been on Github so they haven’t really become part of anyone’s workflow.

Ah, I mostly use GitLab right now and missed that GH started bundling consecutive force-pushes together. There are still usually ways around that. I.e., if you change the .. comparison to a ... one, then the diff won’t include commits that were rebased upon. And I think if there was some time between the force pushes, then GH won’t combine them into a single diff. So most of the time you can still review the changes done in a force push. Anyway, I don’ think there is much reason to rebase during review until the last step before merge for a typical patch.

We do have contacts. And share out feedback. Though, nothing really happens these days (it used to be different) as they are having their own roadmap :wink:

I don’t think this is actionable. I strongly object to any policy to discourage force pushes. I have said the following in my post

In a large repository, avoiding rebases may not be realistic due to other commits frequently modifying nearly identical lines. Some people use a remote branch to save their work. Having to worry about whether a rebase could cause spam makes the branch more difficult to use. When working with both the latest main branch and the pull request branch, switching between branches results in numerous rebuilds. Rebasing follow-up commits could lead to merge conflicts and more pain. In addition, a popular convention among many LLVM contributors is to commit tests before landing the functional change, which also mandates force-push.
Sometimes, only through rebasing can one notice that the patch needs adjustments to more code or tests due to its interaction with another landed patch.

I have seen quite a few times patches that haven’t been rebased recently caused build breakage (not handling an enum that just got a new use, changing a function that just got a new use, etc) or test failure (someone else added a test that needed adjusting by the patch).

Improve workflow when force-pushing during code reviews · community · Discussion #3478 · GitHub (“Improve workflow when force-pushing during code reviews”) is quite relevant.

Another issue I am recently aware of is that GitHub’s pull request merge UI truncates a commit message’s title to 70 columns and may cause strange wrapping: [llvm-libgcc][CMake] Refactor llvm-libgcc by ur4t · Pull Request #65455 · llvm/llvm-project · GitHub

It happens during creation of the pull-request, but the user can manually undo it (and you’ll see it during review). The “squash-and-merge” button does not wrap anything in my experience.

Is there a way to leave comments on collapsed lines with Github PR’s? I find myself wanted to point out code that should be moved or removed, but I don’t see any way to do this without saying “Line x should be removed” if it hasn’t been modified in the diff.

No, Allow PR reviewers to comment on and suggest changes to unedited lines of code · community · Discussion #4452 · GitHub and Allow PR reviewers to comment on *unchanged* files · community · Discussion #9099 · GitHub are about this problem.

1 Like

One significant annoyance I’ve run into is the time wasted on not being able to reopen a closed PR. Consider: Diagnose problematic uses of constructor/destructor attribute by AaronBallman · Pull Request #67360 · llvm/llvm-project · GitHub

PR was posted, reviewed, merged, and reverted. Conversation continued on the PR as to ways to address the issues found. In Phabricator, we would reopen this review, upload a new diff, and continue the review without losing any context. In GitHub, there doesn’t appear to be any way to reopen a closed PR. So the only option is to open an entirely new PR and start over from scratch. This loses all of the previous context from the first review, and it makes it harder on reviewers to tell whether their feedback has been addressed or not.

Does anyone have a better solution than “start over from scratch?”


You can reopen a closed (red in the UI) PR, but not a merged (purple in the UI) one. Unfortunately that distinction matters. I’m not aware of anything clever you can do (unless one were to write some horrendous bot to replay the activity of the original PR).

I don’t know if you got a reply, but no, github never deletes the PR branch once you create it. It always lives at refs/pull/xx/head for all time afterwards. With effort I think you can also recover every past state of it too from accessing what is roughly a reflog in the github API, since every pushevent records both the before and after SHA (GitHub - criswell/github-reflog: reflog like interaction with Github, Recovering a commit from Github's Reflog | Object Partners, GitHub event types - GitHub Docs)

1 Like

Mitchell Hashimoto just published a nice short article on the lack of ‘changesets’ in GitHub PRs, that perhaps outlines what is needed and the benefits more concisely than I have in the past. This might be useful to point to in future conversations. Though I imagine it’s a request GitHub have heard countless times as it’s the most obvious deficiency when moving from Phabricator or Gerrit.