Han-Wen Nienhuys <hanwen@xxxxxxxxxx> writes: > On Wed, Jul 1, 2020 at 2:03 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: >> > Do you have an opinion on >> > https://public-inbox.org/git/pull.665.git.1592580071.gitgitgadget@xxxxxxxxx/ >> > ? >> > >> > There is some overlap with in sequencer.c, and Phillip's approach is >> > likely more principled, so I'd like to base reftable on that. >> >> I assumed that these were offered to you as possible improvements to >> be folded into your series, so I didn't read them very carefully and >> I didn't queue them myself. I expected that I would see them, >> possibly modified to fit the context better, as part of your series >> sent from you, perhaps to become a part of early clean-up portion of >> your topic. > > They are changing the signature of widely used functions, which is > useful for my series but not completely necessary. I would rather that > someone else decides on how to go forward with the series. I was about to say that having written one ref backend, you are likely to have better context to judge how much better Phillip's patches would make the world be than I do ;-), but having worked on the "cleanse caller-supplied reflog message at generic layer" illustration patch, I think I am familiar enough with these three functions Phillip's patch touched to comment on them. I am not sure why delete_ref() needs to be modified to take a caller-supplied repository when refs_delete_ref() is already even a better way to do so (it allows the caller to give a ref_store, which is a part of a repository). The same between update_ref() and refs_update_ref(). Introducing a new repo_delete_ref() to extend the API into a three-tuple, delete_ref(...) { return repo_delete_ref(the_repository, ...); } repo_delete_ref(repo, ...) { return refs_delete_ref(get_main_ref_store(repo), ...); } refs_delete_ref(...) { /* as we already have it */ } _might_ make sense, but then we should do so for not just delete and update, but for all the operations consistently. I do not know if that is worth it offhand. The issue Phillip's patch addresses looks like a mere "lack of convenience", not a fundamental "without this we cannot write correct code easily", at least to me. What disturbed me more while I was looking at refs.c was that some operations are not done (perhaps cannot be done) as a part of a transaction. refs_delete_refs() and refs_rename_ref() directly call into the backend layer, for example, and that does not smell right.
