# git reset --hard in .git causes a checkout in that directory 8 messages from 2009-12-03 to 2009-12-06. Participants: Maarten Lankhorst, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/21840 ## Maarten Lankhorst, 2009-12-03 11:30 Subject: git reset --hard in .git causes a checkout in that directory Message-ID: <4B17A166.60306@gmail.com> URL: https://gitlist.dev/e/4B17A166.60306%40gmail.com ``` 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, 2009-12-04 11:11 Subject: Re: git reset --hard in .git causes a checkout in that directory Message-ID: <20091204111158.GE27495@coredump.intra.peff.net> URL: https://gitlist.dev/e/20091204111158.GE27495%40coredump.intra.peff.net In-Reply-To: <4B17A166.60306@gmail.com> ``` 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 --- 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 ``` ## Junio C Hamano, 2009-12-04 17:25 Subject: Re: git reset --hard in .git causes a checkout in that directory Message-ID: <7vljhislbj.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljhislbj.fsf%40alter.siamese.dyndns.org In-Reply-To: <20091204111158.GE27495@coredump.intra.peff.net> ``` Jeff King 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 ``` ## Junio C Hamano, 2009-12-05 18:24 Subject: Re: git reset --hard in .git causes a checkout in that directory Message-ID: <7vfx7pwa6p.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfx7pwa6p.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vljhislbj.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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" ' ``` ## Junio C Hamano, 2009-12-05 19:06 Subject: Re: git reset --hard in .git causes a checkout in that directory Message-ID: <7vocmdutph.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vocmdutph.fsf%40alter.siamese.dyndns.org In-Reply-To: <20091204111158.GE27495@coredump.intra.peff.net> ``` Jeff King 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? ``` ## Jeff King, 2009-12-06 04:15 Subject: Re: git reset --hard in .git causes a checkout in that directory Message-ID: <20091206041517.GA23983@coredump.intra.peff.net> URL: https://gitlist.dev/e/20091206041517.GA23983%40coredump.intra.peff.net In-Reply-To: <7vfx7pwa6p.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Jeff King, 2009-12-06 04:18 Subject: Re: git reset --hard in .git causes a checkout in that directory Message-ID: <20091206041839.GB23983@coredump.intra.peff.net> URL: https://gitlist.dev/e/20091206041839.GB23983%40coredump.intra.peff.net In-Reply-To: <7vocmdutph.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Junio C Hamano, 2009-12-06 07:54 Subject: Re: git reset --hard in .git causes a checkout in that directory Message-ID: <7vk4x0k059.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4x0k059.fsf%40alter.siamese.dyndns.org In-Reply-To: <20091206041839.GB23983@coredump.intra.peff.net> ``` Jeff King 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. ```