On Tue Oct 15, 2024 at 19:25, Bence Ferdinandy [off-list ref] wrote:

On Tue Oct 15, 2024 at 16:05, Phillip Wood [off-list ref] wrote:

[snip]

quoted

quoted

quoted

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