On Thu, May 10, 2018 at 12:05 PM, Martin Ågren <martin.agren@xxxxxxxxx> wrote: > 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. Given that the commit graph is secondary data, the users work around to quickly get back to a well working repository is most likely to remove the file and regenerate it. As a developer who wants to fix the bug, a stacktrace/datadump and the history of git commands might be most valuable, but I agree we should hide that behind a debug flag. Packfiles and loose objects are primary data, which means that those need a more advanced way to diagnose and repair them, so I would imagine the commit graph fsck is closer to bitmaps fsck, which I would have suspected to be found in t5310, but a quick read doesn't reveal many tests that are checking for integrity. So I guess the test coverage here is ok, (although we should always ask for more) Thanks, Stefan
