Re: Should "git symbolic-ref -d HEAD" be forbidden?
- From
Jeff King <peff@peff.net>
- Date
- Sep 2, 2016, 00:55 UTC
- Message-ID
- <20160902005545.rbfxxdcv7nekzqku@sigill.intra.peff.net>
- In-Reply-To
- <xmqq8tvbuu9r.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 01, 2016 at 03:31:28PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> -- >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().
Show 14 quoted lines
> -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