{"thread":{"id":"38774","subject":"[v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}'","startedAt":"2015-03-10T15:38:02Z","lastAt":"2015-03-10T17:35:38Z","messageCount":5,"participants":["Sundararajan R","Torsten Bögershausen","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"257474","messageId":"1426001883-6423-1-git-send-email-dyoucme@gmail.com","threadId":"38774","inReplyTo":null,"subject":"[v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}'","fromName":"Sundararajan R","fromEmail":"dyoucme@gmail.com","sentAt":"2015-03-10T15:38:02Z","receivedAt":"2015-03-10T15:38:02Z","isPatch":true,"sender":{"key":"dyoucme@gmail.com","avatar":null},"body":"Teaching reset the - shorthand involves checking if any file named '-' exists \nbecause it then becomes ambiguous as to whether the user wants to reset the\nfile '-' or if he wants to reset the working tree to the previous branch.\n\ncheck_filename() is used to perform this check. A similar ambiguity occurs \nwhen the file @{-1} exits. Therefore, when the files '-' or '@{-1}' exist \nthen the program dies with a message about the ambiguous argument.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Sundararajan R \n---\nHave made the modifications suggest by you, Eric.\nRemoved the part where the user is told that he can use ./- instead.\n\n builtin/reset.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4c08ddc..88ce0c5 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+\tint file_named_minus = 0;\n \t/*\n \t * Possible arguments are:\n \t *\n@@ -205,6 +206,12 @@ static void parse_args(struct pathspec *pathspec,\n \t */\n \n \tif (argv[0]) {\n+\t\tif (!strcmp(argv[0], \"-\") && !argv[1]) {\n+\t\t\tif (!check_filename(prefix, \"-\"))\n+\t\t\t\targv[0] = \"@{-1}\";\n+\t\t\telse\n+\t\t\t\tfile_named_minus = 1;\n+\t\t}\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@@ -226,7 +233,13 @@ static void parse_args(struct pathspec *pathspec,\n \t\t\trev = *argv++;\n \t\t} else {\n \t\t\t/* Otherwise we treat this as a filename */\n-\t\t\tverify_filename(prefix, argv[0], 1);\n+\t\t\tif (file_named_minus) {\n+\t\t\t\tdie(_(\"ambiguous argument '-': both revision and filename\\n\"\n+\t\t\t\t\t\"Use '--' to separate paths from revisions, like this:\\n\"\n+\t\t\t\t\t\"'git [...] -- [...]'\"));\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\tverify_filename(prefix, argv[0], 1);\n \t\t}\n \t}\n \t*rev_ret = rev;\n-- \n2.1.0\n"},{"id":"257475","messageId":"1426001883-6423-2-git-send-email-dyoucme@gmail.com","threadId":"38774","inReplyTo":"1426001883-6423-1-git-send-email-dyoucme@gmail.com","subject":"[v2 PATCH 2/2] reset: add tests for git reset -","fromName":"Sundararajan R","fromEmail":"dyoucme@gmail.com","sentAt":"2015-03-10T15:38:03Z","receivedAt":"2015-03-10T15:38:03Z","isPatch":true,"sender":{"key":"dyoucme@gmail.com","avatar":null},"body":"The failure case which occurs on teaching git is taught the '-' shorthand\nis when there exists no branch pointed to by '@{-1}'.\n\nThe ambiguous cases occur when there exist files named '-' or '@{-1}' in \nthe work tree. These are also treated as failure cases but here the user\nis given advice as to how he can proceed.\n\nAdd tests to check the handling of these cases. \nAlso add a test to verify that reset - behaves like reset @{-1} when none\nof the above cases are true.\n\nHelped-by: Junio C Hamano \nHelped-by: Torsten Bögershausen \nHelped-by: Eric Sunshine \nHelped-by: Matthieu Moy \nSigned-off-by: Sundararajan R \n---\nThank you for your feedback Torsten and Eric.\nI have now made the modifications suggested by you.\nI have also incorporated the suggestions given by Matthieu on the archive.\nPlease let me know if there is something else I should add.\n\n t/t7102-reset.sh | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 90 insertions(+)\n\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 98bcfe2..c05dab0 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -568,4 +568,94 @@ test_expect_success 'reset --mixed sets up work tree' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reset - with no @{-1} should fail' '\n+\tgit init new &&\n+\t(\n+\t\tcd new &&\n+\t\ttest_must_fail git reset - 2>actual\n+\t) &&\n+\ttest_i18ngrep \"unknown revision\" new/actual \n+\ttest_when_finished rm -rf new\n+'\n+\n+test_expect_success 'reset - with no @{-1} and file named - should fail' '\n+\tgit init new &&\n+\t(\n+\t\tcd new &&\n+\t\techo \"Hello\" >- &&\n+\t\tgit add - &&\n+\t\ttest_must_fail git reset - 2>actual \n+\t) &&\n+\ttest_i18ngrep \"both revision and filename\" new/actual \n+\ttest_when_finished rm -rf new\n+'\n+\n+test_expect_success 'reset - with @{-1} and file named @{-1} should fail' '\n+\tgit init new &&\n+\t(\n+\t\tcd new && \n+\t\techo \"Hello\" >@{-1} &&\n+\t\tgit add @{-1} &&\n+\t\tgit commit -m \"first_commit\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\t>@{-1} &&\n+\t\tgit add @{-1} &&\n+\t\ttest_must_fail git reset - 2>actual \n+\t) &&\n+\ttest_i18ngrep \"both revision and filename\" new/actual \n+\ttest_when_finished rm -rf new\n+'\n+\n+test_expect_success 'reset - with @{-1} and file named - should fail' '\n+\tgit init new &&\n+\t(\n+\t\tcd new && \n+\t\techo \"Hello\" >- &&\n+\t\tgit add - &&\n+\t\tgit commit -m \"first_commit\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\t>- &&\n+\t\tgit add - &&\n+\t\ttest_must_fail git reset - 2>actual \n+\t) &&\n+\ttest_i18ngrep \"both revision and filename\" new/actual \n+\ttest_when_finished rm -rf new\n+'\n+\n+test_expect_success 'reset - with @{-1} and file named @{-1} and - should fail' '\n+\tgit init new &&\n+\t(\n+\t\tcd new &&\n+\t\t>- &&\n+\t\tgit add - &&\n+\t\tgit commit -m \"first_commit\" &&\n+\t\tgit checkout -b new_branch\n+\t\t>@{-1} &&\n+\t\tgit add @{-1} &&\n+\t\ttest_must_fail git reset - 2>actual\n+\t) &&\n+ \ttest_i18ngrep \"both revision and filename\" new/actual \n+\ttest_when_finished rm -rf new\n+'\n+\n+test_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' '\n+\tgit init new &&\n+\t(\n+\t\tcd new &&\n+\t\techo \"Hey\" >new_file &&\n+\t\tgit add new_file &&\n+\t\tgit commit -m \"first_commit\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\t>new_file &&\n+\t\tgit add new_file &&\n+\t\tgit reset - &&\n+\t\tgit status -uno >file1 &&\n+\t\tgit add new_file &&\n+\t\tgit reset @{-1} &&\n+\t\tgit status -uno >file2 \n+\t) &&\n+\ttest_cmp new/file1 new/file2 \n+\ttest_when_finished rm -rf new\n+'\n+\n test_done\n-- \n2.1.0\n"},{"id":"257478","messageId":"54FF288D.3000304@web.de","threadId":"38774","inReplyTo":"1426001883-6423-2-git-send-email-dyoucme@gmail.com","subject":"Re: [v2 PATCH 2/2] reset: add tests for git reset -","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-03-10T17:23:25Z","receivedAt":"2015-03-10T17:23:25Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-03-10 16.38, Sundararajan R wrote:\n\n> Helped-by: Torsten Bögershausen \nThere seems to be an issue that the mail is encoded\nfrom (what ? Latin-1) into UTF-8 2 times\n\nThe easy solution is to remove the line,\nI'm OK with that, since a review-comment is not necessarily motivating\na Helped-by, at least not for me.\nMentioning it in the comments good and is enough.\n\n\nBut why is the mail \"encoded twice\" ? (this what the header says:)\n X-Mailer: git-send-email 2.1.0\n Content-Type: text/plain; charset=UTF-8\n\nCan somebody help out with a good explanation ?\n\nAnother (minor) thing:\nThere is nothing wrong with the test, but we can make it 3% more \"Git-style\" and\neasier too read when it is more similar to the rest of the code base:\n\ntest_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' '\n+\tgit init new &&\n+\t(\n+\t\tcd new &&\n+\t\techo \"Hey\" >new_file &&\n+\t\tgit add new_file &&\n+\t\tgit commit -m \"first_commit\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\t>new_file &&\n+\t\tgit add new_file &&\n+\t\tgit reset - &&\n+\t\tgit status -uno >file1 &&\n(Side-question: why \"status -uno\")\ntypically \"file\" (or \"file1\") is used for user files, not for the \"expected\" or \"actual\" output.\n\nThen we can compare the files directly in new/.\nAnd if we use new1, new2, new3, we don't need the explicit cleanup, as all tests\nare run in a \"trash directory\" which will be removed anyway.\n\nIn other words, we can write like this:\n(But this is for discussion, please read it as a suggestion)\n\n+test_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' '\n+\tgit init new3 &&\n+\t(\n+\t\tcd new3 &&\n+\t\techo \"Hey\" >new_file &&\n+\t\tgit add new_file &&\n+\t\tgit commit -m \"first_commit\" &&\n+\t\tgit checkout -b new_branch &&\n+\t\t>new_file &&\n+\t\tgit add new_file &&\n+\t\tgit reset - &&\n+\t\tgit status -uno >expected &&\n+\t\tgit add new_file &&\n+\t\tgit reset @{-1} &&\n+\t\tgit status -uno >actual\n+\t\ttest_cmp expected actual \n+\t)\n+'\n"},{"id":"257480","messageId":"CAPig+cS9t6gWdf+2A1MX7tfkS_Eb+MAdNn_Zgo6+oG4PCjP77w@mail.gmail.com","threadId":"38774","inReplyTo":"1426001883-6423-1-git-send-email-dyoucme@gmail.com","subject":"Re: [v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-10T17:25:30Z","receivedAt":"2015-03-10T17:25:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 10, 2015 at 11:38 AM, Sundararajan R wrote:\n> Teaching reset the - shorthand involves checking if any file named '-' exists\n> because it then becomes ambiguous as to whether the user wants to reset the\n> file '-' or if he wants to reset the working tree to the previous branch.\n\nFor clarity, I'd probably mention that the ambiguity arises only in\nthe absence of explicit '--' disambiguation.\n\n> check_filename() is used to perform this check. A similar ambiguity occurs\n> when the file @{-1} exits. Therefore, when the files '-' or '@{-1}' exist\n> then the program dies with a message about the ambiguous argument.\n\nWhy single out @{-1} as a potential file name? Has @{-1} ever been\nconsidered a filename rather than a treeish? Is this patch changing\nthe treatment of @{-1} so that it might be interpreted as a filename?\n\n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunshine \n> Signed-off-by: Sundararajan R \n> ---\n> Have made the modifications suggest by you, Eric.\n> Removed the part where the user is told that he can use ./- instead.\n>\n> builtin/reset.c | 15 ++++++++++++++-\n> 1 file changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 4c08ddc..88ce0c5 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> + int file_named_minus = 0;\n> /*\n> * Possible arguments are:\n> *\n> @@ -205,6 +206,12 @@ static void parse_args(struct pathspec *pathspec,\n> */\n>\n> if (argv[0]) {\n> + if (!strcmp(argv[0], \"-\") && !argv[1]) {\n> + if (!check_filename(prefix, \"-\"))\n> + argv[0] = \"@{-1}\";\n> + else\n> + file_named_minus = 1;\n> + }\n> if (!strcmp(argv[0], \"--\")) {\n> argv++; /* reset to HEAD, possibly with paths */\n> } else if (argv[1] && !strcmp(argv[1], \"--\")) {\n> @@ -226,7 +233,13 @@ static void parse_args(struct pathspec *pathspec,\n> rev = *argv++;\n> } else {\n> /* Otherwise we treat this as a filename */\n> - verify_filename(prefix, argv[0], 1);\n> + if (file_named_minus) {\n> + die(_(\"ambiguous argument '-': both revision and filename\\n\"\n> + \"Use '--' to separate paths from revisions, like this:\\n\"\n> + \"'git [...] -- [...]'\"));\n> + }\n> + else\n> + verify_filename(prefix, argv[0], 1);\n> }\n> }\n> *rev_ret = rev;\n> --\n> 2.1.0\n"},{"id":"257481","messageId":"CAPig+cQekpyaCd45O0NTijUqxvdTyNiZo1bXeuRKsmmYudwHMw@mail.gmail.com","threadId":"38774","inReplyTo":"1426001883-6423-2-git-send-email-dyoucme@gmail.com","subject":"Re: [v2 PATCH 2/2] reset: add tests for git reset -","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-10T17:35:38Z","receivedAt":"2015-03-10T17:35:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 10, 2015 at 11:38 AM, Sundararajan R wrote:\n> reset: add tests for git reset -\n\nSince this patch is changing the tests rather than 'reset' itself,\nyou'd likely want to say:\n\n t7102: add 'reset -' tests\n\n> The failure case which occurs on teaching git is taught the '-' shorthand\n> is when there exists no branch pointed to by '@{-1}'.\n\nECANNOTPARSE\n\n> The ambiguous cases occur when there exist files named '-' or '@{-1}' in\n> the work tree. These are also treated as failure cases but here the user\n> is given advice as to how he can proceed.\n>\n> Add tests to check the handling of these cases.\n> Also add a test to verify that reset - behaves like reset @{-1} when none\n> of the above cases are true.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Torsten Bögershausen \n\nTorsten already pointed out this botch.\n\n> Helped-by: Eric Sunshine \n> Helped-by: Matthieu Moy \n> Signed-off-by: Sundararajan R \n> ---\n> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> index 98bcfe2..c05dab0 100755\n> --- a/t/t7102-reset.sh\n> +++ b/t/t7102-reset.sh\n> @@ -568,4 +568,94 @@ test_expect_success 'reset --mixed sets up work tree' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'reset - with no @{-1} should fail' '\n> + git init new &&\n> + (\n> + cd new &&\n> + test_must_fail git reset - 2>actual\n> + ) &&\n> + test_i18ngrep \"unknown revision\" new/actual\n\nBroken &&-chain here and throughout the patch.\n\n> + test_when_finished rm -rf new\n\nIf one of the statements in the test before this point fails, then\ntest_when_finished() will never be invoked, which means that the \"rm\n-rf new\" cleanup action will never be run. Here, and throughout the\npatch, you need to invoke test_when_finished() at the earliest point\npossible so that the cleanup is effective even if some other part of\nthe test fails. In this case, register the cleanup either just before\nor just after git-init.\n\n> +'\n> +\n> +test_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' '\n> + git init new &&\n> + (\n> + cd new &&\n> + echo \"Hey\" >new_file &&\n> + git add new_file &&\n> + git commit -m \"first_commit\" &&\n> + git checkout -b new_branch &&\n> + >new_file &&\n> + git add new_file &&\n> + git reset - &&\n> + git status -uno >file1 &&\n> + git add new_file &&\n> + git reset @{-1} &&\n> + git status -uno >file2\n> + ) &&\n> + test_cmp new/file1 new/file2\n\nBroken &&-chain.\n\n> + test_when_finished rm -rf new\n> +'\n> +\n> test_done\n> --\n> 2.1.0\n"}]}