On Fri, Jul 16 2021, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > >> On Thu, Jul 15, 2021 at 02:02:40AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >>> > Maybe that is splitting hairs, but I definitely try to err on the side >>> > of caution and over-analysis when touching tricky code (and the >>> > ref-backend code is in my experience one of the trickiest spots for >>> > corner cases, races, etc). >>> >>> I'd entirely forgotten about this, but I had a patch to remove that >>> "oid" call entirely, as it's really an unrelated bug/undesired behavior >>> >>> You also looked at it at the time & Michael Haggerty reviewed it: >>> https://lore.kernel.org/git/20190315155959.12390-9-avarab@xxxxxxxxx/ >>> >>> The state of that patch was that I needed to get to some minor issues >>> with it (commit message rewording, cleaning up some related callers), >>> but the fundamental approach seemed good. >>> >>> I then split it off from the v4 of that series to get the non-tricky >>> changes in: >>> https://lore.kernel.org/git/20190328161434.19200-1-avarab@xxxxxxxxx/ >>> >>> I then just never got to picking it up again, I'll probably re-roll it & >>> make it a part of this series, then we can remove this whole OID != NULL >>> case and will be sure nothing fishy's going on. >> >> Yeah, that sounds like a good path forward. I do think the patch under >> discussion here is probably the right thing to do. But it becomes all >> the more obvious if lock_ref_oid_basic() ends up dropping that parameter >> entirely. > > OK, so what's the final verdict on this step? It is unfortunate > that when Ævar took over a topic from Han-Wen, this patch has been > inserted as the very first step before the patches in the series, so > until we know we are happy with it, it takes several other patches > hostage. I'm just interested in the end result landing sooner than later, so if you think this re-imagining of it hinders more than helps I'm happy to discard it and just take the last version Han-Wen submitted, i.e. the v5: https://lore.kernel.org/git/pull.1012.v5.git.git.1625684869.gitgitgadget@xxxxxxxxx/ I can then re-roll whatever I've come up here that I still find useful on that after it lands. I just thought that given the complexity of the ref code tying any loose ends up before those changes would help, but maybe not. Anyway, you/Han-Wen let me know. I'm happy to re-roll what I have outstanding here based on feedback, but also just to discard it for now and go with his version. I'll hold on any re-rolls in that area pending feedback on what you two would like to do.
