{"thread":{"id":"50723","subject":"BUG: Race condition due to reflog expiry in \"gc\"","startedAt":"2019-03-12T23:28:10Z","lastAt":"2019-03-28T16:14:59Z","messageCount":60,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King","Duy Nguyen","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"371309","messageId":"87tvg7brlm.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":null,"subject":"BUG: Race condition due to reflog expiry in \"gc\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-12T23:28:05Z","receivedAt":"2019-03-12T23:28:10Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I'm still working on fixing a race condition I encountered in \"gc\"\nrecently, but am not 100% sure of the fix. So I thought I'd send a\nbraindump of what I have so far in case it jolts any memories.\n\nThe problem is that when we \"gc\" we run \"reflog expire --all\". This\niterates over the reflogs, and takes a *.lock for each reference.\n\nIt'll fail intermittendly in two ways:\n\n 1. If something is concurrently committing to the repo it'll fail\n because we for a tiny amount of time hold a HEAD.lock file, so HEAD\n can't be updated.\n\n 2. On a repository that's just being \"git fetch\"'d by some concurrent\n process the \"gc\" will fail, because the ref's SHA1 has changed,\n which we inspect as we aquire the lock.\n\nTo reproduce this have a repo getting a bunch of commits and \"fetch\"\nfrom it in a loop[A]. Then try to \"gc\" in it (or \"reflog expire\n--all\"). You'll get case #2 above (#1 can also be manually reproduce):\n\n $ git gc; echo $?\n error: cannot lock ref 'refs/remotes/origin/HEAD': ref 'refs/remotes/origin/HEAD' is at d13c708ae0435c88c8f7580442b70df9a40224d0 but expected 7fb5b7dd9988d040a9d29c0456874472bea2b439\n error: cannot lock ref 'refs/remotes/origin/master': ref 'refs/remotes/origin/master' is at d13c708ae0435c88c8f7580442b70df9a40224d0 but expected 7fb5b7dd9988d040a9d29c0456874472bea2b439\n fatal: failed to run reflog\n 128\n\nThis behavior is due to a combination of:\n\n - 62aad1849f (\"gc --auto: do not lock refs in the background\", 2014-05-25)\n - f3b661f766 (\"expire_reflog(): use a lock_file for rewriting the\n reflog file\", 2014-12-12)\n\nDiscussion around those changes:\n\n https://public-inbox.org/git/1400978309-25235-1-git-send-email-pclouds@gmail.com/\n https://public-inbox.org/git/CAGZ79kaPzMPcxMqsTeW5LjYNc7LbpjHE5eBPHWKvUkBU3NvGJw@mail.gmail.com/\n\nI haven't yet found a reason for why we'd intrinsically need this lock,\nand indeed this doesn't produce corruption AFAICT:\n\n diff --git a/refs/files-backend.c b/refs/files-backend.c\n index ef053f716c..ffb9fbbf3a 100644\n --- a/refs/files-backend.c\n +++ b/refs/files-backend.c\n @@ -3043,7 +3043,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \tif (!lock) {\n \t\terror(\"cannot lock ref '%s': %s\", refname, err.buf);\n \t\tstrbuf_release(&err);\n -\t\treturn -1;\n +\t\treturn 0;\n \t}\n \tif (!refs_reflog_exists(ref_store, refname)) {\n \t\tunlock_ref(lock);\n\nBut that just \"solves\" #2, not #1, and also no-oping the OID check in\nverify_lock() still has all tests pass, so we're likely missing the\nrelevant race condition tests.\n\nA.\n\n (\n cd /tmp &&\n rm -rf git &&\n git init git &&\n cd git &&\n while true\n do\n head -c 10 /dev/urandom | hexdump >out &&\n git add out &&\n git commit -m\"out\"\n done\n )\n\n (\n cd /tmp &&\n rm -rf git-clone &&\n git clone file:///tmp/git git-clone &&\n cd git-clone &&\n while true\n do\n git reset --hard &&\n git pull\n done\n )\n"},{"id":"371315","messageId":"xmqq1s3bh7ky.fsf@gitster-ct.c.googlers.com","threadId":"50723","inReplyTo":"87tvg7brlm.fsf@evledraar.gmail.com","subject":"Re: BUG: Race condition due to reflog expiry in \"gc\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T01:43:57Z","receivedAt":"2019-03-13T01:44:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I'm still working on fixing a race condition I encountered in \"gc\"\n> recently, but am not 100% sure of the fix. So I thought I'd send a\n> braindump of what I have so far in case it jolts any memories.\n>\n> The problem is that when we \"gc\" we run \"reflog expire --all\". This\n> iterates over the reflogs, and takes a *.lock for each reference.\n>\n> It'll fail intermittendly in two ways:\n>\n> 1. If something is concurrently committing to the repo it'll fail\n> because we for a tiny amount of time hold a HEAD.lock file, so HEAD\n> can't be updated.\n>\n> 2. On a repository that's just being \"git fetch\"'d by some concurrent\n> process the \"gc\" will fail, because the ref's SHA1 has changed,\n> which we inspect as we aquire the lock.\n\nBoth sounds very much expected and expectable outcome. I am not\nsure how they need to be called bugs.\n"},{"id":"371346","messageId":"87sgvrax0o.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":"xmqq1s3bh7ky.fsf@gitster-ct.c.googlers.com","subject":"Re: BUG: Race condition due to reflog expiry in \"gc\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T10:28:39Z","receivedAt":"2019-03-13T10:28:46Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 13 2019, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I'm still working on fixing a race condition I encountered in \"gc\"\n>> recently, but am not 100% sure of the fix. So I thought I'd send a\n>> braindump of what I have so far in case it jolts any memories.\n>>\n>> The problem is that when we \"gc\" we run \"reflog expire --all\". This\n>> iterates over the reflogs, and takes a *.lock for each reference.\n>>\n>> It'll fail intermittendly in two ways:\n>>\n>> 1. If something is concurrently committing to the repo it'll fail\n>> because we for a tiny amount of time hold a HEAD.lock file, so HEAD\n>> can't be updated.\n>>\n>> 2. On a repository that's just being \"git fetch\"'d by some concurrent\n>> process the \"gc\" will fail, because the ref's SHA1 has changed,\n>> which we inspect as we aquire the lock.\n>\n> Both sounds very much expected and expectable outcome. I am not\n> sure how they need to be called bugs.\n\nLet's leave aside that I started the subject with \"BUG:\" and let me\nrephrase.\n\nI was under the impression that git-gc was supposed to support operating\non a repository that's concurrently being modified, as long as you don't\nset the likes of gc.pruneExpire too aggressively.\n\nRunning a \"gc\" in a loop without \"git reflog expire --all\" and when\nwatching the repository being GC'd with:\n\n fswatch -l 0.1 -t -r . 2>&1 | grep lock\n\nWe only create .git/MERGE_RR.lock, .git/gc.pid.lock and\ngit/packed-refs.lock. These are all things that would only cause another\nconcurrent GC to fail, not a normal git command.\n\nSo the only reason a concurrent commit (case #1) fails is because of the\nrefs being locked during the reflog iteration, and similarly \"gc\" itself\nwill fail due to a concurrently updating ref (case #2).\n\nIt seems that first of all we need this, I'll submit that as a separate\npatch sometime soon:\n\n diff --git a/builtin/gc.c b/builtin/gc.c\n index 020f725acc..ae488646e1 100644\n --- a/builtin/gc.c\n +++ b/builtin/gc.c\n @@ -127,6 +127,12 @@ static void gc_config(void)\n \t\t\tpack_refs = git_config_bool(\"gc.packrefs\", value);\n \t}\n\n +\tif (!git_config_get_value(\"gc.reflogexpire\", &value) && value &&\n +\t !strcmp(value, \"never\") &&\n +\t !git_config_get_value(\"gc.reflogexpireunreachable\", &value) && value &&\n +\t !strcmp(value, \"never\"))\n +\t\tprune_reflogs = 0;\n +\n \tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\n\nI.e. now even if your gc.* config says you don't want the reflogs\ntouched, we still pointlessly iterate over all of them. The case I'm\nrunning into (a variant of #2) is one solved by that patch, i.e. I'm\nfine \"gc\" just having the reflogs kept forever as a workaround in this\ncase.\n\nSomething like that should have been added back in 62aad1849f (\"gc\n--auto: do not lock refs in the background\", 2014-05-25), i.e. now the\n\"prune_reflogs\" variable is never used, it's just cargo-culted from a\ncopy/pasting of the \"pack_refs\" code.\n\nIn other \"gc\" phases in \"pack-objects\" and \"prune\" we also look at the\nreflogs. This obviously bad patch ignores them entirely:\n\n diff --git a/builtin/prune.c b/builtin/prune.c\n index 97613eccb5..bccee7813e 100644\n --- a/builtin/prune.c\n +++ b/builtin/prune.c\n @@ -41,7 +41,7 @@ static void perform_reachability_traversal(struct rev_info *revs)\n\n \tif (show_progress)\n \t\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0);\n -\tmark_reachable_objects(revs, 1, expire, progress);\n +\tmark_reachable_objects(revs, 0, expire, progress);\n \tstop_progress(&progress);\n \tinitialized = 1;\n }\n diff --git a/builtin/repack.c b/builtin/repack.c\n index 67f8978043..618ffbfe0a 100644\n --- a/builtin/repack.c\n +++ b/builtin/repack.c\n @@ -364,7 +364,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t keep_pack_list.items[i].string);\n \targv_array_push(&cmd.args, \"--non-empty\");\n \targv_array_push(&cmd.args, \"--all\");\n -\targv_array_push(&cmd.args, \"--reflog\");\n \targv_array_push(&cmd.args, \"--indexed-objects\");\n \tif (repository_format_partial_clone)\n \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n\nI'm just including that as illustration that add_reflogs_to_pending() in\nrevision.c during \"gc\" already iterates over the reflogs without locking\nanything, but of course it's just reading them.\n\nSo one thing that would mitigate things a lot is if\nfiles_reflog_expire() and its call to expire_reflog_ent() via\nrefs_for_each_reflog_ent() would lazily aquire the lock on the ref.\n\nDigging a bit further that's actually what we're doing now since\n4ff0f01cb7 (\"refs: retry acquiring reference locks for 100ms\",\n2017-08-21).\n\nBut this runs into the logic we've had for a long time, or since your\nbda3a31cc7 (\"reflog-expire: Avoid creating new files in a directory\ninside readdir(3) loop\", 2008-01-25) where we first loop over all the\nrefs in the process of finding the reflogs, and then will try to lock\nthose refs at those expected SHA-1s. If they've changed in the meantime\nwe error out don't clean up the lockfile.\n\nSo just this fixes that:\n\n diff --git a/refs/files-backend.c b/refs/files-backend.c\n index ef053f716c..b6576f28b9 100644\n --- a/refs/files-backend.c\n +++ b/refs/files-backend.c\n @@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n \t */\n -\tlock = lock_ref_oid_basic(refs, refname, oid,\n +\tlock = lock_ref_oid_basic(refs, refname, NULL,\n \t\t\t\t NULL, NULL, REF_NO_DEREF,\n \t\t\t\t &type, &err);\n \tif (!lock) {\n\nWhich seems sensible to me. We'll still get the lock, we just don't\nassert that the refname we use to get the lock must be at that\nSHA-1. We'll still use it for the purposes of expiry.\n\nBut maybe I've missed some caveat in reflog_expiry_prepare() and friends\nand we really do need the reflog at that OID, then this would suck less:\n\n diff --git a/builtin/reflog.c b/builtin/reflog.c\n index 4d3430900d..4bb272fdc8 100644\n --- a/builtin/reflog.c\n +++ b/builtin/reflog.c\n @@ -625,12 +625,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tfree_worktrees(worktrees);\n \t\tfor (i = 0; i < collected.nr; i++) {\n \t\t\tstruct collected_reflog *e = collected.e[i];\n +\t\t\tint st;\n \t\t\tset_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);\n -\t\t\tstatus |= reflog_expire(e->reflog, &e->oid, flags,\n -\t\t\t\t\t\treflog_expiry_prepare,\n -\t\t\t\t\t\tshould_expire_reflog_ent,\n -\t\t\t\t\t\treflog_expiry_cleanup,\n -\t\t\t\t\t\t&cb);\n +\t\t\tst = reflog_expire(e->reflog, &e->oid, flags,\n +\t\t\t\t\t reflog_expiry_prepare,\n +\t\t\t\t\t should_expire_reflog_ent,\n +\t\t\t\t\t reflog_expiry_cleanup,\n +\t\t\t\t\t &cb);\n +\t\t\tif (st == -2)\n +\t\t\t\tcontinue;\n +\t\t\tstatus |= st;\n \t\t\tfree(e);\n \t\t}\n \t\tfree(collected.e);\n diff --git a/refs/files-backend.c b/refs/files-backend.c\n index ef053f716c..8b0b6b7b85 100644\n --- a/refs/files-backend.c\n +++ b/refs/files-backend.c\n @@ -3041,6 +3041,11 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t\t\t NULL, NULL, REF_NO_DEREF,\n \t\t\t\t &type, &err);\n \tif (!lock) {\n +\t\tif (errno == EBUSY) {\n +\t\t\twarning(\"cannot lock ref '%s': %s. Skipping!\", refname, err.buf);\n +\t\t\tstrbuf_release(&err);\n +\t\t\treturn -2;\n +\t\t}\n \t\terror(\"cannot lock ref '%s': %s\", refname, err.buf);\n \t\tstrbuf_release(&err);\n \t\treturn -1;\n\nI.e. we just detect the EBUSY that verify_lock() sets if the OID doesn't\nmatch, and don't prune that reflog. As seen above \"pack-objects\" and\n\"prune\" will still iterate over the same logs later for the purposes of\nreachability, so this shouldn't get us into a corrupt state due to\nthrowing away objects referenced in those logs, we'll just prune fewer\nthings than we could have.\n\nSo I think I'll use the first patch noted above as a hack to solve the\nnarrow problem I have now, but any comments on the above most\nwelcome. I'm not very familiar with the ref code in case that wasn't\nobvious already.\n\nB.t.w. the mention of f3b661f766 (\"expire_reflog(): use a lock_file for\nrewriting the reflog file\", 2014-12-12) upthread is irrelevant. That's a\ncommit where we use the lockfile code to write out the *new* reflog,\nwhich is unrelated to all of this.\n"},{"id":"371374","messageId":"20190313160204.GD24101@sigill.intra.peff.net","threadId":"50723","inReplyTo":"87sgvrax0o.fsf@evledraar.gmail.com","subject":"Re: BUG: Race condition due to reflog expiry in \"gc\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-13T16:02:05Z","receivedAt":"2019-03-13T16:03:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 11:28:39AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I was under the impression that git-gc was supposed to support operating\n> on a repository that's concurrently being modified, as long as you don't\n> set the likes of gc.pruneExpire too aggressively.\n\nTo some degree. If it has to take locks to modify items, then inherently\nthere's going to be lock contention. But we may be able to work around\nit some.\n\nA big one we used to hit at GitHub is that running `git pack-refs` needs\nto take the individual ref locks when pruning loose refs that have just\nbeen packed. We dealt with that by adding retry-with-timeout logic to\nref lock acquisition, in 4ff0f01cb7 (refs: retry acquiring reference\nlocks for 100ms, 2017-08-21). Since then, I can't remember seeing a\nsingle instance of this coming up in production use.\n\nI don't think we use a timeout with the reflog lock. Maybe we ought to.\nIt might need to be longer than the 100ms default for refs, since I\nthink we'd do more significant work during the reflog expiration. On the\nother hand, I think we hold the lock on the ref itself, too, during that\nexpiration.\n\nWe don't actually expire reflogs regularly at GitHub, so I can't say one\nway or the other if there would be lock contention there.\n\n> Running a \"gc\" in a loop without \"git reflog expire --all\" and when\n> watching the repository being GC'd with:\n> \n> fswatch -l 0.1 -t -r . 2>&1 | grep lock\n> \n> We only create .git/MERGE_RR.lock, .git/gc.pid.lock and\n> git/packed-refs.lock. These are all things that would only cause another\n> concurrent GC to fail, not a normal git command.\n\nThe packed-refs.lock can conflict with a normal operation; regular\nwriters need to update it when they delete a ref.\n\nYou'd also be locking regular refs as part of \"pack-refs --prune\", but\nyou probably don't see it running gc in a loop, because all of the loose\nrefs are pruned after the first run.\n\n> I'm just including that as illustration that add_reflogs_to_pending() in\n> revision.c during \"gc\" already iterates over the reflogs without locking\n> anything, but of course it's just reading them.\n\nRight. It's always safe to read without locking (refs, packed-refs, and\nreflogs).\n\n> So just this fixes that:\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index ef053f716c..b6576f28b9 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n> \t * reference itself, plus we might need to update the\n> \t * reference if --updateref was specified:\n> \t */\n> -\tlock = lock_ref_oid_basic(refs, refname, oid,\n> +\tlock = lock_ref_oid_basic(refs, refname, NULL,\n> \t\t\t\t NULL, NULL, REF_NO_DEREF,\n> \t\t\t\t &type, &err);\n> \tif (!lock) {\n> \n> Which seems sensible to me. We'll still get the lock, we just don't\n> assert that the refname we use to get the lock must be at that\n> SHA-1. We'll still use it for the purposes of expiry.\n\nI _think_ this should be OK, because we don't open the reflog until\nafter we hold the lock. So we don't really care what the value was at.\nWherever we got the lock, we'll do the expiration process atomically at\nthat point.\n\nI don't think this makes all your problems go away, though, because\nyou'd still have the immediate contention of actually taking the lock\n(at any value).\n\n> But maybe I've missed some caveat in reflog_expiry_prepare() and friends\n> and we really do need the reflog at that OID, then this would suck less:\n\nIf I'm reading the code correctly, we don't call reflog_expiry_prepare()\nuntil we're holding the lock (because we pass it in as a callback to\nreflog_expire()). So I think that would be OK.\n\n> \tif (!lock) {\n> +\t\tif (errno == EBUSY) {\n> +\t\t\twarning(\"cannot lock ref '%s': %s. Skipping!\", refname, err.buf);\n> +\t\t\tstrbuf_release(&err);\n> +\t\t\treturn -2;\n> +\t\t}\n> \t\terror(\"cannot lock ref '%s': %s\", refname, err.buf);\n> \t\tstrbuf_release(&err);\n> \t\treturn -1;\n> \n> I.e. we just detect the EBUSY that verify_lock() sets if the OID doesn't\n> match, and don't prune that reflog.\n\nThat saves git-gc from failing. But do we have the problem in the other\ndirection? I.e., that the gc would take a lock, and the actual user\nrequest to update a ref would fail?\n\nThat's what the retry-with-timeout is supposed to address, so maybe it\nworks. But I wouldn't be surprised if it's insufficient in practice,\nsince the reflog code may walk big parts of the graph under lock,\nchecking reachability.\n\n-Peff\n"},{"id":"371376","messageId":"87imwmbv7l.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":"20190313160204.GD24101@sigill.intra.peff.net","subject":"Re: BUG: Race condition due to reflog expiry in \"gc\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T16:22:22Z","receivedAt":"2019-03-13T16:22:30Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 13 2019, Jeff King wrote:\n\n> On Wed, Mar 13, 2019 at 11:28:39AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I was under the impression that git-gc was supposed to support operating\n>> on a repository that's concurrently being modified, as long as you don't\n>> set the likes of gc.pruneExpire too aggressively.\n>\n> To some degree. If it has to take locks to modify items, then inherently\n> there's going to be lock contention. But we may be able to work around\n> it some.\n>\n> A big one we used to hit at GitHub is that running `git pack-refs` needs\n> to take the individual ref locks when pruning loose refs that have just\n> been packed. We dealt with that by adding retry-with-timeout logic to\n> ref lock acquisition, in 4ff0f01cb7 (refs: retry acquiring reference\n> locks for 100ms, 2017-08-21). Since then, I can't remember seeing a\n> single instance of this coming up in production use.\n>\n> I don't think we use a timeout with the reflog lock. Maybe we ought to.\n> It might need to be longer than the 100ms default for refs, since I\n> think we'd do more significant work during the reflog expiration. On the\n> other hand, I think we hold the lock on the ref itself, too, during that\n> expiration.\n>\n> We don't actually expire reflogs regularly at GitHub, so I can't say one\n> way or the other if there would be lock contention there.\n>\n>> Running a \"gc\" in a loop without \"git reflog expire --all\" and when\n>> watching the repository being GC'd with:\n>>\n>> fswatch -l 0.1 -t -r . 2>&1 | grep lock\n>>\n>> We only create .git/MERGE_RR.lock, .git/gc.pid.lock and\n>> git/packed-refs.lock. These are all things that would only cause another\n>> concurrent GC to fail, not a normal git command.\n>\n> The packed-refs.lock can conflict with a normal operation; regular\n> writers need to update it when they delete a ref.\n>\n> You'd also be locking regular refs as part of \"pack-refs --prune\", but\n> you probably don't see it running gc in a loop, because all of the loose\n> refs are pruned after the first run.\n>\n>> I'm just including that as illustration that add_reflogs_to_pending() in\n>> revision.c during \"gc\" already iterates over the reflogs without locking\n>> anything, but of course it's just reading them.\n>\n> Right. It's always safe to read without locking (refs, packed-refs, and\n> reflogs).\n>\n>> So just this fixes that:\n>>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index ef053f716c..b6576f28b9 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n>> \t * reference itself, plus we might need to update the\n>> \t * reference if --updateref was specified:\n>> \t */\n>> -\tlock = lock_ref_oid_basic(refs, refname, oid,\n>> +\tlock = lock_ref_oid_basic(refs, refname, NULL,\n>> \t\t\t\t NULL, NULL, REF_NO_DEREF,\n>> \t\t\t\t &type, &err);\n>> \tif (!lock) {\n>>\n>> Which seems sensible to me. We'll still get the lock, we just don't\n>> assert that the refname we use to get the lock must be at that\n>> SHA-1. We'll still use it for the purposes of expiry.\n>\n> I _think_ this should be OK, because we don't open the reflog until\n> after we hold the lock. So we don't really care what the value was at.\n> Wherever we got the lock, we'll do the expiration process atomically at\n> that point.\n>\n> I don't think this makes all your problems go away, though, because\n> you'd still have the immediate contention of actually taking the lock\n> (at any value).\n\nYeah, but as far as I can tell core.filesRefLockTimeout added in\n4ff0f01cb7 (refs: retry acquiring reference locks for 100ms, 2017-08-21)\nsolves that.\n\nI.e. the problem after that patch isn't the contention, but the call to\nlock_ref_oid_basic() in files_reflog_expire() above which *after* we get\nthe lock effectively asserts that the value must be at the old OID.\n\nIt would then hard-die, so git-reflog would leave behind stay *.lock\nfiles that e.g. a concurrent \"commit\" would die on (still there after\n100ms, and beyond). Whereas if we're not picky about the OID the two\nwill have contention, but core.filesRefLockTimeout will work around it.\n\nI've been running this whole thing in a loop for a while with that\ns/oid/NULL/ fix, and it just works. To the extent that it doesn't (due\nto load) I can just bump core.filesRefLockTimeout up a bit.\n\nSo with the benefit of some sleep & your comment I'm pretty sure that's\nthe right fix. Ideally it would have a test, do we have some simliar\ncontention-creating tests I can piggy-back on, or maybe we should just\nchange this and hope for the best...\n\n>> But maybe I've missed some caveat in reflog_expiry_prepare() and friends\n>> and we really do need the reflog at that OID, then this would suck less:\n>\n> If I'm reading the code correctly, we don't call reflog_expiry_prepare()\n> until we're holding the lock (because we pass it in as a callback to\n> reflog_expire()). So I think that would be OK.\n>\n>> \tif (!lock) {\n>> +\t\tif (errno == EBUSY) {\n>> +\t\t\twarning(\"cannot lock ref '%s': %s. Skipping!\", refname, err.buf);\n>> +\t\t\tstrbuf_release(&err);\n>> +\t\t\treturn -2;\n>> +\t\t}\n>> \t\terror(\"cannot lock ref '%s': %s\", refname, err.buf);\n>> \t\tstrbuf_release(&err);\n>> \t\treturn -1;\n>>\n>> I.e. we just detect the EBUSY that verify_lock() sets if the OID doesn't\n>> match, and don't prune that reflog.\n>\n> That saves git-gc from failing. But do we have the problem in the other\n> direction? I.e., that the gc would take a lock, and the actual user\n> request to update a ref would fail?\n\nNo, because as with s/oid/NULL/ above core.filesRefLockTimeout saves the\nday.\n\n> That's what the retry-with-timeout is supposed to address, so maybe it\n> works. But I wouldn't be surprised if it's insufficient in practice,\n> since the reflog code may walk big parts of the graph under lock,\n> checking reachability.\n\nI suppose this can happen if the reflog for a given branch is so big\nthat it takes us more than 100ms to parse it, but e.g. O(n) refs\nshouldn't matter, since we only hold the lock on one ref at a time.\n"},{"id":"371422","messageId":"20190313235439.30439-1-avarab@gmail.com","threadId":"50723","inReplyTo":"87imwmbv7l.fsf@evledraar.gmail.com","subject":"[PATCH 0/5] gc: minor code cleanup + contention fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T23:54:34Z","receivedAt":"2019-03-13T23:55:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"[45]/5 fix a couple of issues I noted upthread. While I was at it I\nnoticed a few things I could clean up in [123]/5 that I figured I'd\nsend alongside this.\n\nÆvar Arnfjörð Bjarmason (5):\n gc: remove redundant check for gc_auto_threshold\n gc: convert to using the_hash_algo\n gc: refactor a \"call me once\" pattern\n gc: don't run \"reflog expire\" when keeping reflogs\n reflog expire: don't assert the OID when locking refs\n\n builtin/gc.c | 33 +++++++++++++++++++++++++--------\n refs/files-backend.c | 2 +-\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 3 files changed, 45 insertions(+), 9 deletions(-)\n\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371423","messageId":"20190313235439.30439-2-avarab@gmail.com","threadId":"50723","inReplyTo":"87imwmbv7l.fsf@evledraar.gmail.com","subject":"[PATCH 1/5] gc: remove redundant check for gc_auto_threshold","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T23:54:35Z","receivedAt":"2019-03-13T23:55:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Checking gc_auto_threshold in too_many_loose_objects() was added in\n17815501a8 (\"git-gc --auto: run \"repack -A -d -l\" as necessary.\",\n2007-09-17) when need_to_gc() itself was also reliant on\ngc_auto_pack_limit before its early return:\n\n gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0\n\nWhen that check was simplified to just checking \"gc_auto_threshold <=\n0\" in b14d255ba8 (\"builtin-gc.c: allow disabling all auto-gc'ing by\nassigning 0 to gc.auto\", 2008-03-19) this unreachable code should have\nbeen removed. We only call too_many_loose_objects() from within\nneed_to_gc() itself, which will return if this condition holds, and in\ncmd_gc() which will return before ever getting to \"auto_gc &&\ntoo_many_loose_objects()\" if \"auto_gc && !need_to_gc()\" is true\nearlier in the function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 020f725acc..8c2312681c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -157,9 +157,6 @@ static int too_many_loose_objects(void)\n \tint num_loose = 0;\n \tint needed = 0;\n \n-\tif (gc_auto_threshold <= 0)\n-\t\treturn 0;\n-\n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n \t\treturn 0;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371424","messageId":"20190313235439.30439-3-avarab@gmail.com","threadId":"50723","inReplyTo":"87imwmbv7l.fsf@evledraar.gmail.com","subject":"[PATCH 2/5] gc: convert to using the_hash_algo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T23:54:36Z","receivedAt":"2019-03-13T23:55:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There's been a lot of changing of the hardcoded \"40\" values to\nthe_hash_algo->hexsz, but we've so far missed this one where we\nhardcoded 38 for the loose object file length.\n\nThis is because a SHA-1 like abcde[...] gets turned into\nobjects/ab/cde[...]. There's no reason to suppose the same won't be\nthe case for SHA-256, and reading between the lines in\nhash-function-transition.txt the format is planned to be the same.\n\nHowever, we may want to modify this code for the hash function\ntransition. There's a potential pathological case here where we'll\nonly consider the loose objects for the currently active hash, but\nobjects for that hash will share a directory storage with the other\nhash.\n\nThus we could theoretically have 1k SHA-1 loose objects, and say 1\nmillion SHA-256 objects, and not notice because we're currently using\nSHA-1.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 8c2312681c..9c2c63276d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -156,6 +156,8 @@ static int too_many_loose_objects(void)\n \tint auto_threshold;\n \tint num_loose = 0;\n \tint needed = 0;\n+\tconst unsigned hexsz = the_hash_algo->hexsz;\n+\tconst unsigned hexsz_loose = hexsz - 2;\n \n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n@@ -163,8 +165,8 @@ static int too_many_loose_objects(void)\n \n \tauto_threshold = DIV_ROUND_UP(gc_auto_threshold, 256);\n \twhile ((ent = readdir(dir)) != NULL) {\n-\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != 38 ||\n-\t\t ent->d_name[38] != '\\0')\n+\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != hexsz_loose ||\n+\t\t ent->d_name[hexsz_loose] != '\\0')\n \t\t\tcontinue;\n \t\tif (++num_loose > auto_threshold) {\n \t\t\tneeded = 1;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371425","messageId":"20190313235439.30439-4-avarab@gmail.com","threadId":"50723","inReplyTo":"87imwmbv7l.fsf@evledraar.gmail.com","subject":"[PATCH 3/5] gc: refactor a \"call me once\" pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T23:54:37Z","receivedAt":"2019-03-13T23:55:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change an idiom we're using to ensure that gc_before_repack() only\ndoes work once (see 62aad1849f (\"gc --auto: do not lock refs in the\nbackground\", 2014-05-25)) to be more obvious.\n\nNothing except this function cares about the \"pack_refs\" and\n\"prune_reflogs\" variables, so let's not leave the reader wondering if\nthey're being zero'd out for later use somewhere else.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 9c2c63276d..425d0fa830 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -490,14 +490,15 @@ static int report_last_gc_error(void)\n \n static void gc_before_repack(void)\n {\n+\tstatic int done = 0;\n+\tif (done++)\n+\t\treturn;\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, reflog.argv[0]);\n-\n-\tpack_refs = 0;\n-\tprune_reflogs = 0;\n }\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371426","messageId":"20190313235439.30439-5-avarab@gmail.com","threadId":"50723","inReplyTo":"87imwmbv7l.fsf@evledraar.gmail.com","subject":"[PATCH 4/5] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T23:54:38Z","receivedAt":"2019-03-13T23:55:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Don't redundantly run \"git reflog expire --all\" when gc.reflogExpire\nand gc.reflogExpireUnreachable are set to \"never\".\n\nI'm being careful to not repeat the issue fixed in\n8ab5aa4bd8 (\"parseopt: handle malformed --expire arguments more\nnicely\", 2018-04-21). We'll die early if the config variables are set\nto invalid values.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 17 +++++++++++++++++\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 425d0fa830..91b088dbfe 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -116,6 +116,19 @@ static void process_log_file_on_signal(int signo)\n \traise(signo);\n }\n \n+static int gc_config_is_timestamp_never(const char *var)\n+{\n+\tconst char *value;\n+\ttimestamp_t expire;\n+\n+\tif (!git_config_get_value(var, &value) && value) {\n+\t\tif (parse_expiry_date(value, &expire))\n+\t\t\tdie(_(\"failed to parse '%s' value '%s'\"), var, value);\n+\t\treturn expire == 0;\n+\t}\n+\treturn 0;\n+}\n+\n static void gc_config(void)\n {\n \tconst char *value;\n@@ -127,6 +140,10 @@ static void gc_config(void)\n \t\t\tpack_refs = git_config_bool(\"gc.packrefs\", value);\n \t}\n \n+\tif (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n+\t gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n+\t\tprune_reflogs = 0;\n+\n \tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 4684d06552..7411bf7fec 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -120,6 +120,25 @@ test_expect_success 'gc --quiet' '\n \ttest_must_be_empty stderr\n '\n \n+test_expect_success 'gc.reflogExpire{Unreachable,}=never skips \"expire\" via \"gc\"' '\n+\ttest_config gc.reflogExpire never &&\n+\ttest_config gc.reflogExpireUnreachable never &&\n+\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\n+\t# Check that git-pack-refs is run as a sanity check (done via\n+\t# gc_before_repack()) but that git-expire is not.\n+\tgrep -E \"^trace: (built-in|exec|run_command): git pack-refs --\" trace.out &&\n+\t! grep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n+test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"expire\" via \"gc\"' '\n+\t>trace.out &&\n+\ttest_config gc.reflogExpire never &&\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9. Furthermore, the\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371427","messageId":"20190313235439.30439-6-avarab@gmail.com","threadId":"50723","inReplyTo":"87imwmbv7l.fsf@evledraar.gmail.com","subject":"[PATCH 5/5] reflog expire: don't assert the OID when locking refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T23:54:39Z","receivedAt":"2019-03-13T23:55:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The locking protocol for reflogs involves getting a *.lock file on the\nloose ref[1] (even if the actual ref is packed). This isn't needed to\nexpire the reflog, and needlessly results promotes reference update\ncontention to hard errors in e.g. \"gc\".\n\nDuring reflog expiry, the cmd_reflog_expire() function first iterates\nover all known references, and then one-by-one acquires the lock for\neach one to expire its reflog. By the time it gets around to\nre-visiting the references some of the OIDs may have changed.\n\nThis has been the case ever since \"reflog expire\" was initially\nimplemented in 4264dc15e1 (\"git reflog expire\", 2006-12-19). As seen\nin that simpler initial version of the code (and the same is the case\nbefore this change) we subsequently use the OID to inform the expiry,\nbut never needed to use it to lock the reference associated with the\nreflog.\n\nThus the verify_lock() function would fail with e.g. \"ref '%s' is at\n%s but expected %s\" if the repository was being updated concurrent to\nthe \"reflog expire\".\n\nThis made \"reflog expire\" exit with a non-zero exit status, which in\nturn meant that a \"gc\" command (which expires reflogs before forking\nto the background) would encounter a hard error in such a scenario.\n\nIf we instead lock the reference without considering what OID we last\nsaw it at, we won't encounter user-visible contention to the extent\nthat core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 (\"refs:\nretry acquiring reference locks for 100ms\", 2017-08-21).\n\nUnfortunately this sort of probabilistic contention is hard to turn\ninto a test. I've tested this by running the following three subshells\nin concurrent terminals:\n\n (\n cd /tmp &&\n rm -rf git &&\n git init git &&\n cd git &&\n while true\n do\n head -c 10 /dev/urandom | hexdump >out &&\n git add out &&\n git commit -m\"out\"\n done\n )\n\n (\n cd /tmp &&\n rm -rf git-clone &&\n git clone file:///tmp/git git-clone &&\n cd git-clone &&\n while git pull\n do\n date\n done\n )\n\n (\n cd /tmp/git-clone &&\n while git reflog expire --all\n do\n date\n done\n )\n\nBefore this change the \"reflog expire\" would fail really quickly with\na \"but expected\" error. After this change both the \"pull\" and \"reflog\nexpire\" will run for a while, but eventually fail because I get\nunlucky with core.filesRefLockTimeout (the \"reflog expire\" is in a\nreally tight loop). That can be resolved by being more generous with\nhigher values of core.filesRefLockTimeout than the 100ms default.\n\n1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ef053f716c..4d4d226601 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n \t */\n-\tlock = lock_ref_oid_basic(refs, refname, oid,\n+\tlock = lock_ref_oid_basic(refs, refname, NULL /* NOT oid! */,\n \t\t\t\t NULL, NULL, REF_NO_DEREF,\n \t\t\t\t &type, &err);\n \tif (!lock) {\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371432","messageId":"20190314002459.GE31968@sigill.intra.peff.net","threadId":"50723","inReplyTo":"87imwmbv7l.fsf@evledraar.gmail.com","subject":"Re: BUG: Race condition due to reflog expiry in \"gc\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:25:00Z","receivedAt":"2019-03-14T00:26:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 05:22:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > That's what the retry-with-timeout is supposed to address, so maybe it\n> > works. But I wouldn't be surprised if it's insufficient in practice,\n> > since the reflog code may walk big parts of the graph under lock,\n> > checking reachability.\n> \n> I suppose this can happen if the reflog for a given branch is so big\n> that it takes us more than 100ms to parse it, but e.g. O(n) refs\n> shouldn't matter, since we only hold the lock on one ref at a time.\n\nI think it's more than parsing, though.\n\nIt's been a while since I've dug into the reflog expiration code, but my\nrecollection is that it can actually walk parts of the graph (looking\nfor what's reachable) while processing the reflog. That can be tens of\nseconds in decent-sized repositories.\n\n-Peff\n"},{"id":"371433","messageId":"20190314002530.GF31968@sigill.intra.peff.net","threadId":"50723","inReplyTo":"20190313235439.30439-2-avarab@gmail.com","subject":"Re: [PATCH 1/5] gc: remove redundant check for gc_auto_threshold","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:25:30Z","receivedAt":"2019-03-14T00:26:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 12:54:35AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Checking gc_auto_threshold in too_many_loose_objects() was added in\n> 17815501a8 (\"git-gc --auto: run \"repack -A -d -l\" as necessary.\",\n> 2007-09-17) when need_to_gc() itself was also reliant on\n> gc_auto_pack_limit before its early return:\n> \n> gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0\n> \n> When that check was simplified to just checking \"gc_auto_threshold <=\n> 0\" in b14d255ba8 (\"builtin-gc.c: allow disabling all auto-gc'ing by\n> assigning 0 to gc.auto\", 2008-03-19) this unreachable code should have\n> been removed. We only call too_many_loose_objects() from within\n> need_to_gc() itself, which will return if this condition holds, and in\n> cmd_gc() which will return before ever getting to \"auto_gc &&\n> too_many_loose_objects()\" if \"auto_gc && !need_to_gc()\" is true\n> earlier in the function.\n\nYep, this is an obvious good cleanup.\n\n-Peff\n"},{"id":"371434","messageId":"20190314002648.GG31968@sigill.intra.peff.net","threadId":"50723","inReplyTo":"20190313235439.30439-3-avarab@gmail.com","subject":"Re: [PATCH 2/5] gc: convert to using the_hash_algo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:26:48Z","receivedAt":"2019-03-14T00:28:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 12:54:36AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> There's been a lot of changing of the hardcoded \"40\" values to\n> the_hash_algo->hexsz, but we've so far missed this one where we\n> hardcoded 38 for the loose object file length.\n> \n> This is because a SHA-1 like abcde[...] gets turned into\n> objects/ab/cde[...]. There's no reason to suppose the same won't be\n> the case for SHA-256, and reading between the lines in\n> hash-function-transition.txt the format is planned to be the same.\n\nYep, makes sense.\n\n> However, we may want to modify this code for the hash function\n> transition. There's a potential pathological case here where we'll\n> only consider the loose objects for the currently active hash, but\n> objects for that hash will share a directory storage with the other\n> hash.\n> \n> Thus we could theoretically have 1k SHA-1 loose objects, and say 1\n> million SHA-256 objects, and not notice because we're currently using\n> SHA-1.\n\nI agree that we may end up needing to touch this, but I think this patch\ndoesn't make anything worse in that respect (and likely makes it better,\nsince we at least know this \"38\" is supposed to be a hash).\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 8c2312681c..9c2c63276d 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -156,6 +156,8 @@ static int too_many_loose_objects(void)\n> \tint auto_threshold;\n> \tint num_loose = 0;\n> \tint needed = 0;\n> +\tconst unsigned hexsz = the_hash_algo->hexsz;\n> +\tconst unsigned hexsz_loose = hexsz - 2;\n\nIt doesn't look like hexsz gets used anywhere else; is it worth having\nthe extra variable? (Admittedly this quite a nit).\n\n-Peff\n"},{"id":"371435","messageId":"20190314003016.GH31968@sigill.intra.peff.net","threadId":"50723","inReplyTo":"20190313235439.30439-4-avarab@gmail.com","subject":"Re: [PATCH 3/5] gc: refactor a \"call me once\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:30:16Z","receivedAt":"2019-03-14T00:31:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 12:54:37AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Change an idiom we're using to ensure that gc_before_repack() only\n> does work once (see 62aad1849f (\"gc --auto: do not lock refs in the\n> background\", 2014-05-25)) to be more obvious.\n> \n> Nothing except this function cares about the \"pack_refs\" and\n> \"prune_reflogs\" variables, so let's not leave the reader wondering if\n> they're being zero'd out for later use somewhere else.\n\nI agree the existing code is not very obvious about what it's trying to\ndo. I think a comment would have helped a lot.\n\nYour rewrite is definitely better than the original, but I think it\nmight also benefit from a comment. I.e., something like:\n\n> static void gc_before_repack(void)\n> {\n\t/*\n\t * We may be called twice, as both the pre- and post-daemonized\n\t * phases will call us, but running these commands more than\n\t * once is pointless and wasteful.\n\t */\n> +\tstatic int done = 0;\n> +\tif (done++)\n> +\t\treturn;\n\n-Peff\n"},{"id":"371436","messageId":"20190314004006.GI31968@sigill.intra.peff.net","threadId":"50723","inReplyTo":"20190313235439.30439-5-avarab@gmail.com","subject":"Re: [PATCH 4/5] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:40:06Z","receivedAt":"2019-03-14T00:41:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 12:54:38AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Don't redundantly run \"git reflog expire --all\" when gc.reflogExpire\n> and gc.reflogExpireUnreachable are set to \"never\".\n> \n> I'm being careful to not repeat the issue fixed in\n> 8ab5aa4bd8 (\"parseopt: handle malformed --expire arguments more\n> nicely\", 2018-04-21). We'll die early if the config variables are set\n> to invalid values.\n\nI think the general sentiment here makes sense, that reflog expiration\nshould be a noop if we're set to \"never\" anyway.\n\nIt does feel a little funny for \"gc\" to be peeking into the internals of\nhow \"reflog\" will make its decision about how to run, though. I doubt\nthose rules are likely to change, but if they did, there'd be very\nlittle to remind somebody working on reflog that they need to change the\nmatching rules here.\n\nIs our problem running \"git reflog\" at all, or is it that \"git reflog\"\ndoes too much work even when it's been told \"never\"? If the latter,\ncould we just have it exit early as soon as it's figured out the prune\nexpiry dates it's using?\n\nI.e., something like:\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 4d3430900d..aab221f315 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -594,6 +594,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t}\n \n+\t/*\n+\t * If we're not expiring anything and not dropping stale entries,\n+\t * there's no point in even opening the reflogs, since we're guaranteed\n+\t * to do nothing.\n+\t */\n+\tif (!cb.cmd.expire_total && !cb.cmd.expire_unreachable &&\n+\t !cb.cmd.stalefix)\n+\t\treturn 0;\n+\n \t/*\n \t * We can trust the commits and objects reachable from refs\n \t * even in older repository. We cannot trust what's reachable\n\nSeeing \"--stale-fix\" again reminded me: that may be the \"oops, we can\nspend tons of CPU walking history\" bit that I mentioned in the other\npart of the thread. But I don't think git-gc would ever run with that\noption.\n\n-Peff\n"},{"id":"371437","messageId":"20190314004409.GJ31968@sigill.intra.peff.net","threadId":"50723","inReplyTo":"20190313235439.30439-6-avarab@gmail.com","subject":"Re: [PATCH 5/5] reflog expire: don't assert the OID when locking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:44:10Z","receivedAt":"2019-03-14T00:45:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 12:54:39AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> The locking protocol for reflogs involves getting a *.lock file on the\n> loose ref[1] (even if the actual ref is packed). This isn't needed to\n> expire the reflog, and needlessly results promotes reference update\n> contention to hard errors in e.g. \"gc\".\n\nThis first paragraph threw me off. It sounds like you are saying we\ndon't need to take a lock, but we absolutely do. It's just that we don't\nneed to care about the lock having some particular value. Which you do\ngo on to explain, but I think it would be more clear by simply removing\nthis first paragraph.\n\n> If we instead lock the reference without considering what OID we last\n> saw it at, we won't encounter user-visible contention to the extent\n> that core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 (\"refs:\n> retry acquiring reference locks for 100ms\", 2017-08-21).\n\nI think this part is true. I'd love to get a confirmation from Michael\nHaggerty, who has spent way more time thinking about ref and reflog\nlocking than any mortal should have to. Hopefully he even still\nremembers some of it. :)\n\n-Peff\n"},{"id":"371455","messageId":"xmqqmuly9hyg.fsf@gitster-ct.c.googlers.com","threadId":"50723","inReplyTo":"20190314004006.GI31968@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T04:51:35Z","receivedAt":"2019-03-14T04:51:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Seeing \"--stale-fix\" again reminded me: that may be the \"oops, we can\n> spend tons of CPU walking history\" bit that I mentioned in the other\n> part of the thread. But I don't think git-gc would ever run with that\n> option.\n\nThe option was a purely transitional measure to recover from a bad\nreflog state that could have been left by earlier versions of\n\"prune\" and \"repack\" that did not pay attention to the reflog.\nPerhaps we should plan to deprecate and remove it by now?\n"},{"id":"371484","messageId":"20190314123439.4347-1-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 0/7] gc: minor code cleanup + contention fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:32Z","receivedAt":"2019-03-14T12:34:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Addresse Peff's comments to v1. For a couple of patches I've faked up\nhis SOB where I copy/pasted a comment or code from a v1 comment\nverbatim. Will see if he acks that.\n\nThe main change is a better commit message in the last patch (now\n7/7), and 2x new \"reflog\" patches to make it early exit in addition to\n\"gc\" when there's nothing to do.\n\nThere was the \"why do it at all in gc\" feedback on 6/7 in v1. I've\nadjusted the commit message there to justify it, but we'll see what\nPeff things about it this time around...\n\nÆvar Arnfjörð Bjarmason (7):\n gc: remove redundant check for gc_auto_threshold\n gc: convert to using the_hash_algo\n gc: refactor a \"call me once\" pattern\n reflog tests: make use of \"test_config\" idiom\n reflog: exit early if there's no work to do\n gc: don't run \"reflog expire\" when keeping reflogs\n reflog expire: don't assert the OID when locking refs\n\n builtin/gc.c | 37 +++++++++++++++++++++++++++++--------\n builtin/reflog.c | 7 +++++++\n refs/files-backend.c | 2 +-\n t/t1410-reflog.sh | 17 ++++++++---------\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 5 files changed, 64 insertions(+), 18 deletions(-)\n\nRange-diff:\n1: 1635c7fb22 ! 1: e18433f9c6 gc: convert to using the_hash_algo\n @@ -11,16 +11,26 @@\n the case for SHA-256, and reading between the lines in\n hash-function-transition.txt the format is planned to be the same.\n \n - However, we may want to modify this code for the hash function\n - transition. There's a potential pathological case here where we'll\n - only consider the loose objects for the currently active hash, but\n - objects for that hash will share a directory storage with the other\n - hash.\n + In the future we may want to further modify this code for the hash\n + function transition. There's a potential pathological case here where\n + we'll only consider the loose objects for the currently active hash,\n + but objects for that hash will share a directory storage with the\n + other hash.\n \n Thus we could theoretically have 1k SHA-1 loose objects, and say 1\n million SHA-256 objects, and not notice because we're currently using\n SHA-1.\n \n + So assuming that \"gc\" eventually learns to pack up both SHA-1 and\n + SHA-256 objects regardless of what the current the_hash_alg is perhaps\n + this check should be changed to consider all files in objects/17/\n + matching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and\n + SHA-256).\n +\n + But none of that is something we need to worry about now, and\n + supporting both 38 and 62 characters depending on \"the_hash_algo\"\n + removes another case of SHA-1 hardcoding.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n diff --git a/builtin/gc.c b/builtin/gc.c\n @@ -30,8 +40,7 @@\n \tint auto_threshold;\n \tint num_loose = 0;\n \tint needed = 0;\n -+\tconst unsigned hexsz = the_hash_algo->hexsz;\n -+\tconst unsigned hexsz_loose = hexsz - 2;\n ++\tconst unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n \n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n2: ced972826d ! 2: 54e4bce91c gc: refactor a \"call me once\" pattern\n @@ -10,6 +10,7 @@\n \"prune_reflogs\" variables, so let's not leave the reader wondering if\n they're being zero'd out for later use somewhere else.\n \n + Signed-off-by: Jeff King \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n diff --git a/builtin/gc.c b/builtin/gc.c\n @@ -19,6 +20,11 @@\n \n static void gc_before_repack(void)\n {\n ++\t/*\n ++\t * We may be called twice, as both the pre- and\n ++\t * post-daemonized phases will call us, but running these\n ++\t * commands more than once is pointless and wasteful.\n ++\t */\n +\tstatic int done = 0;\n +\tif (done++)\n +\t\treturn;\n-: ---------- > 3: 82f87db134 reflog tests: make use of \"test_config\" idiom\n-: ---------- > 4: c79608dbbb reflog: exit early if there's no work to do\n3: 599772f2bd ! 5: c47dedab58 gc: don't run \"reflog expire\" when keeping reflogs\n @@ -5,10 +5,18 @@\n Don't redundantly run \"git reflog expire --all\" when gc.reflogExpire\n and gc.reflogExpireUnreachable are set to \"never\".\n \n - I'm being careful to not repeat the issue fixed in\n - 8ab5aa4bd8 (\"parseopt: handle malformed --expire arguments more\n - nicely\", 2018-04-21). We'll die early if the config variables are set\n - to invalid values.\n + An earlier change taught \"git reflog expire\" itself to exit early\n + under this scenario, so in some sense this isn't strictly\n + necessary. Reasons to also do it here:\n +\n + 1) Similar to 8ab5aa4bd8 (\"parseopt: handle malformed --expire\n + arguments more nicely\", 2018-04-21). We'll die early if the config\n + variables are set to invalid values. We run \"pack-refs\" before\n + \"reflog expire\", which can take a while, only to then die on an\n + invalid gc.reflogExpire{Unreachable,} configuration.\n +\n + 2) Not invoking the command at all means it won't show up in trace\n + output, which makes what's going on more obvious.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n4: c127265828 ! 6: 55dd203a04 reflog expire: don't assert the OID when locking refs\n @@ -2,35 +2,39 @@\n \n reflog expire: don't assert the OID when locking refs\n \n - The locking protocol for reflogs involves getting a *.lock file on the\n - loose ref[1] (even if the actual ref is packed). This isn't needed to\n - expire the reflog, and needlessly results promotes reference update\n - contention to hard errors in e.g. \"gc\".\n -\n During reflog expiry, the cmd_reflog_expire() function first iterates\n - over all known references, and then one-by-one acquires the lock for\n - each one to expire its reflog. By the time it gets around to\n - re-visiting the references some of the OIDs may have changed.\n + over all reflogs in logs/*, and then one-by-one acquires the lock for\n + each one to expire its reflog by getting a *.lock file on the\n + corresponding loose ref[1] (even if the actual ref is packed).\n +\n + This lock is needed, but what isn't needed is locking the loose ref as\n + a function of the OID we found from that first iteration. By the time\n + we get around to re-visiting the reference some of the OIDs may have\n + changed.\n \n - This has been the case ever since \"reflog expire\" was initially\n - implemented in 4264dc15e1 (\"git reflog expire\", 2006-12-19). As seen\n - in that simpler initial version of the code (and the same is the case\n - before this change) we subsequently use the OID to inform the expiry,\n - but never needed to use it to lock the reference associated with the\n - reflog.\n + Thus the verify_lock() function called by the lock_ref_oid_basic()\n + function being changed here would fail with e.g. \"ref '%s' is at %s\n + but expected %s\" if the repository was being updated concurrent to the\n + \"reflog expire\".\n \n - Thus the verify_lock() function would fail with e.g. \"ref '%s' is at\n - %s but expected %s\" if the repository was being updated concurrent to\n - the \"reflog expire\".\n + By not passing the OID to it we'll try to lock the reference\n + regardless of it last known OID. Locking as a function of the OID\n + would make \"reflog expire\" exit with a non-zero exit status under such\n + contention, which in turn meant that a \"gc\" command (which expires\n + reflogs before forking to the background) would encounter a hard\n + error.\n \n - This made \"reflog expire\" exit with a non-zero exit status, which in\n - turn meant that a \"gc\" command (which expires reflogs before forking\n - to the background) would encounter a hard error in such a scenario.\n + This behavior of considering the OID when locking has been here ever\n + since \"reflog expire\" was initially implemented in 4264dc15e1 (\"git\n + reflog expire\", 2006-12-19). As seen in that simpler initial version\n + of the code we subsequently use the OID to inform the expiry (and\n + still do), but never needed to use it to lock the reference associated\n + with the reflog.\n \n - If we instead lock the reference without considering what OID we last\n - saw it at, we won't encounter user-visible contention to the extent\n - that core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 (\"refs:\n - retry acquiring reference locks for 100ms\", 2017-08-21).\n + By locking the reference without considering what OID we last saw it\n + at, we won't encounter user-visible contention to the extent that\n + core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 (\"refs: retry\n + acquiring reference locks for 100ms\", 2017-08-21).\n \n Unfortunately this sort of probabilistic contention is hard to turn\n into a test. I've tested this by running the following three subshells\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371485","messageId":"20190314123439.4347-2-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 1/7] gc: remove redundant check for gc_auto_threshold","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:33Z","receivedAt":"2019-03-14T12:34:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Checking gc_auto_threshold in too_many_loose_objects() was added in\n17815501a8 (\"git-gc --auto: run \"repack -A -d -l\" as necessary.\",\n2007-09-17) when need_to_gc() itself was also reliant on\ngc_auto_pack_limit before its early return:\n\n gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0\n\nWhen that check was simplified to just checking \"gc_auto_threshold <=\n0\" in b14d255ba8 (\"builtin-gc.c: allow disabling all auto-gc'ing by\nassigning 0 to gc.auto\", 2008-03-19) this unreachable code should have\nbeen removed. We only call too_many_loose_objects() from within\nneed_to_gc() itself, which will return if this condition holds, and in\ncmd_gc() which will return before ever getting to \"auto_gc &&\ntoo_many_loose_objects()\" if \"auto_gc && !need_to_gc()\" is true\nearlier in the function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 020f725acc..8c2312681c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -157,9 +157,6 @@ static int too_many_loose_objects(void)\n \tint num_loose = 0;\n \tint needed = 0;\n \n-\tif (gc_auto_threshold <= 0)\n-\t\treturn 0;\n-\n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n \t\treturn 0;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371486","messageId":"20190314123439.4347-3-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 2/7] gc: convert to using the_hash_algo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:34Z","receivedAt":"2019-03-14T12:34:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There's been a lot of changing of the hardcoded \"40\" values to\nthe_hash_algo->hexsz, but we've so far missed this one where we\nhardcoded 38 for the loose object file length.\n\nThis is because a SHA-1 like abcde[...] gets turned into\nobjects/ab/cde[...]. There's no reason to suppose the same won't be\nthe case for SHA-256, and reading between the lines in\nhash-function-transition.txt the format is planned to be the same.\n\nIn the future we may want to further modify this code for the hash\nfunction transition. There's a potential pathological case here where\nwe'll only consider the loose objects for the currently active hash,\nbut objects for that hash will share a directory storage with the\nother hash.\n\nThus we could theoretically have 1k SHA-1 loose objects, and say 1\nmillion SHA-256 objects, and not notice because we're currently using\nSHA-1.\n\nSo assuming that \"gc\" eventually learns to pack up both SHA-1 and\nSHA-256 objects regardless of what the current the_hash_alg is perhaps\nthis check should be changed to consider all files in objects/17/\nmatching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and\nSHA-256).\n\nBut none of that is something we need to worry about now, and\nsupporting both 38 and 62 characters depending on \"the_hash_algo\"\nremoves another case of SHA-1 hardcoding.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 8c2312681c..733bd7bdf4 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -156,6 +156,7 @@ static int too_many_loose_objects(void)\n \tint auto_threshold;\n \tint num_loose = 0;\n \tint needed = 0;\n+\tconst unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n \n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n@@ -163,8 +164,8 @@ static int too_many_loose_objects(void)\n \n \tauto_threshold = DIV_ROUND_UP(gc_auto_threshold, 256);\n \twhile ((ent = readdir(dir)) != NULL) {\n-\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != 38 ||\n-\t\t ent->d_name[38] != '\\0')\n+\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != hexsz_loose ||\n+\t\t ent->d_name[hexsz_loose] != '\\0')\n \t\t\tcontinue;\n \t\tif (++num_loose > auto_threshold) {\n \t\t\tneeded = 1;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371487","messageId":"20190314123439.4347-4-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 3/7] gc: refactor a \"call me once\" pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:35Z","receivedAt":"2019-03-14T12:35:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change an idiom we're using to ensure that gc_before_repack() only\ndoes work once (see 62aad1849f (\"gc --auto: do not lock refs in the\nbackground\", 2014-05-25)) to be more obvious.\n\nNothing except this function cares about the \"pack_refs\" and\n\"prune_reflogs\" variables, so let's not leave the reader wondering if\nthey're being zero'd out for later use somewhere else.\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 733bd7bdf4..ae716a00d4 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -489,14 +489,20 @@ static int report_last_gc_error(void)\n \n static void gc_before_repack(void)\n {\n+\t/*\n+\t * We may be called twice, as both the pre- and\n+\t * post-daemonized phases will call us, but running these\n+\t * commands more than once is pointless and wasteful.\n+\t */\n+\tstatic int done = 0;\n+\tif (done++)\n+\t\treturn;\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, reflog.argv[0]);\n-\n-\tpack_refs = 0;\n-\tprune_reflogs = 0;\n }\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371488","messageId":"20190314123439.4347-5-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 4/7] reflog tests: make use of \"test_config\" idiom","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:36Z","receivedAt":"2019-03-14T12:35:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a couple of tests that weren't using the helper to use it. This\nmakes the trailing \"--unset\" unnecessary.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1410-reflog.sh | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex ae8a448e34..42f5ac9ed9 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -232,24 +232,21 @@ test_expect_success '--expire=never' '\n '\n \n test_expect_success 'gc.reflogexpire=never' '\n+\ttest_config gc.reflogexpire never &&\n+\ttest_config gc.reflogexpireunreachable never &&\n \n-\tgit config gc.reflogexpire never &&\n-\tgit config gc.reflogexpireunreachable never &&\n \tgit reflog expire --verbose --all &&\n \tgit reflog refs/heads/master >output &&\n \ttest_line_count = 4 output\n '\n \n test_expect_success 'gc.reflogexpire=false' '\n+\ttest_config gc.reflogexpire false &&\n+\ttest_config gc.reflogexpireunreachable false &&\n \n-\tgit config gc.reflogexpire false &&\n-\tgit config gc.reflogexpireunreachable false &&\n \tgit reflog expire --verbose --all &&\n \tgit reflog refs/heads/master >output &&\n-\ttest_line_count = 4 output &&\n-\n-\tgit config --unset gc.reflogexpire &&\n-\tgit config --unset gc.reflogexpireunreachable\n+\ttest_line_count = 4 output\n \n '\n \n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371489","messageId":"20190314123439.4347-6-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 5/7] reflog: exit early if there's no work to do","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:37Z","receivedAt":"2019-03-14T12:35:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\nand --stale-fix isn't in effect (covered by the first part of the \"if\"\nstatement being modified here) we can exit early without pointlessly\nlooping over all the reflogs.\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/reflog.c | 7 +++++++\n t/t1410-reflog.sh | 4 +++-\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 4d3430900d..d95c77ca0e 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -606,6 +606,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tputchar('\\n');\n+\t} else if (!cb.cmd.expire_total && !cb.cmd.expire_unreachable) {\n+\t\t/*\n+\t\t * If we're not expiring anything and not dropping\n+\t\t * stale entries, there's no point in even opening the\n+\t\t * reflogs, since we're guaranteed to do nothing.\n+\t\t */\n+\t\treturn 0;\n \t}\n \n \tif (do_all) {\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 42f5ac9ed9..b98827f082 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -235,7 +235,9 @@ test_expect_success 'gc.reflogexpire=never' '\n \ttest_config gc.reflogexpire never &&\n \ttest_config gc.reflogexpireunreachable never &&\n \n-\tgit reflog expire --verbose --all &&\n+\tgit reflog expire --verbose --all >output &&\n+\ttest_line_count = 0 output &&\n+\n \tgit reflog refs/heads/master >output &&\n \ttest_line_count = 4 output\n '\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371490","messageId":"20190314123439.4347-8-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 7/7] reflog expire: don't assert the OID when locking refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:39Z","receivedAt":"2019-03-14T12:35:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"During reflog expiry, the cmd_reflog_expire() function first iterates\nover all reflogs in logs/*, and then one-by-one acquires the lock for\neach one to expire its reflog by getting a *.lock file on the\ncorresponding loose ref[1] (even if the actual ref is packed).\n\nThis lock is needed, but what isn't needed is locking the loose ref as\na function of the OID we found from that first iteration. By the time\nwe get around to re-visiting the reference some of the OIDs may have\nchanged.\n\nThus the verify_lock() function called by the lock_ref_oid_basic()\nfunction being changed here would fail with e.g. \"ref '%s' is at %s\nbut expected %s\" if the repository was being updated concurrent to the\n\"reflog expire\".\n\nBy not passing the OID to it we'll try to lock the reference\nregardless of it last known OID. Locking as a function of the OID\nwould make \"reflog expire\" exit with a non-zero exit status under such\ncontention, which in turn meant that a \"gc\" command (which expires\nreflogs before forking to the background) would encounter a hard\nerror.\n\nThis behavior of considering the OID when locking has been here ever\nsince \"reflog expire\" was initially implemented in 4264dc15e1 (\"git\nreflog expire\", 2006-12-19). As seen in that simpler initial version\nof the code we subsequently use the OID to inform the expiry (and\nstill do), but never needed to use it to lock the reference associated\nwith the reflog.\n\nBy locking the reference without considering what OID we last saw it\nat, we won't encounter user-visible contention to the extent that\ncore.filesRefLockTimeout mitigates it. See 4ff0f01cb7 (\"refs: retry\nacquiring reference locks for 100ms\", 2017-08-21).\n\nUnfortunately this sort of probabilistic contention is hard to turn\ninto a test. I've tested this by running the following three subshells\nin concurrent terminals:\n\n (\n cd /tmp &&\n rm -rf git &&\n git init git &&\n cd git &&\n while true\n do\n head -c 10 /dev/urandom | hexdump >out &&\n git add out &&\n git commit -m\"out\"\n done\n )\n\n (\n cd /tmp &&\n rm -rf git-clone &&\n git clone file:///tmp/git git-clone &&\n cd git-clone &&\n while git pull\n do\n date\n done\n )\n\n (\n cd /tmp/git-clone &&\n while git reflog expire --all\n do\n date\n done\n )\n\nBefore this change the \"reflog expire\" would fail really quickly with\na \"but expected\" error. After this change both the \"pull\" and \"reflog\nexpire\" will run for a while, but eventually fail because I get\nunlucky with core.filesRefLockTimeout (the \"reflog expire\" is in a\nreally tight loop). That can be resolved by being more generous with\nhigher values of core.filesRefLockTimeout than the 100ms default.\n\n1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ef053f716c..4d4d226601 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n \t */\n-\tlock = lock_ref_oid_basic(refs, refname, oid,\n+\tlock = lock_ref_oid_basic(refs, refname, NULL /* NOT oid! */,\n \t\t\t\t NULL, NULL, REF_NO_DEREF,\n \t\t\t\t &type, &err);\n \tif (!lock) {\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371491","messageId":"20190314123439.4347-7-avarab@gmail.com","threadId":"50723","inReplyTo":"20190313235439.30439-1-avarab@gmail.com","subject":"[PATCH v2 6/7] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T12:34:38Z","receivedAt":"2019-03-14T12:35:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Don't redundantly run \"git reflog expire --all\" when gc.reflogExpire\nand gc.reflogExpireUnreachable are set to \"never\".\n\nAn earlier change taught \"git reflog expire\" itself to exit early\nunder this scenario, so in some sense this isn't strictly\nnecessary. Reasons to also do it here:\n\n 1) Similar to 8ab5aa4bd8 (\"parseopt: handle malformed --expire\n arguments more nicely\", 2018-04-21). We'll die early if the config\n variables are set to invalid values. We run \"pack-refs\" before\n \"reflog expire\", which can take a while, only to then die on an\n invalid gc.reflogExpire{Unreachable,} configuration.\n\n 2) Not invoking the command at all means it won't show up in trace\n output, which makes what's going on more obvious.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 17 +++++++++++++++++\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ae716a00d4..8943bcc300 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -116,6 +116,19 @@ static void process_log_file_on_signal(int signo)\n \traise(signo);\n }\n \n+static int gc_config_is_timestamp_never(const char *var)\n+{\n+\tconst char *value;\n+\ttimestamp_t expire;\n+\n+\tif (!git_config_get_value(var, &value) && value) {\n+\t\tif (parse_expiry_date(value, &expire))\n+\t\t\tdie(_(\"failed to parse '%s' value '%s'\"), var, value);\n+\t\treturn expire == 0;\n+\t}\n+\treturn 0;\n+}\n+\n static void gc_config(void)\n {\n \tconst char *value;\n@@ -127,6 +140,10 @@ static void gc_config(void)\n \t\t\tpack_refs = git_config_bool(\"gc.packrefs\", value);\n \t}\n \n+\tif (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n+\t gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n+\t\tprune_reflogs = 0;\n+\n \tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 4684d06552..7411bf7fec 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -120,6 +120,25 @@ test_expect_success 'gc --quiet' '\n \ttest_must_be_empty stderr\n '\n \n+test_expect_success 'gc.reflogExpire{Unreachable,}=never skips \"expire\" via \"gc\"' '\n+\ttest_config gc.reflogExpire never &&\n+\ttest_config gc.reflogExpireUnreachable never &&\n+\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\n+\t# Check that git-pack-refs is run as a sanity check (done via\n+\t# gc_before_repack()) but that git-expire is not.\n+\tgrep -E \"^trace: (built-in|exec|run_command): git pack-refs --\" trace.out &&\n+\t! grep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n+test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"expire\" via \"gc\"' '\n+\t>trace.out &&\n+\ttest_config gc.reflogExpire never &&\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9. Furthermore, the\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371537","messageId":"20190314192608.GC26250@sigill.intra.peff.net","threadId":"50723","inReplyTo":"xmqqmuly9hyg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/5] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T19:26:09Z","receivedAt":"2019-03-14T19:27:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 01:51:35PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Seeing \"--stale-fix\" again reminded me: that may be the \"oops, we can\n> > spend tons of CPU walking history\" bit that I mentioned in the other\n> > part of the thread. But I don't think git-gc would ever run with that\n> > option.\n> \n> The option was a purely transitional measure to recover from a bad\n> reflog state that could have been left by earlier versions of\n> \"prune\" and \"repack\" that did not pay attention to the reflog.\n> Perhaps we should plan to deprecate and remove it by now?\n\nTrue, though I have definitely used it over the years to clear out\nbroken reflog entries from corrupted repositories. In most cases we\nshould be able to do that much more simply these days, though. Since we\ntry to keep whole segments of history reachable from an otherwise\nunreachable object, you should in general be able to just prune entries\nfor which we are missing the actual object mentioned in the log.\n\nOf course when you are talking about corruption, all rules go out the\nwindow. So it's possible you'd still need --stale-fix to cover really\nbroken cases.\n\nI think these days I'd more often just delete the whole reflog in such a\ncase (once upon a time GitHub tried to use never-expiring reflogs as a\nsort of audit trail, but it had all sorts of complications; these days\nwe log to a separate file).\n\nSo I wouldn't be terribly sad to see --stale-fix go away.\n\nAll that said, I do think --expire-unreachable still has to traverse to\nfind out what's reachable. And I think it does it under lock. If I\ninstrument the tempfile code like the patch below and run:\n\n GIT_TRACE_TEMPFILE=1 git reflog expire --expire-unreachable=now HEAD\n\non a copy of my git.git repo, I get:\n\n 15:22:12.163725 tempfile.c:127 activating tempfile '/home/peff/compile/foo/.git/HEAD.lock'\n 15:22:12.163769 tempfile.c:127 activating tempfile '/home/peff/compile/foo/.git/logs/HEAD.lock'\n 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'\n 15:22:13.053416 tempfile.c:327 deleting tempfile '/home/peff/compile/foo/.git/HEAD.lock'\n\nWe hold HEAD.lock for almost an entire second.\n\n(Actually, it just occurred to me that \"strace -tt git ... 2>&1 | grep\nHEAD.lock\" would produce basically the same results, no patch needed).\n\n-Peff\n\n---\ndiff --git a/tempfile.c b/tempfile.c\nindex d43ad8c191..f7e999d3ca 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -53,6 +53,9 @@\n #include \"cache.h\"\n #include \"tempfile.h\"\n #include \"sigchain.h\"\n+#include \"trace.h\"\n+\n+static struct trace_key trace_tempfile = TRACE_KEY_INIT(TEMPFILE);\n \n static VOLATILE_LIST_HEAD(tempfile_list);\n \n@@ -119,6 +122,9 @@ static void activate_tempfile(struct tempfile *tempfile)\n \tvolatile_list_add(&tempfile->list, &tempfile_list);\n \ttempfile->owner = getpid();\n \ttempfile->active = 1;\n+\n+\ttrace_printf_key(&trace_tempfile, \"activating tempfile '%s'\",\n+\t\t\t tempfile->filename.buf);\n }\n \n static void deactivate_tempfile(struct tempfile *tempfile)\n@@ -302,6 +308,9 @@ int rename_tempfile(struct tempfile **tempfile_p, const char *path)\n \t\treturn -1;\n \t}\n \n+\ttrace_printf_key(&trace_tempfile, \"renaming tempfile '%s' to '%s'\",\n+\t\t\t tempfile->filename.buf, path);\n+\n \tdeactivate_tempfile(tempfile);\n \t*tempfile_p = NULL;\n \treturn 0;\n@@ -314,6 +323,9 @@ void delete_tempfile(struct tempfile **tempfile_p)\n \tif (!is_tempfile_active(tempfile))\n \t\treturn;\n \n+\ttrace_printf_key(&trace_tempfile, \"deleting tempfile '%s'\",\n+\t\t\t tempfile->filename.buf);\n+\n \tclose_tempfile_gently(tempfile);\n \tunlink_or_warn(tempfile->filename.buf);\n \tdeactivate_tempfile(tempfile);\n"},{"id":"371572","messageId":"CACsJy8CikZt_WF79V6y2JRtR0jAosTPPmDBXN8uMRTjm4FAV-A@mail.gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/7] gc: convert to using the_hash_algo","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-15T09:51:06Z","receivedAt":"2019-03-15T09:51:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 14, 2019 at 7:34 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> There's been a lot of changing of the hardcoded \"40\" values to\n> the_hash_algo->hexsz, but we've so far missed this one where we\n> hardcoded 38 for the loose object file length.\n\nWow. Good catch.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 8c2312681c..733bd7bdf4 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -156,6 +156,7 @@ static int too_many_loose_objects(void)\n> int auto_threshold;\n> int num_loose = 0;\n> int needed = 0;\n> + const unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n\nSince you're removing hard coded numbers. Another option is a\ncombination of strlen(basename()) and\nloose_object_path(the_repository, , null_oid). They should also give\nthe same 38. Then if loose object format is updated to use 3+ chars\nfor the directory part, this code will not need update anymore.\n\nThe downside of course is a lot more code. Or you can just introduce a\nnew function to return this \"hexsz - 2\", keep that function close to\nfill_loose_path() with a comment there that the two functions should\nbe aligned.\n\n>\n> dir = opendir(git_path(\"objects/17\"));\n> if (!dir)\n-- \nDuy\n"},{"id":"371573","messageId":"CACsJy8C-zyh6Vea1hqciDHhKk+UvS5qFb1F2SfU15yWniy53Hw@mail.gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-6-avarab@gmail.com","subject":"Re: [PATCH v2 5/7] reflog: exit early if there's no work to do","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-15T10:01:39Z","receivedAt":"2019-03-15T10:02:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\n> and --stale-fix isn't in effect (covered by the first part of the \"if\"\n> statement being modified here) we can exit early without pointlessly\n> looping over all the reflogs.\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/reflog.c | 7 +++++++\n> t/t1410-reflog.sh | 4 +++-\n> 2 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 4d3430900d..d95c77ca0e 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -606,6 +606,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n> if (flags & EXPIRE_REFLOGS_VERBOSE)\n> putchar('\\n');\n> + } else if (!cb.cmd.expire_total && !cb.cmd.expire_unreachable) {\n> + /*\n> + * If we're not expiring anything and not dropping\n> + * stale entries, there's no point in even opening the\n> + * reflogs, since we're guaranteed to do nothing.\n> + */\n\nI'm checking should_expire_reflog_ent(). With both of these being\nzero, we skip most of the \"return 1;\" except the last one\ncb->cmd.recno, added in 552cecc214 (Teach \"git reflog\" a subcommand to\ndelete single entries, 2007-10-17). Will this shortcut affect that use\ncase (I haven't spent time understanding that commit yet, gotta run\nsoon)?\n\n\n> + return 0;\n> }\n-- \nDuy\n"},{"id":"371574","messageId":"CACsJy8B=76dMKhvdzOyyOMK5o-s8t+77ZE-hBmezhxLdv7E9dQ@mail.gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-7-avarab@gmail.com","subject":"Re: [PATCH v2 6/7] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-15T10:05:43Z","receivedAt":"2019-03-15T10:06:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n wrote:\n> @@ -127,6 +140,10 @@ static void gc_config(void)\n> pack_refs = git_config_bool(\"gc.packrefs\", value);\n> }\n>\n> + if (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n> + gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n\nNit. configset api normalizes the key internally, so we can safely\nwrite gc.reflogExpireUnreachable here, which is a bit easier to read.\n-- \nDuy\n"},{"id":"371575","messageId":"877ed0bfl9.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":"CACsJy8B=76dMKhvdzOyyOMK5o-s8t+77ZE-hBmezhxLdv7E9dQ@mail.gmail.com","subject":"Re: [PATCH v2 6/7] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T10:24:18Z","receivedAt":"2019-03-15T10:24:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 15 2019, Duy Nguyen wrote:\n\n> On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>> @@ -127,6 +140,10 @@ static void gc_config(void)\n>> pack_refs = git_config_bool(\"gc.packrefs\", value);\n>> }\n>>\n>> + if (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n>> + gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n>\n> Nit. configset api normalizes the key internally, so we can safely\n> write gc.reflogExpireUnreachable here, which is a bit easier to read.\n\nI didn't know that, do we want to do that?\n\nI'd think that as a matter of coding style always sticking to lower case\nin the C code made more sense, because e.g. you might #define a config\nkey, and then use it both in git_config_*() as well as via strcmp() in\nsome callback. Mixing the two would lead to confusion and possible bugs\nas we'd refactor the former of those patterns to the latter.\n"},{"id":"371576","messageId":"CACsJy8DqABFXAVLObkRZCVCu_uozOVwp1P5bQeU5PCiV8cimwA@mail.gmail.com","threadId":"50723","inReplyTo":"877ed0bfl9.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 6/7] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-15T10:32:21Z","receivedAt":"2019-03-15T10:32:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 15, 2019 at 5:24 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Fri, Mar 15 2019, Duy Nguyen wrote:\n>\n> > On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n> > wrote:\n> >> @@ -127,6 +140,10 @@ static void gc_config(void)\n> >> pack_refs = git_config_bool(\"gc.packrefs\", value);\n> >> }\n> >>\n> >> + if (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n> >> + gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n> >\n> > Nit. configset api normalizes the key internally, so we can safely\n> > write gc.reflogExpireUnreachable here, which is a bit easier to read.\n>\n> I didn't know that, do we want to do that?\n>\n> I'd think that as a matter of coding style always sticking to lower case\n> in the C code made more sense, because e.g. you might #define a config\n> key, and then use it both in git_config_*() as well as via strcmp() in\n> some callback. Mixing the two would lead to confusion and possible bugs\n> as we'd refactor the former of those patterns to the latter.\n\nI did a quick git grep on \"git_config_get_\". The majority is still in\nlower case. And given that this feature is not documented anywhere (I\nhad a quick look at config.h and api-config.txt and ended up checking\nthe code before the previous mail) yeah it's probably just safer to go\nwith lowercase. At least until the old callback style is gone.\n-- \nDuy\n"},{"id":"371578","messageId":"874l84ber7.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":"CACsJy8CikZt_WF79V6y2JRtR0jAosTPPmDBXN8uMRTjm4FAV-A@mail.gmail.com","subject":"Re: [PATCH v2 2/7] gc: convert to using the_hash_algo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T10:42:20Z","receivedAt":"2019-03-15T10:42:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 15 2019, Duy Nguyen wrote:\n\n> On Thu, Mar 14, 2019 at 7:34 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> There's been a lot of changing of the hardcoded \"40\" values to\n>> the_hash_algo->hexsz, but we've so far missed this one where we\n>> hardcoded 38 for the loose object file length.\n>\n> Wow. Good catch.\n>\n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index 8c2312681c..733bd7bdf4 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -156,6 +156,7 @@ static int too_many_loose_objects(void)\n>> int auto_threshold;\n>> int num_loose = 0;\n>> int needed = 0;\n>> + const unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n>\n> Since you're removing hard coded numbers. Another option is a\n> combination of strlen(basename()) and\n> loose_object_path(the_repository, , null_oid). They should also give\n> the same 38. Then if loose object format is updated to use 3+ chars\n> for the directory part, this code will not need update anymore.\n>\n> The downside of course is a lot more code. Or you can just introduce a\n> new function to return this \"hexsz - 2\", keep that function close to\n> fill_loose_path() with a comment there that the two functions should\n> be aligned.\n\nI think it's better to just keep hardcoding \"2\". We're very unlikely to\never change objects/?? in favor of e.g. objects/???, and if we were that\nwould be a huge \"hash function transition\" of its own.\n\n>>\n>> dir = opendir(git_path(\"objects/17\"));\n>> if (!dir)\n"},{"id":"371579","messageId":"CACsJy8B_rOBB5SvqnQs9hkui+3txGVPcz+H62hWE7oPGb789bg@mail.gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-8-avarab@gmail.com","subject":"Re: [PATCH v2 7/7] reflog expire: don't assert the OID when locking refs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-15T11:10:31Z","receivedAt":"2019-03-15T11:11:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> During reflog expiry, the cmd_reflog_expire() function first iterates\n> over all reflogs in logs/*, and then one-by-one acquires the lock for\n> each one to expire its reflog by getting a *.lock file on the\n> corresponding loose ref[1] (even if the actual ref is packed).\n>\n> This lock is needed, but what isn't needed is locking the loose ref as\n> a function of the OID we found from that first iteration. By the time\n> we get around to re-visiting the reference some of the OIDs may have\n> changed.\n>\n> Thus the verify_lock() function called by the lock_ref_oid_basic()\n> function being changed here would fail with e.g. \"ref '%s' is at %s\n> but expected %s\" if the repository was being updated concurrent to the\n> \"reflog expire\".\n>\n> By not passing the OID to it we'll try to lock the reference\n> regardless of it last known OID. Locking as a function of the OID\n> would make \"reflog expire\" exit with a non-zero exit status under such\n> contention, which in turn meant that a \"gc\" command (which expires\n> reflogs before forking to the background) would encounter a hard\n> error.\n\nPassing NULL oid has another side(?) effect, which I don't know if it\nmatters at all. Before, the mustexist flag in lock_ref_oid_basic() is\ntrue. Now it's false. This affects refs_resolve_ref_unsafe() calls in\nthere. But that's where I'm stuck.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index ef053f716c..4d4d226601 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n> * reference itself, plus we might need to update the\n> * reference if --updateref was specified:\n> */\n> - lock = lock_ref_oid_basic(refs, refname, oid,\n> + lock = lock_ref_oid_basic(refs, refname, NULL /* NOT oid! */,\n\nMaybe mention this not oid in the comment block above. Or just drop\nit. Reading this comment without the commit message does not really\nhelp answer \"why not oid?\". Or perhaps /* don't verify oid */\n\n> NULL, NULL, REF_NO_DEREF,\n> &type, &err);\n> if (!lock) {\n> --\n-- \nDuy\n"},{"id":"371598","messageId":"nycvar.QRO.7.76.6.1903151649030.41@tvgsbejvaqbjf.bet","threadId":"50723","inReplyTo":"874l84ber7.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/7] gc: convert to using the_hash_algo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-15T15:49:40Z","receivedAt":"2019-03-15T15:50:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Fri, 15 Mar 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Fri, Mar 15 2019, Duy Nguyen wrote:\n> \n> > On Thu, Mar 14, 2019 at 7:34 PM Ævar Arnfjörð Bjarmason\n> > wrote:\n> >>\n> >> There's been a lot of changing of the hardcoded \"40\" values to\n> >> the_hash_algo->hexsz, but we've so far missed this one where we\n> >> hardcoded 38 for the loose object file length.\n> >\n> > Wow. Good catch.\n> >\n> >> diff --git a/builtin/gc.c b/builtin/gc.c\n> >> index 8c2312681c..733bd7bdf4 100644\n> >> --- a/builtin/gc.c\n> >> +++ b/builtin/gc.c\n> >> @@ -156,6 +156,7 @@ static int too_many_loose_objects(void)\n> >> int auto_threshold;\n> >> int num_loose = 0;\n> >> int needed = 0;\n> >> + const unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n> >\n> > Since you're removing hard coded numbers. Another option is a\n> > combination of strlen(basename()) and\n> > loose_object_path(the_repository, , null_oid). They should also give\n> > the same 38. Then if loose object format is updated to use 3+ chars\n> > for the directory part, this code will not need update anymore.\n> >\n> > The downside of course is a lot more code. Or you can just introduce a\n> > new function to return this \"hexsz - 2\", keep that function close to\n> > fill_loose_path() with a comment there that the two functions should\n> > be aligned.\n> \n> I think it's better to just keep hardcoding \"2\". We're very unlikely to\n> ever change objects/?? in favor of e.g. objects/???, and if we were that\n> would be a huge \"hash function transition\" of its own.\n\nOf course, since it is *such* a common use case, we could also simply add\nanother field in `the_hash_algo`. That would also save on repeated magic\nconstants.\n\nCiao,\nDscho"},{"id":"371599","messageId":"nycvar.QRO.7.76.6.1903151650500.41@tvgsbejvaqbjf.bet","threadId":"50723","inReplyTo":"877ed0bfl9.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 6/7] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-15T15:51:17Z","receivedAt":"2019-03-15T15:51:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Fri, 15 Mar 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Fri, Mar 15 2019, Duy Nguyen wrote:\n> \n> > On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n> > wrote:\n> >> @@ -127,6 +140,10 @@ static void gc_config(void)\n> >> pack_refs = git_config_bool(\"gc.packrefs\", value);\n> >> }\n> >>\n> >> + if (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n> >> + gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n> >\n> > Nit. configset api normalizes the key internally, so we can safely\n> > write gc.reflogExpireUnreachable here, which is a bit easier to read.\n> \n> I didn't know that, do we want to do that?\n> \n> I'd think that as a matter of coding style always sticking to lower case\n> in the C code made more sense, because e.g. you might #define a config\n> key, and then use it both in git_config_*() as well as via strcmp() in\n> some callback. Mixing the two would lead to confusion and possible bugs\n> as we'd refactor the former of those patterns to the latter.\n\nIt also saves on developer and reviewer time to stick to lower case.\n\nCiao,\nDscho"},{"id":"371600","messageId":"871s38b0fd.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":"CACsJy8C-zyh6Vea1hqciDHhKk+UvS5qFb1F2SfU15yWniy53Hw@mail.gmail.com","subject":"Re: [PATCH v2 5/7] reflog: exit early if there's no work to do","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:51:50Z","receivedAt":"2019-03-15T15:51:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 15 2019, Duy Nguyen wrote:\n\n> On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\n>> and --stale-fix isn't in effect (covered by the first part of the \"if\"\n>> statement being modified here) we can exit early without pointlessly\n>> looping over all the reflogs.\n>>\n>> Signed-off-by: Jeff King \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/reflog.c | 7 +++++++\n>> t/t1410-reflog.sh | 4 +++-\n>> 2 files changed, 10 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/reflog.c b/builtin/reflog.c\n>> index 4d3430900d..d95c77ca0e 100644\n>> --- a/builtin/reflog.c\n>> +++ b/builtin/reflog.c\n>> @@ -606,6 +606,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n>> mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n>> if (flags & EXPIRE_REFLOGS_VERBOSE)\n>> putchar('\\n');\n>> + } else if (!cb.cmd.expire_total && !cb.cmd.expire_unreachable) {\n>> + /*\n>> + * If we're not expiring anything and not dropping\n>> + * stale entries, there's no point in even opening the\n>> + * reflogs, since we're guaranteed to do nothing.\n>> + */\n>\n> I'm checking should_expire_reflog_ent(). With both of these being\n> zero, we skip most of the \"return 1;\" except the last one\n> cb->cmd.recno, added in 552cecc214 (Teach \"git reflog\" a subcommand to\n> delete single entries, 2007-10-17). Will this shortcut affect that use\n> case (I haven't spent time understanding that commit yet, gotta run\n> soon)?\n\nThere was a bug related to this. Fixed in v3.\n\n>\n>> + return 0;\n>> }\n"},{"id":"371601","messageId":"87zhpw9ltk.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":"CACsJy8B_rOBB5SvqnQs9hkui+3txGVPcz+H62hWE7oPGb789bg@mail.gmail.com","subject":"Re: [PATCH v2 7/7] reflog expire: don't assert the OID when locking refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:52:39Z","receivedAt":"2019-03-15T15:52:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 15 2019, Duy Nguyen wrote:\n\n> On Thu, Mar 14, 2019 at 7:35 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> During reflog expiry, the cmd_reflog_expire() function first iterates\n>> over all reflogs in logs/*, and then one-by-one acquires the lock for\n>> each one to expire its reflog by getting a *.lock file on the\n>> corresponding loose ref[1] (even if the actual ref is packed).\n>>\n>> This lock is needed, but what isn't needed is locking the loose ref as\n>> a function of the OID we found from that first iteration. By the time\n>> we get around to re-visiting the reference some of the OIDs may have\n>> changed.\n>>\n>> Thus the verify_lock() function called by the lock_ref_oid_basic()\n>> function being changed here would fail with e.g. \"ref '%s' is at %s\n>> but expected %s\" if the repository was being updated concurrent to the\n>> \"reflog expire\".\n>>\n>> By not passing the OID to it we'll try to lock the reference\n>> regardless of it last known OID. Locking as a function of the OID\n>> would make \"reflog expire\" exit with a non-zero exit status under such\n>> contention, which in turn meant that a \"gc\" command (which expires\n>> reflogs before forking to the background) would encounter a hard\n>> error.\n>\n> Passing NULL oid has another side(?) effect, which I don't know if it\n> matters at all. Before, the mustexist flag in lock_ref_oid_basic() is\n> true. Now it's false. This affects refs_resolve_ref_unsafe() calls in\n> there. But that's where I'm stuck.\n\nThis case was tricky, I ended up doing the same thing in v3, but now\nvery carefully explain the rationale since none of it is obvious.\n\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index ef053f716c..4d4d226601 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n>> * reference itself, plus we might need to update the\n>> * reference if --updateref was specified:\n>> */\n>> - lock = lock_ref_oid_basic(refs, refname, oid,\n>> + lock = lock_ref_oid_basic(refs, refname, NULL /* NOT oid! */,\n>\n> Maybe mention this not oid in the comment block above. Or just drop\n> it. Reading this comment without the commit message does not really\n> help answer \"why not oid?\". Or perhaps /* don't verify oid */\n\nAlso fixed.\n\n>> NULL, NULL, REF_NO_DEREF,\n>> &type, &err);\n>> if (!lock) {\n>> --\n"},{"id":"371602","messageId":"20190315155959.12390-1-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 0/8] gc: minor code cleanup + contention fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:51Z","receivedAt":"2019-03-15T16:00:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Changes since v2:\n\n * The \"let's detect that we don't need work in reflog expire\" patch\n is gone, as noted in a new set of tests/commit messages that was\n tricker than expected, and not worth it.\n\n * Minor rewording fixes to commit messages.\n\n * Further paraphrased rationale in commit messages from replies to v2\n feedback (thanks all!)\n\n * The \"no OID\" and \"no mustexist\" case was confusingly conflated, now\n we still do that in 8/8, but with a commit message & commit\n explaining why and how that works.\n\nÆvar Arnfjörð Bjarmason (8):\n gc: remove redundant check for gc_auto_threshold\n gc: convert to using the_hash_algo\n gc: refactor a \"call me once\" pattern\n reflog tests: make use of \"test_config\" idiom\n reflog tests: test for the \"points nowhere\" warning\n reflog tests: assert lack of early exit with expiry=\"never\"\n gc: handle & check gc.reflogExpire config\n reflog expire: don't assert the OID when locking refs\n\n builtin/gc.c | 37 +++++++++++++++++++++++++++++--------\n refs/files-backend.c | 15 ++++++++++++++-\n t/t1410-reflog.sh | 25 +++++++++++++++++--------\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 4 files changed, 79 insertions(+), 17 deletions(-)\n\nRange-diff:\n1: f11699d8e77 = 1: 81694c82130 gc: remove redundant check for gc_auto_threshold\n2: e18433f9c6b ! 2: 4bdcf1d0be2 gc: convert to using the_hash_algo\n @@ -17,20 +17,33 @@\n but objects for that hash will share a directory storage with the\n other hash.\n \n - Thus we could theoretically have 1k SHA-1 loose objects, and say 1\n - million SHA-256 objects, and not notice because we're currently using\n - SHA-1.\n + Thus we could theoretically have e.g. 1k SHA-1 loose objects, and 1\n + million SHA-256 objects. Then not notice that we need to pack them\n + because we're currently using SHA-1, even though our FS may be\n + straining under the stress of such humongous directories.\n \n So assuming that \"gc\" eventually learns to pack up both SHA-1 and\n - SHA-256 objects regardless of what the current the_hash_alg is perhaps\n - this check should be changed to consider all files in objects/17/\n - matching [0-9a-f] 38 or 62 characters in length (i.e. both SHA-1 and\n - SHA-256).\n + SHA-256 objects regardless of what the current the_hash_algo is,\n + perhaps this check should be changed to consider all files in\n + objects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both\n + SHA-1 and SHA-256).\n \n But none of that is something we need to worry about now, and\n supporting both 38 and 62 characters depending on \"the_hash_algo\"\n removes another case of SHA-1 hardcoding.\n \n + As noted in [1] I'm making no effort to somehow remove the hardcoding\n + for \"2\" as in \"use the first two hexdigits for the directory\n + name\". There's no indication that that'll ever change, and somehow\n + generalizing it here would be a drop in the ocean, so there's no point\n + in doing that. It also couldn't be done without coming up with some\n + generalized version of the magical \"objects/17\" directory. See [2] for\n + a discussion of that directory.\n +\n + 1. https://public-inbox.org/git/874l84ber7.fsf@evledraar.gmail.com/\n +\n + 2. https://public-inbox.org/git/87k1mta9x5.fsf@evledraar.gmail.com/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n diff --git a/builtin/gc.c b/builtin/gc.c\n3: 54e4bce91c3 = 3: 9444a1233af gc: refactor a \"call me once\" pattern\n4: 82f87db1348 = 4: 60a06ae6185 reflog tests: make use of \"test_config\" idiom\n-: ----------- > 5: 52838fdc449 reflog tests: test for the \"points nowhere\" warning\n5: c79608dbbb3 ! 6: 6063429f108 reflog: exit early if there's no work to do\n @@ -1,32 +1,25 @@\n Author: Ævar Arnfjörð Bjarmason \n \n - reflog: exit early if there's no work to do\n + reflog tests: assert lack of early exit with expiry=\"never\"\n \n When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\n and --stale-fix isn't in effect (covered by the first part of the \"if\"\n - statement being modified here) we can exit early without pointlessly\n - looping over all the reflogs.\n + statement being modified here) we *could* exit early without\n + pointlessly looping over all the reflogs.\n \n - Signed-off-by: Jeff King \n - Signed-off-by: Ævar Arnfjörð Bjarmason \n + However, as an earlier change to add a test for the \"points nowhere\"\n + warning shows even in such a mode we might want to print out a\n + warning.\n \n - diff --git a/builtin/reflog.c b/builtin/reflog.c\n - --- a/builtin/reflog.c\n - +++ b/builtin/reflog.c\n -@@\n - \t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n - \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n - \t\t\tputchar('\\n');\n -+\t} else if (!cb.cmd.expire_total && !cb.cmd.expire_unreachable) {\n -+\t\t/*\n -+\t\t * If we're not expiring anything and not dropping\n -+\t\t * stale entries, there's no point in even opening the\n -+\t\t * reflogs, since we're guaranteed to do nothing.\n -+\t\t */\n -+\t\treturn 0;\n - \t}\n - \n - \tif (do_all) {\n + So while it's conceivable to implement this, I don't think it's worth\n + it. It's going to be too easy to inadvertently add some flag that'll\n + make the expiry happen anyway, and even with \"never\" we'd like to see\n + all the lines we're going to keep.\n +\n + So let's assert that we're going to loop over all the references even\n + when this configuration is in effect.\n +\n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n --- a/t/t1410-reflog.sh\n @@ -37,7 +30,7 @@\n \n -\tgit reflog expire --verbose --all &&\n +\tgit reflog expire --verbose --all >output &&\n -+\ttest_line_count = 0 output &&\n ++\ttest_line_count = 9 output &&\n +\n \tgit reflog refs/heads/master >output &&\n \ttest_line_count = 4 output\n6: c47dedab58d ! 7: 6693d1d84da gc: don't run \"reflog expire\" when keeping reflogs\n @@ -1,22 +1,38 @@\n Author: Ævar Arnfjörð Bjarmason \n \n - gc: don't run \"reflog expire\" when keeping reflogs\n + gc: handle & check gc.reflogExpire config\n \n Don't redundantly run \"git reflog expire --all\" when gc.reflogExpire\n - and gc.reflogExpireUnreachable are set to \"never\".\n + and gc.reflogExpireUnreachable are set to \"never\", and die immediately\n + if those configuration valuer are bad.\n \n - An earlier change taught \"git reflog expire\" itself to exit early\n - under this scenario, so in some sense this isn't strictly\n - necessary. Reasons to also do it here:\n + As an earlier \"assert lack of early exit\" change to the tests for \"git\n + reflog expire\" shows, an early check of gc.reflogExpire{Unreachable,}\n + isn't wanted in general for \"git reflog expire\", but it makes sense\n + for \"gc\" because:\n \n - 1) Similar to 8ab5aa4bd8 (\"parseopt: handle malformed --expire\n - arguments more nicely\", 2018-04-21). We'll die early if the config\n - variables are set to invalid values. We run \"pack-refs\" before\n - \"reflog expire\", which can take a while, only to then die on an\n - invalid gc.reflogExpire{Unreachable,} configuration.\n + 1) Similarly to 8ab5aa4bd8 (\"parseopt: handle malformed --expire\n + arguments more nicely\", 2018-04-21) we'll now die early if the\n + config variables are set to invalid values.\n +\n + We run \"pack-refs\" before \"reflog expire\", which can take a while,\n + only to then die on an invalid gc.reflogExpire{Unreachable,}\n + configuration.\n \n 2) Not invoking the command at all means it won't show up in trace\n - output, which makes what's going on more obvious.\n + output, which makes what's going on more obvious when the two are\n + set to \"never\".\n +\n + 3) As a later change documents we lock the refs when looping over the\n + refs to expire, even in cases where we end up doing nothing due to\n + this config.\n +\n + For the reasons noted in the earlier \"assert lack of early exit\"\n + change I don't think it's worth it to bend over backwards in \"git\n + reflog expire\" itself to carefully detect if we'll really do\n + nothing given the combination of all its possible options and skip\n + that locking, but that's easy to detect here in \"gc\" where we'll\n + only run \"reflog expire\" in a relatively simple mode.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n7: 55dd203a042 ! 8: e0814569aba reflog expire: don't assert the OID when locking refs\n @@ -79,6 +79,25 @@\n really tight loop). That can be resolved by being more generous with\n higher values of core.filesRefLockTimeout than the 100ms default.\n \n + As noted in the commentary being added here we also need to handle the\n + case of references being racily deleted, that can be tested by adding\n + this to the above:\n +\n + (\n + cd /tmp/git-clone &&\n + while git branch topic master && git branch -D topic\n + do\n + date\n + done\n + )\n +\n + We could change lock_ref_oid_basic() to always pass down\n + RESOLVE_REF_READING to refs_resolve_ref_unsafe() and then\n + files_reflog_expire() to detect the \"is it deleted?\" state. But let's\n + not bother, in the event of such a race we're going to redundantly\n + create a lock on the deleted reference, and shortly afterwards handle\n + that case and others with the refs_reflog_exists() check.\n +\n 1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n @@ -87,11 +106,32 @@\n --- a/refs/files-backend.c\n +++ b/refs/files-backend.c\n @@\n + \t * The reflog file is locked by holding the lock on the\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n ++\t *\n ++\t * We don't pass down the oid here because we'd like to be\n ++\t * tolerant to the OID of the ref having changed, and to\n ++\t * gracefully handle the case where it's been deleted (see oid\n ++\t * -> mustexist -> RESOLVE_REF_READING in\n ++\t * lock_ref_oid_basic()) ...\n \t */\n -\tlock = lock_ref_oid_basic(refs, refname, oid,\n -+\tlock = lock_ref_oid_basic(refs, refname, NULL /* NOT oid! */,\n ++\tlock = lock_ref_oid_basic(refs, refname, NULL,\n \t\t\t\t NULL, NULL, REF_NO_DEREF,\n \t\t\t\t &type, &err);\n \tif (!lock) {\n +@@\n + \t\tstrbuf_release(&err);\n + \t\treturn -1;\n + \t}\n ++\t/*\n ++\t * When refs are deleted their reflog is deleted before the\n ++\t * loose ref is deleted. This catches that case, i.e. when\n ++\t * racing against a ref deletion lock_ref_oid_basic() will\n ++\t * have acquired a lock on the now-deleted ref, but here's\n ++\t * where we find out it has no reflog anymore.\n ++\t */\n + \tif (!refs_reflog_exists(ref_store, refname)) {\n + \t\tunlock_ref(lock);\n + \t\treturn 0;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371603","messageId":"20190315155959.12390-2-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 1/8] gc: remove redundant check for gc_auto_threshold","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:52Z","receivedAt":"2019-03-15T16:00:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Checking gc_auto_threshold in too_many_loose_objects() was added in\n17815501a8 (\"git-gc --auto: run \"repack -A -d -l\" as necessary.\",\n2007-09-17) when need_to_gc() itself was also reliant on\ngc_auto_pack_limit before its early return:\n\n gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0\n\nWhen that check was simplified to just checking \"gc_auto_threshold <=\n0\" in b14d255ba8 (\"builtin-gc.c: allow disabling all auto-gc'ing by\nassigning 0 to gc.auto\", 2008-03-19) this unreachable code should have\nbeen removed. We only call too_many_loose_objects() from within\nneed_to_gc() itself, which will return if this condition holds, and in\ncmd_gc() which will return before ever getting to \"auto_gc &&\ntoo_many_loose_objects()\" if \"auto_gc && !need_to_gc()\" is true\nearlier in the function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 020f725acc4..8c2312681ce 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -157,9 +157,6 @@ static int too_many_loose_objects(void)\n \tint num_loose = 0;\n \tint needed = 0;\n \n-\tif (gc_auto_threshold <= 0)\n-\t\treturn 0;\n-\n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n \t\treturn 0;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371604","messageId":"20190315155959.12390-3-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 2/8] gc: convert to using the_hash_algo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:53Z","receivedAt":"2019-03-15T16:00:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There's been a lot of changing of the hardcoded \"40\" values to\nthe_hash_algo->hexsz, but we've so far missed this one where we\nhardcoded 38 for the loose object file length.\n\nThis is because a SHA-1 like abcde[...] gets turned into\nobjects/ab/cde[...]. There's no reason to suppose the same won't be\nthe case for SHA-256, and reading between the lines in\nhash-function-transition.txt the format is planned to be the same.\n\nIn the future we may want to further modify this code for the hash\nfunction transition. There's a potential pathological case here where\nwe'll only consider the loose objects for the currently active hash,\nbut objects for that hash will share a directory storage with the\nother hash.\n\nThus we could theoretically have e.g. 1k SHA-1 loose objects, and 1\nmillion SHA-256 objects. Then not notice that we need to pack them\nbecause we're currently using SHA-1, even though our FS may be\nstraining under the stress of such humongous directories.\n\nSo assuming that \"gc\" eventually learns to pack up both SHA-1 and\nSHA-256 objects regardless of what the current the_hash_algo is,\nperhaps this check should be changed to consider all files in\nobjects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both\nSHA-1 and SHA-256).\n\nBut none of that is something we need to worry about now, and\nsupporting both 38 and 62 characters depending on \"the_hash_algo\"\nremoves another case of SHA-1 hardcoding.\n\nAs noted in [1] I'm making no effort to somehow remove the hardcoding\nfor \"2\" as in \"use the first two hexdigits for the directory\nname\". There's no indication that that'll ever change, and somehow\ngeneralizing it here would be a drop in the ocean, so there's no point\nin doing that. It also couldn't be done without coming up with some\ngeneralized version of the magical \"objects/17\" directory. See [2] for\na discussion of that directory.\n\n1. https://public-inbox.org/git/874l84ber7.fsf@evledraar.gmail.com/\n\n2. https://public-inbox.org/git/87k1mta9x5.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 8c2312681ce..733bd7bdf46 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -156,6 +156,7 @@ static int too_many_loose_objects(void)\n \tint auto_threshold;\n \tint num_loose = 0;\n \tint needed = 0;\n+\tconst unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n \n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n@@ -163,8 +164,8 @@ static int too_many_loose_objects(void)\n \n \tauto_threshold = DIV_ROUND_UP(gc_auto_threshold, 256);\n \twhile ((ent = readdir(dir)) != NULL) {\n-\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != 38 ||\n-\t\t ent->d_name[38] != '\\0')\n+\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != hexsz_loose ||\n+\t\t ent->d_name[hexsz_loose] != '\\0')\n \t\t\tcontinue;\n \t\tif (++num_loose > auto_threshold) {\n \t\t\tneeded = 1;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371606","messageId":"20190315155959.12390-4-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 3/8] gc: refactor a \"call me once\" pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:54Z","receivedAt":"2019-03-15T16:00:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change an idiom we're using to ensure that gc_before_repack() only\ndoes work once (see 62aad1849f (\"gc --auto: do not lock refs in the\nbackground\", 2014-05-25)) to be more obvious.\n\nNothing except this function cares about the \"pack_refs\" and\n\"prune_reflogs\" variables, so let's not leave the reader wondering if\nthey're being zero'd out for later use somewhere else.\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 733bd7bdf46..ae716a00d4a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -489,14 +489,20 @@ static int report_last_gc_error(void)\n \n static void gc_before_repack(void)\n {\n+\t/*\n+\t * We may be called twice, as both the pre- and\n+\t * post-daemonized phases will call us, but running these\n+\t * commands more than once is pointless and wasteful.\n+\t */\n+\tstatic int done = 0;\n+\tif (done++)\n+\t\treturn;\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, reflog.argv[0]);\n-\n-\tpack_refs = 0;\n-\tprune_reflogs = 0;\n }\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371605","messageId":"20190315155959.12390-5-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 4/8] reflog tests: make use of \"test_config\" idiom","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:55Z","receivedAt":"2019-03-15T16:00:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a couple of tests that weren't using the helper to use it. This\nmakes the trailing \"--unset\" unnecessary.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1410-reflog.sh | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex ae8a448e343..42f5ac9ed95 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -232,24 +232,21 @@ test_expect_success '--expire=never' '\n '\n \n test_expect_success 'gc.reflogexpire=never' '\n+\ttest_config gc.reflogexpire never &&\n+\ttest_config gc.reflogexpireunreachable never &&\n \n-\tgit config gc.reflogexpire never &&\n-\tgit config gc.reflogexpireunreachable never &&\n \tgit reflog expire --verbose --all &&\n \tgit reflog refs/heads/master >output &&\n \ttest_line_count = 4 output\n '\n \n test_expect_success 'gc.reflogexpire=false' '\n+\ttest_config gc.reflogexpire false &&\n+\ttest_config gc.reflogexpireunreachable false &&\n \n-\tgit config gc.reflogexpire false &&\n-\tgit config gc.reflogexpireunreachable false &&\n \tgit reflog expire --verbose --all &&\n \tgit reflog refs/heads/master >output &&\n-\ttest_line_count = 4 output &&\n-\n-\tgit config --unset gc.reflogexpire &&\n-\tgit config --unset gc.reflogexpireunreachable\n+\ttest_line_count = 4 output\n \n '\n \n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371607","messageId":"20190315155959.12390-6-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 5/8] reflog tests: test for the \"points nowhere\" warning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:56Z","receivedAt":"2019-03-15T16:00:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"git reflog expire\" command when given an unknown reference has\nsince 4264dc15e1 (\"git reflog expire\", 2006-12-19) when this command\nwas implemented emit an error, but this has never been tested for.\n\nLet's test for it, also under gc.reflogExpire{Unreachable,}=never in\ncase a future change is tempted to take shortcuts in the presence of\nsuch config.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1410-reflog.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 42f5ac9ed95..e8f8ac97856 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -250,6 +250,16 @@ test_expect_success 'gc.reflogexpire=false' '\n \n '\n \n+test_expect_success 'git reflog expire unknown reference' '\n+\ttest_config gc.reflogexpire never &&\n+\ttest_config gc.reflogexpireunreachable never &&\n+\n+\ttest_must_fail git reflog expire master@{123} 2>stderr &&\n+\ttest_i18ngrep \"points nowhere\" stderr &&\n+\ttest_must_fail git reflog expire does-not-exist 2>stderr &&\n+\ttest_i18ngrep \"points nowhere\" stderr\n+'\n+\n test_expect_success 'checkout should not delete log for packed ref' '\n \ttest $(git reflog master | wc -l) = 4 &&\n \tgit branch foo &&\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371608","messageId":"20190315155959.12390-7-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 6/8] reflog tests: assert lack of early exit with expiry=\"never\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:57Z","receivedAt":"2019-03-15T16:00:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\nand --stale-fix isn't in effect (covered by the first part of the \"if\"\nstatement being modified here) we *could* exit early without\npointlessly looping over all the reflogs.\n\nHowever, as an earlier change to add a test for the \"points nowhere\"\nwarning shows even in such a mode we might want to print out a\nwarning.\n\nSo while it's conceivable to implement this, I don't think it's worth\nit. It's going to be too easy to inadvertently add some flag that'll\nmake the expiry happen anyway, and even with \"never\" we'd like to see\nall the lines we're going to keep.\n\nSo let's assert that we're going to loop over all the references even\nwhen this configuration is in effect.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1410-reflog.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex e8f8ac97856..79f731db37c 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -235,7 +235,9 @@ test_expect_success 'gc.reflogexpire=never' '\n \ttest_config gc.reflogexpire never &&\n \ttest_config gc.reflogexpireunreachable never &&\n \n-\tgit reflog expire --verbose --all &&\n+\tgit reflog expire --verbose --all >output &&\n+\ttest_line_count = 9 output &&\n+\n \tgit reflog refs/heads/master >output &&\n \ttest_line_count = 4 output\n '\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371609","messageId":"20190315155959.12390-8-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 7/8] gc: handle & check gc.reflogExpire config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:58Z","receivedAt":"2019-03-15T16:00:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Don't redundantly run \"git reflog expire --all\" when gc.reflogExpire\nand gc.reflogExpireUnreachable are set to \"never\", and die immediately\nif those configuration valuer are bad.\n\nAs an earlier \"assert lack of early exit\" change to the tests for \"git\nreflog expire\" shows, an early check of gc.reflogExpire{Unreachable,}\nisn't wanted in general for \"git reflog expire\", but it makes sense\nfor \"gc\" because:\n\n 1) Similarly to 8ab5aa4bd8 (\"parseopt: handle malformed --expire\n arguments more nicely\", 2018-04-21) we'll now die early if the\n config variables are set to invalid values.\n\n We run \"pack-refs\" before \"reflog expire\", which can take a while,\n only to then die on an invalid gc.reflogExpire{Unreachable,}\n configuration.\n\n 2) Not invoking the command at all means it won't show up in trace\n output, which makes what's going on more obvious when the two are\n set to \"never\".\n\n 3) As a later change documents we lock the refs when looping over the\n refs to expire, even in cases where we end up doing nothing due to\n this config.\n\n For the reasons noted in the earlier \"assert lack of early exit\"\n change I don't think it's worth it to bend over backwards in \"git\n reflog expire\" itself to carefully detect if we'll really do\n nothing given the combination of all its possible options and skip\n that locking, but that's easy to detect here in \"gc\" where we'll\n only run \"reflog expire\" in a relatively simple mode.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 17 +++++++++++++++++\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ae716a00d4a..8943bcc300d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -116,6 +116,19 @@ static void process_log_file_on_signal(int signo)\n \traise(signo);\n }\n \n+static int gc_config_is_timestamp_never(const char *var)\n+{\n+\tconst char *value;\n+\ttimestamp_t expire;\n+\n+\tif (!git_config_get_value(var, &value) && value) {\n+\t\tif (parse_expiry_date(value, &expire))\n+\t\t\tdie(_(\"failed to parse '%s' value '%s'\"), var, value);\n+\t\treturn expire == 0;\n+\t}\n+\treturn 0;\n+}\n+\n static void gc_config(void)\n {\n \tconst char *value;\n@@ -127,6 +140,10 @@ static void gc_config(void)\n \t\t\tpack_refs = git_config_bool(\"gc.packrefs\", value);\n \t}\n \n+\tif (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n+\t gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n+\t\tprune_reflogs = 0;\n+\n \tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 4684d06552a..7411bf7fecd 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -120,6 +120,25 @@ test_expect_success 'gc --quiet' '\n \ttest_must_be_empty stderr\n '\n \n+test_expect_success 'gc.reflogExpire{Unreachable,}=never skips \"expire\" via \"gc\"' '\n+\ttest_config gc.reflogExpire never &&\n+\ttest_config gc.reflogExpireUnreachable never &&\n+\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\n+\t# Check that git-pack-refs is run as a sanity check (done via\n+\t# gc_before_repack()) but that git-expire is not.\n+\tgrep -E \"^trace: (built-in|exec|run_command): git pack-refs --\" trace.out &&\n+\t! grep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n+test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"expire\" via \"gc\"' '\n+\t>trace.out &&\n+\ttest_config gc.reflogExpire never &&\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9. Furthermore, the\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371610","messageId":"20190315155959.12390-9-avarab@gmail.com","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"[PATCH v3 8/8] reflog expire: don't assert the OID when locking refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T15:59:59Z","receivedAt":"2019-03-15T16:00:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"During reflog expiry, the cmd_reflog_expire() function first iterates\nover all reflogs in logs/*, and then one-by-one acquires the lock for\neach one to expire its reflog by getting a *.lock file on the\ncorresponding loose ref[1] (even if the actual ref is packed).\n\nThis lock is needed, but what isn't needed is locking the loose ref as\na function of the OID we found from that first iteration. By the time\nwe get around to re-visiting the reference some of the OIDs may have\nchanged.\n\nThus the verify_lock() function called by the lock_ref_oid_basic()\nfunction being changed here would fail with e.g. \"ref '%s' is at %s\nbut expected %s\" if the repository was being updated concurrent to the\n\"reflog expire\".\n\nBy not passing the OID to it we'll try to lock the reference\nregardless of it last known OID. Locking as a function of the OID\nwould make \"reflog expire\" exit with a non-zero exit status under such\ncontention, which in turn meant that a \"gc\" command (which expires\nreflogs before forking to the background) would encounter a hard\nerror.\n\nThis behavior of considering the OID when locking has been here ever\nsince \"reflog expire\" was initially implemented in 4264dc15e1 (\"git\nreflog expire\", 2006-12-19). As seen in that simpler initial version\nof the code we subsequently use the OID to inform the expiry (and\nstill do), but never needed to use it to lock the reference associated\nwith the reflog.\n\nBy locking the reference without considering what OID we last saw it\nat, we won't encounter user-visible contention to the extent that\ncore.filesRefLockTimeout mitigates it. See 4ff0f01cb7 (\"refs: retry\nacquiring reference locks for 100ms\", 2017-08-21).\n\nUnfortunately this sort of probabilistic contention is hard to turn\ninto a test. I've tested this by running the following three subshells\nin concurrent terminals:\n\n (\n cd /tmp &&\n rm -rf git &&\n git init git &&\n cd git &&\n while true\n do\n head -c 10 /dev/urandom | hexdump >out &&\n git add out &&\n git commit -m\"out\"\n done\n )\n\n (\n cd /tmp &&\n rm -rf git-clone &&\n git clone file:///tmp/git git-clone &&\n cd git-clone &&\n while git pull\n do\n date\n done\n )\n\n (\n cd /tmp/git-clone &&\n while git reflog expire --all\n do\n date\n done\n )\n\nBefore this change the \"reflog expire\" would fail really quickly with\na \"but expected\" error. After this change both the \"pull\" and \"reflog\nexpire\" will run for a while, but eventually fail because I get\nunlucky with core.filesRefLockTimeout (the \"reflog expire\" is in a\nreally tight loop). That can be resolved by being more generous with\nhigher values of core.filesRefLockTimeout than the 100ms default.\n\nAs noted in the commentary being added here we also need to handle the\ncase of references being racily deleted, that can be tested by adding\nthis to the above:\n\n (\n cd /tmp/git-clone &&\n while git branch topic master && git branch -D topic\n do\n date\n done\n )\n\nWe could change lock_ref_oid_basic() to always pass down\nRESOLVE_REF_READING to refs_resolve_ref_unsafe() and then\nfiles_reflog_expire() to detect the \"is it deleted?\" state. But let's\nnot bother, in the event of such a race we're going to redundantly\ncreate a lock on the deleted reference, and shortly afterwards handle\nthat case and others with the refs_reflog_exists() check.\n\n1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ef053f716c3..c7ed1792b3b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3036,8 +3036,14 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t * The reflog file is locked by holding the lock on the\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n+\t *\n+\t * We don't pass down the oid here because we'd like to be\n+\t * tolerant to the OID of the ref having changed, and to\n+\t * gracefully handle the case where it's been deleted (see oid\n+\t * -> mustexist -> RESOLVE_REF_READING in\n+\t * lock_ref_oid_basic()) ...\n \t */\n-\tlock = lock_ref_oid_basic(refs, refname, oid,\n+\tlock = lock_ref_oid_basic(refs, refname, NULL,\n \t\t\t\t NULL, NULL, REF_NO_DEREF,\n \t\t\t\t &type, &err);\n \tif (!lock) {\n@@ -3045,6 +3051,13 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\tstrbuf_release(&err);\n \t\treturn -1;\n \t}\n+\t/*\n+\t * When refs are deleted their reflog is deleted before the\n+\t * loose ref is deleted. This catches that case, i.e. when\n+\t * racing against a ref deletion lock_ref_oid_basic() will\n+\t * have acquired a lock on the now-deleted ref, but here's\n+\t * where we find out it has no reflog anymore.\n+\t */\n \tif (!refs_reflog_exists(ref_store, refname)) {\n \t\tunlock_ref(lock);\n \t\treturn 0;\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371777","messageId":"xmqq1s344t11.fsf@gitster-ct.c.googlers.com","threadId":"50723","inReplyTo":"877ed0bfl9.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 6/7] gc: don't run \"reflog expire\" when keeping reflogs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T06:04:58Z","receivedAt":"2019-03-18T06:05:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> + if (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n>>> + gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n>>\n>> Nit. configset api normalizes the key internally, so we can safely\n>> write gc.reflogExpireUnreachable here, which is a bit easier to read.\n>\n> I didn't know that, do we want to do that?\n>\n> I'd think that as a matter of coding style always sticking to lower case\n> in the C code made more sense, because e.g. you might #define a config\n> key, and then use it both in git_config_*() as well as via strcmp() in\n> some callback. Mixing the two would lead to confusion and possible bugs\n> as we'd refactor the former of those patterns to the latter.\n\nWhen parsing end-user supplied value in a \"const char *var\", with\n\n #define REU \"gc.reflogexpireunreacahble\"\n\nstrcmp(var, REU) is wrong. You must use strcasecmp(var, REU)\nbecause the end-user supplied value may be in camelCase.\n\nAnd once the code uses strcasecmp(var, REU), using camelCase\n\n #define REU \"gc.reflogExpireUnreacahble\"\n\nto compare against would not break it, either. The only difference\nis that being able to see the word boundary may make it easier to\nread.\n\nSo even when git_config-*() and str*cmp() are used in a mixed way, I\ndo not think it is a good justification to make the string constant\nall lowercase (assuming that camel is easier than all lowercase to\nread, that is).\n\nParsing three-level names is tricky. Without breaking it down into\ncomponents first, you cannot use strcmp nor strcasecmp; comparing\nthe whole variable name with either function is wrong.\n"},{"id":"371779","messageId":"xmqqpnqo3e34.fsf@gitster-ct.c.googlers.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"Re: [PATCH v3 0/8] gc: minor code cleanup + contention fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T06:13:03Z","receivedAt":"2019-03-18T06:15:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Changes since v2:\n>\n> * The \"let's detect that we don't need work in reflog expire\" patch\n> is gone, as noted in a new set of tests/commit messages that was\n> tricker than expected, and not worth it.\n>\n> * Minor rewording fixes to commit messages.\n>\n> * Further paraphrased rationale in commit messages from replies to v2\n> feedback (thanks all!)\n>\n> * The \"no OID\" and \"no mustexist\" case was confusingly conflated, now\n> we still do that in 8/8, but with a commit message & commit\n> explaining why and how that works.\n\nAll made sense (admittedly from a cursory look). Will queue.\n\nThanks, all.\n"},{"id":"371879","messageId":"20190319062034.GA31801@sigill.intra.peff.net","threadId":"50723","inReplyTo":"20190315155959.12390-7-avarab@gmail.com","subject":"Re: [PATCH v3 6/8] reflog tests: assert lack of early exit with expiry=\"never\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-19T06:20:35Z","receivedAt":"2019-03-19T06:20:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 15, 2019 at 04:59:57PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\n> and --stale-fix isn't in effect (covered by the first part of the \"if\"\n> statement being modified here) we *could* exit early without\n> pointlessly looping over all the reflogs.\n\nEr, which \"if\" statement are we modifying here? :)\n\nI assume this is leftover from the earlier attempt which actually did\nmodify it.\n\n-Peff\n"},{"id":"371880","messageId":"20190319062745.GB31801@sigill.intra.peff.net","threadId":"50723","inReplyTo":"20190314123439.4347-1-avarab@gmail.com","subject":"Re: [PATCH v2 0/7] gc: minor code cleanup + contention fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-19T06:27:45Z","receivedAt":"2019-03-19T06:27:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 01:34:32PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Addresse Peff's comments to v1. For a couple of patches I've faked up\n> his SOB where I copy/pasted a comment or code from a v1 comment\n> verbatim. Will see if he acks that.\n\nYep, for the record, those are fine (actually, I guess one of them got\naxed in v3).\n\n> The main change is a better commit message in the last patch (now\n> 7/7), and 2x new \"reflog\" patches to make it early exit in addition to\n> \"gc\" when there's nothing to do.\n> \n> There was the \"why do it at all in gc\" feedback on 6/7 in v1. I've\n> adjusted the commit message there to justify it, but we'll see what\n> Peff things about it this time around...\n\nEven after reading your v3 reasoning, I still think it would probably be\nOK for git-reflog to treat the \"never\" case as a noop. But frankly it's\nnot worth even spending any more brain cycles on. I can't imagine why\nsomebody would bother to run \"reflog expire\" without any expiration,\noutside of git-gc.\n\nThe improved commit messages all made sense to me (including the NULL\noid bits in the final one), with the exception of one nit I pointed out\nin one of the v3 messages.\n\n-Peff\n"},{"id":"372128","messageId":"87pnqkco8v.fsf@evledraar.gmail.com","threadId":"50723","inReplyTo":"b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu","subject":"Re: [PATCH v3 8/8] reflog expire: don't assert the OID when locking refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-21T14:10:08Z","receivedAt":"2019-03-21T14:10:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 19 2019, Michael Haggerty wrote:\n\n> Thanks for your work and for your thorough explanation of the change!\n\nHi. Yes, thanks a lot for the feedback. Just hadn't gotten around to\nlooping back to this yet & digging into the issue you raised.\n\n> On 3/15/19 4:59 PM, Ævar Arnfjörð Bjarmason wrote:\n>> During reflog expiry, the cmd_reflog_expire() function first iterates\n>> over all reflogs in logs/*, and then one-by-one acquires the lock for\n>> each one to expire its reflog by getting a *.lock file on the\n>> corresponding loose ref[1] (even if the actual ref is packed).\n>>\n>> This lock is needed, but what isn't needed is locking the loose ref as\n>> a function of the OID we found from that first iteration. By the time\n>> we get around to re-visiting the reference some of the OIDs may have\n>> changed.\n>\n> Instead of \"what isn't needed is locking the loose ref as a function of\n> the OID we found from that first iteration\", I suggest \"what isn't\n> needed is to insist that the reference still has the OID that we found\n> in that first iteration\".\n>\n>> Thus the verify_lock() function called by the lock_ref_oid_basic()\n>> function being changed here would fail with e.g. \"ref '%s' is at %s\n>> but expected %s\" if the repository was being updated concurrent to the\n>> \"reflog expire\".\n>>\n>> By not passing the OID to it we'll try to lock the reference\n>> regardless of it last known OID. Locking as a function of the OID\n>\n> s/it/its/\n>\n>> would make \"reflog expire\" exit with a non-zero exit status under such\n>> contention, which in turn meant that a \"gc\" command (which expires\n>> reflogs before forking to the background) would encounter a hard\n>> error.\n>\n> The last sentence seems mostly redundant with the previous paragraph.\n>\n>> This behavior of considering the OID when locking has been here ever\n>> since \"reflog expire\" was initially implemented in 4264dc15e1 (\"git\n>> reflog expire\", 2006-12-19). As seen in that simpler initial version\n>> of the code we subsequently use the OID to inform the expiry (and\n>> still do), but never needed to use it to lock the reference associated\n>> with the reflog.\n>>\n>> By locking the reference without considering what OID we last saw it\n>> at, we won't encounter user-visible contention to the extent that\n>> core.filesRefLockTimeout mitigates it. See 4ff0f01cb7 (\"refs: retry\n>> acquiring reference locks for 100ms\", 2017-08-21).\n>>\n>> Unfortunately this sort of probabilistic contention is hard to turn\n>> into a test. I've tested this by running the following three subshells\n>> in concurrent terminals:\n>>\n>> (\n>> cd /tmp &&\n>> rm -rf git &&\n>> git init git &&\n>> cd git &&\n>> while true\n>> do\n>> head -c 10 /dev/urandom | hexdump >out &&\n>> git add out &&\n>> git commit -m\"out\"\n>> done\n>> )\n>>\n>> (\n>> cd /tmp &&\n>> rm -rf git-clone &&\n>> git clone file:///tmp/git git-clone &&\n>> cd git-clone &&\n>> while git pull\n>> do\n>> date\n>> done\n>> )\n>>\n>> (\n>> cd /tmp/git-clone &&\n>> while git reflog expire --all\n>> do\n>> date\n>> done\n>> )\n>>\n>> Before this change the \"reflog expire\" would fail really quickly with\n>> a \"but expected\" error. After this change both the \"pull\" and \"reflog\n>> expire\" will run for a while, but eventually fail because I get\n>> unlucky with core.filesRefLockTimeout (the \"reflog expire\" is in a\n>> really tight loop). That can be resolved by being more generous with\n>> higher values of core.filesRefLockTimeout than the 100ms default.\n>>\n>> As noted in the commentary being added here we also need to handle the\n>> case of references being racily deleted, that can be tested by adding\n>> this to the above:\n>>\n>> (\n>> cd /tmp/git-clone &&\n>> while git branch topic master && git branch -D topic\n>> do\n>> date\n>> done\n>> )\n>>\n>> We could change lock_ref_oid_basic() to always pass down\n>> RESOLVE_REF_READING to refs_resolve_ref_unsafe() and then\n>> files_reflog_expire() to detect the \"is it deleted?\" state. But let's\n>> not bother, in the event of such a race we're going to redundantly\n>> create a lock on the deleted reference, and shortly afterwards handle\n>> that case and others with the refs_reflog_exists() check.\n>>\n>> 1. https://public-inbox.org/git/54857871.5090805@alum.mit.edu/\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> refs/files-backend.c | 15 ++++++++++++++-\n>> 1 file changed, 14 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index ef053f716c3..c7ed1792b3b 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -3036,8 +3036,14 @@ static int files_reflog_expire(struct ref_store *ref_store,\n>> \t * The reflog file is locked by holding the lock on the\n>> \t * reference itself, plus we might need to update the\n>> \t * reference if --updateref was specified:\n>> +\t *\n>> +\t * We don't pass down the oid here because we'd like to be\n>> +\t * tolerant to the OID of the ref having changed, and to\n>> +\t * gracefully handle the case where it's been deleted (see oid\n>> +\t * -> mustexist -> RESOLVE_REF_READING in\n>> +\t * lock_ref_oid_basic()) ...\n>> \t */\n>> -\tlock = lock_ref_oid_basic(refs, refname, oid,\n>> +\tlock = lock_ref_oid_basic(refs, refname, NULL,\n>> \t\t\t\t NULL, NULL, REF_NO_DEREF,\n>> \t\t\t\t &type, &err);\n>\n> This seems totally reasonable. But then later, where `oid` is passed to\n> `(*prepare_fn)()`, I think you must pass `&(lock->old_oid)` instead,\n> since we no longer have a guarantee that `oid` reflects the correct\n> state of the reference. And after that, there is no need for this\n> function to take an `oid` parameter at all (which also makes sense from\n> an abstract point of view). Which means that the signatures of\n> `refs_reflog_expire()`, `reflog_expire()`, `packed_reflog_expire()`, and\n> `reflog_expire_fn` can also be changed, along with callers.\n>\n> I haven't had time yet to inspect those callers to see whether they\n> might actually care that the `oid` that they used to pass to\n> `reflog_expire()` isn't necessarily the one that gets passed back to\n> their callbacks, but following the trail that I just outlined should\n> make it possible to determine that.\n>\n>> \tif (!lock) {\n>> @@ -3045,6 +3051,13 @@ static int files_reflog_expire(struct ref_store *ref_store,\n>> \t\tstrbuf_release(&err);\n>> \t\treturn -1;\n>> \t}\n>> +\t/*\n>> +\t * When refs are deleted their reflog is deleted before the\n>> +\t * loose ref is deleted. This catches that case, i.e. when\n>> +\t * racing against a ref deletion lock_ref_oid_basic() will\n>> +\t * have acquired a lock on the now-deleted ref, but here's\n>> +\t * where we find out it has no reflog anymore.\n>> +\t */\n>> \tif (!refs_reflog_exists(ref_store, refname)) {\n>> \t\tunlock_ref(lock);\n>> \t\treturn 0;\n>>\n>\n> Cheers,\n> Michael\n"},{"id":"372624","messageId":"20190328161434.19200-1-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 0/7] gc: tests and handle reflog expire config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:27Z","receivedAt":"2019-03-28T16:14:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"It seems the list software dislikes Michael Haggerty for some reason,\nbut as seen in my reply to his message my v4 has some unaddressed\nissues in the previous 8/8 that I need to get to:\nhttps://public-inbox.org/git/87pnqkco8v.fsf@evledraar.gmail.com/\n\nI'll have limited time next week to get to that, so in the meantime\nhere's a re-send without that patch, but just the unrelated\ncleanup/tests & gc \"expire\" fix up to v3's 7/8. The only other change\nis fixing commit message nonsense (from an earlier version), which\nPeff pointed out.\n\nI'll then get to the issue mhaggerty noted & submit that independently\nlater, but this should be ready for queuing & moving down to next,\nsince (unlike the previous 8/8) none of it's tricky code we need to be\nreally careful with.\n\nÆvar Arnfjörð Bjarmason (7):\n gc: remove redundant check for gc_auto_threshold\n gc: convert to using the_hash_algo\n gc: refactor a \"call me once\" pattern\n reflog tests: make use of \"test_config\" idiom\n reflog tests: test for the \"points nowhere\" warning\n reflog tests: assert lack of early exit with expiry=\"never\"\n gc: handle & check gc.reflogExpire config\n\n builtin/gc.c | 37 +++++++++++++++++++++++++++++--------\n t/t1410-reflog.sh | 25 +++++++++++++++++--------\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 3 files changed, 65 insertions(+), 16 deletions(-)\n\nRange-diff:\n1: 81694c8213 = 1: be889156db gc: remove redundant check for gc_auto_threshold\n2: 4bdcf1d0be = 2: 764c9a7380 gc: convert to using the_hash_algo\n3: 9444a1233a = 3: d521c22103 gc: refactor a \"call me once\" pattern\n4: 60a06ae618 = 4: 768aba9889 reflog tests: make use of \"test_config\" idiom\n5: 52838fdc44 = 5: 2ddbee93a1 reflog tests: test for the \"points nowhere\" warning\n6: 6063429f10 ! 6: 97e3d74371 reflog tests: assert lack of early exit with expiry=\"never\"\n @@ -3,8 +3,7 @@\n reflog tests: assert lack of early exit with expiry=\"never\"\n \n When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\n - and --stale-fix isn't in effect (covered by the first part of the \"if\"\n - statement being modified here) we *could* exit early without\n + and --stale-fix isn't in effect we *could* exit early without\n pointlessly looping over all the reflogs.\n \n However, as an earlier change to add a test for the \"points nowhere\"\n7: 6693d1d84d = 7: 48e5c234ae gc: handle & check gc.reflogExpire config\n8: e0814569ab < -: ---------- reflog expire: don't assert the OID when locking refs\n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372625","messageId":"20190328161434.19200-2-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 1/7] gc: remove redundant check for gc_auto_threshold","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:28Z","receivedAt":"2019-03-28T16:14:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Checking gc_auto_threshold in too_many_loose_objects() was added in\n17815501a8 (\"git-gc --auto: run \"repack -A -d -l\" as necessary.\",\n2007-09-17) when need_to_gc() itself was also reliant on\ngc_auto_pack_limit before its early return:\n\n gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0\n\nWhen that check was simplified to just checking \"gc_auto_threshold <=\n0\" in b14d255ba8 (\"builtin-gc.c: allow disabling all auto-gc'ing by\nassigning 0 to gc.auto\", 2008-03-19) this unreachable code should have\nbeen removed. We only call too_many_loose_objects() from within\nneed_to_gc() itself, which will return if this condition holds, and in\ncmd_gc() which will return before ever getting to \"auto_gc &&\ntoo_many_loose_objects()\" if \"auto_gc && !need_to_gc()\" is true\nearlier in the function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 020f725acc..8c2312681c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -157,9 +157,6 @@ static int too_many_loose_objects(void)\n \tint num_loose = 0;\n \tint needed = 0;\n \n-\tif (gc_auto_threshold <= 0)\n-\t\treturn 0;\n-\n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n \t\treturn 0;\n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372626","messageId":"20190328161434.19200-3-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 2/7] gc: convert to using the_hash_algo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:29Z","receivedAt":"2019-03-28T16:14:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There's been a lot of changing of the hardcoded \"40\" values to\nthe_hash_algo->hexsz, but we've so far missed this one where we\nhardcoded 38 for the loose object file length.\n\nThis is because a SHA-1 like abcde[...] gets turned into\nobjects/ab/cde[...]. There's no reason to suppose the same won't be\nthe case for SHA-256, and reading between the lines in\nhash-function-transition.txt the format is planned to be the same.\n\nIn the future we may want to further modify this code for the hash\nfunction transition. There's a potential pathological case here where\nwe'll only consider the loose objects for the currently active hash,\nbut objects for that hash will share a directory storage with the\nother hash.\n\nThus we could theoretically have e.g. 1k SHA-1 loose objects, and 1\nmillion SHA-256 objects. Then not notice that we need to pack them\nbecause we're currently using SHA-1, even though our FS may be\nstraining under the stress of such humongous directories.\n\nSo assuming that \"gc\" eventually learns to pack up both SHA-1 and\nSHA-256 objects regardless of what the current the_hash_algo is,\nperhaps this check should be changed to consider all files in\nobjects/17/ matching [0-9a-f] 38 or 62 characters in length (i.e. both\nSHA-1 and SHA-256).\n\nBut none of that is something we need to worry about now, and\nsupporting both 38 and 62 characters depending on \"the_hash_algo\"\nremoves another case of SHA-1 hardcoding.\n\nAs noted in [1] I'm making no effort to somehow remove the hardcoding\nfor \"2\" as in \"use the first two hexdigits for the directory\nname\". There's no indication that that'll ever change, and somehow\ngeneralizing it here would be a drop in the ocean, so there's no point\nin doing that. It also couldn't be done without coming up with some\ngeneralized version of the magical \"objects/17\" directory. See [2] for\na discussion of that directory.\n\n1. https://public-inbox.org/git/874l84ber7.fsf@evledraar.gmail.com/\n\n2. https://public-inbox.org/git/87k1mta9x5.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 8c2312681c..733bd7bdf4 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -156,6 +156,7 @@ static int too_many_loose_objects(void)\n \tint auto_threshold;\n \tint num_loose = 0;\n \tint needed = 0;\n+\tconst unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n \n \tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n@@ -163,8 +164,8 @@ static int too_many_loose_objects(void)\n \n \tauto_threshold = DIV_ROUND_UP(gc_auto_threshold, 256);\n \twhile ((ent = readdir(dir)) != NULL) {\n-\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != 38 ||\n-\t\t ent->d_name[38] != '\\0')\n+\t\tif (strspn(ent->d_name, \"0123456789abcdef\") != hexsz_loose ||\n+\t\t ent->d_name[hexsz_loose] != '\\0')\n \t\t\tcontinue;\n \t\tif (++num_loose > auto_threshold) {\n \t\t\tneeded = 1;\n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372627","messageId":"20190328161434.19200-4-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 3/7] gc: refactor a \"call me once\" pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:30Z","receivedAt":"2019-03-28T16:14:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change an idiom we're using to ensure that gc_before_repack() only\ndoes work once (see 62aad1849f (\"gc --auto: do not lock refs in the\nbackground\", 2014-05-25)) to be more obvious.\n\nNothing except this function cares about the \"pack_refs\" and\n\"prune_reflogs\" variables, so let's not leave the reader wondering if\nthey're being zero'd out for later use somewhere else.\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 733bd7bdf4..ae716a00d4 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -489,14 +489,20 @@ static int report_last_gc_error(void)\n \n static void gc_before_repack(void)\n {\n+\t/*\n+\t * We may be called twice, as both the pre- and\n+\t * post-daemonized phases will call us, but running these\n+\t * commands more than once is pointless and wasteful.\n+\t */\n+\tstatic int done = 0;\n+\tif (done++)\n+\t\treturn;\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n \t\tdie(FAILED_RUN, reflog.argv[0]);\n-\n-\tpack_refs = 0;\n-\tprune_reflogs = 0;\n }\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372628","messageId":"20190328161434.19200-6-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 5/7] reflog tests: test for the \"points nowhere\" warning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:32Z","receivedAt":"2019-03-28T16:14:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"git reflog expire\" command when given an unknown reference has\nsince 4264dc15e1 (\"git reflog expire\", 2006-12-19) when this command\nwas implemented emit an error, but this has never been tested for.\n\nLet's test for it, also under gc.reflogExpire{Unreachable,}=never in\ncase a future change is tempted to take shortcuts in the presence of\nsuch config.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1410-reflog.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 42f5ac9ed9..e8f8ac9785 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -250,6 +250,16 @@ test_expect_success 'gc.reflogexpire=false' '\n \n '\n \n+test_expect_success 'git reflog expire unknown reference' '\n+\ttest_config gc.reflogexpire never &&\n+\ttest_config gc.reflogexpireunreachable never &&\n+\n+\ttest_must_fail git reflog expire master@{123} 2>stderr &&\n+\ttest_i18ngrep \"points nowhere\" stderr &&\n+\ttest_must_fail git reflog expire does-not-exist 2>stderr &&\n+\ttest_i18ngrep \"points nowhere\" stderr\n+'\n+\n test_expect_success 'checkout should not delete log for packed ref' '\n \ttest $(git reflog master | wc -l) = 4 &&\n \tgit branch foo &&\n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372629","messageId":"20190328161434.19200-5-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 4/7] reflog tests: make use of \"test_config\" idiom","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:31Z","receivedAt":"2019-03-28T16:14:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a couple of tests that weren't using the helper to use it. This\nmakes the trailing \"--unset\" unnecessary.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1410-reflog.sh | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex ae8a448e34..42f5ac9ed9 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -232,24 +232,21 @@ test_expect_success '--expire=never' '\n '\n \n test_expect_success 'gc.reflogexpire=never' '\n+\ttest_config gc.reflogexpire never &&\n+\ttest_config gc.reflogexpireunreachable never &&\n \n-\tgit config gc.reflogexpire never &&\n-\tgit config gc.reflogexpireunreachable never &&\n \tgit reflog expire --verbose --all &&\n \tgit reflog refs/heads/master >output &&\n \ttest_line_count = 4 output\n '\n \n test_expect_success 'gc.reflogexpire=false' '\n+\ttest_config gc.reflogexpire false &&\n+\ttest_config gc.reflogexpireunreachable false &&\n \n-\tgit config gc.reflogexpire false &&\n-\tgit config gc.reflogexpireunreachable false &&\n \tgit reflog expire --verbose --all &&\n \tgit reflog refs/heads/master >output &&\n-\ttest_line_count = 4 output &&\n-\n-\tgit config --unset gc.reflogexpire &&\n-\tgit config --unset gc.reflogexpireunreachable\n+\ttest_line_count = 4 output\n \n '\n \n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372630","messageId":"20190328161434.19200-8-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 7/7] gc: handle & check gc.reflogExpire config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:34Z","receivedAt":"2019-03-28T16:14:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Don't redundantly run \"git reflog expire --all\" when gc.reflogExpire\nand gc.reflogExpireUnreachable are set to \"never\", and die immediately\nif those configuration valuer are bad.\n\nAs an earlier \"assert lack of early exit\" change to the tests for \"git\nreflog expire\" shows, an early check of gc.reflogExpire{Unreachable,}\nisn't wanted in general for \"git reflog expire\", but it makes sense\nfor \"gc\" because:\n\n 1) Similarly to 8ab5aa4bd8 (\"parseopt: handle malformed --expire\n arguments more nicely\", 2018-04-21) we'll now die early if the\n config variables are set to invalid values.\n\n We run \"pack-refs\" before \"reflog expire\", which can take a while,\n only to then die on an invalid gc.reflogExpire{Unreachable,}\n configuration.\n\n 2) Not invoking the command at all means it won't show up in trace\n output, which makes what's going on more obvious when the two are\n set to \"never\".\n\n 3) As a later change documents we lock the refs when looping over the\n refs to expire, even in cases where we end up doing nothing due to\n this config.\n\n For the reasons noted in the earlier \"assert lack of early exit\"\n change I don't think it's worth it to bend over backwards in \"git\n reflog expire\" itself to carefully detect if we'll really do\n nothing given the combination of all its possible options and skip\n that locking, but that's easy to detect here in \"gc\" where we'll\n only run \"reflog expire\" in a relatively simple mode.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 17 +++++++++++++++++\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ae716a00d4..8943bcc300 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -116,6 +116,19 @@ static void process_log_file_on_signal(int signo)\n \traise(signo);\n }\n \n+static int gc_config_is_timestamp_never(const char *var)\n+{\n+\tconst char *value;\n+\ttimestamp_t expire;\n+\n+\tif (!git_config_get_value(var, &value) && value) {\n+\t\tif (parse_expiry_date(value, &expire))\n+\t\t\tdie(_(\"failed to parse '%s' value '%s'\"), var, value);\n+\t\treturn expire == 0;\n+\t}\n+\treturn 0;\n+}\n+\n static void gc_config(void)\n {\n \tconst char *value;\n@@ -127,6 +140,10 @@ static void gc_config(void)\n \t\t\tpack_refs = git_config_bool(\"gc.packrefs\", value);\n \t}\n \n+\tif (gc_config_is_timestamp_never(\"gc.reflogexpire\") &&\n+\t gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n+\t\tprune_reflogs = 0;\n+\n \tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 4684d06552..7411bf7fec 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -120,6 +120,25 @@ test_expect_success 'gc --quiet' '\n \ttest_must_be_empty stderr\n '\n \n+test_expect_success 'gc.reflogExpire{Unreachable,}=never skips \"expire\" via \"gc\"' '\n+\ttest_config gc.reflogExpire never &&\n+\ttest_config gc.reflogExpireUnreachable never &&\n+\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\n+\t# Check that git-pack-refs is run as a sanity check (done via\n+\t# gc_before_repack()) but that git-expire is not.\n+\tgrep -E \"^trace: (built-in|exec|run_command): git pack-refs --\" trace.out &&\n+\t! grep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n+test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"expire\" via \"gc\"' '\n+\t>trace.out &&\n+\ttest_config gc.reflogExpire never &&\n+\tGIT_TRACE=$(pwd)/trace.out git gc &&\n+\tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9. Furthermore, the\n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372631","messageId":"20190328161434.19200-7-avarab@gmail.com","threadId":"50723","inReplyTo":"20190315155959.12390-1-avarab@gmail.com","subject":"[PATCH v4 6/7] reflog tests: assert lack of early exit with expiry=\"never\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T16:14:33Z","receivedAt":"2019-03-28T16:14:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When gc.reflogExpire and gc.reflogExpireUnreachable are set to \"never\"\nand --stale-fix isn't in effect we *could* exit early without\npointlessly looping over all the reflogs.\n\nHowever, as an earlier change to add a test for the \"points nowhere\"\nwarning shows even in such a mode we might want to print out a\nwarning.\n\nSo while it's conceivable to implement this, I don't think it's worth\nit. It's going to be too easy to inadvertently add some flag that'll\nmake the expiry happen anyway, and even with \"never\" we'd like to see\nall the lines we're going to keep.\n\nSo let's assert that we're going to loop over all the references even\nwhen this configuration is in effect.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1410-reflog.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex e8f8ac9785..79f731db37 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -235,7 +235,9 @@ test_expect_success 'gc.reflogexpire=never' '\n \ttest_config gc.reflogexpire never &&\n \ttest_config gc.reflogexpireunreachable never &&\n \n-\tgit reflog expire --verbose --all &&\n+\tgit reflog expire --verbose --all >output &&\n+\ttest_line_count = 9 output &&\n+\n \tgit reflog refs/heads/master >output &&\n \ttest_line_count = 4 output\n '\n-- \n2.21.0.392.gf8f6787159e\n\n"}]}