{"thread":{"id":"26844","subject":"[PATCH] diff: remove dead code that flips arguments order","startedAt":"2011-03-23T21:36:43Z","lastAt":"2011-03-28T16:53:54Z","messageCount":10,"participants":["Junio C Hamano","Jakub Narebski","Jay Soffian","Johannes Sixt","Ævar Arnfjörð Bjarmason","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164184","messageId":"7voc51cydw.fsf@alter.siamese.dyndns.org","threadId":"26844","inReplyTo":null,"subject":"[PATCH] diff: remove dead code that flips arguments order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-23T21:36:43Z","receivedAt":"2011-03-23T21:36:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In the olden days, \"git cmd A..B\" used to push ^A and then B into the list\nof parsed revisions. The output from \"git rev-list A..B\" still does this,\nand shows \"B\" first and then \"^A\" for backward compatibility.\n\nThe command line parser for \"git diff A..B\" was written originally to take\nthis swappage into account; when it internally sees an uninteresting\ncommit as its second input argument, it swapped the arguments to make it\nthe origin of the comparison, and the other one the destination.\n\nThese days, there is no need for this swappage, as the internal machinery\nfeeds ^A and then B to builtin_diff_tree(). Remove the dead code.\n\nThe funny thing is, I think this was a dead code from day one when it\nwas first written 6505602 (built-in diff., 2006-04-28); the author of\nthat patch must have known the implementation of the scripted version\nthat needed to deal with the swapped output from rev-parse too deeply\nand didn't realize that the gotcha the dead code attempts to protect\nhim from didn't even apply to this codepath.\n\nThis change _will_ regress if somebody is insane enough to manually call\n\"git diff B ^A\", but that is simply too insane for me to care.\n\nThere is exactly the same tree swapping code in diff-tree; that is not to\nbe changed not to break \"git diff-tree B ^A\" issued by Porcelain scripts,\nbut reword the comment to explain why we swap trees a bit better.\n\nSigned-off-by: Junio C Hamano \n---\n\n builtin/diff-tree.c | 4 ++--\n builtin/diff.c | 13 +------------\n 2 files changed, 3 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 0d2a3e9..c843546 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -133,8 +133,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t}\n \n \t/*\n-\t * NOTE! We expect \"a ^b\" to be equal to \"a..b\", so we\n-\t * reverse the order of the objects if the second one\n+\t * NOTE! We expect \"b ^a\" to have come from \"rev-parse a..b\",\n+\t * so we reverse the order of the objects if the second one\n \t * is marked UNINTERESTING.\n \t */\n \tnr_sha1 = opt->pending.nr;\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 4c9deb2..86614d4 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -146,20 +146,9 @@ static int builtin_diff_tree(struct rev_info *revs,\n \t\t\t int argc, const char **argv,\n \t\t\t struct object_array_entry *ent)\n {\n-\tconst unsigned char *(sha1[2]);\n-\tint swap = 0;\n-\n \tif (argc > 1)\n \t\tusage(builtin_diff_usage);\n-\n-\t/* We saw two trees, ent[0] and ent[1].\n-\t * if ent[1] is uninteresting, they are swapped\n-\t */\n-\tif (ent[1].item->flags & UNINTERESTING)\n-\t\tswap = 1;\n-\tsha1[swap] = ent[0].item->sha1;\n-\tsha1[1-swap] = ent[1].item->sha1;\n-\tdiff_tree_sha1(sha1[0], sha1[1], \"\", &revs->diffopt);\n+\tdiff_tree_sha1(ent[0].item->sha1, ent[1].item->sha1, \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n \treturn 0;\n }\n"},{"id":"164186","messageId":"7vei5xcxzm.fsf@alter.siamese.dyndns.org","threadId":"26844","inReplyTo":"7voc51cydw.fsf@alter.siamese.dyndns.org","subject":"[PATCH] warn use of \"git diff A..B\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-23T21:45:17Z","receivedAt":"2011-03-23T21:45:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git diff\" (and \"diff-tree\") accepts a range notation \"A..B\" from the\ncommand line to specify the two endpoints to be compared; the right way to\nspell this would be \"git diff A B\". This is merely a historical accident\nthat comes from the fact that \"git log\" family of commands and \"git diff\"\nhappens to share some code in their command line parsers.\n\nIt was fine for people who are used to seeing \"git diff A..B\" and silently\ntranslating it to \"git diff A B\" in their head, but it made things hard\nfor new people. It is easy to mistakenly think that \"git diff A..B\" has\nsome similarity with \"git log A..B\" (there isn't).\n\nIndeed, \"git log A..B\" computes for the range \"A..B\" a set of commits that\nare in B but not in A, and if one misunderstands \"git diff\" to somehow\nmagically work with ranges (it doesn't), it is more natural to expect that\n\"git diff A..B\" might show a cumulative changes since B forked from A.\n\nBut that is not what the command gives (it gives the difference between\ntwo endpoints and there is no history cosideration such as \"B forked from\nA\").\n\nNew people can be trained not to say \"git diff A..B\" when they mean to\ncompare two endpoints with \"git diff A B\", and that would reduce the\nconfusion greatly.\n\nWarn the use of \"git diff A..B\" syntax when \"git diff A B\" equally works\nwell, is shorter, and is much more clear what the command is comparing\n(i.e. two endpoints).\n\nThe new code does not issue a warning against \"git diff ..B\" that is used\nas a shorthand for \"git diff HEAD B\", and \"git diff A..\" that is used as a\nshorthand for \"git diff A HEAD\", respectively. These are shorter to type\nand are often useful.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Cumulative effect can be had with \"git diff $(git merge-base A B) B\"\n and there is a short-hand for it \"git diff A...B\", but this change is\n about the two-dot notation, not three-dots.\n\n builtin/diff.c | 16 ++++++++++++++++\n revision.c | 6 ++++--\n revision.h | 1 +\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 86614d4..6d8028b 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -142,12 +142,28 @@ static int builtin_diff_index(struct rev_info *revs,\n \treturn run_diff_index(revs, cached);\n }\n \n+static void check_useless_use_of_range(struct object_array_entry *ent)\n+{\n+\tif (!(ent[0].item->flags & UNINTERESTING) ||\n+\t ent[1].item->flags & UNINTERESTING)\n+\t\treturn; /* not a range made by \"A..B\" notation */\n+\n+\tif ((ent[0].name == dotdot_default_HEAD) ||\n+\t (ent[1].name == dotdot_default_HEAD))\n+\t\treturn;\t/* \"A..\" or \"..B\" */\n+\n+\twarning(\"Do not write 'git diff A..B' but write 'git diff A B'\");\n+\twarning(\"diff is about two endpoints!\");\n+}\n+\n static int builtin_diff_tree(struct rev_info *revs,\n \t\t\t int argc, const char **argv,\n \t\t\t struct object_array_entry *ent)\n {\n \tif (argc > 1)\n \t\tusage(builtin_diff_usage);\n+\n+\tcheck_useless_use_of_range(ent);\n \tdiff_tree_sha1(ent[0].item->sha1, ent[1].item->sha1, \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n \treturn 0;\ndiff --git a/revision.c b/revision.c\nindex e96c281..48a0a44 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1008,6 +1008,8 @@ static void prepare_show_merge(struct rev_info *revs)\n \trevs->limited = 1;\n }\n \n+const char dotdot_default_HEAD[] = \"HEAD\";\n+\n int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\t\tint flags,\n \t\t\tint cant_be_filename)\n@@ -1030,9 +1032,9 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\tnext += symmetric;\n \n \t\tif (!*next)\n-\t\t\tnext = \"HEAD\";\n+\t\t\tnext = dotdot_default_HEAD;\n \t\tif (dotdot == arg)\n-\t\t\tthis = \"HEAD\";\n+\t\t\tthis = dotdot_default_HEAD;\n \t\tif (!get_sha1(this, from_sha1) &&\n \t\t !get_sha1(next, sha1)) {\n \t\t\tstruct commit *a, *b;\ndiff --git a/revision.h b/revision.h\nindex ae94860..27a233c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -151,6 +151,7 @@ struct rev_info {\n /* revision.c */\n typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);\n extern volatile show_early_output_fn_t show_early_output;\n+extern const char dotdot_default_HEAD[];\n \n struct setup_revision_opt {\n \tconst char *def;\n"},{"id":"164237","messageId":"m3tyesv7zh.fsf@localhost.localdomain","threadId":"26844","inReplyTo":"7vei5xcxzm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-24T15:44:57Z","receivedAt":"2011-03-24T15:44:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano writes:\n\n> \"git diff\" (and \"diff-tree\") accepts a range notation \"A..B\" from the\n> command line to specify the two endpoints to be compared; the right way to\n> spell this would be \"git diff A B\". This is merely a historical accident\n> that comes from the fact that \"git log\" family of commands and \"git diff\"\n> happens to share some code in their command line parsers.\n\nI think it is quite useful to acept this notation to allow for\ncopy'n'paste from e.g. \"git fetch\" output:\n\n 5e839c8..cd3065f master -> origin\n\nOn can simply paste \"git diff 5e839c8..cd3065f\".\n ^^^^^^^^^^^^^^^^\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"164245","messageId":"AANLkTikjN_90UApfD2rRksDk8qXyE6pJHhd00Aw-Qe7Y@mail.gmail.com","threadId":"26844","inReplyTo":"7vei5xcxzm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-03-24T18:11:34Z","receivedAt":"2011-03-24T18:11:34Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Mar 23, 2011 at 5:45 PM, Junio C Hamano wrote:\n> New people can be trained not to say \"git diff A..B\" when they mean to\n> compare two endpoints with \"git diff A B\", and that would reduce the\n> confusion greatly.\n\nI think this is ill-advised because...\n\n> Warn the use of \"git diff A..B\" syntax when \"git diff A B\" equally works\n> well, is shorter, and is much more clear what the command is comparing\n> (i.e. two endpoints).\n\n... this is not consistent ...\n\n> The new code does not issue a warning against \"git diff ..B\" that is used\n> as a shorthand for \"git diff HEAD B\", and \"git diff A..\" that is used as a\n> shorthand for \"git diff A HEAD\", respectively.  These are shorter to type\n> and are often useful.\n\n...with this.\n\nI don't think telling newbies \"Use diff A B instead of diff A..B, but\nA.. and ..B are okay\" reduces confusion any. i.e., this makes more\nsense to me:\n\n(1) One sided '..' implies HEAD on the other side with both git log and git diff\n(2) \"diff A..B\" is the same as \"diff A B\"\n\nTherefor:\n\n- \"diff A..\" is the same as \"diff A..HEAD\" (1) is the same as \"diff A HEAD\" (2).\n- \"diff ..B\" is the same as \"diff HEAD..B\" (1) is the same as \"diff HEAD B\" (2).\n\nAlso, the fact that a one-sided '..' implies HEAD on the other side\n\n\n> +static void check_useless_use_of_range(struct object_array_entry *ent)\n> +{\n> +       if (!(ent[0].item->flags & UNINTERESTING) ||\n> +           ent[1].item->flags & UNINTERESTING)\n> +               return; /* not a range made by \"A..B\" notation */\n> +\n> +       if ((ent[0].name == dotdot_default_HEAD) ||\n> +           (ent[1].name == dotdot_default_HEAD))\n> +               return; /* \"A..\" or \"..B\" */\n> +\n> +       warning(\"Do not write 'git diff A..B' but write 'git diff A B'\");\n> +       warning(\"diff is about two endpoints!\");\n> +}\n> +\n\nI use diff A..B all the time, because I've often just used log A..B,\nor I'm about to, and it's one less part of the command line to change.\nPlease make this warning squelch-able at least.\n\nj.\n"},{"id":"164246","messageId":"AANLkTiknZiZbEedOgJwRqwN_9APb2x_9opOFsXYe8cWJ@mail.gmail.com","threadId":"26844","inReplyTo":"AANLkTikjN_90UApfD2rRksDk8qXyE6pJHhd00Aw-Qe7Y@mail.gmail.com","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-03-24T18:16:28Z","receivedAt":"2011-03-24T18:16:28Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Mar 24, 2011 at 2:11 PM, Jay Soffian wrote:\n> Also, the fact that a one-sided '..' implies HEAD on the other side\n\nSorry, stray sentence. Ignore it.\n\nBut also, context matters. I don't get confused that Perl has a \"..\"\noperator which can be both a range operator and a flip-flop operator.\n\nOf all the things that git overloads (reset, checkout, ...), this\nseems the least confusing to explain.\n\n$0.03. :-)\n\nj.\n"},{"id":"164248","messageId":"7vtyes8j1z.fsf@alter.siamese.dyndns.org","threadId":"26844","inReplyTo":"AANLkTikjN_90UApfD2rRksDk8qXyE6pJHhd00Aw-Qe7Y@mail.gmail.com","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-24T18:33:44Z","receivedAt":"2011-03-24T18:33:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> On Wed, Mar 23, 2011 at 5:45 PM, Junio C Hamano wrote:\n> ... this is not consistent ...\n>\n>> The new code does not issue a warning against \"git diff ..B\" that is used\n>> as a shorthand for \"git diff HEAD B\", and \"git diff A..\" that is used as a\n>> shorthand for \"git diff A HEAD\", respectively.  These are shorter to type\n>> and are often useful.\n>\n> ...with this.\n\nI already _know_ it is not consistent. My verdict is that shoter-to-type\nand being useful trumps that minor consistency argument and that is why\nthe patch is like so, and that is why the proposed commit log message\nclearly describes the choice.\n\n> I use diff A..B all the time, because I've often just used log A..B,\n> or I'm about to, and it's one less part of the command line to change.\n\nOK. I hereby designate you as the official cluebat wearer responsible for\nexplaining things whenever new people get confused and say that the output\nfrom \"git diff A..B\" does not match what they expect from \"git log A..B\",\n(and \"git diff A...B\" vs \"git log A...B\") on this list.\n\nThanks ;-).\n"},{"id":"164277","messageId":"4D8C520E.1090409@viscovery.net","threadId":"26844","inReplyTo":"m3tyesv7zh.fsf@localhost.localdomain","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-25T08:27:58Z","receivedAt":"2011-03-25T08:27:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/24/2011 16:44, schrieb Jakub Narebski:\n> Junio C Hamano writes:\n> \n>> \"git diff\" (and \"diff-tree\") accepts a range notation \"A..B\" from the\n>> command line to specify the two endpoints to be compared; the right way to\n>> spell this would be \"git diff A B\". This is merely a historical accident\n>> that comes from the fact that \"git log\" family of commands and \"git diff\"\n>> happens to share some code in their command line parsers.\n> \n> I think it is quite useful to acept this notation to allow for\n> copy'n'paste from e.g. \"git fetch\" output:\n> \n> 5e839c8..cd3065f master -> origin\n> \n> On can simply paste \"git diff 5e839c8..cd3065f\".\n> ^^^^^^^^^^^^^^^^\n\nGood point! Of the inconsistencies that still remain in git, IMO, this is\nthe most bearable one, because it's very easy to explain.\n\n-- Hannes\n"},{"id":"164279","messageId":"AANLkTim4aQQgm290Z+OfHd7+oz8jQ_UfWQdxPdG_6k-H@mail.gmail.com","threadId":"26844","inReplyTo":"7vei5xcxzm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-03-25T09:06:11Z","receivedAt":"2011-03-25T09:06:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Mar 23, 2011 at 22:45, Junio C Hamano wrote:\n\nI agree with Jakub. I often write something like this:\n\n $ git log origin/master..origin/pu\n $ ^log^diff\n git diff origin/master..origin/pu\n\nIt would be annoying to have that start to warn.\n\n> +       warning(\"Do not write 'git diff A..B' but write 'git diff A B'\");\n> +       warning(\"diff is about two endpoints!\");\n\nAnd in any case this warning is way too terse to give the user an idea\nof what this is about.\n"},{"id":"164393","messageId":"AANLkTikQB_2bapZXOyANrO1fsKGs4r6HP1TOuisZXfnr@mail.gmail.com","threadId":"26844","inReplyTo":"7vei5xcxzm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2011-03-27T20:03:11Z","receivedAt":"2011-03-27T20:03:11Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Mar 23, 2011 at 22:45, Junio C Hamano wrote:\n> \"git diff\" (and \"diff-tree\") accepts a range notation \"A..B\" from the\n> command line to specify the two endpoints to be compared; the right way to\n> spell this would be \"git diff A B\".  This is merely a historical accident\n> that comes from the fact that \"git log\" family of commands and \"git diff\"\n> happens to share some code in their command line parsers.\n>\n> It was fine for people who are used to seeing \"git diff A..B\" and silently\n> translating it to \"git diff A B\" in their head, but it made things hard\n> for new people.  It is easy to mistakenly think that \"git diff A..B\" has\n> some similarity with \"git log A..B\" (there isn't).\n\nI like the dot-dot notation a lot.\n"},{"id":"164462","messageId":"7vei5r190d.fsf@alter.siamese.dyndns.org","threadId":"26844","inReplyTo":"AANLkTikQB_2bapZXOyANrO1fsKGs4r6HP1TOuisZXfnr@mail.gmail.com","subject":"Re: [PATCH] warn use of \"git diff A..B\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T16:53:54Z","receivedAt":"2011-03-28T16:53:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> I like the dot-dot notation a lot.\n\nThanks to be the third volunteer. I don't need any more volunteers ;-)\n"}]}