--- v4
+++ v2
@@ -1,9 +1,25 @@
-This version hopefully addresses the weirdness of passing the
-lock->old_oid as the "oid". We still do so, but the "refs API: pass
-the "lock OID" to reflog "prepare"" and its change from s/const// is
-gone, instead the relevant (and much smaller) part of that is squashed
-into the "reflog expire: don't lock reflogs using previously seen OID"
-change.
+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
@@ -11,103 +27,145 @@
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
- refs/files: remove unused "errno != ENOTDIR" condition
- builtin/reflog.c | 13 ++---
+ builtin/reflog.c | 17 +++---
reflog-walk.c | 3 +-
refs.c | 5 +-
- refs.h | 2 +-
- refs/debug.c | 4 +-
- refs/files-backend.c | 130 ++++++++++++------------------------------
+ refs.h | 4 +-
+ refs/debug.c | 10 ++--
+ refs/files-backend.c | 122 ++++++++++--------------------------------
refs/packed-backend.c | 5 ++
- 7 files changed, 53 insertions(+), 109 deletions(-)
+ 7 files changed, 54 insertions(+), 112 deletions(-)
-Range-diff against v3:
- 1: 737d2d8c3d = 1: 92fc3af072 refs/packet: add missing BUG() invocations to reflog callbacks
- 2: dfb9e34076 = 2: 67cd2331fb refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
- 3: 0f2262fec6 = 3: 7d76514b55 refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
- 4: 7fb7ff9749 = 4: 852f86e666 refs/files: remove unused "skip" in lock_raw_ref() too
- 5: 4e526c34aa = 5: 685b48328a refs/debug: re-indent argument list for "prepare"
- 6: 295594fe8a < -: ---------- refs API: pass the "lock OID" to reflog "prepare"
- 7: e45ec439db = 6: b75e7673d7 refs: make repo_dwim_log() accept a NULL oid
- 8: 8ae8e5ac02 = 7: 7fe6c9bd92 refs/files: add a comment about refs_reflog_exists() call
- 9: 1050743e27 ! 8: c9c2da3599 reflog expire: don't lock reflogs using previously seen OID
+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
- [3].
+ remove_empty_directories() and continue.
- I'm leaving behind now-unused code the refs API etc. that takes the
- - now-NULL "oid" argument, and other code that can be simplified now
- + now-NULL "unused_oid" argument, and other code that can be simplified now
- that we never have on OID in that context, that'll be cleaned up in
- subsequent commits, but for now let's narrowly focus on fixing the
- "git gc" issue. As the modified assert() shows we always pass a NULL
- @@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, cons
- should_expire_reflog_ent,
- reflog_expiry_cleanup,
+ 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:
- + ## refs.h ##
- +@@ refs.h: enum expire_reflog_flags {
- + * expiration policy that is desired.
- + *
- + * reflog_expiry_prepare_fn -- Called once after the reference is
- +- * locked.
- ++ * locked. Called with the OID of the locked reference.
- + *
- + * reflog_expiry_should_prune_fn -- Called once for each entry in the
- + * existing reflog. It should return true iff that entry should be
+ - 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 int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
- + }
+ @@ 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;
+
- + static int files_reflog_expire(struct ref_store *ref_store,
- +- const char *refname, const struct object_id *oid,
- ++ const char *refname, const struct object_id *unused_oid,
- + unsigned int flags,
- + reflog_expiry_prepare_fn prepare_fn,
- + reflog_expiry_should_prune_fn should_prune_fn,
- +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
- + int status = 0;
- + int type;
- + struct strbuf err = STRBUF_INIT;
- ++ const struct object_id *oid;
+ + 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);
+
- + memset(&cb, 0, sizeof(cb));
- + cb.flags = flags;
- @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
- * reference itself, plus we might need to update the
- * reference if --updateref was specified:
- @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
- REF_NO_DEREF,
- &type, &err);
- if (!lock) {
- +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
- + strbuf_release(&err);
- + return -1;
- + }
- ++ oid = &lock->old_oid;
- +
- + /*
- + * When refs are deleted, their reflog is deleted before the
- @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
- }
- }
-
- -- assert(oideq(&lock->old_oid, oid));
- -+ assert(!oid);
- - (*prepare_fn)(refname, &lock->old_oid, cb.policy_cb);
- ++ assert(!unused_oid);
- + (*prepare_fn)(refname, oid, cb.policy_cb);
- refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
- (*cleanup_fn)(cb.policy_cb);
-10: 753c20f89b = 9: b61c734cf5 refs/files: remove unused "oid" in lock_ref_oid_basic()
-11: 8a71bbef97 = 10: 009abc9968 refs/files: remove unused "errno == EISDIR" code
-12: 452253d597 = 11: acb131cc1c refs/files: remove unused "errno != ENOTDIR" condition
+ + 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,
--
-2.32.0.956.g6b0c84ceda8
+2.32.0.873.gb6f2f696497