Strange output of git-diff-tree

Strange output of git-diff-tree

[Jakub Narebski]

First (noticed by matled) is that for git-diff-tree with single tree
as an argument it outputs fist commit-id of commit given at input. 
It is not mentioned in documentation and I think totally unnecessary:

1038:jnareb@roke:~/git> git diff-tree --abbrev origin
d5dc6a76d49367cddc015e01d2e9aa22e64d7e28
:040000 040000 44fb36d... 1c26294... M  Documentation


Second, for some combination of options for it returns "..." instead of
0{40} for file creation. It seems that the culprit is "--find-copies-harder"
option:

1043:jnareb@roke:~/git> git rev-list --full-history next | \
 git diff-tree --find-copies-harder -B -C -M -r --full-history --stdin \
 -- gitweb/gitweb.perl gitweb/gitweb.cgi gitweb.cgi | less | grep " A"
:000000 100755 ... 017664b8f440f5ec151cf5653245ee02aefd3db2 A   gitweb.cgi

1047:jnareb@roke:~/git> git rev-list --full-history next | \
 git diff-tree -B -C -M -r --stdin \
 -- gitweb/gitweb.perl gitweb/gitweb.cgi gitweb.cgi | less | grep " A"
:000000 100755 0000000000000000000000000000000000000000 017664b8f440f5ec151cf5653245ee02aefd3db2 A     gitweb.cgi


Third, while it detects that gitweb/gitweb.perl was renamed from 
gitweb/gitweb.cgi:
  [...]  R100   gitweb/gitweb.cgi       gitweb/gitweb.perl
it does not notice that gitweb/gitweb.cgi was gitweb.cgi in 
1130ef362fc8d9c3422c23f5d5a833e93d3f5c13.


All those for git version 1.4.1.1

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Strange output of git-diff-tree

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> First (noticed by matled) is that for git-diff-tree with single tree
> as an argument it outputs fist commit-id of commit given at input. 
> It is not mentioned in documentation and I think totally unnecessary:
>
> 1038:jnareb@roke:~/git> git diff-tree --abbrev origin
> d5dc6a76d49367cddc015e01d2e9aa22e64d7e28
> :040000 040000 44fb36d... 1c26294... M  Documentation

Working as specified as in the original version.  See
core-tutorial and look for "git-diff-tree -p HEAD".

> Second, for some combination of options for it returns "..." instead of
> 0{40} for file creation.

Well spotted.  The minimum reproduction recipe is:

     $ git diff-tree --find-copies-harder 2ad9331

Will look into it.

> Third, while it detects that gitweb/gitweb.perl was renamed from 
> gitweb/gitweb.cgi:
>   [...]  R100   gitweb/gitweb.cgi       gitweb/gitweb.perl
> it does not notice that gitweb/gitweb.cgi was gitweb.cgi in 
> 1130ef362fc8d9c3422c23f5d5a833e93d3f5c13.

Depends on how you ask.

git diff-tree --abbrev --pretty -m -M -r --diff-filter=R 0a8f4f00

or

git diff-tree --abbrev --pretty -c -M -r 0a8f4f00

Re: Strange output of git-diff-tree

[Junio C Hamano]

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

> Jakub Narebski <jnareb@gmail.com> writes:
>
>> First (noticed by matled) is that for git-diff-tree with single tree
>> as an argument it outputs fist commit-id of commit given at input. 
>> It is not mentioned in documentation and I think totally unnecessary:
>>
>> 1038:jnareb@roke:~/git> git diff-tree --abbrev origin
>> d5dc6a76d49367cddc015e01d2e9aa22e64d7e28
>> :040000 040000 44fb36d... 1c26294... M  Documentation
>
> Working as specified as in the original version.  See
> core-tutorial and look for "git-diff-tree -p HEAD".

My mistake -- if you are talking about the first line that shows
the commit object name, I am not sure if this was from the
beginning or a recent slip-up.  Will need to look into it.

I thought you were talking about 1-argument diff-tree, sorry.

Re: Strange output of git-diff-tree

[Junio C Hamano]

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

> Junio C Hamano <junkio@cox.net> writes:
>
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>> First (noticed by matled) is that for git-diff-tree with single tree
>>> as an argument it outputs fist commit-id of commit given at input. 
>>> It is not mentioned in documentation and I think totally unnecessary:
>>>
>>> 1038:jnareb@roke:~/git> git diff-tree --abbrev origin
>>> d5dc6a76d49367cddc015e01d2e9aa22e64d7e28
>>> :040000 040000 44fb36d... 1c26294... M  Documentation
>>
>> Working as specified as in the original version.  See
>> core-tutorial and look for "git-diff-tree -p HEAD".
>
> My mistake -- if you are talking about the first line that shows
> the commit object name, I am not sure if this was from the
> beginning or a recent slip-up.  Will need to look into it.
>
> I thought you were talking about 1-argument diff-tree, sorry.

Turns out that git-diff-tree from 1.0.0 behaves this way.  Since
7384889 (May 18, 2005), one-tree form of diff-tree showed the
"header" line that has the commit object name at the top, and
since 1809266 (Jun 23, 2005) we have exactly one commit object
name there; before that we used to say "commitA (from commitB)".

I agree that showing the single argument that is given on the
command line as the first output line might seem redundant, but
it has been done this way for a long time (eternity in git
timescale), so let's not risk breaking people's scripts by
changing it.

(Unsolicited hint) straightening out stray "git bisect"

[Junio C Hamano]

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

> Junio C Hamano <junkio@cox.net> writes:
>
>> Junio C Hamano <junkio@cox.net> writes:
>>
>>> Jakub Narebski <jnareb@gmail.com> writes:
>>>
>>>> First (noticed by matled) is that for git-diff-tree with single tree
>>>> as an argument it outputs fist commit-id of commit given at input. 
>>>> It is not mentioned in documentation and I think totally unnecessary:
>>>>
>>>> 1038:jnareb@roke:~/git> git diff-tree --abbrev origin
>>>> d5dc6a76d49367cddc015e01d2e9aa22e64d7e28
>>>> :040000 040000 44fb36d... 1c26294... M  Documentation
>>>
> Turns out that git-diff-tree from 1.0.0 behaves this way.  Since
> 7384889 (May 18, 2005), one-tree form of diff-tree showed the
> "header" line that has the commit object name at the top, and
> since 1809266 (Jun 23, 2005) we have exactly one commit object
> name there; before that we used to say "commitA (from commitB)".

BTW, I got curious and run "git bisect" to see when the " (from
commitB)" was dropped.  This turned out to be quite an
interesting one.  You would start from these:

        $ git bisect start
        $ git bisect good 7384889 ;# this said "( from commitB)"
        $ git bisect bad v1.0.0   ;# we know this did not

and keep building and testing "git-diff-tree", to mark the ones
that do not say "( from commitB)" as bad and the ones that do as
good.

At some point, it crosses the "coolest merge ever" and suggests
c2f6a02 to be checked, which is on the original "gitk" branch.
There is no way to build git-diff-tree in that revision ;-).

At this point, "visualize" helps.

	$ git bisect visualize

shows (pardon the ASCII art):

    *--bisect/bad git-rev-list: add option to list all objects
    *  git-rev-parse: re-organize and be more careful
    .
    *  Do a cross-project merge of Paul Mackerras' gitk visualizer 
    |* Try to assign colors so crossing lines have different colors
    |* Account for indentation of the checkin commments by git-rev-list
    |.
    |*-bisect Show heads as well as tags
    |.
    |* Add initial version of gitk to the CVS repository
    o--bisect/good-fae22ac [PATCH] git-apply: tests for --stat and --summary.

Now, we know that gitk did not have diff-tree, so up to the
commit "Try to assign colors..." are irrelevant to the "bug" we
are hunting.  We say:

	$ git bisect good 6c20ff3

(where 6c20ff3 is the commit "Try to assign colors...") to mark
everything leading to that commit (ones on the line on the right
hand side in the above ASCII graph) are good.  After that bisect
continues normally.

Re: Strange output of git-diff-tree

[Junio C Hamano]

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

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