[webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 3:19 PM Michael Catanzaro <mcatanzaro at gnome.org> wrote:
>
> I suppose what I'm describing is Konstantin's Workflow 2, which is
> overwhelmingly popular.
>
> On Tue, Oct 13, 2020 at 2:19 pm, Ryosuke Niwa <rniwa at webkit.org> wrote:
> > Not squashing only helps if each commit can stand on its own. At that
> > point, I'd suggest such a sequence of commits be made into multiple
> > PRs instead of multiple commits in a single PR, each of which requires
> > a separate code review.
>
> Commits are certainly expected to stand on their own (not introduce
> defects). If they can't, then they should be combined into another
> commit! Hence, we should not approve MRs if the MR contains commits
> that fail to meet our usual standards. Such commits should just fail
> code review. (But if you aren't willing to review MRs commit-by-commit,
> then indeed you'll never notice when such problems exist.)

Right, so this will be a problem unless we're gonna review each commit
separately. But if we're doing that, we might as well as create a
separate PR for each commit.

If we have 5-10 separate commits each of which make substantial
changes, we don't all of them to be landed / merged all at once into
the main branch even if they're logically related. Without
intermediate test runs & perf test results, we wouldn't be able to
pin-point what caused it, in which case, we'd be likely forced to
revert all of them anyway.

Furthermore, reverting a part of a sequence of commits coming from a
single PR would be very confusing just reverting just one of many
patches posted on Bugzilla would be confusing.

For all these reasons, I don't see much benefit in allowing multiple
commits from being merged from PR.

> If I have to open a separate MR for each change, though, I'm going to
> wind up combining multiple lightly-related changes into one big commit,
> because a new MR for every tiny cleanup I want to make requires effort.
> Such commits may be common in WebKit, but they would fail code review
> in most other open source projects. E.g. in
> https://trac.webkit.org/changeset/268394/webkit I snuck in a drive-by
> one-line fix that's unrelated to the rest of the commit. I would rarely
> dare to do that outside WebKit, knowing it would probably fail review,
> but we do it commonly in WebKit because we discourage multiple patches
> per bug and don't want to create new bugs for every small cleanup.

That seems totally okay. I land one line fix like that all the time though.

> > This to me is a show stopper. When I'm trying to bisect an issue,
> > etc..., the biggest obstacle I face is any intermediate revisions
> > where builds are broken or otherwise non-functional. I don't think we
> > should let anyone merge a commit into the main branch unless the
> > commit meets the same standards as a regular Bugzilla patch we land
> > today.
>
> I agree. But I would say that a MR with such history should fail
> review, and be rewritten to not suffer from these problems.

The problem is that you're presuming that all WebKit reviewers would
be able to and would be willing to do that review. I certainly do not
feel comfortable having to do that given how troublesome it has been
in WPT land.

- R. Niwa