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.
Michael
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
