| Message ID | 20241209-320-git-refs-migrate-reflogs-v1-4-d4bc37ee860f@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: add reflog support to `git refs migrate` | expand |
On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: > > 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 are also be needed in a following commit where the function s/are also be needed/will also be needed/ > 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 | 43 ++++++++++++++++++++++++++++--------------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/refs.c b/refs.c > index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 > --- a/refs.c > +++ b/refs.c > @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( > return update; > } > > +static int transaction_refname_verification(const char *refname, > + const struct object_id *new_oid, > + unsigned int flags, > + struct strbuf *err) We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while I couldn't find any 'yyy_verification()' function, so it might be better to name it 'transaction_refname_valid()' or maybe 'transaction_refname_ok()'. Also I think it should probably return a bool so 1 if the refname is valid and 0 otherwise, unless we have plans in the future to follow different code paths depending on the different ways it is not valid. > + ret = transaction_refname_verification(refname, new_oid, flags, err); > + if (ret) > + return ret; Then the above could be just: if (!transaction_refname_valid(refname, new_oid, flags, err)) return -1;
Christian Couder <christian.couder@gmail.com> writes: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: >> >> 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 are also be needed in a following commit where the function > > s/are also be needed/will also be needed/ > Will amend. >> 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 | 43 ++++++++++++++++++++++++++++--------------- >> 1 file changed, 28 insertions(+), 15 deletions(-) >> >> diff --git a/refs.c b/refs.c >> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( >> return update; >> } >> >> +static int transaction_refname_verification(const char *refname, >> + const struct object_id *new_oid, >> + unsigned int flags, >> + struct strbuf *err) > > We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while > I couldn't find any 'yyy_verification()' function, so it might be > better to name it 'transaction_refname_valid()' or maybe > 'transaction_refname_ok()'. > I think you're right, it helps to be consistent here. Will change to `transaction_refname_valid()`. > Also I think it should probably return a bool so 1 if the refname is > valid and 0 otherwise, unless we have plans in the future to follow > different code paths depending on the different ways it is not valid. > That is a good idea. >> + ret = transaction_refname_verification(refname, new_oid, flags, err); >> + if (ret) >> + return ret; > > Then the above could be just: > > if (!transaction_refname_valid(refname, new_oid, flags, err)) > return -1; Yup, also will remove the need for the `ret` variable.
On Wed, Dec 11, 2024 at 10:26:31AM +0100, Christian Couder wrote: > On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote: > > diff --git a/refs.c b/refs.c > > index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( > > return update; > > } > > > > +static int transaction_refname_verification(const char *refname, > > + const struct object_id *new_oid, > > + unsigned int flags, > > + struct strbuf *err) > > We have a number of functions named 'xxx_valid()' or 'xxx_ok()' while > I couldn't find any 'yyy_verification()' function, so it might be > better to name it 'transaction_refname_valid()' or maybe > 'transaction_refname_ok()'. > > Also I think it should probably return a bool so 1 if the refname is > valid and 0 otherwise, unless we have plans in the future to follow > different code paths depending on the different ways it is not valid. > > > + ret = transaction_refname_verification(refname, new_oid, flags, err); > > + if (ret) > > + return ret; > > Then the above could be just: > > if (!transaction_refname_valid(refname, new_oid, flags, err)) > return -1; The only question is whether we want to discern between an invalid refname and an error. But reading through the code it doesn't seem like we want to do that as we always return either `-1` on invalid names or `0` otherwise. So agreed, this is a good suggestion. Patrick
diff --git a/refs.c b/refs.c index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..732c236a3fd0cf324cc172b48d3d54f6dbadf4a4 100644 --- a/refs.c +++ b/refs.c @@ -1196,6 +1196,29 @@ struct ref_update *ref_transaction_add_update( return update; } +static int transaction_refname_verification(const char *refname, + const struct object_id *new_oid, + unsigned int flags, + struct strbuf *err) +{ + if (flags & REF_SKIP_REFNAME_VERIFICATION) + return 0; + + if (is_pseudo_ref(refname)) { + strbuf_addf(err, _("refusing to update pseudoref '%s'"), + refname); + return -1; + } 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 -1; + } + + return 0; +} + int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, @@ -1205,6 +1228,8 @@ int ref_transaction_update(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err) { + int ret; + assert(err); if ((flags & REF_FORCE_CREATE_REFLOG) && @@ -1213,21 +1238,9 @@ 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); - return -1; - } - - if (!(flags & REF_SKIP_REFNAME_VERIFICATION) && - is_pseudo_ref(refname)) { - strbuf_addf(err, _("refusing to update pseudoref '%s'"), - refname); - return -1; - } + ret = transaction_refname_verification(refname, new_oid, flags, err); + if (ret) + return ret; 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 are 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 | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-)