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

From: "Bence Ferdinandy" <[email-masked]>
Date: Sun, 20 Oct 2024 00:53:58 +0200

On Tue Oct 15, 2024 at 19:25, Bence Ferdinandy <[email-masked]> wrote:
>
> On Tue Oct 15, 2024 at 16:05, Phillip Wood <[email-masked]> wrote:
[snip]
>>>>
>>>> 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 :)

I started doing this change, but ran into two things which I would yet again,
bring up in defence of strbuf. So not only refs_read_symbolic_ref makes use of strbuf, but

a) I also use it in refs_update_symref[_extended] to check if the caller
actually wants that referent or not (passing a NULL or a strbuf).
b) the consumer, report_set_head_auto check for !buf->len

Both of these sound way more convenient with strbuf than with *char. Ofc I'm
not exactly a C guru so ... Anyhow, v9 does not have this change.

Best,
Bence