From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
Date: Sat, 29 Sep 2012 02:16:41 -0400 [thread overview]
Message-ID: <20120929061641.GA4425@sigill.intra.peff.net> (raw)
In-Reply-To: <7vpq55idbe.fsf@alter.siamese.dyndns.org>
On Fri, Sep 28, 2012 at 10:54:29PM -0700, Junio C Hamano wrote:
> Jeff King <peff@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.
-Peff
next prev parent reply other threads:[~2012-09-29 6:16 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-26 12:12 [PATCH] revision: add --reflog-message=<pattern> to grep reflog messages Nguyễn Thái Ngọc Duy
2012-09-26 14:07 ` Junio C Hamano
2012-09-26 14:15 ` Nguyen Thai Ngoc Duy
2012-09-26 19:28 ` Junio C Hamano
2012-09-27 11:36 ` [PATCH] revision: add --reflog-message " Nguyễn Thái Ngọc Duy
2012-09-27 17:09 ` Junio C Hamano
2012-09-27 17:28 ` Jeff King
2012-09-27 18:16 ` Junio C Hamano
2012-09-28 7:01 ` [PATCH 1/3] grep: generalize header grep code to accept arbitrary headers Nguyễn Thái Ngọc Duy
2012-09-28 7:01 ` [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages Nguyễn Thái Ngọc Duy
2012-09-28 17:55 ` Junio C Hamano
2012-09-29 4:41 ` Nguyễn Thái Ngọc Duy
2012-09-29 4:41 ` [PATCH 1/3] grep: prepare for new header field filter Nguyễn Thái Ngọc Duy
2012-09-29 5:22 ` Jeff King
2012-09-29 4:41 ` [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages Nguyễn Thái Ngọc Duy
2012-09-29 5:30 ` Jeff King
2012-09-29 5:54 ` Junio C Hamano
2012-09-29 6:13 ` Nguyen Thai Ngoc Duy
2012-09-29 19:11 ` Junio C Hamano
2012-09-30 3:45 ` Nguyen Thai Ngoc Duy
2012-09-29 6:16 ` Jeff King [this message]
2012-09-29 4:41 ` [PATCH 3/3] revision: make --grep search in notes too if shown Nguyễn Thái Ngọc Duy
2012-09-29 5:35 ` [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages Junio C Hamano
2012-09-28 7:01 ` [PATCH 3/3] revision: make --grep search in notes too if shown Nguyễn Thái Ngọc Duy
2012-09-28 17:55 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120929061641.GA4425@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).