{"thread":{"id":"53954","subject":"[PATCH] commit-graph: add verify changed paths option","startedAt":"2020-07-31T07:49:29Z","lastAt":"2020-07-31T19:31:29Z","messageCount":8,"participants":["Son Luong Ngoc via GitGitGadget","Christian Couder","Junio C Hamano","Jeff King","Taylor Blau","Son Luong Ngoc"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402522","messageId":"pull.687.git.1596181765336.gitgitgadget@gmail.com","threadId":"53954","inReplyTo":null,"subject":"[PATCH] commit-graph: add verify changed paths option","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-31T07:49:25Z","receivedAt":"2020-07-31T07:49:29Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"From: Son Luong Ngoc <sluongng@gmail.com>\n\nAdd '--has-changed-paths' option to 'git commit-graph verify' subcommand\nto validate whether the commit-graph was written with '--changed-paths'\noption.\n\nSigned-off-by: Son Luong Ngoc <sluongng@gmail.com>\n---\n    Commit-Graph: Verify bloom filter\n    \n    When I was working on git-care(1) and Gitaly(2), the need to check\n    whether a commit-graph (split or non-split) were built with Bloom\n    filter. This is needed especially when a repository primary commit-graph\n    write strategy is '--split' and the bottom chains might rarely be\n    re-written (or never) thus Bloom filter is never applied to the graph.\n    \n    Provides users with a straight forward way to validate the existence of\n    Bloom filter chunks to save user having to read the commit-graph\n    manually as show in (1) and (2).\n    \n    References:\n    \n     1. https://github.com/sluongng/git-care/commit/d0feaa381ea3ec7b0e617c6596ad6e3cf16b884a\n     2. https://gitlab.com/sluongng/gitaly/-/commit/78dba8b73e720b11500482b19b755346ec853025\n    \n    \n    ------------------------------------------------------------------------\n    \n    It's probably going to take me a bit more time to write up some tests\n    for this, so I want to send it out first for comments.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-687%2Fsluongng%2Fsluongngoc%2Fverify-bloom-filter-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-687/sluongng/sluongngoc/verify-bloom-filter-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/687\n\n builtin/commit-graph.c | 12 +++++++++---\n commit-graph.c         | 22 +++++++++++++++++-----\n commit-graph.h         | 12 +++++++++---\n 3 files changed, 35 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 16c9f6101a..ce8a7cbe90 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -18,7 +18,8 @@ static char const * const builtin_commit_graph_usage[] = {\n };\n \n static const char * const builtin_commit_graph_verify_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]\"),\n+\tN_(\"git commit-graph verify [--object-dir <objdir>] [--shallow] \"\n+\t    \"[--has-changed-paths] [--[no-]progress]\"),\n \tNULL\n };\n \n@@ -37,6 +38,7 @@ static struct opts_commit_graph {\n \tint append;\n \tint split;\n \tint shallow;\n+\tint has_changed_paths;\n \tint progress;\n \tint enable_changed_paths;\n } opts;\n@@ -71,12 +73,14 @@ static int graph_verify(int argc, const char **argv)\n \tint open_ok;\n \tint fd;\n \tstruct stat st;\n-\tint flags = 0;\n+\tenum commit_graph_verify_flags flags = 0;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n \t\t\t   N_(\"dir\"),\n \t\t\t   N_(\"The object directory to store the graph\")),\n+\t\tOPT_BOOL(0, \"has-changed-paths\", &opts.has_changed_paths,\n+\t\t\t N_(\"verify that the commit-graph includes changed paths\")),\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n \t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n@@ -94,8 +98,10 @@ static int graph_verify(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \tif (opts.shallow)\n \t\tflags |= COMMIT_GRAPH_VERIFY_SHALLOW;\n+\tif (opts.has_changed_paths)\n+\t\tflags |= COMMIT_GRAPH_VERIFY_CHANGED_PATHS;\n \tif (opts.progress)\n-\t\tflags |= COMMIT_GRAPH_WRITE_PROGRESS;\n+\t\tflags |= COMMIT_GRAPH_VERIFY_PROGRESS;\n \n \todb = find_odb(the_repository, opts.obj_dir);\n \tgraph_name = get_commit_graph_filename(odb);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 1af68c297d..d83f5a2325 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -250,7 +250,7 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n \treturn ret;\n }\n \n-static int verify_commit_graph_lite(struct commit_graph *g)\n+static int verify_commit_graph_lite(struct commit_graph *g, int verify_changed_path)\n {\n \t/*\n \t * Basic validation shared between parse_commit_graph()\n@@ -276,6 +276,16 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \t\terror(\"commit-graph is missing the Commit Data chunk\");\n \t\treturn 1;\n \t}\n+\tif (verify_changed_path) {\n+\t\tif (!g->chunk_bloom_indexes) {\n+\t\t\terror(\"commit-graph is missing Bloom Index chunk\");\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (!g->chunk_bloom_data) {\n+\t\t\terror(\"commit-graph is missing Bloom Data chunk\");\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n \n \treturn 0;\n }\n@@ -439,7 +449,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n \n \thashcpy(graph->oid.hash, graph->data + graph->data_len - graph->hash_len);\n \n-\tif (verify_commit_graph_lite(graph))\n+\tif (verify_commit_graph_lite(graph, 0))\n \t\tgoto free_and_return;\n \n \treturn graph;\n@@ -2216,7 +2226,9 @@ static void graph_report(const char *fmt, ...)\n #define GENERATION_ZERO_EXISTS 1\n #define GENERATION_NUMBER_EXISTS 2\n \n-int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n+int verify_commit_graph(struct repository *r,\n+\t\t\tstruct commit_graph *g,\n+\t\t\tenum commit_graph_verify_flags flags)\n {\n \tuint32_t i, cur_fanout_pos = 0;\n \tstruct object_id prev_oid, cur_oid, checksum;\n@@ -2231,7 +2243,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \t\treturn 1;\n \t}\n \n-\tverify_commit_graph_error = verify_commit_graph_lite(g);\n+\tverify_commit_graph_error = verify_commit_graph_lite(g, flags & COMMIT_GRAPH_VERIFY_CHANGED_PATHS);\n \tif (verify_commit_graph_error)\n \t\treturn verify_commit_graph_error;\n \n@@ -2284,7 +2296,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \tif (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)\n \t\treturn verify_commit_graph_error;\n \n-\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n+\tif (flags & COMMIT_GRAPH_VERIFY_PROGRESS)\n \t\tprogress = start_progress(_(\"Verifying commits in commit graph\"),\n \t\t\t\t\tg->num_commits);\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 28f89cdf3e..29c01b5000 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -94,6 +94,12 @@ enum commit_graph_write_flags {\n \tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3),\n };\n \n+enum commit_graph_verify_flags {\n+\tCOMMIT_GRAPH_VERIFY_SHALLOW       = (1 << 0),\n+\tCOMMIT_GRAPH_VERIFY_CHANGED_PATHS = (1 << 1),\n+\tCOMMIT_GRAPH_VERIFY_PROGRESS      = (1 << 2),\n+};\n+\n enum commit_graph_split_flags {\n \tCOMMIT_GRAPH_SPLIT_UNSPECIFIED      = 0,\n \tCOMMIT_GRAPH_SPLIT_MERGE_PROHIBITED = 1,\n@@ -122,9 +128,9 @@ int write_commit_graph(struct object_directory *odb,\n \t\t       enum commit_graph_write_flags flags,\n \t\t       const struct split_commit_graph_opts *split_opts);\n \n-#define COMMIT_GRAPH_VERIFY_SHALLOW\t(1 << 0)\n-\n-int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags);\n+int verify_commit_graph(struct repository *r,\n+\t\t\tstruct commit_graph *g,\n+\t\t\tenum commit_graph_verify_flags flags);\n \n void close_commit_graph(struct raw_object_store *);\n void free_commit_graph(struct commit_graph *);\n\nbase-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc\n-- \ngitgitgadget\n"},{"id":"402551","messageId":"CAP8UFD3QF9P4UvQqaguc0MgNijBvzC4KhF=D_+MN+NjfaR535g@mail.gmail.com","threadId":"53954","inReplyTo":"pull.687.git.1596181765336.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: add verify changed paths option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-07-31T16:21:54Z","receivedAt":"2020-07-31T16:22:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jul 31, 2020 at 9:52 AM Son Luong Ngoc via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Son Luong Ngoc <sluongng@gmail.com>\n>\n> Add '--has-changed-paths' option to 'git commit-graph verify' subcommand\n> to validate whether the commit-graph was written with '--changed-paths'\n> option.\n>\n> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>\n\n[...]\n\n>     It's probably going to take me a bit more time to write up some tests\n>     for this,\n\nIt would need some documentation too.\n\n> so I want to send it out first for comments.\n\n[...]\n\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 16c9f6101a..ce8a7cbe90 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -18,7 +18,8 @@ static char const * const builtin_commit_graph_usage[] = {\n>  };\n>\n>  static const char * const builtin_commit_graph_verify_usage[] = {\n> -       N_(\"git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]\"),\n> +       N_(\"git commit-graph verify [--object-dir <objdir>] [--shallow] \"\n> +           \"[--has-changed-paths] [--[no-]progress]\"),\n>         NULL\n>  };\n>\n> @@ -37,6 +38,7 @@ static struct opts_commit_graph {\n>         int append;\n>         int split;\n>         int shallow;\n> +       int has_changed_paths;\n>         int progress;\n>         int enable_changed_paths;\n>  } opts;\n> @@ -71,12 +73,14 @@ static int graph_verify(int argc, const char **argv)\n>         int open_ok;\n>         int fd;\n>         struct stat st;\n> -       int flags = 0;\n> +       enum commit_graph_verify_flags flags = 0;\n>\n>         static struct option builtin_commit_graph_verify_options[] = {\n>                 OPT_STRING(0, \"object-dir\", &opts.obj_dir,\n>                            N_(\"dir\"),\n>                            N_(\"The object directory to store the graph\")),\n> +               OPT_BOOL(0, \"has-changed-paths\", &opts.has_changed_paths,\n> +                        N_(\"verify that the commit-graph includes changed paths\")),\n>                 OPT_BOOL(0, \"shallow\", &opts.shallow,\n>                          N_(\"if the commit-graph is split, only verify the tip file\")),\n>                 OPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n> @@ -94,8 +98,10 @@ static int graph_verify(int argc, const char **argv)\n>                 opts.obj_dir = get_object_directory();\n>         if (opts.shallow)\n>                 flags |= COMMIT_GRAPH_VERIFY_SHALLOW;\n> +       if (opts.has_changed_paths)\n> +               flags |= COMMIT_GRAPH_VERIFY_CHANGED_PATHS;\n\nI wonder if OPT_BIT() could be used instead of OPT_BOOL() above to\ndirectly set the above flag, as the 'has_changed_paths' field in\n'struct opts_commit_graph' seems to be used only for the purpose of\nsetting this flag.\n\n>         if (opts.progress)\n> -               flags |= COMMIT_GRAPH_WRITE_PROGRESS;\n> +               flags |= COMMIT_GRAPH_VERIFY_PROGRESS;\n\nDoes this change belong to this patch? I think it would deserve an\nexplanation in the commit message if that's the case.\n\n>         odb = find_odb(the_repository, opts.obj_dir);\n>         graph_name = get_commit_graph_filename(odb);\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 1af68c297d..d83f5a2325 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -250,7 +250,7 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n>         return ret;\n>  }\n>\n> -static int verify_commit_graph_lite(struct commit_graph *g)\n> +static int verify_commit_graph_lite(struct commit_graph *g, int verify_changed_path)\n\n[...]\n\n> -int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n> +int verify_commit_graph(struct repository *r,\n> +                       struct commit_graph *g,\n> +                       enum commit_graph_verify_flags flags)\n\nIt seems to me that it would be more coherent to have both\nverify_commit_graph() and verify_commit_graph_lite() accept an 'enum\ncommit_graph_verify_flags flags' argument.\n\nRight now the \"has_changed_paths\" option is first an int, then it's\nconverted to a flag and then to an int again before being passed to\nverify_commit_graph_lite(). It would be simpler if it could be a flag\nall along.\n\nThanks,\nChristian.\n"},{"id":"402560","messageId":"xmqqh7tn4neo.fsf@gitster.c.googlers.com","threadId":"53954","inReplyTo":"pull.687.git.1596181765336.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: add verify changed paths option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-31T17:14:39Z","receivedAt":"2020-07-31T17:14:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Son Luong Ngoc via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Son Luong Ngoc <sluongng@gmail.com>\n>\n> Add '--has-changed-paths' option to 'git commit-graph verify' subcommand\n> to validate whether the commit-graph was written with '--changed-paths'\n> option.\n\nThe implementation seems to be only about \"does this section exist?\"\nand not \"does this section have healthy/uncorrupted data?\", which\nfeels a bit strange for \"verify\".  Instead of setting ourselves up\nto having to add \"--has-this-section\" and \"--has-that-section\" every\ntime a new kind of data is added to the system, how about giving the\nverify command an option to list all the sections found in the file,\nor a separate \"git commit-graph list-sections\" subcommand?\n\n"},{"id":"402570","messageId":"20200731180235.GA846620@coredump.intra.peff.net","threadId":"53954","inReplyTo":"pull.687.git.1596181765336.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: add verify changed paths option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T18:02:35Z","receivedAt":"2020-07-31T18:02:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 07:49:25AM +0000, Son Luong Ngoc via GitGitGadget wrote:\n\n> From: Son Luong Ngoc <sluongng@gmail.com>\n> \n> Add '--has-changed-paths' option to 'git commit-graph verify' subcommand\n> to validate whether the commit-graph was written with '--changed-paths'\n> option.\n\nIs a single boolean flag sufficient? If you have incrementals, you might\nhave some slices with this chunk and some without. What should the\nboolean be in that case?\n\nI thought we had some way of reporting the number of commits covered by\nfilters, but I can't seem to find it.\n\nOur \"test-tool read-graph\" can report on whether there's a bloom filter\nchunk, but I think it also doesn't distinguish between different slices\n(and anyway, it wouldn't be suitable for tools that don't rely on an\nactual built git.git directory).\n\n-Peff\n"},{"id":"402571","messageId":"20200731180636.GA59489@syl.lan","threadId":"53954","inReplyTo":"xmqqh7tn4neo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit-graph: add verify changed paths option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T18:06:36Z","receivedAt":"2020-07-31T18:06:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 31, 2020 at 10:14:39AM -0700, Junio C Hamano wrote:\n> \"Son Luong Ngoc via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Son Luong Ngoc <sluongng@gmail.com>\n> >\n> > Add '--has-changed-paths' option to 'git commit-graph verify' subcommand\n> > to validate whether the commit-graph was written with '--changed-paths'\n> > option.\n>\n> The implementation seems to be only about \"does this section exist?\"\n> and not \"does this section have healthy/uncorrupted data?\", which\n> feels a bit strange for \"verify\".  Instead of setting ourselves up\n> to having to add \"--has-this-section\" and \"--has-that-section\" every\n> time a new kind of data is added to the system, how about giving the\n> verify command an option to list all the sections found in the file,\n> or a separate \"git commit-graph list-sections\" subcommand?\n\nCompletely agreed. When I suggested that Son work on this, I more had in\nmind something like 'git commit-graph verify --changed-paths' to mean\n\"verify the integrity of the commit-graph(s), including regenerating\nchanged-path Bloom filters and making sure they match\".\n\nIf you are just curious whether or not the section exists, I'd rather\nwrite a script to look for the 'BIDX' or 'BDAT' chunk IDs. That said, if\nthey're spread across incremental, maybe it makes more sense to extend\nthe commit-graph test tool.\n\nI dunno.\n\nThanks,\nTaylor\n"},{"id":"402572","messageId":"20200731180956.GA60133@syl.lan","threadId":"53954","inReplyTo":"20200731180235.GA846620@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: add verify changed paths option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T18:09:56Z","receivedAt":"2020-07-31T18:10:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 31, 2020 at 02:02:35PM -0400, Jeff King wrote:\n> On Fri, Jul 31, 2020 at 07:49:25AM +0000, Son Luong Ngoc via GitGitGadget wrote:\n>\n> > From: Son Luong Ngoc <sluongng@gmail.com>\n> >\n> > Add '--has-changed-paths' option to 'git commit-graph verify' subcommand\n> > to validate whether the commit-graph was written with '--changed-paths'\n> > option.\n>\n> Is a single boolean flag sufficient? If you have incrementals, you might\n> have some slices with this chunk and some without. What should the\n> boolean be in that case?\n\nI think you'd really want to know which layers do and don't have\nfilters. It might be even more interesting to have a tool like what 'git\nshow-index' is to '*.idx' files, maybe something like 'git show-graph'\nor 'git show-commit-graph'. Its output would be one line per commit that\nshows:\n\n  - what layer in the chain it's located at\n  - its graph_pos\n  - its generation number\n  - whether or not it has a Bloom filter\n  - ???\n\nThat would be a useful tool for debugging anyway, even outside of the\ntest suite. It would be even better if we could replace the test-tool\nwith it.\n\nOn an unrelated note; this patch is broken as-is, since it will only\nreport that Bloom filters exist if the top-most graph has them. I have a\npatch to fix this that I have been meaning to send out for most of this\nweek. I'll try to get to it shortly.\n\n> I thought we had some way of reporting the number of commits covered by\n> filters, but I can't seem to find it.\n\nI don't recall having anything like that.\n\n> Our \"test-tool read-graph\" can report on whether there's a bloom filter\n> chunk, but I think it also doesn't distinguish between different slices\n> (and anyway, it wouldn't be suitable for tools that don't rely on an\n> actual built git.git directory).\n>\n> -Peff\nThanks,\nTaylor\n"},{"id":"402577","messageId":"20200731191448.GA848793@coredump.intra.peff.net","threadId":"53954","inReplyTo":"20200731180956.GA60133@syl.lan","subject":"Re: [PATCH] commit-graph: add verify changed paths option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T19:14:48Z","receivedAt":"2020-07-31T19:14:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 02:09:56PM -0400, Taylor Blau wrote:\n\n> > Is a single boolean flag sufficient? If you have incrementals, you might\n> > have some slices with this chunk and some without. What should the\n> > boolean be in that case?\n> \n> I think you'd really want to know which layers do and don't have\n> filters. It might be even more interesting to have a tool like what 'git\n> show-index' is to '*.idx' files, maybe something like 'git show-graph'\n> or 'git show-commit-graph'. Its output would be one line per commit that\n> shows:\n> \n>   - what layer in the chain it's located at\n>   - its graph_pos\n>   - its generation number\n>   - whether or not it has a Bloom filter\n>   - ???\n> \n> That would be a useful tool for debugging anyway, even outside of the\n> test suite. It would be even better if we could replace the test-tool\n> with it.\n\nYeah, that was exactly what I had in mind, except that I'd make it a\nsub-command of \"git commit-graph\" (\"show\" or perhaps \"dump\").\n\n-Peff\n"},{"id":"402580","messageId":"E6157D6A-52FB-4E0D-BFE7-8F3B0848F8A3@gmail.com","threadId":"53954","inReplyTo":"20200731191448.GA848793@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: add verify changed paths option","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-07-31T19:31:23Z","receivedAt":"2020-07-31T19:31:29Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Note: re-send  to mailing list due to me forgot to turn on Plain Text format.\n(sorry for the noise)\n\nHi Peff, Taylor, Junio and Christian,\n\nThanks a lot for the valuable feedbacks.\nThis is exactly what I was hoping for by sending out the patch early!\n\n> On Jul 31, 2020, at 21:14, Jeff King <peff@peff.net> wrote:\n> \n> On Fri, Jul 31, 2020 at 02:09:56PM -0400, Taylor Blau wrote:\n> \n>>> Is a single boolean flag sufficient? If you have incrementals, you might\n>>> have some slices with this chunk and some without. What should the\n>>> boolean be in that case?\n>> \n>> I think you'd really want to know which layers do and don't have\n>> filters. It might be even more interesting to have a tool like what 'git\n>> show-index' is to '*.idx' files, maybe something like 'git show-graph'\n>> or 'git show-commit-graph'. Its output would be one line per commit that\n>> shows:\n>> \n>>  - what layer in the chain it's located at\n>>  - its graph_pos\n>>  - its generation number\n>>  - whether or not it has a Bloom filter\n>>  - ???\n>> \n>> That would be a useful tool for debugging anyway, even outside of the\n>> test suite. It would be even better if we could replace the test-tool\n>> with it.\n> \n> Yeah, that was exactly what I had in mind, except that I'd make it a\n> sub-command of \"git commit-graph\" (\"show\" or perhaps \"dump\").\n\nI loved Junio's initial suggestion and the follow up here.\nI was thinking of something like 'git commit-graph verify --verbose' but \nnow I agree that a distinct command such as 'show' might be more \ndistinct and better communicate the purpose.\n\nI will stick with my poor-man bash/golang script for now to invalidate\nthe commit-graph (chain or no-chain) as it does the job just fine.\n\nLet me see if I have the capacity to implement 'show' sub-command\nafter. ^_^!\n\n> \n> -Peff\n\nCheers,\nSon Luong.\n"}]}