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

From: Martin Ågren <martin.agren@gmail.com>
Date: May 12, 2018, 13:43 UTC
Message-ID: <CAN0heSqYOY4HyinuL7ye0XL3u+rUxWeT9KwsiXWCcsVN_hmc2Q@mail.gmail.com>
In-Reply-To: <20180511211504.79877-4-dstolee@microsoft.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.

Show 30 quoted lines

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

Show 9 quoted lines

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