{"thread":{"id":"45153","subject":"[PATCH 0/4 v4] WIP: allow \"-\" as a shorthand for \"previous branch\"","startedAt":"2017-02-16T15:14:34Z","lastAt":"2017-02-22T06:30:05Z","messageCount":16,"participants":["Siddharth Kannan","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"311762","messageId":"1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com","threadId":"45153","inReplyTo":null,"subject":"[PATCH 0/4 v4] WIP: allow \"-\" as a shorthand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-16T15:14:10Z","receivedAt":"2017-02-16T15:14:34Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"This is as per our discussion[1]. The patches and commit messages are based on\nJunio's patches that were posted as a reply to\n<20170212184132.12375-1-gitster@pobox.com>.\n\nAs per Matthieu's comments, I have updated the tests, but there is still one\nthing that is not working: log -@{yesterday} or log -@{2.days.ago}\n\nFor the other kinds of suffixes, such as -^ or -~ or -~N, the suffix\ninformation is first extracted and then, the function get_sha1_1 is called with\nname=\"-^\" and len=1 (which is the reason for the changed condition inside Patch\n4 of this series).\n\nFor -@{yesterday} kind of queries, the functions dwim_log,\ninterpret_branch_name and interpret_nth_prior_checkout are called.\n\n1. A nice way to solve this would be to extend the replacement of \"-\" with\n\"@{-1}\" one step further. Using strbuf, instead of replacing the whole string\nwith \"@{-1}\" we would simply replace \"-\" with \"@{-1}\" expanding the string\nappropriately. This will ensure that all the code is inside the function\nget_sha1_1. The code to do this is in the cover section of the 4th patch in this\nseries.\n\n2. we could go down the dwim_log codepath, and find another suitable place to\nmake the same \"-\" -> \"@{-1}\" replacement. In the time that I spent till now, it\nseems that the suffix information (i.e. @{yesterday} or @{2.days.ago}) is\nextracted _after_ the branch information has been extracted, so I suspect that\nwe will have to keep that part intact even in this solution. (I am not too\nsure about this. If this is the preferred solution, then I will dig deeper and\nfind the right place as I did for the first part of this patch)\n\nMatthieu: Thanks a lot for your comments on the tests! test_commit has made the\ntests a lot cleaner!\n\n[1]: \n\nSiddharth Kannan (4):\n revision.c: do not update argv with unknown option\n revision.c: swap if/else blocks\n revision.c: args starting with \"-\" might be a revision\n sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"\n\n revision.c | 15 ++++---\n sha1_name.c | 5 +++\n t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 120 insertions(+), 6 deletions(-)\n create mode 100755 t/t4214-log-shorthand.sh\n\n-- \n2.1.4\n\n"},{"id":"311763","messageId":"1487258054-32292-2-git-send-email-kannan.siddharth12@gmail.com","threadId":"45153","inReplyTo":"1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 1/4 v4] revision.c: do not update argv with unknown option","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-16T15:14:11Z","receivedAt":"2017-02-16T15:14:42Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"handle_revision_opt() tries to recognize and handle the given argument. If an\noption was unknown to it, it used to add the option to unkv[(*unkc)++]. This\nincrement of unkc causes the variable in the caller to change.\n\nTeach handle_revision_opt to not update unknown arguments inside unkc anymore.\nThis is now the responsibility of the caller.\n\nThere are two callers of this function:\n\n1. setup_revision: Changes have been made so that setup_revision will now\nupdate the unknown option in argv\n\n2. parse_revision_opt: No changes are required here. This function throws an\nerror whenever the option provided as argument was unknown to\nhandle_revision_opt().\n\nSigned-off-by: Siddharth Kannan \n---\n revision.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b37dbec..5674a9a 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 is an unknown option */\n+\t\t\targv[left++] = arg;\n \t\t\tcontinue;\n \t\t}\n \n-- \n2.1.4\n\n"},{"id":"311764","messageId":"1487258054-32292-3-git-send-email-kannan.siddharth12@gmail.com","threadId":"45153","inReplyTo":"1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 2/4 v4] revision.c: swap if/else blocks","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-16T15:14:12Z","receivedAt":"2017-02-16T15:14:47Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"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: Siddharth Kannan \n---\n revision.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 5674a9a..8d4ddae 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2238,7 +2238,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\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\telse {\n \t\t\tint j;\n \t\t\tif (seen_dashdash || *arg == '^')\n \t\t\t\tdie(\"bad revision '%s'\", arg);\n@@ -2255,8 +2257,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.1.4\n\n"},{"id":"311765","messageId":"1487258054-32292-4-git-send-email-kannan.siddharth12@gmail.com","threadId":"45153","inReplyTo":"1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 3/4 v4] revision.c: args starting with \"-\" might be a revision","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-16T15:14:13Z","receivedAt":"2017-02-16T15:14:50Z","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 or an unknown option.\n\nTeach setup_revisions to check if an argument is a revision before adding it as\nan unknown option (something that setup_revisions didn't understand) to argv,\nand moving on to the next argument.\n\nThis patch prepares the addition of \"-\" as a shorthand for \"previous branch\".\n\nSigned-off-by: Siddharth Kannan \n---\n revision.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8d4ddae..5470c33 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2203,6 +2203,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 maybe_opt = 0;\n \t\tif (*arg == '-') {\n \t\t\tint opts;\n \n@@ -2232,15 +2233,17 @@ 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 is an unknown option */\n-\t\t\targv[left++] = arg;\n-\t\t\tcontinue;\n+\t\t\tmaybe_opt = 1;\n \t\t}\n \n \n \t\tif (!handle_revision_arg(arg, revs, flags, revarg_opt))\n \t\t\tgot_rev_arg = 1;\n-\t\telse {\n+\t\telse if (maybe_opt) {\n+\t\t\t/* arg is an unknown option */\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 \t\t\t\tdie(\"bad revision '%s'\", arg);\n-- \n2.1.4\n\n"},{"id":"311766","messageId":"1487258054-32292-5-git-send-email-kannan.siddharth12@gmail.com","threadId":"45153","inReplyTo":"1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-16T15:14:14Z","receivedAt":"2017-02-16T15:14:58Z","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\nInstead of replacing the whole string, we would expand it accordingly using:\n\nif (*name == '-') {\n if (len == 1) {\n name = \"@{-1}\";\n len = 5;\n } else {\n struct strbuf changed_argument = STRBUF_INIT;\n\n strbuf_addstr(&changed_argument, \"@{-1}\");\n strbuf_addstr(&changed_argument, name + 1);\n\n strbuf_setlen(&changed_argument, strlen(name) + 4);\n\n name = strbuf_detach(&changed_argument, NULL);\n }\n}\n\nJunio's comments on a previous version of the patch which used this same\napproach but inside setup_revisions [1]\n\n[1]: \n\n sha1_name.c | 5 +++\n t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+)\n create mode 100755 t/t4214-log-shorthand.sh\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 73a915f..2f86bc9 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 (*name == '-' && len == 1) {\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..659b100\n--- /dev/null\n+++ b/t/t4214-log-shorthand.sh\n@@ -0,0 +1,106 @@\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+\ttest_commit first &&\n+\ttest_commit second &&\n+\ttest_commit third &&\n+\ttest_commit fourth &&\n+\ttest_commit fifth &&\n+\ttest_commit sixth &&\n+\ttest_commit seventh\n+'\n+\n+test_expect_success '\"log -\" should not work initially' '\n+\ttest_must_fail git log -\n+'\n+\n+test_expect_success 'setup branches for testing' '\n+\tgit checkout -b testing-1 master^ &&\n+\tgit checkout -b testing-2 master~2 &&\n+\tgit checkout master\n+'\n+\n+test_expect_success '\"log -\" should work' '\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+\n+test_expect_success 'multiple separate arguments should be handled properly' '\n+\tgit checkout testing-2 &&\n+\tgit checkout master &&\n+\tgit log - - >expect.1 &&\n+\tgit log @{-1} @{-1} >actual.1 &&\n+\tgit log - HEAD >expect.2 &&\n+\tgit log @{-1} HEAD >actual.2 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2\n+'\n+\n+test_expect_success 'revision ranges with same start and end should be empty' '\n+\tgit checkout testing-2 &&\n+\tgit checkout master &&\n+\ttest 0 -eq $(git log -...- | wc -l) &&\n+\ttest 0 -eq $(git log -..- | wc -l)\n+'\n+\n+test_expect_success 'suffixes to - should work' '\n+\tgit checkout testing-2 &&\n+\tgit checkout master &&\n+\tgit log -~ >expect.1 &&\n+\tgit log @{-1}~ >actual.1 &&\n+\tgit log -~2 >expect.2 &&\n+\tgit log @{-1}~2 >actual.2 &&\n+\tgit log -^ >expect.3 &&\n+\tgit log @{-1}^ >actual.3 &&\n+\tgit log -@{yesterday} >expect.4 &&\n+\tgit log @{-1}@{yesterday} >actual.4 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2 &&\n+\ttest_cmp expect.3 actual.3 &&\n+\ttest_cmp expect.4 actual.4\n+'\n+\n+test_done\n-- \n2.1.4\n\n"},{"id":"311768","messageId":"vpqa89mnl4z.fsf@anie.imag.fr","threadId":"45153","inReplyTo":"1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH 0/4 v4] WIP: allow \"-\" as a shorthand for \"previous branch\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2017-02-16T16:41:32Z","receivedAt":"2017-02-16T16:41:42Z","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> This is as per our discussion[1]. The patches and commit messages are based on\n> Junio's patches that were posted as a reply to\n> <20170212184132.12375-1-gitster@pobox.com>.\n>\n> As per Matthieu's comments, I have updated the tests, but there is still one\n> thing that is not working: log -@{yesterday} or log -@{2.days.ago}\n\nNote that I did not request that these things work, just that they seem\nto be relevant tests: IMHO it's OK to reject them, but for example we\ndon't want them to segfault. And having a test is a good hint that you\nthought about what could happen and to document it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"311770","messageId":"vpqwpcqm69k.fsf@anie.imag.fr","threadId":"45153","inReplyTo":"1487258054-32292-2-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2017-02-16T16:48:07Z","receivedAt":"2017-02-16T16:48:15Z","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> handle_revision_opt() tries to recognize and handle the given argument. If an\n> option was unknown to it, it used to add the option to unkv[(*unkc)++]. This\n> increment of unkc causes the variable in the caller to change.\n>\n> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.\n> This is now the responsibility of the caller.\n>\n> There are two callers of this function:\n>\n> 1. setup_revision: Changes have been made so that setup_revision will now\n> update the unknown option in argv\n\nYou're writting \"Changes have been made\", but I did not see any up to\nthis point in the series.\n\nWe write patch series so that they are bisectable, i.e. each commit\nshould be correct (compileable, pass tests, consistent\ndocumentation, ...). Here, it seems you are introducing a breakage to\nrepair it later.\n\nOther that bisectability, this makes review harder: at this point the\nreader knows it's broken, guesses that it will be repaired later, but\ndoes not know in which patch.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"311786","messageId":"xmqqwpcqxay0.fsf@gitster.mtv.corp.google.com","threadId":"45153","inReplyTo":"vpqwpcqm69k.fsf@anie.imag.fr","subject":"Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-16T18:11:35Z","receivedAt":"2017-02-16T18:11:43Z","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>> handle_revision_opt() tries to recognize and handle the given argument. If an\n>> option was unknown to it, it used to add the option to unkv[(*unkc)++]. This\n>> increment of unkc causes the variable in the caller to change.\n>>\n>> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.\n>> This is now the responsibility of the caller.\n>>\n>> There are two callers of this function:\n>>\n>> 1. setup_revision: Changes have been made so that setup_revision will now\n>> update the unknown option in argv\n>\n> You're writting \"Changes have been made\", but I did not see any up to\n> this point in the series.\n\nActually, I think you misread the patch and explanation.\nhandle_revision_opt() used to be responsible for stuffing unknown\nones to unkv[] array passed from the caller even when it returns 0\n(i.e. \"I do not know what they are\" case, as opposed to \"I know what\nthey are, I am not handling them here and leaving them in unkv[]\"\ncase--the latter returns non-zero). The first hunk makes the\nfunction stop doing so, and to compensate, the second hunk, which is\nin setup_revisions() that calls the function, now makes the caller\ndo the equivalent \"argv[left++] = arg\" there after it receives 0.\n\nSo \"Changes have been made\" to setup_revisions() to compensate for\nthe change of behaviour in the called function.\n\nThe enumerated point 2. (not in your response) explains why such a\ncorresponding compensatory change is not there for the other caller\nof this function whose behaviour has changed.\n\n> We write patch series so that they are bisectable, i.e. each commit\n> should be correct (compileable, pass tests, consistent\n> documentation, ...). Here, it seems you are introducing a breakage to\n> repair it later.\n\nThat is a very good point to stress, but 1. is exactly to avoid\nbreakage in this individual step (and 2. is an explanation why the\nchange does not break the other caller).\n"},{"id":"311790","messageId":"vpqwpcqgfmw.fsf@anie.imag.fr","threadId":"45153","inReplyTo":"xmqqwpcqxay0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2017-02-16T18:22:15Z","receivedAt":"2017-02-16T18:22:23Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> Siddharth Kannan writes:\n>>\n>>> handle_revision_opt() tries to recognize and handle the given argument. If an\n>>> option was unknown to it, it used to add the option to unkv[(*unkc)++]. This\n>>> increment of unkc causes the variable in the caller to change.\n>>>\n>>> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.\n>>> This is now the responsibility of the caller.\n>>>\n>>> There are two callers of this function:\n>>>\n>>> 1. setup_revision: Changes have been made so that setup_revision will now\n>>> update the unknown option in argv\n>>\n>> You're writting \"Changes have been made\", but I did not see any up to\n>> this point in the series.\n>\n> Actually, I think you misread the patch and explanation.\n> handle_revision_opt() used to be responsible for stuffing unknown\n> ones to unkv[] array passed from the caller even when it returns 0\n> (i.e. \"I do not know what they are\" case, as opposed to \"I know what\n> they are, I am not handling them here and leaving them in unkv[]\"\n> case--the latter returns non-zero). The first hunk makes the\n> function stop doing so, and to compensate, the second hunk, which is\n> in setup_revisions()\n\nIndeed, I misread the patch. The explanation could be a little bit more\n\"tired-reviewer-proof\" by not using a past tone, perhaps\n\n1. setup_revision, which is changed to ...\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"311794","messageId":"xmqqinoax96u.fsf@gitster.mtv.corp.google.com","threadId":"45153","inReplyTo":"vpqa89mnl4z.fsf@anie.imag.fr","subject":"Re: [PATCH 0/4 v4] WIP: allow \"-\" as a shorthand for \"previous branch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-16T18:49:29Z","receivedAt":"2017-02-16T18:49:36Z","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>> This is as per our discussion[1]. The patches and commit messages are based on\n>> Junio's patches that were posted as a reply to\n>> <20170212184132.12375-1-gitster@pobox.com>.\n>>\n>> As per Matthieu's comments, I have updated the tests, but there is still one\n>> thing that is not working: log -@{yesterday} or log -@{2.days.ago}\n>\n> Note that I did not request that these things work, just that they seem\n> to be relevant tests: IMHO it's OK to reject them, but for example we\n> don't want them to segfault. And having a test is a good hint that you\n> thought about what could happen and to document it.\n\nThe branch we were on before would be a ref, and the ref may know\nwhere it was yesterday? If @{-1}@{1.day} works it would be natural\nto expect -@{1.day} to, too, but there probably is some disambiguity\nor other reasons that they cannot or should not work that way I am\nmissing, in which case it is fine (\"too much work for too obscure\nfeature that is not expected to be used often\" is also an acceptable\nreason) to punt or deliberately not support it, as long as it is\nexplained in the log and/or doc (future developers need to know if\nwe are simply punting, or if we found a case where it would hurt end\nuser experience if we supported the feature), and as long as it does\nnot do a wrong thing (dying with \"we do not support it\" is OK,\nsegfaulting or doing random other things is not).\n\nThanks.\n"},{"id":"311796","messageId":"xmqq8tp6x8b6.fsf@gitster.mtv.corp.google.com","threadId":"45153","inReplyTo":"1487258054-32292-5-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-16T19:08:29Z","receivedAt":"2017-02-16T19:08:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan writes:\n\n> Instead of replacing the whole string, we would expand it accordingly using:\n>\n> if (*name == '-') {\n> if (len == 1) {\n> name = \"@{-1}\";\n> len = 5;\n> } else {\n> struct strbuf changed_argument = STRBUF_INIT;\n>\n> strbuf_addstr(&changed_argument, \"@{-1}\");\n> strbuf_addstr(&changed_argument, name + 1);\n>\n> strbuf_setlen(&changed_argument, strlen(name) + 4);\n>\n> name = strbuf_detach(&changed_argument, NULL);\n> }\n> }\n>\n> Junio's comments on a previous version of the patch which used this same\n> approach but inside setup_revisions [1]\n>\n> [1]: \n\nWhat I said is that when we know we got \"-\", there is no reason to\nreplace it with and textually parse \"@{-1}\".\n\n> +\tif (*name == '-' && len == 1) {\n> +\t\tname = \"@{-1}\";\n> +\t\tlen = 5;\n> +\t}\n> +\n> \tret = get_sha1_basic(name, len, sha1, lookup_flags);\n\nIf we look at get_sha1_basic(), it obviously is not prepared to\nunderstand \"-\" as \"@{-1}\", and the primary obstacle is that the\nunderlying interpret_nth_prior_checkout() does two things. It\nexpects to take \"@{-}\" as a string, and the first half parses\nthe into \"long nth\". The latter half then finds the nth prior\ncheckout. We probably should factor out the latter half into a\nseparate function find_nth_prior_checkout() that takes \"long nth\" as\ninput, and call it from interpret_nth_prior_checkout(), as a\npreparatory step. Once it is done, get_sha1_basic() can notice that\nit was fed (len == 1 && str[0] == '-') and make a direct call to\nfind_nth_prior_checkout() without going through the \"pass '@{-1}' as\ntext, have interpret_nth_prior_checkout() to parse it to recover 1\",\nwhich is a roundabout way to do what you want to do.\n\nHaving said all that, I do not think the remainder of the code is\nprepared to take \"-\", not yet anyway [*1*], so turning \"-\" into\n\"@{-1}\" this patch does before it calls get_sha1_basic(), while it\nis not an ideal final state, is probably an acceptable milestone to\nstop at.\n\nIt is a separate matter if this patch is sufficient to produce\ncorrect results, though. I haven't studied the callers of this\nchange to make sure yet, and may find bugs in this approach later.\n\n\n[Footnote]\n\n*1* For example, the existing callsite in get_sha1_basic() that\n calls interpret_nth_prior_checkout() does not replace \"str\" with\n what was returned when the HEAD is not detached. The callpath\n then depends on dwim_ref() to also understand \"@{-1}\" it got\n from the caller. If we really want to keep what came from the\n end user as-is so that error message can include it, we'd need\n to teach dwim_ref() about the new \"-\" convention. The extent of\n necessary change will become a lot larger. On the other hand,\n if we allow error messages and reports to use a real refname\n instead of parrotting exactly what the user gave us, I think we\n may be able to arrange to replace str/len in get_sha1_basic()\n when we call interpret/find_nth_prior_checkout() and get a ref,\n without having to teach the new \"-\" convention all over the\n place.\n"},{"id":"311799","messageId":"CAN-3QhrO2FDDNfXLZJm-3DO5fw6m0Ea0QjW4Cu+Ceo9aJJBPWg@mail.gmail.com","threadId":"45153","inReplyTo":"vpqwpcqgfmw.fsf@anie.imag.fr","subject":"Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-16T19:39:41Z","receivedAt":"2017-02-16T19:40:27Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hey Matthieu,\n\nOn 16 February 2017 at 23:52, Matthieu Moy wrote:\n>\n> Indeed, I misread the patch. The explanation could be a little bit more\n> \"tired-reviewer-proof\" by not using a past tone, perhaps\n>\n> 1. setup_revision, which is changed to ...\n\nOh, okay! Sorry about the confusion!\n\nYes, I used the past perfect tense to refer to changes that were made\nin this particular patch!\n\nI will change the message in the next version to something that's in\npresent tense.\n\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n\n\n\n-- \n\nBest Regards,\n\n- Siddharth.\n"},{"id":"311800","messageId":"CAN-3Qhok0WVZHBc-tFgTCNebGKRH87jPGe-rbRwTrsh1qLTfDQ@mail.gmail.com","threadId":"45153","inReplyTo":"xmqqinoax96u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/4 v4] WIP: allow \"-\" as a shorthand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-16T19:43:57Z","receivedAt":"2017-02-16T19:44:43Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"Hey Junio and Matthieu,\n\nOn 17 February 2017 at 00:19, Junio C Hamano wrote:\n> Matthieu Moy writes:\n>\n>> Siddharth Kannan writes:\n>>\n>>> This is as per our discussion[1]. The patches and commit messages are based on\n>>> Junio's patches that were posted as a reply to\n>>> <20170212184132.12375-1-gitster@pobox.com>.\n>>>\n>>> As per Matthieu's comments, I have updated the tests, but there is still one\n>>> thing that is not working: log -@{yesterday} or log -@{2.days.ago}\n>>\n>> Note that I did not request that these things work, just that they seem\n>> to be relevant tests: IMHO it's OK to reject them, but for example we\n>> don't want them to segfault. And having a test is a good hint that you\n>> thought about what could happen and to document it.\n>\n> The branch we were on before would be a ref, and the ref may know\n> where it was yesterday? If @{-1}@{1.day} works it would be natural\n> to expect -@{1.day} to, too, but there probably is some disambiguity\n> or other reasons that they cannot or should not work that way I am\n> missing, in which case it is fine (\"too much work for too obscure\n> feature that is not expected to be used often\" is also an acceptable\n> reason) to punt or deliberately not support it, as long as it is\n> explained in the log and/or doc (future developers need to know if\n> we are simply punting, or if we found a case where it would hurt end\n> user experience if we supported the feature), and as long as it does\n> not do a wrong thing (dying with \"we do not support it\" is OK,\n> segfaulting or doing random other things is not).\n>\n\nRight now, these commands die with an \"fatal: unrecognized argument:\n-@{yesterday}\" or a \"fatal: unrecognized argument: -@{2.days.ago}\".\nSo, it is definitely not doing anything \"random\" :)\n\nI will wait for consensus on whether these should or should not be\nsupported.\n\n-- \n\nBest Regards,\n\n- Siddharth.\n"},{"id":"312153","messageId":"CAN-3QhoXBnLWyfuUsuvvRMYNnoupMrQHxE_G=ysyA_14KX4Yrw@mail.gmail.com","threadId":"45153","inReplyTo":"xmqq8tp6x8b6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-20T14:21:12Z","receivedAt":"2017-02-20T14:22:09Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"On 17 February 2017 at 00:38, Junio C Hamano wrote:\n> Having said all that, I do not think the remainder of the code is\n> prepared to take \"-\", not yet anyway [*1*], so turning \"-\" into\n> \"@{-1}\" this patch does before it calls get_sha1_basic(), while it\n> is not an ideal final state, is probably an acceptable milestone to\n> stop at.\n\nSo, is it okay to stop with just supporting \"-\" and not support things\nlike \"-@{yesterday}\"?\n\nMatthieu's comments on the matter:\n\n Siddharth Kannan writes:\n\n > As per Matthieu's comments, I have updated the tests, but there\nis still one\n > thing that is not working: log -@{yesterday} or log -@{2.days.ago}\n\n Note that I did not request that these things work, just that they seem\n to be relevant tests: IMHO it's OK to reject them, but for example we\n don't want them to segfault. And having a test is a good hint that you\n thought about what could happen and to document it.\n\n[Quoted from email ]\n\n\n>\n> It is a separate matter if this patch is sufficient to produce\n> correct results, though. I haven't studied the callers of this\n> change to make sure yet, and may find bugs in this approach later.\n>\n\n-- \n\nBest Regards,\n\n- Siddharth Kannan.\n"},{"id":"312164","messageId":"xmqqshn8ip0j.fsf@gitster.mtv.corp.google.com","threadId":"45153","inReplyTo":"CAN-3QhoXBnLWyfuUsuvvRMYNnoupMrQHxE_G=ysyA_14KX4Yrw@mail.gmail.com","subject":"Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-20T20:30:20Z","receivedAt":"2017-02-20T20:30:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan writes:\n\n> On 17 February 2017 at 00:38, Junio C Hamano wrote:\n>> Having said all that, I do not think the remainder of the code is\n>> prepared to take \"-\", not yet anyway [*1*], so turning \"-\" into\n>> \"@{-1}\" this patch does before it calls get_sha1_basic(), while it\n>> is not an ideal final state, is probably an acceptable milestone to\n>> stop at.\n>\n> So, is it okay to stop with just supporting \"-\" and not support things\n> like \"-@{yesterday}\"?\n\nIf the approach to turn \"-\" into \"@{-1}\" at that spot you did will\ncause \"-@{yesterday}\" to barf, then I'd say so be it for now ;-).\nWe can later spread the understanding of \"-\" to functions deeper in\nthe callchain and add support for that, no?\n\n>> It is a separate matter if this patch is sufficient to produce\n>> correct results, though. I haven't studied the callers of this\n>> change to make sure yet, and may find bugs in this approach later.\n"},{"id":"312273","messageId":"CAN-3QhrAYJaVNf-4LKpT2uZQr1ubyxpd1Cpo-hVhGL8dh+_SXA@mail.gmail.com","threadId":"45153","inReplyTo":"xmqqshn8ip0j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-22T06:27:13Z","receivedAt":"2017-02-22T06:30:05Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"On 21 February 2017 at 02:00, Junio C Hamano wrote:\n> Siddharth Kannan writes:\n> > So, is it okay to stop with just supporting \"-\" and not support things\n> > like \"-@{yesterday}\"?\n>\n> If the approach to turn \"-\" into \"@{-1}\" at that spot you did will\n> cause \"-@{yesterday}\" to barf, then I'd say so be it for now ;-).\n> We can later spread the understanding of \"-\" to functions deeper in\n> the callchain and add support for that, no?\n\nYes, this can be done later. I will send these patches again, with\nonly the changes that are discussed here.\n\nI will keep the tests for \"-@{yesterday}\" as failing tests, if that\nwould help in finding this again and fixing it later.\n\nThanks for your review, Junio!\n\n-- \n\nBest Regards,\n\n- Siddharth Kannan.\n"}]}