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

[Derrick Stolee <stolee@gmail.com>]

On 5/28/2018 10:05 AM, Jakub Narebski wrote:
> Derrick Stolee <dstolee@microsoft.com> writes:
>
>> 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
>
>> 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.
>
>> 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.

This gives us a constant commit-graph file to work with in the later tests.

To get the "independent test" structure you want for the tests that are 
coming, we need to do one of the following:

1. Write a new commit-graph file for every test (slows things down).
2. Do all corruption/verify checks in a single test (reduces the 
information from a failed test, as it only reports the first failure).

I don't like either of these options, so I went with this "prepare" step.

> 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.
>
>>   
>> +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.
>
>> +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.
>
>> +
>> +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