6 ms·

Understand where author is coming from - but doesn't squash-n-merge (newish github feature) solve the issue of needing to rebase and the issue of having too man

Understand where author is coming from - but doesn't squash-n-merge (newish github feature) solve the issue of needing to rebase and the issue of having too many merge commits? Squash-n-merge has nice property of removing unnecessary local information that probably doesn't matter at a meta level (commits are nice when reviewing PR, doesn't matter much later)

pabs3 5y ago
Squashing commits into one mega-commit isn't great for future investigations of the commit history (code review, bisects etc). It is much better to create separate logical commits, rebase them and pull in the result, either as a branch fast-forward merge, or with a merge commit where appropriate.
- properdine 5y ago
  a year from now, are you actually going to want to test each individual change in a pull request, or are you going to want to test it as an entire unit? I agree that code review you want smaller units but my experience has been that 1-2 years later, you no longer care about the individual units and instead you want the entire patch/PR all together.
  - dboreham 5y ago
    I think it's more about: in a year from now will you understand the purpose of a change to some code you're debugging? If the commit says "merged PR 2234", answer is probably not.
  - pabs3 5y ago
    I definitely will want to do that, especially when bisecting a random bug that was introduced with one of the changes in that PR. The smaller the unit of change the better, as long as they are logically separate changes.
  - pgaddict 5y ago
    I'm pretty sure you want reasonable meaningful commits. On tiny projects it may not matter, but on larger projects it's definitely a huge benefit, because chances are you'll have to investigate a bug in that code, re-learn why it was done this way, etc. And maybe bisect the git history to find which exact commit caused the issue. Which is why larger changes are often split into smaller patches that may be applied and tested incrementally. If you just merge the whole pull request as one huge patch / in merge commit, you just lost most of that.
- rectang 5y ago
  Nirvana is: • Setting `merge.ff=no` in git config to force merge commits by default. • Creating a series of logical commits on `my-feature-branch`. • Merging `my-feature-branch` into `main` with a bona fide merge commit. • Using `git branch -d my-feature-branch` (NOT capital `-D`) to delete the feature branch safely and without worry, since `-d` only deletes the branch if the commits are present on HEAD. • Using `git log --oneline --graph` to see a clean representation of the actual history.
  - pabs3 5y ago
    I would only use merge commits when it is appropriate, like a commit series porting usage of a dependency from an old version to a new one.
    - rectang 5y ago
      Well, there are different views of "appropriate". When you do team development and everything is done via feature branches, it's nice to have merge commits so that the integrity of the each feature development effort is preserved via a merged branch in the history. If everything is flattened, it's harder to see where the branches (standing in for development initiatives) begin and end. You can't always get fast-forward merges anyway. Long-lived branches with merge conflicts are undesirable but unavoidable in the long run. At least some of the time, you're going to have merge commits even when your "appropriateness" test says there shouldn't be one.
      - pabs3 5y ago
        Do you at least agree that merge commits for single-commit PRs aren't "appropriate"?
        rectang 5y ago
        I don't feel strongly about the issue. A good clean fast-forward merge of a single-commit PR is fine. But I've also worked at multiple jobs where every merge to the production branch created a merge commit and that's also fine. It adds a bit of complexity to the history graph, but it's not meaningless complexity. If your commit history is majority single-commit PRs then having additional merge-commits everywhere would be noisy, so in that case it would be too much. I don't tend to work on actively developed projects that match that pattern, though. Most feature development involves multiple-commit branches.
- earthboundkid 5y ago
  When someone invents the git killer, it will have a feature called “subcommits” that will be blindingly obvious in hindsight.
  - renewiltord 5y ago
    You get this by forcing merge commits for every non-single-commit change.
    - urxvtcd 5y ago
      Not sure why you are being downvoted. You actually can use merge commits this way, by viewing the diff produced by git diff $merge_commit^...$merge_commit^2 when interested in whole change-set introduced by a branch or looking at individual commmits when interested in well, individual commits.
    - earthboundkid 5y ago
      Sure, you can use git to do this, but the git killer will have it as an expected capability. I also think that octopus merges are basically always a disaster because they can't be meaningfully reviewed and put your repo into an unknown state. Maybe there's some way to get the advantages of merge commits (preserve all history!) without the disadvantages (jumble all history!).
    - WorldMaker 5y ago
      Yup, and you can use --first-parent to git bisect, git log, git praise to interact at the "macro-level" of those merge commits by default, and dive in to the fuller graph only as necessary.
  - oftenwrong 5y ago
    If I've correctly understood what you mean, I've wanted this for some time now. A way to preserve history while adding a single, linear integration of changes.
fishywang 5y ago
(squash-n-merge isn't new on github, unless you are not talking about the same thing I'm thinking about) Yes squash-n-merge is often needed in github's PR workflow because no one need those un-bisect-able fixup commits in the final merged master/main branch, and also they make the diff between different states of the PR more readable, but it comes with its own problems. Main problem is commit message. As the contributor (the one sending out the PR for maintainer to review), you have no control on what the final commit message in the merged single commit is. The maintainer doing the merge decides that for you, and by default github generates that message by combining all the commit message titles (the first line of the commit messages) of all the commits in that branch, and that's almost never the good choice for the final commit message. Another problem with that is the email in the final commit. When the maintainer use squash-n-merge, github uses your default email on file on your github account, regardless whichever email(s) you configured your git to use and associated with those individual commits inside the PR. As a result, squash-n-merge is more suitable for contributors less familiar with open source contribution, for example people not yet realized the value of a good, concise commit message, and people don't have different email addresses for different projects. For advanced contributors, there's no wonder they would prefer force-push with rebase-merge when they are making contributions on github, because rebase-merge makes sure the exact state of their final commit is preserved, including commit message, email address associated with it, and gpg signature if they use that. But github's rebase-merge strategy has its own issues, as described by the author and more.
- Ar-Curunir 5y ago
  You can work around all of this as a contributor by squashing on your own end before the final merge.
  - fishywang 5y ago
    That comes with all the problems with force push and rebase, bar the history during code review one. For example this still has a commit message issue, just on the maintainer's side: As the maintainer if you are going to use rebase to merge this PR, that means you need to accept whatever commit message the contributor wrote as-is. Are you happy with that? If not, you can't even leave inline comments on that, and it's usually pretty hard to communicate and give feedback on how you want the commit message to be.
cryptonector 5y ago
Sometimes (often!) you want clean history in the upstream but also patches separated by bugs they fix, features they add -- issue/ticket numbers, whatever. And you may want regression tests to come before bug fixes, that way you can see the regression test failing, then the test passing after applying the bug fix. Different upstreams are likely to have different rules. So squash-and-merge is a bad one-size-fits-all. Rebase is a much much better approach: you keep the history as submitted and you lose the useless merge commit. There's no "unnecessary local information" if the submitter did the work of cleaning up their history before submitting. That means doing interactive rebases locally to squash/fixup/edit(and-possibly-split)/reword/drop/reorder their commits -- this is something every developer should know how to do.

pabs3 5y ago

Squashing commits into one mega-commit isn't great for future investigations of the commit history (code review, bisects etc). It is much better to create separate logical commits, rebase them and pull in the result, either as a branch fast-forward merge, or with a merge commit where appropriate.

properdine 5y ago
a year from now, are you actually going to want to test each individual change in a pull request, or are you going to want to test it as an entire unit? I agree that code review you want smaller units but my experience has been that 1-2 years later, you no longer care about the individual units and instead you want the entire patch/PR all together.
- dboreham 5y ago
  I think it's more about: in a year from now will you understand the purpose of a change to some code you're debugging? If the commit says "merged PR 2234", answer is probably not.
- pabs3 5y ago
  I definitely will want to do that, especially when bisecting a random bug that was introduced with one of the changes in that PR. The smaller the unit of change the better, as long as they are logically separate changes.
- pgaddict 5y ago
  I'm pretty sure you want reasonable meaningful commits. On tiny projects it may not matter, but on larger projects it's definitely a huge benefit, because chances are you'll have to investigate a bug in that code, re-learn why it was done this way, etc. And maybe bisect the git history to find which exact commit caused the issue. Which is why larger changes are often split into smaller patches that may be applied and tested incrementally. If you just merge the whole pull request as one huge patch / in merge commit, you just lost most of that.
rectang 5y ago
Nirvana is: • Setting `merge.ff=no` in git config to force merge commits by default. • Creating a series of logical commits on `my-feature-branch`. • Merging `my-feature-branch` into `main` with a bona fide merge commit. • Using `git branch -d my-feature-branch` (NOT capital `-D`) to delete the feature branch safely and without worry, since `-d` only deletes the branch if the commits are present on HEAD. • Using `git log --oneline --graph` to see a clean representation of the actual history.
- pabs3 5y ago
  I would only use merge commits when it is appropriate, like a commit series porting usage of a dependency from an old version to a new one.
  - rectang 5y ago
    Well, there are different views of "appropriate". When you do team development and everything is done via feature branches, it's nice to have merge commits so that the integrity of the each feature development effort is preserved via a merged branch in the history. If everything is flattened, it's harder to see where the branches (standing in for development initiatives) begin and end. You can't always get fast-forward merges anyway. Long-lived branches with merge conflicts are undesirable but unavoidable in the long run. At least some of the time, you're going to have merge commits even when your "appropriateness" test says there shouldn't be one.
    - pabs3 5y ago
      Do you at least agree that merge commits for single-commit PRs aren't "appropriate"?
      - rectang 5y ago
        I don't feel strongly about the issue. A good clean fast-forward merge of a single-commit PR is fine. But I've also worked at multiple jobs where every merge to the production branch created a merge commit and that's also fine. It adds a bit of complexity to the history graph, but it's not meaningless complexity. If your commit history is majority single-commit PRs then having additional merge-commits everywhere would be noisy, so in that case it would be too much. I don't tend to work on actively developed projects that match that pattern, though. Most feature development involves multiple-commit branches.
earthboundkid 5y ago
When someone invents the git killer, it will have a feature called “subcommits” that will be blindingly obvious in hindsight.
- renewiltord 5y ago
  You get this by forcing merge commits for every non-single-commit change.
  - urxvtcd 5y ago
    Not sure why you are being downvoted. You actually can use merge commits this way, by viewing the diff produced by git diff $merge_commit^...$merge_commit^2 when interested in whole change-set introduced by a branch or looking at individual commmits when interested in well, individual commits.
  - earthboundkid 5y ago
    Sure, you can use git to do this, but the git killer will have it as an expected capability. I also think that octopus merges are basically always a disaster because they can't be meaningfully reviewed and put your repo into an unknown state. Maybe there's some way to get the advantages of merge commits (preserve all history!) without the disadvantages (jumble all history!).
  - WorldMaker 5y ago
    Yup, and you can use --first-parent to git bisect, git log, git praise to interact at the "macro-level" of those merge commits by default, and dive in to the fuller graph only as necessary.
- oftenwrong 5y ago
  If I've correctly understood what you mean, I've wanted this for some time now. A way to preserve history while adding a single, linear integration of changes.

fishywang 5y ago

(squash-n-merge isn't new on github, unless you are not talking about the same thing I'm thinking about) Yes squash-n-merge is often needed in github's PR workflow because no one need those un-bisect-able fixup commits in the final merged master/main branch, and also they make the diff between different states of the PR more readable, but it comes with its own problems. Main problem is commit message. As the contributor (the one sending out the PR for maintainer to review), you have no control on what the final commit message in the merged single commit is. The maintainer doing the merge decides that for you, and by default github generates that message by combining all the commit message titles (the first line of the commit messages) of all the commits in that branch, and that's almost never the good choice for the final commit message. Another problem with that is the email in the final commit. When the maintainer use squash-n-merge, github uses your default email on file on your github account, regardless whichever email(s) you configured your git to use and associated with those individual commits inside the PR. As a result, squash-n-merge is more suitable for contributors less familiar with open source contribution, for example people not yet realized the value of a good, concise commit message, and people don't have different email addresses for different projects. For advanced contributors, there's no wonder they would prefer force-push with rebase-merge when they are making contributions on github, because rebase-merge makes sure the exact state of their final commit is preserved, including commit message, email address associated with it, and gpg signature if they use that. But github's rebase-merge strategy has its own issues, as described by the author and more.

Ar-Curunir 5y ago
You can work around all of this as a contributor by squashing on your own end before the final merge.
- fishywang 5y ago
  That comes with all the problems with force push and rebase, bar the history during code review one. For example this still has a commit message issue, just on the maintainer's side: As the maintainer if you are going to use rebase to merge this PR, that means you need to accept whatever commit message the contributor wrote as-is. Are you happy with that? If not, you can't even leave inline comments on that, and it's usually pretty hard to communicate and give feedback on how you want the commit message to be.

cryptonector 5y ago

Sometimes (often!) you want clean history in the upstream but also patches separated by bugs they fix, features they add -- issue/ticket numbers, whatever. And you may want regression tests to come before bug fixes, that way you can see the regression test failing, then the test passing after applying the bug fix. Different upstreams are likely to have different rules. So squash-and-merge is a bad one-size-fits-all. Rebase is a much much better approach: you keep the history as submitted and you lose the useless merge commit. There's no "unnecessary local information" if the submitter did the work of cleaning up their history before submitting. That means doing interactive rebases locally to squash/fixup/edit(and-possibly-split)/reword/drop/reorder their commits -- this is something every developer should know how to do.