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

[Jeff King <peff@peff.net>]

On Thu, Jan 24, 2013 at 07:59:58PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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.
> 
> I've been toying with an idea along this line.
> 
>  commit.h        | 16 ++++++++++++++++
>  builtin/blame.c | 27 ++++++++-------------------
>  commit.c        | 20 ++++++++++++++++++++
>  3 files changed, 44 insertions(+), 19 deletions(-)

I think we are on the same page as far as what needs to happen at the
call sites.

My suggested implementation had a separate buffer, but you are right
that we may need to actually set "commit->buffer" because sub-functions
expect to find it there (the alternative might be cleaning up the
sub-function interfaces). I haven't looked at the call-sites yet.

This:

> +extern int ensure_commit_buffer(struct commit *);
> +extern void discard_commit_buffer(struct commit *);
> +
> +#define with_commit_buffer(commit) \
> +	do { \
> +		int had_buffer_ = !!commit->buffer; \
> +		if (!had_buffer_) \
> +			ensure_commit_buffer(commit); \
> +		do
> +
> +#define done_with_commit_buffer(commit) \
> +		while (0); \
> +		if (!had_buffer_) \
> +			discard_commit_buffer(commit); \
> +	} while (0)

is pretty nasty, though. I know it gets the job done, but in my
experience, macros which do not behave syntactically like functions are
usually a good sign that you are doing something gross and
unmaintainable.

I dunno.

-Peff