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

[Jeff King <peff@peff.net>]

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