Re: [PATCH v3 07/20] commit-graph: verify catches corrupt signature

From: Jakub Narebski <jnareb@gmail.com>
Date: May 28, 2018, 14:05 UTC
Message-ID: <86muwjyixo.fsf@gmail.com>
In-Reply-To: <20180524162504.158394-8-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 5 quoted lines

> This is the first of several commits that add a test to check that
> 'git commit-graph verify' catches corruption in the commit-graph
> file. The first test checks that the command catches an error in
> the file signature. This is a check that exists in the existing
> commit-graph reading code.

Good start.

This handles 3 out of 5 checks in load_commit_graph_one().  The
remaining are:
* too short file (length smaller than minimal commit-graph size)
* more than one chunk of one of 4 defined types

Show 6 quoted lines

> Add a helper method 'corrupt_graph_and_verify' to the test script
> t5318-commit-graph.sh. This helper corrupts the commit-graph file
> at a certain location, runs 'git commit-graph verify', and reports
> the output to the 'err' file. This data is filtered to remove the
> lines added by 'test_must_fail' when the test is run verbosely.
> Then, the output is checked to contain a specific error message.

Thanks for an explanation.

Show 24 quoted lines

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t5318-commit-graph.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 6ca451dfd2..bd64481c7a 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -235,9 +235,52 @@ test_expect_success 'perform fast-forward merge in full repo' '
>  	test_cmp expect output
>  '
>  
> +# the verify tests below expect the commit-graph to contain
> +# exactly the commits reachable from the commits/8 branch.
> +# If the file changes the set of commits in the list, then the
> +# offsets into the binary file will result in different edits
> +# and the tests will likely break.
> +
>  test_expect_success 'git commit-graph verify' '
>  	cd "$TRASH_DIRECTORY/full" &&
> +	git rev-parse commits/8 | git commit-graph write --stdin-commits &&
>  	git commit-graph verify >output
>  '

I don't quite understand what the change is meant to do.

Also, as I said earlier, I'd prefer if tests were as indpendent of each other as possible, to make running individual tests (e.g. only previously falling tests) easier.

I especially do not like mixing running actual test with setting up the repository for future tests, as here.

Show 9 quoted lines

>  
> +GRAPH_BYTE_VERSION=4
> +GRAPH_BYTE_HASH=5
> +
> +# usage: corrupt_graph_and_verify <position> <data> <string>
> +# Manipulates the commit-graph file at the position
> +# by inserting the data, then runs 'git commit-graph verify'
> +# and places the output in the file 'err'. Test 'err' for
> +# the given string.

Very nice to have this description.

Show 8 quoted lines

> +corrupt_graph_and_verify() {
> +	pos=$1
> +	data="${2:-\0}"
> +	grepstr=$3
> +	cd "$TRASH_DIRECTORY/full" &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&

Using 'printf' with octal is much more portable than relying on 'echo' supporting octal escape sequences (or supporting escape sequences at all).

> +	test_must_fail git commit-graph verify 2>test_err &&
> +	grep -v "^+" test_err >err
> +	grep "$grepstr" err

Shouldn't this last 'grep' be 'test_i18ngrep' instead, to allow for translated messages from 'git commit-graph verify' / 'git fsck'?

> +}

This function makes actual tests short and simple, without duplicated code. Very good.

Show 13 quoted lines

> +
> +test_expect_success 'detect bad signature' '
> +	corrupt_graph_and_verify 0 "\0" \
> +		"graph signature"
> +'
> +
> +test_expect_success 'detect bad version' '
> +	corrupt_graph_and_verify $GRAPH_BYTE_VERSION "\02" \
> +		"graph version"
> +'
> +
> +test_expect_success 'detect bad hash version' '
> +	corrupt_graph_and_verify $GRAPH_BYTE_HASH "\02" \

When we move from SHA-1 (hash version 1) to NewHash (hash version 2), this test would start failing... which is actually not a bad idea.

> +		"hash version"
> +'
> +
>  test_done