I agree. As a way to minimise the pain on GitHub today, we disallow force pushing and enforce squash merging. Force pushing is a nightmarish behaviour, once a Pull Request is opened the branch must be append only.
So you have endless "Fix a" "Typo" "fixup" "revert redo" "add y missed in z" commits and then the squash pushes all that crap into the commit message for whatever the final mess will be?
A Pull Request should represent one change to the system (whether that’s a new feature or a bug fix). A commit to the Pull Request branch only has value in the context of the Pull Request. If you want a Pull Request to contain multiple atomic changes then sure, you need to do what you’re describing… but that’s entirely optional (and straying even further from the idea of changesets).
> So you have endless "Fix a" "Typo" "fixup" "revert redo" "add y missed in z" commits and then the squash pushes all that crap into the commit message for whatever the final mess will be?
In Github at least you can set the behavior to take the PR description by default as the squashed commit message. In fairness this is not the default. The default behavior for squash merges is to ask for a new commit message right as you hit the merge button, and the default is all of the messages from the commits being squashed together.
> make no effort to produce high-quality independent commits
I'm partial to sqaush merges when using github. I don't put much effort into the individual commit messages, instead I put lots of effort into the PR description (the thing reviewers will read, and what will eventually become the commit message in revision history). That said, one of my favorite features from gerrit at a past job was that the commit message itself could be reviewed.
> Honestly, squash merges are frequently a sign of people who lack mastery in Git itself or make no effort to produce high-quality independent commits.
So literally any open source project that accepts external contributions. For this reason we default to squash merge but will allow for exceptions if people ask for it and know how to structure their commits.
> Honestly, squash merges are frequently a sign of people who lack mastery in Git itself or make no effort to produce high-quality independent commits.
I don't understand the appeal of squash commits. AFAICT, the only advantage of squash commits is a nice "linear history", but you can get that from regular old merge commits just by adding --first-parent to your `git log` command -- and you still have that additional detail available on the second-parent commit chain if you need it.
As soon as you start squashing commits, you throw away a very useful guarantee git gives you by default, which is that a given code change A is "in" a given branch B if and only if A is reachable from B. This guarantee is very helpful when debugging differences between versions, e.g., in figuring out which commits I've made locally are actually running in a shared environment like staging/UAT.
The point is it enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible.
The short term and long term view of the work should be different.
> The point is [the squash commit method] enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible.
The problem with squash commits is that they tend to make changes to more files and make changes to more lines in a given file. This makes it harder to revert changes after more changes have been applied to the main branch. In contrast, organizing commits into a set of logical changes necessary to implement a feature in a given branch makes it much easier to revert one of those commits even after other feature branches have been merbed into the main branch because the commits affect one or just a few files and not many changes in a file).
> Indeed, but trying to revert lots of tiny commits in one unit is no better.
You shouldn't have to revert a lot of tiny commits if the bug is due to just one of those commits.
> The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.
In my experience, features take more than one commit to implement. The merge commit provides a way to group those multiple commits so that you can see all commits that went into implementing a feature.
If you're trying to make pull requests that are like small commits, then a feature branch becomes
with commits and their associated merge commits for other features interspersed with your set of commits.
Which basically introduces a lot of merge commits (effectively doubling the number of commits) where the first parent is the previous merge commit and the second parent is the single commit for that pull request. You have no way to really group related commits that were used to implement a feature since there's no merge commit that groups them all together. In that case, you could halve the number of commits by dispensing with merge commits entirely and just adding the #PR-number in the commit message to link to the PR discussion.
Because squash merging results in horrifically poor commit messages that are use to no one in future, because GitHub can’t be bothered to implement anything beyond a simple textarea for editing them.
Not only that, they also result in commits that change a lot of files and make many changes in each file. Try to revert one of those commits is difficult after other changes have been merged into the main branch.
Force pushes makes reviews hard to follow, especially if there are multiple rounds. GitLab handles it much better.
Relatedly, does GitHub merge understand !squash and !fixup commands? I kind of gave up on those and just accepted a few trailing bugfix commits on PRs to not mess up peoples reviews (some projects also invalidate review on force push).
> some projects also invalidate review on force push
Those should also invalidate on a "normal" push if any changes are made. But yeah, it'd be nice to have the option to keep reviews valid on force-push if diff is identic and the only difference is rearranging commits/messages while still invalidating on any actual changes.
I’m assuming they mean squash merging into main once the PR is accepted. That way you have 1 commit on main that links back to 1 PR with N commits on it. Which is easier to follow on main branch rather than a million commits per PR
That makes "git bisect" and "git blame" fairly useless. Please don't do that. Better to keep "oops" commits than squashing a series of logical changes into one monster commit.
"oops" commits are bad for this too (harder to find the regression when there's unrelated broken states), but you're right that it's less bad.
Note that you can bisect --first-parent if you want the "just find the topic branch that introduced this" behavior, without taking away the ability to find the actual commit!
I don’t think there is a one size fits all answer here. “Oops” commits make blame useless too plus they make following any meaningful change very hard.
You want small commits in main, so send small PRs. If you have a feature, you can do a feature branch and merge into it the small commits then merge (don’t squash) feature branch into main.
I see now that the grandparent was enforcing squash commits, did not mean to single out your comment.
It just blows my mind as someone with a somewhat perfectionist approach to rebasing (to the extent I sometimes intentionally break up or reorder commits to make the changes more intentional or natural, even when it causes nasty local merge conflicts) :P
Well, I’d appreciate it if I collaborate on a repo with you. I end up giving up commit hygiene due to the absolute lack of interest from folks I work with unfortunately :)
I’ve been contemplating pulling the trigger on this one myself. I’m pretty much sold on it. I’ve been on board with squash merging for years. Best thing I’ve ever done for our project. I eventually came to the realisation that the majority of people who were against it were so because some purist greybeard had beat it into them.
