> "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.
To be clear; I'm talking about *passing all tests*, not building. It should
always build. Most of the comments you've made seem to be in the context of
building.
(I think tidy should be included in "passing all tests" too mostly because
I can't envision a case where we'd ever want to bisect on tidy; and
currently tidy is oversensitive about import order to an extent that makes
it a disproportionate amount of work to retroactively enforce tidy on each
commit.)
> 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.
"Fix unit tests" was a mistake, sorry about that. I meant to fold that in
and forgot. It's the other one ("update test expectations" or whatever I
called it) that I'm talking about. Also the intermediate commits being
incomplete fluctuate between failing a bunch of transform css-tests. Having
that history checked in is not helpful at all. The only option is to squash
the entire pull requests.
Though I ... don't really see your point here. You're advocating squashing
the entire pull request in this thread; that has approximately the same
effect as a separate "fix unit tests" commit vis a vis tracking breaking
changes in an API? With a separate commit you have to go back to the pull
request and look at the full diff or guess which commit introduced the
change. When the entire pull request is a single squashed commit you have
the same problem, except now you have no way of looking at the big commit
aside from all at once.
> 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".
I wasn't saying "all large refactorings", I was saying "many". I've done
those too.
> I just don't want to hear "shrug, I don't have time for this" (which is
something I've already encountered in the past).
To be clear; that's not what I was doing for the transforms PR. I could
have squashed it in a second; however I was of the opinion it shouldn't be
squashed since otherwise there's just too much going on in a single commit.
I'd also been structuring the commits so as to be eventually landable as-is
as far as building and logical structure is concerned. I made exactly that
tradeoff; I felt bisectability within the PR was not worth having a giant
commit that changes everything at once.
> That bisect can work over first-parent doesn't mean you shouldn't strive
to have all commits building.
Again, I'm talking about running tests, not building; and I'd like to see
some rationale here; because as it stands first-parent bisect is the
tradeoff-breaker here, it lets us have working bisects *and* it lets us
have bite-sized commits that may fail tests but do build in our PRs.
I also think that if we care so strongly about tests passing per-commit we
should be testing that (though that's also expensive); because as-is I'm
quite certain that bisect without first-parent would *regularly* break
tests on the way; and that's not just because *I*'ve not been following
that rule. I'm pretty sure nobody runs try on every commit they make; so
I'd expect to see lots of test-broken commits in our history; enough to
break the ability to bisect over all tests.
Thanks,
-Manish Goregaokar
On Fri, Nov 3, 2017 at 6:58 AM, Anthony Ramine <aram...@mozilla.com> wrote:
> 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
>
_______________________________________________
dev-servo mailing list
dev-servo@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-servo
