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

[Jeff King <peff@peff.net>]

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