On Thu, Nov 2, 2017 at 5:00 PM, Manish Goregaokar <manishsm...@gmail.com> 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. > > When we land pull requests in servo/servo we like individual commits to > build so that git bisect works. > > 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 > > > 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 > > > Thoughts? The Firefox repo has the same dilemma. Ideally history is good down to the commit level. In reality, we only test on "push heads" (commits that were heads at time of their push) so this is the only thing we try to claim. Adding more reality, we have tons of bad push heads and even this guarantee is weak. (There's a plan to employ the forced use of autoland combined with history rewriting to tackle this. Bug 1266863 tracks.) If you squash commits, you potentially lose a lot of valuable data. Depends if the squashed-over commits are valuable or not. I agree in not wanting to lose this important history just to maintain commit-level bisectability. The lack of tools being able to bisect over arbitrary commits is a tooling problem IMO. `hg bisect --skip <revset>` solves this nicely in Mercurial land. You can get something similar with Git but Git's commit selection facilities aren't as powerful, so it is a bit harder to use. Improving tooling is a solvable problem and I don't think should be used as justification to throw away useful commit-level data! FWIW, Mercurial 4.4 supports revsets that read revisions from files or command output. So you can maintain a list of "known good" or "known bad" bisectable commits in the repo and feed that into `hg bisect --skip`. At repos as large as Servo and Firefox, commit-level bisection would require a lot of testing resources. Maybe you can do lightweight tests on every commit. But all the tests is almost certainly prohibitively expensive. Instead of imposing a hard or fast rule, mine would be phrased as a goal. "Attempt to write commits that are all "good." But this property can be a) intentionally violated if squashing commits would result in loss of information important to archeology or b) unintentionally violated because not all commits are actually tested." Or something along those lines. _______________________________________________ dev-servo mailing list dev-servo@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-servo
