# [v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}' 5 messages from 2015-03-10 to 2015-03-10. Participants: Sundararajan R, Torsten Bögershausen, Eric Sunshine. Thread: https://gitlist.dev/t/38774 ## Sundararajan R, 2015-03-10 15:38 Subject: [v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}' Message-ID: <1426001883-6423-1-git-send-email-dyoucme@gmail.com> URL: https://gitlist.dev/e/1426001883-6423-1-git-send-email-dyoucme%40gmail.com ``` Teaching reset the - shorthand involves checking if any file named '-' exists because it then becomes ambiguous as to whether the user wants to reset the file '-' or if he wants to reset the working tree to the previous branch. check_filename() is used to perform this check. A similar ambiguity occurs when the file @{-1} exits. Therefore, when the files '-' or '@{-1}' exist then the program dies with a message about the ambiguous argument. Helped-by: Junio C Hamano Helped-by: Eric Sunshine Signed-off-by: Sundararajan R --- Have made the modifications suggest by you, Eric. Removed the part where the user is told that he can use ./- instead. builtin/reset.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index 4c08ddc..88ce0c5 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec, { const char *rev = "HEAD"; unsigned char unused[20]; + int file_named_minus = 0; /* * Possible arguments are: * @@ -205,6 +206,12 @@ static void parse_args(struct pathspec *pathspec, */ if (argv[0]) { + if (!strcmp(argv[0], "-") && !argv[1]) { + if (!check_filename(prefix, "-")) + argv[0] = "@{-1}"; + else + file_named_minus = 1; + } if (!strcmp(argv[0], "--")) { argv++; /* reset to HEAD, possibly with paths */ } else if (argv[1] && !strcmp(argv[1], "--")) { @@ -226,7 +233,13 @@ static void parse_args(struct pathspec *pathspec, rev = *argv++; } else { /* Otherwise we treat this as a filename */ - verify_filename(prefix, argv[0], 1); + if (file_named_minus) { + die(_("ambiguous argument '-': both revision and filename\n" + "Use '--' to separate paths from revisions, like this:\n" + "'git [...] -- [...]'")); + } + else + verify_filename(prefix, argv[0], 1); } } *rev_ret = rev; -- 2.1.0 ``` ## Sundararajan R, 2015-03-10 15:38 Subject: [v2 PATCH 2/2] reset: add tests for git reset - Message-ID: <1426001883-6423-2-git-send-email-dyoucme@gmail.com> URL: https://gitlist.dev/e/1426001883-6423-2-git-send-email-dyoucme%40gmail.com In-Reply-To: <1426001883-6423-1-git-send-email-dyoucme@gmail.com> ``` The failure case which occurs on teaching git is taught the '-' shorthand is when there exists no branch pointed to by '@{-1}'. The ambiguous cases occur when there exist files named '-' or '@{-1}' in the work tree. These are also treated as failure cases but here the user is given advice as to how he can proceed. Add tests to check the handling of these cases. Also add a test to verify that reset - behaves like reset @{-1} when none of the above cases are true. Helped-by: Junio C Hamano Helped-by: Torsten Bögershausen Helped-by: Eric Sunshine Helped-by: Matthieu Moy Signed-off-by: Sundararajan R --- Thank you for your feedback Torsten and Eric. I have now made the modifications suggested by you. I have also incorporated the suggestions given by Matthieu on the archive. Please let me know if there is something else I should add. t/t7102-reset.sh | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 98bcfe2..c05dab0 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -568,4 +568,94 @@ test_expect_success 'reset --mixed sets up work tree' ' test_cmp expect actual ' +test_expect_success 'reset - with no @{-1} should fail' ' + git init new && + ( + cd new && + test_must_fail git reset - 2>actual + ) && + test_i18ngrep "unknown revision" new/actual + test_when_finished rm -rf new +' + +test_expect_success 'reset - with no @{-1} and file named - should fail' ' + git init new && + ( + cd new && + echo "Hello" >- && + git add - && + test_must_fail git reset - 2>actual + ) && + test_i18ngrep "both revision and filename" new/actual + test_when_finished rm -rf new +' + +test_expect_success 'reset - with @{-1} and file named @{-1} should fail' ' + git init new && + ( + cd new && + echo "Hello" >@{-1} && + git add @{-1} && + git commit -m "first_commit" && + git checkout -b new_branch && + >@{-1} && + git add @{-1} && + test_must_fail git reset - 2>actual + ) && + test_i18ngrep "both revision and filename" new/actual + test_when_finished rm -rf new +' + +test_expect_success 'reset - with @{-1} and file named - should fail' ' + git init new && + ( + cd new && + echo "Hello" >- && + git add - && + git commit -m "first_commit" && + git checkout -b new_branch && + >- && + git add - && + test_must_fail git reset - 2>actual + ) && + test_i18ngrep "both revision and filename" new/actual + test_when_finished rm -rf new +' + +test_expect_success 'reset - with @{-1} and file named @{-1} and - should fail' ' + git init new && + ( + cd new && + >- && + git add - && + git commit -m "first_commit" && + git checkout -b new_branch + >@{-1} && + git add @{-1} && + test_must_fail git reset - 2>actual + ) && + test_i18ngrep "both revision and filename" new/actual + test_when_finished rm -rf new +' + +test_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' ' + git init new && + ( + cd new && + echo "Hey" >new_file && + git add new_file && + git commit -m "first_commit" && + git checkout -b new_branch && + >new_file && + git add new_file && + git reset - && + git status -uno >file1 && + git add new_file && + git reset @{-1} && + git status -uno >file2 + ) && + test_cmp new/file1 new/file2 + test_when_finished rm -rf new +' + test_done -- 2.1.0 ``` ## Torsten Bögershausen, 2015-03-10 17:23 Subject: Re: [v2 PATCH 2/2] reset: add tests for git reset - Message-ID: <54FF288D.3000304@web.de> URL: https://gitlist.dev/e/54FF288D.3000304%40web.de In-Reply-To: <1426001883-6423-2-git-send-email-dyoucme@gmail.com> ``` On 2015-03-10 16.38, Sundararajan R wrote: > Helped-by: Torsten Bögershausen There seems to be an issue that the mail is encoded from (what ? Latin-1) into UTF-8 2 times The easy solution is to remove the line, I'm OK with that, since a review-comment is not necessarily motivating a Helped-by, at least not for me. Mentioning it in the comments good and is enough. But why is the mail "encoded twice" ? (this what the header says:) X-Mailer: git-send-email 2.1.0 Content-Type: text/plain; charset=UTF-8 Can somebody help out with a good explanation ? Another (minor) thing: There is nothing wrong with the test, but we can make it 3% more "Git-style" and easier too read when it is more similar to the rest of the code base: test_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' ' + git init new && + ( + cd new && + echo "Hey" >new_file && + git add new_file && + git commit -m "first_commit" && + git checkout -b new_branch && + >new_file && + git add new_file && + git reset - && + git status -uno >file1 && (Side-question: why "status -uno") typically "file" (or "file1") is used for user files, not for the "expected" or "actual" output. Then we can compare the files directly in new/. And if we use new1, new2, new3, we don't need the explicit cleanup, as all tests are run in a "trash directory" which will be removed anyway. In other words, we can write like this: (But this is for discussion, please read it as a suggestion) +test_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' ' + git init new3 && + ( + cd new3 && + echo "Hey" >new_file && + git add new_file && + git commit -m "first_commit" && + git checkout -b new_branch && + >new_file && + git add new_file && + git reset - && + git status -uno >expected && + git add new_file && + git reset @{-1} && + git status -uno >actual + test_cmp expected actual + ) +' ``` ## Eric Sunshine, 2015-03-10 17:25 Subject: Re: [v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}' Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS9t6gWdf%2B2A1MX7tfkS_Eb%2BMAdNn_Zgo6%2BoG4PCjP77w%40mail.gmail.com In-Reply-To: <1426001883-6423-1-git-send-email-dyoucme@gmail.com> ``` On Tue, Mar 10, 2015 at 11:38 AM, Sundararajan R wrote: > Teaching reset the - shorthand involves checking if any file named '-' exists > because it then becomes ambiguous as to whether the user wants to reset the > file '-' or if he wants to reset the working tree to the previous branch. For clarity, I'd probably mention that the ambiguity arises only in the absence of explicit '--' disambiguation. > check_filename() is used to perform this check. A similar ambiguity occurs > when the file @{-1} exits. Therefore, when the files '-' or '@{-1}' exist > then the program dies with a message about the ambiguous argument. Why single out @{-1} as a potential file name? Has @{-1} ever been considered a filename rather than a treeish? Is this patch changing the treatment of @{-1} so that it might be interpreted as a filename? > Helped-by: Junio C Hamano > Helped-by: Eric Sunshine > Signed-off-by: Sundararajan R > --- > Have made the modifications suggest by you, Eric. > Removed the part where the user is told that he can use ./- instead. > > builtin/reset.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/builtin/reset.c b/builtin/reset.c > index 4c08ddc..88ce0c5 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec, > { > const char *rev = "HEAD"; > unsigned char unused[20]; > + int file_named_minus = 0; > /* > * Possible arguments are: > * > @@ -205,6 +206,12 @@ static void parse_args(struct pathspec *pathspec, > */ > > if (argv[0]) { > + if (!strcmp(argv[0], "-") && !argv[1]) { > + if (!check_filename(prefix, "-")) > + argv[0] = "@{-1}"; > + else > + file_named_minus = 1; > + } > if (!strcmp(argv[0], "--")) { > argv++; /* reset to HEAD, possibly with paths */ > } else if (argv[1] && !strcmp(argv[1], "--")) { > @@ -226,7 +233,13 @@ static void parse_args(struct pathspec *pathspec, > rev = *argv++; > } else { > /* Otherwise we treat this as a filename */ > - verify_filename(prefix, argv[0], 1); > + if (file_named_minus) { > + die(_("ambiguous argument '-': both revision and filename\n" > + "Use '--' to separate paths from revisions, like this:\n" > + "'git [...] -- [...]'")); > + } > + else > + verify_filename(prefix, argv[0], 1); > } > } > *rev_ret = rev; > -- > 2.1.0 ``` ## Eric Sunshine, 2015-03-10 17:35 Subject: Re: [v2 PATCH 2/2] reset: add tests for git reset - Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQekpyaCd45O0NTijUqxvdTyNiZo1bXeuRKsmmYudwHMw%40mail.gmail.com In-Reply-To: <1426001883-6423-2-git-send-email-dyoucme@gmail.com> ``` On Tue, Mar 10, 2015 at 11:38 AM, Sundararajan R wrote: > reset: add tests for git reset - Since this patch is changing the tests rather than 'reset' itself, you'd likely want to say: t7102: add 'reset -' tests > The failure case which occurs on teaching git is taught the '-' shorthand > is when there exists no branch pointed to by '@{-1}'. ECANNOTPARSE > The ambiguous cases occur when there exist files named '-' or '@{-1}' in > the work tree. These are also treated as failure cases but here the user > is given advice as to how he can proceed. > > Add tests to check the handling of these cases. > Also add a test to verify that reset - behaves like reset @{-1} when none > of the above cases are true. > > Helped-by: Junio C Hamano > Helped-by: Torsten Bögershausen Torsten already pointed out this botch. > Helped-by: Eric Sunshine > Helped-by: Matthieu Moy > Signed-off-by: Sundararajan R > --- > diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh > index 98bcfe2..c05dab0 100755 > --- a/t/t7102-reset.sh > +++ b/t/t7102-reset.sh > @@ -568,4 +568,94 @@ test_expect_success 'reset --mixed sets up work tree' ' > test_cmp expect actual > ' > > +test_expect_success 'reset - with no @{-1} should fail' ' > + git init new && > + ( > + cd new && > + test_must_fail git reset - 2>actual > + ) && > + test_i18ngrep "unknown revision" new/actual Broken &&-chain here and throughout the patch. > + test_when_finished rm -rf new If one of the statements in the test before this point fails, then test_when_finished() will never be invoked, which means that the "rm -rf new" cleanup action will never be run. Here, and throughout the patch, you need to invoke test_when_finished() at the earliest point possible so that the cleanup is effective even if some other part of the test fails. In this case, register the cleanup either just before or just after git-init. > +' > + > +test_expect_success 'reset - with @{-1} and no file named - or @{-1} should succeed' ' > + git init new && > + ( > + cd new && > + echo "Hey" >new_file && > + git add new_file && > + git commit -m "first_commit" && > + git checkout -b new_branch && > + >new_file && > + git add new_file && > + git reset - && > + git status -uno >file1 && > + git add new_file && > + git reset @{-1} && > + git status -uno >file2 > + ) && > + test_cmp new/file1 new/file2 Broken &&-chain. > + test_when_finished rm -rf new > +' > + > test_done > -- > 2.1.0 ```