{"thread":{"id":"38825","subject":"[PATCH/RFC 0/2][GSoC] revision.c: Allow \"-\" as stand-in for \"@{-1}\" everywhere a branch is allowed","startedAt":"2015-03-16T15:11:41Z","lastAt":"2015-03-25T22:24:26Z","messageCount":9,"participants":["Kenny Lee Sin Cheong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"257786","messageId":"1426518703-15785-1-git-send-email-kenny.lee28@gmail.com","threadId":"38825","inReplyTo":null,"subject":"[PATCH/RFC 0/2][GSoC] revision.c: Allow \"-\" as stand-in for \"@{-1}\" everywhere a branch is allowed","fromName":"Kenny Lee Sin Cheong","fromEmail":"kenny.lee28@gmail.com","sentAt":"2015-03-16T15:11:41Z","receivedAt":"2015-03-16T15:11:41Z","isPatch":true,"sender":{"key":"kenny.lee28@gmail.com","avatar":null},"body":"This is an attempt at a microproject for GSoC\n\nAn attempt to add revision range support to Junio's JFF patch sent a few days ago. The first patch is the a copy of the one he posted.\n\nI was wondering if it was a good idea to add support for commands like \"<rev>..-\". Files that starts with \"-\" requires \"--\" or a \"./\" format but what if we have a file named \"next..-\" and call \"git log next..-\" ?\n\nJunio C Hamano (1):\n  \"-\" and \"@{-1}\" on various programs\n\nKenny Lee Sin Cheong (1):\n  Add revision range support on \"-\" and \"@{-1}\"\n\n builtin/checkout.c |  3 ---\n builtin/merge.c    |  3 +--\n builtin/revert.c   |  2 --\n revision.c         | 16 +++++++++++++--\n sha1_name.c        | 57 +++++++++++++++++++++++++++++++++---------------------\n 5 files changed, 50 insertions(+), 31 deletions(-)\n\n-- \n2.3.2.225.gebdc58a\n"},{"id":"257787","messageId":"1426518703-15785-2-git-send-email-kenny.lee28@gmail.com","threadId":"38825","inReplyTo":"1426518703-15785-1-git-send-email-kenny.lee28@gmail.com","subject":"[PATCH 1/2] \"-\" and \"@{-1}\" on various programs","fromName":"Kenny Lee Sin Cheong","fromEmail":"kenny.lee28@gmail.com","sentAt":"2015-03-16T15:11:42Z","receivedAt":"2015-03-16T15:11:42Z","isPatch":true,"sender":{"key":"kenny.lee28@gmail.com","avatar":null},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nJFF stands for just for fun.\n\nThis is not meant to give out a model answer and is known to be\nincomplete, but I was wondering if it would be a better direction to\nallow \"-\" as a stand-in for \"@{-1}\" everywhere we allow a branch\nname, losing workarounds at the surface level we have for checkout,\nmerge and revert.\n\nThe first three paths are to remove the surface workarounds that\nbecome unnecessary.  The one in sha1_name.c is the central change.\n\nThe change in revision.c is to allow a single \"-\" to be recognized\nas a potential revision name (without this change, what begins with\n\"-\" is either an option or an unknown option).\n\nSo you could do things like \"git reset - $path\" but also things like\n\"git log -\" after switching out of a branch.\n\nWhat does not work are what needs further tweaking in revision.c\nparser.  \"git checkout master && git checkout next && git log -..\"\nshould show what next has on top of master but I didn't touch the\nrange notation so it does not work, for example.\n\n builtin/checkout.c |  3 ---\n builtin/merge.c    |  3 +--\n builtin/revert.c   |  2 --\n revision.c         |  2 +-\n sha1_name.c        | 57 +++++++++++++++++++++++++++++++++---------------------\n 5 files changed, 37 insertions(+), 30 deletions(-)\n\nSigned-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>\n---\n builtin/checkout.c |  3 ---\n builtin/merge.c    |  3 +--\n builtin/revert.c   |  2 --\n revision.c         |  2 +-\n sha1_name.c        | 57 +++++++++++++++++++++++++++++++++---------------------\n 5 files changed, 37 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3e141fc..f86bad7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -951,9 +951,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n \telse if (dash_dash_pos >= 2)\n \t\tdie(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n \n-\tif (!strcmp(arg, \"-\"))\n-\t\targ = \"@{-1}\";\n-\n \tif (get_sha1_mb(arg, rev)) {\n \t\t/*\n \t\t * Either case (3) or (4), with <something> not being\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 3b0f8f9..03b260f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1164,8 +1164,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\targc = setup_with_upstream(&argv);\n \t\t\telse\n \t\t\t\tdie(_(\"No commit specified and merge.defaultToUpstream not set.\"));\n-\t\t} else if (argc == 1 && !strcmp(argv[0], \"-\"))\n-\t\t\targv[0] = \"@{-1}\";\n+\t\t}\n \t}\n \tif (!argc)\n \t\tusage_with_options(builtin_merge_usage,\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 56a2c36..dc98b4e 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -170,8 +170,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\topts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;\n \t\tif (argc < 2)\n \t\t\tusage_with_options(usage_str, options);\n-\t\tif (!strcmp(argv[1], \"-\"))\n-\t\t\targv[1] = \"@{-1}\";\n \t\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \t\ts_r_opt.assume_dashdash = 1;\n \t\targc = setup_revisions(argc, argv, opts->revs, &s_r_opt);\ndiff --git a/revision.c b/revision.c\nindex 66520c6..7778bbd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2198,7 +2198,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[0] == '-' && arg[1]) {\n \t\t\tint opts;\n \n \t\t\topts = handle_revision_pseudo_opt(submodule,\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 95f9f8f..7a621ba 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -483,6 +483,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n+\t} else if (len == 1 && str[0] == '-') {\n+\t\tnth_prior = 1;\n \t}\n \n \t/* Accept only unambiguous ref paths. */\n@@ -491,13 +493,16 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,\n \n \tif (nth_prior) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint detached;\n+\t\tint status;\n \n \t\tif (interpret_nth_prior_checkout(str, len, &buf) > 0) {\n-\t\t\tdetached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));\n+\t\t\tif (get_sha1(buf.buf, sha1))\n+\t\t\t\t/* bad---the previous branch no longer exists? */\n+\t\t\t\tstatus = -1;\n+\t\t\telse\n+\t\t\t\tstatus = 0; /* detached */\n \t\t\tstrbuf_release(&buf);\n-\t\t\tif (detached)\n-\t\t\t\treturn 0;\n+\t\t\treturn status;\n \t\t}\n \t}\n \n@@ -931,35 +936,43 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,\n \t\t\t\t\tstruct strbuf *buf)\n {\n \tlong nth;\n-\tint retval;\n+\tint consumed;\n \tstruct grab_nth_branch_switch_cbdata cb;\n-\tconst char *brace;\n-\tchar *num_end;\n \n-\tif (namelen < 4)\n-\t\treturn -1;\n-\tif (name[0] != '@' || name[1] != '{' || name[2] != '-')\n-\t\treturn -1;\n-\tbrace = memchr(name, '}', namelen);\n-\tif (!brace)\n-\t\treturn -1;\n-\tnth = strtol(name + 3, &num_end, 10);\n-\tif (num_end != brace)\n-\t\treturn -1;\n-\tif (nth <= 0)\n-\t\treturn -1;\n+\tif (namelen == 1 && name[0] == '-') {\n+\t\tnth = 1;\n+\t\tconsumed = 1;\n+\t} else {\n+\t\tconst char *brace;\n+\t\tchar *num_end;\n+\n+\t\tif (namelen < 4)\n+\t\t\treturn -1;\n+\t\tif (name[0] != '@' || name[1] != '{' || name[2] != '-')\n+\t\t\treturn -1;\n+\t\tbrace = memchr(name, '}', namelen);\n+\t\tif (!brace)\n+\t\t\treturn -1;\n+\t\tnth = strtol(name + 3, &num_end, 10);\n+\t\tif (num_end != brace)\n+\t\t\treturn -1;\n+\t\tif (nth <= 0)\n+\t\t\treturn -1;\n+\t\tconsumed = brace - name + 1;\n+\t}\n+\n \tcb.remaining = nth;\n \tstrbuf_init(&cb.buf, 20);\n \n-\tretval = 0;\n \tif (0 < for_each_reflog_ent_reverse(\"HEAD\", grab_nth_branch_switch, &cb)) {\n \t\tstrbuf_reset(buf);\n \t\tstrbuf_addbuf(buf, &cb.buf);\n-\t\tretval = brace - name + 1;\n+\t} else {\n+\t\tconsumed = 0;\n \t}\n \n \tstrbuf_release(&cb.buf);\n-\treturn retval;\n+\treturn consumed;\n }\n \n int get_sha1_mb(const char *name, unsigned char *sha1)\n-- \n2.3.2.225.gebdc58a\n"},{"id":"257788","messageId":"1426518703-15785-3-git-send-email-kenny.lee28@gmail.com","threadId":"38825","inReplyTo":"1426518703-15785-1-git-send-email-kenny.lee28@gmail.com","subject":"[PATCH 2/2] Add revision range support on \"-\" and \"@{-1}\"","fromName":"Kenny Lee Sin Cheong","fromEmail":"kenny.lee28@gmail.com","sentAt":"2015-03-16T15:11:43Z","receivedAt":"2015-03-16T15:11:43Z","isPatch":true,"sender":{"key":"kenny.lee28@gmail.com","avatar":null},"body":"Currently it is not be possible to do something like \"git checkout\nmaster && git checkout next && git log -..\" to see what master has on\ntop of master.\n\nAllows use of the revision range such as <rev>..- or -..<rev> to see\nwhat HEAD has on top of <rev> or vice versa, respectively.\n\nAlso allows use of symmetric differences such as <rev>...- and -...<rev>\n\nThis is written on top of Junio's \"Just For Fun\" patch ($Gmane/265260).\n\nSigned-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>\n---\n revision.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 7778bbd..a79b443 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1490,6 +1490,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\tint symmetric = *next == '.';\n \t\tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n \t\tstatic const char head_by_default[] = \"HEAD\";\n+\t\tstatic const char prev_rev[] = \"@{-1}\";\n \t\tunsigned int a_flags;\n \n \t\t*dotdot = 0;\n@@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\tnext = head_by_default;\n \t\tif (dotdot == arg)\n \t\t\tthis = head_by_default;\n+\t\t/*  Allows -..<rev> and <rev>..- */\n+\t\tif (!strcmp(this, \"-\")) {\n+\t\t\tthis = prev_rev;\n+\t\t}\n+\t\tif (!strcmp(next, \"-\")) {\n+\t\t\tnext = prev_rev;\n+\t\t}\n \t\tif (this == head_by_default && next == head_by_default &&\n \t\t    !symmetric) {\n \t\t\t/*\n@@ -2198,7 +2206,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[0] == '-' && arg[1]) {\n+\t\tif (arg[0] == '-' && !strstr(arg, \"..\")) {\n \t\t\tint opts;\n \n \t\t\topts = handle_revision_pseudo_opt(submodule,\n@@ -2220,6 +2228,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\tcontinue;\n \t\t\t}\n \n+\n \t\t\topts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);\n \t\t\tif (opts > 0) {\n \t\t\t\ti += opts - 1;\n@@ -2229,7 +2238,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\texit(128);\n \t\t\tcontinue;\n \t\t}\n-\n+\t\tif (strstr(arg, \"..\")) {\n+\t\t\thandle_revision_arg(arg, revs, flags, revarg_opt);\n+\t\t\tcontinue;\n+\t\t}\n \n \t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n \t\t\tint j;\n-- \n2.3.2.225.gebdc58a\n"},{"id":"257808","messageId":"xmqqlhiwredj.fsf@gitster.dls.corp.google.com","threadId":"38825","inReplyTo":"1426518703-15785-3-git-send-email-kenny.lee28@gmail.com","subject":"Re: [PATCH 2/2] Add revision range support on \"-\" and \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-16T18:22:48Z","receivedAt":"2015-03-16T18:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kenny Lee Sin Cheong <kenny.lee28@gmail.com> writes:\n\n> diff --git a/revision.c b/revision.c\n> index 7778bbd..a79b443 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1490,6 +1490,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n>  \t\tint symmetric = *next == '.';\n>  \t\tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n>  \t\tstatic const char head_by_default[] = \"HEAD\";\n> +\t\tstatic const char prev_rev[] = \"@{-1}\";\n>  \t\tunsigned int a_flags;\n>  \n>  \t\t*dotdot = 0;\n> @@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n>  \t\t\tnext = head_by_default;\n>  \t\tif (dotdot == arg)\n>  \t\t\tthis = head_by_default;\n> +\t\t/*  Allows -..<rev> and <rev>..- */\n> +\t\tif (!strcmp(this, \"-\")) {\n> +\t\t\tthis = prev_rev;\n> +\t\t}\n> +\t\tif (!strcmp(next, \"-\")) {\n> +\t\t\tnext = prev_rev;\n> +\t\t}\n\nThe above two hunks are disappointing.  \"this\" and \"next\" are passed\nto get_sha1_committish() and the point of the [1/2] patch was to\nallow \"-\" to be just as usable as \"@{-1}\" anywhere a branch name can\nbe used.\n\n>  \t\tif (this == head_by_default && next == head_by_default &&\n>  \t\t    !symmetric) {\n>  \t\t\t/*\n> @@ -2198,7 +2206,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[0] == '-' && arg[1]) {\n> +\t\tif (arg[0] == '-' && !strstr(arg, \"..\")) {\n\nIsn't this way too loose?  \"--some-opt=I.wish...\" would have \"..\"\nin it, and we would want to leave room to add new options that may\ntake arbitrary string as an argument.\n\nI would have expected it would be more like\n\n\t\tif (arg[0] == '-' && arg[1] && !starts_with(arg + 1, \"..\")) {\n\nThat is, \"anything that begins with '-', if it is to be taken as an\noption, must not begin with '-..'\", which I think should be strict\nenough.\n\n> @@ -2220,6 +2228,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n>  \n> +\n>  \t\t\topts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);\n>  \t\t\tif (opts > 0) {\n>  \t\t\t\ti += opts - 1;\n\nNoise.\n\n> @@ -2229,7 +2238,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\t\t\texit(128);\n>  \t\t\tcontinue;\n>  \t\t}\n> -\n> +\t\tif (strstr(arg, \"..\")) {\n> +\t\t\thandle_revision_arg(arg, revs, flags, revarg_opt);\n> +\t\t\tcontinue;\n> +\t\t}\n\nWhat is this for?  We will call handle_revision_arg() whether arg\nhas \"..\" or not immediately after this one.\n\n>  \t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n>  \t\t\tint j;\n"},{"id":"257835","messageId":"xmqq8uewp183.fsf@gitster.dls.corp.google.com","threadId":"38825","inReplyTo":"xmqqlhiwredj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Add revision range support on \"-\" and \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T06:49:48Z","receivedAt":"2015-03-17T06:49:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> @@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n>>  \t\t\tnext = head_by_default;\n>>  \t\tif (dotdot == arg)\n>>  \t\t\tthis = head_by_default;\n>> +\t\t/*  Allows -..<rev> and <rev>..- */\n>> +\t\tif (!strcmp(this, \"-\")) {\n>> +\t\t\tthis = prev_rev;\n>> +\t\t}\n>> +\t\tif (!strcmp(next, \"-\")) {\n>> +\t\t\tnext = prev_rev;\n>> +\t\t}\n>\n> The above two hunks are disappointing.  \"this\" and \"next\" are passed\n> to get_sha1_committish() and the point of the [1/2] patch was to\n> allow \"-\" to be just as usable as \"@{-1}\" anywhere a branch name can\n> be used.\n>\n>>  \t\tif (this == head_by_default && next == head_by_default &&\n>>  \t\t    !symmetric) {\n>>  \t\t\t/*\n>> @@ -2198,7 +2206,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[0] == '-' && arg[1]) {\n>> +\t\tif (arg[0] == '-' && !strstr(arg, \"..\")) {\n>\n> Isn't this way too loose?  \"--some-opt=I.wish...\" would have \"..\"\n> in it, and we would want to leave room to add new options that may\n> take arbitrary string as an argument.\n>\n> I would have expected it would be more like\n>\n> \t\tif (arg[0] == '-' && arg[1] && !starts_with(arg + 1, \"..\")) {\n>\n> That is, \"anything that begins with '-', if it is to be taken as an\n> option, must not begin with '-..'\", which I think should be strict\n> enough.\n\nI have an updated version to handle the simplest forms of range\nnotations on 'pu' as d40f108d (\"-\" and \"@{-1}\" on various programs,\n2015-03-16).  I do not think either your !strstr(arg, \"..\") or my\n!starts_with(arg + 1, \"..\")  is correct, if we really wanted to make\n\"-\" a true stand-in for @{-1}, as we would need to stop ourselves\nfall into this \"This begins with a dash, so it has to be a dashed\noption\" block when things like these are given:\n\n    \"-^\"\n    \"-~10\"\n    \"-^{/^### match next}\"\n\nI have a suspicion that it might be a matter of swapping the if\nclauses, that is, instead of the current way\n\n\tif (starts with '-') {\n        \tdo the option thing;\n                continue;\n\t}\n\tif (try to see if it is a revision or revision range) {\n        \t/* if failed, args must be pathspecs from here on */\n\t\tcheck the '--' disambiguation;\n                add pathspec to prune-data;\n\t} else {\n\t\tgot_rev_arg = 1;\n\t}\n\nwhich tries \"the option thing\" first, do something like this:\n\n\tif (try to see if it is a revision or regvision range) {\n        \t/* if failed ... */\n\t\tif (starts with '-') {\n                \tdo the option thing;\n                        continue;\n\t\t}\n\t\t/* args must be pathspecs from here on */\n                check the  '--' disambiguation;\n                add pathspec to prune-data;\n\t} else {\n\t\tgot_rev_arg = 1;\n\t}\n\nbut I didn't trace the logic myself to see if that would work.\n"},{"id":"257894","messageId":"87egons4du.fsf@gmail.com","threadId":"38825","inReplyTo":"xmqq8uewp183.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Add revision range support on \"-\" and \"@{-1}\"","fromName":"Kenny Lee Sin Cheong","fromEmail":"kenny.lee28@gmail.com","sentAt":"2015-03-17T21:25:33Z","receivedAt":"2015-03-17T21:25:33Z","isPatch":true,"sender":{"key":"kenny.lee28@gmail.com","avatar":null},"body":"On Tue, Mar 17 2015 at 02:49:48 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> \tif (try to see if it is a revision or regvision range) {\n>         \t/* if failed ... */\n> \t\tif (starts with '-') {\n>                 \tdo the option thing;\n>                         continue;\n> \t\t}\n> \t\t/* args must be pathspecs from here on */\n>                 check the  '--' disambiguation;\n>                 add pathspec to prune-data;\n> \t} else {\n> \t\tgot_rev_arg = 1;\n> \t}\n>\n> but I didn't trace the logic myself to see if that would work.\n\nYou're right. I was actually going to try and check all possible\nsuffixes of \"-\" but your solution saves us from doing that, and it\ndidn't break any tests.\n\nOn a similar note, would it be relevant to add similar changes to\nrev-parse? While trying to write some test, I noticed that rev-parse\ndoesn't support \"-\". If I'm not mistaking it assumes everything that starts with \"-\"\nmust be an option. But since it is a plumbing tool I don't know if it\nwould be worth it or even an improvement.\n"},{"id":"257896","messageId":"xmqqpp87mfqx.fsf@gitster.dls.corp.google.com","threadId":"38825","inReplyTo":"87egons4du.fsf@gmail.com","subject":"Re: [PATCH 2/2] Add revision range support on \"-\" and \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T22:16:38Z","receivedAt":"2015-03-17T22:16:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kenny Lee Sin Cheong <kenny.lee28@gmail.com> writes:\n\n> On Tue, Mar 17 2015 at 02:49:48 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>> \tif (try to see if it is a revision or regvision range) {\n>>         \t/* if failed ... */\n>> \t\tif (starts with '-') {\n>>                 \tdo the option thing;\n>>                         continue;\n>> \t\t}\n>> \t\t/* args must be pathspecs from here on */\n>>                 check the  '--' disambiguation;\n>>                 add pathspec to prune-data;\n>> \t} else {\n>> \t\tgot_rev_arg = 1;\n>> \t}\n>>\n>> but I didn't trace the logic myself to see if that would work.\n>\n> You're right. I was actually going to try and check all possible\n> suffixes of \"-\" but your solution saves us from doing that, and it\n> didn't break any tests.\n\n\"It didn't break any tests\" does not tell us much, though.\n\nI also notice that handle_revision_arg() would die() by calling it\ndirectly or indirectly via verify_non_filename(), etc., but the\ncaller actually is expecting it to silently return non-zero when it\nfinds an argument that cannot be interpreted as a revision or as a\nrevision range.  \n\nIf we feed the function a string that has \"..\" in it, with\ncant_be_filename unset, and if that string _can_ be parsed as a\nvalid range (e.g. \"master..next\"), we would check if a file whose\nname is that string and die, e.g.\n\n    $ >master..next ; git log master..next\n    fatal: ambigous argument 'master..next': both revision and filename\n\nIf we swap the order to do the \"revision\" first before \"option\",\nhowever, we would end up getting the same for a name that begins\nwith \"-\" and has \"..\" in it.  I see no guarantee that future\npossible option name cannot be misinterpreted as a range to trigger\nthis check.\n\nBut \"git cmd -$option\" for any value of $option does not have to be\ndisambiguated when there is a file whose name is \"-$option\".  The\nexisting die()'s in the handle_revision_arg() function _will_ break\nthat promise.  Currently, because we check the options first,\nhandle_revision_arg() does not cause us any problem, but swapping\nthe order will have fallouts.\n\nIf we want to really do the swapping (and I think that is the only\nsensible way if we wanted to allow \"-\" and any extended SHA-1 that\nbegins with \"-\" as \"the previous branch\"), I think the \"OK, it looks\nlike a revision (or revision range); as we didn't see dashdash, it\nmust not be a filename\" check has to be moved to the caller, perhaps\nlike this:\n\n\tif (try to see if it is a revision or a revision range) {\n        \t/* failed */\n                ...\n\t} else {\n        \t/* it can be read as a revision or a revision range */\n                if (!seen_dashdash)\n\t\t\tverify_non_filename(arg);\n\t\tgot_rev_arg = 1;\n\t}\n\nThe \"missing\" cases should also silently return failure and have the\ncaller deal with that.\n\n> On a similar note, would it be relevant to add similar changes to\n> rev-parse?\n\nIf the goal is \"to allow '-' everywhere '@{-1}' is allowed, and used\nas such\", then yes, of course, such an update is needed.\n\nBut I am not sure if that is a worthwhile goal to aim for in the\nfirst place, though.  You would need to accept -@{two.days.ago} as a\n\"short-hand\" for @{-1}@{two.days.ago}, etc., which does not look\nvery readable way in the first place.\n"},{"id":"258390","messageId":"87r3sfz25t.fsf@gmail.com","threadId":"38825","inReplyTo":"xmqqpp87mfqx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Add revision range support on \"-\" and \"@{-1}\"","fromName":"Kenny Lee Sin Cheong","fromEmail":"kenny.lee28@gmail.com","sentAt":"2015-03-24T00:09:50Z","receivedAt":"2015-03-24T00:09:50Z","isPatch":true,"sender":{"key":"kenny.lee28@gmail.com","avatar":null},"body":"On Tue, Mar 17 2015 at 06:16:38 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> I also notice that handle_revision_arg() would die() by calling it\n> directly or indirectly via verify_non_filename(), etc., but the\n> caller actually is expecting it to silently return non-zero when it\n> finds an argument that cannot be interpreted as a revision or as a\n> revision range.  \n>\n> If we feed the function a string that has \"..\" in it, with\n> cant_be_filename unset, and if that string _can_ be parsed as a\n> valid range (e.g. \"master..next\"), we would check if a file whose\n> name is that string and die, e.g.\n>\n>     $ >master..next ; git log master..next\n>     fatal: ambigous argument 'master..next': both revision and filename\n>\n> If we swap the order to do the \"revision\" first before \"option\",\n> however, we would end up getting the same for a name that begins\n> with \"-\" and has \"..\" in it.  I see no guarantee that future\n> possible option name cannot be misinterpreted as a range to trigger\n> this check.\n>\nIf I'm understanding correctly, the problem of checking revisions before\narg is that an option fed to handle_revision_arg() might die() before getting\nchecked as an option in cases where a file with the same name exists?\n\nBut doesn't verify_non_filename() already return silently if arg begins\nwith \"-\"? It die() only after making that check.\n\nIf an option with \"..\" in it such as -$opt..ion is really given to\nhandle_revision_arg() then verify_non_filename should not be a problem.\n\n> But \"git cmd -$option\" for any value of $option does not have to be\n> disambiguated when there is a file whose name is \"-$option\".  The\n> existing die()'s in the handle_revision_arg() function _will_ break\n> that promise.  Currently, because we check the options first,\n> handle_revision_arg() does not cause us any problem, but swapping\n> the order will have fallouts.\n>\n\nThe only other way handle_revision_arg() can die() is if given a \"..\"\nrange, either revisions return null when passed their sha1 to\nparse_object().\n\nSo something like you proposed earlier:\n\n      if(try to see if it is a revision or a revision range) {\n              /* if failed ... */\n              if (starts with '-') {\n                      do the option thing;\n                      continue;\n              }\n              /*\n               * args must be pathspecs from here on.\n               * We already checked that rev arg cannot be\n               * interpreted as a filename at this point\n               */\n              if(dashdash)\n                      verify_filename()\n                     \n      } else {\n              got_rev_arg = 1;\n      }\n\nshould work. I'm still getting familiar to how it works so I might be missing\nsomething but shouldn't this be fine? At least concerning the possible fallouts\nthat you've raised.\n\n> If we want to really do the swapping (and I think that is the only\n> sensible way if we wanted to allow \"-\" and any extended SHA-1 that\n> begins with \"-\" as \"the previous branch\"), I think the \"OK, it looks\n> like a revision (or revision range); as we didn't see dashdash, it\n> must not be a filename\" check has to be moved to the caller, perhaps\n> like this:\n>\n> \tif (try to see if it is a revision or a revision range) {\n>         \t/* failed */\n>                 ...\n> \t} else {\n>         \t/* it can be read as a revision or a revision range */\n>                 if (!seen_dashdash)\n> \t\t\tverify_non_filename(arg);\n> \t\tgot_rev_arg = 1;\n> \t}\n>\nIf what I'm saying makes sense, then verify_non_filename(arg) would be\nalready working as intended in handle_revision_arg(), so moving it to\nthe caller wouldn't be necessary.\n\n> The \"missing\" cases should also silently return failure and have the\n> caller deal with that.\n"},{"id":"258528","messageId":"xmqqa8z01zs5.fsf@gitster.dls.corp.google.com","threadId":"38825","inReplyTo":"87r3sfz25t.fsf@gmail.com","subject":"Re: [PATCH 2/2] Add revision range support on \"-\" and \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-25T22:24:26Z","receivedAt":"2015-03-25T22:24:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kenny Lee Sin Cheong <kenny.lee28@gmail.com> writes:\n\n> If I'm understanding correctly, the problem of checking revisions before\n> arg is that an option fed to handle_revision_arg() might die() before getting\n> checked as an option in cases where a file with the same name exists?\n>\n> But doesn't verify_non_filename() already return silently if arg begins\n> with \"-\"? It die() only after making that check.\n>\n> If an option with \"..\" in it such as -$opt..ion is really given to\n> handle_revision_arg() then verify_non_filename should not be a problem.\n\nYes, but should we be relying on that behaviour?  The special casing\nto assume that no sane person would name a file starting with a dash\nis what I find somewhat disturbing.\n"}]}