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.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to