Re: [PATCH 02/23] parse_commit_buffer(): treat lookup_tree() failure as parse error

From: Jeff King <peff@peff.net>
Date: Oct 24, 2019, 23:22 UTC
Message-ID: <20191024232207.GA32602@sigill.intra.peff.net>
In-Reply-To: <20191024231220.96547-1-jonathantanmy@google.com>

On Thu, Oct 24, 2019 at 04:12:20PM -0700, Jonathan Tan wrote:

Show 17 quoted lines

> > And
> > certainly we _have_ seen real-world cases, such as the one fixed by
> > 806278dead (commit-graph.c: handle corrupt/missing trees, 2019-09-05).
> > 
> > Note that we can't quite drop the check in the caller added by that
> > commit yet, as there's some subtlety with repeated parsings (which will
> > be addressed in a future commit).
> 
> I tried figuring out what the subtlety is by undoing the check you
> describe, and did get a segfault in one of the tests, but couldn't
> figure out exactly what is going on. Looking at the code, is it because
> load_tree_for_commit() in commit-graph.c uses the return value of
> lookup_tree() indiscriminately (which is the issue that this patch
> fixes)?
> 
> This patch itself and patch 1 looks good (with the reasoning in the
> commit message), of course.

It's the issue addressed in patch 4. The issue is that some _other_ code already called parse_commit(), got an error, but then for whatever reason ignored it. Now when we call parse_commit(), the "parsed" flag is set, so it returns success even though the tree is still NULL.

That commit fixes the problem and does drop the extra tree check.

-Peff