From 7d879ad7e02f1f4d7f504862b280e503db403a36 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 10 Jun 2021 08:57:05 -0400 Subject: [PATCH 1/3] ll_binary_merge(): handle XDL_MERGE_FAVOR_UNION Prior to commit a944af1d86 (merge: teach -Xours/-Xtheirs to binary ll-merge driver, 2012-09-08), we always reported a conflict from ll_binary_merge() by returning "1" (in the xdl_merge and ll_merge code, this value is the number of conflict hunks). After that commit, we report zero conflicts if the "variant" flag is set, under the assumption that it is one of XDL_MERGE_FAVOR_OURS or XDL_MERGE_FAVOR_THEIRS. But this gets confused by XDL_MERGE_FAVOR_UNION. We do not know how to do a binary union merge, but erroneously report no conflicts anyway (and just blindly use the "ours" content as the result). Let's tighten our check to just the cases that a944af1d86 meant to cover. This fixes the union case (which existed already back when that commit was made), as well as future-proofing us against any other variants that get added later. Note that you can't trigger this from "git merge-file --union", as that bails on binary files before even calling into the ll-merge machinery. The test here uses the "union" merge attribute, which does erroneously report a successful merge. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ll-merge.c | 4 +++- t/t6406-merge-attr.sh | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/ll-merge.c b/ll-merge.c index 9a8a2c365c..145deb12fa 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -91,7 +91,9 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused, * With -Xtheirs or -Xours, we have cleanly merged; * otherwise we got a conflict. */ - return (opts->variant ? 0 : 1); + return opts->variant == XDL_MERGE_FAVOR_OURS || + opts->variant == XDL_MERGE_FAVOR_THEIRS ? + 0 : 1; } static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh index d5a4ac2d81..c1c458d933 100755 --- a/t/t6406-merge-attr.sh +++ b/t/t6406-merge-attr.sh @@ -207,4 +207,21 @@ test_expect_success 'custom merge does not lock index' ' git merge main ' +test_expect_success 'binary files with union attribute' ' + git checkout -b bin-main && + printf "base\0" >bin.txt && + echo "bin.txt merge=union" >.gitattributes && + git add bin.txt .gitattributes && + git commit -m base && + + printf "one\0" >bin.txt && + git commit -am one && + + git checkout -b bin-side HEAD^ && + printf "two\0" >bin.txt && + git commit -am two && + + test_must_fail git merge bin-main +' + test_done From 7f53f78b04b49c2060c23df6566aceb3ba394aea Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 10 Jun 2021 08:58:43 -0400 Subject: [PATCH 2/3] ll_union_merge(): pass name labels to ll_xdl_merge() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since cd1d61c44f (make union merge an xdl merge favor, 2010-03-01), we pass NULL to ll_xdl_merge() for the "name" labels of the ancestor, ours and theirs buffers. We usually use these for annotating conflict markers left in a file. For a union merge, these shouldn't matter; the point of it is that we'd never leave conflict markers in the first place. But there is one code path where we may dereference them: if the file contents appear to be binary, ll_binary_merge() will give up and pass them to warning() to generate a message for the user (that was true even when cd1d61c44f was written, though the warning was in ll_xdl_merge() back then). That can result in a segfault, though on many systems (including glibc), the printf routines will helpfully just say "(null)" instead. We can extend our binary-union test in t6406 to check stderr, which catches the problem on all systems. This also fixes a warning from "gcc -O3". Unlike lower optimization levels, it inlines enough to see that the NULL can make it to warning() and complains: In function ‘ll_binary_merge’, inlined from ‘ll_xdl_merge’ at ll-merge.c:115:10, inlined from ‘ll_union_merge’ at ll-merge.c:151:9: ll-merge.c:74:4: warning: ‘%s’ directive argument is null [-Wformat-overflow=] 74 | warning("Cannot merge binary files: %s (%s vs. %s)", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 75 | path, name1, name2); | ~~~~~~~~~~~~~~~~~~~ Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ll-merge.c | 2 +- t/t6406-merge-attr.sh | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ll-merge.c b/ll-merge.c index 145deb12fa..0ee34d8a01 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -151,7 +151,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused, o = *opts; o.variant = XDL_MERGE_FAVOR_UNION; return ll_xdl_merge(drv_unused, result, path_unused, - orig, NULL, src1, NULL, src2, NULL, + orig, orig_name, src1, name1, src2, name2, &o, marker_size); } diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh index c1c458d933..8494645837 100755 --- a/t/t6406-merge-attr.sh +++ b/t/t6406-merge-attr.sh @@ -221,7 +221,8 @@ test_expect_success 'binary files with union attribute' ' printf "two\0" >bin.txt && git commit -am two && - test_must_fail git merge bin-main + test_must_fail git merge bin-main 2>stderr && + grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr ' test_done From 382b601acde12b298bb84faa11b3f42868716a0d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 10 Jun 2021 12:14:12 -0400 Subject: [PATCH 3/3] ll_union_merge(): rename path_unused parameter The "path" parameter to ll_union_merge() is named "path_unused", since we don't ourselves use it. But we do pass it to ll_xdl_merge(), which may look at it (it gets passed to ll_binary_merge(), which may pass it to warning()). Let's rename it to correct this inaccuracy (both of the other functions correctly do not call this "unused"). Note that we also pass drv_unused, but it truly is unused by the rest of the stack (it only exists at all to provide a generic interface that matches what ll_ext_merge() needs). Reported-by: Elijah Newren Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ll-merge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ll-merge.c b/ll-merge.c index 0ee34d8a01..261657578c 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -138,7 +138,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, static int ll_union_merge(const struct ll_merge_driver *drv_unused, mmbuffer_t *result, - const char *path_unused, + const char *path, mmfile_t *orig, const char *orig_name, mmfile_t *src1, const char *name1, mmfile_t *src2, const char *name2, @@ -150,7 +150,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused, assert(opts); o = *opts; o.variant = XDL_MERGE_FAVOR_UNION; - return ll_xdl_merge(drv_unused, result, path_unused, + return ll_xdl_merge(drv_unused, result, path, orig, orig_name, src1, name1, src2, name2, &o, marker_size); }