On Fri, Sep 28, 2012 at 10:54:29PM -0700, Junio C Hamano wrote:

> Jeff King <p...@peff.net> writes:
> >> +  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.

Hmm. Yeah, I guess so. I was thinking that the grep code did not know
there was a commit or reflog involved at all (we just pass it a buffer,
and how we prepare it is our business), but it does already know about
the magic GREP_HEADER_* variables, and this is definitely part of that.

> 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.

Good point. I think the docs in the patch just say it is ignored unless
walking, but it would be better to flag the error.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to