So, I've done some more experimentation with Gerrit's various merge strategies.
First a reminder that a Gerrit review is always of a single commit, not a branch. This massively limits our ability to share branches before we even consider the Gerrit merge strategy, because it forces us to squash changes down to single commits. If we truly want to "solve" that problem, we'll need a much more extensive change to working method than just the Gerrit merge strategy. Also, any method that solved that problem and still had meaningful code reviews would involve reviewers needing to individually review every commit from the developer's branch. In all honesty, it might be easier to use a completely different review tool than Gerrit at that point. Anyway, at the risk of drowing this conversation completely... there are five merge strategies available in Gerrit; here are what I see as their pros, cons, and gotchas. 1. *Cherry-pick*. This is what we use today: the commit under review is cherry-picked onto master, resulting in a new commit. Pros: - Introduces the "Reviewed-on" header in the commit on master, which allows linking back to the review page. - Ensure linear master commit history, since there are no merge commits. Cons: - Cherry-picks are always new commits (although it seems that rebase and merge can handle that OK - the real damage has already been done by the squashing before it even gets to Gerrit) 2. *Always merge*. This means that the commit under review will be put into the repository, and then a merge commit will be created (by Gerrit itself) with the current master tip as one parent and the new commit as another. Basically the same as using "git merge --no-ff" locally. Pros: - Original commit maintains SHA, etc. - Original commit does not necessarily have to be rebased to master tip (although if it isn't there can be merge conflicts) Cons: - Master history can be a bit messy (although "git log --first-parent" works reasonably well) - The commit message Gerrit creates is always: ' *Merge "first line of commit message"* ' which means the master commit history loses a ton of information. 3. *Fast-forward only*. This means that the commit under review must have the current master tip as its parent. When the commit is submitted, Gerrit will simply fast-forward the master branch to the new commit. Basically the same as using "git merge --ff-only" locally. Pros: - Original commit maintains SHA, etc. - Master log history is kept very clear. - No possibility of merge conflicts at Gerrit time. Cons: - Since you must rebase to master before submitting, you potentially lose the ability to share your working branch with others more often than is strictly necessary. - Also since you must rebase to master before submitting, you will potentially have more submissions that need to be rebased and reproposed than is strictly necessary. 4. *Merge if necessary*. This is basically the same as the default behaviour of "git merge": fast-forward if possible (ie, the commit under review's parent is the current master tip), otherwise create a merge commit. Pros: - Good compromise between "keeping local branch commits shareable" and "don't force unnecessary rebases" - possibly the least developer-intrusive option. Cons: - Master history is inconsistent, with a mix of "real" commits and merge commits. - Merge commits still have the extremely abbreviated log message, which is IMHO even worse here since some commits will have full messages and some won't. 5. *Rebase if necessary*. This will fast-forward master if the commit under review has the current master tip as its parent. If not, Gerrit itself will attempt to rebase the commit onto master, and then fast-forward it onto master. This may result in conflicts at Submit time. Pros: - Less developer interaction required than "fast-forward only". - Master log history is kept very clear. Cons: - Commits on master will sometimes be new commits, basically cherry-picks (without the advantages of cherry-pick) Anyway, I'm not sure any of this information is useful. There isn't a merge strategy which is clearly better or worse than any other, and the problem of wanting more flexibility in development branches is only tangentially related to that anyway. But, here's the information in case anyone wants to think about it any more. I'm going to do a little bit more exploration with "git gerrit" to see whether I could change anything there that would make a difference. Ceej aka Chris Hillery On Thu, Sep 24, 2015 at 5:52 PM, Chris Hillery <[email protected]> wrote: > On Thu, Sep 24, 2015 at 11:05 AM, Jianfeng Jia <[email protected]> > wrote: > >> If we can somehow enforce the rebase before merge, then I think it won’t >> have the merge “tree” problem. >> > > Actually the merge-tree problem was introduced in a situation where a part > of a branch was cherry-picked to master. It would also happen if part of a > branch were cherry-picked over to another branch (like you wanted to do > with Taewoo's branch) and then both were eventually merged to master. In a > normal scenario this wouldn't happen, with or without rebasing the feature > branch before merge. > > As for whether "rebase before merge" should be enforced, I'm in favor of > it personally. I'm even in favor of doing local interactive rebases with > selective squashing and so forth to "clean up" local branch history before > merging - although as always, this is destructively rewriting history and > therefore should not be done if the branch was shared with anyone else. > > Ceej > aka Chris Hillery >
