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

From: "Bence Ferdinandy" <[email-masked]>
Date: Fri, 20 Sep 2024 16:11:56 +0200

On Fri Sep 20, 2024 at 00:46, Junio C Hamano <[email-masked]> wrote:
> Bence Ferdinandy <[email-masked]> 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/[email-masked]/  

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