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

From: Jeff King <peff@peff.net>
Date: Jul 14, 2021, 16:21 UTC
Message-ID: <YO8PBBJZ2Q+5ZqFs@coredump.intra.peff.net>
In-Reply-To: <patch-1.1-de0838fe99-20210714T111351Z-avarab@gmail.com>

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

Show 20 quoted lines

> 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