How to review a merge commit

jekyll 23 comments suggest edit

Git does a pretty amazing job when it merges one branch into another. Most of the time, it merges without conflict. In a fairy tale world with rainbow skittles and peanut butter butterflies, every merge would be without conflict. But we live in the real world where it rains a lot and where merge conflicts are an inevitable fact of life.

Is this what an Octopus merge looks like? - Dhaka traffic by Ranveig Thatta license CC BY 2.0

Git can only do so much to resolve conflicts. If two developers change the same line of code in different ways, someone has to figure out what the end result should be. That can’t be done automatically.

The impact of merge conflicts can be mitigated by doing work in small iterations and merging often. But even so, the occasional long running branch and gnarly merge conflict are unavoidable.

Often, we treat the work to resolve a merge conflict as trivial. Worse, merges often are not reviewed very carefully (I’ll explain why later). A major merge conflict may contain a significant amount of work to resolve it. And any time there is significant work, others should probably review that code in a pull request (PR for short). After all, a bad merge conflict resolution could introduce or reintroduce subtle bugs that were presumed to be fixed already.

As a great example, take a look at the recent Apple Goto Fail bug. I’m not suggesting this was the result of a bad merge conflict resolution, but you could easily see how a bad merge conflict might produce such a bug and bypass careful code review.

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, @signedParams)) != 0)
    goto fail;
    goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

When my team is in this situation, we actually push the merge commit into its own branch and send a pull request.

For example, suppose I want to merge the master branch into a branch named long-running-branch. I’ll create a new branch named something like merge-master-into-long-running-branch. I’ll then perform the merge in that branch (when I am resolving a gnarly merge my outbursts can be rightly described as a performance). When I’m done and everything is working, I’ll push that new branch to GitHub and create a pull request from it for others to review.

In git that looks like:

git checkout long-running-branch
git checkout -b merge-master-into-long-running-branch
git merge master
# Manually do a lot of work to resolve the conflicts and commit those changes
git push origin merge-master-into-long-running-branch

The first command just makes sure I’m in the long-running-branch. The second command uses the -b to create a new branch named merge-master-into-long-running-branch based off the current one. I then merge master into this branch. And finally I push it to GitHub.

That way, someone can do a quick review to make sure the merge doesn’t break anything and merge it in.

However, this runs into some problems as articulated by my quotable co-worker Paul Betts. In a recent merge commit PR that I sent, he made the following comment just before he merged my PR.

I have no idea how to review a merge commit

The problem he alludes to is that when you merge one branch into another, the diff of that merge commit will show every change since the last merge. For the most part, that’s all code that’s already been reviewed and doesn’t need to be reviewed again.

What you really want to look at is whether there were conflicts and what shenanigans did the person have to do to resolve those conflicts.

Well my hero Russell Belfer (no blog, but he’s @arrbee on Twitter) to the rescue! He works on LibGit2 so as you’d expect, he knows a thing or two about how Git works.

Recall that when you merge one branch into another, a new merge commit is created that points to both branches. In fact, a merge commit may have two or more parents as it’s possible to merge multiple branches into one at the same time. But in most cases a merge commit has exactly two parents.

Let’s look at an example of such a merge commit from the SignalR project. This commit merges their release branch into their dev branch. The SHA for this commit is cc5b002a5140e2d60184de42554a8737981c846c which is pretty easy to remember but to be fair to those with drug addled brains, I’ll use cc5b002a as a shorthand to reference this commit.

You can use git diff to look at either side of the merge. For example:

git diff cc5b002a^1 cc5b002a
git diff cc5b002a^2 cc5b002a

Recall that the ^ caret is used to denote which parent of a commit we want to look at. So ^1 is the first parent, ^2 is the second parent, and so on.

So how do we see only the lines that changed as part of the conflict resolution?

git diff-tree --cc cc5b002a

UPDATE: I just now learned from @jspahrsummers that git show cc5b002a works just as well and in my shell gives you the color coded diff. The merge commit generally won’t contain any content except for the conflict resolution.

git show cc5b002a

As I’ll show later, the --cc option is useful for finding interesting commits like this.

You can see the output of the git show command in this gist. Notice how much less there is there compared to the full diff of the merge commit.

The git diff-tree command is a lower level command and if I had to guess, git show builds on top of it.

If we look at the git diff-tree documentation, we can see that the --cc flag is the one that’s interesting to us.

–cc This flag changes the way a merge commit patch is displayed, in a similar way to the -c option. It implies the -c and -p options and further compresses the patch output by omitting uninteresting hunks whose the contents in the parents have only two variants and the merge result picks one of them without modification. When all hunks are uninteresting, the commit itself and the commit log message is not shown, just like in any other “empty diff” case.

Since the --cc option describes itself in terms of the -c option, let’s look at that too.

-c This flag changes the way a merge commit is displayed (which means it is useful only when the command is given one , or --stdin). It shows the differences from each of the parents to the merge result simultaneously instead of showing pairwise diff between a parent and the result one at a time (which is what the -m option does). Furthermore, it lists only files which were modified from all parents.

The -p option mentioned generates a patch output rather than a normal diff output.

If you’re not well versed in Git (and perhaps even if you are) that’s a mouthful to read and a bit hard to fully understand what it’s saying. But the outcome of the flag is simple. This option displays ONLY what is different in this commit from all of the parents of this commit. If there were no conflicts, this would be empty. But if there were conflicts, it shows us what changed in order to resolve the conflicts.

As I mentioned earlier, the work to resolve a merge conflict could itself introduce bugs. This technique provides a handy tool to help focus a code review on those changes and reduce the risk of bugs. Now go review some code!

If you’re wondering how I found this example commit, I ran git log --min-parents=2 -p --cc and looked for a commit with a diff.

That filters the git log with commits that have at least two parents.

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.

Comments

avatar

23 responses

  1. Avatar for Nicolás Bevacqua
    Nicolás Bevacqua February 24th, 2014

    Great article!

    gist show -> git show

  2. Avatar for haacked
    haacked February 24th, 2014

    Freudian slip! Thanks! I'll fix it.

  3. Avatar for hg
    hg February 25th, 2014

    Does anyone know of a Mercurial equivalent to git show --cc? I don't think any of the standard features can do it, but maybe an extension can.

  4. Avatar for Eduardo Cucharro
    Eduardo Cucharro February 25th, 2014

    I've just migrated from tfs to git and it amuse's me every day. Thank you very much for this article!

  5. Avatar for Jeroen Pluimers
    Jeroen Pluimers February 28th, 2014

    I love the article, as Merges scare too many people.

    Two questions though:
    - When do you merge back this separate branch?
    - And how do you cope when at that moment a new merge occurs?

  6. Avatar for haacked
    haacked February 28th, 2014

    > When do you merge back this separate branch?

    I merge it as soon as it's been reviewed. Usually a teammate who signs off on it will merge it. We usually try to prioritize these because they can change a lot.

    > And how do you cope when at that moment a new merge occurs?

    I assume you mean if someone ends up merging another branch into the mainline while this branch is under review? Depends on that other branch. It's often fine, git is great at merging. If it produces conflicts, I'll do another merge into this branch. At this point, this merge branch is like any other branch and the workflow doesn't really change.

  7. Avatar for Max Battcher
    Max Battcher March 3rd, 2014

    Excellent research. Hopefully you've already considered this, but it would be excellent to see a button in GitHub on multi-parent commits to switch to this --cc view. Even better, it sounds like a useful new "collapsed" default, such as the "uninteresting diff" collapses in ordinary commits.

  8. Avatar for adantj
    adantj March 5th, 2014

    I can't seem to get any output from either git show --cc hash or git diff-tree --cc hash , both return just the commit, I see it works when I do it on a commit that does not have a conflict. What I might be doing wrong?

  9. Avatar for haacked
    haacked March 5th, 2014

    No need for --cc. Just git show.

  10. Avatar for adantj
    adantj March 5th, 2014

    on your update it says: git show --cc cc5b002a

  11. Avatar for haacked
    haacked March 5th, 2014

    Whoops! That's wrong. I'll fix it.

  12. Avatar for Joe
    Joe March 6th, 2014

    I'm just curious to know what people think, in general, about command line git vs using a gui. I feel like I have a much better understanding of what is going on with a gui because the graph is visible and I can interact with it much easier.

  13. Avatar for yassershaikh
    yassershaikh March 10th, 2014

    +1 for the pic

  14. Avatar for Christoph DeMaskey
    Christoph DeMaskey July 10th, 2014

    Great article and suggestion to resolve these issues!

  15. Avatar for Brandon
    Brandon December 17th, 2014

    Any chance that GitHub's Commit view could be the equivalent of `git show` instead of showing all the changes of the merged branch? Maybe with a toggle at the top when people want the full view?

    Right now, GitHub's own docs say to merge in order to resolve conflicts, but then that resulting merge commit is pretty useless as far as code review is concerned.

  16. Avatar for Ivan
    Ivan January 26th, 2015

    git config hooks.showrev "git show -C %s; echo; git show %s | grep -q ^Merge: && git diff-tree --root -C --cc \`git show %s | grep ^Merge: | rev | cut -d' ' -f1 | rev\`; echo"

  17. Avatar for HiThere9
    HiThere9 October 29th, 2015

    UPDATE: I just now learned from @jspahrsummers that git show cc5b002a works just as well and in my shell gives you the color coded diff. The merge commit generally won't contain any content except for the conflict resolution.

    git show cc5b002a

    So, the short version is just:

    git show @
  18. Avatar for mcarel
    mcarel December 5th, 2015

    2 years have past. Still not yet fixed ;)

  19. Avatar for haacked
    haacked December 7th, 2015

    LOL! See, if it's not logged as an issue (https://github.com/Haacked/..., I forget about it. :P

    I just fixed it, though it takes a few seconds to regenerate the site. https://github.com/Haacked/...

  20. Avatar for G. Sylvie Davies
    G. Sylvie Davies January 20th, 2017

    -c and --cc only show conflict resolutions that touched the same file on both sides of the merge. Many conflicts are resolved that way, but if the developers happened to use "theirs" or "ours" as the resolution for a particular file (e.g., "let origin/master win for this file"), then -c / --cc won't show those.

    I'm working on a way to detect these. For now this is what I've come up with: http://stackoverflow.com/a/...

  21. Avatar for Ankush Sharma
    Ankush Sharma April 11th, 2017

    Thanks for the article !

  22. Avatar for Abdul Rauf
    Abdul Rauf July 18th, 2018

    Great article. I recently logged a feature request for Sourcetree referring this article https://jira.atlassian.com/browse/SRCTREEWIN-8959 Note that feature request for TortoiseGit was already referring this article https://gitlab.com/tortoisegit/tortoisegit/issues/2469

  23. Avatar for Mariusz
    Mariusz December 5th, 2018

    Thanks. After 4 years it’s still a great article! Shame that many online git hosting sites still don’t show this “combined diff”. I learned about it because GitExtensions showed it to me and I find it super useful.