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

[Phillip Wood <phillip.wood123@gmail.com>]

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?

> (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.

Best Wishes

Phillip