Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>,
	git@vger.kernel.org, Han-Wen Nienhuys <hanwen@google.com>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH] refs file backend: remove dead "errno == EISDIR" code
Date: Sat, 17 Jul 2021 23:36:59 +0200	[thread overview]
Message-ID: <87mtqkwrc6.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqk0lppw49.fsf@gitster.g>

On Fri, Jul 16 2021, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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@gmail.com/
>>> 
>>> 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@gmail.com/
>>> 
>>> 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@gmail.com/

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.