On 10 May 2018 at 19:34, Derrick Stolee <dstolee@xxxxxxxxxxxxx> wrote: > Commits 01-07 focus on the 'git commit-graph verify' subcommand. These > are ready for full, rigorous review. I don't know about "full" and "rigorous", but I tried to offer my thoughts. A lingering feeling I have is that users could possibly benefit from seeing "the commit-graph has a bad foo" a bit more than just "the commit-graph is bad". But adding "the bar is baz, should have been frotz" might not bring that much. Maybe you could keep the translatable string somewhat simple, then, if the more technical data could be useful to Git developers, dump it in a non-translated format. (I guess it could be hidden behind a debug switch, but let's take one step at a time..) This is nothing I feel strongly about. > t/t5318-commit-graph.sh | 25 +++++ I wonder about tests. Some tests seem to use `dd` to corrupt files and check that it gets caught. Doing this in a a hash-agnostic way could be tricky, but maybe not impossible. I guess we could do something probabilistic, like "set the first two bytes of the very last OID to zero -- surely all OIDs can't start with 16 zero bits". Hmm, that might still require knowing the size of the OIDs... I hope to find time to do some more hands-on testing of this to see that errors actually do get caught. I saw you redirect stdout to a file "output", and anticipated later commits to actually look into it. I never saw that though. (I did not apply the patches, so I could have missed something.) Martin
