Re: [PATCH 1/6] refs: create and use `ref_update_ref_must_exist()`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 16, 2024, 11:09 UTC
- Message-ID
- <ZkXpW0BPvR3vr2jx@tanuki>
- In-Reply-To
- <20240514124411.1037019-2-knayak@gitlab.com>
On Tue, May 14, 2024 at 02:44:06PM +0200, Karthik Nayak wrote:
Show 49 quoted lines
> 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?
Show 16 quoted lines
> 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