On the topic of separate merge commits
Hi all, I recently received some criticism for merging a PR that had only 1 commit and producing a single merge commit on top of it (this happens to be the default option I have/what I've been doing for years). See here: https://github.com/scipy/scipy/pull/14630 I'd like to mention a few things here: - are we still "ok" with folks doing this? frankly, I find taking heat for precise merging philosophy a bit unwelcoming even as someone who has been contributing for a while; sometimes it is important to squash 100 messy commits, but otherwise do a good job reviewing PRs and relaxing on the purist "git history" attacks seem a bit more welcoming to me - I find those merge commits somewhat useful when navigating back to GitHub anyway, maybe because I'm so used to them - why does GitHub allow us to do this if it really isn't useful at all? Tyler
Git and GitHub allow a plethora of modalities. Many of them quite ill-advised. Others simply suitable for different development strategies. It’s up to the project to decide how it wants to work. I express no opinion about merge commits, but I found your response in that thread over the top. On Aug 24, 2021, at 10:05 AM, Tyler Reddy <tyler.je.reddy@gmail.com<mailto:tyler.je.reddy@gmail.com>> wrote: Hi all, I recently received some criticism for merging a PR that had only 1 commit and producing a single merge commit on top of it (this happens to be the default option I have/what I've been doing for years). See here: https://github.com/scipy/scipy/pull/14630 I'd like to mention a few things here: - are we still "ok" with folks doing this? frankly, I find taking heat for precise merging philosophy a bit unwelcoming even as someone who has been contributing for a while; sometimes it is important to squash 100 messy commits, but otherwise do a good job reviewing PRs and relaxing on the purist "git history" attacks seem a bit more welcoming to me - I find those merge commits somewhat useful when navigating back to GitHub anyway, maybe because I'm so used to them - why does GitHub allow us to do this if it really isn't useful at all? Tyler _______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org<mailto:SciPy-Dev@python.org> https://mail.python.org/mailman/listinfo/scipy-dev
On 24.08.2021, at 16:05, Tyler Reddy <tyler.je.reddy@gmail.com> wrote:
Hi all,
I recently received some criticism for merging a PR that had only 1 commit and producing a single merge commit on top of it (this happens to be the default option I have/what I've been doing for years). See here: https://github.com/scipy/scipy/pull/14630 <https://github.com/scipy/scipy/pull/14630>
I'd like to mention a few things here: - are we still "ok" with folks doing this? frankly, I find taking heat for precise merging philosophy a bit unwelcoming even as someone who has been contributing for a while; sometimes it is important to squash 100 messy commits, but otherwise do a good job reviewing PRs and relaxing on the purist "git history" attacks seem a bit more welcoming to me - I find those merge commits somewhat useful when navigating back to GitHub anyway, maybe because I'm so used to them - why does GitHub allow us to do this if it really isn't useful at all?
Hi Tyler, I am sorry that you saw an attack in my comment. That was not my intention and I strongly think that it is not what my message and replies convey. The essence of my comment was: why merge instead of squash for a single commit. I asked you directly for 2 reasons: one, it’s on one of my PR; two, I saw you do this on multiple PRs. Hence I thought you would be the best placed to answer my interrogations. As I said, if it’s for the authorship and presence in the history (which I totally agree, when reviewing you should be in the history. I even opened a feature request on GitHub: https://github.com/github/feedback/discussions/4525 <https://github.com/github/feedback/discussions/4525>), then I suggest to use the “Co-authored-by” feature. As for the default, GitHub is considering the last action you did. If you merged a PR, next time it will show you “Merge Pull Request”. In my case for instance, I last “Squash and merge” and GitHub is showing me this. You can just click on the arrow to change this. Pamphile
Hence I thought you would be the best placed to answer my interrogations.
I am certainly not the appropriate person to target for "interrogations" on this matter. It is a long-standing practice in the community, and a feature GitHub has in their GUI to create separate merge commits even when there is only a single commit in the PR. If SciPy or the community strongly prefers not to do it, then I'll switch to squashing, but until then I strongly disagree that I should have to defend PRs where I do that. Tyler On Tue, 24 Aug 2021 at 08:32, Pamphile Roy <roy.pamphile@gmail.com> wrote:
On 24.08.2021, at 16:05, Tyler Reddy <tyler.je.reddy@gmail.com> wrote:
Hi all,
I recently received some criticism for merging a PR that had only 1 commit and producing a single merge commit on top of it (this happens to be the default option I have/what I've been doing for years). See here: https://github.com/scipy/scipy/pull/14630
I'd like to mention a few things here: - are we still "ok" with folks doing this? frankly, I find taking heat for precise merging philosophy a bit unwelcoming even as someone who has been contributing for a while; sometimes it is important to squash 100 messy commits, but otherwise do a good job reviewing PRs and relaxing on the purist "git history" attacks seem a bit more welcoming to me - I find those merge commits somewhat useful when navigating back to GitHub anyway, maybe because I'm so used to them - why does GitHub allow us to do this if it really isn't useful at all?
Hi Tyler,
I am sorry that you saw an attack in my comment.
That was not my intention and I strongly think that it is not what my message and replies convey.
The essence of my comment was: why merge instead of squash for a single commit. I asked you directly for 2 reasons: one, it’s on one of my PR; two, I saw you do this on multiple PRs. Hence I thought you would be the best placed to answer my interrogations.
As I said, if it’s for the authorship and presence in the history (which I totally agree, when reviewing you should be in the history. I even opened a feature request on GitHub: https://github.com/github/feedback/discussions/4525), then I suggest to use the “Co-authored-by” feature.
As for the default, GitHub is considering the last action you did. If you merged a PR, next time it will show you “Merge Pull Request”. In my case for instance, I last “Squash and merge” and GitHub is showing me this. You can just click on the arrow to change this.
Pamphile _______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org https://mail.python.org/mailman/listinfo/scipy-dev
Hi, On Tue, Aug 24, 2021 at 4:51 PM Tyler Reddy <tyler.je.reddy@gmail.com> wrote:
Hence I thought you would be the best placed to answer my interrogations.
I am certainly not the appropriate person to target for "interrogations" on this matter. It is a long-standing practice in the community, and a feature GitHub has in their GUI to create separate merge commits even when there is only a single commit in the PR.
If SciPy or the community strongly prefers not to do it, then I'll switch to squashing, but until then I strongly disagree that I should have to defend PRs where I do that.
I must say - that's what I tend to do (merge, not squash). I can see arguments for squashing, if the history is very messy, but if the history is good, and the commits are well organized, squashing seems like the wrong thing to do. Why not leave it to the judgement of the person doing the merge? Cheers, Matthew
On Tue, Aug 24, 2021 at 4:51 PM Tyler Reddy <tyler.je.reddy@gmail.com> wrote:
Hence I thought you would be the best placed to answer my interrogations.
I am certainly not the appropriate person to target for "interrogations" on this matter. It is a long-standing practice in the community, and a feature GitHub has in their GUI to create separate merge commits even when there is only a single commit in the PR.
If SciPy or the community strongly prefers not to do it, then I'll switch to squashing, but until then I strongly disagree that I should have to defend PRs where I do that.
I must say - that's what I tend to do (merge, not squash). I can see arguments for squashing, if the history is very messy, but if the history is good, and the commits are well organized, squashing seems like the wrong thing to do. Why not leave it to the judgement of the person doing the merge?
I'd second what Mattew said. (I would also create a merge commit without much thinking, btw). There are clear limiting cases (messy history; a single 5kLOC commit etc), but otherwise it's really a judgement call of a person who does the merge. While Github does offer a bunch of options, deciding to just not think about the merge mode is a perfectly valid strategy. I personally don't think we need to standardize it, or even think about it too much. In the project of our size, there's going to be some variation between maintainers, and it's likely OK. One person prefers to squash, the other prefers a merge commit --- and it's fine IMO. Cheers, Evgeni
I'm okay with each maintainer following their preference, as there are arguments either way. That said, if people don't care much either way or want to think about it, I'd advocate for "squash and merge" as the default policy. I started using it (whenever they first introduced it years ago) across multiple repos with no observed practical drawbacks so far. On the other hand, it has improved maintainability in at least the following ways: 1) It makes `git bisect` easier as we are (almost) guaranteed that tests worked before the commit and after the commit. It's not 100% guaranteed because maintainers might miss a red CI/failing test in a PR, but this is rare. When we merge without squashing, intermediate commits often have errors. Squash+merge means there is no need for skipping steps in the bisect, all can be good/bad. 2) It keeps history smaller. 3) It makes a nicer/more direct mapping between commits to scipy:master and PRs. 4) Because of 1-3, browsing commits and running `git bisect` are faster. The only real disadvantage as I see it is that individual commits are lost (the flipside of (2)), so if these are really helpful being split for some reason, you'd have to go back to the original PR / branch to see them (assuming the branch is not deleted). For what it's worth, though, I have not run into a case where split commits are actually helpful across many `git bisect`s and many PRs across repos, so to me it's rare enough not to outweigh the benefits. The few remaining times I do a plain `merge` is when I see that the commits in the PR all have green CIs (or at least were likely to if they had run) and probably could have been individual PRs on their own (e.g., "move function", "fix bug", "add enhancement" all as part of making one thing more usable in a single PR). Some contributors are very good about structuring/restructuring their PRs this way, but it seems to be the (rare) exception and not the rule. If nobody objects to this line of reasoning, it might be worth putting a "if you don't care, make your default squash + merge and use it; if you do care, use your best judgment" in the maintainers guide somewhere. If people don't buy my arguments, then we should just put "use your best judgment". My 2c, Eric On Wed, Aug 25, 2021 at 7:15 AM Evgeni Burovski <evgeny.burovskiy@gmail.com> wrote:
On Tue, Aug 24, 2021 at 4:51 PM Tyler Reddy <tyler.je.reddy@gmail.com> wrote:
Hence I thought you would be the best placed to answer my
interrogations.
I am certainly not the appropriate person to target for
"interrogations" on this matter. It is a long-standing practice in the community, and a feature GitHub has in their GUI to create separate merge commits even when there is only a single commit in the PR.
If SciPy or the community strongly prefers not to do it, then I'll
switch to squashing, but until then I strongly disagree that I should have to defend PRs where I do that.
I must say - that's what I tend to do (merge, not squash). I can see arguments for squashing, if the history is very messy, but if the history is good, and the commits are well organized, squashing seems like the wrong thing to do. Why not leave it to the judgement of the person doing the merge?
I'd second what Mattew said. (I would also create a merge commit without much thinking, btw). There are clear limiting cases (messy history; a single 5kLOC commit etc), but otherwise it's really a judgement call of a person who does the merge. While Github does offer a bunch of options, deciding to just not think about the merge mode is a perfectly valid strategy. I personally don't think we need to standardize it, or even think about it too much. In the project of our size, there's going to be some variation between maintainers, and it's likely OK. One person prefers to squash, the other prefers a merge commit --- and it's fine IMO.
Cheers,
Evgeni _______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org https://mail.python.org/mailman/listinfo/scipy-dev
On Wed, Aug 25, 2021 at 10:04 AM Eric Larson <larson.eric.d@gmail.com> wrote:
I'm okay with each maintainer following their preference, as there are arguments either way.
That said, if people don't care much either way or want to think about it, I'd advocate for "squash and merge" as the default policy. I started using it (whenever they first introduced it years ago) across multiple repos with no observed practical drawbacks so far. On the other hand, it has improved maintainability in at least the following ways:
1) It makes `git bisect` easier as we are (almost) guaranteed that tests worked before the commit and after the commit. It's not 100% guaranteed because maintainers might miss a red CI/failing test in a PR, but this is rare. When we merge without squashing, intermediate commits often have errors. Squash+merge means there is no need for skipping steps in the bisect, all can be good/bad. 2) It keeps history smaller. 3) It makes a nicer/more direct mapping between commits to scipy:master and PRs. 4) Because of 1-3, browsing commits and running `git bisect` are faster.
For similar reasons we do the opposite in statsmodels. only merging pull requests is allowed. The commit history clearly marks merges by "Merge pull request <https://github.com/statsmodels/statsmodels/commit/e0f08722b8d26b8f16069627e2e62f6288f0f883> ...." and is easy to search. intermediate commits can be ignored for maintenance, but are important to understand and bughunt the code. Jowd
The only real disadvantage as I see it is that individual commits are lost (the flipside of (2)), so if these are really helpful being split for some reason, you'd have to go back to the original PR / branch to see them (assuming the branch is not deleted). For what it's worth, though, I have not run into a case where split commits are actually helpful across many `git bisect`s and many PRs across repos, so to me it's rare enough not to outweigh the benefits.
The few remaining times I do a plain `merge` is when I see that the commits in the PR all have green CIs (or at least were likely to if they had run) and probably could have been individual PRs on their own (e.g., "move function", "fix bug", "add enhancement" all as part of making one thing more usable in a single PR). Some contributors are very good about structuring/restructuring their PRs this way, but it seems to be the (rare) exception and not the rule.
If nobody objects to this line of reasoning, it might be worth putting a "if you don't care, make your default squash + merge and use it; if you do care, use your best judgment" in the maintainers guide somewhere. If people don't buy my arguments, then we should just put "use your best judgment".
My 2c, Eric
On Wed, Aug 25, 2021 at 7:15 AM Evgeni Burovski < evgeny.burovskiy@gmail.com> wrote:
On Tue, Aug 24, 2021 at 4:51 PM Tyler Reddy <tyler.je.reddy@gmail.com> wrote:
Hence I thought you would be the best placed to answer my
interrogations.
I am certainly not the appropriate person to target for
"interrogations" on this matter. It is a long-standing practice in the community, and a feature GitHub has in their GUI to create separate merge commits even when there is only a single commit in the PR.
If SciPy or the community strongly prefers not to do it, then I'll
switch to squashing, but until then I strongly disagree that I should have to defend PRs where I do that.
I must say - that's what I tend to do (merge, not squash). I can see arguments for squashing, if the history is very messy, but if the history is good, and the commits are well organized, squashing seems like the wrong thing to do. Why not leave it to the judgement of the person doing the merge?
I'd second what Mattew said. (I would also create a merge commit without much thinking, btw). There are clear limiting cases (messy history; a single 5kLOC commit etc), but otherwise it's really a judgement call of a person who does the merge. While Github does offer a bunch of options, deciding to just not think about the merge mode is a perfectly valid strategy. I personally don't think we need to standardize it, or even think about it too much. In the project of our size, there's going to be some variation between maintainers, and it's likely OK. One person prefers to squash, the other prefers a merge commit --- and it's fine IMO.
Cheers,
Evgeni _______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org https://mail.python.org/mailman/listinfo/scipy-dev
_______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org https://mail.python.org/mailman/listinfo/scipy-dev
On Wed, Aug 25, 2021, at 07:04, Eric Larson wrote:
Some contributors are very good about structuring/restructuring their PRs this way, but it seems to be the (rare) exception and not the rule.
This is in line with my experience. If you have developers who are willing to craft their patches very carefully, a merge strategy makes sense. Otherwise, since PRs are meant to be focused in scope already, squashing works well in practice. Github also implicitly encourages an append-to-PR workflow. They have "view only what has changed" when you only append patches, and if I'm not mistaken cannot always track comments across rebase. Perhaps most importantly, I think it is unreasonable to expect newcomers to rebase their work when merge is so much easier. So, for those PRs we should squash anyway. Eric, I agree with what you wrote. For `git bisect` the `--first-parent` flag could be useful in only evaluating commits on the root of the commit tree. Stéfan
Hi, On Wed, Aug 25, 2021 at 5:06 PM Stefan van der Walt <stefanv@berkeley.edu> wrote:
On Wed, Aug 25, 2021, at 07:04, Eric Larson wrote:
Some contributors are very good about structuring/restructuring their PRs this way, but it seems to be the (rare) exception and not the rule.
This is in line with my experience. If you have developers who are willing to craft their patches very carefully, a merge strategy makes sense. Otherwise, since PRs are meant to be focused in scope already, squashing works well in practice.
Github also implicitly encourages an append-to-PR workflow. They have "view only what has changed" when you only append patches, and if I'm not mistaken cannot always track comments across rebase.
Perhaps most importantly, I think it is unreasonable to expect newcomers to rebase their work when merge is so much easier. So, for those PRs we should squash anyway.
Eric, I agree with what you wrote. For `git bisect` the `--first-parent` flag could be useful in only evaluating commits on the root of the commit tree.
Surely the `--first-parent` option to git bisect switches the argument back in favor of merge rather than squash, in that you can get the advantages of squash from that flag, without the disadvantage of having to go back to the pull request history on the Github interface, or by recovering the PR history somehow. It's surely true that many less experienced developers don't give a clean history, but for those that do, it's a clear advantage to have it available with a standard checkout. So it still seems to me that a simple 'merge or squash, it's your call' policy is the best one. Cheers, Matthew
On Wed, Aug 25, 2021 at 8:35 PM Matthew Brett <matthew.brett@gmail.com> wrote:
Hi,
On Wed, Aug 25, 2021 at 5:06 PM Stefan van der Walt <stefanv@berkeley.edu> wrote:
On Wed, Aug 25, 2021, at 07:04, Eric Larson wrote:
Some contributors are very good about structuring/restructuring their
PRs this way, but it seems to be the (rare) exception and not the rule.
This is in line with my experience. If you have developers who are
willing to craft their patches very carefully, a merge strategy makes sense. Otherwise, since PRs are meant to be focused in scope already, squashing works well in practice.
Github also implicitly encourages an append-to-PR workflow. They have
"view only what has changed" when you only append patches, and if I'm not mistaken cannot always track comments across rebase.
Perhaps most importantly, I think it is unreasonable to expect newcomers
to rebase their work when merge is so much easier. So, for those PRs we should squash anyway.
Eric, I agree with what you wrote. For `git bisect` the
`--first-parent` flag could be useful in only evaluating commits on the root of the commit tree.
Surely the `--first-parent` option to git bisect switches the argument back in favor of merge rather than squash, in that you can get the advantages of squash from that flag, without the disadvantage of having to go back to the pull request history on the Github interface, or by recovering the PR history somehow.
It's surely true that many less experienced developers don't give a clean history, but for those that do, it's a clear advantage to have it available with a standard checkout.
So it still seems to me that a simple 'merge or squash, it's your call' policy is the best one.
I agree with this, the maintainer should apply some judgement. There are cases where it's a toss-up (like single-commit PRs), there are messy PRs that clearly need squashing, and there are PRs with good granular commits that should be preserved. Cheers, Ralf
participants (9)
-
Eric Larson -
Evgeni Burovski -
Guyer, Jonathan E. Dr. (Fed) -
josef.pktd＠gmail.com -
Matthew Brett -
Pamphile Roy -
Ralf Gommers -
Stefan van der Walt -
Tyler Reddy