Squash when merging?

My $0.02: I am in agreement with what @jryans , @gus-massa and @mflatt write about how commits should look and @greghendershott 's "JIT squash" comment but I wanted to add a comment about a rationale (which is implicit in @mflatt 's message).

I think that our goal with the history that appears on the main/master branch should be that it is the most helpful it can be to some future programmer who is looking backwards trying to track down some bug (or otherwise understand the current state of the codebase). In that sense, a linearly ordered sequence of individually meaningful commits is the most helpful and so that's probably how the current (implicit) policy came to be.

It seems to me, based on this conversation, that this policy and the rationale for it aren't obvious to folks (especially given that other projects do other things) so the policy and the rationale seem like a good candidate for Style Guide inclusion (perhaps via an addition to this section or a nearby revision somehow).

5 Likes

I agree with everything in this thread, I think.

A recent example that I am glad was not squashed when merging is Add `#:indent` to `write-json` [reprise] by LiberalArtist ยท Pull Request #4686 ยท racket/racket ยท GitHub, which consisted of four commits:

  1. [json]: Add `#:indent` to `write-json`. ยท racket/racket@92ba659 ยท GitHub
  2. json: add tests for `#:indent` ยท racket/racket@d10c2fc ยท GitHub
  3. json: fixup indentation of objects ยท racket/racket@6f8824d ยท GitHub
  4. json: remove `#:indent` test datum 2155a30 ยท racket/racket@7e9b9c6 ยท GitHub

The first commit was written a year before the rest and by a different author. (I did squash the 11 commits from the earlier PR when picking it up.)

I'm especially glad the third commit was not squashed because there was a lot of discussion in the earlier PR and at Json Beautifier - #2 by LiberalArtist about exactly what behavior the #:indent option should implement. I imagined it could have been horribly confusing to the future bug-hunter not to note in the history the subtle change from the almost-correct behavior.

The last commit was a more marginal case, but the underlying problem confused me enough to err on the side of being explicit.

1 Like

(Replying to this message because it seemed the most relevant, but really these are just some free-form thoughts.)

I really like the way the Git community works with people who submit patches: they give feedback not just on the code, but on the structure of the patch series (analogous to a PR). If a commit message needs work of if a commit should be split, squashed, etc., that becomes part of review. It's expected that you resubmit several times (if necessary) to incorporate appropriate feedback. On GitHub, you would do this by rebasing (--interactively, if needed) and force-pushing to your PR.

Fortunately, the same community is usually pretty willing to explain how to accomplish these things, or at least point you to the docs and help you figure out what you need to do.

On the maintainer side, this is maybe more upfront work per PR. The end result is more satisfying in terms of valuable information in each commit. It also doesn't deny you the choice of rebase-and-fast-forward or merge, once the PR is ready.

In extremely trivial cases, I could also see the following flow: a maintainer fetches the PR, does some minor editing (--amend, etc.) and pushes, closing the PR (but the original author retains credit, thanks to the difference between author and committer). Then, the maintainer comments on the PR explaining what they changed, why, and how. This eases the burden of initial, small contributions but teaches "how to fish" for next time.