{"thread":{"id":"38796","subject":"[PATCH v5 1/2] reset: enable '-' short-hand for previous branch","startedAt":"2015-03-13T18:18:35Z","lastAt":"2015-03-16T11:40:26Z","messageCount":6,"participants":["Sudhanshu Shekhar","Eric Sunshine"],"isPatch":true,"patchVersion":5,"patchTotal":2},"messages":[{"id":"257645","messageId":"1426270716-22405-1-git-send-email-sudshekhar02@gmail.com","threadId":"38796","inReplyTo":null,"subject":"[PATCH v5 1/2] reset: enable '-' short-hand for previous branch","fromName":"Sudhanshu Shekhar","fromEmail":"sudshekhar02@gmail.com","sentAt":"2015-03-13T18:18:35Z","receivedAt":"2015-03-13T18:18:35Z","isPatch":true,"sender":{"key":"sudshekhar02@gmail.com","avatar":null},"body":"git reset '-' will reset to the previous branch. To reset a file named\n\"-\" use either \"git reset ./-\" or \"git reset -- -\".\n\nChange error message to treat single \"-\" as an ambigous revision or\npath rather than a bad flag.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nHelped-by: Matthieu Moy \nSigned-off-by: Sudhanshu Shekhar \n---\nI have changed the logic to ensure argv[0] isn't changed at any point.\nCreating a modified_argv0 variable let's me do that.\n\nI couldn't think of any other way to achieve this, apart from changing things\ndirectly in the sha1_name.c file (like Junio's changes). Please let me know\nif some further changes are needed.\n\n builtin/reset.c | 17 +++++++++++++----\n setup.c | 2 +-\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4c08ddc..bc50e14 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec,\n {\n \tconst char *rev = \"HEAD\";\n \tunsigned char unused[20];\n+\tconst char *modified_argv0 = argv[0];\n \t/*\n \t * Possible arguments are:\n \t *\n@@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,\n \t */\n \n \tif (argv[0]) {\n+\t\tif (!strcmp(argv[0], \"-\")) {\n+\t\t\tmodified_argv0 = \"@{-1}\";\n+\t\t}\n+\t\telse {\n+\t\t\tmodified_argv0 = argv[0];\n+\t\t}\n+\n \t\tif (!strcmp(argv[0], \"--\")) {\n \t\t\targv++; /* reset to HEAD, possibly with paths */\n \t\t} else if (argv[1] && !strcmp(argv[1], \"--\")) {\n-\t\t\trev = argv[0];\n+\t\t\trev = modified_argv0;\n \t\t\targv += 2;\n \t\t}\n \t\t/*\n@@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,\n \t\t * has to be unambiguous. If there is a single argument, it\n \t\t * can not be a tree\n \t\t */\n-\t\telse if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||\n-\t\t\t (argv[1] && !get_sha1_treeish(argv[0], unused))) {\n+\t\telse if ((!argv[1] && !get_sha1_committish(modified_argv0, unused)) ||\n+\t\t\t (argv[1] && !get_sha1_treeish(modified_argv0, unused))) {\n \t\t\t/*\n \t\t\t * Ok, argv[0] looks like a commit/tree; it should not\n \t\t\t * be a filename.\n \t\t\t */\n \t\t\tverify_non_filename(prefix, argv[0]);\n-\t\t\trev = *argv++;\n+\t\t\trev = modified_argv0;\n+\t\t\targv++;\n \t\t} else {\n \t\t\t/* Otherwise we treat this as a filename */\n \t\t\tverify_filename(prefix, argv[0], 1);\ndiff --git a/setup.c b/setup.c\nindex 979b13f..b621b62 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -200,7 +200,7 @@ void verify_filename(const char *prefix,\n \t\t int diagnose_misspelt_rev)\n {\n \tif (*arg == '-')\n-\t\tdie(\"bad flag '%s' used after filename\", arg);\n+\t\tdie(\"ambiguous argument '%s': unknown revision or path\", arg);\n \tif (check_filename(prefix, arg))\n \t\treturn;\n \tdie_verify_filename(prefix, arg, diagnose_misspelt_rev);\n-- \n2.3.1.277.gd67f9d5.dirty\n"},{"id":"257646","messageId":"1426270716-22405-2-git-send-email-sudshekhar02@gmail.com","threadId":"38796","inReplyTo":"1426270716-22405-1-git-send-email-sudshekhar02@gmail.com","subject":"[PATCH v5 2/2] t7102: add 'reset -' tests","fromName":"Sudhanshu Shekhar","fromEmail":"sudshekhar02@gmail.com","sentAt":"2015-03-13T18:18:36Z","receivedAt":"2015-03-13T18:18:36Z","isPatch":true,"sender":{"key":"sudshekhar02@gmail.com","avatar":null},"body":"Add following test cases:\n1) Confirm error message when git reset is used with no previous branch\n2) Confirm git reset - works like git reset @{-1}\n3) Confirm \"-\" is always treated as a commit unless the -- file option\nis specified\n4) Confirm \"git reset -\" works normally even when a file named @{-1} is\npresent\n\nHelped-by: Eric Sunshine \nHelped-by: Matthieu Moy \nHelped-by: David Aguilar \nSigned-off-by: Sudhanshu Shekhar \n---\nEric: Thank you for pointing out the mistake. The '&&' after the Here Docs\nwas causing the issue. I have removed the concatenation from there, hope\nthat's okay.\nRegarding the @{-1} test case, I created it as a check for Junio's comment\non the error message generated by \"git reset -\" when a file named @{-1} is there.\nSince, in this situation \"git reset @{-1}\" will return an error (but \"reset -\"\nshouldn't).\nI have renamed the folder to 'dash' as suggested by you, keeping the old name only\nwhere it made sense.\n t/t7102-reset.sh | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 158 insertions(+)\n\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 98bcfe2..18523c1 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -568,4 +568,162 @@ test_expect_success 'reset --mixed sets up work tree' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reset - with no previous branch fails' '\n+\tgit init no_previous &&\n+\ttest_when_finished rm -rf no_previous &&\n+\t(\n+\t\tcd no_previous &&\n+\t\ttest_must_fail git reset - 2>actual\n+\t) &&\n+\ttest_i18ngrep \"ambiguous argument\" no_previous/actual\n+'\n+\n+test_expect_success 'reset - while having file named - and no previous branch fails' '\n+\tgit init no_previous &&\n+\ttest_when_finished rm -rf no_previous &&\n+\t(\n+\t\tcd no_previous &&\n+\t\t>- &&\n+\t\ttest_must_fail git reset - 2>actual\n+\t) &&\n+\ttest_i18ngrep \"ambiguous argument\" no_previous/actual\n+'\n+\n+\n+test_expect_success \\\n+\t'reset - in the presence of file named - with previous branch resets commit' '\n+\tcat >expect <<-EOF\n+\tUnstaged changes after reset:\n+\tM\t-\n+\tM\tfile\n+\tEOF\n+\tgit init dash &&\n+\ttest_when_finished rm -rf dash &&\n+\t(\n+\t\tcd dash &&\n+\t\t>- &&\n+\t\t>file &&\n+\t\tgit add file - &&\n+\t\tgit commit -m \"add base files\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\techo \"random\" >- &&\n+\t\techo \"wow\" >file &&\n+\t\tgit add file - &&\n+\t\tgit reset - >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success \\\n+\t'reset - in the presence of file named - with -- option resets commit' '\n+\tcat >expect <<-EOF\n+\tUnstaged changes after reset:\n+\tM\t-\n+\tM\tfile\n+\tEOF\n+\tgit init dash &&\n+\ttest_when_finished rm -rf dash &&\n+\t(\n+\t\tcd dash &&\n+\t\t>- &&\n+\t\t>file &&\n+\t\tgit add file - &&\n+\t\tgit commit -m \"add base files\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\techo \"random\" >- &&\n+\t\techo \"wow\" >file &&\n+\t\tgit add file - &&\n+\t\tgit reset - -- >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reset - in the presence of file named - with -- file option resets file' '\n+\tcat >expect <<-EOF\n+\tUnstaged changes after reset:\n+\tM\t-\n+\tEOF\n+\tgit init dash &&\n+\ttest_when_finished rm -rf dash &&\n+\t(\n+\t\tcd dash &&\n+\t\t>- &&\n+\t\t>file &&\n+\t\tgit add file - &&\n+\t\tgit commit -m \"add base files\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\techo \"random\" >- &&\n+\t\techo \"wow\" >file &&\n+\t\tgit add file - &&\n+\t\tgit reset -- - >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success \\\n+\t'reset - in the presence of file named - with both pre and post -- option resets file' '\n+\tcat >expect <<-EOF\n+\tUnstaged changes after reset:\n+\tM\t-\n+\tEOF\n+\tgit init dash &&\n+\ttest_when_finished rm -rf dash &&\n+\t(\n+\t\tcd dash &&\n+\t\t>- &&\n+\t\t>file &&\n+\t\tgit add file - &&\n+\t\tgit commit -m \"add base files\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\techo \"random\" >- &&\n+\t\techo \"wow\" >file &&\n+\t\tgit add file - &&\n+\t\tgit reset - -- - >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reset - works same as reset @{-1}' '\n+\tgit init dash &&\n+\ttest_when_finished rm -rf dash &&\n+\t(\n+\t\tcd dash &&\n+\t\techo \"file1\" >file1 &&\n+\t\tgit add file1 &&\n+\t\tgit commit -m \"base commit\" &&\n+\t\tgit checkout -b temp &&\n+\t\techo \"new file\" >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"added file\" &&\n+\t\tgit reset - &&\n+\t\tgit status --porcelain >../actual &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"added file\" &&\n+\t\tgit reset @{-1} &&\n+\t\tgit status --porcelain >../expect\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reset - with file named @{-1} succeeds' '\n+\tcat >expect <<-EOF\n+\tUnstaged changes after reset:\n+\tM\t@{-1}\n+\tM\tfile\n+\tEOF\n+\tgit init dash &&\n+\ttest_when_finished rm -rf dash &&\n+\t(\n+\t\tcd dash &&\n+\t\techo \"random\" >@{-1} &&\n+\t\techo \"random\" >file &&\n+\t\tgit add @{-1} file &&\n+\t\tgit commit -m \"base commit\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\techo \"additional stuff\" >>file &&\n+\t\techo \"additional stuff\" >>@{-1} &&\n+\t\tgit add file @{-1} &&\n+\t\tgit reset - >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.3.1.277.gd67f9d5.dirty\n"},{"id":"257648","messageId":"CAPig+cTuCtS6EKr9AqGkH_dDkxdYtk9ZfkpCLDLY9XYQO04ANA@mail.gmail.com","threadId":"38796","inReplyTo":"1426270716-22405-1-git-send-email-sudshekhar02@gmail.com","subject":"Re: [PATCH v5 1/2] reset: enable '-' short-hand for previous branch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-13T20:48:10Z","receivedAt":"2015-03-13T20:48:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar\n wrote:\n> git reset '-' will reset to the previous branch. To reset a file named\n> \"-\" use either \"git reset ./-\" or \"git reset -- -\".\n>\n> Change error message to treat single \"-\" as an ambigous revision or\n> path rather than a bad flag.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunshine \n> Helped-by: Matthieu Moy \n> Signed-off-by: Sudhanshu Shekhar \n> ---\n> I have changed the logic to ensure argv[0] isn't changed at any point.\n> Creating a modified_argv0 variable let's me do that.\n>\n> I couldn't think of any other way to achieve this, apart from changing things\n> directly in the sha1_name.c file (like Junio's changes). Please let me know\n> if some further changes are needed.\n>\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 4c08ddc..bc50e14 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec,\n> {\n> const char *rev = \"HEAD\";\n> unsigned char unused[20];\n> + const char *modified_argv0 = argv[0];\n\nThis variable is only needed inside the 'if (argv[0])' block below, so\nits declaration should be moved there. Also the initialization to\nargv[0] is wasted since the assignment is overwritten below.\n\nThe variable name itself could be better. Unlike a name such as\n'orig_arg0', \"modified\" doesn't tell us much. Modified how? Modified\nto be what? Consideration where and how the variable is used, we can\nsee that it will be holding a value that _might_ be a \"rev\". This\nsuggests a better name such as 'maybe_rev' or something similar.\n\n> /*\n> * Possible arguments are:\n> *\n> @@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,\n> */\n>\n> if (argv[0]) {\n> + if (!strcmp(argv[0], \"-\")) {\n> + modified_argv0 = \"@{-1}\";\n> + }\n> + else {\n\nStyle: cuddle the 'else' and braces: \"} else {\"\n\n> + modified_argv0 = argv[0];\n> + }\n\nThe unnecessary braces make this harder to read than it could be since\nit is so spread out vertically. Dropping the braces would help. The\nternary operator ?: might improve readability (though it might also\nmake it worse).\n\n> if (!strcmp(argv[0], \"--\")) {\n> argv++; /* reset to HEAD, possibly with paths */\n> } else if (argv[1] && !strcmp(argv[1], \"--\")) {\n> - rev = argv[0];\n> + rev = modified_argv0;\n> argv += 2;\n> }\n> /*\n> @@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,\n> * has to be unambiguous. If there is a single argument, it\n> * can not be a tree\n> */\n> - else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||\n> - (argv[1] && !get_sha1_treeish(argv[0], unused))) {\n> + else if ((!argv[1] && !get_sha1_committish(modified_argv0, unused)) ||\n> + (argv[1] && !get_sha1_treeish(modified_argv0, unused))) {\n> /*\n> * Ok, argv[0] looks like a commit/tree; it should not\n> * be a filename.\n> */\n> verify_non_filename(prefix, argv[0]);\n> - rev = *argv++;\n> + rev = modified_argv0;\n> + argv++;\n\nGood. This is much better than the previous rounds, and is the sort of\nsolution I had hoped to see when prodding you in previous reviews to\navoid argv[] kludges. Unlike the previous band-aid approach, this\ndemonstrates that you took the time to understand the overall logic\nflow rather than merely plopping in a \"quick fix\".\n\n> } else {\n> /* Otherwise we treat this as a filename */\n> verify_filename(prefix, argv[0], 1);\n> diff --git a/setup.c b/setup.c\n> index 979b13f..b621b62 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -200,7 +200,7 @@ void verify_filename(const char *prefix,\n> int diagnose_misspelt_rev)\n> {\n> if (*arg == '-')\n> - die(\"bad flag '%s' used after filename\", arg);\n> + die(\"ambiguous argument '%s': unknown revision or path\", arg);\n\nThe conditional is only checking if the first character of 'arg' is\nhyphen; it's not checking if 'arg' is exactly \"-\". It's purpose is to\nrecognize -flags or --flags, so it's inappropriate to change the error\nmessage like this. I think this also doesn't help the case when there\nreally is a file named \"-\", since this conditional will just claim\nthat it's ambiguous.\n\nIt might or might not be appropriate to add a special case here to\nallow an exact \"-\" to fall through to the check_filename() call below,\nhowever, it would be necessary to thoroughly check for possible\nrepercussions first. (I haven't checked.) If all else fails, you could\nchange parse_args() to do something a bit ugly like this:\n\n const char *f = strcmp(argv[0], \"-\") ? argv[0] : \"./-\";\n verify_filename(prefix, f);\n\n> if (check_filename(prefix, arg))\n> return;\n> die_verify_filename(prefix, arg, diagnose_misspelt_rev);\n> --\n\nBy now, you've had a taste of what it's like to participate in the Git\nproject and what will be expected of you, and GSoC mentors have\n(hopefully) formed an opinion of your abilities and how you interact\nwith reviewers, so I'm not sure that it makes sense for you to\nresubmit again. Junio's proposal[1] to generalize \"-\" recognition as\nan alias for @{-1} may be worth pursing by someone, but may be too\nlarge for a micro-project.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/265260\n"},{"id":"257649","messageId":"CAPig+cRQ9ZZ9573SvA9HPNLtE6s0U6TGnDfgpRD-osReAQgJJA@mail.gmail.com","threadId":"38796","inReplyTo":"1426270716-22405-2-git-send-email-sudshekhar02@gmail.com","subject":"Re: [PATCH v5 2/2] t7102: add 'reset -' tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-13T21:10:43Z","receivedAt":"2015-03-13T21:10:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar\n wrote:\n> Add following test cases:\n> 1) Confirm error message when git reset is used with no previous branch\n> 2) Confirm git reset - works like git reset @{-1}\n> 3) Confirm \"-\" is always treated as a commit unless the -- file option\n> is specified\n> 4) Confirm \"git reset -\" works normally even when a file named @{-1} is\n> present\n>\n> Helped-by: Eric Sunshine \n> Helped-by: Matthieu Moy \n> Helped-by: David Aguilar \n> Signed-off-by: Sudhanshu Shekhar \n> ---\n> Eric: Thank you for pointing out the mistake. The '&&' after the Here\n> Docs was causing the issue. I have removed the concatenation from\n> there, hope that's okay.\n\nThe && needs to go on the first line, not the last line of the here-doc.\n\nHowever, that was not my main concern in the previous review. What\ndisturbed me was that the new tests, which were supposed to be\nchecking if \"-\" behaved as @{-1}, were succeeding even without patch\n1/2 applied which implemented the \"-\" alias for @{-1}. That seems\nwrong. I don't think you particularly addressed that issue in this\nversion (even though the first couple tests will now fail without 1/2\ndue to the changed error message).\n\n> Regarding the @{-1} test case, I created it as a check for Junio's\n> comment on the error message generated by \"git reset -\" when a file\n> named @{-1} is there. Since, in this situation \"git reset @{-1}\" will\n> return an error (but \"reset -\" shouldn't).\n\nReminder: Wrap commentary to about column 72, as you would the commit\nmessage. (I re-wrapped it manually to reply to it.)\n\n> I have renamed the folder to 'dash' as suggested by you, keeping the\n> old name only where it made sense.\n\nConsidering that the test titles already tell us the intent of the\ntests, I don't find that the directory name \"no_previous\" adds much\nvalue to tests checking the behavior of \"-\" with no previous branch. A\nsingle, consistent name used throughout all these tests would be less\nsurprising and place smaller cognitive load on the reader.\n\nMore below.\n\n> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> index 98bcfe2..18523c1 100755\n> --- a/t/t7102-reset.sh\n> +++ b/t/t7102-reset.sh\n> @@ -568,4 +568,162 @@ test_expect_success 'reset --mixed sets up work tree' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'reset - with no previous branch fails' '\n> + git init no_previous &&\n> + test_when_finished rm -rf no_previous &&\n> + (\n> + cd no_previous &&\n> + test_must_fail git reset - 2>actual\n> + ) &&\n> + test_i18ngrep \"ambiguous argument\" no_previous/actual\n> +'\n> +\n> +test_expect_success 'reset - while having file named - and no previous branch fails' '\n> + git init no_previous &&\n> + test_when_finished rm -rf no_previous &&\n> + (\n> + cd no_previous &&\n> + >- &&\n> + test_must_fail git reset - 2>actual\n> + ) &&\n> + test_i18ngrep \"ambiguous argument\" no_previous/actual\n> +'\n> +\n> +\n\nStyle: Unnecessary extra blank line.\n\n> +test_expect_success \\\n> + 'reset - in the presence of file named - with previous branch resets commit' '\n> + cat >expect <<-EOF\n\nPlace the && at the end of this line. Also, prefix EOF with a\nbackslash to indicate that you don't intend any interpolation to occur\nwithin the here-doc. So:\n\n cat >expect <<-\\EOF &&\n\nDitto for the remaining tests.\n\n> + Unstaged changes after reset:\n> + M -\n> + M file\n> + EOF\n> + git init dash &&\n> + test_when_finished rm -rf dash &&\n> + (\n> + cd dash &&\n> + >- &&\n> + >file &&\n> + git add file - &&\n> + git commit -m \"add base files\" &&\n> + git checkout -b new_branch &&\n> + echo \"random\" >- &&\n> + echo \"wow\" >file &&\n> + git add file - &&\n> + git reset - >../actual\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +test_expect_success \\\n> + 'reset - in the presence of file named - with -- option resets commit' '\n> + cat >expect <<-EOF\n> + Unstaged changes after reset:\n> + M -\n> + M file\n> + EOF\n> + git init dash &&\n> + test_when_finished rm -rf dash &&\n> + (\n> + cd dash &&\n> + >- &&\n> + >file &&\n> + git add file - &&\n> + git commit -m \"add base files\" &&\n> + git checkout -b new_branch &&\n> + echo \"random\" >- &&\n> + echo \"wow\" >file &&\n> + git add file - &&\n> + git reset - -- >../actual\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'reset - in the presence of file named - with -- file option resets file' '\n> + cat >expect <<-EOF\n> + Unstaged changes after reset:\n> + M -\n> + EOF\n> + git init dash &&\n> + test_when_finished rm -rf dash &&\n> + (\n> + cd dash &&\n> + >- &&\n> + >file &&\n> + git add file - &&\n> + git commit -m \"add base files\" &&\n> + git checkout -b new_branch &&\n> + echo \"random\" >- &&\n> + echo \"wow\" >file &&\n> + git add file - &&\n> + git reset -- - >../actual\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +test_expect_success \\\n> + 'reset - in the presence of file named - with both pre and post -- option resets file' '\n> + cat >expect <<-EOF\n> + Unstaged changes after reset:\n> + M -\n> + EOF\n> + git init dash &&\n> + test_when_finished rm -rf dash &&\n> + (\n> + cd dash &&\n> + >- &&\n> + >file &&\n> + git add file - &&\n> + git commit -m \"add base files\" &&\n> + git checkout -b new_branch &&\n> + echo \"random\" >- &&\n> + echo \"wow\" >file &&\n> + git add file - &&\n> + git reset - -- - >../actual\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'reset - works same as reset @{-1}' '\n> + git init dash &&\n> + test_when_finished rm -rf dash &&\n> + (\n> + cd dash &&\n> + echo \"file1\" >file1 &&\n> + git add file1 &&\n> + git commit -m \"base commit\" &&\n> + git checkout -b temp &&\n> + echo \"new file\" >file &&\n> + git add file &&\n> + git commit -m \"added file\" &&\n> + git reset - &&\n> + git status --porcelain >../actual &&\n> + git add file &&\n> + git commit -m \"added file\" &&\n> + git reset @{-1} &&\n> + git status --porcelain >../expect\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'reset - with file named @{-1} succeeds' '\n> + cat >expect <<-EOF\n> + Unstaged changes after reset:\n> + M @{-1}\n> + M file\n> + EOF\n> + git init dash &&\n> + test_when_finished rm -rf dash &&\n> + (\n> + cd dash &&\n> + echo \"random\" >@{-1} &&\n> + echo \"random\" >file &&\n> + git add @{-1} file &&\n> + git commit -m \"base commit\" &&\n> + git checkout -b new_branch &&\n> + echo \"additional stuff\" >>file &&\n> + echo \"additional stuff\" >>@{-1} &&\n> + git add file @{-1} &&\n> + git reset - >../actual\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.3.1.277.gd67f9d5.dirty\n"},{"id":"257771","messageId":"CAODo60q-jNEyOncEdtCHqR_xgBY1Ju-vx_Ywj=YxbYWYVJ4rHQ@mail.gmail.com","threadId":"38796","inReplyTo":"CAPig+cRQ9ZZ9573SvA9HPNLtE6s0U6TGnDfgpRD-osReAQgJJA@mail.gmail.com","subject":"Re: [PATCH v5 2/2] t7102: add 'reset -' tests","fromName":"Sudhanshu Shekhar","fromEmail":"sudshekhar02@gmail.com","sentAt":"2015-03-16T07:14:51Z","receivedAt":"2015-03-16T07:14:51Z","isPatch":true,"sender":{"key":"sudshekhar02@gmail.com","avatar":null},"body":"Hi,\nOn Sat, Mar 14, 2015 at 2:40 AM, Eric Sunshine wrote:\n> On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar\n> wrote:\n>> Add following test cases:\n>> 1) Confirm error message when git reset is used with no previous branch\n>> 2) Confirm git reset - works like git reset @{-1}\n>> 3) Confirm \"-\" is always treated as a commit unless the -- file option\n>> is specified\n>> 4) Confirm \"git reset -\" works normally even when a file named @{-1} is\n>> present\n>>\n>> Helped-by: Eric Sunshine \n>> Helped-by: Matthieu Moy \n>> Helped-by: David Aguilar \n>> Signed-off-by: Sudhanshu Shekhar \n>> ---\n>> Eric: Thank you for pointing out the mistake. The '&&' after the Here\n>> Docs was causing the issue. I have removed the concatenation from\n>> there, hope that's okay.\n>\n> The && needs to go on the first line, not the last line of the here-doc.\n>\n> However, that was not my main concern in the previous review. What\n> disturbed me was that the new tests, which were supposed to be\n> checking if \"-\" behaved as @{-1}, were succeeding even without patch\n> 1/2 applied which implemented the \"-\" alias for @{-1}. That seems\n> wrong. I don't think you particularly addressed that issue in this\n> version (even though the first couple tests will now fail without 1/2\n> due to the changed error message).\n\nActually, The issue was caused due a HERE docs error. If you run this\npatch now, you will see that 7 out of the 8 test cases fail.\n\n>\n>> Regarding the @{-1} test case, I created it as a check for Junio's\n>> comment on the error message generated by \"git reset -\" when a file\n>> named @{-1} is there. Since, in this situation \"git reset @{-1}\" will\n>> return an error (but \"reset -\" shouldn't).\n>\n> Reminder: Wrap commentary to about column 72, as you would the commit\n> message. (I re-wrapped it manually to reply to it.)\n>\n>> I have renamed the folder to 'dash' as suggested by you, keeping the\n>> old name only where it made sense.\n>\n> Considering that the test titles already tell us the intent of the\n> tests, I don't find that the directory name \"no_previous\" adds much\n> value to tests checking the behavior of \"-\" with no previous branch. A\n> single, consistent name used throughout all these tests would be less\n> surprising and place smaller cognitive load on the reader.\n>\n> More below.\n>\n>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n>> index 98bcfe2..18523c1 100755\n>> --- a/t/t7102-reset.sh\n>> +++ b/t/t7102-reset.sh\n>> @@ -568,4 +568,162 @@ test_expect_success 'reset --mixed sets up work tree' '\n>> test_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'reset - with no previous branch fails' '\n>> + git init no_previous &&\n>> + test_when_finished rm -rf no_previous &&\n>> + (\n>> + cd no_previous &&\n>> + test_must_fail git reset - 2>actual\n>> + ) &&\n>> + test_i18ngrep \"ambiguous argument\" no_previous/actual\n>> +'\n>> +\n>> +test_expect_success 'reset - while having file named - and no previous branch fails' '\n>> + git init no_previous &&\n>> + test_when_finished rm -rf no_previous &&\n>> + (\n>> + cd no_previous &&\n>> + >- &&\n>> + test_must_fail git reset - 2>actual\n>> + ) &&\n>> + test_i18ngrep \"ambiguous argument\" no_previous/actual\n>> +'\n>> +\n>> +\n>\n> Style: Unnecessary extra blank line.\n>\n>> +test_expect_success \\\n>> + 'reset - in the presence of file named - with previous branch resets commit' '\n>> + cat >expect <<-EOF\n>\n> Place the && at the end of this line. Also, prefix EOF with a\n> backslash to indicate that you don't intend any interpolation to occur\n> within the here-doc. So:\n>\n> cat >expect <<-\\EOF &&\n>\n> Ditto for the remaining tests.\n>\nThank you for taking the time out to point out these style changes to\nme. There is a lot I have to learn about open source contribution yet\nand I believe during the course of this microproject, I did learn\nsomething about this (By making some very silly mistakes). Thank you\nto all the developers who took time out to review my code and point\nout the mistakes I had done.\n\nRegards,\nSudhanshu\n"},{"id":"257779","messageId":"CAODo60oP4AJ-Av_KaWhR9_OkY=OKEy_egjtQ8w+uvBpc5tWVVQ@mail.gmail.com","threadId":"38796","inReplyTo":"CAPig+cTuCtS6EKr9AqGkH_dDkxdYtk9ZfkpCLDLY9XYQO04ANA@mail.gmail.com","subject":"Re: [PATCH v5 1/2] reset: enable '-' short-hand for previous branch","fromName":"Sudhanshu Shekhar","fromEmail":"sudshekhar02@gmail.com","sentAt":"2015-03-16T11:40:26Z","receivedAt":"2015-03-16T11:40:26Z","isPatch":true,"sender":{"key":"sudshekhar02@gmail.com","avatar":null},"body":"Thank you all for your reviews and feedback. I will try and submit\nthis patch by taking my time to think about the various solutions and\ncoming up with the best one. I will also try and see if I can assist\nJunio with his JFF patch.\n\nI have learned a lot during the process of implementing this micro\nproject and believe that if I get selected for a gsoc under git, it\nwill help me become a better developer overall.\n\nI have gone over the ideas page and I am interested in the \"git bisect\n--first-parent\" and \"git bisect fixed/unfixed\" projects. I am looking\nthe source code at git-bisect.sh and will try and come up with a\nproposal for review by the git community.\n\nThank you all for you time and patience.\n\nRegards,\nSudhanshu\n\nOn Sat, Mar 14, 2015 at 2:18 AM, Eric Sunshine wrote:\n> On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar\n> wrote:\n>> git reset '-' will reset to the previous branch. To reset a file named\n>> \"-\" use either \"git reset ./-\" or \"git reset -- -\".\n>>\n>> Change error message to treat single \"-\" as an ambigous revision or\n>> path rather than a bad flag.\n>>\n>> Helped-by: Junio C Hamano \n>> Helped-by: Eric Sunshine \n>> Helped-by: Matthieu Moy \n>> Signed-off-by: Sudhanshu Shekhar \n>> ---\n>> I have changed the logic to ensure argv[0] isn't changed at any point.\n>> Creating a modified_argv0 variable let's me do that.\n>>\n>> I couldn't think of any other way to achieve this, apart from changing things\n>> directly in the sha1_name.c file (like Junio's changes). Please let me know\n>> if some further changes are needed.\n>>\n>> diff --git a/builtin/reset.c b/builtin/reset.c\n>> index 4c08ddc..bc50e14 100644\n>> --- a/builtin/reset.c\n>> +++ b/builtin/reset.c\n>> @@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec,\n>> {\n>> const char *rev = \"HEAD\";\n>> unsigned char unused[20];\n>> + const char *modified_argv0 = argv[0];\n>\n> This variable is only needed inside the 'if (argv[0])' block below, so\n> its declaration should be moved there. Also the initialization to\n> argv[0] is wasted since the assignment is overwritten below.\n>\n> The variable name itself could be better. Unlike a name such as\n> 'orig_arg0', \"modified\" doesn't tell us much. Modified how? Modified\n> to be what? Consideration where and how the variable is used, we can\n> see that it will be holding a value that _might_ be a \"rev\". This\n> suggests a better name such as 'maybe_rev' or something similar.\n>\n>> /*\n>> * Possible arguments are:\n>> *\n>> @@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,\n>> */\n>>\n>> if (argv[0]) {\n>> + if (!strcmp(argv[0], \"-\")) {\n>> + modified_argv0 = \"@{-1}\";\n>> + }\n>> + else {\n>\n> Style: cuddle the 'else' and braces: \"} else {\"\n>\n>> + modified_argv0 = argv[0];\n>> + }\n>\n> The unnecessary braces make this harder to read than it could be since\n> it is so spread out vertically. Dropping the braces would help. The\n> ternary operator ?: might improve readability (though it might also\n> make it worse).\n>\n>> if (!strcmp(argv[0], \"--\")) {\n>> argv++; /* reset to HEAD, possibly with paths */\n>> } else if (argv[1] && !strcmp(argv[1], \"--\")) {\n>> - rev = argv[0];\n>> + rev = modified_argv0;\n>> argv += 2;\n>> }\n>> /*\n>> @@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,\n>> * has to be unambiguous. If there is a single argument, it\n>> * can not be a tree\n>> */\n>> - else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||\n>> - (argv[1] && !get_sha1_treeish(argv[0], unused))) {\n>> + else if ((!argv[1] && !get_sha1_committish(modified_argv0, unused)) ||\n>> + (argv[1] && !get_sha1_treeish(modified_argv0, unused))) {\n>> /*\n>> * Ok, argv[0] looks like a commit/tree; it should not\n>> * be a filename.\n>> */\n>> verify_non_filename(prefix, argv[0]);\n>> - rev = *argv++;\n>> + rev = modified_argv0;\n>> + argv++;\n>\n> Good. This is much better than the previous rounds, and is the sort of\n> solution I had hoped to see when prodding you in previous reviews to\n> avoid argv[] kludges. Unlike the previous band-aid approach, this\n> demonstrates that you took the time to understand the overall logic\n> flow rather than merely plopping in a \"quick fix\".\n>\n>> } else {\n>> /* Otherwise we treat this as a filename */\n>> verify_filename(prefix, argv[0], 1);\n>> diff --git a/setup.c b/setup.c\n>> index 979b13f..b621b62 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -200,7 +200,7 @@ void verify_filename(const char *prefix,\n>> int diagnose_misspelt_rev)\n>> {\n>> if (*arg == '-')\n>> - die(\"bad flag '%s' used after filename\", arg);\n>> + die(\"ambiguous argument '%s': unknown revision or path\", arg);\n>\n> The conditional is only checking if the first character of 'arg' is\n> hyphen; it's not checking if 'arg' is exactly \"-\". It's purpose is to\n> recognize -flags or --flags, so it's inappropriate to change the error\n> message like this. I think this also doesn't help the case when there\n> really is a file named \"-\", since this conditional will just claim\n> that it's ambiguous.\n>\n> It might or might not be appropriate to add a special case here to\n> allow an exact \"-\" to fall through to the check_filename() call below,\n> however, it would be necessary to thoroughly check for possible\n> repercussions first. (I haven't checked.) If all else fails, you could\n> change parse_args() to do something a bit ugly like this:\n>\n> const char *f = strcmp(argv[0], \"-\") ? argv[0] : \"./-\";\n> verify_filename(prefix, f);\n>\n>> if (check_filename(prefix, arg))\n>> return;\n>> die_verify_filename(prefix, arg, diagnose_misspelt_rev);\n>> --\n>\n> By now, you've had a taste of what it's like to participate in the Git\n> project and what will be expected of you, and GSoC mentors have\n> (hopefully) formed an opinion of your abilities and how you interact\n> with reviewers, so I'm not sure that it makes sense for you to\n> resubmit again. Junio's proposal[1] to generalize \"-\" recognition as\n> an alias for @{-1} may be worth pursing by someone, but may be too\n> large for a micro-project.\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/265260\n"}]}