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

On Wed, Jul 14 2021, Jeff King wrote:

On Wed, Jul 14, 2021 at 09:07:41PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

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/ (local)

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/ (local)

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.