Re: [PATCH] git-apply: fix --3way with binary patch

[Junio C Hamano <gitster@pobox.com>]

Jerry Zhang <jerry@skydio.com> writes:

> So basically, another way of stating the problem would be that binary
> patches can apply cleanly with direct application in some cases where
> merge application is not clean. If i understand correctly this is unique
> to binary files, although it would be possible for a user to supply a custom
> merge driver for text files that is worse than direct application, that is
> most likely heavy user error that we shouldn't have to cater to.

Not really.  The built-in binary merge driver luckily had such
characteristics to allow us to catch this regression, but I see no
reason to believe that it is unique to binary.  Funky merge backends
like union merges can turn an otherwise conflicting merge into a
clean merge even for non-binary files.  And no, it is not an error
for a merge driver to fail "apply --3way" merge on incoming data
that "apply --no-3way" would apply cleanly.

> However
> the issue with binary is that the *default* merge driver is actually worse
> than direct application (in some cases).

> 1. do as you suggest and run 3way -> direct -> 3way. I would modify
> this and say we should only attempt this for binary patches, since a text
> file that fails 3way would most likely also fail direct,...

No, I do not trust our (myself and your) unsubstantiated belief that
it is limited to binary.  We saw a problem with binary, and I would
think it is a tip of iceberg for any non-straight-text-merge backend
(and I do not have any sound reason to believe that straight
text-merge backend will not have this issue).  I'd rather treat this
as coalmine canary.

I think the real problem, even without the "try threeway, fall back
to direct application, and then try threeway again", is that after
swapping the fallback order, a failed threeway does *not* fall back
to direct application in this case.  Regardless of what ll_merge()
and its backend does, if they fail, shouldn't the caller of
try_threeway() notice the failure and fall back to direct
application, just like the earlier code tried direct application
first and then _always_ fell back to threeway if it failed?  I do
not know exactly why today's code fails to do so, but I suspect that
fixing that is the real solution, no?

Independent from that, I suspect that it may be a good thing to do
to (at least optionally) allow ll_merge() to notice trivial merges
that proper merge frontends would never ask it to do and resolve
them trivially.  The patch you saw from me to ll_merge_binary() may
do so at a wrong layer (doing it in ll_merge() before it dispatches
to ll_merge_binary() and other backends might be a better approach)
but would be a good starting point for that independent effort, but
"apply --3way" should work correctly even with user-configured merge
drivers (after all, the "direct application first and then fall back
to 3way" code would have worked perfectly fine even with broken
custom merge drivers in the case we are discussing right now).

Thanks.