--- v5
+++ v3
@@ -1,25 +1,21 @@
A re-roll of
-https://lore.kernel.org/git/cover-00.11-0000000000-20210726T234237Z-avarab@gmail.com
-
-This incorporates Jeff King's
-https://lore.kernel.org/git/YQyGe9qOPRA0nf5v@coredump.intra.peff.net;
-so this replaces ab/refs-files-cleanup and
-jk/refs-files-cleanup-cleanup.
-
-There's also a trivial commit message grammar fix here that Junio
-already squashed into his version of v4. The rest of the changes are
-all due to the rebase on Jeff's commit to get rid of the "flags"
-parameter.
-
-But most importantly there's a new "refs API: remove OID argument to
-reflog_expire()" patch here, to change the function signature of
-reflog_expire() to not pass the OID. Leaving it in place caused a
-segfault in the reftable topic, now it just won't compile in
-combination with this. I'm submitting update(s) to those other topics
-on top to make these all work nicely together.
-
-Jeff King (1):
- refs: drop unused "flags" parameter to lock_ref_oid_basic()
+https://lore.kernel.org/git/cover-00.11-00000000000-20210716T140631Z-avarab@gmail.com/;
+hopefully addressing all the comments by Peff & Junio, thanks
+both!
+
+Changes:
+
+ * Lots of commit message/comment changes, see range-diff
+
+ * I referred to an assert() I forgot to add, the series now juggles
+ an assert() of thei oid back and forth, for clarity of what we're
+ actually doing.
+
+ As noted in v2 I've got a series prepped for this one to finally
+ clean up those loose ends/dead code, but let's focus on the
+ behavior changes for now.
+
+ * There's now a 12th patch to remove the "ENOTDIR" case Peff noted,
Ævar Arnfjörð Bjarmason (12):
refs/packet: add missing BUG() invocations to reflog callbacks
@@ -27,203 +23,247 @@
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 API: remove OID argument to reflog_expire()
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 | 13 +++--
- refs.h | 9 ++-
- refs/debug.c | 8 +--
- refs/files-backend.c | 132 +++++++++++-------------------------------
- refs/packed-backend.c | 7 ++-
- refs/refs-internal.h | 2 +-
- 8 files changed, 64 insertions(+), 123 deletions(-)
-
-Range-diff against v4:
- 1: 92fc3af0727 = 1: 61cf49b9582 refs/packet: add missing BUG() invocations to reflog callbacks
- 2: 67cd2331fb4 ! 2: a20548c1a4d refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
+ refs.c | 5 +-
+ refs.h | 4 +-
+ refs/debug.c | 10 ++--
+ refs/files-backend.c | 128 +++++++++++-------------------------------
+ refs/packed-backend.c | 5 ++
+ 7 files changed, 58 insertions(+), 114 deletions(-)
+
+Range-diff against v2:
+ 1: 30bd7679a5c = 1: 737d2d8c3d1 refs/packet: add missing BUG() invocations to reflog callbacks
+ 2: 033c0cec33d ! 2: dfb9e34076e refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
@@ Metadata
## Commit message ##
refs/files: remove unused REF_DELETING in lock_ref_oid_basic()
- - The lock_ref_oid_basic() function has gradually been by most callers
- - no longer performing a low-level "acquire lock, update and release",
- - and instead using the ref transaction API. So there are only 4
- - remaining callers of lock_ref_oid_basic().
- + The lock_ref_oid_basic() function has gradually been replaced by
- + most callers no longer performing a low-level "acquire lock,
- + update and release", and instead using the ref transaction API.
- + So there are only 4 remaining callers of lock_ref_oid_basic().
-
- None of those callers pass REF_DELETING anymore, the last caller went
- away in 92b1551b1d (refs: resolve symbolic refs first,
- -: ----------- > 3: d3216a6b1d8 refs: drop unused "flags" parameter to lock_ref_oid_basic()
- 3: 7d76514b559 ! 4: 3e538eb3008 refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
- @@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
- const struct object_id *old_oid,
- - const struct string_list *extras,
- - const struct string_list *skip,
- - unsigned int flags, int *type,
- - struct strbuf *err)
- + int *type, struct strbuf *err)
- {
- + struct strbuf ref_file = STRBUF_INIT;
- @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- last_errno = errno;
- if (!refs_verify_refname_available(
- @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
+ - The lock_ref_oid_basic() function has gradually been replaced by use
+ - of the file transaction API, there are only 4 remaining callers of
+ - it.
+ -
+ - None of those callers pass REF_DELETING, the last such caller went
+ - away in 8df4e511387 (struct ref_update: move "have_old" into "flags",
+ - 2015-02-17). This is the start of even more removal of unused code in
+ - and around this function.
+ + The lock_ref_oid_basic() function has gradually been by most callers
+ + no longer performing a low-level "acquire lock, update and release",
+ + and instead using the ref transaction API. So there are only 4
+ + remaining callers of lock_ref_oid_basic().
+ +
+ + None of those callers pass REF_DELETING anymore, the last caller went
+ + away in 92b1551b1d (refs: resolve symbolic refs first,
+ + 2016-04-25).
+ +
+ + Before that we'd refactored and moved this code in:
+ +
+ + - 8df4e511387 (struct ref_update: move "have_old" into "flags",
+ + 2015-02-17)
+ +
+ + - 7bd9bcf372d (refs: split filesystem-based refs code into a new
+ + file, 2015-11-09)
+ +
+ + - 165056b2fc (lock_ref_for_update(): new function, 2016-04-24)
+ +
+ + We then finally stopped using it in 92b1551b1d (noted above). So let's
+ + remove the handling of this parameter.
+ +
+ + By itself this change doesn't benefit us much, but it's the start of
+ + even more removal of unused code in and around this function in
+ + subsequent commits.
+
+ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+
+ 3: 94ffcd8cfda = 3: 0f2262fec69 refs/files: remove unused "extras/skip" in lock_ref_oid_basic()
+ 4: 61f9e0fc864 = 4: 7fb7ff97491 refs/files: remove unused "skip" in lock_raw_ref() too
+ 5: 95101c322b7 = 5: 4e526c34aaa refs/debug: re-indent argument list for "prepare"
+ 6: e93465f4137 ! 6: 295594fe8ae refs API: pass the "lock OID" to reflog "prepare"
+ @@ Commit message
+ refs API: pass the "lock OID" to reflog "prepare"
+
+ Don't pass the object ID we pass into reflog_expire() back to the
+ - caller, but rather our locked OID.
+ + caller, but rather our locked OID. As the assert shows these two were
+ + the same thing in practice as we'd exit earlier in this function if we
+ + couldn't lock the desired OID.
+
+ - As the assert shows these two were the same thing in practice as we'd
+ - exit earlier in this function if we couldn't lock the desired OID, but
+ - as part of removing the passing of the OID to other functions further
+ - on I'm splitting up these concerns.
+ + This is in preparation for a subsequent change of not having
+ + reflog_expire() pass in the OID at all, also remove the "const" from
+ + the parameter since the "struct ref_lock" does not have it on its
+ + "old_oid" member.
+
+ As we'll see in a subsequent commit we don't actually want to assert
+ that we locked a given OID, we want this API to do the locking and
+ - tell us what the OID is, but for now let's just setup the scaffolding
+ - for that.
+ + tell us what the OID is, but for now let's just setup the basic
+ + scaffolding for that.
+
+ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+
+ @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
+ }
- logmoved = log;
-
- +-
- - lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
- -- REF_NO_DEREF, NULL, &err);
- -+ lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
- -+ &err);
- +- NULL, &err);
- ++ lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err);
+ - (*prepare_fn)(refname, oid, cb.policy_cb);
+ ++ assert(oideq(&lock->old_oid, oid));
+ + (*prepare_fn)(refname, &lock->old_oid, cb.policy_cb);
+ refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
+ (*cleanup_fn)(cb.policy_cb);
+ 7: 0fff2d32cfc = 7: e45ec439db0 refs: make repo_dwim_log() accept a NULL oid
+ 8: 1e25b7c59c5 ! 8: 8ae8e5ac029 refs/files: add a comment about refs_reflog_exists() call
+ @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
+ }
+ +
+ + /*
+ -+ * When refs are deleted their reflog is deleted before the
+ -+ * ref itself deleted. This race happens because there's no
+ -+ * such thing as a lock on the reflog, instead we always lock
+ -+ * the "loose ref" (even if packet) above with
+ ++ * When refs are deleted, their reflog is deleted before the
+ ++ * ref itself is deleted. This is because there is no separate
+ ++ * lock for reflog; instead we take a lock on the ref with
+ + * lock_ref_oid_basic().
+ + *
+ -+ * If race happens we've got nothing more to do, we were asked
+ -+ * to delete the reflog, and it's not there anymore. Great!
+ ++ * If a race happens and the reflog doesn't exist after we've
+ ++ * acquired the lock that's OK. We've got nothing more to do;
+ ++ * We were asked to delete the reflog, but someone else
+ ++ * deleted it! The caller doesn't care that we deleted it,
+ ++ * just that it is deleted. So we can return successfully.
+ + */
+ if (!refs_reflog_exists(ref_store, refname)) {
+ unlock_ref(lock);
+ 9: 60d6cf342fc ! 9: 1050743e27c reflog expire: don't lock reflogs using previously seen OID
+ @@ Commit message
+
+ 1. Lookup the reflog name/OID via dwim_log()
+ 2. With that OID, lock the reflog
+ - 3. Later in builtin/reflog.c we use the we looked as input to
+ + 3. Later in builtin/reflog.c we use the OID we looked as input to
+ lookup_commit_reference_gently(), assured that it's equal to the
+ OID we got from dwim_log().
+
+ - What do I mean with "mostly" above? It mostly mitigates it because
+ - we'll still run into cases where the ref is locked and being updated
+ - as we want to expire it, and other git processes wanting to update the
+ - refs will in turn race with us as we expire the reflog.
+ + We can be sure that this change is safe to make because between
+ + dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other
+ + logic relevant to the OID or expiry run in the cmd_reflog_expire()
+ + caller.
+
+ - This remaining race can in turn be mitigated with the
+ + We can thus treat that code as a black box, before and after this
+ + change it would get an OID that's been locked, the only difference is
+ + that now we mostly won't be failing to get the lock due to the TOCTOU
+ + race[0]. That failure was purely an implementation detail in how the
+ + "current OID" was looked up, it was divorced from the locking
+ + mechanism.
+ +
+ + What do we mean with "mostly"? It mostly mitigates it because we'll
+ + still run into cases where the ref is locked and being updated as we
+ + want to expire it, and other git processes wanting to update the refs
+ + will in turn race with us as we expire the reflog.
+ +
+ + That remaining race can in turn be mitigated with the
+ core.filesRefLockTimeout setting, see 4ff0f01cb7 ("refs: retry
+ acquiring reference locks for 100ms", 2017-08-21). In practice if that
+ value is high enough we'll probably never have ref updates or reflog
+ @@ Commit message
+
+ See [1] for an initial report of how this impacted "git gc" and a
+ large discussion about this change in early 2019. In particular patch
+ - looked good to Michael Haggerty his[2], but that seems to not have
+ - made it to the ML archive, its content is quoted in full in my [3].
+ + looked good to Michael Haggerty, see his[2]. That message seems to not
+ + have made it to the ML archive, its content is quoted in full in my
+ + [3].
+
+ 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
+ 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
+ + "git gc" issue. As the modified assert() shows we always pass a NULL
+ + oid to reflog_expire() now.
+
+ Unfortunately this sort of probabilistic contention is hard to turn
+ into a test. I've tested this by running the following three subshells
+ @@ Commit message
+ )
+
+ Before this change the "reflog expire" would fail really quickly with
+ - a "but expected" error.
+ + the "but expected" error noted above.
+
+ After this change both the "pull" and "reflog expire" will run for a
+ while, but eventually fail because I get unlucky with
+ @@ Commit message
+ With core.filesRefLockTimeout set to 10 seconds (it can probably be a
+ lot lower) I managed to run all four of these concurrently for about
+ an hour, and accumulated ~125k commits, auto-gc's and all, and didn't
+ - have a single failure.
+ + have a single failure. The loops visibly stall while waiting for the
+ + lock, but that's expected and desired behavior.
+
+ + 0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
+ 1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/
+ 2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu
+ 3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/
+ @@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store
+ REF_NO_DEREF,
+ &type, &err);
if (!lock) {
- if (copy)
- error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
- @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
-
- rollback:
- - lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
- -+ lock = lock_ref_oid_basic(refs, oldrefname, NULL,
- - REF_NO_DEREF, NULL, &err);
- +- NULL, &err);
- ++ lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err);
- if (!lock) {
- error("unable to lock %s for rollback: %s", oldrefname, err.buf);
- + strbuf_release(&err);
- @@ refs/files-backend.c: static int files_create_symref(struct ref_store *ref_store,
- + struct ref_lock *lock;
- int ret;
-
- - lock = lock_ref_oid_basic(refs, refname, NULL,
- -- NULL, NULL, REF_NO_DEREF, NULL,
- -+ REF_NO_DEREF, NULL,
- - &err);
- +- lock = lock_ref_oid_basic(refs, refname, NULL,
- +- NULL, NULL, NULL,
- +- &err);
- ++ lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err);
- if (!lock) {
- error("%s", err.buf);
- + strbuf_release(&err);
- @@ 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:
- */
- - lock = lock_ref_oid_basic(refs, refname, oid,
- -- NULL, NULL, REF_NO_DEREF,
- -+ REF_NO_DEREF,
- - &type, &err);
- +- lock = lock_ref_oid_basic(refs, refname, oid,
- +- NULL, NULL, &type, &err);
- ++ lock = lock_ref_oid_basic(refs, refname, oid, &type, &err);
- if (!lock) {
- error("cannot lock ref '%s': %s", refname, err.buf);
- + strbuf_release(&err);
- 4: 852f86e666f = 5: b7335e79f8b refs/files: remove unused "skip" in lock_raw_ref() too
- 5: 685b48328af = 6: 24449766060 refs/debug: re-indent argument list for "prepare"
- 6: b75e7673d70 = 7: 3b7daf03e5a refs: make repo_dwim_log() accept a NULL oid
- 7: 7fe6c9bd921 = 8: 51abe459e70 refs/files: add a comment about refs_reflog_exists() call
- 8: c9c2da35997 ! 9: aba12606cea reflog expire: don't lock reflogs using previously seen OID
- @@ 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:
- */
- -- lock = lock_ref_oid_basic(refs, refname, oid,
- -+ lock = lock_ref_oid_basic(refs, refname, NULL,
- - REF_NO_DEREF,
- - &type, &err);
- +- lock = lock_ref_oid_basic(refs, refname, oid, &type, &err);
- ++ lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err);
- if (!lock) {
- -@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
- + error("cannot lock ref '%s': %s", refname, err.buf);
- strbuf_release(&err);
- return -1;
- }
- -: ----------- > 10: 5afa8f1be29 refs API: remove OID argument to reflog_expire()
- 9: b61c734cf5c ! 11: 7712e29abe6 refs/files: remove unused "oid" in lock_ref_oid_basic()
- @@ refs/files-backend.c: static struct ref_iterator *files_ref_iterator_begin(
- {
- /*
- @@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
- + * On failure errno is set to something meaningful.
- */
- static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- - const char *refname,
- +- const char *refname,
- - const struct object_id *old_oid,
- - unsigned int flags, int *type,
- - struct strbuf *err)
- +- int *type, struct strbuf *err)
- ++ const char *refname, int *type,
- ++ struct strbuf *err)
- {
- struct strbuf ref_file = STRBUF_INIT;
- struct ref_lock *lock;
- @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
-
- logmoved = log;
-
- -- lock = lock_ref_oid_basic(refs, newrefname, NULL, REF_NO_DEREF, NULL,
- -- &err);
- -+ lock = lock_ref_oid_basic(refs, newrefname, REF_NO_DEREF, NULL, &err);
- +- lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err);
- ++ lock = lock_ref_oid_basic(refs, newrefname, NULL, &err);
- if (!lock) {
- if (copy)
- error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
- @@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_
- goto out;
-
- rollback:
- -- lock = lock_ref_oid_basic(refs, oldrefname, NULL,
- -- REF_NO_DEREF, NULL, &err);
- -+ lock = lock_ref_oid_basic(refs, oldrefname, REF_NO_DEREF, NULL, &err);
- +- lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err);
- ++ lock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);
- if (!lock) {
- error("unable to lock %s for rollback: %s", oldrefname, err.buf);
- strbuf_release(&err);
- @@ refs/files-backend.c: static int files_create_symref(struct ref_store *ref_store
- struct ref_lock *lock;
- int ret;
-
- -- lock = lock_ref_oid_basic(refs, refname, NULL,
- -- REF_NO_DEREF, NULL,
- -- &err);
- -+ lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, NULL, &err);
- +- lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err);
- ++ lock = lock_ref_oid_basic(refs, refname, NULL, &err);
- if (!lock) {
- error("%s", err.buf);
- strbuf_release(&err);
- @@ 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:
- */
- -- lock = lock_ref_oid_basic(refs, refname, NULL,
- -- REF_NO_DEREF,
- -- &type, &err);
- -+ lock = lock_ref_oid_basic(refs, refname, REF_NO_DEREF, &type, &err);
- +- lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err);
- ++ lock = lock_ref_oid_basic(refs, refname, &type, &err);
- if (!lock) {
- error("cannot lock ref '%s': %s", refname, err.buf);
- strbuf_release(&err);
-10: 009abc99688 = 12: f746939a27a refs/files: remove unused "errno == EISDIR" code
-11: acb131cc1c5 = 13: 2e30ee04edb refs/files: remove unused "errno != ENOTDIR" condition
+ +@@ 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);
+ + refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb);
+ + (*cleanup_fn)(cb.policy_cb);
+10: 09dd8836437 ! 10: 753c20f89bf refs/files: remove unused "oid" in lock_ref_oid_basic()
+ @@ Commit message
+ In the preceding commit the last caller that passed a non-NULL OID was
+ changed to pass NULL to lock_ref_oid_basic(). As noted in preceding
+ commits use of this API has been going away (we should use ref
+ - transactions, or lock_raw_rew()), so we're unlikely to gain new
+ + transactions, or lock_raw_ref()), so we're unlikely to gain new
+ callers that want to pass the "oid".
+
+ So let's remove it, doing so means we can remove the "mustexist"
+ condition, and therefore anything except the "flags =
+ - RESOLVE_REF_NO_RECURSE" case. Furthermore, since the verify_lock()
+ - function we called did most of its work when the "oid" was passed (as
+ - "old_oid") we can inline the trivial part of it that remains in what's
+ - now its only caller.
+ + RESOLVE_REF_NO_RECURSE" case.
+ +
+ + Furthermore, since the verify_lock() function we called did most of
+ + its work when the "oid" was passed (as "old_oid") we can inline the
+ + trivial part of it that remains in its only remaining caller. Without
+ + a NULL "oid" passed it was equivalent to calling refs_read_ref_full()
+ + followed by oidclr().
+
+ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+
+11: 96c3e5e9f79 = 11: 8a71bbef972 refs/files: remove unused "errno == EISDIR" code
+ -: ----------- > 12: 452253d597d refs/files: remove unused "errno != ENOTDIR" condition
--
-2.33.0.662.g438caf9576d
-
+2.32.0.874.ge7a9d58bfcf
+