{"thread":{"id":"54079","subject":"[PATCH] refs: remove lookup cache for reference-transaction hook","startedAt":"2020-08-21T08:29:36Z","lastAt":"2020-08-25T18:29:28Z","messageCount":10,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404123","messageId":"0db8ad8cdb69afb9d6453bf60a808e8b82382a4e.1597998473.git.ps@pks.im","threadId":"54079","inReplyTo":null,"subject":"[PATCH] refs: remove lookup cache for reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-08-21T08:29:18Z","receivedAt":"2020-08-21T08:29:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding the reference-transaction hook, there were concerns about\nthe performance impact it may have on setups which do not make use of\nthe new hook at all. After all, it gets executed every time a reftx is\nprepared, committed or aborted, which linearly scales with the number of\nreference-transactions created per session. And as there are code paths\nlike `git push` which create a new transaction for each reference to be\nupdated, this may translate to calling `find_hook()` quite a lot.\n\nTo address this concern, a cache was added with the intention to not\nrepeatedly do negative hook lookups. Turns out this cache caused a\nregression, which was fixed via e5256c82e5 (refs: fix interleaving hook\ncalls with reference-transaction hook, 2020-08-07). In the process of\ndiscussing the fix, we realized that the cache doesn't really help even\nin the negative-lookup case. While performance tests added to benchmark\nthis did show a slight improvement in the 1% range, this really doesn't\nwarrent having a cache. Furthermore, it's quite flaky, too. E.g. running\nit twice in succession produces the following results:\n\nTest master pks-reftx-hook-remove-cache\n--------------------------------------------------------------------------\n1400.2: update-ref 2.79(2.16+0.74) 2.73(2.12+0.71) -2.2%\n1400.3: update-ref --stdin 0.22(0.08+0.14) 0.21(0.08+0.12) -4.5%\n\nTest master pks-reftx-hook-remove-cache\n--------------------------------------------------------------------------\n1400.2: update-ref 2.70(2.09+0.72) 2.74(2.13+0.71) +1.5%\n1400.3: update-ref --stdin 0.21(0.10+0.10) 0.21(0.08+0.13) +0.0%\n\nOne case notably absent from those benchmarks is a single executable\nsearching for the hook hundreds of times, which is exactly the case for\nwhich the negative cache was added. p1400.2 will spawn a new update-ref\nfor each transaction and p1400.3 only has a single reference-transaction\nfor all reference updates. So this commit adds a third benchmark, which\nperforms an non-atomic push of a thousand references. This will create a\nnew reference transaction per reference. But even for this case, the\nnegative cache doesn't consistently improve performance:\n\nTest master pks-reftx-hook-remove-cache\n--------------------------------------------------------------------------\n1400.4: nonatomic push 6.63(6.50+0.13) 6.81(6.67+0.14) +2.7%\n1400.4: nonatomic push 6.35(6.21+0.14) 6.39(6.23+0.16) +0.6%\n1400.4: nonatomic push 6.43(6.31+0.13) 6.42(6.28+0.15) -0.2%\n\nSo let's just remove the cache altogether to simplify the code.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 11 ++---------\n t/perf/p1400-update-ref.sh | 9 ++++++++-\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cf91711968..cb9bfc5c5c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1924,24 +1924,17 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n-static const char hook_not_found;\n-static const char *hook;\n-\n static int run_transaction_hook(struct ref_transaction *transaction,\n \t\t\t\tconst char *state)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *hook;\n \tint ret = 0, i;\n \n-\tif (hook == &hook_not_found)\n-\t\treturn ret;\n+\thook = find_hook(\"reference-transaction\");\n \tif (!hook)\n-\t\thook = xstrdup_or_null(find_hook(\"reference-transaction\"));\n-\tif (!hook) {\n-\t\thook = &hook_not_found;\n \t\treturn ret;\n-\t}\n \n \tstrvec_pushl(&proc.args, hook, state, NULL);\n \tproc.in = -1;\ndiff --git a/t/perf/p1400-update-ref.sh b/t/perf/p1400-update-ref.sh\nindex d275a81248..5d6da3cb70 100755\n--- a/t/perf/p1400-update-ref.sh\n+++ b/t/perf/p1400-update-ref.sh\n@@ -7,11 +7,13 @@ test_description=\"Tests performance of update-ref\"\n test_perf_fresh_repo\n \n test_expect_success \"setup\" '\n+\tgit init --bare target-repo.git &&\n \ttest_commit PRE &&\n \ttest_commit POST &&\n \tprintf \"create refs/heads/%d PRE\\n\" $(test_seq 1000) >create &&\n \tprintf \"update refs/heads/%d POST PRE\\n\" $(test_seq 1000) >update &&\n-\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete\n+\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete &&\n+\tprintf \"create refs/heads/branch-%d PRE\\n\" $(test_seq 1000) | git update-ref --stdin\n '\n \n test_perf \"update-ref\" '\n@@ -29,4 +31,9 @@ test_perf \"update-ref --stdin\" '\n \tgit update-ref --stdin One case notably absent from those benchmarks is a single executable\n> searching for the hook hundreds of times, which is exactly the case for\n> which the negative cache was added. p1400.2 will spawn a new update-ref\n> for each transaction and p1400.3 only has a single reference-transaction\n> for all reference updates. So this commit adds a third benchmark, which\n> performs an non-atomic push of a thousand references. This will create a\n> new reference transaction per reference. But even for this case, the\n> negative cache doesn't consistently improve performance:\n\nAh, right, I forgot that update-ref would use one single transaction. So\nwhat we were testing in our earlier discussion was not even useful. :)\n\n> test_expect_success \"setup\" '\n> +\tgit init --bare target-repo.git &&\n> \ttest_commit PRE &&\n> \ttest_commit POST &&\n> \tprintf \"create refs/heads/%d PRE\\n\" $(test_seq 1000) >create &&\n> \tprintf \"update refs/heads/%d POST PRE\\n\" $(test_seq 1000) >update &&\n> -\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete\n> +\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete &&\n> +\tprintf \"create refs/heads/branch-%d PRE\\n\" $(test_seq 1000) | git update-ref --stdin\n> '\n\nOK, we need these new branches to have something to push into and delete\nfrom the remote. They might impact the timings of the other tests,\nthough (since we now have 1000 entries in .git/refs/heads/, which might\naffect filesystem performance). But it should do so uniformly, so I\ndon't think it invalidates their results.\n\nHowever, I wondered...\n\n> +test_perf \"nonatomic push\" '\n> +\tgit push ./target-repo.git branch-{1..1000} &&\n> +\tgit push --delete ./target-repo.git branch-{1..1000}\n> +'\n\n...if it might make the test more consistent (not to mention isolated\nfrom the cost of other parts of the push) if we used update-ref here, as\nwell. You added the code necessary to control individual transactions,\nso I thought that:\n\n printf 'start\\ncreate refs/heads/%d PRE\\ncommit\\n' \\\n $(test_seq 1000) >create-transaction\n\nmight work. But it doesn't, because after the first transaction is\nclosed, we refuse to accept any other commands. That makes sense for\n\"prepare\", etc, but there's no reason we couldn't start a new one.\n\nIs that worth supporting? It would allow a caller to use a single\nupdate-ref to make a series of non-atomic updates, which is something\nthat can't currently be done. And we're so close.\n\nEven if it is, though, that's definitely outside the scope of this\npatch, and I think we should take it as-is with \"push\".\n\n-Peff\n"},{"id":"404148","messageId":"xmqqeeo09chm.fsf@gitster.c.googlers.com","threadId":"54079","inReplyTo":"20200821143727.GA3241139@coredump.intra.peff.net","subject":"Re: [PATCH] refs: remove lookup cache for reference-transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T16:42:45Z","receivedAt":"2020-08-21T16:44:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> However, I wondered...\n>\n>> +test_perf \"nonatomic push\" '\n>> +\tgit push ./target-repo.git branch-{1..1000} &&\n>> +\tgit push --delete ./target-repo.git branch-{1..1000}\n>> +'\n\nIs this a bash-and-ksh-only test? At least, the above would not try\nto push 1000 branches with the version of dash I have.\n"},{"id":"404167","messageId":"20200821172137.GA3261095@coredump.intra.peff.net","threadId":"54079","inReplyTo":"xmqqeeo09chm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] refs: remove lookup cache for reference-transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T17:21:37Z","receivedAt":"2020-08-21T17:21:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 21, 2020 at 09:42:45AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > However, I wondered...\n> >\n> >> +test_perf \"nonatomic push\" '\n> >> +\tgit push ./target-repo.git branch-{1..1000} &&\n> >> +\tgit push --delete ./target-repo.git branch-{1..1000}\n> >> +'\n> \n> Is this a bash-and-ksh-only test? At least, the above would not try\n> to push 1000 branches with the version of dash I have.\n\nHeh, I was so focused on the \"push\" part of it that I didn't even look\ncarefully at the second half of the command-line. ;)\n\nI think pushing \"refs/heads/branch-*\" would work for pushing. For\ndeletion, though, I don't think we allow wildcards in the refspecs.\nYou could abuse pruning:\n\n git push --prune ../dst.git refs/heads/does-not-exist/*:refs/heads/*\n\nIt also may be OK to just omit that half of the test. I think the\ninitial push exercises the case we care about. Though I guess we do run\nthe test repeatedly, so we might have to do:\n\n rm -rf dst.git &&\n git init dst.git &&\n git push dst.git refs/heads/branch-*\n\n-Peff\n"},{"id":"404249","messageId":"20200822085951.GB1069@ncase.pks.im","threadId":"54079","inReplyTo":"20200821143727.GA3241139@coredump.intra.peff.net","subject":"Re: [PATCH] refs: remove lookup cache for reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-08-22T08:59:51Z","receivedAt":"2020-08-22T08:59:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 21, 2020 at 10:37:27AM -0400, Jeff King wrote:\n> On Fri, Aug 21, 2020 at 10:29:18AM +0200, Patrick Steinhardt wrote:\n> \n> > One case notably absent from those benchmarks is a single executable\n> > searching for the hook hundreds of times, which is exactly the case for\n> > which the negative cache was added. p1400.2 will spawn a new update-ref\n> > for each transaction and p1400.3 only has a single reference-transaction\n> > for all reference updates. So this commit adds a third benchmark, which\n> > performs an non-atomic push of a thousand references. This will create a\n> > new reference transaction per reference. But even for this case, the\n> > negative cache doesn't consistently improve performance:\n> \n> Ah, right, I forgot that update-ref would use one single transaction. So\n> what we were testing in our earlier discussion was not even useful. :)\n> \n> > test_expect_success \"setup\" '\n> > +\tgit init --bare target-repo.git &&\n> > \ttest_commit PRE &&\n> > \ttest_commit POST &&\n> > \tprintf \"create refs/heads/%d PRE\\n\" $(test_seq 1000) >create &&\n> > \tprintf \"update refs/heads/%d POST PRE\\n\" $(test_seq 1000) >update &&\n> > -\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete\n> > +\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete &&\n> > +\tprintf \"create refs/heads/branch-%d PRE\\n\" $(test_seq 1000) | git update-ref --stdin\n> > '\n> \n> OK, we need these new branches to have something to push into and delete\n> from the remote. They might impact the timings of the other tests,\n> though (since we now have 1000 entries in .git/refs/heads/, which might\n> affect filesystem performance). But it should do so uniformly, so I\n> don't think it invalidates their results.\n> \n> However, I wondered...\n> \n> > +test_perf \"nonatomic push\" '\n> > +\tgit push ./target-repo.git branch-{1..1000} &&\n> > +\tgit push --delete ./target-repo.git branch-{1..1000}\n> > +'\n> \n> ...if it might make the test more consistent (not to mention isolated\n> from the cost of other parts of the push) if we used update-ref here, as\n> well. You added the code necessary to control individual transactions,\n> so I thought that:\n> \n> printf 'start\\ncreate refs/heads/%d PRE\\ncommit\\n' \\\n> $(test_seq 1000) >create-transaction\n> \n> might work. But it doesn't, because after the first transaction is\n> closed, we refuse to accept any other commands. That makes sense for\n> \"prepare\", etc, but there's no reason we couldn't start a new one.\n> \n> Is that worth supporting? It would allow a caller to use a single\n> update-ref to make a series of non-atomic updates, which is something\n> that can't currently be done. And we're so close.\n\nYeah, I had the exact same thought and I do think it's useful to be able\nto create multiple reference transactions per git-update-ref(1) session.\nI might whip something up as soon as I find the time to do so, it really\nshouldn't be a lot of work.\n\nPatrick\n\n> Even if it is, though, that's definitely outside the scope of this\n> patch, and I think we should take it as-is with \"push\".\n> \n> -Peff\n"},{"id":"404250","messageId":"20200822090234.GC1069@ncase.pks.im","threadId":"54079","inReplyTo":"20200821172137.GA3261095@coredump.intra.peff.net","subject":"Re: [PATCH] refs: remove lookup cache for reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-08-22T09:02:34Z","receivedAt":"2020-08-22T09:02:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 21, 2020 at 01:21:37PM -0400, Jeff King wrote:\n> On Fri, Aug 21, 2020 at 09:42:45AM -0700, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > However, I wondered...\n> > >\n> > >> +test_perf \"nonatomic push\" '\n> > >> +\tgit push ./target-repo.git branch-{1..1000} &&\n> > >> +\tgit push --delete ./target-repo.git branch-{1..1000}\n> > >> +'\n> > \n> > Is this a bash-and-ksh-only test? At least, the above would not try\n> > to push 1000 branches with the version of dash I have.\n\nI didn't realize it's shell-specific behaviour, thanks for highlighting.\n\n> Heh, I was so focused on the \"push\" part of it that I didn't even look\n> carefully at the second half of the command-line. ;)\n> \n> I think pushing \"refs/heads/branch-*\" would work for pushing. For\n> deletion, though, I don't think we allow wildcards in the refspecs.\n> You could abuse pruning:\n> \n> git push --prune ../dst.git refs/heads/does-not-exist/*:refs/heads/*\n> \n> It also may be OK to just omit that half of the test. I think the\n> initial push exercises the case we care about. Though I guess we do run\n> the test repeatedly, so we might have to do:\n> \n> rm -rf dst.git &&\n> git init dst.git &&\n> git push dst.git refs/heads/branch-*\n\nI'm not too keen to use `rm -rf && git init` as it muddies the subject\nunder test a bit. I'll try to come up with a non-shell-specific version\nof this on Monday.\n\nPatrick\n"},{"id":"404398","messageId":"c1cae6dd19ffe00e4456e4f96ad92277ceeced27.1598349284.git.ps@pks.im","threadId":"54079","inReplyTo":"0db8ad8cdb69afb9d6453bf60a808e8b82382a4e.1597998473.git.ps@pks.im","subject":"[PATCH v2] refs: remove lookup cache for reference-transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-08-25T10:35:24Z","receivedAt":"2020-08-25T10:35:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding the reference-transaction hook, there were concerns about\nthe performance impact it may have on setups which do not make use of\nthe new hook at all. After all, it gets executed every time a reftx is\nprepared, committed or aborted, which linearly scales with the number of\nreference-transactions created per session. And as there are code paths\nlike `git push` which create a new transaction for each reference to be\nupdated, this may translate to calling `find_hook()` quite a lot.\n\nTo address this concern, a cache was added with the intention to not\nrepeatedly do negative hook lookups. Turns out this cache caused a\nregression, which was fixed via e5256c82e5 (refs: fix interleaving hook\ncalls with reference-transaction hook, 2020-08-07). In the process of\ndiscussing the fix, we realized that the cache doesn't really help even\nin the negative-lookup case. While performance tests added to benchmark\nthis did show a slight improvement in the 1% range, this really doesn't\nwarrent having a cache. Furthermore, it's quite flaky, too. E.g. running\nit twice in succession produces the following results:\n\nTest master pks-reftx-hook-remove-cache\n--------------------------------------------------------------------------\n1400.2: update-ref 2.79(2.16+0.74) 2.73(2.12+0.71) -2.2%\n1400.3: update-ref --stdin 0.22(0.08+0.14) 0.21(0.08+0.12) -4.5%\n\nTest master pks-reftx-hook-remove-cache\n--------------------------------------------------------------------------\n1400.2: update-ref 2.70(2.09+0.72) 2.74(2.13+0.71) +1.5%\n1400.3: update-ref --stdin 0.21(0.10+0.10) 0.21(0.08+0.13) +0.0%\n\nOne case notably absent from those benchmarks is a single executable\nsearching for the hook hundreds of times, which is exactly the case for\nwhich the negative cache was added. p1400.2 will spawn a new update-ref\nfor each transaction and p1400.3 only has a single reference-transaction\nfor all reference updates. So this commit adds a third benchmark, which\nperforms an non-atomic push of a thousand references. This will create a\nnew reference transaction per reference. But even for this case, the\nnegative cache doesn't consistently improve performance:\n\nTest master pks-reftx-hook-remove-cache\n--------------------------------------------------------------------------\n1400.4: nonatomic push 6.63(6.50+0.13) 6.81(6.67+0.14) +2.7%\n1400.4: nonatomic push 6.35(6.21+0.14) 6.39(6.23+0.16) +0.6%\n1400.4: nonatomic push 6.43(6.31+0.13) 6.42(6.28+0.15) -0.2%\n\nSo let's just remove the cache altogether to simplify the code.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nThe only change compared to v1 is that I've addressed the unportable\n`branch-{1..1000}` syntax in favor of `test_seq`. I had to setup refs as\npart of the setup and change the ordering for \"update-ref --stdin\" from\ncreate/update/delete to update/delete/create, but I don't think that's\ntoo bad. At least timings didn't seem to really change because of that.\n\n refs.c | 11 ++---------\n t/perf/p1400-update-ref.sh | 13 ++++++++++---\n 2 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cf91711968..cb9bfc5c5c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1924,24 +1924,17 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n-static const char hook_not_found;\n-static const char *hook;\n-\n static int run_transaction_hook(struct ref_transaction *transaction,\n \t\t\t\tconst char *state)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *hook;\n \tint ret = 0, i;\n \n-\tif (hook == &hook_not_found)\n-\t\treturn ret;\n+\thook = find_hook(\"reference-transaction\");\n \tif (!hook)\n-\t\thook = xstrdup_or_null(find_hook(\"reference-transaction\"));\n-\tif (!hook) {\n-\t\thook = &hook_not_found;\n \t\treturn ret;\n-\t}\n \n \tstrvec_pushl(&proc.args, hook, state, NULL);\n \tproc.in = -1;\ndiff --git a/t/perf/p1400-update-ref.sh b/t/perf/p1400-update-ref.sh\nindex d275a81248..ce5ac3ed85 100755\n--- a/t/perf/p1400-update-ref.sh\n+++ b/t/perf/p1400-update-ref.sh\n@@ -7,11 +7,13 @@ test_description=\"Tests performance of update-ref\"\n test_perf_fresh_repo\n \n test_expect_success \"setup\" '\n+\tgit init --bare target-repo.git &&\n \ttest_commit PRE &&\n \ttest_commit POST &&\n \tprintf \"create refs/heads/%d PRE\\n\" $(test_seq 1000) >create &&\n \tprintf \"update refs/heads/%d POST PRE\\n\" $(test_seq 1000) >update &&\n-\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete\n+\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete &&\n+\tgit update-ref --stdin The only change compared to v1 is that I've addressed the unportable\n> `branch-{1..1000}` syntax in favor of `test_seq`. I had to setup refs as\n> part of the setup and change the ordering for \"update-ref --stdin\" from\n> create/update/delete to update/delete/create, but I don't think that's\n> too bad. At least timings didn't seem to really change because of that.\n\nAnother option instead of changing the order in the other tests is to do\nanother untimed setup step before the push test. I'm OK either way,\nthough.\n\n> +test_perf \"nonatomic push\" '\n> +\tgit push ./target-repo.git $(test_seq 1000) &&\n> +\tgit push --delete ./target-repo.git $(test_seq 1000)\n> '\n\nThis works as far as Git is concerned, but \"seq 1000\" output with NULs\nis 3893 bytes. I wonder if some platforms might run into command-line\nlimits there. I guess we will see when Windows CI runs. :)\n\n-Peff\n"},{"id":"404436","messageId":"xmqqd03e1tsd.fsf@gitster.c.googlers.com","threadId":"54079","inReplyTo":"20200825151053.GA1409139@coredump.intra.peff.net","subject":"Re: [PATCH v2] refs: remove lookup cache for reference-transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T18:09:54Z","receivedAt":"2020-08-25T18:10:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 25, 2020 at 12:35:24PM +0200, Patrick Steinhardt wrote:\n>\n>> The only change compared to v1 is that I've addressed the unportable\n>> `branch-{1..1000}` syntax in favor of `test_seq`. I had to setup refs as\n>> part of the setup and change the ordering for \"update-ref --stdin\" from\n>> create/update/delete to update/delete/create, but I don't think that's\n>> too bad. At least timings didn't seem to really change because of that.\n>\n> Another option instead of changing the order in the other tests is to do\n> another untimed setup step before the push test. I'm OK either way,\n> though.\n>\n>> +test_perf \"nonatomic push\" '\n>> +\tgit push ./target-repo.git $(test_seq 1000) &&\n>> +\tgit push --delete ./target-repo.git $(test_seq 1000)\n>> '\n>\n> This works as far as Git is concerned, but \"seq 1000\" output with NULs\n> is 3893 bytes. I wonder if some platforms might run into command-line\n> limits there.\n\nThat was my thought when I saw the above as well. In addition, I do\nnot think it is a good idea to encourage digit-only refnames.\n\n"},{"id":"404439","messageId":"20200825182922.GB1417288@coredump.intra.peff.net","threadId":"54079","inReplyTo":"xmqqd03e1tsd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] refs: remove lookup cache for reference-transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T18:29:22Z","receivedAt":"2020-08-25T18:29:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 11:09:54AM -0700, Junio C Hamano wrote:\n\n> >> +test_perf \"nonatomic push\" '\n> >> +\tgit push ./target-repo.git $(test_seq 1000) &&\n> >> +\tgit push --delete ./target-repo.git $(test_seq 1000)\n> >> '\n> >\n> > This works as far as Git is concerned, but \"seq 1000\" output with NULs\n> > is 3893 bytes. I wonder if some platforms might run into command-line\n> > limits there.\n> \n> That was my thought when I saw the above as well. In addition, I do\n> not think it is a good idea to encourage digit-only refnames.\n\nGood point. It gets hairy at four digits:\n\n $ git show 1000\n error: short SHA1 1000 is ambiguous\n hint: The candidates are:\n hint: 10000434d2 tree\n hint: 10007bcb9e tree\n hint: 10008a0e22 tree\n hint: 1000bdf512 tree\n hint: 1000dc2368 blob\n fatal: ambiguous argument '1000': unknown revision or path not in the working tree.\n\nSo I think if the test works it may be relying on the exact object ids\nthat happen to be generated (which fortunately are at least\ndeterministic these days, but it may be a trap waiting to spring for\nsomebody later).\n\n-Peff\n"}]}