Squash-merging and other problems with GitHub

By , August 16, 2017 6:45 pm

(Thanks to Ben North, Colleen Murphy, and Nicolas Bock for reviewing earlier drafts of this post.)

In April 2016, GitHub announced a new feature supporting squashing of multiple commits in a PR at merge-time (announced on April 1st, but it was actually bona-fide 😉 ).

I appreciate that there was high demand for this feature (and similarly on GitLab), and apparently that many projects have a “squash before submitting a PR” policy, but I’d like to contend that this is a poor-man’s workaround for the lack of a real solution to the underlying problems.

Why squash-merge?

So what are the underlying problems which made this such a frequently requested feature? From reading the various links above, it seems that by far the biggest motivator is that people frequently submit pull requests (or merge requests, in GitLab-speak) which contain multiple commits, and these commits are seen as too “noisy” / fine-grained. In other words there is a desire to not pollute the target/trunk branch (e.g. master) with these fine-grained commits, and instead only have larger, less fine-grained commits merged.

But where does this desire come from? Well, if the fine-grained commits which accumulate on a PR branch are frequently amendments to earlier commits in the same PR (like “oops, fix typo I just made” or “oops, fix bug I just introduced”) then this desire is entirely understandable, because noone wants to see that kind of mess on master. However the real problem here is that that kind of mess should have never made it onto GitHub in the first place – not even onto a PR branch! It should have instead been fixed in the developer’s local repository. That is why there is a whole section in the “Pro Git” book dedicated to explaining how to rewrite local history, and why git-commit(1) and git-rebase(1) have native support for creating and squashing “fixup” commits into commits which they fix.

Use the force-push, Luke

If an existing PR needs to be amended, make the change and then rewrite local history so that it’s clean. The new version of the branch can then be force-pushed to GitHub via git push -f, which is an operation GitHub understands and in many situations handles reasonably gracefully. I have previously blogged about why this way is better, but one way of quickly summarising it is: don’t wash your dirty linen in public any more than you have to.

In other words, if you do your history rewriting locally, there will be a substantial increase in the quality of what you publish to the PR, because you will only ever publish commits which (at the time) you believed to be the final candidate, ready for merging. (And later, when you benefit from the hindsight of peer reviews and/or your own realisations about how to make things better, you simply force-push a new final candidate.) Consequently, people reviewing your PR don’t have to waste time reviewing anything unless it was a candidate for merging, and nor do CI systems.

Granted, there are some exceptions to this approach, where you really do want to publish a PR which is still work in progress in order to get early feedback. In that case you can explicitly state in the PR description (or via a label) that this PR is WIP so that anyone reviewing it will know to expect some mess, and that’s totally fine.

The force-push is not so strong with this one

Unfortunately, the force-push approach is not perfect either: the full history (a.k.a. reflog) of the PR branch is not exposed via GitHub’s UI, nor are there even guarantees that it will be preserved. This is one of GitHub’s serious flaws, which in contrast Gerrit gets right, and services integrating with GitHub such as https://reviewable.io/ and http://gerrithub.io/ can also help fill in the gaps here. The poor man’s workaround is to manually add comments every time you force-push, which explain what changed and how the changes address feedback from reviewers, but this of course requires discipline and extra effort, and does not show the exact changes, so it’s far from ideal.

I’ve got a bad feeling about this

These drawbacks with GitHub are another reason that some people prefer to append fixup commits to the PR and then squash at merge-time – at least then the history of the PR is visible to all. But this approach only works with PRs which represent a single logical change. If you squash multiple logical changes into a single commit, you lose valuable history. (In fact, this value can extend as far as using the history for literate programming.) So I strongly recommend keeping the GitHub repo configured with the default setting which allows normal merges, so that you don’t force this history loss on merge of PRs.

In response to the previous issue, you could decide a policy where you insist on clean commits and normal merging if the PR contains multiple logical changes, but allow unclean commits and squash merging if the PR contains only a single logical change. However the decision whether to do squash merge or normal merge is left to the merger, and it’s made at merge-time, outside the peer review process. So on a bigger project where many people have merge permissions, that reduces the chance of merges being done in a correct and consistent way.

By the way, I’m not the first person who has highlighted these problems.

I find your lack of dependencies disturbing

OK sorry, that’s enough Star Wars quotes.

But this awkwardness highlights another fundamental issue with GitHub: it does not conveniently support “stacking” one PR on top of another, i.e. where the head of the first is the base of the second, with a dependency modelled between the two. (You can do it, but it’s awkward when the PRs come from different forks, and you also have to change the base branch of the second once the first is merged.)

So the only convenient way to group two dependent commits together is to put them in the same PR. This encourages the creation of PRs with many commits, which suck up more and more review effort each time their history is rewritten, and take longer to merge on average, because controversial and non-controversial commits tend to get mixed up within the same PR, and the former block the latter.

This is another area where Gerrit beats GitHub, since Gerrit natively supports dependencies between reviews, making the need for multiple commits per review redundant. Another advantage of this is that you don’t have to write a separate description for the PR, since all the info the reviewer requires is provided in a single commit message.

Anyway, I digress; back to squash merging. Another issue with it is that the final editing of the squashed commit message is after the review process, not during. If, like me, you work on projects which care deeply about the quality of the commit message, this is a deal-breaker.

Finally, the GitHub web UI doesn’t show when squash merging has been used, which can create confusion.

Summary

To wrap up, there is no perfect solution right now. However the following guidelines should help:

  • Always adhere to best practice for constructing git commits.
  • Try to keep your mess to yourself as much as possible, by rewriting history locally before publishing to or updating a GitHub PR.
  • Squash merging will not work well for projects with stricter review policies, or projects which receive PRs comprising of multiple logical changes.
  • Squash merging might work OK as a workaround for smaller projects with less stringent review policies, where PRs only ever consist of a single logical change which can reasonably be squashed into a single commit without harm.
  • Keep PRs as small as possible, and aim for quick, short review cycles.

And in case the right people are listening:

Share

2 Responses to “Squash-merging and other problems with GitHub”

  1. In our team we also have a desire for either squashes or forced push. I even implemented the support for different merge modes in the tool we use to merge pull requests: https://github.com/christofdamian/plus-pull

    I am not a fan of either. I rather have a dirty history than no history at all.

    The problem with forced pushes in pull requests for me is that I might have requested a change to a pull request. When this is then force pushed I have to read the whole patch again instead of just checking the most recent commit(s).

    Somehow I don’t see a good solution for this anytime soon.

Leave a Reply

 

Panorama Theme by Themocracy