Re: [PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 22, 2019, 23:13 UTC
- Message-ID
- <xmqqzhqno1r3.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190221223753.20070-8-avarab@gmail.com>
Æ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 ;-).
Show 5 quoted lines
> 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.
Show 25 quoted lines
> 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.
Show 10 quoted lines
> 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 > '