[PATCH v3 1/2] reset: enable '-' short-hand for previous branch

[PATCH v3 1/2] reset: enable '-' short-hand for previous branch

[Sudhanshu Shekhar]

'git reset -' will reset to the previous branch. It will behave similar
to @{-1} except when a file named '@{-1}' is present. To refer to a file
named '-', use ./- or the -- flag.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
Thank you all for your feedback. I have made changes and I hope this patch meets community standards. Please let me know if any further changes are required.

Regards,
Sudhanshu

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

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..8abd300 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 substituted_minus = 0;
 	/*
 	 * Possible arguments are:
 	 *
@@ -205,6 +206,10 @@ static void parse_args(struct pathspec *pathspec,
 	 */
 
 	if (argv[0]) {
+		if (!strcmp(argv[0], "-")) {
+			argv[0] = "@{-1}";
+			substituted_minus = 1;
+		}
 		if (!strcmp(argv[0], "--")) {
 			argv++; /* reset to HEAD, possibly with paths */
 		} else if (argv[1] && !strcmp(argv[1], "--")) {
@@ -222,15 +227,21 @@ static void parse_args(struct pathspec *pathspec,
 			 * Ok, argv[0] looks like a commit/tree; it should not
 			 * be a filename.
 			 */
+			if (substituted_minus)
+				argv[0] = "-";
 			verify_non_filename(prefix, argv[0]);
+			if (substituted_minus)
+				argv[0] = "@{-1}";
 			rev = *argv++;
 		} else {
+			/* We were treating "-" as a commit and not a file */
+			if (substituted_minus)
+				argv[0] = "-";
 			/* Otherwise we treat this as a filename */
 			verify_filename(prefix, argv[0], 1);
 		}
 	}
 	*rev_ret = rev;
-
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
-- 
2.3.1.279.gd534259

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

[Sudhanshu Shekhar]

Added the following test cases:
    1) Confirm error message when git reset is used with no previous branch
    2) Confirm git reset - works like git reset @{-1}
    3) Confirm "-" is always treated as a commit unless the -- file option
    is specified
    4) Confirm "git reset -" works normally even when a file named @{-1} is
    present

Helped-by: David Aguilar <davvid@gmail.com>
Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
I have tried to keep each test self sufficient. Please let me know if any changes are required.
Thank you!

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

diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 98bcfe2..0faf241 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,4 +568,143 @@ test_expect_success 'reset --mixed sets up work tree' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reset - with no previous branch' '
+	git init no_previous &&
+	(
+		cd no_previous &&
+		test_must_fail git reset - 2>output
+	) &&
+	test_i18ngrep "bad flag" no_previous/output
+'
+
+test_expect_success 'reset - while having file named - and no previous branch' '
+	git init no_previous &&
+	(
+		cd no_previous &&
+		>./- &&
+		test_must_fail git reset - 2>output
+	) &&
+	test_i18ngrep "bad flag" no_previous/output
+'
+
+
+test_expect_success 'reset - in the presence of file named - with previous branch' '
+	echo "Unstaged changes after reset:" >expect &&
+	echo "M	-" >>expect &&
+	echo "M	1" >>expect &&
+	git init no_previous &&
+	(
+		cd no_previous &&
+		>./- &&
+		>1 &&
+		git add 1 - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >./- &&
+		echo "wow" >1 &&
+		git add 1 - &&
+		git reset - >../output
+	) &&
+	rm -rf no_previous &&
+	test_cmp output expect
+'
+test_expect_success 'reset - in the presence of file named - with -- option' '
+	echo "Unstaged changes after reset:" >expect &&
+	echo "M	-" >>expect &&
+	echo "M	1" >>expect &&
+	git init no_previous &&
+	(
+		cd no_previous &&
+		>./- &&
+		>1 &&
+		git add 1 - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >./- &&
+		echo "wow" >1 &&
+		git add 1 - &&
+		git reset - -- >../output
+	) &&
+	rm -rf no_previous &&
+	test_cmp output expect
+'
+
+test_expect_success 'reset - in the presence of file named - with -- file option' '
+	echo "Unstaged changes after reset:" >expect &&
+	echo "M	-" >>expect &&
+	git init no_previous &&
+	(
+		cd no_previous &&
+		>./- &&
+		>1 &&
+		git add 1 - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >./- &&
+		echo "wow" >1 &&
+		git add 1 - &&
+		git reset -- - >../output
+	) &&
+	rm -rf no_previous
+	test_cmp output expect
+'
+test_expect_success 'reset - in the presence of file named - with both pre and post -- option' '
+	echo "Unstaged changes after reset:" >expect &&
+	echo "M	-" >>expect &&
+	git init no_previous &&
+	(
+		cd no_previous &&
+		>./- &&
+		>1 &&
+		git add 1 - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >./- &&
+		echo "wow" >1 &&
+		git add 1 - &&
+		git reset - -- - >../output
+	) &&
+	rm -rf no_previous
+	test_cmp output expect
+'
+
+test_expect_success 'reset - works same as reset @{-1}' '
+	git init no_previous &&
+	(
+		cd no_previous &&
+		echo "random" >random &&
+		git add random &&
+		git commit -m "base commit" &&
+		git checkout -b temp &&
+		echo new-file >new-file &&
+		git add new-file &&
+		git commit -m "added new-file" &&
+		git reset - &&
+		git status --porcelain >../first &&
+		git add new-file &&
+		git commit -m "added new-file" &&
+		git reset @{-1} &&
+		git status --porcelain >../second
+	) &&
+	test_cmp first second
+'
+
+test_expect_success 'reset - with file named @{-1}' '
+	echo "Unstaged changes after reset:" >expect &&
+	echo "M	@{-1}" >>expect &&
+	git init no_previous &&
+	(
+		cd no_previous &&
+		echo "random" >./@{-1} &&
+		git add ./@{-1} &&
+		git commit -m "base commit" &&
+		git checkout -b new_branch &&
+		echo "additional stuff" >>./@{-1} &&
+		git add ./@{-1} &&
+		git reset - >../output
+	) &&
+	rm -rf no_previous &&
+	test_cmp output expect
+'
+
 test_done
-- 
2.3.1.279.gd534259

Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch

[Matthieu Moy]

Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:

>  	*rev_ret = rev;
> -
>  	if (read_cache() < 0)

Please don't make whitespace-only changes like this in your patches.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Matthieu Moy]

Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:

> +test_expect_success 'reset - while having file named - and no previous branch' '

I like having the expected behavior in the test name too. e.g. add
"fails" at the end of the sentence.

> +test_expect_success 'reset - in the presence of file named - with previous branch' '
> +	echo "Unstaged changes after reset:" >expect &&
> +	echo "M	-" >>expect &&
> +	echo "M	1" >>expect &&

Here and elsewhere: why not

	cat >expect <<-EOF
	Unstaged changes after reset:
	M -
	M 1

?

> +	rm -rf no_previous &&

That would be best done in a test_when_finished, so that the directory
is removed regardless of whether the test failed before this line or
not.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Sudhanshu Shekhar]

Hi,

On Tue, Mar 10, 2015 at 6:56 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:
>
>> +test_expect_success 'reset - while having file named - and no previous branch' '
>
> I like having the expected behavior in the test name too. e.g. add
> "fails" at the end of the sentence.
>
Sure. I will update this in the next patch.

>> +test_expect_success 'reset - in the presence of file named - with previous branch' '
>> +     echo "Unstaged changes after reset:" >expect &&
>> +     echo "M -" >>expect &&
>> +     echo "M 1" >>expect &&
>
> Here and elsewhere: why not
>
>         cat >expect <<-EOF
>         Unstaged changes after reset:
>         M -
>         M 1
>
> ?

I was confused whether to use cat or echo. I thought using cat will
disrupt the indentation as the EOF needs to be on a single line. This
is why I chose echo. Please let me know your thoughts on this.

>> +     rm -rf no_previous &&
>
> That would be best done in a test_when_finished, so that the directory
> is removed regardless of whether the test failed before this line or
> not.
Thanks for pointing this out. I will update this accordingly.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Regards,
Sudhanshu

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

[Eric Sunshine]

On Tue, Mar 10, 2015 at 1:52 PM, Sudhanshu Shekhar
<sudshekhar02@gmail.com> wrote:
> On Tue, Mar 10, 2015 at 6:56 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:
>>> +test_expect_success 'reset - in the presence of file named - with previous branch' '
>>> +     echo "Unstaged changes after reset:" >expect &&
>>> +     echo "M -" >>expect &&
>>> +     echo "M 1" >>expect &&
>>
>> Here and elsewhere: why not
>>
>>         cat >expect <<-EOF
>>         Unstaged changes after reset:
>>         M -
>>         M 1
>>
>> ?
> I was confused whether to use cat or echo. I thought using cat will
> disrupt the indentation as the EOF needs to be on a single line. This
> is why I chose echo. Please let me know your thoughts on this.

Here-docs are easier to compose and read than individual 'echo'
statements for multi-line content.

The '-' in front of EOF allows you to indent the entire body. Even
better, use -\EOF to signify that you don't expect any interpolation
to occur within the body.

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

[Eric Sunshine]

On Tue, Mar 10, 2015 at 6:52 AM, Sudhanshu Shekhar
<sudshekhar02@gmail.com> wrote:
> Added tests for reset -

Mention the area of the project you are changing, followed by a colon,
followed by a short summary of the change. Drop capitalization. Write
in imperative mood.

    t7102: add 'reset -' tests

> Added the following test cases:

Imperative mood: "Add test cases:"

>     1) Confirm error message when git reset is used with no previous branch
>     2) Confirm git reset - works like git reset @{-1}
>     3) Confirm "-" is always treated as a commit unless the -- file option
>     is specified
>     4) Confirm "git reset -" works normally even when a file named @{-1} is
>     present
>
> Helped-by: David Aguilar <davvid@gmail.com>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..0faf241 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,143 @@ test_expect_success 'reset --mixed sets up work tree' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'reset - in the presence of file named - with previous branch' '
> +       echo "Unstaged changes after reset:" >expect &&
> +       echo "M -" >>expect &&
> +       echo "M 1" >>expect &&

Mentioned by Matthieu: Use "cat <<-\EOF".

> +       git init no_previous &&
> +       (
> +               cd no_previous &&
> +               >./- &&

Unnecessarily complex ">./-" when ">-" will suffice.

> +               >1 &&
> +               git add 1 - &&
> +               git commit -m "add base files" &&
> +               git checkout -b new_branch &&
> +               echo "random" >./- &&
> +               echo "wow" >1 &&
> +               git add 1 - &&
> +               git reset - >../output

Since you will be comparing this output with "expected" output, it is
customary to name this file "actual".

> +       ) &&
> +       rm -rf no_previous &&

Mentioned by Matthieu: Use test_when_finished(). You want this cleanup
action invoked even if any part of the test fails, so register it as
early as possible for it to be effective; either just before or after
git-init.

> +       test_cmp output expect

When test_cmp() detects a difference in the expected and actual
output, it dumps the difference (in 'diff' format) as debugging
output. It's easier to read 'diff' output, and matches our
expectations more closely, if 'expect' is mentioned before 'actual',
so:

    test_cmp expect actual

> +'
> +test_expect_success 'reset - in the presence of file named - with -- file option' '
> +       echo "Unstaged changes after reset:" >expect &&
> +       echo "M -" >>expect &&
> +       git init no_previous &&
> +       (
> +               cd no_previous &&
> +               >./- &&
> +               >1 &&
> +               git add 1 - &&
> +               git commit -m "add base files" &&
> +               git checkout -b new_branch &&
> +               echo "random" >./- &&
> +               echo "wow" >1 &&
> +               git add 1 - &&
> +               git reset -- - >../output
> +       ) &&
> +       rm -rf no_previous

Broken &&-chain.

> +       test_cmp output expect
> +'
> +test_expect_success 'reset - in the presence of file named - with both pre and post -- option' '
> +       echo "Unstaged changes after reset:" >expect &&
> +       echo "M -" >>expect &&
> +       git init no_previous &&
> +       (
> +               cd no_previous &&
> +               >./- &&
> +               >1 &&
> +               git add 1 - &&
> +               git commit -m "add base files" &&
> +               git checkout -b new_branch &&
> +               echo "random" >./- &&
> +               echo "wow" >1 &&
> +               git add 1 - &&
> +               git reset - -- - >../output
> +       ) &&
> +       rm -rf no_previous

Broken &&-chain.

> +       test_cmp output expect
> +'
> +
> +test_expect_success 'reset - works same as reset @{-1}' '
> +       git init no_previous &&
> +       (
> +               cd no_previous &&
> +               echo "random" >random &&
> +               git add random &&
> +               git commit -m "base commit" &&
> +               git checkout -b temp &&
> +               echo new-file >new-file &&
> +               git add new-file &&
> +               git commit -m "added new-file" &&
> +               git reset - &&
> +               git status --porcelain >../first &&
> +               git add new-file &&
> +               git commit -m "added new-file" &&
> +               git reset @{-1} &&
> +               git status --porcelain >../second
> +       ) &&

In other tests, you performed "rm -rf no_previous &&" cleanup at this
point, but it's missing here.

> +       test_cmp first second
> +'
> +
> +test_expect_success 'reset - with file named @{-1}' '
> +       echo "Unstaged changes after reset:" >expect &&
> +       echo "M @{-1}" >>expect &&
> +       git init no_previous &&
> +       (
> +               cd no_previous &&
> +               echo "random" >./@{-1} &&
> +               git add ./@{-1} &&
> +               git commit -m "base commit" &&
> +               git checkout -b new_branch &&
> +               echo "additional stuff" >>./@{-1} &&
> +               git add ./@{-1} &&
> +               git reset - >../output
> +       ) &&
> +       rm -rf no_previous &&
> +       test_cmp output expect
> +'
> +
>  test_done
> --
> 2.3.1.279.gd534259

Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch

[Eric Sunshine]

On Tue, Mar 10, 2015 at 6:52 AM, Sudhanshu Shekhar
<sudshekhar02@gmail.com> wrote:
> 'git reset -' will reset to the previous branch. It will behave similar
> to @{-1} except when a file named '@{-1}' is present. To refer to a file
> named '-', use ./- or the -- flag.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..8abd300 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -205,6 +206,10 @@ static void parse_args(struct pathspec *pathspec,
>          */
>
>         if (argv[0]) {
> +               if (!strcmp(argv[0], "-")) {
> +                       argv[0] = "@{-1}";

It's somewhat ugly to modify an element of argv[] since you don't own
the array and the contract does not particularly suggest that is
modifiable by you. A more serious concern is that doing so creates a
tighter binding between parse_args() and its caller since parse_args()
doesn't know how the caller will be disposing of argv[] when finished
with it. Say, for instance, that the caller disposes of each argv[]
element by invoking free(argv[n]). The literal "@{-1}" you've assigned
here is not allocated on the heap, so free()ing it would be wrong.

However, some of the other commands which alias "-" to "@{-1}" also
modify argv[] in this way, so we'll let it slide for the moment.

> +                       substituted_minus = 1;
> +               }
>                 if (!strcmp(argv[0], "--")) {
>                         argv++; /* reset to HEAD, possibly with paths */
>                 } else if (argv[1] && !strcmp(argv[1], "--")) {
> @@ -222,15 +227,21 @@ static void parse_args(struct pathspec *pathspec,
>                          * Ok, argv[0] looks like a commit/tree; it should not
>                          * be a filename.
>                          */
> +                       if (substituted_minus)
> +                               argv[0] = "-";
>                         verify_non_filename(prefix, argv[0]);
> +                       if (substituted_minus)
> +                               argv[0] = "@{-1}";

Do you find it ugly that you have to undo and then redo your argv[]
change from a few lines above? Rather than jumping through such hoops,
can't you instead define a new variable which holds the appropriate
value ("@{-1}"), or rework the logic to avoid such kludges altogether?

>                         rev = *argv++;
>                 } else {
> +                       /* We were treating "-" as a commit and not a file */
> +                       if (substituted_minus)
> +                               argv[0] = "-";
>                         /* Otherwise we treat this as a filename */
>                         verify_filename(prefix, argv[0], 1);
>                 }
>         }
>         *rev_ret = rev;
> -

Mentioned by Matthieu: Don't sneak in unrelated changes. This change
was probably unintentional, but serves as a good reminder that you
should review the patch yourself before sending it. (And, if you do
have a need to make cleanups, it's typically best to do so as a
separate preparatory patch.)

>         if (read_cache() < 0)
>                 die(_("index file corrupt"));
>
> --
> 2.3.1.279.gd534259

[PATCH v3 1/2] reset: enable '-' short-hand for previous branch

[Sudhanshu Shekhar]

git reset -' will reset to the previous branch. It will behave similar
to @{-1} except when a file named '@{-1}' is present. To refer to a file
named '-', use ./- or the -- flag.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
Eric, I have added a user_input variable to record the input entered by the user. This way I can avoid the multiple 'if' clauses. Thank you for the suggestion.
I have also removed the unrelated change that I had unintentionally committed. I am sending this patch on the thread for further review. Once both the patches are reviewed and accepted, I will create a new mail for it. Hope that is okay.

Regards,
Sudhanshu

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

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..b428241 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -192,6 +192,8 @@ static void parse_args(struct pathspec *pathspec,
 {
 	const char *rev = "HEAD";
 	unsigned char unused[20];
+	int substituted_minus = 0;
+	char *user_input = argv[0];
 	/*
 	 * Possible arguments are:
 	 *
@@ -205,6 +207,10 @@ static void parse_args(struct pathspec *pathspec,
 	 */
 
 	if (argv[0]) {
+		if (!strcmp(argv[0], "-")) {
+			argv[0] = "@{-1}";
+			substituted_minus = 1;
+		}
 		if (!strcmp(argv[0], "--")) {
 			argv++; /* reset to HEAD, possibly with paths */
 		} else if (argv[1] && !strcmp(argv[1], "--")) {
@@ -222,9 +228,12 @@ static void parse_args(struct pathspec *pathspec,
 			 * Ok, argv[0] looks like a commit/tree; it should not
 			 * be a filename.
 			 */
-			verify_non_filename(prefix, argv[0]);
+			verify_non_filename(prefix, user_input);
 			rev = *argv++;
 		} else {
+			/* We were treating "-" as a commit and not a file */
+			if (substituted_minus)
+				argv[0] = "-";
 			/* Otherwise we treat this as a filename */
 			verify_filename(prefix, argv[0], 1);
 		}
-- 
2.3.1.278.ge5c7b1f.dirty

[PATCH v3 2/2] t7102: add 'reset -' tests

[Sudhanshu Shekhar]

Add following test cases:

1) Confirm error message when git reset is used with no previous branch
2) Confirm git reset - works like git reset @{-1}
3) Confirm "-" is always treated as a commit unless the -- file option is specified
4) Confirm "git reset -" works normally even when a file named @{-1} is present

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Helped-by: David Aguilar <davvid@gmail.com>
Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
First of all, thank you for being so incredibly patient and helpful. I am very grateful for your remarks and reviews. I have implemented your suggestions in this patch. Please let me know if I have missed out on anything else. Also, sorry for sending PATCH 1/2 on the wrong thread, I entered the Message-ID incorrectly (still getting used to send-email :/ ).

Regards,
Sudhanshu

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

diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 98bcfe2..d3a5874 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,4 +568,163 @@ test_expect_success 'reset --mixed sets up work tree' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reset - with no previous branch fails' '
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		test_must_fail git reset - 2>actual
+	) &&
+	test_i18ngrep "bad flag" no_previous/actual
+'
+
+test_expect_success 'reset - while having file named - and no previous branch fails' '
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		>- &&
+		test_must_fail git reset - 2>actual
+	) &&
+	test_i18ngrep "bad flag" no_previous/actual
+'
+
+test_expect_success \
+	'reset - in the presence of file named - with previous branch resets commit' '
+	cat >expect <<-\EOF
+	Unstaged changes after reset:
+	M	-
+	M	file
+	EOF &&
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		>- &&
+		>file &&
+		git add file - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >- &&
+		echo "wow" >file &&
+		git add file - &&
+		git reset - >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'reset - in the presence of file named - with -- option resets commit' '
+	cat >expect <<-\EOF
+	Unstaged changes after reset:
+	M	-
+	M	file
+	EOF &&
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		>- &&
+		>file &&
+		git add file - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >- &&
+		echo "wow" >file &&
+		git add file - &&
+		git reset - -- >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'reset - in the presence of file named - with -- file option resets file' '
+	cat >expect <<-\EOF
+	Unstaged changes after reset:
+	M	-
+	M	file
+	EOF &&
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		>- &&
+		>file &&
+		git add file - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >- &&
+		echo "wow" >file &&
+		git add file - &&
+		git reset -- - >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success \
+	'reset - in the presence of file named - with both pre and post -- option resets file' '
+	cat >expect <<-\EOF
+	Unstaged changes after reset:" >expect &&
+	M	-
+	EOF &&
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		>- &&
+		>file &&
+		git add file - &&
+		git commit -m "add base files" &&
+		git checkout -b new_branch &&
+		echo "random" >- &&
+		echo "wow" >file &&
+		git add file - &&
+		git reset - -- - >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'reset - works same as reset @{-1}' '
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		echo "file1" >file1 &&
+		git add file1 &&
+		git commit -m "base commit" &&
+		git checkout -b temp &&
+		echo "new file" >file &&
+		git add file &&
+		git commit -m "added file" &&
+		git reset - &&
+		git status --porcelain >../actual &&
+		git add file &&
+		git commit -m "added file" &&
+		git reset @{-1} &&
+		git status --porcelain >../expect
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'reset - with file named @{-1} succeeds' '
+	cat >expect <<EOF
+	Unstaged changes after reset:
+	M	file
+	M	@{-1}
+	EOF &&
+	git init no_previous &&
+	test_when_finished rm -rf no_previous &&
+	(
+		cd no_previous &&
+		echo "random" >@{-1} &&
+		echo "random" >file &&
+		git add @{-1} file &&
+		git commit -m "base commit" &&
+		git checkout -b new_branch &&
+		echo "additional stuff" >>file &&
+		echo "additional stuff" >>@{-1} &&
+		git add file @{-1} &&
+		git reset - >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.3.1.278.ge5c7b1f.dirty

[PATCH v3 1/2] reset: enable '-' short-hand for previous branch

[Sudhanshu Shekhar]

git reset -' will reset to the previous branch. It will behave similar
to @{-1} except when a file named '@{-1}' is present. To refer to a file
named '-', use ./- or the -- flag.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
Eric, I have added a user_input variable to record the input entered by the user. This way I can avoid the multiple 'if' clauses. Thank you for the suggestion.
I have also removed the unrelated change that I had unintentionally committed. I am sending this patch on the thread for further review. Once both the patches are reviewed and accepted, I will create a new mail for it. Hope that is okay.

Regards,
Sudhanshu

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

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..b428241 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -192,6 +192,8 @@ static void parse_args(struct pathspec *pathspec,
 {
 	const char *rev = "HEAD";
 	unsigned char unused[20];
+	int substituted_minus = 0;
+	char *user_input = argv[0];
 	/*
 	 * Possible arguments are:
 	 *
@@ -205,6 +207,10 @@ static void parse_args(struct pathspec *pathspec,
 	 */
 
 	if (argv[0]) {
+		if (!strcmp(argv[0], "-")) {
+			argv[0] = "@{-1}";
+			substituted_minus = 1;
+		}
 		if (!strcmp(argv[0], "--")) {
 			argv++; /* reset to HEAD, possibly with paths */
 		} else if (argv[1] && !strcmp(argv[1], "--")) {
@@ -222,9 +228,12 @@ static void parse_args(struct pathspec *pathspec,
 			 * Ok, argv[0] looks like a commit/tree; it should not
 			 * be a filename.
 			 */
-			verify_non_filename(prefix, argv[0]);
+			verify_non_filename(prefix, user_input);
 			rev = *argv++;
 		} else {
+			/* We were treating "-" as a commit and not a file */
+			if (substituted_minus)
+				argv[0] = "-";
 			/* Otherwise we treat this as a filename */
 			verify_filename(prefix, argv[0], 1);
 		}
-- 
2.3.1.278.ge5c7b1f.dirty

Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch

[Eric Sunshine]

On Tue, Mar 10, 2015 at 6:03 PM, Sudhanshu Shekhar
<sudshekhar02@gmail.com> wrote:
> [PATCH v3 1/2] reset: enable '-' short-hand for previous branch

This should be v4, I think, not v3.

> git reset -' will reset to the previous branch. It will behave similar
> to @{-1} except when a file named '@{-1}' is present.

The way this is phrased, the reader is left hanging: "What happens
when a file named @{-1} is present?"

> To refer to a file named '-', use ./- or the -- flag.

A documentation update to reflect this change would be appropriate.
See, for instance, 696acf45 (checkout: implement "-" abbreviation, add
docs and tests;  2009-01-17).

> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---
> Eric, I have added a user_input variable to record the input entered
> by the user. This way I can avoid the multiple 'if' clauses. Thank you
> for the suggestion.
>
> I have also removed the unrelated change that I had unintentionally
> committed. I am sending this patch on the thread for further review.
> Once both the patches are reviewed and accepted, I will create a new
> mail for it. Hope that is okay.

Please wrap commentary to about 72 columns, just as you would the
commit message, rather than typing it all on a single line. (I wrapped
it manually here in order to reply to it.)

> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..b428241 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -192,6 +192,8 @@ static void parse_args(struct pathspec *pathspec,
>  {
>         const char *rev = "HEAD";
>         unsigned char unused[20];
> +       int substituted_minus = 0;
> +       char *user_input = argv[0];

You're assigning a 'const char *' to a 'char *'. The compiler should
have warned about it.

This variable name is not as expressive as it could be. Try to name
the variable after what you expect it to represent, for instance
"maybe_rev" or "rev_or_path" or something more suitable. Even
"orig_argv0" would be more informative than "user_input".

>         /*
>          * Possible arguments are:
>          *
> @@ -205,6 +207,10 @@ static void parse_args(struct pathspec *pathspec,
>          */
>
>         if (argv[0]) {
> +               if (!strcmp(argv[0], "-")) {
> +                       argv[0] = "@{-1}";
> +                       substituted_minus = 1;
> +               }
>                 if (!strcmp(argv[0], "--")) {
>                         argv++; /* reset to HEAD, possibly with paths */
>                 } else if (argv[1] && !strcmp(argv[1], "--")) {
> @@ -222,9 +228,12 @@ static void parse_args(struct pathspec *pathspec,
>                          * Ok, argv[0] looks like a commit/tree; it should not
>                          * be a filename.
>                          */
> -                       verify_non_filename(prefix, argv[0]);
> +                       verify_non_filename(prefix, user_input);
>                         rev = *argv++;
>                 } else {
> +                       /* We were treating "-" as a commit and not a file */
> +                       if (substituted_minus)
> +                               argv[0] = "-";

In my review of the previous version[1], I mentioned that it was ugly
to muck with argv[0]; moreover that it was particularly ugly to have
to muck with it multiple times, undoing and redoing assignments.
Although you've eliminated some reassignments via 'user_input', my
intent, by asking if you could rework the logic, was to prompt you to
take a couple steps back and examine the overall logic of the function
more closely. The munging of argv[0] is effectively a fragile and
undesirable band-aid approach. It is possible to support '-' as an
alias for '@{-1}' without having to resort to such kludges at all.

One other concern: When there is no previous branch, and no file named
"-", then 'git reset -' misleadingly complains "bad flag '-' used
after filename", rather than the more appropriate "ambiguous argument
'-': unknown revision or path".

[1]: http://article.gmane.org/gmane.comp.version-control.git/265255

>                         /* Otherwise we treat this as a filename */
>                         verify_filename(prefix, argv[0], 1);
>                 }
> --
> 2.3.1.278.ge5c7b1f.dirty

Re: [PATCH v3 2/2] t7102: add 'reset -' tests

[Eric Sunshine]

On Tue, Mar 10, 2015 at 6:10 PM, Sudhanshu Shekhar
<sudshekhar02@gmail.com> wrote:
> Add following test cases:
>
> 1) Confirm error message when git reset is used with no previous branch
> 2) Confirm git reset - works like git reset @{-1}
> 3) Confirm "-" is always treated as a commit unless the -- file option is specified
> 4) Confirm "git reset -" works normally even when a file named @{-1} is present

Does it concern you that all new tests pass except the last one even
when patch 1/2 is not applied? I would have expected most or all of
these tests to fail without patch 1/2.

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Helped-by: David Aguilar <davvid@gmail.com>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..d3a5874 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,163 @@ test_expect_success 'reset --mixed sets up work tree' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'reset - with no previous branch fails' '
> +       git init no_previous &&

I understand the intention of the name "no_previous" in tests for
which there is no @{-1}, however...

> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               test_must_fail git reset - 2>actual
> +       ) &&
> +       test_i18ngrep "bad flag" no_previous/actual
> +'
> +
> +test_expect_success 'reset - while having file named - and no previous branch fails' '
> +       git init no_previous &&
> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               >- &&
> +               test_must_fail git reset - 2>actual
> +       ) &&
> +       test_i18ngrep "bad flag" no_previous/actual
> +'
> +
> +test_expect_success \
> +       'reset - in the presence of file named - with previous branch resets commit' '
> +       cat >expect <<-\EOF
> +       Unstaged changes after reset:
> +       M       -
> +       M       file
> +       EOF &&
> +       git init no_previous &&

I don't understand the name "no_previous" in tests for which @{-1} can
resolve. It probably would be better to choose a more generic name and
use it for all these tests. For instance, "resetdash" or just "dash"
or something better.

> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               >- &&
> +               >file &&
> +               git add file - &&
> +               git commit -m "add base files" &&
> +               git checkout -b new_branch &&
> +               echo "random" >- &&
> +               echo "wow" >file &&
> +               git add file - &&
> +               git reset - >../actual
> +       ) &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'reset - with file named @{-1} succeeds' '

I may be missing something obvious, but why is it necessary to single
out a file named @{-1} at all, particuarly when there are so many
other ways to specify revisions, and there may be files mirroring
those spellings, as well?

> +       cat >expect <<EOF
> +       Unstaged changes after reset:
> +       M       file
> +       M       @{-1}
> +       EOF &&
> +       git init no_previous &&
> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               echo "random" >@{-1} &&
> +               echo "random" >file &&
> +               git add @{-1} file &&
> +               git commit -m "base commit" &&
> +               git checkout -b new_branch &&
> +               echo "additional stuff" >>file &&
> +               echo "additional stuff" >>@{-1} &&
> +               git add file @{-1} &&
> +               git reset - >../actual
> +       ) &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.3.1.278.ge5c7b1f.dirty