4 ms·

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

by Aeolun 3y ago

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
        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 :)
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.

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

by Aeolun 3y ago

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

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
      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 :)

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.