BUG: Race condition due to reflog expiry in "gc"

I'm still working on fixing a race condition I encountered in "gc" recently, but am not 100% sure of the fix. So I thought I'd send a braindump of what I have so far in case it jolts any memories.

The problem is that when we "gc" we run "reflog expire --all". This iterates over the reflogs, and takes a *.lock for each reference.

It'll fail intermittendly in two ways:

 1. If something is concurrently committing to the repo it'll fail
    because we for a tiny amount of time hold a HEAD.lock file, so HEAD
    can't be updated.

 2. On a repository that's just being "git fetch"'d by some concurrent
    process the "gc" will fail, because the ref's SHA1 has changed,
    which we inspect as we aquire the lock.

To reproduce this have a repo getting a bunch of commits and "fetch" from it in a loop[A]. Then try to "gc" in it (or "reflog expire --all"). You'll get case #2 above (#1 can also be manually reproduce):

    $ git gc; echo $?
    error: cannot lock ref 'refs/remotes/origin/HEAD': ref 'refs/remotes/origin/HEAD' is at d13c708ae0435c88c8f7580442b70df9a40224d0 but expected 7fb5b7dd9988d040a9d29c0456874472bea2b439
    error: cannot lock ref 'refs/remotes/origin/master': ref 'refs/remotes/origin/master' is at d13c708ae0435c88c8f7580442b70df9a40224d0 but expected 7fb5b7dd9988d040a9d29c0456874472bea2b439
    fatal: failed to run reflog
    128

This behavior is due to a combination of:

 - 62aad1849f ("gc --auto: do not lock refs in the background", 2014-05-25)
 - f3b661f766 ("expire_reflog(): use a lock_file for rewriting the
   reflog file", 2014-12-12)

Discussion around those changes:

    https://public-inbox.org/git/1400978309-25235-1-git-send-email-pclouds@gmail.com/
    https://public-inbox.org/git/CAGZ79kaPzMPcxMqsTeW5LjYNc7LbpjHE5eBPHWKvUkBU3NvGJw@mail.gmail.com/

I haven't yet found a reason for why we'd intrinsically need this lock, and indeed this doesn't produce corruption AFAICT:

    diff --git a/refs/files-backend.c b/refs/files-backend.c
    index ef053f716c..ffb9fbbf3a 100644
    --- a/refs/files-backend.c
    +++ b/refs/files-backend.c
    @@ -3043,7 +3043,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
     	if (!lock) {
     		error("cannot lock ref '%s': %s", refname, err.buf);
     		strbuf_release(&err);
    -		return -1;
    +		return 0;
     	}
     	if (!refs_reflog_exists(ref_store, refname)) {
     		unlock_ref(lock);

But that just "solves" #2, not #1, and also no-oping the OID check in verify_lock() still has all tests pass, so we're likely missing the relevant race condition tests.

A.

    (
        cd /tmp &&
        rm -rf git &&
        git init git &&
        cd git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >out &&
            git add out &&
            git commit -m"out"
        done
    )

    (
        cd /tmp &&
        rm -rf git-clone &&
        git clone file:///tmp/git git-clone &&
        cd git-clone &&
        while true
        do
            git reset --hard &&
            git pull
        done
    )

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Both sounds very much expected and expectable outcome. I am not sure how they need to be called bugs.

On Wed, Mar 13 2019, Junio C Hamano wrote:

Let's leave aside that I started the subject with "BUG:" and let me rephrase.

I was under the impression that git-gc was supposed to support operating on a repository that's concurrently being modified, as long as you don't set the likes of gc.pruneExpire too aggressively.

Running a "gc" in a loop without "git reflog expire --all" and when watching the repository being GC'd with:

    fswatch -l 0.1 -t -r . 2>&1 | grep lock

We only create .git/MERGE_RR.lock, .git/gc.pid.lock and git/packed-refs.lock. These are all things that would only cause another concurrent GC to fail, not a normal git command.

So the only reason a concurrent commit (case #1) fails is because of the refs being locked during the reflog iteration, and similarly "gc" itself will fail due to a concurrently updating ref (case #2).

It seems that first of all we need this, I'll submit that as a separate patch sometime soon:

    diff --git a/builtin/gc.c b/builtin/gc.c
    index 020f725acc..ae488646e1 100644
    --- a/builtin/gc.c
    +++ b/builtin/gc.c
    @@ -127,6 +127,12 @@ static void gc_config(void)
     			pack_refs = git_config_bool("gc.packrefs", value);
     	}

    +	if (!git_config_get_value("gc.reflogexpire", &value) && value &&
    +	    !strcmp(value, "never") &&
    +	    !git_config_get_value("gc.reflogexpireunreachable", &value) && value &&
    +	    !strcmp(value, "never"))
    +		prune_reflogs = 0;
    +
     	git_config_get_int("gc.aggressivewindow", &aggressive_window);
     	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
     	git_config_get_int("gc.auto", &gc_auto_threshold);

I.e. now even if your gc.* config says you don't want the reflogs touched, we still pointlessly iterate over all of them. The case I'm running into (a variant of #2) is one solved by that patch, i.e. I'm fine "gc" just having the reflogs kept forever as a workaround in this case.

Something like that should have been added back in 62aad1849f ("gc --auto: do not lock refs in the background", 2014-05-25), i.e. now the "prune_reflogs" variable is never used, it's just cargo-culted from a copy/pasting of the "pack_refs" code.

In other "gc" phases in "pack-objects" and "prune" we also look at the reflogs. This obviously bad patch ignores them entirely:

    diff --git a/builtin/prune.c b/builtin/prune.c
    index 97613eccb5..bccee7813e 100644
    --- a/builtin/prune.c
    +++ b/builtin/prune.c
    @@ -41,7 +41,7 @@ static void perform_reachability_traversal(struct rev_info *revs)

     	if (show_progress)
     		progress = start_delayed_progress(_("Checking connectivity"), 0);
    -	mark_reachable_objects(revs, 1, expire, progress);
    +	mark_reachable_objects(revs, 0, expire, progress);
     	stop_progress(&progress);
     	initialized = 1;
     }
    diff --git a/builtin/repack.c b/builtin/repack.c
    index 67f8978043..618ffbfe0a 100644
    --- a/builtin/repack.c
    +++ b/builtin/repack.c
    @@ -364,7 +364,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
     				 keep_pack_list.items[i].string);
     	argv_array_push(&cmd.args, "--non-empty");
     	argv_array_push(&cmd.args, "--all");
    -	argv_array_push(&cmd.args, "--reflog");
     	argv_array_push(&cmd.args, "--indexed-objects");
     	if (repository_format_partial_clone)
     		argv_array_push(&cmd.args, "--exclude-promisor-objects");

I'm just including that as illustration that add_reflogs_to_pending() in revision.c during "gc" already iterates over the reflogs without locking anything, but of course it's just reading them.

So one thing that would mitigate things a lot is if files_reflog_expire() and its call to expire_reflog_ent() via refs_for_each_reflog_ent() would lazily aquire the lock on the ref.

Digging a bit further that's actually what we're doing now since 4ff0f01cb7 ("refs: retry acquiring reference locks for 100ms", 2017-08-21).

But this runs into the logic we've had for a long time, or since your bda3a31cc7 ("reflog-expire: Avoid creating new files in a directory inside readdir(3) loop", 2008-01-25) where we first loop over all the refs in the process of finding the reflogs, and then will try to lock those refs at those expected SHA-1s. If they've changed in the meantime we error out don't clean up the lockfile.

So just this fixes that:

    diff --git a/refs/files-backend.c b/refs/files-backend.c
    index ef053f716c..b6576f28b9 100644
    --- a/refs/files-backend.c
    +++ b/refs/files-backend.c
    @@ -3037,7 +3037,7 @@ 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,
     				  NULL, NULL, REF_NO_DEREF,
     				  &type, &err);
     	if (!lock) {

Which seems sensible to me. We'll still get the lock, we just don't assert that the refname we use to get the lock must be at that SHA-1. We'll still use it for the purposes of expiry.

But maybe I've missed some caveat in reflog_expiry_prepare() and friends and we really do need the reflog at that OID, then this would suck less:

    diff --git a/builtin/reflog.c b/builtin/reflog.c
    index 4d3430900d..4bb272fdc8 100644
    --- a/builtin/reflog.c
    +++ b/builtin/reflog.c
    @@ -625,12 +625,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
     		free_worktrees(worktrees);
     		for (i = 0; i < collected.nr; i++) {
     			struct collected_reflog *e = collected.e[i];
    +			int st;
     			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
    -			status |= reflog_expire(e->reflog, &e->oid, flags,
    -						reflog_expiry_prepare,
    -						should_expire_reflog_ent,
    -						reflog_expiry_cleanup,
    -						&cb);
    +			st = reflog_expire(e->reflog, &e->oid, flags,
    +					   reflog_expiry_prepare,
    +					   should_expire_reflog_ent,
    +					   reflog_expiry_cleanup,
    +					   &cb);
    +			if (st == -2)
    +				continue;
    +			status |= st;
     			free(e);
     		}
     		free(collected.e);
    diff --git a/refs/files-backend.c b/refs/files-backend.c
    index ef053f716c..8b0b6b7b85 100644
    --- a/refs/files-backend.c
    +++ b/refs/files-backend.c
    @@ -3041,6 +3041,11 @@ static int files_reflog_expire(struct ref_store *ref_store,
     				  NULL, NULL, REF_NO_DEREF,
     				  &type, &err);
     	if (!lock) {
    +		if (errno == EBUSY) {
    +			warning("cannot lock ref '%s': %s. Skipping!", refname, err.buf);
    +			strbuf_release(&err);
    +			return -2;
    +		}
     		error("cannot lock ref '%s': %s", refname, err.buf);
     		strbuf_release(&err);
     		return -1;

I.e. we just detect the EBUSY that verify_lock() sets if the OID doesn't match, and don't prune that reflog. As seen above "pack-objects" and "prune" will still iterate over the same logs later for the purposes of reachability, so this shouldn't get us into a corrupt state due to throwing away objects referenced in those logs, we'll just prune fewer things than we could have.

So I think I'll use the first patch noted above as a hack to solve the narrow problem I have now, but any comments on the above most welcome. I'm not very familiar with the ref code in case that wasn't obvious already.

B.t.w. the mention of f3b661f766 ("expire_reflog(): use a lock_file for rewriting the reflog file", 2014-12-12) upthread is irrelevant. That's a commit where we use the lockfile code to write out the *new* reflog, which is unrelated to all of this.

On Wed, Mar 13, 2019 at 11:28:39AM +0100, Ævar Arnfjörð Bjarmason wrote:

To some degree. If it has to take locks to modify items, then inherently there's going to be lock contention. But we may be able to work around it some.

A big one we used to hit at GitHub is that running `git pack-refs` needs to take the individual ref locks when pruning loose refs that have just been packed. We dealt with that by adding retry-with-timeout logic to ref lock acquisition, in 4ff0f01cb7 (refs: retry acquiring reference locks for 100ms, 2017-08-21). Since then, I can't remember seeing a single instance of this coming up in production use.

I don't think we use a timeout with the reflog lock. Maybe we ought to. It might need to be longer than the 100ms default for refs, since I think we'd do more significant work during the reflog expiration. On the other hand, I think we hold the lock on the ref itself, too, during that expiration.

We don't actually expire reflogs regularly at GitHub, so I can't say one way or the other if there would be lock contention there.

The packed-refs.lock can conflict with a normal operation; regular writers need to update it when they delete a ref.

You'd also be locking regular refs as part of "pack-refs --prune", but you probably don't see it running gc in a loop, because all of the loose refs are pruned after the first run.

Right. It's always safe to read without locking (refs, packed-refs, and reflogs).

I _think_ this should be OK, because we don't open the reflog until after we hold the lock. So we don't really care what the value was at. Wherever we got the lock, we'll do the expiration process atomically at that point.

I don't think this makes all your problems go away, though, because you'd still have the immediate contention of actually taking the lock (at any value).

If I'm reading the code correctly, we don't call reflog_expiry_prepare() until we're holding the lock (because we pass it in as a callback to reflog_expire()). So I think that would be OK.

That saves git-gc from failing. But do we have the problem in the other direction? I.e., that the gc would take a lock, and the actual user request to update a ref would fail?

That's what the retry-with-timeout is supposed to address, so maybe it works. But I wouldn't be surprised if it's insufficient in practice, since the reflog code may walk big parts of the graph under lock, checking reachability.

-Peff

On Wed, Mar 13 2019, Jeff King wrote:

Yeah, but as far as I can tell core.filesRefLockTimeout added in 4ff0f01cb7 (refs: retry acquiring reference locks for 100ms, 2017-08-21) solves that.

I.e. the problem after that patch isn't the contention, but the call to lock_ref_oid_basic() in files_reflog_expire() above which *after* we get the lock effectively asserts that the value must be at the old OID.

It would then hard-die, so git-reflog would leave behind stay *.lock files that e.g. a concurrent "commit" would die on (still there after 100ms, and beyond). Whereas if we're not picky about the OID the two will have contention, but core.filesRefLockTimeout will work around it.

I've been running this whole thing in a loop for a while with that s/oid/NULL/ fix, and it just works. To the extent that it doesn't (due to load) I can just bump core.filesRefLockTimeout up a bit.

So with the benefit of some sleep & your comment I'm pretty sure that's the right fix. Ideally it would have a test, do we have some simliar contention-creating tests I can piggy-back on, or maybe we should just change this and hope for the best...

No, because as with s/oid/NULL/ above core.filesRefLockTimeout saves the day.

I suppose this can happen if the reflog for a given branch is so big that it takes us more than 100ms to parse it, but e.g. O(n) refs shouldn't matter, since we only hold the lock on one ref at a time.

[45]/5 fix a couple of issues I noted upthread. While I was at it I noticed a few things I could clean up in [123]/5 that I figured I'd send alongside this.

Ævar Arnfjörð Bjarmason (5):
  gc: remove redundant check for gc_auto_threshold
  gc: convert to using the_hash_algo
  gc: refactor a "call me once" pattern
  gc: don't run "reflog expire" when keeping reflogs
  reflog expire: don't assert the OID when locking refs

 builtin/gc.c         | 33 +++++++++++++++++++++++++--------
 refs/files-backend.c |  2 +-
 t/t6500-gc.sh        | 19 +++++++++++++++++++
 3 files changed, 45 insertions(+), 9 deletions(-)

Addresse Peff's comments to v1. For a couple of patches I've faked up his SOB where I copy/pasted a comment or code from a v1 comment verbatim. Will see if he acks that.

The main change is a better commit message in the last patch (now 7/7), and 2x new "reflog" patches to make it early exit in addition to "gc" when there's nothing to do.

There was the "why do it at all in gc" feedback on 6/7 in v1. I've adjusted the commit message there to justify it, but we'll see what Peff things about it this time around...

Ævar Arnfjörð Bjarmason (7):
  gc: remove redundant check for gc_auto_threshold
  gc: convert to using the_hash_algo
  gc: refactor a "call me once" pattern
  reflog tests: make use of "test_config" idiom
  reflog: exit early if there's no work to do
  gc: don't run "reflog expire" when keeping reflogs
  reflog expire: don't assert the OID when locking refs

 builtin/gc.c         | 37 +++++++++++++++++++++++++++++--------
 builtin/reflog.c     |  7 +++++++
 refs/files-backend.c |  2 +-
 t/t1410-reflog.sh    | 17 ++++++++---------
 t/t6500-gc.sh        | 19 +++++++++++++++++++
 5 files changed, 64 insertions(+), 18 deletions(-)

Range-diff:
1:  1635c7fb22 ! 1:  e18433f9c6 gc: convert to using the_hash_algo
    @@ -11,16 +11,26 @@
         the case for SHA-256, and reading between the lines in
         hash-function-transition.txt the format is planned to be the same.
     
    -    However, we may want to modify this code for the hash function
    -    transition. There's a potential pathological case here where we'll
    -    only consider the loose objects for the currently active hash, but
    -    objects for that hash will share a directory storage with the other
    -    hash.
    +    In the future we may want to further modify this code for the hash
    +    function transition. There's a potential pathological case here where
    +    we'll only consider the loose objects for the currently active hash,
    +    but objects for that hash will share a directory storage with the
    +    other hash.
     
         Thus we could theoretically have 1k SHA-1 loose objects, and say 1
         million SHA-256 objects, and not notice because we're currently using
         SHA-1.
     
    +    So assuming that "gc" eventually learns to pack up both SHA-1 and
    +    SHA-256 objects regardless of what the current the_hash_alg is perhaps
    +    this check should be changed to consider all files in objects/17/
    +    matching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and
    +    SHA-256).
    +
    +    But none of that is something we need to worry about now, and
    +    supporting both 38 and 62 characters depending on "the_hash_algo"
    +    removes another case of SHA-1 hardcoding.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      diff --git a/builtin/gc.c b/builtin/gc.c
    @@ -30,8 +40,7 @@
      	int auto_threshold;
      	int num_loose = 0;
      	int needed = 0;
    -+	const unsigned hexsz = the_hash_algo->hexsz;
    -+	const unsigned hexsz_loose = hexsz - 2;
    ++	const unsigned hexsz_loose = the_hash_algo->hexsz - 2;
      
      	dir = opendir(git_path("objects/17"));
      	if (!dir)
2:  ced972826d ! 2:  54e4bce91c gc: refactor a "call me once" pattern
    @@ -10,6 +10,7 @@
         "prune_reflogs" variables, so let's not leave the reader wondering if
         they're being zero'd out for later use somewhere else.
     
    +    Signed-off-by: Jeff King <peff@peff.net>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      diff --git a/builtin/gc.c b/builtin/gc.c
    @@ -19,6 +20,11 @@
      
      static void gc_before_repack(void)
      {
    ++	/*
    ++	 * We may be called twice, as both the pre- and
    ++	 * post-daemonized phases will call us, but running these
    ++	 * commands more than once is pointless and wasteful.
    ++	 */
     +	static int done = 0;
     +	if (done++)
     +		return;
-:  ---------- > 3:  82f87db134 reflog tests: make use of "test_config" idiom
-:  ---------- > 4:  c79608dbbb reflog: exit early if there's no work to do
3:  599772f2bd ! 5:  c47dedab58 gc: don't run "reflog expire" when keeping reflogs
    @@ -5,10 +5,18 @@
         Don't redundantly run "git reflog expire --all" when gc.reflogExpire
         and gc.reflogExpireUnreachable are set to "never".
     
    -    I'm being careful to not repeat the issue fixed in
    -    8ab5aa4bd8 ("parseopt: handle malformed --expire arguments more
    -    nicely", 2018-04-21). We'll die early if the config variables are set
    -    to invalid values.
    +    An earlier change taught "git reflog expire" itself to exit early
    +    under this scenario, so in some sense this isn't strictly
    +    necessary. Reasons to also do it here:
    +
    +     1) Similar to 8ab5aa4bd8 ("parseopt: handle malformed --expire
    +        arguments more nicely", 2018-04-21). We'll die early if the config
    +        variables are set to invalid values. We run "pack-refs" before
    +        "reflog expire", which can take a while, only to then die on an
    +        invalid gc.reflogExpire{Unreachable,} configuration.
    +
    +     2) Not invoking the command at all means it won't show up in trace
    +        output, which makes what's going on more obvious.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
4:  c127265828 ! 6:  55dd203a04 reflog expire: don't assert the OID when locking refs
    @@ -2,35 +2,39 @@
     
         reflog expire: don't assert the OID when locking refs
     
    -    The locking protocol for reflogs involves getting a *.lock file on the
    -    loose ref[1] (even if the actual ref is packed). This isn't needed to
    -    expire the reflog, and needlessly results promotes reference update
    -    contention to hard errors in e.g. "gc".
    -
         During reflog expiry, the cmd_reflog_expire() function first iterates
    -    over all known references, and then one-by-one acquires the lock for
    -    each one to expire its reflog. By the time it gets around to
    -    re-visiting the references some of the OIDs may have changed.
    +    over all reflogs in logs/*, and then one-by-one acquires the lock for
    +    each one to expire its reflog by getting a *.lock file on the
    +    corresponding loose ref[1] (even if the actual ref is packed).
    +
    +    This lock is needed, but what isn't needed is locking the loose ref as
    +    a function of the OID we found from that first iteration. By the time
    +    we get around to re-visiting the reference some of the OIDs may have
    +    changed.
     
    -    This has been the case ever since "reflog expire" was initially
    -    implemented in 4264dc15e1 ("git reflog expire", 2006-12-19). As seen
    -    in that simpler initial version of the code (and the same is the case
    -    before this change) we subsequently use the OID to inform the expiry,
    -    but never needed to use it to lock the reference associated with the
    -    reflog.
    +    Thus the verify_lock() function called by the lock_ref_oid_basic()
    +    function being changed here would fail with e.g. "ref '%s' is at %s
    +    but expected %s" if the repository was being updated concurrent to the
    +    "reflog expire".
     
    -    Thus the verify_lock() function would fail with e.g. "ref '%s' is at
    -    %s but expected %s" if the repository was being updated concurrent to
    -    the "reflog expire".
    +    By not passing the OID to it we'll try to lock the reference
    +    regardless of it last known OID. Locking as a function of the OID
    +    would make "reflog expire" exit with a non-zero exit status under such
    +    contention, which in turn meant that a "gc" command (which expires
    +    reflogs before forking to the background) would encounter a hard
    +    error.
     
    -    This made "reflog expire" exit with a non-zero exit status, which in
    -    turn meant that a "gc" command (which expires reflogs before forking
    -    to the background) would encounter a hard error in such a scenario.
    +    This behavior of considering the OID when locking has been here ever
    +    since "reflog expire" was initially implemented in 4264dc15e1 ("git
    +    reflog expire", 2006-12-19). As seen in that simpler initial version
    +    of the code we subsequently use the OID to inform the expiry (and
    +    still do), but never needed to use it to lock the reference associated
    +    with the reflog.
     
    -    If we instead lock the reference without considering what OID we last
    -    saw it at, we won't encounter user-visible contention to the extent
    -    that core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 ("refs:
    -    retry acquiring reference locks for 100ms", 2017-08-21).
    +    By locking the reference without considering what OID we last saw it
    +    at, we won't encounter user-visible contention to the extent that
    +    core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 ("refs: retry
    +    acquiring reference locks for 100ms", 2017-08-21).
     
         Unfortunately this sort of probabilistic contention is hard to turn
         into a test. I've tested this by running the following three subshells

Changes since v2:

 * The "let's detect that we don't need work in reflog expire" patch
   is gone, as noted in a new set of tests/commit messages that was
   tricker than expected, and not worth it.

 * Minor rewording fixes to commit messages.

 * Further paraphrased rationale in commit messages from replies to v2
   feedback (thanks all!)

 * The "no OID" and "no mustexist" case was confusingly conflated, now
   we still do that in 8/8, but with a commit message & commit
   explaining why and how that works.

Ævar Arnfjörð Bjarmason (8):
  gc: remove redundant check for gc_auto_threshold
  gc: convert to using the_hash_algo
  gc: refactor a "call me once" pattern
  reflog tests: make use of "test_config" idiom
  reflog tests: test for the "points nowhere" warning
  reflog tests: assert lack of early exit with expiry="never"
  gc: handle & check gc.reflogExpire config
  reflog expire: don't assert the OID when locking refs

 builtin/gc.c         | 37 +++++++++++++++++++++++++++++--------
 refs/files-backend.c | 15 ++++++++++++++-
 t/t1410-reflog.sh    | 25 +++++++++++++++++--------
 t/t6500-gc.sh        | 19 +++++++++++++++++++
 4 files changed, 79 insertions(+), 17 deletions(-)

Range-diff:
1:  f11699d8e77 = 1:  81694c82130 gc: remove redundant check for gc_auto_threshold
2:  e18433f9c6b ! 2:  4bdcf1d0be2 gc: convert to using the_hash_algo
    @@ -17,20 +17,33 @@
         but objects for that hash will share a directory storage with the
         other hash.
     
    -    Thus we could theoretically have 1k SHA-1 loose objects, and say 1
    -    million SHA-256 objects, and not notice because we're currently using
    -    SHA-1.
    +    Thus we could theoretically have e.g. 1k SHA-1 loose objects, and 1
    +    million SHA-256 objects. Then not notice that we need to pack them
    +    because we're currently using SHA-1, even though our FS may be
    +    straining under the stress of such humongous directories.
     
         So assuming that "gc" eventually learns to pack up both SHA-1 and
    -    SHA-256 objects regardless of what the current the_hash_alg is perhaps
    -    this check should be changed to consider all files in objects/17/
    -    matching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and
    -    SHA-256).
    +    SHA-256 objects regardless of what the current the_hash_algo is,
    +    perhaps this check should be changed to consider all files in
    +    objects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both
    +    SHA-1 and SHA-256).
     
         But none of that is something we need to worry about now, and
         supporting both 38 and 62 characters depending on "the_hash_algo"
         removes another case of SHA-1 hardcoding.
     
    +    As noted in [1] I'm making no effort to somehow remove the hardcoding
    +    for "2" as in "use the first two hexdigits for the directory
    +    name". There's no indication that that'll ever change, and somehow
    +    generalizing it here would be a drop in the ocean, so there's no point
    +    in doing that. It also couldn't be done without coming up with some
    +    generalized version of the magical "objects/17" directory. See [2] for
    +    a discussion of that directory.
    +
    +    1. https://public-inbox.org/git/874l84ber7.fsf@evledraar.gmail.com/
    +
    +    2. https://public-inbox.org/git/87k1mta9x5.fsf@evledraar.gmail.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      diff --git a/builtin/gc.c b/builtin/gc.c
3:  54e4bce91c3 = 3:  9444a1233af gc: refactor a "call me once" pattern
4:  82f87db1348 = 4:  60a06ae6185 reflog tests: make use of "test_config" idiom
-:  ----------- > 5:  52838fdc449 reflog tests: test for the "points nowhere" warning
5:  c79608dbbb3 ! 6:  6063429f108 reflog: exit early if there's no work to do
    @@ -1,32 +1,25 @@
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    -    reflog: exit early if there's no work to do
    +    reflog tests: assert lack of early exit with expiry="never"
     
         When gc.reflogExpire and gc.reflogExpireUnreachable are set to "never"
         and --stale-fix isn't in effect (covered by the first part of the "if"
    -    statement being modified here) we can exit early without pointlessly
    -    looping over all the reflogs.
    +    statement being modified here) we *could* exit early without
    +    pointlessly looping over all the reflogs.
     
    -    Signed-off-by: Jeff King <peff@peff.net>
    -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    However, as an earlier change to add a test for the "points nowhere"
    +    warning shows even in such a mode we might want to print out a
    +    warning.
     
    - diff --git a/builtin/reflog.c b/builtin/reflog.c
    - --- a/builtin/reflog.c
    - +++ b/builtin/reflog.c
    -@@
    - 		mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);
    - 		if (flags & EXPIRE_REFLOGS_VERBOSE)
    - 			putchar('\n');
    -+	} else if (!cb.cmd.expire_total && !cb.cmd.expire_unreachable) {
    -+		/*
    -+		 * If we're not expiring anything and not dropping
    -+		 * stale entries, there's no point in even opening the
    -+		 * reflogs, since we're guaranteed to do nothing.
    -+		 */
    -+		return 0;
    - 	}
    - 
    - 	if (do_all) {
    +    So while it's conceivable to implement this, I don't think it's worth
    +    it. It's going to be too easy to inadvertently add some flag that'll
    +    make the expiry happen anyway, and even with "never" we'd like to see
    +    all the lines we're going to keep.
    +
    +    So let's assert that we're going to loop over all the references even
    +    when this configuration is in effect.
    +
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
      --- a/t/t1410-reflog.sh
    @@ -37,7 +30,7 @@
      
     -	git reflog expire --verbose --all &&
     +	git reflog expire --verbose --all >output &&
    -+	test_line_count = 0 output &&
    ++	test_line_count = 9 output &&
     +
      	git reflog refs/heads/master >output &&
      	test_line_count = 4 output
6:  c47dedab58d ! 7:  6693d1d84da gc: don't run "reflog expire" when keeping reflogs
    @@ -1,22 +1,38 @@
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    -    gc: don't run "reflog expire" when keeping reflogs
    +    gc: handle & check gc.reflogExpire config
     
         Don't redundantly run "git reflog expire --all" when gc.reflogExpire
    -    and gc.reflogExpireUnreachable are set to "never".
    +    and gc.reflogExpireUnreachable are set to "never", and die immediately
    +    if those configuration valuer are bad.
     
    -    An earlier change taught "git reflog expire" itself to exit early
    -    under this scenario, so in some sense this isn't strictly
    -    necessary. Reasons to also do it here:
    +    As an earlier "assert lack of early exit" change to the tests for "git
    +    reflog expire" shows, an early check of gc.reflogExpire{Unreachable,}
    +    isn't wanted in general for "git reflog expire", but it makes sense
    +    for "gc" because:
     
    -     1) Similar to 8ab5aa4bd8 ("parseopt: handle malformed --expire
    -        arguments more nicely", 2018-04-21). We'll die early if the config
    -        variables are set to invalid values. We run "pack-refs" before
    -        "reflog expire", which can take a while, only to then die on an
    -        invalid gc.reflogExpire{Unreachable,} configuration.
    +     1) Similarly to 8ab5aa4bd8 ("parseopt: handle malformed --expire
    +        arguments more nicely", 2018-04-21) we'll now die early if the
    +        config variables are set to invalid values.
    +
    +        We run "pack-refs" before "reflog expire", which can take a while,
    +        only to then die on an invalid gc.reflogExpire{Unreachable,}
    +        configuration.
     
          2) Not invoking the command at all means it won't show up in trace
    -        output, which makes what's going on more obvious.
    +        output, which makes what's going on more obvious when the two are
    +        set to "never".
    +
    +     3) As a later change documents we lock the refs when looping over the
    +        refs to expire, even in cases where we end up doing nothing due to
    +        this config.
    +
    +        For the reasons noted in the earlier "assert lack of early exit"
    +        change I don't think it's worth it to bend over backwards in "git
    +        reflog expire" itself to carefully detect if we'll really do
    +        nothing given the combination of all its possible options and skip
    +        that locking, but that's easy to detect here in "gc" where we'll
    +        only run "reflog expire" in a relatively simple mode.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
7:  55dd203a042 ! 8:  e0814569aba reflog expire: don't assert the OID when locking refs
    @@ -79,6 +79,25 @@
         really tight loop). That can be resolved by being more generous with
         higher values of core.filesRefLockTimeout than the 100ms default.
     
    +    As noted in the commentary being added here we also need to handle the
    +    case of references being racily deleted, that can be tested by adding
    +    this to the above:
    +
    +        (
    +            cd /tmp/git-clone &&
    +            while git branch topic master && git branch -D topic
    +            do
    +                date
    +            done
    +        )
    +
    +    We could change lock_ref_oid_basic() to always pass down
    +    RESOLVE_REF_READING to refs_resolve_ref_unsafe() and then
    +    files_reflog_expire() to detect the "is it deleted?" state. But let's
    +    not bother, in the event of such a race we're going to redundantly
    +    create a lock on the deleted reference, and shortly afterwards handle
    +    that case and others with the refs_reflog_exists() check.
    +
         1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    @@ -87,11 +106,32 @@
      --- a/refs/files-backend.c
      +++ b/refs/files-backend.c
     @@
    + 	 * The reflog file is locked by holding the lock on the
      	 * reference itself, plus we might need to update the
      	 * reference if --updateref was specified:
    ++	 *
    ++	 * We don't pass down the oid here because we'd like to be
    ++	 * tolerant to the OID of the ref having changed, and to
    ++	 * gracefully handle the case where it's been deleted (see oid
    ++	 * -> mustexist -> RESOLVE_REF_READING in
    ++	 * lock_ref_oid_basic()) ...
      	 */
     -	lock = lock_ref_oid_basic(refs, refname, oid,
    -+	lock = lock_ref_oid_basic(refs, refname, NULL /* NOT oid! */,
    ++	lock = lock_ref_oid_basic(refs, refname, NULL,
      				  NULL, NULL, REF_NO_DEREF,
      				  &type, &err);
      	if (!lock) {
    +@@
    + 		strbuf_release(&err);
    + 		return -1;
    + 	}
    ++	/*
    ++	 * When refs are deleted their reflog is deleted before the
    ++	 * loose ref is deleted. This catches that case, i.e. when
    ++	 * racing against a ref deletion lock_ref_oid_basic() will
    ++	 * have acquired a lock on the now-deleted ref, but here's
    ++	 * where we find out it has no reflog anymore.
    ++	 */
    + 	if (!refs_reflog_exists(ref_store, refname)) {
    + 		unlock_ref(lock);
    + 		return 0;

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

All made sense (admittedly from a cursory look).  Will queue.

Thanks, all.

It seems the list software dislikes Michael Haggerty for some reason, but as seen in my reply to his message my v4 has some unaddressed issues in the previous 8/8 that I need to get to: https://public-inbox.org/git/87pnqkco8v.fsf@evledraar.gmail.com/

I'll have limited time next week to get to that, so in the meantime here's a re-send without that patch, but just the unrelated cleanup/tests & gc "expire" fix up to v3's 7/8. The only other change is fixing commit message nonsense (from an earlier version), which Peff pointed out.

I'll then get to the issue mhaggerty noted & submit that independently later, but this should be ready for queuing & moving down to next, since (unlike the previous 8/8) none of it's tricky code we need to be really careful with.

Ævar Arnfjörð Bjarmason (7):
  gc: remove redundant check for gc_auto_threshold
  gc: convert to using the_hash_algo
  gc: refactor a "call me once" pattern
  reflog tests: make use of "test_config" idiom
  reflog tests: test for the "points nowhere" warning
  reflog tests: assert lack of early exit with expiry="never"
  gc: handle & check gc.reflogExpire config

 builtin/gc.c      | 37 +++++++++++++++++++++++++++++--------
 t/t1410-reflog.sh | 25 +++++++++++++++++--------
 t/t6500-gc.sh     | 19 +++++++++++++++++++
 3 files changed, 65 insertions(+), 16 deletions(-)

Range-diff:
1:  81694c8213 = 1:  be889156db gc: remove redundant check for gc_auto_threshold
2:  4bdcf1d0be = 2:  764c9a7380 gc: convert to using the_hash_algo
3:  9444a1233a = 3:  d521c22103 gc: refactor a "call me once" pattern
4:  60a06ae618 = 4:  768aba9889 reflog tests: make use of "test_config" idiom
5:  52838fdc44 = 5:  2ddbee93a1 reflog tests: test for the "points nowhere" warning
6:  6063429f10 ! 6:  97e3d74371 reflog tests: assert lack of early exit with expiry="never"
    @@ -3,8 +3,7 @@
         reflog tests: assert lack of early exit with expiry="never"
     
         When gc.reflogExpire and gc.reflogExpireUnreachable are set to "never"
    -    and --stale-fix isn't in effect (covered by the first part of the "if"
    -    statement being modified here) we *could* exit early without
    +    and --stale-fix isn't in effect we *could* exit early without
         pointlessly looping over all the reflogs.
     
         However, as an earlier change to add a test for the "points nowhere"
7:  6693d1d84d = 7:  48e5c234ae gc: handle & check gc.reflogExpire config
8:  e0814569ab < -:  ---------- reflog expire: don't assert the OID when locking refs

Checking gc_auto_threshold in too_many_loose_objects() was added in 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.", 2007-09-17) when need_to_gc() itself was also reliant on gc_auto_pack_limit before its early return:

    gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0

When that check was simplified to just checking "gc_auto_threshold <= 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by assigning 0 to gc.auto", 2008-03-19) this unreachable code should have been removed. We only call too_many_loose_objects() from within need_to_gc() itself, which will return if this condition holds, and in cmd_gc() which will return before ever getting to "auto_gc && too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true earlier in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 3 ---
 1 file changed, 3 deletions(-)

There's been a lot of changing of the hardcoded "40" values to the_hash_algo->hexsz, but we've so far missed this one where we hardcoded 38 for the loose object file length.

This is because a SHA-1 like abcde[...] gets turned into objects/ab/cde[...]. There's no reason to suppose the same won't be the case for SHA-256, and reading between the lines in hash-function-transition.txt the format is planned to be the same.

In the future we may want to further modify this code for the hash function transition. There's a potential pathological case here where we'll only consider the loose objects for the currently active hash, but objects for that hash will share a directory storage with the other hash.

Thus we could theoretically have e.g. 1k SHA-1 loose objects, and 1 million SHA-256 objects. Then not notice that we need to pack them because we're currently using SHA-1, even though our FS may be straining under the stress of such humongous directories.

So assuming that "gc" eventually learns to pack up both SHA-1 and SHA-256 objects regardless of what the current the_hash_algo is, perhaps this check should be changed to consider all files in objects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and SHA-256).

But none of that is something we need to worry about now, and supporting both 38 and 62 characters depending on "the_hash_algo" removes another case of SHA-1 hardcoding.

As noted in [1] I'm making no effort to somehow remove the hardcoding for "2" as in "use the first two hexdigits for the directory name". There's no indication that that'll ever change, and somehow generalizing it here would be a drop in the ocean, so there's no point in doing that. It also couldn't be done without coming up with some generalized version of the magical "objects/17" directory. See [2] for a discussion of that directory.

1. https://public-inbox.org/git/874l84ber7.fsf@evledraar.gmail.com/

2. https://public-inbox.org/git/87k1mta9x5.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Change an idiom we're using to ensure that gc_before_repack() only does work once (see 62aad1849f ("gc --auto: do not lock refs in the background", 2014-05-25)) to be more obvious.

Nothing except this function cares about the "pack_refs" and "prune_reflogs" variables, so let's not leave the reader wondering if they're being zero'd out for later use somewhere else.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

The "git reflog expire" command when given an unknown reference has since 4264dc15e1 ("git reflog expire", 2006-12-19) when this command was implemented emit an error, but this has never been tested for.

Let's test for it, also under gc.reflogExpire{Unreachable,}=never in case a future change is tempted to take shortcuts in the presence of such config.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1410-reflog.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

Change a couple of tests that weren't using the helper to use it. This makes the trailing "--unset" unnecessary.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1410-reflog.sh | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Don't redundantly run "git reflog expire --all" when gc.reflogExpire and gc.reflogExpireUnreachable are set to "never", and die immediately if those configuration valuer are bad.

As an earlier "assert lack of early exit" change to the tests for "git reflog expire" shows, an early check of gc.reflogExpire{Unreachable,} isn't wanted in general for "git reflog expire", but it makes sense for "gc" because:

 1) Similarly to 8ab5aa4bd8 ("parseopt: handle malformed --expire
    arguments more nicely", 2018-04-21) we'll now die early if the
    config variables are set to invalid values.

    We run "pack-refs" before "reflog expire", which can take a while,
    only to then die on an invalid gc.reflogExpire{Unreachable,}
    configuration.

 2) Not invoking the command at all means it won't show up in trace
    output, which makes what's going on more obvious when the two are
    set to "never".

 3) As a later change documents we lock the refs when looping over the
    refs to expire, even in cases where we end up doing nothing due to
    this config.

    For the reasons noted in the earlier "assert lack of early exit"
    change I don't think it's worth it to bend over backwards in "git
    reflog expire" itself to carefully detect if we'll really do
    nothing given the combination of all its possible options and skip
    that locking, but that's easy to detect here in "gc" where we'll
    only run "reflog expire" in a relatively simple mode.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c  | 17 +++++++++++++++++
 t/t6500-gc.sh | 19 +++++++++++++++++++
 2 files changed, 36 insertions(+)

When gc.reflogExpire and gc.reflogExpireUnreachable are set to "never" and --stale-fix isn't in effect we *could* exit early without pointlessly looping over all the reflogs.

However, as an earlier change to add a test for the "points nowhere" warning shows even in such a mode we might want to print out a warning.

So while it's conceivable to implement this, I don't think it's worth it. It's going to be too easy to inadvertently add some flag that'll make the expiry happen anyway, and even with "never" we'd like to see all the lines we're going to keep.

So let's assert that we're going to loop over all the references even when this configuration is in effect.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1410-reflog.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Checking gc_auto_threshold in too_many_loose_objects() was added in 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.", 2007-09-17) when need_to_gc() itself was also reliant on gc_auto_pack_limit before its early return:

    gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0

When that check was simplified to just checking "gc_auto_threshold <= 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by assigning 0 to gc.auto", 2008-03-19) this unreachable code should have been removed. We only call too_many_loose_objects() from within need_to_gc() itself, which will return if this condition holds, and in cmd_gc() which will return before ever getting to "auto_gc && too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true earlier in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 3 ---
 1 file changed, 3 deletions(-)

There's been a lot of changing of the hardcoded "40" values to the_hash_algo->hexsz, but we've so far missed this one where we hardcoded 38 for the loose object file length.

This is because a SHA-1 like abcde[...] gets turned into objects/ab/cde[...]. There's no reason to suppose the same won't be the case for SHA-256, and reading between the lines in hash-function-transition.txt the format is planned to be the same.

In the future we may want to further modify this code for the hash function transition. There's a potential pathological case here where we'll only consider the loose objects for the currently active hash, but objects for that hash will share a directory storage with the other hash.

Thus we could theoretically have e.g. 1k SHA-1 loose objects, and 1 million SHA-256 objects. Then not notice that we need to pack them because we're currently using SHA-1, even though our FS may be straining under the stress of such humongous directories.

So assuming that "gc" eventually learns to pack up both SHA-1 and SHA-256 objects regardless of what the current the_hash_algo is, perhaps this check should be changed to consider all files in objects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and SHA-256).

But none of that is something we need to worry about now, and supporting both 38 and 62 characters depending on "the_hash_algo" removes another case of SHA-1 hardcoding.

As noted in [1] I'm making no effort to somehow remove the hardcoding for "2" as in "use the first two hexdigits for the directory name". There's no indication that that'll ever change, and somehow generalizing it here would be a drop in the ocean, so there's no point in doing that. It also couldn't be done without coming up with some generalized version of the magical "objects/17" directory. See [2] for a discussion of that directory.

1. https://public-inbox.org/git/874l84ber7.fsf@evledraar.gmail.com/

2. https://public-inbox.org/git/87k1mta9x5.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Change an idiom we're using to ensure that gc_before_repack() only does work once (see 62aad1849f ("gc --auto: do not lock refs in the background", 2014-05-25)) to be more obvious.

Nothing except this function cares about the "pack_refs" and "prune_reflogs" variables, so let's not leave the reader wondering if they're being zero'd out for later use somewhere else.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Change a couple of tests that weren't using the helper to use it. This makes the trailing "--unset" unnecessary.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1410-reflog.sh | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

The "git reflog expire" command when given an unknown reference has since 4264dc15e1 ("git reflog expire", 2006-12-19) when this command was implemented emit an error, but this has never been tested for.

Let's test for it, also under gc.reflogExpire{Unreachable,}=never in case a future change is tempted to take shortcuts in the presence of such config.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1410-reflog.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

When gc.reflogExpire and gc.reflogExpireUnreachable are set to "never" and --stale-fix isn't in effect (covered by the first part of the "if" statement being modified here) we *could* exit early without pointlessly looping over all the reflogs.

However, as an earlier change to add a test for the "points nowhere" warning shows even in such a mode we might want to print out a warning.

So while it's conceivable to implement this, I don't think it's worth it. It's going to be too easy to inadvertently add some flag that'll make the expiry happen anyway, and even with "never" we'd like to see all the lines we're going to keep.

So let's assert that we're going to loop over all the references even when this configuration is in effect.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1410-reflog.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Fri, Mar 15, 2019 at 04:59:57PM +0100, Ævar Arnfjörð Bjarmason wrote:

Er, which "if" statement are we modifying here? :)

I assume this is leftover from the earlier attempt which actually did modify it.

-Peff

Don't redundantly run "git reflog expire --all" when gc.reflogExpire and gc.reflogExpireUnreachable are set to "never", and die immediately if those configuration valuer are bad.

As an earlier "assert lack of early exit" change to the tests for "git reflog expire" shows, an early check of gc.reflogExpire{Unreachable,} isn't wanted in general for "git reflog expire", but it makes sense for "gc" because:

 1) Similarly to 8ab5aa4bd8 ("parseopt: handle malformed --expire
    arguments more nicely", 2018-04-21) we'll now die early if the
    config variables are set to invalid values.

    We run "pack-refs" before "reflog expire", which can take a while,
    only to then die on an invalid gc.reflogExpire{Unreachable,}
    configuration.

 2) Not invoking the command at all means it won't show up in trace
    output, which makes what's going on more obvious when the two are
    set to "never".

 3) As a later change documents we lock the refs when looping over the
    refs to expire, even in cases where we end up doing nothing due to
    this config.

    For the reasons noted in the earlier "assert lack of early exit"
    change I don't think it's worth it to bend over backwards in "git
    reflog expire" itself to carefully detect if we'll really do
    nothing given the combination of all its possible options and skip
    that locking, but that's easy to detect here in "gc" where we'll
    only run "reflog expire" in a relatively simple mode.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c  | 17 +++++++++++++++++
 t/t6500-gc.sh | 19 +++++++++++++++++++
 2 files changed, 36 insertions(+)

During reflog expiry, the cmd_reflog_expire() function first iterates over all reflogs in logs/*, and then one-by-one acquires the lock for each one to expire its reflog by getting a *.lock file on the corresponding loose ref[1] (even if the actual ref is packed).

This lock is needed, but what isn't needed is locking the loose ref as a function of the OID we found from that first iteration. By the time we get around to re-visiting the reference some of the OIDs may have changed.

Thus the verify_lock() function called by the lock_ref_oid_basic() function being changed here would fail with e.g. "ref '%s' is at %s but expected %s" if the repository was being updated concurrent to the "reflog expire".

By not passing the OID to it we'll try to lock the reference regardless of it last known OID. Locking as a function of the OID would make "reflog expire" exit with a non-zero exit status under such contention, which in turn meant that a "gc" command (which expires reflogs before forking to the background) would encounter a hard error.

This behavior of considering the OID when locking has been here ever since "reflog expire" was initially implemented in 4264dc15e1 ("git reflog expire", 2006-12-19). As seen in that simpler initial version of the code we subsequently use the OID to inform the expiry (and still do), but never needed to use it to lock the reference associated with the reflog.

By locking the reference without considering what OID we last saw it at, we won't encounter user-visible contention to the extent that core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 ("refs: retry acquiring reference locks for 100ms", 2017-08-21).

Unfortunately this sort of probabilistic contention is hard to turn into a test. I've tested this by running the following three subshells in concurrent terminals:

    (
        cd /tmp &&
        rm -rf git &&
        git init git &&
        cd git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >out &&
            git add out &&
            git commit -m"out"
        done
    )

    (
        cd /tmp &&
        rm -rf git-clone &&
        git clone file:///tmp/git git-clone &&
        cd git-clone &&
        while git pull
        do
            date
        done
    )

    (
        cd /tmp/git-clone &&
        while git reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with a "but expected" error. After this change both the "pull" and "reflog expire" will run for a while, but eventually fail because I get unlucky with core.filesRefLockTimeout (the "reflog expire" is in a really tight loop). That can be resolved by being more generous with higher values of core.filesRefLockTimeout than the 100ms default.

As noted in the commentary being added here we also need to handle the case of references being racily deleted, that can be tested by adding this to the above:

    (
        cd /tmp/git-clone &&
        while git branch topic master && git branch -D topic
        do
            date
        done
    )

We could change lock_ref_oid_basic() to always pass down RESOLVE_REF_READING to refs_resolve_ref_unsafe() and then files_reflog_expire() to detect the "is it deleted?" state. But let's not bother, in the event of such a race we're going to redundantly create a lock on the deleted reference, and shortly afterwards handle that case and others with the refs_reflog_exists() check.

1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

On Thu, Mar 14, 2019 at 01:34:32PM +0100, Ævar Arnfjörð Bjarmason wrote:

Yep, for the record, those are fine (actually, I guess one of them got axed in v3).

Even after reading your v3 reasoning, I still think it would probably be OK for git-reflog to treat the "never" case as a noop. But frankly it's not worth even spending any more brain cycles on. I can't imagine why somebody would bother to run "reflog expire" without any expiration, outside of git-gc.

The improved commit messages all made sense to me (including the NULL oid bits in the final one), with the exception of one nit I pointed out in one of the v3 messages.

-Peff

Checking gc_auto_threshold in too_many_loose_objects() was added in 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.", 2007-09-17) when need_to_gc() itself was also reliant on gc_auto_pack_limit before its early return:

    gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0

When that check was simplified to just checking "gc_auto_threshold <= 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by assigning 0 to gc.auto", 2008-03-19) this unreachable code should have been removed. We only call too_many_loose_objects() from within need_to_gc() itself, which will return if this condition holds, and in cmd_gc() which will return before ever getting to "auto_gc && too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true earlier in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 3 ---
 1 file changed, 3 deletions(-)

There's been a lot of changing of the hardcoded "40" values to the_hash_algo->hexsz, but we've so far missed this one where we hardcoded 38 for the loose object file length.

This is because a SHA-1 like abcde[...] gets turned into objects/ab/cde[...]. There's no reason to suppose the same won't be the case for SHA-256, and reading between the lines in hash-function-transition.txt the format is planned to be the same.

In the future we may want to further modify this code for the hash function transition. There's a potential pathological case here where we'll only consider the loose objects for the currently active hash, but objects for that hash will share a directory storage with the other hash.

Thus we could theoretically have 1k SHA-1 loose objects, and say 1 million SHA-256 objects, and not notice because we're currently using SHA-1.

So assuming that "gc" eventually learns to pack up both SHA-1 and SHA-256 objects regardless of what the current the_hash_alg is perhaps this check should be changed to consider all files in objects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and SHA-256).

But none of that is something we need to worry about now, and supporting both 38 and 62 characters depending on "the_hash_algo" removes another case of SHA-1 hardcoding.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Thu, Mar 14, 2019 at 7:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Wow. Good catch.

Since you're removing hard coded numbers. Another option is a combination of strlen(basename()) and loose_object_path(the_repository, , null_oid). They should also give the same 38. Then if loose object format is updated to use 3+ chars for the directory part, this code will not need update anymore.

The downside of course is a lot more code. Or you can just introduce a new function to return this "hexsz - 2", keep that function close to fill_loose_path() with a comment there that the two functions should be aligned.

On Fri, Mar 15 2019, Duy Nguyen wrote:

I think it's better to just keep hardcoding "2". We're very unlikely to ever change objects/?? in favor of e.g. objects/???, and if we were that would be a huge "hash function transition" of its own.

Hi Ævar,

On Fri, 15 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

Of course, since it is *such* a common use case, we could also simply add another field in `the_hash_algo`. That would also save on repeated magic constants.

Ciao, Dscho

Change an idiom we're using to ensure that gc_before_repack() only does work once (see 62aad1849f ("gc --auto: do not lock refs in the background", 2014-05-25)) to be more obvious.

Nothing except this function cares about the "pack_refs" and "prune_reflogs" variables, so let's not leave the reader wondering if they're being zero'd out for later use somewhere else.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Change a couple of tests that weren't using the helper to use it. This makes the trailing "--unset" unnecessary.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1410-reflog.sh | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

When gc.reflogExpire and gc.reflogExpireUnreachable are set to "never" and --stale-fix isn't in effect (covered by the first part of the "if" statement being modified here) we can exit early without pointlessly looping over all the reflogs.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c  | 7 +++++++
 t/t1410-reflog.sh | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I'm checking should_expire_reflog_ent(). With both of these being zero, we skip most of the "return 1;" except the last one cb->cmd.recno, added in 552cecc214 (Teach "git reflog" a subcommand to delete single entries, 2007-10-17). Will this shortcut affect that use case (I haven't spent time understanding that commit yet, gotta run soon)?

On Fri, Mar 15 2019, Duy Nguyen wrote:

There was a bug related to this. Fixed in v3.

During reflog expiry, the cmd_reflog_expire() function first iterates over all reflogs in logs/*, and then one-by-one acquires the lock for each one to expire its reflog by getting a *.lock file on the corresponding loose ref[1] (even if the actual ref is packed).

This lock is needed, but what isn't needed is locking the loose ref as a function of the OID we found from that first iteration. By the time we get around to re-visiting the reference some of the OIDs may have changed.

Thus the verify_lock() function called by the lock_ref_oid_basic() function being changed here would fail with e.g. "ref '%s' is at %s but expected %s" if the repository was being updated concurrent to the "reflog expire".

By not passing the OID to it we'll try to lock the reference regardless of it last known OID. Locking as a function of the OID would make "reflog expire" exit with a non-zero exit status under such contention, which in turn meant that a "gc" command (which expires reflogs before forking to the background) would encounter a hard error.

This behavior of considering the OID when locking has been here ever since "reflog expire" was initially implemented in 4264dc15e1 ("git reflog expire", 2006-12-19). As seen in that simpler initial version of the code we subsequently use the OID to inform the expiry (and still do), but never needed to use it to lock the reference associated with the reflog.

By locking the reference without considering what OID we last saw it at, we won't encounter user-visible contention to the extent that core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 ("refs: retry acquiring reference locks for 100ms", 2017-08-21).

Unfortunately this sort of probabilistic contention is hard to turn into a test. I've tested this by running the following three subshells in concurrent terminals:

    (
        cd /tmp &&
        rm -rf git &&
        git init git &&
        cd git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >out &&
            git add out &&
            git commit -m"out"
        done
    )

    (
        cd /tmp &&
        rm -rf git-clone &&
        git clone file:///tmp/git git-clone &&
        cd git-clone &&
        while git pull
        do
            date
        done
    )

    (
        cd /tmp/git-clone &&
        while git reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with a "but expected" error. After this change both the "pull" and "reflog expire" will run for a while, but eventually fail because I get unlucky with core.filesRefLockTimeout (the "reflog expire" is in a really tight loop). That can be resolved by being more generous with higher values of core.filesRefLockTimeout than the 100ms default.

1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Passing NULL oid has another side(?) effect, which I don't know if it matters at all. Before, the mustexist flag in lock_ref_oid_basic() is true. Now it's false. This affects refs_resolve_ref_unsafe() calls in there. But that's where I'm stuck.

Maybe mention this not oid in the comment block above. Or just drop it. Reading this comment without the commit message does not really help answer "why not oid?". Or perhaps /* don't verify oid */

On Fri, Mar 15 2019, Duy Nguyen wrote:

This case was tricky, I ended up doing the same thing in v3, but now very carefully explain the rationale since none of it is obvious.

Also fixed.

Don't redundantly run "git reflog expire --all" when gc.reflogExpire and gc.reflogExpireUnreachable are set to "never".

An earlier change taught "git reflog expire" itself to exit early under this scenario, so in some sense this isn't strictly necessary. Reasons to also do it here:

 1) Similar to 8ab5aa4bd8 ("parseopt: handle malformed --expire
    arguments more nicely", 2018-04-21). We'll die early if the config
    variables are set to invalid values. We run "pack-refs" before
    "reflog expire", which can take a while, only to then die on an
    invalid gc.reflogExpire{Unreachable,} configuration.

 2) Not invoking the command at all means it won't show up in trace
    output, which makes what's going on more obvious.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c  | 17 +++++++++++++++++
 t/t6500-gc.sh | 19 +++++++++++++++++++
 2 files changed, 36 insertions(+)

On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Nit. configset api normalizes the key internally, so we can safely write gc.reflogExpireUnreachable here, which is a bit easier to read.

On Fri, Mar 15 2019, Duy Nguyen wrote:

I didn't know that, do we want to do that?

I'd think that as a matter of coding style always sticking to lower case in the C code made more sense, because e.g. you might #define a config key, and then use it both in git_config_*() as well as via strcmp() in some callback. Mixing the two would lead to confusion and possible bugs as we'd refactor the former of those patterns to the latter.

On Fri, Mar 15, 2019 at 5:24 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I did a quick git grep on "git_config_get_". The majority is still in lower case. And given that this feature is not documented anywhere (I had a quick look at config.h and api-config.txt and ended up checking the code before the previous mail) yeah it's probably just safer to go with lowercase. At least until the old callback style is gone.

Hi Ævar,

On Fri, 15 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

It also saves on developer and reviewer time to stick to lower case.

Ciao, Dscho

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

When parsing end-user supplied value in a "const char *var", with

    #define REU "gc.reflogexpireunreacahble"

strcmp(var, REU) is wrong. You must use strcasecmp(var, REU) because the end-user supplied value may be in camelCase.

And once the code uses strcasecmp(var, REU), using camelCase

    #define REU "gc.reflogExpireUnreacahble"

to compare against would not break it, either. The only difference is that being able to see the word boundary may make it easier to read.

So even when git_config-*() and str*cmp() are used in a mixed way, I do not think it is a good justification to make the string constant all lowercase (assuming that camel is easier than all lowercase to read, that is).

Parsing three-level names is tricky. Without breaking it down into components first, you cannot use strcmp nor strcasecmp; comparing the whole variable name with either function is wrong.

Checking gc_auto_threshold in too_many_loose_objects() was added in 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.", 2007-09-17) when need_to_gc() itself was also reliant on gc_auto_pack_limit before its early return:

    gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0

When that check was simplified to just checking "gc_auto_threshold <= 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by assigning 0 to gc.auto", 2008-03-19) this unreachable code should have been removed. We only call too_many_loose_objects() from within need_to_gc() itself, which will return if this condition holds, and in cmd_gc() which will return before ever getting to "auto_gc && too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true earlier in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 3 ---
 1 file changed, 3 deletions(-)

On Thu, Mar 14, 2019 at 12:54:35AM +0100, Ævar Arnfjörð Bjarmason wrote:

Yep, this is an obvious good cleanup.

-Peff

There's been a lot of changing of the hardcoded "40" values to the_hash_algo->hexsz, but we've so far missed this one where we hardcoded 38 for the loose object file length.

This is because a SHA-1 like abcde[...] gets turned into objects/ab/cde[...]. There's no reason to suppose the same won't be the case for SHA-256, and reading between the lines in hash-function-transition.txt the format is planned to be the same.

However, we may want to modify this code for the hash function transition. There's a potential pathological case here where we'll only consider the loose objects for the currently active hash, but objects for that hash will share a directory storage with the other hash.

Thus we could theoretically have 1k SHA-1 loose objects, and say 1 million SHA-256 objects, and not notice because we're currently using SHA-1.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On Thu, Mar 14, 2019 at 12:54:36AM +0100, Ævar Arnfjörð Bjarmason wrote:

Yep, makes sense.

I agree that we may end up needing to touch this, but I think this patch doesn't make anything worse in that respect (and likely makes it better, since we at least know this "38" is supposed to be a hash).

It doesn't look like hexsz gets used anywhere else; is it worth having the extra variable? (Admittedly this quite a nit).

-Peff

Change an idiom we're using to ensure that gc_before_repack() only does work once (see 62aad1849f ("gc --auto: do not lock refs in the background", 2014-05-25)) to be more obvious.

Nothing except this function cares about the "pack_refs" and "prune_reflogs" variables, so let's not leave the reader wondering if they're being zero'd out for later use somewhere else.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

On Thu, Mar 14, 2019 at 12:54:37AM +0100, Ævar Arnfjörð Bjarmason wrote:

I agree the existing code is not very obvious about what it's trying to do. I think a comment would have helped a lot.

Your rewrite is definitely better than the original, but I think it might also benefit from a comment. I.e., something like:

	/*
	 * We may be called twice, as both the pre- and post-daemonized
	 * phases will call us, but running these commands more than
	 * once is pointless and wasteful.
	 */

-Peff

Don't redundantly run "git reflog expire --all" when gc.reflogExpire and gc.reflogExpireUnreachable are set to "never".

I'm being careful to not repeat the issue fixed in 8ab5aa4bd8 ("parseopt: handle malformed --expire arguments more nicely", 2018-04-21). We'll die early if the config variables are set to invalid values.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c  | 17 +++++++++++++++++
 t/t6500-gc.sh | 19 +++++++++++++++++++
 2 files changed, 36 insertions(+)

On Thu, Mar 14, 2019 at 12:54:38AM +0100, Ævar Arnfjörð Bjarmason wrote:

I think the general sentiment here makes sense, that reflog expiration should be a noop if we're set to "never" anyway.

It does feel a little funny for "gc" to be peeking into the internals of how "reflog" will make its decision about how to run, though. I doubt those rules are likely to change, but if they did, there'd be very little to remind somebody working on reflog that they need to change the matching rules here.

Is our problem running "git reflog" at all, or is it that "git reflog" does too much work even when it's been told "never"? If the latter, could we just have it exit early as soon as it's figured out the prune expiry dates it's using?

I.e., something like:

Jeff King <peff@peff.net> writes:

The option was a purely transitional measure to recover from a bad reflog state that could have been left by earlier versions of "prune" and "repack" that did not pay attention to the reflog. Perhaps we should plan to deprecate and remove it by now?

On Thu, Mar 14, 2019 at 01:51:35PM +0900, Junio C Hamano wrote:

True, though I have definitely used it over the years to clear out broken reflog entries from corrupted repositories. In most cases we should be able to do that much more simply these days, though. Since we try to keep whole segments of history reachable from an otherwise unreachable object, you should in general be able to just prune entries for which we are missing the actual object mentioned in the log.

Of course when you are talking about corruption, all rules go out the window. So it's possible you'd still need --stale-fix to cover really broken cases.

I think these days I'd more often just delete the whole reflog in such a case (once upon a time GitHub tried to use never-expiring reflogs as a sort of audit trail, but it had all sorts of complications; these days we log to a separate file).

So I wouldn't be terribly sad to see --stale-fix go away.

All that said, I do think --expire-unreachable still has to traverse to find out what's reachable. And I think it does it under lock. If I instrument the tempfile code like the patch below and run:

  GIT_TRACE_TEMPFILE=1 git reflog expire --expire-unreachable=now HEAD

on a copy of my git.git repo, I get:

  15:22:12.163725 tempfile.c:127          activating tempfile '/home/peff/compile/foo/.git/HEAD.lock'
  15:22:12.163769 tempfile.c:127          activating tempfile '/home/peff/compile/foo/.git/logs/HEAD.lock'
  15:22:13.053404 tempfile.c:312          renaming tempfile '/home/peff/compile/foo/.git/logs/HEAD.lock' to '/home/peff/compile/foo/.git/logs/HEAD'
  15:22:13.053416 tempfile.c:327          deleting tempfile '/home/peff/compile/foo/.git/HEAD.lock'

We hold HEAD.lock for almost an entire second.

(Actually, it just occurred to me that "strace -tt git ... 2>&1 | grep HEAD.lock" would produce basically the same results, no patch needed).

-Peff

---

The locking protocol for reflogs involves getting a *.lock file on the loose ref[1] (even if the actual ref is packed). This isn't needed to expire the reflog, and needlessly results promotes reference update contention to hard errors in e.g. "gc".

During reflog expiry, the cmd_reflog_expire() function first iterates over all known references, and then one-by-one acquires the lock for each one to expire its reflog. By the time it gets around to re-visiting the references some of the OIDs may have changed.

This has been the case ever since "reflog expire" was initially implemented in 4264dc15e1 ("git reflog expire", 2006-12-19). As seen in that simpler initial version of the code (and the same is the case before this change) we subsequently use the OID to inform the expiry, but never needed to use it to lock the reference associated with the reflog.

Thus the verify_lock() function would fail with e.g. "ref '%s' is at %s but expected %s" if the repository was being updated concurrent to the "reflog expire".

This made "reflog expire" exit with a non-zero exit status, which in turn meant that a "gc" command (which expires reflogs before forking to the background) would encounter a hard error in such a scenario.

If we instead lock the reference without considering what OID we last saw it at, we won't encounter user-visible contention to the extent that core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 ("refs: retry acquiring reference locks for 100ms", 2017-08-21).

Unfortunately this sort of probabilistic contention is hard to turn into a test. I've tested this by running the following three subshells in concurrent terminals:

    (
        cd /tmp &&
        rm -rf git &&
        git init git &&
        cd git &&
        while true
        do
            head -c 10 /dev/urandom | hexdump >out &&
            git add out &&
            git commit -m"out"
        done
    )

    (
        cd /tmp &&
        rm -rf git-clone &&
        git clone file:///tmp/git git-clone &&
        cd git-clone &&
        while git pull
        do
            date
        done
    )

    (
        cd /tmp/git-clone &&
        while git reflog expire --all
        do
            date
        done
    )

Before this change the "reflog expire" would fail really quickly with a "but expected" error. After this change both the "pull" and "reflog expire" will run for a while, but eventually fail because I get unlucky with core.filesRefLockTimeout (the "reflog expire" is in a really tight loop). That can be resolved by being more generous with higher values of core.filesRefLockTimeout than the 100ms default.

1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, Mar 14, 2019 at 12:54:39AM +0100, Ævar Arnfjörð Bjarmason wrote:

This first paragraph threw me off. It sounds like you are saying we don't need to take a lock, but we absolutely do. It's just that we don't need to care about the lock having some particular value. Which you do go on to explain, but I think it would be more clear by simply removing this first paragraph.

I think this part is true. I'd love to get a confirmation from Michael Haggerty, who has spent way more time thinking about ref and reflog locking than any mortal should have to. Hopefully he even still remembers some of it. :)

-Peff

On Wed, Mar 13, 2019 at 05:22:22PM +0100, Ævar Arnfjörð Bjarmason wrote:

I think it's more than parsing, though.

It's been a while since I've dug into the reflog expiration code, but my recollection is that it can actually walk parts of the graph (looking for what's reachable) while processing the reflog. That can be tens of seconds in decent-sized repositories.

-Peff

On Tue, Mar 19 2019, Michael Haggerty wrote:

Hi. Yes, thanks a lot for the feedback. Just hadn't gotten around to looping back to this yet & digging into the issue you raised.