threads / patch / 15856

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

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

## tl;dr

4 messages between Oct 11, 2008 and Oct 11, 2008. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Matt McCutchen· Oct 11, 2008, 01:56 UTC · lore

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
Show changes to 3 files +29 −3

builtin-diff.c, t/t4013-diff-various.sh, 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
Junio C Hamano· Oct 11, 2008, 11:29 UTC · re: Matt McCutchen · lore

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

Matt McCutchen <matt@mattmccutchen.net> writes:
Show 16 quoted lines
> 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.

Show 9 quoted lines
> 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.

Mikael Magnusson· Oct 11, 2008, 12:53 UTC · re: Junio C Hamano · lore

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

2008/10/11 Junio C Hamano <gitster@pobox.com>:
Show 5 quoted lines
> 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
Junio C Hamano· Oct 11, 2008, 21:36 UTC · re: Mikael Magnusson · lore

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

"Mikael Magnusson" <mikachu@gmail.com> writes:
Show 8 quoted lines
> 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].

← back to recent threads