Derrick Stolee <dstolee@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> 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
