Re: [PATCH v7 1/6] refs: atomically record overwritten ref in update_symref

["Bence Ferdinandy" <bence@ferdinandy.com>]

On Tue Oct 15, 2024 at 16:05, Phillip Wood <phillip.wood123@gmail.com> wrote:
> Hi Bence
>
> On 13/10/2024 22:24, Bence Ferdinandy wrote:
>> 
>> On Sun Oct 13, 2024 at 15:52, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> On 13/10/2024 00:03, Bence Ferdinandy wrote:
>>>> Record the value after the
>>>> ref has been locked if the caller of refs_update_symref requests it via
>>>> a new variable in the function call.
>>>
>>> To me this patch and patch 5 feel quite disruptive to all the existing
>>> callers which don't need this specialized functionality. I think it
>>> would be less disruptive over all if you used a ref transaction rather
>>> than calling refs_update_symref() in the final patch. That would enable
>>> us to keep the simpler interface for refs_update_symref().
>> 
>> The extra parameter introduced here is actually used in two places by the end
>> of the series, in remote set-head and fetch (of course you could make a similar
>> argument for the functionality added in 5/6 which is only used in fetch by the
>> final patch). To avoid code duplication I think even if we did not touch
>> refs_update_symref() it would make sense to create
>> a "refs_update_symref_extended()" and make refs_update_symref() a wrapper
>> around that with a few less parameters. That would be similar to how
>> refs_update_symref() and refs_update_ref() predetermine a couple of parameters
>> to say transaction_update().
>> 
>> Currently there are 15 calls to refs_update_symref() in total, of these
>> 5 do not use the complete functionality of the function (they pass NULL as
>> logmsg), so the current implementation would not be completely unprecedented.
>
> As those figures show it's pretty unusual not to pass a reflog message 
> when updating a ref, on the other hand it is very unusual to want the 
> old value so I don't think the two are comparable. At a high level the 
> two callers that want to be able to check the old value are both doing 
> essentially the same thing so can we create a specialized function that 
> encapsulates the functionality needed by --set-head and uses a ref 
> transaction?

Ok, so let's not change the signature of refs_update_symref(). On the other
hand the best approach here is still not quite clear to me. As it is now, the
series added two extra parameters to refs_update_symref(), the referent and the
create_only option. Obviously any new function would also take all the
parameters refs_update_symref takes. Should I rename the current (v8)
refs_update_symref to something else (name suggestion would be welcome,
refs_update_symref_extended?, refs_update_symref_check_current? checking the
currently existing symref is the common theme for the two new parameters) and
leave refs_update_symref as it is? Or is this really about just not having to
change all the already existing callers? Because in that case I'd just wrap the
new function, something like

int refs_update_symref(struct ref_store *refs, const char *ref,
		       const char *target, const char *logmsg)
	
	return refs_update_symref_extended(*refs, *ref, *target, *logmsg, NULL, 0);

Let me know what you think, I'll hold off on a v9 until I see more clearly here
on what would be preferred.

>
>> (This tally did make me catch an error on my side: the logmsg in fetch's
>> set_head should be "fetch" not "remote set-head", I'll fix that in a v8).
>> 
>> Imho, even if I manage to come up with a better name than
>> "refs_update_symref_extended()" wouldn't it be more confusing to have two ways
>> to update symrefs via a function call, rather than have one, where _usually_
>> you pass two NULL-s at the end?
>> 
>>>
>>> I'm also not sure about the proposed interface I would have thought it
>>> would be simpler to take a "char**" rather than an "struct strbuf*" if
>>> we do decide that it is useful for callers of refs_update_symref() to
>>> query the old value.
>> 
>> refs_read_symbolic_ref requires a strbuf, so one would need to be created
>> anyway and I also sort of got the feeling that the project likes to handle refs
>> in strbufs (which may be wrong). Are there any downsides I'm not seeing?
>
> It's true that refs_read_symbolic_ref takes and strbuf. I'd argue that's 
> a mistake for a function that is just returning a string in an "out" 
> parameter as I think it is more continent for the caller not to have to 
> initialize an strbuf just to retrieve the target of a symbolic ref. I 
> alse think that it is inconsistent with functions like 
> refs_resolve_refdup() that return a string.

Ok, I'll change this to use **char. On the other hand, if
refs_read_symbolic_ref is inconsistent would it make sense to change it to also
use a **char instead of strbuf? There's only four calls to it including the one
I just added. Although I might rather do that _after_ this series is resolved :)

Thanks (to everyone) for your time!

Best,
Bence


-- 
bence.ferdinandy.com