{"thread":{"id":"55439","subject":"[PATCH] git-apply: try threeway first when \"--3way\" is used","startedAt":"2021-04-06T02:56:02Z","lastAt":"2021-04-07T00:19:24Z","messageCount":6,"participants":["Jerry Zhang","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"421038","messageId":"20210406025551.25213-1-jerry@skydio.com","threadId":"55439","inReplyTo":null,"subject":"[PATCH] git-apply: try threeway first when \"--3way\" is used","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-06T02:55:51Z","receivedAt":"2021-04-06T02:56:02Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"The apply_fragments() method of \"git apply\"\ncan silently apply patches incorrectly if\na file has repeating contents. In these\ncases a three-way merge can apply it correctly\nor show a conflict. However, because the patches\napply \"successfully\" using apply_fragments(),\ngit will never fall back to the merge, even\nif the \"--3way\" flag is used, and the user has\nno way to ensure correctness by forcing the\nthree-way merge method.\n\nChange the behavior so that when \"--3way\" is\nused, git will always try the three-way merge\nfirst and will only fall back to apply_fragments()\nin caseswhere blobs are not available or some other\nerror (but not in the case of a merge conflict).\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\n Documentation/git-apply.txt |  5 ++---\n apply.c                     | 13 ++++++-------\n t/t4108-apply-threeway.sh   | 20 ++++++++++++++++++++\n 3 files changed, 28 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 91d9a8601c8c316d4649c405af42e531c39991a8..9144575299c264dd299b542b7b5948eef35f211c 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -84,9 +84,8 @@ OPTIONS\n \n -3::\n --3way::\n-\tWhen the patch does not apply cleanly, fall back on 3-way merge if\n-\tthe patch records the identity of blobs it is supposed to apply to,\n-\tand we have those blobs available locally, possibly leaving the\n+\tAttempt 3-way merge if the patch records the identity of blobs it is supposed\n+\tto apply to and we have those blobs available locally, possibly leaving the\n \tconflict markers in the files in the working tree for the user to\n \tresolve.  This option implies the `--index` option, and is incompatible\n \twith the `--reject` and the `--cached` options.\ndiff --git a/apply.c b/apply.c\nindex 6695a931e979a968b28af88d425d0c76ba17d0d4..62d65ef8d9c0b68857db55198c73db1f41589df1 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3569,10 +3569,10 @@ static int try_threeway(struct apply_state *state,\n \t\twrite_object_file(\"\", 0, blob_type, &pre_oid);\n \telse if (get_oid(patch->old_oid_prefix, &pre_oid) ||\n \t\t read_blob_object(&buf, &pre_oid, patch->old_mode))\n-\t\treturn error(_(\"repository lacks the necessary blob to fall back on 3-way merge.\"));\n+\t\treturn error(_(\"repository lacks the necessary blob to do 3-way merge.\"));\n \n \tif (state->apply_verbosity > verbosity_silent)\n-\t\tfprintf(stderr, _(\"Falling back to three-way merge...\\n\"));\n+\t\tfprintf(stderr, _(\"Doing three-way merge...\\n\"));\n \n \timg = strbuf_detach(&buf, &len);\n \tprepare_image(&tmp_image, img, len, 1);\n@@ -3604,7 +3604,7 @@ static int try_threeway(struct apply_state *state,\n \tif (status < 0) {\n \t\tif (state->apply_verbosity > verbosity_silent)\n \t\t\tfprintf(stderr,\n-\t\t\t\t_(\"Failed to fall back on three-way merge...\\n\"));\n+\t\t\t\t_(\"Failed to do three-way merge...\\n\"));\n \t\treturn status;\n \t}\n \n@@ -3637,10 +3637,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \tif (load_preimage(state, &image, patch, st, ce) < 0)\n \t\treturn -1;\n \n-\tif (patch->direct_to_threeway ||\n-\t    apply_fragments(state, &image, patch) < 0) {\n+\tif (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n \t\t/* Note: with --reject, apply_fragments() returns 0 */\n-\t\tif (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0)\n+\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)\n \t\t\treturn -1;\n \t}\n \tpatch->result = image.buf;\n@@ -5017,7 +5016,7 @@ int apply_parse_options(int argc, const char **argv,\n \t\tOPT_BOOL(0, \"apply\", force_apply,\n \t\t\tN_(\"also apply the patch (use with --stat/--summary/--check)\")),\n \t\tOPT_BOOL('3', \"3way\", &state->threeway,\n-\t\t\t N_( \"attempt three-way merge if a patch does not apply\")),\n+\t\t\t N_( \"attempt three-way merge, fall back on normal patch if that fails\")),\n \t\tOPT_FILENAME(0, \"build-fake-ancestor\", &state->fake_ancestor,\n \t\t\tN_(\"build a temporary index based on embedded index information\")),\n \t\t/* Think twice before adding \"--nul\" synonym to this */\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex d62db3fbe16f35a625a4a14eebb70034f695d3eb..0a7332fed5f60a8a2c9c25fc6713d513c3f0ace1 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -160,4 +160,24 @@ test_expect_success 'apply -3 with add/add conflict (dirty working tree)' '\n \ttest_cmp three.save three\n '\n \n+test_expect_success 'apply -3 with ambiguous repeating file' '\n+\tgit reset --hard &&\n+\ttest_write_lines 1 2 1 2 1 2 1 2 1 2 1>one_two_repeat &&\n+\tgit add one_two_repeat &&\n+\tgit commit -m \"init one\" &&\n+\ttest_write_lines 1 2 1 2 1 2 1 2 one 2 1>one_two_repeat &&\n+\tgit commit -a -m \"change one\" &&\n+\n+\tgit diff HEAD~ >Repeat.diff &&\n+\tgit reset --hard HEAD~ &&\n+\n+\ttest_write_lines 1 2 1 2 1 2 one 2 1 2 one>one_two_repeat &&\n+\tgit commit -a -m \"change surrounding one\" &&\n+\n+\tgit apply --index --3way Repeat.diff &&\n+\ttest_write_lines 1 2 1 2 1 2 one 2 one 2 one>expect &&\n+\n+\ttest_cmp expect one_two_repeat\n+'\n+\n test_done\n-- \n2.29.0\n\n"},{"id":"421045","messageId":"xmqqblas2b52.fsf@gitster.g","threadId":"55439","inReplyTo":"20210406025551.25213-1-jerry@skydio.com","subject":"Re: [PATCH] git-apply: try threeway first when \"--3way\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-06T06:13:45Z","receivedAt":"2021-04-06T06:13:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n> The apply_fragments() method of \"git apply\"\n> can silently apply patches incorrectly if\n> a file has repeating contents. In these\n> cases a three-way merge can apply it correctly\n\nIs that \"can apply\"?  Isn't it \"has a better chance to correctly\napply\"?\n\n> or show a conflict. However, because the patches\n> apply \"successfully\" using apply_fragments(),\n> git will never fall back to the merge, even\n> if the \"--3way\" flag is used, and the user has\n> no way to ensure correctness by forcing the\n> three-way merge method.\n>\n> Change the behavior so that when \"--3way\" is\n> used, git will always try the three-way merge\n> first and will only fall back to apply_fragments()\n> in caseswhere blobs are not available or some other\n\nMissing SP before two words.\n\n> error (but not in the case of a merge conflict).\n\nWe may want to note a possible backward compatibility fallout to\nwarn reviewers here in the proposed log message.\n\n>  -3::\n>  --3way::\n> +\tAttempt 3-way merge if the patch records the identity of blobs it is supposed\n> +\tto apply to and we have those blobs available locally, possibly leaving the\n>  \tconflict markers in the files in the working tree for the user to\n>  \tresolve.  This option implies the `--index` option, and is incompatible\n>  \twith the `--reject` and the `--cached` options.\n\nOK.  This patch obviously expects it to graduate before the other\n\"--3way and --cached at the same time\" patch.\n\n> diff --git a/apply.c b/apply.c\n> index 6695a931e979a968b28af88d425d0c76ba17d0d4..62d65ef8d9c0b68857db55198c73db1f41589df1 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3569,10 +3569,10 @@ static int try_threeway(struct apply_state *state,\n>  \t\twrite_object_file(\"\", 0, blob_type, &pre_oid);\n>  \telse if (get_oid(patch->old_oid_prefix, &pre_oid) ||\n>  \t\t read_blob_object(&buf, &pre_oid, patch->old_mode))\n> -\t\treturn error(_(\"repository lacks the necessary blob to fall back on 3-way merge.\"));\n> +\t\treturn error(_(\"repository lacks the necessary blob to do 3-way merge.\"));\n\ns/do/perform/ perhaps?\n\n> @@ -3637,10 +3637,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n>  \tif (load_preimage(state, &image, patch, st, ce) < 0)\n>  \t\treturn -1;\n>  \n> -\tif (patch->direct_to_threeway ||\n> -\t    apply_fragments(state, &image, patch) < 0) {\n\nThe original was \"If the logic flow that came before us already\ndecided we should skip the straight application of the patch and\njump directly to the three-way codepath.  Otherwise try the straight\napplication and perform 3-way only when it fails\".\n\nThe \"direct-to-threeway\" logic was introduced by 099f3c42 (apply:\n--3way with add/add conflict, 2012-06-07).\n\n> +\tif (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n>  \t\t/* Note: with --reject, apply_fragments() returns 0 */\n> -\t\tif (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0)\n> +\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)\n>  \t\t\treturn -1;\n\nThis says something different.  \"If 3-way was not asked, jump\ndirectly to inside the block.  Otherwise, try 3-way first, and go\ninside the block only if 3-way did not work.\"  And the inside the\nblock is the straight patch application.  It says \"if we have\nalready decided we should do the 3-way and nothing else, just fail.\nOtherwise try the straight patch application and if it fails, then\nfail the whole thing.\"\n\nThis looks like a correct \"inversion\" of the fallback codepath.\n\n> @@ -5017,7 +5016,7 @@ int apply_parse_options(int argc, const char **argv,\n>  \t\tOPT_BOOL(0, \"apply\", force_apply,\n>  \t\t\tN_(\"also apply the patch (use with --stat/--summary/--check)\")),\n>  \t\tOPT_BOOL('3', \"3way\", &state->threeway,\n> -\t\t\t N_( \"attempt three-way merge if a patch does not apply\")),\n> +\t\t\t N_( \"attempt three-way merge, fall back on normal patch if that fails\")),\n\nOK.\n\nOverall, the change is very cleanly done.\n\nWill queue.  Thanks.\n\n\n"},{"id":"421046","messageId":"xmqq7dlg2b3d.fsf@gitster.g","threadId":"55439","inReplyTo":"20210406025551.25213-1-jerry@skydio.com","subject":"Re: [PATCH] git-apply: try threeway first when \"--3way\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-06T06:14:46Z","receivedAt":"2021-04-06T06:14:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index d62db3fbe16f35a625a4a14eebb70034f695d3eb..0a7332fed5f60a8a2c9c25fc6713d513c3f0ace1 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -160,4 +160,24 @@ test_expect_success 'apply -3 with add/add conflict (dirty working tree)' '\n>  \ttest_cmp three.save three\n>  '\n>  \n> +test_expect_success 'apply -3 with ambiguous repeating file' '\n> +\tgit reset --hard &&\n> +\ttest_write_lines 1 2 1 2 1 2 1 2 1 2 1>one_two_repeat &&\n\nMissing SP before '>' (same issue on other redirections below).\n\n> +\tgit add one_two_repeat &&\n> +\tgit commit -m \"init one\" &&\n> +\ttest_write_lines 1 2 1 2 1 2 1 2 one 2 1>one_two_repeat &&\n> +\tgit commit -a -m \"change one\" &&\n> +\n> +\tgit diff HEAD~ >Repeat.diff &&\n> +\tgit reset --hard HEAD~ &&\n> +\n> +\ttest_write_lines 1 2 1 2 1 2 one 2 1 2 one>one_two_repeat &&\n> +\tgit commit -a -m \"change surrounding one\" &&\n> +\n> +\tgit apply --index --3way Repeat.diff &&\n> +\ttest_write_lines 1 2 1 2 1 2 one 2 one 2 one>expect &&\n> +\n> +\ttest_cmp expect one_two_repeat\n> +'\n> +\n>  test_done\n"},{"id":"421101","messageId":"xmqqk0pf0zxo.fsf@gitster.g","threadId":"55439","inReplyTo":"xmqqblas2b52.fsf@gitster.g","subject":"Re: [PATCH] git-apply: try threeway first when \"--3way\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-06T23:13:23Z","receivedAt":"2021-04-06T23:13:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Will queue.  Thanks.\n\nJust to avoid confusion, \"Will queue\" does not mean \"No further\nupdates are necessary from you\".\n\nIt is a short-hand to say \"Even though the version I just reviewed\nmay still want to be improved, it is in good enough shape to be\ntested with other topics on the 'seen' branch, so I'll do so\nprimarily to see if there are any funny interactions with them\".\n\nIOW, I expect the patch to be rerolled before it is ready to be\nmerged to 'next' and below.\n\nThanks.\n\n"},{"id":"421103","messageId":"20210406232532.3543-1-jerry@skydio.com","threadId":"55439","inReplyTo":"20210406025551.25213-1-jerry@skydio.com","subject":"[PATCH] git-apply: try threeway first when \"--3way\" is used","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-06T23:25:32Z","receivedAt":"2021-04-06T23:25:38Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"The apply_fragments() method of \"git apply\"\ncan silently apply patches incorrectly if\na file has repeating contents. In these\ncases a three-way merge is capable of applying\nit correctly in more situations, and will\nshow a conflict rather than applying it\nincorrectly. However, because the patches\napply \"successfully\" using apply_fragments(),\ngit will never fall back to the merge, even\nif the \"--3way\" flag is used, and the user has\nno way to ensure correctness by forcing the\nthree-way merge method.\n\nChange the behavior so that when \"--3way\" is used,\ngit will always try the three-way merge first and\nwill only fall back to apply_fragments() in cases\nwhere blobs are not available or some other error\n(but not in the case of a merge conflict).\n\nSince user-facing results will be different,\nthis has backwards compatibility implications\nfor users depending on the old behavior. In\naddition, the three-way merge will be slower\nthan direct patch application.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\n Documentation/git-apply.txt |  5 ++---\n apply.c                     | 13 ++++++-------\n t/t4108-apply-threeway.sh   | 20 ++++++++++++++++++++\n 3 files changed, 28 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 91d9a8601c8c316d4649c405af42e531c39991a8..9144575299c264dd299b542b7b5948eef35f211c 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -84,9 +84,8 @@ OPTIONS\n \n -3::\n --3way::\n-\tWhen the patch does not apply cleanly, fall back on 3-way merge if\n-\tthe patch records the identity of blobs it is supposed to apply to,\n-\tand we have those blobs available locally, possibly leaving the\n+\tAttempt 3-way merge if the patch records the identity of blobs it is supposed\n+\tto apply to and we have those blobs available locally, possibly leaving the\n \tconflict markers in the files in the working tree for the user to\n \tresolve.  This option implies the `--index` option, and is incompatible\n \twith the `--reject` and the `--cached` options.\ndiff --git a/apply.c b/apply.c\nindex 6695a931e979a968b28af88d425d0c76ba17d0d4..9bd4efcbced842d2c5c030a0f2178ddb36114600 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3569,10 +3569,10 @@ static int try_threeway(struct apply_state *state,\n \t\twrite_object_file(\"\", 0, blob_type, &pre_oid);\n \telse if (get_oid(patch->old_oid_prefix, &pre_oid) ||\n \t\t read_blob_object(&buf, &pre_oid, patch->old_mode))\n-\t\treturn error(_(\"repository lacks the necessary blob to fall back on 3-way merge.\"));\n+\t\treturn error(_(\"repository lacks the necessary blob to perform 3-way merge.\"));\n \n \tif (state->apply_verbosity > verbosity_silent)\n-\t\tfprintf(stderr, _(\"Falling back to three-way merge...\\n\"));\n+\t\tfprintf(stderr, _(\"Performing three-way merge...\\n\"));\n \n \timg = strbuf_detach(&buf, &len);\n \tprepare_image(&tmp_image, img, len, 1);\n@@ -3604,7 +3604,7 @@ static int try_threeway(struct apply_state *state,\n \tif (status < 0) {\n \t\tif (state->apply_verbosity > verbosity_silent)\n \t\t\tfprintf(stderr,\n-\t\t\t\t_(\"Failed to fall back on three-way merge...\\n\"));\n+\t\t\t\t_(\"Failed to perform three-way merge...\\n\"));\n \t\treturn status;\n \t}\n \n@@ -3637,10 +3637,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \tif (load_preimage(state, &image, patch, st, ce) < 0)\n \t\treturn -1;\n \n-\tif (patch->direct_to_threeway ||\n-\t    apply_fragments(state, &image, patch) < 0) {\n+\tif (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n \t\t/* Note: with --reject, apply_fragments() returns 0 */\n-\t\tif (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0)\n+\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)\n \t\t\treturn -1;\n \t}\n \tpatch->result = image.buf;\n@@ -5017,7 +5016,7 @@ int apply_parse_options(int argc, const char **argv,\n \t\tOPT_BOOL(0, \"apply\", force_apply,\n \t\t\tN_(\"also apply the patch (use with --stat/--summary/--check)\")),\n \t\tOPT_BOOL('3', \"3way\", &state->threeway,\n-\t\t\t N_( \"attempt three-way merge if a patch does not apply\")),\n+\t\t\t N_( \"attempt three-way merge, fall back on normal patch if that fails\")),\n \t\tOPT_FILENAME(0, \"build-fake-ancestor\", &state->fake_ancestor,\n \t\t\tN_(\"build a temporary index based on embedded index information\")),\n \t\t/* Think twice before adding \"--nul\" synonym to this */\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex d62db3fbe16f35a625a4a14eebb70034f695d3eb..9ff313f976422f9c12dc8032d14567b54cfe3765 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -160,4 +160,24 @@ test_expect_success 'apply -3 with add/add conflict (dirty working tree)' '\n \ttest_cmp three.save three\n '\n \n+test_expect_success 'apply -3 with ambiguous repeating file' '\n+\tgit reset --hard &&\n+\ttest_write_lines 1 2 1 2 1 2 1 2 1 2 1 >one_two_repeat &&\n+\tgit add one_two_repeat &&\n+\tgit commit -m \"init one\" &&\n+\ttest_write_lines 1 2 1 2 1 2 1 2 one 2 1 >one_two_repeat &&\n+\tgit commit -a -m \"change one\" &&\n+\n+\tgit diff HEAD~ >Repeat.diff &&\n+\tgit reset --hard HEAD~ &&\n+\n+\ttest_write_lines 1 2 1 2 1 2 one 2 1 2 one >one_two_repeat &&\n+\tgit commit -a -m \"change surrounding one\" &&\n+\n+\tgit apply --index --3way Repeat.diff &&\n+\ttest_write_lines 1 2 1 2 1 2 one 2 one 2 one >expect &&\n+\n+\ttest_cmp expect one_two_repeat\n+'\n+\n test_done\n-- \n2.29.0\n\n"},{"id":"421105","messageId":"xmqqczv70wvy.fsf@gitster.g","threadId":"55439","inReplyTo":"20210406232532.3543-1-jerry@skydio.com","subject":"Re: [PATCH] git-apply: try threeway first when \"--3way\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-07T00:19:13Z","receivedAt":"2021-04-07T00:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n> Subject: Re: [PATCH] git-apply: try threeway first when \"--3way\" is used\n\nJust for future reference, it is customery to start with [PATCH v2],\n[PATCH v3], etc. when sending an updated patch to make sure it is\nobvious to readers of the list which one is the latest.\n\n> The apply_fragments() method of \"git apply\" can silently apply\n> patches incorrectly if a file has repeating contents. In these\n> cases a three-way merge is capable of applying it correctly in\n> more situations, and will show a conflict rather than applying it\n> incorrectly. However, because the patches apply \"successfully\"\n> using apply_fragments(), git will never fall back to the merge,\n> even if the \"--3way\" flag is used, and the user has no way to\n> ensure correctness by forcing the three-way merge method.\n\nI think this version addresses all issues I noticed in the previous\nversion.  Unless somebody else finds some more issues in a coming\nfew days, let's declare victory and merge it down to 'next'.\n\nBy the way, as my last response bounced for the address\nbrian.kubisiak@skydio.com you had on the CC list, I'm excluding it\nfrom the Cc list of this message.\n\nThanks.\n"}]}