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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2012, 08:55 UTC
Message-ID: <7vk3vzfwme.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120910140317.GA7906@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> 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,