Re: [PATCH 00/12] Integrate commit-graph into fsck, gc, and fetch

> On Thu, May 10, 2018 at 12:05 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>> On 10 May 2018 at 19:34, Derrick Stolee <dstolee@microsoft.com> 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)