On Mon, Sep 30, 2024 at 12:58:05AM +0200, Bence Ferdinandy wrote: > > On Sun Sep 22, 2024 at 00:19, Bence Ferdinandy <bence@xxxxxxxxxxxxxx> wrote: > > > > On Sat Sep 21, 2024 at 15:40, Phillip Wood <phillip.wood123@xxxxxxxxx> 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
