{"thread":{"id":"60976","subject":"[PATCH 0/2] commit-graph: suggest deleting corrupt graphs","startedAt":"2024-02-22T23:19:10Z","lastAt":"2024-04-24T21:29:03Z","messageCount":6,"participants":["Josh Steadmon","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"489187","messageId":"cover.1708643825.git.steadmon@google.com","threadId":"60976","inReplyTo":null,"subject":"[PATCH 0/2] commit-graph: suggest deleting corrupt graphs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-02-22T23:19:05Z","receivedAt":"2024-02-22T23:19:10Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"At $WORK, we've had a few occasions where someone's commit-graph becomes\ncorrupt, and hits various BUG()s that block their day-to-day work. When\nthis happens, we advise the user to either disable the commit graph, or\nto delete it and let it be regenerated.\n\nIt would be a nicer user experience if we can make this a self-serve\nprocedure. To do this, let's add a new `git commit-graph clear`\nsubcommand so that users don't need to manually delete files under their\n.git directories. And to make it self-documenting, update various BUG(),\ndie(), and error() messages to suggest removing the commit graph to\nrecover from the corruption.\n\nThis approach was suggested in [1] and generally positively received\n[2], but was never implemented.\n\n[1] https://lore.kernel.org/git/YBoBBie2t1EhcLAN@google.com/\n[2] https://lore.kernel.org/git/xmqqk0rpc7uj.fsf@gitster.c.googlers.com/\n\nOpen questions for reviewers:\n* Should we turn this into an advice setting instead?\n* Should we also suggest running `commit-graph write` after clearing\n  the graph? I lean towards no; everything will still function as normal\n  without a commit graph.\n* Does it make sense to add the suggestion in all of these corruption\n  error messages? There are many other error()s in commit-graph.c,\n  should we add this for all of them, or just the ones that specifically\n  mention corruption? Or maybe just the fatal BUG()s and die()s?\n* Any other places this suggestion should be added that I've missed?\n\n\nJosh Steadmon (2):\n  commit-graph: add `git commit-graph clear` subcommand\n  commit-graph: suggest removing corrupt graphs\n\n Documentation/git-commit-graph.txt |  5 ++++\n builtin/commit-graph.c             | 40 +++++++++++++++++++++++++++\n commit-graph.c                     | 43 +++++++++++++++++++++++++++---\n commit-graph.h                     |  1 +\n commit-reach.c                     |  4 ++-\n t/t5318-commit-graph.sh            | 17 ++++++++++--\n t/t5324-split-commit-graph.sh      | 26 ++++++++++++------\n 7 files changed, 122 insertions(+), 14 deletions(-)\n\n\nbase-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b\n-- \n2.44.0.rc0.258.g7320e95886-goog\n\n"},{"id":"489188","messageId":"57344e9aa39b1fb1abf8275c6504cf80157ae0e6.1708643825.git.steadmon@google.com","threadId":"60976","inReplyTo":"cover.1708643825.git.steadmon@google.com","subject":"[PATCH 1/2] commit-graph: add `git commit-graph clear` subcommand","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-02-22T23:19:06Z","receivedAt":"2024-02-22T23:19:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"In the event the commit graph becomes corrupted, one option for recovery\nis to simply delete it and then rewrite it from scratch. However, this\nrequires users to manually delete files and directories under .git/,\nwhich is generally discouraged.\n\nAdd a new subcommand `git commit-graph clear` to provide a convenient\noption for removing the commit graph. Include tests for both single-file\nand split-file commit graphs. While we're at it, replace various cleanup\nsteps in the commit graph tests with `git commit-graph clear`.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\n Documentation/git-commit-graph.txt |  5 ++++\n builtin/commit-graph.c             | 40 ++++++++++++++++++++++++++++++\n commit-graph.c                     | 27 ++++++++++++++++++++\n commit-graph.h                     |  1 +\n t/t5318-commit-graph.sh            | 13 ++++++++--\n t/t5324-split-commit-graph.sh      | 26 +++++++++++++------\n 6 files changed, 102 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 903b16830e..0c96c428e6 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -14,6 +14,7 @@ SYNOPSIS\n \t\t\t[--split[=<strategy>]] [--reachable | --stdin-packs | --stdin-commits]\n \t\t\t[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress]\n \t\t\t<split-options>\n+'git commit-graph clear' [--object-dir <dir>]\n \n \n DESCRIPTION\n@@ -114,6 +115,10 @@ database. Used to check for corrupted data.\n With the `--shallow` option, only check the tip commit-graph file in\n a chain of split commit-graphs.\n \n+'clear'::\n+\n+Delete the commit graph file(s) and directory, if any exist.\n+\n \n EXAMPLES\n --------\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 7102ee90a0..0e2fecae50 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -23,6 +23,9 @@\n \t   \"                       [--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress]\\n\" \\\n \t   \"                       <split-options>\")\n \n+#define BUILTIN_COMMIT_GRAPH_CLEAR_USAGE \\\n+\tN_(\"git commit-graph clear [--object-dir <dir>]\")\n+\n static const char * builtin_commit_graph_verify_usage[] = {\n \tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n \tNULL\n@@ -33,9 +36,15 @@ static const char * builtin_commit_graph_write_usage[] = {\n \tNULL\n };\n \n+static const char * builtin_commit_graph_clear_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_CLEAR_USAGE,\n+\tNULL\n+};\n+\n static char const * const builtin_commit_graph_usage[] = {\n \tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n \tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n+\tBUILTIN_COMMIT_GRAPH_CLEAR_USAGE,\n \tNULL,\n };\n \n@@ -331,12 +340,43 @@ static int graph_write(int argc, const char **argv, const char *prefix)\n \treturn result;\n }\n \n+static int graph_clear(int argc, const char **argv, const char *prefix) {\n+\tint ret = 0;\n+\tstruct object_directory *odb = NULL;\n+\tchar *path;\n+\tstatic struct option builtin_commit_graph_clear_options[] = {\n+\t\tOPT_END(),\n+\t};\n+\tstruct option *options = add_common_options(builtin_commit_graph_clear_options);\n+\n+\ttrace2_cmd_mode(\"clear\");\n+\n+\targc = parse_options(argc, argv, NULL,\n+\t\t\t     builtin_commit_graph_clear_options,\n+\t\t\t     builtin_commit_graph_clear_usage, 0);\n+\n+\tif (!opts.obj_dir)\n+\t\topts.obj_dir = get_object_directory();\n+\n+\todb = find_odb(the_repository, opts.obj_dir);\n+\n+\tpath = get_commit_graph_filename(odb);\n+\tret |= unlink_or_warn(path);\n+\tret |= rm_commit_graph_chain(odb);\n+\n+\tFREE_AND_NULL(options);\n+\tfree(path);\n+\n+\treturn ret;\n+}\n+\n int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n {\n \tparse_opt_subcommand_fn *fn = NULL;\n \tstruct option builtin_commit_graph_options[] = {\n \t\tOPT_SUBCOMMAND(\"verify\", &fn, graph_verify),\n \t\tOPT_SUBCOMMAND(\"write\", &fn, graph_write),\n+\t\tOPT_SUBCOMMAND(\"clear\", &fn, graph_clear),\n \t\tOPT_END(),\n \t};\n \tstruct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 45417d7412..ca84423042 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -206,6 +206,33 @@ char *get_commit_graph_chain_filename(struct object_directory *odb)\n \treturn xstrfmt(\"%s/info/commit-graphs/commit-graph-chain\", odb->path);\n }\n \n+int rm_commit_graph_chain(struct object_directory *odb)\n+{\n+\tint ret = 0;\n+\tstruct strbuf chain_dir = STRBUF_INIT, file_path = STRBUF_INIT;\n+\tstruct dirent *d;\n+\tDIR *dir;\n+\n+\tstrbuf_addf(&chain_dir, \"%s/info/commit-graphs/\", odb->path);\n+\tstrbuf_addbuf(&file_path, &chain_dir);\n+\tdir = opendir(chain_dir.buf);\n+\tif (!dir)\n+\t\tgoto cleanup;\n+\twhile ((d = readdir(dir))) {\n+\t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n+\t\t\tcontinue;\n+\t\tstrbuf_setlen(&file_path, chain_dir.len);\n+\t\tstrbuf_addstr(&file_path, d->d_name);\n+\t\tret |= unlink_or_warn(file_path.buf);\n+\t}\n+\tclosedir(dir);\n+\trmdir_or_warn(chain_dir.buf);\n+cleanup:\n+\tstrbuf_release(&chain_dir);\n+\tstrbuf_release(&file_path);\n+\treturn ret;\n+}\n+\n static struct commit_graph *alloc_commit_graph(void)\n {\n \tstruct commit_graph *g = xcalloc(1, sizeof(*g));\ndiff --git a/commit-graph.h b/commit-graph.h\nindex e519cb81cb..1a6002767c 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -31,6 +31,7 @@ struct string_list;\n \n char *get_commit_graph_filename(struct object_directory *odb);\n char *get_commit_graph_chain_filename(struct object_directory *odb);\n+int rm_commit_graph_chain(struct object_directory *odb);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex a2b4442660..35354bddcb 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -397,7 +397,7 @@ test_expect_success 'warn on improper hash version' '\n test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'lower layers have overflow chunk' '\n \tUNIX_EPOCH_ZERO=\"@0 +0000\" &&\n \tFUTURE_DATE=\"@4147483646 +0000\" &&\n-\trm -f full/.git/objects/info/commit-graph &&\n+\tgit -C full commit-graph clear &&\n \ttest_commit -C full --date \"$FUTURE_DATE\" future-1 &&\n \ttest_commit -C full --date \"$UNIX_EPOCH_ZERO\" old-1 &&\n \tgit -C full commit-graph write --reachable &&\n@@ -824,7 +824,7 @@ test_expect_success 'overflow during generation version upgrade' '\n \n corrupt_chunk () {\n \tgraph=full/.git/objects/info/commit-graph &&\n-\ttest_when_finished \"rm -rf $graph\" &&\n+\ttest_when_finished \"git -C full commit-graph clear\" &&\n \tgit -C full commit-graph write --reachable &&\n \tcorrupt_chunk_file $graph \"$@\"\n }\n@@ -945,4 +945,13 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '\n \t)\n '\n \n+test_expect_success 'commit-graph clear removes files' '\n+\tgit -C full commit-graph write &&\n+\tgit -C full commit-graph verify &&\n+\ttest_path_is_file full/.git/objects/info/commit-graph &&\n+\tgit -C full commit-graph clear &&\n+\t! test_path_exists full/.git/objects/info/commit-graph &&\n+\t! test_path_exists full/.git/objects/info/commit-graphs\n+'\n+\n test_done\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 281266f788..ab5bc67fb6 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -120,7 +120,7 @@ test_expect_success 'fork and fail to base a chain on a commit-graph file' '\n \tgit clone . fork &&\n \t(\n \t\tcd fork &&\n-\t\trm .git/objects/info/commit-graph &&\n+\t\tgit commit-graph clear &&\n \t\techo \"$(pwd)/../.git/objects\" >.git/objects/info/alternates &&\n \t\ttest_commit new-commit &&\n \t\tgit commit-graph write --reachable --split &&\n@@ -177,7 +177,7 @@ test_expect_success 'create fork and chain across alternate' '\n \t(\n \t\tcd fork &&\n \t\tgit config core.commitGraph true &&\n-\t\trm -rf $graphdir &&\n+\t\tgit commit-graph clear &&\n \t\techo \"$(pwd)/../.git/objects\" >.git/objects/info/alternates &&\n \t\ttest_commit 13 &&\n \t\tgit branch commits/13 &&\n@@ -387,7 +387,7 @@ test_expect_success 'verify across alternates' '\n \tgit clone --no-hardlinks . verify-alt &&\n \t(\n \t\tcd verify-alt &&\n-\t\trm -rf $graphdir &&\n+\t\tgit commit-graph clear &&\n \t\taltdir=\"$(pwd)/../.git/objects\" &&\n \t\techo \"$altdir\" >.git/objects/info/alternates &&\n \t\tgit commit-graph verify --object-dir=\"$altdir/\" &&\n@@ -435,7 +435,7 @@ test_expect_success 'split across alternate where alternate is not split' '\n \tgit clone --no-hardlinks . alt-split &&\n \t(\n \t\tcd alt-split &&\n-\t\trm -f .git/objects/info/commit-graph &&\n+\t\tgit commit-graph clear &&\n \t\techo \"$(pwd)\"/../.git/objects >.git/objects/info/alternates &&\n \t\ttest_commit 18 &&\n \t\tgit commit-graph write --reachable --split &&\n@@ -446,7 +446,7 @@ test_expect_success 'split across alternate where alternate is not split' '\n \n test_expect_success '--split=no-merge always writes an incremental' '\n \ttest_when_finished rm -rf a b &&\n-\trm -rf $graphdir $infodir/commit-graph &&\n+\tgit commit-graph clear &&\n \tgit reset --hard commits/2 &&\n \tgit rev-list HEAD~1 >a &&\n \tgit rev-list HEAD >b &&\n@@ -456,7 +456,7 @@ test_expect_success '--split=no-merge always writes an incremental' '\n '\n \n test_expect_success '--split=replace replaces the chain' '\n-\trm -rf $graphdir $infodir/commit-graph &&\n+\tgit commit-graph clear &&\n \tgit reset --hard commits/3 &&\n \tgit rev-list -1 HEAD~2 >a &&\n \tgit rev-list -1 HEAD~1 >b &&\n@@ -490,7 +490,7 @@ test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion'\n while read mode modebits\n do\n \ttest_expect_success POSIXPERM \"split commit-graph respects core.sharedrepository $mode\" '\n-\t\trm -rf $graphdir $infodir/commit-graph &&\n+\t\tgit commit-graph clear &&\n \t\tgit reset --hard commits/1 &&\n \t\ttest_config core.sharedrepository \"$mode\" &&\n \t\tgit commit-graph write --split --reachable &&\n@@ -508,7 +508,7 @@ done <<\\EOF\n EOF\n \n test_expect_success '--split=replace with partial Bloom data' '\n-\trm -rf $graphdir $infodir/commit-graph &&\n+\tgit commit-graph clear &&\n \tgit reset --hard commits/3 &&\n \tgit rev-list -1 HEAD~2 >a &&\n \tgit rev-list -1 HEAD~1 >b &&\n@@ -718,4 +718,14 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl\n \t)\n '\n \n+\n+test_expect_success 'commit-graph clear removes files' '\n+\tgit commit-graph write &&\n+\tgit commit-graph verify &&\n+\t! test_dir_is_empty .git/objects/info/commit-graphs &&\n+\tgit commit-graph clear &&\n+\t! test_path_exists .git/objects/info/commit-graph &&\n+\t! test_path_exists .git/objects/info/commit-graphs\n+'\n+\n test_done\n-- \n2.44.0.rc0.258.g7320e95886-goog\n\n"},{"id":"489189","messageId":"55241dacc5f84c76bd661ab376deadec2c78f4f6.1708643825.git.steadmon@google.com","threadId":"60976","inReplyTo":"cover.1708643825.git.steadmon@google.com","subject":"[PATCH 2/2] commit-graph: suggest removing corrupt graphs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-02-22T23:19:07Z","receivedAt":"2024-02-22T23:19:14Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"There are various ways the commit graph can be corrupted. When we detect\nthese, we issue an error(), BUG(), or die(). However, this doesn't help\nthe user correct the problem.\n\nSince the commit graph can be regenerated from scratch, it may make\nsense to just delete corrupt graphs. Suggest running the new\n`git commit-graph clear` command in relevant error/BUG/die messages.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\n commit-graph.c          | 16 +++++++++++++---\n commit-reach.c          |  4 +++-\n t/t5318-commit-graph.sh |  4 ++++\n 3 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ca84423042..0d5474852c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -418,6 +418,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \tif (graph_signature != GRAPH_SIGNATURE) {\n \t\terror(_(\"commit-graph signature %X does not match signature %X\"),\n \t\t      graph_signature, GRAPH_SIGNATURE);\n+\t\terror(_(\"try running: git commit-graph clear\"));\n \t\treturn NULL;\n \t}\n \n@@ -425,6 +426,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \tif (graph_version != GRAPH_VERSION) {\n \t\terror(_(\"commit-graph version %X does not match version %X\"),\n \t\t      graph_version, GRAPH_VERSION);\n+\t\terror(_(\"try running: git commit-graph clear\"));\n \t\treturn NULL;\n \t}\n \n@@ -432,6 +434,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \tif (hash_version != oid_version(the_hash_algo)) {\n \t\terror(_(\"commit-graph hash version %X does not match version %X\"),\n \t\t      hash_version, oid_version(the_hash_algo));\n+\t\terror(_(\"try running: git commit-graph clear\"));\n \t\treturn NULL;\n \t}\n \n@@ -447,6 +450,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t GRAPH_FANOUT_SIZE + the_hash_algo->rawsz) {\n \t\terror(_(\"commit-graph file is too small to hold %u chunks\"),\n \t\t      graph->num_chunks);\n+\t\terror(_(\"try running: git commit-graph clear\"));\n \t\tfree(graph);\n \t\treturn NULL;\n \t}\n@@ -459,14 +463,17 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \n \tif (read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph)) {\n \t\terror(_(\"commit-graph required OID fanout chunk missing or corrupted\"));\n+\t\terror(_(\"try running: git commit-graph clear\"));\n \t\tgoto free_and_return;\n \t}\n \tif (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {\n \t\terror(_(\"commit-graph required OID lookup chunk missing or corrupted\"));\n+\t\terror(_(\"try running: git commit-graph clear\"));\n \t\tgoto free_and_return;\n \t}\n \tif (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {\n \t\terror(_(\"commit-graph required commit data chunk missing or corrupted\"));\n+\t\terror(_(\"try running: git commit-graph clear\"));\n \t\tgoto free_and_return;\n \t}\n \n@@ -860,7 +867,8 @@ static void load_oid_from_graph(struct commit_graph *g,\n \t\tBUG(\"NULL commit-graph\");\n \n \tif (pos >= g->num_commits + g->num_commits_in_base)\n-\t\tdie(_(\"invalid commit position. commit-graph is likely corrupt\"));\n+\t\tdie(_(\"invalid commit position. The commit-graph is likely corrupt,\\n\"\n+\t\t      \"try running:\\n\\tgit commit-graph clear\"));\n \n \tlex_index = pos - g->num_commits_in_base;\n \n@@ -876,7 +884,8 @@ static struct commit_list **insert_parent_or_die(struct repository *r,\n \tstruct object_id oid;\n \n \tif (pos >= g->num_commits + g->num_commits_in_base)\n-\t\tdie(\"invalid parent position %\"PRIu32, pos);\n+\t\tdie(\"invalid parent position %\"PRIu32\". The commit-graph is likely corrupt,\\n\"\n+\t\t    \"try running:\\n\\tgit commit-graph clear\", pos);\n \n \tload_oid_from_graph(g, pos, &oid);\n \tc = lookup_commit(r, &oid);\n@@ -897,7 +906,8 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,\n \t\tg = g->base_graph;\n \n \tif (pos >= g->num_commits + g->num_commits_in_base)\n-\t\tdie(_(\"invalid commit position. commit-graph is likely corrupt\"));\n+\t\tdie(_(\"invalid commit position. commit-graph is likely corrupt,\\n\"\n+\t\t      \"try running:\\n\\tgit commit-graph clear\"));\n \n \tlex_index = pos - g->num_commits_in_base;\n \tcommit_data = g->chunk_commit_data + st_mult(GRAPH_DATA_WIDTH, lex_index);\ndiff --git a/commit-reach.c b/commit-reach.c\nindex ecc913fc99..16765ce39b 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -81,7 +81,9 @@ static struct commit_list *paint_down_to_common(struct repository *r,\n \t\ttimestamp_t generation = commit_graph_generation(commit);\n \n \t\tif (min_generation && generation > last_gen)\n-\t\t\tBUG(\"bad generation skip %\"PRItime\" > %\"PRItime\" at %s\",\n+\t\t\tBUG(\"bad generation skip %\"PRItime\" > %\"PRItime\" at %s\\n\"\n+\t\t\t    \"The commit graph is likely corrupt, try running:\\n\"\n+\t\t\t    \"\\tgit commit-graph clear\",\n \t\t\t    generation, last_gen,\n \t\t\t    oid_to_hex(&commit->object.oid));\n \t\tlast_gen = generation;\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 35354bddcb..f4553b1916 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -843,6 +843,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\n \tcat >expect.err <<-\\EOF &&\n \terror: commit-graph oid fanout chunk is wrong size\n \terror: commit-graph required OID fanout chunk missing or corrupted\n+\terror: try running: git commit-graph clear\n \tEOF\n \ttest_cmp expect.err err\n '\n@@ -852,6 +853,7 @@ test_expect_success 'reader notices fanout/lookup table mismatch' '\n \tcat >expect.err <<-\\EOF &&\n \terror: commit-graph OID lookup chunk is the wrong size\n \terror: commit-graph required OID lookup chunk missing or corrupted\n+\terror: try running: git commit-graph clear\n \tEOF\n \ttest_cmp expect.err err\n '\n@@ -868,6 +870,7 @@ test_expect_success 'reader notices out-of-bounds fanout' '\n \tcat >expect.err <<-\\EOF &&\n \terror: commit-graph fanout values out of order\n \terror: commit-graph required OID fanout chunk missing or corrupted\n+\terror: try running: git commit-graph clear\n \tEOF\n \ttest_cmp expect.err err\n '\n@@ -877,6 +880,7 @@ test_expect_success 'reader notices too-small commit data chunk' '\n \tcat >expect.err <<-\\EOF &&\n \terror: commit-graph commit data chunk is wrong size\n \terror: commit-graph required commit data chunk missing or corrupted\n+\terror: try running: git commit-graph clear\n \tEOF\n \ttest_cmp expect.err err\n '\n-- \n2.44.0.rc0.258.g7320e95886-goog\n\n"},{"id":"489192","messageId":"xmqqwmqw82pv.fsf@gitster.g","threadId":"60976","inReplyTo":"cover.1708643825.git.steadmon@google.com","subject":"Re: [PATCH 0/2] commit-graph: suggest deleting corrupt graphs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-23T00:05:16Z","receivedAt":"2024-02-23T00:05:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> At $WORK, we've had a few occasions where someone's commit-graph becomes\n> corrupt, and hits various BUG()s that block their day-to-day work. When\n> this happens, we advise the user to either disable the commit graph, or\n> to delete it and let it be regenerated.\n>\n> It would be a nicer user experience if we can make this a self-serve\n> procedure. To do this, let's add a new `git commit-graph clear`\n> subcommand so that users don't need to manually delete files under their\n> .git directories. And to make it self-documenting, update various BUG(),\n> die(), and error() messages to suggest removing the commit graph to\n> recover from the corruption.\n\nI am of two minds.\n\nFor one, if we know there is a corruption and if we know that we\nwill certainly recover cleanly if we removed these files, it would\nbe fair for an end-user to respond with: instead of telling me to\nrun \"commit-graph clear\", you can run it for me, can't you?\n\nThe other one is if it hinders debugging the root cause to run\n\"clear\", whether it is done by the end-user or by the mechanism that\ndetects and dies upon discovery of a corruption.  Do we know how\nthese commit-graph files become corrupt?  How valuable would these\ncorrupt files be to help us track down where the corruption comes\nfrom?  If they are not all that useful in debugging, then removing\nthem ourselves or telling users to remove them may be OK, of course.\n\nDo these BUG()s come from corruption that can be diagnosed upfront\nwhen we \"open\" the commit-graph files?  I am wondering if it would\nbe the matter of teaching prepare_commit_graph() to check for\ncorruption and return without enabling the support.\n\nThanks.\n"},{"id":"493435","messageId":"sc4xgsvzioqglqtsaqww56vctehfzkomy7wr574y3su6optu6t@punugznrq7bk","threadId":"60976","inReplyTo":"xmqqwmqw82pv.fsf@gitster.g","subject":"Re: [PATCH 0/2] commit-graph: suggest deleting corrupt graphs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-24T19:30:05Z","receivedAt":"2024-04-24T19:30:11Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.02.22 16:05, Junio C Hamano wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > At $WORK, we've had a few occasions where someone's commit-graph becomes\n> > corrupt, and hits various BUG()s that block their day-to-day work. When\n> > this happens, we advise the user to either disable the commit graph, or\n> > to delete it and let it be regenerated.\n> >\n> > It would be a nicer user experience if we can make this a self-serve\n> > procedure. To do this, let's add a new `git commit-graph clear`\n> > subcommand so that users don't need to manually delete files under their\n> > .git directories. And to make it self-documenting, update various BUG(),\n> > die(), and error() messages to suggest removing the commit graph to\n> > recover from the corruption.\n> \n> I am of two minds.\n> \n> For one, if we know there is a corruption and if we know that we\n> will certainly recover cleanly if we removed these files, it would\n> be fair for an end-user to respond with: instead of telling me to\n> run \"commit-graph clear\", you can run it for me, can't you?\n> \n> The other one is if it hinders debugging the root cause to run\n> \"clear\", whether it is done by the end-user or by the mechanism that\n> detects and dies upon discovery of a corruption.  Do we know how\n> these commit-graph files become corrupt?  How valuable would these\n> corrupt files be to help us track down where the corruption comes\n> from?  If they are not all that useful in debugging, then removing\n> them ourselves or telling users to remove them may be OK, of course.\n> \n> Do these BUG()s come from corruption that can be diagnosed upfront\n> when we \"open\" the commit-graph files?  I am wondering if it would\n> be the matter of teaching prepare_commit_graph() to check for\n> corruption and return without enabling the support.\n> \n> Thanks.\n\nSorry for the late reply, this got buried in my inbox. The corruption we\nsaw was related to a generation numbers bug [1] that I think was only\npresent for a short while in 'next'.\n\n[1] https://lore.kernel.org/git/YBn3fxFe978Up5Ly@google.com/\n\nI believe that being able to examine the files after the corruption was\ndetected did help us narrow down the issue, so I would lean towards not\nautomatically deleting them upon detecting corruption.\n\nI don't think that this case would be detectable without running a full\n`git commit-graph verify` up front.\n"},{"id":"493441","messageId":"xmqqjzkmxxeu.fsf@gitster.g","threadId":"60976","inReplyTo":"sc4xgsvzioqglqtsaqww56vctehfzkomy7wr574y3su6optu6t@punugznrq7bk","subject":"Re: [PATCH 0/2] commit-graph: suggest deleting corrupt graphs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-24T21:28:57Z","receivedAt":"2024-04-24T21:29:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> I believe that being able to examine the files after the corruption was\n> detected did help us narrow down the issue, so I would lean towards not\n> automatically deleting them upon detecting corruption.\n\nUnderstood.\n\n> I don't think that this case would be detectable without running a full\n> `git commit-graph verify` up front.\n\nOK, then that approach is not worth pursuing.  Thanks for a dose of\nsanity.\n"}]}