Let me add some necessary additions to this quite rude email... I am sorry, Manish.
> Le 3 nov. 2017 à 10:33, Anthony Ramine <n.ox...@gmail.com> a écrit : > > >> 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. But past discussions were on IRC, so you probably can't find them anymore, teehee! This is a debate that comes back from times to times on the channel, mostly when Emilio or I or someone else see a PR with commits that should be squashed. > "Fix unit tests" is not a commit I want to see in master *ever*. In addition to commits prior to such changes being probably broken, such commits ("fix unit tests") also mean you sometimes lose a good way to track breaking changes in an API in pull requests with a lot of commits. >> 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. "Always been the case" = I remember back in 2015 that reviewers often asked me whether intermediate commits were building properly. Maybe that was just out of curiosity, but to me it sounded like something we should strive for. >> 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. And by wrong I mean "I respectfully disagree, here are a list of pull requests I wrote that didn't need to be a single commit". > 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 Regards, I wish you all the baguettes. _______________________________________________ dev-servo mailing list dev-servo@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-servo
