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

[karthik nayak <karthik.188@gmail.com>]

On 04/05/2015 01:16 PM, Junio C Hamano wrote:
> karthik nayak <karthik.188@gmail.com> writes:
>
>>> 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.


Yes! I do agree with you, If you read my previous reply, I did mention 
what you said about not having a guarantee that typep and typename are 
both set and another user might have a bug with this.

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

Yes! this is a better approach as it will enforce that typep must be set 
when typename is set.