From: karthik nayak Date: Fri, 20 Dec 2024 10:30:43 GMT Subject: Re: [PATCH v4 4/8] refs: extract out refname verification in transactions Message-ID: In-Reply-To: <87a5crh3oy.fsf@iotcl.com> Toon Claes writes: > Karthik Nayak 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