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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Han-Wen Nienhuys" <hanwen@google.com>,
	"Michael Haggerty" <mhagger@alum.mit.edu>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH] refs file backend: remove dead "errno == EISDIR" code
Date: Wed, 14 Jul 2021 13:17:14 +0200	[thread overview]
Message-ID: <patch-1.1-de0838fe99-20210714T111351Z-avarab@gmail.com> (raw)

When we lock a reference like "foo" we need to handle the case where
"foo" exists, but is an empty directory. That's what this code added
in bc7127ef0f (ref locking: allow 'foo' when 'foo/bar' used to exist
but not anymore., 2006-09-30) seems like it should be dealing with.

Except it doesn't, and we never take this branch. The reason is that
when bc7127ef0f was written this looked like:

	ref = resolve_ref([...]);
	if (!ref && errno == EISDIR) {
	[...]

And in resolve_ref() we had this code:

	fd = open(path, O_RDONLY);
	if (fd < 0)
		return NULL;

I.e. we would attempt to read "foo" with open(), which would fail with
EISDIR and we'd return NULL. We'd then take this branch, call
remove_empty_directories() and continue.

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;

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.

We then proceed with the entire ref update and don't call
remove_empty_directories() until we call commit_ref_update(). See
5387c0d883 (commit_ref(): if there is an empty dir in the way, delete
it, 2016-05-05) for the addition of that code.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

While working on a re-roll of the review/helping out Han-Wen with the
refs backend at [1] I discovered that this codepath in
lock_ref_oid_basic() has been unused for some time. In that series I
added a BUG() to it[2], but it's even better to remove it entirely.

I'll submit any proposed re-roll of [1] on top of this, I thought it
was better to review this isolated and more easily understood change
on its own.

1. https://lore.kernel.org/git/cover-00.17-00000000000-20210711T162803Z-avarab@gmail.com
2. https://lore.kernel.org/git/patch-07.17-10a40c9244e-20210711T162803Z-avarab@gmail.com/

 refs/files-backend.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 677b7e4cdd..7e4963fd07 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -941,26 +941,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	resolved = !!refs_resolve_ref_unsafe(&refs->base,
 					     refname, resolve_flags,
 					     &lock->old_oid, type);
-	if (!resolved && errno == EISDIR) {
-		/*
-		 * we are trying to lock foo but we used to
-		 * have foo/bar which now does not exist;
-		 * it is normal for the empty directory 'foo'
-		 * to remain.
-		 */
-		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
-			if (!refs_verify_refname_available(
-					    &refs->base,
-					    refname, extras, skip, err))
-				strbuf_addf(err, "there are still refs under '%s'",
-					    refname);
-			goto error_return;
-		}
-		resolved = !!refs_resolve_ref_unsafe(&refs->base,
-						     refname, resolve_flags,
-						     &lock->old_oid, type);
-	}
 	if (!resolved) {
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
-- 
2.32.0.851.g0fc62a9785

