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

[Junio C Hamano <gitster@pobox.com>]

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.

Perhaps like this.  Just like the previous one (which should be
discarded), this makes the function always use the temporary strbuf,
so doing this upfront actually loses more code than it adds ;-)

 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;
 }