[SciPy-Dev] On the topic of separate merge commits

[josef.pktd at gmail.com]

On Wed, Aug 25, 2021 at 10:04 AM Eric Larson <larson.eric.d at 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 at gmail.com> wrote:
>
>> > On Tue, Aug 24, 2021 at 4:51 PM Tyler Reddy <tyler.je.reddy at 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 at python.org
>> https://mail.python.org/mailman/listinfo/scipy-dev
>>
> _______________________________________________
> SciPy-Dev mailing list
> SciPy-Dev at python.org
> https://mail.python.org/mailman/listinfo/scipy-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.python.org/pipermail/scipy-dev/attachments/20210825/3d35868e/attachment.html>