Hi Bence
On 13/10/2024 22:24, Bence Ferdinandy wrote:
On Sun Oct 13, 2024 at 15:52, Phillip Wood <phillip.wood123@xxxxxxxxx> 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
