Re: [PATCH v2 1/7] refs: accept symref values in `ref_transaction[_add]_update`

[Phillip Wood <phillip.wood123@gmail.com>]

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