Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 29, 2012, 05:54 UTC
- Message-ID
- <7vpq55idbe.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20120929053013.GB3330@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 23 quoted lines
>> + 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.I do not think it is a layering violation. compile_grep_exp() should be aware of the short-cut possibilities and your "our expression is interested in reflog so we need to read it" is very similar in spirit to the existing opt->extended bit.
It will obviously allow us to avoid reading reflog information unnecessarily here. I think it makes perfect sense.
We may also want to flag the use of the --grep-reflog option when the --walk-reflogs option is not in effect in setup_revisions() as an error, or something.