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

["Martin Ågren" <martin.agren@gmail.com>]

On 11 May 2018 at 23:15, Derrick Stolee <dstolee@microsoft.com> 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