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

> 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).