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

[Derrick Stolee]

On 5/21/2018 2:53 PM, Jakub Narebski wrote:

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


Windows does not, but it is also missing many things this test suite 
needs. 'dd' is included in the Git for Windows SDK. I rebased this 
series onto Git for Windows and the tests passed when run in an SDK shell.


Thanks,
-Stolee

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

[Jakub Narebski]

Derrick Stolee  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 
> Signed-off-by: Derrick Stolee 
> ---
>  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   []
> +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 

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

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

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

I was a bit surprised at the "cd outside subshell", but then realized
that this file already does that. It will only be a problem if later
tests think they're somewhere else. Let's read on.

> +   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" &&
> +   test_must_fail git commit-graph verify 2>err &&
> +   grep -v "^\+" err > verify-errors &&
> +   test_line_count = 1 verify-errors &&
> +   grep "graph signature" verify-errors
> +'
> +
> +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" &&
> +   test_must_fail git commit-graph verify 2>err &&
> +   grep -v "^\+" err > verify-errors &&
> +   test_line_count = 1 verify-errors &&
> +   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" &&
> +   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
> +'

These look a bit boiler-platey. Maybe not too bad though.

> +test_expect_success 'detect too small chunk-count' '

s/too small/bad/?

To be honest, I wrote this title without thinking too hard about the
problem. In general, it would be quite hard for `git commit-graph
verify` to say "*this* is wrong in your file" ("the number of chunks is
too small") -- it should be much easier to say "*something* is wrong".

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

Maybe these tests could go with the previous patch(es). IMVHO I would
prefer reading the test with the implementation. A separate commit for
the tests might make sense if they're really tricky and need some
explaining, but I don't think that's the case here.

All of these comments are just minor nits, or not even that. I will
continue with the other patches at another time.

Thank you, I'm really looking forward to Git with commit-graph magic.

Martin

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

[Derrick Stolee]

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 
Signed-off-by: Derrick Stolee 
---
 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   []
+corrupt_data() {
+   file=$1
+   pos=$2
+   data="${3:-\0}"
+   printf "$data" | dd of="$file" bs=1 seek="$pos" conv=notrunc
+}
+
+test_expect_success 'detect bad signature' '
+   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 0 "\0" &&
+   test_must_fail git commit-graph verify 2>err &&
+   grep -v "^\+" err > verify-errors &&
+   test_line_count = 1 verify-errors &&
+   grep "graph signature" verify-errors
+'
+
+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" &&
+   test_must_fail git commit-graph verify 2>err &&
+   grep -v "^\+" err > verify-errors &&
+   test_line_count = 1 verify-errors &&
+   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" &&
+   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
+'
+
+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
+'
+
 test_done
-- 
2.16.2.329.gfb62395de6