@matheusbsbs / matheus-bagliano / commits / 43d3561805

commit-graph write: don't die if the existing graph is corrupt

When the commit-graph is written we end up calling parse_commit(). This will in turn invoke code that'll consult the existing commit-graph about the commit, if the graph is corrupted we die. We thus get into a state where a failing "commit-graph verify" can't be followed-up with a "commit-graph write" if core.commitGraph=true is set, the graph either needs to be manually removed to proceed, or core.commitGraph needs to be set to "false". Change the "commit-graph write" codepath to use a new parse_commit_no_graph() helper instead of parse_commit() to avoid this. The latter will call repo_parse_commit_internal() with use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit graph with commit parsing", 2018-04-10). Not using the old graph at all slows down the writing of the new graph by some small amount, but is a sensible way to prevent an error in the existing commit-graph from spreading. Just fixing the current issue would be likely to result in code that's inadvertently broken in the future. New code might use the commit-graph at a distance. To detect such cases introduce a "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD" setting used when we do our corruption tests, and test that a "write/verify" combo works after every one of our current test cases where we now detect commit-graph corruption. Some of the code changes here might be strictly unnecessary, e.g. I was unable to find cases where the parse_commit() called from write_graph_chunk_data() didn't exit early due to "item->object.parsed" being true in repo_parse_commit_internal() (before the use_commit_graph=1 has any effect). But let's also convert those cases for good measure, we do not have exhaustive tests for all possible types of commit-graph corruption. This might need to be re-visited if we learn to write the commit-graph incrementally, but probably not. Hopefully we'll just start by finding out what commits we have in total, then read the old graph(s) to see what they cover, and finally write a new graph file with everything that's missing. In that case the new graph writing code just needs to continue to use e.g. a parse_commit() that doesn't consult the existing commit-graphs. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Mar 25, 2019 at 13:08 UTC 43d356180556180b4ef6ac232a14498a5bb2b446

4 files changed +23 -5

commit-graph.c

+7 -3

@@ -311,6 +311,10 @@ static int prepare_commit_graph(struct repository *r)

311 struct object_directory *odb;

312 int config_value;

313

314 + if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))

315 + die("dying as requested by the '%s' variable on commit-graph load!",

316 + GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);

317 +

318 if (r->objects->commit_graph_attempted)

319 return !!r->objects->commit_graph;

320 r->objects->commit_graph_attempted = 1;

@@ -575,7 +579,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,

579 uint32_t packedDate[2];

580 display_progress(progress, ++*progress_cnt);

581

578 - parse_commit(*list);

582 + parse_commit_no_graph(*list);

583 hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);

584

585 parent = (*list)->parents;

@@ -772,7 +776,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)

776 display_progress(progress, i + 1);

777 commit = lookup_commit(the_repository, &oids->list[i]);

778

775 - if (commit && !parse_commit(commit))

779 + if (commit && !parse_commit_no_graph(commit))

780 add_missing_parents(oids, commit);

781 }

782 stop_progress(&progress);

@@ -1021,7 +1025,7 @@ void write_commit_graph(const char *obj_dir,

1025 continue;

1026

1027 commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);

1024 - parse_commit(commits.list[commits.nr]);

1028 + parse_commit_no_graph(commits.list[commits.nr]);

1029

1030 for (parent = commits.list[commits.nr]->parents;

1031 parent; parent = parent->next)

commit-graph.h

@@ -7,6 +7,7 @@

7 #include "cache.h"

9 #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"

10 +#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"

12 struct commit;

commit.h

@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)

89 {

90 return repo_parse_commit_gently(r, item, 0);

91 }

92 +

93 +static inline int parse_commit_no_graph(struct commit *commit)

94 +{

95 + return repo_parse_commit_internal(the_repository, commit, 0, 0);

96 +}

97 +

98 #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS

99 #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)

100 #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)

t/t5318-commit-graph.sh

+9 -2

@@ -377,7 +377,13 @@ corrupt_graph_verify() {

377 test_must_fail git commit-graph verify 2>test_err &&

378 grep -v "^+" test_err >err &&

379 test_i18ngrep "$grepstr" err &&

380 - git status --short

380 + if test "$2" != "no-copy"

381 + then

382 + cp $objdir/info/commit-graph commit-graph-pre-write-test

383 + fi &&

384 + git status --short &&

385 + GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&

386 + git commit-graph verify

387 }

388

389 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]

@@ -403,7 +409,7 @@ corrupt_graph_and_verify() {

409 test_expect_success POSIXPERM,SANITY 'detect permission problem' '

410 corrupt_graph_setup &&

411 chmod 000 $objdir/info/commit-graph &&

406 - corrupt_graph_verify "Could not open"

412 + corrupt_graph_verify "Could not open" "no-copy"

413 '

414

415 test_expect_success 'detect too small' '

@@ -522,6 +528,7 @@ test_expect_success 'git fsck (checks commit-graph)' '

528 git fsck &&

529 corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \

530 "incorrect checksum" &&

531 + cp commit-graph-pre-write-test $objdir/info/commit-graph &&

532 test_must_fail git fsck

533 '

534