Re: Strange output of git-diff-tree

From: Junio C Hamano <junkio@cox.net>
Date: Aug 9, 2006, 20:17 UTC
Message-ID: <7v7j1h3bq0.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <7vfyg54vjb.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> writes:

Show 5 quoted lines

> Well spotted.  The minimum reproduction recipe is:
>
>      $ git diff-tree --find-copies-harder 2ad9331
>
> Will look into it.

This is because the original command line has -C and -M in this order, and the minimum reproduction does not have either.

diff_setup_done() "silently" complains about it but the caller was not taking notice.

What happens is that the check at the top of diff_setup_done() fails, and does not set up options->abbrev to 40 upon seeing abbrev is set to zero. This later causes find_unique_abbrev() to be called with len=0, which returns an empty string, because it is not expecing to get something like that.

Incidentally, there is another funny breakage related to this.

	$ git diff-tree --find-copies-harder v1.4.1

complains quite a lot about "feeding unmodified" file pairs. This is because "harder" is given without -C.

So there are three things to fix:

 (1) callers of diff_setup_done() that do not check and die
     should do so like others.

 (2) find_unique_abbrev() should prepare being called with
     len=0.

 (3) make --find-copies-harder imply -C.

Strictly speaking, the third is an incompatible change, but it makes something that used to be an invalid/undefined operation to a valid one, so it would not be so bad.

*1*

diff --git a/builtin-diff.c b/builtin-diff.c
index 1075855..dd9886c 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -253,7 +253,8 @@ int cmd_diff(int argc, const char **argv
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	if (!rev.diffopt.output_format) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
-		diff_setup_done(&rev.diffopt);
+		if (diff_setup_done(&rev.diffopt) < 0)
+			die("diff_setup_done failed");
 	}
 
 	/* Do we have --cached and not have a pending object, then
diff --git a/revision.c b/revision.c
index a58257a..5a91d06 100644
--- a/revision.c
+++ b/revision.c
@@ -936,7 +936,8 @@ int setup_revisions(int argc, const char
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 	}
 	revs->diffopt.abbrev = revs->abbrev;
-	diff_setup_done(&revs->diffopt);
+	if (diff_setup_done(&revs->diffopt) < 0)
+		die("diff_setup_done failed");
 
 	return left;
 }

*2*

diff --git a/sha1_name.c b/sha1_name.c
index 5fe8e5d..c5a05fa 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -193,7 +193,7 @@ const char *find_unique_abbrev(const uns
 
 	is_null = !memcmp(sha1, null_sha1, 20);
 	memcpy(hex, sha1_to_hex(sha1), 40);
-	if (len == 40)
+	if (len == 40 || !len)
 		return hex;
 	while (len < 40) {
 		unsigned char sha1_ret[20];

*3*

diff --git a/diff.c b/diff.c
index 895c137..02a409d 100644
--- a/diff.c
+++ b/diff.c
@@ -1515,9 +1515,10 @@ void diff_setup(struct diff_options *opt
 
 int diff_setup_done(struct diff_options *options)
 {
-	if ((options->find_copies_harder &&
-	     options->detect_rename != DIFF_DETECT_COPY) ||
-	    (0 <= options->rename_limit && !options->detect_rename))
+	if (options->find_copies_harder)
+		options->detect_rename = DIFF_DETECT_COPY;
+
+	if ((0 <= options->rename_limit && !options->detect_rename)
 		return -1;
 
 	if (options->output_format & (DIFF_FORMAT_NAME |
diff --git a/sha1_name.c b/sha1_name.c
index 5fe8e5d..c5a05fa 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -193,7 +193,7 @@ const char *find_unique_abbrev(const uns
 
 	is_null = !memcmp(sha1, null_sha1, 20);
 	memcpy(hex, sha1_to_hex(sha1), 40);
-	if (len == 40)
+	if (len == 40 || !len)
 		return hex;
 	while (len < 40) {
 		unsigned char sha1_ret[20];