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

From: Patrick Steinhardt <[email-masked]>
Date: Fri, 19 Apr 2024 11:40:23 +0200

On Thu, Apr 18, 2024 at 04:08:00PM +0100, Phillip Wood wrote:
[snip]
> > 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.

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.

Patrick