Re: segmentation fault (nullpointer) with git log --submodule -p

> On Thu, Jan 24, 2013 at 03:56:23PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > ... (e.g., how should "log" know that a submodule diff might later want
>> > to see the same entry? Should we optimistically free and then make it
>> > easier for the later user to reliably ensure the buffer is primed? Or
>> > should we err on the side of keeping it in place?).
>>
>> My knee-jerk reaction is that we should consider that commit->buffer
>> belongs to the revision traversal machinery.  Any other uses bolted
>> on later can borrow it if buffer still exists (I do not think pretty
>> code rewrites the buffer contents in place in any way), or they can
>> ask read_sha1_file() to read it themselves and free when they are
>> done.
>
> Yeah, that is probably the sanest way forward. It at least leaves
> ownership in one place, and everybody else is an opportunistic consumer.
> We do need to annotate each user site though with something like the
> "ensure" function I mentioned.
>
> If they are not owners, then the better pattern is probably something
> like:

>
>   /*
>    * Get the commit buffer, either opportunistically using
>    * the cached version attached to the commit object, or loading it
>    * from disk if necessary.
>    */
>   const char *use_commit_buffer(struct commit *c)
>   {
>           enum object_type type;
>           unsigned long size;
>
>           if (c->buffer)
>                   return c->buffer;
>           /*
>            * XXX check type == OBJ_COMMIT?
>            * XXX die() on NULL as a convenience to callers?
>            */
>           return read_sha1_file(c->object.sha1, &type, &size);
>   }
>
>   void unuse_commit_buffer(const char *buf, struct commit *c)
>   {
>           /*
>            * If we used the cached copy attached to the commit,
>            * we don't want to free it; it's not our responsibility.
>            */
>           if (buf == c->buffer)
>                   return;
>
>           free((char *)buf);
>   }
>
> I suspect that putting a use/unuse pair inside format_commit_message
> would handle most cases.
>
> -Peff