2

We recently went through some merge issues with git, and while it probably did 'The Right Thing'(TM) it wasn't what I expected. I have reduced an issue to a small public github repository located at https://github.com/geretz/merge-quirk.

quirk.c exists on master. merge-src and merge-dst are both branched from the same version on master, which is the common ancestor for the eventual merge. merge-src adds a comment and some code. merge-dst reformats the code and changes the exiting comments but does not have the added comment or code.

git merge detects and declares a conflict.

git clone https://github.com/geretz/merge-quirk.git
cd merge-quirk
git checkout merge-dst
git merge origin/merge-src
Auto-merging quirk.c
CONFLICT (content): Merge conflict in quirk.c
Automatic merge failed; fix conflicts and then commit the result.

However the thisLineMightDisapper (sic) function call will get lost if a naive/trusting developer chooses the origin/merge-src code block in the conflict flagged quirk.c .

In my mental model, if the thisLineMightDisapper function call exists in both HEAD and origin/src in conflicting states it should exist in both of the conflict blocks, if it doesn't exist in conflicting states it should exist outside of both conflict blocks. Why does it appear only inside the HEAD block ?

<<<<<<< HEAD
    // a few comment lines - change 1
    // that existed - change 2
    // in the common ancestor - change 3
    // that get changed - change 4
    if(1)
    {
        if (f(1, 2))
        {
            if (thisLineMightDisapper(42))
=======
    // a few comment lines
    // that existed 
    // in the common ancestor
    // added this line in merge-src branch
    // that get changed
    if(1) {
            if (f(1, 2))
>>>>>>> origin/merge-src
            {
                t = time(0);
            }
        }
    }

    if (anotherFunction(1,2))
    {
        t = time(0)
        f(0);
    }
}

The files

master/quirk.c

    // a few comment lines
    // that existed 
    // in the common ancestor
    // that get changed
    if(1) {
            if (f(1, 2))
            {
                if (thisLineMightDisapper(42)) {
                    t = time(0);
                }
            }
    }
}

merge-src/quirk.c

    // a few comment lines
    // that existed 
    // in the common ancestor
    // added this line in merge-src branch
    // that get changed
    if(1) {
            if (f(1, 2))
            {
                if (thisLineMightDisapper(42)) {
                    t = time(0);
                }
            }
    }

    if (anotherFunction(1,2))
    {
        t = time(0)
        f(0);
    }
}

merge-dst/quirk.c

    // a few comment lines - change 1
    // that existed - change 2
    // in the common ancestor - change 3
    // that get changed - change 4
    if(1)
    {
        if (f(1, 2))
        {
            if (thisLineMightDisapper(42))
            {
                t = time(0);
            }
        }
    }
}
Chaim Geretz
  • 826
  • 5
  • 23
  • 3
    A merge conflict happens when git can't figure out how to automatically resolve changes. You, the developer, must actually read it and figure out how to resolve. – SLaks Jul 11 '17 at 21:15
  • 1
    I recommend setting `merge.conflictStyle` to `diff3` as well: this shows what Git saw in the original (common base) file, vs the two end-points being merged, i.e., HEAD / `--ours` and other / `--theirs`. Note that if you are looking at the conflict now, and have not resolved it yet, you can also do `git checkout --conflict=diff3 quirk.c` to replace its contents with a new conflicted-merge, but this time with the chosen style. – torek Jul 11 '17 at 21:37
  • 1
    @torek diff3 dosn't resolve the underlying problem. thisLineMightDisapper call is missing from the diff3 common ancestors block as well, only appears in the HEAD block. – Chaim Geretz Jul 11 '17 at 21:53
  • Well, see Dietrich Epp's answer; but the above is a comment, not an answer, because I find it much easier to reason about what Git saw (and thus did) when I can see the original base as well. Git only knows to match bases to both tips, and combine what it sees as changes. I will also note here that even if *Git* thought it resolved two diffs correctly, that's no guarantee that it *did*. – torek Jul 11 '17 at 21:59
  • @torek Thanks for pointing me in the right direction (git matching base to both tips). since merge-dst reformatted there was an addition as well as a deletion of thisLineMightDisapper code in two distinct hunks. The deletion evidently didn't conflict so that part of the merge 'worked', the addition conflicted with a different hunk that didn't contain that line, so the function only appeared in one conflict block. – Chaim Geretz Jul 11 '17 at 22:12
  • @torek If you want to write this as an answer I probably will accept. – Chaim Geretz Jul 11 '17 at 22:13
  • I don't have time at the moment, but I think jthill's answer is good. – torek Jul 11 '17 at 22:48

1 Answers1

3

What's happening here is you're confusing semantic with textual identity. Git sees the results of shifting and splitting as entirely different lines. You wind up with a conflicting hunk and an unconflicting hunk, and a line added in the conflicting hunk that's (very) confusingly similar to a line deleted in the unconflicting one: even though they're semantically identical,

                            if (thisLineMightDisapper(42)) {

and

                    if (thisLineMightDisapper(42))
                    {

are not the same, and git would ordinarily be very wrong to act as if they are. The second one was untouched in master and merge-src and deleted in merge-dst, separated from the conflicting hunk with the addition by the introduction of a spurious match. So git regarded the deletion as unconflicted and automerged it.

When, as here, even git checkout -m --conflict diff3 is confusing, you can pull the hunks merge is looking at with

$ sh -xc 'git diff ...MERGE_HEAD; git diff MERGE_HEAD...'
+ git diff ...MERGE_HEAD
diff --git a/quirk.c b/quirk.c
index c149623..79dc4a2 100644
--- a/quirk.c
+++ b/quirk.c
@@ -2,6 +2,7 @@
    // a few comment lines
    // that existed 
    // in the common ancestor
+   // added this line in merge-src branch
    // that get changed
    if(1) {
            if (f(1, 2))
@@ -11,4 +12,10 @@
                }
            }
    }
+
+   if (anotherFunction(1,2))
+   {
+       t = time(0)
+       f(0);
+   }
 }
+ git diff MERGE_HEAD...
diff --git a/quirk.c b/quirk.c
index c149623..0de7516 100644
--- a/quirk.c
+++ b/quirk.c
@@ -1,14 +1,16 @@

-   // a few comment lines
-   // that existed 
-   // in the common ancestor
-   // that get changed
-   if(1) {
-           if (f(1, 2))
+   // a few comment lines - change 1
+   // that existed - change 2
+   // in the common ancestor - change 3
+   // that get changed - change 4
+   if(1)
+   {
+       if (f(1, 2))
+       {
+           if (thisLineMightDisapper(42))
            {
-               if (thisLineMightDisapper(42)) {
-                   t = time(0);
-               }
+               t = time(0);
            }
+       }
    }
 }
+ git diff ...MERGE_HEAD

and a bit of study will show you git identified a lone brace as common content, causing it to treat

-               if (thisLineMightDisapper(42)) {
-                   t = time(0);
-               }
+               t = time(0);

as an unconflicted change.

One thing that works for me here is

git merge --abort
git merge -Xignore-space-change origin/merge-src

which properly ignores the reformat when identifying change boundaries. As with hammering anything together, hammering source changes together sometimes requires exactly the right tool, and if you try it with the wrong one it won't end well. So it is here: you needed a slightly different head on your hammer, a slightly different option on your merge. In more common cases, ignoring space changes is a recipe for tab damage.

jthill
  • 55,082
  • 5
  • 77
  • 137