# Slightly obscure merge-file inconsistency 5 messages from 2009-05-25 to 2009-05-25. Participants: Charles Bailey, Johannes Schindelin. Thread: https://gitlist.dev/t/19490 ## Charles Bailey, 2009-05-25 00:21 Subject: Slightly obscure merge-file inconsistency Message-ID: <1243210874-18451-1-git-send-email-charles@hashpling.org> URL: https://gitlist.dev/e/1243210874-18451-1-git-send-email-charles%40hashpling.org ``` This was spotted by someone on stackoverflow.com. It seems that git merge-file will wipe the merged file in the degenerate case when all the inputs have identical contents. As far as I can tell this is a bug, albeit one which will never be triggered in practice because merge-file won't ever be called on files that are identical in base, local and remote versions. I think that the problem is just an overzealous null check in xdl_merge. Certainly with the fix, the test suite still passes but there may be some use cases of xdl_merge that I'm not aware of that rely on the current behaviour. http://stackoverflow.com/questions/903966/ ``` ## Charles Bailey, 2009-05-25 00:21 Subject: [PATCH 1/2] merge-file fails to output anything for a degenerate merge Message-ID: <1243210874-18451-2-git-send-email-charles@hashpling.org> URL: https://gitlist.dev/e/1243210874-18451-2-git-send-email-charles%40hashpling.org In-Reply-To: <1243210874-18451-1-git-send-email-charles@hashpling.org> ``` In the case that merge-file is passed three files with identical contents it wipes the contents of the output file instead of leaving it unchanged. Althought merge-file is porcelain and this will never happen in normal usage, it is still wrong. Signed-off-by: Charles Bailey --- t/t6023-merge-file.sh | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh index f8942bc..44141af 100755 --- a/t/t6023-merge-file.sh +++ b/t/t6023-merge-file.sh @@ -54,6 +54,11 @@ deduxit me super semitas jusitiae, EOF printf "propter nomen suum." >> new4.txt +cp orig.txt test.txt +test_expect_success "merge with no changes" \ + "git merge-file test.txt orig.txt orig.txt && + test_cmp test.txt orig.txt" + cp new1.txt test.txt test_expect_success "merge without conflict" \ "git merge-file test.txt orig.txt new2.txt" -- 1.6.3.1.72.gbd1ec ``` ## Charles Bailey, 2009-05-25 00:21 Subject: [PATCH 2/2] Change xdl_merge to generate output even for null merges Message-ID: <1243210874-18451-3-git-send-email-charles@hashpling.org> URL: https://gitlist.dev/e/1243210874-18451-3-git-send-email-charles%40hashpling.org In-Reply-To: <1243210874-18451-2-git-send-email-charles@hashpling.org> ``` xdl_merge used to have a check to ensure that there was at least some change in one or other side being merged but this suppressed output for the degenerate case when base, local and remote contents were all identical. Removing this check enables correct output in the degenerate case and xdl_free_script handles freeing NULL scripts so there is no need to have the check for these calls. Signed-off-by: Charles Bailey --- xdiff/xmerge.c | 32 ++++++++++++++++---------------- 1 files changed, 16 insertions(+), 16 deletions(-) diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index d9737f0..9cdc3ac 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -563,23 +563,23 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1, return -1; } status = 0; - if (xscr1 || xscr2) { - if (!xscr1) { - result->ptr = xdl_malloc(mf2->size); - memcpy(result->ptr, mf2->ptr, mf2->size); - result->size = mf2->size; - } else if (!xscr2) { - result->ptr = xdl_malloc(mf1->size); - memcpy(result->ptr, mf1->ptr, mf1->size); - result->size = mf1->size; - } else { - status = xdl_do_merge(&xe1, xscr1, name1, - &xe2, xscr2, name2, - flags, xpp, result); - } - xdl_free_script(xscr1); - xdl_free_script(xscr2); + + if (!xscr1) { + result->ptr = xdl_malloc(mf2->size); + memcpy(result->ptr, mf2->ptr, mf2->size); + result->size = mf2->size; + } else if (!xscr2) { + result->ptr = xdl_malloc(mf1->size); + memcpy(result->ptr, mf1->ptr, mf1->size); + result->size = mf1->size; + } else { + status = xdl_do_merge(&xe1, xscr1, name1, + &xe2, xscr2, name2, + flags, xpp, result); } + xdl_free_script(xscr1); + xdl_free_script(xscr2); + xdl_free_env(&xe1); xdl_free_env(&xe2); -- 1.6.3.1.72.gbd1ec ``` ## Johannes Schindelin, 2009-05-25 07:51 Subject: Re: [PATCH 2/2] Change xdl_merge to generate output even for null merges Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0905250948420.26154%40pacific.mpi-cbg.de In-Reply-To: <1243210874-18451-3-git-send-email-charles@hashpling.org> ``` Hi, On Mon, 25 May 2009, Charles Bailey wrote: > xdl_merge used to have a check to ensure that there was at least > some change in one or other side being merged but this suppressed > output for the degenerate case when base, local and remote > contents were all identical. > > Removing this check enables correct output in the degenerate case > and xdl_free_script handles freeing NULL scripts so there is no > need to have the check for these calls. > > Signed-off-by: Charles Bailey Maybe mention here that it just so happens that when there were no changes in base->local, it does not matter what changes (_if any_) were made to base->remote, we should output the remote version. Took me more than 2 seconds to find that out, so let's spare the occasional reader this time (2 seconds is my attention span). Your fix is correct. Ciao, Dscho ``` ## Johannes Schindelin, 2009-05-25 07:52 Subject: Re: [PATCH 1/2] merge-file fails to output anything for a degenerate merge Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0905250951120.26154%40pacific.mpi-cbg.de In-Reply-To: <1243210874-18451-2-git-send-email-charles@hashpling.org> ``` Hi, On Mon, 25 May 2009, Charles Bailey wrote: > In the case that merge-file is passed three files with identical > contents it wipes the contents of the output file instead of > leaving it unchanged. > > Althought merge-file is porcelain and this will never happen in > normal usage, it is still wrong. > > Signed-off-by: Charles Bailey Your patch is good, but I'd like to have it mention in the commit subject that this is not a fix, but a patch to the test suite. A prefix "t6023:" would suffice, methinks. Thanks, Dscho ```