7 ms·

you don't want to shove them all into an opaque commit that says fixes, because you believe in atomic commits. Sure I do. The whole branch will be squashed any

you don't want to shove them all into an opaque commit that says fixes, because you believe in atomic commits. Sure I do. The whole branch will be squashed anyway before it's merged in, and a single "fixes" commit while still on its own branch will be easier to track in a PR for addressing everything pointed out earlier. I mean, don't let me stop anyone from using this or --fixup if this is your flow, but this solves a problem neither I nor anyone in my last 10 years of working with code has.

krick 2y ago
Good for you, but you (and, apparently, everyone in your last 10 years of working with) would have a problem if I was the one reviewing your commits. I mean, to be fair, I often did let it slide (for political/social/practical reasons) and use autosquash, but I always actively discouraged it, so if you are a junior or a new hire with uncertain usefulness status, I'd at least talk to you about that and ask you to fix it. This is not an ok practice, it's only acceptable because too many people don't have a habit of making their own bed and much bigger kinds of technical debt get ignored because in the end it's about shipping stuff, so you just sigh and say "oh, whatever".
- hamandcheese 2y ago
  It has always been enough for me to trace a change back to its original pull request. Extra granularity might be nice sometimes but never essential. And I do a fair bit of digging through git history. Not sure what makes it "not an ok practice". Squashing works fine and lets the 80% of people who don't care spend their efforts thinking about something other than a perfect git history.
- notlinus 2y ago
  For what it's worth, you're right.
- keybored 2y ago
  I personally agree that what you advocate is better. But apparently people get by without it. It’s better. But not essential. No bridges will collapse because of squash merges. I also disagree with the framing: some policy can always be enforced if you have some kind of authority over the other person. Now we’re veering away from arguing on the technical merits.
imiric 2y ago
Every team is free to choose what works best for them, but IMO always squashing PRs is not a good strategy. Sometimes you do want to preserve the change history, particularly if the PR does more than a single atomic change, which in practice is very common. There shouldn't be a static merge type preference at all, and this should be chosen on a case-by-case basis. At the risk of sounding judgemental, I think this preference for always squashing PRs comes from a place of either not understanding atomic commits, not caring about the benefits of them, or just choosing to be lazy. In any case, the loss of history inevitably comes at a cost of making reverting and cherry-picking changes more difficult later, making `git bisect` pretty much worthless, and losing the context of why a change was made.
- hamandcheese 2y ago
  git bisect is still perfectly useful. It will locate the squashed commit/pr which is more than enough for me to zero in on whatever the bug is.
  - imiric 2y ago
    How helpful is it if it points you to a commit with 10 different changes? Will you go back to the PR to see the context of each change, and try to guess where the issue is based on the behavior? You're right back at manual debugging at that point. This workflow really shines when it points you to the actual atomic commit that introduced the issue. Then you're simply a `git revert` away from undoing it, without risking breaking anything else, barring functional conflicts, of course. Try doing that with a squashed commit.
    - recursive 2y ago
      I've not seen code where this revert thing would work. If it's in the PR something else now depends on it.
    - burntsushi 2y ago
      I'm with you (see my other top level comment), but > Then you're simply a `git revert` away from undoing it, without risking breaking anything else This needs careful qualification. On GitHub at least, it is difficult to ensure every commit passes CI. This can result in skipping during bisect for a busted commit. It doesn't happen often enough in my experience too convince me to give up a cleaner history, but it's a downside we should acknowledge. Ideally GitHub would make testing each individual commit easier.
rom1v 2y ago
> The whole branch will be squashed anyway before it's merged in That looks like a very wrong process to me. Why would you even want to do that?
notlinus 2y ago
I wish people would stop saying "atomic commits" and start saying "main/master is stable", because that's what they actually mean. Every git commit is atomic, by definition... But people want every single possible revision to be green and buildable, which is different, and has nothing to do with git. I don't think it makes sense (tags are a lot more helpful for marking what is and isn't stable), but hey.
- usr1106 2y ago
  Yes, atomic commits is not a very descriptive term. But main/master is stable is only a necessary condition, not a sufficient one. If you squash (or mix from the beginning) several unrelated changes into a single commit, main/master would be stable. AFAIK atomic commits means nothing can be taken away without breaking the change and nothing needs to be added to make the change work. How to express that without 2 clauses is indeed a good question.
  - assiniboine 2y ago
    Atomic commits ensure each change works independently.
    - usr1106 2y ago
      That cannot be achieved in general. Later commits often depend on some earlier one.
- keybored 2y ago
  People say what they mean. “Atomic” sometimes means conceptually: does one thing. Sometimes it means “builds”. Or “works”. And yes: they do mean every single commit builds. Not just on main. You don’t have to put words in people’s mouth.
travisjungroth 2y ago
That whole paragraph is the conditions under which this tool would be useful. If they’re not true for you, this isn’t for you. This is like quoting “You have a feature branch” and saying “No I don’t.”

krick 2y ago

Good for you, but you (and, apparently, everyone in your last 10 years of working with) would have a problem if I was the one reviewing your commits. I mean, to be fair, I often did let it slide (for political/social/practical reasons) and use autosquash, but I always actively discouraged it, so if you are a junior or a new hire with uncertain usefulness status, I'd at least talk to you about that and ask you to fix it. This is not an ok practice, it's only acceptable because too many people don't have a habit of making their own bed and much bigger kinds of technical debt get ignored because in the end it's about shipping stuff, so you just sigh and say "oh, whatever".

hamandcheese 2y ago
It has always been enough for me to trace a change back to its original pull request. Extra granularity might be nice sometimes but never essential. And I do a fair bit of digging through git history. Not sure what makes it "not an ok practice". Squashing works fine and lets the 80% of people who don't care spend their efforts thinking about something other than a perfect git history.
notlinus 2y ago
For what it's worth, you're right.
keybored 2y ago
I personally agree that what you advocate is better. But apparently people get by without it. It’s better. But not essential. No bridges will collapse because of squash merges. I also disagree with the framing: some policy can always be enforced if you have some kind of authority over the other person. Now we’re veering away from arguing on the technical merits.

imiric 2y ago

Every team is free to choose what works best for them, but IMO always squashing PRs is not a good strategy. Sometimes you do want to preserve the change history, particularly if the PR does more than a single atomic change, which in practice is very common. There shouldn't be a static merge type preference at all, and this should be chosen on a case-by-case basis. At the risk of sounding judgemental, I think this preference for always squashing PRs comes from a place of either not understanding atomic commits, not caring about the benefits of them, or just choosing to be lazy. In any case, the loss of history inevitably comes at a cost of making reverting and cherry-picking changes more difficult later, making `git bisect` pretty much worthless, and losing the context of why a change was made.

hamandcheese 2y ago
git bisect is still perfectly useful. It will locate the squashed commit/pr which is more than enough for me to zero in on whatever the bug is.
- imiric 2y ago
  How helpful is it if it points you to a commit with 10 different changes? Will you go back to the PR to see the context of each change, and try to guess where the issue is based on the behavior? You're right back at manual debugging at that point. This workflow really shines when it points you to the actual atomic commit that introduced the issue. Then you're simply a `git revert` away from undoing it, without risking breaking anything else, barring functional conflicts, of course. Try doing that with a squashed commit.
  - recursive 2y ago
    I've not seen code where this revert thing would work. If it's in the PR something else now depends on it.
  - burntsushi 2y ago
    I'm with you (see my other top level comment), but > Then you're simply a `git revert` away from undoing it, without risking breaking anything else This needs careful qualification. On GitHub at least, it is difficult to ensure every commit passes CI. This can result in skipping during bisect for a busted commit. It doesn't happen often enough in my experience too convince me to give up a cleaner history, but it's a downside we should acknowledge. Ideally GitHub would make testing each individual commit easier.

rom1v 2y ago

> The whole branch will be squashed anyway before it's merged in That looks like a very wrong process to me. Why would you even want to do that?

notlinus 2y ago

I wish people would stop saying "atomic commits" and start saying "main/master is stable", because that's what they actually mean. Every git commit is atomic, by definition... But people want every single possible revision to be green and buildable, which is different, and has nothing to do with git. I don't think it makes sense (tags are a lot more helpful for marking what is and isn't stable), but hey.

usr1106 2y ago
Yes, atomic commits is not a very descriptive term. But main/master is stable is only a necessary condition, not a sufficient one. If you squash (or mix from the beginning) several unrelated changes into a single commit, main/master would be stable. AFAIK atomic commits means nothing can be taken away without breaking the change and nothing needs to be added to make the change work. How to express that without 2 clauses is indeed a good question.
- assiniboine 2y ago
  Atomic commits ensure each change works independently.
  - usr1106 2y ago
    That cannot be achieved in general. Later commits often depend on some earlier one.
keybored 2y ago
People say what they mean. “Atomic” sometimes means conceptually: does one thing. Sometimes it means “builds”. Or “works”. And yes: they do mean every single commit builds. Not just on main. You don’t have to put words in people’s mouth.

travisjungroth 2y ago

That whole paragraph is the conditions under which this tool would be useful. If they’re not true for you, this isn’t for you. This is like quoting “You have a feature branch” and saying “No I don’t.”