| Message ID | 20240514124411.1037019-2-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | update-ref: add symref support for --stdin | expand |
On Tue, May 14, 2024 at 02:44:06PM +0200, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> > > The files and reftable backend, need to check if a ref must exist, so > that the required validation can be done. A ref must exist only when the > `old_oid` value of the update has been explicitly set and it is not the > `null_oid` value. > > Since we also support symrefs now, we need to ensure that even when > `old_target` is set a ref must exist. While this was missed when we > added symref support in transactions, there are no active users of this > path. As we introduce the 'symref-verify' command in the upcoming > commits, it is important to fix this. > > So let's export this to a function called `ref_update_ref_must_exist()` > and expose it internally via 'refs-internal.h'. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > refs.c | 6 ++++++ > refs/files-backend.c | 3 +-- > refs/refs-internal.h | 6 ++++++ > refs/reftable-backend.c | 2 +- > 4 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/refs.c b/refs.c > index fa5471d219..59858fafdb 100644 > --- a/refs.c > +++ b/refs.c > @@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, > referent, update->old_target); > return -1; > } > + > +int ref_update_ref_must_exist(struct ref_update *update) > +{ > + return (update->flags & REF_HAVE_OLD) && > + (!is_null_oid(&update->old_oid) || update->old_target); > +} > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 3957bfa579..2df204f891 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, > struct strbuf *err) > { > struct strbuf referent = STRBUF_INIT; > - int mustexist = (update->flags & REF_HAVE_OLD) && > - !is_null_oid(&update->old_oid); > + int mustexist = ref_update_ref_must_exist(update); Okay. So we didn't notice this was broken because even though we started writing symrefs via transactions now, none of the calles ever assert that the old ref exists? > int ret = 0; > struct ref_lock *lock; > > diff --git a/refs/refs-internal.h b/refs/refs-internal.h > index 53a6c5d842..5da3029e6c 100644 > --- a/refs/refs-internal.h > +++ b/refs/refs-internal.h > @@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update); > int ref_update_check_old_target(const char *referent, struct ref_update *update, > struct strbuf *err); > > +/* > + * Check if the ref must exist, this means that the old_oid or > + * old_target is non NULL. > + */ > +int ref_update_ref_must_exist(struct ref_update *update); Seeing `ref_update_ref_must_exist()` as a standalone function wouldn't quite tell me what it really does. It sounds a bit like this would already assert the ref exists at the time of calling it. We could call this `ref_upate_expects_existing_old_ref()`, which might clarify the intent a bit. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Tue, May 14, 2024 at 02:44:06PM +0200, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> >> >> The files and reftable backend, need to check if a ref must exist, so >> that the required validation can be done. A ref must exist only when the >> `old_oid` value of the update has been explicitly set and it is not the >> `null_oid` value. >> >> Since we also support symrefs now, we need to ensure that even when >> `old_target` is set a ref must exist. While this was missed when we >> added symref support in transactions, there are no active users of this >> path. As we introduce the 'symref-verify' command in the upcoming >> commits, it is important to fix this. >> >> So let's export this to a function called `ref_update_ref_must_exist()` >> and expose it internally via 'refs-internal.h'. >> >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> >> --- >> refs.c | 6 ++++++ >> refs/files-backend.c | 3 +-- >> refs/refs-internal.h | 6 ++++++ >> refs/reftable-backend.c | 2 +- >> 4 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/refs.c b/refs.c >> index fa5471d219..59858fafdb 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, >> referent, update->old_target); >> return -1; >> } >> + >> +int ref_update_ref_must_exist(struct ref_update *update) >> +{ >> + return (update->flags & REF_HAVE_OLD) && >> + (!is_null_oid(&update->old_oid) || update->old_target); >> +} >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 3957bfa579..2df204f891 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, >> struct strbuf *err) >> { >> struct strbuf referent = STRBUF_INIT; >> - int mustexist = (update->flags & REF_HAVE_OLD) && >> - !is_null_oid(&update->old_oid); >> + int mustexist = ref_update_ref_must_exist(update); > > Okay. So we didn't notice this was broken because even though we started > writing symrefs via transactions now, none of the calles ever assert > that the old ref exists? > Yup, that's correct. The `git-symbolic-ref(1)` command doesn't ever check for the old value and it is the only user of transactional symrefs at this point. >> int ret = 0; >> struct ref_lock *lock; >> >> diff --git a/refs/refs-internal.h b/refs/refs-internal.h >> index 53a6c5d842..5da3029e6c 100644 >> --- a/refs/refs-internal.h >> +++ b/refs/refs-internal.h >> @@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update); >> int ref_update_check_old_target(const char *referent, struct ref_update *update, >> struct strbuf *err); >> >> +/* >> + * Check if the ref must exist, this means that the old_oid or >> + * old_target is non NULL. >> + */ >> +int ref_update_ref_must_exist(struct ref_update *update); > > Seeing `ref_update_ref_must_exist()` as a standalone function wouldn't > quite tell me what it really does. It sounds a bit like this would > already assert the ref exists at the time of calling it. > > We could call this `ref_upate_expects_existing_old_ref()`, which might > clarify the intent a bit. > > Patrick Yeah, that's better, will change. Thanks.
diff --git a/refs.c b/refs.c index fa5471d219..59858fafdb 100644 --- a/refs.c +++ b/refs.c @@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, referent, update->old_target); return -1; } + +int ref_update_ref_must_exist(struct ref_update *update) +{ + return (update->flags & REF_HAVE_OLD) && + (!is_null_oid(&update->old_oid) || update->old_target); +} diff --git a/refs/files-backend.c b/refs/files-backend.c index 3957bfa579..2df204f891 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, struct strbuf *err) { struct strbuf referent = STRBUF_INIT; - int mustexist = (update->flags & REF_HAVE_OLD) && - !is_null_oid(&update->old_oid); + int mustexist = ref_update_ref_must_exist(update); int ret = 0; struct ref_lock *lock; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 53a6c5d842..5da3029e6c 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update); int ref_update_check_old_target(const char *referent, struct ref_update *update, struct strbuf *err); +/* + * Check if the ref must exist, this means that the old_oid or + * old_target is non NULL. + */ +int ref_update_ref_must_exist(struct ref_update *update); + #endif /* REFS_REFS_INTERNAL_H */ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 98cebbcf39..975061d103 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -825,7 +825,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, ¤t_oid, &referent, &u->type); if (ret < 0) goto done; - if (ret > 0 && (!(u->flags & REF_HAVE_OLD) || is_null_oid(&u->old_oid))) { + if (ret > 0 && !ref_update_ref_must_exist(u)) { /* * The reference does not exist, and we either have no * old object ID or expect the reference to not exist.