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

[Stephen Bash <bash@genarts.com>]

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