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

On Wed, Jul 14, 2021 at 01:17:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

Since a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for
writes, 2017-10-06) we don't, because our our callstack will look
something like:

    files_copy_or_rename_ref() -> lock_ref_oid_basic() -> refs_resolve_ref_unsafe()

And then the refs_resolve_ref_unsafe() call here will in turn (in the
code added in a1c1d8170d) do the equivalent of this (via a call to
refs_read_raw_ref()):

	/* Via refs_read_raw_ref() */
	fd = open(path, O_RDONLY);
	if (fd < 0)
		/* get errno == EISDIR */
	/* later, in refs_resolve_ref_unsafe() */
	if ([...] && errno != EISDIR)
		return NULL;
	[...]
	/* returns the refs/heads/foo to the caller, even though it's a directory */
	return refname;

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.e. even though we got an "errno == EISDIR" we won't take this
branch, since in cases of EISDIR "resolved" is always
non-NULL. I.e. we pretend at this point as though everything's OK and
there is no "foo" directory.

So when is RESOLVE_REF_READING set? The resolve_flags parameter is
passed in by the caller. In lock_ref_oid_basic(), it comes from this:

    int mustexist = (old_oid && !is_null_oid(old_oid));
    [...]
    if (mustexist)
            resolve_flags |= RESOLVE_REF_READING;

So do any callers pass in old_oid? Surprisingly few. It used to be
called from other locking functions, but these days it looks like it is
only files_reflog_expire().

I'm not sure if this case is important or not. If we're expecting the
ref to exist, then an in-the-way directory is going to mean failure
either way. It could still exist within the packed-refs file, but then
refs_read_raw_ref() would not return failure.

So...I think it's fine? But the argument in your commit message seems to
have missed this case entirely.

-Peff