Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
- From
Jeff King <peff@peff.net>
- Date
- Sep 12, 2012, 13:17 UTC
- Message-ID
- <20120912131702.GA13710@sigill.intra.peff.net>
- In-Reply-To
- <7vk3vzfwme.fsf@alter.siamese.dyndns.org>
On Wed, Sep 12, 2012 at 01:55:53AM -0700, Junio C Hamano wrote:
Show 35 quoted lines
> > 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