* 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).