Hey Phillip,
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> Hi Karthik
>
> I agree with Christian's comments on this patch, I've got a couple of
> additional comments below
>
> On 12/04/2024 10:59, Karthik Nayak wrote:
>> diff --git a/refs.c b/refs.c
>> index 55d2e0b2cb..967c81167e 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1228,6 +1228,7 @@ struct ref_update *ref_transaction_add_update(
>> const char *refname, unsigned int flags,
>> const struct object_id *new_oid,
>> const struct object_id *old_oid,
>> + const char *new_ref, const char *old_ref,
>> const char *msg)
>> {
>> struct ref_update *update;
>> @@ -1253,6 +1254,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>> const char *refname,
>> const struct object_id *new_oid,
>> const struct object_id *old_oid,
>> + const char *new_ref, const char *old_ref,
>> unsigned int flags, const char *msg,
>> struct strbuf *err)
>> {
>
> Adding these two new parameters is quite disruptive as all the existing
> callers have to be updated. It makes it easy for callers to misuse this
> function for example by providing old_oid and old_ref (I'm assuming that
> is an error but it is hard to know for sure without any documentation).
> It also makes the calling code harder to read because there are so many
> parameters it is hard to keep track of exactly what is being passed. An
> alternative strategy would be to add a new function that takes a struct
> instead of lots of individual parameters. That would make the calling
> code more readable as it would be clear which struct members are being
> set (see reset.h for an example of this). The approach of adding a
> struct is still prone to setting the wrong combination of options so
We do already have refs-internal.h:ref_update and this struct would be
the best candidate for what you're saying. I even thought of exposing
this struct and using it. I think I realized that it's a lot more work
to do this, because there are checks and cleanups which are built around
this struct. So exposing and using it would mean we refactor a bunch of
that code. Which while necessary I believe should be out of this series.
I'd actually be happy to do it right after we can agree that this is a
good direction.
> either way it would be helpful to add some assertions to detect mistakes
>
> if (old_oid && old_ref)
> BUG("Only one of old_oid and old_ref should be non NULL");
> if (new_oid && new_ref)
> BUG("Only one of new_oid and new_ref should be non NULL");
>
I have slightly modified it to:
if (old_oid && !is_null_oid(old_oid) && old_ref)
BUG("Only one of old_oid and old_ref should be non NULL");
if (new_oid && !is_null_oid(new_oid) && new_ref)
BUG("Only one of new_oid and new_ref should be non NULL");
But I agree, this is needed and have added it.
>> diff --git a/refs.h b/refs.h
>> index d278775e08..645fe9fdb8 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -696,13 +696,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>> */
>> #define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
>>
>> +/*
>> + * The reference update is considered to be done on a symbolic reference. This
>> + * ensures that we verify, delete, create and update the ref correspondingly.
>> + */
>> +#define REF_SYMREF_UPDATE (1 << 12)
>
> I'm confused as to why we need this as I assumed that we could use the
> presence of old_ref/new_ref to determine that the caller wants to update
> symbolic ref. Having this flag means that there are more possibilities
> to misuse the new API setting this flag but providing NULL for old_ref
> and new_ref.
>
I think I started with this flag but as the direction of the series
changed and I moved using zero_oid values for deletion or for using the
verify command, this is not really needed anymore. I just tried removing
all the code around the flags and fixing up things and all the tests
still pass. Thanks for brining this up.
Patrick Steinhardt <ps@xxxxxx> writes:
>> I'm confused as to why we need this as I assumed that we could use the
>> presence of old_ref/new_ref to determine that the caller wants to update
>> symbolic ref. Having this flag means that there are more possibilities to
>> misuse the new API setting this flag but providing NULL for old_ref and
>> new_ref.
>
> In my opinion the same comment applies to `REF_HAVE_NEW` and
> `REF_HAVE_OLD`, which I found to be redundant, as well. Those may make
> sense in the internals when the object IDs are stored as non-pointers,
> but queueing ref updates only accepts pointers anyway.
>
Yeah like you mentioned, since we're dealing with pointers, checking the
if its set is enough indication, which doesn't work with the static OID
values.
Attachment:
signature.asc
Description: PGP signature
