{"thread":{"id":"45219","subject":"[PATCH 0/6 v5] allow \"-\" as a shorthand for \"previous branch\"","startedAt":"2017-02-25T07:25:02Z","lastAt":"2017-03-14T02:10:46Z","messageCount":12,"participants":["Siddharth Kannan","Junio C Hamano","mash"],"isPatch":true,"patchVersion":5,"patchTotal":6},"messages":[{"id":"312613","messageId":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":null,"subject":"[PATCH 0/6 v5] allow \"-\" as a shorthand for \"previous branch\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:24:41Z","receivedAt":"2017-02-25T07:25:02Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"An updated version of the patch [1]. Discussion here[1] has been taken into\naccount. The test for \"-@{yesterday}\" is there inside the log-shorthand test,\nit is commented out for now.\n\nI have removed the redundant pieces of code in merge.c and revert.c as mentioned\nby Matthieu in [2]. As analysed by Junio[3], the same type of code inside\ncheckout.c and worktree.c can not be removed because the appropriate functions\ninside revision.c are not called in their codepaths.\n\nThanks for your review of the previous versions, Junio and Matthieu!\n\n[1]: 1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com\n[2]: vpqbmu768on.fsf@anie.imag.fr\n[3]: xmqq1sv1euob.fsf@gitster.mtv.corp.google.com\n\nSiddharth Kannan (6):\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  merge.c: delegate handling of \"-\" shorthand to revision.c:get_sha1\n  revert.c: delegate handling of \"-\" shorthand to setup_revisions\n\n builtin/merge.c                   |   2 -\n builtin/revert.c                  |   2 -\n revision.c                        |  15 +++---\n sha1_name.c                       |   5 ++\n t/t3035-merge-hyphen-shorthand.sh |  33 ++++++++++++\n t/t3514-revert-shorthand.sh       |  25 +++++++++\n t/t4214-log-shorthand.sh          | 106 ++++++++++++++++++++++++++++++++++++++\n 7 files changed, 178 insertions(+), 10 deletions(-)\n create mode 100755 t/t3035-merge-hyphen-shorthand.sh\n create mode 100755 t/t3514-revert-shorthand.sh\n create mode 100755 t/t4214-log-shorthand.sh\n\n-- \n2.1.4\n\n"},{"id":"312614","messageId":"1488007487-12965-4-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 3/6 v5] revision.c: args starting with \"-\" might be a revision","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:24:44Z","receivedAt":"2017-02-25T07:25:23Z","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 <kannan.siddharth12@gmail.com>\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":"312615","messageId":"1488007487-12965-7-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 6/6 v5] revert.c: delegate handling of \"-\" shorthand to setup_revisions","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:24:47Z","receivedAt":"2017-02-25T07:25:26Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"revert.c:run_sequencer calls setup_revisions right after replacing \"-\" with\n\"@{-1}\" for this shorthand. A previous patch taught setup_revisions to handle\nthis shorthand by doing the required replacement inside revision.c:get_sha1_1.\n\nHence, the code here is redundant and has been removed.\n\nThis patch also adds a test to check that revert recognizes the \"-\" shorthand.\n\nSigned-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>\n---\n builtin/revert.c            |  2 --\n t/t3514-revert-shorthand.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 25 insertions(+), 2 deletions(-)\n create mode 100755 t/t3514-revert-shorthand.sh\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 4ca5b51..0bc6657 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -155,8 +155,6 @@ static int run_sequencer(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/t/t3514-revert-shorthand.sh b/t/t3514-revert-shorthand.sh\nnew file mode 100755\nindex 0000000..51f8c81d\n--- /dev/null\n+++ b/t/t3514-revert-shorthand.sh\n@@ -0,0 +1,25 @@\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+'\n+\n+test_expect_success 'setup branches' '\n+        echo \"hello\" >hello &&\n+        cat hello >expect &&\n+        git add hello &&\n+        git commit -m \"hello first commit\" &&\n+        echo \"world\" >>hello &&\n+        git commit -am \"hello second commit\" &&\n+        git checkout -b testing-1 &&\n+        git checkout master &&\n+        git revert --no-edit - &&\n+        cat hello >actual &&\n+        test_cmp expect actual\n+'\n+\n+test_done\n-- \n2.1.4\n\n"},{"id":"312616","messageId":"1488007487-12965-6-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 5/6 v5] merge.c: delegate handling of \"-\" shorthand to revision.c:get_sha1","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:24:46Z","receivedAt":"2017-02-25T07:25:30Z","isPatch":true,"sender":{"key":"kannan.siddharth12@gmail.com","avatar":"https://gravatar.com/avatar/f555b34abc9cec49fd7e7ca4a55b58be0e0af0fc58e75b7887210199b7d4ca5a?d=mp&s=160"},"body":"The callchain for handling each argument contains the function\nrevision.c:get_sha1 where the shorthand for \"-\" ~ \"@{-1}\" has already been\nimplemented in a previous patch; the complete callchain leading to that\nfunction is:\n\n1. merge.c:collect_parents\n2. commit.c:get_merge_parent : this function calls revision.c:get_sha1\n\nThis patch also adds a test for checking that the shorthand works properly\n\nSigned-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>\n---\n builtin/merge.c                   |  2 --\n t/t3035-merge-hyphen-shorthand.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n create mode 100755 t/t3035-merge-hyphen-shorthand.sh\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a96d4fb..36ff420 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1228,8 +1228,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\targc = setup_with_upstream(&argv);\n \t\telse\n \t\t\tdie(_(\"No commit specified and merge.defaultToUpstream not set.\"));\n-\t} else if (argc == 1 && !strcmp(argv[0], \"-\")) {\n-\t\targv[0] = \"@{-1}\";\n \t}\n \n \tif (!argc)\ndiff --git a/t/t3035-merge-hyphen-shorthand.sh b/t/t3035-merge-hyphen-shorthand.sh\nnew file mode 100755\nindex 0000000..fd37ff9\n--- /dev/null\n+++ b/t/t3035-merge-hyphen-shorthand.sh\n@@ -0,0 +1,33 @@\n+#!/bin/sh\n+\n+test_description='merge uses the 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 'setup branches' '\n+        git checkout master &&\n+        git checkout -b testing-2 &&\n+        git checkout -b testing-1 &&\n+        test_commit eigth &&\n+        test_commit ninth\n+'\n+\n+test_expect_success 'merge - should work' '\n+        git checkout testing-2 &&\n+        git merge - &&\n+        git rev-parse HEAD HEAD^^ | sort >actual &&\n+        git rev-parse master testing-1 | sort >expect &&\n+        test_cmp expect actual\n+'\n+\n+test_done\n-- \n2.1.4\n\n"},{"id":"312617","messageId":"1488007487-12965-5-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:24:45Z","receivedAt":"2017-02-25T07:25:32Z","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\nSuffixes like \"-@{yesterday}\" and \"-@{2.days.ago}\" are not enabled by this\npatch. This is something that needs to be fixed later by making changes deeper\ndown the callchain.\n\nSigned-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>\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..8be2de1\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+\t# git log -@{yesterday} >expect.4 &&\n+\t# git 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+\t# test_cmp expect.4 actual.4\n+'\n+\n+test_done\n-- \n2.1.4\n\n"},{"id":"312618","messageId":"1488007487-12965-3-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 2/6 v5] revision.c: swap if/else blocks","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:24:43Z","receivedAt":"2017-02-25T07:31:14Z","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 <kannan.siddharth12@gmail.com>\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":"312619","messageId":"1488007487-12965-2-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 1/6 v5] revision.c: do not update argv with unknown option","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:24:42Z","receivedAt":"2017-02-25T07:32:23Z","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 <kannan.siddharth12@gmail.com>\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":"312620","messageId":"1488007921-13432-1-git-send-email-kannan.siddharth12@gmail.com","threadId":"45219","inReplyTo":"1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 1/6 v5] revision.c: do not update argv with unknown option","fromName":"Siddharth Kannan","fromEmail":"kannan.siddharth12@gmail.com","sentAt":"2017-02-25T07:32:01Z","receivedAt":"2017-02-25T07:32:24Z","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 <kannan.siddharth12@gmail.com>\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":"312991","messageId":"xmqqpoi0eho8.fsf@gitster.mtv.corp.google.com","threadId":"45219","inReplyTo":"1488007487-12965-6-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH 5/6 v5] merge.c: delegate handling of \"-\" shorthand to revision.c:get_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T22:49:43Z","receivedAt":"2017-03-01T22:51:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n\n> The callchain for handling each argument contains the function\n> revision.c:get_sha1 where the shorthand for \"-\" ~ \"@{-1}\" has already been\n> implemented in a previous patch; the complete callchain leading to that\n> function is:\n>\n> 1. merge.c:collect_parents\n> 2. commit.c:get_merge_parent : this function calls revision.c:get_sha1\n>\n> This patch also adds a test for checking that the shorthand works properly\n\nThis breaks \"git merge\".\n\n> +test_expect_success 'merge - should work' '\n> +        git checkout testing-2 &&\n> +        git merge - &&\n> +        git rev-parse HEAD HEAD^^ | sort >actual &&\n> +        git rev-parse master testing-1 | sort >expect &&\n> +        test_cmp expect actual\n\nThis test is not sufficient to catch a regression I seem to be\nseeing.\n\n\t$ git checkout side\n\t$ git checkout pu\n\t$ git merge -\n\nused to say \"Merge branch 'side' into pu\".  With this series merged,\nI seem to be getting \"Merge commit '-' into pu\".\n"},{"id":"312999","messageId":"xmqqinnsegxb.fsf@gitster.mtv.corp.google.com","threadId":"45219","inReplyTo":"xmqqpoi0eho8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6 v5] merge.c: delegate handling of \"-\" shorthand to revision.c:get_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T23:05:52Z","receivedAt":"2017-03-01T23:07:06Z","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> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n>\n>> The callchain for handling each argument contains the function\n>> revision.c:get_sha1 where the shorthand for \"-\" ~ \"@{-1}\" has already been\n>> implemented in a previous patch; the complete callchain leading to that\n>> function is:\n>>\n>> 1. merge.c:collect_parents\n>> 2. commit.c:get_merge_parent : this function calls revision.c:get_sha1\n>>\n>> This patch also adds a test for checking that the shorthand works properly\n>\n> This breaks \"git merge\".\n>\n>> +test_expect_success 'merge - should work' '\n>> +        git checkout testing-2 &&\n>> +        git merge - &&\n>> +        git rev-parse HEAD HEAD^^ | sort >actual &&\n>> +        git rev-parse master testing-1 | sort >expect &&\n>> +        test_cmp expect actual\n>\n> This test is not sufficient to catch a regression I seem to be\n> seeing.\n>\n> \t$ git checkout side\n> \t$ git checkout pu\n> \t$ git merge -\n>\n> used to say \"Merge branch 'side' into pu\".  With this series merged,\n> I seem to be getting \"Merge commit '-' into pu\".\n\nYou stopped at get_sha1_1() in your 3817cebabc (\"sha1_name.c: teach\nget_sha1_1 \"-\" shorthand for \"@{-1}\"\", 2017-02-25), instead of going\ndown to get_sha1_basic() and teaching it that \"-\" means the same\nthing as \"@{-1}\", which would in turn require you to teach\ndwim_ref() that \"-\" is the same thing as \"@{-1}\".  As dwim_ref()\ndoes not know about \"-\" and does not expand it to the refname like\nit expands \"@{-1}\", it would break and that is why 3817cebabc punts\nat a bit higher in the callchain.\n\nThe breakage by this patch to \"git merge\" happens for the same\nreason.  cmd_merge() calls collect_parents() which annotates the\ncommits that are merged with their textual name, which used to be\n\"@{-1}\" without this patch but now \"-\" is passed as-is.  This\nannotation will be given to merge_name(), and the first thing it\ndoes is dwim_ref().  The function knows what to do with \"@{-1}\",\nbut it does not know what to do with \"-\", and that is why you end up\nproducing \"Merge commit '-' into ...\".\n\nDropping this patch from the series would make things consistent\nwith what was done in 3817cebabc and I think that is a sensible\nplace to stop.  After the dust settles, We _can_ later dig further\nby teaching dwim_ref() and friends what \"-\" means, and when it is\ndone, this patch would become useful.\n\nThanks.\n\n\n\n\n\n"},{"id":"313006","messageId":"xmqqa894egbj.fsf@gitster.mtv.corp.google.com","threadId":"45219","inReplyTo":"1488007487-12965-7-git-send-email-kannan.siddharth12@gmail.com","subject":"Re: [PATCH 6/6 v5] revert.c: delegate handling of \"-\" shorthand to setup_revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T23:18:56Z","receivedAt":"2017-03-01T23:25:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Kannan <kannan.siddharth12@gmail.com> writes:\n\n> revert.c:run_sequencer calls setup_revisions right after replacing \"-\" with\n> \"@{-1}\" for this shorthand. A previous patch taught setup_revisions to handle\n> this shorthand by doing the required replacement inside revision.c:get_sha1_1.\n>\n> Hence, the code here is redundant and has been removed.\n>\n> This patch also adds a test to check that revert recognizes the \"-\" shorthand.\n\nUnlike \"merge\" [*1*], I think this one is OK because \"git revert\n$commit\" does not try to say _how_ the commit was given, and most\nimportantly, it does not say what branch the reverted thing was.\n\nThanks.\n\n[Footnote]\n\n*1* Probably \"checkout\" would exhibit the same issue as we saw in\n    5/6 for \"git merge\" if you remove the \"- to @{-1}\" conversion\n    from it.\n\n\n"},{"id":"314022","messageId":"1cnbut-0000Vd-Rz@crossperf.com","threadId":"45219","inReplyTo":"1488007487-12965-5-git-send-email-kannan.siddharth12@gmail.com","subject":"[PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily","fromName":"mash","fromEmail":"mash+git@crossperf.com","sentAt":"2017-03-14T02:10:24Z","receivedAt":"2017-03-14T02:10:46Z","isPatch":true,"sender":{"key":"mash+git@crossperf.com","avatar":null},"body":"Move dash is previous branch check to get_sha1_basic.\nIntroduce helper function that gets nth prior branch switch from reflog.\n\nSigned-off-by: mash <mash+git@crossperf.com>\n---\nRE: [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 \"-\" shorthand for \"@{-1}\"\n> +\tif (*name == '-' && len == 1) {\n> +\t\tname = \"@{-1}\";\n> +\t\tlen = 5;\n> +\t}\n\nWe could avoid parsing @{-1} unnecessarily with something like this patch.\n\nForgive me I don't understand how the patch numbering works just yet. This is\n6/6 because format-patch made it 6/6 with however I got the patches applied on\nmy end. This should apply cleanly on pu anyways.\n\nThanks to Stefan since he suggested that I might want to review this.\n\nmash\n\n sha1_name.c | 39 ++++++++++++++++++++++++---------------\n 1 file changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 2f86bc9..363bbe7 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -568,6 +568,7 @@ static inline int push_mark(const char *string, int len)\n }\n \n static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);\n+static int get_branch_switch(int nth, struct strbuf *buf);\n static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1,\n@@ -628,11 +629,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,\n \tif (len && ambiguous_path(str, len))\n \t\treturn -1;\n \n-\tif (nth_prior) {\n+\tif (nth_prior || !strcmp(str, \"-\")) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint detached;\n \n-\t\tif (interpret_nth_prior_checkout(str, len, &buf) > 0) {\n+\t\tif (nth_prior ? interpret_nth_prior_checkout(str, len, &buf) > 0\n+\t\t\t      : get_branch_switch(1, &buf) > 0) {\n \t\t\tdetached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));\n \t\t\tstrbuf_release(&buf);\n \t\t\tif (detached)\n@@ -1078,6 +1080,25 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n \treturn 0;\n }\n \n+static int get_branch_switch(int nth, struct strbuf *buf)\n+{\n+\tint retval;\n+\tstruct grab_nth_branch_switch_cbdata cb;\n+\n+\tcb.remaining = nth;\n+\tstrbuf_init(&cb.buf, 20);\n+\n+\tretval = for_each_reflog_ent_reverse(\"HEAD\", grab_nth_branch_switch,\n+\t\t\t\t\t     &cb);\n+\tif (0 < retval) {\n+\t\tstrbuf_reset(buf);\n+\t\tstrbuf_addbuf(buf, &cb.buf);\n+\t}\n+\n+\tstrbuf_release(&cb.buf);\n+\treturn retval;\n+}\n+\n /*\n  * Parse @{-N} syntax, return the number of characters parsed\n  * if successful; otherwise signal an error with negative value.\n@@ -1086,8 +1107,6 @@ 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-\tstruct grab_nth_branch_switch_cbdata cb;\n \tconst char *brace;\n \tchar *num_end;\n \n@@ -1103,18 +1122,8 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,\n \t\treturn -1;\n \tif (nth <= 0)\n \t\treturn -1;\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}\n-\n-\tstrbuf_release(&cb.buf);\n-\treturn retval;\n+\treturn 0 < get_branch_switch(nth, buf) ? brace - name + 1 : 0;\n }\n \n int get_oid_mb(const char *name, struct object_id *oid)\n-- \n2.9.3\n"}]}