>>> +/*
>>> + * For performance reasons, a commit loaded from the graph does not
>>> + * have a tree loaded until trying to consume it for the first time.
>>
>> That is the theme of this series/patch, but do we need to write it down
>> into the codebase? I'd be inclined to omit this part and only go with:
>>
>> Load the root tree of a commit and return the tree.
>
>
> OK.
This was just a suggestion, others may want to chime in on the terseness.
>
>>
>>> struct tree *get_commit_tree(const struct commit *commit)
>>> {
>>> - return commit->tree;
>>> + if (commit->tree || !commit->object.parsed)
>>
>> I understand to return the tree from the commit
>> when we have the tree in the commit object (the first
>> part).
>>
>> But 'when we have not (yet) parsed the commit object',
>> we also just return its tree? Could you explain the
>> second part of the condition?
>> Is that for commits that are not part of the commit graph?
>> (But then why does it need to be negated?)
>
>
> Some callers check the value of 'commit->tree' without a guarantee that the
> commit was parsed. In this case, the way to preserve the existing behavior
> is to continue returning NULL. If I remove the "|| !commit->object.parsed"
> then the BUG("commit has NULL tree, but was not loaded from commit-graph")
Oh. That totally makes sense now. I should have more coffee (got up extra
early to see a dentist before going into work)
> is hit in these two tests:
>
> t6012-rev-list-simplify.sh
> t6110-rev-list-sparse.sh
>
> I prefer to keep the BUG() statement and instead use this if statement. If
> someone has more clarity on why this is a good existing behavior, then
> please chime in.
>
I would also keep the BUG statement; I am unsure if we'd want to
have a comment explaining the situation, or if it is obvious enough
and I was just not focused enough.
This totally makes sense now and I'd keep the code as is.
Thanks,
Stefan
