| Message ID | 20190221223753.20070-8-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: segfault & other fixes for broken graphs | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. Irony ;-). > 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). That may slow down writing the graph but would be a sensible way to prevent an error in the existing commit-graph from spreading. > This might need to be re-visited if we learn to write the commit-graph > incrementally. Probably yes. If we were doing "repack -a -d (without -f)" style of "incremental", then we do need to revisit this, which is a moral equivalent of saying "do not reuse delta" to "repack", and it would make it impossible to be incremental. Hopefully a true "incremental" shouldn't even have to touch existing data, perhaps similar to "repack (without -a)", in which case the equation may be different. If we'd be computing reachability for only new things, there is nothing gained by allowing parse_commit() to peek into existing commit-graph file (by definition, these new things are not in there---that's the reason why we are incrementally extending the commit-graph by computing the reachability for them). I dunno. > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > index 1ee00fa333..6db658ed66 100755 > --- a/t/t5318-commit-graph.sh > +++ b/t/t5318-commit-graph.sh > @@ -377,7 +377,13 @@ corrupt_graph_verify() { > test_must_fail git commit-graph verify 2>test_err && > grep -v "^+" test_err >err && > test_i18ngrep "$grepstr" err && > - git status --short > + if test -z "$NO_WRITE_TEST_BACKUP" > + then > + cp $objdir/info/commit-graph commit-graph-pre-write-test > + fi && > + git status --short && > + GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write && > + git commit-graph verify > } > > # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] > @@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' ' > # "chmod 000 file" does not yield EACCES on e.g. "cat file" > if ! test -r $objdir/info/commit-graph > then > - corrupt_graph_verify "Could not open" > + NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open" This would not work as you think it would; corrupt_graph_verify is a shell function, so you cannot VAR=VAL prefix to export an environment variable only for the duration of the command. > fi > ' > > @@ -528,6 +534,7 @@ test_expect_success 'git fsck (checks commit-graph)' ' > git fsck && > corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \ > "incorrect checksum" && > + cp commit-graph-pre-write-test $objdir/info/commit-graph && > test_must_fail git fsck > '
On Fri, Feb 22, 2019 at 6:13 PM Junio C Hamano <gitster@pobox.com> wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > > @@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' ' > > # "chmod 000 file" does not yield EACCES on e.g. "cat file" > > if ! test -r $objdir/info/commit-graph > > then > > - corrupt_graph_verify "Could not open" > > + NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open" > > This would not work as you think it would; corrupt_graph_verify is a > shell function, so you cannot VAR=VAL prefix to export an environment > variable only for the duration of the command. As of a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13), this sort of problem is correctly flagged by "make test-lint", which typically is run as part of "make test" or "make prove".
diff --git a/commit-graph.c b/commit-graph.c index d945e8f3e0..6b3ade9496 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -281,6 +281,10 @@ static int prepare_commit_graph(struct repository *r) struct object_directory *odb; int config_value; + if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0)) + die("Dying as requested by the '%s' variable on commit-graph load!", + GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD); + if (r->objects->commit_graph_attempted) return !!r->objects->commit_graph; r->objects->commit_graph_attempted = 1; @@ -545,7 +549,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len, uint32_t packedDate[2]; display_progress(progress, ++*progress_cnt); - parse_commit(*list); + parse_commit_no_graph(*list); hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len); parent = (*list)->parents; @@ -742,7 +746,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress) display_progress(progress, i + 1); commit = lookup_commit(the_repository, &oids->list[i]); - if (commit && !parse_commit(commit)) + if (commit && !parse_commit_no_graph(commit)) add_missing_parents(oids, commit); } stop_progress(&progress); @@ -991,7 +995,7 @@ void write_commit_graph(const char *obj_dir, continue; commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]); - parse_commit(commits.list[commits.nr]); + parse_commit_no_graph(commits.list[commits.nr]); for (parent = commits.list[commits.nr]->parents; parent; parent = parent->next) diff --git a/commit-graph.h b/commit-graph.h index 36d8109901..6021ababa2 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -7,6 +7,7 @@ #include "cache.h" #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH" +#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD" struct commit; diff --git a/commit.h b/commit.h index 42728c2906..5d33477e78 100644 --- a/commit.h +++ b/commit.h @@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item) { return repo_parse_commit_gently(r, item, 0); } + +static inline int parse_commit_no_graph(struct commit *commit) +{ + return repo_parse_commit_internal(the_repository, commit, 0, 0); +} + #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use) #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet) diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 1ee00fa333..6db658ed66 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -377,7 +377,13 @@ corrupt_graph_verify() { test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err && - git status --short + if test -z "$NO_WRITE_TEST_BACKUP" + then + cp $objdir/info/commit-graph commit-graph-pre-write-test + fi && + git status --short && + GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write && + git commit-graph verify } # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] @@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' ' # "chmod 000 file" does not yield EACCES on e.g. "cat file" if ! test -r $objdir/info/commit-graph then - corrupt_graph_verify "Could not open" + NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open" fi ' @@ -528,6 +534,7 @@ test_expect_success 'git fsck (checks commit-graph)' ' git fsck && corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \ "incorrect checksum" && + cp commit-graph-pre-write-test $objdir/info/commit-graph && test_must_fail git fsck '
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). 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- commit-graph.c | 10 +++++++--- commit-graph.h | 1 + commit.h | 6 ++++++ t/t5318-commit-graph.sh | 11 +++++++++-- 4 files changed, 23 insertions(+), 5 deletions(-)