Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2019, 17:11 UTC
Message-ID: <xmqq5zm5nydu.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqqo8zxnz0m.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

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

Show 9 quoted lines

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