asterixdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris \"Ceej\" Hillery" <c...@couchbase.com>
Subject Merging vs. squashing
Date Fri, 18 Sep 2015 08:04:33 GMT
I'm pulling this conversation into a separate thread from the "Commit
message proposal".

There are certainly long-running and contentious arguments about whether
commits to master should be merges or squashes/cherry-picks. That ranks
right up there with vi vs. emacs. Before going too far into that, however,
you should be aware of some additional quirks/limitations that Gerrit
brings into the story.

First, we have Gerrit configured to always cherry-pick changes when
submitting them. We do this for a couple of reasons, but the main one is
that Gerrit gives you a little additional value in this case: It introduces
Reviewed-on: and Reviewed-by: headers into the commit, which provide you
with a link back to the Gerrit review. For instance,
https://github.com/apache/incubator-asterixdb-hyracks/commit/d885779b13c4fd9bd551d306a460df2894fb18cb
- see that there is a hyperlink there back to Gerrit. This is not possible
if you use any merge strategy other than cherry-pick, because only
cherry-picking creates a *new* commit object that Gerrit can decorate.

Moving on - In Gerrit, it is not possible to review a "branch merge", at
least not in the way you would hope. A review in Gerrit is always of a
*single commit*. If you make a number of commits on your local working
branch and then push that branch to Gerrit's refs/for/master, it will
create one review for each commit. This is occasionally desirable for
larger changes that can reasonably be broken up, but as a default working
method it is usually more trouble than it's worth - especially because
Gerrit won't prevent you from Submitting a subset of the changes or merging
them out of order. This is the main reason that I implemented git-gerrit to
do a squash before uploading a change to Gerrit.

What if you make changes on a branch, then merge them to your local master
and then propose just the merge commit to Gerrit? That's just one commit
(the merge commit) and therefore one review, right? Well, no - that would
only work if all of your branch commits were already known to Gerrit, say
because they were submitted (one at a time) to a non-master branch.
Otherwise, you'll get N+1 separate reviews on Gerrit, and the merge commit
review won't even make sense anymore.

It *is* possible to do this "right" with Gerrit if you want to get merge
commits onto the master branch. You need to create a secondary branch,
"unstable" or "testing" or similar, and propose your individual changes
there. We could even create multiple of those branches for individual
feature work if we wanted. Then once that branch is in a state that you
want to merge to master, you can propose a single merge commit change to
Gerrit. The biggest downside to this is that when reviewing a merge commit,
Gerrit does NOT show you any information about the changes being introduced
into the target branch. It only shows you any diffs which were introduced
during conflict resolution of the merge. Also, merge commits cannot be
cherry-picked, so you lose the Reviewed-on: header for the merge itself.
These aren't necessarily fatal limitations - we have a couple teams in
Couchbase using exactly this methodology with success.

Anyway. The upshot is, handling git history is a mess, and Gerrit makes it
messier. The "always squash branches, always cherry-pick changes" approach
that we currently use for AsterixDB seems to me to be the least available
evil, and it at least results in a clean if abbreviated commit history on
master. It definitely has its downsides as well, though - Jianfeng's
experience is one such problem, and it doesn't have a clean solution.

Ceej
aka Chris Hillery

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message