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

From: Jeff King <peff@peff.net>
Date: Sep 5, 2019, 06:08 UTC
Message-ID: <20190905060838.GB21450@sigill.intra.peff.net>
In-Reply-To: <20190904212121.GB20904@syl.local>

On Wed, Sep 04, 2019 at 05:21:21PM -0400, Taylor Blau wrote:

Show 15 quoted lines

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

That rule matches my understanding. A BUG() should be about asserting invariants or catching should-not-happen cases, etc. Any time a BUG() triggers, that is truly a bug in Git, no matter what input got thrown at it, what syscalls failed, etc, and is worth fixing (even if the only sensible thing is to die()).

As a side note, we've generally treated segfaults the same way. It doesn't matter if the files on disk or the program input is garbage, we should say so and abort the operation cleanly.

-Peff