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:
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.
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):
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.
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.
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).
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.
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).
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.
