| Message ID | 20241216-320-git-refs-migrate-reflogs-v4-4-d7cd3f197453@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | add2c4f6e225083747e86c2e2e89e80c97b28733 |
| Headers | show |
| Series | refs: add reflog support to `git refs migrate` | expand |
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 Claes <toon@iotcl.com> writes: > 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. >> + } 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
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; + } 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; + } + + return 1; +} + int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, @@ -1213,21 +1235,8 @@ int ref_transaction_update(struct ref_transaction *transaction, return -1; } - if (!(flags & REF_SKIP_REFNAME_VERIFICATION) && - ((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); + if (!transaction_refname_valid(refname, new_oid, flags, err)) return -1; - } - - if (!(flags & REF_SKIP_REFNAME_VERIFICATION) && - is_pseudo_ref(refname)) { - strbuf_addf(err, _("refusing to update pseudoref '%s'"), - refname); - return -1; - } if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS) BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
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(-)