[webkit-dev] WebKit Transition to Git

[Michael Catanzaro]

On Tue, Oct 13, 2020 at 12:32 pm, Maciej Stachowiak <mjs at apple.com> 
wrote:
> I’m assuming your objection is to regular merges, but how do you 
> feel about squash merges? Or do you think all PRs should be landed by 
> rebasing?

If we want a linear history (we do), all PRs ultimately have to land as 
fast-forward merges, one way or the other. I think rebasing is the 
simplest way to accomplish that, but squashes work too if all your 
changes are sufficiently self-contained to land as one commit.

I suggest leaving this up to the discretion of the developer and 
reviewer rather than mandating one way or the other, because there are 
advantages and disadvantages to both approaches. If your local commit 
history is a mess, sometimes squashing it all into one commit is an 
easy way to make it acceptable for upstream. If you hate rewriting 
history to make it look good, and your MR isn't too huge, a squash 
might be appropriate. But more often than not, I'd say that would 
result in a worse commit history.

> My own preference would be to require squash merge, because it keeps 
> the history simple for the main branch, but does not risk putting 
> intermediate revisions which may work or even build on the main 
> branch.

The disadvantage is that if you squash before merging, you encourage 
fewer, bigger commits that might be harder to read and potentially much 
less fun to discover at the end of a bisect. E.g. consider a very 
common case: developer wants to fix a handful of separate but related 
bugs in a particular section of code. We could land all the fixes in 
one huge commit, but that's inevitably going to be harder to read, 
harder to deal with if it introduces a regression, and will certainly 
have a more confusing commit message (either because it gives less 
detail about the changes, or because it's very long describing multiple 
related changes that could have been separate commits). We could split 
it up into different MRs for the different commits, but that becomes 
annoying when the commits are related, and hard to keep the different 
MRs in order. So I don't normally squash (except when committing small 
fixup commits via the web UI), because the disadvantages generally 
outweigh the benefits.

On the other hand, if you don't squash, it's indeed possible that your 
commit history might be messy, e.g. intermediate commits might break 
tests fixed by subsequent commits, or intermediate commits might not 
build at all. Squashing before merging does eliminate those risks, but 
I recommend code review instead: commit history and style is subject to 
review just like code changes are. Sometimes I see a merge request with 
a messy commit history that just begs for two or more commits to be 
squashed together. (This is common for students new to open source. 
WebKit does not have this problem currently.) Sometimes I see the 
opposite, where too many changes are bundled into one big commit. (This 
is more common in WebKit, since we normally try to have one patch per 
bug, and splitting changes into multiple bugs is inconvenient.) But 
usually the developer submitting a MR has found a good balance between 
the two extremes. Ultimately, what's most important is landing a clean 
commit history with reviewable commits. Again, the scope of commits is 
subject to review just like code changes are.

I agree that we don't want to merge WIP commits of any form, and 
reviewers should complain if these appear in a MR. E.g. if making an 
API change that requires updates in 200 files, we surely want them all 
updated in one commit, because we don't want broken intermediate 
commits on master that would break bisections. So sometimes huge 
commits are unavoidable. Similarly, if an intermediate commit is known 
to break a test, that's not good either because it could mess up 
bisects. (I don't think we necessarily need to run tests on every 
intermediate commit -- that might be too much for our infrastructure -- 
but we could if desired.) What we don't want is to wind up merging 
low-quality stream-of-consciousness style commits that looks like they 
were committed in the order the code was written. I think that's what 
you're trying to avoid by suggesting squashes. Instead, I suggest 
developers should aggressively use 'git add -p' and 'git rebase -i' to 
selectively commit, rewrite, and reorder history to look good before 
opening the MR. This isn't optional for most open source projects: if 
you propose an MR with ugly commit history, it won't be merged until 
fixed. For a typical MR of moderate complexity, I'll use 'git rebase 
-i' at least a couple times before the history is clean enough for a 
MR, and to make fixup commits disappear into the most-appropriate 
commit in the sequence.

Regarding commit messages, I don't understand why there's any 
expectation that they need to be checked into files in order to be 
detailed and complete. If a commit message doesn't adequately describe 
what it's fixing, then the reviewer should open a review thread to 
complain and request more detailed commit messages. If a change is very 
straightforward and obvious, sometimes a quick sentence might suffice, 
but usually a good commit message should be at least a paragraph or 
two, and often more. Function-level ChangeLog-style details are hard to 
read and rarely helpful.

Michael