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

From

Junio C Hamano <gitster@pobox.com>

Date

Feb 9, 2013, 00:47 UTC

Message-ID

<7va9rexqii.fsf@alter.siamese.dyndns.org>

In-Reply-To

<7vfw16xqvj.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> 
>>> > Thomas Haller <thom311@gmail.com> writes:
>>> >
>>> >> it happens in file revision.c:2306 because "commit->buffer" is zero:
>>> >>
>>> >>                 retval = grep_buffer(&opt->grep_filter,
>>> >>                                      commit->buffer, strlen(commit->buffer));
>>> >
>>> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily
>>> > load missing commit buffers, 2013-01-26); I haven't merged it to any
>>> > of the maintenance releases, but the tip of 'master' should have it
>>> > already.
>>> 
>>> Ah, no, this shares the same roots as the breakage the said commit
>>> fixed, and the solution should use the same idea, but not fixed.
>>
>> Yeah, I think this needs separate treatment. However, this is a perfect
>> example of the "case-by-case" I mention in the final two paragraphs
>> there.
>>
>> What's the right encoding to be grepping in? The original, what we will
>> output in, or even something else? IOW, I wonder if this should be using
>> logmsg_reencode in the first place (the obvious reason not to want to do
>> so is speed, but logmsg_reencode is written to only have an impact in
>> the case that we do indeed need to reencode).
>
> Yeah, that actually is a good point.  We should be using logmsg_reencode
> so that we look for strings in the user's encoding.

 revision.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/revision.c b/revision.c
index d7562ee..07bf728 100644
--- a/revision.c
+++ b/revision.c
@@ -2269,6 +2269,9 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 {
 	int retval;
 	struct strbuf buf = STRBUF_INIT;
+	char *message;
+	const char *encoding;
+
 	if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
 		return 1;
 
@@ -2279,32 +2282,24 @@ 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)
-		strbuf_addstr(&buf, commit->buffer);
+	encoding = get_log_output_encoding();
+	message = logmsg_reencode(commit, encoding);
+	strbuf_addstr(&buf, message);
+	if (message != commit->buffer)
+		free(message);
 
 	if (opt->grep_filter.header_list && opt->mailmap) {
-		if (!buf.len)
-			strbuf_addstr(&buf, commit->buffer);
-
 		commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
 		commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
 	}
 
 	/* Append "fake" message parts as needed */
-	if (opt->show_notes) {
-		if (!buf.len)
-			strbuf_addstr(&buf, commit->buffer);
+	if (opt->show_notes)
 		format_display_notes(commit->object.sha1, &buf,
-				     get_log_output_encoding(), 1);
-	}
+				     encoding, 1);
+
+	retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
 
-	/* Find either in the commit object, or in the temporary */
-	if (buf.len)
-		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
-	else
-		retval = grep_buffer(&opt->grep_filter,
-				     commit->buffer, strlen(commit->buffer));
 	strbuf_release(&buf);
 	return retval;
 }