git reset --hard in .git causes a checkout in that directory

DISCUSSION Maarten Lankhorst 3 participants 8 messages Jun 15, 2016

Share & tools

Maarten Lankhorst

author

2016-06-15 22:47 UTC #0 lore raw

Hi all,

When I was working on my code and made a mess that I wanted to undo, I accidentally did it in the .git directory, and had a whole clone of my last committed tree there.

It can be triggered easily:

mkdir test; cd test; git init; touch foo; git add foo; git commit -m 'add foo'; cd .git; git reset --hard; [ -f foo ] && echo hello beauty

Other parts of git could be affected, I haven't checked where exactly the bug hides, so I was afraid to send in a patch

Cheers,
Maarten

Jeff King

2016-06-15 22:47 UTC #1 lore raw

Quoted 11 lines · Maarten Lankhorst

When I was working on my code and made a mess that I wanted to undo, I accidentally did it in the .git directory, and had a whole clone of my last committed tree there.
It can be triggered easily:
mkdir test; cd test; git init; touch foo; git add foo; git commit -m 'add foo'; cd .git; git reset --hard; [ -f foo ] && echo hello beauty
Other parts of git could be affected, I haven't checked where exactly the bug hides, so I was afraid to send in a patch

Yuck. Thanks for the bug report. This is due to a too-loose check on my part in 49b9362 (git-reset: refuse to do hard reset in a bare repository, 2007-12-31).

Junio, I think the following should go to maint (I didn't bother splitting the --merge and --hard code; --merge is in v1.6.2. I assumed we don't care about maint releases that far back).

-- >8 --
Subject: [PATCH] reset: improve worktree safety valves

The existing code checked to make sure we were not in a bare repository when doing a hard reset. However, we should take this one step further, and make sure we are in a worktree. Otherwise, we can end up munging files inside of '.git'.

Furthermore, we should do the same check for --merge resets, which have the same properties. Actually, a merge reset of HEAD^ would already complain, since further down in the code we want a worktree. However, it is nicer to check up-front; then we are sure we cover all cases ("git reset --merge" would run, even though it wasn't doing anything) and we can give a more specific message.

Signed-off-by Jeff King <[email protected]>

---
 builtin-reset.c       |    6 ++++--
 t/t7103-reset-bare.sh |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

builtin-reset.c +4 -2

index 73e6022..11d1c6e 100644
@@ -286,8 +286,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (reset_type == NONE)
 		reset_type = MIXED; /* by default */
 
-	if (reset_type == HARD && is_bare_repository())
-		die("hard reset makes no sense in a bare repository");
+	if ((reset_type == HARD || reset_type == MERGE)
+	    && !is_inside_work_tree())
+		die("%s reset requires a work tree",
+		    reset_type_names[reset_type]);
 
 	/* Soft reset does not touch the index file nor the working tree
 	 * at all, but requires them in a good order.  Other resets reset

t/t7103-reset-bare.sh +14 -0

index 42bf518..3ddf0ac 100755
@@ -11,6 +11,16 @@ test_expect_success 'setup non-bare' '
 	git commit -a -m two
 '
 
+test_expect_success 'hard reset requires a worktree' '
+	(cd .git &&
+	 test_must_fail git reset --hard)
+'
+
+test_expect_success 'merge reset requires a worktree' '
+	(cd .git &&
+	 test_must_fail git reset --merge)
+'
+
 test_expect_success 'setup bare' '
 	git clone --bare . bare.git &&
 	cd bare.git

@@ -20,6 +30,10 @@ test_expect_success 'hard reset is not allowed' '
 	test_must_fail  git reset --hard HEAD^
 '
 
+test_expect_success 'merge reset is not allowed' '
+	test_must_fail git reset --merge HEAD^
+'
+
 test_expect_success 'soft reset is allowed' '
 	git reset --soft HEAD^ &&
 	test "`git show --pretty=format:%s | head -n 1`" = "one"

Signature · 1 line

1.6.6.rc1.18.ga777f.dirty

Junio C Hamano

2016-06-15 22:47 UTC #2 lore raw

Quoted 3 lines · Jeff King

Junio, I think the following should go to maint (I didn't bother splitting the --merge and --hard code; --merge is in v1.6.2. I assumed we don't care about maint releases that far back).

Thanks. The test already checks that the change won't break soft reset, which is good, but it does not seem to check/specify what should happen in the mixed reset in this case (I think it should be allowed). Could you add one while at it?

Quoted 34 lines

t/t7103-reset-bare.sh +14 -0

index 42bf518..3ddf0ac 100755
@@ -11,6 +11,16 @@ test_expect_success 'setup non-bare' '
 	git commit -a -m two
 '
 
+test_expect_success 'hard reset requires a worktree' '
+	(cd .git &&
+	 test_must_fail git reset --hard)
+'
+
+test_expect_success 'merge reset requires a worktree' '
+	(cd .git &&
+	 test_must_fail git reset --merge)
+'
+
 test_expect_success 'setup bare' '
 	git clone --bare . bare.git &&
 	cd bare.git

@@ -20,6 +30,10 @@ test_expect_success 'hard reset is not allowed' '
 	test_must_fail  git reset --hard HEAD^
 '
 
+test_expect_success 'merge reset is not allowed' '
+	test_must_fail git reset --merge HEAD^
+'
+
 test_expect_success 'soft reset is allowed' '
 	git reset --soft HEAD^ &&
 	test "`git show --pretty=format:%s | head -n 1`" = "one"

Signature · 1 line

1.6.6.rc1.18.ga777f.dirty

Junio C Hamano

2016-06-15 22:47 UTC #3 lore raw

Quoted 3 lines · Junio C Hamano

... The test already checks that the change won't break soft reset, which is good, but it does not seem to check/specify what should happen in the mixed reset in this case (I think it should be allowed).

Heh, I was not thinking straight. A bare repository does not have the index, so allowing (cd .git && git reset) is Ok but mixed in a bare repository (cd bare.git && git reset) is not.

Let's cover a few more missing cases.

 t/t7103-reset-bare.sh |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

t/t7103-reset-bare.sh +16 -4

index 3ddf0ac..68041df 100755
@@ -21,20 +21,32 @@ test_expect_success 'merge reset requires a worktree' '
 	 test_must_fail git reset --merge)
 '
 
+test_expect_success 'mixed reset is ok' '
+	(cd .git && git reset)
+'
+
+test_expect_success 'soft reset is ok' '
+	(cd .git && git reset --soft)
+'
+
 test_expect_success 'setup bare' '
 	git clone --bare . bare.git &&
 	cd bare.git
 '
 
-test_expect_success 'hard reset is not allowed' '
-	test_must_fail  git reset --hard HEAD^
+test_expect_success 'hard reset is not allowed in bare' '
+	test_must_fail git reset --hard HEAD^
 '
 
-test_expect_success 'merge reset is not allowed' '
+test_expect_success 'merge reset is not allowed in bare' '
 	test_must_fail git reset --merge HEAD^
 '
 
-test_expect_success 'soft reset is allowed' '
+test_expect_success 'mixed reset is not allowed in bare' '
+	test_must_fail git reset --mixed HEAD^
+'
+
+test_expect_success 'soft reset is allowed in bare' '
 	git reset --soft HEAD^ &&
 	test "`git show --pretty=format:%s | head -n 1`" = "one"
 '

Jeff King

2016-06-15 22:47 UTC #4 lore raw

Quoted 7 lines · Junio C Hamano

Quoted 3 lines
... The test already checks that the change won't break soft reset, which is good, but it does not seem to check/specify what should happen in the mixed reset in this case (I think it should be allowed).
Heh, I was not thinking straight. A bare repository does not have the index, so allowing (cd .git && git reset) is Ok but mixed in a bare repository (cd bare.git && git reset) is not.

Hmm. I would have thought it would be allowed in a bare repository, to explicitly let people treat the bare repo as a pseudo-database, just pulling out the files when they want to. And I was all set to argue against restricting it, but looking at your tests, it seems we already disallow it. So I don't see a harm in verifying the current behavior.

-Peff

Junio C Hamano

2016-06-15 22:47 UTC #5 lore raw

In reply to Jeff King

Quoted 14 lines · Jeff King

Quoted 11 lines · Maarten Lankhorst
When I was working on my code and made a mess that I wanted to undo, I accidentally did it in the .git directory, and had a whole clone of my last committed tree there.
It can be triggered easily:
mkdir test; cd test; git init; touch foo; git add foo; git commit -m 'add foo'; cd .git; git reset --hard; [ -f foo ] && echo hello beauty
Other parts of git could be affected, I haven't checked where exactly the bug hides, so I was afraid to send in a patch

Quoted 7 lines

Yuck. Thanks for the bug report. This is due to a too-loose check on my part in 49b9362 (git-reset: refuse to do hard reset in a bare repository, 2007-12-31).
Junio, I think the following should go to maint (I didn't bother splitting the --merge and --hard code; --merge is in v1.6.2. I assumed we don't care about maint releases that far back).

Although I'll apply your patch to 'maint' and will merge it for 1.6.6, I am not quite sure if this is the best fix in the longer run. Shouldn't we go back to the top of the work tree and running what was asked there?

Jeff King

2016-06-15 22:47 UTC #6 lore raw

Quoted 7 lines · Junio C Hamano

Quoted 3 lines
Junio, I think the following should go to maint (I didn't bother splitting the --merge and --hard code; --merge is in v1.6.2. I assumed we don't care about maint releases that far back).
Although I'll apply your patch to 'maint' and will merge it for 1.6.6, I am not quite sure if this is the best fix in the longer run. Shouldn't we go back to the top of the work tree and running what was asked there?

I actually considered that, too, when writing the patch. But that would be inconsistent with all of the other commands that use SETUP_WORK_TREE. For example:

  $ git init && cd .git && git clean
  fatal: This operation must be run in a work tree

So I think we are better to be consistent with the other commands. If somebody wants to make a separate patch to discover the work tree while in the $GIT_DIR and chdir to it, that should then be applied to all commands. I'm not opposed to it, but I also don't see it as a particularly pressing need.

-Peff

Junio C Hamano

2016-06-15 22:47 UTC #7 lore raw

Quoted 4 lines · Jeff King

I actually considered that, too, when writing the patch. But that would be inconsistent with all of the other commands that use SETUP_WORK_TREE. For example:

Quoted 8 lines

  $ git init && cd .git && git clean
  fatal: This operation must be run in a work tree
So I think we are better to be consistent with the other commands. If somebody wants to make a separate patch to discover the work tree while in the $GIT_DIR and chdir to it, that should then be applied to all commands. I'm not opposed to it, but I also don't see it as a particularly pressing need.

Yes, we would of course want to do this consistently. I haven't followed the codepath yet, but I suspect this will end up being connected with running "rev-parse --show-cdup" inside .git/ of a non-bare repository.