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
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");
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.
Best Wishes
Phillip
