threads / discuss / 19490

Slightly obscure merge-file inconsistency

Subject: Slightly obscure merge-file inconsistency

## tl;dr

5 messages between May 25, 2009 and May 25, 2009.

replies: 4people: 2as markdown or json

Charles Bailey· May 25, 2009, 00:21 UTC · lore
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· May 25, 2009, 00:21 UTC · re: Charles Bailey · lore

[PATCH 1/2] merge-file fails to output anything for a degenerate merge

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 <charles@hashpling.org>
---
 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· May 25, 2009, 00:21 UTC · re: Charles Bailey · lore

[PATCH 2/2] Change xdl_merge to generate output even for null merges

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 <charles@hashpling.org>
---
 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· May 25, 2009, 07:51 UTC · re: Charles Bailey · lore

Re: [PATCH 2/2] Change xdl_merge to generate output even for null merges

Hi,
On Mon, 25 May 2009, Charles Bailey wrote:
Show 10 quoted lines
> 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 <charles@hashpling.org>

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· May 25, 2009, 07:52 UTC · re: Charles Bailey · lore

Re: [PATCH 1/2] merge-file fails to output anything for a degenerate merge

Hi,
On Mon, 25 May 2009, Charles Bailey wrote:
Show 8 quoted lines
> 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 <charles@hashpling.org>

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

← back to recent threads