On 11 May 2018 at 23:15, Derrick Stolee [off-list ref] 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