[scikit-learn] The culture of commit squashing

[Juan Nunez-Iglesias]

I think the main idea behind commit squashes is to make sure that every
*commit* passes testing, rather than only merge commits. This is important
because there's no way to tell git bisect to only look at merge commits. So
when you are doing a git bisect to hunt down a regression or bug, it is
very annoying to have a huge string of commits that don't even build —
which turn a binary search into a linear search.

(Some people are concerned that big commit strings aren't proportional to
effort, but I think they are certainly *more* proportional than a squash.)

So I think that's a worthwhile goal, but one that I think GitHub doesn't
support very well. There's a whole demographic of developers that hate
GitHub for code review and prefer Gerrit, and I never really understood
what they were talking about until I read this blog post a couple of months
ago:
https://www.beepsend.com/2016/04/05/abandoning-gitflow-github-favour-gerrit/

If you've never used Gerrit, it's very much worth reading in full.
Sometimes you don't know what you're missing out on until you see it. I
hope that increased pressure from the community will push GitHub to improve
their tooling.

Finally, in the meantime, Stéfan van der Walt and I have started toying
with the idea of using Reviewable, which appears to implement most of
Gerrit's features with the advantage that it integrates with GitHub:
https://reviewable.io/

I hope this helps!

Juan.

On Mon, Jun 13, 2016 at 11:04 PM Joel Nothman <joel.nothman at gmail.com>
wrote:

> My concern is that people are responding to being asked to squash on one
> PR by squashing during development on the next (as if merge were always
> imminent). I want that to stop. Is part of the solution to stop squashing,
> or make the person merging always perform the squash?
>
> On 14 June 2016 at 12:53, Sebastian Raschka <mail at sebastianraschka.com>
> wrote:
>
>> Hi, Joel,
>> in my opinion, it really depends on the particular case, but in general I
>> am pro squashing — that is, when it happens at the very end. I also agree
>> that squashing and force pushing while there’s still a review going on is
>> clearly disruptive.
>> Say there’s a new estimator being added. This often comes with an insane
>> number of pull requests.
>>
>> - first take on EstimatorX
>> - fix xyz in EstimatorX
>> - address test case issue x
>> - add additional test case to test edge case x
>> - add code example for estimator x
>> - fix typo in documentation for estimator x
>> …
>>
>> So, once everything looks fine to the reviewers, everyone gave their
>> okay, the CI tests pass, I think there’s nothing against summarizing it to
>> a single commit:
>>
>> - implement EstimatorX
>>
>> In my opinion, it helps tracking down code in the commit history in the
>> long run, but that’s just my personal opinion.
>>
>> Best,
>> Sebastian
>>
>>
>> > On Jun 13, 2016, at 9:36 PM, Joel Nothman <joel.nothman at gmail.com>
>> wrote:
>> >
>> > For the last few years, there's been a notion that we should squash PRs
>> down to a single commit before merging. Squashing can give a cleaner commit
>> history, and avoid overrepresentation of minor work given silly commit
>> count metrics used by Github and others. I'm not sure if there are other
>> motivations.
>> >
>> > Recently I've seen several contributors amending commits and
>> force-pushing changes. I find this disruptive to the reviewing process in a
>> number of ways (links are broken; what's changed is hard to discern, when
>> it could have been indicated in a commit message and diff; etc.). I have
>> had to ask these several users to cease and desist.
>> >
>> > I also find that performing the squash can be unnecessary overhead
>> either for the merger or the PR developer.
>> >
>> > I think squashing is more trouble than it's worth, except where:
>> > * there are embarrassingly many minor commits in a PR
>> > * squashing first makes a rebase easier because of concurrent changes
>> to the codebase
>> > * otherwise for cosmetic reasons only when there is low reviewing
>> activity on the PR
>> >
>> > While squashing is far from the slowest part of our review process,
>> being able to hit the merge button and move on would be great.
>> >
>> > Do others agree that a culture of amending commits in the ordinary case
>> is counterproductive?
>> >
>> > (And apologies for wasting your time on such a silly issue, but I'm
>> sick of clicking links in emails to find the commit's disappeared.)
>> > _______________________________________________
>> > scikit-learn mailing list
>> > scikit-learn at python.org
>> > https://mail.python.org/mailman/listinfo/scikit-learn
>>
>> _______________________________________________
>> scikit-learn mailing list
>> scikit-learn at python.org
>> https://mail.python.org/mailman/listinfo/scikit-learn
>>
>
> _______________________________________________
> scikit-learn mailing list
> scikit-learn at python.org
> https://mail.python.org/mailman/listinfo/scikit-learn
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/scikit-learn/attachments/20160614/a3eeac6b/attachment-0001.html>