> Le 3 nov. 2017 à 01:00, Manish Goregaokar <manishsm...@gmail.com> a écrit : > > So I and emilio were discussing whether or not to squash > https://github.com/servo/servo/pull/18750 and it seemed like we have > different ideas of how "atomic" commits should be before landing.
It should definitely have been squashed, this is not the first time such a thing is discussed. "Fix unit tests" is not a commit I want to see in master *ever*. > When we land pull requests in servo/servo we like individual commits to > build so that git bisect works. Yes. > Do we require them to pass all tests as well? If not, should we? I don't > *think* we do, I'm pretty sure we didn't back in 2014, but emilio pointed > out we ask for this in CONTRIBUTING.md and I'm confused now Yes we do want them to pass all tests, this has always been the case. > In my opinion we should not require tests to pass on individual commits > (just the PR), for a variety of reasons: > > - git blame is used a lot more than git bisect IME . It is very helpful > to have as-tiny-as-possible commits for git blame. "Fix unit tests" is pretty damn bad for git-blame. > - I don't think we ever bisect with the entire testsuite ; it's usually > just a couple tests > - you *can* bisect over first-parent (it's tricky, but there are > commands out there you can copy) making this whole thing moot. However when > squashing a large pull request you're effectively losing all that > information, and that's not something you can work around That bisect can work over first-parent doesn't mean you shouldn't strive to have all commits building. > - This basically forces large refactorings to be a single commit Wrong. https://github.com/servo/servo/pull/18635 https://github.com/servo/servo/pull/18600 https://github.com/servo/servo/pull/18533 https://github.com/servo/servo/pull/18480 _______________________________________________ dev-servo mailing list dev-servo@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-servo
