Re: [PATCH v7 1/4] sha1_file.c: support reading from a loose object of unknown type

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 5, 2015, 07:46 UTC
Message-ID: <xmqqiodbdnkn.fsf@gitster.dls.corp.google.com>
In-Reply-To: <55204141.9070100@gmail.com>

karthik nayak <karthik.188@gmail.com> writes:

Show 12 quoted lines

>> So, it makes me wonder what guarantee we have that this does not
>> dereference a NULL here.
>>
> As per my code, oi->typename is only pointing to something when oi->typep
> is ( As oi->typename is currently only used in cat-file.c).
> But what you're saying also is true, there is no other guarantee, as a user may
> set oi->typename to point to a struct strbuf and leave out oi->typep.
>
>  if (oi->typename && oi->typep)
>          strbuf_addstr(oi->typename, typename(*oi->typep));
>
> This should suffice. Do you want me to re-roll this?

I'd rather avoid the thinking along the lines of "at this moment, there happens to be only one caller that asks for typename and the caller also sets typep, so we will be safe as long as we make sure the caller passed typep before giving him typename back".

Somebody else may write new code that wants to learn the typename, forgets to set typep, calls into this codepath, and ends up scratching his head wondering why the typename string is returned to him. Surely the code may not crash at the new code you wrote, but you are not helping him.

If it semantically does not make sense to ask for the typename without asking for the type code, then we can and should make that as a new calling convention _all_ callers must follow.

In other words, I think it is better to have

	if (oi->typename && !oi->typep)
		die("BUG");

somewhere near the beginning of the callchain that takes oi; that will ensure all callers understand the rule.