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

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

> > 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().
> 
> In general (and not being too familiar with this area) and per:
> 
>     7521cc4611 (refs.c: make delete_ref use a transaction, 2014-04-30)
>     92b1551b1d (refs: resolve symbolic refs first, 2016-04-25)
>     029cdb4ab2 (refs.c: make prune_ref use a transaction to delete the ref, 2014-04-30)
> 
> And:
> 
>     https://lore.kernel.org/git/20140902205841.GA18279@google.com/    
> 
> I wonder if these remaining cases can be migrated over to lock_raw_ref()
> or the transaction API, as many other similar callers have been already.
> 
> But that's a bigger change, I won't be doing that now, just wondering if
> these are some #leftoverbits or if there's a good reason they were left.

> > So...I think it's fine? But the argument in your commit message seems to
> > have missed this case entirely.
> 
> Perhaps more succinctly: If we have a directory in the way, it's going
> to be impossible for the "old_oid" condition to be satisfied in any case
> in the file backend.
> 
> Even if we still had a caller that did "care" about that what could they
> hope to get from an "old_oid=<some-OID>" for a lock on "foo/bar" where
> "foo" is an empty directory?
> 
> Except of course for the case where it's not a directory but packed, but
> as you noted that's handled in another case.