Squash when merging?

[LiberalArtist]

I agree with everything in this thread, I think.

@mflatt

There are sometimes larger PRs where keeping the commits makes sense, but that's not the usual case.

I can see why you'd ask, because PR #4676 is a good example where squashing is better. The commit messages in that case make sense as part of a PR revision process, but not as a part of the Racket source history. I imagine the PR author expected squashing in in this case (maybe because I almost always squash when merging).

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.