{"thread":{"id":"56171","subject":"[PATCH] git-apply: fix --3way with binary patch","startedAt":"2021-07-28T02:45:51Z","lastAt":"2021-09-07T20:15:40Z","messageCount":16,"participants":["Jerry Zhang","Junio C Hamano","Elijah Newren","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"431376","messageId":"20210728024434.20230-1-jerry@skydio.com","threadId":"56171","inReplyTo":null,"subject":"[PATCH] git-apply: fix --3way with binary patch","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-07-28T02:44:34Z","receivedAt":"2021-07-28T02:45:51Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Binary patches applied with \"--3way\" will\nalways return a conflict even if the patch\nshould cleanly apply because the low level\nmerge function considers all binary merges\nwithout a variant to be conflicting.\n\nFix by falling back to normal patch application\nfor all binary patches.\n\nAdd tests for --3way and normal applications\nof binary patches.\n\nFixes: 923cd87ac8 (\"git-apply: try threeway first when \"--3way\" is used\")\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\n apply.c                   |  3 ++-\n t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 1d2d7e124e..78e52f0dc1 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3638,7 +3638,8 @@ 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 (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n+\tif (!state->threeway || patch->is_binary ||\n+\t\ttry_threeway(state, &image, patch, st, ce) < 0) {\n \t\tif (state->apply_verbosity > verbosity_silent &&\n \t\t    state->threeway && !patch->direct_to_threeway)\n \t\t\tfprintf(stderr, _(\"Falling back to direct application...\\n\"));\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex 65147efdea..d32748f899 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n \ttest_cmp expect.diff actual.diff\n '\n \n+test_expect_success 'apply binary file patch' '\n+\tgit reset --hard main &&\n+\tcp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp $TEST_DIRECTORY/test-binary-2.png bin.png &&\n+\n+\tgit diff --binary >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply bin.diff\n+'\n+\n+test_expect_success 'apply binary file patch with 3way' '\n+\tgit reset --hard main &&\n+\tcp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp $TEST_DIRECTORY/test-binary-2.png bin.png &&\n+\n+\tgit diff --binary >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply --3way --index bin.diff\n+'\n+\n+test_expect_success 'apply full-index patch with 3way' '\n+\tgit reset --hard main &&\n+\tcp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp $TEST_DIRECTORY/test-binary-2.png bin.png &&\n+\n+\tgit diff --full-index >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply --3way --index bin.diff\n+'\n+\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"431380","messageId":"xmqqim0vawof.fsf@gitster.g","threadId":"56171","inReplyTo":"20210728024434.20230-1-jerry@skydio.com","subject":"Re: [PATCH] git-apply: fix --3way with binary patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T04:29:04Z","receivedAt":"2021-07-28T04:29:10Z","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 65147efdea..d32748f899 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n>  \ttest_cmp expect.diff actual.diff\n>  '\n>  \n> +test_expect_success 'apply binary file patch' '\n> +\tgit reset --hard main &&\n> +\tcp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n\nIs it safe to use $TEST_DIRECTORY without quoting?  I doubt it, as\nit is $(pwd) of whereever the testing user extracted our source\ntarball.  \n\nIn other words, you'd need this.\n\ndiff --git w/t/t4108-apply-threeway.sh c/t/t4108-apply-threeway.sh\nindex d32748f899..cc3aa3314a 100755\n--- w/t/t4108-apply-threeway.sh\n+++ c/t/t4108-apply-threeway.sh\n@@ -232,11 +232,11 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n \n test_expect_success 'apply binary file patch' '\n \tgit reset --hard main &&\n-\tcp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n \tgit add bin.png &&\n \tgit commit -m \"add binary file\" &&\n \n-\tcp $TEST_DIRECTORY/test-binary-2.png bin.png &&\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n \n \tgit diff --binary >bin.diff &&\n \tgit reset --hard &&\n@@ -247,11 +247,11 @@ test_expect_success 'apply binary file patch' '\n \n test_expect_success 'apply binary file patch with 3way' '\n \tgit reset --hard main &&\n-\tcp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n \tgit add bin.png &&\n \tgit commit -m \"add binary file\" &&\n \n-\tcp $TEST_DIRECTORY/test-binary-2.png bin.png &&\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n \n \tgit diff --binary >bin.diff &&\n \tgit reset --hard &&\n@@ -262,11 +262,11 @@ test_expect_success 'apply binary file patch with 3way' '\n \n test_expect_success 'apply full-index patch with 3way' '\n \tgit reset --hard main &&\n-\tcp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n \tgit add bin.png &&\n \tgit commit -m \"add binary file\" &&\n \n-\tcp $TEST_DIRECTORY/test-binary-2.png bin.png &&\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n \n \tgit diff --full-index >bin.diff &&\n \tgit reset --hard &&\n"},{"id":"431381","messageId":"xmqqh7gfawlt.fsf@gitster.g","threadId":"56171","inReplyTo":"20210728024434.20230-1-jerry@skydio.com","subject":"Re: [PATCH] git-apply: fix --3way with binary patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T04:30:38Z","receivedAt":"2021-07-28T04:30:42Z","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> Binary patches applied with \"--3way\" will\n> always return a conflict even if the patch\n> should cleanly apply because the low level\n> merge function considers all binary merges\n> without a variant to be conflicting.\n>\n> Fix by falling back to normal patch application\n> for all binary patches.\n>\n> Add tests for --3way and normal applications\n> of binary patches.\n>\n> Fixes: 923cd87ac8 (\"git-apply: try threeway first when \"--3way\" is used\")\n> Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> ---\n>  apply.c                   |  3 ++-\n>  t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 47 insertions(+), 1 deletion(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 1d2d7e124e..78e52f0dc1 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3638,7 +3638,8 @@ 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 (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n> +\tif (!state->threeway || patch->is_binary ||\n> +\t\ttry_threeway(state, &image, patch, st, ce) < 0) {\n\nThanks for a quick turnaround.  However.\n\nBecause apply.c::three_way_merge() calls into ll_merge() that lets\nthe low-level custom merge drivers to take over the actual merge, I\ndo not think your \"if binary, bypass and never call try_threway() at\nall\" is the right solution.  The custom merge driver user uses for\nthe path may successfully perform such a \"trivial\" three-way merge\nand return success.\n\nWhy does the current code that lets threeway tried first fails to\nfall back to direct application?  The code before your change, if\nfed a binary patch that does not apply, would have failed the direct\napplication first *and* then fell back to the threeway (if only to\nfail because we do not let binary files be merged), no?\n\nIs it that try_threeway()'s way to express failure slightly\ndifferent from how direct application reports failure, but your\nchange used the same \"only if it is negative, we fail and fallback\"\nlogic?  IIRC, apply_fragments() which is the meat of the direct\napplication logic reports failures by negative, but try_threeway()\ncan return positive non-zero to signal a \"recoverable\" failure (aka\n\"conflicted merge\").  Which should lead us to explore a different\napproach, which is ...\n\n    Would it be possible for a patch to leave conflicts when\n    try_threeway() was attempted, but will cleanly apply if direct\n    application is done?\n\nIf so, perhaps\n\n - we first run try_threeway() and see if it cleanly resolves; if\n   so, we are done.\n\n - then we try direct application and see if it cleanly applies; if\n   so, we are done.\n\n - finally we run try_threeway() again and let it fail with\n   conflict.\n\nmight be the right sequence?  We theoretically could omit the first\nof these three steps, but that would mean we'd write 923cd87a\n(git-apply: try threeway first when \"--3way\" is used, 2021-04-06)\noff as a failed experiment and revert it, which would not be ideal.\n\n\nAlso, independent from this \"if we claim we try threeway first and\nfall back to direct application, we really should do so\" fix we are\ndiscussing, I think our default binary merge can be a bit more\nlenient and resolve this particular case of applying the binary\npatch taken from itself (i.e. a patch that takes A to B gets applied\nusing --3way option to A).  I wonder if it can be as simple as the\nattached patch.  FWIW, this change is sufficient (without the change\nto apply.c we are reviewing here) to make your new tests in t4108\npass.\n\n---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\nSubject: ll-merge: teach ll_binary_merge() a trivial three-way merge\n\nThe low-level binary merge code assumed that the caller will not\nfeed trivial merges that would have been resolved at the tree level;\nbecause of this, ll_binary_merge() assumes the ancestor is different\nfrom either side, always failing the merge in conflict unless -Xours\nor -Xtheirs is in effect.\n\nBut \"git apply --3way\" codepath could ask us to perform three-way\nmerge between two binaries A and B using A as the ancestor version.\nThe current code always fails such an application, but when given a\nbinary patch that turns A into B and asked to apply it to A, there\nis no reason to fail such a request---we can trivially tell that the\nresult must be B.\n\nArguably, this fix may belong to one level higher at ll_merge()\nfunction, which dispatches to lower-level merge drivers, possibly\neven before it renormalizes the three input buffers.  But let's\nfirst see how this goes.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n ll-merge.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 39 insertions(+), 17 deletions(-)\n\ndiff --git c/ll-merge.c w/ll-merge.c\nindex 261657578c..bc8038d404 100644\n--- c/ll-merge.c\n+++ w/ll-merge.c\n@@ -46,6 +46,13 @@ void reset_merge_attributes(void)\n \tmerge_attributes = NULL;\n }\n \n+static int same_mmfile(mmfile_t *a, mmfile_t *b)\n+{\n+\tif (a->size != b->size)\n+\t\treturn 0;\n+\treturn !memcmp(a->ptr, b->ptr, a->size);\n+}\n+\n /*\n  * Built-in low-levels\n  */\n@@ -58,9 +65,18 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t\t\t   const struct ll_merge_options *opts,\n \t\t\t   int marker_size)\n {\n+\tint status;\n \tmmfile_t *stolen;\n \tassert(opts);\n \n+\t/*\n+\t * With -Xtheirs or -Xours, we have cleanly merged;\n+\t * otherwise we got a conflict, unless 3way trivially\n+\t * resolves.\n+\t */\n+\tstatus = (opts->variant == XDL_MERGE_FAVOR_OURS ||\n+\t\t  opts->variant == XDL_MERGE_FAVOR_THEIRS) ? 0 : 1;\n+\n \t/*\n \t * The tentative merge result is the common ancestor for an\n \t * internal merge.  For the final merge, it is \"ours\" by\n@@ -68,18 +84,30 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t */\n \tif (opts->virtual_ancestor) {\n \t\tstolen = orig;\n+\t\tstatus = 0;\n \t} else {\n-\t\tswitch (opts->variant) {\n-\t\tdefault:\n-\t\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n-\t\t\t\tpath, name1, name2);\n-\t\t\t/* fallthru */\n-\t\tcase XDL_MERGE_FAVOR_OURS:\n-\t\t\tstolen = src1;\n-\t\t\tbreak;\n-\t\tcase XDL_MERGE_FAVOR_THEIRS:\n+\t\tif (same_mmfile(orig, src1)) {\n \t\t\tstolen = src2;\n-\t\t\tbreak;\n+\t\t\tstatus = 0;\n+\t\t} else if (same_mmfile(orig, src2)) { \n+\t\t\tstolen = src1;\n+\t\t\tstatus = 0;\n+\t\t} else if (same_mmfile(src1, src2)) {\n+\t\t\tstolen = src1;\n+\t\t\tstatus = 0;\n+\t\t} else {\n+\t\t\tswitch (opts->variant) {\n+\t\t\tdefault:\n+\t\t\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n+\t\t\t\t\tpath, name1, name2);\n+\t\t\t\t/* fallthru */\n+\t\t\tcase XDL_MERGE_FAVOR_OURS:\n+\t\t\t\tstolen = src1;\n+\t\t\t\tbreak;\n+\t\t\tcase XDL_MERGE_FAVOR_THEIRS:\n+\t\t\t\tstolen = src2;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \t}\n \n@@ -87,13 +115,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \tresult->size = stolen->size;\n \tstolen->ptr = NULL;\n \n-\t/*\n-\t * With -Xtheirs or -Xours, we have cleanly merged;\n-\t * otherwise we got a conflict.\n-\t */\n-\treturn opts->variant == XDL_MERGE_FAVOR_OURS ||\n-\t       opts->variant == XDL_MERGE_FAVOR_THEIRS ?\n-\t       0 : 1;\n+\treturn status;\n }\n \n static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n"},{"id":"431413","messageId":"xmqqeebi9vd0.fsf_-_@gitster.g","threadId":"56171","inReplyTo":"xmqqh7gfawlt.fsf@gitster.g","subject":"[PATCH] ll-merge: teach ll_binary_merge() a trivial three-way merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T17:55:07Z","receivedAt":"2021-07-28T17:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The low-level binary merge code assumed that the caller will not\nfeed trivial merges that would have been resolved at the tree level;\nbecause of this, ll_binary_merge() assumes the ancestor is different\nfrom either side, always failing the merge in conflict unless -Xours\nor -Xtheirs is in effect.\n\nBut \"git apply --3way\" codepath could ask us to perform three-way\nmerge between two binaries A and B using A as the ancestor version.\nThe current code always fails such an application, but when given a\nbinary patch that turns A into B and asked to apply it to A, there\nis no reason to fail such a request---we can trivially tell that the\nresult must be B.\n\nArguably, this fix may belong to one level higher at ll_merge()\nfunction, which dispatches to lower-level merge drivers, possibly\neven before it renormalizes the three input buffers.  But let's\nfirst see how this goes.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n[jc: stolen new tests from Jerry's patch]\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This time as a proper patch form.  I am asking Elijah's input as\n   I suspect this belongs to ll_merge() layer and it may impact not\n   just \"apply --3way\" codepath (which is the primary intended user\n   of this \"feature\") but the merge strategies.  On the other hand,\n   properly written merge strategies would not pass trivial merges\n   down to the low-level backends, so it may not matter much to,\n   say, \"merge -sort\" and friends.\n\n ll-merge.c                | 56 +++++++++++++++++++++++++++------------\n t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++\n 2 files changed, 84 insertions(+), 17 deletions(-)\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 261657578c..301e244971 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -46,6 +46,13 @@ void reset_merge_attributes(void)\n \tmerge_attributes = NULL;\n }\n \n+static int same_mmfile(mmfile_t *a, mmfile_t *b)\n+{\n+\tif (a->size != b->size)\n+\t\treturn 0;\n+\treturn !memcmp(a->ptr, b->ptr, a->size);\n+}\n+\n /*\n  * Built-in low-levels\n  */\n@@ -58,9 +65,18 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t\t\t   const struct ll_merge_options *opts,\n \t\t\t   int marker_size)\n {\n+\tint status;\n \tmmfile_t *stolen;\n \tassert(opts);\n \n+\t/*\n+\t * With -Xtheirs or -Xours, we have cleanly merged;\n+\t * otherwise we got a conflict, unless 3way trivially\n+\t * resolves.\n+\t */\n+\tstatus = (opts->variant == XDL_MERGE_FAVOR_OURS ||\n+\t\t  opts->variant == XDL_MERGE_FAVOR_THEIRS) ? 0 : 1;\n+\n \t/*\n \t * The tentative merge result is the common ancestor for an\n \t * internal merge.  For the final merge, it is \"ours\" by\n@@ -68,18 +84,30 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t */\n \tif (opts->virtual_ancestor) {\n \t\tstolen = orig;\n+\t\tstatus = 0;\n \t} else {\n-\t\tswitch (opts->variant) {\n-\t\tdefault:\n-\t\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n-\t\t\t\tpath, name1, name2);\n-\t\t\t/* fallthru */\n-\t\tcase XDL_MERGE_FAVOR_OURS:\n-\t\t\tstolen = src1;\n-\t\t\tbreak;\n-\t\tcase XDL_MERGE_FAVOR_THEIRS:\n+\t\tif (same_mmfile(orig, src1)) {\n \t\t\tstolen = src2;\n-\t\t\tbreak;\n+\t\t\tstatus = 0;\n+\t\t} else if (same_mmfile(orig, src2)) {\n+\t\t\tstolen = src1;\n+\t\t\tstatus = 0;\n+\t\t} else if (same_mmfile(src1, src2)) {\n+\t\t\tstolen = src1;\n+\t\t\tstatus = 0;\n+\t\t} else {\n+\t\t\tswitch (opts->variant) {\n+\t\t\tdefault:\n+\t\t\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n+\t\t\t\t\tpath, name1, name2);\n+\t\t\t\t/* fallthru */\n+\t\t\tcase XDL_MERGE_FAVOR_OURS:\n+\t\t\t\tstolen = src1;\n+\t\t\t\tbreak;\n+\t\t\tcase XDL_MERGE_FAVOR_THEIRS:\n+\t\t\t\tstolen = src2;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \t}\n \n@@ -87,13 +115,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \tresult->size = stolen->size;\n \tstolen->ptr = NULL;\n \n-\t/*\n-\t * With -Xtheirs or -Xours, we have cleanly merged;\n-\t * otherwise we got a conflict.\n-\t */\n-\treturn opts->variant == XDL_MERGE_FAVOR_OURS ||\n-\t       opts->variant == XDL_MERGE_FAVOR_THEIRS ?\n-\t       0 : 1;\n+\treturn status;\n }\n \n static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex 65147efdea..cc3aa3314a 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n \ttest_cmp expect.diff actual.diff\n '\n \n+test_expect_success 'apply binary file patch' '\n+\tgit reset --hard main &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n+\n+\tgit diff --binary >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply bin.diff\n+'\n+\n+test_expect_success 'apply binary file patch with 3way' '\n+\tgit reset --hard main &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n+\n+\tgit diff --binary >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply --3way --index bin.diff\n+'\n+\n+test_expect_success 'apply full-index patch with 3way' '\n+\tgit reset --hard main &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n+\n+\tgit diff --full-index >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply --3way --index bin.diff\n+'\n+\n test_done\n-- \n2.32.0-561-g6177dfa0d2\n\n"},{"id":"431429","messageId":"CAMKO5CvZCMHuzRLSs2aHJ3iUH-LBJfFP3fG+GgwtQvsKQPtT5Q@mail.gmail.com","threadId":"56171","inReplyTo":"xmqqh7gfawlt.fsf@gitster.g","subject":"Re: [PATCH] git-apply: fix --3way with binary patch","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-07-28T19:38:20Z","receivedAt":"2021-07-28T19:38:34Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Tue, Jul 27, 2021 at 9:30 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jerry Zhang <jerry@skydio.com> writes:\n>\n> > Binary patches applied with \"--3way\" will\n> > always return a conflict even if the patch\n> > should cleanly apply because the low level\n> > merge function considers all binary merges\n> > without a variant to be conflicting.\n> >\n> > Fix by falling back to normal patch application\n> > for all binary patches.\n> >\n> > Add tests for --3way and normal applications\n> > of binary patches.\n> >\n> > Fixes: 923cd87ac8 (\"git-apply: try threeway first when \"--3way\" is used\")\n> > Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> > ---\n> >  apply.c                   |  3 ++-\n> >  t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 47 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/apply.c b/apply.c\n> > index 1d2d7e124e..78e52f0dc1 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -3638,7 +3638,8 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n> >       if (load_preimage(state, &image, patch, st, ce) < 0)\n> >               return -1;\n> >\n> > -     if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n> > +     if (!state->threeway || patch->is_binary ||\n> > +             try_threeway(state, &image, patch, st, ce) < 0) {\n>\n> Thanks for a quick turnaround.  However.\n>\n> Because apply.c::three_way_merge() calls into ll_merge() that lets\n> the low-level custom merge drivers to take over the actual merge, I\n> do not think your \"if binary, bypass and never call try_threway() at\n> all\" is the right solution.  The custom merge driver user uses for\n> the path may successfully perform such a \"trivial\" three-way merge\n> and return success.\nI understand now, thanks for the explanation\n>\n> Why does the current code that lets threeway tried first fails to\n> fall back to direct application?  The code before your change, if\n> fed a binary patch that does not apply, would have failed the direct\n> application first *and* then fell back to the threeway (if only to\n> fail because we do not let binary files be merged), no?\n>\n> Is it that try_threeway()'s way to express failure slightly\n> different from how direct application reports failure, but your\n> change used the same \"only if it is negative, we fail and fallback\"\n> logic?  IIRC, apply_fragments() which is the meat of the direct\n> application logic reports failures by negative, but try_threeway()\n> can return positive non-zero to signal a \"recoverable\" failure (aka\n> \"conflicted merge\").  Which should lead us to explore a different\n> approach, which is ...\n>\n>     Would it be possible for a patch to leave conflicts when\n>     try_threeway() was attempted, but will cleanly apply if direct\n>     application is done?\n>\n> If so, perhaps\n>\n>  - we first run try_threeway() and see if it cleanly resolves; if\n>    so, we are done.\n>\n>  - then we try direct application and see if it cleanly applies; if\n>    so, we are done.\n>\n>  - finally we run try_threeway() again and let it fail with\n>    conflict.\n>\n> might be the right sequence?  We theoretically could omit the first\n> of these three steps, but that would mean we'd write 923cd87a\n> (git-apply: try threeway first when \"--3way\" is used, 2021-04-06)\n> off as a failed experiment and revert it, which would not be ideal.\n>\n>\n> Also, independent from this \"if we claim we try threeway first and\n> fall back to direct application, we really should do so\" fix we are\n> discussing, I think our default binary merge can be a bit more\n> lenient and resolve this particular case of applying the binary\n> patch taken from itself (i.e. a patch that takes A to B gets applied\n> using --3way option to A).  I wonder if it can be as simple as the\n> attached patch.  FWIW, this change is sufficient (without the change\n> to apply.c we are reviewing here) to make your new tests in t4108\n> pass.\nSo basically, another way of stating the problem would be that binary\npatches can apply cleanly with direct application in some cases where\nmerge application is not clean. If i understand correctly this is unique\nto binary files, although it would be possible for a user to supply a custom\nmerge driver for text files that is worse than direct application, that is\nmost likely heavy user error that we shouldn't have to cater to. However\nthe issue with binary is that the *default* merge driver is actually worse\nthan direct application (in some cases). Therefore our options are\n\n1. do as you suggest and run 3way -> direct -> 3way. I would modify\nthis and say we should only attempt this for binary patches, since a text\nfile that fails 3way would most likely also fail direct, so it would be a waste\nof time to try it. furthermore if we cache results from the first 3way and\nreturn them after attempting direct, it can save us from having to compute\nthe 3way twice, so would be no worse than our current performance.\n\n2. improve the default binary merge driver to be at least as good as direct\napplication. this would allow us to say overall that \"merge drivers should\nbe at least as intelligent as direct patch application\" and would greatly\nsimplify logic in apply.c. Your change is a good first step in allowing it\nto handle more cases. A trivial way to make the binary merge driver\nat least as good as patch application is to generate a patch and apply\nit as part of the merge. I imagine this would have other consequences\nthough as many parts of git use the binary merge driver.\n\nSeparately I think it would be a worthwhile follow-up patch to also handle\ntrivial three-way merges in try_threeway(). This would:\n1. Allow us to compare oid instead of the entire file buffer, which would be\nfaster.\n2. Handle trivial merges of all file types, which would save time.\n\n>\n> ---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\n> Subject: ll-merge: teach ll_binary_merge() a trivial three-way merge\n>\n> The low-level binary merge code assumed that the caller will not\n> feed trivial merges that would have been resolved at the tree level;\n> because of this, ll_binary_merge() assumes the ancestor is different\n> from either side, always failing the merge in conflict unless -Xours\n> or -Xtheirs is in effect.\n>\n> But \"git apply --3way\" codepath could ask us to perform three-way\n> merge between two binaries A and B using A as the ancestor version.\n> The current code always fails such an application, but when given a\n> binary patch that turns A into B and asked to apply it to A, there\n> is no reason to fail such a request---we can trivially tell that the\n> result must be B.\n>\n> Arguably, this fix may belong to one level higher at ll_merge()\n> function, which dispatches to lower-level merge drivers, possibly\n> even before it renormalizes the three input buffers.  But let's\n> first see how this goes.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  ll-merge.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------\n>  1 file changed, 39 insertions(+), 17 deletions(-)\n>\n> diff --git c/ll-merge.c w/ll-merge.c\n> index 261657578c..bc8038d404 100644\n> --- c/ll-merge.c\n> +++ w/ll-merge.c\n> @@ -46,6 +46,13 @@ void reset_merge_attributes(void)\n>         merge_attributes = NULL;\n>  }\n>\n> +static int same_mmfile(mmfile_t *a, mmfile_t *b)\n> +{\n> +       if (a->size != b->size)\n> +               return 0;\n> +       return !memcmp(a->ptr, b->ptr, a->size);\n> +}\n> +\n>  /*\n>   * Built-in low-levels\n>   */\n> @@ -58,9 +65,18 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n>                            const struct ll_merge_options *opts,\n>                            int marker_size)\n>  {\n> +       int status;\n>         mmfile_t *stolen;\n>         assert(opts);\n>\n> +       /*\n> +        * With -Xtheirs or -Xours, we have cleanly merged;\n> +        * otherwise we got a conflict, unless 3way trivially\n> +        * resolves.\n> +        */\n> +       status = (opts->variant == XDL_MERGE_FAVOR_OURS ||\n> +                 opts->variant == XDL_MERGE_FAVOR_THEIRS) ? 0 : 1;\n> +\n>         /*\n>          * The tentative merge result is the common ancestor for an\n>          * internal merge.  For the final merge, it is \"ours\" by\n> @@ -68,18 +84,30 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n>          */\n>         if (opts->virtual_ancestor) {\n>                 stolen = orig;\n> +               status = 0;\n>         } else {\n> -               switch (opts->variant) {\n> -               default:\n> -                       warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> -                               path, name1, name2);\n> -                       /* fallthru */\n> -               case XDL_MERGE_FAVOR_OURS:\n> -                       stolen = src1;\n> -                       break;\n> -               case XDL_MERGE_FAVOR_THEIRS:\n> +               if (same_mmfile(orig, src1)) {\n>                         stolen = src2;\n> -                       break;\n> +                       status = 0;\n> +               } else if (same_mmfile(orig, src2)) {\n> +                       stolen = src1;\n> +                       status = 0;\n> +               } else if (same_mmfile(src1, src2)) {\n> +                       stolen = src1;\n> +                       status = 0;\n> +               } else {\n> +                       switch (opts->variant) {\n> +                       default:\n> +                               warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> +                                       path, name1, name2);\n> +                               /* fallthru */\n> +                       case XDL_MERGE_FAVOR_OURS:\n> +                               stolen = src1;\n> +                               break;\n> +                       case XDL_MERGE_FAVOR_THEIRS:\n> +                               stolen = src2;\n> +                               break;\n> +                       }\n>                 }\n>         }\n>\n> @@ -87,13 +115,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n>         result->size = stolen->size;\n>         stolen->ptr = NULL;\n>\n> -       /*\n> -        * With -Xtheirs or -Xours, we have cleanly merged;\n> -        * otherwise we got a conflict.\n> -        */\n> -       return opts->variant == XDL_MERGE_FAVOR_OURS ||\n> -              opts->variant == XDL_MERGE_FAVOR_THEIRS ?\n> -              0 : 1;\n> +       return status;\n>  }\n>\n>  static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n"},{"id":"431431","messageId":"CAMKO5CsPK65wzkJSYv6dHO4wjsX1+U7yX5P95jS3UHXKThMjqw@mail.gmail.com","threadId":"56171","inReplyTo":"CAMKO5CvZCMHuzRLSs2aHJ3iUH-LBJfFP3fG+GgwtQvsKQPtT5Q@mail.gmail.com","subject":"Re: [PATCH] git-apply: fix --3way with binary patch","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-07-28T20:04:31Z","receivedAt":"2021-07-28T20:04:45Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Wed, Jul 28, 2021 at 12:38 PM Jerry Zhang <jerry@skydio.com> wrote:\n>\n> On Tue, Jul 27, 2021 at 9:30 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Jerry Zhang <jerry@skydio.com> writes:\n> >\n> > > Binary patches applied with \"--3way\" will\n> > > always return a conflict even if the patch\n> > > should cleanly apply because the low level\n> > > merge function considers all binary merges\n> > > without a variant to be conflicting.\n> > >\n> > > Fix by falling back to normal patch application\n> > > for all binary patches.\n> > >\n> > > Add tests for --3way and normal applications\n> > > of binary patches.\n> > >\n> > > Fixes: 923cd87ac8 (\"git-apply: try threeway first when \"--3way\" is used\")\n> > > Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> > > ---\n> > >  apply.c                   |  3 ++-\n> > >  t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++\n> > >  2 files changed, 47 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/apply.c b/apply.c\n> > > index 1d2d7e124e..78e52f0dc1 100644\n> > > --- a/apply.c\n> > > +++ b/apply.c\n> > > @@ -3638,7 +3638,8 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n> > >       if (load_preimage(state, &image, patch, st, ce) < 0)\n> > >               return -1;\n> > >\n> > > -     if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n> > > +     if (!state->threeway || patch->is_binary ||\n> > > +             try_threeway(state, &image, patch, st, ce) < 0) {\n> >\n> > Thanks for a quick turnaround.  However.\n> >\n> > Because apply.c::three_way_merge() calls into ll_merge() that lets\n> > the low-level custom merge drivers to take over the actual merge, I\n> > do not think your \"if binary, bypass and never call try_threway() at\n> > all\" is the right solution.  The custom merge driver user uses for\n> > the path may successfully perform such a \"trivial\" three-way merge\n> > and return success.\n> I understand now, thanks for the explanation\n> >\n> > Why does the current code that lets threeway tried first fails to\n> > fall back to direct application?  The code before your change, if\n> > fed a binary patch that does not apply, would have failed the direct\n> > application first *and* then fell back to the threeway (if only to\n> > fail because we do not let binary files be merged), no?\n> >\n> > Is it that try_threeway()'s way to express failure slightly\n> > different from how direct application reports failure, but your\n> > change used the same \"only if it is negative, we fail and fallback\"\n> > logic?  IIRC, apply_fragments() which is the meat of the direct\n> > application logic reports failures by negative, but try_threeway()\n> > can return positive non-zero to signal a \"recoverable\" failure (aka\n> > \"conflicted merge\").  Which should lead us to explore a different\n> > approach, which is ...\n> >\n> >     Would it be possible for a patch to leave conflicts when\n> >     try_threeway() was attempted, but will cleanly apply if direct\n> >     application is done?\n> >\n> > If so, perhaps\n> >\n> >  - we first run try_threeway() and see if it cleanly resolves; if\n> >    so, we are done.\n> >\n> >  - then we try direct application and see if it cleanly applies; if\n> >    so, we are done.\n> >\n> >  - finally we run try_threeway() again and let it fail with\n> >    conflict.\n> >\n> > might be the right sequence?  We theoretically could omit the first\n> > of these three steps, but that would mean we'd write 923cd87a\n> > (git-apply: try threeway first when \"--3way\" is used, 2021-04-06)\n> > off as a failed experiment and revert it, which would not be ideal.\n> >\n> >\n> > Also, independent from this \"if we claim we try threeway first and\n> > fall back to direct application, we really should do so\" fix we are\n> > discussing, I think our default binary merge can be a bit more\n> > lenient and resolve this particular case of applying the binary\n> > patch taken from itself (i.e. a patch that takes A to B gets applied\n> > using --3way option to A).  I wonder if it can be as simple as the\n> > attached patch.  FWIW, this change is sufficient (without the change\n> > to apply.c we are reviewing here) to make your new tests in t4108\n> > pass.\n> So basically, another way of stating the problem would be that binary\n> patches can apply cleanly with direct application in some cases where\n> merge application is not clean. If i understand correctly this is unique\n> to binary files, although it would be possible for a user to supply a custom\n> merge driver for text files that is worse than direct application, that is\n> most likely heavy user error that we shouldn't have to cater to. However\n> the issue with binary is that the *default* merge driver is actually worse\n> than direct application (in some cases). Therefore our options are\n>\n> 1. do as you suggest and run 3way -> direct -> 3way. I would modify\n> this and say we should only attempt this for binary patches, since a text\n> file that fails 3way would most likely also fail direct, so it would be a waste\n> of time to try it. furthermore if we cache results from the first 3way and\n> return them after attempting direct, it can save us from having to compute\n> the 3way twice, so would be no worse than our current performance.\n>\n> 2. improve the default binary merge driver to be at least as good as direct\n> application. this would allow us to say overall that \"merge drivers should\n> be at least as intelligent as direct patch application\" and would greatly\n> simplify logic in apply.c. Your change is a good first step in allowing it\n> to handle more cases. A trivial way to make the binary merge driver\n> at least as good as patch application is to generate a patch and apply\n> it as part of the merge. I imagine this would have other consequences\n> though as many parts of git use the binary merge driver.\n\nHmm I would have thought that binary patches allow context, similar to this\ntest snippet\n\"\n test_expect_success 'apply complex binary file patch' '\n     git reset --hard main &&\n     cp $TEST_DIRECTORY/test-binary-1.png bin.png &&\n     git add bin.png &&\n     git commit -m \"add binary file\" &&\n\n     echo 1 >>bin.png &&\n     git diff --binary >bin.diff &&\n     git reset --hard &&\n\n     cat $TEST_DIRECTORY/test-binary-2.png\n$TEST_DIRECTORY/test-binary-1.png >bin.png &&\n     git add bin.png &&\n     git commit -m \"change binary file\" &&\n\n     # Apply must succeed.\n     git apply bin.diff\n '\n\"\nbut upon running it I see that normal patch application still requires the\npreimage to match exactly.\n\"\nerror: the patch applies to 'bin.png'\n(836481bd1b9b6bd7a1bb8939cf4ea01e05946850), which does not match the\ncurrent contents.\nerror: bin.png: patch does not apply\n\"\n\nSo at least in regards to making the default binary merge driver \"at\nleast as intelligent\" as\ndirect patch application, your patch ought to do it.\n\n>\n> Separately I think it would be a worthwhile follow-up patch to also handle\n> trivial three-way merges in try_threeway(). This would:\n> 1. Allow us to compare oid instead of the entire file buffer, which would be\n> faster.\n> 2. Handle trivial merges of all file types, which would save time.\n>\n> >\n> > ---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\n> > Subject: ll-merge: teach ll_binary_merge() a trivial three-way merge\n> >\n> > The low-level binary merge code assumed that the caller will not\n> > feed trivial merges that would have been resolved at the tree level;\n> > because of this, ll_binary_merge() assumes the ancestor is different\n> > from either side, always failing the merge in conflict unless -Xours\n> > or -Xtheirs is in effect.\n> >\n> > But \"git apply --3way\" codepath could ask us to perform three-way\n> > merge between two binaries A and B using A as the ancestor version.\n> > The current code always fails such an application, but when given a\n> > binary patch that turns A into B and asked to apply it to A, there\n> > is no reason to fail such a request---we can trivially tell that the\n> > result must be B.\n> >\n> > Arguably, this fix may belong to one level higher at ll_merge()\n> > function, which dispatches to lower-level merge drivers, possibly\n> > even before it renormalizes the three input buffers.  But let's\n> > first see how this goes.\n> >\n> > Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> > ---\n> >  ll-merge.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------\n> >  1 file changed, 39 insertions(+), 17 deletions(-)\n> >\n> > diff --git c/ll-merge.c w/ll-merge.c\n> > index 261657578c..bc8038d404 100644\n> > --- c/ll-merge.c\n> > +++ w/ll-merge.c\n> > @@ -46,6 +46,13 @@ void reset_merge_attributes(void)\n> >         merge_attributes = NULL;\n> >  }\n> >\n> > +static int same_mmfile(mmfile_t *a, mmfile_t *b)\n> > +{\n> > +       if (a->size != b->size)\n> > +               return 0;\n> > +       return !memcmp(a->ptr, b->ptr, a->size);\n> > +}\n> > +\n> >  /*\n> >   * Built-in low-levels\n> >   */\n> > @@ -58,9 +65,18 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n> >                            const struct ll_merge_options *opts,\n> >                            int marker_size)\n> >  {\n> > +       int status;\n> >         mmfile_t *stolen;\n> >         assert(opts);\n> >\n> > +       /*\n> > +        * With -Xtheirs or -Xours, we have cleanly merged;\n> > +        * otherwise we got a conflict, unless 3way trivially\n> > +        * resolves.\n> > +        */\n> > +       status = (opts->variant == XDL_MERGE_FAVOR_OURS ||\n> > +                 opts->variant == XDL_MERGE_FAVOR_THEIRS) ? 0 : 1;\n> > +\n> >         /*\n> >          * The tentative merge result is the common ancestor for an\n> >          * internal merge.  For the final merge, it is \"ours\" by\n> > @@ -68,18 +84,30 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n> >          */\n> >         if (opts->virtual_ancestor) {\n> >                 stolen = orig;\n> > +               status = 0;\n> >         } else {\n> > -               switch (opts->variant) {\n> > -               default:\n> > -                       warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> > -                               path, name1, name2);\n> > -                       /* fallthru */\n> > -               case XDL_MERGE_FAVOR_OURS:\n> > -                       stolen = src1;\n> > -                       break;\n> > -               case XDL_MERGE_FAVOR_THEIRS:\n> > +               if (same_mmfile(orig, src1)) {\n> >                         stolen = src2;\n> > -                       break;\n> > +                       status = 0;\n> > +               } else if (same_mmfile(orig, src2)) {\n> > +                       stolen = src1;\n> > +                       status = 0;\n> > +               } else if (same_mmfile(src1, src2)) {\n> > +                       stolen = src1;\n> > +                       status = 0;\n> > +               } else {\n> > +                       switch (opts->variant) {\n> > +                       default:\n> > +                               warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> > +                                       path, name1, name2);\n> > +                               /* fallthru */\n> > +                       case XDL_MERGE_FAVOR_OURS:\n> > +                               stolen = src1;\n> > +                               break;\n> > +                       case XDL_MERGE_FAVOR_THEIRS:\n> > +                               stolen = src2;\n> > +                               break;\n> > +                       }\n> >                 }\n> >         }\n> >\n> > @@ -87,13 +115,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n> >         result->size = stolen->size;\n> >         stolen->ptr = NULL;\n> >\n> > -       /*\n> > -        * With -Xtheirs or -Xours, we have cleanly merged;\n> > -        * otherwise we got a conflict.\n> > -        */\n> > -       return opts->variant == XDL_MERGE_FAVOR_OURS ||\n> > -              opts->variant == XDL_MERGE_FAVOR_THEIRS ?\n> > -              0 : 1;\n> > +       return status;\n> >  }\n> >\n> >  static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n"},{"id":"431432","messageId":"xmqq1r7i9p7f.fsf@gitster.g","threadId":"56171","inReplyTo":"CAMKO5CvZCMHuzRLSs2aHJ3iUH-LBJfFP3fG+GgwtQvsKQPtT5Q@mail.gmail.com","subject":"Re: [PATCH] git-apply: fix --3way with binary patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T20:08:04Z","receivedAt":"2021-07-28T20:08:10Z","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> So basically, another way of stating the problem would be that binary\n> patches can apply cleanly with direct application in some cases where\n> merge application is not clean. If i understand correctly this is unique\n> to binary files, although it would be possible for a user to supply a custom\n> merge driver for text files that is worse than direct application, that is\n> most likely heavy user error that we shouldn't have to cater to.\n\nNot really.  The built-in binary merge driver luckily had such\ncharacteristics to allow us to catch this regression, but I see no\nreason to believe that it is unique to binary.  Funky merge backends\nlike union merges can turn an otherwise conflicting merge into a\nclean merge even for non-binary files.  And no, it is not an error\nfor a merge driver to fail \"apply --3way\" merge on incoming data\nthat \"apply --no-3way\" would apply cleanly.\n\n> However\n> the issue with binary is that the *default* merge driver is actually worse\n> than direct application (in some cases).\n\n> 1. do as you suggest and run 3way -> direct -> 3way. I would modify\n> this and say we should only attempt this for binary patches, since a text\n> file that fails 3way would most likely also fail direct,...\n\nNo, I do not trust our (myself and your) unsubstantiated belief that\nit is limited to binary.  We saw a problem with binary, and I would\nthink it is a tip of iceberg for any non-straight-text-merge backend\n(and I do not have any sound reason to believe that straight\ntext-merge backend will not have this issue).  I'd rather treat this\nas coalmine canary.\n\nI think the real problem, even without the \"try threeway, fall back\nto direct application, and then try threeway again\", is that after\nswapping the fallback order, a failed threeway does *not* fall back\nto direct application in this case.  Regardless of what ll_merge()\nand its backend does, if they fail, shouldn't the caller of\ntry_threeway() notice the failure and fall back to direct\napplication, just like the earlier code tried direct application\nfirst and then _always_ fell back to threeway if it failed?  I do\nnot know exactly why today's code fails to do so, but I suspect that\nfixing that is the real solution, no?\n\nIndependent from that, I suspect that it may be a good thing to do\nto (at least optionally) allow ll_merge() to notice trivial merges\nthat proper merge frontends would never ask it to do and resolve\nthem trivially.  The patch you saw from me to ll_merge_binary() may\ndo so at a wrong layer (doing it in ll_merge() before it dispatches\nto ll_merge_binary() and other backends might be a better approach)\nbut would be a good starting point for that independent effort, but\n\"apply --3way\" should work correctly even with user-configured merge\ndrivers (after all, the \"direct application first and then fall back\nto 3way\" code would have worked perfectly fine even with broken\ncustom merge drivers in the case we are discussing right now).\n\nThanks.\n\n\n"},{"id":"431433","messageId":"CAMKO5CszNvzd6Y5VdTqw6JDGxOyQ-CNA3fxgf5ChQbGwZ9v_rw@mail.gmail.com","threadId":"56171","inReplyTo":"xmqq1r7i9p7f.fsf@gitster.g","subject":"Re: [PATCH] git-apply: fix --3way with binary patch","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-07-28T20:37:50Z","receivedAt":"2021-07-28T20:38:05Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Wed, Jul 28, 2021 at 1:08 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jerry Zhang <jerry@skydio.com> writes:\n>\n> > So basically, another way of stating the problem would be that binary\n> > patches can apply cleanly with direct application in some cases where\n> > merge application is not clean. If i understand correctly this is unique\n> > to binary files, although it would be possible for a user to supply a custom\n> > merge driver for text files that is worse than direct application, that is\n> > most likely heavy user error that we shouldn't have to cater to.\n>\n> Not really.  The built-in binary merge driver luckily had such\n> characteristics to allow us to catch this regression, but I see no\n> reason to believe that it is unique to binary.  Funky merge backends\n> like union merges can turn an otherwise conflicting merge into a\n> clean merge even for non-binary files.  And no, it is not an error\n> for a merge driver to fail \"apply --3way\" merge on incoming data\n> that \"apply --no-3way\" would apply cleanly.\n>\n> > However\n> > the issue with binary is that the *default* merge driver is actually worse\n> > than direct application (in some cases).\n>\n> > 1. do as you suggest and run 3way -> direct -> 3way. I would modify\n> > this and say we should only attempt this for binary patches, since a text\n> > file that fails 3way would most likely also fail direct,...\n>\n> No, I do not trust our (myself and your) unsubstantiated belief that\n> it is limited to binary.  We saw a problem with binary, and I would\n> think it is a tip of iceberg for any non-straight-text-merge backend\n> (and I do not have any sound reason to believe that straight\n> text-merge backend will not have this issue).  I'd rather treat this\n> as coalmine canary.\n\nI see, this could be true. But then we would also have the case where\na merge driver results in conflict and direct patch application applies cleanly,\n*but* the direct patch application is actually incorrect (for reasons relating\nto the original purpose of the patch to switch the order, that direct patch\napplication can be wrong for files of repeating content). In this case\nthe user might actually want to see the conflict as it is more correct, but\nit is being hidden by our fallback.\n\nIf the backwards compatibility story is going to get messy like this, perhaps\nthe best solution is to make a new flag similar to \"--actually-3way\" that\nwill attempt 3way and nothing else, and users who know what they want\ncan use that to get what they want.\n>\n> I think the real problem, even without the \"try threeway, fall back\n> to direct application, and then try threeway again\", is that after\n> swapping the fallback order, a failed threeway does *not* fall back\n> to direct application in this case.  Regardless of what ll_merge()\n> and its backend does, if they fail, shouldn't the caller of\n> try_threeway() notice the failure and fall back to direct\n> application, just like the earlier code tried direct application\n> first and then _always_ fell back to threeway if it failed?  I do\n> not know exactly why today's code fails to do so, but I suspect that\n> fixing that is the real solution, no?\n\nWell it isn't really failing right? Failing 3way would be not finding\nthe object ids in the database, which would indicate a failure to\neven attempt 3way. This would result in fallback to direct application.\nWhat we're seeing is that 3way application results in conflicts where\ndirect application would not result in conflicts. Having a conflict is\ncurrently not a reason for the code to fall back to direct application,\nhere is the relevant line:\n\"\n         try_threeway(state, &image, patch, st, ce) < 0) {\n\"\ntry_threeway returns 1 in case of conflict, 0 for success, and -1\nfor true errors.\n\n>\n> Independent from that, I suspect that it may be a good thing to do\n> to (at least optionally) allow ll_merge() to notice trivial merges\n> that proper merge frontends would never ask it to do and resolve\n> them trivially.  The patch you saw from me to ll_merge_binary() may\n> do so at a wrong layer (doing it in ll_merge() before it dispatches\n> to ll_merge_binary() and other backends might be a better approach)\n> but would be a good starting point for that independent effort, but\n> \"apply --3way\" should work correctly even with user-configured merge\n> drivers (after all, the \"direct application first and then fall back\n> to 3way\" code would have worked perfectly fine even with broken\n> custom merge drivers in the case we are discussing right now).\n>\n> Thanks.\n>\n>\n"},{"id":"431436","messageId":"xmqqpmv2885j.fsf@gitster.g","threadId":"56171","inReplyTo":"CAMKO5CszNvzd6Y5VdTqw6JDGxOyQ-CNA3fxgf5ChQbGwZ9v_rw@mail.gmail.com","subject":"Re: [PATCH] git-apply: fix --3way with binary patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T21:01:44Z","receivedAt":"2021-07-28T21:01: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> Well it isn't really failing right? Failing 3way would be not finding\n> the object ids in the database, which would indicate a failure to\n> even attempt 3way. This would result in fallback to direct application.\n> What we're seeing is that 3way application results in conflicts where\n> direct application would not result in conflicts. Having a conflict is\n> currently not a reason for the code to fall back to direct application,\n> here is the relevant line:\n> \"\n>          try_threeway(state, &image, patch, st, ce) < 0) {\n> \"\n> try_threeway returns 1 in case of conflict, 0 for success, and -1\n> for true errors.\n\nYup, I know.  That is why I questioned if this \"< 0\" is a bug in my\nearlier message in this exchange.\n\nThanks.\n"},{"id":"431447","messageId":"CABPp-BFh3uV9-X8iaKHA771TUneBDYmOKU5+5y9XsE-11UL7tQ@mail.gmail.com","threadId":"56171","inReplyTo":"xmqqeebi9vd0.fsf_-_@gitster.g","subject":"Re: [PATCH] ll-merge: teach ll_binary_merge() a trivial three-way merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-28T23:49:13Z","receivedAt":"2021-07-28T23:49:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 28, 2021 at 11:55 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> The low-level binary merge code assumed that the caller will not\n> feed trivial merges that would have been resolved at the tree level;\n> because of this, ll_binary_merge() assumes the ancestor is different\n> from either side, always failing the merge in conflict unless -Xours\n> or -Xtheirs is in effect.\n>\n> But \"git apply --3way\" codepath could ask us to perform three-way\n> merge between two binaries A and B using A as the ancestor version.\n> The current code always fails such an application, but when given a\n> binary patch that turns A into B and asked to apply it to A, there\n> is no reason to fail such a request---we can trivially tell that the\n> result must be B.\n>\n> Arguably, this fix may belong to one level higher at ll_merge()\n> function, which dispatches to lower-level merge drivers, possibly\n> even before it renormalizes the three input buffers.  But let's\n> first see how this goes.\n>\n> Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> [jc: stolen new tests from Jerry's patch]\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * This time as a proper patch form.  I am asking Elijah's input as\n>    I suspect this belongs to ll_merge() layer and it may impact not\n>    just \"apply --3way\" codepath (which is the primary intended user\n>    of this \"feature\") but the merge strategies.  On the other hand,\n>    properly written merge strategies would not pass trivial merges\n>    down to the low-level backends, so it may not matter much to,\n>    say, \"merge -sort\" and friends.\n\nThe patch looks correct if we choose to modify the ll_binary_merge level.\n\nI agree that properly written merge strategies (at least both\nmerge-recursive and merge-ort) would not pass trivial merges down to\nthe low-level backends...but I think this change still matters to them\nfrom a performance perspective.  Additional up-front full content\ncomparisons feel like an unnecessary performance penalty, so if we do\nsomething like this, keeping it at the ll_binary_merge() level to\nlimit it to binary files would limit the penalty.  However...\n\nIt appears that try_threeway() in apply.c is already computing the\nOIDs of the blobs involved, so it looks like the full content\ncomparison is unnecessary even in the apply --3way case.  If we moved\nthe trivial-merge check to that function, it could just compare the\nOIDs rather than comparing the full content.\n\n>  ll-merge.c                | 56 +++++++++++++++++++++++++++------------\n>  t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++\n>  2 files changed, 84 insertions(+), 17 deletions(-)\n>\n> diff --git a/ll-merge.c b/ll-merge.c\n> index 261657578c..301e244971 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -46,6 +46,13 @@ void reset_merge_attributes(void)\n>         merge_attributes = NULL;\n>  }\n>\n> +static int same_mmfile(mmfile_t *a, mmfile_t *b)\n> +{\n> +       if (a->size != b->size)\n> +               return 0;\n> +       return !memcmp(a->ptr, b->ptr, a->size);\n> +}\n> +\n>  /*\n>   * Built-in low-levels\n>   */\n> @@ -58,9 +65,18 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n>                            const struct ll_merge_options *opts,\n>                            int marker_size)\n>  {\n> +       int status;\n>         mmfile_t *stolen;\n>         assert(opts);\n>\n> +       /*\n> +        * With -Xtheirs or -Xours, we have cleanly merged;\n> +        * otherwise we got a conflict, unless 3way trivially\n> +        * resolves.\n> +        */\n> +       status = (opts->variant == XDL_MERGE_FAVOR_OURS ||\n> +                 opts->variant == XDL_MERGE_FAVOR_THEIRS) ? 0 : 1;\n> +\n>         /*\n>          * The tentative merge result is the common ancestor for an\n>          * internal merge.  For the final merge, it is \"ours\" by\n> @@ -68,18 +84,30 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n>          */\n>         if (opts->virtual_ancestor) {\n>                 stolen = orig;\n> +               status = 0;\n>         } else {\n> -               switch (opts->variant) {\n> -               default:\n> -                       warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> -                               path, name1, name2);\n> -                       /* fallthru */\n> -               case XDL_MERGE_FAVOR_OURS:\n> -                       stolen = src1;\n> -                       break;\n> -               case XDL_MERGE_FAVOR_THEIRS:\n> +               if (same_mmfile(orig, src1)) {\n>                         stolen = src2;\n> -                       break;\n> +                       status = 0;\n> +               } else if (same_mmfile(orig, src2)) {\n> +                       stolen = src1;\n> +                       status = 0;\n> +               } else if (same_mmfile(src1, src2)) {\n> +                       stolen = src1;\n> +                       status = 0;\n> +               } else {\n> +                       switch (opts->variant) {\n> +                       default:\n> +                               warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> +                                       path, name1, name2);\n> +                               /* fallthru */\n> +                       case XDL_MERGE_FAVOR_OURS:\n> +                               stolen = src1;\n> +                               break;\n> +                       case XDL_MERGE_FAVOR_THEIRS:\n> +                               stolen = src2;\n> +                               break;\n> +                       }\n>                 }\n>         }\n>\n> @@ -87,13 +115,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n>         result->size = stolen->size;\n>         stolen->ptr = NULL;\n>\n> -       /*\n> -        * With -Xtheirs or -Xours, we have cleanly merged;\n> -        * otherwise we got a conflict.\n> -        */\n> -       return opts->variant == XDL_MERGE_FAVOR_OURS ||\n> -              opts->variant == XDL_MERGE_FAVOR_THEIRS ?\n> -              0 : 1;\n> +       return status;\n>  }\n>\n>  static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index 65147efdea..cc3aa3314a 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n>         test_cmp expect.diff actual.diff\n>  '\n>\n> +test_expect_success 'apply binary file patch' '\n> +       git reset --hard main &&\n> +       cp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n> +       git add bin.png &&\n> +       git commit -m \"add binary file\" &&\n> +\n> +       cp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n> +\n> +       git diff --binary >bin.diff &&\n> +       git reset --hard &&\n> +\n> +       # Apply must succeed.\n> +       git apply bin.diff\n> +'\n> +\n> +test_expect_success 'apply binary file patch with 3way' '\n> +       git reset --hard main &&\n> +       cp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n> +       git add bin.png &&\n> +       git commit -m \"add binary file\" &&\n> +\n> +       cp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n> +\n> +       git diff --binary >bin.diff &&\n> +       git reset --hard &&\n> +\n> +       # Apply must succeed.\n> +       git apply --3way --index bin.diff\n> +'\n> +\n> +test_expect_success 'apply full-index patch with 3way' '\n> +       git reset --hard main &&\n> +       cp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n> +       git add bin.png &&\n> +       git commit -m \"add binary file\" &&\n> +\n> +       cp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n> +\n> +       git diff --full-index >bin.diff &&\n> +       git reset --hard &&\n> +\n> +       # Apply must succeed.\n> +       git apply --3way --index bin.diff\n> +'\n> +\n>  test_done\n> --\n> 2.32.0-561-g6177dfa0d2\n"},{"id":"431451","messageId":"xmqqczr26i9f.fsf@gitster.g","threadId":"56171","inReplyTo":"CABPp-BFh3uV9-X8iaKHA771TUneBDYmOKU5+5y9XsE-11UL7tQ@mail.gmail.com","subject":"Re: [PATCH] ll-merge: teach ll_binary_merge() a trivial three-way merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-29T01:06:20Z","receivedAt":"2021-07-29T01:06:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> It appears that try_threeway() in apply.c is already computing the\n> OIDs of the blobs involved, so it looks like the full content\n> comparison is unnecessary even in the apply --3way case.  If we moved\n> the trivial-merge check to that function, it could just compare the\n> OIDs rather than comparing the full content.\n\nYeah, if we trust merge backends and only fix \"apply --3way\"\ncodepath, which I actually am OK with, I agree that it would be\nvastly simpler and nicer to do it in try_threeway().\n\nThanks.\n"},{"id":"434734","messageId":"20210905190657.2906699-1-gitster@pobox.com","threadId":"56171","inReplyTo":"xmqqczr26i9f.fsf@gitster.g","subject":"[PATCH v2] apply: resolve trivial merge without hitting ll-merge with \"--3way\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-05T19:06:57Z","receivedAt":"2021-09-05T19:07:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The ll_binary_merge() function assumes that the ancestor blob is\ndifferent from either side of the new versions, and always fails\nthe merge in conflict, unless -Xours or -Xtheirs is in effect.\n\nThe normal \"merge\" machineries all resolve the trivial cases\n(e.g. if our side changed while their side did not, the result\nis ours) without triggering the file-level merge drivers, so the\nassumption is warranted.\n\nThe code path in \"git apply --3way\", however, does not check for\nthe trivial three-way merge situation and always calls the\nfile-level merge drivers.  This used to be perfectly OK back\nwhen we always first attempted a straight patch application and\nused the three-way code path only as a fallback.  Any binary\npatch that can be applied as a trivial three-way merge (e.g. the\npatch is based exactly on the version we happen to have) would\nalways cleanly apply, so the ll_binary_merge() that is not\nprepared to see the trivial case would not have to handle such a\ncase.\n\nThis no longer is true after we made \"--3way\" to mean \"first try\nthree-way and then fall back to straight application\", and made\n\"git apply -3\" on a binary patch that is based on the current\nversion no longer apply.\n\nTeach \"git apply -3\" to first check for the trivial merge cases\nand resolve them without hitting the file-level merge drivers.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n[jc: stolen tests from Jerry's patch]\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n apply.c                   | 21 ++++++++++++++++++\n t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+)\n\ndiff --git a/apply.c b/apply.c\nindex 44bc31d6eb..c9f9503e90 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3467,6 +3467,21 @@ static int load_preimage(struct apply_state *state,\n \treturn 0;\n }\n \n+static int resolve_to(struct image *image, const struct object_id *result_id)\n+{\n+\tunsigned long size;\n+\tenum object_type type;\n+\n+\tclear_image(image);\n+\n+\timage->buf = read_object_file(result_id, &type, &size);\n+\tif (!image->buf || type != OBJ_BLOB)\n+\t\tdie(\"unable to read blob object %s\", oid_to_hex(result_id));\n+\timage->len = size;\n+\n+\treturn 0;\n+}\n+\n static int three_way_merge(struct apply_state *state,\n \t\t\t   struct image *image,\n \t\t\t   char *path,\n@@ -3478,6 +3493,12 @@ static int three_way_merge(struct apply_state *state,\n \tmmbuffer_t result = { NULL };\n \tint status;\n \n+\t/* resolve trivial cases first */\n+\tif (oideq(base, ours))\n+\t\treturn resolve_to(image, theirs);\n+\telse if (oideq(base, theirs) || oideq(ours, theirs))\n+\t\treturn resolve_to(image, ours);\n+\n \tread_mmblob(&base_file, base);\n \tread_mmblob(&our_file, ours);\n \tread_mmblob(&their_file, theirs);\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex 65147efdea..cc3aa3314a 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n \ttest_cmp expect.diff actual.diff\n '\n \n+test_expect_success 'apply binary file patch' '\n+\tgit reset --hard main &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n+\n+\tgit diff --binary >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply bin.diff\n+'\n+\n+test_expect_success 'apply binary file patch with 3way' '\n+\tgit reset --hard main &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n+\n+\tgit diff --binary >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply --3way --index bin.diff\n+'\n+\n+test_expect_success 'apply full-index patch with 3way' '\n+\tgit reset --hard main &&\n+\tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n+\tgit add bin.png &&\n+\tgit commit -m \"add binary file\" &&\n+\n+\tcp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n+\n+\tgit diff --full-index >bin.diff &&\n+\tgit reset --hard &&\n+\n+\t# Apply must succeed.\n+\tgit apply --3way --index bin.diff\n+'\n+\n test_done\n-- \n2.33.0-408-g8e1aa136b3\n\n"},{"id":"434800","messageId":"CABPp-BGrg7eBCeq7SLvx3N5p7HyKGwS7qwTe=+En6OfiKhiXPQ@mail.gmail.com","threadId":"56171","inReplyTo":"20210905190657.2906699-1-gitster@pobox.com","subject":"Re: [PATCH v2] apply: resolve trivial merge without hitting ll-merge with \"--3way\"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-06T18:57:34Z","receivedAt":"2021-09-06T18:57:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Sep 5, 2021 at 12:07 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> The ll_binary_merge() function assumes that the ancestor blob is\n> different from either side of the new versions, and always fails\n> the merge in conflict, unless -Xours or -Xtheirs is in effect.\n>\n> The normal \"merge\" machineries all resolve the trivial cases\n> (e.g. if our side changed while their side did not, the result\n> is ours) without triggering the file-level merge drivers, so the\n> assumption is warranted.\n>\n> The code path in \"git apply --3way\", however, does not check for\n> the trivial three-way merge situation and always calls the\n> file-level merge drivers.  This used to be perfectly OK back\n> when we always first attempted a straight patch application and\n> used the three-way code path only as a fallback.  Any binary\n> patch that can be applied as a trivial three-way merge (e.g. the\n> patch is based exactly on the version we happen to have) would\n> always cleanly apply, so the ll_binary_merge() that is not\n> prepared to see the trivial case would not have to handle such a\n> case.\n>\n> This no longer is true after we made \"--3way\" to mean \"first try\n> three-way and then fall back to straight application\", and made\n> \"git apply -3\" on a binary patch that is based on the current\n> version no longer apply.\n>\n> Teach \"git apply -3\" to first check for the trivial merge cases\n> and resolve them without hitting the file-level merge drivers.\n>\n> Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> [jc: stolen tests from Jerry's patch]\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  apply.c                   | 21 ++++++++++++++++++\n>  t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 66 insertions(+)\n>\n> diff --git a/apply.c b/apply.c\n> index 44bc31d6eb..c9f9503e90 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3467,6 +3467,21 @@ static int load_preimage(struct apply_state *state,\n>         return 0;\n>  }\n>\n> +static int resolve_to(struct image *image, const struct object_id *result_id)\n> +{\n> +       unsigned long size;\n> +       enum object_type type;\n> +\n> +       clear_image(image);\n> +\n> +       image->buf = read_object_file(result_id, &type, &size);\n> +       if (!image->buf || type != OBJ_BLOB)\n> +               die(\"unable to read blob object %s\", oid_to_hex(result_id));\n> +       image->len = size;\n> +\n> +       return 0;\n> +}\n> +\n>  static int three_way_merge(struct apply_state *state,\n>                            struct image *image,\n>                            char *path,\n> @@ -3478,6 +3493,12 @@ static int three_way_merge(struct apply_state *state,\n>         mmbuffer_t result = { NULL };\n>         int status;\n>\n> +       /* resolve trivial cases first */\n> +       if (oideq(base, ours))\n> +               return resolve_to(image, theirs);\n> +       else if (oideq(base, theirs) || oideq(ours, theirs))\n> +               return resolve_to(image, ours);\n> +\n>         read_mmblob(&base_file, base);\n>         read_mmblob(&our_file, ours);\n>         read_mmblob(&their_file, theirs);\n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index 65147efdea..cc3aa3314a 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n>         test_cmp expect.diff actual.diff\n>  '\n>\n> +test_expect_success 'apply binary file patch' '\n> +       git reset --hard main &&\n> +       cp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n> +       git add bin.png &&\n> +       git commit -m \"add binary file\" &&\n> +\n> +       cp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n> +\n> +       git diff --binary >bin.diff &&\n> +       git reset --hard &&\n> +\n> +       # Apply must succeed.\n> +       git apply bin.diff\n> +'\n> +\n> +test_expect_success 'apply binary file patch with 3way' '\n> +       git reset --hard main &&\n> +       cp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n> +       git add bin.png &&\n> +       git commit -m \"add binary file\" &&\n> +\n> +       cp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n> +\n> +       git diff --binary >bin.diff &&\n> +       git reset --hard &&\n> +\n> +       # Apply must succeed.\n> +       git apply --3way --index bin.diff\n> +'\n> +\n> +test_expect_success 'apply full-index patch with 3way' '\n> +       git reset --hard main &&\n> +       cp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n> +       git add bin.png &&\n> +       git commit -m \"add binary file\" &&\n> +\n> +       cp \"$TEST_DIRECTORY/test-binary-2.png\" bin.png &&\n> +\n> +       git diff --full-index >bin.diff &&\n> +       git reset --hard &&\n> +\n> +       # Apply must succeed.\n> +       git apply --3way --index bin.diff\n> +'\n> +\n>  test_done\n> --\n> 2.33.0-408-g8e1aa136b3\n\nReviewed-by: Elijah Newren <newren@gmail.com>\n"},{"id":"434803","messageId":"87pmtlnyu7.fsf@evledraar.gmail.com","threadId":"56171","inReplyTo":"20210905190657.2906699-1-gitster@pobox.com","subject":"Re: [PATCH v2] apply: resolve trivial merge without hitting ll-merge with \"--3way\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-06T21:59:42Z","receivedAt":"2021-09-06T22:06:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Sep 05 2021, Junio C Hamano wrote:\n\n> +\tif (!image->buf || type != OBJ_BLOB)\n> +\t\tdie(\"unable to read blob object %s\", oid_to_hex(result_id));\n\nThis die() message seems to only be applicable to the first condition\nhere, shouldn't this be:\n\n    if (!image->buf)\n        die(_(\"unable to read blob object %s\"), oid_to_hex(result_id));\n    if (type != OBJ_BLOB)\n        die(_(\"object %s is %s, expected blob\"), oid_to_hex(result_id), type_name(type));\n\nAlso as shown there, missing _() for marking the translation.\n\n> [...]\n> +test_expect_success 'apply binary file patch' '\n> +\tgit reset --hard main &&\n\nPartly this is cleaning up a mess after an existing test, but here\nthere's no reason we can't use test_when_finished() for all the new\ntests to make them clean up after themselves:\n\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex cc3aa3314a3..c3c9b52e30d 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -232,6 +232,8 @@ test_expect_success 'apply with --3way --cached and conflicts' '\n \n test_expect_success 'apply binary file patch' '\n \tgit reset --hard main &&\n+\ttest_when_finished \"git reset --hard main\" &&\n+\n \tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n \tgit add bin.png &&\n \tgit commit -m \"add binary file\" &&\n@@ -246,7 +248,8 @@ test_expect_success 'apply binary file patch' '\n '\n \n test_expect_success 'apply binary file patch with 3way' '\n-\tgit reset --hard main &&\n+\ttest_when_finished \"git reset --hard main\" &&\n+\n \tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n \tgit add bin.png &&\n \tgit commit -m \"add binary file\" &&\n@@ -261,7 +264,8 @@ test_expect_success 'apply binary file patch with 3way' '\n '\n \n test_expect_success 'apply full-index patch with 3way' '\n-\tgit reset --hard main &&\n+\ttest_when_finished \"git reset --hard main\" &&\n+\n \tcp \"$TEST_DIRECTORY/test-binary-1.png\" bin.png &&\n \tgit add bin.png &&\n \tgit commit -m \"add binary file\" &&\n"},{"id":"434814","messageId":"xmqqo895cdyl.fsf@gitster.g","threadId":"56171","inReplyTo":"87pmtlnyu7.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] apply: resolve trivial merge without hitting ll-merge with \"--3way\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T02:32:50Z","receivedAt":"2021-09-07T02:32:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Sun, Sep 05 2021, Junio C Hamano wrote:\n>\n>> +\tif (!image->buf || type != OBJ_BLOB)\n>> +\t\tdie(\"unable to read blob object %s\", oid_to_hex(result_id));\n>\n> This die() message seems to only be applicable to the first condition\n> here, shouldn't this be:\n\nAs this directly was lifted from read_mmblob(), it should be exactly\nspelled as I wrote.\n"},{"id":"434927","messageId":"xmqqy2889m6u.fsf@gitster.g","threadId":"56171","inReplyTo":"87pmtlnyu7.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] apply: resolve trivial merge without hitting ll-merge with \"--3way\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T20:15:37Z","receivedAt":"2021-09-07T20:15:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Partly this is cleaning up a mess after an existing test, but here\n> there's no reason we can't use test_when_finished() for all the new\n> tests to make them clean up after themselves:\n\nI do not mind if somebody wants to send in a janitorial patch after\nthe dust settles, but adding \"test_when_finished reset --hard\" after\neach \"refs --hard\" at the beginning of each test is not something I\nwould expect to see.  Such a patch should first choose between \"each\ntest cleans after itself\" and \"expect previous ones may have left a\nmess, so each test clears the slate sufficiently before it starts\"\nand then stick to the approach, not mixture of both, I would think.\n\nThanks.\n"}]}