The culture of commit squashing
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.)
My research work involves frequently contributing small changes. I like to keep these around as a record of what I've done, until I've finished with that part of the code. However, I also hate having large numbers of commits (frequently can commit 50+ times a day without much substantitve progress). To combine these, usually I will avoid squashing commits in a branch until right before I merge it. This way you can review everything which has been done until you're finished with that branch, but also avoid having a large number of trivial commits. In this case, only after you've been given MRG +2 would you squash the PR. That would have a negative side effect of preventing the second reviewer from quickly merging the branch, though. What are your thoughts on that, Joel? On Mon, Jun 13, 2016 at 6:36 PM, Joel Nothman <joel.nothman@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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
I don't even think that squashing them before the merge is actually sound. You will still need the history of why something happened several years down the road (and rebasing actually has a similar issue). This bit me quite often (having just one big commit to analyze after a merge from ancient VCS). Git was created to keep the history when merging, why would we explicitly remove knowledge? Just my 2 cents. 2016-06-14 4:40 GMT+02:00 Jacob Schreiber <jmschreiber91@gmail.com>:
My research work involves frequently contributing small changes. I like to keep these around as a record of what I've done, until I've finished with that part of the code. However, I also hate having large numbers of commits (frequently can commit 50+ times a day without much substantitve progress). To combine these, usually I will avoid squashing commits in a branch until right before I merge it. This way you can review everything which has been done until you're finished with that branch, but also avoid having a large number of trivial commits. In this case, only after you've been given MRG +2 would you squash the PR. That would have a negative side effect of preventing the second reviewer from quickly merging the branch, though.
What are your thoughts on that, Joel?
On Mon, Jun 13, 2016 at 6:36 PM, Joel Nothman <joel.nothman@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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
-- Information System Engineer, Ph.D. Blog: http://blog.audio-tk.com/ LinkedIn: http://www.linkedin.com/in/matthieubrucher
We could stop squashing during development, and use the *new* Squash-and-Merge <https://github.com/blog/2141-squash-your-commits> button on GitHub. What do you think? Tom 2016-06-14 8:06 GMT+02:00 Matthieu Brucher <matthieu.brucher@gmail.com>:
I don't even think that squashing them before the merge is actually sound. You will still need the history of why something happened several years down the road (and rebasing actually has a similar issue). This bit me quite often (having just one big commit to analyze after a merge from ancient VCS). Git was created to keep the history when merging, why would we explicitly remove knowledge? Just my 2 cents.
2016-06-14 4:40 GMT+02:00 Jacob Schreiber <jmschreiber91@gmail.com>:
My research work involves frequently contributing small changes. I like to keep these around as a record of what I've done, until I've finished with that part of the code. However, I also hate having large numbers of commits (frequently can commit 50+ times a day without much substantitve progress). To combine these, usually I will avoid squashing commits in a branch until right before I merge it. This way you can review everything which has been done until you're finished with that branch, but also avoid having a large number of trivial commits. In this case, only after you've been given MRG +2 would you squash the PR. That would have a negative side effect of preventing the second reviewer from quickly merging the branch, though.
What are your thoughts on that, Joel?
On Mon, Jun 13, 2016 at 6:36 PM, Joel Nothman <joel.nothman@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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
-- Information System Engineer, Ph.D. Blog: http://blog.audio-tk.com/ LinkedIn: http://www.linkedin.com/in/matthieubrucher
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
Sounds good to me. Thank goodness someone reads the documentation! On 14 June 2016 at 19:51, Alexandre Gramfort < alexandre.gramfort@telecom-paristech.fr> wrote:
We could stop squashing during development, and use the new Squash-and-Merge button on GitHub. What do you think?
+1
the reason I see for squashing during dev is to avoid killing the browser when reviewing. It really rarely happens though.
A _______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
I'm +1 for using the button when appropriate. I think it should be up to the merging person to make a call whether a squash is a better logical unit than all the commits. I would set like a soft limit at ~5 commits or something. If your PR has more than 5 separate big logical units, it's probably too big. The button is enabled in the settings but I can't see it. Am I being stupid? On 06/14/2016 06:58 AM, Joel Nothman wrote:
Sounds good to me. Thank goodness someone reads the documentation!
On 14 June 2016 at 19:51, Alexandre Gramfort <alexandre.gramfort@telecom-paristech.fr <mailto:alexandre.gramfort@telecom-paristech.fr>> wrote:
> We could stop squashing during development, and use the new Squash-and-Merge > button on GitHub. > What do you think?
+1
the reason I see for squashing during dev is to avoid killing the browser when reviewing. It really rarely happens though.
A _______________________________________________ scikit-learn mailing list scikit-learn@python.org <mailto:scikit-learn@python.org> https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
@Andreas It's a bit hidden: You need to click on "Merge pull-request", then do *not* click on "Confirm merge", but on the small arrow to the right, and select "Squash and merge". 2016-06-14 18:13 GMT+02:00 Andreas Mueller <t3kcit@gmail.com>:
I'm +1 for using the button when appropriate. I think it should be up to the merging person to make a call whether a squash is a better logical unit than all the commits. I would set like a soft limit at ~5 commits or something. If your PR has more than 5 separate big logical units, it's probably too big.
The button is enabled in the settings but I can't see it. Am I being stupid?
On 06/14/2016 06:58 AM, Joel Nothman wrote:
Sounds good to me. Thank goodness someone reads the documentation!
On 14 June 2016 at 19:51, Alexandre Gramfort < alexandre.gramfort@telecom-paristech.fr> wrote:
We could stop squashing during development, and use the new Squash-and-Merge button on GitHub. What do you think?
+1
the reason I see for squashing during dev is to avoid killing the browser when reviewing. It really rarely happens though.
A _______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing listscikit-learn@python.orghttps://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
IMHO the squash and merge should not be used when there are commits from 2 or more different authors to avoid crediting only a single author. On Tue, Jun 14, 2016 at 6:40 PM, Tom DLT <tom.duprelatour@orange.fr> wrote:
@Andreas It's a bit hidden: You need to click on "Merge pull-request", then do *not* click on "Confirm merge", but on the small arrow to the right, and select "Squash and merge".
2016-06-14 18:13 GMT+02:00 Andreas Mueller <t3kcit@gmail.com>:
I'm +1 for using the button when appropriate. I think it should be up to the merging person to make a call whether a squash is a better logical unit than all the commits. I would set like a soft limit at ~5 commits or something. If your PR has more than 5 separate big logical units, it's probably too big.
The button is enabled in the settings but I can't see it. Am I being stupid?
On 06/14/2016 06:58 AM, Joel Nothman wrote:
Sounds good to me. Thank goodness someone reads the documentation!
On 14 June 2016 at 19:51, Alexandre Gramfort < alexandre.gramfort@telecom-paristech.fr> wrote:
We could stop squashing during development, and use the new Squash-and-Merge button on GitHub. What do you think?
+1
the reason I see for squashing during dev is to avoid killing the browser when reviewing. It really rarely happens though.
A _______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing listscikit-learn@python.orghttps://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
Yes, I tried to work that out regarding @afouchet and @tguillemot's work, but I may have failed to. However, both are credited in what's new, etc... And commit counts really say little. On 19 June 2016 at 09:18, Raghav R V <ragvrv@gmail.com> wrote:
IMHO the squash and merge should not be used when there are commits from 2 or more different authors to avoid crediting only a single author.
On Tue, Jun 14, 2016 at 6:40 PM, Tom DLT <tom.duprelatour@orange.fr> wrote:
@Andreas It's a bit hidden: You need to click on "Merge pull-request", then do *not* click on "Confirm merge", but on the small arrow to the right, and select "Squash and merge".
2016-06-14 18:13 GMT+02:00 Andreas Mueller <t3kcit@gmail.com>:
I'm +1 for using the button when appropriate. I think it should be up to the merging person to make a call whether a squash is a better logical unit than all the commits. I would set like a soft limit at ~5 commits or something. If your PR has more than 5 separate big logical units, it's probably too big.
The button is enabled in the settings but I can't see it. Am I being stupid?
On 06/14/2016 06:58 AM, Joel Nothman wrote:
Sounds good to me. Thank goodness someone reads the documentation!
On 14 June 2016 at 19:51, Alexandre Gramfort < alexandre.gramfort@telecom-paristech.fr> wrote:
We could stop squashing during development, and use the new Squash-and-Merge button on GitHub. What do you think?
+1
the reason I see for squashing during dev is to avoid killing the browser when reviewing. It really rarely happens though.
A _______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing listscikit-learn@python.orghttps://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
Oh wow, that looks like a neat feature, didn’t know about this, thanks for sharing! (And I would be in favor of this)
On Jun 14, 2016, at 5:34 AM, Tom DLT <tom.duprelatour@orange.fr> wrote:
We could stop squashing during development, and use the new Squash-and-Merge button on GitHub. What do you think? Tom
2016-06-14 8:06 GMT+02:00 Matthieu Brucher <matthieu.brucher@gmail.com>: I don't even think that squashing them before the merge is actually sound. You will still need the history of why something happened several years down the road (and rebasing actually has a similar issue). This bit me quite often (having just one big commit to analyze after a merge from ancient VCS). Git was created to keep the history when merging, why would we explicitly remove knowledge? Just my 2 cents.
2016-06-14 4:40 GMT+02:00 Jacob Schreiber <jmschreiber91@gmail.com>: My research work involves frequently contributing small changes. I like to keep these around as a record of what I've done, until I've finished with that part of the code. However, I also hate having large numbers of commits (frequently can commit 50+ times a day without much substantitve progress). To combine these, usually I will avoid squashing commits in a branch until right before I merge it. This way you can review everything which has been done until you're finished with that branch, but also avoid having a large number of trivial commits. In this case, only after you've been given MRG +2 would you squash the PR. That would have a negative side effect of preventing the second reviewer from quickly merging the branch, though.
What are your thoughts on that, Joel?
On Mon, Jun 13, 2016 at 6:36 PM, Joel Nothman <joel.nothman@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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
-- Information System Engineer, Ph.D. Blog: http://blog.audio-tk.com/ LinkedIn: http://www.linkedin.com/in/matthieubrucher
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
I agree that it adds some annoying overhead. For me, one of the main motivations is to make cherry picks for bugfix releases easier. It's very hard to cherry pick things that are spread out over many commits, and it's hard to find the relevant bug fixes among hundreds of minor commits. This really mostly impacts me an Olivier and maybe Gael (not sure if you did that at some point, too). It is somewhat related to our practice of merging by rebase, which I think we mostly stopped. When merging by rebase, you don't have merge commits, so finding out which commit belongs to which changeset is hard. I think the rebasing is actually not a great idea for that reason (and also it breaks the nice links from commits to PRs which github provides these days). If we do standard merges and don't squash, I guess the cherry picking is still possible, though somewhat harder. It's not that common a use-case, though, and I guess we can remove the hassle of the rebase if it's too much of a nuisance. On 06/13/2016 09:36 PM, Joel Nothman 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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
On 06/13/2016 09:36 PM, Joel Nothman wrote:
(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.) I really see no reason why someone should squash something before it is ready to be merged. (as Jacob suggested).
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@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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
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@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@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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
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@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@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@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@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
_______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
participants (10)
-
Alexandre Gramfort -
Andreas Mueller -
Andy
-
Jacob Schreiber -
Joel Nothman -
Juan Nunez-Iglesias -
Matthieu Brucher -
Raghav R V -
Sebastian Raschka -
Tom DLT