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 <[email protected]> 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 _______________________________________________ webkit-dev mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-dev
