On Thu, Oct 24, 2019 at 04:12:20PM -0700, Jonathan Tan wrote: > > 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
