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

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

[Maarten Lankhorst]

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

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

[Jeff King]

On Thu, Dec 03, 2009 at 12:30:46PM +0100, Maarten Lankhorst wrote:

> 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 <peff@peff.net>
---
 builtin-reset.c       |    6 ++++--
 t/t7103-reset-bare.sh |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/builtin-reset.c b/builtin-reset.c
index 73e6022..11d1c6e 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -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
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index 42bf518..3ddf0ac 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -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"
-- 
1.6.6.rc1.18.ga777f.dirty

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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?

> diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
> index 42bf518..3ddf0ac 100755
> --- a/t/t7103-reset-bare.sh
> +++ b/t/t7103-reset-bare.sh
> @@ -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"
> -- 
> 1.6.6.rc1.18.ga777f.dirty

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...  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(-)

diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index 3ddf0ac..68041df 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -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"
 '

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

[Jeff King]

On Sat, Dec 05, 2009 at 10:24:46AM -0800, Junio C Hamano wrote:

> > ...  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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 03, 2009 at 12:30:46PM +0100, Maarten Lankhorst wrote:
>
>> 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).

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?

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

[Jeff King]

On Sat, Dec 05, 2009 at 11:06:02AM -0800, Junio C Hamano wrote:

> > 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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.