Re: [PATCH] reset: allow "-" short hand for previous commit

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] reset: allow "-" short hand for previous commit

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:03:59 

Sudhanshu Shekhar [off-list ref] writes:
quoted
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.

[PATCH 1/2] reset: allow "-" short hand for previous commit

From: Sudhanshu Shekhar <hidden>
Date: 2016-06-15 23:04:04 

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

Signed-off-by: Sudhanshu Shekhar <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help