On Tue Oct 15, 2024 at 19:25, Bence Ferdinandy <bence@xxxxxxxxxxxxxx> wrote: > > On Tue Oct 15, 2024 at 16:05, Phillip Wood <phillip.wood123@xxxxxxxxx> 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
