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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 4, 2019, 21:21 UTC
Message-ID: <20190904212121.GB20904@syl.local>
In-Reply-To: <c5ba4eaa-f995-5f2e-4f0f-a8f59ba65fd3@gmail.com>

Hi Garima,

On Wed, Sep 04, 2019 at 02:25:55PM -0400, Garima Singh wrote:

Show 20 quoted lines

>
> 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.

Show 10 quoted lines

> >
> >    * 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.

> -Garima

Thanks, Taylor