{"thread":{"id":"15856","subject":"[PATCH] \"git diff {3,}\": do not reverse order of arguments","startedAt":"2008-10-11T01:56:15Z","lastAt":"2008-10-11T21:36:55Z","messageCount":4,"participants":["Matt McCutchen","Junio C Hamano","Mikael Magnusson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"92780","messageId":"1223690175.2828.26.camel@mattlaptop2.local","threadId":"15856","inReplyTo":null,"subject":"[PATCH] \"git diff {3,}\": do not reverse order of arguments","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-10-11T01:56:15Z","receivedAt":"2008-10-11T01:56:15Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"According to the message of commit 0fe7c1de16f71312e6adac4b85bddf0d62a47168,\n\"git diff\" with three or more trees expects the merged tree first followed by\nthe parents, in order. However, this command reversed the order of its\narguments, resulting in confusing diffs. A comment /* Again, the revs are all\nreverse */ suggested there was a reason for this, but I can't figure out the\nreason, so I removed the reversal of the arguments. Test case included.\n\nSigned-off-by: Matt McCutchen \n---\n builtin-diff.c | 4 +---\n t/t4013-diff-various.sh | 1 +\n t/t4013/diff.diff_master_master^_side | 29 +++++++++++++++++++++++++++++\n 3 files changed, 31 insertions(+), 3 deletions(-)\n create mode 100644 t/t4013/diff.diff_master_master^_side\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 35da366..9c8c295 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -177,10 +177,8 @@ static int builtin_diff_combined(struct rev_info *revs,\n \tif (!revs->dense_combined_merges && !revs->combine_merges)\n \t\trevs->dense_combined_merges = revs->combine_merges = 1;\n \tparent = xmalloc(ents * sizeof(*parent));\n-\t/* Again, the revs are all reverse */\n \tfor (i = 0; i < ents; i++)\n-\t\thashcpy((unsigned char *)(parent + i),\n-\t\t\tent[ents - 1 - i].item->sha1);\n+\t\thashcpy((unsigned char *)(parent + i), ent[i].item->sha1);\n \tdiff_tree_combined(parent[0], parent + 1, ents - 1,\n \t\t\t revs->dense_combined_merges, revs);\n \treturn 0;\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 1a6b522..fe6080d 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -258,6 +258,7 @@ diff --patch-with-stat -r initial..side\n diff --patch-with-raw -r initial..side\n diff --name-status dir2 dir\n diff --no-index --name-status dir2 dir\n+diff master master^ side\n EOF\n \n test_done\ndiff --git a/t/t4013/diff.diff_master_master^_side b/t/t4013/diff.diff_master_master^_side\nnew file mode 100644\nindex 0000000..50ec9ca\n--- /dev/null\n+++ b/t/t4013/diff.diff_master_master^_side\n@@ -0,0 +1,29 @@\n+$ git diff master master^ side\n+diff --cc dir/sub\n+index cead32e,7289e35..992913c\n+--- a/dir/sub\n++++ b/dir/sub\n+@@@ -1,6 -1,4 +1,8 @@@\n+ A\n+ B\n+ +C\n+ +D\n+ +E\n+ +F\n++ 1\n++ 2\n+diff --cc file0\n+index b414108,f4615da..10a8a9f\n+--- a/file0\n++++ b/file0\n+@@@ -1,6 -1,6 +1,9 @@@\n+ 1\n+ 2\n+ 3\n+ +4\n+ +5\n+ +6\n++ A\n++ B\n++ C\n+$\n-- \n1.6.0.2.519.gf6ee\n"},{"id":"92790","messageId":"7vwsgfjrp6.fsf@gitster.siamese.dyndns.org","threadId":"15856","inReplyTo":"1223690175.2828.26.camel@mattlaptop2.local","subject":"Re: [PATCH] \"git diff {3,}\": do not reverse order of arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-11T11:29:09Z","receivedAt":"2008-10-11T11:29:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt McCutchen writes:\n\n> diff --git a/builtin-diff.c b/builtin-diff.c\n> index 35da366..9c8c295 100644\n> --- a/builtin-diff.c\n> +++ b/builtin-diff.c\n> @@ -177,10 +177,8 @@ static int builtin_diff_combined(struct rev_info *revs,\n> \tif (!revs->dense_combined_merges && !revs->combine_merges)\n> \t\trevs->dense_combined_merges = revs->combine_merges = 1;\n> \tparent = xmalloc(ents * sizeof(*parent));\n> -\t/* Again, the revs are all reverse */\n> \tfor (i = 0; i < ents; i++)\n> -\t\thashcpy((unsigned char *)(parent + i),\n> -\t\t\tent[ents - 1 - i].item->sha1);\n> +\t\thashcpy((unsigned char *)(parent + i), ent[i].item->sha1);\n> \tdiff_tree_combined(parent[0], parent + 1, ents - 1,\n> \t\t\t revs->dense_combined_merges, revs);\n> \treturn 0;\n\nInteresting.\n\nSomehow 0fe7c1d (built-in diff: assorted updates., 2006-04-29) thought\nthat setup_revisions() give the ent[] list in reverse order. This was a\nthinko. I somehow thought that setup_revisions() queued the revs in\nreverse back then, and kept that misconception to this day, so I had to\nlook it up in the history. It (meaning, the callchain from\nsetup_revisions to add_object) never did.\n\nPerhaps the thinko was caused by discrepancy between the way internal\nrevision parser handles A..B and the way git-rev-parse parses it. While\nthe internal revision parser parses it into \"A ^B\", rev-parse gives them\nin reverse order, i.e. \"B ^A\" (which is not going to change). In any\ncase, thanks for spotting this.\n\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 1a6b522..fe6080d 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -258,6 +258,7 @@ diff --patch-with-stat -r initial..side\n> diff --patch-with-raw -r initial..side\n> diff --name-status dir2 dir\n> diff --no-index --name-status dir2 dir\n> +diff master master^ side\n\nAs \"diff master master^ side\" would mean \"the tip of master was made by\nmerging side into it, show that tip of master like 'git show' does\", I\nthink this makes sense.\n"},{"id":"92797","messageId":"237967ef0810110553r662df370ud1ec34de402bfe1c@mail.gmail.com","threadId":"15856","inReplyTo":"7vwsgfjrp6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] \"git diff {3,}\": do not reverse order of arguments","fromName":"Mikael Magnusson","fromEmail":"mikachu@gmail.com","sentAt":"2008-10-11T12:53:12Z","receivedAt":"2008-10-11T12:53:12Z","isPatch":true,"sender":{"key":"mikachu@gmail.com","avatar":null},"body":"2008/10/11 Junio C Hamano :\n> Perhaps the thinko was caused by discrepancy between the way internal\n> revision parser handles A..B and the way git-rev-parse parses it. While\n> the internal revision parser parses it into \"A ^B\", rev-parse gives them\n> in reverse order, i.e. \"B ^A\" (which is not going to change). In any\n> case, thanks for spotting this.\n\nEhm, do you mean the internal parses it into \"A ^B\" and rev-parse gives \"^B A\"?\n\n-- \nMikael Magnusson\n"},{"id":"92811","messageId":"7vtzbiizk8.fsf@gitster.siamese.dyndns.org","threadId":"15856","inReplyTo":"237967ef0810110553r662df370ud1ec34de402bfe1c@mail.gmail.com","subject":"Re: [PATCH] \"git diff {3,}\": do not reverse order of arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-11T21:36:55Z","receivedAt":"2008-10-11T21:36:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Mikael Magnusson\" writes:\n\n> 2008/10/11 Junio C Hamano :\n>> Perhaps the thinko was caused by discrepancy between the way internal\n>> revision parser handles A..B and the way git-rev-parse parses it. While\n>> the internal revision parser parses it into \"A ^B\", rev-parse gives them\n>> in reverse order, i.e. \"B ^A\" (which is not going to change). In any\n>> case, thanks for spotting this.\n>\n> Ehm, do you mean the internal parses it into \"A ^B\" and rev-parse gives \"^B A\"?\n\nSorry; a typo is in my description on the result from the internal parser.\n\nFor A..B, rev-parse gives B^ then A, and internal gives ^A in ent[0] and B\nin ent[1].\n"}]}