Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

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

Michael Haggerty <mhagger@alum.mit.edu> writes:

> If the user has asked that a new value be set for a reference, we use
> check_refname_format() to verify that the reference name satisfies all
> of the rules. But in other cases, at least check that refname_is_safe().

It isn't clear to me what "in other cases" exactly refers to.  A
request to delete a ref would obviously one of those that do not
"ask that a new value be set", but are there other cases?

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> There are remaining problems in this area of the code. For example,
> check_refname_format() is *less* strict than refname_is_safe(). It
> allows almost arbitrary top-level reference names like "foo" and
> "refs". (The latter is especially fun because if you manage to create
> a reference called "refs", Git stops recognizing the directory as a
> Git repository.) On the other hand, some callers call
> check_refname_format() with incomplete reference names (e.g., branch
> names like "master"), so the functions can't be made stricter until
> those callers are changed.
>
> I'll address these problems separately if I find the time.

Thanks.

>  refs.c                  | 5 +++--
>  t/t1400-update-ref.sh   | 2 +-
>  t/t1430-bad-ref-name.sh | 2 +-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 858b6d7..41eb9e2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
>  		die("BUG: REF_ISPRUNING set without REF_NODEREF");
>  
> -	if (new_sha1 && !is_null_sha1(new_sha1) &&
> -	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> +	if ((new_sha1 && !is_null_sha1(new_sha1)) ?
> +	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> +	    !refname_is_safe(refname)) {
>  		strbuf_addf(err, "refusing to update ref with bad name '%s'",
>  			    refname);
>  		return -1;
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 40b0cce..08bd8fd 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -23,7 +23,7 @@ test_expect_success setup '
>  m=refs/heads/master
>  n_dir=refs/heads/gu
>  n=$n_dir/fixes
> -outside=foo
> +outside=refs/foo
>  
>  test_expect_success \
>  	"create $m" \
> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> index 25ddab4..8937e25 100755
> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '
>  	echo precious >expect &&
>  	test_must_fail git update-ref -d my-private-file >output 2>error &&
>  	test_must_be_empty output &&
> -	test_i18ngrep -e "cannot lock .*: unable to resolve reference" error &&
> +	test_i18ngrep -e "refusing to update ref with bad name" error &&
>  	test_cmp expect .git/my-private-file
>  '