Re: [PATCH v3 1/2] update_symref: add REF_CREATE_ONLY option

[Patrick Steinhardt <ps@pks.im>]

On Mon, Sep 30, 2024 at 12:58:05AM +0200, Bence Ferdinandy wrote:
> 
> On Sun Sep 22, 2024 at 00:19, Bence Ferdinandy <bence@ferdinandy.com> wrote:
> >
> > On Sat Sep 21, 2024 at 15:40, Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> On 19/09/2024 13:13, Bence Ferdinandy wrote:
> >> > Add a new REF_CREATE_ONLY flag for use by the files backend which will
> >> > only update the symref if it doesn't already exist. Add the possibility
> >> > to pass extra flags to refs_update_symref so that it can utilize this
> >> > new flag.
> >>
> >> I'm not sure we need a new flag to do this as it is already supported by
> >> the ref transaction api.
> >
> > Thanks, I was not aware of ref_transaction_create. It also seems to return with
> > TRANSACTION_NAME_CONFLICT so we should be able to see from the error code if
> > indeed the existence was the problem or something else went wrong.
> 
> Unfortunately, it seems that my reading of the code did not pass practice. When
> using ref_transaction_create ref_transaction_commit will return with -2 if the
> reference already exists, but it also returns with -2 for various other issues,
> like if the lock file already exists. I could parse the error message to see
> what was the cause, but that doesn't feel like a robust solution. Since fetch
> should _not_ error out on this, I think the REF_CREATE_ONLY flag is warranted.
> As it stands, it would serve a different purpose than ref_transaction_create,
> i.e. a "silent" create-only. 
> 
> I'll send a v4 tomorrow hopefully.

I don't think that is a good reason to introduce this new flag though.
If we need to have a proper way to identify this specific failure case
we should rather update the already-existing mechanism to give us useful
signals, shouldn't we?

The problem with this flag is that it basically duplicates functionality
that already exists, and it needs to be wired up by every ref backend
that we have and that we're adding in the future. Your patch for example
only implements the functionality for the "files" backend, but it must
also be wired up for the "reftable" backend or otherwise it would be
broken.

Another issue is that it gives you more ways to create nonsensical ref
updates. With it you could for example create requests with a non-zero
old object ID, and if it has `REF_CREATE_ONLY` set it would never be
possible to fulfill the request. There's probably other cases where you
can create nonsensical ref updates already, but we shouldn't add more
ways of doing that.

Mind you, if we go the way I propose and improve the error reporting
we'd also have to adapt both backends to do so. But that would be
plugging a gap for which we have no proper solution right now instead of
circumventing the current design by duplicating the functionality that
we already have in a way that makes us able to handle this.

Patrick