From: karthik nayak Date: Sun, 05 Apr 2015 07:52:50 GMT Subject: Re: [PATCH v7 1/4] sha1_file.c: support reading from a loose object of unknown type Message-ID: <5520E9D2.4010603@gmail.com> In-Reply-To: On 04/05/2015 01:16 PM, Junio C Hamano wrote: > karthik nayak 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.