[webkit-dev] WebKit Transition to Git

[Michael Catanzaro]

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.)

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.

> 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.

> I disagree. Detailed descriptions and function-level change logs are
> exactly what I use to dig up all the code history and figure out
> what's causing the bug and how to fix in numerous occasions. Not
> having that would be a massive regression.
> 
> - R. Niwa

Detailed descriptions are very important. I don't think function-level 
changelogs are; documenting changes in individual functions is 
generally busywork to say what you can plainly see by just looking at 
the diff. I'll submit 
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1686/commits as an 
example of my preferred commit granularity and verbosity. (We can 
pretend it is not currently failing CI. ;) Here the commits are 
lightly-related and could perhaps be split into multiple MRs, but 
honestly that becomes annoying and unwieldy, especially as some of the 
commits depend on each other and need to land in a particular order, 
and since creating new branches and MRs (or bugs on Bugzilla) for each 
commit becomes annoying. So it's nicer to do it all in one. One of 
these commits actually makes changes that are undone by a subsequent 
commit, and is primarily there just so that I could write a commit 
message about it and show the history in two steps rather than one! 
History like that would be lost in Konstantin's Workflow 1. I don't 
think ChangeLog-style function-level detail would be helpful in any of 
them; in WebKit, I usually ignore that anyway. But all of the commits 
do have a detailed commit message, except for "fix typo" and "fix 
whitespace" (can't expect an essay for those).

Regarding line-by-line commit review... well, it would be nice to have, 
of course.  But I don't think it's as important as you suggest. 
Problems with commit messages are usually general problems with the 
entire commit message rather than problems with a specific line of the 
commit message. An exception is typos. It is harder to point out typos 
without a line-by-line review tool. But still, it's not too hard to 
tell somebody to fix a typo without being able to highlight the right 
line.

Michael