Re: [PATCH v2 03/12] commit-graph: test that 'verify' finds corruption

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <dstolee@microsoft.com> writes:

> Add test cases to t5318-commit-graph.sh that corrupt the commit-graph
> file and check that the 'git commit-graph verify' command fails. These
> tests verify the header and chunk information is checked carefully.
>
> Helped-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t5318-commit-graph.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 6ca451dfd2..0cb88232fa 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -240,4 +240,57 @@ test_expect_success 'git commit-graph verify' '
>  	git commit-graph verify >output
>  '
>  
> +# usage: corrupt_data <file> <pos> [<data>]
> +corrupt_data() {
> +	file=$1
> +	pos=$2
> +	data="${3:-\0}"
> +	printf "$data" | dd of="$file" bs=1 seek="$pos" conv=notrunc
> +}

First, if we do this that way (and not by adding a test helper), the use
of this function should be, I think, protected using appropriate test
prerequisite.  Not everyone has 'dd' tool installed, for example on
MS Windows.

Second, the commit-graph file format has H-byte HASH-checksum of all of
the contents excluding checksum trailer.  It feels like any corruption
should have been caught by checksum test; thus to actually test that
contents is verified we should adjust checksum too, e.g. with sha1sum if
available or with test helper... oh, actually we have t/helper/test-sha1.
Unfortulately, it looks like it has no docs (beside commit message).

> +
> +test_expect_success 'detect bad signature' '
> +	cd "$TRASH_DIRECTORY/full" &&

This 'cd' outside subshell and withou accompanying change back feels a
bit strange to me.

> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 0 "\0" &&

So 'CGPH' signature is currupted into '\0GPH'.

> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&

Minor nit: redirection should be cuddled to the file, i.e.:

  +	grep -v "^\+" err >verify-errors &&

A question: why do you filter-out lines starting with "+" here?

> +	test_line_count = 1 verify-errors &&
> +	grep "graph signature" verify-errors

If messages from 'git commit-graph verify' can be localized (are
translatable), then it should be i18n_grep, isn't it?

> +'
> +
> +test_expect_success 'detect bad version number' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 4 "\02" &&

All right, so we replace commit-graph format version 1 ("\01") with
version 2 ("\02").  First, why 2 and not 0?  Second, is "\02" portable?

> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&
> +	test_line_count = 1 verify-errors &&

The above three lines is common across all test cases; I wonder if it
would be possible to extract it into function, to avoid code
duplication.

> +	grep "graph version" verify-errors
> +'
> +
> +test_expect_success 'detect bad hash version' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 5 "\02" &&

All right, so we change / corrupt hash version from value of 1, which
means SHA-1, to value of 2... which would soon meen NewHash.  Why not
"\777" (i.e. 0xff)?

> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&
> +	test_line_count = 1 verify-errors &&
> +	grep "hash version" verify-errors
> +'


Note: all of the above tests check in load_commit_graph_one(), not the
one in verify_commit_graph().  Just FYI.

> +
> +test_expect_success 'detect too small chunk-count' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 6 "\01" &&
> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&
> +	test_line_count = 2 verify-errors &&
> +	grep "missing the OID Lookup chunk" verify-errors &&
> +	grep "missing the Commit Data chunk" verify-errors

This feels too implementation specific.  We should have at least two
chunks missing (there are 3 required chunks, and number of chunks was
changed to 1), but commit-graph format specification does not state that
OID Fanout must be first, and thus it is two remaining required chunks
that would be missing.

> +'
> +
>  test_done

One test that I would like to see that 'git commit-grph verify'
correctly detects without crashing is if commit-graph file gets
truncated at various lengths: shorter than smallest possible
commit-graph file size, in the middle of fixed header, in the middle of
chunk lookup part, in the middle of chunk, just the trailer chopped off.

Best regards,
-- 
Jakub Narębski