{"thread":{"id":"50554","subject":"[PATCH 0/8] commit-graph: segfault & other fixes for broken graphs","startedAt":"2019-02-21T22:38:06Z","lastAt":"2019-05-01T18:31:11Z","messageCount":36,"participants":["Ævar Arnfjörð Bjarmason","SZEDER Gábor","Junio C Hamano","Eric Sunshine","Derrick Stolee","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"369862","messageId":"20190221223753.20070-1-avarab@gmail.com","threadId":"50554","inReplyTo":null,"subject":"[PATCH 0/8] commit-graph: segfault & other fixes for broken graphs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:45Z","receivedAt":"2019-02-21T22:38:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This one isn't for 2.21.0, these issues are already in v2.20.0, I just\nspotted them when looking at & fixing the commit-graph test broken on\nNetBSD[1].\n\nThis series touches (among other things) some of the same test code,\nbut merges cleanly with that \"dd\" fix, and it's not required for\ntesting it (unless you're on NetBSD).\n\nInstrumenting the commit-graph tests reveals that if you have a broken\ncommit-graph git will either segfault or abort early with cryptic\nerrors on such basic commands as \"status\".\n\nFurthemore, if our graph is corrupt and we've set\ncore.commitGraph=true we can't even write a new commit-graph, because\nwriting a new one has grown to implicitly depend on reading the old\none!\n\nThis series fixes all of that.\n\n1. https://public-inbox.org/git/20190221192849.6581-3-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (8):\n commit-graph tests: split up corrupt_graph_and_verify()\n commit-graph tests: test a graph that's too small\n commit-graph: fix segfault on e.g. \"git status\"\n commit-graph: don't early exit(1) on e.g. \"git status\"\n commit-graph: don't pass filename to load_commit_graph_one_fd_st()\n commit-graph verify: detect inability to read the graph\n commit-graph write: don't die if the existing graph is corrupt\n commit-graph: improve & i18n error messages\n\n builtin/commit-graph.c | 23 +++++--\n commit-graph.c | 132 +++++++++++++++++++++++++++-------------\n commit-graph.h | 4 ++\n commit.h | 6 ++\n t/t5318-commit-graph.sh | 48 +++++++++++++--\n 5 files changed, 160 insertions(+), 53 deletions(-)\n\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369863","messageId":"20190221223753.20070-2-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 1/8] commit-graph tests: split up corrupt_graph_and_verify()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:46Z","receivedAt":"2019-02-21T22:38:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Split up the corrupt_graph_and_verify() function added in\nd9b9f8a6fd (\"commit-graph: verify catches corrupt signature\",\n2018-06-27) into its logical components of setting up the test itself,\ndoing the corruption in a particular way with \"dd\", and then finally\ntesting that stderr is what we expect.\n\nThis allows for re-using everything except the now slimmer\ncorrupt_graph_and_verify() to corrupt the graph in a way that doesn't\ninvolve inserting a given byte sequence at a given position,\ne.g. truncating it entirely to a custom value.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5318-commit-graph.sh | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d4bd1522fe..725b7ce51f 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -366,6 +366,19 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n+corrupt_graph_setup() {\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n+\tcp $objdir/info/commit-graph commit-graph-backup\n+}\n+\n+corrupt_graph_verify() {\n+\tgrepstr=$1\n+\ttest_must_fail git commit-graph verify 2>test_err &&\n+\tgrep -v \"^+\" test_err >err &&\n+\ttest_i18ngrep \"$grepstr\" err\n+}\n+\n # usage: corrupt_graph_and_verify []\n # Manipulates the commit-graph file at the position\n # by inserting the data, optionally zeroing the file\n@@ -376,17 +389,14 @@ corrupt_graph_and_verify() {\n \tpos=$1\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n-\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tcorrupt_graph_setup &&\n \torig_size=$(wc -c < $objdir/info/commit-graph) &&\n \tzero_pos=${4:-${orig_size}} &&\n-\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n-\tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n \tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n \tgenerate_zero_bytes $(($orig_size - $zero_pos)) >>\"$objdir/info/commit-graph\" &&\n-\ttest_must_fail git commit-graph verify 2>test_err &&\n-\tgrep -v \"^+\" test_err >err &&\n-\ttest_i18ngrep \"$grepstr\" err\n+\tcorrupt_graph_verify \"$grepstr\"\n+\n }\n \n test_expect_success 'detect bad signature' '\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369864","messageId":"20190221223753.20070-3-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 2/8] commit-graph tests: test a graph that's too small","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:47Z","receivedAt":"2019-02-21T22:38:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the recently split-up components of the corrupt_graph_and_verify()\nfunction to assert that we error on graphs that are too small. The\nerror was added in 2a2e32bdc5 (\"commit-graph: implement git\ncommit-graph read\", 2018-04-10), but there was no test for it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5318-commit-graph.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 725b7ce51f..733be2ed30 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -399,6 +399,12 @@ corrupt_graph_and_verify() {\n \n }\n \n+test_expect_success 'detect too small' '\n+\tcorrupt_graph_setup &&\n+\techo \"a small graph\" >$objdir/info/commit-graph &&\n+\tcorrupt_graph_verify \"too small\"\n+'\n+\n test_expect_success 'detect bad signature' '\n \tcorrupt_graph_and_verify 0 \"\\0\" \\\n \t\t\"graph signature\"\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369865","messageId":"20190221223753.20070-4-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 3/8] commit-graph: fix segfault on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:48Z","receivedAt":"2019-02-21T22:38:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When core.commitGraph=true is set, various common commands now consult\nthe commit graph. Because the commit-graph code is very trusting of\nits input data, it's possibly to construct a graph that'll cause an\nimmediate segfault on e.g. \"status\" (and e.g. \"log\", \"blame\", ...). In\nsome other cases where git immediately exits with a cryptic error\nabout the graph being broken.\n\nThe root cause of this is that while the \"commit-graph verify\"\nsub-command exhaustively verifies the graph, other users of the graph\nsimply trust the graph, and will e.g. deference data found at certain\noffsets as pointers, causing segfaults.\n\nThis change does the bare minimum to ensure that we don't segfault in\nthe common fill_commit_in_graph() codepath called by\ne.g. setup_revisions(), to do this instrument the \"commit-graph\nverify\" tests to always check if \"status\" would subsequently\nsegfault. This fixes the following tests which would previously\nsegfault:\n\n not ok 50 - detect low chunk count\n not ok 51 - detect missing OID fanout chunk\n not ok 52 - detect missing OID lookup chunk\n not ok 53 - detect missing commit data chunk\n\nThose happened because with the commit-graph enabled setup_revisions()\nwould eventually call fill_commit_in_graph(), where e.g.\ng->chunk_commit_data is used early as an offset (and will be\n0x0). With this change we get far enough to detect that the graph is\nbroken, and show an error instead. E.g.:\n\n $ git status; echo $?\n error: commit-graph is missing the Commit Data chunk\n 1\n\nThat also sucks, we should *warn* and not hard-fail \"status\" just\nbecause the commit-graph is corrupt, but fixing is left to a follow-up\nchange.\n\nA side-effect of changing the reporting from graph_report() to error()\nis that we now have an \"error: \" prefix for these even for\n\"commit-graph verify\". Pseudo-diff before/after:\n\n $ git commit-graph verify\n -commit-graph is missing the Commit Data chunk\n +error: commit-graph is missing the Commit Data chunk\n\nChanging that is OK. Various errors it emits now early on are prefixed\nwith \"error: \", moving these over and changing the output doesn't\nbreak anything.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 43 ++++++++++++++++++++++++++++++++---------\n commit-graph.h | 1 +\n t/t5318-commit-graph.sh | 3 ++-\n 3 files changed, 37 insertions(+), 10 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 47e9be0a3a..980fbf47ea 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -233,6 +233,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tlast_chunk_offset = chunk_offset;\n \t}\n \n+\tif (verify_commit_graph_lite(graph))\n+\t\treturn NULL;\n+\n \treturn graph;\n }\n \n@@ -1075,6 +1078,36 @@ 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_lite(struct commit_graph *g)\n+{\n+\t/*\n+\t * Basic validation shared between parse_commit_graph()\n+\t * which'll be called every time the graph is used, and the\n+\t * much more expensive verify_commit_graph() used by\n+\t * \"commit-graph verify\".\n+\t *\n+\t * There should only be very basic checks here to ensure that\n+\t * we don't e.g. segfault in fill_commit_in_graph(), but\n+\t * because this is a very hot codepath nothing that e.g. loops\n+\t * over g->num_commits, or runs a checksum on the commit-graph\n+\t * itself.\n+\t */\n+\tif (!g->chunk_oid_fanout) {\n+\t\terror(\"commit-graph is missing the OID Fanout chunk\");\n+\t\treturn 1;\n+\t}\n+\tif (!g->chunk_oid_lookup) {\n+\t\terror(\"commit-graph is missing the OID Lookup chunk\");\n+\t\treturn 1;\n+\t}\n+\tif (!g->chunk_commit_data) {\n+\t\terror(\"commit-graph is missing the Commit Data chunk\");\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n int verify_commit_graph(struct repository *r, struct commit_graph *g)\n {\n \tuint32_t i, cur_fanout_pos = 0;\n@@ -1089,15 +1122,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\treturn 1;\n \t}\n \n-\tverify_commit_graph_error = 0;\n-\n-\tif (!g->chunk_oid_fanout)\n-\t\tgraph_report(\"commit-graph is missing the OID Fanout chunk\");\n-\tif (!g->chunk_oid_lookup)\n-\t\tgraph_report(\"commit-graph is missing the OID Lookup chunk\");\n-\tif (!g->chunk_commit_data)\n-\t\tgraph_report(\"commit-graph is missing the Commit Data chunk\");\n-\n+\tverify_commit_graph_error = verify_commit_graph_lite(g);\n \tif (verify_commit_graph_error)\n \t\treturn verify_commit_graph_error;\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 096d8bac34..275f97d006 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -70,6 +70,7 @@ void write_commit_graph(const char *obj_dir,\n \t\t\tstruct string_list *commit_hex,\n \t\t\tint append, int report_progress);\n \n+int verify_commit_graph_lite(struct commit_graph *g);\n int verify_commit_graph(struct repository *r, struct commit_graph *g);\n \n void close_commit_graph(struct repository *);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 733be2ed30..b3f3e515fc 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -376,7 +376,8 @@ corrupt_graph_verify() {\n \tgrepstr=$1\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n-\ttest_i18ngrep \"$grepstr\" err\n+\ttest_i18ngrep \"$grepstr\" err &&\n+\ttest_might_fail git status --short\n }\n \n # usage: corrupt_graph_and_verify []\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369866","messageId":"20190221223753.20070-5-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 4/8] commit-graph: don't early exit(1) on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:49Z","receivedAt":"2019-02-21T22:38:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the commit-graph loading code work as a library that returns an\nerror code instead of calling exit(1) when the commit-graph is\ncorrupt. This means that e.g. \"status\" will now report commit-graph\ncorruption as an \"error: [...]\" at the top of its output, but then\nproceed to work normally.\n\nThis required splitting up the load_commit_graph_one() function so\nthat the code that deals with open()-ing and stat()-ing the graph can\nnow be called independently as open_commit_graph().\n\nThis is needed because \"commit-graph verify\" where the graph doesn't\nexist isn't an error. See the third paragraph in\n283e68c72f (\"commit-graph: add 'verify' subcommand\",\n2018-06-27). There's a bug in that logic where we conflate the\nintended ENOENT with other errno values (e.g. EACCES), but this change\ndoesn't address that. That'll be addressed in a follow-up change.\n\nI'm then splitting most of the logic out of load_commit_graph_one()\ninto load_commit_graph_one_fd_st(), which allows for providing an\nexisting file descriptor and stat information to the loading\ncode. This isn't strictly needed, but it would be redundant and\nconfusing to open() and stat() the file twice for some of the\ncodepaths, this allows for calling open_commit_graph() followed by\nload_commit_graph_one_fd_st(). The \"graph_file\" still needs to be\npassed to that function for the the \"graph file %s is too small\" error\nmessage.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 21 +++++++++++++++++----\n commit-graph.c | 42 +++++++++++++++++++++++++++++------------\n commit-graph.h | 3 +++\n t/t5318-commit-graph.sh | 2 +-\n 4 files changed, 51 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 4ae502754c..32bcc63427 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n \tchar *graph_name;\n+\tint open_ok;\n+\tint fd;\n+\tstruct stat st;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n-\tgraph = load_commit_graph_one(graph_name);\n+\topen_ok = open_commit_graph(graph_name, &fd, &st);\n+\tif (!open_ok)\n+\t\treturn 0;\n+\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n \tFREE_AND_NULL(graph_name);\n \n \tif (!graph)\n-\t\treturn 0;\n+\t\treturn 1;\n \n \tUNLEAK(graph);\n \treturn verify_commit_graph(the_repository, graph);\n@@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n \tchar *graph_name;\n+\tint open_ok;\n+\tint fd;\n+\tstruct stat st;\n \n \tstatic struct option builtin_commit_graph_read_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n-\tgraph = load_commit_graph_one(graph_name);\n \n+\topen_ok = open_commit_graph(graph_name, &fd, &st);\n+\tif (!open_ok)\n+\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n+\n+\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n \tif (!graph)\n-\t\tdie(\"graph file %s does not exist\", graph_name);\n+\t\treturn 1;\n \n \tFREE_AND_NULL(graph_name);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 980fbf47ea..b1ba7a09cc 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one(const char *graph_file)\n+int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n+{\n+\t*fd = git_open(graph_file);\n+\tif (*fd < 0)\n+\t\treturn 0;\n+\tif (fstat(*fd, st)) {\n+\t\tclose(*fd);\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n+\t\t\t\t\t\t int fd, struct stat *st)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n-\tstruct stat st;\n \tstruct commit_graph *ret;\n-\tint fd = git_open(graph_file);\n \n-\tif (fd < 0)\n-\t\treturn NULL;\n-\tif (fstat(fd, &st)) {\n-\t\tclose(fd);\n-\t\treturn NULL;\n-\t}\n-\tgraph_size = xsize_t(st.st_size);\n+\tgraph_size = xsize_t(st->st_size);\n \n \tif (graph_size < GRAPH_MIN_SIZE) {\n \t\tclose(fd);\n-\t\tdie(_(\"graph file %s is too small\"), graph_file);\n+\t\terror(_(\"graph file %s is too small\"), graph_file);\n+\t\treturn 0;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tret = parse_commit_graph(graph_map, fd, graph_size);\n@@ -106,12 +112,24 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!ret) {\n \t\tmunmap(graph_map, graph_size);\n \t\tclose(fd);\n-\t\texit(1);\n \t}\n \n \treturn ret;\n }\n \n+struct commit_graph *load_commit_graph_one(const char *graph_file)\n+{\n+\n+\tstruct stat st;\n+\tint fd;\n+\tint open_ok = open_commit_graph(graph_file, &fd, &st);\n+\n+\tif (!open_ok)\n+\t\treturn NULL;\n+\n+\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n+}\n+\n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size)\n {\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 275f97d006..e4f17071e2 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -11,6 +11,7 @@\n struct commit;\n \n char *get_commit_graph_filename(const char *obj_dir);\n+int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n \n /*\n * Given a commit struct, try to fill the commit struct info, including:\n@@ -53,6 +54,8 @@ struct commit_graph {\n };\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n+\t\t\t\t\t\t int fd, struct stat *st);\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex b3f3e515fc..0d012f55e5 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -377,7 +377,7 @@ corrupt_graph_verify() {\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err &&\n-\ttest_might_fail git status --short\n+\tgit status --short\n }\n \n # usage: corrupt_graph_and_verify []\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369867","messageId":"20190221223753.20070-7-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 6/8] commit-graph verify: detect inability to read the graph","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:51Z","receivedAt":"2019-02-21T22:38:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change \"commit-graph verify\" to error on open() failures other than\nENOENT. As noted in the third paragraph of 283e68c72f (\"commit-graph:\nadd 'verify' subcommand\", 2018-06-27) and the test it added it's\nintentional that \"commit-graph verify\" doesn't error out when the file\ndoesn't exist.\n\nBut let's not be overly promiscuous in what we accept. If we can't\nread the file for other reasons, e.g. permission errors, bad file\ndescriptor etc. we'd like to report an error to the user.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 +++-\n t/t5318-commit-graph.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 8196fdbe9c..537fdfd0f0 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -62,8 +62,10 @@ static int graph_verify(int argc, const char **argv)\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n-\tif (!open_ok)\n+\tif (!open_ok && errno == ENOENT)\n \t\treturn 0;\n+\tif (!open_ok)\n+\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tFREE_AND_NULL(graph_name);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 0d012f55e5..1ee00fa333 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -400,6 +400,18 @@ corrupt_graph_and_verify() {\n \n }\n \n+test_expect_success 'detect permission problem' '\n+\tcorrupt_graph_setup &&\n+\tchmod 000 $objdir/info/commit-graph &&\n+\n+\t# Skip as root, or in other cases (odd fs or OS) where a\n+\t# \"chmod 000 file\" does not yield EACCES on e.g. \"cat file\"\n+\tif ! test -r $objdir/info/commit-graph\n+\tthen\n+\t\tcorrupt_graph_verify \"Could not open\"\n+\tfi\n+'\n+\n test_expect_success 'detect too small' '\n \tcorrupt_graph_setup &&\n \techo \"a small graph\" >$objdir/info/commit-graph &&\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369868","messageId":"20190221223753.20070-6-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 5/8] commit-graph: don't pass filename to load_commit_graph_one_fd_st()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:50Z","receivedAt":"2019-02-21T22:38:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"An earlier change implemented load_commit_graph_one_fd_st() in a way\nthat was bug-compatible with earlier code in terms of the \"graph file\n%s is too small\" error message printing out the path to the\ncommit-graph (\".git/objects/info/commit-graph\").\n\nBut change that, because:\n\n * A function that takes an already-open file descriptor also needing\n the filename isn't very intuitive.\n\n * The vast majority of errors we might emit when loading the graph\n come from parse_commit_graph(), which doesn't report the\n filename. Let's not do that either in this case for consistency.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ++--\n commit-graph.c | 7 +++----\n commit-graph.h | 3 +--\n 3 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 32bcc63427..8196fdbe9c 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -64,7 +64,7 @@ static int graph_verify(int argc, const char **argv)\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok)\n \t\treturn 0;\n-\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tFREE_AND_NULL(graph_name);\n \n \tif (!graph)\n@@ -102,7 +102,7 @@ static int graph_read(int argc, const char **argv)\n \tif (!open_ok)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \n-\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tif (!graph)\n \t\treturn 1;\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b1ba7a09cc..d945e8f3e0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -92,8 +92,7 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n-\t\t\t\t\t\t int fd, struct stat *st)\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n@@ -103,7 +102,7 @@ struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n \n \tif (graph_size < GRAPH_MIN_SIZE) {\n \t\tclose(fd);\n-\t\terror(_(\"graph file %s is too small\"), graph_file);\n+\t\terror(_(\"commit-graph file is too small\"));\n \t\treturn 0;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n@@ -127,7 +126,7 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!open_ok)\n \t\treturn NULL;\n \n-\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n+\treturn load_commit_graph_one_fd_st(fd, &st);\n }\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\ndiff --git a/commit-graph.h b/commit-graph.h\nindex e4f17071e2..36d8109901 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -54,8 +54,7 @@ struct commit_graph {\n };\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n-\t\t\t\t\t\t int fd, struct stat *st);\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369869","messageId":"20190221223753.20070-8-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:52Z","receivedAt":"2019-02-21T22:38:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the commit-graph is written we end up calling\nparse_commit(). This will in turn invoke code that'll consult the\nexisting commit-graph about the commit, if the graph is corrupted we\ndie.\n\nWe thus get into a state where a failing \"commit-graph verify\" can't\nbe followed-up with a \"commit-graph write\" if core.commitGraph=true is\nset, the graph either needs to be manually removed to proceed, or\ncore.commitGraph needs to be set to \"false\".\n\nChange the \"commit-graph write\" codepath to use a new\nparse_commit_no_graph() helper instead of parse_commit() to avoid\nthis. The latter will call repo_parse_commit_internal() with\nuse_commit_graph=1 as seen in 177722b344 (\"commit: integrate commit\ngraph with commit parsing\", 2018-04-10).\n\nJust fixing the current issue would be likely to result in code that's\ninadvertently broken in the future. New code might use the\ncommit-graph at a distance. To detect such cases introduce a\n\"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\" setting used when we do our\ncorruption tests, and test that a \"write/verify\" combo works after\nevery one of our current test cases where we now detect commit-graph\ncorruption.\n\nSome of the code changes here might be strictly unnecessary, e.g. I\nwas unable to find cases where the parse_commit() called from\nwrite_graph_chunk_data() didn't exit early due to\n\"item->object.parsed\" being true in\nrepo_parse_commit_internal() (before the use_commit_graph=1 has any\neffect). But let's also convert those cases for good measure, we do\nnot have exhaustive tests for all possible types of commit-graph\ncorruption.\n\nThis might need to be re-visited if we learn to write the commit-graph\nincrementally.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 10 +++++++---\n commit-graph.h | 1 +\n commit.h | 6 ++++++\n t/t5318-commit-graph.sh | 11 +++++++++--\n 4 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex d945e8f3e0..6b3ade9496 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -281,6 +281,10 @@ static int prepare_commit_graph(struct repository *r)\n \tstruct object_directory *odb;\n \tint config_value;\n \n+\tif (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n+\t\tdie(\"Dying as requested by the '%s' variable on commit-graph load!\",\n+\t\t GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n+\n \tif (r->objects->commit_graph_attempted)\n \t\treturn !!r->objects->commit_graph;\n \tr->objects->commit_graph_attempted = 1;\n@@ -545,7 +549,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(progress, ++*progress_cnt);\n \n-\t\tparse_commit(*list);\n+\t\tparse_commit_no_graph(*list);\n \t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n \n \t\tparent = (*list)->parents;\n@@ -742,7 +746,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)\n \t\tdisplay_progress(progress, i + 1);\n \t\tcommit = lookup_commit(the_repository, &oids->list[i]);\n \n-\t\tif (commit && !parse_commit(commit))\n+\t\tif (commit && !parse_commit_no_graph(commit))\n \t\t\tadd_missing_parents(oids, commit);\n \t}\n \tstop_progress(&progress);\n@@ -991,7 +995,7 @@ void write_commit_graph(const char *obj_dir,\n \t\t\tcontinue;\n \n \t\tcommits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);\n-\t\tparse_commit(commits.list[commits.nr]);\n+\t\tparse_commit_no_graph(commits.list[commits.nr]);\n \n \t\tfor (parent = commits.list[commits.nr]->parents;\n \t\t parent; parent = parent->next)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 36d8109901..6021ababa2 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -7,6 +7,7 @@\n #include \"cache.h\"\n \n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n+#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n \n struct commit;\n \ndiff --git a/commit.h b/commit.h\nindex 42728c2906..5d33477e78 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)\n {\n \treturn repo_parse_commit_gently(r, item, 0);\n }\n+\n+static inline int parse_commit_no_graph(struct commit *commit)\n+{\n+\treturn repo_parse_commit_internal(the_repository, commit, 0, 0);\n+}\n+\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)\n #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 1ee00fa333..6db658ed66 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -377,7 +377,13 @@ corrupt_graph_verify() {\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err &&\n-\tgit status --short\n+\tif test -z \"$NO_WRITE_TEST_BACKUP\"\n+\tthen\n+\t\tcp $objdir/info/commit-graph commit-graph-pre-write-test\n+\tfi &&\n+\tgit status --short &&\n+\tGIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&\n+\tgit commit-graph verify\n }\n \n # usage: corrupt_graph_and_verify []\n@@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' '\n \t# \"chmod 000 file\" does not yield EACCES on e.g. \"cat file\"\n \tif ! test -r $objdir/info/commit-graph\n \tthen\n-\t\tcorrupt_graph_verify \"Could not open\"\n+\t\tNO_WRITE_TEST_BACKUP=1 corrupt_graph_verify \"Could not open\"\n \tfi\n '\n \n@@ -528,6 +534,7 @@ test_expect_success 'git fsck (checks commit-graph)' '\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n \ttest_must_fail git fsck\n '\n \n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369870","messageId":"20190221223753.20070-9-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH 8/8] commit-graph: improve & i18n error messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T22:37:53Z","receivedAt":"2019-02-21T22:38:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the error emitted when a commit-graph file is corrupt so that\nwe actually mention the commit-graph, e.g. change errors like:\n\n error: improper chunk offset 0000000000385e0c\n\nTo:\n\n error: commit-graph improper chunk offset 0000000000385e0c\n\nAs discussed in the commits leading up to this one the commit-graph\nmachinery is now used by common commands like \"status\". If the graph\nwas corrupt we'd often emit some error that gave no indication what\nwas wrong. Now some of them are still cryptic, but they'll at least\nmention \"commit-graph\" to give the user a hint as to where to look.\n\nWhile I'm at it mark some of the strings that hadn't been marked for\ntranslation. It's clear from the commit history and the code that this\nwas merely forgotten at the time, and wasn't intentional.p5\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 38 +++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6b3ade9496..4cf7d6c294 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -150,21 +150,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n-\t\terror(_(\"graph signature %X does not match signature %X\"),\n+\t\terror(_(\"commit-graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n \t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n-\t\terror(_(\"graph version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n \t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != oid_version()) {\n-\t\terror(_(\"hash version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph hash version %X does not match version %X\"),\n \t\t hash_version, oid_version());\n \t\treturn NULL;\n \t}\n@@ -187,7 +187,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \t\tif (data + graph_size - chunk_lookup <\n \t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n-\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\terror(_(\"commit-graph chunk lookup table entry missing; file may be incomplete\"));\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\n@@ -198,7 +198,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - the_hash_algo->rawsz) {\n-\t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n+\t\t\terror(_(\"commit-graph improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n@@ -235,7 +235,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t}\n \n \t\tif (chunk_repeated) {\n-\t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n+\t\t\terror(_(\"commit-graph chunk id %08x appears multiple times\"), chunk_id);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\n@@ -1162,7 +1162,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n \n \t\tif (i && oidcmp(&prev_oid, &cur_oid) >= 0)\n-\t\t\tgraph_report(\"commit-graph has incorrect OID order: %s then %s\",\n+\t\t\tgraph_report(_(\"commit-graph has incorrect OID order: %s then %s\"),\n \t\t\t\t oid_to_hex(&prev_oid),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n@@ -1172,14 +1172,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t\tuint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);\n \n \t\t\tif (i != fanout_value)\n-\t\t\t\tgraph_report(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\",\n+\t\t\t\tgraph_report(_(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\"),\n \t\t\t\t\t cur_fanout_pos, fanout_value, i);\n \t\t\tcur_fanout_pos++;\n \t\t}\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \t\tif (!parse_commit_in_graph_one(r, g, graph_commit))\n-\t\t\tgraph_report(\"failed to parse %s from commit-graph\",\n+\t\t\tgraph_report(_(\"failed to parse commit %s from commit-graph\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \t}\n \n@@ -1187,7 +1187,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\tuint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);\n \n \t\tif (g->num_commits != fanout_value)\n-\t\t\tgraph_report(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\",\n+\t\t\tgraph_report(_(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\"),\n \t\t\t\t cur_fanout_pos, fanout_value, i);\n \n \t\tcur_fanout_pos++;\n@@ -1209,14 +1209,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \t\todb_commit = (struct commit *)create_object(r, cur_oid.hash, alloc_commit_node(r));\n \t\tif (parse_commit_internal(odb_commit, 0, 0)) {\n-\t\t\tgraph_report(\"failed to parse %s from object database\",\n+\t\t\tgraph_report(_(\"failed to parse commit %s from object database for commit-graph\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!oideq(&get_commit_tree_in_graph_one(r, g, graph_commit)->object.oid,\n \t\t\t get_commit_tree_oid(odb_commit)))\n-\t\t\tgraph_report(\"root tree OID for commit %s in commit-graph is %s != %s\",\n+\t\t\tgraph_report(_(\"root tree OID for commit %s in commit-graph is %s != %s\"),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t oid_to_hex(get_commit_tree_oid(graph_commit)),\n \t\t\t\t oid_to_hex(get_commit_tree_oid(odb_commit)));\n@@ -1226,13 +1226,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \n \t\twhile (graph_parents) {\n \t\t\tif (odb_parents == NULL) {\n-\t\t\t\tgraph_report(\"commit-graph parent list for commit %s is too long\",\n+\t\t\t\tgraph_report(_(\"commit-graph parent list for commit %s is too long\"),\n \t\t\t\t\t oid_to_hex(&cur_oid));\n \t\t\t\tbreak;\n \t\t\t}\n \n \t\t\tif (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid))\n-\t\t\t\tgraph_report(\"commit-graph parent for %s is %s != %s\",\n+\t\t\t\tgraph_report(_(\"commit-graph parent for %s is %s != %s\"),\n \t\t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t\t oid_to_hex(&graph_parents->item->object.oid),\n \t\t\t\t\t oid_to_hex(&odb_parents->item->object.oid));\n@@ -1245,16 +1245,16 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t}\n \n \t\tif (odb_parents != NULL)\n-\t\t\tgraph_report(\"commit-graph parent list for commit %s terminates early\",\n+\t\t\tgraph_report(_(\"commit-graph parent list for commit %s terminates early\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n \t\tif (!graph_commit->generation) {\n \t\t\tif (generation_zero == GENERATION_NUMBER_EXISTS)\n-\t\t\t\tgraph_report(\"commit-graph has generation number zero for commit %s, but non-zero elsewhere\",\n+\t\t\t\tgraph_report(_(\"commit-graph has generation number zero for commit %s, but non-zero elsewhere\"),\n \t\t\t\t\t oid_to_hex(&cur_oid));\n \t\t\tgeneration_zero = GENERATION_ZERO_EXISTS;\n \t\t} else if (generation_zero == GENERATION_ZERO_EXISTS)\n-\t\t\tgraph_report(\"commit-graph has non-zero generation number for commit %s, but zero elsewhere\",\n+\t\t\tgraph_report(_(\"commit-graph has non-zero generation number for commit %s, but zero elsewhere\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n \t\tif (generation_zero == GENERATION_ZERO_EXISTS)\n@@ -1269,13 +1269,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t\tmax_generation--;\n \n \t\tif (graph_commit->generation != max_generation + 1)\n-\t\t\tgraph_report(\"commit-graph generation for commit %s is %u != %u\",\n+\t\t\tgraph_report(_(\"commit-graph generation for commit %s is %u != %u\"),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t graph_commit->generation,\n \t\t\t\t max_generation + 1);\n \n \t\tif (graph_commit->date != odb_commit->date)\n-\t\t\tgraph_report(\"commit date for commit %s in commit-graph is %\"PRItime\" != %\"PRItime,\n+\t\t\tgraph_report(_(\"commit date for commit %s in commit-graph is %\"PRItime\" != %\"PRItime),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t graph_commit->date,\n \t\t\t\t odb_commit->date);\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369871","messageId":"20190221231525.GA13328@szeder.dev","threadId":"50554","inReplyTo":"20190221223753.20070-7-avarab@gmail.com","subject":"Re: [PATCH 6/8] commit-graph verify: detect inability to read the graph","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-21T23:15:25Z","receivedAt":"2019-02-21T23:15:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Feb 21, 2019 at 11:37:51PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 0d012f55e5..1ee00fa333 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -400,6 +400,18 @@ corrupt_graph_and_verify() {\n> \n> }\n> \n> +test_expect_success 'detect permission problem' '\n> +\tcorrupt_graph_setup &&\n> +\tchmod 000 $objdir/info/commit-graph &&\n\nThis needs the POSIXPERM prereq.\n\n> +\n> +\t# Skip as root, or in other cases (odd fs or OS) where a\n> +\t# \"chmod 000 file\" does not yield EACCES on e.g. \"cat file\"\n> +\tif ! test -r $objdir/info/commit-graph\n\nAnd this condition would be unnecessary with the SANITY prereq.\n\n> +\tthen\n> +\t\tcorrupt_graph_verify \"Could not open\"\n> +\tfi\n> +'\n> +\n> test_expect_success 'detect too small' '\n> \tcorrupt_graph_setup &&\n> \techo \"a small graph\" >$objdir/info/commit-graph &&\n> -- \n> 2.21.0.rc0.258.g878e2cd30e\n> \n"},{"id":"369999","messageId":"xmqqzhqno1r3.fsf@gitster-ct.c.googlers.com","threadId":"50554","inReplyTo":"20190221223753.20070-8-avarab@gmail.com","subject":"Re: [PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T23:13:36Z","receivedAt":"2019-02-22T23:13:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> When the commit-graph is written we end up calling\n> parse_commit(). This will in turn invoke code that'll consult the\n> existing commit-graph about the commit, if the graph is corrupted we\n> die.\n\nIrony ;-).\n\n> Change the \"commit-graph write\" codepath to use a new\n> parse_commit_no_graph() helper instead of parse_commit() to avoid\n> this. The latter will call repo_parse_commit_internal() with\n> use_commit_graph=1 as seen in 177722b344 (\"commit: integrate commit\n> graph with commit parsing\", 2018-04-10).\n\nThat may slow down writing the graph but would be a sensible way to\nprevent an error in the existing commit-graph from spreading.\n\n> This might need to be re-visited if we learn to write the commit-graph\n> incrementally.\n\nProbably yes. If we were doing \"repack -a -d (without -f)\" style of\n\"incremental\", then we do need to revisit this, which is a moral\nequivalent of saying \"do not reuse delta\" to \"repack\", and it would\nmake it impossible to be incremental.\n\nHopefully a true \"incremental\" shouldn't even have to touch existing\ndata, perhaps similar to \"repack (without -a)\", in which case the\nequation may be different. If we'd be computing reachability for\nonly new things, there is nothing gained by allowing parse_commit()\nto peek into existing commit-graph file (by definition, these new\nthings are not in there---that's the reason why we are incrementally\nextending the commit-graph by computing the reachability for them).\nI dunno.\n\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 1ee00fa333..6db658ed66 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -377,7 +377,13 @@ corrupt_graph_verify() {\n> \ttest_must_fail git commit-graph verify 2>test_err &&\n> \tgrep -v \"^+\" test_err >err &&\n> \ttest_i18ngrep \"$grepstr\" err &&\n> -\tgit status --short\n> +\tif test -z \"$NO_WRITE_TEST_BACKUP\"\n> +\tthen\n> +\t\tcp $objdir/info/commit-graph commit-graph-pre-write-test\n> +\tfi &&\n> +\tgit status --short &&\n> +\tGIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&\n> +\tgit commit-graph verify\n> }\n> \n> # usage: corrupt_graph_and_verify []\n> @@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' '\n> \t# \"chmod 000 file\" does not yield EACCES on e.g. \"cat file\"\n> \tif ! test -r $objdir/info/commit-graph\n> \tthen\n> -\t\tcorrupt_graph_verify \"Could not open\"\n> +\t\tNO_WRITE_TEST_BACKUP=1 corrupt_graph_verify \"Could not open\"\n\nThis would not work as you think it would; corrupt_graph_verify is a\nshell function, so you cannot VAR=VAL prefix to export an environment\nvariable only for the duration of the command.\n\n> \tfi\n> '\n> \n> @@ -528,6 +534,7 @@ test_expect_success 'git fsck (checks commit-graph)' '\n> \tgit fsck &&\n> \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> \t\t\"incorrect checksum\" &&\n> +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n> \ttest_must_fail git fsck\n> '\n"},{"id":"370007","messageId":"CAPig+cRb78wPKJGjWaMiUkazDK5LpidikDLKR8nbQ-+u1=2w=A@mail.gmail.com","threadId":"50554","inReplyTo":"xmqqzhqno1r3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-23T09:29:26Z","receivedAt":"2019-02-23T09:29:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 22, 2019 at 6:13 PM Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > @@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' '\n> > # \"chmod 000 file\" does not yield EACCES on e.g. \"cat file\"\n> > if ! test -r $objdir/info/commit-graph\n> > then\n> > - corrupt_graph_verify \"Could not open\"\n> > + NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify \"Could not open\"\n>\n> This would not work as you think it would; corrupt_graph_verify is a\n> shell function, so you cannot VAR=VAL prefix to export an environment\n> variable only for the duration of the command.\n\nAs of a0a630192d (t/check-non-portable-shell: detect \"FOO=bar\nshell_func\", 2018-07-13), this sort of problem is correctly flagged by\n\"make test-lint\", which typically is run as part of \"make test\" or\n\"make prove\".\n"},{"id":"371540","messageId":"20190314214740.23360-1-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 0/8] commit-graph: segfault & other fixes for broken graphs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:32Z","receivedAt":"2019-03-14T21:47:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"See the v1 cover letter for details:\nhttps://public-inbox.org/git/20190221223753.20070-1-avarab@gmail.com/\n\nI'd forgotten this after 2.21 was released.\n\nThis addresses all the comments on v1 and rebases it. A range-diff is\nbelow. I also improved 7/8's commit message a bit.\n\nI fixed a test to avoid the pattern a0a630192d\n(t/check-non-portable-shell: detect \"FOO=bar shell_func\", 2018-07-13)\ntests for. The new pattern is more obvious.\n\nAs an aside I don't get how that doesn't work as intended from the\ncommit message of that commit & its series.\n\n $ cat /tmp/x.sh \n sayit() { echo \"saying '$SAY'\"; };\n SAY=hi sayit; sayit;\n $ sh /tmp/x.sh\n saying 'hi'\n saying ''\n\nI get the same thing on bash, dash, NetBSD ksh, Solaris's shell &\nAIX's. I.e. it's explicitly not assigning $SAY for the duration of the\nshell as this would do:\n\n $ cat /tmp/y.sh \n sayit() { echo \"saying '$SAY'\"; };\n SAY=hi; sayit; sayit;\n $ sh /tmp/y.sh\n saying 'hi'\n saying 'hi'\n\nWhich is the impression I get from the commit message.\n\nÆvar Arnfjörð Bjarmason (8):\n commit-graph tests: split up corrupt_graph_and_verify()\n commit-graph tests: test a graph that's too small\n commit-graph: fix segfault on e.g. \"git status\"\n commit-graph: don't early exit(1) on e.g. \"git status\"\n commit-graph: don't pass filename to load_commit_graph_one_fd_st()\n commit-graph verify: detect inability to read the graph\n commit-graph write: don't die if the existing graph is corrupt\n commit-graph: improve & i18n error messages\n\n builtin/commit-graph.c | 23 +++++--\n commit-graph.c | 132 +++++++++++++++++++++++++++-------------\n commit-graph.h | 4 ++\n commit.h | 6 ++\n t/t5318-commit-graph.sh | 42 +++++++++++--\n 5 files changed, 154 insertions(+), 53 deletions(-)\n\nRange-diff:\n1: 9d318d5106 ! 1: 2f8ba0adf8 commit-graph tests: split up corrupt_graph_and_verify()\n @@ -49,7 +49,7 @@\n -\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n -\tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n - \tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n + \tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" if=/dev/null &&\n \tgenerate_zero_bytes $(($orig_size - $zero_pos)) >>\"$objdir/info/commit-graph\" &&\n -\ttest_must_fail git commit-graph verify 2>test_err &&\n -\tgrep -v \"^+\" test_err >err &&\n2: 73849add5e = 2: 800b17edde commit-graph tests: test a graph that's too small\n3: 6bfce758e1 = 3: 7083ab81c7 commit-graph: fix segfault on e.g. \"git status\"\n4: ac07ff415e = 4: d00564ae89 commit-graph: don't early exit(1) on e.g. \"git status\"\n5: b2dd394cc7 = 5: 25ee185bf7 commit-graph: don't pass filename to load_commit_graph_one_fd_st()\n6: 9987149e5c ! 6: 7619b46987 commit-graph verify: detect inability to read the graph\n @@ -37,16 +37,10 @@\n \n }\n \n -+test_expect_success 'detect permission problem' '\n ++test_expect_success POSIXPERM,SANITY 'detect permission problem' '\n +\tcorrupt_graph_setup &&\n +\tchmod 000 $objdir/info/commit-graph &&\n -+\n -+\t# Skip as root, or in other cases (odd fs or OS) where a\n -+\t# \"chmod 000 file\" does not yield EACCES on e.g. \"cat file\"\n -+\tif ! test -r $objdir/info/commit-graph\n -+\tthen\n -+\t\tcorrupt_graph_verify \"Could not open\"\n -+\tfi\n ++\tcorrupt_graph_verify \"Could not open\"\n +'\n +\n test_expect_success 'detect too small' '\n7: 0e35b12a1a ! 7: 17ee4fc050 commit-graph write: don't die if the existing graph is corrupt\n @@ -18,6 +18,10 @@\n use_commit_graph=1 as seen in 177722b344 (\"commit: integrate commit\n graph with commit parsing\", 2018-04-10).\n \n + Not using the old graph at all slows down the writing of the new graph\n + by some small amount, but is a sensible way to prevent an error in the\n + existing commit-graph from spreading.\n +\n Just fixing the current issue would be likely to result in code that's\n inadvertently broken in the future. New code might use the\n commit-graph at a distance. To detect such cases introduce a\n @@ -36,7 +40,12 @@\n corruption.\n \n This might need to be re-visited if we learn to write the commit-graph\n - incrementally.\n + incrementally, but probably not. Hopefully we'll just start by finding\n + out what commits we have in total, then read the old graph(s) to see\n + what they cover, and finally write a new graph file with everything\n + that's missing. In that case the new graph writing code just needs to\n + continue to use e.g. a parse_commit() that doesn't consult the\n + existing commit-graphs.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ -119,7 +128,7 @@\n \tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err &&\n -\tgit status --short\n -+\tif test -z \"$NO_WRITE_TEST_BACKUP\"\n ++\tif test \"$2\" != \"no-copy\"\n +\tthen\n +\t\tcp $objdir/info/commit-graph commit-graph-pre-write-test\n +\tfi &&\n @@ -130,14 +139,14 @@\n \n # usage: corrupt_graph_and_verify []\n @@\n - \t# \"chmod 000 file\" does not yield EACCES on e.g. \"cat file\"\n - \tif ! test -r $objdir/info/commit-graph\n - \tthen\n --\t\tcorrupt_graph_verify \"Could not open\"\n -+\t\tNO_WRITE_TEST_BACKUP=1 corrupt_graph_verify \"Could not open\"\n - \tfi\n + test_expect_success POSIXPERM,SANITY 'detect permission problem' '\n + \tcorrupt_graph_setup &&\n + \tchmod 000 $objdir/info/commit-graph &&\n +-\tcorrupt_graph_verify \"Could not open\"\n ++\tcorrupt_graph_verify \"Could not open\" \"no-copy\"\n '\n \n + test_expect_success 'detect too small' '\n @@\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n8: a74d0f0f6f = 8: 29ab2895b7 commit-graph: improve & i18n error messages\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371541","messageId":"20190314214740.23360-2-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 1/8] commit-graph tests: split up corrupt_graph_and_verify()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:33Z","receivedAt":"2019-03-14T21:47:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Split up the corrupt_graph_and_verify() function added in\nd9b9f8a6fd (\"commit-graph: verify catches corrupt signature\",\n2018-06-27) into its logical components of setting up the test itself,\ndoing the corruption in a particular way with \"dd\", and then finally\ntesting that stderr is what we expect.\n\nThis allows for re-using everything except the now slimmer\ncorrupt_graph_and_verify() to corrupt the graph in a way that doesn't\ninvolve inserting a given byte sequence at a given position,\ne.g. truncating it entirely to a custom value.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5318-commit-graph.sh | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 561796f280..56a616831e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -366,6 +366,19 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n+corrupt_graph_setup() {\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n+\tcp $objdir/info/commit-graph commit-graph-backup\n+}\n+\n+corrupt_graph_verify() {\n+\tgrepstr=$1\n+\ttest_must_fail git commit-graph verify 2>test_err &&\n+\tgrep -v \"^+\" test_err >err &&\n+\ttest_i18ngrep \"$grepstr\" err\n+}\n+\n # usage: corrupt_graph_and_verify []\n # Manipulates the commit-graph file at the position\n # by inserting the data, optionally zeroing the file\n@@ -376,17 +389,14 @@ corrupt_graph_and_verify() {\n \tpos=$1\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n-\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tcorrupt_graph_setup &&\n \torig_size=$(wc -c < $objdir/info/commit-graph) &&\n \tzero_pos=${4:-${orig_size}} &&\n-\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n-\tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n \tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" if=/dev/null &&\n \tgenerate_zero_bytes $(($orig_size - $zero_pos)) >>\"$objdir/info/commit-graph\" &&\n-\ttest_must_fail git commit-graph verify 2>test_err &&\n-\tgrep -v \"^+\" test_err >err &&\n-\ttest_i18ngrep \"$grepstr\" err\n+\tcorrupt_graph_verify \"$grepstr\"\n+\n }\n \n test_expect_success 'detect bad signature' '\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371542","messageId":"20190314214740.23360-3-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 2/8] commit-graph tests: test a graph that's too small","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:34Z","receivedAt":"2019-03-14T21:48:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the recently split-up components of the corrupt_graph_and_verify()\nfunction to assert that we error on graphs that are too small. The\nerror was added in 2a2e32bdc5 (\"commit-graph: implement git\ncommit-graph read\", 2018-04-10), but there was no test for it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5318-commit-graph.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 56a616831e..ce3459a6f5 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -399,6 +399,12 @@ corrupt_graph_and_verify() {\n \n }\n \n+test_expect_success 'detect too small' '\n+\tcorrupt_graph_setup &&\n+\techo \"a small graph\" >$objdir/info/commit-graph &&\n+\tcorrupt_graph_verify \"too small\"\n+'\n+\n test_expect_success 'detect bad signature' '\n \tcorrupt_graph_and_verify 0 \"\\0\" \\\n \t\t\"graph signature\"\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371543","messageId":"20190314214740.23360-4-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 3/8] commit-graph: fix segfault on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:35Z","receivedAt":"2019-03-14T21:48:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When core.commitGraph=true is set, various common commands now consult\nthe commit graph. Because the commit-graph code is very trusting of\nits input data, it's possibly to construct a graph that'll cause an\nimmediate segfault on e.g. \"status\" (and e.g. \"log\", \"blame\", ...). In\nsome other cases where git immediately exits with a cryptic error\nabout the graph being broken.\n\nThe root cause of this is that while the \"commit-graph verify\"\nsub-command exhaustively verifies the graph, other users of the graph\nsimply trust the graph, and will e.g. deference data found at certain\noffsets as pointers, causing segfaults.\n\nThis change does the bare minimum to ensure that we don't segfault in\nthe common fill_commit_in_graph() codepath called by\ne.g. setup_revisions(), to do this instrument the \"commit-graph\nverify\" tests to always check if \"status\" would subsequently\nsegfault. This fixes the following tests which would previously\nsegfault:\n\n not ok 50 - detect low chunk count\n not ok 51 - detect missing OID fanout chunk\n not ok 52 - detect missing OID lookup chunk\n not ok 53 - detect missing commit data chunk\n\nThose happened because with the commit-graph enabled setup_revisions()\nwould eventually call fill_commit_in_graph(), where e.g.\ng->chunk_commit_data is used early as an offset (and will be\n0x0). With this change we get far enough to detect that the graph is\nbroken, and show an error instead. E.g.:\n\n $ git status; echo $?\n error: commit-graph is missing the Commit Data chunk\n 1\n\nThat also sucks, we should *warn* and not hard-fail \"status\" just\nbecause the commit-graph is corrupt, but fixing is left to a follow-up\nchange.\n\nA side-effect of changing the reporting from graph_report() to error()\nis that we now have an \"error: \" prefix for these even for\n\"commit-graph verify\". Pseudo-diff before/after:\n\n $ git commit-graph verify\n -commit-graph is missing the Commit Data chunk\n +error: commit-graph is missing the Commit Data chunk\n\nChanging that is OK. Various errors it emits now early on are prefixed\nwith \"error: \", moving these over and changing the output doesn't\nbreak anything.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 43 ++++++++++++++++++++++++++++++++---------\n commit-graph.h | 1 +\n t/t5318-commit-graph.sh | 3 ++-\n 3 files changed, 37 insertions(+), 10 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 47e9be0a3a..980fbf47ea 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -233,6 +233,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tlast_chunk_offset = chunk_offset;\n \t}\n \n+\tif (verify_commit_graph_lite(graph))\n+\t\treturn NULL;\n+\n \treturn graph;\n }\n \n@@ -1075,6 +1078,36 @@ 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_lite(struct commit_graph *g)\n+{\n+\t/*\n+\t * Basic validation shared between parse_commit_graph()\n+\t * which'll be called every time the graph is used, and the\n+\t * much more expensive verify_commit_graph() used by\n+\t * \"commit-graph verify\".\n+\t *\n+\t * There should only be very basic checks here to ensure that\n+\t * we don't e.g. segfault in fill_commit_in_graph(), but\n+\t * because this is a very hot codepath nothing that e.g. loops\n+\t * over g->num_commits, or runs a checksum on the commit-graph\n+\t * itself.\n+\t */\n+\tif (!g->chunk_oid_fanout) {\n+\t\terror(\"commit-graph is missing the OID Fanout chunk\");\n+\t\treturn 1;\n+\t}\n+\tif (!g->chunk_oid_lookup) {\n+\t\terror(\"commit-graph is missing the OID Lookup chunk\");\n+\t\treturn 1;\n+\t}\n+\tif (!g->chunk_commit_data) {\n+\t\terror(\"commit-graph is missing the Commit Data chunk\");\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n int verify_commit_graph(struct repository *r, struct commit_graph *g)\n {\n \tuint32_t i, cur_fanout_pos = 0;\n@@ -1089,15 +1122,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\treturn 1;\n \t}\n \n-\tverify_commit_graph_error = 0;\n-\n-\tif (!g->chunk_oid_fanout)\n-\t\tgraph_report(\"commit-graph is missing the OID Fanout chunk\");\n-\tif (!g->chunk_oid_lookup)\n-\t\tgraph_report(\"commit-graph is missing the OID Lookup chunk\");\n-\tif (!g->chunk_commit_data)\n-\t\tgraph_report(\"commit-graph is missing the Commit Data chunk\");\n-\n+\tverify_commit_graph_error = verify_commit_graph_lite(g);\n \tif (verify_commit_graph_error)\n \t\treturn verify_commit_graph_error;\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 096d8bac34..275f97d006 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -70,6 +70,7 @@ void write_commit_graph(const char *obj_dir,\n \t\t\tstruct string_list *commit_hex,\n \t\t\tint append, int report_progress);\n \n+int verify_commit_graph_lite(struct commit_graph *g);\n int verify_commit_graph(struct repository *r, struct commit_graph *g);\n \n void close_commit_graph(struct repository *);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ce3459a6f5..ad3a695f76 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -376,7 +376,8 @@ corrupt_graph_verify() {\n \tgrepstr=$1\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n-\ttest_i18ngrep \"$grepstr\" err\n+\ttest_i18ngrep \"$grepstr\" err &&\n+\ttest_might_fail git status --short\n }\n \n # usage: corrupt_graph_and_verify []\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371544","messageId":"20190314214740.23360-6-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 5/8] commit-graph: don't pass filename to load_commit_graph_one_fd_st()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:37Z","receivedAt":"2019-03-14T21:48:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"An earlier change implemented load_commit_graph_one_fd_st() in a way\nthat was bug-compatible with earlier code in terms of the \"graph file\n%s is too small\" error message printing out the path to the\ncommit-graph (\".git/objects/info/commit-graph\").\n\nBut change that, because:\n\n * A function that takes an already-open file descriptor also needing\n the filename isn't very intuitive.\n\n * The vast majority of errors we might emit when loading the graph\n come from parse_commit_graph(), which doesn't report the\n filename. Let's not do that either in this case for consistency.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ++--\n commit-graph.c | 7 +++----\n commit-graph.h | 3 +--\n 3 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 32bcc63427..8196fdbe9c 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -64,7 +64,7 @@ static int graph_verify(int argc, const char **argv)\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok)\n \t\treturn 0;\n-\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tFREE_AND_NULL(graph_name);\n \n \tif (!graph)\n@@ -102,7 +102,7 @@ static int graph_read(int argc, const char **argv)\n \tif (!open_ok)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \n-\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tif (!graph)\n \t\treturn 1;\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b1ba7a09cc..d945e8f3e0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -92,8 +92,7 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n-\t\t\t\t\t\t int fd, struct stat *st)\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n@@ -103,7 +102,7 @@ struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n \n \tif (graph_size < GRAPH_MIN_SIZE) {\n \t\tclose(fd);\n-\t\terror(_(\"graph file %s is too small\"), graph_file);\n+\t\terror(_(\"commit-graph file is too small\"));\n \t\treturn 0;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n@@ -127,7 +126,7 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!open_ok)\n \t\treturn NULL;\n \n-\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n+\treturn load_commit_graph_one_fd_st(fd, &st);\n }\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\ndiff --git a/commit-graph.h b/commit-graph.h\nindex e4f17071e2..36d8109901 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -54,8 +54,7 @@ struct commit_graph {\n };\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n-\t\t\t\t\t\t int fd, struct stat *st);\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371545","messageId":"20190314214740.23360-5-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 4/8] commit-graph: don't early exit(1) on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:36Z","receivedAt":"2019-03-14T21:48:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the commit-graph loading code work as a library that returns an\nerror code instead of calling exit(1) when the commit-graph is\ncorrupt. This means that e.g. \"status\" will now report commit-graph\ncorruption as an \"error: [...]\" at the top of its output, but then\nproceed to work normally.\n\nThis required splitting up the load_commit_graph_one() function so\nthat the code that deals with open()-ing and stat()-ing the graph can\nnow be called independently as open_commit_graph().\n\nThis is needed because \"commit-graph verify\" where the graph doesn't\nexist isn't an error. See the third paragraph in\n283e68c72f (\"commit-graph: add 'verify' subcommand\",\n2018-06-27). There's a bug in that logic where we conflate the\nintended ENOENT with other errno values (e.g. EACCES), but this change\ndoesn't address that. That'll be addressed in a follow-up change.\n\nI'm then splitting most of the logic out of load_commit_graph_one()\ninto load_commit_graph_one_fd_st(), which allows for providing an\nexisting file descriptor and stat information to the loading\ncode. This isn't strictly needed, but it would be redundant and\nconfusing to open() and stat() the file twice for some of the\ncodepaths, this allows for calling open_commit_graph() followed by\nload_commit_graph_one_fd_st(). The \"graph_file\" still needs to be\npassed to that function for the the \"graph file %s is too small\" error\nmessage.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 21 +++++++++++++++++----\n commit-graph.c | 42 +++++++++++++++++++++++++++++------------\n commit-graph.h | 3 +++\n t/t5318-commit-graph.sh | 2 +-\n 4 files changed, 51 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 4ae502754c..32bcc63427 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n \tchar *graph_name;\n+\tint open_ok;\n+\tint fd;\n+\tstruct stat st;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n-\tgraph = load_commit_graph_one(graph_name);\n+\topen_ok = open_commit_graph(graph_name, &fd, &st);\n+\tif (!open_ok)\n+\t\treturn 0;\n+\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n \tFREE_AND_NULL(graph_name);\n \n \tif (!graph)\n-\t\treturn 0;\n+\t\treturn 1;\n \n \tUNLEAK(graph);\n \treturn verify_commit_graph(the_repository, graph);\n@@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n \tchar *graph_name;\n+\tint open_ok;\n+\tint fd;\n+\tstruct stat st;\n \n \tstatic struct option builtin_commit_graph_read_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n-\tgraph = load_commit_graph_one(graph_name);\n \n+\topen_ok = open_commit_graph(graph_name, &fd, &st);\n+\tif (!open_ok)\n+\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n+\n+\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n \tif (!graph)\n-\t\tdie(\"graph file %s does not exist\", graph_name);\n+\t\treturn 1;\n \n \tFREE_AND_NULL(graph_name);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 980fbf47ea..b1ba7a09cc 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one(const char *graph_file)\n+int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n+{\n+\t*fd = git_open(graph_file);\n+\tif (*fd < 0)\n+\t\treturn 0;\n+\tif (fstat(*fd, st)) {\n+\t\tclose(*fd);\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n+\t\t\t\t\t\t int fd, struct stat *st)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n-\tstruct stat st;\n \tstruct commit_graph *ret;\n-\tint fd = git_open(graph_file);\n \n-\tif (fd < 0)\n-\t\treturn NULL;\n-\tif (fstat(fd, &st)) {\n-\t\tclose(fd);\n-\t\treturn NULL;\n-\t}\n-\tgraph_size = xsize_t(st.st_size);\n+\tgraph_size = xsize_t(st->st_size);\n \n \tif (graph_size < GRAPH_MIN_SIZE) {\n \t\tclose(fd);\n-\t\tdie(_(\"graph file %s is too small\"), graph_file);\n+\t\terror(_(\"graph file %s is too small\"), graph_file);\n+\t\treturn 0;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tret = parse_commit_graph(graph_map, fd, graph_size);\n@@ -106,12 +112,24 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!ret) {\n \t\tmunmap(graph_map, graph_size);\n \t\tclose(fd);\n-\t\texit(1);\n \t}\n \n \treturn ret;\n }\n \n+struct commit_graph *load_commit_graph_one(const char *graph_file)\n+{\n+\n+\tstruct stat st;\n+\tint fd;\n+\tint open_ok = open_commit_graph(graph_file, &fd, &st);\n+\n+\tif (!open_ok)\n+\t\treturn NULL;\n+\n+\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n+}\n+\n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size)\n {\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 275f97d006..e4f17071e2 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -11,6 +11,7 @@\n struct commit;\n \n char *get_commit_graph_filename(const char *obj_dir);\n+int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n \n /*\n * Given a commit struct, try to fill the commit struct info, including:\n@@ -53,6 +54,8 @@ struct commit_graph {\n };\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n+\t\t\t\t\t\t int fd, struct stat *st);\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ad3a695f76..71d775e313 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -377,7 +377,7 @@ corrupt_graph_verify() {\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err &&\n-\ttest_might_fail git status --short\n+\tgit status --short\n }\n \n # usage: corrupt_graph_and_verify []\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371546","messageId":"20190314214740.23360-7-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 6/8] commit-graph verify: detect inability to read the graph","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:38Z","receivedAt":"2019-03-14T21:48:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change \"commit-graph verify\" to error on open() failures other than\nENOENT. As noted in the third paragraph of 283e68c72f (\"commit-graph:\nadd 'verify' subcommand\", 2018-06-27) and the test it added it's\nintentional that \"commit-graph verify\" doesn't error out when the file\ndoesn't exist.\n\nBut let's not be overly promiscuous in what we accept. If we can't\nread the file for other reasons, e.g. permission errors, bad file\ndescriptor etc. we'd like to report an error to the user.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 +++-\n t/t5318-commit-graph.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 8196fdbe9c..537fdfd0f0 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -62,8 +62,10 @@ static int graph_verify(int argc, const char **argv)\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n-\tif (!open_ok)\n+\tif (!open_ok && errno == ENOENT)\n \t\treturn 0;\n+\tif (!open_ok)\n+\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tFREE_AND_NULL(graph_name);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 71d775e313..1cb0355c7e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -400,6 +400,12 @@ corrupt_graph_and_verify() {\n \n }\n \n+test_expect_success POSIXPERM,SANITY 'detect permission problem' '\n+\tcorrupt_graph_setup &&\n+\tchmod 000 $objdir/info/commit-graph &&\n+\tcorrupt_graph_verify \"Could not open\"\n+'\n+\n test_expect_success 'detect too small' '\n \tcorrupt_graph_setup &&\n \techo \"a small graph\" >$objdir/info/commit-graph &&\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371547","messageId":"20190314214740.23360-8-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 7/8] commit-graph write: don't die if the existing graph is corrupt","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:39Z","receivedAt":"2019-03-14T21:48:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the commit-graph is written we end up calling\nparse_commit(). This will in turn invoke code that'll consult the\nexisting commit-graph about the commit, if the graph is corrupted we\ndie.\n\nWe thus get into a state where a failing \"commit-graph verify\" can't\nbe followed-up with a \"commit-graph write\" if core.commitGraph=true is\nset, the graph either needs to be manually removed to proceed, or\ncore.commitGraph needs to be set to \"false\".\n\nChange the \"commit-graph write\" codepath to use a new\nparse_commit_no_graph() helper instead of parse_commit() to avoid\nthis. The latter will call repo_parse_commit_internal() with\nuse_commit_graph=1 as seen in 177722b344 (\"commit: integrate commit\ngraph with commit parsing\", 2018-04-10).\n\nNot using the old graph at all slows down the writing of the new graph\nby some small amount, but is a sensible way to prevent an error in the\nexisting commit-graph from spreading.\n\nJust fixing the current issue would be likely to result in code that's\ninadvertently broken in the future. New code might use the\ncommit-graph at a distance. To detect such cases introduce a\n\"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\" setting used when we do our\ncorruption tests, and test that a \"write/verify\" combo works after\nevery one of our current test cases where we now detect commit-graph\ncorruption.\n\nSome of the code changes here might be strictly unnecessary, e.g. I\nwas unable to find cases where the parse_commit() called from\nwrite_graph_chunk_data() didn't exit early due to\n\"item->object.parsed\" being true in\nrepo_parse_commit_internal() (before the use_commit_graph=1 has any\neffect). But let's also convert those cases for good measure, we do\nnot have exhaustive tests for all possible types of commit-graph\ncorruption.\n\nThis might need to be re-visited if we learn to write the commit-graph\nincrementally, but probably not. Hopefully we'll just start by finding\nout what commits we have in total, then read the old graph(s) to see\nwhat they cover, and finally write a new graph file with everything\nthat's missing. In that case the new graph writing code just needs to\ncontinue to use e.g. a parse_commit() that doesn't consult the\nexisting commit-graphs.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 10 +++++++---\n commit-graph.h | 1 +\n commit.h | 6 ++++++\n t/t5318-commit-graph.sh | 11 +++++++++--\n 4 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex d945e8f3e0..6b3ade9496 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -281,6 +281,10 @@ static int prepare_commit_graph(struct repository *r)\n \tstruct object_directory *odb;\n \tint config_value;\n \n+\tif (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n+\t\tdie(\"Dying as requested by the '%s' variable on commit-graph load!\",\n+\t\t GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n+\n \tif (r->objects->commit_graph_attempted)\n \t\treturn !!r->objects->commit_graph;\n \tr->objects->commit_graph_attempted = 1;\n@@ -545,7 +549,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(progress, ++*progress_cnt);\n \n-\t\tparse_commit(*list);\n+\t\tparse_commit_no_graph(*list);\n \t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n \n \t\tparent = (*list)->parents;\n@@ -742,7 +746,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)\n \t\tdisplay_progress(progress, i + 1);\n \t\tcommit = lookup_commit(the_repository, &oids->list[i]);\n \n-\t\tif (commit && !parse_commit(commit))\n+\t\tif (commit && !parse_commit_no_graph(commit))\n \t\t\tadd_missing_parents(oids, commit);\n \t}\n \tstop_progress(&progress);\n@@ -991,7 +995,7 @@ void write_commit_graph(const char *obj_dir,\n \t\t\tcontinue;\n \n \t\tcommits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);\n-\t\tparse_commit(commits.list[commits.nr]);\n+\t\tparse_commit_no_graph(commits.list[commits.nr]);\n \n \t\tfor (parent = commits.list[commits.nr]->parents;\n \t\t parent; parent = parent->next)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 36d8109901..6021ababa2 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -7,6 +7,7 @@\n #include \"cache.h\"\n \n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n+#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n \n struct commit;\n \ndiff --git a/commit.h b/commit.h\nindex 42728c2906..5d33477e78 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)\n {\n \treturn repo_parse_commit_gently(r, item, 0);\n }\n+\n+static inline int parse_commit_no_graph(struct commit *commit)\n+{\n+\treturn repo_parse_commit_internal(the_repository, commit, 0, 0);\n+}\n+\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)\n #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 1cb0355c7e..d146cf4982 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -377,7 +377,13 @@ corrupt_graph_verify() {\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err &&\n-\tgit status --short\n+\tif test \"$2\" != \"no-copy\"\n+\tthen\n+\t\tcp $objdir/info/commit-graph commit-graph-pre-write-test\n+\tfi &&\n+\tgit status --short &&\n+\tGIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&\n+\tgit commit-graph verify\n }\n \n # usage: corrupt_graph_and_verify []\n@@ -403,7 +409,7 @@ corrupt_graph_and_verify() {\n test_expect_success POSIXPERM,SANITY 'detect permission problem' '\n \tcorrupt_graph_setup &&\n \tchmod 000 $objdir/info/commit-graph &&\n-\tcorrupt_graph_verify \"Could not open\"\n+\tcorrupt_graph_verify \"Could not open\" \"no-copy\"\n '\n \n test_expect_success 'detect too small' '\n@@ -522,6 +528,7 @@ test_expect_success 'git fsck (checks commit-graph)' '\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n \ttest_must_fail git fsck\n '\n \n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371548","messageId":"20190314214740.23360-9-avarab@gmail.com","threadId":"50554","inReplyTo":"20190221223753.20070-1-avarab@gmail.com","subject":"[PATCH v2 8/8] commit-graph: improve & i18n error messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-14T21:47:40Z","receivedAt":"2019-03-14T21:48:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the error emitted when a commit-graph file is corrupt so that\nwe actually mention the commit-graph, e.g. change errors like:\n\n error: improper chunk offset 0000000000385e0c\n\nTo:\n\n error: commit-graph improper chunk offset 0000000000385e0c\n\nAs discussed in the commits leading up to this one the commit-graph\nmachinery is now used by common commands like \"status\". If the graph\nwas corrupt we'd often emit some error that gave no indication what\nwas wrong. Now some of them are still cryptic, but they'll at least\nmention \"commit-graph\" to give the user a hint as to where to look.\n\nWhile I'm at it mark some of the strings that hadn't been marked for\ntranslation. It's clear from the commit history and the code that this\nwas merely forgotten at the time, and wasn't intentional.p5\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 38 +++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6b3ade9496..4cf7d6c294 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -150,21 +150,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n-\t\terror(_(\"graph signature %X does not match signature %X\"),\n+\t\terror(_(\"commit-graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n \t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n-\t\terror(_(\"graph version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n \t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != oid_version()) {\n-\t\terror(_(\"hash version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph hash version %X does not match version %X\"),\n \t\t hash_version, oid_version());\n \t\treturn NULL;\n \t}\n@@ -187,7 +187,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \t\tif (data + graph_size - chunk_lookup <\n \t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n-\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\terror(_(\"commit-graph chunk lookup table entry missing; file may be incomplete\"));\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\n@@ -198,7 +198,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - the_hash_algo->rawsz) {\n-\t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n+\t\t\terror(_(\"commit-graph improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n@@ -235,7 +235,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t}\n \n \t\tif (chunk_repeated) {\n-\t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n+\t\t\terror(_(\"commit-graph chunk id %08x appears multiple times\"), chunk_id);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\n@@ -1162,7 +1162,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n \n \t\tif (i && oidcmp(&prev_oid, &cur_oid) >= 0)\n-\t\t\tgraph_report(\"commit-graph has incorrect OID order: %s then %s\",\n+\t\t\tgraph_report(_(\"commit-graph has incorrect OID order: %s then %s\"),\n \t\t\t\t oid_to_hex(&prev_oid),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n@@ -1172,14 +1172,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t\tuint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);\n \n \t\t\tif (i != fanout_value)\n-\t\t\t\tgraph_report(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\",\n+\t\t\t\tgraph_report(_(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\"),\n \t\t\t\t\t cur_fanout_pos, fanout_value, i);\n \t\t\tcur_fanout_pos++;\n \t\t}\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \t\tif (!parse_commit_in_graph_one(r, g, graph_commit))\n-\t\t\tgraph_report(\"failed to parse %s from commit-graph\",\n+\t\t\tgraph_report(_(\"failed to parse commit %s from commit-graph\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \t}\n \n@@ -1187,7 +1187,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\tuint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);\n \n \t\tif (g->num_commits != fanout_value)\n-\t\t\tgraph_report(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\",\n+\t\t\tgraph_report(_(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\"),\n \t\t\t\t cur_fanout_pos, fanout_value, i);\n \n \t\tcur_fanout_pos++;\n@@ -1209,14 +1209,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \t\todb_commit = (struct commit *)create_object(r, cur_oid.hash, alloc_commit_node(r));\n \t\tif (parse_commit_internal(odb_commit, 0, 0)) {\n-\t\t\tgraph_report(\"failed to parse %s from object database\",\n+\t\t\tgraph_report(_(\"failed to parse commit %s from object database for commit-graph\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!oideq(&get_commit_tree_in_graph_one(r, g, graph_commit)->object.oid,\n \t\t\t get_commit_tree_oid(odb_commit)))\n-\t\t\tgraph_report(\"root tree OID for commit %s in commit-graph is %s != %s\",\n+\t\t\tgraph_report(_(\"root tree OID for commit %s in commit-graph is %s != %s\"),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t oid_to_hex(get_commit_tree_oid(graph_commit)),\n \t\t\t\t oid_to_hex(get_commit_tree_oid(odb_commit)));\n@@ -1226,13 +1226,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \n \t\twhile (graph_parents) {\n \t\t\tif (odb_parents == NULL) {\n-\t\t\t\tgraph_report(\"commit-graph parent list for commit %s is too long\",\n+\t\t\t\tgraph_report(_(\"commit-graph parent list for commit %s is too long\"),\n \t\t\t\t\t oid_to_hex(&cur_oid));\n \t\t\t\tbreak;\n \t\t\t}\n \n \t\t\tif (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid))\n-\t\t\t\tgraph_report(\"commit-graph parent for %s is %s != %s\",\n+\t\t\t\tgraph_report(_(\"commit-graph parent for %s is %s != %s\"),\n \t\t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t\t oid_to_hex(&graph_parents->item->object.oid),\n \t\t\t\t\t oid_to_hex(&odb_parents->item->object.oid));\n@@ -1245,16 +1245,16 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t}\n \n \t\tif (odb_parents != NULL)\n-\t\t\tgraph_report(\"commit-graph parent list for commit %s terminates early\",\n+\t\t\tgraph_report(_(\"commit-graph parent list for commit %s terminates early\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n \t\tif (!graph_commit->generation) {\n \t\t\tif (generation_zero == GENERATION_NUMBER_EXISTS)\n-\t\t\t\tgraph_report(\"commit-graph has generation number zero for commit %s, but non-zero elsewhere\",\n+\t\t\t\tgraph_report(_(\"commit-graph has generation number zero for commit %s, but non-zero elsewhere\"),\n \t\t\t\t\t oid_to_hex(&cur_oid));\n \t\t\tgeneration_zero = GENERATION_ZERO_EXISTS;\n \t\t} else if (generation_zero == GENERATION_ZERO_EXISTS)\n-\t\t\tgraph_report(\"commit-graph has non-zero generation number for commit %s, but zero elsewhere\",\n+\t\t\tgraph_report(_(\"commit-graph has non-zero generation number for commit %s, but zero elsewhere\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n \t\tif (generation_zero == GENERATION_ZERO_EXISTS)\n@@ -1269,13 +1269,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t\tmax_generation--;\n \n \t\tif (graph_commit->generation != max_generation + 1)\n-\t\t\tgraph_report(\"commit-graph generation for commit %s is %u != %u\",\n+\t\t\tgraph_report(_(\"commit-graph generation for commit %s is %u != %u\"),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t graph_commit->generation,\n \t\t\t\t max_generation + 1);\n \n \t\tif (graph_commit->date != odb_commit->date)\n-\t\t\tgraph_report(\"commit date for commit %s in commit-graph is %\"PRItime\" != %\"PRItime,\n+\t\t\tgraph_report(_(\"commit date for commit %s in commit-graph is %\"PRItime\" != %\"PRItime),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t graph_commit->date,\n \t\t\t\t odb_commit->date);\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"371568","messageId":"CAPig+cTZAhRekr0Yvmtb-gTEATEX19RO0=Zh-ycCPkyEc0OxxA@mail.gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"Re: [PATCH v2 0/8] commit-graph: segfault & other fixes for broken graphs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-15T07:47:42Z","receivedAt":"2019-03-15T07:47:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 14, 2019 at 5:47 PM Ævar Arnfjörð Bjarmason\n wrote:\n> I fixed a test to avoid the pattern a0a630192d\n> (t/check-non-portable-shell: detect \"FOO=bar shell_func\", 2018-07-13)\n> tests for. The new pattern is more obvious.\n>\n> As an aside I don't get how that doesn't work as intended from the\n> commit message of that commit & its series.\n>\n> $ cat /tmp/x.sh\n> sayit() { echo \"saying '$SAY'\"; };\n> SAY=hi sayit; sayit;\n> $ sh /tmp/x.sh\n> saying 'hi'\n> saying ''\n>\n> I get the same thing on bash, dash, NetBSD ksh, Solaris's shell &\n> AIX's. I.e. it's explicitly not assigning $SAY for the duration of the\n> shell [...]\n\nThe shells you tested may all behave \"sanely\" given that input, but\nnot all shells do. For instance, on MacOS, the factory-supplied bash\n3.2.57 behaves in the \"broken\" way in 'sh' compatibility mode:\n\n$ cat /tmp/x.sh\nsayit() { echo \"saying '$SAY'\"; };\nSAY=hi sayit; sayit;\n\n$ /bin/sh /tmp/x.sh\nsaying 'hi'\nsaying 'hi'\n\n$ /bin/bash /tmp/x.sh\nsaying 'hi'\nsaying ''\n"},{"id":"371577","messageId":"875zskbevm.fsf@evledraar.gmail.com","threadId":"50554","inReplyTo":"CAPig+cTZAhRekr0Yvmtb-gTEATEX19RO0=Zh-ycCPkyEc0OxxA@mail.gmail.com","subject":"Re: [PATCH v2 0/8] commit-graph: segfault & other fixes for broken graphs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T10:39:41Z","receivedAt":"2019-03-15T10:39:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 15 2019, Eric Sunshine wrote:\n\n> On Thu, Mar 14, 2019 at 5:47 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>> I fixed a test to avoid the pattern a0a630192d\n>> (t/check-non-portable-shell: detect \"FOO=bar shell_func\", 2018-07-13)\n>> tests for. The new pattern is more obvious.\n>>\n>> As an aside I don't get how that doesn't work as intended from the\n>> commit message of that commit & its series.\n>>\n>> $ cat /tmp/x.sh\n>> sayit() { echo \"saying '$SAY'\"; };\n>> SAY=hi sayit; sayit;\n>> $ sh /tmp/x.sh\n>> saying 'hi'\n>> saying ''\n>>\n>> I get the same thing on bash, dash, NetBSD ksh, Solaris's shell &\n>> AIX's. I.e. it's explicitly not assigning $SAY for the duration of the\n>> shell [...]\n>\n> The shells you tested may all behave \"sanely\" given that input, but\n> not all shells do. For instance, on MacOS, the factory-supplied bash\n> 3.2.57 behaves in the \"broken\" way in 'sh' compatibility mode:\n>\n> $ cat /tmp/x.sh\n> sayit() { echo \"saying '$SAY'\"; };\n> SAY=hi sayit; sayit;\n>\n> $ /bin/sh /tmp/x.sh\n> saying 'hi'\n> saying 'hi'\n>\n> $ /bin/bash /tmp/x.sh\n> saying 'hi'\n> saying ''\n\nThanks. Also I found (with help from Freenode ##POSIX) that this part of\nthe spec[1] talks about it:\n\nBEGIN QUOTE\n Variable assignments shall be performed as follows:\n [...]\n\n * If the command name is a function that is not a standard utility\n implemented as a function, variable assignments shall affect the\n current execution environment during the execution of the\n function. It is unspecified:\n\n - Whether or not the variable assignments persist after the\n completion of the function\n\n - Whether or not the variables gain the export attribute during\n the execution of the function\n\n - Whether or not export attributes gained as a result of the\n variable assignments persist after the completion of the\n function (if variable assignments persist after the completion\n of the function)\nEND QUOTE\n\nI.e. this is undefined behavior that just happened to work on the\nvarious shells I tested. Makes sense, was just wondering if I'd entirely\nmissed out on what behavior was, turns out it's just fairly rare.\n\nFWIW My bash 5.0.2 on Debian behaves as I noted in both sh and bash\nmode.\n\n1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html\n"},{"id":"372416","messageId":"20190325120834.15529-1-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 0/8] commit-graph: segfault & other fixes for broken graphs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:26Z","receivedAt":"2019-03-25T12:08:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This v3 fixes issues raised by Ramsay in\nhttps://public-inbox.org/git/1908832c-8dd0-377e-917b-acb33b00273c@ramsayjones.plus.com/\n\nWhile I was at it I changed a die(\"Whatevs\") to\ndie(\"whatevs\"). I.e. start with lower-case as discussed in other\nplaces on-list recently.\n\nThe range-diff looks scarier than this v2..v3 really is. There's no\ncode changes aside from the s/0/NULL/ in one place, but marking a\ncouple of functions as \"static\" required moving them around, and\nremoving their entry from the corresponding *.h file.\n\nÆvar Arnfjörð Bjarmason (8):\n commit-graph tests: split up corrupt_graph_and_verify()\n commit-graph tests: test a graph that's too small\n commit-graph: fix segfault on e.g. \"git status\"\n commit-graph: don't early exit(1) on e.g. \"git status\"\n commit-graph: don't pass filename to load_commit_graph_one_fd_st()\n commit-graph verify: detect inability to read the graph\n commit-graph write: don't die if the existing graph is corrupt\n commit-graph: improve & i18n error messages\n\n builtin/commit-graph.c | 23 +++++--\n commit-graph.c | 132 +++++++++++++++++++++++++++-------------\n commit-graph.h | 4 +-\n commit.h | 6 ++\n t/t5318-commit-graph.sh | 42 +++++++++++--\n 5 files changed, 153 insertions(+), 54 deletions(-)\n\nRange-diff:\n1: 2f8ba0adf8 = 1: 83ff92a39d commit-graph tests: split up corrupt_graph_and_verify()\n2: 800b17edde = 2: b9170c35e6 commit-graph tests: test a graph that's too small\n3: 7083ab81c7 ! 3: daf38a9af7 commit-graph: fix segfault on e.g. \"git status\"\n @@ -58,20 +58,10 @@\n --- a/commit-graph.c\n +++ b/commit-graph.c\n @@\n - \t\tlast_chunk_offset = chunk_offset;\n - \t}\n - \n -+\tif (verify_commit_graph_lite(graph))\n -+\t\treturn NULL;\n -+\n - \treturn graph;\n + \treturn ret;\n }\n \n -@@\n - #define GENERATION_ZERO_EXISTS 1\n - #define GENERATION_NUMBER_EXISTS 2\n - \n -+int verify_commit_graph_lite(struct commit_graph *g)\n ++static int verify_commit_graph_lite(struct commit_graph *g)\n +{\n +\t/*\n +\t * Basic validation shared between parse_commit_graph()\n @@ -101,9 +91,19 @@\n +\treturn 0;\n +}\n +\n - int verify_commit_graph(struct repository *r, struct commit_graph *g)\n + struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n + \t\t\t\t\tsize_t graph_size)\n {\n - \tuint32_t i, cur_fanout_pos = 0;\n +@@\n + \t\tlast_chunk_offset = chunk_offset;\n + \t}\n + \n ++\tif (verify_commit_graph_lite(graph))\n ++\t\treturn NULL;\n ++\n + \treturn graph;\n + }\n + \n @@\n \t\treturn 1;\n \t}\n @@ -122,18 +122,6 @@\n \t\treturn verify_commit_graph_error;\n \n \n - diff --git a/commit-graph.h b/commit-graph.h\n - --- a/commit-graph.h\n - +++ b/commit-graph.h\n -@@\n - \t\t\tstruct string_list *commit_hex,\n - \t\t\tint append, int report_progress);\n - \n -+int verify_commit_graph_lite(struct commit_graph *g);\n - int verify_commit_graph(struct repository *r, struct commit_graph *g);\n - \n - void close_commit_graph(struct repository *);\n -\n diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n --- a/t/t5318-commit-graph.sh\n +++ b/t/t5318-commit-graph.sh\n4: d00564ae89 ! 4: 3d7d8c4deb commit-graph: don't early exit(1) on e.g. \"git status\"\n @@ -29,7 +29,15 @@\n passed to that function for the the \"graph file %s is too small\" error\n message.\n \n + This leaves load_commit_graph_one() unused by everything except the\n + internal prepare_commit_graph_one() function, so let's mark it as\n + \"static\". If someone needs it in the future we can remove the \"static\"\n + attribute. I could also rewrite its sole remaining\n + user (\"prepare_commit_graph_one()\") to use\n + load_commit_graph_one_fd_st() instead, but let's leave it at this.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n + Signed-off-by: Ramsay Jones \n \n diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n --- a/builtin/commit-graph.c\n @@ -131,7 +139,7 @@\n \t\tclose(fd);\n -\t\tdie(_(\"graph file %s is too small\"), graph_file);\n +\t\terror(_(\"graph file %s is too small\"), graph_file);\n -+\t\treturn 0;\n ++\t\treturn NULL;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tret = parse_commit_graph(graph_map, fd, graph_size);\n @@ -143,9 +151,11 @@\n \t}\n \n \treturn ret;\n +@@\n + \treturn graph;\n }\n \n -+struct commit_graph *load_commit_graph_one(const char *graph_file)\n ++static struct commit_graph *load_commit_graph_one(const char *graph_file)\n +{\n +\n +\tstruct stat st;\n @@ -158,9 +168,9 @@\n +\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n +}\n +\n - struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n - \t\t\t\t\tsize_t graph_size)\n + static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n {\n + \tchar *graph_name;\n \n diff --git a/commit-graph.h b/commit-graph.h\n --- a/commit-graph.h\n @@ -174,9 +184,10 @@\n /*\n * Given a commit struct, try to fill the commit struct info, including:\n @@\n + \tconst unsigned char *chunk_extra_edges;\n };\n \n - struct commit_graph *load_commit_graph_one(const char *graph_file);\n +-struct commit_graph *load_commit_graph_one(const char *graph_file);\n +struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n +\t\t\t\t\t\t int fd, struct stat *st);\n \n5: 25ee185bf7 ! 5: 15687fb21f commit-graph: don't pass filename to load_commit_graph_one_fd_st()\n @@ -59,7 +59,7 @@\n \t\tclose(fd);\n -\t\terror(_(\"graph file %s is too small\"), graph_file);\n +\t\terror(_(\"commit-graph file is too small\"));\n - \t\treturn 0;\n + \t\treturn NULL;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n @@\n @@ -70,15 +70,15 @@\n +\treturn load_commit_graph_one_fd_st(fd, &st);\n }\n \n - struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n + static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n \n diff --git a/commit-graph.h b/commit-graph.h\n --- a/commit-graph.h\n +++ b/commit-graph.h\n @@\n + \tconst unsigned char *chunk_extra_edges;\n };\n \n - struct commit_graph *load_commit_graph_one(const char *graph_file);\n -struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n -\t\t\t\t\t\t int fd, struct stat *st);\n +struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n6: 7619b46987 = 6: 30145c2dca commit-graph verify: detect inability to read the graph\n7: 17ee4fc050 ! 7: 51e3aa651b commit-graph write: don't die if the existing graph is corrupt\n @@ -57,7 +57,7 @@\n \tint config_value;\n \n +\tif (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n -+\t\tdie(\"Dying as requested by the '%s' variable on commit-graph load!\",\n ++\t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n +\t\t GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n +\n \tif (r->objects->commit_graph_attempted)\n8: 29ab2895b7 = 8: e7c801f73b commit-graph: improve & i18n error messages\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372417","messageId":"20190325120834.15529-2-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 1/8] commit-graph tests: split up corrupt_graph_and_verify()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:27Z","receivedAt":"2019-03-25T12:08:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Split up the corrupt_graph_and_verify() function added in\nd9b9f8a6fd (\"commit-graph: verify catches corrupt signature\",\n2018-06-27) into its logical components of setting up the test itself,\ndoing the corruption in a particular way with \"dd\", and then finally\ntesting that stderr is what we expect.\n\nThis allows for re-using everything except the now slimmer\ncorrupt_graph_and_verify() to corrupt the graph in a way that doesn't\ninvolve inserting a given byte sequence at a given position,\ne.g. truncating it entirely to a custom value.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5318-commit-graph.sh | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 561796f280..56a616831e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -366,6 +366,19 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n+corrupt_graph_setup() {\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n+\tcp $objdir/info/commit-graph commit-graph-backup\n+}\n+\n+corrupt_graph_verify() {\n+\tgrepstr=$1\n+\ttest_must_fail git commit-graph verify 2>test_err &&\n+\tgrep -v \"^+\" test_err >err &&\n+\ttest_i18ngrep \"$grepstr\" err\n+}\n+\n # usage: corrupt_graph_and_verify []\n # Manipulates the commit-graph file at the position\n # by inserting the data, optionally zeroing the file\n@@ -376,17 +389,14 @@ corrupt_graph_and_verify() {\n \tpos=$1\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n-\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tcorrupt_graph_setup &&\n \torig_size=$(wc -c < $objdir/info/commit-graph) &&\n \tzero_pos=${4:-${orig_size}} &&\n-\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n-\tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n \tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" if=/dev/null &&\n \tgenerate_zero_bytes $(($orig_size - $zero_pos)) >>\"$objdir/info/commit-graph\" &&\n-\ttest_must_fail git commit-graph verify 2>test_err &&\n-\tgrep -v \"^+\" test_err >err &&\n-\ttest_i18ngrep \"$grepstr\" err\n+\tcorrupt_graph_verify \"$grepstr\"\n+\n }\n \n test_expect_success 'detect bad signature' '\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372418","messageId":"20190325120834.15529-3-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 2/8] commit-graph tests: test a graph that's too small","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:28Z","receivedAt":"2019-03-25T12:08:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the recently split-up components of the corrupt_graph_and_verify()\nfunction to assert that we error on graphs that are too small. The\nerror was added in 2a2e32bdc5 (\"commit-graph: implement git\ncommit-graph read\", 2018-04-10), but there was no test for it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5318-commit-graph.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 56a616831e..ce3459a6f5 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -399,6 +399,12 @@ corrupt_graph_and_verify() {\n \n }\n \n+test_expect_success 'detect too small' '\n+\tcorrupt_graph_setup &&\n+\techo \"a small graph\" >$objdir/info/commit-graph &&\n+\tcorrupt_graph_verify \"too small\"\n+'\n+\n test_expect_success 'detect bad signature' '\n \tcorrupt_graph_and_verify 0 \"\\0\" \\\n \t\t\"graph signature\"\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372419","messageId":"20190325120834.15529-4-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 3/8] commit-graph: fix segfault on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:29Z","receivedAt":"2019-03-25T12:08:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When core.commitGraph=true is set, various common commands now consult\nthe commit graph. Because the commit-graph code is very trusting of\nits input data, it's possibly to construct a graph that'll cause an\nimmediate segfault on e.g. \"status\" (and e.g. \"log\", \"blame\", ...). In\nsome other cases where git immediately exits with a cryptic error\nabout the graph being broken.\n\nThe root cause of this is that while the \"commit-graph verify\"\nsub-command exhaustively verifies the graph, other users of the graph\nsimply trust the graph, and will e.g. deference data found at certain\noffsets as pointers, causing segfaults.\n\nThis change does the bare minimum to ensure that we don't segfault in\nthe common fill_commit_in_graph() codepath called by\ne.g. setup_revisions(), to do this instrument the \"commit-graph\nverify\" tests to always check if \"status\" would subsequently\nsegfault. This fixes the following tests which would previously\nsegfault:\n\n not ok 50 - detect low chunk count\n not ok 51 - detect missing OID fanout chunk\n not ok 52 - detect missing OID lookup chunk\n not ok 53 - detect missing commit data chunk\n\nThose happened because with the commit-graph enabled setup_revisions()\nwould eventually call fill_commit_in_graph(), where e.g.\ng->chunk_commit_data is used early as an offset (and will be\n0x0). With this change we get far enough to detect that the graph is\nbroken, and show an error instead. E.g.:\n\n $ git status; echo $?\n error: commit-graph is missing the Commit Data chunk\n 1\n\nThat also sucks, we should *warn* and not hard-fail \"status\" just\nbecause the commit-graph is corrupt, but fixing is left to a follow-up\nchange.\n\nA side-effect of changing the reporting from graph_report() to error()\nis that we now have an \"error: \" prefix for these even for\n\"commit-graph verify\". Pseudo-diff before/after:\n\n $ git commit-graph verify\n -commit-graph is missing the Commit Data chunk\n +error: commit-graph is missing the Commit Data chunk\n\nChanging that is OK. Various errors it emits now early on are prefixed\nwith \"error: \", moving these over and changing the output doesn't\nbreak anything.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 43 ++++++++++++++++++++++++++++++++---------\n t/t5318-commit-graph.sh | 3 ++-\n 2 files changed, 36 insertions(+), 10 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 47e9be0a3a..f8201d888b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -112,6 +112,36 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \treturn ret;\n }\n \n+static int verify_commit_graph_lite(struct commit_graph *g)\n+{\n+\t/*\n+\t * Basic validation shared between parse_commit_graph()\n+\t * which'll be called every time the graph is used, and the\n+\t * much more expensive verify_commit_graph() used by\n+\t * \"commit-graph verify\".\n+\t *\n+\t * There should only be very basic checks here to ensure that\n+\t * we don't e.g. segfault in fill_commit_in_graph(), but\n+\t * because this is a very hot codepath nothing that e.g. loops\n+\t * over g->num_commits, or runs a checksum on the commit-graph\n+\t * itself.\n+\t */\n+\tif (!g->chunk_oid_fanout) {\n+\t\terror(\"commit-graph is missing the OID Fanout chunk\");\n+\t\treturn 1;\n+\t}\n+\tif (!g->chunk_oid_lookup) {\n+\t\terror(\"commit-graph is missing the OID Lookup chunk\");\n+\t\treturn 1;\n+\t}\n+\tif (!g->chunk_commit_data) {\n+\t\terror(\"commit-graph is missing the Commit Data chunk\");\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size)\n {\n@@ -233,6 +263,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tlast_chunk_offset = chunk_offset;\n \t}\n \n+\tif (verify_commit_graph_lite(graph))\n+\t\treturn NULL;\n+\n \treturn graph;\n }\n \n@@ -1089,15 +1122,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\treturn 1;\n \t}\n \n-\tverify_commit_graph_error = 0;\n-\n-\tif (!g->chunk_oid_fanout)\n-\t\tgraph_report(\"commit-graph is missing the OID Fanout chunk\");\n-\tif (!g->chunk_oid_lookup)\n-\t\tgraph_report(\"commit-graph is missing the OID Lookup chunk\");\n-\tif (!g->chunk_commit_data)\n-\t\tgraph_report(\"commit-graph is missing the Commit Data chunk\");\n-\n+\tverify_commit_graph_error = verify_commit_graph_lite(g);\n \tif (verify_commit_graph_error)\n \t\treturn verify_commit_graph_error;\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ce3459a6f5..ad3a695f76 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -376,7 +376,8 @@ corrupt_graph_verify() {\n \tgrepstr=$1\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n-\ttest_i18ngrep \"$grepstr\" err\n+\ttest_i18ngrep \"$grepstr\" err &&\n+\ttest_might_fail git status --short\n }\n \n # usage: corrupt_graph_and_verify []\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372420","messageId":"20190325120834.15529-5-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:30Z","receivedAt":"2019-03-25T12:08:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the commit-graph loading code work as a library that returns an\nerror code instead of calling exit(1) when the commit-graph is\ncorrupt. This means that e.g. \"status\" will now report commit-graph\ncorruption as an \"error: [...]\" at the top of its output, but then\nproceed to work normally.\n\nThis required splitting up the load_commit_graph_one() function so\nthat the code that deals with open()-ing and stat()-ing the graph can\nnow be called independently as open_commit_graph().\n\nThis is needed because \"commit-graph verify\" where the graph doesn't\nexist isn't an error. See the third paragraph in\n283e68c72f (\"commit-graph: add 'verify' subcommand\",\n2018-06-27). There's a bug in that logic where we conflate the\nintended ENOENT with other errno values (e.g. EACCES), but this change\ndoesn't address that. That'll be addressed in a follow-up change.\n\nI'm then splitting most of the logic out of load_commit_graph_one()\ninto load_commit_graph_one_fd_st(), which allows for providing an\nexisting file descriptor and stat information to the loading\ncode. This isn't strictly needed, but it would be redundant and\nconfusing to open() and stat() the file twice for some of the\ncodepaths, this allows for calling open_commit_graph() followed by\nload_commit_graph_one_fd_st(). The \"graph_file\" still needs to be\npassed to that function for the the \"graph file %s is too small\" error\nmessage.\n\nThis leaves load_commit_graph_one() unused by everything except the\ninternal prepare_commit_graph_one() function, so let's mark it as\n\"static\". If someone needs it in the future we can remove the \"static\"\nattribute. I could also rewrite its sole remaining\nuser (\"prepare_commit_graph_one()\") to use\nload_commit_graph_one_fd_st() instead, but let's leave it at this.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Ramsay Jones \n---\n builtin/commit-graph.c | 21 +++++++++++++++++----\n commit-graph.c | 42 +++++++++++++++++++++++++++++------------\n commit-graph.h | 4 +++-\n t/t5318-commit-graph.sh | 2 +-\n 4 files changed, 51 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 4ae502754c..32bcc63427 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n \tchar *graph_name;\n+\tint open_ok;\n+\tint fd;\n+\tstruct stat st;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n-\tgraph = load_commit_graph_one(graph_name);\n+\topen_ok = open_commit_graph(graph_name, &fd, &st);\n+\tif (!open_ok)\n+\t\treturn 0;\n+\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n \tFREE_AND_NULL(graph_name);\n \n \tif (!graph)\n-\t\treturn 0;\n+\t\treturn 1;\n \n \tUNLEAK(graph);\n \treturn verify_commit_graph(the_repository, graph);\n@@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)\n {\n \tstruct commit_graph *graph = NULL;\n \tchar *graph_name;\n+\tint open_ok;\n+\tint fd;\n+\tstruct stat st;\n \n \tstatic struct option builtin_commit_graph_read_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)\n \t\topts.obj_dir = get_object_directory();\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n-\tgraph = load_commit_graph_one(graph_name);\n \n+\topen_ok = open_commit_graph(graph_name, &fd, &st);\n+\tif (!open_ok)\n+\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n+\n+\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n \tif (!graph)\n-\t\tdie(\"graph file %s does not exist\", graph_name);\n+\t\treturn 1;\n \n \tFREE_AND_NULL(graph_name);\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex f8201d888b..3acc032c1b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one(const char *graph_file)\n+int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n+{\n+\t*fd = git_open(graph_file);\n+\tif (*fd < 0)\n+\t\treturn 0;\n+\tif (fstat(*fd, st)) {\n+\t\tclose(*fd);\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n+\t\t\t\t\t\t int fd, struct stat *st)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n-\tstruct stat st;\n \tstruct commit_graph *ret;\n-\tint fd = git_open(graph_file);\n \n-\tif (fd < 0)\n-\t\treturn NULL;\n-\tif (fstat(fd, &st)) {\n-\t\tclose(fd);\n-\t\treturn NULL;\n-\t}\n-\tgraph_size = xsize_t(st.st_size);\n+\tgraph_size = xsize_t(st->st_size);\n \n \tif (graph_size < GRAPH_MIN_SIZE) {\n \t\tclose(fd);\n-\t\tdie(_(\"graph file %s is too small\"), graph_file);\n+\t\terror(_(\"graph file %s is too small\"), graph_file);\n+\t\treturn NULL;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tret = parse_commit_graph(graph_map, fd, graph_size);\n@@ -106,7 +112,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!ret) {\n \t\tmunmap(graph_map, graph_size);\n \t\tclose(fd);\n-\t\texit(1);\n \t}\n \n \treturn ret;\n@@ -269,6 +274,19 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \treturn graph;\n }\n \n+static struct commit_graph *load_commit_graph_one(const char *graph_file)\n+{\n+\n+\tstruct stat st;\n+\tint fd;\n+\tint open_ok = open_commit_graph(graph_file, &fd, &st);\n+\n+\tif (!open_ok)\n+\t\treturn NULL;\n+\n+\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n+}\n+\n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n {\n \tchar *graph_name;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 096d8bac34..77cc739bc0 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -11,6 +11,7 @@\n struct commit;\n \n char *get_commit_graph_filename(const char *obj_dir);\n+int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n \n /*\n * Given a commit struct, try to fill the commit struct info, including:\n@@ -52,7 +53,8 @@ struct commit_graph {\n \tconst unsigned char *chunk_extra_edges;\n };\n \n-struct commit_graph *load_commit_graph_one(const char *graph_file);\n+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n+\t\t\t\t\t\t int fd, struct stat *st);\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ad3a695f76..71d775e313 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -377,7 +377,7 @@ corrupt_graph_verify() {\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err &&\n-\ttest_might_fail git status --short\n+\tgit status --short\n }\n \n # usage: corrupt_graph_and_verify []\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372421","messageId":"20190325120834.15529-6-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 5/8] commit-graph: don't pass filename to load_commit_graph_one_fd_st()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:31Z","receivedAt":"2019-03-25T12:08:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"An earlier change implemented load_commit_graph_one_fd_st() in a way\nthat was bug-compatible with earlier code in terms of the \"graph file\n%s is too small\" error message printing out the path to the\ncommit-graph (\".git/objects/info/commit-graph\").\n\nBut change that, because:\n\n * A function that takes an already-open file descriptor also needing\n the filename isn't very intuitive.\n\n * The vast majority of errors we might emit when loading the graph\n come from parse_commit_graph(), which doesn't report the\n filename. Let's not do that either in this case for consistency.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ++--\n commit-graph.c | 7 +++----\n commit-graph.h | 3 +--\n 3 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 32bcc63427..8196fdbe9c 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -64,7 +64,7 @@ static int graph_verify(int argc, const char **argv)\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n \tif (!open_ok)\n \t\treturn 0;\n-\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tFREE_AND_NULL(graph_name);\n \n \tif (!graph)\n@@ -102,7 +102,7 @@ static int graph_read(int argc, const char **argv)\n \tif (!open_ok)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \n-\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n+\tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tif (!graph)\n \t\treturn 1;\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 3acc032c1b..a26d266663 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -92,8 +92,7 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n \treturn 1;\n }\n \n-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n-\t\t\t\t\t\t int fd, struct stat *st)\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)\n {\n \tvoid *graph_map;\n \tsize_t graph_size;\n@@ -103,7 +102,7 @@ struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n \n \tif (graph_size < GRAPH_MIN_SIZE) {\n \t\tclose(fd);\n-\t\terror(_(\"graph file %s is too small\"), graph_file);\n+\t\terror(_(\"commit-graph file is too small\"));\n \t\treturn NULL;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n@@ -284,7 +283,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)\n \tif (!open_ok)\n \t\treturn NULL;\n \n-\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n+\treturn load_commit_graph_one_fd_st(fd, &st);\n }\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 77cc739bc0..ada7aea9ed 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -53,8 +53,7 @@ struct commit_graph {\n \tconst unsigned char *chunk_extra_edges;\n };\n \n-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n-\t\t\t\t\t\t int fd, struct stat *st);\n+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);\n \n struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\t\t\tsize_t graph_size);\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372422","messageId":"20190325120834.15529-9-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 8/8] commit-graph: improve & i18n error messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:34Z","receivedAt":"2019-03-25T12:08:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the error emitted when a commit-graph file is corrupt so that\nwe actually mention the commit-graph, e.g. change errors like:\n\n error: improper chunk offset 0000000000385e0c\n\nTo:\n\n error: commit-graph improper chunk offset 0000000000385e0c\n\nAs discussed in the commits leading up to this one the commit-graph\nmachinery is now used by common commands like \"status\". If the graph\nwas corrupt we'd often emit some error that gave no indication what\nwas wrong. Now some of them are still cryptic, but they'll at least\nmention \"commit-graph\" to give the user a hint as to where to look.\n\nWhile I'm at it mark some of the strings that hadn't been marked for\ntranslation. It's clear from the commit history and the code that this\nwas merely forgotten at the time, and wasn't intentional.p5\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 38 +++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 34ecaaf857..66865acbd7 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -167,21 +167,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n-\t\terror(_(\"graph signature %X does not match signature %X\"),\n+\t\terror(_(\"commit-graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n \t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n-\t\terror(_(\"graph version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n \t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != oid_version()) {\n-\t\terror(_(\"hash version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph hash version %X does not match version %X\"),\n \t\t hash_version, oid_version());\n \t\treturn NULL;\n \t}\n@@ -204,7 +204,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \t\tif (data + graph_size - chunk_lookup <\n \t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n-\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\terror(_(\"commit-graph chunk lookup table entry missing; file may be incomplete\"));\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\n@@ -215,7 +215,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - the_hash_algo->rawsz) {\n-\t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n+\t\t\terror(_(\"commit-graph improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n@@ -252,7 +252,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t}\n \n \t\tif (chunk_repeated) {\n-\t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n+\t\t\terror(_(\"commit-graph chunk id %08x appears multiple times\"), chunk_id);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\n@@ -1162,7 +1162,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n \n \t\tif (i && oidcmp(&prev_oid, &cur_oid) >= 0)\n-\t\t\tgraph_report(\"commit-graph has incorrect OID order: %s then %s\",\n+\t\t\tgraph_report(_(\"commit-graph has incorrect OID order: %s then %s\"),\n \t\t\t\t oid_to_hex(&prev_oid),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n@@ -1172,14 +1172,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t\tuint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);\n \n \t\t\tif (i != fanout_value)\n-\t\t\t\tgraph_report(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\",\n+\t\t\t\tgraph_report(_(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\"),\n \t\t\t\t\t cur_fanout_pos, fanout_value, i);\n \t\t\tcur_fanout_pos++;\n \t\t}\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \t\tif (!parse_commit_in_graph_one(r, g, graph_commit))\n-\t\t\tgraph_report(\"failed to parse %s from commit-graph\",\n+\t\t\tgraph_report(_(\"failed to parse commit %s from commit-graph\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \t}\n \n@@ -1187,7 +1187,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\tuint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);\n \n \t\tif (g->num_commits != fanout_value)\n-\t\t\tgraph_report(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\",\n+\t\t\tgraph_report(_(\"commit-graph has incorrect fanout value: fanout[%d] = %u != %u\"),\n \t\t\t\t cur_fanout_pos, fanout_value, i);\n \n \t\tcur_fanout_pos++;\n@@ -1209,14 +1209,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \t\todb_commit = (struct commit *)create_object(r, cur_oid.hash, alloc_commit_node(r));\n \t\tif (parse_commit_internal(odb_commit, 0, 0)) {\n-\t\t\tgraph_report(\"failed to parse %s from object database\",\n+\t\t\tgraph_report(_(\"failed to parse commit %s from object database for commit-graph\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!oideq(&get_commit_tree_in_graph_one(r, g, graph_commit)->object.oid,\n \t\t\t get_commit_tree_oid(odb_commit)))\n-\t\t\tgraph_report(\"root tree OID for commit %s in commit-graph is %s != %s\",\n+\t\t\tgraph_report(_(\"root tree OID for commit %s in commit-graph is %s != %s\"),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t oid_to_hex(get_commit_tree_oid(graph_commit)),\n \t\t\t\t oid_to_hex(get_commit_tree_oid(odb_commit)));\n@@ -1226,13 +1226,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \n \t\twhile (graph_parents) {\n \t\t\tif (odb_parents == NULL) {\n-\t\t\t\tgraph_report(\"commit-graph parent list for commit %s is too long\",\n+\t\t\t\tgraph_report(_(\"commit-graph parent list for commit %s is too long\"),\n \t\t\t\t\t oid_to_hex(&cur_oid));\n \t\t\t\tbreak;\n \t\t\t}\n \n \t\t\tif (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid))\n-\t\t\t\tgraph_report(\"commit-graph parent for %s is %s != %s\",\n+\t\t\t\tgraph_report(_(\"commit-graph parent for %s is %s != %s\"),\n \t\t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t\t oid_to_hex(&graph_parents->item->object.oid),\n \t\t\t\t\t oid_to_hex(&odb_parents->item->object.oid));\n@@ -1245,16 +1245,16 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t}\n \n \t\tif (odb_parents != NULL)\n-\t\t\tgraph_report(\"commit-graph parent list for commit %s terminates early\",\n+\t\t\tgraph_report(_(\"commit-graph parent list for commit %s terminates early\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n \t\tif (!graph_commit->generation) {\n \t\t\tif (generation_zero == GENERATION_NUMBER_EXISTS)\n-\t\t\t\tgraph_report(\"commit-graph has generation number zero for commit %s, but non-zero elsewhere\",\n+\t\t\t\tgraph_report(_(\"commit-graph has generation number zero for commit %s, but non-zero elsewhere\"),\n \t\t\t\t\t oid_to_hex(&cur_oid));\n \t\t\tgeneration_zero = GENERATION_ZERO_EXISTS;\n \t\t} else if (generation_zero == GENERATION_ZERO_EXISTS)\n-\t\t\tgraph_report(\"commit-graph has non-zero generation number for commit %s, but zero elsewhere\",\n+\t\t\tgraph_report(_(\"commit-graph has non-zero generation number for commit %s, but zero elsewhere\"),\n \t\t\t\t oid_to_hex(&cur_oid));\n \n \t\tif (generation_zero == GENERATION_ZERO_EXISTS)\n@@ -1269,13 +1269,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t\tmax_generation--;\n \n \t\tif (graph_commit->generation != max_generation + 1)\n-\t\t\tgraph_report(\"commit-graph generation for commit %s is %u != %u\",\n+\t\t\tgraph_report(_(\"commit-graph generation for commit %s is %u != %u\"),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t graph_commit->generation,\n \t\t\t\t max_generation + 1);\n \n \t\tif (graph_commit->date != odb_commit->date)\n-\t\t\tgraph_report(\"commit date for commit %s in commit-graph is %\"PRItime\" != %\"PRItime,\n+\t\t\tgraph_report(_(\"commit date for commit %s in commit-graph is %\"PRItime\" != %\"PRItime),\n \t\t\t\t oid_to_hex(&cur_oid),\n \t\t\t\t graph_commit->date,\n \t\t\t\t odb_commit->date);\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372423","messageId":"20190325120834.15529-8-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 7/8] commit-graph write: don't die if the existing graph is corrupt","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:33Z","receivedAt":"2019-03-25T12:08:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the commit-graph is written we end up calling\nparse_commit(). This will in turn invoke code that'll consult the\nexisting commit-graph about the commit, if the graph is corrupted we\ndie.\n\nWe thus get into a state where a failing \"commit-graph verify\" can't\nbe followed-up with a \"commit-graph write\" if core.commitGraph=true is\nset, the graph either needs to be manually removed to proceed, or\ncore.commitGraph needs to be set to \"false\".\n\nChange the \"commit-graph write\" codepath to use a new\nparse_commit_no_graph() helper instead of parse_commit() to avoid\nthis. The latter will call repo_parse_commit_internal() with\nuse_commit_graph=1 as seen in 177722b344 (\"commit: integrate commit\ngraph with commit parsing\", 2018-04-10).\n\nNot using the old graph at all slows down the writing of the new graph\nby some small amount, but is a sensible way to prevent an error in the\nexisting commit-graph from spreading.\n\nJust fixing the current issue would be likely to result in code that's\ninadvertently broken in the future. New code might use the\ncommit-graph at a distance. To detect such cases introduce a\n\"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\" setting used when we do our\ncorruption tests, and test that a \"write/verify\" combo works after\nevery one of our current test cases where we now detect commit-graph\ncorruption.\n\nSome of the code changes here might be strictly unnecessary, e.g. I\nwas unable to find cases where the parse_commit() called from\nwrite_graph_chunk_data() didn't exit early due to\n\"item->object.parsed\" being true in\nrepo_parse_commit_internal() (before the use_commit_graph=1 has any\neffect). But let's also convert those cases for good measure, we do\nnot have exhaustive tests for all possible types of commit-graph\ncorruption.\n\nThis might need to be re-visited if we learn to write the commit-graph\nincrementally, but probably not. Hopefully we'll just start by finding\nout what commits we have in total, then read the old graph(s) to see\nwhat they cover, and finally write a new graph file with everything\nthat's missing. In that case the new graph writing code just needs to\ncontinue to use e.g. a parse_commit() that doesn't consult the\nexisting commit-graphs.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n commit-graph.c | 10 +++++++---\n commit-graph.h | 1 +\n commit.h | 6 ++++++\n t/t5318-commit-graph.sh | 11 +++++++++--\n 4 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a26d266663..34ecaaf857 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -311,6 +311,10 @@ static int prepare_commit_graph(struct repository *r)\n \tstruct object_directory *odb;\n \tint config_value;\n \n+\tif (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n+\t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n+\t\t GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n+\n \tif (r->objects->commit_graph_attempted)\n \t\treturn !!r->objects->commit_graph;\n \tr->objects->commit_graph_attempted = 1;\n@@ -575,7 +579,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(progress, ++*progress_cnt);\n \n-\t\tparse_commit(*list);\n+\t\tparse_commit_no_graph(*list);\n \t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n \n \t\tparent = (*list)->parents;\n@@ -772,7 +776,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)\n \t\tdisplay_progress(progress, i + 1);\n \t\tcommit = lookup_commit(the_repository, &oids->list[i]);\n \n-\t\tif (commit && !parse_commit(commit))\n+\t\tif (commit && !parse_commit_no_graph(commit))\n \t\t\tadd_missing_parents(oids, commit);\n \t}\n \tstop_progress(&progress);\n@@ -1021,7 +1025,7 @@ void write_commit_graph(const char *obj_dir,\n \t\t\tcontinue;\n \n \t\tcommits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);\n-\t\tparse_commit(commits.list[commits.nr]);\n+\t\tparse_commit_no_graph(commits.list[commits.nr]);\n \n \t\tfor (parent = commits.list[commits.nr]->parents;\n \t\t parent; parent = parent->next)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex ada7aea9ed..7dfb8c896f 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -7,6 +7,7 @@\n #include \"cache.h\"\n \n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n+#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n \n struct commit;\n \ndiff --git a/commit.h b/commit.h\nindex 42728c2906..5d33477e78 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)\n {\n \treturn repo_parse_commit_gently(r, item, 0);\n }\n+\n+static inline int parse_commit_no_graph(struct commit *commit)\n+{\n+\treturn repo_parse_commit_internal(the_repository, commit, 0, 0);\n+}\n+\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)\n #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 1cb0355c7e..d146cf4982 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -377,7 +377,13 @@ corrupt_graph_verify() {\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err &&\n-\tgit status --short\n+\tif test \"$2\" != \"no-copy\"\n+\tthen\n+\t\tcp $objdir/info/commit-graph commit-graph-pre-write-test\n+\tfi &&\n+\tgit status --short &&\n+\tGIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&\n+\tgit commit-graph verify\n }\n \n # usage: corrupt_graph_and_verify []\n@@ -403,7 +409,7 @@ corrupt_graph_and_verify() {\n test_expect_success POSIXPERM,SANITY 'detect permission problem' '\n \tcorrupt_graph_setup &&\n \tchmod 000 $objdir/info/commit-graph &&\n-\tcorrupt_graph_verify \"Could not open\"\n+\tcorrupt_graph_verify \"Could not open\" \"no-copy\"\n '\n \n test_expect_success 'detect too small' '\n@@ -522,6 +528,7 @@ test_expect_success 'git fsck (checks commit-graph)' '\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n \ttest_must_fail git fsck\n '\n \n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372424","messageId":"20190325120834.15529-7-avarab@gmail.com","threadId":"50554","inReplyTo":"20190314214740.23360-1-avarab@gmail.com","subject":"[PATCH v3 6/8] commit-graph verify: detect inability to read the graph","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T12:08:32Z","receivedAt":"2019-03-25T12:09:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change \"commit-graph verify\" to error on open() failures other than\nENOENT. As noted in the third paragraph of 283e68c72f (\"commit-graph:\nadd 'verify' subcommand\", 2018-06-27) and the test it added it's\nintentional that \"commit-graph verify\" doesn't error out when the file\ndoesn't exist.\n\nBut let's not be overly promiscuous in what we accept. If we can't\nread the file for other reasons, e.g. permission errors, bad file\ndescriptor etc. we'd like to report an error to the user.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 +++-\n t/t5318-commit-graph.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 8196fdbe9c..537fdfd0f0 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -62,8 +62,10 @@ static int graph_verify(int argc, const char **argv)\n \n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \topen_ok = open_commit_graph(graph_name, &fd, &st);\n-\tif (!open_ok)\n+\tif (!open_ok && errno == ENOENT)\n \t\treturn 0;\n+\tif (!open_ok)\n+\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \tgraph = load_commit_graph_one_fd_st(fd, &st);\n \tFREE_AND_NULL(graph_name);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 71d775e313..1cb0355c7e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -400,6 +400,12 @@ corrupt_graph_and_verify() {\n \n }\n \n+test_expect_success POSIXPERM,SANITY 'detect permission problem' '\n+\tcorrupt_graph_setup &&\n+\tchmod 000 $objdir/info/commit-graph &&\n+\tcorrupt_graph_verify \"Could not open\"\n+'\n+\n test_expect_success 'detect too small' '\n \tcorrupt_graph_setup &&\n \techo \"a small graph\" >$objdir/info/commit-graph &&\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"374600","messageId":"87y33vr41k.fsf@evledraar.gmail.com","threadId":"50554","inReplyTo":"20190325120834.15529-5-avarab@gmail.com","subject":"Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-27T13:06:15Z","receivedAt":"2019-04-27T13:06:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 25 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> Make the commit-graph loading code work as a library that returns an\n> error code instead of calling exit(1) when the commit-graph is\n> corrupt. This means that e.g. \"status\" will now report commit-graph\n> corruption as an \"error: [...]\" at the top of its output, but then\n> proceed to work normally.\n>\n> This required splitting up the load_commit_graph_one() function so\n> that the code that deals with open()-ing and stat()-ing the graph can\n> now be called independently as open_commit_graph().\n>\n> This is needed because \"commit-graph verify\" where the graph doesn't\n> exist isn't an error. See the third paragraph in\n> 283e68c72f (\"commit-graph: add 'verify' subcommand\",\n> 2018-06-27). There's a bug in that logic where we conflate the\n> intended ENOENT with other errno values (e.g. EACCES), but this change\n> doesn't address that. That'll be addressed in a follow-up change.\n>\n> I'm then splitting most of the logic out of load_commit_graph_one()\n> into load_commit_graph_one_fd_st(), which allows for providing an\n> existing file descriptor and stat information to the loading\n> code. This isn't strictly needed, but it would be redundant and\n> confusing to open() and stat() the file twice for some of the\n> codepaths, this allows for calling open_commit_graph() followed by\n> load_commit_graph_one_fd_st(). The \"graph_file\" still needs to be\n> passed to that function for the the \"graph file %s is too small\" error\n> message.\n>\n> This leaves load_commit_graph_one() unused by everything except the\n> internal prepare_commit_graph_one() function, so let's mark it as\n> \"static\". If someone needs it in the future we can remove the \"static\"\n> attribute. I could also rewrite its sole remaining\n> user (\"prepare_commit_graph_one()\") to use\n> load_commit_graph_one_fd_st() instead, but let's leave it at this.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Ramsay Jones \n> ---\n> builtin/commit-graph.c | 21 +++++++++++++++++----\n> commit-graph.c | 42 +++++++++++++++++++++++++++++------------\n> commit-graph.h | 4 +++-\n> t/t5318-commit-graph.sh | 2 +-\n> 4 files changed, 51 insertions(+), 18 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 4ae502754c..32bcc63427 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)\n> {\n> \tstruct commit_graph *graph = NULL;\n> \tchar *graph_name;\n> +\tint open_ok;\n> +\tint fd;\n> +\tstruct stat st;\n>\n> \tstatic struct option builtin_commit_graph_verify_options[] = {\n> \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n> @@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)\n> \t\topts.obj_dir = get_object_directory();\n>\n> \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n> -\tgraph = load_commit_graph_one(graph_name);\n> +\topen_ok = open_commit_graph(graph_name, &fd, &st);\n> +\tif (!open_ok)\n> +\t\treturn 0;\n> +\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n> \tFREE_AND_NULL(graph_name);\n>\n> \tif (!graph)\n> -\t\treturn 0;\n> +\t\treturn 1;\n>\n> \tUNLEAK(graph);\n> \treturn verify_commit_graph(the_repository, graph);\n> @@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)\n> {\n> \tstruct commit_graph *graph = NULL;\n> \tchar *graph_name;\n> +\tint open_ok;\n> +\tint fd;\n> +\tstruct stat st;\n>\n> \tstatic struct option builtin_commit_graph_read_options[] = {\n> \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n> @@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)\n> \t\topts.obj_dir = get_object_directory();\n>\n> \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n> -\tgraph = load_commit_graph_one(graph_name);\n>\n> +\topen_ok = open_commit_graph(graph_name, &fd, &st);\n> +\tif (!open_ok)\n> +\t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n> +\n> +\tgraph = load_commit_graph_one_fd_st(graph_name, fd, &st);\n> \tif (!graph)\n> -\t\tdie(\"graph file %s does not exist\", graph_name);\n> +\t\treturn 1;\n>\n> \tFREE_AND_NULL(graph_name);\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f8201d888b..3acc032c1b 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)\n> \treturn 1;\n> }\n>\n> -struct commit_graph *load_commit_graph_one(const char *graph_file)\n> +int open_commit_graph(const char *graph_file, int *fd, struct stat *st)\n> +{\n> +\t*fd = git_open(graph_file);\n> +\tif (*fd < 0)\n> +\t\treturn 0;\n> +\tif (fstat(*fd, st)) {\n> +\t\tclose(*fd);\n> +\t\treturn 0;\n> +\t}\n> +\treturn 1;\n> +}\n> +\n> +struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n> +\t\t\t\t\t\t int fd, struct stat *st)\n> {\n> \tvoid *graph_map;\n> \tsize_t graph_size;\n> -\tstruct stat st;\n> \tstruct commit_graph *ret;\n> -\tint fd = git_open(graph_file);\n>\n> -\tif (fd < 0)\n> -\t\treturn NULL;\n> -\tif (fstat(fd, &st)) {\n> -\t\tclose(fd);\n> -\t\treturn NULL;\n> -\t}\n> -\tgraph_size = xsize_t(st.st_size);\n> +\tgraph_size = xsize_t(st->st_size);\n>\n> \tif (graph_size < GRAPH_MIN_SIZE) {\n> \t\tclose(fd);\n> -\t\tdie(_(\"graph file %s is too small\"), graph_file);\n> +\t\terror(_(\"graph file %s is too small\"), graph_file);\n> +\t\treturn NULL;\n> \t}\n> \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> \tret = parse_commit_graph(graph_map, fd, graph_size);\n> @@ -106,7 +112,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \tif (!ret) {\n> \t\tmunmap(graph_map, graph_size);\n> \t\tclose(fd);\n> -\t\texit(1);\n> \t}\n>\n> \treturn ret;\n> @@ -269,6 +274,19 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> \treturn graph;\n> }\n>\n> +static struct commit_graph *load_commit_graph_one(const char *graph_file)\n> +{\n> +\n> +\tstruct stat st;\n> +\tint fd;\n> +\tint open_ok = open_commit_graph(graph_file, &fd, &st);\n> +\n> +\tif (!open_ok)\n> +\t\treturn NULL;\n> +\n> +\treturn load_commit_graph_one_fd_st(graph_file, fd, &st);\n> +}\n> +\n> static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n> {\n> \tchar *graph_name;\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 096d8bac34..77cc739bc0 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -11,6 +11,7 @@\n> struct commit;\n>\n> char *get_commit_graph_filename(const char *obj_dir);\n> +int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n>\n> /*\n> * Given a commit struct, try to fill the commit struct info, including:\n> @@ -52,7 +53,8 @@ struct commit_graph {\n> \tconst unsigned char *chunk_extra_edges;\n> };\n>\n> -struct commit_graph *load_commit_graph_one(const char *graph_file);\n> +struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,\n> +\t\t\t\t\t\t int fd, struct stat *st);\n>\n> struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> \t\t\t\t\tsize_t graph_size);\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index ad3a695f76..71d775e313 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -377,7 +377,7 @@ corrupt_graph_verify() {\n> \ttest_must_fail git commit-graph verify 2>test_err &&\n> \tgrep -v \"^+\" test_err >err &&\n> \ttest_i18ngrep \"$grepstr\" err &&\n> -\ttest_might_fail git status --short\n> +\tgit status --short\n> }\n>\n> # usage: corrupt_graph_and_verify []\n\nThere's still cases left where we'll exit early, e.g. if you do:\n\n $ git diff -U1\n diff --git a/commit-graph.c b/commit-graph.c\n index 66865acbd7..63773764ce 100644\n --- a/commit-graph.c\n +++ b/commit-graph.c\n @@ -1074,3 +1074,3 @@ void write_commit_graph(const char *obj_dir,\n chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;\n - chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;\n + chunk_offsets[2] = chunk_offsets[0] + hashsz * commits.nr;\n chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;\n\nWhich is obviously bad, but something I encounterd while hacking up [1]\nwe'll still hard die as before this patch on:\n\n $ git status\n fatal: invalid parent position 1734910766\n $\n\nStacktrace:\n\n (gdb) bt\n #0 die (err=0x5555558008cd \"invalid parent position %lu\") at usage.c:153\n #1 0x000055555565e88c in insert_parent_or_die (r=0x5555558d6320 , g=0x5555559d4ff0, pos=1734910766, pptr=0x5555559c0f00) at commit-graph.c:383\n #2 0x000055555565eae0 in fill_commit_in_graph (r=0x5555558d6320 , item=0x5555559c0ed0, g=0x5555559d4ff0, pos=102470) at commit-graph.c:426\n #3 0x000055555565ec56 in parse_commit_in_graph_one (r=0x5555558d6320 , g=0x5555559d4ff0, item=0x5555559c0ed0) at commit-graph.c:469\n #4 0x000055555565eca5 in parse_commit_in_graph (r=0x5555558d6320 , item=0x5555559c0ed0) at commit-graph.c:478\n #5 0x000055555565aaf4 in repo_parse_commit_internal (r=0x5555558d6320 , item=0x5555559c0ed0, quiet_on_missing=0, use_commit_graph=1) at commit.c:465\n #6 0x000055555565ac32 in repo_parse_commit_gently (r=0x5555558d6320 , item=0x5555559c0ed0, quiet_on_missing=0) at commit.c:490\n #7 0x000055555573347f in repo_parse_commit (r=0x5555558d6320 , item=0x5555559c0ed0) at commit.h:90\n #8 0x0000555555734764 in get_reference (revs=0x7fffffffcf50, name=0x55555582ceb8 \"HEAD\", oid=0x7fffffffce60, flags=0) at revision.c:365\n #9 0x000055555573b0e2 in setup_revisions (argc=0, argv=0x0, revs=0x7fffffffcf50, opt=0x7fffffffcf30) at revision.c:2611\n #10 0x00005555557a6985 in wt_status_collect_changes_index (s=0x5555558a3de0 ) at wt-status.c:618\n #11 0x00005555557a70a5 in wt_status_collect (s=0x5555558a3de0 ) at wt-status.c:761\n #12 0x0000555555595ede in cmd_status (argc=0, argv=0x7fffffffdda8, prefix=0x0) at builtin/commit.c:1408\n #13 0x000055555556f8a2 in run_builtin (p=0x5555558931a8 , argc=1, argv=0x7fffffffdda8) at git.c:444\n #14 0x000055555556fbe3 in handle_builtin (argc=1, argv=0x7fffffffdda8) at git.c:676\n #15 0x000055555556fe4d in run_argv (argcp=0x7fffffffdc4c, argv=0x7fffffffdc40) at git.c:743\n #16 0x000055555557014b in cmd_main (argc=1, argv=0x7fffffffdda8) at git.c:877\n #17 0x000055555562d6d2 in main (argc=3, argv=0x7fffffffdd98) at common-main.c:50\n\nI might (or not) get to this some other time, meanwhile sending an\nE-Mail report...\n\n1. https://public-inbox.org/git/87zhobr4fl.fsf@evledraar.gmail.com/\n"},{"id":"374638","messageId":"3518ad3e-bc4a-c2c3-d4bd-c87f9e828b1c@gmail.com","threadId":"50554","inReplyTo":"87y33vr41k.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. \"git status\"","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-04-29T12:48:51Z","receivedAt":"2019-04-29T12:48:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/27/2019 9:06 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> There's still cases left where we'll exit early, e.g. if you do:\n> \n> $ git diff -U1\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 66865acbd7..63773764ce 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1074,3 +1074,3 @@ void write_commit_graph(const char *obj_dir,\n> chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;\n> - chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;\n> + chunk_offsets[2] = chunk_offsets[0] + hashsz * commits.nr;\n> chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;\n> \n> Which is obviously bad, but something I encounterd while hacking up [1]\n> we'll still hard die as before this patch on:\n> \n> $ git status\n> fatal: invalid parent position 1734910766\n> $\n\nI really appreciate you digging in deep into these kinds of issues. You\nseem to be hitting corrupted commit-graph files more often than we are\n(in VFS for Git world). However, we should be _very careful_ when turning\nsome of these errors to warnings. At the very least, we should do some\nhigh-level planning for how to handle this case.\n\nThe biggest issue is that we have some logic that is run after a call to\ngeneration_numbers_enabled(), such as the `git rev-list --topo-order`\nlogic, that relies on the commit-graph for correctness. If we output a\nwarning and then stop using the commit-graph, then we will start having\ncommits with finite generation pointing to commits with infinite generation.\n\nPerhaps, with some care, we can alert the algorithm to change the \"minimum\ngeneration\" that limits how far we dequeue the priority-queue. Changing it\nto zero will cause the algorithm to behave like the old algorithm.\n\nBut, having an algorithm that usually takes 0.1 seconds suddenly take 10+\nseconds also violates some expectations.\n\nQ: How should we handle a detectably-invalid commit-graph?\n\nI think most of your patches have done a good job so far of detecting\nan invalid header, and responding by ignoring the commit-graph. This case\nof a detectable error in the chunk data itself is not something we can\ncheck on the first load without serious performance issues.\n\nI hope we can decide on a good solution.\n\nThanks,\n-Stolee\n"},{"id":"374642","messageId":"87wojcrii9.fsf@evledraar.gmail.com","threadId":"50554","inReplyTo":"3518ad3e-bc4a-c2c3-d4bd-c87f9e828b1c@gmail.com","subject":"Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. \"git status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-29T14:30:38Z","receivedAt":"2019-04-29T14:30:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 29 2019, Derrick Stolee wrote:\n\n> On 4/27/2019 9:06 AM, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> There's still cases left where we'll exit early, e.g. if you do:\n>>\n>> $ git diff -U1\n>> diff --git a/commit-graph.c b/commit-graph.c\n>> index 66865acbd7..63773764ce 100644\n>> --- a/commit-graph.c\n>> +++ b/commit-graph.c\n>> @@ -1074,3 +1074,3 @@ void write_commit_graph(const char *obj_dir,\n>> chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;\n>> - chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;\n>> + chunk_offsets[2] = chunk_offsets[0] + hashsz * commits.nr;\n>> chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;\n>>\n>> Which is obviously bad, but something I encounterd while hacking up [1]\n>> we'll still hard die as before this patch on:\n>>\n>> $ git status\n>> fatal: invalid parent position 1734910766\n>> $\n>\n> I really appreciate you digging in deep into these kinds of issues. You\n> seem to be hitting corrupted commit-graph files more often than we are\n> (in VFS for Git world).\n\nFWIW I've never encountered any of these in the wild. I just started\npoking at this in 2ac138d568 (\"commit-graph: fix segfault on e.g. \"git\nstatus\"\", 2019-03-25) because I was looking at the commit graph, running\nits tests with -d, and we'd segfault previously on e.g. \"git status\" in\nour own graph corruption tests.\n\n> However, we should be _very careful_ when turning some of these errors\n> to warnings. At the very least, we should do some high-level planning\n> for how to handle this case.\n\nIndeed. I should have been explicit, I don't think it's sane to do\nanything except return NULL up the stack and say \"the graph is screwed,\nwe can't use it\" when initially parsing it/headers, but reading on...\n\n> The biggest issue is that we have some logic that is run after a call to\n> generation_numbers_enabled(), such as the `git rev-list --topo-order`\n> logic, that relies on the commit-graph for correctness. If we output a\n> warning and then stop using the commit-graph, then we will start having\n> commits with finite generation pointing to commits with infinite generation.\n>\n> Perhaps, with some care, we can alert the algorithm to change the \"minimum\n> generation\" that limits how far we dequeue the priority-queue. Changing it\n> to zero will cause the algorithm to behave like the old algorithm.\n>\n> But, having an algorithm that usually takes 0.1 seconds suddenly take 10+\n> seconds also violates some expectations.\n>\n> Q: How should we handle a detectably-invalid commit-graph?\n\n...I don't think we need to do any paranoid algorithm fallback in\ngeneral. As you point out that's going to be a PITA as we read the\nactual data in the graph in some cases.\n\n> I think most of your patches have done a good job so far of detecting\n> an invalid header, and responding by ignoring the commit-graph. This case\n> of a detectable error in the chunk data itself is not something we can\n> check on the first load without serious performance issues.\n>\n> I hope we can decide on a good solution.\n\n...OK, so this is one of those PITA cases. I think it's fine to just\nleave it.\n\nAlthough maybe we'd still want to be more paranoid with O(n) cases like\n\"--contains\" or \"--topo-order\" and cases that are surely just \"look up\nmy immediate commit data\", like \"status\" dying on this particular error.\n\nBut I think it's fine to just decide to nothing about this. I mainly\njust wanted to make a note to myself & CC the list in case there was\ninterest...\n"},{"id":"374774","messageId":"20190501183108.GE4109@sigill.intra.peff.net","threadId":"50554","inReplyTo":"3518ad3e-bc4a-c2c3-d4bd-c87f9e828b1c@gmail.com","subject":"Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. \"git status\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-01T18:31:08Z","receivedAt":"2019-05-01T18:31:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2019 at 08:48:51AM -0400, Derrick Stolee wrote:\n\n> Q: How should we handle a detectably-invalid commit-graph?\n> \n> I think most of your patches have done a good job so far of detecting\n> an invalid header, and responding by ignoring the commit-graph. This case\n> of a detectable error in the chunk data itself is not something we can\n> check on the first load without serious performance issues.\n> \n> I hope we can decide on a good solution.\n\nI think it's OK to die() unceremoniously in these cases. We already have\nsimilar behavior when dealing with packfiles. While we try to just\nreturn an error for bit-flips in data (so we can try finding the object\nelsewhere), if there are things like nonsensical file offsets, we\ngenerally just give up.\n\nSo I think it makes sense to start with the _safe_ thing, which is just\ngiving up on the operation and informing the user. If somebody wants to\nthen loosen things up on a case by case basis (after seeing that it\nwouldn't produce incorrect results to do so), that can be future work.\n\nFor the commit-graph, though, and given that we're talking about file\ncorruption, I don't know that it's even worth the effort. The solution\nis basically always going to be \"delete the commit-graph file and\nregenerate it\". It's perhaps slightly inconvenient compared to falling\nback, but this just isn't something we'd expect to happen often.\n\n-Peff\n"}]}