Hi Manish, On 11/03/2017 01:00 AM, Manish Goregaokar wrote: > 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.
First of all thanks for landing that PR btw, it's super nice to have that property outside of the mako system finally! \o/ > When we land pull requests in servo/servo we like individual commits to > build so that git bisect works. For that same reason, you probably also want the code to not be broken. > 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 FWIW, this was added in https://github.com/servo/servo/pull/8377. > 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. > - 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 > - This basically forces large refactorings to be a single commit I don't think tiny but broken (in the "code is not correct" sense) commits help more, necessarily. More to the point, your argument is based on the premise that large refactorings cannot be done in smaller atomic patches (or in different PRs, or what not), and I think that premise is and can be proven false (happy to look for examples if you want me to, a few come to mind). We can discuss whether keeping that policy is useful or not, given you can indeed bisect just based on merges. I think making each commit atomic is better than not, and helps getting a consistent view of the history of the repo (plus, you may want to bisect within a single PR). In general I think we should keep that policy. Of course we have (at least right now) no practical way to enforce it automatically, but keeping it as a rule of thumb at least seems useful to me. I also understand that, in the case you present (having done a big refactoring and not having split it from the beginning), it takes effort to split in multiple atomic commits after the fact (though that would of course have been ideal, also / specially from the reviewers' point of view :P). That's why instead of telling you to do that I suggested to squash. In any case, I do think landing a single squashed commit (with a good commit message) that passes all tests is way more preferable than multiple WIP commits that are dependent on each other to be correct in the end. -- Emilio _______________________________________________ dev-servo mailing list dev-servo@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-servo
