{"thread":{"id":"55865","subject":"[PATCH 4/4] read-cache perf: add a perf test for refresh_index()","startedAt":"2021-06-07T11:58:59Z","lastAt":"2021-08-27T07:50:38Z","messageCount":18,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Emily Shaffer","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"426576","messageId":"patch-4.4-120a37acae-20210607T115454Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","subject":"[PATCH 4/4] read-cache perf: add a perf test for refresh_index()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T11:58:27Z","receivedAt":"2021-06-07T11:58:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a perf test for the refresh_index() function to compliment the\nexisting read()/discard() in a loop perf test added in\n1ecb5ff141f (read-cache: add simple performance test, 2013-06-09).\n\nSince this test is much slower (around 10x) than the previous\nread()/discard() test let's run it 100 times instead of the 1000 time\nthe first one runs.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-read-cache-perf.c | 12 ++++++++++++\n t/perf/p0002-read-cache.sh      |  5 +++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c\nindex 54ad0c3135..a2802559a5 100644\n--- a/t/helper/test-read-cache-perf.c\n+++ b/t/helper/test-read-cache-perf.c\n@@ -11,8 +11,11 @@ int cmd__read_cache_perf(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n \tint cnt = -1;\n+\tint refresh = 0;\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"count\", &cnt, \"number of passes\"),\n+\t\tOPT_BOOL(0, \"refresh\", &refresh,\n+\t\t\t \"call refresh_index() in a loop, not read()/discard()\"),\n \t\tOPT_END()\n \t};\n \n@@ -26,10 +29,19 @@ int cmd__read_cache_perf(int argc, const char **argv)\n \t\t\t      options);\n \n \tsetup_git_directory();\n+\tif (refresh)\n+\t\trepo_read_index(r);\n \twhile (cnt--) {\n+\t\tif (refresh) {\n+\t\t\tunsigned int flags = REFRESH_QUIET|REFRESH_PROGRESS;\n+\t\t\trefresh_index(r->index, flags, NULL, NULL, NULL);\n+\t\t\tcontinue;\n+\t\t}\n \t\trepo_read_index(r);\n \t\tdiscard_index(r->index);\n \t}\n+\tif (refresh)\n+\t\tdiscard_index(r->index);\n \n \treturn 0;\n }\ndiff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh\nindex 1762b64865..cbccc5ace9 100755\n--- a/t/perf/p0002-read-cache.sh\n+++ b/t/perf/p0002-read-cache.sh\n@@ -11,4 +11,9 @@ test_perf \"read_cache/discard_cache $count times\" \"\n \ttest-tool read-cache-perf --count=$count\n \"\n \n+count=100\n+test_perf \"refresh_index() $count times\" \"\n+\ttest-tool read-cache-perf --count=$count --refresh\n+\"\n+\n test_done\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426577","messageId":"patch-3.4-36f4072b13-20210607T115454Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","subject":"[PATCH 3/4] test-tools: migrate read-cache-again to parse_options()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T11:58:26Z","receivedAt":"2021-06-07T11:59:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the newly added (but then mostly copy/pasted) read-cache-perf\nto use the parse_options() API. I have no plans to further modify\nread-cache-again, but making these commands consistent has a value in\nand of itself.\n\nSince we check the \"cnt = < 1\" case now via more idiomatic\npost-parse_options() assertions we can move from the for-loop to a\nwhile-loop and ditch the \"i\" variable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-read-cache-again.c | 28 ++++++++++++++++++++++------\n t/t7519-status-fsmonitor.sh      |  2 +-\n 2 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c\nindex 707db036cb..8487f79d04 100644\n--- a/t/helper/test-read-cache-again.c\n+++ b/t/helper/test-read-cache-again.c\n@@ -1,20 +1,36 @@\n #include \"test-tool.h\"\n #include \"cache.h\"\n+#include \"parse-options.h\"\n+\n+static const char *read_cache_again_usage[] = {\n+\t\"test-tool read-cache-again [<options>...] <file>\",\n+\tNULL\n+};\n \n int cmd__read_cache_again(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n-\tint i, cnt;\n+\tint cnt = -1;\n \tconst char *name;\n+\tstruct option options[] = {\n+\t\tOPT_INTEGER(0, \"count\", &cnt, \"number of passes\"),\n+\t\tOPT_END()\n+\t};\n \n-\tif (argc != 2)\n-\t\tdie(\"usage: test-tool read-cache-again <count> <file>\");\n-\n-\tcnt = strtol(argv[0], NULL, 0);\n+\targc = parse_options(argc, argv, \"test-tools\", options,\n+\t\t\t     read_cache_again_usage, 0);\n+\tif (argc != 1)\n+\t\tusage_msg_opt(\"Too many arguments.\", read_cache_again_usage,\n+\t\t\t      options);\n+\tif (cnt == -1)\n+\t\tcnt = 2;\n+\telse if (cnt < 1)\n+\t\tusage_msg_opt(\"Need at least one pass.\", read_cache_again_usage,\n+\t\t\t      options);\n \tname = argv[2];\n \n \tsetup_git_directory();\n-\tfor (i = 0; i < cnt; i++) {\n+\twhile (cnt--) {\n \t\tint pos;\n \t\trepo_read_index(r);\n \t\trefresh_index(r->index, REFRESH_QUIET,\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 4c199c16d4..fd0815f6b7 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -359,7 +359,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n test_expect_success 'discard_index() also discards fsmonitor info' '\n \ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-all\" &&\n \ttest_might_fail git update-index --refresh &&\n-\ttest-tool read-cache-again 2 tracked >actual &&\n+\ttest-tool read-cache-again --count=2 tracked >actual &&\n \tprintf \"tracked is%s up to date\\n\" \"\" \" not\" >expect &&\n \ttest_cmp expect actual\n '\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426578","messageId":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","threadId":"55865","inReplyTo":null,"subject":"[PATCH 0/4] test-tool: split up \"read-cache\" tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T11:58:23Z","receivedAt":"2021-06-07T11:59:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the sparse index topic was being discussed I suggested that the\nt/helper/read-cache.c tool was getting to the point of doing too many\nthings and should be split up.\n\nSince that series has landed on master here's that suggestion again in\nthe form of patches on top of master. The 4/4 patch is a \"while I was\nat it\" addition of an extra perf test for index refreshing.\n\n1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (4):\n  test-tool: split up test-tool read-cache\n  test-tools: migrate read-cache-perf to parse_options()\n  test-tools: migrate read-cache-again to parse_options()\n  read-cache perf: add a perf test for refresh_index()\n\n Makefile                         |  2 ++\n t/helper/test-read-cache-again.c | 47 +++++++++++++++++++++++++++\n t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++\n t/helper/test-read-cache.c       | 56 +++++++++++++-------------------\n t/helper/test-tool.c             |  2 ++\n t/helper/test-tool.h             |  2 ++\n t/perf/p0002-read-cache.sh       |  7 +++-\n t/t7519-status-fsmonitor.sh      |  2 +-\n 8 files changed, 130 insertions(+), 35 deletions(-)\n create mode 100644 t/helper/test-read-cache-again.c\n create mode 100644 t/helper/test-read-cache-perf.c\n\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426579","messageId":"patch-2.4-07f392e087-20210607T115454Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","subject":"[PATCH 2/4] test-tools: migrate read-cache-perf to parse_options()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T11:58:25Z","receivedAt":"2021-06-07T11:59:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the newly added (but then mostly copy/pasted) read-cache-perf\nto use the parse_options() API. This will make things easier as we add\nnew options.\n\nSince we check the \"cnt = < 1\" case now via more idiomatic\npost-parse_options() assertions we can move from the for-loop to a\nwhile-loop and ditch the \"i\" variable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-read-cache-perf.c | 26 ++++++++++++++++++++------\n t/perf/p0002-read-cache.sh      |  2 +-\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c\nindex 90176c010a..54ad0c3135 100644\n--- a/t/helper/test-read-cache-perf.c\n+++ b/t/helper/test-read-cache-perf.c\n@@ -1,18 +1,32 @@\n #include \"test-tool.h\"\n #include \"cache.h\"\n+#include \"parse-options.h\"\n+\n+static const char *read_cache_perf_usage[] = {\n+\t\"test-tool read-cache-perf [<options>...]\",\n+\tNULL\n+};\n \n int cmd__read_cache_perf(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n-\tint i, cnt = 1;\n+\tint cnt = -1;\n+\tstruct option options[] = {\n+\t\tOPT_INTEGER(0, \"count\", &cnt, \"number of passes\"),\n+\t\tOPT_END()\n+\t};\n \n-\tif (argc == 2)\n-\t\tcnt = strtol(argv[1], NULL, 0);\n-\telse\n-\t\tdie(\"usage: test-tool read-cache-perf [<count>]\");\n+\targc = parse_options(argc, argv, \"test-tools\", options,\n+\t\t\t     read_cache_perf_usage, 0);\n+\tif (argc > 0)\n+\t\tusage_msg_opt(\"Too many arguments.\", read_cache_perf_usage,\n+\t\t\t      options);\n+\tif (cnt < 1)\n+\t\tusage_msg_opt(\"Need at least one pass.\", read_cache_perf_usage,\n+\t\t\t      options);\n \n \tsetup_git_directory();\n-\tfor (i = 0; i < cnt; i++) {\n+\twhile (cnt--) {\n \t\trepo_read_index(r);\n \t\tdiscard_index(r->index);\n \t}\ndiff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh\nindex d0ba5173fb..1762b64865 100755\n--- a/t/perf/p0002-read-cache.sh\n+++ b/t/perf/p0002-read-cache.sh\n@@ -8,7 +8,7 @@ test_perf_default_repo\n \n count=1000\n test_perf \"read_cache/discard_cache $count times\" \"\n-\ttest-tool read-cache-perf $count\n+\ttest-tool read-cache-perf --count=$count\n \"\n \n test_done\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426580","messageId":"patch-1.4-6e7fcd4693-20210607T115454Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","subject":"[PATCH 1/4] test-tool: split up test-tool read-cache","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T11:58:24Z","receivedAt":"2021-06-07T11:59:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since the \"test-tool read-cache\" was originally added back in\n1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) it's\nbeen growing all sorts of bells and whistles that aren't very\nconducive to performance testing the index, e.g. it learned how to\nread config.\n\nThen in recent changes in e2df6c3972 (test-read-cache: print cache\nentries with --table, 2021-03-30) and 2782db3eed (test-tool: don't\nforce full index, 2021-03-30) we gained even more logic to deal with\nsparse index testing.\n\nI think that having one test tool do so many different things makes it\nharder to read its code. Let's instead split up the \"again\" and \"perf\"\nuses for it into their own tools.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile                         |  2 ++\n t/helper/test-read-cache-again.c | 31 ++++++++++++++++++\n t/helper/test-read-cache-perf.c  | 21 ++++++++++++\n t/helper/test-read-cache.c       | 56 +++++++++++++-------------------\n t/helper/test-tool.c             |  2 ++\n t/helper/test-tool.h             |  2 ++\n t/perf/p0002-read-cache.sh       |  2 +-\n t/t7519-status-fsmonitor.sh      |  2 +-\n 8 files changed, 83 insertions(+), 35 deletions(-)\n create mode 100644 t/helper/test-read-cache-again.c\n create mode 100644 t/helper/test-read-cache-perf.c\n\ndiff --git a/Makefile b/Makefile\nindex c3565fc0f8..142303bd17 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -732,6 +732,8 @@ TEST_BUILTINS_OBJS += test-prio-queue.o\n TEST_BUILTINS_OBJS += test-proc-receive.o\n TEST_BUILTINS_OBJS += test-progress.o\n TEST_BUILTINS_OBJS += test-reach.o\n+TEST_BUILTINS_OBJS += test-read-cache-again.o\n+TEST_BUILTINS_OBJS += test-read-cache-perf.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-read-graph.o\n TEST_BUILTINS_OBJS += test-read-midx.o\ndiff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c\nnew file mode 100644\nindex 0000000000..707db036cb\n--- /dev/null\n+++ b/t/helper/test-read-cache-again.c\n@@ -0,0 +1,31 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+\n+int cmd__read_cache_again(int argc, const char **argv)\n+{\n+\tstruct repository *r = the_repository;\n+\tint i, cnt;\n+\tconst char *name;\n+\n+\tif (argc != 2)\n+\t\tdie(\"usage: test-tool read-cache-again <count> <file>\");\n+\n+\tcnt = strtol(argv[0], NULL, 0);\n+\tname = argv[2];\n+\n+\tsetup_git_directory();\n+\tfor (i = 0; i < cnt; i++) {\n+\t\tint pos;\n+\t\trepo_read_index(r);\n+\t\trefresh_index(r->index, REFRESH_QUIET,\n+\t\t\t      NULL, NULL, NULL);\n+\t\tpos = index_name_pos(r->index, name, strlen(name));\n+\t\tif (pos < 0)\n+\t\t\tdie(\"%s not in index\", name);\n+\t\tprintf(\"%s is%s up to date\\n\", name,\n+\t\t       ce_uptodate(r->index->cache[pos]) ? \"\" : \" not\");\n+\t\twrite_file(name, \"%d\\n\", cnt);\n+\t\tdiscard_index(r->index);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c\nnew file mode 100644\nindex 0000000000..90176c010a\n--- /dev/null\n+++ b/t/helper/test-read-cache-perf.c\n@@ -0,0 +1,21 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+\n+int cmd__read_cache_perf(int argc, const char **argv)\n+{\n+\tstruct repository *r = the_repository;\n+\tint i, cnt = 1;\n+\n+\tif (argc == 2)\n+\t\tcnt = strtol(argv[1], NULL, 0);\n+\telse\n+\t\tdie(\"usage: test-tool read-cache-perf [<count>]\");\n+\n+\tsetup_git_directory();\n+\tfor (i = 0; i < cnt; i++) {\n+\t\trepo_read_index(r);\n+\t\tdiscard_index(r->index);\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\nindex b52c174acc..ae4b9b70ad 100644\n--- a/t/helper/test-read-cache.c\n+++ b/t/helper/test-read-cache.c\n@@ -5,6 +5,12 @@\n #include \"commit.h\"\n #include \"tree.h\"\n #include \"sparse-index.h\"\n+#include \"parse-options.h\"\n+\n+static const char *read_cache_usage[] = {\n+\t\"test-tool read-cache [<options>...]\",\n+\tNULL\n+};\n \n static void print_cache_entry(struct cache_entry *ce)\n {\n@@ -34,49 +40,33 @@ static void print_cache(struct index_state *istate)\n int cmd__read_cache(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n-\tint i, cnt = 1;\n-\tconst char *name = NULL;\n \tint table = 0, expand = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"table\", &table,\n+\t\t\t \"print a dump of the cache\"),\n+\t\tOPT_BOOL(0, \"expand\", &expand,\n+\t\t\t \"call ensure_full_index()\"),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, \"test-tools\", options, read_cache_usage, 0);\n+\tif (argc > 0)\n+\t\tusage_msg_opt(\"Too many arguments.\", read_cache_usage, options);\n \n \tinitialize_the_repository();\n \tprepare_repo_settings(r);\n \tr->settings.command_requires_full_index = 0;\n \n-\tfor (++argv, --argc; *argv && starts_with(*argv, \"--\"); ++argv, --argc) {\n-\t\tif (skip_prefix(*argv, \"--print-and-refresh=\", &name))\n-\t\t\tcontinue;\n-\t\tif (!strcmp(*argv, \"--table\"))\n-\t\t\ttable = 1;\n-\t\telse if (!strcmp(*argv, \"--expand\"))\n-\t\t\texpand = 1;\n-\t}\n-\n-\tif (argc == 1)\n-\t\tcnt = strtol(argv[0], NULL, 0);\n \tsetup_git_directory();\n \tgit_config(git_default_config, NULL);\n+\trepo_read_index(r);\n \n-\tfor (i = 0; i < cnt; i++) {\n-\t\trepo_read_index(r);\n-\n-\t\tif (expand)\n-\t\t\tensure_full_index(r->index);\n+\tif (expand)\n+\t\tensure_full_index(r->index);\n \n-\t\tif (name) {\n-\t\t\tint pos;\n+\tif (table)\n+\t\tprint_cache(r->index);\n+\tdiscard_index(r->index);\n \n-\t\t\trefresh_index(r->index, REFRESH_QUIET,\n-\t\t\t\t      NULL, NULL, NULL);\n-\t\t\tpos = index_name_pos(r->index, name, strlen(name));\n-\t\t\tif (pos < 0)\n-\t\t\t\tdie(\"%s not in index\", name);\n-\t\t\tprintf(\"%s is%s up to date\\n\", name,\n-\t\t\t       ce_uptodate(r->index->cache[pos]) ? \"\" : \" not\");\n-\t\t\twrite_file(name, \"%d\\n\", i);\n-\t\t}\n-\t\tif (table)\n-\t\t\tprint_cache(r->index);\n-\t\tdiscard_index(r->index);\n-\t}\n \treturn 0;\n }\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex c5bd0c6d4c..b0300f70c7 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -54,6 +54,8 @@ static struct test_cmd cmds[] = {\n \t{ \"progress\", cmd__progress },\n \t{ \"reach\", cmd__reach },\n \t{ \"read-cache\", cmd__read_cache },\n+\t{ \"read-cache-again\", cmd__read_cache_again },\n+\t{ \"read-cache-perf\", cmd__read_cache_perf },\n \t{ \"read-graph\", cmd__read_graph },\n \t{ \"read-midx\", cmd__read_midx },\n \t{ \"ref-store\", cmd__ref_store },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex e8069a3b22..7f451a1eb5 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -43,6 +43,8 @@ int cmd__proc_receive(int argc, const char **argv);\n int cmd__progress(int argc, const char **argv);\n int cmd__reach(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n+int cmd__read_cache_again(int argc, const char **argv);\n+int cmd__read_cache_perf(int argc, const char **argv);\n int cmd__read_graph(int argc, const char **argv);\n int cmd__read_midx(int argc, const char **argv);\n int cmd__ref_store(int argc, const char **argv);\ndiff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh\nindex cdd105a594..d0ba5173fb 100755\n--- a/t/perf/p0002-read-cache.sh\n+++ b/t/perf/p0002-read-cache.sh\n@@ -8,7 +8,7 @@ test_perf_default_repo\n \n count=1000\n test_perf \"read_cache/discard_cache $count times\" \"\n-\ttest-tool read-cache $count\n+\ttest-tool read-cache-perf $count\n \"\n \n test_done\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 637391c6ce..4c199c16d4 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -359,7 +359,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n test_expect_success 'discard_index() also discards fsmonitor info' '\n \ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-all\" &&\n \ttest_might_fail git update-index --refresh &&\n-\ttest-tool read-cache --print-and-refresh=tracked 2 >actual &&\n+\ttest-tool read-cache-again 2 tracked >actual &&\n \tprintf \"tracked is%s up to date\\n\" \"\" \" not\" >expect &&\n \ttest_cmp expect actual\n '\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426671","messageId":"xmqqr1hdthla.fsf@gitster.g","threadId":"55865","inReplyTo":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","subject":"Re: [PATCH 0/4] test-tool: split up \"read-cache\" tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-07T22:50:25Z","receivedAt":"2021-06-07T22:50:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> When the sparse index topic was being discussed I suggested that the\n> t/helper/read-cache.c tool was getting to the point of doing too many\n> things and should be split up.\n>\n> Since that series has landed on master here's that suggestion again in\n> the form of patches on top of master. The 4/4 patch is a \"while I was\n> at it\" addition of an extra perf test for index refreshing.\n>\n> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/\n>\n> Ævar Arnfjörð Bjarmason (4):\n>   test-tool: split up test-tool read-cache\n>   test-tools: migrate read-cache-perf to parse_options()\n>   test-tools: migrate read-cache-again to parse_options()\n>   read-cache perf: add a perf test for refresh_index()\n\nIs the contrast between tool and tools deliberate?\n\n\n>  Makefile                         |  2 ++\n>  t/helper/test-read-cache-again.c | 47 +++++++++++++++++++++++++++\n>  t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++\n>  t/helper/test-read-cache.c       | 56 +++++++++++++-------------------\n>  t/helper/test-tool.c             |  2 ++\n>  t/helper/test-tool.h             |  2 ++\n>  t/perf/p0002-read-cache.sh       |  7 +++-\n>  t/t7519-status-fsmonitor.sh      |  2 +-\n>  8 files changed, 130 insertions(+), 35 deletions(-)\n>  create mode 100644 t/helper/test-read-cache-again.c\n>  create mode 100644 t/helper/test-read-cache-perf.c\n"},{"id":"426757","messageId":"87tum838u8.fsf@evledraar.gmail.com","threadId":"55865","inReplyTo":"xmqqr1hdthla.fsf@gitster.g","subject":"Re: [PATCH 0/4] test-tool: split up \"read-cache\" tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T11:14:36Z","receivedAt":"2021-06-08T11:17:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 08 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> When the sparse index topic was being discussed I suggested that the\n>> t/helper/read-cache.c tool was getting to the point of doing too many\n>> things and should be split up.\n>>\n>> Since that series has landed on master here's that suggestion again in\n>> the form of patches on top of master. The 4/4 patch is a \"while I was\n>> at it\" addition of an extra perf test for index refreshing.\n>>\n>> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/\n>>\n>> Ævar Arnfjörð Bjarmason (4):\n>>   test-tool: split up test-tool read-cache\n>>   test-tools: migrate read-cache-perf to parse_options()\n>>   test-tools: migrate read-cache-again to parse_options()\n>>   read-cache perf: add a perf test for refresh_index()\n>\n> Is the contrast between tool and tools deliberate?\n\nYes, I figured \"test-tool:\" describes the main entry point of the\n\"test-tool somecmd\", so if I split up \"somecmd\" into \"othercmd\" that's a\n\"test-tool\" change.\n\nBut \"test-tools:\" when I'm modifying particular tools, I can change 2/4\nand 3/4 it to \"test-tool read-cache-perf:\" and \"test-tool\nread-cache-again\" (or another thing you suggest) if you think this\nwarrants a re-roll.\n\n>>  Makefile                         |  2 ++\n>>  t/helper/test-read-cache-again.c | 47 +++++++++++++++++++++++++++\n>>  t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++\n>>  t/helper/test-read-cache.c       | 56 +++++++++++++-------------------\n>>  t/helper/test-tool.c             |  2 ++\n>>  t/helper/test-tool.h             |  2 ++\n>>  t/perf/p0002-read-cache.sh       |  7 +++-\n>>  t/t7519-status-fsmonitor.sh      |  2 +-\n>>  8 files changed, 130 insertions(+), 35 deletions(-)\n>>  create mode 100644 t/helper/test-read-cache-again.c\n>>  create mode 100644 t/helper/test-read-cache-perf.c\n\n"},{"id":"426824","messageId":"xmqq8s3kndk4.fsf@gitster.g","threadId":"55865","inReplyTo":"87tum838u8.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/4] test-tool: split up \"read-cache\" tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-08T23:26:19Z","receivedAt":"2021-06-08T23:26:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>>> Ævar Arnfjörð Bjarmason (4):\n>>>   test-tool: split up test-tool read-cache\n>>>   test-tools: migrate read-cache-perf to parse_options()\n>>>   test-tools: migrate read-cache-again to parse_options()\n>>>   read-cache perf: add a perf test for refresh_index()\n>>\n>> Is the contrast between tool and tools deliberate?\n>\n> Yes, I figured \"test-tool:\" describes the main entry point of the\n> \"test-tool somecmd\", so if I split up \"somecmd\" into \"othercmd\" that's a\n> \"test-tool\" change.\n>\n> But \"test-tools:\" when I'm modifying particular tools, I can change 2/4\n> and 3/4 it to \"test-tool read-cache-perf:\" and \"test-tool\n> read-cache-again\" (or another thing you suggest) if you think this\n> warrants a re-roll.\n\nI actually meant s/test-tools:/test-tool:/ and nothing else, as\nchanges to read-cache-perf and changes to read-cache-again both fall\ninto the same test-tool umbrella.  It's not like we benefit from\nhaving two separate <area> (as in \"<area>: <description>\")\ndesignators for read-cache-perf and read-cache-again---in a larger\npicture, they are both things around test-tool helper.\n\nThanks.\n\n\n"},{"id":"428855","messageId":"YNzp4WIlZIzqD7PU@google.com","threadId":"55865","inReplyTo":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","subject":"Re: [PATCH 0/4] test-tool: split up \"read-cache\" tool","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-30T22:02:41Z","receivedAt":"2021-06-30T22:02:52Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Jun 07, 2021 at 01:58:23PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> When the sparse index topic was being discussed I suggested that the\n> t/helper/read-cache.c tool was getting to the point of doing too many\n> things and should be split up.\n> \n> Since that series has landed on master here's that suggestion again in\n> the form of patches on top of master. The 4/4 patch is a \"while I was\n> at it\" addition of an extra perf test for index refreshing.\n> \n> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/\n> \n\nSince \"What's cooking\" mentioned this series was starved for review, we\ntook a look at it today. Most of my comments are pretty general, so I'll\nreply primarily to the cover letter instead.\n\nThe result after this series is that we have three forms of 'test-tool\nread-cache':\n - one for unit testing, with no iteration (test-tool read-cache)\n - one for not-so-perfy iteration (test-tool read-cache-again)\n - one for perfy iteration and refresh_index benching (test-tool\n   read-cache-perf)\n\nBefore I read patch 4, I said, \"'again' and 'perf' have a lot of code in\ncommon, but I guess we are trying to reduce the amount of overhead for\ntight-loop performance testing, so OK.\" But patch 4 adds an alternative\nbody for the inside of the loop, which *looks* not very performant\n(although is probably optimized well) by checking an unchanging bool on\nevery iteration of the loop.\n\nI tend to think it would be easier to read\nand understand to have two forms of 'test-tool read-cache' - one which\niterates and one which does not. Maybe the one that iterates should be\ncalled -perf, maybe it should be called something else, whatever. And\nperhaps it makes sense for the iterating one to look like so (heavily\npseudocoded, hope you can follow along with the rough sketch):\n\n  enum iteration_mode;\n\n  parse_options();\n  if (should_do_again) {\n    iteration_mode = AGAIN;\n  else if (should_do_perf) {\n    iteration_mode = PERF;\n  else if (should_do_refresh) {\n    iteration_mode = REFRESH;\n  }\n\n  while (passes--)\n    switch (iteration_mode) {\n    case AGAIN:\n      read index;\n      refresh index;\n      index_name_pos;\n      error reporting;\n      write_file;\n      discard_index;\n      break;\n    case PERF:\n      read index;\n      discard index;\n      break;\n    case REFRESH:\n      refresh_index;\n      break;\n    }\n\nThis would put all our \"loop lots of times for performance benchmarking\"\ninto one place. We know that the switch statement is very performant,\nespecially if we manage to const-ify iteration_mode.  The cases make it\nvery clear that the body of the loop is being swapped out depending on\nthe arguments, and that entirely different behavior is happening in each\nscenario.\n\nThere's also an orthogonal bit of cleanup here by moving to\nparse_options(), which I am excited about in general :) but which I\nthink wasn't done very cleanly in this series. In patch 1, the commit\nmessage makes no mention of the fairly significant refactor happening to\nmove 'test-tool read-cache' to parse_options(), and I think the mix of\ncut&paste with refactor makes the patch a little muddy. What about a\nseries like so:\n\n 1/3: teach test-tool read-cache to use parse_options()\n 2/3: add test-tool read-cache-(perf|iterating|whatever)\n 3/3: teach test-tool read-cache-perf \"--refresh\"\n\n\nThanks, and hopefully this is a welcome necromancy and not an annoying\none ;)\n\n - Emily\n"},{"id":"428857","messageId":"87pmw3dlkl.fsf@evledraar.gmail.com","threadId":"55865","inReplyTo":"YNzp4WIlZIzqD7PU@google.com","subject":"Re: [PATCH 0/4] test-tool: split up \"read-cache\" tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T22:24:37Z","receivedAt":"2021-06-30T22:39:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 30 2021, Emily Shaffer wrote:\n\n> On Mon, Jun 07, 2021 at 01:58:23PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> When the sparse index topic was being discussed I suggested that the\n>> t/helper/read-cache.c tool was getting to the point of doing too many\n>> things and should be split up.\n>> \n>> Since that series has landed on master here's that suggestion again in\n>> the form of patches on top of master. The 4/4 patch is a \"while I was\n>> at it\" addition of an extra perf test for index refreshing.\n>> \n>> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/\n>> \n>\n> Since \"What's cooking\" mentioned this series was starved for review, we\n> took a look at it today. Most of my comments are pretty general, so I'll\n> reply primarily to the cover letter instead.\n>\n> The result after this series is that we have three forms of 'test-tool\n> read-cache':\n>  - one for unit testing, with no iteration (test-tool read-cache)\n>  - one for not-so-perfy iteration (test-tool read-cache-again)\n>  - one for perfy iteration and refresh_index benching (test-tool\n>    read-cache-perf)\n>\n> Before I read patch 4, I said, \"'again' and 'perf' have a lot of code in\n> common, but I guess we are trying to reduce the amount of overhead for\n> tight-loop performance testing, so OK.\" But patch 4 adds an alternative\n> body for the inside of the loop, which *looks* not very performant\n> (although is probably optimized well) by checking an unchanging bool on\n> every iteration of the loop.\n>\n> I tend to think it would be easier to read\n> and understand to have two forms of 'test-tool read-cache' - one which\n> iterates and one which does not. Maybe the one that iterates should be\n> called -perf, maybe it should be called something else, whatever. And\n> perhaps it makes sense for the iterating one to look like so (heavily\n> pseudocoded, hope you can follow along with the rough sketch):\n>\n>   enum iteration_mode;\n>\n>   parse_options();\n>   if (should_do_again) {\n>     iteration_mode = AGAIN;\n>   else if (should_do_perf) {\n>     iteration_mode = PERF;\n>   else if (should_do_refresh) {\n>     iteration_mode = REFRESH;\n>   }\n>\n>   while (passes--)\n>     switch (iteration_mode) {\n>     case AGAIN:\n>       read index;\n>       refresh index;\n>       index_name_pos;\n>       error reporting;\n>       write_file;\n>       discard_index;\n>       break;\n>     case PERF:\n>       read index;\n>       discard index;\n>       break;\n>     case REFRESH:\n>       refresh_index;\n>       break;\n>     }\n>\n> This would put all our \"loop lots of times for performance benchmarking\"\n> into one place. We know that the switch statement is very performant,\n> especially if we manage to const-ify iteration_mode.  The cases make it\n> very clear that the body of the loop is being swapped out depending on\n> the arguments, and that entirely different behavior is happening in each\n> scenario.\n>\n> There's also an orthogonal bit of cleanup here by moving to\n> parse_options(), which I am excited about in general :) but which I\n> think wasn't done very cleanly in this series. In patch 1, the commit\n> message makes no mention of the fairly significant refactor happening to\n> move 'test-tool read-cache' to parse_options(), and I think the mix of\n> cut&paste with refactor makes the patch a little muddy. What about a\n> series like so:\n>\n>  1/3: teach test-tool read-cache to use parse_options()\n>  2/3: add test-tool read-cache-(perf|iterating|whatever)\n>  3/3: teach test-tool read-cache-perf \"--refresh\"\n>\n>\n> Thanks, and hopefully this is a welcome necromancy and not an annoying\n> one ;)\n\nThanks.\n\nYes 1/4 is a bit confsing, but the alternative to converting it to\nparse_option while we're at it would be to introduce support for\nunderstanding \"print-and-refresh\" in one commit, only to remove it in\nanother. So I aimed for something that would play as well with the diff\nrename detection as possible for 1/4. Do you think it's worth it to add\nsuch an back & forth step?\n\nFor the rest I think there's been a bit of a misunderstanding (which is\non me in not explaining this well enough), i.e. your:\n\n    I guess we are trying to reduce the amount of overhead for\n    tight-loop performance testing\n\nThat wasn't really what I was going for at all, but rather (copied from\nthe 1st commit message):\n\n    I think that having one test tool do so many different things makes it\n    harder to read its code. Let's instead split up the \"again\" and \"perf\"\n    uses for it into their own tools.\n\nThis series was extracted out of an RFC-reply of mine to one of\nDerrick's recent patches where he was adding more knobs to\ntest-read-cache.c (and I understand the plan in the near-term is to add\nmore sparse-specific things to it).\n\nSo the goal here was only to split these different use cases all sharing\none function so that they used 3 functions instead.\n\nYour suggestion of the switch/case loop would work, and would be\nperformant. Optimizing this wasn't a goal at all though, making it more\nreadable was.\n\nI don't think it matters much if at all how performant the scaffolding\nof the test-tool itself is, if it was relatively more expensive we could\njust run more loops and we'd eventually get meaningful numbers in the\nperf test. The \"perf\" in the name is just \"this is for the perf test\"\nnot \"this is the performant version\".\n\nI think that for the test tools where we're mostly not aiming for\ngeneralized but helpers narrowly tailored to specific tests it makes\nmore sense to split early than have tools that take N options, with\nfunctions that use some small subset of those N options for any given\nuse-case.\n\nThere's going to be duplication in the scaffolding\n(e.g. parse_options()), but that sort of thing is easy to read, whereas\nreasoning about some inner loop and wondering \"hrm, did this config call\nthere impact any of this\" takes more time than it should.\n\nAnyway, knowing that's the goal (and performance of the tool itself\nwasn't, at all), do you think the end result makes more/less sense than\nbefore?\n"},{"id":"433515","messageId":"cover-v2-0.4-00000000000-20210824T091204Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com","subject":"[PATCH v2 0/4] test-tool: split up \"read-cache\" tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T09:15:21Z","receivedAt":"2021-08-24T09:15:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A re-roll addressing the feedback v1 got, see\nhttps://lore.kernel.org/git/cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com\n\nI think the gist of that feedback from Emily was that v1 could be\nunderstood as aiming to optimize the perf test, but the main reason\nI'm doing this split up is to make the code easier to read. The\nchanged commit message summarizes both goals.\n\nI also changed some nits in the code I spotted myself, e.g. \"argc > 0\"\nchecks to just \"argc\", and a simpler way of providing the \"cnt\"\ndefault.\n\nÆvar Arnfjörð Bjarmason (4):\n  test-tool: split up test-tool read-cache\n  test-tool: migrate read-cache-perf to parse_options()\n  test-tool: migrate read-cache-again to parse_options()\n  read-cache perf: add a perf test for refresh_index()\n\n Makefile                         |  2 ++\n t/helper/test-read-cache-again.c | 45 +++++++++++++++++++++++++\n t/helper/test-read-cache-perf.c  | 47 +++++++++++++++++++++++++++\n t/helper/test-read-cache.c       | 56 +++++++++++++-------------------\n t/helper/test-tool.c             |  2 ++\n t/helper/test-tool.h             |  2 ++\n t/perf/p0002-read-cache.sh       |  7 +++-\n t/t7519-status-fsmonitor.sh      |  2 +-\n 8 files changed, 128 insertions(+), 35 deletions(-)\n create mode 100644 t/helper/test-read-cache-again.c\n create mode 100644 t/helper/test-read-cache-perf.c\n\nRange-diff against v1:\n1:  6e7fcd46934 ! 1:  adb3f989a29 test-tool: split up test-tool read-cache\n    @@ Commit message\n         test-tool: split up test-tool read-cache\n     \n         Since the \"test-tool read-cache\" was originally added back in\n    -    1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) it's\n    -    been growing all sorts of bells and whistles that aren't very\n    -    conducive to performance testing the index, e.g. it learned how to\n    -    read config.\n    +    1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) the\n    +    test-read-cache.c tool has been growing various features that make the\n    +    code harder to read. I.e. sometimes running as a one-off, sometimes looping.\n    +\n    +    It's also been unconditionally reading config since\n    +    dc76852df2f (fsmonitor: demonstrate that it is not refreshed after\n    +    discard_index(), 2019-05-07), which introduces unnecessary noise into\n    +    the performance test.\n     \n         Then in recent changes in e2df6c3972 (test-read-cache: print cache\n         entries with --table, 2021-03-30) and 2782db3eed (test-tool: don't\n    @@ Commit message\n     \n         I think that having one test tool do so many different things makes it\n         harder to read its code. Let's instead split up the \"again\" and \"perf\"\n    -    uses for it into their own tools.\n    +    uses for it into their own small tools, this makes the main\n    +    \"test-read-cache.c\" a simpler.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    @@ t/helper/test-read-cache.c: static void print_cache(struct index_state *istate)\n     +\t};\n     +\n     +\targc = parse_options(argc, argv, \"test-tools\", options, read_cache_usage, 0);\n    -+\tif (argc > 0)\n    ++\tif (argc)\n     +\t\tusage_msg_opt(\"Too many arguments.\", read_cache_usage, options);\n      \n      \tinitialize_the_repository();\n2:  07f392e0878 ! 2:  a68fa4a6355 test-tools: migrate read-cache-perf to parse_options()\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    test-tools: migrate read-cache-perf to parse_options()\n    +    test-tool: migrate read-cache-perf to parse_options()\n     \n         Change the newly added (but then mostly copy/pasted) read-cache-perf\n         to use the parse_options() API. This will make things easier as we add\n    @@ t/helper/test-read-cache-perf.c\n     -\t\tdie(\"usage: test-tool read-cache-perf [<count>]\");\n     +\targc = parse_options(argc, argv, \"test-tools\", options,\n     +\t\t\t     read_cache_perf_usage, 0);\n    -+\tif (argc > 0)\n    ++\tif (argc)\n     +\t\tusage_msg_opt(\"Too many arguments.\", read_cache_perf_usage,\n     +\t\t\t      options);\n     +\tif (cnt < 1)\n3:  36f4072b131 ! 3:  a34e69eaa48 test-tools: migrate read-cache-again to parse_options()\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    test-tools: migrate read-cache-again to parse_options()\n    +    test-tool: migrate read-cache-again to parse_options()\n     \n         Change the newly added (but then mostly copy/pasted) read-cache-perf\n         to use the parse_options() API. I have no plans to further modify\n    @@ t/helper/test-read-cache-again.c\n      {\n      \tstruct repository *r = the_repository;\n     -\tint i, cnt;\n    -+\tint cnt = -1;\n    ++\tint cnt = 2;\n      \tconst char *name;\n     +\tstruct option options[] = {\n     +\t\tOPT_INTEGER(0, \"count\", &cnt, \"number of passes\"),\n    @@ t/helper/test-read-cache-again.c\n     +\tif (argc != 1)\n     +\t\tusage_msg_opt(\"Too many arguments.\", read_cache_again_usage,\n     +\t\t\t      options);\n    -+\tif (cnt == -1)\n    -+\t\tcnt = 2;\n    -+\telse if (cnt < 1)\n    ++\tif (cnt < 1)\n     +\t\tusage_msg_opt(\"Need at least one pass.\", read_cache_again_usage,\n     +\t\t\t      options);\n      \tname = argv[2];\n4:  120a37acaef = 4:  e3648bf78c7 read-cache perf: add a perf test for refresh_index()\n-- \n2.33.0.663.gbaff4edb973\n\n"},{"id":"433516","messageId":"patch-v2-1.4-adb3f989a29-20210824T091204Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-v2-0.4-00000000000-20210824T091204Z-avarab@gmail.com","subject":"[PATCH v2 1/4] test-tool: split up test-tool read-cache","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T09:15:22Z","receivedAt":"2021-08-24T09:15:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since the \"test-tool read-cache\" was originally added back in\n1ecb5ff141 (read-cache: add simple performance test, 2013-06-09) the\ntest-read-cache.c tool has been growing various features that make the\ncode harder to read. I.e. sometimes running as a one-off, sometimes looping.\n\nIt's also been unconditionally reading config since\ndc76852df2f (fsmonitor: demonstrate that it is not refreshed after\ndiscard_index(), 2019-05-07), which introduces unnecessary noise into\nthe performance test.\n\nThen in recent changes in e2df6c3972 (test-read-cache: print cache\nentries with --table, 2021-03-30) and 2782db3eed (test-tool: don't\nforce full index, 2021-03-30) we gained even more logic to deal with\nsparse index testing.\n\nI think that having one test tool do so many different things makes it\nharder to read its code. Let's instead split up the \"again\" and \"perf\"\nuses for it into their own small tools, this makes the main\n\"test-read-cache.c\" a simpler.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile                         |  2 ++\n t/helper/test-read-cache-again.c | 31 ++++++++++++++++++\n t/helper/test-read-cache-perf.c  | 21 ++++++++++++\n t/helper/test-read-cache.c       | 56 +++++++++++++-------------------\n t/helper/test-tool.c             |  2 ++\n t/helper/test-tool.h             |  2 ++\n t/perf/p0002-read-cache.sh       |  2 +-\n t/t7519-status-fsmonitor.sh      |  2 +-\n 8 files changed, 83 insertions(+), 35 deletions(-)\n create mode 100644 t/helper/test-read-cache-again.c\n create mode 100644 t/helper/test-read-cache-perf.c\n\ndiff --git a/Makefile b/Makefile\nindex 9573190f1d7..4d3f8ece74f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -739,6 +739,8 @@ TEST_BUILTINS_OBJS += test-prio-queue.o\n TEST_BUILTINS_OBJS += test-proc-receive.o\n TEST_BUILTINS_OBJS += test-progress.o\n TEST_BUILTINS_OBJS += test-reach.o\n+TEST_BUILTINS_OBJS += test-read-cache-again.o\n+TEST_BUILTINS_OBJS += test-read-cache-perf.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-read-graph.o\n TEST_BUILTINS_OBJS += test-read-midx.o\ndiff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c\nnew file mode 100644\nindex 00000000000..707db036cb1\n--- /dev/null\n+++ b/t/helper/test-read-cache-again.c\n@@ -0,0 +1,31 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+\n+int cmd__read_cache_again(int argc, const char **argv)\n+{\n+\tstruct repository *r = the_repository;\n+\tint i, cnt;\n+\tconst char *name;\n+\n+\tif (argc != 2)\n+\t\tdie(\"usage: test-tool read-cache-again <count> <file>\");\n+\n+\tcnt = strtol(argv[0], NULL, 0);\n+\tname = argv[2];\n+\n+\tsetup_git_directory();\n+\tfor (i = 0; i < cnt; i++) {\n+\t\tint pos;\n+\t\trepo_read_index(r);\n+\t\trefresh_index(r->index, REFRESH_QUIET,\n+\t\t\t      NULL, NULL, NULL);\n+\t\tpos = index_name_pos(r->index, name, strlen(name));\n+\t\tif (pos < 0)\n+\t\t\tdie(\"%s not in index\", name);\n+\t\tprintf(\"%s is%s up to date\\n\", name,\n+\t\t       ce_uptodate(r->index->cache[pos]) ? \"\" : \" not\");\n+\t\twrite_file(name, \"%d\\n\", cnt);\n+\t\tdiscard_index(r->index);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c\nnew file mode 100644\nindex 00000000000..90176c010a1\n--- /dev/null\n+++ b/t/helper/test-read-cache-perf.c\n@@ -0,0 +1,21 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+\n+int cmd__read_cache_perf(int argc, const char **argv)\n+{\n+\tstruct repository *r = the_repository;\n+\tint i, cnt = 1;\n+\n+\tif (argc == 2)\n+\t\tcnt = strtol(argv[1], NULL, 0);\n+\telse\n+\t\tdie(\"usage: test-tool read-cache-perf [<count>]\");\n+\n+\tsetup_git_directory();\n+\tfor (i = 0; i < cnt; i++) {\n+\t\trepo_read_index(r);\n+\t\tdiscard_index(r->index);\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\nindex b52c174acc7..616894072d1 100644\n--- a/t/helper/test-read-cache.c\n+++ b/t/helper/test-read-cache.c\n@@ -5,6 +5,12 @@\n #include \"commit.h\"\n #include \"tree.h\"\n #include \"sparse-index.h\"\n+#include \"parse-options.h\"\n+\n+static const char *read_cache_usage[] = {\n+\t\"test-tool read-cache [<options>...]\",\n+\tNULL\n+};\n \n static void print_cache_entry(struct cache_entry *ce)\n {\n@@ -34,49 +40,33 @@ static void print_cache(struct index_state *istate)\n int cmd__read_cache(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n-\tint i, cnt = 1;\n-\tconst char *name = NULL;\n \tint table = 0, expand = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"table\", &table,\n+\t\t\t \"print a dump of the cache\"),\n+\t\tOPT_BOOL(0, \"expand\", &expand,\n+\t\t\t \"call ensure_full_index()\"),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, \"test-tools\", options, read_cache_usage, 0);\n+\tif (argc)\n+\t\tusage_msg_opt(\"Too many arguments.\", read_cache_usage, options);\n \n \tinitialize_the_repository();\n \tprepare_repo_settings(r);\n \tr->settings.command_requires_full_index = 0;\n \n-\tfor (++argv, --argc; *argv && starts_with(*argv, \"--\"); ++argv, --argc) {\n-\t\tif (skip_prefix(*argv, \"--print-and-refresh=\", &name))\n-\t\t\tcontinue;\n-\t\tif (!strcmp(*argv, \"--table\"))\n-\t\t\ttable = 1;\n-\t\telse if (!strcmp(*argv, \"--expand\"))\n-\t\t\texpand = 1;\n-\t}\n-\n-\tif (argc == 1)\n-\t\tcnt = strtol(argv[0], NULL, 0);\n \tsetup_git_directory();\n \tgit_config(git_default_config, NULL);\n+\trepo_read_index(r);\n \n-\tfor (i = 0; i < cnt; i++) {\n-\t\trepo_read_index(r);\n-\n-\t\tif (expand)\n-\t\t\tensure_full_index(r->index);\n+\tif (expand)\n+\t\tensure_full_index(r->index);\n \n-\t\tif (name) {\n-\t\t\tint pos;\n+\tif (table)\n+\t\tprint_cache(r->index);\n+\tdiscard_index(r->index);\n \n-\t\t\trefresh_index(r->index, REFRESH_QUIET,\n-\t\t\t\t      NULL, NULL, NULL);\n-\t\t\tpos = index_name_pos(r->index, name, strlen(name));\n-\t\t\tif (pos < 0)\n-\t\t\t\tdie(\"%s not in index\", name);\n-\t\t\tprintf(\"%s is%s up to date\\n\", name,\n-\t\t\t       ce_uptodate(r->index->cache[pos]) ? \"\" : \" not\");\n-\t\t\twrite_file(name, \"%d\\n\", i);\n-\t\t}\n-\t\tif (table)\n-\t\t\tprint_cache(r->index);\n-\t\tdiscard_index(r->index);\n-\t}\n \treturn 0;\n }\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 3ce5585e53a..fb537c158e9 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -57,6 +57,8 @@ static struct test_cmd cmds[] = {\n \t{ \"progress\", cmd__progress },\n \t{ \"reach\", cmd__reach },\n \t{ \"read-cache\", cmd__read_cache },\n+\t{ \"read-cache-again\", cmd__read_cache_again },\n+\t{ \"read-cache-perf\", cmd__read_cache_perf },\n \t{ \"read-graph\", cmd__read_graph },\n \t{ \"read-midx\", cmd__read_midx },\n \t{ \"ref-store\", cmd__ref_store },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 9f0f5228508..e5616e6f7a7 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -46,6 +46,8 @@ int cmd__proc_receive(int argc, const char **argv);\n int cmd__progress(int argc, const char **argv);\n int cmd__reach(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n+int cmd__read_cache_again(int argc, const char **argv);\n+int cmd__read_cache_perf(int argc, const char **argv);\n int cmd__read_graph(int argc, const char **argv);\n int cmd__read_midx(int argc, const char **argv);\n int cmd__ref_store(int argc, const char **argv);\ndiff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh\nindex cdd105a5945..d0ba5173fb1 100755\n--- a/t/perf/p0002-read-cache.sh\n+++ b/t/perf/p0002-read-cache.sh\n@@ -8,7 +8,7 @@ test_perf_default_repo\n \n count=1000\n test_perf \"read_cache/discard_cache $count times\" \"\n-\ttest-tool read-cache $count\n+\ttest-tool read-cache-perf $count\n \"\n \n test_done\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex deea88d4431..7de5fcb1bd7 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -360,7 +360,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n test_expect_success 'discard_index() also discards fsmonitor info' '\n \ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-all\" &&\n \ttest_might_fail git update-index --refresh &&\n-\ttest-tool read-cache --print-and-refresh=tracked 2 >actual &&\n+\ttest-tool read-cache-again 2 tracked >actual &&\n \tprintf \"tracked is%s up to date\\n\" \"\" \" not\" >expect &&\n \ttest_cmp expect actual\n '\n-- \n2.33.0.663.gbaff4edb973\n\n"},{"id":"433517","messageId":"patch-v2-2.4-a68fa4a6355-20210824T091204Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-v2-0.4-00000000000-20210824T091204Z-avarab@gmail.com","subject":"[PATCH v2 2/4] test-tool: migrate read-cache-perf to parse_options()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T09:15:23Z","receivedAt":"2021-08-24T09:15:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the newly added (but then mostly copy/pasted) read-cache-perf\nto use the parse_options() API. This will make things easier as we add\nnew options.\n\nSince we check the \"cnt = < 1\" case now via more idiomatic\npost-parse_options() assertions we can move from the for-loop to a\nwhile-loop and ditch the \"i\" variable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-read-cache-perf.c | 26 ++++++++++++++++++++------\n t/perf/p0002-read-cache.sh      |  2 +-\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c\nindex 90176c010a1..301e98797b4 100644\n--- a/t/helper/test-read-cache-perf.c\n+++ b/t/helper/test-read-cache-perf.c\n@@ -1,18 +1,32 @@\n #include \"test-tool.h\"\n #include \"cache.h\"\n+#include \"parse-options.h\"\n+\n+static const char *read_cache_perf_usage[] = {\n+\t\"test-tool read-cache-perf [<options>...]\",\n+\tNULL\n+};\n \n int cmd__read_cache_perf(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n-\tint i, cnt = 1;\n+\tint cnt = -1;\n+\tstruct option options[] = {\n+\t\tOPT_INTEGER(0, \"count\", &cnt, \"number of passes\"),\n+\t\tOPT_END()\n+\t};\n \n-\tif (argc == 2)\n-\t\tcnt = strtol(argv[1], NULL, 0);\n-\telse\n-\t\tdie(\"usage: test-tool read-cache-perf [<count>]\");\n+\targc = parse_options(argc, argv, \"test-tools\", options,\n+\t\t\t     read_cache_perf_usage, 0);\n+\tif (argc)\n+\t\tusage_msg_opt(\"Too many arguments.\", read_cache_perf_usage,\n+\t\t\t      options);\n+\tif (cnt < 1)\n+\t\tusage_msg_opt(\"Need at least one pass.\", read_cache_perf_usage,\n+\t\t\t      options);\n \n \tsetup_git_directory();\n-\tfor (i = 0; i < cnt; i++) {\n+\twhile (cnt--) {\n \t\trepo_read_index(r);\n \t\tdiscard_index(r->index);\n \t}\ndiff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh\nindex d0ba5173fb1..1762b648654 100755\n--- a/t/perf/p0002-read-cache.sh\n+++ b/t/perf/p0002-read-cache.sh\n@@ -8,7 +8,7 @@ test_perf_default_repo\n \n count=1000\n test_perf \"read_cache/discard_cache $count times\" \"\n-\ttest-tool read-cache-perf $count\n+\ttest-tool read-cache-perf --count=$count\n \"\n \n test_done\n-- \n2.33.0.663.gbaff4edb973\n\n"},{"id":"433518","messageId":"patch-v2-3.4-a34e69eaa48-20210824T091204Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-v2-0.4-00000000000-20210824T091204Z-avarab@gmail.com","subject":"[PATCH v2 3/4] test-tool: migrate read-cache-again to parse_options()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T09:15:24Z","receivedAt":"2021-08-24T09:15:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the newly added (but then mostly copy/pasted) read-cache-perf\nto use the parse_options() API. I have no plans to further modify\nread-cache-again, but making these commands consistent has a value in\nand of itself.\n\nSince we check the \"cnt = < 1\" case now via more idiomatic\npost-parse_options() assertions we can move from the for-loop to a\nwhile-loop and ditch the \"i\" variable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-read-cache-again.c | 26 ++++++++++++++++++++------\n t/t7519-status-fsmonitor.sh      |  2 +-\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-read-cache-again.c b/t/helper/test-read-cache-again.c\nindex 707db036cb1..fa456ca229e 100644\n--- a/t/helper/test-read-cache-again.c\n+++ b/t/helper/test-read-cache-again.c\n@@ -1,20 +1,34 @@\n #include \"test-tool.h\"\n #include \"cache.h\"\n+#include \"parse-options.h\"\n+\n+static const char *read_cache_again_usage[] = {\n+\t\"test-tool read-cache-again [<options>...] <file>\",\n+\tNULL\n+};\n \n int cmd__read_cache_again(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n-\tint i, cnt;\n+\tint cnt = 2;\n \tconst char *name;\n+\tstruct option options[] = {\n+\t\tOPT_INTEGER(0, \"count\", &cnt, \"number of passes\"),\n+\t\tOPT_END()\n+\t};\n \n-\tif (argc != 2)\n-\t\tdie(\"usage: test-tool read-cache-again <count> <file>\");\n-\n-\tcnt = strtol(argv[0], NULL, 0);\n+\targc = parse_options(argc, argv, \"test-tools\", options,\n+\t\t\t     read_cache_again_usage, 0);\n+\tif (argc != 1)\n+\t\tusage_msg_opt(\"Too many arguments.\", read_cache_again_usage,\n+\t\t\t      options);\n+\tif (cnt < 1)\n+\t\tusage_msg_opt(\"Need at least one pass.\", read_cache_again_usage,\n+\t\t\t      options);\n \tname = argv[2];\n \n \tsetup_git_directory();\n-\tfor (i = 0; i < cnt; i++) {\n+\twhile (cnt--) {\n \t\tint pos;\n \t\trepo_read_index(r);\n \t\trefresh_index(r->index, REFRESH_QUIET,\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 7de5fcb1bd7..817e642f58e 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -360,7 +360,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n test_expect_success 'discard_index() also discards fsmonitor info' '\n \ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-all\" &&\n \ttest_might_fail git update-index --refresh &&\n-\ttest-tool read-cache-again 2 tracked >actual &&\n+\ttest-tool read-cache-again --count=2 tracked >actual &&\n \tprintf \"tracked is%s up to date\\n\" \"\" \" not\" >expect &&\n \ttest_cmp expect actual\n '\n-- \n2.33.0.663.gbaff4edb973\n\n"},{"id":"433519","messageId":"patch-v2-4.4-e3648bf78c7-20210824T091204Z-avarab@gmail.com","threadId":"55865","inReplyTo":"cover-v2-0.4-00000000000-20210824T091204Z-avarab@gmail.com","subject":"[PATCH v2 4/4] read-cache perf: add a perf test for refresh_index()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T09:15:25Z","receivedAt":"2021-08-24T09:15:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a perf test for the refresh_index() function to compliment the\nexisting read()/discard() in a loop perf test added in\n1ecb5ff141f (read-cache: add simple performance test, 2013-06-09).\n\nSince this test is much slower (around 10x) than the previous\nread()/discard() test let's run it 100 times instead of the 1000 time\nthe first one runs.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-read-cache-perf.c | 12 ++++++++++++\n t/perf/p0002-read-cache.sh      |  5 +++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/t/helper/test-read-cache-perf.c b/t/helper/test-read-cache-perf.c\nindex 301e98797b4..630c126fbf3 100644\n--- a/t/helper/test-read-cache-perf.c\n+++ b/t/helper/test-read-cache-perf.c\n@@ -11,8 +11,11 @@ int cmd__read_cache_perf(int argc, const char **argv)\n {\n \tstruct repository *r = the_repository;\n \tint cnt = -1;\n+\tint refresh = 0;\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"count\", &cnt, \"number of passes\"),\n+\t\tOPT_BOOL(0, \"refresh\", &refresh,\n+\t\t\t \"call refresh_index() in a loop, not read()/discard()\"),\n \t\tOPT_END()\n \t};\n \n@@ -26,10 +29,19 @@ int cmd__read_cache_perf(int argc, const char **argv)\n \t\t\t      options);\n \n \tsetup_git_directory();\n+\tif (refresh)\n+\t\trepo_read_index(r);\n \twhile (cnt--) {\n+\t\tif (refresh) {\n+\t\t\tunsigned int flags = REFRESH_QUIET|REFRESH_PROGRESS;\n+\t\t\trefresh_index(r->index, flags, NULL, NULL, NULL);\n+\t\t\tcontinue;\n+\t\t}\n \t\trepo_read_index(r);\n \t\tdiscard_index(r->index);\n \t}\n+\tif (refresh)\n+\t\tdiscard_index(r->index);\n \n \treturn 0;\n }\ndiff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh\nindex 1762b648654..cbccc5ace95 100755\n--- a/t/perf/p0002-read-cache.sh\n+++ b/t/perf/p0002-read-cache.sh\n@@ -11,4 +11,9 @@ test_perf \"read_cache/discard_cache $count times\" \"\n \ttest-tool read-cache-perf --count=$count\n \"\n \n+count=100\n+test_perf \"refresh_index() $count times\" \"\n+\ttest-tool read-cache-perf --count=$count --refresh\n+\"\n+\n test_done\n-- \n2.33.0.663.gbaff4edb973\n\n"},{"id":"433671","messageId":"YSVwioizjGKVSXQA@nand.local","threadId":"55865","inReplyTo":"cover-v2-0.4-00000000000-20210824T091204Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/4] test-tool: split up \"read-cache\" tool","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-24T22:19:54Z","receivedAt":"2021-08-24T22:19:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 24, 2021 at 11:15:21AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> A re-roll addressing the feedback v1 got, see\n> https://lore.kernel.org/git/cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com\n>\n> I think the gist of that feedback from Emily was that v1 could be\n> understood as aiming to optimize the perf test, but the main reason\n> I'm doing this split up is to make the code easier to read. The\n> changed commit message summarizes both goals.\n>\n> I also changed some nits in the code I spotted myself, e.g. \"argc > 0\"\n> checks to just \"argc\", and a simpler way of providing the \"cnt\"\n> default.\n\nPlaying devil's advocate for a moment: I think that the current\nimplementation is somewhat easier to read as it lives in a single file,\nand makes clear that each option implies different behavior through the\nbody of the loop.\n\nIf we're looking for things to clean up, I do like the conversion to the\nparse-options API instead of reading argv ourselves, but probably\notherwise prefer the code as-is instead of split across many files.\n\nBut I may be in the minority, and there may be others who do find the\nsplit-up version easier to grok.\n\nThanks,\nTaylor\n"},{"id":"433680","messageId":"xmqq35qyl6ks.fsf@gitster.g","threadId":"55865","inReplyTo":"YSVwioizjGKVSXQA@nand.local","subject":"Re: [PATCH v2 0/4] test-tool: split up \"read-cache\" tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T00:18:59Z","receivedAt":"2021-08-25T00:19:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> If we're looking for things to clean up, I do like the conversion to the\n> parse-options API instead of reading argv ourselves, but probably\n> otherwise prefer the code as-is instead of split across many files.\n>\n> But I may be in the minority, and there may be others who do find the\n> split-up version easier to grok.\n\nFWIW, you're not alone.  I too like the use of parse_options, but I\nfail to be enthused by changes to churn the test helper binary.\n"},{"id":"433910","messageId":"878s0nz5q2.fsf@evledraar.gmail.com","threadId":"55865","inReplyTo":"xmqq35qyl6ks.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] test-tool: split up \"read-cache\" tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-27T07:24:12Z","receivedAt":"2021-08-27T07:50:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 24 2021, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n>> If we're looking for things to clean up, I do like the conversion to the\n>> parse-options API instead of reading argv ourselves, but probably\n>> otherwise prefer the code as-is instead of split across many files.\n>>\n>> But I may be in the minority, and there may be others who do find the\n>> split-up version easier to grok.\n>\n> FWIW, you're not alone.  I too like the use of parse_options, but I\n> fail to be enthused by changes to churn the test helper binary.\n\nI think it's an improvement as-is & would like you to pick it up, but I\nunderstand if you disagree.\n\nThis series converts the code to parse_options(), but also changes the\nperf test to not load config, and adds a --refresh option for a new perf\ntest.\n\nI tried to make that readable all in one function, but I think it just\nends up being more of a confusing if/else jungle than the current\nstate.\n\nI don't think I'm the right person to re-roll this in that way & argue\nfor it being an improvement when I don't think it would be.\n"}]}