| Message ID | patch-1.1-de0838fe99-20210714T111351Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs file backend: remove dead "errno == EISDIR" code | expand |
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
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
On Wed, Jul 14, 2021 at 09:07:41PM +0200, Ævar Arnfjörð Bjarmason wrote: > > 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. It's not clear to me that we don't take that path, though. The call in files_reflog_expire() looks like it violates the assertion in your commit message (that we would never return NULL with errno as EISDIR). So I'm not entirely sure that the code is in fact dead (though I couldn't find an easy way to trigger it from the command line). I do think it probably can't do anything useful, and it is probably still OK to delete. But in my mind that is quite a different argument. Maybe that is splitting hairs, but I definitely try to err on the side of caution and over-analysis when touching tricky code (and the ref-backend code is in my experience one of the trickiest spots for corner cases, races, etc). > > 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. Quite possibly. It's been a while since I've looked this deep at the ref code. It is weird that only one remaining caller passes old_oid. If even that one could be converted, the whole lock_ref_oid_basic() could be simplified a bit. I agree that's a bigger change, so it might make sense to do smaller cleanups in the interim. > > 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. Yeah, I think that's reasonably compelling. It's possible there are some races unaccounted for here (like somebody else creating and deleting shared-prefix loose refs at the same time), but it may be OK to just accept those. The code is "we saw a failure, see if deleting stale directories helps". And if the worst case is that this doesn't kick in an obscure race (where we'd probably end up failing the whole operation anyway), that's OK. -Peff
On Wed, Jul 14 2021, Jeff King wrote: > On Wed, Jul 14, 2021 at 09:07:41PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > 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. > > It's not clear to me that we don't take that path, though. The call in > files_reflog_expire() looks like it violates the assertion in your > commit message (that we would never return NULL with errno as EISDIR). > > So I'm not entirely sure that the code is in fact dead (though I > couldn't find an easy way to trigger it from the command line). I do > think it probably can't do anything useful, and it is probably still OK > to delete. But in my mind that is quite a different argument. > > Maybe that is splitting hairs, but I definitely try to err on the side > of caution and over-analysis when touching tricky code (and the > ref-backend code is in my experience one of the trickiest spots for > corner cases, races, etc). I'd entirely forgotten about this, but I had a patch to remove that "oid" call entirely, as it's really an unrelated bug/undesired behavior You also looked at it at the time & Michael Haggerty reviewed it: https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/ The state of that patch was that I needed to get to some minor issues with it (commit message rewording, cleaning up some related callers), but the fundamental approach seemed good. I then split it off from the v4 of that series to get the non-tricky changes in: https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/ I then just never got to picking it up again, I'll probably re-roll it & make it a part of this series, then we can remove this whole OID != NULL case and will be sure nothing fishy's going on.
On Thu, Jul 15, 2021 at 02:02:40AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Maybe that is splitting hairs, but I definitely try to err on the side > > of caution and over-analysis when touching tricky code (and the > > ref-backend code is in my experience one of the trickiest spots for > > corner cases, races, etc). > > I'd entirely forgotten about this, but I had a patch to remove that > "oid" call entirely, as it's really an unrelated bug/undesired behavior > > You also looked at it at the time & Michael Haggerty reviewed it: > https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/ > > The state of that patch was that I needed to get to some minor issues > with it (commit message rewording, cleaning up some related callers), > but the fundamental approach seemed good. > > I then split it off from the v4 of that series to get the non-tricky > changes in: > https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/ > > I then just never got to picking it up again, I'll probably re-roll it & > make it a part of this series, then we can remove this whole OID != NULL > case and will be sure nothing fishy's going on. Yeah, that sounds like a good path forward. I do think the patch under discussion here is probably the right thing to do. But it becomes all the more obvious if lock_ref_oid_basic() ends up dropping that parameter entirely. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Jul 15, 2021 at 02:02:40AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > Maybe that is splitting hairs, but I definitely try to err on the side >> > of caution and over-analysis when touching tricky code (and the >> > ref-backend code is in my experience one of the trickiest spots for >> > corner cases, races, etc). >> >> I'd entirely forgotten about this, but I had a patch to remove that >> "oid" call entirely, as it's really an unrelated bug/undesired behavior >> >> You also looked at it at the time & Michael Haggerty reviewed it: >> https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/ >> >> The state of that patch was that I needed to get to some minor issues >> with it (commit message rewording, cleaning up some related callers), >> but the fundamental approach seemed good. >> >> I then split it off from the v4 of that series to get the non-tricky >> changes in: >> https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/ >> >> I then just never got to picking it up again, I'll probably re-roll it & >> make it a part of this series, then we can remove this whole OID != NULL >> case and will be sure nothing fishy's going on. > > Yeah, that sounds like a good path forward. I do think the patch under > discussion here is probably the right thing to do. But it becomes all > the more obvious if lock_ref_oid_basic() ends up dropping that parameter > entirely. OK, so what's the final verdict on this step? It is unfortunate that when Ævar took over a topic from Han-Wen, this patch has been inserted as the very first step before the patches in the series, so until we know we are happy with it, it takes several other patches hostage. Thanks.
On Fri, Jul 16, 2021 at 06:28:06PM -0700, Junio C Hamano wrote: > >> I then just never got to picking it up again, I'll probably re-roll it & > >> make it a part of this series, then we can remove this whole OID != NULL > >> case and will be sure nothing fishy's going on. > > > > Yeah, that sounds like a good path forward. I do think the patch under > > discussion here is probably the right thing to do. But it becomes all > > the more obvious if lock_ref_oid_basic() ends up dropping that parameter > > entirely. > > OK, so what's the final verdict on this step? It is unfortunate > that when Ævar took over a topic from Han-Wen, this patch has been > inserted as the very first step before the patches in the series, so > until we know we are happy with it, it takes several other patches > hostage. I just read through v2. Modulo a few small nits (mostly typos, but a few commit message suggestions), it looks good to me. I agree it's a lot to stick in front of another set of patches, but I think in this case we can proceed quickly enough to make it worth doing in the order Ævar suggests. -Peff
On Fri, Jul 16 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >> On Thu, Jul 15, 2021 at 02:02:40AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >>> > Maybe that is splitting hairs, but I definitely try to err on the side >>> > of caution and over-analysis when touching tricky code (and the >>> > ref-backend code is in my experience one of the trickiest spots for >>> > corner cases, races, etc). >>> >>> I'd entirely forgotten about this, but I had a patch to remove that >>> "oid" call entirely, as it's really an unrelated bug/undesired behavior >>> >>> You also looked at it at the time & Michael Haggerty reviewed it: >>> https://lore.kernel.org/git/20190315155959.12390-9-avarab@gmail.com/ >>> >>> The state of that patch was that I needed to get to some minor issues >>> with it (commit message rewording, cleaning up some related callers), >>> but the fundamental approach seemed good. >>> >>> I then split it off from the v4 of that series to get the non-tricky >>> changes in: >>> https://lore.kernel.org/git/20190328161434.19200-1-avarab@gmail.com/ >>> >>> I then just never got to picking it up again, I'll probably re-roll it & >>> make it a part of this series, then we can remove this whole OID != NULL >>> case and will be sure nothing fishy's going on. >> >> Yeah, that sounds like a good path forward. I do think the patch under >> discussion here is probably the right thing to do. But it becomes all >> the more obvious if lock_ref_oid_basic() ends up dropping that parameter >> entirely. > > OK, so what's the final verdict on this step? It is unfortunate > that when Ævar took over a topic from Han-Wen, this patch has been > inserted as the very first step before the patches in the series, so > until we know we are happy with it, it takes several other patches > hostage. I'm just interested in the end result landing sooner than later, so if you think this re-imagining of it hinders more than helps I'm happy to discard it and just take the last version Han-Wen submitted, i.e. the v5: https://lore.kernel.org/git/pull.1012.v5.git.git.1625684869.gitgitgadget@gmail.com/ I can then re-roll whatever I've come up here that I still find useful on that after it lands. I just thought that given the complexity of the ref code tying any loose ends up before those changes would help, but maybe not. Anyway, you/Han-Wen let me know. I'm happy to re-roll what I have outstanding here based on feedback, but also just to discard it for now and go with his version. I'll hold on any re-rolls in that area pending feedback on what you two would like to do.
Jeff King <peff@peff.net> writes: > On Fri, Jul 16, 2021 at 06:28:06PM -0700, Junio C Hamano wrote: > >> >> I then just never got to picking it up again, I'll probably re-roll it & >> >> make it a part of this series, then we can remove this whole OID != NULL >> >> case and will be sure nothing fishy's going on. >> > >> > Yeah, that sounds like a good path forward. I do think the patch under >> > discussion here is probably the right thing to do. But it becomes all >> > the more obvious if lock_ref_oid_basic() ends up dropping that parameter >> > entirely. >> >> OK, so what's the final verdict on this step? It is unfortunate >> that when Ævar took over a topic from Han-Wen, this patch has been >> inserted as the very first step before the patches in the series, so >> until we know we are happy with it, it takes several other patches >> hostage. > > I just read through v2. Modulo a few small nits (mostly typos, but a few > commit message suggestions), it looks good to me. I agree it's a lot to > stick in front of another set of patches, but I think in this case we > can proceed quickly enough to make it worth doing in the order Ævar > suggests. I did not check the v2 thouroughly myself, but read it enough to convince me that it would be good preliminary clean-up steps to come before the main series (modulo typoes and nits, as you pointed out). Thanks.
Junio C Hamano <gitster@pobox.com> writes: > I did not check the v2 thouroughly myself, but read it enough to > convince me that it would be good preliminary clean-up steps to come > before the main series (modulo typoes and nits, as you pointed out). I've came to the same conclusion as you did. The patches themselves are good, modulo typos and some inaccuracies in the comment string and commit log messages, that would not hurt immediate execution of the resulting code but they need updates to help future developers who need to interact with these commits and its results. Thanks.
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 ||
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(-)