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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 19, 2024, 09:40 UTC
Message-ID: <ZiI8B4VOL3agHBgK@tanuki>
In-Reply-To: <c182c68c-77f6-422a-81ed-f6199c6b8a6b@gmail.com>

On Thu, Apr 18, 2024 at 04:08:00PM +0100, Phillip Wood wrote: [snip]

Show 18 quoted lines

> > 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