[PATCH] "git diff <tree>{3,}": do not reverse order of arguments

[PATCH] "git diff <tree>{3,}": do not reverse order of arguments

[Matt McCutchen]

According to the message of commit 0fe7c1de16f71312e6adac4b85bddf0d62a47168,
"git diff" with three or more trees expects the merged tree first followed by
the parents, in order.  However, this command reversed the order of its
arguments, resulting in confusing diffs.  A comment /* Again, the revs are all
reverse */ suggested there was a reason for this, but I can't figure out the
reason, so I removed the reversal of the arguments.  Test case included.

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
 builtin-diff.c                        |    4 +---
 t/t4013-diff-various.sh               |    1 +
 t/t4013/diff.diff_master_master^_side |   29 +++++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)
 create mode 100644 t/t4013/diff.diff_master_master^_side

diff --git a/builtin-diff.c b/builtin-diff.c
index 35da366..9c8c295 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -177,10 +177,8 @@ static int builtin_diff_combined(struct rev_info *revs,
 	if (!revs->dense_combined_merges && !revs->combine_merges)
 		revs->dense_combined_merges = revs->combine_merges = 1;
 	parent = xmalloc(ents * sizeof(*parent));
-	/* Again, the revs are all reverse */
 	for (i = 0; i < ents; i++)
-		hashcpy((unsigned char *)(parent + i),
-			ent[ents - 1 - i].item->sha1);
+		hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
 	diff_tree_combined(parent[0], parent + 1, ents - 1,
 			   revs->dense_combined_merges, revs);
 	return 0;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 1a6b522..fe6080d 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -258,6 +258,7 @@ diff --patch-with-stat -r initial..side
 diff --patch-with-raw -r initial..side
 diff --name-status dir2 dir
 diff --no-index --name-status dir2 dir
+diff master master^ side
 EOF
 
 test_done
diff --git a/t/t4013/diff.diff_master_master^_side b/t/t4013/diff.diff_master_master^_side
new file mode 100644
index 0000000..50ec9ca
--- /dev/null
+++ b/t/t4013/diff.diff_master_master^_side
@@ -0,0 +1,29 @@
+$ git diff master master^ side
+diff --cc dir/sub
+index cead32e,7289e35..992913c
+--- a/dir/sub
++++ b/dir/sub
+@@@ -1,6 -1,4 +1,8 @@@
+  A
+  B
+ +C
+ +D
+ +E
+ +F
++ 1
++ 2
+diff --cc file0
+index b414108,f4615da..10a8a9f
+--- a/file0
++++ b/file0
+@@@ -1,6 -1,6 +1,9 @@@
+  1
+  2
+  3
+ +4
+ +5
+ +6
++ A
++ B
++ C
+$
-- 
1.6.0.2.519.gf6ee

Re: [PATCH] "git diff <tree>{3,}": do not reverse order of arguments

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

> diff --git a/builtin-diff.c b/builtin-diff.c
> index 35da366..9c8c295 100644
> --- a/builtin-diff.c
> +++ b/builtin-diff.c
> @@ -177,10 +177,8 @@ static int builtin_diff_combined(struct rev_info *revs,
>  	if (!revs->dense_combined_merges && !revs->combine_merges)
>  		revs->dense_combined_merges = revs->combine_merges = 1;
>  	parent = xmalloc(ents * sizeof(*parent));
> -	/* Again, the revs are all reverse */
>  	for (i = 0; i < ents; i++)
> -		hashcpy((unsigned char *)(parent + i),
> -			ent[ents - 1 - i].item->sha1);
> +		hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
>  	diff_tree_combined(parent[0], parent + 1, ents - 1,
>  			   revs->dense_combined_merges, revs);
>  	return 0;

Interesting.

Somehow 0fe7c1d (built-in diff: assorted updates., 2006-04-29) thought
that setup_revisions() give the ent[] list in reverse order.  This was a
thinko.  I somehow thought that setup_revisions() queued the revs in
reverse back then, and kept that misconception to this day, so I had to
look it up in the history.  It (meaning, the callchain from
setup_revisions to add_object) never did.

Perhaps the thinko was caused by discrepancy between the way internal
revision parser handles A..B and the way git-rev-parse parses it.  While
the internal revision parser parses it into "A ^B", rev-parse gives them
in reverse order, i.e. "B ^A" (which is not going to change).  In any
case, thanks for spotting this.

> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 1a6b522..fe6080d 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -258,6 +258,7 @@ diff --patch-with-stat -r initial..side
>  diff --patch-with-raw -r initial..side
>  diff --name-status dir2 dir
>  diff --no-index --name-status dir2 dir
> +diff master master^ side

As "diff master master^ side" would mean "the tip of master was made by
merging side into it, show that tip of master like 'git show' does", I
think this makes sense.

Re: [PATCH] "git diff <tree>{3,}": do not reverse order of arguments

[Mikael Magnusson]

2008/10/11 Junio C Hamano <gitster@pobox.com>:
> Perhaps the thinko was caused by discrepancy between the way internal
> revision parser handles A..B and the way git-rev-parse parses it.  While
> the internal revision parser parses it into "A ^B", rev-parse gives them
> in reverse order, i.e. "B ^A" (which is not going to change).  In any
> case, thanks for spotting this.

Ehm, do you mean the internal parses it into "A ^B" and rev-parse gives "^B A"?

-- 
Mikael Magnusson

Re: [PATCH] "git diff <tree>{3,}": do not reverse order of arguments

[Junio C Hamano]

"Mikael Magnusson" <mikachu@gmail.com> writes:

> 2008/10/11 Junio C Hamano <gitster@pobox.com>:
>> Perhaps the thinko was caused by discrepancy between the way internal
>> revision parser handles A..B and the way git-rev-parse parses it.  While
>> the internal revision parser parses it into "A ^B", rev-parse gives them
>> in reverse order, i.e. "B ^A" (which is not going to change).  In any
>> case, thanks for spotting this.
>
> Ehm, do you mean the internal parses it into "A ^B" and rev-parse gives "^B A"?

Sorry; a typo is in my description on the result from the internal parser.

For A..B, rev-parse gives B^ then A, and internal gives ^A in ent[0] and B
in ent[1].