[PATCH] revision: add --reflog-message=<pattern> to grep reflog messages

[PATCH] revision: add --reflog-message=<pattern> to grep reflog messages

[Nguyễn Thái Ngọc Duy]

Both "git log" and "git reflog show" recognize this option.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Itch: how to show reflogs for checkout operation only?

 Instead of ignoring when -g is not given, we might want to imply -g.

 Still itch: grep highlight! For all applicable areas: commit headers
 including reflog messages, commit body, diff.

 Documentation/rev-list-options.txt |  5 +++++
 revision.c                         | 30 ++++++++++++++++++++++++++++++
 revision.h                         |  1 +
 3 files changed, 36 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1fc2a18..aeaa58c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -51,6 +51,11 @@ endif::git-rev-list[]
 	commits whose author matches any of the given patterns are
 	chosen (similarly for multiple `--committer=<pattern>`).
 
+--reflog-message=<pattern>::
+	Limit the commits output to ones with reflog messages that
+	match the specified pattern (regular expression). Ignored unless
+	--walk-reflogs is given.
+
 --grep=<pattern>::
 
 	Limit the commits output to ones with log message that
diff --git a/revision.c b/revision.c
index ae12e11..ee55bb2 100644
--- a/revision.c
+++ b/revision.c
@@ -1053,6 +1053,11 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->grep_filter.header_tail = &(revs->grep_filter.header_list);
 	revs->grep_filter.regflags = REG_NEWLINE;
 
+	revs->reflog_filter.status_only = 1;
+	revs->reflog_filter.pattern_tail = &(revs->reflog_filter.pattern_list);
+	revs->reflog_filter.header_tail = &(revs->reflog_filter.header_list);
+	revs->reflog_filter.regflags = REG_NEWLINE;
+
 	diff_setup(&revs->diffopt);
 	if (prefix && !revs->diffopt.prefix) {
 		revs->diffopt.prefix = prefix;
@@ -1298,6 +1303,12 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 	add_grep(revs, pattern, GREP_PATTERN_BODY);
 }
 
+static void add_reflog_grep(struct rev_info *revs, const char *ptn)
+{
+	append_grep_pattern(&revs->reflog_filter, ptn,
+			    "command line", 0, GREP_PATTERN);
+}
+
 static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
 			       int *unkc, const char **unkv)
 {
@@ -1600,15 +1611,23 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		return argcount;
 	} else if (!strcmp(arg, "--grep-debug")) {
 		revs->grep_filter.debug = 1;
+	} else if ((argcount = parse_long_opt("reflog-message",
+					      argv, &optarg))) {
+		add_reflog_grep(revs, optarg);
+		return argcount;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
 		revs->grep_filter.regflags |= REG_EXTENDED;
+		revs->reflog_filter.regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
 		revs->grep_filter.regflags |= REG_ICASE;
+		revs->reflog_filter.regflags |= REG_ICASE;
 		DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
 	} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
 		revs->grep_filter.fixed = 1;
+		revs->reflog_filter.fixed = 1;
 	} else if (!strcmp(arg, "--all-match")) {
 		revs->grep_filter.all_match = 1;
+		revs->reflog_filter.all_match = 1;
 	} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
 		if (strcmp(optarg, "none"))
 			git_log_output_encoding = xstrdup(optarg);
@@ -1891,6 +1910,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	diff_setup_done(&revs->diffopt);
 
 	compile_grep_patterns(&revs->grep_filter);
+	compile_grep_patterns(&revs->reflog_filter);
 
 	if (revs->reverse && revs->reflog_info)
 		die("cannot combine --reverse with --walk-reflogs");
@@ -2242,6 +2262,16 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
 		    ((revs->max_parents >= 0) && (n > revs->max_parents)))
 			return commit_ignore;
 	}
+	if (revs->reflog_info &&
+	    revs->reflog_filter.pattern_list) {
+		struct strbuf sb = STRBUF_INIT;
+		int ignore;
+		get_reflog_message(&sb, revs->reflog_info);
+		ignore = !grep_buffer(&revs->reflog_filter, sb.buf, sb.len);
+		strbuf_release(&sb);
+		if (ignore)
+			return commit_ignore;
+	}
 	if (!commit_match(commit, revs))
 		return commit_ignore;
 	if (revs->prune && revs->dense) {
diff --git a/revision.h b/revision.h
index a95bd0b..0ebe34b 100644
--- a/revision.h
+++ b/revision.h
@@ -145,6 +145,7 @@ struct rev_info {
 
 	/* Filter by commit log message */
 	struct grep_opt	grep_filter;
+	struct grep_opt	reflog_filter;
 
 	/* Display history graph */
 	struct git_graph *graph;
-- 
1.7.12.1.406.g6ab07c4

Re: [PATCH] revision: add --reflog-message=<pattern> to grep reflog messages

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Both "git log" and "git reflog show" recognize this option.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

How well does it interact with --grep and/or --all-match?

Re: [PATCH] revision: add --reflog-message=<pattern> to grep reflog messages

[Nguyen Thai Ngoc Duy]

On Wed, Sep 26, 2012 at 9:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Both "git log" and "git reflog show" recognize this option.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>
> How well does it interact with --grep and/or --all-match?

Good point. It currently works like and operator. But people might
expect to combine them in different ways.
-- 
Duy

Re: [PATCH] revision: add --reflog-message=<pattern> to grep reflog messages

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Wed, Sep 26, 2012 at 9:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Both "git log" and "git reflog show" recognize this option.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>
>> How well does it interact with --grep and/or --all-match?
>
> Good point. It currently works like and operator. But people might
> expect to combine them in different ways.

The current commit_match() runs grep_buffer() on commit->buffer.  It
probably makes sense to instead notice from opt that we are running
log with "-g", prepare a temporary strbuf and add in the reflog
message to the string in commit->buffer, and run grep_buffer() on
that temporary strbuf on it.

I personally think it is sufficient ot just reuse --grep on
concatenation of commit->buffer with "Reflog message: checkout:
moving from as/check-ignore to pu".

If you really want to go fancier, you could add --grep-reflog option
that behaves like the existing --author and --committer options to
add "header match" elements to the grep expression, splice a fake
"reflog " header to the string copied from commit->buffer, e.g.
prepare something like this in your temporary strbuf:

    tree b4429f218782165faf101ccb0f4ba1cdd6d1d349
    parent de5cd03876e546d6d264ab28a01daa978f3eae78
    parent b378e5a25658e07e6d0c0f4db79e87cb21de5489
    author Junio C Hamano <gitster@pobox.com> 1348616180 -0700
    committer Junio C Hamano <gitster@pobox.com> 1348616180 -0700
    reflog checkout: moving from as/check-ignore to pu

    Merge branch 'jk/lua-hackery' into pu

    * jk/lua-hackery:
      Minimum compilation fixup
      Makefile: make "lua" a bit more configurable
      add a "lua" pretty format
      add basic lua infrastructure
      pretty: make some commit-parsing helpers more public

that way, you can take advantage of the existing logic used for the
author/committer match that matches only in the commit object
header.

Again, I personally doubt the fancier option is worth it, but the
starting point may look something like this.

 revision.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git c/revision.c w/revision.c
index ae12e11..b0f4d5b 100644
--- c/revision.c
+++ w/revision.c
@@ -2212,8 +2212,20 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 {
 	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) {
+		int retval;
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "reflog %s\n", opt->reflog_info->message);
+		strbuf_addstr(&buf, commit->buffer);
+		retval = grep_buffer(&opt->grep_filter,
+				     buf.buf, buf.len);
+		strbuf_release(&buf);
+		return retval;
+	} else {
+		return grep_buffer(&opt->grep_filter,
+				   commit->buffer, strlen(commit->buffer));
+	}
 }
 
 static inline int want_ancestry(struct rev_info *revs)

[PATCH] revision: add --reflog-message to grep reflog messages

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Thu, Sep 27, 2012 at 2:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
 > The current commit_match() runs grep_buffer() on commit->buffer.  It
 > probably makes sense to instead notice from opt that we are running
 > log with "-g", prepare a temporary strbuf and add in the reflog
 > message to the string in commit->buffer, and run grep_buffer() on
 > that temporary strbuf on it.

 Yeah. I was starting to think that way too, otherwise combining grep
 options would be a mess. I was hoping by injecting a fake reflog
 header, we could simplify reflog exceptions in pretty.c. But it
 was not that easy.

 > I personally think it is sufficient ot just reuse --grep on
 > concatenation of commit->buffer with "Reflog message: checkout:
 > moving from as/check-ignore to pu".

 --grep only reads the commit body. So either we append reflog
 message to commit body, or we put it in the header and add a new
 option for it. I don't like appending things to the commit body as
 --grep may hit reflog message while users do not mean so.

 > If you really want to go fancier, you could add --grep-reflog option
 > that behaves like the existing --author and --committer options to
 > add "header match" elements to the grep expression, splice a fake
 > "reflog " header to the string copied from commit->buffer

 Inserting at the beginning of the commit like your demo patch works
 just fine and is simpler. I'm tempted to take the undocumented option
 name --reflog for this purpose. But it's probably not worth the risk.

 Documentation/rev-list-options.txt |  5 +++++
 grep.c                             |  1 +
 grep.h                             |  5 +++--
 revision.c                         | 19 +++++++++++++++++--
 t/t7810-grep.sh                    |  6 ++++++
 5 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1fc2a18..aeaa58c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -51,6 +51,11 @@ endif::git-rev-list[]
 	commits whose author matches any of the given patterns are
 	chosen (similarly for multiple `--committer=<pattern>`).
 
+--reflog-message=<pattern>::
+	Limit the commits output to ones with reflog entries that
+	match the specified pattern (regular expression). Ignored unless
+	--walk-reflogs is given.
+
 --grep=<pattern>::
 
 	Limit the commits output to ones with log message that
diff --git a/grep.c b/grep.c
index 898be6e..72ac1bf 100644
--- a/grep.c
+++ b/grep.c
@@ -697,6 +697,7 @@ static struct {
 } header_field[] = {
 	{ "author ", 7 },
 	{ "committer ", 10 },
+	{ "reflog ", 7 },
 };
 
 static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
diff --git a/grep.h b/grep.h
index 8a28a67..1416ad7 100644
--- a/grep.h
+++ b/grep.h
@@ -29,9 +29,10 @@ enum grep_context {
 
 enum grep_header_field {
 	GREP_HEADER_AUTHOR = 0,
-	GREP_HEADER_COMMITTER
+	GREP_HEADER_COMMITTER,
+	GREP_HEADER_REFLOG,
+	GREP_HEADER_FIELD_MAX
 };
-#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1)
 
 struct grep_pat {
 	struct grep_pat *next;
diff --git a/revision.c b/revision.c
index ae12e11..837051c 100644
--- a/revision.c
+++ b/revision.c
@@ -1595,6 +1595,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((argcount = parse_long_opt("committer", argv, &optarg))) {
 		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
 		return argcount;
+	} else if ((argcount = parse_long_opt("reflog-message", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_REFLOG, optarg);
+		return argcount;
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
 		return argcount;
@@ -2212,8 +2215,20 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 {
 	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) {
+		int retval;
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addstr(&buf, "reflog ");
+		get_reflog_message(&buf, opt->reflog_info);
+		strbuf_addch(&buf, '\n');
+		strbuf_addstr(&buf, commit->buffer);
+		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
+		strbuf_release(&buf);
+		return retval;
+	} else {
+		return grep_buffer(&opt->grep_filter,
+				   commit->buffer, strlen(commit->buffer));
+	}
 }
 
 static inline int want_ancestry(struct rev_info *revs)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 91db352..a0f519e 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -546,6 +546,12 @@ test_expect_success 'log grep (6)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log grep (7)' '
+	git log -g --reflog-message="commit: third" --pretty=tformat:%s >actual &&
+	echo third >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log with multiple --grep uses union' '
 	git log --grep=i --grep=r --format=%s >actual &&
 	{
-- 
1.7.12.1.406.g6ab07c4

Re: [PATCH] revision: add --reflog-message to grep reflog messages

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Plase explain yourself in the space above.

> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 1fc2a18..aeaa58c 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -51,6 +51,11 @@ endif::git-rev-list[]
>  	commits whose author matches any of the given patterns are
>  	chosen (similarly for multiple `--committer=<pattern>`).
>  
> +--reflog-message=<pattern>::
> +	Limit the commits output to ones with reflog entries that
> +	match the specified pattern (regular expression). Ignored unless
> +	--walk-reflogs is given.
> +

I am debating myself if it is sane for this option to have no hint
that it is about "limiting" in its name.  "--author/--committer"
don't and it is clear from the context of the command that they are
not about setting author/committer, so "--reflog-message" may be
interpreted the same, perhaps.

The entry in the context above talks about multiple occurrence of
that option. Shouldn't this new one also say what happens when it is
given twice?

> diff --git a/grep.c b/grep.c
> index 898be6e..72ac1bf 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -697,6 +697,7 @@ static struct {
>  } header_field[] = {
>  	{ "author ", 7 },
>  	{ "committer ", 10 },
> +	{ "reflog ", 7 },
>  };
>  
>  static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
> diff --git a/grep.h b/grep.h
> index 8a28a67..1416ad7 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -29,9 +29,10 @@ enum grep_context {
>  
>  enum grep_header_field {
>  	GREP_HEADER_AUTHOR = 0,
> -	GREP_HEADER_COMMITTER
> +	GREP_HEADER_COMMITTER,
> +	GREP_HEADER_REFLOG,
> +	GREP_HEADER_FIELD_MAX
>  };
> -#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1)

Please add comment to ensure that FIELD_MAX stays at the end; if you
ensure that, the result is much better than the original "we know
committer is at the end so add one".

I think I wrote prep_header_patterns() and compile_grep_patterns()
carefully enough not to assume the headers are only the author and
committer names, so the various combinations i.e. all-match,
author(s), committer(s), grep(s), and reflog-message(s), should work
out of the box, but have you actually tested them?

I do not know offhand the matching side is prepared to take random
garbage fields.  IIRC, we strip the trailing timestamp from committer
and author header lines when we match, and a new code needs to be
added to control when that stripping should / should not kick in
depending on the header.

> diff --git a/revision.c b/revision.c
> index ae12e11..837051c 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1595,6 +1595,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if ((argcount = parse_long_opt("committer", argv, &optarg))) {
>  		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
>  		return argcount;
> +	} else if ((argcount = parse_long_opt("reflog-message", argv, &optarg))) {
> +		add_header_grep(revs, GREP_HEADER_REFLOG, optarg);
> +		return argcount;
>  	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
>  		add_message_grep(revs, optarg);
>  		return argcount;
> @@ -2212,8 +2215,20 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
>  {
>  	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) {
> +		int retval;
> +		struct strbuf buf = STRBUF_INIT;
> +		strbuf_addstr(&buf, "reflog ");
> +		get_reflog_message(&buf, opt->reflog_info);
> +		strbuf_addch(&buf, '\n');
> +		strbuf_addstr(&buf, commit->buffer);
> +		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
> +		strbuf_release(&buf);
> +		return retval;
> +	} else {
> +		return grep_buffer(&opt->grep_filter,
> +				   commit->buffer, strlen(commit->buffer));
> +	}
>  }

This part looks familiar and smells sane ;-)

Re: [PATCH] revision: add --reflog-message to grep reflog messages

[Jeff King]

On Thu, Sep 27, 2012 at 10:09:28AM -0700, Junio C Hamano wrote:

> > +--reflog-message=<pattern>::
> > +	Limit the commits output to ones with reflog entries that
> > +	match the specified pattern (regular expression). Ignored unless
> > +	--walk-reflogs is given.
> > +
> 
> I am debating myself if it is sane for this option to have no hint
> that it is about "limiting" in its name.  "--author/--committer"
> don't and it is clear from the context of the command that they are
> not about setting author/committer, so "--reflog-message" may be
> interpreted the same, perhaps.

I also found the name confusing on first-read. While "--author" is an
example in one direction, the fact that "--grep" is not called "--body"
is a counter-example.

I'd much rather see it as "--grep-reflog" or something. You could also
do "--grep-reflog-message", which would match a later
"--grep-reflog-author", but I am not sure anybody would want the latter,
and it makes the current name a lot longer.

I actually think just checking the reflog when we call "--grep" would
the most common workflow, and requires no extra work from the user.  My
only hesitation is that if somebody _does_ want to distinguish, there's
no escape hatch. Of course, the reflog walker is already full of such
weird conflations (e.g., the rewriting of parent and date information).

-Peff

Re: [PATCH] revision: add --reflog-message to grep reflog messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I actually think just checking the reflog when we call "--grep" would
> the most common workflow, and requires no extra work from the user.  My
> only hesitation is that if somebody _does_ want to distinguish, there's
> no escape hatch. Of course, the reflog walker is already full of such
> weird conflations (e.g., the rewriting of parent and date information).

Yes, that reasoning more-or-less matches the reason why I said we do
not necessarily want to go "fancier".  But "log -g" output already
makes it fairly clear that the additional information is not part of
the regular log and is a some header-like thingy, so I actually am
OK with --reflog-grep and --grep being different.

The output from "log --show-notes", on the other hand, is even more
conflated and a casual user would view it as part of the message, so
I would imagine that if we ever do the extention to cover notes
data, the normal "--grep" should apply to it.

[PATCH 1/3] grep: generalize header grep code to accept arbitrary headers

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Fri, Sep 28, 2012 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
 >>  enum grep_header_field {
 >>       GREP_HEADER_AUTHOR = 0,
 >> -     GREP_HEADER_COMMITTER
 >> +     GREP_HEADER_COMMITTER,
 >> +     GREP_HEADER_REFLOG,
 >> +     GREP_HEADER_FIELD_MAX
 >>  };
 >> -#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1)
 >
 > Please add comment to ensure that FIELD_MAX stays at the end; if you
 > ensure that, the result is much better than the original "we know
 > committer is at the end so add one".

 It's probably even better to remove the enum. Say one day I got drunk
 and decided to add --grep-encoding. This patch makes sure that I could
 submit such a patch even in drunk state.

 grep.c     | 76 +++++++++++++++++++++++++++++++++-----------------------------
 grep.h     | 11 +++------
 revision.c |  8 +++----
 3 files changed, 47 insertions(+), 48 deletions(-)

diff --git a/grep.c b/grep.c
index 898be6e..0c72262 100644
--- a/grep.c
+++ b/grep.c
@@ -10,7 +10,7 @@ static int grep_source_is_binary(struct grep_source *gs);
 static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,
 					const char *origin, int no,
 					enum grep_pat_token t,
-					enum grep_header_field field)
+					const char *header, size_t header_len)
 {
 	struct grep_pat *p = xcalloc(1, sizeof(*p));
 	p->pattern = xmemdupz(pat, patlen);
@@ -18,7 +18,8 @@ static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,
 	p->origin = origin;
 	p->no = no;
 	p->token = t;
-	p->field = field;
+	p->header = header;
+	p->header_len = header_len;
 	return p;
 }
 
@@ -45,7 +46,8 @@ static void do_append_grep_pat(struct grep_pat ***tail, struct grep_pat *p)
 			if (!nl)
 				break;
 			new_pat = create_grep_pat(nl + 1, len - 1, p->origin,
-						  p->no, p->token, p->field);
+						  p->no, p->token,
+						  p->header, p->header_len);
 			new_pat->next = p->next;
 			if (!p->next)
 				*tail = &new_pat->next;
@@ -59,11 +61,13 @@ static void do_append_grep_pat(struct grep_pat ***tail, struct grep_pat *p)
 	}
 }
 
+/* header must not be freed while grep is running */
 void append_header_grep_pattern(struct grep_opt *opt,
-				enum grep_header_field field, const char *pat)
+				const char *header, const char *pat)
 {
 	struct grep_pat *p = create_grep_pat(pat, strlen(pat), "header", 0,
-					     GREP_PATTERN_HEAD, field);
+					     GREP_PATTERN_HEAD,
+					     header, strlen(header));
 	do_append_grep_pat(&opt->header_tail, p);
 }
 
@@ -76,7 +80,7 @@ void append_grep_pattern(struct grep_opt *opt, const char *pat,
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen,
 		     const char *origin, int no, enum grep_pat_token t)
 {
-	struct grep_pat *p = create_grep_pat(pat, patlen, origin, no, t, 0);
+	struct grep_pat *p = create_grep_pat(pat, patlen, origin, no, t, NULL, 0);
 	do_append_grep_pat(&opt->pattern_tail, p);
 }
 
@@ -92,7 +96,7 @@ struct grep_opt *grep_opt_dup(const struct grep_opt *opt)
 	for(pat = opt->pattern_list; pat != NULL; pat = pat->next)
 	{
 		if(pat->token == GREP_PATTERN_HEAD)
-			append_header_grep_pattern(ret, pat->field,
+			append_header_grep_pattern(ret, pat->header,
 						   pat->pattern);
 		else
 			append_grep_pat(ret, pat->pattern, pat->patternlen,
@@ -359,7 +363,7 @@ static void dump_grep_pat(struct grep_pat *p)
 	switch (p->token) {
 	default: break;
 	case GREP_PATTERN_HEAD:
-		fprintf(stderr, "<head %d>", p->field); break;
+		fprintf(stderr, "<head %s>", p->header); break;
 	case GREP_PATTERN_BODY:
 		fprintf(stderr, "<body>"); break;
 	}
@@ -436,9 +440,10 @@ static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *
 static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 {
 	struct grep_pat *p;
-	struct grep_expr *header_expr;
-	struct grep_expr *(header_group[GREP_HEADER_FIELD_MAX]);
-	enum grep_header_field fld;
+	struct grep_expr *header_expr = NULL;
+	struct grep_expr **header_group;
+	struct string_list header = STRING_LIST_INIT_NODUP;
+	int fld;
 
 	if (!opt->header_list)
 		return NULL;
@@ -446,37 +451,45 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	for (p = opt->header_list; p; p = p->next) {
 		if (p->token != GREP_PATTERN_HEAD)
 			die("bug: a non-header pattern in grep header list.");
-		if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
-			die("bug: unknown header field %d", p->field);
+		if (!p->header || !p->header_len)
+			die("bug: unknown header field");
 		compile_regexp(p, opt);
+		string_list_append(&header, p->header);
 	}
 
-	for (fld = 0; fld < GREP_HEADER_FIELD_MAX; fld++)
-		header_group[fld] = NULL;
+	sort_string_list(&header);
+	string_list_remove_duplicates(&header, 0);
+	header_group = xmalloc(sizeof(*header_group) * header.nr);
+	memset(header_group, 0, sizeof(*header_group) * header.nr);
 
 	for (p = opt->header_list; p; p = p->next) {
 		struct grep_expr *h;
 		struct grep_pat *pp = p;
+		struct string_list_item *item;
 
 		h = compile_pattern_atom(&pp);
 		if (!h || pp != p->next)
 			die("bug: malformed header expr");
-		if (!header_group[p->field]) {
-			header_group[p->field] = h;
+		item = string_list_lookup(&header, p->header);
+		if (!item)
+			die("bug: malformed header expr");
+		fld = item - header.items;
+		if (!header_group[fld]) {
+			header_group[fld] = h;
 			continue;
 		}
-		header_group[p->field] = grep_or_expr(h, header_group[p->field]);
+		header_group[fld] = grep_or_expr(h, header_group[fld]);
 	}
 
-	header_expr = NULL;
-
-	for (fld = 0; fld < GREP_HEADER_FIELD_MAX; fld++) {
-		if (!header_group[fld])
-			continue;
+	for (fld = 0; fld < header.nr; fld++) {
 		if (!header_expr)
 			header_expr = grep_true_expr();
 		header_expr = grep_or_expr(header_group[fld], header_expr);
 	}
+
+	string_list_clear(&header, 0);
+	free(header_group);
+
 	return header_expr;
 }
 
@@ -691,14 +704,6 @@ static int strip_timestamp(char *bol, char **eol_p)
 	return 0;
 }
 
-static struct {
-	const char *field;
-	size_t len;
-} header_field[] = {
-	{ "author ", 7 },
-	{ "committer ", 10 },
-};
-
 static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
 			     enum grep_context ctx,
 			     regmatch_t *pmatch, int eflags)
@@ -714,12 +719,11 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
 	if (p->token == GREP_PATTERN_HEAD) {
 		const char *field;
 		size_t len;
-		assert(p->field < ARRAY_SIZE(header_field));
-		field = header_field[p->field].field;
-		len = header_field[p->field].len;
-		if (strncmp(bol, field, len))
+		field = p->header;
+		len = p->header_len;
+		if (strncmp(bol, field, len) || bol[len] != ' ')
 			return 0;
-		bol += len;
+		bol += len + 1;
 		saved_ch = strip_timestamp(bol, &eol);
 	}
 
diff --git a/grep.h b/grep.h
index 8a28a67..bc00f2a 100644
--- a/grep.h
+++ b/grep.h
@@ -27,12 +27,6 @@ enum grep_context {
 	GREP_CONTEXT_BODY
 };
 
-enum grep_header_field {
-	GREP_HEADER_AUTHOR = 0,
-	GREP_HEADER_COMMITTER
-};
-#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1)
-
 struct grep_pat {
 	struct grep_pat *next;
 	const char *origin;
@@ -40,7 +34,8 @@ struct grep_pat {
 	enum grep_pat_token token;
 	char *pattern;
 	size_t patternlen;
-	enum grep_header_field field;
+	const char *header;
+	size_t header_len;
 	regex_t regexp;
 	pcre *pcre_regexp;
 	pcre_extra *pcre_extra_info;
@@ -136,7 +131,7 @@ struct grep_opt {
 
 extern void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
 extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);
-extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);
+extern void append_header_grep_pattern(struct grep_opt *, const char *, const char *);
 extern void compile_grep_patterns(struct grep_opt *opt);
 extern void free_grep_patterns(struct grep_opt *opt);
 extern int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size);
diff --git a/revision.c b/revision.c
index ae12e11..f7cf385 100644
--- a/revision.c
+++ b/revision.c
@@ -1288,9 +1288,9 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
 	append_grep_pattern(&revs->grep_filter, ptn, "command line", 0, what);
 }
 
-static void add_header_grep(struct rev_info *revs, enum grep_header_field field, const char *pattern)
+static void add_header_grep(struct rev_info *revs, const char *header, const char *pattern)
 {
-	append_header_grep_pattern(&revs->grep_filter, field, pattern);
+	append_header_grep_pattern(&revs->grep_filter, header, pattern);
 }
 
 static void add_message_grep(struct rev_info *revs, const char *pattern)
@@ -1590,10 +1590,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	 * Grepping the commit log
 	 */
 	else if ((argcount = parse_long_opt("author", argv, &optarg))) {
-		add_header_grep(revs, GREP_HEADER_AUTHOR, optarg);
+		add_header_grep(revs, "author", optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("committer", argv, &optarg))) {
-		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
+		add_header_grep(revs, "committer", optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
-- 
1.7.12.1.405.gb727dc9

[PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Nguyễn Thái Ngọc Duy]

Similar to --author/--committer which filters commits by author and
committer header fields. --grep-reflog adds a fake "reflog" header to
commit and a grep filter to search on that line.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Fri, Sep 28, 2012 at 12:09 AM, Junio C Hamano <gitster@pobox.com>
 wrote:
 > I am debating myself if it is sane for this option to have no hint
 > that it is about "limiting" in its name.  "--author/--committer"
 > don't and it is clear from the context of the command that they are
 > not about setting author/committer, so "--reflog-message" may be
 > interpreted the same, perhaps.
 >
 > The entry in the context above talks about multiple occurrence of
 > that option. Shouldn't this new one also say what happens when it
 > is
 > given twice?

 Fixed.

 > I think I wrote prep_header_patterns() and compile_grep_patterns()
 > carefully enough not to assume the headers are only the author and
 > committer names, so the various combinations i.e. all-match,
 > author(s), committer(s), grep(s), and reflog-message(s), should
 > work
 > out of the box, but have you actually tested them?

 I did not. I do now, tests are also added.

 On Fri, Sep 28, 2012 at 12:28 AM, Jeff King <peff@peff.net> wrote:
 > I also found the name confusing on first-read. While "--author" is
 > an
 > example in one direction, the fact that "--grep" is not called
 > "--body"
 > is a counter-example.
 >
 > I'd much rather see it as "--grep-reflog" or something. You could
 > also
 > do "--grep-reflog-message", which would match a later
 > "--grep-reflog-author", but I am not sure anybody would want the
 > latter,
 > and it makes the current name a lot longer.

 Changed it to --grep-reflog. I was tempted to add --grep-* but I
 can't error out if user gives an invalid header. So --grep-reflog
 only for now.

 Documentation/rev-list-options.txt |  8 ++++++++
 revision.c                         | 20 ++++++++++++++++++--
 t/t7810-grep.sh                    | 26 ++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1fc2a18..aa7cd9d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -51,6 +51,14 @@ endif::git-rev-list[]
 	commits whose author matches any of the given patterns are
 	chosen (similarly for multiple `--committer=<pattern>`).
 
+--grep-reflog=<pattern>::
+
+	Limit the commits output to ones with reflog entries that
+	match the specified pattern (regular expression). With
+	more than one `--grep-reflog`, commits whose reflog message
+	matches any of the given patterns are chosen. Ignored unless
+	`--walk-reflogs` is given.
+
 --grep=<pattern>::
 
 	Limit the commits output to ones with log message that
diff --git a/revision.c b/revision.c
index f7cf385..cfa0e2e 100644
--- a/revision.c
+++ b/revision.c
@@ -1595,6 +1595,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((argcount = parse_long_opt("committer", argv, &optarg))) {
 		add_header_grep(revs, "committer", optarg);
 		return argcount;
+	} else if ((argcount = parse_long_opt("grep-reflog", argv, &optarg))) {
+		add_header_grep(revs, "reflog", optarg);
+		return argcount;
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
 		return argcount;
@@ -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;
 }
 
 static inline int want_ancestry(struct rev_info *revs)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 91db352..f42a605 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -546,6 +546,32 @@ test_expect_success 'log grep (6)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log grep (7)' '
+	git log -g --grep-reflog="commit: third" --pretty=tformat:%s >actual &&
+	echo third >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log grep (8)' '
+	git log -g --grep-reflog="commit: third" --grep-reflog="commit: second" --pretty=tformat:%s >actual &&
+	{
+		echo third && echo second
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log grep (9)' '
+	git log -g --grep-reflog="commit: third" --author="Thor" --pretty=tformat:%s >actual &&
+	echo third >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log grep (9)' '
+	git log -g --grep-reflog="commit: third" --author="non-existant" --pretty=tformat:%s >actual &&
+	: >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log with multiple --grep uses union' '
 	git log --grep=i --grep=r --format=%s >actual &&
 	{
-- 
1.7.12.1.405.gb727dc9

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> Similar to --author/--committer which filters commits by author and
> committer header fields. --grep-reflog adds a fake "reflog" header to
> commit and a grep filter to search on that line.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  On Fri, Sep 28, 2012 at 12:09 AM, Junio C Hamano <gitster@pobox.com>
>  wrote:
>  > I am debating myself if it is sane for this option to have no hint
>  > that it is about "limiting" in its name.  "--author/--committer"
>  > don't and it is clear from the context of the command that they are
>  > not about setting author/committer, so "--reflog-message" may be
>  > interpreted the same, perhaps.
>  >
>  > The entry in the context above talks about multiple occurrence of
>  > that option. Shouldn't this new one also say what happens when it
>  > is
>  > given twice?
>
>  Fixed.
>
>  > I think I wrote prep_header_patterns() and compile_grep_patterns()
>  > carefully enough not to assume the headers are only the author and
>  > committer names, so the various combinations i.e. all-match,
>  > author(s), committer(s), grep(s), and reflog-message(s), should
>  > work
>  > out of the box, but have you actually tested them?
>
>  I did not. I do now, tests are also added.

You seem to have skipped a lot more important comment from the
message that came immediately after the above part.  Did you take a
look at grep.c::match_one_pattern() that calls strip_timestamp() for
any and all GREP_PATTERN_HEAD items?

This is why I think that the [1/3] in this series shows a total lack
of design taste.  Each element in "headers", unlike "body", is a
structured field and its payload has meaning depending on what field
name is associated with it.  That is why we do not give a way to
users to willy-nilly add ad-hoc header fields.

The other side of this coin is that Git ought to be able to take
advantage of the meaning of each header.  Stripping the timestamp
part when doing a textual match against --author/--committer is one
example.  The code in match_one_pattern() to deal with the header
patterns need to be _extended_ to take into account the special
treatment each header, includign this new "reflog " fake header,
wants when doing a textual match, and when we add any other match
against header (either real or fake), we should be able to add
specific matching logic tailored to handle the particular meaning of
the header would have.

For that to happen, the code _must_ know what kind of headers we
would support; discarding the existing enum is going in a wrong
direction.

When we introduce "git log --header=frotz:xyzzy" option that looks
for a generic "frotz " header and tries to see if "xyzzy" textually
appears in the field, we may want to add a generic "this is not a
header that we have special treatment for" enum to the mix.  But for
known kinds of headers, we would need a list of what each header is
and what semantics it wants.

So please reconsider undoing [1/3], and rerolling [2/3] that depends
on it.

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Nguyễn Thái Ngọc Duy]

On Sat, Sep 29, 2012 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> For that to happen, the code _must_ know what kind of headers we
> would support; discarding the existing enum is going in a wrong
> direction.

Or what kind of manipulation is required for a header. The caller can
decide if it wants such manipulation or not. Somebody might want to
grep committer's date, for example.

> When we introduce "git log --header=frotz:xyzzy" option that looks
> for a generic "frotz " header and tries to see if "xyzzy" textually
> appears in the field, we may want to add a generic "this is not a
> header that we have special treatment for" enum to the mix.  But for
> known kinds of headers, we would need a list of what each header is
> and what semantics it wants.
>
> So please reconsider undoing [1/3], and rerolling [2/3] that depends
> on it.

Done. The enum is kept. I added a few tests about grepping timestamp
in 1/3 to keep people (or myself) from making the same mistake I did.

3/3 is reposted for completeness, I don't care much about notes (so
far). It's up to you to take or drop it.

Nguyễn Thái Ngọc Duy (3):
  grep: prepare for new header field filter
  revision: add --grep-reflog to filter commits by reflog messages
  revision: make --grep search in notes too if shown

 Documentation/rev-list-options.txt |  8 ++++++++
 grep.c                             | 10 +++++++++-
 grep.h                             |  7 +++++--
 revision.c                         | 26 ++++++++++++++++++++++++--
 t/t7810-grep.sh                    | 38 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+), 5 deletions(-)

-- 
1.7.12.1.406.g6ab07c4

[PATCH 1/3] grep: prepare for new header field filter

[Nguyễn Thái Ngọc Duy]

grep supports only author and committer headers, which have the same
special treatment that later headers may or may not have. Check for
field type and only strip_timestamp() when the field is either author
or committer.

GREP_HEADER_FIELD_MAX is put in the grep_header_field enum to be
calculated automatically, correctly, as long as it's at the end of the
enum.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 grep.c          |  9 ++++++++-
 grep.h          |  6 ++++--
 t/t7810-grep.sh | 12 ++++++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index 898be6e..8d73995 100644
--- a/grep.c
+++ b/grep.c
@@ -720,7 +720,14 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
 		if (strncmp(bol, field, len))
 			return 0;
 		bol += len;
-		saved_ch = strip_timestamp(bol, &eol);
+		switch (p->field) {
+		case GREP_HEADER_AUTHOR:
+		case GREP_HEADER_COMMITTER:
+			saved_ch = strip_timestamp(bol, &eol);
+			break;
+		default:
+			break;
+		}
 	}
 
  again:
diff --git a/grep.h b/grep.h
index 8a28a67..d54adbe 100644
--- a/grep.h
+++ b/grep.h
@@ -29,9 +29,11 @@ enum grep_context {
 
 enum grep_header_field {
 	GREP_HEADER_AUTHOR = 0,
-	GREP_HEADER_COMMITTER
+	GREP_HEADER_COMMITTER,
+
+	/* Must be at the end of the enum */
+	GREP_HEADER_FIELD_MAX
 };
-#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1)
 
 struct grep_pat {
 	struct grep_pat *next;
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 91db352..30eaa9a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -628,6 +628,18 @@ test_expect_success 'log --all-match --grep --grep --author takes intersection'
 	test_cmp expect actual
 '
 
+test_expect_success 'log --author does not search in timestamp' '
+	: >expect &&
+	git log --author="$GIT_AUTHOR_DATE" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --committer does not search in timestamp' '
+	: >expect &&
+	git log --committer="$GIT_COMMITTER_DATE" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'grep with CE_VALID file' '
 	git update-index --assume-unchanged t/t &&
 	rm t/t &&
-- 
1.7.12.1.406.g6ab07c4

Re: [PATCH 1/3] grep: prepare for new header field filter

[Jeff King]

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

> diff --git a/grep.c b/grep.c
> index 898be6e..8d73995 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -720,7 +720,14 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
>  		if (strncmp(bol, field, len))
>  			return 0;
>  		bol += len;
> -		saved_ch = strip_timestamp(bol, &eol);
> +		switch (p->field) {
> +		case GREP_HEADER_AUTHOR:
> +		case GREP_HEADER_COMMITTER:
> +			saved_ch = strip_timestamp(bol, &eol);
> +			break;
> +		default:
> +			break;
> +		}

Reading this hunk, I wondered what happens to saved_ch if we do not set
it here. Fortunately it is initialized to 0, as we already have to
handle the non-header case. Then later we do this, which does introduce
a new condition (saved_ch was not set, but we trigger the first half of
the conditional):

      if (p->token == GREP_PATTERN_HEAD && saved_ch)
                *eol = saved_ch;

However, the second half of that conditional (which previously was only
triggered when we tried to split the timestamp, but there was a bogus
line with no ">" on it) prevents us from overwriting *eol.

So I think it is good, but it was non-obvious enough that I wanted to
save other reviewers from investigating it.  The rest of the patch looks
good to me, as well.

-Peff

[PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Nguyễn Thái Ngọc Duy]

Similar to --author/--committer which filters commits by author and
committer header fields. --grep-reflog adds a fake "reflog" header to
commit and a grep filter to search on that line.

All rules to --author/--committer apply except no timestamp stripping.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/rev-list-options.txt |  8 ++++++++
 grep.c                             |  1 +
 grep.h                             |  1 +
 revision.c                         | 20 ++++++++++++++++++--
 t/t7810-grep.sh                    | 26 ++++++++++++++++++++++++++
 5 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1fc2a18..aa7cd9d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -51,6 +51,14 @@ endif::git-rev-list[]
 	commits whose author matches any of the given patterns are
 	chosen (similarly for multiple `--committer=<pattern>`).
 
+--grep-reflog=<pattern>::
+
+	Limit the commits output to ones with reflog entries that
+	match the specified pattern (regular expression). With
+	more than one `--grep-reflog`, commits whose reflog message
+	matches any of the given patterns are chosen. Ignored unless
+	`--walk-reflogs` is given.
+
 --grep=<pattern>::
 
 	Limit the commits output to ones with log message that
diff --git a/grep.c b/grep.c
index 8d73995..d70dcdf 100644
--- a/grep.c
+++ b/grep.c
@@ -697,6 +697,7 @@ static struct {
 } header_field[] = {
 	{ "author ", 7 },
 	{ "committer ", 10 },
+	{ "reflog ", 7 },
 };
 
 static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
diff --git a/grep.h b/grep.h
index d54adbe..6e78b96 100644
--- a/grep.h
+++ b/grep.h
@@ -30,6 +30,7 @@ enum grep_context {
 enum grep_header_field {
 	GREP_HEADER_AUTHOR = 0,
 	GREP_HEADER_COMMITTER,
+	GREP_HEADER_REFLOG,
 
 	/* Must be at the end of the enum */
 	GREP_HEADER_FIELD_MAX
diff --git a/revision.c b/revision.c
index ae12e11..109bec1 100644
--- a/revision.c
+++ b/revision.c
@@ -1595,6 +1595,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((argcount = parse_long_opt("committer", argv, &optarg))) {
 		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
 		return argcount;
+	} else if ((argcount = parse_long_opt("grep-reflog", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_REFLOG, optarg);
+		return argcount;
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
 		return argcount;
@@ -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;
 }
 
 static inline int want_ancestry(struct rev_info *revs)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 30eaa9a..3a5d0fd 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -546,6 +546,32 @@ test_expect_success 'log grep (6)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log grep (7)' '
+	git log -g --grep-reflog="commit: third" --pretty=tformat:%s >actual &&
+	echo third >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log grep (8)' '
+	git log -g --grep-reflog="commit: third" --grep-reflog="commit: second" --pretty=tformat:%s >actual &&
+	{
+		echo third && echo second
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log grep (9)' '
+	git log -g --grep-reflog="commit: third" --author="Thor" --pretty=tformat:%s >actual &&
+	echo third >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log grep (9)' '
+	git log -g --grep-reflog="commit: third" --author="non-existant" --pretty=tformat:%s >actual &&
+	: >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log with multiple --grep uses union' '
 	git log --grep=i --grep=r --format=%s >actual &&
 	{
-- 
1.7.12.1.406.g6ab07c4

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Jeff King]

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

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Junio C Hamano]

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.

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.

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Nguyen Thai Ngoc Duy]

On Sat, Sep 29, 2012 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.

reflog, in terms of both the number of commits and message length, is
usually short enough that slowdown does not really show, especially
when used with git-log, an interactive command.

Without the changes:

$ time git log -g --grep . >/dev/null

real    0m0.480s
user    0m0.451s
sys     0m0.025s

With the changes:

$ time ./git log -g --grep . >/dev/null

real    0m0.490s
user    0m0.471s
sys     0m0.018s

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

That's why I put "Ignored unless --walk-reflogs is given" in the
document. But an error would be fine too. I suppose an error is
preferable in case users do not read document carefully?
-- 
Duy

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> reflog, in terms of both the number of commits and message length, is
> usually short enough that slowdown does not really show, especially
> when used with git-log, an interactive command.

You shouldn't do things you can easily tell you do not need to,
especially when the effort to avoid doing unnecessary allocation,
copying and deallocation is not excessively larger than the saving.

I personally think that lack of perceived performance impact is not
an excuse to be sloppy. These things tend to add up.

> That's why I put "Ignored unless --walk-reflogs is given" in the
> document. But an error would be fine too. I suppose an error is
> preferable in case users do not read document carefully?


A reader who reads documentation carefully will spot that it is a
sloppy coding that such a nonsense combination of two options are
not caught and diagnosed as an error, when accepting and silently
ignoring the option would not help anybody.

An alternative may be to turn -g on when --grep-reflog is given.
Starting from a version that forbids --grep-reflog without -g will
let us change the command line parser to do so in the future without
backward incompatibility, but you cannot do so if you start from a
version that accepts and silently ignores.

How about squashing this in?  I've future-proofed commit_match() a
bit while at it; it would be cleaner to add new fake headers if the
function is done this way.

 Documentation/rev-list-options.txt |  4 ++--
 grep.c                             |  2 ++
 grep.h                             |  1 +
 revision.c                         | 13 +++++++++++--
 t/t7810-grep.sh                    |  4 ++++
 5 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index aa7cd9d..ca22106 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -56,8 +56,8 @@ endif::git-rev-list[]
 	Limit the commits output to ones with reflog entries that
 	match the specified pattern (regular expression). With
 	more than one `--grep-reflog`, commits whose reflog message
-	matches any of the given patterns are chosen. Ignored unless
-	`--walk-reflogs` is given.
+	matches any of the given patterns are chosen.  It is an
+	error to use this option unless `--walk-reflogs` is in use.
 
 --grep=<pattern>::
 
diff --git a/grep.c b/grep.c
index d70dcdf..edc7776 100644
--- a/grep.c
+++ b/grep.c
@@ -64,6 +64,8 @@ void append_header_grep_pattern(struct grep_opt *opt,
 {
 	struct grep_pat *p = create_grep_pat(pat, strlen(pat), "header", 0,
 					     GREP_PATTERN_HEAD, field);
+	if (field == GREP_HEADER_REFLOG)
+		opt->use_reflog_filter = 1;
 	do_append_grep_pat(&opt->header_tail, p);
 }
 
diff --git a/grep.h b/grep.h
index 6e78b96..c256ac6 100644
--- a/grep.h
+++ b/grep.h
@@ -107,6 +107,7 @@ struct grep_opt {
 #define GREP_BINARY_TEXT	2
 	int binary;
 	int extended;
+	int use_reflog_filter;
 	int pcre;
 	int relative;
 	int pathname;
diff --git a/revision.c b/revision.c
index 109bec1..9ad72df 100644
--- a/revision.c
+++ b/revision.c
@@ -1908,6 +1908,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	if (revs->reflog_info && revs->graph)
 		die("cannot combine --walk-reflogs with --graph");
+	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
+		die("cannot use --grep-reflog without --walk-reflogs");
 
 	return left;
 }
@@ -2217,12 +2219,19 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 	struct strbuf buf = STRBUF_INIT;
 	if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
 		return 1;
-	if (opt->reflog_info) {
+
+	/* Prepend "fake" headers as needed */
+	if (opt->grep_filter.use_reflog_filter) {
 		strbuf_addstr(&buf, "reflog ");
 		get_reflog_message(&buf, opt->reflog_info);
 		strbuf_addch(&buf, '\n');
-		strbuf_addstr(&buf, commit->buffer);
 	}
+
+	/* Copy the commit to temporary if we are using "fake" headers */
+	if (buf.len)
+		strbuf_addstr(&buf, commit->buffer);
+
+	/* Find either in the commit object, or in the temporary */
 	if (buf.len)
 		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
 	else
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 3a5d0fd..f698001 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -572,6 +572,10 @@ test_expect_success 'log grep (9)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log --grep-reflog can only be used under -g' '
+	test_must_fail git log --grep-reflog="commit: third"
+'
+
 test_expect_success 'log with multiple --grep uses union' '
 	git log --grep=i --grep=r --format=%s >actual &&
 	{
-- 
1.7.12.1.484.g1764bc0

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Nguyen Thai Ngoc Duy]

On Sun, Sep 30, 2012 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> How about squashing this in?  I've future-proofed commit_match() a
> bit while at it; it would be cleaner to add new fake headers if the
> function is done this way.

Sure. It looks good to me.

> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -56,8 +56,8 @@ endif::git-rev-list[]
>         Limit the commits output to ones with reflog entries that
>         match the specified pattern (regular expression). With
>         more than one `--grep-reflog`, commits whose reflog message
> -       matches any of the given patterns are chosen. Ignored unless
> -       `--walk-reflogs` is given.
> +       matches any of the given patterns are chosen.  It is an
> +       error to use this option unless `--walk-reflogs` is in use.

We could even drop the part about --walk-reflogs. Users will soon
figure it out when they try it.
-- 
Duy

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Jeff King]

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

[PATCH 3/3] revision: make --grep search in notes too if shown

[Nguyễn Thái Ngọc Duy]

Notes are shown after commit body. From user perspective it looks
pretty much like commit body and they may assume --grep would search
in that part too. Make it so.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/revision.c b/revision.c
index 109bec1..dff6fb7 100644
--- a/revision.c
+++ b/revision.c
@@ -2223,6 +2223,12 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 		strbuf_addch(&buf, '\n');
 		strbuf_addstr(&buf, commit->buffer);
 	}
+	if (opt->show_notes) {
+		if (!buf.len)
+			strbuf_addstr(&buf, commit->buffer);
+		format_display_notes(commit->object.sha1, &buf,
+				     get_log_output_encoding(), 0);
+	}
 	if (buf.len)
 		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
 	else
-- 
1.7.12.1.406.g6ab07c4

Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> On Sat, Sep 29, 2012 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> For that to happen, the code _must_ know what kind of headers we
>> would support; discarding the existing enum is going in a wrong
>> direction.
>
> Or what kind of manipulation is required for a header. The caller can
> decide if it wants such manipulation or not. Somebody might want to
> grep committer's date, for example.

Yes, if we wanted to filter log outputs to commits authored only on
Thursdays, we would introduce --author-date="<some expression that
yields true only on Thursdays, given the timestmap" option, add
GREP_HEADER_AUTHOR_TIMESTAMP_EXPRESSION=2 to enum grep_header_field,
and add a new element { "author ", 7 } to grep.c::header_field[2]
(the index 2 matches GREP_HEADER_AUTHOR_TIMESTAMP_EXPRESSION), and
implement the evaluator for <some expression that yields true only
on Thursdays, given the timestamp> language in the GREP_PATTERN_HEAD
part of match_one_pattern() and use the p->field to select that
logic.  At that point, we may be better off introducing a helper
function to split the more complex logic for GREP_PATTERN_HEAD out
of the match_one_pattern() function.

[PATCH 3/3] revision: make --grep search in notes too if shown

[Nguyễn Thái Ngọc Duy]

Notes are shown after commit body. From user perspective it looks
pretty much like commit body and they may assume --grep would search
in that part too. Make it so.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Fri, Sep 28, 2012 at 1:16 AM, Junio C Hamano <gitster@pobox.com>
 wrote:
 > The output from "log --show-notes", on the other hand, is even more
 > conflated and a casual user would view it as part of the message,
 > so
 > I would imagine that if we ever do the extention to cover notes
 > data, the normal "--grep" should apply to it.

 Something like this?

 revision.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/revision.c b/revision.c
index cfa0e2e..febb4d7 100644
--- a/revision.c
+++ b/revision.c
@@ -2223,6 +2223,12 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 		strbuf_addch(&buf, '\n');
 		strbuf_addstr(&buf, commit->buffer);
 	}
+	if (opt->show_notes) {
+		if (!buf.len)
+			strbuf_addstr(&buf, commit->buffer);
+		format_display_notes(commit->object.sha1, &buf,
+				     get_log_output_encoding(), 0);
+	}
 	if (buf.len)
 		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
 	else
-- 
1.7.12.1.405.gb727dc9

Re: [PATCH 3/3] revision: make --grep search in notes too if shown

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> Notes are shown after commit body. From user perspective it looks
> pretty much like commit body and they may assume --grep would search
> in that part too. Make it so.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  On Fri, Sep 28, 2012 at 1:16 AM, Junio C Hamano <gitster@pobox.com>
>  wrote:
>  > The output from "log --show-notes", on the other hand, is even more
>  > conflated and a casual user would view it as part of the message,
>  > so
>  > I would imagine that if we ever do the extention to cover notes
>  > data, the normal "--grep" should apply to it.
>
>  Something like this?

Yes, that was what I had in mind.

>  revision.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index cfa0e2e..febb4d7 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2223,6 +2223,12 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
>  		strbuf_addch(&buf, '\n');
>  		strbuf_addstr(&buf, commit->buffer);
>  	}
> +	if (opt->show_notes) {
> +		if (!buf.len)
> +			strbuf_addstr(&buf, commit->buffer);
> +		format_display_notes(commit->object.sha1, &buf,
> +				     get_log_output_encoding(), 0);
> +	}
>  	if (buf.len)
>  		retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
>  	else