merge-recursive: do not rudely die on binary merge

When you try to merge a path that involves binary file-level merge, merge-recursive died rudely without cleaning up its own mess. A files added by the merge were left in the working tree, but the index was not written out (because it just punted and died), so it was cumbersome for the user to retry it by first running "git reset --hard".

This changes merge-recursive to still warn but do the "binary" merge for such a path; leave the "our" version in the working tree, but still keep the path unmerged so that the user can sort it out.

---
 merge-recursive.c       |   51 +++++++++++++++++++----------------
 t/t6027-merge-binary.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 23 deletions(-)

index f7d1b84..5326d7c 100644
@@ -677,6 +677,26 @@ struct ll_merge_driver {
 /*
  * Built-in low-levels
  */
+static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
+			   const char *path_unused,
+			   mmfile_t *orig,
+			   mmfile_t *src1, const char *name1,
+			   mmfile_t *src2, const char *name2,
+			   mmbuffer_t *result)
+{
+	/*
+	 * The tentative merge result is "ours" for the final round,
+	 * or common ancestor for an internal merge.  Still return
+	 * "conflicted merge" status.
+	 */
+	mmfile_t *stolen = index_only ? orig : src1;
+
+	result->ptr = stolen->ptr;
+	result->size = stolen->size;
+	stolen->ptr = NULL;
+	return 1;
+}
+
 static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			const char *path_unused,
 			mmfile_t *orig,

@@ -687,10 +707,15 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	xpparam_t xpp;
 
 	if (buffer_is_binary(orig->ptr, orig->size) ||
-			buffer_is_binary(src1->ptr, src1->size) ||
-			buffer_is_binary(src2->ptr, src2->size))
-		return error("Cannot merge binary files: %s vs. %s\n",
+	    buffer_is_binary(src1->ptr, src1->size) ||
+	    buffer_is_binary(src2->ptr, src2->size)) {
+		warning("Cannot merge binary files: %s vs. %s\n",
 			name1, name2);
+		return ll_binary_merge(drv_unused, path_unused,
+				       orig, src1, name1,
+				       src2, name2,
+				       result);
+	}
 
 	memset(&xpp, 0, sizeof(xpp));
 	return xdl_merge(orig,

@@ -743,26 +768,6 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
 	return 0;
 }
 
-static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
-			   const char *path_unused,
-			   mmfile_t *orig,
-			   mmfile_t *src1, const char *name1,
-			   mmfile_t *src2, const char *name2,
-			   mmbuffer_t *result)
-{
-	/*
-	 * The tentative merge result is "ours" for the final round,
-	 * or common ancestor for an internal merge.  Still return
-	 * "conflicted merge" status.
-	 */
-	mmfile_t *stolen = index_only ? orig : src1;
-
-	result->ptr = stolen->ptr;
-	result->size = stolen->size;
-	stolen->ptr = NULL;
-	return 1;
-}
-
 #define LL_BINARY_MERGE 0
 #define LL_TEXT_MERGE 1
 #define LL_UNION_MERGE 2

Very nice. Thanks, Junio. As an additional convenience, it would be nice to make the "theirs" version easily accessible. Perhaps, by leaving an untracked file in the working tree, with the original filename, suffixed with a hash-prefix. Or alternatively, cut-n-pastable instuctions on stdout for replacing the file with the "theirs" version.

On the other hand, I tend to think that "theirs" would be a better default than "ours" anyway - still leaving the path unmerged, of course.

-chris

Forgot to ask. I did this because you had trouble on #git yesterday and then at around the same time today somebody else had the same issue. Did this patch solve your problem? I do not think this has big risk of regression, but if it does not help anything there is no reason to put it in 1.5.3, so I am asking for a success report.

Yes, with this patch, the merge completes, and leaves the index and working tree in a sane state. Thanks again.

-chris

I got bit by this one earlier, and the patch fixed it here.

Chris Larson - clarson at kergoth dot com
Dedicated Engineer - MontaVista - clarson at mvista dot com
Core Developer/Architect - TSLib, BitBake, OpenEmbedded, OpenZaurus

People should learn this command. Really.

	$ git cat-file -p :$n:path

where $n == 2 is ours, $n == 1 is common ancestor, and $n == 3 is theirs.

The git-rev-parse manpage talks about the :$n:path notation (buried deep in a list of other syntax) but it just says $n is a "stage number" -- someone who is not familiar with the internals of git's merge implementation is never going to be able to figure out that "1", "2", and "3" mean what Junio said.
---

Not sure if this is correct for octopus merges -- corrections welcome.

Documentation/git-rev-parse.txt | 5 ++++-

1 files changed, 4 insertions(+), 1 deletions(-)

index 4b4d229..4758c33 100644
@@ -215,7 +215,10 @@ blobs contained in a commit.

* A colon, optionally followed by a stage number (0 to 3) and a colon, followed by a path; this names a blob object in the index at the given path. Missing stage number (and the colon

-  that follows it) names an stage 0 entry.
+  that follows it) names an stage 0 entry. During a merge, stage
+  1 is the common ancestor, stage 2 is the target branch's version
+  (typically the current branch), and stage 3 is the version from
+  the branch being merged.

Here is an illustration, by Jon Loeliger. Both node B and C are a commit parents of commit node A. Parent commits are ordered

1.5.3.rc2.4.g726f9

I often forget which number corresponds to which source. I seem to recall somebody proposing :ours:$path a while ago, but I couldn't find any reference in the archive, so perhaps I just dreamed it.

Am I the only one who messes this up? If not, patch is below.

-- >8 --
sha1_name: allow human-readable stage aliases

This adds the alias ":ours:$path" to mean the same thing as ":2:$path", as well as "base" (for 1) and "theirs" (for 2), for those of us who merge infrequently and forget which is which.

The parsing is as strict as possible in order to minimize impact on filenames with colons. However, for some (presumably unlikely) filenames, the behavior is changed. Previously, you could look at stage 0 of any file beginning with the string "ours:" as simply "git-show :ours:foo". Now, because of the parsing conflict, you must use "git-show :0:ours:foo".

---

 sha1_name.c |   35 +++++++++++++++++++++++++++++------
 1 files changed, 29 insertions(+), 6 deletions(-)

index 2d727d5..eaa6bd7 100644
@@ -664,6 +664,7 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 	/* sha1:path --> object name of path in ent sha1
 	 * :path -> object name of path in index
 	 * :[0-3]:path -> object name of path in index at stage
+	 * :base|ours|theirs:path -> same as :[1-3]:path
 	 */
 	if (name[0] == ':') {
 		int stage = 0;

@@ -671,14 +672,36 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 		int pos;
 		if (namelen > 2 && name[1] == '/')
 			return get_sha1_oneline(name + 2, sha1);
-		if (namelen < 3 ||
-		    name[2] != ':' ||
-		    name[1] < '0' || '3' < name[1])
-			cp = name + 1;
-		else {
-			stage = name[1] - '0';
+		if (!strncmp(name+1, "0:", 2)) {
+			stage = 0;
+			cp = name + 3;
+		}
+		else if (!strncmp(name+1, "1:", 2)) {
+			stage = 1;
+			cp = name + 3;
+		}
+		else if (!strncmp(name+1, "base:", 5)) {
+			stage = 1;
+			cp = name + 6;
+		}
+		else if (!strncmp(name+1, "2:", 2)) {
+			stage = 2;
 			cp = name + 3;
 		}
+		else if (!strncmp(name+1, "ours:", 5)) {
+			stage = 2;
+			cp = name + 6;
+		}
+		else if (!strncmp(name+1, "3:", 2)) {
+			stage = 3;
+			cp = name + 3;
+		}
+		else if (!strncmp(name+1, "theirs:", 7)) {
+			stage = 3;
+			cp = name + 8;
+		}
+		else
+			cp = name + 1;
 		namelen = namelen - (cp - name);
 		if (!active_cache)
 			read_cache();

Maybe. ;-)

I've memorized it long long ago. But my coworkers haven't and always get it wrong, and look at me funny when I tell them "trust me, your data is in stage 2 and theirs is in stage 3... because that's the convention all of the tools you are using follows".

Keywords in that last part: "convention" and "tools you are using". Someone could redefine what the stages mean and load content into them using update index --index-info. You might even be able to load the stages in odd ways yourself from Porcelain.

Oh, like say git-rebase. During a rebase "theirs" (stage 3) is your file and "ours" (stage 2) is the upstream. Confusing now, ain't it? Mine is theirs and ours is theirs? Huh? Yeeaaaah.

This is why I've never liked most merge tools. They get hung up on what is theirs and what is mine and then at some point they wind up confusing the stages and getting them inverted. And this is exactly why git-merge.sh/git-rebase.sh/git-am.sh try to setup GITHEAD_* for git-merge-recursive, and why they set it up using branch names and patch subject lines, because it makes the conflict markers easier to understand.

Shawn.

Actually, what's wrong with the following:

git show HEAD:foo.c
git show MERGE_HEAD:foo.c

?

That gives you yours (HEAD) and theirs (MERGE_HEAD). And it doesn't abuse sha1_file.c. Granted it only works during a true merge and doesn't work during a rebase, but remember I just pointed out life is backwards anyway during a rebase, so uh, yea...

Shawn.

* Shawn O. Pearce:

I think that in the staged versions, the non-conflicting parts of the merge are in fact merged. For the HEAD/MERGE_HEAD versions, this isn't the case, obviously.

Florian Weimer                <[email protected]>
BFK edv-consulting GmbH       http://www.bfk.de/
Kriegsstraße 100              tel: +49-721-96201-1
D-76133 Karlsruhe             fax: +49-721-96201-99

No, the stage versions are not merged at all (but the working tree copy has all non-conflicting parts merged).

Here's a script that creates a conflicted file with one easily resolved part and one conflict. You can see the staged hashes at the end (and check the working tree copy to see that only the one conflict is marked).

-Peff

-- >8 --
mkdir repo && cd repo && git-init

head -n 100 </usr/share/dict/words >words
git-add words
git-commit -m words

sed '10d' <words >words.tmp
mv words.tmp words
git-commit -a -m 'remove 10'

git-checkout -b other HEAD^
sed '9d

     90d' <words >words.tmp

mv words.tmp words
git-commit -a -m 'remove 9 and 90'

git-merge master

echo "stage 2 git-rev-parse :2:words"
echo "HEAD git-rev-parse HEAD:words"
echo "stage 3 git-rev-parse :3:words"
echo "MERGE_HEAD git-rev-parse MERGE_HEAD:words"

Ugh, I hadn't even thought of that. git-diff _does_ respect "--base", "--ours", and "--theirs" to mean the same thing, but I am now wondering if that is a bit of a mistake.

However, as the intent of my patch was to _increase_ usability, I think a gotcha like that is probably counterproductive. OTOH, users of git-rebase already have to make the switch mentally.

Yes, I agree that the GITHEAD markers are much more sensible. Unfortunately, I'm not sure of the best way to translate them into stage names. The MERGE_HEAD/HEAD suggestion you made is a nice way of avoiding the whole issue, though it doesn't easily provide the "base" version.

I was about to, but your message has convinced me that this is perhaps not a very good idea.

-Peff

I am not _opposed_ to :ours:$path syntax, but I suspect there is something else that is wrong if you need to use :$n:$path syntax that often.

I have never been in a situation I had to say :base:$path, unless I am debugging the merge driver. So it is between :ours:$path and :theirs:$path.

But aren't they by definition HEAD:$path and MERGE_HEAD:$path, which are far more descriptive?

That's the problem: I don't use it that often, so when I do, the numbers seem nonsensical.

I used to need it to look at the 3-way merge, but git-mergetool now does a nice job of hiding these details from me.

Thinking about it more, I really _haven't_ used the stages at all since git-mergetool came around. This thread just reminded me of all the prior times when I had had trouble with it.

So Junio, please consider my patch retracted. What _would_ be useful is GITHEAD_* support for git-mergetool, which has been on my 'todo' list for some time. I'll see if I can work up a patch for that.

-Peff

Nice idea, if only this was mentioned in the documentation...

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

prefixcmp()?

-- Hannes

The patch makes sense. Thanks.

Just to give historical background to new readers, this is primarily because the really core level of the plumbing started as not caring between stages 2 and 3 (iow, as far as the merge is concerned, both heads are equal), and the description in the manual was written back then.

These days, all the merge strategies and other non-merge programs such as "git am" that can record conflicts as multi-stage index entries consistently use stage #2 as our version, and stages #2 and #3 are not equals anymore.

Pardon me? In what are they not equal?

In merge, the parents *are* equall. They are just recorded in the resulting commit in particular order and the stages are in that order.

Besides if I prepare a merge locally to push out to a shared repo, I will probably switch to the mainline and merge my branch in, so it will actually be my changes in stage #3. That is to say 'ours' and 'theirs' don't really express what is going on IMHO.

						 Jan 'Bulb' Hudec <[email protected]>

The version left in the worktree always corresponds to stage #2. There is no way to say "please use stage #2 for their version and use stage #3 for our version" (that is not necessary, so don't take this as if I am complaining about a lack of feature). "git-read-tree -m" knows that stage #2 corresopnds to the index and the worktree and performs up-to-date check on them to make sure you do not lose local changes.

Merge parent order does not matter --- they are more equal than stages between #2 and #3. But that's not what we are discussing here.

In some minor corners merge parents are not exactly equal -- history simplification and other places that wants to pick one parent iterates from the first to the last parent and pick the first one, so strictly speaking earlier parents have a slight edge over later ones.

A question related to this: as a user, how can I tell if a command is something I'm expected to use, or if thinking I need it is a sign that I'm doing something wrong?

Git has many commands, and telling the business-as-usual apart from the deviant ones is not always easy. It may be that it's just a question of knowing what is plumbing and what is a porcelain, but I'm not sure.

Cheers,

-- Nikodemus

Good question.

I guess "git show" instead of "git cat-file -p" is probably the recommended way these days. Mostly sticking to what the user manual demonstrates would be a safe thing to do, as J. Bruce Fields has really did a great job carefully picking the order of the commands to be presented in the manual.