From: Phillip Wood Date: Tue, 15 Oct 2024 14:05:58 GMT Subject: Re: [PATCH v7 1/6] refs: atomically record overwritten ref in update_symref Message-ID: In-Reply-To: Hi Bence On 13/10/2024 22:24, Bence Ferdinandy wrote: > > On Sun Oct 13, 2024 at 15:52, Phillip Wood 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