git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] reset: allow "-" short hand for previous commit
@ 2015-03-03 20:51 Sudhanshu Shekhar
  2015-03-03 22:10 ` Matthieu Moy
  0 siblings, 1 reply; 12+ messages in thread
From: Sudhanshu Shekhar @ 2015-03-03 20:51 UTC (permalink / raw)
  To: git; +Cc: SudShekhar

From: SudShekhar <sudshekhar02@gmail.com>

Teach reset the same shorthand as checkout and merge. "-" means the
"previous commit".

Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
This is done as a microproject for gsoc purposes. I am looking forward to your feedback/comments. Thanks
builtin/reset.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..3e0378b 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -205,6 +205,8 @@ static void parse_args(struct pathspec *pathspec,
 	 */
 
 	if (argv[0]) {
+		if(!strcmp(argv[0],"-"))
+			argv[0]="@{-1}";
 		if (!strcmp(argv[0], "--")) {
 			argv++; /* reset to HEAD, possibly with paths */
 		} else if (argv[1] && !strcmp(argv[1], "--")) {
-- 
2.3.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] reset: allow "-" short hand for previous commit
  2015-03-03 20:51 [PATCH] reset: allow "-" short hand for previous commit Sudhanshu Shekhar
@ 2015-03-03 22:10 ` Matthieu Moy
  2015-03-03 23:17   ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Matthieu Moy @ 2015-03-03 22:10 UTC (permalink / raw)
  To: Sudhanshu Shekhar; +Cc: git

Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:

> From: SudShekhar <sudshekhar02@gmail.com>

Please, set your configuration to have the same identity for commit and
send-email. It seems your commiter ID (user.name) does not contain your
last name.

> builtin/reset.c | 2 ++

Doesn't this deserve a test?

+		if(!strcmp(argv[0],"-"))
+			argv[0]="@{-1}";

Wrong spacing (around = and after ,).

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] reset: allow "-" short hand for previous commit
  2015-03-03 22:10 ` Matthieu Moy
@ 2015-03-03 23:17   ` Junio C Hamano
  2015-03-04  7:07     ` Sudhanshu Shekhar
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2015-03-03 23:17 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Sudhanshu Shekhar, git

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:
>
>> From: SudShekhar <sudshekhar02@gmail.com>
>
> Please, set your configuration to have the same identity for commit and
> send-email. It seems your commiter ID (user.name) does not contain your
> last name.

Actually, the token does not match either of the two names; it looks
like two names smashed together into a single nickname token.

>> builtin/reset.c | 2 ++
>
> Doesn't this deserve a test?
>
> +		if(!strcmp(argv[0],"-"))
> +			argv[0]="@{-1}";
>
> Wrong spacing (around = and after ,).

What should worry us even more is what the user would get when @{-1}
does not resolve to something the command can use.  It would be bad
if we give an error message with @{-1} in it that the user never
typed (and may not even understand what it means).

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] reset: allow "-" short hand for previous commit
  2015-03-03 23:17   ` Junio C Hamano
@ 2015-03-04  7:07     ` Sudhanshu Shekhar
  2015-03-04  7:09       ` Sudhanshu Shekhar
                         ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Sudhanshu Shekhar @ 2015-03-04  7:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matthieu Moy, git

Hi,

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:
>>
>>> From: SudShekhar <sudshekhar02@gmail.com>
>>
>> Please, set your configuration to have the same identity for commit and
>> send-email. It seems your commiter ID (user.name) does not contain your
>> last name.
>
> Actually, the token does not match either of the two names; it looks
> like two names smashed together into a single nickname token.

Sorry, I had set my irc nick as the commiter id. I have changed this to my name.

>>> builtin/reset.c | 2 ++
>>
>> Doesn't this deserve a test?
>>
>> +             if(!strcmp(argv[0],"-"))
>> +                     argv[0]="@{-1}";
>>
>> Wrong spacing (around = and after ,).

Thanks for pointing this out. I have corrected it.

> What should worry us even more is what the user would get when @{-1}
> does not resolve to something the command can use.  It would be bad
> if we give an error message with @{-1} in it that the user never
> typed (and may not even understand what it means).


I apologize for having overlooked this use case. I have made some
changes and I think this should work now. I will email the updated
patch in the next email (I hope this is the correct procedure). Please
let me know your thoughts.
Also, this use case hasn't been handled in the case of git merge.

Another thing, can someone guide me regarding the right place to add a
test case? Should it be t7102-reset.sh or some other file?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] reset: allow "-" short hand for previous commit
  2015-03-04  7:07     ` Sudhanshu Shekhar
@ 2015-03-04  7:09       ` Sudhanshu Shekhar
  2015-03-04  7:10       ` Eric Sunshine
  2015-03-05  0:34       ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Sudhanshu Shekhar @ 2015-03-04  7:09 UTC (permalink / raw)
  To: git; +Cc: Sudhanshu Shekhar

Teach reset the same shorthand as checkout and merge. "-" means the
"previous commit".

Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
 builtin/reset.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..9f8967d 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], "--")) {
@@ -225,12 +230,14 @@ static void parse_args(struct pathspec *pathspec,
 			verify_non_filename(prefix, argv[0]);
 			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.168.gfd4bc34.dirty

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] reset: allow "-" short hand for previous commit
  2015-03-04  7:07     ` Sudhanshu Shekhar
  2015-03-04  7:09       ` Sudhanshu Shekhar
@ 2015-03-04  7:10       ` Eric Sunshine
  2015-03-05  0:34       ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Eric Sunshine @ 2015-03-04  7:10 UTC (permalink / raw)
  To: Sudhanshu Shekhar; +Cc: Junio C Hamano, Matthieu Moy, Git List

On Wed, Mar 4, 2015 at 2:07 AM, Sudhanshu Shekhar
<sudshekhar02@gmail.com> wrote:
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>> Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:
>>>> From: SudShekhar <sudshekhar02@gmail.com>
>>>
>>> +             if(!strcmp(argv[0],"-"))
>>> +                     argv[0]="@{-1}";
>>>
>>> Wrong spacing (around = and after ,).
>
> Thanks for pointing this out. I have corrected it.

Also, add a space after 'if'.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] reset: allow "-" short hand for previous commit
  2015-03-04  7:07     ` Sudhanshu Shekhar
  2015-03-04  7:09       ` Sudhanshu Shekhar
  2015-03-04  7:10       ` Eric Sunshine
@ 2015-03-05  0:34       ` Junio C Hamano
  2015-03-07 21:04         ` [PATCH 1/2] " Sudhanshu Shekhar
  2 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2015-03-05  0:34 UTC (permalink / raw)
  To: Sudhanshu Shekhar; +Cc: Matthieu Moy, git

Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:

>> What should worry us even more is what the user would get when @{-1}
>> does not resolve to something the command can use.  It would be bad
>> if we give an error message with @{-1} in it that the user never
>> typed (and may not even understand what it means).
>
> I apologize for having overlooked this use case. 

Thanking is fine, but there is no need to apologize in response to
review comments. We are imperfect humans and review exchanges are
designed to allow us cover points each of us missed in our
collective effort to make Git better.

> Another thing, can someone guide me regarding the right place to add a
> test case? Should it be t7102-reset.sh or some other file?

At the end of that file sounds like a reasonable choice.  You would
want to test various cases, such as (1) what happens when there is
no @{-1} at all (you would need a newly initialied test repository,
just like the last test in that file creates mixed_worktree
repository for its own use), (2) what happens when there is, split
into two , i.e. (1-a) @{-1} does not exist but there is a file whose
name is "-"; (1-b) @{-1} does not exist and there is no file whose
name is "-"; (2-a) @{-1} exists but there is a file whose name is
"-"; and (2-b) @{-1} exists and there is no file whose name is "-".

Do not just test (2-b) and declare victory---making sure that a
feature does not trigger when it should not is also important.

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] reset: allow "-" short hand for previous commit
  2015-03-05  0:34       ` Junio C Hamano
@ 2015-03-07 21:04         ` Sudhanshu Shekhar
  2015-03-08 10:33           ` Matthieu Moy
  0 siblings, 1 reply; 12+ messages in thread
From: Sudhanshu Shekhar @ 2015-03-07 21:04 UTC (permalink / raw)
  To: git; +Cc: Matthieu.Moy, gitster

Teach reset the same shorthand as checkout and merge. "-" means the
"previous commit".

Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
 builtin/reset.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..9f8967d 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], "--")) {
@@ -225,12 +230,14 @@ static void parse_args(struct pathspec *pathspec,
 			verify_non_filename(prefix, argv[0]);
 			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.168.g0c82976.dirty


>From 21f0298c17768aaa11ff0a677cdefc8f54ac9515 Mon Sep 17 00:00:00 2001
From: Sudhanshu Shekhar <sudshekhar02@gmail.com>
Date: Sun, 8 Mar 2015 02:13:57 +0530
Subject: [PATCH 2/2] Added test cases for reset -

Four test cases have been added

1) when user does reset - without any previous branch => Leads to error
2) when user does reset - with a previous branch      => Ensure it
behaves like @{-1}

Other two deal with the situation when we have a file named '-'. We
ignore such a file and - is always treated either as a previous branch
or a bad filename. Users who wish to reset a file named '-' should specify
it as './-'
---
 builtin/reset.c  |  4 ++--
 t/t7102-reset.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 9f8967d..02f33ef 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -206,7 +206,7 @@ static void parse_args(struct pathspec *pathspec,
 	 */
 
 	if (argv[0]) {
-		if(!strcmp(argv[0], "-")) {
+		if (!strcmp(argv[0], "-")) {
 			argv[0] = "@{-1}";
 			substituted_minus = 1;
 		}
@@ -231,7 +231,7 @@ static void parse_args(struct pathspec *pathspec,
 			rev = *argv++;
 		} else {
 			/* We were treating "-" as a commit and not a file */
-			if(substituted_minus)
+			if (substituted_minus)
 				argv[0] = "-";
 			/* Otherwise we treat this as a filename */
 			verify_filename(prefix, argv[0], 1);
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 98bcfe2..4b8d7f5 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,4 +568,66 @@ test_expect_success 'reset --mixed sets up work tree' '
 	test_cmp expect actual
 '
 
+cat > expect << EOF
+fatal: bad flag '-' used after filename
+EOF
+
+test_expect_success 'reset - with no previous branch' '
+	git init no_previous --quiet &&
+	(
+	cd no_previous
+	) &&
+	test_must_fail git reset - 2> output &&
+	test_cmp expect output
+'
+
+test_expect_success 'reset - while having file named - and no previous branch' '
+	git init no_previous --quiet &&
+	(
+	cd no_previous &&
+	touch ./-
+	) &&
+	test_must_fail git reset - 2> output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
+Unstaged changes after reset:
+M	-
+M	1
+EOF
+
+test_expect_success 'reset - in the prescence of file named - with previou branch' '
+	git init no_previous --quiet &&
+	cd no_previous &&
+	touch ./- 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 &&
+	test_cmp output ../expect
+'
+test_expect_success 'reset - works same as reset @{-1}' '
+	git init no_previous --quiet &&
+	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 > ../first &&
+	git add new-file &&
+	git commit -m "added new-file" &&
+	git reset @{-1} &&
+	git status > ../second &&
+	test_cmp ../first ../second
+'
+
 test_done
-- 
2.3.1.168.g0c82976.dirty

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] reset: allow "-" short hand for previous commit
  2015-03-07 21:04         ` [PATCH 1/2] " Sudhanshu Shekhar
@ 2015-03-08 10:33           ` Matthieu Moy
  2015-03-08 14:58             ` [PATCH 1/2] Teach reset the same short-hand as checkout Sudhanshu Shekhar
  0 siblings, 1 reply; 12+ messages in thread
From: Matthieu Moy @ 2015-03-08 10:33 UTC (permalink / raw)
  To: Sudhanshu Shekhar; +Cc: git, gitster

Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:

> +		if(!strcmp(argv[0], "-")) {
[...]
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 9f8967d..02f33ef 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -206,7 +206,7 @@ static void parse_args(struct pathspec *pathspec,
>  	 */
>  
>  	if (argv[0]) {
> -		if(!strcmp(argv[0], "-")) {
> +		if (!strcmp(argv[0], "-")) {

Please, squash this hunk into the previous patch, so that reviewers get
the right version right away.

Also, send each patch as a separate email (git send-email can do that
for you).

> +	test_must_fail git reset - 2> output &&

Here and elsewhere: no space after >.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] Teach reset the same short-hand as checkout
  2015-03-08 10:33           ` Matthieu Moy
@ 2015-03-08 14:58             ` Sudhanshu Shekhar
  2015-03-08 14:58               ` [PATCH 2/2] Added test cases for git reset - Sudhanshu Shekhar
  0 siblings, 1 reply; 12+ messages in thread
From: Sudhanshu Shekhar @ 2015-03-08 14:58 UTC (permalink / raw)
  To: git; +Cc: Matthieu.Moy, gitster, Sudhanshu Shekhar

"-" now means the previous branch.

Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
Thanks-to: Junio C Hamano, Matthieu Moy, Eric Sunshine
---
Thank you all for your feedback. Please let me know if I am missing out on anything else.


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

diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..02f33ef 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], "--")) {
@@ -225,12 +230,14 @@ static void parse_args(struct pathspec *pathspec,
 			verify_non_filename(prefix, argv[0]);
 			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.168.g0c82976.dirty

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] Added test cases for git reset -
  2015-03-08 14:58             ` [PATCH 1/2] Teach reset the same short-hand as checkout Sudhanshu Shekhar
@ 2015-03-08 14:58               ` Sudhanshu Shekhar
  2015-03-08 18:09                 ` David Aguilar
  0 siblings, 1 reply; 12+ messages in thread
From: Sudhanshu Shekhar @ 2015-03-08 14:58 UTC (permalink / raw)
  To: git; +Cc: Matthieu.Moy, gitster, Sudhanshu Shekhar

Four test cases have been added

1) when user does reset - without any previous branch => Leads to error
2) when user does reset - with a previous branch      => Ensure it
behaves like  <at> {-1}

Other two deal with the situation when we have a file named '-'. We
ignore such a file and - is always treated either as a previous branch
or a bad filename. Users who wish to reset a file named '-' should
specify
it as './-'

Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
---
I have created test cases for git reset -. @Junio, I tried incorporating your suggestions while developing these test cases.
However, since the verify_filename function ignores files starting with "-", git reset - will always refer to the branch only. Kindly let me know your thoughts and views on this and also your reviews about the test cases I have created.

Regards,
Sudhanshu

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

diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 98bcfe2..ade3d6a 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,4 +568,66 @@ test_expect_success 'reset --mixed sets up work tree' '
 	test_cmp expect actual
 '
 
+cat > expect << EOF
+fatal: bad flag '-' used after filename
+EOF
+
+test_expect_success 'reset - with no previous branch' '
+	git init no_previous --quiet &&
+	(
+	cd no_previous
+	) &&
+	test_must_fail git reset - 2>output &&
+	test_cmp expect output
+'
+
+test_expect_success 'reset - while having file named - and no previous branch' '
+	git init no_previous --quiet &&
+	(
+	cd no_previous &&
+	touch ./-
+	) &&
+	test_must_fail git reset - 2>output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
+Unstaged changes after reset:
+M	-
+M	1
+EOF
+
+test_expect_success 'reset - in the prescence of file named - with previou branch' '
+	git init no_previous --quiet &&
+	cd no_previous &&
+	touch ./- 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 &&
+	test_cmp output ../expect
+'
+test_expect_success 'reset - works same as reset @{-1}' '
+	git init no_previous --quiet &&
+	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 >../first &&
+	git add new-file &&
+	git commit -m "added new-file" &&
+	git reset @{-1} &&
+	git status >../second &&
+	test_cmp ../first ../second
+'
+
 test_done
-- 
2.3.1.168.g0c82976.dirty

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Added test cases for git reset -
  2015-03-08 14:58               ` [PATCH 2/2] Added test cases for git reset - Sudhanshu Shekhar
@ 2015-03-08 18:09                 ` David Aguilar
  0 siblings, 0 replies; 12+ messages in thread
From: David Aguilar @ 2015-03-08 18:09 UTC (permalink / raw)
  To: Sudhanshu Shekhar; +Cc: git, Matthieu.Moy, gitster

Hi,

On Sun, Mar 08, 2015 at 08:28:40PM +0530, Sudhanshu Shekhar wrote:
> Four test cases have been added
> 
> 1) when user does reset - without any previous branch => Leads to error
> 2) when user does reset - with a previous branch      => Ensure it
> behaves like  <at> {-1}
> 
> Other two deal with the situation when we have a file named '-'. We
> ignore such a file and - is always treated either as a previous branch
> or a bad filename. Users who wish to reset a file named '-' should
> specify
> it as './-'
> 
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---


Please reword the commit message so that it uses an imperative
mood; see ~line 112 in Documentation/SubmittingPatches for an
explanation.


>  t/t7102-reset.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..ade3d6a 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,66 @@ test_expect_success 'reset --mixed sets up work tree' '
>  	test_cmp expect actual
>  '
>  
> +cat > expect << EOF


Please drop the space after ">" and "<<".


> +fatal: bad flag '-' used after filename
> +EOF
> +
> +test_expect_success 'reset - with no previous branch' '
> +	git init no_previous --quiet &&
> +	(
> +	cd no_previous
> +	) &&
> +	test_must_fail git reset - 2>output &&
> +	test_cmp expect output
> +'


Please indent lines inside the (...) parens so that it's easier
to notice that they are executing within a subshell, e.g.

	git init no_previous --quiet &&
	(
		cd no_previous &&
		...
	) &&
	...

As written, the above test verifies that we can "cd" into
"no_previous", but because it's a subshell the subsequent
commands after the parens will not be run within that
subdirectory.

Thus, the "git reset -" that we assert must fail is happening in
the outer directory, not the inner no_previous repo.

If that's all we wanted to verify then the (...) sub-shelled cd
command could be replaced entirely by "test -d no_previous", but
I don't think that's the intention of this test.

I believe you may have intended to write the following instead:

test_expect_success 'reset - with no previous branch' '
	git init no_previous --quiet &&
	(
		cd no_previous &&
		test_must_fail git reset - 2>../output
	) &&
	test_cmp expect output
'

"output" becomes "../output" because we're one directory down.


> +
> +test_expect_success 'reset - while having file named - and no previous branch' '
> +	git init no_previous --quiet &&
> +	(
> +	cd no_previous &&
> +	touch ./-
> +	) &&
> +	test_must_fail git reset - 2>output &&
> +	test_cmp expect output
> +'


Ditto; please indent (...) subshells and move the "git reset"
invocation into the subshell so that it runs in the context of
the no_previous repo.  The output path will need the same
adjustment as above.


> +
> +cat > expect << EOF


Ditto; no space after > and <<.


> +Unstaged changes after reset:
> +M	-
> +M	1
> +EOF
> +
> +test_expect_success 'reset - in the prescence of file named - with previou branch' '
> +	git init no_previous --quiet &&
> +	cd no_previous &&


Tests that need to change the current directory with "cd" should
generally always use a (...) subshell.  It prevents them from
needing to manually undo the "cd" before running subsequent
commands that need to be in the original, parent directory.


> +	touch ./- 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 &&
> +	test_cmp output ../expect


When applying the previous note, if we keep the test_cmp line
outside of the (...) subshell then we won't need to use "../"
when referring to "expect" here, but we will need it for
"../output" file on the line above it.


> +'
> +test_expect_success 'reset - works same as reset @{-1}' '
> +	git init no_previous --quiet &&
> +	cd no_previous &&


Ditto; please use a subshell when entering "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 >../first &&
> +	git add new-file &&
> +	git commit -m "added new-file" &&
> +	git reset @{-1} &&
> +	git status >../second &&
> +	test_cmp ../first ../second
> +'
> +
>  test_done


This test uses "git status" to capture the worktree state so
that we can verify that calling "reset -" and "reset @{-1}" are
equivalent.

Bare "git status" is not a plumbing command.  This doesn't make a
practical difference for the purpose of this test, but it's
probably a good idea to use the plumbing form of "git status" by
passing the "--porcelain" flag when calling it here.


In addition to these tests, it might also be worth adding test
cases to ensure that we unambiguously differentiate the "-"
shortcut from a file when the "--" marker is used in a repo that
contains a "-" file.  When running the following two commands,

	git reset - --
	git reset -- -

we should test that these are unambiguous because of the "--".

The first command should notice the dash-dash and treat "-" like
a shortcut despite the existence of a file named "-".

The second command should operate on the "-" file only and
otherwise leave the repo state as-is.

If we want to be especially thorough then we should also test
this invocation:

	git reset - -- -

This invocation should reset the index to the previous commit
for the "-" file only.

I don't want to increase the scope of this commit so it might
not hurt to add these additional tests as a separate patch.
-- 
David

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-03-08 18:09 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-03 20:51 [PATCH] reset: allow "-" short hand for previous commit Sudhanshu Shekhar
2015-03-03 22:10 ` Matthieu Moy
2015-03-03 23:17   ` Junio C Hamano
2015-03-04  7:07     ` Sudhanshu Shekhar
2015-03-04  7:09       ` Sudhanshu Shekhar
2015-03-04  7:10       ` Eric Sunshine
2015-03-05  0:34       ` Junio C Hamano
2015-03-07 21:04         ` [PATCH 1/2] " Sudhanshu Shekhar
2015-03-08 10:33           ` Matthieu Moy
2015-03-08 14:58             ` [PATCH 1/2] Teach reset the same short-hand as checkout Sudhanshu Shekhar
2015-03-08 14:58               ` [PATCH 2/2] Added test cases for git reset - Sudhanshu Shekhar
2015-03-08 18:09                 ` David Aguilar

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).