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

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

[Sudhanshu Shekhar]

git reset '-' will reset to the previous branch. To reset a file named
"-" use either "git reset ./-" or "git reset -- -".

Change error message to treat single "-" as an ambigous revision or
path rather than a bad 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>
---
I have changed the logic to ensure argv[0] isn't changed at any point.
Creating a modified_argv0 variable let's me do that.

I couldn't think of any other way to achieve this, apart from changing things
directly in the sha1_name.c file (like Junio's changes). Please let me know
if some further changes are needed.

 builtin/reset.c | 17 +++++++++++++----
 setup.c         |  2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..bc50e14 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];
+	const char *modified_argv0 = argv[0];
 	/*
 	 * Possible arguments are:
 	 *
@@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,
 	 */
 
 	if (argv[0]) {
+		if (!strcmp(argv[0], "-")) {
+			modified_argv0 = "@{-1}";
+		}
+		else {
+			modified_argv0 = argv[0];
+		}
+
 		if (!strcmp(argv[0], "--")) {
 			argv++; /* reset to HEAD, possibly with paths */
 		} else if (argv[1] && !strcmp(argv[1], "--")) {
-			rev = argv[0];
+			rev = modified_argv0;
 			argv += 2;
 		}
 		/*
@@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,
 		 * has to be unambiguous. If there is a single argument, it
 		 * can not be a tree
 		 */
-		else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||
-			 (argv[1] && !get_sha1_treeish(argv[0], unused))) {
+		else if ((!argv[1] && !get_sha1_committish(modified_argv0, unused)) ||
+			 (argv[1] && !get_sha1_treeish(modified_argv0, unused))) {
 			/*
 			 * Ok, argv[0] looks like a commit/tree; it should not
 			 * be a filename.
 			 */
 			verify_non_filename(prefix, argv[0]);
-			rev = *argv++;
+			rev = modified_argv0;
+			argv++;
 		} else {
 			/* Otherwise we treat this as a filename */
 			verify_filename(prefix, argv[0], 1);
diff --git a/setup.c b/setup.c
index 979b13f..b621b62 100644
--- a/setup.c
+++ b/setup.c
@@ -200,7 +200,7 @@ void verify_filename(const char *prefix,
 		     int diagnose_misspelt_rev)
 {
 	if (*arg == '-')
-		die("bad flag '%s' used after filename", arg);
+		die("ambiguous argument '%s': unknown revision or path", arg);
 	if (check_filename(prefix, arg))
 		return;
 	die_verify_filename(prefix, arg, diagnose_misspelt_rev);
-- 
2.3.1.277.gd67f9d5.dirty

[PATCH v5 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>
---
Eric: Thank you for pointing out the mistake. The '&&' after the Here Docs
was causing the issue. I have removed the concatenation from there, hope
that's okay.
Regarding the @{-1} test case, I created it as a check for Junio's comment
on the error message generated by "git reset -" when a file named @{-1} is there.
Since, in this situation "git reset @{-1}" will return an error (but "reset -"
shouldn't).
I have renamed the folder to 'dash' as suggested by you, keeping the old name only
where it made sense.
 t/t7102-reset.sh | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)

diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 98bcfe2..18523c1 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,4 +568,162 @@ 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 "ambiguous argument" 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 "ambiguous argument" 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 dash &&
+	test_when_finished rm -rf dash &&
+	(
+		cd dash &&
+		>- &&
+		>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 dash &&
+	test_when_finished rm -rf dash &&
+	(
+		cd dash &&
+		>- &&
+		>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	-
+	EOF
+	git init dash &&
+	test_when_finished rm -rf dash &&
+	(
+		cd dash &&
+		>- &&
+		>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:
+	M	-
+	EOF
+	git init dash &&
+	test_when_finished rm -rf dash &&
+	(
+		cd dash &&
+		>- &&
+		>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 dash &&
+	test_when_finished rm -rf dash &&
+	(
+		cd dash &&
+		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	@{-1}
+	M	file
+	EOF
+	git init dash &&
+	test_when_finished rm -rf dash &&
+	(
+		cd dash &&
+		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.277.gd67f9d5.dirty

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

[Eric Sunshine]

On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
<sudshekhar02@gmail.com> wrote:
> git reset '-' will reset to the previous branch. To reset a file named
> "-" use either "git reset ./-" or "git reset -- -".
>
> Change error message to treat single "-" as an ambigous revision or
> path rather than a bad 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>
> ---
> I have changed the logic to ensure argv[0] isn't changed at any point.
> Creating a modified_argv0 variable let's me do that.
>
> I couldn't think of any other way to achieve this, apart from changing things
> directly in the sha1_name.c file (like Junio's changes). Please let me know
> if some further changes are needed.
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..bc50e14 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];
> +       const char *modified_argv0 = argv[0];

This variable is only needed inside the 'if (argv[0])' block below, so
its declaration should be moved there. Also the initialization to
argv[0] is wasted since the assignment is overwritten below.

The variable name itself could be better. Unlike a name such as
'orig_arg0', "modified" doesn't tell us much. Modified how? Modified
to be what? Consideration where and how the variable is used, we can
see that it will be holding a value that _might_ be a "rev". This
suggests a better name such as 'maybe_rev' or something similar.

>         /*
>          * Possible arguments are:
>          *
> @@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,
>          */
>
>         if (argv[0]) {
> +               if (!strcmp(argv[0], "-")) {
> +                       modified_argv0 = "@{-1}";
> +               }
> +               else {

Style: cuddle the 'else' and braces: "} else {"

> +                       modified_argv0 = argv[0];
> +               }

The unnecessary braces make this harder to read than it could be since
it is so spread out vertically. Dropping the braces would help. The
ternary operator ?: might improve readability (though it might also
make it worse).

>                 if (!strcmp(argv[0], "--")) {
>                         argv++; /* reset to HEAD, possibly with paths */
>                 } else if (argv[1] && !strcmp(argv[1], "--")) {
> -                       rev = argv[0];
> +                       rev = modified_argv0;
>                         argv += 2;
>                 }
>                 /*
> @@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,
>                  * has to be unambiguous. If there is a single argument, it
>                  * can not be a tree
>                  */
> -               else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||
> -                        (argv[1] && !get_sha1_treeish(argv[0], unused))) {
> +               else if ((!argv[1] && !get_sha1_committish(modified_argv0, unused)) ||
> +                        (argv[1] && !get_sha1_treeish(modified_argv0, unused))) {
>                         /*
>                          * Ok, argv[0] looks like a commit/tree; it should not
>                          * be a filename.
>                          */
>                         verify_non_filename(prefix, argv[0]);
> -                       rev = *argv++;
> +                       rev = modified_argv0;
> +                       argv++;

Good. This is much better than the previous rounds, and is the sort of
solution I had hoped to see when prodding you in previous reviews to
avoid argv[] kludges. Unlike the previous band-aid approach, this
demonstrates that you took the time to understand the overall logic
flow rather than merely plopping in a "quick fix".

>                 } else {
>                         /* Otherwise we treat this as a filename */
>                         verify_filename(prefix, argv[0], 1);
> diff --git a/setup.c b/setup.c
> index 979b13f..b621b62 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -200,7 +200,7 @@ void verify_filename(const char *prefix,
>                      int diagnose_misspelt_rev)
>  {
>         if (*arg == '-')
> -               die("bad flag '%s' used after filename", arg);
> +               die("ambiguous argument '%s': unknown revision or path", arg);

The conditional is only checking if the first character of 'arg' is
hyphen; it's not checking if 'arg' is exactly "-". It's purpose is to
recognize -flags or --flags, so it's inappropriate to change the error
message like this. I think this also doesn't help the case when there
really is a file named "-", since this conditional will just claim
that it's ambiguous.

It might or might not be appropriate to add a special case here to
allow an exact "-" to fall through to the check_filename() call below,
however, it would be necessary to thoroughly check for possible
repercussions first. (I haven't checked.) If all else fails, you could
change parse_args() to do something a bit ugly like this:

    const char *f = strcmp(argv[0], "-") ? argv[0] : "./-";
    verify_filename(prefix, f);

>         if (check_filename(prefix, arg))
>                 return;
>         die_verify_filename(prefix, arg, diagnose_misspelt_rev);
> --

By now, you've had a taste of what it's like to participate in the Git
project and what will be expected of you, and GSoC mentors have
(hopefully) formed an opinion of your abilities and how you interact
with reviewers, so I'm not sure that it makes sense for you to
resubmit again. Junio's proposal[1] to generalize "-" recognition as
an alias for @{-1} may be worth pursing by someone, but may be too
large for a micro-project.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/265260

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

[Eric Sunshine]

On Fri, Mar 13, 2015 at 2:18 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
>
> 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>
> ---
> Eric: Thank you for pointing out the mistake. The '&&' after the Here
> Docs was causing the issue. I have removed the concatenation from
> there, hope that's okay.

The && needs to go on the first line, not the last line of the here-doc.

However, that was not my main concern in the previous review. What
disturbed me was that the new tests, which were supposed to be
checking if "-" behaved as @{-1}, were succeeding even without patch
1/2 applied which implemented the "-" alias for @{-1}. That seems
wrong. I don't think you particularly addressed that issue in this
version (even though the first couple tests will now fail without 1/2
due to the changed error message).

> Regarding the @{-1} test case, I created it as a check for Junio's
> comment on the error message generated by "git reset -" when a file
> named @{-1} is there.  Since, in this situation "git reset @{-1}" will
> return an error (but "reset -" shouldn't).

Reminder: Wrap commentary to about column 72, as you would the commit
message. (I re-wrapped it manually to reply to it.)

> I have renamed the folder to 'dash' as suggested by you, keeping the
> old name only where it made sense.

Considering that the test titles already tell us the intent of the
tests, I don't find that the directory name "no_previous" adds much
value to tests checking the behavior of "-" with no previous branch. A
single, consistent name used throughout all these tests would be less
surprising and place smaller cognitive load on the reader.

More below.

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..18523c1 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,162 @@ 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 "ambiguous argument" 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 "ambiguous argument" no_previous/actual
> +'
> +
> +

Style: Unnecessary extra blank line.

> +test_expect_success \
> +       'reset - in the presence of file named - with previous branch resets commit' '
> +       cat >expect <<-EOF

Place the && at the end of this line. Also, prefix EOF with a
backslash to indicate that you don't intend any interpolation to occur
within the here-doc. So:

    cat >expect <<-\EOF &&

Ditto for the remaining tests.

> +       Unstaged changes after reset:
> +       M       -
> +       M       file
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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 dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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       -
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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:
> +       M       -
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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 dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               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       @{-1}
> +       M       file
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               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.277.gd67f9d5.dirty

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

[Sudhanshu Shekhar]

Hi,
On Sat, Mar 14, 2015 at 2:40 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Mar 13, 2015 at 2:18 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
>>
>> 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>
>> ---
>> Eric: Thank you for pointing out the mistake. The '&&' after the Here
>> Docs was causing the issue. I have removed the concatenation from
>> there, hope that's okay.
>
> The && needs to go on the first line, not the last line of the here-doc.
>
> However, that was not my main concern in the previous review. What
> disturbed me was that the new tests, which were supposed to be
> checking if "-" behaved as @{-1}, were succeeding even without patch
> 1/2 applied which implemented the "-" alias for @{-1}. That seems
> wrong. I don't think you particularly addressed that issue in this
> version (even though the first couple tests will now fail without 1/2
> due to the changed error message).

Actually, The issue was caused due a HERE docs error. If you run this
patch now, you will see that 7 out of the 8 test cases fail.

>
>> Regarding the @{-1} test case, I created it as a check for Junio's
>> comment on the error message generated by "git reset -" when a file
>> named @{-1} is there.  Since, in this situation "git reset @{-1}" will
>> return an error (but "reset -" shouldn't).
>
> Reminder: Wrap commentary to about column 72, as you would the commit
> message. (I re-wrapped it manually to reply to it.)
>
>> I have renamed the folder to 'dash' as suggested by you, keeping the
>> old name only where it made sense.
>
> Considering that the test titles already tell us the intent of the
> tests, I don't find that the directory name "no_previous" adds much
> value to tests checking the behavior of "-" with no previous branch. A
> single, consistent name used throughout all these tests would be less
> surprising and place smaller cognitive load on the reader.
>
> More below.
>
>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
>> index 98bcfe2..18523c1 100755
>> --- a/t/t7102-reset.sh
>> +++ b/t/t7102-reset.sh
>> @@ -568,4 +568,162 @@ 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 "ambiguous argument" 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 "ambiguous argument" no_previous/actual
>> +'
>> +
>> +
>
> Style: Unnecessary extra blank line.
>
>> +test_expect_success \
>> +       'reset - in the presence of file named - with previous branch resets commit' '
>> +       cat >expect <<-EOF
>
> Place the && at the end of this line. Also, prefix EOF with a
> backslash to indicate that you don't intend any interpolation to occur
> within the here-doc. So:
>
>     cat >expect <<-\EOF &&
>
> Ditto for the remaining tests.
>
Thank you for taking the time out to point out these style changes to
me. There is a lot I have to learn about open source contribution yet
and I believe during the course of this microproject, I did learn
something about this (By making some very silly mistakes). Thank you
to all the developers who took time out to review my code and point
out the mistakes I had done.

Regards,
Sudhanshu

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

[Sudhanshu Shekhar]

Thank you all for your reviews and feedback. I will try and submit
this patch by taking my time to think about the various solutions and
coming up with the best one. I will also try and see if I can assist
Junio with his JFF patch.

I have learned a lot during the process of implementing this micro
project and believe that if I get selected for a gsoc under git, it
will help me become a better developer overall.

I have gone over the ideas page and I am interested in the "git bisect
--first-parent" and "git bisect fixed/unfixed" projects. I am looking
the source code at git-bisect.sh and will try and come up with a
proposal for review by the git community.

Thank you all for you time and patience.

Regards,
Sudhanshu

On Sat, Mar 14, 2015 at 2:18 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
> <sudshekhar02@gmail.com> wrote:
>> git reset '-' will reset to the previous branch. To reset a file named
>> "-" use either "git reset ./-" or "git reset -- -".
>>
>> Change error message to treat single "-" as an ambigous revision or
>> path rather than a bad 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>
>> ---
>> I have changed the logic to ensure argv[0] isn't changed at any point.
>> Creating a modified_argv0 variable let's me do that.
>>
>> I couldn't think of any other way to achieve this, apart from changing things
>> directly in the sha1_name.c file (like Junio's changes). Please let me know
>> if some further changes are needed.
>>
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index 4c08ddc..bc50e14 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];
>> +       const char *modified_argv0 = argv[0];
>
> This variable is only needed inside the 'if (argv[0])' block below, so
> its declaration should be moved there. Also the initialization to
> argv[0] is wasted since the assignment is overwritten below.
>
> The variable name itself could be better. Unlike a name such as
> 'orig_arg0', "modified" doesn't tell us much. Modified how? Modified
> to be what? Consideration where and how the variable is used, we can
> see that it will be holding a value that _might_ be a "rev". This
> suggests a better name such as 'maybe_rev' or something similar.
>
>>         /*
>>          * Possible arguments are:
>>          *
>> @@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,
>>          */
>>
>>         if (argv[0]) {
>> +               if (!strcmp(argv[0], "-")) {
>> +                       modified_argv0 = "@{-1}";
>> +               }
>> +               else {
>
> Style: cuddle the 'else' and braces: "} else {"
>
>> +                       modified_argv0 = argv[0];
>> +               }
>
> The unnecessary braces make this harder to read than it could be since
> it is so spread out vertically. Dropping the braces would help. The
> ternary operator ?: might improve readability (though it might also
> make it worse).
>
>>                 if (!strcmp(argv[0], "--")) {
>>                         argv++; /* reset to HEAD, possibly with paths */
>>                 } else if (argv[1] && !strcmp(argv[1], "--")) {
>> -                       rev = argv[0];
>> +                       rev = modified_argv0;
>>                         argv += 2;
>>                 }
>>                 /*
>> @@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,
>>                  * has to be unambiguous. If there is a single argument, it
>>                  * can not be a tree
>>                  */
>> -               else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||
>> -                        (argv[1] && !get_sha1_treeish(argv[0], unused))) {
>> +               else if ((!argv[1] && !get_sha1_committish(modified_argv0, unused)) ||
>> +                        (argv[1] && !get_sha1_treeish(modified_argv0, unused))) {
>>                         /*
>>                          * Ok, argv[0] looks like a commit/tree; it should not
>>                          * be a filename.
>>                          */
>>                         verify_non_filename(prefix, argv[0]);
>> -                       rev = *argv++;
>> +                       rev = modified_argv0;
>> +                       argv++;
>
> Good. This is much better than the previous rounds, and is the sort of
> solution I had hoped to see when prodding you in previous reviews to
> avoid argv[] kludges. Unlike the previous band-aid approach, this
> demonstrates that you took the time to understand the overall logic
> flow rather than merely plopping in a "quick fix".
>
>>                 } else {
>>                         /* Otherwise we treat this as a filename */
>>                         verify_filename(prefix, argv[0], 1);
>> diff --git a/setup.c b/setup.c
>> index 979b13f..b621b62 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -200,7 +200,7 @@ void verify_filename(const char *prefix,
>>                      int diagnose_misspelt_rev)
>>  {
>>         if (*arg == '-')
>> -               die("bad flag '%s' used after filename", arg);
>> +               die("ambiguous argument '%s': unknown revision or path", arg);
>
> The conditional is only checking if the first character of 'arg' is
> hyphen; it's not checking if 'arg' is exactly "-". It's purpose is to
> recognize -flags or --flags, so it's inappropriate to change the error
> message like this. I think this also doesn't help the case when there
> really is a file named "-", since this conditional will just claim
> that it's ambiguous.
>
> It might or might not be appropriate to add a special case here to
> allow an exact "-" to fall through to the check_filename() call below,
> however, it would be necessary to thoroughly check for possible
> repercussions first. (I haven't checked.) If all else fails, you could
> change parse_args() to do something a bit ugly like this:
>
>     const char *f = strcmp(argv[0], "-") ? argv[0] : "./-";
>     verify_filename(prefix, f);
>
>>         if (check_filename(prefix, arg))
>>                 return;
>>         die_verify_filename(prefix, arg, diagnose_misspelt_rev);
>> --
>
> By now, you've had a taste of what it's like to participate in the Git
> project and what will be expected of you, and GSoC mentors have
> (hopefully) formed an opinion of your abilities and how you interact
> with reviewers, so I'm not sure that it makes sense for you to
> resubmit again. Junio's proposal[1] to generalize "-" recognition as
> an alias for @{-1} may be worth pursing by someone, but may be too
> large for a micro-project.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/265260