Re: "git symbolic-ref" doesn't do a very good job

[Jeff King <peff@peff.net>]

On Sat, Jul 30, 2022 at 01:21:50PM -0700, Linus Torvalds wrote:

> Again - this is such a low-level plumbing thing that maybe nobody
> cares, but it just struck me as a bad idea to have these kinds of
> maintenance commands that can be used to just mess up your repository.
> And if you have a bare repo, this really does look like the command
> that *should* be used to change HEAD, since it's not about "git
> checkout"

I think it is probably worth addressing. I'm sure it has bitten me
before[0], and the HEAD logic from afe5d3d516 (symbolic ref: refuse
non-ref targets in HEAD, 2009-01-29) was a cowardly attempt to fix the
most egregious cases without breaking anything.

> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
> index e547a08d6c..5354cfb4f1 100644
> --- a/builtin/symbolic-ref.c
> +++ b/builtin/symbolic-ref.c
> @@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
>  		if (!strcmp(argv[0], "HEAD") &&
>  		    !starts_with(argv[1], "refs/"))
>  			die("Refusing to point HEAD outside of refs/");
> +		if (check_refname_format(argv[1], 0) < 0)
> +			die("Refusing to set '%s' to invalid ref '%s'", argv[0], argv[1]);

This forbids syntactically-invalid refnames, which is good.

Since you don't pass REFNAME_ALLOW_ONELEVEL, it also forbids nonsense
names like "nowhere". But that also breaks some probably-stupid cases
that are currently possible, like:

  git symbolic-ref refs/heads/foo FETCH_HEAD

I'm not really sure why anybody would want to do that, but it does work
currently. I'm tempted to say that the symref-reading code should
actually complain about following something outside of "refs/", but that
carries an even higher possibility of breaking somebody. But it seems
like we should be consistent between what we allow to be read, and what
we allow to be written.

At any rate, with the code as you have it above, I think the "make sure
HEAD starts with refs/" code is now redundant.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 9723c2827c..b194c1b09b 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -723,9 +723,9 @@ test_expect_success 'deleting a symref' '
>  '
>  
>  test_expect_success 'deleting a dangling symref' '
> -	git symbolic-ref refs/heads/dangling-symref nowhere &&
> +	git symbolic-ref refs/heads/dangling-symref refs/heads/nowhere &&
>  	test_path_is_file .git/refs/heads/dangling-symref &&
> -	echo "Deleted branch dangling-symref (was nowhere)." >expect &&
> +	echo "Deleted branch dangling-symref (was refs/heads/nowhere)." >expect &&

This is a sensible change. With ALLOW_ONELEVEL, it wouldn't be
necessary, but I think it is representing a more plausible real-world
scenario.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 6e66352558..427b06442d 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -2114,7 +2114,7 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '
>  
>  test_expect_success 'log diagnoses bogus HEAD symref' '
>  	git init empty &&
> -	git --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&
> +	echo "ref: refs/heads/invalid.lock" > empty/.git/HEAD &&
>  	test_must_fail git -C empty log 2>stderr &&
>  	test_i18ngrep broken stderr &&
>  	test_must_fail git -C empty log --default totally-bogus 2>stderr &&

After this change, I think this would need to be marked with a REFFILES
prereq similar to the test right before it.

-Peff

[0] Curiously there was a very similar patch to yours posted a while
    ago:

      https://lore.kernel.org/git/4FDE3D7D.4090502@elegosoft.com/

    There was some discussion and a followup:

      https://lore.kernel.org/git/1342440781-18816-1-git-send-email-mschub@elegosoft.com/

    but nothing seems to have been applied. I don't see any arguments
    against it there; I think the author simply didn't push it forward
    enough.

    There's also some bits in the sub-thread about limiting HEAD to
    "refs/heads/", which we couldn't quite do at the time. That might be
    worth revisiting, but definitely shouldn't hold up your patch.