Binary file-friendly merge -Xours or -Xtheirs?

Binary file-friendly merge -Xours or -Xtheirs?

[Stephen Bash]

Hi all-

Helping a coworker resolve merge conflicts today I found I wanted a -Xtheirs that completely replaces conflicted binary files with the copy from the incoming branch.  In other words rather than doing

  $ git merge maint
  ... conflicts occur ...
  $ git checkout --theirs -- path/to/binary-file
  $ git add path/to/binary-file
  $ git commit

I'd like to do

  $ git merge -Xbinary_theirs maint
  ... binary conflicts are resolved automatically ...

>From reading the docs it's obvious the current -Xours and -Xtheirs expect to work on hunks, so I (mostly) understand the current behavior, but as a user it feels like "I'm telling you how to resolve conflicts, please do the same thing for binary files".

I am missing a solution that already exists?  Or is this perhaps an improvement that could be made?

Thanks,
Stephen

Re: Binary file-friendly merge -Xours or -Xtheirs?

[Junio C Hamano]

Stephen Bash <bash@genarts.com> writes:

> From reading the docs it's obvious the current -Xours and -Xtheirs
> expect to work on hunks, so I (mostly) understand the current
> behavior, but as a user it feels like "I'm telling you how to
> resolve conflicts, please do the same thing for binary files".

Even though merge-recursive accepts -Xours/-Xtheirs, I do not think
the low-level merge machinery for anything but the text merge is
aware of the option.

It may be just the matter of something like this, though (completely
untested).



 ll-merge.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git i/ll-merge.c w/ll-merge.c
index f3f7692..fee578f 100644
--- i/ll-merge.c
+++ w/ll-merge.c
@@ -46,16 +46,31 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 	assert(opts);
 
 	/*
-	 * The tentative merge result is "ours" for the final round,
-	 * or common ancestor for an internal merge.  Still return
-	 * "conflicted merge" status.
+	 * The tentative merge result is the or common ancestor for an internal merge.
 	 */
-	stolen = opts->virtual_ancestor ? orig : src1;
+	if (opts->virtual_ancestor) {
+		stolen = orig;
+	} else {
+		switch (opts->variant) {
+		default:
+		case XDL_MERGE_FAVOR_OURS:
+			stolen = src1;
+			break;
+		case XDL_MERGE_FAVOR_THEIRS:
+			stolen = src2;
+			break;
+		}
+	}
 
 	result->ptr = stolen->ptr;
 	result->size = stolen->size;
 	stolen->ptr = NULL;
-	return 1;
+
+	/*
+	 * With -Xtheirs or -Xours, we have cleanly merged;
+	 * otherwise we got a conflict.
+	 */
+	return (opts->variant ? 0 : 1);
 }
 
 static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,

[PATCH 0/2] Teaching -Xours/-Xtheirs to binary ll-merge driver

[Junio C Hamano]

The part that grants Stephen's wish is unchanged from the earlier
"perhaps like this" patch, but this time with a bit of documentation
and test.

A more important change between the two actually is [PATCH 2/2].
When a "binary" synthetic attribute is given to a path, we used to
(1) disable textual diff and (2) disable CR/LF conversion, but it is
also sane to disable the textual merge for a path marked as
"binary", and setting the "-merge" attribute to summon the "binary"
ll-merge driver is the way to do so.

Junio C Hamano (2):
  merge: teach -Xours/-Xtheirs to binary ll-merge driver
  attr: "binary" attribute should choose built-in "binary" merge driver

 Documentation/gitattributes.txt    |  2 +-
 Documentation/merge-strategies.txt |  3 ++-
 attr.c                             |  2 +-
 ll-merge.c                         | 25 ++++++++++++++++++++-----
 t/t6037-merge-ours-theirs.sh       | 14 +++++++++++++-
 5 files changed, 37 insertions(+), 9 deletions(-)

-- 
1.7.12.322.g2c7d289

[PATCH 1/2] merge: teach -Xours/-Xtheirs to binary ll-merge driver

[Junio C Hamano]

The (discouraged) -Xours/-Xtheirs modes of merge are supposed to
give a quick and dirty way to come up with a random mixture of
cleanly merged parts and punted conflict resolution to take contents
from one side in conflicting parts.  These options however were only
passed down to the low level merge driver for text.

Teach the built-in binary merge driver to notice them as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/merge-strategies.txt |  3 ++-
 ll-merge.c                         | 25 ++++++++++++++++++++-----
 t/t6037-merge-ours-theirs.sh       | 14 +++++++++++++-
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 595a3cf..66db802 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -32,13 +32,14 @@ ours;;
 	This option forces conflicting hunks to be auto-resolved cleanly by
 	favoring 'our' version.  Changes from the other tree that do not
 	conflict with our side are reflected to the merge result.
+	For a binary file, the entire contents are taken from our side.
 +
 This should not be confused with the 'ours' merge strategy, which does not
 even look at what the other tree contains at all.  It discards everything
 the other tree did, declaring 'our' history contains all that happened in it.
 
 theirs;;
-	This is opposite of 'ours'.
+	This is the opposite of 'ours'.
 
 patience;;
 	With this option, 'merge-recursive' spends a little extra time
diff --git a/ll-merge.c b/ll-merge.c
index da59738..8535e2d 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -46,16 +46,31 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 	assert(opts);
 
 	/*
-	 * The tentative merge result is "ours" for the final round,
-	 * or common ancestor for an internal merge.  Still return
-	 * "conflicted merge" status.
+	 * The tentative merge result is the or common ancestor for an internal merge.
 	 */
-	stolen = opts->virtual_ancestor ? orig : src1;
+	if (opts->virtual_ancestor) {
+		stolen = orig;
+	} else {
+		switch (opts->variant) {
+		default:
+		case XDL_MERGE_FAVOR_OURS:
+			stolen = src1;
+			break;
+		case XDL_MERGE_FAVOR_THEIRS:
+			stolen = src2;
+			break;
+		}
+	}
 
 	result->ptr = stolen->ptr;
 	result->size = stolen->size;
 	stolen->ptr = NULL;
-	return 1;
+
+	/*
+	 * With -Xtheirs or -Xours, we have cleanly merged;
+	 * otherwise we got a conflict.
+	 */
+	return (opts->variant ? 0 : 1);
 }
 
 static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 2cf42c7..8d05671 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -53,7 +53,19 @@ test_expect_success 'recursive favouring ours' '
 	! grep 1 file
 '
 
-test_expect_success 'pull with -X' '
+test_expect_success 'binary file with -Xours/-Xtheirs' '
+	echo "file -merge" >.gitattributes &&
+
+	git reset --hard master &&
+	git merge -s recursive -X theirs side &&
+	git diff --exit-code side HEAD -- file &&
+
+	git reset --hard master &&
+	git merge -s recursive -X ours side &&
+	git diff --exit-code master HEAD -- file
+'
+
+test_expect_success 'pull passes -X to underlying merge' '
 	git reset --hard master && git pull -s recursive -Xours . side &&
 	git reset --hard master && git pull -s recursive -X ours . side &&
 	git reset --hard master && git pull -s recursive -Xtheirs . side &&
-- 
1.7.12.322.g2c7d289

[PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

[Junio C Hamano]

The built-in "binary" attribute macro expands to "-diff -text", so
that textual diff is not produced, and the contents will not go
through any CR/LF conversion ever.  During a merge, it should also
choose the "binary" low-level merge driver, but it didn't.

Make it expand to "-diff -merge -text".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/gitattributes.txt | 2 +-
 attr.c                          | 2 +-
 t/t6037-merge-ours-theirs.sh    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index a85b187..ead7254 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -904,7 +904,7 @@ file at the toplevel (i.e. not in any subdirectory).  The built-in
 macro attribute "binary" is equivalent to:
 
 ------------
-[attr]binary -diff -text
+[attr]binary -diff -merge -text
 ------------
 
 
diff --git a/attr.c b/attr.c
index 303751f..3f581b3 100644
--- a/attr.c
+++ b/attr.c
@@ -306,7 +306,7 @@ static void free_attr_elem(struct attr_stack *e)
 }
 
 static const char *builtin_attr[] = {
-	"[attr]binary -diff -text",
+	"[attr]binary -diff -merge -text",
 	NULL,
 };
 
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 8d05671..3889eca 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -54,7 +54,7 @@ test_expect_success 'recursive favouring ours' '
 '
 
 test_expect_success 'binary file with -Xours/-Xtheirs' '
-	echo "file -merge" >.gitattributes &&
+	echo file binary >.gitattributes &&
 
 	git reset --hard master &&
 	git merge -s recursive -X theirs side &&
-- 
1.7.12.322.g2c7d289

Re: [PATCH 0/2] Teaching -Xours/-Xtheirs to binary ll-merge driver

[Stephen Bash]

----- Original Message -----
> From: "Junio C Hamano" <gitster@pobox.com>
> Sent: Sunday, September 9, 2012 12:40:37 AM
> Subject: [PATCH 0/2] Teaching -Xours/-Xtheirs to binary ll-merge driver
> 
> The part that grants Stephen's wish is unchanged from the earlier
> "perhaps like this" patch, but this time with a bit of documentation
> and test.
> 
> A more important change between the two actually is [PATCH 2/2].
> When a "binary" synthetic attribute is given to a path, we used to
> (1) disable textual diff and (2) disable CR/LF conversion, but it is
> also sane to disable the textual merge for a path marked as
> "binary", and setting the "-merge" attribute to summon the "binary"
> ll-merge driver is the way to do so.
> 
> Junio C Hamano (2):
>   merge: teach -Xours/-Xtheirs to binary ll-merge driver
>   attr: "binary" attribute should choose built-in "binary" merge
>   driver
> 
>  Documentation/gitattributes.txt    |  2 +-
>  Documentation/merge-strategies.txt |  3 ++-
>  attr.c                             |  2 +-
>  ll-merge.c                         | 25 ++++++++++++++++++++-----
>  t/t6037-merge-ours-theirs.sh       | 14 +++++++++++++-
>  5 files changed, 37 insertions(+), 9 deletions(-)

Thanks for this Junio.  After figuring out how to make the corporate email server NOT munge the patches, I was able to apply these with no problem.  Then 

  $ git merge -Xtheirs maint

mostly does what I want, but conflicting files deleted on the incoming branch are still marked unmerged/"deleted by them" (binary or not).  I'm not sure what the best (most common?) resolution is for that.  In my case I would be happy for the -Xtheirs to also delete the files, but that might not always be the right answer (but then again -Xtheirs is a pretty heavy hammer to begin with).

Thanks,
Stephen

Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

[Jeff King]

On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:

> The built-in "binary" attribute macro expands to "-diff -text", so
> that textual diff is not produced, and the contents will not go
> through any CR/LF conversion ever.  During a merge, it should also
> choose the "binary" low-level merge driver, but it didn't.
> 
> Make it expand to "-diff -merge -text".

Yeah, that seems like the obviously correct thing to do. In practice,
most files would end up in the first few lines of ll_xdl_merge checking
buffer_is_binary anyway, so I think this would really only make a
difference when our "is it binary?" heuristic guesses wrong.

>  Documentation/gitattributes.txt | 2 +-
>  attr.c                          | 2 +-
>  t/t6037-merge-ours-theirs.sh    | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

Patch itself looks good to me.

-Peff

Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:
>
>> The built-in "binary" attribute macro expands to "-diff -text", so
>> that textual diff is not produced, and the contents will not go
>> through any CR/LF conversion ever.  During a merge, it should also
>> choose the "binary" low-level merge driver, but it didn't.
>> 
>> Make it expand to "-diff -merge -text".
>
> Yeah, that seems like the obviously correct thing to do. In practice,
> most files would end up in the first few lines of ll_xdl_merge checking
> buffer_is_binary anyway, so I think this would really only make a
> difference when our "is it binary?" heuristic guesses wrong.

You made me look at that part again and then made me notice
something unrelated.

	if (buffer_is_binary(orig->ptr, orig->size) ||
	    buffer_is_binary(src1->ptr, src1->size) ||
	    buffer_is_binary(src2->ptr, src2->size)) {
		warning("Cannot merge binary files: %s (%s vs. %s)",
			path, name1, name2);
		return ll_binary_merge(drv_unused, result,
				       path,
				       orig, orig_name,
				       src1, name1,
				       src2, name2,
				       opts, marker_size);
	}

Given that we now may know how to merge these things, the
unconditional warning feels very wrong.

Perhaps something like this makes it better.

A path that is explicitly marked as binary did not get any such
warning, but it will start to get warned just like a path that was
auto-detected to be a binary.

It is a behaviour change, but I think it is a good one that makes
two cases more consistent.

And we won't see the warning when -Xtheirs/-Xours large sledgehammer
is in use, which tells us how to resolve these things "cleanly".

 ll-merge.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git i/ll-merge.c w/ll-merge.c
index 8535e2d..307315b 100644
--- i/ll-merge.c
+++ w/ll-merge.c
@@ -35,7 +35,7 @@ struct ll_merge_driver {
  */
 static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   mmbuffer_t *result,
-			   const char *path_unused,
+			   const char *path,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
@@ -53,6 +53,9 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 	} else {
 		switch (opts->variant) {
 		default:
+			warning("Cannot merge binary files: %s (%s vs. %s)\n",
+				path, name1, name2);
+			/* fallthru */
 		case XDL_MERGE_FAVOR_OURS:
 			stolen = src1;
 			break;
@@ -88,8 +91,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	if (buffer_is_binary(orig->ptr, orig->size) ||
 	    buffer_is_binary(src1->ptr, src1->size) ||
 	    buffer_is_binary(src2->ptr, src2->size)) {
-		warning("Cannot merge binary files: %s (%s vs. %s)\n",
-			path, name1, name2);
 		return ll_binary_merge(drv_unused, result,
 				       path,
 				       orig, orig_name,

Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

[Stephen Bash]

----- Original Message -----
> From: "Junio C Hamano" <gitster@pobox.com>
> Sent: Wednesday, September 12, 2012 4:55:53 AM
> Subject: Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
> 
> Jeff King <peff@peff.net> writes:
> 
> > On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:
> >
> >> The built-in "binary" attribute macro expands to "-diff -text", so
> >> that textual diff is not produced, and the contents will not go
> >> through any CR/LF conversion ever.  During a merge, it should also
> >> choose the "binary" low-level merge driver, but it didn't.
> >> 
> >> Make it expand to "-diff -merge -text".
> >
> > Yeah, that seems like the obviously correct thing to do. In
> > practice,
> > most files would end up in the first few lines of ll_xdl_merge
> > checking
> > buffer_is_binary anyway, so I think this would really only make a
> > difference when our "is it binary?" heuristic guesses wrong.
> 
> You made me look at that part again and then made me notice
> something unrelated.
> 
> 	if (buffer_is_binary(orig->ptr, orig->size) ||
> 	    buffer_is_binary(src1->ptr, src1->size) ||
> 	    buffer_is_binary(src2->ptr, src2->size)) {
> 		warning("Cannot merge binary files: %s (%s vs. %s)",
> 			path, name1, name2);
> 		return ll_binary_merge(drv_unused, result,
> 				       path,
> 				       orig, orig_name,
> 				       src1, name1,
> 				       src2, name2,
> 				       opts, marker_size);
> 	}
> 
> Given that we now may know how to merge these things, the
> unconditional warning feels very wrong.
> 
> Perhaps something like this makes it better.

Patch didn't apply on top of the previous two for me, but after making the edits manually does what it claims to do (and makes the merge output much nicer to read, thanks!).  The only remaining question for me is should -Xtheirs resolve "deleted by them" conflicts?

Thanks,
Stephen

Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

[Jeff King]

On Wed, Sep 12, 2012 at 01:55:53AM -0700, Junio C Hamano wrote:

> > Yeah, that seems like the obviously correct thing to do. In practice,
> > most files would end up in the first few lines of ll_xdl_merge checking
> > buffer_is_binary anyway, so I think this would really only make a
> > difference when our "is it binary?" heuristic guesses wrong.
> 
> You made me look at that part again and then made me notice
> something unrelated.
> 
> 	if (buffer_is_binary(orig->ptr, orig->size) ||
> 	    buffer_is_binary(src1->ptr, src1->size) ||
> 	    buffer_is_binary(src2->ptr, src2->size)) {
> 		warning("Cannot merge binary files: %s (%s vs. %s)",
> 			path, name1, name2);
> 		return ll_binary_merge(drv_unused, result,
> 				       path,
> 				       orig, orig_name,
> 				       src1, name1,
> 				       src2, name2,
> 				       opts, marker_size);
> 	}
> 
> Given that we now may know how to merge these things, the
> unconditional warning feels very wrong.
> 
> Perhaps something like this makes it better.
> 
> A path that is explicitly marked as binary did not get any such
> warning, but it will start to get warned just like a path that was
> auto-detected to be a binary.
> 
> It is a behaviour change, but I think it is a good one that makes
> two cases more consistent.
> 
> And we won't see the warning when -Xtheirs/-Xours large sledgehammer
> is in use, which tells us how to resolve these things "cleanly".

Yeah, I think it is the right thing to do. I noticed that the warning
would not trigger in the "-merge" case and wondered if it should, but
figured it was not a big deal either way.

However, I agree it is very bad for it to trigger with -Xours/theirs,
and that is worth fixing.  That it triggers in the "-merge" case
afterwards is a slight bonus.

-Peff

Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

[Junio C Hamano]

Stephen Bash <bash@genarts.com> writes:

>> Perhaps something like this makes it better.
>
> Patch didn't apply on top of the previous two for me,...

Look at 'pu' and see how it applies.

> ...  The only remaining
> question for me is should -Xtheirs resolve "deleted by them"
> conflicts?

I do not know, and I do not care to worry about it too deeply, which
means I would rather err on the safe side and have users inspect the
situation, make the decision when such a conflict happens and
resolve them themselves, instead of claiming a clean resolution that
is possibly wrong.

Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

[Stephen Bash]

----- Original Message -----
> From: "Junio C Hamano" <gitster@pobox.com>
> Sent: Wednesday, September 12, 2012 1:01:30 PM
> Subject: Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
> 
> >> Perhaps something like this makes it better.
> >
> > Patch didn't apply on top of the previous two for me,...
> 
> Look at 'pu' and see how it applies.

Ah, that seems to work better.
 
> > ...  The only remaining
> > question for me is should -Xtheirs resolve "deleted by them"
> > conflicts?
> 
> I do not know, and I do not care to worry about it too deeply, which
> means I would rather err on the safe side and have users inspect the
> situation, make the decision when such a conflict happens and
> resolve them themselves, instead of claiming a clean resolution that
> is possibly wrong.

A reasonable approach.  Thanks for clarifying.

Stephen