Improve workflow when force-pushing during code reviews

[sebastianst]

The Github Flow article encourages pushing new commits while iterating over a pull requests during code reviews. However, this sometimes leads to a messy commit history and some teams prefer to continuously force-push clean commit histories rather than pushing little review commits.

This workflow doesn't work well with Github yet, unfortunately. In 2018, Github added a hyperlink to the word force-pushed in the conversation timeline to show the diff after a force-push, which is already a great improvement.

The following points are still missing in the Web UI to support smooth force-push review iterations:

• Add review comments to individual line(s) in the comparison view. Clicking on force-pushed shows the diff, but no line-comments can be made.
• Show all PR versions/heads in the comparison view, so that individual PR versions can be compared and reviewed.
• Show old line-comments on new force-pushed versions, if possible. This requires some sort of smart algorithm to match a line-comment to the new version.

Happy to add more points. Note that Gitlab and other platforms have better support for this kind of workflow for some time now.

Related issue: isaacs/github#999

If you want to see these features being implemented, please vote by clicking on the ⬆️ above the counter. Giving a 👍 does not count as a vote, see also #3474

[sebastianst]

Edit: Moved into "General Feedback" category, where it is well-placed. So the below discussion is obsolete.

Not sure if this was the right place to post this feature request 👉 Reposted to github.community.

[evi-liu]

Thank you for reposting to GitHub community! This space is currently reserved for GitHub Discussions feedback only.

[sebastianst]

Are there plans to open up github/feedback to other categories like the Pull Requests UI? I think the Discussions interface is nicer than github.community for discussions like these.

[aspiers]

See also isaacs/github#1985

[sebastianst]

I moved this post into the "General Feedback" category, where it should be well placed. This makes my original comment obsolete.

[victorlin]

I would also like to see less clutter on the PR page when iterating on in-progress changes. Especially for draft PRs – I'm reluctant to open drafts with incomplete changes to avoid clutter on the PR page. This defeats the purpose of having a "draft" state.

In Azure DevOps, there is a similar amount of verbosity in commits/force-pushes, but it's not so bad because the PR page displays updates with most recent at the top. I prefer that over GitHub PRs where you have to scroll all the way down just to see the most recent changes/comments.

[hazz0003]

Azure DevOps some how has the same information density with about half the clutter.

[Empact]

Documented my prior experience with this in #8808 - in my experience it's a significant source of friction between people who want to keep the review history and others who want to optimize for the source history.

[victorlin]

Github added a hyperlink to the word force-pushed in the conversation timeline to show the diff after a force-push, which is already a great improvement.

I don't know if this was a recent change, but the designers somewhat addressed the verbosity issue here by compressing back-to-back force-pushes with a message <username> force-pushed the <branch> branch n times, most recently from <commit> to <commit>.

However, the force-pushed link and Compare button only show the most recent force-push diff. There's no way to view the prior force-pushes, so this feels like an incomplete feature now.

[alpipego]

This should absolutely be expandable so each individual force push diff can be viewed. Moreover, often it would be more useful to show the diff of all the recent force pushes. I think the way the UI is done currently, it's impossible to get the individual hashes.

[danilaml]

Yes. It's actually a UX downgrade, since if one force-push was a rebase, and another was a fixup, it would combine their diffs into unreviewable mess, instead of comparing rebase with fixup.

[RalfJung]

Yeah, right now this "feature" means that it is impossible to reliably keep track of what changes in force-pushes. It's pretty bad.

Github might document "add new commits", but in my experience, a large fraction of contributors will use force-pushes anyway. It takes a lot of time an effort to teach them not to do that. It would be better if Github hat native support for standard git workflows such as force-push and rebase.

[alpipego]

One more thing that would be really helpful: The diff should not show rebased commits from the target branch of the PR.

If a feature branch gets rebased on the target branch and there are also code changes in the feature branch, the diff for the force pushes shows the changes of all commits from the target branch as well.

Since they have already been reviewed and merged, it's often very hard if not impossible for reviewers to identify the actual code changes of the push.

[Hnasar]

I sure hope this gets implemented one day! I think this is an area that GitLab does better than GitHub.

git on the command line offers range-diff (docs) which properly shows the diff between two sequences of commits and is a way that sensible changes can be seen, even in the face of force pushes and rebases.

This would be a killer code review feature for GitHub if it offered it in the web UI.

[RalfJung]

Another problem with the current situation is that when there were many force-pushes since a review, one has to look at all of them separately. It would be much better if Github kept track of the state (commit) of a PR when a review is submitted, and then offer a "show changes since this review" button that covers all the force-pushes (and other pushes) since then. Right now, I don't think there is even a manual way to get that commit, so I often have no choice but to re-review and entire PR, losing a lot of time.

[aspiers]

Unfortunately GitHub's PR flow of multiple commits per code review is fundamentally broken, and I doubt it will ever get properly fixed without changing the model. This was already clear in 2017 when I first submitted isaacs/github#999 (and also isaacs/github#997 and isaacs/github#998). At that point I'd used both GitHub and Gerrit for 5+ years, and it was already very obvious how much better Gerrit's 1-commit-per-review model was, even before Gerrit enjoyed substantial improvements in UX etc. So I fear this is flogging a dead horse.

One of the biggest flaws of this model is that GitHub has no stable unique id per commit in the PR, so it can't keep track of which commit is which during a force-push, since a force-push can add, remove, amend, and reorder commits arbitrarily. If a new patch-id appears, it's impossible to tell whether that is related to a previous commit. Consequently the UI has no way to present the change from the previous version of the PR in an intelligible way. Gerrit solves this by adding a stable unique id to every commit message as part of the review submission request, and it's a total game-changer.

This broken model leads to many additional problems, not least huge pains when trying to stack PRs on top of other PRs. Many attempts have been made to solve this, but unfortunately they are all just bandaids.

The real solution is to limit PRs to 1 commit, use stable uuids per commit, use git commit's graph to natively express dependencies between reviews, and provide tooling which makes it quick and easy to submit a chain of n commits as n PRs. In fact, it's possible to do this today using Gerrit with some extra GitHub compatibility plugins. IIRC open source projects can even get hosting for free from GerritHub. The drawback is that it's not a well-trodden path, so things like GitHub Actions will require effort to integrate.

[aspiers]

This is not what's being asked here.

It's what I'm asking for. I posit that any review system incapable of doing this is fundamentally too limited. If one only ever has to review small PRs which arrive infrequently enough not to have to worry about stacking, then OK this might be overkill. But for any serious software project of size and change volume this is an absolute necessity.

You only need to view the diff from state A and force-pushed state B

If by "state A" and "state B" you are referring to the tip of the PR branch, then I don't see how this satisfies any of the three requirements I stated above. It sounds like you are assuming that

• there are only small changes between A and B (which can be completely invalidated by a rebase if the base of B is significantly newer than the base of A)
• these small changes can be easily attributed to amendments of specific commits within the 20 just by quick visual inspection
• B hasn't introduced commits which make substantial changes and then following commits which cancel out the majority of those

But none of these are guaranteed true, and even if they are, to be sure I would have to re-review 20 commits every single force-push, which is hopelessly inefficient.

(so you could tell if force-push was a simple rebase or actually contained some changes that need a new review)

How exactly would you do this? Again it sounds like you are relying on visual inspection, but when you are dealing with a PR with a large number of commits this is hopelessly inefficient.

So you'd only see the changes between original and amended commit. That's enough to improve workflow dramatically and is currently possible to do offline via CLI.

I've tried this technique for a well over a decade at this point. Yes for simple cases it can be sufficient, but as soon as things get complex it's totally inadequate.

[modesto15]

Helping

[danilaml]

It's what I'm asking for.

Great. It's irrelevant, since it's not your issue and this discussion started with you disagreeing that GH has enough data already to implement what's being asked in the current issue, not some imaginary workflow you imagine.

It sounds like you are assuming that

I don't assume any of that.

these small changes can be easily attributed to amendments of specific commits within the 20 just by quick visual inspection

git blame

I would have to re-review 20 commits every single force-push, which is hopelessly inefficient.

No. You review a single pull request as a whole. The number of commits is largely irrelevant. They could be all squashed into one for all we know. It's no different from gerrit. It just allows to split work into logically different chunks (or keep "address feedback"/"fix formatting" commits to show the evolution of the PR until they are all amended before merge).

[aspiers]

It's what I'm asking for.

Great. It's irrelevant, since it's not your issue

It's not yours either. How is that relevant to anything? This is a public forum 🤷‍♂️

and this discussion started with you disagreeing that GH has enough data already to implement what's being asked in the current issue, not some imaginary workflow you imagine.

Actually no, it started in 2017 when I posted isaacs/github#999 and other related issues, from which this discussion is a continuation (as referenced by the OP).

The post of my recent post was to explain why GitHub's PR model has fundamental limitations which means it's very unlikely that many of the issues associated with force-pushing will ever be fixed.

these small changes can be easily attributed to amendments of specific commits within the 20 just by quick visual inspection

git blame

I would have to re-review 20 commits every single force-push, which is hopelessly inefficient.

No. You review a single pull request as a whole. The number of commits is largely irrelevant. They could be all squashed into one for all we know. It's no different from gerrit. It just allows to split work into logically different chunks (or keep "address feedback"/"fix formatting" commits to show the evolution of the PR until they are all amended before merge).

Sorry, but reviewing a single PR as a whole and doing squash merging is an entirely different (and simpler) workflow which has all the limitations I outlined above. If it works for you, great. Indeed, I already acknowledged that it can work OK in simple projects. But please don't try to claim that means this workflow also works as a substitute for an entirely different workflow, which is used by many of the top FOSS projects globally, including Chromium, Linux, and git itself (which I have personally been involved with in the past).

This conversation is unfortunately going nowhere, so this will be my last reply to you.

[RalfJung]

I don't see how this is sufficient. If a rebase followed by force push amends a commit to slightly change its contents and reorders the sequence of commits, how can you link the new version of that commit with the old one?

You compare the new version of the PR against the old one. This thread isn't about linking commits, it's about force-pushes to PRs.

It's what I'm asking for. I posit that any review system incapable of doing this is fundamentally too limited.

Then you should start a new discussion. This one here is specifically about how force-pushes interact with the existing PR system. Saying that review should be per-commit is entirely off-topic.

Yes, this is a public forum, and in a public forum it is generally frowned upon to derail a discussion by bringing up a loosely related but clearly distinct topic.

The relation of this topic with isaacs/github#999 is in the part where it says "if you amend PR branches via force-push, there is no way in the web UI to see the difference between the versions of the PR". A proper range-diff between the two PR states would easily solve that. Nothing here says or implies "PRs should only have a single commit". And in fact if PRs had a single commit, that alone would solve nothing! We'd still need range-diff to compare two versions of that commit. So a single-commit-per-PR policy is neither necessary nor sufficient for the problem at hand, and therefore entirely off-topic.