{"thread":{"id":"21840","subject":"git reset --hard in .git causes a checkout in that directory","startedAt":"2009-12-03T11:30:46Z","lastAt":"2009-12-06T07:54:42Z","messageCount":8,"participants":["Maarten Lankhorst","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"129115","messageId":"4B17A166.60306@gmail.com","threadId":"21840","inReplyTo":null,"subject":"git reset --hard in .git causes a checkout in that directory","fromName":"Maarten Lankhorst","fromEmail":"m.b.lankhorst@gmail.com","sentAt":"2009-12-03T11:30:46Z","receivedAt":"2009-12-03T11:30:46Z","isPatch":false,"sender":{"key":"m.b.lankhorst@gmail.com","avatar":null},"body":"Hi all,\n\nWhen I was working on my code and made a mess that I wanted to undo, I \naccidentally did it in the .git directory, and had a whole clone of my \nlast committed tree there.\n\nIt can be triggered easily:\n\nmkdir test; cd test; git init; touch foo; git add foo; git commit -m \n'add foo'; cd .git; git reset --hard; [ -f foo ] && echo hello beauty\n\nOther parts of git could be affected, I haven't checked where exactly \nthe bug hides, so I was afraid to send in a patch\n\nCheers,\nMaarten\n"},{"id":"129174","messageId":"20091204111158.GE27495@coredump.intra.peff.net","threadId":"21840","inReplyTo":"4B17A166.60306@gmail.com","subject":"Re: git reset --hard in .git causes a checkout in that directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-04T11:11:58Z","receivedAt":"2009-12-04T11:11:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 03, 2009 at 12:30:46PM +0100, Maarten Lankhorst wrote:\n\n> When I was working on my code and made a mess that I wanted to undo,\n> I accidentally did it in the .git directory, and had a whole clone of\n> my last committed tree there.\n> \n> It can be triggered easily:\n> \n> mkdir test; cd test; git init; touch foo; git add foo; git commit -m\n> 'add foo'; cd .git; git reset --hard; [ -f foo ] && echo hello beauty\n> \n> Other parts of git could be affected, I haven't checked where exactly\n> the bug hides, so I was afraid to send in a patch\n\nYuck. Thanks for the bug report. This is due to a too-loose check on my\npart in 49b9362 (git-reset: refuse to do hard reset in a bare\nrepository, 2007-12-31).\n\nJunio, I think the following should go to maint (I didn't bother\nsplitting the --merge and --hard code; --merge is in v1.6.2. I assumed\nwe don't care about maint releases that far back).\n\n-- >8 --\nSubject: [PATCH] reset: improve worktree safety valves\n\nThe existing code checked to make sure we were not in a bare\nrepository when doing a hard reset. However, we should take\nthis one step further, and make sure we are in a worktree.\nOtherwise, we can end up munging files inside of '.git'.\n\nFurthermore, we should do the same check for --merge resets,\nwhich have the same properties. Actually, a merge reset of\nHEAD^ would already complain, since further down in the code\nwe want a worktree. However, it is nicer to check up-front;\nthen we are sure we cover all cases (\"git reset --merge\"\nwould run, even though it wasn't doing anything) and we can\ngive a more specific message.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-reset.c       |    6 ++++--\n t/t7103-reset-bare.sh |   14 ++++++++++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex 73e6022..11d1c6e 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -286,8 +286,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (reset_type == NONE)\n \t\treset_type = MIXED; /* by default */\n \n-\tif (reset_type == HARD && is_bare_repository())\n-\t\tdie(\"hard reset makes no sense in a bare repository\");\n+\tif ((reset_type == HARD || reset_type == MERGE)\n+\t    && !is_inside_work_tree())\n+\t\tdie(\"%s reset requires a work tree\",\n+\t\t    reset_type_names[reset_type]);\n \n \t/* Soft reset does not touch the index file nor the working tree\n \t * at all, but requires them in a good order.  Other resets reset\ndiff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh\nindex 42bf518..3ddf0ac 100755\n--- a/t/t7103-reset-bare.sh\n+++ b/t/t7103-reset-bare.sh\n@@ -11,6 +11,16 @@ test_expect_success 'setup non-bare' '\n \tgit commit -a -m two\n '\n \n+test_expect_success 'hard reset requires a worktree' '\n+\t(cd .git &&\n+\t test_must_fail git reset --hard)\n+'\n+\n+test_expect_success 'merge reset requires a worktree' '\n+\t(cd .git &&\n+\t test_must_fail git reset --merge)\n+'\n+\n test_expect_success 'setup bare' '\n \tgit clone --bare . bare.git &&\n \tcd bare.git\n@@ -20,6 +30,10 @@ test_expect_success 'hard reset is not allowed' '\n \ttest_must_fail  git reset --hard HEAD^\n '\n \n+test_expect_success 'merge reset is not allowed' '\n+\ttest_must_fail git reset --merge HEAD^\n+'\n+\n test_expect_success 'soft reset is allowed' '\n \tgit reset --soft HEAD^ &&\n \ttest \"`git show --pretty=format:%s | head -n 1`\" = \"one\"\n-- \n1.6.6.rc1.18.ga777f.dirty\n"},{"id":"129217","messageId":"7vljhislbj.fsf@alter.siamese.dyndns.org","threadId":"21840","inReplyTo":"20091204111158.GE27495@coredump.intra.peff.net","subject":"Re: git reset --hard in .git causes a checkout in that directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-04T17:25:36Z","receivedAt":"2009-12-04T17:25:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Junio, I think the following should go to maint (I didn't bother\n> splitting the --merge and --hard code; --merge is in v1.6.2. I assumed\n> we don't care about maint releases that far back).\n\nThanks.  The test already checks that the change won't break soft reset,\nwhich is good, but it does not seem to check/specify what should happen in\nthe mixed reset in this case (I think it should be allowed).  Could you\nadd one while at it?\n\n> diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh\n> index 42bf518..3ddf0ac 100755\n> --- a/t/t7103-reset-bare.sh\n> +++ b/t/t7103-reset-bare.sh\n> @@ -11,6 +11,16 @@ test_expect_success 'setup non-bare' '\n>  \tgit commit -a -m two\n>  '\n>  \n> +test_expect_success 'hard reset requires a worktree' '\n> +\t(cd .git &&\n> +\t test_must_fail git reset --hard)\n> +'\n> +\n> +test_expect_success 'merge reset requires a worktree' '\n> +\t(cd .git &&\n> +\t test_must_fail git reset --merge)\n> +'\n> +\n>  test_expect_success 'setup bare' '\n>  \tgit clone --bare . bare.git &&\n>  \tcd bare.git\n> @@ -20,6 +30,10 @@ test_expect_success 'hard reset is not allowed' '\n>  \ttest_must_fail  git reset --hard HEAD^\n>  '\n>  \n> +test_expect_success 'merge reset is not allowed' '\n> +\ttest_must_fail git reset --merge HEAD^\n> +'\n> +\n>  test_expect_success 'soft reset is allowed' '\n>  \tgit reset --soft HEAD^ &&\n>  \ttest \"`git show --pretty=format:%s | head -n 1`\" = \"one\"\n> -- \n> 1.6.6.rc1.18.ga777f.dirty\n"},{"id":"129277","messageId":"7vfx7pwa6p.fsf@alter.siamese.dyndns.org","threadId":"21840","inReplyTo":"7vljhislbj.fsf@alter.siamese.dyndns.org","subject":"Re: git reset --hard in .git causes a checkout in that directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-05T18:24:46Z","receivedAt":"2009-12-05T18:24:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...  The test already checks that the change won't break soft reset,\n> which is good, but it does not seem to check/specify what should happen in\n> the mixed reset in this case (I think it should be allowed).\n\nHeh, I was not thinking straight. A bare repository does not have the\nindex, so allowing (cd .git && git reset) is Ok but mixed in a bare\nrepository (cd bare.git && git reset) is not.\n\nLet's cover a few more missing cases.\n\n t/t7103-reset-bare.sh |   20 ++++++++++++++++----\n 1 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh\nindex 3ddf0ac..68041df 100755\n--- a/t/t7103-reset-bare.sh\n+++ b/t/t7103-reset-bare.sh\n@@ -21,20 +21,32 @@ test_expect_success 'merge reset requires a worktree' '\n \t test_must_fail git reset --merge)\n '\n \n+test_expect_success 'mixed reset is ok' '\n+\t(cd .git && git reset)\n+'\n+\n+test_expect_success 'soft reset is ok' '\n+\t(cd .git && git reset --soft)\n+'\n+\n test_expect_success 'setup bare' '\n \tgit clone --bare . bare.git &&\n \tcd bare.git\n '\n \n-test_expect_success 'hard reset is not allowed' '\n-\ttest_must_fail  git reset --hard HEAD^\n+test_expect_success 'hard reset is not allowed in bare' '\n+\ttest_must_fail git reset --hard HEAD^\n '\n \n-test_expect_success 'merge reset is not allowed' '\n+test_expect_success 'merge reset is not allowed in bare' '\n \ttest_must_fail git reset --merge HEAD^\n '\n \n-test_expect_success 'soft reset is allowed' '\n+test_expect_success 'mixed reset is not allowed in bare' '\n+\ttest_must_fail git reset --mixed HEAD^\n+'\n+\n+test_expect_success 'soft reset is allowed in bare' '\n \tgit reset --soft HEAD^ &&\n \ttest \"`git show --pretty=format:%s | head -n 1`\" = \"one\"\n '\n"},{"id":"129280","messageId":"7vocmdutph.fsf@alter.siamese.dyndns.org","threadId":"21840","inReplyTo":"20091204111158.GE27495@coredump.intra.peff.net","subject":"Re: git reset --hard in .git causes a checkout in that directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-05T19:06:02Z","receivedAt":"2009-12-05T19:06:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Dec 03, 2009 at 12:30:46PM +0100, Maarten Lankhorst wrote:\n>\n>> When I was working on my code and made a mess that I wanted to undo,\n>> I accidentally did it in the .git directory, and had a whole clone of\n>> my last committed tree there.\n>> \n>> It can be triggered easily:\n>> \n>> mkdir test; cd test; git init; touch foo; git add foo; git commit -m\n>> 'add foo'; cd .git; git reset --hard; [ -f foo ] && echo hello beauty\n>> \n>> Other parts of git could be affected, I haven't checked where exactly\n>> the bug hides, so I was afraid to send in a patch\n>\n> Yuck. Thanks for the bug report. This is due to a too-loose check on my\n> part in 49b9362 (git-reset: refuse to do hard reset in a bare\n> repository, 2007-12-31).\n>\n> Junio, I think the following should go to maint (I didn't bother\n> splitting the --merge and --hard code; --merge is in v1.6.2. I assumed\n> we don't care about maint releases that far back).\n\nAlthough I'll apply your patch to 'maint' and will merge it for 1.6.6, I\nam not quite sure if this is the best fix in the longer run.  Shouldn't we\ngo back to the top of the work tree and running what was asked there?\n"},{"id":"129309","messageId":"20091206041517.GA23983@coredump.intra.peff.net","threadId":"21840","inReplyTo":"7vfx7pwa6p.fsf@alter.siamese.dyndns.org","subject":"Re: git reset --hard in .git causes a checkout in that directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-06T04:15:17Z","receivedAt":"2009-12-06T04:15:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 05, 2009 at 10:24:46AM -0800, Junio C Hamano wrote:\n\n> > ...  The test already checks that the change won't break soft reset,\n> > which is good, but it does not seem to check/specify what should happen in\n> > the mixed reset in this case (I think it should be allowed).\n> \n> Heh, I was not thinking straight. A bare repository does not have the\n> index, so allowing (cd .git && git reset) is Ok but mixed in a bare\n> repository (cd bare.git && git reset) is not.\n\nHmm. I would have thought it would be allowed in a bare repository, to\nexplicitly let people treat the bare repo as a pseudo-database, just\npulling out the files when they want to. And I was all set to argue\nagainst restricting it, but looking at your tests, it seems we already\ndisallow it. So I don't see a harm in verifying the current behavior.\n\n-Peff\n"},{"id":"129310","messageId":"20091206041839.GB23983@coredump.intra.peff.net","threadId":"21840","inReplyTo":"7vocmdutph.fsf@alter.siamese.dyndns.org","subject":"Re: git reset --hard in .git causes a checkout in that directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-06T04:18:40Z","receivedAt":"2009-12-06T04:18:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 05, 2009 at 11:06:02AM -0800, Junio C Hamano wrote:\n\n> > Junio, I think the following should go to maint (I didn't bother\n> > splitting the --merge and --hard code; --merge is in v1.6.2. I assumed\n> > we don't care about maint releases that far back).\n> \n> Although I'll apply your patch to 'maint' and will merge it for 1.6.6, I\n> am not quite sure if this is the best fix in the longer run.  Shouldn't we\n> go back to the top of the work tree and running what was asked there?\n\nI actually considered that, too, when writing the patch. But that would\nbe inconsistent with all of the other commands that use SETUP_WORK_TREE.\nFor example:\n\n  $ git init && cd .git && git clean\n  fatal: This operation must be run in a work tree\n\nSo I think we are better to be consistent with the other commands. If\nsomebody wants to make a separate patch to discover the work tree while\nin the $GIT_DIR and chdir to it, that should then be applied to all\ncommands.  I'm not opposed to it, but I also don't see it as a\nparticularly pressing need.\n\n-Peff\n"},{"id":"129314","messageId":"7vk4x0k059.fsf@alter.siamese.dyndns.org","threadId":"21840","inReplyTo":"20091206041839.GB23983@coredump.intra.peff.net","subject":"Re: git reset --hard in .git causes a checkout in that directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-06T07:54:42Z","receivedAt":"2009-12-06T07:54:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I actually considered that, too, when writing the patch. But that would\n> be inconsistent with all of the other commands that use SETUP_WORK_TREE.\n> For example:\n>\n>   $ git init && cd .git && git clean\n>   fatal: This operation must be run in a work tree\n>\n> So I think we are better to be consistent with the other commands. If\n> somebody wants to make a separate patch to discover the work tree while\n> in the $GIT_DIR and chdir to it, that should then be applied to all\n> commands.  I'm not opposed to it, but I also don't see it as a\n> particularly pressing need.\n\nYes, we would of course want to do this consistently.  I haven't followed\nthe codepath yet, but I suspect this will end up being connected with\nrunning \"rev-parse --show-cdup\" inside .git/ of a non-bare repository.\n"}]}