From: Jeff King <peff@peff.net>
Date: Sat, 29 Sep 2012 05:30:13 GMT
Subject: Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
Message-ID: <20120929053013.GB3330@sigill.intra.peff.net>
In-Reply-To: <1348893689-20240-3-git-send-email-pclouds@gmail.com>

On Sat, Sep 29, 2012 at 11:41:28AM +0700, Nguyen Thai Ngoc Duy wrote:

> @@ -2210,10 +2213,23 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
>  
>  static int commit_match(struct commit *commit, struct rev_info *opt)
>  {
> +	int retval;
> +	struct strbuf buf = STRBUF_INIT;
>  	if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
>  		return 1;
> -	return grep_buffer(&opt->grep_filter,
> -			   commit->buffer, strlen(commit->buffer));
> +	if (opt->reflog_info) {
> +		strbuf_addstr(&buf, "reflog ");
> +		get_reflog_message(&buf, opt->reflog_info);
> +		strbuf_addch(&buf, '\n');
> +		strbuf_addstr(&buf, commit->buffer);
> +	}
> +	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;

I like how callers not doing a reflog walk do not have to pay the price
to do the extra allocating. We could further limit it to only when
--grep-reflog is in effect, but I guess that would mean wading through
grep_filter's patterns, since it could be buried amidst ANDs and ORs?

One alternative would be to set a bit in the grep_opt when we call
append_header_grep_pattern. It feels a bit like a layering violation,
though. I guess the bit could also go into rev_info. It may not even be
a measurable slowdown, though. Premature optimization and all that.

Other than that, I didn't notice anything wrong in the patch.

-Peff