{"thread":{"id":"55896","subject":"[PATCH 0/2] fix union merge with binary files","startedAt":"2021-06-10T12:51:21Z","lastAt":"2021-06-11T03:36:16Z","messageCount":7,"participants":["Jeff King","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"426985","messageId":"YMIKwsEFnkqz6PWa@coredump.intra.peff.net","threadId":"55896","inReplyTo":null,"subject":"[PATCH 0/2] fix union merge with binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T12:51:14Z","receivedAt":"2021-06-10T12:51:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This started as an attempt to silence a \"gcc -O3\" warning. But I was\ncurious if we could trigger the problem it complains about in practice\n(spoiler: we can), so I wrote a test. And it seems there was an even\nbigger bug lurking, where we'd generate bogus merge results. :)\n\nThis fixes both bugs.\n\n [1/2]: ll_binary_merge(): handle XDL_MERGE_FAVOR_UNION\n [2/2]: ll_union_merge(): pass name labels to ll_xdl_merge()\n\n ll-merge.c | 6 ++++--\n t/t6406-merge-attr.sh | 18 ++++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\n-Peff\n"},{"id":"426986","messageId":"YMIMIdnDB1Jq+lzp@coredump.intra.peff.net","threadId":"55896","inReplyTo":"YMIKwsEFnkqz6PWa@coredump.intra.peff.net","subject":"[PATCH 1/2] ll_binary_merge(): handle XDL_MERGE_FAVOR_UNION","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T12:57:05Z","receivedAt":"2021-06-10T12:57:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prior to commit a944af1d86 (merge: teach -Xours/-Xtheirs to binary\nll-merge driver, 2012-09-08), we always reported a conflict from\nll_binary_merge() by returning \"1\" (in the xdl_merge and ll_merge code,\nthis value is the number of conflict hunks). After that commit, we\nreport zero conflicts if the \"variant\" flag is set, under the assumption\nthat it is one of XDL_MERGE_FAVOR_OURS or XDL_MERGE_FAVOR_THEIRS.\n\nBut this gets confused by XDL_MERGE_FAVOR_UNION. We do not know how to\ndo a binary union merge, but erroneously report no conflicts anyway (and\njust blindly use the \"ours\" content as the result).\n\nLet's tighten our check to just the cases that a944af1d86 meant to\ncover. This fixes the union case (which existed already back when that\ncommit was made), as well as future-proofing us against any other\nvariants that get added later.\n\nNote that you can't trigger this from \"git merge-file --union\", as that\nbails on binary files before even calling into the ll-merge machinery.\nThe test here uses the \"union\" merge attribute, which does erroneously\nreport a successful merge.\n\nSigned-off-by: Jeff King \n---\n ll-merge.c | 4 +++-\n t/t6406-merge-attr.sh | 17 +++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 9a8a2c365c..145deb12fa 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -91,7 +91,9 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t * With -Xtheirs or -Xours, we have cleanly merged;\n \t * otherwise we got a conflict.\n \t */\n-\treturn (opts->variant ? 0 : 1);\n+\treturn opts->variant == XDL_MERGE_FAVOR_OURS ||\n+\t opts->variant == XDL_MERGE_FAVOR_THEIRS ?\n+\t 0 : 1;\n }\n \n static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex d5a4ac2d81..c1c458d933 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -207,4 +207,21 @@ test_expect_success 'custom merge does not lock index' '\n \tgit merge main\n '\n \n+test_expect_success 'binary files with union attribute' '\n+\tgit checkout -b bin-main &&\n+\tprintf \"base\\0\" >bin.txt &&\n+\techo \"bin.txt merge=union\" >.gitattributes &&\n+\tgit add bin.txt .gitattributes &&\n+\tgit commit -m base &&\n+\n+\tprintf \"one\\0\" >bin.txt &&\n+\tgit commit -am one &&\n+\n+\tgit checkout -b bin-side HEAD^ &&\n+\tprintf \"two\\0\" >bin.txt &&\n+\tgit commit -am two &&\n+\n+\ttest_must_fail git merge bin-main\n+'\n+\n test_done\n-- \n2.32.0.529.g079a794268\n\n"},{"id":"426991","messageId":"YMIMg+uXDjzS70g5@coredump.intra.peff.net","threadId":"55896","inReplyTo":"YMIKwsEFnkqz6PWa@coredump.intra.peff.net","subject":"[PATCH 2/2] ll_union_merge(): pass name labels to ll_xdl_merge()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T12:58:43Z","receivedAt":"2021-06-10T12:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since cd1d61c44f (make union merge an xdl merge favor, 2010-03-01), we\npass NULL to ll_xdl_merge() for the \"name\" labels of the ancestor, ours\nand theirs buffers. We usually use these for annotating conflict markers\nleft in a file. For a union merge, these shouldn't matter; the point of\nit is that we'd never leave conflict markers in the first place.\n\nBut there is one code path where we may dereference them: if the file\ncontents appear to be binary, ll_binary_merge() will give up and pass\nthem to warning() to generate a message for the user (that was true even\nwhen cd1d61c44f was written, though the warning was in ll_xdl_merge()\nback then).\n\nThat can result in a segfault, though on many systems (including glibc),\nthe printf routines will helpfully just say \"(null)\" instead. We can\nextend our binary-union test in t6406 to check stderr, which catches the\nproblem on all systems.\n\nThis also fixes a warning from \"gcc -O3\". Unlike lower optimization\nlevels, it inlines enough to see that the NULL can make it to warning()\nand complains:\n\n In function ‘ll_binary_merge’,\n inlined from ‘ll_xdl_merge’ at ll-merge.c:115:10,\n inlined from ‘ll_union_merge’ at ll-merge.c:151:9:\n ll-merge.c:74:4: warning: ‘%s’ directive argument is null [-Wformat-overflow=]\n 74 | warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n 75 | path, name1, name2);\n | ~~~~~~~~~~~~~~~~~~~\n\nSigned-off-by: Jeff King \n---\n ll-merge.c | 2 +-\n t/t6406-merge-attr.sh | 3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 145deb12fa..0ee34d8a01 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -151,7 +151,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n \to = *opts;\n \to.variant = XDL_MERGE_FAVOR_UNION;\n \treturn ll_xdl_merge(drv_unused, result, path_unused,\n-\t\t\t orig, NULL, src1, NULL, src2, NULL,\n+\t\t\t orig, orig_name, src1, name1, src2, name2,\n \t\t\t &o, marker_size);\n }\n \ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex c1c458d933..8494645837 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -221,7 +221,8 @@ test_expect_success 'binary files with union attribute' '\n \tprintf \"two\\0\" >bin.txt &&\n \tgit commit -am two &&\n \n-\ttest_must_fail git merge bin-main\n+\ttest_must_fail git merge bin-main 2>stderr &&\n+\tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n '\n \n test_done\n-- \n2.32.0.529.g079a794268\n"},{"id":"427026","messageId":"CABPp-BGEJ7TX9G_kdZt3ZqhhFMe9-8KHdSJ8bJ+avtsXdnUUnw@mail.gmail.com","threadId":"55896","inReplyTo":"YMIKwsEFnkqz6PWa@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] fix union merge with binary files","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-10T15:19:48Z","receivedAt":"2021-06-10T15:20:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 10, 2021 at 5:54 AM Jeff King wrote:\n>\n> This started as an attempt to silence a \"gcc -O3\" warning. But I was\n> curious if we could trigger the problem it complains about in practice\n> (spoiler: we can), so I wrote a test. And it seems there was an even\n> bigger bug lurking, where we'd generate bogus merge results. :)\n>\n> This fixes both bugs.\n\nNice catches, and fixes. I had a minor comment on 2/2, but with or\nwithout fixing up the 'path_unused' variable name both patches are:\n\nReviewed-by: Elijah Newren \n"},{"id":"427027","messageId":"CABPp-BFu0dJa=KxVtzzDG0RrcPo6bGCVx8P8VhB9sV8OjaYQNQ@mail.gmail.com","threadId":"55896","inReplyTo":"YMIMg+uXDjzS70g5@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ll_union_merge(): pass name labels to ll_xdl_merge()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-10T15:19:02Z","receivedAt":"2021-06-10T15:20:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 10, 2021 at 6:00 AM Jeff King wrote:\n>\n> Since cd1d61c44f (make union merge an xdl merge favor, 2010-03-01), we\n> pass NULL to ll_xdl_merge() for the \"name\" labels of the ancestor, ours\n> and theirs buffers. We usually use these for annotating conflict markers\n> left in a file. For a union merge, these shouldn't matter; the point of\n> it is that we'd never leave conflict markers in the first place.\n>\n> But there is one code path where we may dereference them: if the file\n> contents appear to be binary, ll_binary_merge() will give up and pass\n> them to warning() to generate a message for the user (that was true even\n> when cd1d61c44f was written, though the warning was in ll_xdl_merge()\n> back then).\n>\n> That can result in a segfault, though on many systems (including glibc),\n> the printf routines will helpfully just say \"(null)\" instead. We can\n> extend our binary-union test in t6406 to check stderr, which catches the\n> problem on all systems.\n\nNice catch (as is your 1/2 as well).\n\n> This also fixes a warning from \"gcc -O3\". Unlike lower optimization\n> levels, it inlines enough to see that the NULL can make it to warning()\n> and complains:\n>\n> In function ‘ll_binary_merge’,\n> inlined from ‘ll_xdl_merge’ at ll-merge.c:115:10,\n> inlined from ‘ll_union_merge’ at ll-merge.c:151:9:\n> ll-merge.c:74:4: warning: ‘%s’ directive argument is null [-Wformat-overflow=]\n> 74 | warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> 75 | path, name1, name2);\n> | ~~~~~~~~~~~~~~~~~~~\n\nSo the warning uses path as well as name1 and name2...\n\n>\n> Signed-off-by: Jeff King \n> ---\n> ll-merge.c | 2 +-\n> t/t6406-merge-attr.sh | 3 ++-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/ll-merge.c b/ll-merge.c\n> index 145deb12fa..0ee34d8a01 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -151,7 +151,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n> o = *opts;\n> o.variant = XDL_MERGE_FAVOR_UNION;\n> return ll_xdl_merge(drv_unused, result, path_unused,\n\nShould we also rename 'path_unused' to 'path', since it is actually used?\n\n> - orig, NULL, src1, NULL, src2, NULL,\n> + orig, orig_name, src1, name1, src2, name2,\n> &o, marker_size);\n> }\n>\n> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\n> index c1c458d933..8494645837 100755\n> --- a/t/t6406-merge-attr.sh\n> +++ b/t/t6406-merge-attr.sh\n> @@ -221,7 +221,8 @@ test_expect_success 'binary files with union attribute' '\n> printf \"two\\0\" >bin.txt &&\n> git commit -am two &&\n>\n> - test_must_fail git merge bin-main\n> + test_must_fail git merge bin-main 2>stderr &&\n> + grep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n> '\n>\n> test_done\n> --\n> 2.32.0.529.g079a794268\n\nThis patch has a minor textual conflict with my remerge-diff series,\nbut since I haven't submitted it yet, that's my problem rather than\nyours...and it will be an easy fix anyway.\n\nAnyway, good catches. Other than maybe considering fixing the name of\n'path_unused', this looks good to me.\n"},{"id":"427033","messageId":"YMI6VADKYmK+aV/C@coredump.intra.peff.net","threadId":"55896","inReplyTo":"CABPp-BFu0dJa=KxVtzzDG0RrcPo6bGCVx8P8VhB9sV8OjaYQNQ@mail.gmail.com","subject":"[PATCH 3/2] ll_union_merge(): rename path_unused parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T16:14:12Z","receivedAt":"2021-06-10T16:14:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 10, 2021 at 08:19:02AM -0700, Elijah Newren wrote:\n\n> > Signed-off-by: Jeff King \n> > ---\n> > ll-merge.c | 2 +-\n> > t/t6406-merge-attr.sh | 3 ++-\n> > 2 files changed, 3 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/ll-merge.c b/ll-merge.c\n> > index 145deb12fa..0ee34d8a01 100644\n> > --- a/ll-merge.c\n> > +++ b/ll-merge.c\n> > @@ -151,7 +151,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n> > o = *opts;\n> > o.variant = XDL_MERGE_FAVOR_UNION;\n> > return ll_xdl_merge(drv_unused, result, path_unused,\n> \n> Should we also rename 'path_unused' to 'path', since it is actually used?\n\nThat seems reasonable, but I'd prefer to do it as a separate patch. So\nhere that is.\n\nIronically, orig_path was unused both before this patch (where we threw\nit away and passed NULL instead) and after (where we pass it into the\nxdl merge, but the union merge should ignore it completely). But I\nprefer not to go too wild with these kind of annotations, as they can\neasily become inaccurate or out of date. If we're passing it, then we\nshouldn't make too many assumptions about what xdl_merge() is doing\nunder the hood.\n\nSo we could also rename drv_unused mentioned below, but I didn't here.\n\n-- >8 --\nSubject: [PATCH] ll_union_merge(): rename path_unused parameter\n\nThe \"path\" parameter to ll_union_merge() is named \"path_unused\", since\nwe don't ourselves use it. But we do pass it to ll_xdl_merge(), which\nmay look at it (it gets passed to ll_binary_merge(), which may pass it\nto warning()). Let's rename it to correct this inaccuracy (both of the\nother functions correctly do not call this \"unused\").\n\nNote that we also pass drv_unused, but it truly is unused by the rest of\nthe stack (it only exists at all to provide a generic interface that\nmatches what ll_ext_merge() needs).\n\nReported-by: Elijah Newren \nSigned-off-by: Jeff King \n---\n ll-merge.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 0ee34d8a01..261657578c 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -138,7 +138,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \n static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n \t\t\t mmbuffer_t *result,\n-\t\t\t const char *path_unused,\n+\t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n \t\t\t mmfile_t *src2, const char *name2,\n@@ -150,7 +150,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n \tassert(opts);\n \to = *opts;\n \to.variant = XDL_MERGE_FAVOR_UNION;\n-\treturn ll_xdl_merge(drv_unused, result, path_unused,\n+\treturn ll_xdl_merge(drv_unused, result, path,\n \t\t\t orig, orig_name, src1, name1, src2, name2,\n \t\t\t &o, marker_size);\n }\n-- \n2.32.0.529.g079a794268\n\n"},{"id":"427078","messageId":"xmqq4ke55az9.fsf@gitster.g","threadId":"55896","inReplyTo":"YMIMIdnDB1Jq+lzp@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ll_binary_merge(): handle XDL_MERGE_FAVOR_UNION","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-11T03:36:10Z","receivedAt":"2021-06-11T03:36:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Prior to commit a944af1d86 (merge: teach -Xours/-Xtheirs to binary\n> ll-merge driver, 2012-09-08), we always reported a conflict from\n> ll_binary_merge() by returning \"1\" (in the xdl_merge and ll_merge code,\n> this value is the number of conflict hunks). After that commit, we\n> report zero conflicts if the \"variant\" flag is set, under the assumption\n> that it is one of XDL_MERGE_FAVOR_OURS or XDL_MERGE_FAVOR_THEIRS.\n>\n> But this gets confused by XDL_MERGE_FAVOR_UNION. We do not know how to\n> do a binary union merge, but erroneously report no conflicts anyway (and\n> just blindly use the \"ours\" content as the result).\n>\n> Let's tighten our check to just the cases that a944af1d86 meant to\n> cover. This fixes the union case (which existed already back when that\n> commit was made), as well as future-proofing us against any other\n> variants that get added later.\n\nMakes sense.\n\n> Note that you can't trigger this from \"git merge-file --union\", as that\n> bails on binary files before even calling into the ll-merge machinery.\n> The test here uses the \"union\" merge attribute, which does erroneously\n> report a successful merge.\n\nNice discovery.\n\nThanks.\n"}]}