On Sun Oct 13, 2024 at 15:52, Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > Hi Bence > > On 13/10/2024 00:03, Bence Ferdinandy wrote: >> When updating a symref it's currently not possible to know for sure what >> was the previous value that was overwritten. > > It is if you use a ref transaction rather than call refs_update_symref() > and query the ref after calling ref_transaction_prepare() and before > calling ref_transaction_commit() which is what the code below does. Yeah, it would be more clear if that sentence would say "when using update_symref". > >> 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. (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? Thanks, Bence
