undef/git-commit-vandalism
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refs/files: remove unused "errno == EISDIR" code

Browse Source 
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. E.g. in the case of
files_copy_or_rename_ref() our callstack will look something like:

	[...] ->
	files_copy_or_rename_ref() ->
	lock_ref_oid_basic() ->
	refs_resolve_ref_unsafe()

At that point the first (now only) refs_resolve_ref_unsafe() call in
lock_ref_oid_basic() would do the equivalent of this in the resulting
call to refs_read_raw_ref() in refs_resolve_ref_unsafe():

	/* 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, and
a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for writes,
2017-10-06) for the commit that changed the original codepath added in
bc7127ef0f to use this "EISDIR" handling.

Further historical commentary:

Before the two preceding commits the caller in files_reflog_expire()
was the only one out of our 4 callers that would pass non-NULL as an
oid. We would then set a (now gone) "resolve_flags" to
"RESOLVE_REF_READING" and just before that "errno != EISDIR" check do:

	if (resolve_flags & RESOLVE_REF_READING)
		return NULL;

There may have been some case where this ended up mattering and we
couldn't safely make this change before we removed the "oid"
parameter, but I don't think there was, see [1] for some discussion on
that.

In any case, now that we've removed the "oid" parameter in a preceding
commit we can be sure that this code is redundant, so let's remove it.

1. http://lore.kernel.org/git/871r801yp6.fsf@evledraar.gmail.com

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Ævar Arnfjörð Bjarmason 2021-08-23 13:36:13 +02:00 committed by Junio C Hamano
parent ff7a2e4dbb
commit 245fbba46d
1 changed files with 3 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
refs/files-backend.c
View File

@ -882,7 +882,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
struct strbuf ref_file = STRBUF_INIT;
struct ref_lock *lock;
int last_errno = 0;
int resolved;
files_assert_main_repository(refs, "lock_ref_oid_basic");
assert(err);
@ -890,30 +889,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
CALLOC_ARRAY(lock, 1);
files_ref_path(refs, &ref_file, refname);
resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
RESOLVE_REF_NO_RECURSE,
&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, NULL, NULL, err))
strbuf_addf(err, "there are still refs under '%s'",
refname);
goto error_return;
}
resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
RESOLVE_REF_NO_RECURSE,
&lock->old_oid, type);
}
if (!resolved) {
if (!refs_resolve_ref_unsafe(&refs->base, refname,
RESOLVE_REF_NO_RECURSE,
&lock->old_oid, type)) {
last_errno = errno;
if (last_errno != ENOTDIR ||
!refs_verify_refname_available(&refs->base, refname,