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:47:01PM -0800, Junio C Hamano wrote:
>>
>>> > 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 ;-)
>>
>> I didn't see this in What's Cooking or pu. We should probably pick an
>> approach and go with it.
>>
>> I think using logmsg_reencode makes sense. I'd be in favor of avoiding
>> the extra copy in the common case, so something like the patch below. If
>> you feel strongly about the code cleanup at the minor run-time expense,
>> I won't argue too much, though.
>
> Sounds good to me.  Care to do the log message while at it?

Heh, how about this?  I still need a sign-off from you.

Thanks.

commit 6a76814cd08cac15c1faff5bd97c9e94ac8b6a01
Author: Jeff King <peff@peff.net>
Date:   Mon Feb 11 14:16:07 2013 -0500

    log --grep: look for the given string in log output encoding
    
    We used to grep in the raw commit buffer contents, possibly pieces
    of notes encoded in log output encoding appended to it, which was
    insane.
    
    Convert the contents of the commit message also to log output
    encoding before looking for the string.  This incidentally fixes a
    possible NULL dereference that can happen when commit->buffer has
    already been freed, which can happen with
    
    	git commit -m 'text1' --allow-empty
    	git commit -m 'text2' --allow-empty
    	git log --graph --no-walk --grep 'text2'
    
    which arguably does not make any sense (--graph inherently wants a
    connected history, and by --no-walk the command line is telling us
    to show discrete points in history without connectivity), and we
    probably should forbid the combination, but that is a separate
    issue.