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

From: karthik nayak <karthik.188@gmail.com>
Date: Dec 20, 2024, 10:30 UTC
Message-ID: <CAOLa=ZRnwqsChx9SiMv_95Leyijh_E7zEhkeHQOea=gjfGZ-Qg@mail.gmail.com>
In-Reply-To: <87a5crh3oy.fsf@iotcl.com>

Toon Claes <toon@iotcl.com> writes:

Show 16 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>> +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?
>

You mean we could simply have

  if { ... check for pseudoref ... }

  if { ... check for bad refname ... }

  return -1;

then, you're right, that would work too. No specific reason that I added an else.

Show 16 quoted lines

>> +	} 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?
>

I actually didn't notice that I did swap them. It doesn't change the logic. However, for creation of a pseudoref, in the old flow, we'd check if the refname is safe and then go to the section where we check for pseudorefs. Now we simply skip to the pseudoref check. I'll add more details in the commit message locally for now and will include it if I do re-roll!

> --
> Toon