Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
> '
