{"thread":{"id":"53778","subject":"[PATCH 0/3] commit-graph: introduce 'core.useBloomFilters'","startedAt":"2020-06-30T17:17:42Z","lastAt":"2020-08-03T19:02:15Z","messageCount":16,"participants":["Taylor Blau","Eric Sunshine","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"400836","messageId":"cover.1593536481.git.me@ttaylorr.com","threadId":"53778","inReplyTo":null,"subject":"[PATCH 0/3] commit-graph: introduce 'core.useBloomFilters'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-30T17:17:36Z","receivedAt":"2020-06-30T17:17:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere are some patches that we have been using at GitHub to control\nwhether or not Bloom filters stored in commit-graphs are read during\nnormal operation.\n\nWe're planning on using these patches as part of a two-phase roll-out of\nchanged-path Bloom filters, where the first phase conditions whether or\nnot repositories *write* Bloom filters, and the second phase (controlled\nvia the new 'core.useBloomFilters') controls whether repositories *read*\ntheir Bloom filters.\n\nThis can also be handy for debugging purposes, say, for e.g., if Bloom\nfilters are suspected to be corrupt, they can be softly disabled without\ndropping the rest of the data in the commit-graph.\n\nThanks in advance for your review.\n\n-Taylor\n\nTaylor Blau (3):\n  commit-graph: pass a 'struct repository *' in more places\n  t4216: fix broken '&&'-chain\n  commit-graph: respect 'core.useBloomFilters'\n\n Documentation/config/core.txt |  5 +++++\n builtin/commit-graph.c        |  2 +-\n commit-graph.c                | 17 ++++++++++-------\n commit-graph.h                |  4 +++-\n fuzz-commit-graph.c           |  5 +++--\n repo-settings.c               |  3 +++\n repository.h                  |  1 +\n t/helper/test-read-graph.c    |  3 ++-\n t/t4216-log-bloom.sh          |  6 ++++--\n 9 files changed, 32 insertions(+), 14 deletions(-)\n\n--\n2.27.0.224.g4cfa086e50\n"},{"id":"400837","messageId":"4ea9933b50fe0bc2738ab0e0dc52a4f17c4a2cb4.1593536481.git.me@ttaylorr.com","threadId":"53778","inReplyTo":"cover.1593536481.git.me@ttaylorr.com","subject":"[PATCH 1/3] commit-graph: pass a 'struct repository *' in more places","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-30T17:17:41Z","receivedAt":"2020-06-30T17:17:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a future commit, some commit-graph internals will want access to\n'r->settings', but we only have the 'struct object_directory *'\ncorresponding to that repository.\n\nAdd an additional parameter to pass the repository around in more\nplaces. In the next patch, we will remove the object directory (and\ninstead reference it with 'r->odb').\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/commit-graph.c |  2 +-\n commit-graph.c         | 13 ++++++++-----\n commit-graph.h         |  4 +++-\n fuzz-commit-graph.c    |  5 +++--\n 4 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 75455da138..7c0f98531d 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -106,7 +106,7 @@ static int graph_verify(int argc, const char **argv)\n \tFREE_AND_NULL(graph_name);\n \n \tif (open_ok)\n-\t\tgraph = load_commit_graph_one_fd_st(fd, &st, odb);\n+\t\tgraph = load_commit_graph_one_fd_st(fd, &st, the_repository, odb);\n \telse\n \t\tgraph = read_commit_graph_one(the_repository, odb);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2ff042fbf4..fdfb0888f0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -170,6 +170,7 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n }\n \n struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n+\t\t\t\t\t\t struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb)\n {\n \tvoid *graph_map;\n@@ -185,7 +186,7 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tret = parse_commit_graph(graph_map, graph_size);\n+\tret = parse_commit_graph(r, graph_map, graph_size);\n \n \tif (ret)\n \t\tret->odb = odb;\n@@ -225,7 +226,8 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \treturn 0;\n }\n \n-struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n+struct commit_graph *parse_commit_graph(struct repository *r,\n+\t\t\t\t\tvoid *graph_map, size_t graph_size)\n {\n \tconst unsigned char *data, *chunk_lookup;\n \tuint32_t i;\n@@ -396,6 +398,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n }\n \n static struct commit_graph *load_commit_graph_one(const char *graph_file,\n+\t\t\t\t\t\t  struct repository *r,\n \t\t\t\t\t\t  struct object_directory *odb)\n {\n \n@@ -407,7 +410,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file,\n \tif (!open_ok)\n \t\treturn NULL;\n \n-\tg = load_commit_graph_one_fd_st(fd, &st, odb);\n+\tg = load_commit_graph_one_fd_st(fd, &st, r, odb);\n \n \tif (g)\n \t\tg->filename = xstrdup(graph_file);\n@@ -419,7 +422,7 @@ static struct commit_graph *load_commit_graph_v1(struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb)\n {\n \tchar *graph_name = get_commit_graph_filename(odb);\n-\tstruct commit_graph *g = load_commit_graph_one(graph_name, odb);\n+\tstruct commit_graph *g = load_commit_graph_one(graph_name, r, odb);\n \tfree(graph_name);\n \n \treturn g;\n@@ -500,7 +503,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \t\tvalid = 0;\n \t\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n \t\t\tchar *graph_name = get_split_graph_filename(odb, line.buf);\n-\t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name, odb);\n+\t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name, r, odb);\n \n \t\t\tfree(graph_name);\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 3ba0da1e5f..03d848e168 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -76,10 +76,12 @@ struct commit_graph {\n };\n \n struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n+\t\t\t\t\t\t struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n-struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);\n+struct commit_graph *parse_commit_graph(struct repository *r,\n+\t\t\t\t\tvoid *graph_map, size_t graph_size);\n \n /*\n  * Return 1 if and only if the repository has a commit-graph\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nindex 430817214d..e7cf6d5b0f 100644\n--- a/fuzz-commit-graph.c\n+++ b/fuzz-commit-graph.c\n@@ -1,7 +1,8 @@\n #include \"commit-graph.h\"\n #include \"repository.h\"\n \n-struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);\n+struct commit_graph *parse_commit_graph(struct repository *r,\n+\t\t\t\t\tvoid *graph_map, size_t graph_size);\n \n int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n \n@@ -10,7 +11,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tstruct commit_graph *g;\n \n \tinitialize_the_repository();\n-\tg = parse_commit_graph((void *)data, size);\n+\tg = parse_commit_graph(the_repository, (void *)data, size);\n \trepo_clear(the_repository);\n \tfree_commit_graph(g);\n \n-- \n2.27.0.224.g4cfa086e50\n\n"},{"id":"400838","messageId":"5a20a97658fa8e6c874c9c9cafb2cf49e39f94d6.1593536481.git.me@ttaylorr.com","threadId":"53778","inReplyTo":"cover.1593536481.git.me@ttaylorr.com","subject":"[PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-30T17:17:44Z","receivedAt":"2020-06-30T17:17:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a759bfa9ee (t4216: add end to end tests for git log with Bloom\nfilters, 2020-04-06), a 'rm' invocation was added without a\ncorresponding '&&' chain.\n\nThis ends up working fine when the file already exists, in which case\n'rm' exits cleanly and the rest of the function executes normally. When\nthe file does _not_ exist, however, 'rm' returns an unclean exit code,\ncausing the function to terminate.\n\nFix this by making the test use an '&&'-chain, and passing '-f' to\nignore missing files (as can be the case when specifying which tests are\n'--run').\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t4216-log-bloom.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex c855bcd3e7..0b4cc4f8d1 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF\n sane_unset GIT_TRACE2_CONFIG_PARAMS\n \n setup () {\n-\trm \"$TRASH_DIRECTORY/trace.perf\"\n+\trm -f \"$TRASH_DIRECTORY/trace.perf\" &&\n \tgit -c core.commitGraph=false log --pretty=\"format:%s\" $1 >log_wo_bloom &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.perf\" git -c core.commitGraph=true log --pretty=\"format:%s\" $1 >log_w_bloom\n }\n-- \n2.27.0.224.g4cfa086e50\n\n"},{"id":"400839","messageId":"4cfa086e503e19763a9d581fcb6a2ef818776dfc.1593536481.git.me@ttaylorr.com","threadId":"53778","inReplyTo":"cover.1593536481.git.me@ttaylorr.com","subject":"[PATCH 3/3] commit-graph: respect 'core.useBloomFilters'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-30T17:17:48Z","receivedAt":"2020-06-30T17:17:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Git uses the 'core.commitGraph' configuration value to control whether\nor not the commit graph is used when parsing commits or performing a\ntraversal.\n\nNow that commit-graphs can also contain a section for changed-path Bloom\nfilters, administrators that already have commit-graphs may find it\nconvenient to use those graphs without relying on their changed-path\nBloom filters. This can happen, for example, during a staged roll-out,\nor in the event of an incident.\n\nIntroduce 'core.useBloomFilters' to control whether or not Bloom filters\nare read. Note that this configuration is independent from both:\n\n  - 'core.commitGraph', to allow flexibility in using all parts of a\n    commit-graph _except_ for its Bloom filters.\n\n  - The '--changed-paths' option for 'git commit-graph write', to allow\n    reading and writing Bloom filters to be controlled independently.\n\nWhen the variable is set, pretend as if no Bloom data was specified at\nall. This avoids adding additional special-casing outside of the\ncommit-graph internals.\n\nSuggested-by: Derrick Stolee <dstolee@microsoft.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/core.txt | 5 +++++\n commit-graph.c                | 4 ++--\n repo-settings.c               | 3 +++\n repository.h                  | 1 +\n t/helper/test-read-graph.c    | 3 ++-\n t/t4216-log-bloom.sh          | 4 +++-\n 6 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 74619a9c03..b146bf8d34 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -599,6 +599,11 @@ core.commitGraph::\n \tto parse the graph structure of commits. Defaults to true. See\n \tlinkgit:git-commit-graph[1] for more information.\n \n+core.useBloomFilters::\n+\tIf true, then git will use the changed-path Bloom filters in the\n+\tcommit-graph file (if it exists, and they are present). Defaults to\n+\ttrue. See linkgit:git-commit-graph[1] for more information.\n+\n core.useReplaceRefs::\n \tIf set to `false`, behave as if the `--no-replace-objects`\n \toption was given on the command line. See linkgit:git[1] and\ndiff --git a/commit-graph.c b/commit-graph.c\nindex fdfb0888f0..03c00415c4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -337,14 +337,14 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \t\tcase GRAPH_CHUNKID_BLOOMINDEXES:\n \t\t\tif (graph->chunk_bloom_indexes)\n \t\t\t\tchunk_repeated = 1;\n-\t\t\telse\n+\t\t\telse if (r->settings.core_use_bloom_filters)\n \t\t\t\tgraph->chunk_bloom_indexes = data + chunk_offset;\n \t\t\tbreak;\n \n \t\tcase GRAPH_CHUNKID_BLOOMDATA:\n \t\t\tif (graph->chunk_bloom_data)\n \t\t\t\tchunk_repeated = 1;\n-\t\t\telse {\n+\t\t\telse if (r->settings.core_use_bloom_filters) {\n \t\t\t\tuint32_t hash_version;\n \t\t\t\tgraph->chunk_bloom_data = data + chunk_offset;\n \t\t\t\thash_version = get_be32(data + chunk_offset);\ndiff --git a/repo-settings.c b/repo-settings.c\nindex dc6817daa9..d8e3b1c61e 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -17,9 +17,12 @@ void prepare_repo_settings(struct repository *r)\n \n \tif (!repo_config_get_bool(r, \"core.commitgraph\", &value))\n \t\tr->settings.core_commit_graph = value;\n+\tif (!repo_config_get_bool(r, \"core.usebloomfilters\", &value))\n+\t\tr->settings.core_use_bloom_filters = value;\n \tif (!repo_config_get_bool(r, \"gc.writecommitgraph\", &value))\n \t\tr->settings.gc_write_commit_graph = value;\n \tUPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);\n+\tUPDATE_DEFAULT_BOOL(r->settings.core_use_bloom_filters, 1);\n \tUPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);\n \n \tif (!repo_config_get_int(r, \"index.version\", &value))\ndiff --git a/repository.h b/repository.h\nindex 3c1f7d54bd..cc61533122 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -29,6 +29,7 @@ struct repo_settings {\n \tint initialized;\n \n \tint core_commit_graph;\n+\tint core_use_bloom_filters;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n \ndiff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\nindex 6d0c962438..5f585a1725 100644\n--- a/t/helper/test-read-graph.c\n+++ b/t/helper/test-read-graph.c\n@@ -12,11 +12,12 @@ int cmd__read_graph(int argc, const char **argv)\n \tsetup_git_directory();\n \todb = the_repository->objects->odb;\n \n+\tprepare_repo_settings(the_repository);\n+\n \tgraph = read_commit_graph_one(the_repository, odb);\n \tif (!graph)\n \t\treturn 1;\n \n-\n \tprintf(\"header: %08x %d %d %d %d\\n\",\n \t\tntohl(*(uint32_t*)graph->data),\n \t\t*(unsigned char*)(graph->data + 4),\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex 0b4cc4f8d1..b1a247477e 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -90,7 +90,9 @@ do\n \t\t      \"--ancestry-path side..master\"\n \tdo\n \t\ttest_expect_success \"git log option: $option for path: $path\" '\n-\t\t\ttest_bloom_filters_used \"$option -- $path\"\n+\t\t\ttest_bloom_filters_used \"$option -- $path\" &&\n+\t\t\ttest_config core.useBloomFilters false &&\n+\t\t\ttest_bloom_filters_not_used \"$option -- $path\"\n \t\t'\n \tdone\n done\n-- \n2.27.0.224.g4cfa086e50\n"},{"id":"400840","messageId":"CAPig+cQv3cHe5ci3mDvNKYXbVQt6Rp5icG-woaDCqfAtzZ6SZw@mail.gmail.com","threadId":"53778","inReplyTo":"5a20a97658fa8e6c874c9c9cafb2cf49e39f94d6.1593536481.git.me@ttaylorr.com","subject":"Re: [PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-30T17:50:22Z","receivedAt":"2020-06-30T17:50:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2020 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote:\n> In a759bfa9ee (t4216: add end to end tests for git log with Bloom\n> filters, 2020-04-06), a 'rm' invocation was added without a\n> corresponding '&&' chain.\n>\n> This ends up working fine when the file already exists, in which case\n> 'rm' exits cleanly and the rest of the function executes normally. When\n> the file does _not_ exist, however, 'rm' returns an unclean exit code,\n> causing the function to terminate.\n\nThis explanation makes no sense. Since this command was not part of\nthe &&-chain, its failure would not cause the function to terminate\nprematurely nor would it affect the return value of the function. This\nexplanation would make sense, however, if you're talking about the\nbehavior _after_ fixing the broken &&-chain.\n\n> Fix this by making the test use an '&&'-chain, and passing '-f' to\n> ignore missing files (as can be the case when specifying which tests are\n> '--run').\n\nThe entire commit message is talking about implementation details and\nmerely repeats what the subject and patch itself already say quite\nclearly; anyone familiar with 'rm' understands implicitly that '-f'\nmust be added to incorporate it into the &&-chain if the file's\npresence is not guaranteed. Thus, you could drop the entire body of\nthe commit message without losing clarity...\n\nWith one minor exception: What is much more interesting for the reader\nto know is whether the file being removed is guaranteed to exist (in\nwhich case '-f' is unnecessary) or may be missing (requiring '-f'),\nand under what conditions it might be missing. The very last part of\nthe last sentence of the current commit message gives a good hint\nabout the latter, thus would be a good bit to retain.\n\n> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\n> @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF\n>  sane_unset GIT_TRACE2_CONFIG_PARAMS\n\nNot related to this patch, but 'sane_unset' is pointless outside of a\ntest since there is no &&-chain to maintain. Plain 'unset' would work\njust as well and be less misleading.\n\n>  setup () {\n> -       rm \"$TRASH_DIRECTORY/trace.perf\"\n> +       rm -f \"$TRASH_DIRECTORY/trace.perf\" &&\n"},{"id":"400841","messageId":"20200630183928.GB26550@syl.lan","threadId":"53778","inReplyTo":"CAPig+cQv3cHe5ci3mDvNKYXbVQt6Rp5icG-woaDCqfAtzZ6SZw@mail.gmail.com","subject":"Re: [PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-30T18:39:28Z","receivedAt":"2020-06-30T18:39:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 30, 2020 at 01:50:22PM -0400, Eric Sunshine wrote:\n> On Tue, Jun 30, 2020 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote:\n> > In a759bfa9ee (t4216: add end to end tests for git log with Bloom\n> > filters, 2020-04-06), a 'rm' invocation was added without a\n> > corresponding '&&' chain.\n> >\n> > This ends up working fine when the file already exists, in which case\n> > 'rm' exits cleanly and the rest of the function executes normally. When\n> > the file does _not_ exist, however, 'rm' returns an unclean exit code,\n> > causing the function to terminate.\n>\n> This explanation makes no sense. Since this command was not part of\n> the &&-chain, its failure would not cause the function to terminate\n> prematurely nor would it affect the return value of the function. This\n> explanation would make sense, however, if you're talking about the\n> behavior _after_ fixing the broken &&-chain.\n\nFair enough. For what it's worth, this explanation *does* make sense if\nyou 'set -e' beforehand, which I am accustomed to (and had incorrectly\nassumed that tests in 't' also have 'set -e', when they do not).\n\nI've corrected the patch and shortened it to account for your\nsuggestions. Mind taking a look at the updated version and telling me\nwhat you think?\n\n--- >8 ---\n\nSubject: [PATCH] t4216: fix broken '&&'-chain\n\nThe 'rm' added in a759bfa9ee (t4216: add end to end tests for git log\nwith Bloom filters, 2020-04-06) should be placed within the function's\n'&&'-chain.\n\nThe file being removed may not exist (for eg., in the case of '--run',\nin which case it may not be generated beforehand by a skipped test), and\nso add '-f' to account for the file's optional existence.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t4216-log-bloom.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex c855bcd3e7..0b4cc4f8d1 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF\n sane_unset GIT_TRACE2_CONFIG_PARAMS\n\n setup () {\n-\trm \"$TRASH_DIRECTORY/trace.perf\"\n+\trm -f \"$TRASH_DIRECTORY/trace.perf\" &&\n \tgit -c core.commitGraph=false log --pretty=\"format:%s\" $1 >log_wo_bloom &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.perf\" git -c core.commitGraph=true log --pretty=\"format:%s\" $1 >log_w_bloom\n }\n--\n2.27.0.224.g4cfa086e50\n\n"},{"id":"400842","messageId":"20200630185536.GA1888406@coredump.intra.peff.net","threadId":"53778","inReplyTo":"CAPig+cQv3cHe5ci3mDvNKYXbVQt6Rp5icG-woaDCqfAtzZ6SZw@mail.gmail.com","subject":"Re: [PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-30T18:55:36Z","receivedAt":"2020-06-30T18:55:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2020 at 01:50:22PM -0400, Eric Sunshine wrote:\n\n> > diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\n> > @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF\n> >  sane_unset GIT_TRACE2_CONFIG_PARAMS\n> \n> Not related to this patch, but 'sane_unset' is pointless outside of a\n> test since there is no &&-chain to maintain. Plain 'unset' would work\n> just as well and be less misleading.\n\nWe do this in lots of other places, too. Try:\n\n  git grep sane_unset | grep -v '&&'\n\nThough seeing the variables they cover, I think many of them may come\nfrom the same few authors. :)\n\nI wonder if it is worth keeping these as sane_unset, though. Your\ncomment is based on the knowledge that the \"sane\" part of the function\nis ignoring the return value. But we could conceivably have other\nportability fixes (it's not impossible that a shell wants one \"unset\"\nper variable, for example), in which case we'd want it in more places.\n\nThat's hypothetical, of course, but saying \"just use our portable unset\nwrapper that behaves sanely\" seems like exactly what these call sites\nwant, now and in a hypothetical future.\n\n-Peff\n"},{"id":"400843","messageId":"20200630190325.GB1888406@coredump.intra.peff.net","threadId":"53778","inReplyTo":"20200630183928.GB26550@syl.lan","subject":"Re: [PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-30T19:03:25Z","receivedAt":"2020-06-30T19:03:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2020 at 02:39:28PM -0400, Taylor Blau wrote:\n\n> > > This ends up working fine when the file already exists, in which case\n> > > 'rm' exits cleanly and the rest of the function executes normally. When\n> > > the file does _not_ exist, however, 'rm' returns an unclean exit code,\n> > > causing the function to terminate.\n> >\n> > This explanation makes no sense. Since this command was not part of\n> > the &&-chain, its failure would not cause the function to terminate\n> > prematurely nor would it affect the return value of the function. This\n> > explanation would make sense, however, if you're talking about the\n> > behavior _after_ fixing the broken &&-chain.\n> \n> Fair enough. For what it's worth, this explanation *does* make sense if\n> you 'set -e' beforehand, which I am accustomed to (and had incorrectly\n> assumed that tests in 't' also have 'set -e', when they do not).\n\nIf we _really_ want to nitpick, it probably wouldn't terminate under\n\"set -e\" because the call to \"setup\" is itself part of an &&-chain,\nwhich suppresses \"-e\" handling (which is one of the many confusing \"set\n-e\" behaviors that led us to avoid it in the first place).\n\nBut definitely your revised commit message below is more accurate.\n\nHowever...\n\n> --- >8 ---\n> \n> Subject: [PATCH] t4216: fix broken '&&'-chain\n> \n> The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log\n> with Bloom filters, 2020-04-06) should be placed within the function's\n> '&&'-chain.\n> \n> The file being removed may not exist (for eg., in the case of '--run',\n> in which case it may not be generated beforehand by a skipped test), and\n> so add '-f' to account for the file's optional existence.\n\nIs the &&-chain really broken, or is the first command simply not part\nof that chain? Perhaps a question for philosophers, but the more applied\nquestion here is: what are we improving, and why?\n\nThe original code handled the fact that the file might not exist by not\nincluding its exit code in the &&-chain which leads to the function's\nreturn value. Your new code does so by putting it in the &&-chain but\nasking \"rm\" to ignore errors. Is one better than the other?\n\nI think so, but my argument would be more along the lines of:\n\n  - without \"-f\", \"rm\" will complain about a missing file, which is\n    distracting noise in the test log\n\n  - once \"-f\" is added in to suppress that, we might as well add the\n    command to the &&-chain. That's our normal style, so readers don't\n    have to wonder if it's important or not. Plus it would help avoid a\n    broken chain if more commands are added at the beginning of the\n    function.\n\n-Peff\n"},{"id":"400844","messageId":"20200630191231.GA30767@syl.lan","threadId":"53778","inReplyTo":"20200630190325.GB1888406@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-30T19:12:31Z","receivedAt":"2020-06-30T19:12:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 30, 2020 at 03:03:25PM -0400, Jeff King wrote:\n> On Tue, Jun 30, 2020 at 02:39:28PM -0400, Taylor Blau wrote:\n>\n> > > > This ends up working fine when the file already exists, in which case\n> > > > 'rm' exits cleanly and the rest of the function executes normally. When\n> > > > the file does _not_ exist, however, 'rm' returns an unclean exit code,\n> > > > causing the function to terminate.\n> > >\n> > > This explanation makes no sense. Since this command was not part of\n> > > the &&-chain, its failure would not cause the function to terminate\n> > > prematurely nor would it affect the return value of the function. This\n> > > explanation would make sense, however, if you're talking about the\n> > > behavior _after_ fixing the broken &&-chain.\n> >\n> > Fair enough. For what it's worth, this explanation *does* make sense if\n> > you 'set -e' beforehand, which I am accustomed to (and had incorrectly\n> > assumed that tests in 't' also have 'set -e', when they do not).\n>\n> If we _really_ want to nitpick, it probably wouldn't terminate under\n> \"set -e\" because the call to \"setup\" is itself part of an &&-chain,\n> which suppresses \"-e\" handling (which is one of the many confusing \"set\n> -e\" behaviors that led us to avoid it in the first place).\n\nI learned something new about 'set -e'! I don't mind nitpicking at all,\nit's useful information to know...\n\n> But definitely your revised commit message below is more accurate.\n>\n> However...\n>\n> > --- >8 ---\n> >\n> > Subject: [PATCH] t4216: fix broken '&&'-chain\n> >\n> > The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log\n> > with Bloom filters, 2020-04-06) should be placed within the function's\n> > '&&'-chain.\n> >\n> > The file being removed may not exist (for eg., in the case of '--run',\n> > in which case it may not be generated beforehand by a skipped test), and\n> > so add '-f' to account for the file's optional existence.\n>\n> Is the &&-chain really broken, or is the first command simply not part\n> of that chain? Perhaps a question for philosophers, but the more applied\n> question here is: what are we improving, and why?\n>\n> The original code handled the fact that the file might not exist by not\n> including its exit code in the &&-chain which leads to the function's\n> return value. Your new code does so by putting it in the &&-chain but\n> asking \"rm\" to ignore errors. Is one better than the other?\n>\n> I think so, but my argument would be more along the lines of:\n>\n>   - without \"-f\", \"rm\" will complain about a missing file, which is\n>     distracting noise in the test log\n>\n>   - once \"-f\" is added in to suppress that, we might as well add the\n>     command to the &&-chain. That's our normal style, so readers don't\n>     have to wonder if it's important or not. Plus it would help avoid a\n>     broken chain if more commands are added at the beginning of the\n>     function.\n\nI made the change for basically these reasons, but mostly to bring this\nfunction into good style as with the rest of our test suite (there are a\nhandful of other minor nits that we could look at, such as some odd\nspacing, etc.).\n\nWhether or not all of this needs to go into the commit message... I\ndon't know. On the one hand, I think that your explanation here is\nclearer than what I wrote in the commit message, but on the other hand,\nI think that amending it again may be belaboring an otherwise simple\nchange.\n\nIf you feel strongly, though, I'm happy to send a revised patch.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"400846","messageId":"20200630191834.GC1888406@coredump.intra.peff.net","threadId":"53778","inReplyTo":"4cfa086e503e19763a9d581fcb6a2ef818776dfc.1593536481.git.me@ttaylorr.com","subject":"Re: [PATCH 3/3] commit-graph: respect 'core.useBloomFilters'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-30T19:18:34Z","receivedAt":"2020-06-30T19:18:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2020 at 01:17:48PM -0400, Taylor Blau wrote:\n\n> Git uses the 'core.commitGraph' configuration value to control whether\n> or not the commit graph is used when parsing commits or performing a\n> traversal.\n\nI think this is a good thing to have, and the patch itself makes sense\nto me (this is actually my first time reviewing it, despite its intended\nuse within GitHub :) ).\n\nIf I may bikeshed for a moment:\n\n> Introduce 'core.useBloomFilters' to control whether or not Bloom filters\n> are read. Note that this configuration is independent from both:\n> \n>   - 'core.commitGraph', to allow flexibility in using all parts of a\n>     commit-graph _except_ for its Bloom filters.\n> \n>   - The '--changed-paths' option for 'git commit-graph write', to allow\n>     reading and writing Bloom filters to be controlled independently.\n\nShould we avoid exposing the user to the words \"Bloom filter\"?\n\nThe command-line option for writing them was genericized to\n\"changed-paths\", which I think is good. The use of Bloom filters is an\nimplementation detail. What the user cares about is whether we can\noptimize queries of which paths changed in a commit.\n\nWhen we introduced reachability bitmaps long ago, we made the mistake of\njust calling them \"bitmaps\". That jargon is well understood by people\nwho work with that code, but it's confusing outside of that (even within\nother parts of Git) because bitmaps are just a generic data structure.\nYou can have a bitmap of just about anything (and indeed we do use other\nbitmaps these days). Consistently calling them \"reachability bitmaps\",\nespecially in the user facing bits, would have reduced confusion over\nthe years.\n\nSimilarly, Bloom filters are a generic structure we might use elsewhere.\nI don't really care if we use the word \"Bloom\" internally to refer to\nthis feature, but we'll be stuck with this config option for all time. I\nthink it's worth picking something more clear.\n\nIt might even be worth considering whether \"changed paths\" needs more\ncontext (or would if we add new features in the future). On a \"git\ncommit-graph write\" command-line it is perfectly clear, but would\ncore.commitGraphChangedPaths be worth it? It's definitely more specific,\nbut it's also way more ugly. ;)\n\n> diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\n> index 6d0c962438..5f585a1725 100644\n> --- a/t/helper/test-read-graph.c\n> +++ b/t/helper/test-read-graph.c\n> @@ -12,11 +12,12 @@ int cmd__read_graph(int argc, const char **argv)\n>  \tsetup_git_directory();\n>  \todb = the_repository->objects->odb;\n>  \n> +\tprepare_repo_settings(the_repository);\n> +\n>  \tgraph = read_commit_graph_one(the_repository, odb);\n\nI wondered why we would need this prepare_repo_settings() now, when it\nshould have been needed already to cover core.commitGraph already. I\nstrongly suspect the answer is: \"test-tool read-graph\" never properly\nrespected core.commitGraph in the first place.\n\nAnd now presumably it would. If true, I don't think any tests need\nadjusted because the only places we set it are:\n\n  - on a \"git -c\" command line, which wouldn't run a test-tool helper\n\n  - when we do set it, it is always to \"true\", which is the default\n    anyway\n\n>  \tif (!graph)\n>  \t\treturn 1;\n>  \n> -\n>  \tprintf(\"header: %08x %d %d %d %d\\n\",\n>  \t\tntohl(*(uint32_t*)graph->data),\n>  \t\t*(unsigned char*)(graph->data + 4),\n\nOh good, I happened to be looking at this code earlier today for an\nunrelated reason and was bothered by this extra newline. :)\n\n-Peff\n"},{"id":"400847","messageId":"20200630191941.GD1888406@coredump.intra.peff.net","threadId":"53778","inReplyTo":"20200630191231.GA30767@syl.lan","subject":"Re: [PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-30T19:19:41Z","receivedAt":"2020-06-30T19:19:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2020 at 03:12:31PM -0400, Taylor Blau wrote:\n\n> > I think so, but my argument would be more along the lines of:\n> >\n> >   - without \"-f\", \"rm\" will complain about a missing file, which is\n> >     distracting noise in the test log\n> >\n> >   - once \"-f\" is added in to suppress that, we might as well add the\n> >     command to the &&-chain. That's our normal style, so readers don't\n> >     have to wonder if it's important or not. Plus it would help avoid a\n> >     broken chain if more commands are added at the beginning of the\n> >     function.\n> \n> I made the change for basically these reasons, but mostly to bring this\n> function into good style as with the rest of our test suite (there are a\n> handful of other minor nits that we could look at, such as some odd\n> spacing, etc.).\n> \n> Whether or not all of this needs to go into the commit message... I\n> don't know. On the one hand, I think that your explanation here is\n> clearer than what I wrote in the commit message, but on the other hand,\n> I think that amending it again may be belaboring an otherwise simple\n> change.\n> \n> If you feel strongly, though, I'm happy to send a revised patch.\n\nI agree it's a pretty trivial patch, but I think if it's worth applying\nat all, then it's worth justifying it appropriately.\n\n-Peff\n"},{"id":"400848","messageId":"20200630192718.GA31522@syl.lan","threadId":"53778","inReplyTo":"20200630191834.GC1888406@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph: respect 'core.useBloomFilters'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-30T19:27:18Z","receivedAt":"2020-06-30T19:27:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 30, 2020 at 03:18:34PM -0400, Jeff King wrote:\n> On Tue, Jun 30, 2020 at 01:17:48PM -0400, Taylor Blau wrote:\n>\n> > Git uses the 'core.commitGraph' configuration value to control whether\n> > or not the commit graph is used when parsing commits or performing a\n> > traversal.\n>\n> I think this is a good thing to have, and the patch itself makes sense\n> to me (this is actually my first time reviewing it, despite its intended\n> use within GitHub :) ).\n>\n> If I may bikeshed for a moment:\n>\n> > Introduce 'core.useBloomFilters' to control whether or not Bloom filters\n> > are read. Note that this configuration is independent from both:\n> >\n> >   - 'core.commitGraph', to allow flexibility in using all parts of a\n> >     commit-graph _except_ for its Bloom filters.\n> >\n> >   - The '--changed-paths' option for 'git commit-graph write', to allow\n> >     reading and writing Bloom filters to be controlled independently.\n>\n> Should we avoid exposing the user to the words \"Bloom filter\"?\n>\n> The command-line option for writing them was genericized to\n> \"changed-paths\", which I think is good. The use of Bloom filters is an\n> implementation detail. What the user cares about is whether we can\n> optimize queries of which paths changed in a commit.\n>\n> When we introduced reachability bitmaps long ago, we made the mistake of\n> just calling them \"bitmaps\". That jargon is well understood by people\n> who work with that code, but it's confusing outside of that (even within\n> other parts of Git) because bitmaps are just a generic data structure.\n> You can have a bitmap of just about anything (and indeed we do use other\n> bitmaps these days). Consistently calling them \"reachability bitmaps\",\n> especially in the user facing bits, would have reduced confusion over\n> the years.\n>\n> Similarly, Bloom filters are a generic structure we might use elsewhere.\n> I don't really care if we use the word \"Bloom\" internally to refer to\n> this feature, but we'll be stuck with this config option for all time. I\n> think it's worth picking something more clear.\n\nAll good thoughts. I wondered about this, too, when writing the patch,\nbut ultimately decided to expose the name since this is the only usage\nof Bloom filters within Git to date. Whether that will continue to be\ntrue, I'm not sure, so it probably isn't a great idea to lock ourselves\ninto that decision within the 'core' namespace.\n\nSo, I'm certainly open to changing it, although I'm not sure that I'm as\nworried about exposing the implementation detail as I am about squatting\non Bloom filters within Git in general. I don't think that this\nconfiguration will end up getting used by folks other than server\nadministrators and for debugging purposes, so those populations are\nalready likely to be aware of changed-path Bloom filters beforehand.\n\nBut, hiding the implementation detail seems like sane advice either way.\n\n> It might even be worth considering whether \"changed paths\" needs more\n> context (or would if we add new features in the future). On a \"git\n> commit-graph write\" command-line it is perfectly clear, but would\n> core.commitGraphChangedPaths be worth it? It's definitely more specific,\n> but it's also way more ugly. ;)\n\nHere's a third option what about 'graph.readChangedPaths'. I think that\nStolee and I discussed a new top-level 'graph' section, since we now\nhave a few commit-graph-related configuration variables in 'core'.\n\nThat's a little shorter, and it adds the verb 'read', which is more\ndescriptive than 'use' (I touch on this in the third patch, where I say\nthat this configuration variable _doesn't_ affect the '--changed-path'\noption when writing).\n\nEither way, I'd love to hear your thoughts and others', too, to figure\nout what we think the most agreeable configuration name is.\n\n> > diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c\n> > index 6d0c962438..5f585a1725 100644\n> > --- a/t/helper/test-read-graph.c\n> > +++ b/t/helper/test-read-graph.c\n> > @@ -12,11 +12,12 @@ int cmd__read_graph(int argc, const char **argv)\n> >  \tsetup_git_directory();\n> >  \todb = the_repository->objects->odb;\n> >\n> > +\tprepare_repo_settings(the_repository);\n> > +\n> >  \tgraph = read_commit_graph_one(the_repository, odb);\n>\n> I wondered why we would need this prepare_repo_settings() now, when it\n> should have been needed already to cover core.commitGraph already. I\n> strongly suspect the answer is: \"test-tool read-graph\" never properly\n> respected core.commitGraph in the first place.\n\nYep. Could probably be broken out into a separate patch (or mentioned as\nan aside in this one), but you're right: this helper did not respect\nany configuration that 'prepare_repo_settings' picks up.\n\n> And now presumably it would. If true, I don't think any tests need\n> adjusted because the only places we set it are:\n>\n>   - on a \"git -c\" command line, which wouldn't run a test-tool helper\n>\n>   - when we do set it, it is always to \"true\", which is the default\n>     anyway\n>\n> >  \tif (!graph)\n> >  \t\treturn 1;\n> >\n> > -\n> >  \tprintf(\"header: %08x %d %d %d %d\\n\",\n> >  \t\tntohl(*(uint32_t*)graph->data),\n> >  \t\t*(unsigned char*)(graph->data + 4),\n>\n> Oh good, I happened to be looking at this code earlier today for an\n> unrelated reason and was bothered by this extra newline. :)\n\nI hoped that nobody would mine me sneaking this in ;-).\n\n>\n> -Peff\nThanks,\nTaylor\n"},{"id":"400849","messageId":"20200630193340.GA1889576@coredump.intra.peff.net","threadId":"53778","inReplyTo":"20200630192718.GA31522@syl.lan","subject":"Re: [PATCH 3/3] commit-graph: respect 'core.useBloomFilters'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-30T19:33:40Z","receivedAt":"2020-06-30T19:33:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2020 at 03:27:18PM -0400, Taylor Blau wrote:\n\n> So, I'm certainly open to changing it, although I'm not sure that I'm as\n> worried about exposing the implementation detail as I am about squatting\n> on Bloom filters within Git in general. I don't think that this\n> configuration will end up getting used by folks other than server\n> administrators and for debugging purposes, so those populations are\n> already likely to be aware of changed-path Bloom filters beforehand.\n\nYeah, the squatting thing is definitely my bigger concern (having been\nthrough the \"bitmaps\" version of the same thing).\n\n> > It might even be worth considering whether \"changed paths\" needs more\n> > context (or would if we add new features in the future). On a \"git\n> > commit-graph write\" command-line it is perfectly clear, but would\n> > core.commitGraphChangedPaths be worth it? It's definitely more specific,\n> > but it's also way more ugly. ;)\n> \n> Here's a third option what about 'graph.readChangedPaths'. I think that\n> Stolee and I discussed a new top-level 'graph' section, since we now\n> have a few commit-graph-related configuration variables in 'core'.\n\nYes, I like that even better. Probably \"graph\" is sufficiently specific\nwithin Git's context, though I guess it _could_ bring to mind \"git log\n--graph\". So many overloaded terms. :)\n\n> That's a little shorter, and it adds the verb 'read', which is more\n> descriptive than 'use' (I touch on this in the third patch, where I say\n> that this configuration variable _doesn't_ affect the '--changed-path'\n> option when writing).\n\nYeah, saying \"read\" specifically is much nicer.\n\n> > > +\tprepare_repo_settings(the_repository);\n> > > +\n> > >  \tgraph = read_commit_graph_one(the_repository, odb);\n> >\n> > I wondered why we would need this prepare_repo_settings() now, when it\n> > should have been needed already to cover core.commitGraph already. I\n> > strongly suspect the answer is: \"test-tool read-graph\" never properly\n> > respected core.commitGraph in the first place.\n> \n> Yep. Could probably be broken out into a separate patch (or mentioned as\n> an aside in this one), but you're right: this helper did not respect\n> any configuration that 'prepare_repo_settings' picks up.\n\nI'd probably just note it in the commit message, but I'd be fine with\nthat or with a separate patch.\n\n-Peff\n"},{"id":"400851","messageId":"CAPig+cTYg6DggGRkCmFb2aKfj_3OysjMNXofqMSOuewRotrQaQ@mail.gmail.com","threadId":"53778","inReplyTo":"20200630190325.GB1888406@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] t4216: fix broken '&&'-chain","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-30T19:48:36Z","receivedAt":"2020-06-30T19:49:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2020 at 3:03 PM Jeff King <peff@peff.net> wrote:\n> [...] what are we improving, and why?\n>\n> The original code handled the fact that the file might not exist by not\n> including its exit code in the &&-chain which leads to the function's\n> return value. Your new code does so by putting it in the &&-chain but\n> asking \"rm\" to ignore errors. Is one better than the other?\n>\n> I think so, but my argument would be more along the lines of:\n>\n>   - without \"-f\", \"rm\" will complain about a missing file, which is\n>     distracting noise in the test log\n\nIndeed, a nice detail when reading verbose test output; one less thing\nto distract the attention from the real/important problems.\n\n>   - once \"-f\" is added in to suppress that, we might as well add the\n>     command to the &&-chain. That's our normal style, so readers don't\n>     have to wonder if it's important or not. Plus it would help avoid a\n>     broken chain if more commands are added at the beginning of the\n>     function.\n\nThe bit about commands possibly being added at the beginning of the\nfunction probably deserves its own bullet point. I often (relatively\nspeaking) cite that reason when asking people to &&-chain variable\nassignments at the beginning of a function.\n\n    func () {\n        a=1 &&\n        b=2 &&\n        ...\n    }\n"},{"id":"400854","messageId":"00a3658e-323c-0719-5f26-4689e2a90374@gmail.com","threadId":"53778","inReplyTo":"4ea9933b50fe0bc2738ab0e0dc52a4f17c4a2cb4.1593536481.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] commit-graph: pass a 'struct repository *' in more places","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-30T20:52:15Z","receivedAt":"2020-06-30T20:52:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/30/2020 1:17 PM, Taylor Blau wrote:\n> In a future commit, some commit-graph internals will want access to\n> 'r->settings', but we only have the 'struct object_directory *'\n> corresponding to that repository.\n\nIt is good to use \"struct repository *\" more.\n\n> Add an additional parameter to pass the repository around in more\n> places. In the next patch, we will remove the object directory (and\n> instead reference it with 'r->odb').\n\nAnd this is a good reason why we need the repository here: we will\nneed the settings AND odb.\n\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 3ba0da1e5f..03d848e168 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -76,10 +76,12 @@ struct commit_graph {\n>  };\n>  \n>  struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n> +\t\t\t\t\t\t struct repository *r,\n>  \t\t\t\t\t\t struct object_directory *odb);\n\nI suppose my only nit is that the struct repository pointer is\nnot always the first parameter. I understand that you have grouped\nit where the 'odb' parameter is here, so perhaps that is fine.\n\nFeel free to ignore this nit.\n\nThanks,\n-Stolee\n"},{"id":"402738","messageId":"20200803190211.GA67539@syl.lan","threadId":"53778","inReplyTo":"cover.1593536481.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] commit-graph: introduce 'core.useBloomFilters'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-03T19:02:11Z","receivedAt":"2020-08-03T19:02:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 30, 2020 at 01:17:36PM -0400, Taylor Blau wrote:\n> Hi,\n>\n> Here are some patches that we have been using at GitHub to control\n> whether or not Bloom filters stored in commit-graphs are read during\n> normal operation.\n\nI took the suggestions raised here and incorporated them into a\nmuch-larger series that contains updated versions of these patches here:\n\n  https://lore.kernel.org/git/20200803185947.GA67482@syl.lan/\n\nThis thread should be discarded, and future discussion redirected above.\n\nThanks.\n\n>\n> -Taylor\n>\n> Taylor Blau (3):\n>   commit-graph: pass a 'struct repository *' in more places\n>   t4216: fix broken '&&'-chain\n>   commit-graph: respect 'core.useBloomFilters'\n>\n>  Documentation/config/core.txt |  5 +++++\n>  builtin/commit-graph.c        |  2 +-\n>  commit-graph.c                | 17 ++++++++++-------\n>  commit-graph.h                |  4 +++-\n>  fuzz-commit-graph.c           |  5 +++--\n>  repo-settings.c               |  3 +++\n>  repository.h                  |  1 +\n>  t/helper/test-read-graph.c    |  3 ++-\n>  t/t4216-log-bloom.sh          |  6 ++++--\n>  9 files changed, 32 insertions(+), 14 deletions(-)\n>\n> --\n> 2.27.0.224.g4cfa086e50\n\nThanks,\nTaylor\n"}]}