{"thread":{"id":"56169","subject":"git apply --3way behaves abnormally when the patch contains binary changes.","startedAt":"2021-07-27T14:34:35Z","lastAt":"2021-07-28T04:45:46Z","messageCount":5,"participants":["lilinchao@oschina.cn","Jerry Zhang","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"431311","messageId":"fdfd283aeee311ebbfb50024e87935e7@oschina.cn","threadId":"56169","inReplyTo":null,"subject":"git apply --3way behaves abnormally when the patch contains binary changes.","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2021-07-27T14:07:32Z","receivedAt":"2021-07-27T14:34:35Z","isPatch":false,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":"I see the latest change about `git apply --3way` is 923cd87, but it doesn't seem to have been fully tested\n(in t4108-apply-threeway.sh).\nOn latest Git version 2.32.0, consider test case below:\n\"\ntest_expect_success 'apply binary file patch with --3way' '\n        # 1. on new branch, commit binary file \n        git checkout -b left &&\n        cat \"$TEST_DIRECTORY\"/test-binary-1.png >bin.png &&\n        git add bin.png &&\n        git commit -m \"add binary file\" &&\n\n        # 2. based on left_bin branch, make any change, and commit\n        git checkout -b right &&\n        cat bin.png bin.png > bin.png &&\n        git add bin.png &&\n        git commit -m \"update binary file\" &&\n\n        # 3. make patch\n        git diff --binary left..right >bin.diff &&\n        # apply --3way, and it will fail\n        test_must_fail git apply --index --3way bin.diff\n'\n\"\n\nBut  \"git apply --index --3way bin.diff\" will not faill on Git version 2.31.0.\n\n\n"},{"id":"431369","messageId":"CAMKO5Cs1HP7JNmJLYKti0kajGmD4XK+Boc3WRV2Dpph5a3b5Xw@mail.gmail.com","threadId":"56169","inReplyTo":"fdfd283aeee311ebbfb50024e87935e7@oschina.cn","subject":"Re: git apply --3way behaves abnormally when the patch contains binary changes.","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-07-27T22:44:08Z","receivedAt":"2021-07-27T22:44:23Z","isPatch":false,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Tue, Jul 27, 2021 at 7:07 AM lilinchao@oschina.cn\n<lilinchao@oschina.cn> wrote:\n>\n> I see the latest change about `git apply --3way` is 923cd87, but it doesn't seem to have been fully tested\n> (in t4108-apply-threeway.sh).\n> On latest Git version 2.32.0, consider test case below:\n> \"\n> test_expect_success 'apply binary file patch with --3way' '\n>         # 1. on new branch, commit binary file\n>         git checkout -b left &&\n>         cat \"$TEST_DIRECTORY\"/test-binary-1.png >bin.png &&\n>         git add bin.png &&\n>         git commit -m \"add binary file\" &&\n>\n>         # 2. based on left_bin branch, make any change, and commit\n>         git checkout -b right &&\n>         cat bin.png bin.png > bin.png &&\n>         git add bin.png &&\n>         git commit -m \"update binary file\" &&\n>\n>         # 3. make patch\n>         git diff --binary left..right >bin.diff &&\n>         # apply --3way, and it will fail\n>         test_must_fail git apply --index --3way bin.diff\n> '\n> \"\n>\n> But  \"git apply --index --3way bin.diff\" will not faill on Git version 2.31.0.\nAre you sure? I checked out to \"commit\na5828ae6b52137b913b978e16cd2334482eb4c1f (HEAD, tag: v2.31.0)\" and\nrebuilt and ran your test snippet and it still failed.\n\nThis was the message from the failure on 2.31.0\n\"error: the patch applies to 'bin.png'\n(e69de29bb2d1d6434b8b29ae775ad8c2e48c5391), which does not match the\ncurrent contents.\nFalling back to three-way merge...\nwarning: Cannot merge binary files: bin.png (ours vs. theirs)\nApplied patch to 'bin.png' with conflicts.\nU bin.png\"\n\nVersus the message on 2.32.0\n\"warning: Cannot merge binary files: bin.png (ours vs. theirs)\nApplied patch to 'bin.png' with conflicts.\nU bin.png\"\n\nSo the failure messaging is different but it returns 1 both times. Is\nthere a difference between how we're testing?\n\nI did have to modify your test to add\ntest_expect_success 'apply binary file patch with --3way' '\n       # 1. on new branch, commit binary file\n       git checkout -b left &&\n+       git reset --hard &&\n\nIf this behavior is important I'd urge you to add this test to the suite.\n>\n>\n"},{"id":"431374","messageId":"xmqqv94vb5z6.fsf@gitster.g","threadId":"56169","inReplyTo":"CAMKO5Cs1HP7JNmJLYKti0kajGmD4XK+Boc3WRV2Dpph5a3b5Xw@mail.gmail.com","subject":"Re: git apply --3way behaves abnormally when the patch contains binary changes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T01:08:13Z","receivedAt":"2021-07-28T01:08:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n>>         # 2. based on left_bin branch, make any change, and commit\n>>         git checkout -b right &&\n>>         cat bin.png bin.png > bin.png &&\n>>         git add bin.png &&\n>>         git commit -m \"update binary file\" &&\n>>\n>>         # 3. make patch\n>>         git diff --binary left..right >bin.diff &&\n>>         # apply --3way, and it will fail\n>>         test_must_fail git apply --index --3way bin.diff\n>> '\n>> \"\n>>\n>> But  \"git apply --index --3way bin.diff\" will not faill on Git version 2.31.0.\n> Are you sure? I checked out to \"commit\n> a5828ae6b52137b913b978e16cd2334482eb4c1f (HEAD, tag: v2.31.0)\" and\n> rebuilt and ran your test snippet and it still failed.\n\nIsn't it just because the reproduction recipe is simply wrong?\n\nIt says\n\n    * be on left branch and have a binary file\n    * be on right branch and have a modified binary file\n    * create a patch to take left to right\n\nNotice that we have a patch and we are still on the right branch.\nOf course, applying the patch to take us from left to right would\nfail from that state, but I _think_ the intent of the reproduction\nrecipe was, after all of the above, do this here:\n\n    * switch to left branch and attempt to apply the patch.\n\nAnd the patch is meant to take us from left to right, and we are on\npristine left, the application ought to cleanly succeed, no?\n\n\"git apply bin.diff\" would probably work correctly but I do not know\noffhand what the code after your change does with --3way enabled.\n\nWe refuse to merge binary files, so I would not be surprised if we\nfailed the 3way in this case (even though we _could_ fast-forward,\nit may not be worth complicating the --3way logic---nobody sane\nwould say --3way when it is unnecessary) but after 3way fails, do we\nstill correctly fall back to \"straight application\" like we do for\ntext patches with your change?  Before your change, we would have\nfirst attempted the \"straight application\", which would succeed and\nwouldn't have hit \"3way will refuse to merge binaries\" at all.\n\nSo, I do not think it is implausible that we are seeing a legit\nregression report.\n\nThanks.\n"},{"id":"431375","messageId":"CAMKO5CvM-FUMTxGeaiYY--PvXPRYESbA7r_-=A3668Vd7AHqxQ@mail.gmail.com","threadId":"56169","inReplyTo":"xmqqv94vb5z6.fsf@gitster.g","subject":"Re: git apply --3way behaves abnormally when the patch contains binary changes.","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-07-28T01:37:58Z","receivedAt":"2021-07-28T01:38:12Z","isPatch":false,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Tue, Jul 27, 2021 at 6:08 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jerry Zhang <jerry@skydio.com> writes:\n>\n> >>         # 2. based on left_bin branch, make any change, and commit\n> >>         git checkout -b right &&\n> >>         cat bin.png bin.png > bin.png &&\n> >>         git add bin.png &&\n> >>         git commit -m \"update binary file\" &&\n> >>\n> >>         # 3. make patch\n> >>         git diff --binary left..right >bin.diff &&\n> >>         # apply --3way, and it will fail\n> >>         test_must_fail git apply --index --3way bin.diff\n> >> '\n> >> \"\n> >>\n> >> But  \"git apply --index --3way bin.diff\" will not faill on Git version 2.31.0.\n> > Are you sure? I checked out to \"commit\n> > a5828ae6b52137b913b978e16cd2334482eb4c1f (HEAD, tag: v2.31.0)\" and\n> > rebuilt and ran your test snippet and it still failed.\n>\n> Isn't it just because the reproduction recipe is simply wrong?\n>\n> It says\n>\n>     * be on left branch and have a binary file\n>     * be on right branch and have a modified binary file\n>     * create a patch to take left to right\n>\n> Notice that we have a patch and we are still on the right branch.\n> Of course, applying the patch to take us from left to right would\n> fail from that state, but I _think_ the intent of the reproduction\n> recipe was, after all of the above, do this here:\n>\n>     * switch to left branch and attempt to apply the patch.\n>\n> And the patch is meant to take us from left to right, and we are on\n> pristine left, the application ought to cleanly succeed, no?\n>\n> \"git apply bin.diff\" would probably work correctly but I do not know\n> offhand what the code after your change does with --3way enabled.\n>\n> We refuse to merge binary files, so I would not be surprised if we\n> failed the 3way in this case (even though we _could_ fast-forward,\n> it may not be worth complicating the --3way logic---nobody sane\n> would say --3way when it is unnecessary) but after 3way fails, do we\n> still correctly fall back to \"straight application\" like we do for\n> text patches with your change?  Before your change, we would have\n> first attempted the \"straight application\", which would succeed and\n> wouldn't have hit \"3way will refuse to merge binaries\" at all.\nAh yep it's exactly as you say. I'll add the fix and a couple of test cases\ninto a new patch.\n>\n> So, I do not think it is implausible that we are seeing a legit\n> regression report.\n>\n> Thanks.\n"},{"id":"431382","messageId":"9f510f56ef5e11eb90f70026b95c99cc@oschina.cn","threadId":"56169","inReplyTo":"4eb90a4eef4011ebab68d4ae5272fd1139378@pobox.com","subject":"Re: Re: git apply --3way behaves abnormally when the patch contains binary changes.","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2021-07-28T04:45:21Z","receivedAt":"2021-07-28T04:45:46Z","isPatch":false,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":"The defective test demo I provided is not that important(for me), the purpose of this is to\nbring out our topic that \"git apply -3\" behaves differently on different Git version when\nthe patch is binary.\nMaybe I would say this breaks backward compatibility, but the poor test demo didn't prove this. \nIf anyone would like to see the incompatibility, he can run \"git apply --index  -3 binary.diff\" in command line in different Git version environment.\n\n>Jerry Zhang <jerry@skydio.com> writes:\n>\n>>>         # 2. based on left_bin branch, make any change, and commit\n>>>         git checkout -b right &&\n>>>         cat bin.png bin.png > bin.png &&\n>>>         git add bin.png &&\n>>>         git commit -m \"update binary file\" &&\n>>>\n>>>         # 3. make patch\n>>>         git diff --binary left..right >bin.diff &&\n>>>         # apply --3way, and it will fail\n>>>         test_must_fail git apply --index --3way bin.diff\n>>> '\n>>> \"\n>>>\n>>> But  \"git apply --index --3way bin.diff\" will not faill on Git version 2.31.0.\n>> Are you sure? I checked out to \"commit\n>> a5828ae6b52137b913b978e16cd2334482eb4c1f (HEAD, tag: v2.31.0)\" and\n>> rebuilt and ran your test snippet and it still failed.\n>\n>Isn't it just because the reproduction recipe is simply wrong?\n>\n>It says\n>\n>    * be on left branch and have a binary file\n>    * be on right branch and have a modified binary file\n>    * create a patch to take left to right\n>\n>Notice that we have a patch and we are still on the right branch.\n>Of course, applying the patch to take us from left to right would\n>fail from that state, but I _think_ the intent of the reproduction\n>recipe was, after all of the above, do this here:\n>\n>    * switch to left branch and attempt to apply the patch.\n>\n>And the patch is meant to take us from left to right, and we are on\n>pristine left, the application ought to cleanly succeed, no?\n>\n>\"git apply bin.diff\" would probably work correctly but I do not know\n>offhand what the code after your change does with --3way enabled.\n>\n>We refuse to merge binary files, so I would not be surprised if we\n>failed the 3way in this case (even though we _could_ fast-forward,\n>it may not be worth complicating the --3way logic---nobody sane\n>would say --3way when it is unnecessary) but after 3way fails, do we\n>still correctly fall back to \"straight application\" like we do for\n>text patches with your change?  Before your change, we would have\n>first attempted the \"straight application\", which would succeed and\n>wouldn't have hit \"3way will refuse to merge binaries\" at all.\n>\n>So, I do not think it is implausible that we are seeing a legit\n>regression report.\n>\n>Thanks."}]}