| Message ID | cover-00.11-00000000000-20210716T140631Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix "git reflog expire" race & get rid of EISDIR in refs API | expand |
On Fri, Jul 16, 2021 at 04:12:56PM +0200, Ævar Arnfjörð Bjarmason wrote: > builtin/reflog.c | 17 +++--- > reflog-walk.c | 3 +- > refs.c | 5 +- > refs.h | 4 +- > refs/debug.c | 10 ++-- > refs/files-backend.c | 122 ++++++++++-------------------------------- > refs/packed-backend.c | 5 ++ > 7 files changed, 54 insertions(+), 112 deletions(-) The diffstat certainly looks nice. :) I left a few comments on specific patches, mostly just little things. The ones I didn't comment on looked good to me. Overall, it seems like a very nice cleanup. Thanks for putting it together. -Peff
This is a follow-up to the much smaller one-patch v1: https://lore.kernel.org/git/patch-1.1-de0838fe99-20210714T111351Z-avarab@gmail.com/ As noted in the discussion there there's a potential loose end with one of the 4 callers of lock_ref_oid_basic(). I'd forgotten that I fixed a bug in 2019 by removing the OID call to that one caller[1]. It didn't get integrated for no particularly good reason, had some "prep" series it depended on, it looked good to reviewers, but I just forgot to pursue it after the prep patches landed. That patch has been running in a large production for a long time, and as far as I know still is. The version of it we end up with here is the same in all the important ways, I just clean up the immediate caller as well (and there's some prep patches for that). There's still some loose ends left in builtin/reflog.c after that, but it's not important for correctness. I've got a 7-patch series post-this for that, hopefully I'll remember to submit it this time. 1. https://lore.kernel.org/git/875yxczbd6.fsf@evledraar.gmail.com/ Ævar Arnfjörð Bjarmason (11): refs/packet: add missing BUG() invocations to reflog callbacks refs/files: remove unused REF_DELETING in lock_ref_oid_basic() refs/files: remove unused "extras/skip" in lock_ref_oid_basic() refs/files: remove unused "skip" in lock_raw_ref() too refs/debug: re-indent argument list for "prepare" refs API: pass the "lock OID" to reflog "prepare" refs: make repo_dwim_log() accept a NULL oid refs/files: add a comment about refs_reflog_exists() call reflog expire: don't lock reflogs using previously seen OID refs/files: remove unused "oid" in lock_ref_oid_basic() refs/files: remove unused "errno == EISDIR" code builtin/reflog.c | 17 +++--- reflog-walk.c | 3 +- refs.c | 5 +- refs.h | 4 +- refs/debug.c | 10 ++-- refs/files-backend.c | 122 ++++++++++-------------------------------- refs/packed-backend.c | 5 ++ 7 files changed, 54 insertions(+), 112 deletions(-) Range-diff against v1: -: ----------- > 1: 30bd7679a5c refs/packet: add missing BUG() invocations to reflog callbacks -: ----------- > 2: 033c0cec33d refs/files: remove unused REF_DELETING in lock_ref_oid_basic() -: ----------- > 3: 94ffcd8cfda refs/files: remove unused "extras/skip" in lock_ref_oid_basic() -: ----------- > 4: 61f9e0fc864 refs/files: remove unused "skip" in lock_raw_ref() too -: ----------- > 5: 95101c322b7 refs/debug: re-indent argument list for "prepare" -: ----------- > 6: e93465f4137 refs API: pass the "lock OID" to reflog "prepare" -: ----------- > 7: 0fff2d32cfc refs: make repo_dwim_log() accept a NULL oid -: ----------- > 8: 1e25b7c59c5 refs/files: add a comment about refs_reflog_exists() call -: ----------- > 9: 60d6cf342fc reflog expire: don't lock reflogs using previously seen OID -: ----------- > 10: 09dd8836437 refs/files: remove unused "oid" in lock_ref_oid_basic() 1: de0838fe996 ! 11: 96c3e5e9f79 refs file backend: remove dead "errno == EISDIR" code @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Commit message ## - refs file backend: remove dead "errno == EISDIR" code + refs/files: remove unused "errno == EISDIR" code 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 @@ Commit message 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: + 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() + [...] -> + 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()): + 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); @@ Commit message 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. + it, 2016-05-05) for the addition of that code, and + a1c1d8170db (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> ## refs/files-backend.c ## @@ refs/files-backend.c: 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); + 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); +@@ refs/files-backend.c: 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 @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_re - last_errno = errno; - if (!refs_verify_refname_available( - &refs->base, -- refname, extras, skip, err)) +- 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_flags, +- resolved = !!refs_resolve_ref_unsafe(&refs->base, refname, +- RESOLVE_REF_NO_RECURSE, - &lock->old_oid, type); - } - if (!resolved) { +- 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,