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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html