5 ms·

I disagree entirely. A new feature or bug fix should be a single commit on the main branch. There's no need to see previous attempts to fix a bug or implement

I disagree entirely. A new feature or bug fix should be a single commit on the main branch. There's no need to see previous attempts to fix a bug or implement a feature in the git history. Especially commits that merely fix formatting.

Fire-Dragon-DoL 3y ago
Making meaningful commit makes sense, however with git you can show parent commits only, to me that's sufficient to say "put whatever garbage you want inside the pr", because you can still look at merge commits only. Github is the problem here. And squash merge is "useful" as in when I would have squashed a commit even before merging, I don't think we should squash merge otherwise
smusamashah 3y ago
Depends on the codebase perhaps. I sometimes touch parts which are not directly relevant to the the task e.g. mostly a minor reactor, a minor bug fix etc. It's nice to leave a trail on that particular change that it was just a refactor instead of exploring whole changeset only to find out that this change was irrelevant.
- dier 3y ago
  I've found worktrees[1] are a great tool for this use case. "oh. i should fix this, but it doesn't have anything to do with what i'm currently solving." cd ../worktree-checkout, make a branch, do the fix, push, open a pr. then cd ../original-checkout and continue on. rebasing the fix into my wip when the PR is merged (or cherry-pick if you're confident) as needed. 1. https://git-scm.com/docs/git-worktree https://git-scm.com/docs/git-worktree
silverlake 3y ago
‘git log —-no-merges’
JohnFen 3y ago
Yes, I understand. The reason that I disagree is because of the number of times that having greater granularity than that has been enormously valuable. In my opinion, a VCS should maintain a history of all commits, because it's impossible to know which commits will be the key to solving a problem in the future. Even commits that are later reversed can occasionally be of tremendous value. A truly great VCS would allow you to do a "visual squash", hiding the intermediate commits unless you need to see them.
- WirelessGigabit 3y ago
  That already exists. The merge into main is a single merge commit. You can view the merge as a single unit, or unpack it into the individual commits that comprise of the feature branch.
- dier 3y ago
  I think the preference to squash and rebase instead of merge is because of Github, honestly. It flattens the history out when the history is not flat at all. If Github's UI showed branches diverging, commits being made to that branch, then the merge back to trunk like gitk does then devs could visually see why it's valuable.
- iza 3y ago
  git log --first-parent Works best if your merge commits have useful messages.
- ninkendo 3y ago
  Say I run “git commit -am wip” in a for loop in the background, then squash everything before I make a PR. Is it your position to at every single character change I make in my editor should be preserved? That’s kind of crazy right? I think you need to recognize that you have some implicit assumptions about what a commit even is, when you make such statements as “all commits [should be preserved]”.
  - Brian_K_White 3y ago
    It's equally wrong to rig up a context like that and say that therefore prs should be squashed. In that frankly ridiculous example, nothing has actually changed but the location and the label for where the meaningful thing happens. In your example, since you've artificially injected automation to snapshot every 11th keystroke, those keystrokes and those commits are indeed not meaningful. (I would argue that they're not useful either but I allow it's possible to argue they're useful) But there is still some point where you do decide "here there is a meaningful milestone" and you mark that spot. Other people do exactly that same thing with commit. You've just artificially gone out of your way to make your own commits useless for that.
RadiozRadioz 3y ago
There is a need when your feature causes a problem. Any nontrivial feature will require multiple distinct conceptual changes to the codebase, any of which could be at fault. Squashing completely neuters git bisect's ability to find the actual change that caused the issue. With squashed commits, you're stuck digging through a giant diff when you could instead be looking at a small unit of change.
david_allison 3y ago
The commit history should be clean, but not necessarily squashed WIP commits should be rebased out before the branch is pushed Commits which fix formatting should definitely be a separate commit so they can be added to `.git-blame-ignore-revs`. This allows anyone looking through the history of a change to see the functional fix, rather than having it mixed in with a refactor/reformat operation. In this case, each commit should have a useful commit message, both allowing them to be searchable, and to help understanding of the change
- throwworhtthrow 3y ago
  Did you purposely write this comment so it reads like a series of commit messages, or are you just wired that way? (I agree on all points.)
pornel 3y ago
By squashing, you lose ability to effectively bisect. Instead of finding an offending commit that has only a few lines to analyze, you only get a massive feature-landing commit that doesn't narrow it down. Interactive rebase or amend can be used to squash "oops forgot a semicolon" junk commits.
- thealistra 3y ago
  This is literally furthest front the truth. Instead of having develop of all building test passing squashed commits you have to fiddle with bisect around commits that accidentally break the build and fail test and god knows why. Squash is basically a hard requirement for bisect to be useful at all. Especially for compiled languages.
  - pornel 3y ago
    I think you're typically dealing with much messier git histories than I do. Anyway, accidentally-breaking commits can be skipped, but squashed commits can't be unsquashed.
plorkyeran 3y ago
"There's no need to see previous attempts" and "a new feature should be a single commit" are very different things. I mostly agree with the first and don't think it's worth saving a failed attempt at something just because it happened to get committed at some point, but I still regularly implement features in multiple commits. It's very normal to do some refactoring to make the new feature easier to implement and then implement the feature, and those are separate changes that should be separate commits. A commit that only changes formatting is a prime example of a useful secondary commit. Separating formatting changes from code changes makes it much easier to see what the actual functional changes were rather than them being hidden in a mess of non-functional changes.

Fire-Dragon-DoL 3y ago

Making meaningful commit makes sense, however with git you can show parent commits only, to me that's sufficient to say "put whatever garbage you want inside the pr", because you can still look at merge commits only. Github is the problem here. And squash merge is "useful" as in when I would have squashed a commit even before merging, I don't think we should squash merge otherwise

smusamashah 3y ago

Depends on the codebase perhaps. I sometimes touch parts which are not directly relevant to the the task e.g. mostly a minor reactor, a minor bug fix etc. It's nice to leave a trail on that particular change that it was just a refactor instead of exploring whole changeset only to find out that this change was irrelevant.

dier 3y ago
I've found worktrees[1] are a great tool for this use case. "oh. i should fix this, but it doesn't have anything to do with what i'm currently solving." cd ../worktree-checkout, make a branch, do the fix, push, open a pr. then cd ../original-checkout and continue on. rebasing the fix into my wip when the PR is merged (or cherry-pick if you're confident) as needed. 1. https://git-scm.com/docs/git-worktree https://git-scm.com/docs/git-worktree

silverlake 3y ago

‘git log —-no-merges’

JohnFen 3y ago

Yes, I understand. The reason that I disagree is because of the number of times that having greater granularity than that has been enormously valuable. In my opinion, a VCS should maintain a history of all commits, because it's impossible to know which commits will be the key to solving a problem in the future. Even commits that are later reversed can occasionally be of tremendous value. A truly great VCS would allow you to do a "visual squash", hiding the intermediate commits unless you need to see them.

WirelessGigabit 3y ago
That already exists. The merge into main is a single merge commit. You can view the merge as a single unit, or unpack it into the individual commits that comprise of the feature branch.
dier 3y ago
I think the preference to squash and rebase instead of merge is because of Github, honestly. It flattens the history out when the history is not flat at all. If Github's UI showed branches diverging, commits being made to that branch, then the merge back to trunk like gitk does then devs could visually see why it's valuable.
iza 3y ago
git log --first-parent Works best if your merge commits have useful messages.
ninkendo 3y ago
Say I run “git commit -am wip” in a for loop in the background, then squash everything before I make a PR. Is it your position to at every single character change I make in my editor should be preserved? That’s kind of crazy right? I think you need to recognize that you have some implicit assumptions about what a commit even is, when you make such statements as “all commits [should be preserved]”.
- Brian_K_White 3y ago
  It's equally wrong to rig up a context like that and say that therefore prs should be squashed. In that frankly ridiculous example, nothing has actually changed but the location and the label for where the meaningful thing happens. In your example, since you've artificially injected automation to snapshot every 11th keystroke, those keystrokes and those commits are indeed not meaningful. (I would argue that they're not useful either but I allow it's possible to argue they're useful) But there is still some point where you do decide "here there is a meaningful milestone" and you mark that spot. Other people do exactly that same thing with commit. You've just artificially gone out of your way to make your own commits useless for that.

RadiozRadioz 3y ago

There is a need when your feature causes a problem. Any nontrivial feature will require multiple distinct conceptual changes to the codebase, any of which could be at fault. Squashing completely neuters git bisect's ability to find the actual change that caused the issue. With squashed commits, you're stuck digging through a giant diff when you could instead be looking at a small unit of change.

david_allison 3y ago

The commit history should be clean, but not necessarily squashed WIP commits should be rebased out before the branch is pushed Commits which fix formatting should definitely be a separate commit so they can be added to `.git-blame-ignore-revs`. This allows anyone looking through the history of a change to see the functional fix, rather than having it mixed in with a refactor/reformat operation. In this case, each commit should have a useful commit message, both allowing them to be searchable, and to help understanding of the change

throwworhtthrow 3y ago
Did you purposely write this comment so it reads like a series of commit messages, or are you just wired that way? (I agree on all points.)

pornel 3y ago

By squashing, you lose ability to effectively bisect. Instead of finding an offending commit that has only a few lines to analyze, you only get a massive feature-landing commit that doesn't narrow it down. Interactive rebase or amend can be used to squash "oops forgot a semicolon" junk commits.

thealistra 3y ago
This is literally furthest front the truth. Instead of having develop of all building test passing squashed commits you have to fiddle with bisect around commits that accidentally break the build and fail test and god knows why. Squash is basically a hard requirement for bisect to be useful at all. Especially for compiled languages.
- pornel 3y ago
  I think you're typically dealing with much messier git histories than I do. Anyway, accidentally-breaking commits can be skipped, but squashed commits can't be unsquashed.

plorkyeran 3y ago

"There's no need to see previous attempts" and "a new feature should be a single commit" are very different things. I mostly agree with the first and don't think it's worth saving a failed attempt at something just because it happened to get committed at some point, but I still regularly implement features in multiple commits. It's very normal to do some refactoring to make the new feature easier to implement and then implement the feature, and those are separate changes that should be separate commits. A commit that only changes formatting is a prime example of a useful secondary commit. Separating formatting changes from code changes makes it much easier to see what the actual functional changes were rather than them being hidden in a mess of non-functional changes.