On Tue, Oct 13, 2020 at 12:32 pm, Maciej Stachowiak <m...@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.


webkit-dev mailing list

Reply via email to