Re: [PATCH v4 4/8] refs: extract out refname verification in transactions

[Toon Claes <toon@iotcl.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,
> the refname of the update is verified for:
>
>   - Ensuring it is not a pseudoref.
>   - Checking the refname format.
>
> These checks will also be needed in a following commit where the
> function to add reflog updates to the transaction is introduced. Extract
> the code out into a new static function.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..9c9f4940c60d3cdd34ce8f1e668d17b9da3cd801 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1196,6 +1196,28 @@ struct ref_update *ref_transaction_add_update(
>  	return update;
>  }
>  
> +static int transaction_refname_valid(const char *refname,
> +				     const struct object_id *new_oid,
> +				     unsigned int flags, struct strbuf *err)
> +{
> +	if (flags & REF_SKIP_REFNAME_VERIFICATION)
> +		return 1;
> +
> +	if (is_pseudo_ref(refname)) {
> +		strbuf_addf(err, _("refusing to update pseudoref '%s'"),
> +			    refname);
> +		return 0;

With this early return you don't need the `else` below? Why did you add
it?

> +	} else if ((new_oid && !is_null_oid(new_oid)) ?
> +		 check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> +		 !refname_is_safe(refname)) {
> +		strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
> +			    refname);
> +		return 0;
> +	}

I see you've swapped order of checking whether it's a pseudoref with
checking whether the format is okay. I think this shouldn't make a big
difference, but it will give a different error message when attempting
to update an illformatted pseudoref. For me it makes more sense how
you've done it now. But because you mention both checks as bullet points
in the commit message, do you think it would make sense to say something
about them being swapped?

-- 
Toon