Hey Phillip, Phillip Wood 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 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.