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