{"thread":{"id":"45313","subject":"[PATCH] diff: allow \"-\" as a short-hand for \"last branch\"","startedAt":"2017-03-08T11:34:45Z","lastAt":"2017-03-10T05:02:11Z","messageCount":4,"participants":["mash","Siddharth Kannan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313487","messageId":"1clZj4-0006vN-9q@crossperf.com","threadId":"45313","inReplyTo":null,"subject":"[PATCH] diff: allow \"-\" as a short-hand for \"last branch\"","fromName":"mash","fromEmail":"mash+git@crossperf.com","sentAt":"2017-03-08T09:50:53Z","receivedAt":"2017-03-08T11:34:45Z","isPatch":true,"sender":{"key":"mash+git@crossperf.com","avatar":null},"body":"Just like \"git merge -\" is a short-hand for \"git merge @{-1}\" to\nconveniently merge the previous branch, \"git diff -\" is a short-hand for\n\"git diff @{-1}\" to conveniently diff against the previous branch.\n\nAllow the usage of \"-\" in the dot dot notation to allow the use of\n\"git diff -..HEAD^\" as a short-hand for \"git diff @{-1}..HEAD^\".\n\nSigned-off-by: mash <mash+git@crossperf.com>\n---\nThis is a GSoC microproject. I'm not sure how useful this change is.\nPlease review it and take it apart.\n\nI'm not very happy with the change of handle_revision_arg.\nMaybe I should teach sha1_name.c:get_sha1_basic how to handle a dash\ninstead.\n\nDocumentation was not updated. I could only think of updating revisions.txt but\nthat might be misleading since the use of dash does not work everywhere.\n\n revision.c           | 22 ++++++++++++++++++--\n t/t4063-diff-last.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+), 2 deletions(-)\n create mode 100755 t/t4063-diff-last.sh\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec..c331bd5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1439,6 +1439,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \tconst char *arg = arg_;\n \tint cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;\n \tunsigned get_sha1_flags = 0;\n+\tstatic const char previous_branch[] = \"@{-1}\";\n \n \tflags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;\n \n@@ -1457,6 +1458,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \n \t\tif (!*next)\n \t\t\tnext = head_by_default;\n+\t\telse if (!strcmp(next, \"-\"))\n+\t\t\tnext = previous_branch;\n \t\tif (dotdot == arg)\n \t\t\tthis = head_by_default;\n \t\tif (this == head_by_default && next == head_by_default &&\n@@ -1469,6 +1472,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\t*dotdot = '.';\n \t\t\t\treturn -1;\n \t\t\t}\n+\t\t} else if (!strcmp(this, \"-\")) {\n+\t\t\tthis = previous_branch;\n \t\t}\n \t\tif (!get_sha1_committish(this, from_sha1) &&\n \t\t    !get_sha1_committish(next, sha1)) {\n@@ -1568,6 +1573,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \tif (revarg_opt & REVARG_COMMITTISH)\n \t\tget_sha1_flags = GET_SHA1_COMMITTISH;\n \n+\tif (!strcmp(arg, \"-\"))\n+\t\targ = previous_branch;\n \tif (get_sha1_with_context(arg, get_sha1_flags, sha1, &oc))\n \t\treturn revs->ignore_missing ? 0 : -1;\n \tif (!cant_be_filename)\n@@ -1578,6 +1585,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \treturn 0;\n }\n \n+/*\n+ * Check if the argument is supposed to be a revision argument instead of an\n+ * option even though it starts with a dash.\n+ */\n+static int is_revision_arg(const char *arg)\n+{\n+\treturn *arg == '\\0' || starts_with(arg, \"..\");\n+}\n+\n struct cmdline_pathspec {\n \tint alloc;\n \tint nr;\n@@ -1621,7 +1637,9 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t\tseen_dashdash = 1;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\t\tif (!is_revision_arg(sb.buf + 1)) {\n+\t\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\t\t}\n \t\t}\n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n@@ -2205,7 +2223,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tread_from_stdin = 0;\n \tfor (left = i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n-\t\tif (*arg == '-') {\n+\t\tif (*arg == '-' && !is_revision_arg(arg + 1)) {\n \t\t\tint opts;\n \n \t\t\topts = handle_revision_pseudo_opt(submodule,\ndiff --git a/t/t4063-diff-last.sh b/t/t4063-diff-last.sh\nnew file mode 100755\nindex 0000000..1f635cb\n--- /dev/null\n+++ b/t/t4063-diff-last.sh\n@@ -0,0 +1,58 @@\n+#!/bin/sh\n+\n+test_description='diff against last branch'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hello >world &&\n+\tgit add world &&\n+\tgit commit -m initial &&\n+\tgit branch other &&\n+\techo \"hello again\" >>world &&\n+\tgit add world &&\n+\tgit commit -m second\n+'\n+\n+test_expect_success '\"diff -\" does not work initially' '\n+\ttest_must_fail git diff -\n+'\n+\n+test_expect_success '\"diff -\" diffs against previous branch' '\n+\tgit checkout other &&\n+\n+\tcat <<-\\EOF >expect &&\n+\tdiff --git a/world b/world\n+\tindex c66f159..ce01362 100644\n+\t--- a/world\n+\t+++ b/world\n+\t@@ -1,2 +1 @@\n+\t hello\n+\t-hello again\n+\tEOF\n+\n+\tgit diff - >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success '\"diff -..\" diffs against previous branch' '\n+\tgit diff -.. >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success '\"diff ..-\" diffs inverted' '\n+\tcat <<-\\EOF >expect &&\n+\tdiff --git a/world b/world\n+\tindex ce01362..c66f159 100644\n+\t--- a/world\n+\t+++ b/world\n+\t@@ -1 +1,2 @@\n+\t hello\n+\t+hello again\n+\tEOF\n+\n+\tgit diff ..- >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_done\n-- \n2.9.3\n"},{"id":"313676","messageId":"1cm4dm-0007OE-MZ@crossperf.com","threadId":"45313","inReplyTo":"1clZj4-0006vN-9q@crossperf.com","subject":"[PATCH v2 GSoC RFC] diff: allow \"-\" as a short-hand for \"last branch\"","fromName":"mash","fromEmail":"mash+git@crossperf.com","sentAt":"2017-03-09T20:26:24Z","receivedAt":"2017-03-09T20:26:40Z","isPatch":true,"sender":{"key":"mash+git@crossperf.com","avatar":null},"body":"Just like \"git merge -\" is a short-hand for \"git merge @{-1}\" to\nconveniently merge the previous branch, \"git diff -\" is a short-hand for\n\"git diff @{-1}\" to conveniently diff against the previous branch.\n\nAllow the usage of \"-\" in the dot dot notation to allow the use of\n\"git diff -..HEAD^\" as a short-hand for \"git diff @{-1}..HEAD^\".\n\nSigned-off-by: mash <mash+git@crossperf.com>\n---\nAdd tests to confirm that passing in this short-hand from stdin works.\n\nHandling the dash in sha1_name:get_sha1_basic is not an issue but git was\ndesigned with the dash in mind for options not for this weird short-hand so as\nlong as there's no decision made that git should actually have this short-hand\neverywhere it does not seem like a good idea to change anything in there\nbecause it would probably have unwanted side-effects.\n\nFor example for now just handle_revision_arg was modified which is mainly used\nby git diff but also used in builtin/pack-objects.c:get_object_list#2785 which\nis terrible since this may have already introduced an unwanted the side-effect.\n\nBypassing the whatever starts with a dash is always an option filters is not a\nnice thing to do either.\n\nOverall I see the benefit of the dash short-hand when doing checkouts since\nit's very similar to \"cd -\" and switching between two branches is something one\nwould commonly do but for every git command where executing it twice results\ninto the same action achieving the same result twice it seems like a not that\nuseful short-hand.\n\nExample:\nmaster# g co maint\nmaint# g co -  # Switch to master\nmaster# g co -  # Switch to maint (different result)\nmaint# g d -  # diff master..HEAD\nmaint# g d -  # diff master..HEAD (same result - less useful)\n\nThis is obviously only my own option.\nBecause of what was stated above I've now marked this as open for discussion\nsince I'm currently not convinced that applying the patch is a good idea.\n\n revision.c           | 22 +++++++++++++++--\n t/t4063-diff-last.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 88 insertions(+), 2 deletions(-)\n create mode 100755 t/t4063-diff-last.sh\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec..c331bd5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1439,6 +1439,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \tconst char *arg = arg_;\n \tint cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;\n \tunsigned get_sha1_flags = 0;\n+\tstatic const char previous_branch[] = \"@{-1}\";\n \n \tflags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;\n \n@@ -1457,6 +1458,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \n \t\tif (!*next)\n \t\t\tnext = head_by_default;\n+\t\telse if (!strcmp(next, \"-\"))\n+\t\t\tnext = previous_branch;\n \t\tif (dotdot == arg)\n \t\t\tthis = head_by_default;\n \t\tif (this == head_by_default && next == head_by_default &&\n@@ -1469,6 +1472,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\t*dotdot = '.';\n \t\t\t\treturn -1;\n \t\t\t}\n+\t\t} else if (!strcmp(this, \"-\")) {\n+\t\t\tthis = previous_branch;\n \t\t}\n \t\tif (!get_sha1_committish(this, from_sha1) &&\n \t\t    !get_sha1_committish(next, sha1)) {\n@@ -1568,6 +1573,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \tif (revarg_opt & REVARG_COMMITTISH)\n \t\tget_sha1_flags = GET_SHA1_COMMITTISH;\n \n+\tif (!strcmp(arg, \"-\"))\n+\t\targ = previous_branch;\n \tif (get_sha1_with_context(arg, get_sha1_flags, sha1, &oc))\n \t\treturn revs->ignore_missing ? 0 : -1;\n \tif (!cant_be_filename)\n@@ -1578,6 +1585,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \treturn 0;\n }\n \n+/*\n+ * Check if the argument is supposed to be a revision argument instead of an\n+ * option even though it starts with a dash.\n+ */\n+static int is_revision_arg(const char *arg)\n+{\n+\treturn *arg == '\\0' || starts_with(arg, \"..\");\n+}\n+\n struct cmdline_pathspec {\n \tint alloc;\n \tint nr;\n@@ -1621,7 +1637,9 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t\tseen_dashdash = 1;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\t\tif (!is_revision_arg(sb.buf + 1)) {\n+\t\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\t\t}\n \t\t}\n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n@@ -2205,7 +2223,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tread_from_stdin = 0;\n \tfor (left = i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n-\t\tif (*arg == '-') {\n+\t\tif (*arg == '-' && !is_revision_arg(arg + 1)) {\n \t\t\tint opts;\n \n \t\t\topts = handle_revision_pseudo_opt(submodule,\ndiff --git a/t/t4063-diff-last.sh b/t/t4063-diff-last.sh\nnew file mode 100755\nindex 0000000..dc28f9d\n--- /dev/null\n+++ b/t/t4063-diff-last.sh\n@@ -0,0 +1,68 @@\n+#!/bin/sh\n+\n+test_description='diff against last branch'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hello >world &&\n+\tgit add world &&\n+\tgit commit -m initial &&\n+\tgit branch other &&\n+\techo \"hello again\" >>world &&\n+\tgit add world &&\n+\tgit commit -m second\n+'\n+\n+test_expect_success '\"diff -\" does not work initially' '\n+\ttest_must_fail git diff -\n+'\n+\n+test_expect_success '\"diff -\" diffs against previous branch' '\n+\tgit checkout other &&\n+\n+\tcat <<-\\EOF >expect &&\n+\tdiff --git a/world b/world\n+\tindex c66f159..ce01362 100644\n+\t--- a/world\n+\t+++ b/world\n+\t@@ -1,2 +1 @@\n+\t hello\n+\t-hello again\n+\tEOF\n+\n+\tgit diff - >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success '\"diff -\" arguments from stdin' '\n+\techo \"-\" | git diff --stdin >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success '\"diff -..\" diffs against previous branch' '\n+\tgit diff -.. >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success '\"diff -..\" arguments from stdin' '\n+\techo \"-..\" | git diff --stdin >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success '\"diff ..-\" diffs inverted' '\n+\tcat <<-\\EOF >expect &&\n+\tdiff --git a/world b/world\n+\tindex ce01362..c66f159 100644\n+\t--- a/world\n+\t+++ b/world\n+\t@@ -1 +1,2 @@\n+\t hello\n+\t+hello again\n+\tEOF\n+\n+\tgit diff ..- >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_done\n-- \n2.9.3\n"},{"id":"313731","messageId":"1cmCXH-0000ND-9K@crossperf.com","threadId":"45313","inReplyTo":"1cm4dm-0007OE-MZ@crossperf.com","subject":"RE: [PATCH v2 GSoC RFC] diff: allow \"-\" as a short-hand for \"last branch\"","fromName":"mash","fromEmail":"mash+git@crossperf.com","sentAt":"2017-03-10T04:52:07Z","receivedAt":"2017-03-10T04:52:33Z","isPatch":true,"sender":{"key":"mash+git@crossperf.com","avatar":null},"body":"> From the discussion over the different versions of my patch, I get\n> the feeling that enabling this shorthand for all the commands is the\n> direction that git wants to move in.\n\nInteresting.\n\n> Sorry about the time you spent on this patch.\n\nDon't worry about it. I just seem to be too stupid to search through the\nmailing list archive properly.\n\nMaybe you can reuse the diff tests. I'll do another microproject then.\n\nmash\n\nThe original message doesn't seem to cc the mailing list:\n> Hey, I have already worked on this, and I made the change inside\n> sha1_name.c.\n\n> The final version of my patch is here[1].\n\n> > Handling the dash in sha1_name:get_sha1_basic is not an issue but git\n> > was designed with the dash in mind for options not for this weird\n> > short-hand so as long as there's no decision made that git should\n> > actually have this short-hand everywhere it does not seem like a good\n> > idea to change anything in there because it would probably have\n> > unwanted side-effects.\n\n> Actually, this was discussed even when I was working on this patch.\n\n> I said [2]\n\n> > Making a change in sha1_name.c will touch a lot of commands\n> > (setup_revisions is called from everywhere in the codebase), so, I am\n> > still trying to figure out how to do this such that the rest of the\n> > codepath remains unchanged.\n\n> Matthieu replied to this [3]\n\n> > I don't have strong opinion on this: I tend to favor consistency and\n> > supporting \"-\" everywhere goes in this direction, but I think the\n> > downsides should be considered too. A large part of the exercice here\n> > is to write a good commit message!\n\n> From the discussion over the different versions of my patch, I get\n> the feeling that enabling this shorthand for all the commands is the\n> direction that git wants to move in.\n\n> Sorry about the time you spent on this patch.\n\n> [1]: http://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com/\n> [2]: https://public-inbox.org/git/20170207191450.GA5569@ubuntu-512mb-blr1-01.localdomain/\n> [3]: https://public-inbox.org/git/vpqh944eof7.fsf@anie.imag.fr/\n\n> Thanks,\n> Siddharth.\n"},{"id":"313733","messageId":"20170310050046.GB2417@instance-1.c.mfqp-source.internal","threadId":"45313","inReplyTo":"1cmCXH-0000ND-9K@crossperf.com","subject":"Re: [PATCH v2 GSoC RFC] diff: allow \"-\" as a short-hand for \"last branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-03-10T05:00:46Z","receivedAt":"2017-03-10T05:02:11Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"On Fri, Mar 10, 2017 at 04:52:07AM +0000, mash wrote:\n> Maybe you can reuse the diff tests. I'll do another microproject then.\n\nYeah, definitely. If there are more tests required, then I will reuse\nyour ones!\n> \n> mash\n> \n> The original message doesn't seem to cc the mailing list:\n\nThanks! It was rather daft of me to not realise this. I was waiting\nfor it to appear on public-inbox.\n\nI re-sent it with the CC. The timestamp is a little bit\nskewed, but I think it should make sense.\n\nThanks,\nSiddharth.\n"}]}