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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 14 2021, Jeff King wrote:

> On Wed, Jul 14, 2021 at 09:07:41PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > Isn't that pseudo-code missing a conditional that's there in the real
>> > code? In refs_resolve_ref_unsafe(), I see:
>> >
>> >        if (refs_read_raw_ref(refs, refname,
>> >                              oid, &sb_refname, &read_flags)) {
>> >                *flags |= read_flags;
>> >
>> >                /* In reading mode, refs must eventually resolve */
>> >                if (resolve_flags & RESOLVE_REF_READING)
>> >                        return NULL;
>> >
>> >                /*
>> >                 * Otherwise a missing ref is OK. But the files backend
>> >                 * may show errors besides ENOENT if there are
>> >                 * similarly-named refs.
>> >                 */
>> >                if (errno != ENOENT &&
>> >                    errno != EISDIR &&
>> >                    errno != ENOTDIR)
>> >                        return NULL;
>> >
>> > So if RESOLVE_REF_READING is set, we can return NULL immediately, with
>> > errno set to EISDIR. Which contradicts this:
>> 
>> I opted (perhaps unwisely) to elide that since as you note above we
>> don't take that path in relation to the removed code. I.e. I'm
>> describing the relevant codepath we take nowadays given the code & its
>> callers.
>
> It's not clear to me that we don't take that path, though. The call in
> files_reflog_expire() looks like it violates the assertion in your
> commit message (that we would never return NULL with errno as EISDIR).
>
> So I'm not entirely sure that the code is in fact dead (though I
> couldn't find an easy way to trigger it from the command line). I do
> think it probably can't do anything useful, and it is probably still OK
> to delete. But in my mind that is quite a different argument.
>
> 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.