Hi Karthik
Sorry for the slow response on this
On 19/04/2024 16:47, Karthik Nayak wrote:
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.
I'm confused as to why we want to allow old_ref in conjunction with
old_oid being the null oid. Reading the documentation a null oid means
"this ref must not exist" and NULL means "I don't care about the current
state of the ref" so NULL and the null oid are not equivalent.
Best Wishes
Phillip
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.
