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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 14 2021, Jeff King wrote:

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

But will reword etc., thanks.

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

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.

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

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.

Perhaps it's informative that the below diff-on-top also passes all
tests, i.e. that we have largely the same
"refs_read_raw_ref(refs->packed_ref_store" copy/pasted in
files_read_raw_ref() in two adjacent places, we're just changing what
errno we pass upwards.

It thoroughly tramples on Han-Wen's series, and it's easier to deal with
(if at all) once his lands, just thought it might be interesting:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7e4963fd07..4a97cd48d9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -356,6 +356,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	int ret = -1;
 	int save_errno;
 	int remaining_retries = 3;
+	int lstat_bad_or_not_file = 0;
+	int lstat_errno = 0;
 
 	*type = 0;
 	strbuf_reset(&sb_path);
@@ -382,11 +384,28 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 		goto out;
 
 	if (lstat(path, &st) < 0) {
-		if (errno != ENOENT)
+		lstat_bad_or_not_file = 1;
+		lstat_errno = errno;
+	} else if (S_ISDIR(st.st_mode)) {
+		/*
+		 * Maybe it's an empty directory, maybe it's not, in
+		 * either case this ref does not exist in the files
+		 * backend (but may be packet), later code will handle
+		 * the "create and maybe remove_empty_directories()"
+		 * case if needed, or die otherwise.
+		 */
+		lstat_bad_or_not_file = 1;
+	}
+
+	if (lstat_bad_or_not_file) {
+		if (lstat_errno && lstat_errno != ENOENT)
 			goto out;
 		if (refs_read_raw_ref(refs->packed_ref_store, refname,
 				      oid, referent, type)) {
-			errno = ENOENT;
+			if (lstat_errno)
+				errno = ENOENT;
+			else
+				errno = EISDIR;
 			goto out;
 		}
 		ret = 0;
@@ -417,22 +436,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 		 */
 	}
 
-	/* Is it a directory? */
-	if (S_ISDIR(st.st_mode)) {
-		/*
-		 * Even though there is a directory where the loose
-		 * ref is supposed to be, there could still be a
-		 * packed ref:
-		 */
-		if (refs_read_raw_ref(refs->packed_ref_store, refname,
-				      oid, referent, type)) {
-			errno = EISDIR;
-			goto out;
-		}
-		ret = 0;
-		goto out;
-	}
-
 	/*
 	 * Anything else, just open it and try to use it as
 	 * a ref