Re: segfault for git log --graph --no-walk --grep a

From: Jeff King <peff@peff.net>
Date: Feb 9, 2013, 00:39 UTC
Message-ID: <20130209003921.GC5570@sigill.intra.peff.net>
In-Reply-To: <7vk3qixrc8.fsf@alter.siamese.dyndns.org>

On Fri, Feb 08, 2013 at 04:29:11PM -0800, Junio C Hamano wrote:

Show 9 quoted lines

> Perhaps something along this line...
> 
> -- >8 --
> Subject: "log --grep": commit's buffer may already have been discarded
> 
> Following up on be5c9fb9049e (logmsg_reencode: lazily load missing
> commit buffers, 2013-01-26), extract the part that reads the commit
> buffer data into a separate helper function, and use it when we
> apply the grep filter on the commit during the log walk.

This obviously makes sense if we don't want to get the route of re-encoding for grep. Re-encoding would be a user-visible change, but I wonder if it is the right thing to be doing.

Show 20 quoted lines

> diff --git a/revision.c b/revision.c
> index d7562ee..caf8ef3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
>  		strbuf_addch(&buf, '\n');
>  	}
>  
> -	/* Copy the commit to temporary if we are using "fake" headers */
> -	if (buf.len)
> +	if (!commit->buffer) {
> +		/* we may not have commit->buffer */
> +		unsigned long size;
> +		char *msg = read_commit_object_data(commit, &size);
> +		strbuf_add(&buf, msg, size);
> +		free(msg);
> +	} else if (buf.len) {
> +		/* Copy the commit to temporary if we are using "fake" headers */
>  		strbuf_addstr(&buf, commit->buffer);
> +	}

Hmm. It would be nice to avoid the extra copy when we do not otherwise need to use the strbuf. I would have expected something more like:

  const char *msg = commit->buffer;
  if (!msg)
          msg = read_commit_object_data(commit, NULL);

  [...]

  if (buf.len)
          retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
  else
          retval = grep_buffer(&opt->grep_filter, msg, strlen(msg));

  strbuf_release(&buf);
  if (msg != commit->buffer)
          free(msg);
  return retval;

You would also need to adjust the other uses of commit->buffer throughout the function to refer to msg.

-Peff