Re: [PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt

[Junio C Hamano <gitster@pobox.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 ;-).

> 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
>  '