Patrick Steinhardt <ps@xxxxxx> writes:
> On Tue, May 14, 2024 at 02:44:06PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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.
Attachment:
signature.asc
Description: PGP signature
