Slightly obscure merge-file inconsistency

Slightly obscure merge-file inconsistency

[Charles Bailey]

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/

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

[Charles Bailey]

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

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

[Charles Bailey]

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

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

[Johannes Schindelin]

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 <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

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

[Johannes Schindelin]

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 <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