[webkit-dev] WebKit Transition to Git
mcatanzaro at gnome.org
Tue Oct 13 15:18:58 PDT 2020
I suppose what I'm describing is Konstantin's Workflow 2, which is
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
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
More information about the webkit-dev