{"thread":{"id":"43978","subject":"Should \"git symbolic-ref -d HEAD\" be forbidden?","startedAt":"2016-09-01T21:09:34Z","lastAt":"2016-09-02T09:19:39Z","messageCount":5,"participants":["Junio C Hamano","Jeff King","Andreas Schwab"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"300831","messageId":"xmqqpoonuy4n.fsf@gitster.mtv.corp.google.com","threadId":"43978","inReplyTo":null,"subject":"Should \"git symbolic-ref -d HEAD\" be forbidden?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-01T21:08:08Z","receivedAt":"2016-09-01T21:09:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think we should.\n\nt1401 expects to be able to, but if you really do it:\n\n\t$ cd /tmp\n\t$ git init throwaway\n        $ cd throwaway\n        $ git symbolic-ref -d HEAD\n\nthe setup machinery considers that you are no longer in a working\ntree that is controlled by a repository at .git/ because .git/ is\nno longer a valid repository, so you cannot even do\n\n\t$ git symbolic-ref HEAD refs/heads/master\n\nto recover.\n"},{"id":"300838","messageId":"20160901211907.iivokwu3yjuxz3qf@sigill.intra.peff.net","threadId":"43978","inReplyTo":"xmqqpoonuy4n.fsf@gitster.mtv.corp.google.com","subject":"Re: Should \"git symbolic-ref -d HEAD\" be forbidden?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-01T21:19:07Z","receivedAt":"2016-09-01T21:19:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote:\n\n> I think we should.\n> \n> t1401 expects to be able to, but if you really do it:\n> \n> \t$ cd /tmp\n> \t$ git init throwaway\n>         $ cd throwaway\n>         $ git symbolic-ref -d HEAD\n> \n> the setup machinery considers that you are no longer in a working\n> tree that is controlled by a repository at .git/ because .git/ is\n> no longer a valid repository, so you cannot even do\n> \n> \t$ git symbolic-ref HEAD refs/heads/master\n> \n> to recover.\n\nYes, I think we should, too. The same reasoning from afe5d3d (symbolic\nref: refuse non-ref targets in HEAD, 2009-01-29) applies.\n\n-Peff\n"},{"id":"300849","messageId":"xmqq8tvbuu9r.fsf@gitster.mtv.corp.google.com","threadId":"43978","inReplyTo":"20160901211907.iivokwu3yjuxz3qf@sigill.intra.peff.net","subject":"Re: Should \"git symbolic-ref -d HEAD\" be forbidden?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-01T22:31:28Z","receivedAt":"2016-09-01T22:32:55Z","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, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote:\n>\n>> I think we should.\n>> \n>> t1401 expects to be able to, but if you really do it:\n>> \n>> \t$ cd /tmp\n>> \t$ git init throwaway\n>>         $ cd throwaway\n>>         $ git symbolic-ref -d HEAD\n>> \n>> the setup machinery considers that you are no longer in a working\n>> tree that is controlled by a repository at .git/ because .git/ is\n>> no longer a valid repository, so you cannot even do\n>> \n>> \t$ git symbolic-ref HEAD refs/heads/master\n>> \n>> to recover.\n>\n> Yes, I think we should, too. The same reasoning from afe5d3d (symbolic\n> ref: refuse non-ref targets in HEAD, 2009-01-29) applies.\n\n-- >8 --\nSubject: symbolic-ref -d: do not allow removal of HEAD\n\nIf you delete the symbolic-ref HEAD from a repository, Git no longer\nconsiders it valid, and even \"git symbolic-ref HEAD refs/heads/master\"\nwould not be able to recover from that state.\n\nIn the spirit similar to afe5d3d5 (\"symbolic ref: refuse non-ref\ntargets in HEAD\", 2009-01-29), forbid removal of HEAD.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n I decided against it for now for no good reason, other than I am a\n bit superstitious, but it may be a good idea to move these safety\n checks to delete_ref() and create_symref() in the longer term.\n\n builtin/symbolic-ref.c  |  2 ++\n t/t1401-symbolic-ref.sh | 19 ++++++++++++-------\n 2 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex 9c29a64..96eed94 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -56,6 +56,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \t\tret = check_symref(argv[0], 1, 0, 0);\n \t\tif (ret)\n \t\t\tdie(\"Cannot delete %s, not a symbolic ref\", argv[0]);\n+\t\tif (!strcmp(argv[0], \"HEAD\"))\n+\t\t\tdie(\"deleting '%s' is not allowed\", argv[0]);\n \t\treturn delete_ref(argv[0], NULL, REF_NODEREF);\n \t}\n \ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex ca3fa40..5c30f94 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -33,18 +33,23 @@ test_expect_success 'symbolic-ref refuses bare sha1' '\n '\n reset_to_sane\n \n-test_expect_success 'symbolic-ref deletes HEAD' '\n-\tgit symbolic-ref -d HEAD &&\n+test_expect_success 'HEAD cannot be removed' '\n+\ttest_must_fail git symbolic-ref -d HEAD\n+'\n+\n+test_expect_success 'symbolic-ref can be deleted' '\n+\tgit symbolic-ref NOTHEAD refs/heads/foo &&\n+\tgit symbolic-ref -d NOTHEAD &&\n \ttest_path_is_file .git/refs/heads/foo &&\n-\ttest_path_is_missing .git/HEAD\n+\ttest_path_is_missing .git/NOTHEAD\n '\n reset_to_sane\n \n-test_expect_success 'symbolic-ref deletes dangling HEAD' '\n-\tgit symbolic-ref HEAD refs/heads/missing &&\n-\tgit symbolic-ref -d HEAD &&\n+test_expect_success 'symbolic-ref can delete dangling symref' '\n+\tgit symbolic-ref NOTHEAD refs/heads/missing &&\n+\tgit symbolic-ref -d NOTHEAD &&\n \ttest_path_is_missing .git/refs/heads/missing &&\n-\ttest_path_is_missing .git/HEAD\n+\ttest_path_is_missing .git/NOTHEAD\n '\n reset_to_sane\n \n"},{"id":"300862","messageId":"20160902005545.rbfxxdcv7nekzqku@sigill.intra.peff.net","threadId":"43978","inReplyTo":"xmqq8tvbuu9r.fsf@gitster.mtv.corp.google.com","subject":"Re: Should \"git symbolic-ref -d HEAD\" be forbidden?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-02T00:55:45Z","receivedAt":"2016-09-02T00:55:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2016 at 03:31:28PM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: symbolic-ref -d: do not allow removal of HEAD\n> \n> If you delete the symbolic-ref HEAD from a repository, Git no longer\n> considers it valid, and even \"git symbolic-ref HEAD refs/heads/master\"\n> would not be able to recover from that state.\n> \n> In the spirit similar to afe5d3d5 (\"symbolic ref: refuse non-ref\n> targets in HEAD\", 2009-01-29), forbid removal of HEAD.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n\nMakes sense. You might want to change \"it\" in \"no longer considers it\nvalid\" to \"the repository\". At first I thought \"it\" referred to the\nsymref. Which obviously shouldn't be valid after being deleted. :)\n\n>  I decided against it for now for no good reason, other than I am a\n>  bit superstitious, but it may be a good idea to move these safety\n>  checks to delete_ref() and create_symref() in the longer term.\n\nYeah, that somehow feels weird and too low-level to me. After all, we\n_do_ want to drop HEAD as a symref when we turn it into a detached HEAD.\nThe point of this (and afe5d3d5) is to prevent people from shooting\nthemselves in the foot. Internal Git code should know to avoid this\nfoot-shooting itself.\n\nOTOH, I think \"git update-ref --no-deref -d HEAD\" is another user-facing\nhole-in-foot opportunity, and it would be blocked by putting this into\ndelete_ref().\n\n> -test_expect_success 'symbolic-ref deletes HEAD' '\n> -\tgit symbolic-ref -d HEAD &&\n> +test_expect_success 'HEAD cannot be removed' '\n> +\ttest_must_fail git symbolic-ref -d HEAD\n> +'\n> +\n> +test_expect_success 'symbolic-ref can be deleted' '\n> +\tgit symbolic-ref NOTHEAD refs/heads/foo &&\n> +\tgit symbolic-ref -d NOTHEAD &&\n>  \ttest_path_is_file .git/refs/heads/foo &&\n> -\ttest_path_is_missing .git/HEAD\n> +\ttest_path_is_missing .git/NOTHEAD\n>  '\n>  reset_to_sane\n\nDo you want another \"reset_to_sane\" call after your new test? Otherwise\nif it fails the \"symbolic-ref can be deleted\" test will start operating\non the parent repository.\n\n-Peff\n"},{"id":"300877","messageId":"87wpiuy7z2.fsf@linux-m68k.org","threadId":"43978","inReplyTo":"xmqqpoonuy4n.fsf@gitster.mtv.corp.google.com","subject":"Re: Should \"git symbolic-ref -d HEAD\" be forbidden?","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-09-02T09:19:29Z","receivedAt":"2016-09-02T09:19:39Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Sep 01 2016, Junio C Hamano <gitster@pobox.com> wrote:\n\n> I think we should.\n>\n> t1401 expects to be able to, but if you really do it:\n>\n> \t$ cd /tmp\n> \t$ git init throwaway\n>         $ cd throwaway\n>         $ git symbolic-ref -d HEAD\n>\n> the setup machinery considers that you are no longer in a working\n> tree that is controlled by a repository at .git/ because .git/ is\n> no longer a valid repository, so you cannot even do\n>\n> \t$ git symbolic-ref HEAD refs/heads/master\n>\n> to recover.\n\ngit init recovers it, though.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"}]}