5 ms·

We also do squash and merge by default in my company. Each PR is supposed to represent one and only one feature and/or change. Can you please better explain the

by vault 2y ago

We also do squash and merge by default in my company. Each PR is supposed to represent one and only one feature and/or change. Can you please better explain the disadvantages of this? Of which editing are you talking about? What needs to be edited after the squash and merge?

dpkirchner 2y ago
Bisecting is more efficient when individual commits are preserved, IMO/E.
- slimsag 2y ago
  Inevitably you will encounter (a) a junior developer who considers commits to be 'save states' rather than individual logical changes or (b) someone who is used to squash merge and did not think commit history mattered. Without squash merge, it then becomes very likely most commits do not even build making bisecting a bit of a nightmare.
  - account42 2y ago
    That is where you teach that developer to rebase his commits into logical and self-contained units before merging the PR.
    - boolemancer 2y ago
      That seems like significantly more work for marginal (if any) benefit over just having a single squash commit per PR. I don't think I've ever found myself in a situation where I needed more granularity than the PR level when looking back through the history of a repo. What are the situations where this is actually useful enough to make it worth the effort?
      - mathstuf 2y ago
        I've used it many times. Not just for my changes, but over others' changes. In CMake, we rewrite branches heavily to keep a sensible history within MRs. Not all changes make sense landing commit-by-commit yet also work as a single commit. It also allows for much easier reverting of specific changes in case some part of a topic needs removed. Some examples: - https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9486/commits https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9486... One commit to improve messages; another to add a test case that also uses these messages. Forcing separate CI runs for these dependent commits doesn't make sense, but they also don't belong in a single commit. - https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8996/commits https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8996... Adds two test cases for a regression and then finally reverts the specific regression-exposing commit from https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8197/commits https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8197... while keeping the still-good parts. If the 8197 merge had been squashed, one would have had to manually bisect the hunks to find out which one actually caused the problem.
        boolemancer 2y ago
        > It also allows for much easier reverting of specific changes in case some part of a topic needs removed. I guess I struggle to see where reverting entire commits makes more sense than just deleting the offending code in a new commit.
        mathstuf 2y ago
        Even if that were the case, being able to bisect using `git bisect` over the logical hunks instead of having to manually bisect over them can help determine which code needs deleted without going overboard.
norman784 2y ago
I guess that one disadvantage is that PR's are not a git feature, but a feature of the git provider, so you might get locked in with a provider, I think you can export your PR history, but that means the provider where you are migrating needs to support also that feature.

by vault 2y ago

dpkirchner 2y ago

Bisecting is more efficient when individual commits are preserved, IMO/E.

slimsag 2y ago
Inevitably you will encounter (a) a junior developer who considers commits to be 'save states' rather than individual logical changes or (b) someone who is used to squash merge and did not think commit history mattered. Without squash merge, it then becomes very likely most commits do not even build making bisecting a bit of a nightmare.
- account42 2y ago
  That is where you teach that developer to rebase his commits into logical and self-contained units before merging the PR.
  - boolemancer 2y ago
    That seems like significantly more work for marginal (if any) benefit over just having a single squash commit per PR. I don't think I've ever found myself in a situation where I needed more granularity than the PR level when looking back through the history of a repo. What are the situations where this is actually useful enough to make it worth the effort?
    - mathstuf 2y ago
      I've used it many times. Not just for my changes, but over others' changes. In CMake, we rewrite branches heavily to keep a sensible history within MRs. Not all changes make sense landing commit-by-commit yet also work as a single commit. It also allows for much easier reverting of specific changes in case some part of a topic needs removed. Some examples: - https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9486/commits https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9486... One commit to improve messages; another to add a test case that also uses these messages. Forcing separate CI runs for these dependent commits doesn't make sense, but they also don't belong in a single commit. - https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8996/commits https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8996... Adds two test cases for a regression and then finally reverts the specific regression-exposing commit from https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8197/commits https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8197... while keeping the still-good parts. If the 8197 merge had been squashed, one would have had to manually bisect the hunks to find out which one actually caused the problem.
      - boolemancer 2y ago
        > It also allows for much easier reverting of specific changes in case some part of a topic needs removed. I guess I struggle to see where reverting entire commits makes more sense than just deleting the offending code in a new commit.
        mathstuf 2y ago
        Even if that were the case, being able to bisect using `git bisect` over the logical hunks instead of having to manually bisect over them can help determine which code needs deleted without going overboard.

norman784 2y ago

I guess that one disadvantage is that PR's are not a git feature, but a feature of the git provider, so you might get locked in with a provider, I think you can export your PR history, but that means the provider where you are migrating needs to support also that feature.