[PATCH 1/2] Adding - shorthand for @{-1} in RESET command

[PATCH 1/2] Adding - shorthand for @{-1} in RESET command

[Sundararajan R]

Please give feedback and suggest things I may have missed out on. 
I hope I have incorporated all the suggestions.

Signed-off-by: Sundararajan R <dyoucme@gmail.com>
Thanks-to: Junio C Hamano
---
I have attempted to resolve the ambiguity when there exists a file named -
by communicating to the user that he/she can use ./- when he/she wants to refer
to the - file. I perform this check using the check_filename() function.

 builtin/reset.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..2bdd5cd 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,14 @@ 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 ./- for file named -\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

[PATCH 2/2] Added tests for git reset -

[Sundararajan R]

As you had suggested @Junio, I have added the required tests.
Please let me know if there is something is I should add.

Signed-off-by: Sundararajan R <dyoucme@gmail.com>
Thanks-to: Junio C Hamano
---
I have added 6 tests to check for the following cases:
git reset - with no @{-1}
git reset - with no @{-1} and file named -
git reset - with @{-1} and file named @{-1}
git reset - with @{-1} and file named - 
git reset - with @{-1} and file named @{-1} and - 
git reset - with @{-1} and no file named - or @{-1} 
The 1st test with no previous branch results in the error
The 2nd,3rd,4th and 5th result in the ambiguous argument error 
The 6th test has - working like @{-1}

 t/t7102-reset.sh | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 98bcfe2..a670938 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,4 +568,111 @@ test_expect_success 'reset --mixed sets up work tree' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reset - with no @{-1}' '
+	git init new --quiet &&
+	cd new &&
+	test_must_fail git reset - >actual &&
+	touch expect &&
+	test_cmp expect actual
+'
+
+rm -rf new
+
+cat >expect <<EOF
+fatal: ambiguous argument '-': both revision and filename
+Use ./- for file named -
+Use '--' to separate paths from revisions, like this:
+'git <command> [<revision>...] -- [<file>...]'
+EOF
+
+test_expect_success 'reset - with no @{-1} and file named -' '
+	git init new --quiet &&
+	cd new &&
+	echo "Hello" > - &&
+	git add -
+	test_must_fail git reset - 2>actual &&
+	test_cmp ../expect actual
+'
+
+cd ..
+rm -rf new
+
+cat >expect <<EOF
+fatal: ambiguous argument '@{-1}': both revision and filename
+Use '--' to separate paths from revisions, like this:
+'git <command> [<revision>...] -- [<file>...]'
+EOF
+
+test_expect_success 'reset - with @{-1} and file named @{-1}' '
+	git init new --quiet &&
+	cd new && 
+	echo "Hello" >@{-1} &&
+	git add @{-1} &&
+	git commit -m "first_commit" &&
+	git checkout -b new_branch &&
+	touch @{-1} &&
+	git add @{-1} &&
+	test_must_fail git reset - 2>actual &&
+	test_cmp ../expect actual
+'
+
+cd ..
+rm -rf new
+
+cat >expect <<EOF
+fatal: ambiguous argument '-': both revision and filename
+Use ./- for file named -
+Use '--' to separate paths from revisions, like this:
+'git <command> [<revision>...] -- [<file>...]'
+EOF
+
+test_expect_success 'reset - with @{-1} and file named - ' '
+	git init new --quiet &&
+	cd new && 
+	echo "Hello" > - &&
+	git add - &&
+	git commit -m "first_commit" &&
+	git checkout -b new_branch &&
+	touch - &&
+	git add - &&
+	test_must_fail git reset - 2>actual &&
+	test_cmp ../expect actual
+'
+
+cd ..
+rm -rf new
+
+test_expect_success 'reset - with @{-1} and file named @{-1} and - ' '
+	git init new --quiet &&
+	cd new &&
+	echo "Hello" > - &&
+	git add - &&
+	git commit -m "first_commit" &&
+	git checkout -b new_branch
+	echo "Hello" >@{-1} &&
+	git add @{-1} &&
+	test_must_fail git reset - 2>actual &&
+	test_cmp ../expect actual
+'
+
+cd ..
+rm -rf new
+
+test_expect_success 'reset - with @{-1} and no file named - or @{-1} ' '
+	git init new --quiet &&
+	cd new &&
+	echo "Hello" >new_file &&
+	git add new_file &&
+	git commit -m "first_commit" &&
+	git checkout -b new_branch &&
+	echo "Hey" >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 file1 file2
+'
+
 test_done
-- 
2.1.0

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

[Torsten Bögershausen]

On 03/09/2015 09:46 PM, Sundararajan R wrote:
> As you had suggested @Junio, I have added the required tests.
> Please let me know if there is something is I should add.
>
> Signed-off-by: Sundararajan R <dyoucme@gmail.com>
> Thanks-to: Junio C Hamano
> ---
> I have added 6 tests to check for the following cases:
> git reset - with no @{-1}
> git reset - with no @{-1} and file named -
> git reset - with @{-1} and file named @{-1}
> git reset - with @{-1} and file named -
> git reset - with @{-1} and file named @{-1} and -
> git reset - with @{-1} and no file named - or @{-1}
> The 1st test with no previous branch results in the error
> The 2nd,3rd,4th and 5th result in the ambiguous argument error
> The 6th test has - working like @{-1}
>
>   t/t7102-reset.sh | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 107 insertions(+)
>
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..a670938 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,111 @@ test_expect_success 'reset --mixed sets up work tree' '
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'reset - with no @{-1}' '
> +	git init new --quiet &&
> +	cd new &&
> +	test_must_fail git reset - >actual &&
> +	touch expect &&
> +	test_cmp expect actual
> +'
> +
> +rm -rf new
> +
> +cat >expect <<EOF
> +fatal: ambiguous argument '-': both revision and filename
> +Use ./- for file named -
> +Use '--' to separate paths from revisions, like this:
> +'git <command> [<revision>...] -- [<file>...]'
> +EOF
> +
> +test_expect_success 'reset - with no @{-1} and file named -' '
> +	git init new --quiet &&
> +	cd new &&
> +	echo "Hello" > - &&
> +	git add -
> +	test_must_fail git reset - 2>actual &&
> +	test_cmp ../expect actual
> +'
> +
> +cd ..
> +rm -rf new
> +
> +cat >expect <<EOF
> +fatal: ambiguous argument '@{-1}': both revision and filename
> +Use '--' to separate paths from revisions, like this:
> +'git <command> [<revision>...] -- [<file>...]'
> +EOF
> +
> +test_expect_success 'reset - with @{-1} and file named @{-1}' '
> +	git init new --quiet &&
> +	cd new &&
If the shell changes the directory, this should be done in a subshell

+	git init new --quiet &&
+	(
		cd new &&

                # All the stuff
             )

+'

+cd ..

And the the .. should  be removed
(Same problem further down)

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

[Eric Sunshine]

On Mon, Mar 9, 2015 at 4:46 PM, Sundararajan R <dyoucme@gmail.com> wrote:
> As you had suggested @Junio, I have added the required tests.
> Please let me know if there is something is I should add.
>
> Signed-off-by: Sundararajan R <dyoucme@gmail.com>
> Thanks-to: Junio C Hamano
> ---
> I have added 6 tests to check for the following cases:
> git reset - with no @{-1}
> git reset - with no @{-1} and file named -
> git reset - with @{-1} and file named @{-1}
> git reset - with @{-1} and file named -
> git reset - with @{-1} and file named @{-1} and -
> git reset - with @{-1} and no file named - or @{-1}
> The 1st test with no previous branch results in the error
> The 2nd,3rd,4th and 5th result in the ambiguous argument error
> The 6th test has - working like @{-1}

See my review of patch 1 regarding where to place commentary, how to
construct a good commit message, and how to formulate the trailers
(Helped-by:, Signed-off-by:).

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..a670938 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,111 @@ test_expect_success 'reset --mixed sets up work tree' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'reset - with no @{-1}' '
> +       git init new --quiet &&

Why --quiet?

> +       cd new &&

As Torsten mentioned already, wrap a subshell (via '(' and ')') around
the 'cd' and commands which should be invoked within the subdirectory.
Doing so ensures that tests following this one aren't incorrectly run
within the working directory set by this test.

> +       test_must_fail git reset - >actual &&
> +       touch expect &&

Unless the timestamp of 'expect' is significant, don't use "touch" to
create the file. Instead, just say ">expect &&" on a line by itself.

> +       test_cmp expect actual

What is the intention here? Rather than git-reset's stderr, you've
captured stdout, which doesn't seem particularly interesting.

> +'
> +
> +rm -rf new

This is ineffective since the test just above left you inside the
'new' subdirectory, and there is no 'new' subdirectory within that one
to clean up.

> +cat >expect <<EOF
> +fatal: ambiguous argument '-': both revision and filename
> +Use ./- for file named -
> +Use '--' to separate paths from revisions, like this:
> +'git <command> [<revision>...] -- [<file>...]'
> +EOF

Reproducing the error message verbatim is fragile. Any minor change to
the text will cause the test to fail. Generally speaking, it is
sufficient just to use test_must_fail to ensure that the command fails
as expected. If you feel particularly strongly about checking that the
correct error condition occurred, then just check the error output
using test_i18ngrep() for a significant phrase, such as "ambiguous
argument".

> +test_expect_success 'reset - with no @{-1} and file named -' '
> +       git init new --quiet &&
> +       cd new &&
> +       echo "Hello" > - &&

Drop the space after the redirection operator.

> +       git add -

Broken &&-chain.

> +       test_must_fail git reset - 2>actual &&
> +       test_cmp ../expect actual
> +'
> +
> +cd ..

This is problematic. If the preceding test fails in git-init, before
the "cd new", then this "cd .." will change to the wrong place. Use of
a subshell, as explained above, avoids such problems.

> +rm -rf new
> +
> +cat >expect <<EOF
> +fatal: ambiguous argument '@{-1}': both revision and filename
> +Use '--' to separate paths from revisions, like this:
> +'git <command> [<revision>...] -- [<file>...]'
> +EOF

Rather than doing cleanup ("rm") and preparation ("cat >expect") at
the top-level, place them within the test which requires them, thus
making each test self-contained.

I'll stop reviewing here since the above comments also apply to the
rest of the tests.

> +test_expect_success 'reset - with @{-1} and file named @{-1}' '
> +       git init new --quiet &&
> +       cd new &&
> +       echo "Hello" >@{-1} &&
> +       git add @{-1} &&
> +       git commit -m "first_commit" &&
> +       git checkout -b new_branch &&
> +       touch @{-1} &&
> +       git add @{-1} &&
> +       test_must_fail git reset - 2>actual &&
> +       test_cmp ../expect actual
> +'
> +
> +cd ..
> +rm -rf new
> +
> +cat >expect <<EOF
> +fatal: ambiguous argument '-': both revision and filename
> +Use ./- for file named -
> +Use '--' to separate paths from revisions, like this:
> +'git <command> [<revision>...] -- [<file>...]'
> +EOF
> +
> +test_expect_success 'reset - with @{-1} and file named - ' '
> +       git init new --quiet &&
> +       cd new &&
> +       echo "Hello" > - &&
> +       git add - &&
> +       git commit -m "first_commit" &&
> +       git checkout -b new_branch &&
> +       touch - &&
> +       git add - &&
> +       test_must_fail git reset - 2>actual &&
> +       test_cmp ../expect actual
> +'
> +
> +cd ..
> +rm -rf new
> +
> +test_expect_success 'reset - with @{-1} and file named @{-1} and - ' '
> +       git init new --quiet &&
> +       cd new &&
> +       echo "Hello" > - &&
> +       git add - &&
> +       git commit -m "first_commit" &&
> +       git checkout -b new_branch
> +       echo "Hello" >@{-1} &&
> +       git add @{-1} &&
> +       test_must_fail git reset - 2>actual &&
> +       test_cmp ../expect actual
> +'
> +
> +cd ..
> +rm -rf new
> +
> +test_expect_success 'reset - with @{-1} and no file named - or @{-1} ' '
> +       git init new --quiet &&
> +       cd new &&
> +       echo "Hello" >new_file &&
> +       git add new_file &&
> +       git commit -m "first_commit" &&
> +       git checkout -b new_branch &&
> +       echo "Hey" >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 file1 file2
> +'
> +
>  test_done
> --
> 2.1.0

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +test_expect_success 'reset - with no @{-1}' '
>> +       git init new --quiet &&
>
> Why --quiet?

Also, to make sure tests serve as good examples, tests should stick
to "options first and then arguments", i.e. "git init --quiet new",
if it passes options.

Re: [PATCH 1/2] Adding - shorthand for @{-1} in RESET command

[Eric Sunshine]

On Mon, Mar 9, 2015 at 4:46 PM, Sundararajan R <dyoucme@gmail.com> wrote:
> Please give feedback and suggest things I may have missed out on.
> I hope I have incorporated all the suggestions.

If you haven't already, read Documentation/SubmittingPatches. Pay
particular attention to section #2 which explains how to write a good
commit message, and to section #4 to learn where to place patch
commentary not intended as part of the permanent commit history. The
above lines are commentary, not meant as part of the commit message.
Place such commentary below the '---' line just before the diffstat.

> Subject: Adding - shorthand for @{-1} in RESET command

Prefix the first line of the commit message with the module or command
you re changing. Drop capitalization. Write in imperative mood. For
instance:

    reset: add '-' shorthand for '@{-1}'

> Signed-off-by: Sundararajan R <dyoucme@gmail.com>
> Thanks-to: Junio C Hamano

Place your sign-off last. Use Helped-by: rather than Thanks-to: and
include the person's full name and email address.

> ---

Here, just below the '---' line is where you should place commentary
not intended for the permanent commit record.

> I have attempted to resolve the ambiguity when there exists a file named -
> by communicating to the user that he/she can use ./- when he/she wants to refer
> to the - file. I perform this check using the check_filename() function.

This is important information for the commit message itself above the
'---' line, though you would want to rephrase it just to state the
facts. No need to mention "I did this" or "I did that" since the patch
itself implies that you made the changes.

>  builtin/reset.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..2bdd5cd 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;

Style: Here and elsewhere, add a space around '='.

>         /*
>          * 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,"-"))

Style: Here and elsewhere, add space after 'if'.
Style: Add space after comma.

> +                               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,14 @@ 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 ./- for file named -\n"
> +                                       "Use '--' to separate paths from revisions, like this:\n"
> +                                       "'git <command> [<revision>...] -- [<file>...]'"));

This seems odd. If arguments following '--' are unconditionally
treated as paths, why is it be necessary to tell the user to spell out
file '-' as './-'? Shouldn't "git reset -- -" be sufficient?

> +                       }
> +                       else
> +                               verify_filename(prefix, argv[0], 1);
>                 }
>         }
>         *rev_ret = rev;
> --
> 2.1.0

Re: [PATCH 1/2] Adding - shorthand for @{-1} in RESET command

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -226,7 +233,14 @@ 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 ./- for file named -\n"
>> +                                       "Use '--' to separate paths from revisions, like this:\n"
>> +                                       "'git <command> [<revision>...] -- [<file>...]'"));
>
> This seems odd. If arguments following '--' are unconditionally
> treated as paths, why is it be necessary to tell the user to spell out
> file '-' as './-'? Shouldn't "git reset -- -" be sufficient?

I find that the presense of the if statement itself even odder.

 - verify_filename() and verify_non_filename() are designed to check
   that the string "-" given by the end-user is or is not a filename
   on the filesystem.  Why isn't this caller letting the callee do
   the job it was designed to do and doing that itself instead?

 - we know "-" aka "@{-1}" does not resolve to a committish at this
   point, so it must be a filename.  If "-" exists, then why should
   the user even need to differenciate it as ./- (or with "-- -")?

   After all, if there is no branch whose name is 'foo' and a file
   'foo' exists on the filesystem, the user can say "git reset foo"
   without disambiguation to reset that path, no?