Toon Claes <toon@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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.
>> + } 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
Attachment:
signature.asc
Description: PGP signature
