# [PATCH] "git diff {3,}": do not reverse order of arguments 4 messages from 2008-10-11 to 2008-10-11. Participants: Matt McCutchen, Junio C Hamano, Mikael Magnusson. Thread: https://gitlist.dev/t/15856 ## Matt McCutchen, 2008-10-11 01:56 Subject: [PATCH] "git diff {3,}": do not reverse order of arguments Message-ID: <1223690175.2828.26.camel@mattlaptop2.local> URL: https://gitlist.dev/e/1223690175.2828.26.camel%40mattlaptop2.local ``` 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 --- 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 ``` ## Junio C Hamano, 2008-10-11 11:29 Subject: Re: [PATCH] "git diff {3,}": do not reverse order of arguments Message-ID: <7vwsgfjrp6.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwsgfjrp6.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1223690175.2828.26.camel@mattlaptop2.local> ``` Matt McCutchen 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. ``` ## Mikael Magnusson, 2008-10-11 12:53 Subject: Re: [PATCH] "git diff {3,}": do not reverse order of arguments Message-ID: <237967ef0810110553r662df370ud1ec34de402bfe1c@mail.gmail.com> URL: https://gitlist.dev/e/237967ef0810110553r662df370ud1ec34de402bfe1c%40mail.gmail.com In-Reply-To: <7vwsgfjrp6.fsf@gitster.siamese.dyndns.org> ``` 2008/10/11 Junio C Hamano : > 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, 2008-10-11 21:36 Subject: Re: [PATCH] "git diff {3,}": do not reverse order of arguments Message-ID: <7vtzbiizk8.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzbiizk8.fsf%40gitster.siamese.dyndns.org In-Reply-To: <237967ef0810110553r662df370ud1ec34de402bfe1c@mail.gmail.com> ``` "Mikael Magnusson" writes: > 2008/10/11 Junio C Hamano : >> 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]. ```