9 ms·

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 P

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.

tmpX7dMeXU 3y ago
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.
- iimblack 3y ago
  Majority of people aren’t making atomic commits. In the off chance they are it’s not hard to switch from squash for those one-offs.
huijzer 3y ago
Oh man yes I dislike non-squash merges very much because they make it so hard to go from the Git blame to the pull request discussion
Aeolun 3y ago
I really don’t see the difference between force pushing and not when you are going to squash merge anyway.
- eddythompson80 3y ago
  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
  - mbakke 3y ago
    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.
    - juped 3y ago
      "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!
      - mbakke 3y ago
        I didn't know about --first-parent, thanks! That gives me something to do while narrowing down the exact commit :)
    - eddythompson80 3y ago
      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.
      - mbakke 3y ago
        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
        eddythompson80 3y ago
- mbakke 3y ago
  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).
  - 3np 3y ago
    > 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.
deleted 3y ago
[deleted]
stefan_ 3y ago
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? Here is a random series from DRM patchwork: https://patchwork.kernel.org/project/dri-devel/list/?series=788478 https://patchwork.kernel.org/project/dri-devel/list/?series=... Would you squash that? Hell no, of course not. You would be mixing atomic changes in different subsystems. It breaks bisect and makes blames a mess. 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.
- avita1 3y ago
  > 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.
  - piotrkaminski 3y ago
    > That said, one of my favorite features from gerrit at a past job was that the commit message itself could be reviewed. Reviewable lets you review your commit messages just like any other file, BTW! (Disclosure: I'm the founder.)
- oldtownroad 3y ago
  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).
deleted 3y ago
[deleted]
akoboldfrying 3y ago
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.
- jen20 3y ago
  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.
  - u801e 3y ago
    > 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).
    - jen20 3y ago
      Indeed, but trying to revert lots of tiny commits in one unit is no better. Really it just requires discipline to either make entire related changes as patches or pull requests, which doesn’t matter. The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.
arwineap 3y ago
If you're going to squash on merge, why would you not allow squashing on the branch?
mmcnl 3y ago
That's a better solution than changing the Git(Hub) model. If this works for you, then why the need to change anything?
- jen20 3y ago
  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.
  - u801e 3y ago
    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.

tmpX7dMeXU 3y ago

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.

iimblack 3y ago
Majority of people aren’t making atomic commits. In the off chance they are it’s not hard to switch from squash for those one-offs.

huijzer 3y ago

Oh man yes I dislike non-squash merges very much because they make it so hard to go from the Git blame to the pull request discussion

Aeolun 3y ago

I really don’t see the difference between force pushing and not when you are going to squash merge anyway.

eddythompson80 3y ago
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
- mbakke 3y ago
  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.
  - juped 3y ago
    "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!
    - mbakke 3y ago
      I didn't know about --first-parent, thanks! That gives me something to do while narrowing down the exact commit :)
  - eddythompson80 3y ago
    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.
    - mbakke 3y ago
      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
      - eddythompson80 3y ago
mbakke 3y ago
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).
- 3np 3y ago
  > 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.

deleted 3y ago

[deleted]

stefan_ 3y ago

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? Here is a random series from DRM patchwork: https://patchwork.kernel.org/project/dri-devel/list/?series=788478 https://patchwork.kernel.org/project/dri-devel/list/?series=... Would you squash that? Hell no, of course not. You would be mixing atomic changes in different subsystems. It breaks bisect and makes blames a mess. 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.

avita1 3y ago
> 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.
- piotrkaminski 3y ago
  > That said, one of my favorite features from gerrit at a past job was that the commit message itself could be reviewed. Reviewable lets you review your commit messages just like any other file, BTW! (Disclosure: I'm the founder.)
oldtownroad 3y ago
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).

deleted 3y ago

[deleted]

akoboldfrying 3y ago

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.

jen20 3y ago
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.
- u801e 3y ago
  > 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).
  - jen20 3y ago
    Indeed, but trying to revert lots of tiny commits in one unit is no better. Really it just requires discipline to either make entire related changes as patches or pull requests, which doesn’t matter. The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.

arwineap 3y ago

If you're going to squash on merge, why would you not allow squashing on the branch?

mmcnl 3y ago

That's a better solution than changing the Git(Hub) model. If this works for you, then why the need to change anything?

jen20 3y ago
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.
- u801e 3y ago
  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.