On Fri Sep 20, 2024 at 00:46, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Bence Ferdinandy <bence@xxxxxxxxxxxxxx> writes: [ snip] > If I wanted to create a symref that points at A, there are three cases: > > (1) the symref does not exist. > (2) the symref exists and points at A. > (3) the symref exists and points at B. > > I'll see a symref that points at A at the end in the first two > cases, and my request is silently ignored in the third case. > > I'd expect that the caller can tell the failing case apart from the > successful case with the return value or something. The caller > might want to tell between the first two cases for reporting > purposes, but I do not care as much as I would care about detecting > true failures. Hmm. So in case I'm passing REF_CREATE_ONLY I would not expect the above cases to be error in the sense that transaction_finish should not report a failure and thus have all callers assume things went wrong. On the other hand it's a valid concern, that the caller may want to check what happened. Actually, the idea that I mentioned in https://lore.kernel.org/git/D4AK4USDVP5T.10INJOFE2I8LE@xxxxxxxxxxxxxx/ may actually be useful here as well. We could record the state of the reference atomically before the transaction in update, and then if the caller is interested, they can match this against what they requested. That way they can figure out which of the 3 cases they were in without raceconditions after the situation. Actually, this way any feedback could be given to the user post transaction here as well. If this sounds sensible, then I guess it would make sense to rejoin the set-head patch into this thread as well ... [snip] > > > diff --git a/refs.c b/refs.c > > index ceb72d4bd7..7afe46cadc 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -2085,8 +2085,9 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct > > return peel_object(r, base, peeled) ? -1 : 0; > > } > > > > + > > int refs_update_symref(struct ref_store *refs, const char *ref, > > - const char *target, const char *logmsg) > > + const char *target, const unsigned int extra_flags, const char *logmsg) > > While it is not _wrong_ per-se to mark an "unsigned int" parameter > as "const", it is a bit unusual in this code base. The only thing > it prevents us from doing is to mutate it until this function > returns, which does not help all that much in making the code safer, > as opposed to marking a parameter of a pointer type as a const > pointer. Makes sense, I'll drop it. Thanks very much for your patience! Best, Bence -- bence.ferdinandy.com
