[v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}'

[v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}'

[Sundararajan R]

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 <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Sundararajan R <dyoucme@gmail.com>
---
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 <command> [<revision>...] -- [<file>...]'"));
+			}
+			else
+				verify_filename(prefix, argv[0], 1);
 		}
 	}
 	*rev_ret = rev;
-- 
2.1.0

[v2 PATCH 2/2] reset: add tests for git reset -

[Sundararajan R]

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 <gitster@pobox.com>
Helped-by: Torsten Bögershausen <tboegi@web.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Sundararajan R <dyoucme@gmail.com>
---
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

Re: [v2 PATCH 2/2] reset: add tests for git reset -

[Torsten Bögershausen]

On 2015-03-10 16.38, Sundararajan R wrote:

> Helped-by: Torsten Bögershausen <tboegi@web.de>
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 
+	)
+'

Re: [v2 PATCH 1/2] reset: add '-' shorthand for '@{-1}'

[Eric Sunshine]

On Tue, Mar 10, 2015 at 11:38 AM, Sundararajan R <dyoucme@gmail.com> 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 <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Sundararajan R <dyoucme@gmail.com>
> ---
> 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 <command> [<revision>...] -- [<file>...]'"));
> +                       }
> +                       else
> +                               verify_filename(prefix, argv[0], 1);
>                 }
>         }
>         *rev_ret = rev;
> --
> 2.1.0

Re: [v2 PATCH 2/2] reset: add tests for git reset -

[Eric Sunshine]

On Tue, Mar 10, 2015 at 11:38 AM, Sundararajan R <dyoucme@gmail.com> 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 <gitster@pobox.com>
> Helped-by: Torsten Bögershausen <tboegi@web.de>

Torsten already pointed out this botch.

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Sundararajan R <dyoucme@gmail.com>
> ---
> 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