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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2021, 20:08 UTC
Message-ID: <xmqq1r7i9p7f.fsf@gitster.g>
In-Reply-To: <CAMKO5CvZCMHuzRLSs2aHJ3iUH-LBJfFP3fG+GgwtQvsKQPtT5Q@mail.gmail.com>

Jerry Zhang <jerry@skydio.com> writes:

Show 6 quoted lines

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