threads / discuss / 43978

Should "git symbolic-ref -d HEAD" be forbidden?

Subject: Should "git symbolic-ref -d HEAD" be forbidden?

## tl;dr

5 messages between Sep 1, 2016 and Sep 2, 2016.

replies: 4people: 3as markdown or json

Junio C Hamano· Sep 1, 2016, 21:08 UTC · lore

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.

Jeff King· Sep 1, 2016, 21:19 UTC · re: Junio C Hamano · lore

Re: Should "git symbolic-ref -d HEAD" be forbidden?

On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> 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

Junio C Hamano· Sep 1, 2016, 22:31 UTC · re: Jeff King · lore

Re: Should "git symbolic-ref -d HEAD" be forbidden?

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> 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

Jeff King· Sep 2, 2016, 00:55 UTC · re: Junio C Hamano · lore

Re: Should "git symbolic-ref -d HEAD" be forbidden?

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

Andreas Schwab· Sep 2, 2016, 09:19 UTC · re: Junio C Hamano · lore

Re: Should "git symbolic-ref -d HEAD" be forbidden?

On Sep 01 2016, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> 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."