On Mon Sep 30, 2024 at 08:40, Patrick Steinhardt <ps@xxxxxx> wrote: > 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 You make a convincing argument :) I'll try to see if I can add another transaction error code for when you only want to create not overwrite and the ref already exists and pass it up (for both files and reftables, you also don't mention it, but I think this would not concern packed after a quick glance at the code). Best, Bence -- bence.ferdinandy.com
