6 ms·

Never understood the appeal of squash commits at merge time, assuming the PR contains atomic, logical commits (all bets are off if your team's PR process accept

Never understood the appeal of squash commits at merge time, assuming the PR contains atomic, logical commits (all bets are off if your team's PR process accepts ad hoc commits..). You lose the utility of git bisect, conventional commits, etc, and also have larger, noisier commits forming your history/documentation. Is there a benefit to squash commits other than allowing developers to forget about that as they work? I may be biased against squash commits as I have spent enough time diving through garbage commit history to figure out bugs/Chesterton's fence that good commits as documentation appeals to me.

phailhaus 4y ago
> assuming the PR contains atomic, logical commits This is impossible to enforce or guarantee at scale. Squashing PRs, though, is practically fool-proof: PRs already represent a single, atomic unit of work that passes all CI checks and is safe to merge. No such thing is true (or should be!) of individual commits within that PR. Whether we like it or not, a branch commit really only represents a "save point" for a developer.
- kbr- 4y ago
  What you say is impossible, we pretty successfully apply at ScyllaDB (see https://github.com/scylladb/scylladb/commits/master https://github.com/scylladb/scylladb/commits/master). I'm not sure 100% of the commits compile & pass all tests - there may be some mistakes - but generally we're in a pretty good state, and the clean git log is being successfully used for bisecting. If you want even larger scale - if I understand correctly, the Linux kernel practices a similar thing, which is where we got this practice from (ScyllaDB founders came from kernel development). And since Git was originally created to help developing Linux - that's where you want to look for good practices.
  - throwaway_au_1 4y ago
    I also found the comment you replied to a little unconvincing. The remark concerning scale in particular did not hit home, as I would guess the vast majority of teams are <10 devs which I would hardly call 'scale'. I left my previous role for several reasons but one was the constant "Microsoft does x".. Microsoft has 100k devs, we had 5. Not the same.
    - phailhaus 4y ago
      The point is that you can try, but IMO it's wasted effort. Commits are immutable and really hard to manipulate retroactively, and humans are guaranteed to make mistakes. Why put so much effort into trying to make your commits atomic, when it's unlikely that they ever truly are? Sure, you can git bisect to the exact commit that introduced a bug, but that commit was part of a larger PR, and you probably can't revert just that commit alone. So what was gained?
- WorldMaker 4y ago
  You can get the same guarantees without squashes with {log, bisect, blame} --first-parent and merge --no-ff (force all PRs to make merge commits). You can preserve more of the graph and use it to revisit "inside" of PRs when necessary.
  - tcoff91 4y ago
    So much this. Everyone needs to learn about --first-parent, it makes git log, git bisect, etc... so much more powerful and prevents people from just squashing PRs into giant mega-commits that make the history so much less useful.
    - WorldMaker 4y ago
      I keep joking that all we need is one good Git GUI to go viral that defaults to --first-parent in every view and we might eventually convince more people they don't need to squash/rebase as much. One of these days I may even take the joke far enough along to prototype something.
  - phailhaus 4y ago
    What's the point? It's unlikely you'll be able to revert it in isolation. You can do the same thing with squashed PR's, except you also get a description of the overall work, all discussions related to it, and a higher likelihood that you can revert it in one piece.
    - WorldMaker 4y ago
      First parent applies to revert, too (with the -m flag): git revert MERGECOMMITHASH -m 1 # revert to first parent of merge (revert changes of second parent/other branch) You can include good descriptions in your PR merge commits including discussion comments and everything. Some PR tools automate that, some do not. I don't know any technical reason why any PR tool automation would create better squash merge commit messages than normal merge commit messages. (ETA: Also you may want to reconsider your workflow if you rely on reverts that often. I had to look up the command argument, even though I knew it existed, because I haven't needed to do it in a while and I'm very thankful for that.)
debaserab2 4y ago
You don't lose that much utility of git bisect unless your PR's are typically large. One thing squash and merge does is make reverting trivial.
- tcoff91 4y ago
  You can revert a merge commit.
- throwaway_au_1 4y ago
  Easy reverting is certainly not a bad thing, but IMHO, and not to suggest your comment implied anything either way, it should be a minority case and optimising for a minority case doesn't seem generally sensible. Maybe reverting has a use outside of 'uh oh' that I'm not conscious of (release management?) but if 'uh oh' reverting is not a minority case, sounds like there may be bigger problems at play. Do you come across many situations to use reverts day to day or week to week? I've probably only made a few in the last few years
ivanche 4y ago
I much prefer to see one squashed commit over an endless stream of "fix typo", "small fix", "fixup", "forgot XYZ"...
- tcoff91 4y ago
  Don't approve PRs that have those commits. A PR doesn't necessarily have to be a single commit to be a good PR. Sometimes, it makes sense for a PR to have several atomic commits. Those tiny messy commits should be fixed-up into the atomic commit that they are fixing.
  - ivanche 4y ago
    That is 100% correct and I agree. Actually, I always try to make a PR as a series of small, self-contained commits. I just wanted to point out the, IME, strong correlation between people that leave a bunch of "small fix" commits and them not using squash/rebase but "normal" merge.
    - tcoff91 4y ago
      I've been taking a different tactic. I have been using git-patch-stack https://git-ps.sh/ https://git-ps.sh/, and making all my PRs be 1 commit, and trying to keep my PRs smaller using feature flags and things of that nature that allow me to deliver smaller incremental units of work. This tool is amazing, i highly recommend it. It's wonderful for being able to hack away on a huge change and still deliver tiny incremental PRs from it as you go along.

phailhaus 4y ago

> assuming the PR contains atomic, logical commits This is impossible to enforce or guarantee at scale. Squashing PRs, though, is practically fool-proof: PRs already represent a single, atomic unit of work that passes all CI checks and is safe to merge. No such thing is true (or should be!) of individual commits within that PR. Whether we like it or not, a branch commit really only represents a "save point" for a developer.

kbr- 4y ago
What you say is impossible, we pretty successfully apply at ScyllaDB (see https://github.com/scylladb/scylladb/commits/master https://github.com/scylladb/scylladb/commits/master). I'm not sure 100% of the commits compile & pass all tests - there may be some mistakes - but generally we're in a pretty good state, and the clean git log is being successfully used for bisecting. If you want even larger scale - if I understand correctly, the Linux kernel practices a similar thing, which is where we got this practice from (ScyllaDB founders came from kernel development). And since Git was originally created to help developing Linux - that's where you want to look for good practices.
- throwaway_au_1 4y ago
  I also found the comment you replied to a little unconvincing. The remark concerning scale in particular did not hit home, as I would guess the vast majority of teams are <10 devs which I would hardly call 'scale'. I left my previous role for several reasons but one was the constant "Microsoft does x".. Microsoft has 100k devs, we had 5. Not the same.
  - phailhaus 4y ago
    The point is that you can try, but IMO it's wasted effort. Commits are immutable and really hard to manipulate retroactively, and humans are guaranteed to make mistakes. Why put so much effort into trying to make your commits atomic, when it's unlikely that they ever truly are? Sure, you can git bisect to the exact commit that introduced a bug, but that commit was part of a larger PR, and you probably can't revert just that commit alone. So what was gained?
WorldMaker 4y ago
You can get the same guarantees without squashes with {log, bisect, blame} --first-parent and merge --no-ff (force all PRs to make merge commits). You can preserve more of the graph and use it to revisit "inside" of PRs when necessary.
- tcoff91 4y ago
  So much this. Everyone needs to learn about --first-parent, it makes git log, git bisect, etc... so much more powerful and prevents people from just squashing PRs into giant mega-commits that make the history so much less useful.
  - WorldMaker 4y ago
    I keep joking that all we need is one good Git GUI to go viral that defaults to --first-parent in every view and we might eventually convince more people they don't need to squash/rebase as much. One of these days I may even take the joke far enough along to prototype something.
- phailhaus 4y ago
  What's the point? It's unlikely you'll be able to revert it in isolation. You can do the same thing with squashed PR's, except you also get a description of the overall work, all discussions related to it, and a higher likelihood that you can revert it in one piece.
  - WorldMaker 4y ago
    First parent applies to revert, too (with the -m flag): git revert MERGECOMMITHASH -m 1 # revert to first parent of merge (revert changes of second parent/other branch) You can include good descriptions in your PR merge commits including discussion comments and everything. Some PR tools automate that, some do not. I don't know any technical reason why any PR tool automation would create better squash merge commit messages than normal merge commit messages. (ETA: Also you may want to reconsider your workflow if you rely on reverts that often. I had to look up the command argument, even though I knew it existed, because I haven't needed to do it in a while and I'm very thankful for that.)

debaserab2 4y ago

You don't lose that much utility of git bisect unless your PR's are typically large. One thing squash and merge does is make reverting trivial.

tcoff91 4y ago
You can revert a merge commit.
throwaway_au_1 4y ago
Easy reverting is certainly not a bad thing, but IMHO, and not to suggest your comment implied anything either way, it should be a minority case and optimising for a minority case doesn't seem generally sensible. Maybe reverting has a use outside of 'uh oh' that I'm not conscious of (release management?) but if 'uh oh' reverting is not a minority case, sounds like there may be bigger problems at play. Do you come across many situations to use reverts day to day or week to week? I've probably only made a few in the last few years

ivanche 4y ago

I much prefer to see one squashed commit over an endless stream of "fix typo", "small fix", "fixup", "forgot XYZ"...

tcoff91 4y ago
Don't approve PRs that have those commits. A PR doesn't necessarily have to be a single commit to be a good PR. Sometimes, it makes sense for a PR to have several atomic commits. Those tiny messy commits should be fixed-up into the atomic commit that they are fixing.
- ivanche 4y ago
  That is 100% correct and I agree. Actually, I always try to make a PR as a series of small, self-contained commits. I just wanted to point out the, IME, strong correlation between people that leave a bunch of "small fix" commits and them not using squash/rebase but "normal" merge.
  - tcoff91 4y ago
    I've been taking a different tactic. I have been using git-patch-stack https://git-ps.sh/ https://git-ps.sh/, and making all my PRs be 1 commit, and trying to keep my PRs smaller using feature flags and things of that nature that allow me to deliver smaller incremental units of work. This tool is amazing, i highly recommend it. It's wonderful for being able to hack away on a huge change and still deliver tiny incremental PRs from it as you go along.