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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Just to keep things moving forward, here it is with a commit message. I
> left you as the author, but if you're OK with it, please tell Junio he
> can forge your sign-off.
>
> -- >8 --
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Subject: [PATCH] symbolic-ref: refuse to set syntactically invalid target
>
> You can feed absolute garbage to symbolic-ref as a target like:
>
>   git symbolic-ref HEAD refs/heads/foo..bar
>
> While this doesn't technically break the repo entirely (our "is it a git
> directory" detector looks only for "refs/" at the start), we would never
> resolve such a ref, as the ".." is invalid within a refname.
>
> Let's flag these as invalid at creation time to help the caller realize
> that what they're asking for is bogus.
>
> A few notes:
>
>   - We use REFNAME_ALLOW_ONELEVEL here, which lets:
>
>      git update-ref refs/heads/foo FETCH_HEAD
>
>     continue to work. It's unclear whether anybody wants to do something
>     so odd, but it does work now, so this is erring on the conservative
>     side. There's a test to make sure we didn't accidentally break this,
>     but don't take that test as an endorsement that it's a good idea, or
>     something we might not change in the future.

OK.  Even if it were HEAD, it does look like a funny thing to do to
point at a shallower ref with a more concrete ref.

>   - The test in t4202-log.sh checks how we handle such an invalid ref on
>     the reading side, so it has to be updated to touch the HEAD file
>     directly.
>
>   - We need to keep our HEAD-specific check for "does it start with
>     refs/". The ALLOW_ONELEVEL flag means we won't be enforcing that for
>     other refs, but HEAD is special here because of the checks in
>     validate_headref().

OK.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/symbolic-ref.c  |  2 ++
>  t/t1401-symbolic-ref.sh | 10 ++++++++++
>  t/t4202-log.sh          |  4 ++--
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
> index e547a08d6c..1b0f10225f 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], REFNAME_ALLOW_ONELEVEL) < 0)
> +			die("Refusing to set '%s' to invalid ref '%s'", argv[0], argv[1]);

Makes sense.  Rejecting syntactically invalid thing like double-dot
is something we should have done from day one.

> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> index 9fb0b90f25..0c204089b8 100755
> --- a/t/t1401-symbolic-ref.sh
> +++ b/t/t1401-symbolic-ref.sh
> @@ -165,4 +165,14 @@ test_expect_success 'symbolic-ref can resolve d/f name (ENOTDIR)' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'symbolic-ref refuses invalid target for non-HEAD' '
> +	test_must_fail git symbolic-ref refs/heads/invalid foo..bar
> +'

Good.

> +test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '
> +	git symbolic-ref refs/heads/top-level FETCH_HEAD &&
> +	git update-ref FETCH_HEAD HEAD &&
> +	test_cmp_rev top-level HEAD
> +'
>  test_done

Strange, but OK.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 6e66352558..f0aaa1fa02 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -2112,9 +2112,9 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '
>  	test_i18ngrep broken stderr
>  '
>  
> -test_expect_success 'log diagnoses bogus HEAD symref' '
> +test_expect_success REFFILES '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 &&

OK.

>  	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 &&