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@xxxxxxxxx>
---
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@xxxxxxxxx
2. https://lore.kernel.org/git/patch-07.17-10a40c9244e-20210711T162803Z-avarab@xxxxxxxxx/
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
