| Message ID | YMIMIdnDB1Jq+lzp@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7d879ad7e02f1f4d7f504862b280e503db403a36 |
| Headers | show |
| Series | fix union merge with binary files | expand |
Jeff King <peff@peff.net> writes: > 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. Makes sense. > 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. Nice discovery. Thanks.
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
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 <peff@peff.net> --- ll-merge.c | 4 +++- t/t6406-merge-attr.sh | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)