threads / discuss / 21840
Subject: git reset --hard in .git causes a checkout in that directory
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 KingRe: git reset --hard in .git causes a checkout in that directory
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
Junio C HamanoRe: git reset --hard in .git causes a checkout in that directory
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
Junio C HamanoRe: git reset --hard in .git causes a checkout in that directory
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" '
Jeff KingRe: git reset --hard in .git causes a checkout in that directory
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
Junio C HamanoRe: git reset --hard in .git causes a checkout in that directory
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?
Jeff KingRe: git reset --hard in .git causes a checkout in that directory
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 HamanoRe: git reset --hard in .git causes a checkout in that directory
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.