Re: git apply --3way behaves abnormally when the patch contains binary changes.

Jerry Zhang [off-list ref] writes:

quoted

        # 2. based on left_bin branch, make any change, and commit
        git checkout -b right &&
        cat bin.png bin.png > bin.png &&
        git add bin.png &&
        git commit -m "update binary file" &&

        # 3. make patch
        git diff --binary left..right >bin.diff &&
        # apply --3way, and it will fail
        test_must_fail git apply --index --3way bin.diff
'
"

But  "git apply --index --3way bin.diff" will not faill on Git version 2.31.0.

Are you sure? I checked out to "commit
a5828ae6b52137b913b978e16cd2334482eb4c1f (HEAD, tag: v2.31.0)" and
rebuilt and ran your test snippet and it still failed.

Isn't it just because the reproduction recipe is simply wrong?

It says

    * be on left branch and have a binary file
    * be on right branch and have a modified binary file
    * create a patch to take left to right

Notice that we have a patch and we are still on the right branch.
Of course, applying the patch to take us from left to right would
fail from that state, but I _think_ the intent of the reproduction
recipe was, after all of the above, do this here:

    * switch to left branch and attempt to apply the patch.

And the patch is meant to take us from left to right, and we are on
pristine left, the application ought to cleanly succeed, no?

"git apply bin.diff" would probably work correctly but I do not know
offhand what the code after your change does with --3way enabled.

We refuse to merge binary files, so I would not be surprised if we
failed the 3way in this case (even though we _could_ fast-forward,
it may not be worth complicating the --3way logic---nobody sane
would say --3way when it is unnecessary) but after 3way fails, do we
still correctly fall back to "straight application" like we do for
text patches with your change?  Before your change, we would have
first attempted the "straight application", which would succeed and
wouldn't have hit "3way will refuse to merge binaries" at all.

So, I do not think it is implausible that we are seeing a legit
regression report.

Thanks.

On Tue, Jul 27, 2021 at 6:08 PM Junio C Hamano [off-list ref] wrote:

Jerry Zhang [off-list ref] writes:

quoted

quoted

        # 2. based on left_bin branch, make any change, and commit
        git checkout -b right &&
        cat bin.png bin.png > bin.png &&
        git add bin.png &&
        git commit -m "update binary file" &&

        # 3. make patch
        git diff --binary left..right >bin.diff &&
        # apply --3way, and it will fail
        test_must_fail git apply --index --3way bin.diff
'
"

But  "git apply --index --3way bin.diff" will not faill on Git version 2.31.0.

Are you sure? I checked out to "commit
a5828ae6b52137b913b978e16cd2334482eb4c1f (HEAD, tag: v2.31.0)" and
rebuilt and ran your test snippet and it still failed.

Isn't it just because the reproduction recipe is simply wrong?

It says

    * be on left branch and have a binary file
    * be on right branch and have a modified binary file
    * create a patch to take left to right

Notice that we have a patch and we are still on the right branch.
Of course, applying the patch to take us from left to right would
fail from that state, but I _think_ the intent of the reproduction
recipe was, after all of the above, do this here:

    * switch to left branch and attempt to apply the patch.

And the patch is meant to take us from left to right, and we are on
pristine left, the application ought to cleanly succeed, no?

"git apply bin.diff" would probably work correctly but I do not know
offhand what the code after your change does with --3way enabled.

We refuse to merge binary files, so I would not be surprised if we
failed the 3way in this case (even though we _could_ fast-forward,
it may not be worth complicating the --3way logic---nobody sane
would say --3way when it is unnecessary) but after 3way fails, do we
still correctly fall back to "straight application" like we do for
text patches with your change?  Before your change, we would have
first attempted the "straight application", which would succeed and
wouldn't have hit "3way will refuse to merge binaries" at all.

Ah yep it's exactly as you say. I'll add the fix and a couple of test cases
into a new patch.

So, I do not think it is implausible that we are seeing a legit
regression report.

Thanks.