16 ms·

I never understood the point of squashing if you just want the short version, only read the merge commits; if you want the full details to figure out some bug o

by emn13 5y ago

I never understood the point of squashing if you just want the short version, only read the merge commits; if you want the full details to figure out some bug or whatever, then yes, I want those fix typo commits most definitely, because as often as not, those are at fault. Squashing buys you next to nothing, and costs you the ability to dive into the history in greater detail. I suppose if your project is truly huge, it becomes worth it to reduce load on your VCS, but beyond that...

pdimitar 5y ago
Having full history in the feature branch is mandatory -- nobody is disputing that, myself included. All due diligence is done there, not in the main branch. The main branch only needs to have one big commit saying "merging PR #2169". If you need more details you'll go that PR/branch and get your info. The "fix typo" commit being in the main branch buys you nothing. It's only useful in its separate branch.
- emn13 5y ago
  But that's what a merge commit is - the diff along the merge commit is what the squashed commit would be; the only thing squashing does is "forget" that the second parent exists (terminology for non-git VCS's may be slightly different). Why not merge?
  - pdimitar 5y ago
    Because in big commercial projects several people or teams merge inside the main branch on a regular basis. It's easier to look at a history only including squashed commits each encompassing an entire PR (feature or a bug). It gives you better high-level observability and a good bird's-eye view. And again -- if you need more details you can go and check all separate commits in the PR/branch anyway. And finally, squashed commits are kind of atomic commits. Imagine history of three separate PRs happening at roughly the same time. And now all those commits are interspersed in the history of the main branch. How is that useful or informative? It's chaos. EDIT: my bad, I conflated merging with rebasing. Still, I prefer a single squashed commit for most of the reasons above, plus those of the other two commenters (useful git blame output and buildable history).
    - emn13 5y ago
      Oh yeah, rebasing branches as a "merge" policy is definitely tricky like that. (I mean, I'm sure some people do that with and perhaps with good reason, but it makes this kind of stuff clearly worse).
- skeeter2020 5y ago
  This is fine if its how your team develops but not for everyone. We don't care about full history in branches; maybe it has more detail than main/master but it should still be contextually meaningful. I'd never approve a commit into main with the message "merging PR #xxx" either; it's redundant (merging), has no summary about what it actually does and relies on an external system (your PR/MR process) for details. I do agree that keeping noise out of your main is key, but would go even further than you to keep it clean AND self-contained.
  - pdimitar 5y ago
    Well sure, the title was just an example. It usually is more like this: "Encrypt project's sensitive fields (#1234)" With the number being a PR or an issue # (which does contain a link to the PR). I do care about history in branches though. And many others do. I agree that it varies from team to team.
throwaway525142 5y ago
Squashing for example allows me to have a history where each commit builds. This has been very useful for bisecting for me. I wouldn't call it "next to nothing". The "greater detail" part can cost me a lot of time.
- emn13 5y ago
  ideally your branch commits would usually build too; but admittedly, I tend to bisect by hand, following the --first-parent. I suppose if the set of commits were truly huge that might be more of an issue. And of course, there are people that have hacked their way to success here: https://stackoverflow.com/a/5652323 https://stackoverflow.com/a/5652323, but that sounds a little fragile.
- jiofih 5y ago
  Having commits that do not build represents the history more accurately. It could very well be a “fix” to some build error that silently introduces an issue, that context is lost when you squash.
  - throwaway525142 5y ago
    You're not saying anything new as far as I can tell. Your grandparent already said what you said. I only disputed the "this costs next to nothing" part, which you don't seem to comment on.
    - jiofih 5y ago
      I was commenting exactly on that. You think having every commit build, at the cost of destroying history, is gaining something. I think representing history correctly is best, and agree that “squashing buys you next to nothing” other than visually pleasant output. Clearer? My favorite approach is rebase + non-ff merge. Best of both worlds.
      - jhasse 5y ago
        No. They think having every commit build is gaining something. You think the disadvantage (destroying history) is more important, but you can't say that it "buys you nothing".
layoutIfNeeded 5y ago
I want to be able to tell why a given line of code was introduced. Seeing "Fix indentation" in the output of `git blame` won't help me with that.
- emn13 5y ago
  git blame --first-parent
- elliotf 5y ago
  I also want to be able to tell _why_ which is why I dislike working on codebases that squash commmits. Too many times I've done a blame to see why a change was made, and it's a giant (> 10) list of commit messages. Oftentimes, the macro description of what was going on does not help me with the line-level detail. Also, in case it helps you in the future, `blame -wC` is what I use when doing blame; it ignores whitespace changes and tracks changes across files (changes happened before a rename, for example.)
- gregmac 5y ago
  Neither does squashing though: you still can't tell if that line was introduced or modified. I've come across "fix indentation" or "fix typo" commits where a bug was introduced, like someone accidentally comitted a change (maybe they were debugging something, or just accidentally modified it). For example: I'm tracing a bug where a value isn't staying cached. I find a line of code DefaultCacheAge=10 (which looks way too short) and git blame shows the last change was modifying that value from 86400. What I do next will be very different if the commit message says "fix indentation" vs "added new foobar feature" or "reduced default cache time for (reason)".
bastardoperator 5y ago
Who squashes like this? I rebase all of my PR's not because I want to trash history, but because I want history to be meaningful. If I include all of my "whoops typo fix" and "pay respect to the linter gods" commits, I have made my default branch history much less readable. I would say what you're describing is a break down in CI/CD and code review. How is code that is that broken getting into your default branch in the first place?
- emn13 5y ago
  I certainly don't, but it's a standard option in git merge (and IIRC github supports it), so I'm pretty sure some teams do. As to rebases to clean up history (and not just the PR itself)... personally, I don't think that's worth it. My experience with history like this is that it's relevant during review, and then around 95% of it is irrelevant - you may not know which 5 % are relevant beforehand, but it's always some small minority. It's worth cleaning up a PR for review, but not for posterity. And when it comes to review, I like commits like "pay respect to the linter gods" and the like, because they're easy to ignore, whereas if you touch code and reformat even slightly in one commit, it's often harder to skip the boring bits; to the point that I'll even intentionally commit poorly formatted code such that the diff is easy to read and then do the reformat later. Removing clear noise (as in code that changes back and forth and for no good reason) is of course nice, but it's easy to overdo; a few typo commits barely impact review-ability (imho), and rebases can and do introduce bugs - you must have encountered semantic merge conflicts before, and those are 10 times as bad with rebases, because they're generally silent (assuming you don't test each commit post-rebase), but leave the code in a really confusing situation, especially when people fix the final commit in the PR, but not the one were the semantic merge conflict was introduced, and laziness certainly encourages that. It also depends on how proficient you are with merge conflicts and git chicanery. If you are; then the history is yours to reshape; but not everybody is, and then I'd rather review an honest history with some cruft, rather than a frankenstein history with odd seams and mismatched stuff in a commit that basically exists because "I kept on prodding git till it worked".
skeeter2020 5y ago
greater detail can ultimately lead to less information if noisy commits crowd out the important ones. IME you want a commit to mean something and that usually leads to tweaking the work/change/commit relationship, which is where squashing helps.

by emn13 5y ago

pdimitar 5y ago

Having full history in the feature branch is mandatory -- nobody is disputing that, myself included. All due diligence is done there, not in the main branch. The main branch only needs to have one big commit saying "merging PR #2169". If you need more details you'll go that PR/branch and get your info. The "fix typo" commit being in the main branch buys you nothing. It's only useful in its separate branch.

emn13 5y ago
But that's what a merge commit is - the diff along the merge commit is what the squashed commit would be; the only thing squashing does is "forget" that the second parent exists (terminology for non-git VCS's may be slightly different). Why not merge?
- pdimitar 5y ago
  Because in big commercial projects several people or teams merge inside the main branch on a regular basis. It's easier to look at a history only including squashed commits each encompassing an entire PR (feature or a bug). It gives you better high-level observability and a good bird's-eye view. And again -- if you need more details you can go and check all separate commits in the PR/branch anyway. And finally, squashed commits are kind of atomic commits. Imagine history of three separate PRs happening at roughly the same time. And now all those commits are interspersed in the history of the main branch. How is that useful or informative? It's chaos. EDIT: my bad, I conflated merging with rebasing. Still, I prefer a single squashed commit for most of the reasons above, plus those of the other two commenters (useful git blame output and buildable history).
  - emn13 5y ago
    Oh yeah, rebasing branches as a "merge" policy is definitely tricky like that. (I mean, I'm sure some people do that with and perhaps with good reason, but it makes this kind of stuff clearly worse).
skeeter2020 5y ago
This is fine if its how your team develops but not for everyone. We don't care about full history in branches; maybe it has more detail than main/master but it should still be contextually meaningful. I'd never approve a commit into main with the message "merging PR #xxx" either; it's redundant (merging), has no summary about what it actually does and relies on an external system (your PR/MR process) for details. I do agree that keeping noise out of your main is key, but would go even further than you to keep it clean AND self-contained.
- pdimitar 5y ago
  Well sure, the title was just an example. It usually is more like this: "Encrypt project's sensitive fields (#1234)" With the number being a PR or an issue # (which does contain a link to the PR). I do care about history in branches though. And many others do. I agree that it varies from team to team.

throwaway525142 5y ago

Squashing for example allows me to have a history where each commit builds. This has been very useful for bisecting for me. I wouldn't call it "next to nothing". The "greater detail" part can cost me a lot of time.

emn13 5y ago
ideally your branch commits would usually build too; but admittedly, I tend to bisect by hand, following the --first-parent. I suppose if the set of commits were truly huge that might be more of an issue. And of course, there are people that have hacked their way to success here: https://stackoverflow.com/a/5652323 https://stackoverflow.com/a/5652323, but that sounds a little fragile.
jiofih 5y ago
Having commits that do not build represents the history more accurately. It could very well be a “fix” to some build error that silently introduces an issue, that context is lost when you squash.
- throwaway525142 5y ago
  You're not saying anything new as far as I can tell. Your grandparent already said what you said. I only disputed the "this costs next to nothing" part, which you don't seem to comment on.
  - jiofih 5y ago
    I was commenting exactly on that. You think having every commit build, at the cost of destroying history, is gaining something. I think representing history correctly is best, and agree that “squashing buys you next to nothing” other than visually pleasant output. Clearer? My favorite approach is rebase + non-ff merge. Best of both worlds.
    - jhasse 5y ago
      No. They think having every commit build is gaining something. You think the disadvantage (destroying history) is more important, but you can't say that it "buys you nothing".

layoutIfNeeded 5y ago

I want to be able to tell why a given line of code was introduced. Seeing "Fix indentation" in the output of `git blame` won't help me with that.

emn13 5y ago
git blame --first-parent
elliotf 5y ago
I also want to be able to tell _why_ which is why I dislike working on codebases that squash commmits. Too many times I've done a blame to see why a change was made, and it's a giant (> 10) list of commit messages. Oftentimes, the macro description of what was going on does not help me with the line-level detail. Also, in case it helps you in the future, `blame -wC` is what I use when doing blame; it ignores whitespace changes and tracks changes across files (changes happened before a rename, for example.)
gregmac 5y ago
Neither does squashing though: you still can't tell if that line was introduced or modified. I've come across "fix indentation" or "fix typo" commits where a bug was introduced, like someone accidentally comitted a change (maybe they were debugging something, or just accidentally modified it). For example: I'm tracing a bug where a value isn't staying cached. I find a line of code DefaultCacheAge=10 (which looks way too short) and git blame shows the last change was modifying that value from 86400. What I do next will be very different if the commit message says "fix indentation" vs "added new foobar feature" or "reduced default cache time for (reason)".

bastardoperator 5y ago

Who squashes like this? I rebase all of my PR's not because I want to trash history, but because I want history to be meaningful. If I include all of my "whoops typo fix" and "pay respect to the linter gods" commits, I have made my default branch history much less readable. I would say what you're describing is a break down in CI/CD and code review. How is code that is that broken getting into your default branch in the first place?

emn13 5y ago
I certainly don't, but it's a standard option in git merge (and IIRC github supports it), so I'm pretty sure some teams do. As to rebases to clean up history (and not just the PR itself)... personally, I don't think that's worth it. My experience with history like this is that it's relevant during review, and then around 95% of it is irrelevant - you may not know which 5 % are relevant beforehand, but it's always some small minority. It's worth cleaning up a PR for review, but not for posterity. And when it comes to review, I like commits like "pay respect to the linter gods" and the like, because they're easy to ignore, whereas if you touch code and reformat even slightly in one commit, it's often harder to skip the boring bits; to the point that I'll even intentionally commit poorly formatted code such that the diff is easy to read and then do the reformat later. Removing clear noise (as in code that changes back and forth and for no good reason) is of course nice, but it's easy to overdo; a few typo commits barely impact review-ability (imho), and rebases can and do introduce bugs - you must have encountered semantic merge conflicts before, and those are 10 times as bad with rebases, because they're generally silent (assuming you don't test each commit post-rebase), but leave the code in a really confusing situation, especially when people fix the final commit in the PR, but not the one were the semantic merge conflict was introduced, and laziness certainly encourages that. It also depends on how proficient you are with merge conflicts and git chicanery. If you are; then the history is yours to reshape; but not everybody is, and then I'd rather review an honest history with some cruft, rather than a frankenstein history with odd seams and mismatched stuff in a commit that basically exists because "I kept on prodding git till it worked".

skeeter2020 5y ago

greater detail can ultimately lead to less information if noisy commits crowd out the important ones. IME you want a commit to mean something and that usually leads to tweaking the work/change/commit relationship, which is where squashing helps.