* Should "git symbolic-ref -d HEAD" be forbidden?
@ 2016-09-01 21:08 Junio C Hamano
2016-09-01 21:19 ` Jeff King
2016-09-02 9:19 ` Andreas Schwab
0 siblings, 2 replies; 5+ messages in thread
From: Junio C Hamano @ 2016-09-01 21:08 UTC (permalink / raw)
To: git
I think we should.
t1401 expects to be able to, but if you really do it:
$ cd /tmp
$ git init throwaway
$ cd throwaway
$ git symbolic-ref -d HEAD
the setup machinery considers that you are no longer in a working
tree that is controlled by a repository at .git/ because .git/ is
no longer a valid repository, so you cannot even do
$ git symbolic-ref HEAD refs/heads/master
to recover.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Should "git symbolic-ref -d HEAD" be forbidden? 2016-09-01 21:08 Should "git symbolic-ref -d HEAD" be forbidden? Junio C Hamano @ 2016-09-01 21:19 ` Jeff King 2016-09-01 22:31 ` Junio C Hamano 2016-09-02 9:19 ` Andreas Schwab 1 sibling, 1 reply; 5+ messages in thread From: Jeff King @ 2016-09-01 21:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote: > I think we should. > > t1401 expects to be able to, but if you really do it: > > $ cd /tmp > $ git init throwaway > $ cd throwaway > $ git symbolic-ref -d HEAD > > the setup machinery considers that you are no longer in a working > tree that is controlled by a repository at .git/ because .git/ is > no longer a valid repository, so you cannot even do > > $ git symbolic-ref HEAD refs/heads/master > > to recover. Yes, I think we should, too. The same reasoning from afe5d3d (symbolic ref: refuse non-ref targets in HEAD, 2009-01-29) applies. -Peff ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Should "git symbolic-ref -d HEAD" be forbidden? 2016-09-01 21:19 ` Jeff King @ 2016-09-01 22:31 ` Junio C Hamano 2016-09-02 0:55 ` Jeff King 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2016-09-01 22:31 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote: > >> I think we should. >> >> t1401 expects to be able to, but if you really do it: >> >> $ cd /tmp >> $ git init throwaway >> $ cd throwaway >> $ git symbolic-ref -d HEAD >> >> the setup machinery considers that you are no longer in a working >> tree that is controlled by a repository at .git/ because .git/ is >> no longer a valid repository, so you cannot even do >> >> $ git symbolic-ref HEAD refs/heads/master >> >> to recover. > > Yes, I think we should, too. The same reasoning from afe5d3d (symbolic > ref: refuse non-ref targets in HEAD, 2009-01-29) applies. -- >8 -- Subject: symbolic-ref -d: do not allow removal of HEAD If you delete the symbolic-ref HEAD from a repository, Git no longer considers it valid, and even "git symbolic-ref HEAD refs/heads/master" would not be able to recover from that state. In the spirit similar to afe5d3d5 ("symbolic ref: refuse non-ref targets in HEAD", 2009-01-29), forbid removal of HEAD. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- I decided against it for now for no good reason, other than I am a bit superstitious, but it may be a good idea to move these safety checks to delete_ref() and create_symref() in the longer term. builtin/symbolic-ref.c | 2 ++ t/t1401-symbolic-ref.sh | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c index 9c29a64..96eed94 100644 --- a/builtin/symbolic-ref.c +++ b/builtin/symbolic-ref.c @@ -56,6 +56,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix) ret = check_symref(argv[0], 1, 0, 0); if (ret) die("Cannot delete %s, not a symbolic ref", argv[0]); + if (!strcmp(argv[0], "HEAD")) + die("deleting '%s' is not allowed", argv[0]); return delete_ref(argv[0], NULL, REF_NODEREF); } diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh index ca3fa40..5c30f94 100755 --- a/t/t1401-symbolic-ref.sh +++ b/t/t1401-symbolic-ref.sh @@ -33,18 +33,23 @@ test_expect_success 'symbolic-ref refuses bare sha1' ' ' reset_to_sane -test_expect_success 'symbolic-ref deletes HEAD' ' - git symbolic-ref -d HEAD && +test_expect_success 'HEAD cannot be removed' ' + test_must_fail git symbolic-ref -d HEAD +' + +test_expect_success 'symbolic-ref can be deleted' ' + git symbolic-ref NOTHEAD refs/heads/foo && + git symbolic-ref -d NOTHEAD && test_path_is_file .git/refs/heads/foo && - test_path_is_missing .git/HEAD + test_path_is_missing .git/NOTHEAD ' reset_to_sane -test_expect_success 'symbolic-ref deletes dangling HEAD' ' - git symbolic-ref HEAD refs/heads/missing && - git symbolic-ref -d HEAD && +test_expect_success 'symbolic-ref can delete dangling symref' ' + git symbolic-ref NOTHEAD refs/heads/missing && + git symbolic-ref -d NOTHEAD && test_path_is_missing .git/refs/heads/missing && - test_path_is_missing .git/HEAD + test_path_is_missing .git/NOTHEAD ' reset_to_sane ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Should "git symbolic-ref -d HEAD" be forbidden? 2016-09-01 22:31 ` Junio C Hamano @ 2016-09-02 0:55 ` Jeff King 0 siblings, 0 replies; 5+ messages in thread From: Jeff King @ 2016-09-02 0:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Sep 01, 2016 at 03:31:28PM -0700, Junio C Hamano wrote: > -- >8 -- > Subject: symbolic-ref -d: do not allow removal of HEAD > > If you delete the symbolic-ref HEAD from a repository, Git no longer > considers it valid, and even "git symbolic-ref HEAD refs/heads/master" > would not be able to recover from that state. > > In the spirit similar to afe5d3d5 ("symbolic ref: refuse non-ref > targets in HEAD", 2009-01-29), forbid removal of HEAD. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- Makes sense. You might want to change "it" in "no longer considers it valid" to "the repository". At first I thought "it" referred to the symref. Which obviously shouldn't be valid after being deleted. :) > I decided against it for now for no good reason, other than I am a > bit superstitious, but it may be a good idea to move these safety > checks to delete_ref() and create_symref() in the longer term. Yeah, that somehow feels weird and too low-level to me. After all, we _do_ want to drop HEAD as a symref when we turn it into a detached HEAD. The point of this (and afe5d3d5) is to prevent people from shooting themselves in the foot. Internal Git code should know to avoid this foot-shooting itself. OTOH, I think "git update-ref --no-deref -d HEAD" is another user-facing hole-in-foot opportunity, and it would be blocked by putting this into delete_ref(). > -test_expect_success 'symbolic-ref deletes HEAD' ' > - git symbolic-ref -d HEAD && > +test_expect_success 'HEAD cannot be removed' ' > + test_must_fail git symbolic-ref -d HEAD > +' > + > +test_expect_success 'symbolic-ref can be deleted' ' > + git symbolic-ref NOTHEAD refs/heads/foo && > + git symbolic-ref -d NOTHEAD && > test_path_is_file .git/refs/heads/foo && > - test_path_is_missing .git/HEAD > + test_path_is_missing .git/NOTHEAD > ' > reset_to_sane Do you want another "reset_to_sane" call after your new test? Otherwise if it fails the "symbolic-ref can be deleted" test will start operating on the parent repository. -Peff ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Should "git symbolic-ref -d HEAD" be forbidden? 2016-09-01 21:08 Should "git symbolic-ref -d HEAD" be forbidden? Junio C Hamano 2016-09-01 21:19 ` Jeff King @ 2016-09-02 9:19 ` Andreas Schwab 1 sibling, 0 replies; 5+ messages in thread From: Andreas Schwab @ 2016-09-02 9:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Sep 01 2016, Junio C Hamano <gitster@pobox.com> wrote: > I think we should. > > t1401 expects to be able to, but if you really do it: > > $ cd /tmp > $ git init throwaway > $ cd throwaway > $ git symbolic-ref -d HEAD > > the setup machinery considers that you are no longer in a working > tree that is controlled by a repository at .git/ because .git/ is > no longer a valid repository, so you cannot even do > > $ git symbolic-ref HEAD refs/heads/master > > to recover. git init recovers it, though. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-02 9:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-01 21:08 Should "git symbolic-ref -d HEAD" be forbidden? Junio C Hamano 2016-09-01 21:19 ` Jeff King 2016-09-01 22:31 ` Junio C Hamano 2016-09-02 0:55 ` Jeff King 2016-09-02 9:19 ` Andreas Schwab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).