Re: [PATCH] get_oid: handle NULL repo->index

From: Duy Nguyen <pclouds@gmail.com>
Date: May 15, 2019, 09:29 UTC
Message-ID: <CACsJy8BS8NR6aZR29VLYUrRjBE_oyzH=L6X7CSpCO9G+sPjcbA@mail.gmail.com>
In-Reply-To: <xmqqh89w70w8.fsf@gitster-ct.c.googlers.com>

On Wed, May 15, 2019 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

>
> Jeff King <peff@peff.net> writes:
>
> > Also from my earlier message, if you missed it:
> >
> >   I also wondered if we should simply allocate an empty index whenever
> >   we have a non-toplevel "struct repository", which might be less
> >   surprising to other callers. I don't have a strong opinion either way.
> >   I did grep around for other callers which might have similar problems,
> >   but couldn't find any.
>
> That is an approach to make it harder to make mistakes by accepting
> possibly a small wasted resource; but at that point, I think calling
> repo_read_index() unconditionally from here and similar places would
> be a simpler fix in the same spirit.

For repo_read_index() case, maybe. But we have a lot of "r(epo)->index->something". All or most of these references traditionally are the_index.something, which is always safe to dereference. Submodule repos with the optionally NULL repo->index break this assumption.

So either we audit all the code and make sure "repo->index" cannot be NULL because repo_read_index() has been called before (and may have to repeat auditing), or we just stick to the old assumption and make sure repo->index is not NULL from the beginning. This makes me think the small extra resource is worth it. Much less time will be spent on similar issues now and in the future.

PS. Sorry Jeff for the noise. I should have waited until I come home and can read your mail more carefully.

-- 
Duy