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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 28, 2021 at 1:08 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Jerry Zhang <jerry@xxxxxxxxxx> 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 see, this could be true. But then we would also have the case where
a merge driver results in conflict and direct patch application applies cleanly,
*but* the direct patch application is actually incorrect (for reasons relating
to the original purpose of the patch to switch the order, that direct patch
application can be wrong for files of repeating content). In this case
the user might actually want to see the conflict as it is more correct, but
it is being hidden by our fallback.

If the backwards compatibility story is going to get messy like this, perhaps
the best solution is to make a new flag similar to "--actually-3way" that
will attempt 3way and nothing else, and users who know what they want
can use that to get what they want.
>
> 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?

Well it isn't really failing right? Failing 3way would be not finding
the object ids in the database, which would indicate a failure to
even attempt 3way. This would result in fallback to direct application.
What we're seeing is that 3way application results in conflicts where
direct application would not result in conflicts. Having a conflict is
currently not a reason for the code to fall back to direct application,
here is the relevant line:
"
         try_threeway(state, &image, patch, st, ce) < 0) {
"
try_threeway returns 1 in case of conflict, 0 for success, and -1
for true errors.

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux