Junio C Hamano <gits...@pobox.com> writes: > ... > Is there even a single caller that is prepared to react to NULL? > > Answer. There is a single hit inside fsck.c that wants to report > an error without killing ourselves in fsck_commit_buffer(). I > however doubt its use of get_commit_tree() is correct in the > first place. The function is about validating the commit object > payload manually, without trusting the result of parse_commit(), > and it does read the object name of the tree object; the call to > get_commit_tree() used for reporting the error there should > probably become has_object() on the tree_oid.
At least we need to ensure, not just has_object(), but the object indeed claims to be a tree object. It is OK if it is a corrupt tree object---we'll catch its brokenness separately anyway. Hmm, the should we also tolerate the pointed object to be broken in a way that it is not even able to claim to be a tree object? That would mean that has_object() is sufficient to check here. OK, so... > So, after fixing the above, we may safely be able to die inside > get_commit_tree() instead of returning NULL. > > By the way, I think get_commit_tree() and parse_commit() in fsck > should always use the value obtained from the underlying object and > bypass any caches like commit graph---if they pay attention to the > caches, they should be fixed. Secondary caches like commit graph > should of course be validated against what are recorded in the > underlying object, but that should be done separately.