{"thread":{"id":"45104","subject":"[PATCH 0/2 v3] WIP: allow \"-\" as a shorthand for \"previous branch\"","startedAt":"2017-02-10T18:56:53Z","lastAt":"2017-02-14T04:23:37Z","messageCount":18,"participants":["Siddharth Kannan","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"311285","messageId":"1486752926-12020-1-git-send-email-kannan.siddharth12@gmail.com","threadId":"45104","inReplyTo":null,"subject":"[PATCH 0/2 v3] WIP: allow \"-\" as a shorthand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-10T18:55:24Z","receivedAt":"2017-02-10T18:56:53Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Thanks a lot, Matthieu, for your comments on an earlier version of this \npatch! [1]\n\nAfter the discussion there, I have considered the changes that have been made\nand I broke them into two separate commits.\n\nI have included the list of commands that are touched by this patch series in\nthe second commit message. (idea from the v1 discussion [2]) Reproduced here:\n\n * builtin/blame.c\n * builtin/diff.c\n * builtin/diff-files.c\n * builtin/diff-index.c\n * builtin/diff-tree.c\n * builtin/log.c\n * builtin/rev-list.c\n * builtin/shortlog.c\n * builtin/fast-export.c\n * builtin/fmt-merge-msg.c\n builtin/add.c\n builtin/checkout.c\n builtin/commit.c\n builtin/merge.c\n builtin/pack-objects.c\n builtin/revert.c\n\n * marked commands are information-only.\n\nI have added the WIP tag because I am still unsure if the tests that I have\nadded (for git-log) are sufficient for this patch or more comprehensive tests\nneed to be added. So, please help me with some feedback on that.\n\nI have removed the \"log:\" tag from the subject line because this patch now\naffects commands other than log.\n\nI have run the test suite locally and on Travis CI! [3]\n\n[1]: https://public-inbox.org/git/vpqh944eof7.fsf@anie.imag.fr/#t\n[2]: https://public-inbox.org/git/CAN-3QhoZN_wYvqbVdU_c1h4vUOaT5FOBFL7k+FemNpqkxjWDDA@mail.gmail.com/\n[3]: https://travis-ci.org/icyflame/git/builds/200431159\n\nSiddharth Kannan (2):\n revision.c: args starting with \"-\" might be a revision\n sha1_name: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"\n\n revision.c | 12 ++++++--\n sha1_name.c | 5 ++++\n t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 88 insertions(+), 2 deletions(-)\n create mode 100755 t/t4214-log-shorthand.sh\n\n-- \n2.1.4\n\n"},{"id":"311286","messageId":"1486752926-12020-2-git-send-email-kannan.siddharth12@gmail.com","threadId":"45104","inReplyTo":"1486752926-12020-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-10T18:55:25Z","receivedAt":"2017-02-10T18:57:00Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"setup_revisions used to consider any argument starting with \"-\" to be either a\nvalid option or nothing at all. This patch will teach it to check if the\nargument is a revision before declaring that it is nothing at all.\n\nBefore this patch, handle_revision_arg was not called for arguments starting\nwith \"-\" and once for arguments that didn't start with \"-\". Now, it will be\ncalled once per argument.\n\nThis patch prepares the addition of \"-\" as a shorthand for \"previous branch\".\n\nSigned-off-by: Siddharth Kannan \n---\n revision.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec..4131ad5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2205,6 +2205,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\tint handle_rev_arg_called = 0, args;\n \t\tif (*arg == '-') {\n \t\t\tint opts;\n \n@@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \t\t\tif (opts < 0)\n \t\t\t\texit(128);\n-\t\t\tcontinue;\n+\n+\t\t\targs = handle_revision_arg(arg, revs, flags, revarg_opt);\n+\t\t\thandle_rev_arg_called = 1;\n+\t\t\tif (args)\n+\t\t\t\tcontinue;\n+\t\t\telse\n+\t\t\t\t--left;\n \t\t}\n \n \n-\t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n+\t\tif ((handle_rev_arg_called && args) ||\n+\t\t\t\thandle_revision_arg(arg, revs, flags, revarg_opt)) {\n \t\t\tint j;\n \t\t\tif (seen_dashdash || *arg == '^')\n \t\t\t\tdie(\"bad revision '%s'\", arg);\n-- \n2.1.4\n\n"},{"id":"311287","messageId":"1486752926-12020-3-git-send-email-kannan.siddharth12@gmail.com","threadId":"45104","inReplyTo":"1486752926-12020-2-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 2/2 v3] sha1_name: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-10T18:55:26Z","receivedAt":"2017-02-10T18:57:17Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"This patch introduces \"-\" as a method to refer to a revision, and adds tests to\ntest that git-log works with this shorthand.\n\nThis change will touch the following commands (through\nrevision.c:setup_revisions):\n\n* builtin/blame.c\n* builtin/diff.c\n* builtin/diff-files.c\n* builtin/diff-index.c\n* builtin/diff-tree.c\n* builtin/log.c\n* builtin/rev-list.c\n* builtin/shortlog.c\n* builtin/fast-export.c\n* builtin/fmt-merge-msg.c\nbuiltin/add.c\nbuiltin/checkout.c\nbuiltin/commit.c\nbuiltin/merge.c\nbuiltin/pack-objects.c\nbuiltin/revert.c\n\n* marked commands are information-only.\n\nAs most commands in this list are not of the rm-variety, (i.e a command that\nwould delete something), this change does not make it easier for people to\ndelete. (eg: \"git branch -d -\" is *not* enabled by this patch)\n\nSigned-off-by: Siddharth Kannan \n---\n sha1_name.c | 5 ++++\n t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+)\n create mode 100755 t/t4214-log-shorthand.sh\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 73a915f..d774e46 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l\n \tif (!ret)\n \t\treturn 0;\n \n+\tif (!strcmp(name, \"-\")) {\n+\t\tname = \"@{-1}\";\n+\t\tlen = 5;\n+\t}\n+\n \tret = get_sha1_basic(name, len, sha1, lookup_flags);\n \tif (!ret)\n \t\treturn 0;\ndiff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh\nnew file mode 100755\nindex 0000000..dec966c\n--- /dev/null\n+++ b/t/t4214-log-shorthand.sh\n@@ -0,0 +1,73 @@\n+#!/bin/sh\n+\n+test_description='log can show previous branch using shorthand - for @{-1}'\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+\techo \"hello second time\" >>world &&\n+\tgit add world &&\n+\tgit commit -m second &&\n+\techo \"hello other file\" >>planet &&\n+\tgit add planet &&\n+\tgit commit -m third &&\n+\techo \"hello yet another file\" >>city &&\n+\tgit add city &&\n+\tgit commit -m fourth\n+'\n+\n+test_expect_success '\"log -\" should not work initially' '\n+\ttest_must_fail git log -\n+'\n+\n+test_expect_success '\"log -\" should work' '\n+\tgit checkout -b testing-1 master^ &&\n+\tgit checkout -b testing-2 master~2 &&\n+\tgit checkout master &&\n+\n+\tgit log testing-2 >expect &&\n+\tgit log - >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'symmetric revision range should work when one end is left empty' '\n+\tgit checkout testing-2 &&\n+\tgit checkout master &&\n+\tgit log ...@{-1} > expect.first_empty &&\n+\tgit log @{-1}... > expect.last_empty &&\n+\tgit log ...- > actual.first_empty &&\n+\tgit log -... > actual.last_empty &&\n+\ttest_cmp expect.first_empty actual.first_empty &&\n+\ttest_cmp expect.last_empty actual.last_empty\n+'\n+\n+test_expect_success 'asymmetric revision range should work when one end is left empty' '\n+\tgit checkout testing-2 &&\n+\tgit checkout master &&\n+\tgit log ..@{-1} > expect.first_empty &&\n+\tgit log @{-1}.. > expect.last_empty &&\n+\tgit log ..- > actual.first_empty &&\n+\tgit log -.. > actual.last_empty &&\n+\ttest_cmp expect.first_empty actual.first_empty &&\n+\ttest_cmp expect.last_empty actual.last_empty\n+'\n+\n+test_expect_success 'symmetric revision range should work when both ends are given' '\n+\tgit checkout testing-2 &&\n+\tgit checkout master &&\n+\tgit log -...testing-1 >expect &&\n+\tgit log testing-2...testing-1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'asymmetric revision range should work when both ends are given' '\n+\tgit checkout testing-2 &&\n+\tgit checkout master &&\n+\tgit log -..testing-1 >expect &&\n+\tgit log testing-2..testing-1 >actual &&\n+\ttest_cmp expect actual\n+'\n+test_done\n-- \n2.1.4\n\n"},{"id":"311329","messageId":"xmqqh941ippo.fsf@gitster.mtv.corp.google.com","threadId":"45104","inReplyTo":"1486752926-12020-2-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T23:35:47Z","receivedAt":"2017-02-10T23:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan writes:\n\n> @@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> \t\t\t}\n> \t\t\tif (opts < 0)\n> \t\t\t\texit(128);\n> -\t\t\tcontinue;\n> +\n> +\t\t\targs = handle_revision_arg(arg, revs, flags, revarg_opt);\n> +\t\t\thandle_rev_arg_called = 1;\n> +\t\t\tif (args)\n> +\t\t\t\tcontinue;\n> +\t\t\telse\n> +\t\t\t\t--left;\n> \t\t}\n> \n> \n> -\t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n> +\t\tif ((handle_rev_arg_called && args) ||\n> +\t\t\t\thandle_revision_arg(arg, revs, flags, revarg_opt)) {\n\nNaively I would have expected that removing the \"continue\" at the\nend and letting the control go to the existing\n\n\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n\nwould be all that is needed. The latter half of the patch is an\nartifact of having ane xtra \"handle_revision_arg() calls inside the\n\"if it begins with dash\" block to avoid calling it twice.\n\nSo the difference is just \"--left\" (by the way, our codebase seem to\nprefer \"left--\" when there is no difference between pre- or post-\ndecrement/increment) that adjusts the slot in argv[] where the next\nunknown argument is stuffed to.\n\nThe adjustment is needed as the call to handle_revision_opt() that\nis before the pre-context of this hunk stuffed the unknown thing\nthat begins with \"-\" into argv[left++]; if that thing turns out to\nbe a valid rev, then you would need to take it back, because after\nall, that is not an unknown command line argument.\n\nI am wondering if writing it like the following is easier to\nunderstand. I had a hard time figuring out what you are trying to\ndo, partly because \"args\" is quite a misnomer---implying \"how many\narguments did we see\" that is similar to opts that does mean \"how\nmany options did handle_revision_opts() see?\" The variable means\nmeans \"yes we saw a valid rev\" when it is zero. The rewrite\nbelow may avoid such a confusion. I dunno.\n\n revision.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec378..e238430948 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2204,6 +2204,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\trevarg_opt |= REVARG_CANNOT_BE_FILENAME;\n \tread_from_stdin = 0;\n \tfor (left = i = 1; i < argc; i++) {\n+\t\tint maybe_rev = 0;\n \t\tconst char *arg = argv[i];\n \t\tif (*arg == '-') {\n \t\t\tint opts;\n@@ -2234,11 +2235,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \t\t\tif (opts < 0)\n \t\t\t\texit(128);\n-\t\t\tcontinue;\n+\t\t\tmaybe_rev = 1;\n+\t\t\tleft--; /* tentatively cancel \"unknown opt\" */\n \t\t}\n \n-\n-\t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n+\t\tif (!handle_revision_arg(arg, revs, flags, revarg_opt)) {\n+\t\t\tgot_rev_arg = 1;\n+\t\t} else if (maybe_rev) {\n+\t\t\tleft++; /* it turns out that it was \"unknown opt\" */\n+\t\t\tcontinue;\n+\t\t} else {\n \t\t\tint j;\n \t\t\tif (seen_dashdash || *arg == '^')\n \t\t\t\tdie(\"bad revision '%s'\", arg);\n@@ -2255,8 +2261,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tappend_prune_data(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n-\t\telse\n-\t\t\tgot_rev_arg = 1;\n \t}\n \n \tif (prune_data.nr) {\n"},{"id":"311333","messageId":"20170211075254.GA16053@ubuntu-512mb-blr1-01.localdomain","threadId":"45104","inReplyTo":"xmqqh941ippo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-11T07:52:54Z","receivedAt":"2017-02-11T08:00:19Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hey Junio,\nOn Fri, Feb 10, 2017 at 03:35:47PM -0800, Junio C Hamano wrote:\n> So the difference is just \"--left\" (by the way, our codebase seem to\n> prefer \"left--\" when there is no difference between pre- or post-\n> decrement/increment) that adjusts the slot in argv[] where the next\n> unknown argument is stuffed to.\n\nUnderstood, I will use post decrement.\n\n> I am wondering if writing it like the following is easier to\n> understand. I had a hard time figuring out what you are trying to\n> do, partly because \"args\" is quite a misnomer---implying \"how many\n> arguments did we see\" that is similar to opts that does mean \"how\n> many options did handle_revision_opts() see?\" \n\nUm, okay, I see that \"args\" is very confusing. Would it help if this variable\nwas called \"arg_not_rev\"? Because the value that is returned from\nhandle_revision_arg is 1 when it is not a revision, and 0 when it is a\nrevision. The changed block of code would look like this:\n\n---\n revision.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec..4131ad5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2205,6 +2205,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\tint handle_rev_arg_called = 0, arg_not_rev;\n \t\tif (*arg == '-') {\n \t\t\tint opts;\n@@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n }\n if (opts < 0)\n exit(128);\n- continue;\n+\n+ arg_not_rev = handle_revision_arg(arg, revs, flags, revarg_opt);\n+ handle_rev_arg_called = 1;\n+ if (arg_not_rev)\n+ continue; /* arg is neither an option nor a revision */\n+ else\n+ left--; /* arg is a revision! */\n }\n \n \n- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n+ if ((handle_rev_arg_called && arg_not_rev) ||\n+ handle_revision_arg(arg, revs, flags, revarg_opt)) {\n\n> The rewrite below may avoid such a confusion. I dunno.\n\nUm, I am sorry, but I feel that decrementing left, and incrementing it again is\nalso confusing. I think that with a better name for the return value from\nhandle_revision_arg, the earlier confusion should be resolved.\n\nI base this on my previous experience following the codepath. It was easy for\nme to understand with the previous code that \"continue\" will be executed from\nwithin the first if block whenever arg begins with a \"-\" and it is determined\nthat it is not an option. \n\ngoing by that, now, \"continue\" will be executed whenever it's not an option and\n_also_ not an argument. Otherwise, the further parts of the code will execute\nas before, and there are no continue statements there. I hope this argument\nmakes sense.\n\nAlso worth noting, The two `if` lines look better now:\n\n1. If arg is not a revision, go to the next arg (because we have already\ndetermined that it is not an option)\n\n2. If handle_rev_arg was called AND the argument was not a revision, OR\nif handle_revision_arg says that arg is not a rev, execute the following block.\n\nPerhaps, someone else could please have a look at the changes in the block\nabove and the block below and give some feedback on which one is easier to\nunderstand and the reason that they feel so. Thanks a lot!\n\n> \n> revision.c | 14 +++++++++-----\n> 1 file changed, 9 insertions(+), 5 deletions(-)\n> \n> diff --git a/revision.c b/revision.c\n> index b37dbec378..e238430948 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2204,6 +2204,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> revarg_opt |= REVARG_CANNOT_BE_FILENAME;\n> read_from_stdin = 0;\n> for (left = i = 1; i < argc; i++) {\n> + int maybe_rev = 0;\n> const char *arg = argv[i];\n> if (*arg == '-') {\n> int opts;\n> @@ -2234,11 +2235,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> }\n> if (opts < 0)\n> exit(128);\n> - continue;\n> + maybe_rev = 1;\n> + left--; /* tentatively cancel \"unknown opt\" */\n> }\n> \n> -\n> - if (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n> + if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {\n> + got_rev_arg = 1;\n> + } else if (maybe_rev) {\n> + left++; /* it turns out that it was \"unknown opt\" */\n> + continue;\n> + } else {\n> int j;\n> if (seen_dashdash || *arg == '^')\n> die(\"bad revision '%s'\", arg);\n> @@ -2255,8 +2261,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> append_prune_data(&prune_data, argv + i);\n> break;\n> }\n> - else\n> - got_rev_arg = 1;\n> }\n> \n> if (prune_data.nr) {\n\nThanks Junio, for the time you spent analysing and writing the above version of\nthe patch!\n\nRegards,\n\n- Siddharth Kannan\n"},{"id":"311350","messageId":"xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com","threadId":"45104","inReplyTo":"20170211075254.GA16053@ubuntu-512mb-blr1-01.localdomain","subject":"Re: [PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-11T21:08:09Z","receivedAt":"2017-02-11T21:08:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan writes:\n\n> On Fri, Feb 10, 2017 at 03:35:47PM -0800, Junio C Hamano wrote:\n>\n>> I am wondering if writing it like the following is easier to\n>> understand. I had a hard time figuring out what you are trying to\n>> do, partly because \"args\" is quite a misnomer---implying \"how many\n>> arguments did we see\" that is similar to opts that does mean \"how\n>> many options did handle_revision_opts() see?\"\n>\n> Um, okay, I see that \"args\" is very confusing. Would it help if this variable\n> was called \"arg_not_rev\"?\n\nNot really. If we absolutely need to have one variable that is\nmeant to escape the \"if it begins with a dash\" block and to affect\nwhat comes next, I think the variable should mean \"we know we saw a\nrevision and you do not have to call it again\". IOW the code that\nneeds to do \"handle_rev_arg_called && arg_not_rev\" is just being\nsilly. At that point in the codeflow, I do not see why the code\nneeds to take two bits of information and combine them; the one that\nsets these two variables should have done the work for it.\n\nAnd that would make the if statement slightly easier to read\ncompared to the original. I am however not suggesting to do that;\nread on.\n\n> Because the value that is returned from\n> handle_revision_arg is 1 when it is not a revision, and 0 when it is a\n> revision.\n\nThe function follows the convention to return 0 for success, -1 for\nerror/unexpected, by the way.\n\n> Um, I am sorry, but I feel that decrementing left, and incrementing it again is\n> also confusing.\n\nYes, but it is no more confusing than your original \"left--\".\n\nIf we want to make the flow of logic easier to follow, we need to\nstep back and view what the codepath _wants_ to do at the higher\nlevel, which is:\n\n * If it is an option known to us, handle it and go to the next arg.\n\n * If it is an option that we do not understand, stuff it in\n argv[left++] and go to the next arg.\n\n * If it is a rev, handle it, and note that fact in got_rev_arg.\n\n * If it is not a rev and we haven't seen dashdash, verify that it\n and everything that follows it are pathnames (which is an inexact\n but a cheap way to avoid ambiguity), make all them the prune_data\n and conclude.\n\nBecause the second step currently is implemented by calling\nhandle_opt(), which not just tells if it is an option we understand\nor not, but also mucks with argv[left++], you need to undo it once\nyou start making it possible for a valid \"rev\" to begin with a dash.\nThat is what your left-- was, and that is what \"decrement and then\nincrement when it turns out it was an unknown option after all\" is.\n\nThe first step to a saner flow _could_ be to stop passing the unkv\nand unkc to handle_revision_opt() and instead make the caller\nresponsible for doing that. That would express what your patch\nwanted to do in the most natural way, i.e.\n\n * If it is an option known to us, handle it and go to the next arg.\n\n * If it is a rev, handle it, and note that fact in got_rev_arg\n (this change of order enables you to allow a rev that begins with\n a dash, which would have been misrecognised as a possible unknown\n option).\n\n * If it looks like an option (i.e. \"begins with a dash\"), then we\n already know that it is not something we understand, because the\n first step would have caught it already. Stuff it in\n argv[left++] and go to the next arg.\n\n * If it is not a rev and we haven't seen dashdash, verify that it\n and everything that follows it are pathnames (which is an inexact\n but a cheap way to avoid ambiguity), make all them the prune_data\n and conclude.\n\nSuch a change to handle_revision_opt() unfortunately affects other\ncallers of the function, so it may not be worth it, but I think\n\"decrement and then increment, because this codepath wants to check\nto see something that may ordinarily be clasified as an unknown\noption if it is a rev\" is an ugly workaround, just like your left--\nwas. But I think the resulting code flow is much closer to the\nabove ideal.\n\n"},{"id":"311351","messageId":"xmqqa89sguu4.fsf@gitster.mtv.corp.google.com","threadId":"45104","inReplyTo":"xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-11T23:40:19Z","receivedAt":"2017-02-11T23:40:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Such a change to handle_revision_opt() unfortunately affects other\n> callers of the function, so it may not be worth it, and I think\n> \"decrement and then increment, because this codepath wants to check\n> to see something that may ordinarily be clasified as an unknown\n> option if it is a rev\" is an ugly workaround, just like your left--\n> was. But I think the resulting code flow is much closer to the\n> above ideal.\n\nHaving re-analysed the codepath like so, I realize that the new\nvariable I introduced was misnamed. Its purpose is to let the\n\"if arg begins with dash, do this\" block communicate that what the\nlater part of the code is told to inspect in \"arg\" may be an option\nthat we do not recognise. So I shouldn't have called it maybe_rev;\nthe message from the former to the latter is \"this may be an unknown\noption\" and I should have called it \"maybe_unknown_opt\".\n\n"},{"id":"311356","messageId":"vpqbmu768on.fsf@anie.imag.fr","threadId":"45104","inReplyTo":"1486752926-12020-3-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2017-02-12T09:48:56Z","receivedAt":"2017-02-12T09:49:05Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Siddharth Kannan writes:\n\n> sha1_name.c | 5 ++++\n> t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 78 insertions(+)\n> create mode 100755 t/t4214-log-shorthand.sh\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 73a915f..d774e46 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l\n> \tif (!ret)\n> \t\treturn 0;\n> \n> +\tif (!strcmp(name, \"-\")) {\n> +\t\tname = \"@{-1}\";\n> +\t\tlen = 5;\n> +\t}\n\nOne drawback of this approach is that further error messages will be\ngiven from the \"@{-1}\" string that the user never typed.\n\nAfter you do that, the existing \"turn - into @{-1}\" pieces of code\nbecome useless and you should remove it (probably in a further patch).\n\nThere are at least:\n\n$ git grep -n -A1 'strcmp.*\"-\"' | grep -B 1 '@\\{1\\}'\nbuiltin/checkout.c:975: if (!strcmp(arg, \"-\"))\nbuiltin/checkout.c-976- arg = \"@{-1}\";\n--\nbuiltin/merge.c:1231: } else if (argc == 1 && !strcmp(argv[0], \"-\")) {\nbuiltin/merge.c-1232- argv[0] = \"@{-1}\";\n--\nbuiltin/revert.c:158: if (!strcmp(argv[1], \"-\"))\nbuiltin/revert.c-159- argv[1] = \"@{-1}\";\n--\nbuiltin/worktree.c:344: if (!strcmp(branch, \"-\"))\nbuiltin/worktree.c-345- branch = \"@{-1}\";\n\nIn the final version, obviously the same \"refactoring\" (specific\ncommand -> git-wide) should be done for documentation (it should be in\nthis patch to avoid letting not-up-to-date documentation even for a\nsingle commit).\n\n> diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh\n> new file mode 100755\n> index 0000000..dec966c\n> --- /dev/null\n> +++ b/t/t4214-log-shorthand.sh\n> @@ -0,0 +1,73 @@\n> +#!/bin/sh\n> +\n> +test_description='log can show previous branch using shorthand - for @{-1}'\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> +\techo \"hello second time\" >>world &&\n> +\tgit add world &&\n> +\tgit commit -m second &&\n> +\techo \"hello other file\" >>planet &&\n> +\tgit add planet &&\n> +\tgit commit -m third &&\n> +\techo \"hello yet another file\" >>city &&\n> +\tgit add city &&\n> +\tgit commit -m fourth\n> +'\n\nYou may use test_commit to save a few lines of code.\n\n> +test_expect_success '\"log -\" should work' '\n> +\tgit checkout -b testing-1 master^ &&\n> +\tgit checkout -b testing-2 master~2 &&\n> +\tgit checkout master &&\n> +\n> +\tgit log testing-2 >expect &&\n> +\tgit log - >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI'd have split this into a \"setup branches\" and a '\"log -\" should work'\ntest (to actually see where \"setup branches\" happen in the output, and\nto allow running the setup step separately if needed). Not terribly\nimportant.\n\n> +test_expect_success 'symmetric revision range should work when one end is left empty' '\n> +\tgit checkout testing-2 &&\n> +\tgit checkout master &&\n> +\tgit log ...@{-1} > expect.first_empty &&\n> +\tgit log @{-1}... > expect.last_empty &&\n> +\tgit log ...- > actual.first_empty &&\n> +\tgit log -... > actual.last_empty &&\n\nNitpick: we stick the > and the filename (as you did in most places\nalready).\n\nIt may be worth adding tests for more cases like\n\n* Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.\n\n* -..- -> to make sure you handle the presence of two - properly.\n\n* multiple separate arguments to make sure you handle them all, e.g.\n \"git log - -\", \"git log HEAD -\", \"git log - HEAD\".\n\nThe last two may be overkill, but the first one is probably important.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"311357","messageId":"20170212104220.GA20317@ubuntu-512mb-blr1-01.localdomain","threadId":"45104","inReplyTo":"vpqbmu768on.fsf@anie.imag.fr","subject":"Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-12T10:42:20Z","receivedAt":"2017-02-12T10:42:29Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hey Matthieu,\nOn Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote:\n> Siddharth Kannan writes:\n> \n> > sha1_name.c | 5 ++++\n> > t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 78 insertions(+)\n> > create mode 100755 t/t4214-log-shorthand.sh\n> >\n> > diff --git a/sha1_name.c b/sha1_name.c\n> > index 73a915f..d774e46 100644\n> > --- a/sha1_name.c\n> > +++ b/sha1_name.c\n> > @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l\n> > \tif (!ret)\n> > \t\treturn 0;\n> > \n> > +\tif (!strcmp(name, \"-\")) {\n> > +\t\tname = \"@{-1}\";\n> > +\t\tlen = 5;\n> > +\t}\n> \n> After you do that, the existing \"turn - into @{-1}\" pieces of code\n> become useless and you should remove it (probably in a further patch).\n\nYeah, this is currently also implemented in checkout, apart from the\ngrepped list that you have supplied here. I will find all the\ninstances, and ensure that they work, and remove them. (This will\nrequire some more digging into the codepath the commands, to ensure\nthat get_sha1_1 is called somewhere down the line)\n> \n> > diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh\n> > ...\n> > +test_expect_success 'setup' '\n> > +\techo hello >world &&\n> > +\tgit add world &&\n> > +\tgit commit -m initial &&\n> > +\techo \"hello second time\" >>world &&\n> > ...\n> \n> You may use test_commit to save a few lines of code.\n\nOh, yeah! I will use that. I need to work on improving the tests, as\nwell as adding the documentation.\n> \n> > +test_expect_success 'symmetric revision range should work when one end is left empty' '\n> > +\tgit checkout testing-2 &&\n> > +\tgit checkout master &&\n> > +\tgit log ...@{-1} > expect.first_empty &&\n> > +\tgit log @{-1}... > expect.last_empty &&\n> > +\tgit log ...- > actual.first_empty &&\n> > +\tgit log -... > actual.last_empty &&\n> \n> Nitpick: we stick the > and the filename (as you did in most places\n> already).\nSorry, slipped my mind!\n> \n> It may be worth adding tests for more cases like\n> \n> * Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.\n\nThese do not work right now. The first and last cases here are handled\nby peel_onion, if I remember correctly. I have to find out why exactly\nthese are not working. Thanks for mentioning this!\n\n> \n> * -..- -> to make sure you handle the presence of two - properly.\n> \n> * multiple separate arguments to make sure you handle them all, e.g.\n> \"git log - -\", \"git log HEAD -\", \"git log - HEAD\".\n\nYeah, will add these tests.\n\n> \n> The last two may be overkill, but the first one is probably important.\n> \n> -- \n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n\n--\nRegards,\n\nSiddharth Kannan.\n"},{"id":"311359","messageId":"20170212123630.GA20872@ubuntu-512mb-blr1-01.localdomain","threadId":"45104","inReplyTo":"xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-12T12:36:30Z","receivedAt":"2017-02-12T12:36:40Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote:\n> Siddharth Kannan writes:\n> \n> > Um, I am sorry, but I feel that decrementing left, and incrementing it again is\n> > also confusing.\n> \n> Yes, but it is no more confusing than your original \"left--\".\n> ...\n> \n> * If it is an option known to us, handle it and go to the next arg.\n> \n> * If it is an option that we do not understand, stuff it in\n> argv[left++] and go to the next arg.\n> \n> Because the second step currently is implemented by calling\n> handle_opt(), which not just tells if it is an option we understand\n> or not, but also mucks with argv[left++], you need to undo it once\n> you start making it possible for a valid \"rev\" to begin with a dash.\n> That is what your left-- was, and that is what \"decrement and then\n> increment when it turns out it was an unknown option after all\" is.\n\nSo, our problem here is that the function handle_revision_opt is opaquely also\nincrementing \"left\", which we need to decrement somehow.\n\nOr: we could change the flow of the code so that this incrementing\nwill happen only when we have decided that the argument is not a\nrevision.\n> \n> * If it is an option known to us, handle it and go to the next arg.\n> \n> * If it is a rev, handle it, and note that fact in got_rev_arg\n> (this change of order enables you to allow a rev that begins with\n> a dash, which would have been misrecognised as a possible unknown\n> option).\n> \n> * If it looks like an option (i.e. \"begins with a dash\"), then we\n> already know that it is not something we understand, because the\n> first step would have caught it already. Stuff it in\n> argv[left++] and go to the next arg.\n> \n> * If it is not a rev and we haven't seen dashdash, verify that it\n> and everything that follows it are pathnames (which is an inexact\n> but a cheap way to avoid ambiguity), make all them the prune_data\n> and conclude.\n\nThis \"changing the order\" gave me the idea to change the flow. I tried to\nimplement the above steps without touching the function handle_revision_opt. By\ninserting the handle_revision_arg call just before calling handle_revision_opt.\n\nThe decrementing then incrementing or \"left--\" things have now been removed.\n(But there is still one thing which doesn't look good)\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec..8c0acea 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2203,11 +2203,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tif (seen_dashdash)\n \t\trevarg_opt |= REVARG_CANNOT_BE_FILENAME;\n \tread_from_stdin = 0;\n+\n \tfor (left = i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n+\t\tint opts;\n \t\tif (*arg == '-') {\n-\t\t\tint opts;\n-\n \t\t\topts = handle_revision_pseudo_opt(submodule,\n \t\t\t\t\t\trevs, argc - i, argv + i,\n \t\t\t\t\t\t&flags);\n@@ -2226,7 +2226,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t}\n \n+\t\tif (!handle_revision_arg(arg, revs, flags, revarg_opt))\n+\t\t\tgot_rev_arg = 1;\n+\t\telse if (*arg == '-') {\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@@ -2234,11 +2238,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \t\t\tif (opts < 0)\n \t\t\t\texit(128);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\n-\t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n+\t\t} else {\n \t\t\tint j;\n \t\t\tif (seen_dashdash || *arg == '^')\n \t\t\t\tdie(\"bad revision '%s'\", arg);\n@@ -2255,8 +2255,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tappend_prune_data(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n-\t\telse\n-\t\t\tgot_rev_arg = 1;\n \t}\n \n \tif (prune_data.nr) {\n\n\nThe \"if (*arg =='-')\" line is repeated. On analysing the resulting\nrevision.c:setup_revisions function, I feel that the codepath is still as\neasily followable as it was earlier, and there is definitely no confusion\nbecause of a mysterious decrement. Also, the repeated condition doesn't make it\nany harder (it looks like a useful check because we already know that every\noption would start with a \"-\"). But that's only my opinion, and you definitely\nknow better.\n\nnow the flow is very close to the ideal flow that we prefer:\n\n1. If it is a pseudo_opt or --stdin, handle and go to the next arg\n2. If it is a revision, note that in \"got_rev_arg\", and go to the next arg\n3. If it starts with a \"-\" and is a known option, handle and go to the next arg\n4. If it is none of {revision, known-option} and we haven't seen dashdash,\n verify that it and everything that follows it are pathnames (which is an\n inexact but a cheap way to avoid ambiguity), make all them the prune_data and\n conclude.\n\n> But I think the resulting code flow is much closer to the\n> above ideal.\n\n(about Junio's version of the patch): Yes, I agree with you on this. It's like\nthe ideal, but the argv has already been populated, so the only remaining step\nis \"left++\".\n\n> \n> Such a change to handle_revision_opt() unfortunately affects other\n> callers of the function, so it may not be worth it.\n\nhandle_revision_opt is called once apart from within setup_revisions,\nfrom within revision.c:parse_revision_opt.\n\nIf this version is not acceptable, we should either revert back to your version\nof the patch with the fixed variable name OR consider re-writing\nhandle_revision_opt, as per your suggested flow. Note that this will put the\ncode for \"Stuff it in argv[left++]\" in every caller.\n\nThank you for the time you have spent on analysing each version of the patch!\n\n--\nBest Regards,\n\nSiddharth Kannan.\n"},{"id":"311364","messageId":"20170212184132.12375-1-gitster@pobox.com","threadId":"45104","inReplyTo":"xmqqa89sguu4.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/3] prepare for a rev/range that begins with a dash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-12T18:41:29Z","receivedAt":"2017-02-12T18:41:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It turns out that telling handle_revision_opt() not to molest argv[left++]\ndoes not have heavy fallout.\n\nJunio C Hamano (3):\n handle_revision_opt(): do not update argv[left++] with an unknown arg\n setup_revisions(): swap if/else bodies to make the next step more readable\n setup_revisions(): allow a rev that begins with a dash\n\n revision.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\n-- \n2.12.0-rc1-212-ga9adfb24fa\n\n"},{"id":"311365","messageId":"20170212184132.12375-2-gitster@pobox.com","threadId":"45104","inReplyTo":"20170212184132.12375-1-gitster@pobox.com","subject":"[PATCH 1/3] handle_revision_opt(): do not update argv[left++] with an unknown arg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-12T18:41:30Z","receivedAt":"2017-02-12T18:41:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In future steps, we will make it possible for a rev or a revision\nrange (i.e. what is understood by handle_revision_arg() helper) to\nbegin with a dash. The setup_revisions() function however currently\nconsiders anything that begins with a dash to be:\n\n - an option it itself understands and handles (some take effect by\n setting fields in the revision structure, some others are left\n in the argv[left++] to be handled in later steps);\n - an option handle_revision_opt() understands and tells us to skip;\n - an option handle_revision_opt() found to be incorrect; or\n - an option handle_revision_opt() did not understand, which is\n stuffed in argv[left++].\n\nand does not give a chance to handle_revision_arg() to inspect it.\nThe handle_revision_opt() function returns a positive count, a\nnegative count or zero to allow the caller to tell the latter three\ncases apart. A rev that begins with a dash would be thrown into the\nlast category.\n\nTeach handle_revision_opt() not to touch argv[left++] in the last\ncase. Because the other one among the two callers of this function\nimmediately errors out with the usage string when it returns zero\n(i.e. the last case above), there is no negative effect to that\ncaller.\n\nIn setup_revisions(), which is the other caller of this function,\nwe need to stuff the unknown arg to argv[left++] in this case, to\npreserve the current behaviour.\n\nSigned-off-by: Junio C Hamano \n---\n revision.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec378..4f46b8ba81 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->ignore_missing = 1;\n \t} else {\n \t\tint opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);\n-\t\tif (!opts)\n-\t\t\tunkv[(*unkc)++] = arg;\n \t\treturn opts;\n \t}\n \tif (revs->graph && revs->track_linear)\n@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \t\t\tif (opts < 0)\n \t\t\t\texit(128);\n+\t\t\t/* arg looks like an opt but something we do not recognise. */\n+\t\t\targv[left++] = arg;\n \t\t\tcontinue;\n \t\t}\n \n-- \n2.12.0-rc1-212-ga9adfb24fa\n\n"},{"id":"311366","messageId":"20170212184132.12375-3-gitster@pobox.com","threadId":"45104","inReplyTo":"20170212184132.12375-1-gitster@pobox.com","subject":"[PATCH 2/3] setup_revisions(): swap if/else bodies to make the next step more readable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-12T18:41:31Z","receivedAt":"2017-02-12T18:41:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Swap the condition and bodies of an \"if (A) do_A else do_B\" in\nsetup_revisions() to \"if (!A) do_B else do A\", to make the change in\nthe the next step easier to read. \n\nNo behaviour change is intended in this step.\n\nSigned-off-by: Junio C Hamano \n---\n revision.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 4f46b8ba81..eccf1ab695 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2237,8 +2237,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tcontinue;\n \t\t}\n \n-\n-\t\tif (handle_revision_arg(arg, revs, flags, revarg_opt)) {\n+\t\tif (!handle_revision_arg(arg, revs, flags, revarg_opt)) {\n+\t\t\tgot_rev_arg = 1;\n+\t\t} else {\n \t\t\tint j;\n \t\t\tif (seen_dashdash || *arg == '^')\n \t\t\t\tdie(\"bad revision '%s'\", arg);\n@@ -2255,8 +2256,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tappend_prune_data(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n-\t\telse\n-\t\t\tgot_rev_arg = 1;\n \t}\n \n \tif (prune_data.nr) {\n-- \n2.12.0-rc1-212-ga9adfb24fa\n\n"},{"id":"311367","messageId":"20170212184132.12375-4-gitster@pobox.com","threadId":"45104","inReplyTo":"20170212184132.12375-1-gitster@pobox.com","subject":"[PATCH 3/3] setup_revisions(): allow a rev that begins with a dash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-12T18:41:32Z","receivedAt":"2017-02-12T18:41:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Now all the preparatory pieces are in place, it is a matter of\nhandling a truly unknown option _after_ handle_revision_arg()\ndecides that arg is not a rev. \n\nSigned-off-by: Junio C Hamano \n---\n\n We _could_ do without a new variable maybe_opt and instead check if\n arg begins with a dash one more time, but it is cleaner to do it\n the way this patch does to avoid writing the same check twice. We\n may be hit with a desire similar to but an opposite of the current\n topic (which wants to allow a rev that begins with a dash), to\n start allowing an option that does not begin with a dash someday.\n\n revision.c | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex eccf1ab695..0f772ba73d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2203,6 +2203,8 @@ 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\tint maybe_opt = 0;\n+\n \t\tif (*arg == '-') {\n \t\t\tint opts;\n \n@@ -2232,13 +2234,20 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \t\t\tif (opts < 0)\n \t\t\t\texit(128);\n-\t\t\t/* arg looks like an opt but something we do not recognise. */\n-\t\t\targv[left++] = arg;\n-\t\t\tcontinue;\n+\t\t\t/*\n+\t\t\t * arg looks like an opt but something we do not recognise.\n+\t\t\t * It may be a rev that begins with a dash; fall through to\n+\t\t\t * let handle_revision_arg() have a say in this.\n+\t\t\t */\n+\t\t\tmaybe_opt = 1;\n \t\t}\n \n \t\tif (!handle_revision_arg(arg, revs, flags, revarg_opt)) {\n \t\t\tgot_rev_arg = 1;\n+\t\t} else if (maybe_opt) {\n+\t\t\t/* it turns out that it is not a rev after all */\n+\t\t\targv[left++] = arg;\n+\t\t\tcontinue;\n \t\t} else {\n \t\t\tint j;\n \t\t\tif (seen_dashdash || *arg == '^')\n-- \n2.12.0-rc1-212-ga9adfb24fa\n\n"},{"id":"311368","messageId":"xmqqwpcvfdao.fsf@gitster.mtv.corp.google.com","threadId":"45104","inReplyTo":"20170212123630.GA20872@ubuntu-512mb-blr1-01.localdomain","subject":"Re: [PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-12T18:56:47Z","receivedAt":"2017-02-12T18:56:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan writes:\n\n> This \"changing the order\" gave me the idea to change the flow. I tried to\n> implement the above steps without touching the function handle_revision_opt. By\n> inserting the handle_revision_arg call just before calling handle_revision_opt.\n\nChanging the order is changing the order of the function calls,\ni.e. changing the flow. So at the idea level we are on the same\npage.\n\nI was shooting for not having to duplicate calls to\nhandle_revision_arg(). \n\n>> But I think the resulting code flow is much closer to the\n>> above ideal.\n>\n> (about Junio's version of the patch): Yes, I agree with you on this. It's like\n> the ideal, but the argv has already been populated, so the only remaining step\n> is \"left++\".\n>> \n>> Such a change to handle_revision_opt() unfortunately affects other\n>> callers of the function, so it may not be worth it.\n\nSee the 3-patch series I just sent out. I didn't think it through\nvery carefully (especially the error message the other caller\nproduces), but the whole thing _smells_ correct to me.\n"},{"id":"311426","messageId":"xmqq1sv1euob.fsf@gitster.mtv.corp.google.com","threadId":"45104","inReplyTo":"vpqbmu768on.fsf@anie.imag.fr","subject":"Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T19:51:16Z","receivedAt":"2017-02-13T19:51:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Siddharth Kannan writes:\n>\n>> +\tif (!strcmp(name, \"-\")) {\n>> +\t\tname = \"@{-1}\";\n>> +\t\tlen = 5;\n>> +\t}\n>\n> One drawback of this approach is that further error messages will be\n> given from the \"@{-1}\" string that the user never typed.\n\nRight.\n\n> There are at least:\n>\n> $ git grep -n -A1 'strcmp.*\"-\"' | grep -B 1 '@\\{1\\}'\n> builtin/checkout.c:975: if (!strcmp(arg, \"-\"))\n> builtin/checkout.c-976- arg = \"@{-1}\";\n\nI didn't check the surrounding context to be sure, but I think this\n\"- to @{-1}\" conversion cannot be delegated down to revision parsing\nthat eventually wants to return a 40-hex as the result. \n\nWe do want a branch _name_ sometimes when we say \"@{-1}\"; \"checkout\nmaster\" (i.e. checkout by name) and \"checkout master^0\" (i.e. the\nsame commit object, but not by name) do different things.\n\n> builtin/merge.c:1231: } else if (argc == 1 && !strcmp(argv[0], \"-\")) {\n> builtin/merge.c-1232- argv[0] = \"@{-1}\";\n> --\n> builtin/revert.c:158: if (!strcmp(argv[1], \"-\"))\n> builtin/revert.c-159- argv[1] = \"@{-1}\";\n\nThese should be safe to delegate down.\n\n> builtin/worktree.c:344: if (!strcmp(branch, \"-\"))\n> builtin/worktree.c-345- branch = \"@{-1}\";\n\nI do not know about this one, but it smells like a branch name that\nwants to be used before it gets turned into 40-hex.\n"},{"id":"311427","messageId":"xmqqwpctdfj9.fsf@gitster.mtv.corp.google.com","threadId":"45104","inReplyTo":"xmqq1sv1euob.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T20:03:38Z","receivedAt":"2017-02-13T20:03:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> Siddharth Kannan writes:\n>>\n>>> +\tif (!strcmp(name, \"-\")) {\n>>> +\t\tname = \"@{-1}\";\n>>> +\t\tlen = 5;\n>>> +\t}\n>>\n>> One drawback of this approach is that further error messages will be\n>> given from the \"@{-1}\" string that the user never typed.\n>\n> Right.\n>\n>> There are at least:\n>>\n>> $ git grep -n -A1 'strcmp.*\"-\"' | grep -B 1 '@\\{1\\}'\n>> builtin/checkout.c:975: if (!strcmp(arg, \"-\"))\n>> builtin/checkout.c-976- arg = \"@{-1}\";\n>\n> I didn't check the surrounding context to be sure, but I think this\n> \"- to @{-1}\" conversion cannot be delegated down to revision parsing\n> that eventually wants to return a 40-hex as the result. \n>\n> We do want a branch _name_ sometimes when we say \"@{-1}\"; \"checkout\n> master\" (i.e. checkout by name) and \"checkout master^0\" (i.e. the\n> same commit object, but not by name) do different things.\n\nFYI, the \"@{-} to branch name\" translation happens in\ninterpret_branch_name(). I do not offhand recall if any callers\nprotect their calls to the function with conditionals that assume\nthe thing must begin with \"@{\" or cannot begin with \"-\" (the latter\nof which is similar to the topic of patch 1/2 of this series), but I\nsuspect that teaching the function that \"-\" means the same as\n\"@{-1}\" would bring us closer to where we want to go.\n\n"},{"id":"311507","messageId":"20170214042329.GA24543@ubuntu-512mb-blr1-01.localdomain","threadId":"45104","inReplyTo":"xmqqwpcvfdao.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2 v3] revision.c: args starting with \"-\" might be a revision","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-14T04:23:29Z","receivedAt":"2017-02-14T04:23:37Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hey Junio,\n> \n> See the 3-patch series I just sent out. I didn't think it through\n> very carefully (especially the error message the other caller\n> produces), but the whole thing _smells_ correct to me.\n\nOkay, got it! I will write-up those changes, and make sure nothing bad\nhappens. (Also, the one other function that calls handle_revision_opt,\nparse_revision_opt needs to be fixed for any changes in\nhandle_revision_opt.)\n\nI will do all of this in the next week (Unfortunately, exams!) and\nsubmit a new version of this patch (Also, I need to update tests, add\ndocumentation, and remove code that does this shorthand stuff for\nother commands as per Matthieu's comments)\n\n--\nBest Regards,\n\nSiddharth Kannan.\n"}]}