Re: [RFC PATCH 0/1] commit-graph.c: handle corrupt commit trees

> Hi Garima,
> 
> On Wed, Sep 04, 2019 at 02:25:55PM -0400, Garima Singh wrote:
>>
>> On 9/3/2019 10:22 PM, Taylor Blau wrote:
>>> Hi,
>>>
>>> I was running some of the new 'git commit-graph' commands, and noticed
>>> that I could consistently get 'git commit-graph write --reachable' to
>>> segfault when a commit's root tree is corrupt.
>>>
>>> I have an extremely-unfinished fix attached as an RFC PATCH below, but I
>>> wanted to get a few thoughts on this before sending it out as a non-RFC.
>>>
>>> In my patch, I simply 'die()' when a commit isn't able to be parsed
>>> (i.e., when 'parse_commit_no_graph' returns a non-zero code), but I
>>> wanted to see if others thought that this was an OK approach. Some
>>> thoughts:
>>
>> I like the idea of completely bailing if the commit can't be parsed too.
>> Only question: Is there a reason you chose to die() instead of BUG() like
>> the other two places in that function? What is the criteria of choosing one
>> over the other?
> 
> I did not call 'BUG' here because 'BUG' is traditionally used to
> indicate an internal bug, e.g., an unexpected state or some such. On the
> other side of that coin, 'BUG' is _not_ used to indicate repository
> corruption, since that is not an issue in the Git codebase, rather in
> the user's repository.
> 
> Though, to be honest, I've never seen that rule written out explicitly
> (maybe if it were to be written somewhere, it could be stored in
> Documentation/CodingGuidelines?). I think that this is some good
> #leftoverbits material.
> 
>>>
>>>    * It seems like we could write a commit-graph by placing a "filler"
>>>      entry where the broken commit would have gone. I don't see any place
>>>      where this is implemented currently, but this seems like a viable
>>>      alternative to not writing _any_ commits into the commit-graph.
>>
>> I would rather we didn't do this cause it will probably kick open the can of
>> always watching for that filler when we are working with the commit-graph.
>> Or do we already do that today? Maybe @stolee can chime in on what we do in
>> cases of shallow clones and other potential gaps in the walk
> 
> Yeah, I think that the consensus is that it makes sense to just die
> here, which is fine by me.