threads / patch / 31504

patchrev-list/log: document logic with several limiting options

Subject: [PATCH] rev-list/log: document logic with several limiting options

## tl;dr

43 messages between Sep 11, 2012 and Sep 14, 2012. Diffs are folded; open one to read it.

replies: 42people: 2as markdown or json

Michael J Gruber· Sep 11, 2012, 14:45 UTC · lore

The current behavior is probably as useful as it is confusing. In any case it is going to stay. So, document it.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
I would have written a test but don't really know where to stick it in.
rev-list has many small tests where it doesn't fit.
 Documentation/rev-list-options.txt | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to Documentation/rev-list-options.txt +6 −0
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5436eba..9c13df3 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -6,6 +6,12 @@ special notations explained in the description, additional commit
 limiting may be applied. Note that they are applied before commit
 ordering and formatting options, such as '--reverse'.
 
+All occurrences of the same option are ORed: '--grep=foo --grep=bar'
+limits to commits which match at least one of these conditions.
+
+Different options are ANDed: '--author=bar --grep=foo'
+limits to commits which match both conditions.
+
 --
 
 -n 'number'::

-- 
1.7.12.463.gbd9d638
Junio C Hamano· Sep 11, 2012, 16:22 UTC · re: Michael J Gruber · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 7 quoted lines
> The current behavior is probably as useful as it is confusing. In any
> case it is going to stay. So, document it.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> I would have written a test but don't really know where to stick it in.
> rev-list has many small tests where it doesn't fit.
"git log" would be the more natural place, I would say.
Show 14 quoted lines
>
>  Documentation/rev-list-options.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 5436eba..9c13df3 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -6,6 +6,12 @@ special notations explained in the description, additional commit
>  limiting may be applied. Note that they are applied before commit
>  ordering and formatting options, such as '--reverse'.
>  
> +All occurrences of the same option are ORed: '--grep=foo --grep=bar'
> +limits to commits which match at least one of these conditions.
How would this interact with "--all-match"?
Michael J Gruber· Sep 12, 2012, 13:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Junio C Hamano venit, vidit, dixit 11.09.2012 18:22:
Show 11 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> The current behavior is probably as useful as it is confusing. In any
>> case it is going to stay. So, document it.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>> I would have written a test but don't really know where to stick it in.
>> rev-list has many small tests where it doesn't fit.
> 
> "git log" would be the more natural place, I would say.

Well, to me, "git log" is the ui to "git rev-list", so everything which tests the revision walker should be tested using "git rev-list". But I don't care as long as it fits in - and can be found later.

That being said, I now see lots of "log --grep" and "log --author" tests in the grep-test. So I guess there it fits best (despite the misnomer).

Show 16 quoted lines
>>
>>  Documentation/rev-list-options.txt | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
>> index 5436eba..9c13df3 100644
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -6,6 +6,12 @@ special notations explained in the description, additional commit
>>  limiting may be applied. Note that they are applied before commit
>>  ordering and formatting options, such as '--reverse'.
>>  
>> +All occurrences of the same option are ORed: '--grep=foo --grep=bar'
>> +limits to commits which match at least one of these conditions.
> 
> How would this interact with "--all-match"?
Badly.

It was introduced in 0ab7befa with a clear meaning (AND everything), then the general logic (without --all-match) was modified in 80235ba7 (to take headermatch AND (all greps ORed)), and 5aaeb733 finally made multiple authors resp. committers get ORed among each other. All of this in an attempt to make the standard usage most useful, of course. As a consequence, --all-match does not influence multiple --author options at all (contrary to the doc), e.g.

I don't see any of this reflected in the doc, though. I noticed only by reading t/t7810-grep.sh. Before that, I had only gone by my own testing which didn't reveal the multiple author multiple special casing effect.

I guess I'll have to wrap my head around the current implementation a few more times before trying to describe the status quo in the documentation...

Junio C Hamano· Sep 12, 2012, 17:25 UTC · re: Michael J Gruber · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 15 quoted lines
> It was introduced in 0ab7befa with a clear meaning (AND everything),
> then the general logic (without --all-match) was modified in 80235ba7
> (to take headermatch AND (all greps ORed)), and 5aaeb733 finally made
> multiple authors resp. committers get ORed among each other. All of this
> in an attempt to make the standard usage most useful, of course. As a
> consequence, --all-match does not influence multiple --author options at
> all (contrary to the doc), e.g.
>
> I don't see any of this reflected in the doc, though. I noticed only by
> reading t/t7810-grep.sh. Before that, I had only gone by my own testing
> which didn't reveal the multiple author multiple special casing effect.
>
> I guess I'll have to wrap my head around the current implementation a
> few more times before trying to describe the status quo in the
> documentation...

This is what I used to use when adding these generalized grep boolean expressions.

With this applied, you can try things like these:
    $ git log --grep=regexp --grep=nosuch --all-match >/dev/null
    $ git log --grep=regexp --grep=nosuch --author=Michael >/dev/null

For example, "--all-match --grep=regexp --author=Michael --author=Linus" turns into

    [all-match]
    (or
     pattern_body<body>regexp
     (or
      (or
       pattern_head<head 0>Linus
       pattern_head<head 0>Michael
      )
      true
     )
    )

that says "body must have 'regexp' in it, and authored by either Linus or Michael".

The semantics of "--all-match" is different from "--and" (which, together with "--or", ")", and "(", is not availble to rev-list command line parser primarily because "--not" is not available). After applying all the "or"-ed terms, it checks the top-level nodes that are "or"-ed and makes sure all of them fired (possibly and usually on different lines).

Junio C Hamano· Sep 12, 2012, 17:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Junio C Hamano <gitster@pobox.com> writes:
> This is what I used to use when adding these generalized grep
> boolean expressions.
>
> With this applied,...
And this is the "this" X-<.
 grep.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)
Show changes to grep.c +89 −1
diff --git a/grep.c b/grep.c
index 04e3ec6..566c4cc 100644
--- a/grep.c
+++ b/grep.c
@@ -332,6 +332,87 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)
 	return compile_pattern_or(list);
 }
 
+static void indent(int in)
+{
+	while (in-- > 0)
+		fputc(' ', stderr);
+}
+
+static void dump_grep_pat(struct grep_pat *p)
+{
+	switch (p->token) {
+	case GREP_AND: fprintf(stderr, "*and*"); break;
+	case GREP_OPEN_PAREN: fprintf(stderr, "*(*"); break;
+	case GREP_CLOSE_PAREN: fprintf(stderr, "*)*"); break;
+	case GREP_NOT: fprintf(stderr, "*not*"); break;
+	case GREP_OR: fprintf(stderr, "*or*"); break;
+
+	case GREP_PATTERN: fprintf(stderr, "pattern"); break;
+	case GREP_PATTERN_HEAD: fprintf(stderr, "pattern_head"); break;
+	case GREP_PATTERN_BODY: fprintf(stderr, "pattern_body"); break;
+	}
+
+	switch (p->token) {
+	default: break;
+	case GREP_PATTERN_HEAD:
+		fprintf(stderr, "<head %d>", p->field); break;
+	case GREP_PATTERN_BODY:
+		fprintf(stderr, "<body>"); break;
+	}
+	switch (p->token) {
+	default: break;
+	case GREP_PATTERN_HEAD:
+	case GREP_PATTERN_BODY:
+	case GREP_PATTERN:
+		fprintf(stderr, "%.*s", (int)p->patternlen, p->pattern);
+		break;
+	}
+	fputc('\n', stderr);
+}
+
+static void dump_grep_expression_1(struct grep_expr *x, int in)
+{
+	indent(in);
+	switch (x->node) {
+	case GREP_NODE_TRUE:
+		fprintf(stderr, "true\n");
+		break;
+	case GREP_NODE_ATOM:
+		dump_grep_pat(x->u.atom);
+		break;
+	case GREP_NODE_NOT:
+		fprintf(stderr, "(not\n");
+		dump_grep_expression_1(x->u.unary, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	case GREP_NODE_AND:
+		fprintf(stderr, "(and\n");
+		dump_grep_expression_1(x->u.binary.left, in+1);
+		dump_grep_expression_1(x->u.binary.right, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	case GREP_NODE_OR:
+		fprintf(stderr, "(or\n");
+		dump_grep_expression_1(x->u.binary.left, in+1);
+		dump_grep_expression_1(x->u.binary.right, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	}
+}
+
+void dump_grep_expression(struct grep_opt *opt)
+{
+	struct grep_expr *x = opt->pattern_expression;
+
+	if (opt->all_match)
+		fprintf(stderr, "[all-match]\n");
+	dump_grep_expression_1(x, 0);
+	fflush(NULL);
+}
+
 static struct grep_expr *grep_true_expr(void)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
@@ -395,7 +476,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	return header_expr;
 }
 
-void compile_grep_patterns(struct grep_opt *opt)
+static void compile_grep_patterns_real(struct grep_opt *opt)
 {
 	struct grep_pat *p;
 	struct grep_expr *header_expr = prep_header_patterns(opt);
@@ -432,9 +513,16 @@ void compile_grep_patterns(struct grep_opt *opt)
 	else
 		opt->pattern_expression = grep_or_expr(opt->pattern_expression,
 						       header_expr);
+
 	opt->all_match = 1;
 }
 
+void compile_grep_patterns(struct grep_opt *opt)
+{
+	compile_grep_patterns_real(opt);
+	dump_grep_expression(opt);
+}
+
 static void free_pattern_expr(struct grep_expr *x)
 {
 	switch (x->node) {

-- 
1.7.12.414.g1a62b7a
Michael J Gruber· Sep 13, 2012, 14:04 UTC · re: Junio C Hamano · lore

[PATCHv2 0/6] rev-list/log: document logic with several limiting options

So, this turned out to be a bit longer. I decided not to implement "--debug" for "git log --grep" and such because the current code does a lot of special casing, so that the existing debug code happily outputs OR nodes in cases where the code in grep.c effectively does AND (without changing the expression nodes).

So, this series sets up a few more tests to prove to myself that I'm not completely off in my understanding of the limiting options, and to make me confident enough for the documentation patch 6/6 (v2 of the old 1/1).

5/6 documents (by a test) an issue which I consider a (now) known failure: 'git log --all-match --author=me --grep=foo --grep=bar' does not AND the greps (whereas it does without --author). I don't describe this corner case in the doc patch.

Michael J Gruber (6):
  t7810-grep: bring log --grep tests in common form
  t7810-grep: test multiple --grep with and without --all-match
  t7810-grep: test multiple --author with --all-match
  t7810-grep: test interaction of multiple --grep and --author options
  t7810-grep: test --all-match with multiple --grep and --author
    options
  rev-list/log: document logic with several limiting options
 Documentation/rev-list-options.txt | 15 ++++++-
 t/t7810-grep.sh                    | 90 ++++++++++++++++++++++++++++++++------
 2 files changed, 90 insertions(+), 15 deletions(-)
-- 
1.7.12.463.gbd9d638
Junio C Hamano· Sep 13, 2012, 20:18 UTC · re: Michael J Gruber · lore

Re: [PATCHv2 0/6] rev-list/log: document logic with several limiting options

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 5 quoted lines
> So, this turned out to be a bit longer.
> I decided not to implement "--debug" for "git log --grep" and such
> because the current code does a lot of special casing, so that the
> existing debug code happily outputs OR nodes in cases where the code
> in grep.c effectively does AND (without changing the expression nodes).

Is that something we (not necessarily you) would want to fix? How many are they?

The reason I am asking is because I do not think of any. The entire document is considered unmatched unless all of the OR nodes in the top-level backbone have triggered for some line in the document when "all-match" is in effect, but that is quite different from turning OR nodes into AND nodes (they both are about "does this line match the pattern?"), so...

Michael J Gruber· Sep 13, 2012, 14:04 UTC · re: Junio C Hamano · lore

[PATCHv2 1/6] t7810-grep: bring log --grep tests in common form

The log --grep tests generate the expected out in different ways. Make them all use command blocks so that subshells are avoided and the expected output is easier to grasp visually.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
Show changes to t/t7810-grep.sh +18 −6
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 35d357d..9b683ac 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -502,31 +502,41 @@ test_expect_success 'log grep setup' '
 
 test_expect_success 'log grep (1)' '
 	git log --author=author --pretty=tformat:%s >actual &&
-	( echo third ; echo initial ) >expect &&
+	{
+		echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (2)' '
 	git log --author=" * " -F --pretty=tformat:%s >actual &&
-	( echo second ) >expect &&
+	{
+		echo second
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (3)' '
 	git log --author="^A U" --pretty=tformat:%s >actual &&
-	( echo third ; echo initial ) >expect &&
+	{
+		echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (4)' '
 	git log --author="frotz\.com>$" --pretty=tformat:%s >actual &&
-	( echo second ) >expect &&
+	{
+		echo second
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (5)' '
 	git log --author=Thor -F --pretty=tformat:%s >actual &&
-	( echo third ; echo initial ) >expect &&
+	{
+		echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 
@@ -540,7 +550,9 @@ test_expect_success 'log --grep --author implicitly uses all-match' '
 	# grep matches initial and second but not third
 	# author matches only initial and third
 	git log --author="A U Thor" --grep=s --grep=l --format=%s >actual &&
-	echo initial >expect &&
+	{
+		echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 

-- 
1.7.12.463.gbd9d638
Michael J Gruber· Sep 13, 2012, 14:04 UTC · re: Junio C Hamano · lore

[PATCHv2 2/6] t7810-grep: test multiple --grep with and without --all-match

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Show changes to t/t7810-grep.sh +16 −0
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 9b683ac..1db3dcb 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -546,6 +546,22 @@ test_expect_success 'log grep (6)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log with multiple --grep uses union' '
+	git log --grep=i --grep=r --format=%s >actual &&
+	{
+		echo fourth && echo third && echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --all-match with multiple --grep uses intersection' '
+	git log --all-match --grep=i --grep=r --format=%s >actual &&
+	{
+		echo third
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log --grep --author implicitly uses all-match' '
 	# grep matches initial and second but not third
 	# author matches only initial and third

-- 
1.7.12.463.gbd9d638
Michael J Gruber· Sep 13, 2012, 14:04 UTC · re: Junio C Hamano · lore

[PATCHv2 3/6] t7810-grep: test multiple --author with --all-match

--all-match is ignored for author matching on purpose.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/t7810-grep.sh +8 −0
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1db3dcb..9bc63a3 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -580,6 +580,14 @@ test_expect_success 'log with multiple --author uses union' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log --all-match with multiple --author still uses union' '
+	git log --all-match --author="Thor" --author="Aster" --format=%s >actual &&
+	{
+	    echo third && echo second && echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log with --grep and multiple --author uses all-match' '
 	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
 	{

-- 
1.7.12.463.gbd9d638
Junio C Hamano· Sep 13, 2012, 20:47 UTC · re: Michael J Gruber · lore

Re: [PATCHv2 3/6] t7810-grep: test multiple --author with --all-match

Michael J Gruber <git@drmicha.warpmail.net> writes:
> --all-match is ignored for author matching on purpose.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---

What the added test expects is correct, but I do not think the above description is correct. "all-match" is implicitly turned on when you use header match.

When you say
	git log --grep=Linus --grep=Junio
you will get
    (or
     pattern_body<body>Junio
     pattern_body<body>Linus
    )
but when you say
	git log --author=Linus --author=Junio
you will get
    [all-match]
    (or
     (or
      pattern_head<head 0>Linus
      pattern_head<head 0>Junio
     )
     true
    )

instead. Notice that there is one extra level of "OR" node, so that two OR nodes on the top-level backbone (think of these as cons cells with car and cdr) are "author is either Linus or junio" and "True". Because "all-match" is about rejecting a document as non-matching unless all the OR nodes on the top-level backbone have fired, this allows commit that is authored either by Linus or by Junio to match, and "on purpose" part in your message is correct.

But
	git log --author=Linus --author=Junio --grep=commit
will be parsed to
    [all-match]
    (or
     pattern_body<body>commit
     (or
      (or
       pattern_head<head 0>Linus
       pattern_head<head 0>Junio
      )
      true
     )
    )

to have three OR nodes on the backbone: "the log message must have commit", "authored by either Linus or Junio", and "true". All three must match somewhere in the "git cat-file commit" output for the commit to be considered a match (but obviously they do not have to match on the same line).

So what is giving commits made by Linus, even though it is not authored by Junio, with "--author=Linus --author=Junio" is not the lack of --all-match. In fact, --all-match is implicitly set for other things, so that the last example finds commits that mention "commit" authored by one of these two people. Commits that do mention "commit" but are done by other people are excluded. Commits that do not mention "commit" are excluded even if they were done by Linus or Junio.

	git log --committer=Linus --author=Junio
turns into
    [all-match]
    (or
     pattern_head<head 1>Linus
     (or
      pattern_head<head 0>Junio
      true
     )
    )

which has "committed by Linus", "authored by Junio" on the top-level backbone, so both has to be true for a commit to match.

Adding --grep=commit makes it
    [all-match]
    (or
     pattern_body<body>commit
     (or
      pattern_head<head 1>Linus
      (or
       pattern_head<head 0>Junio
       true
      )
     )
    )

which has "committed by Linus", "authored by Junio", "mentions commit" on the top-level, and all three has to be true.

	git log --committer=Linus --author=Junio --grep=commit --grep=tag

groups the "mentions commit" and "mentions tag" under a new top-level OR node, i.e.

    [all-match]
    (or
     (or
      pattern_body<body>commit
      pattern_body<body>tag
     )
     (or
      pattern_head<head 1>Linus
      (or
       pattern_head<head 0>Junio
       true
      )
     )
    )
so the top-level backbone "all-match" works on becomes
 - Mentions either commit or tag,
 - Committed by Linus,
 - Authored by Junio

One possible improvement we can make is to parse the command line in the last example with "--all-match" to

    [all-match]
    (or
     pattern_body<body>commit
     (or
      pattern_body<body>tag
      (or
       pattern_head<head 1>Linus
       (or
        pattern_head<head 0>Junio
        true
       )
      )
     )
    )
so that the backbone becomes
 - Mentions commit,
 - Mentions tag,
 - Committed by Linus,
 - Authored by Junio
to require that both commit and tag are mentioned in the message.
Show 22 quoted lines
>  t/t7810-grep.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 1db3dcb..9bc63a3 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -580,6 +580,14 @@ test_expect_success 'log with multiple --author uses union' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'log --all-match with multiple --author still uses union' '
> +	git log --all-match --author="Thor" --author="Aster" --format=%s >actual &&
> +	{
> +	    echo third && echo second && echo initial
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'log with --grep and multiple --author uses all-match' '
>  	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
>  	{
Junio C Hamano· Sep 13, 2012, 23:26 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 3/6] t7810-grep: test multiple --author with --all-match

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> One possible improvement we can make is to parse the command line in
> the last example with "--all-match" to
>
>     [all-match]
>     (or
>      pattern_body<body>commit
>      (or
>       pattern_body<body>tag
>       (or
>        pattern_head<head 1>Linus
>        (or
>         pattern_head<head 0>Junio
>         true
>        )
>       )
>      )
>     )
>
> so that the backbone becomes
>
>  - Mentions commit,
>  - Mentions tag,
>  - Committed by Linus,
>  - Authored by Junio
>
> to require that both commit and tag are mentioned in the message.

And this is an attempt to do exactly that. Earlier, when we have both header expression (which by the way has to be an OR node by construction) and pattern expression (which could be anything), we created a top-level OR node (again, look at this as if you are reading LISP),

           OR
        /        \
       /          \
   pattern            OR
     / \           /     \
    .....    committer    OR
                         /   \ 
                     author   TRUE

in other words, the three elements on the top-level backbone are "pattern", "committer" and "author"; when there are more than one elements in the "pattern", the top-level node of it is OR, so that node is inspected by "all-match", hence the result ends up ignoring the "--all-match" given from the command line.

This patch turns it into
	     OR
          /      \
         /         \
        /              OR
    committer        /    \
                 author    \
                           pattern

when "--all-match" was given from the command line, so that the "committer", "author" and elements on the top-level backbone in "pattern" form the top-level backbone of the resulting expression to be inspected by the "all-match" logic.

Does this pass the expect-failure test in your [PATCH 5/6]?
 grep.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Show changes to diff +19 −0
diff --git c/grep.c w/grep.c
index be15c47..925aa92 100644
--- c/grep.c
+++ w/grep.c
@@ -476,6 +476,22 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	return header_expr;
 }
 
+static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y)
+{
+	struct grep_expr *z = x;
+
+	while (x) {
+		assert(x->node == GREP_NODE_OR);
+		if (x->u.binary.right &&
+		    x->u.binary.right->node == GREP_NODE_TRUE) {
+			x->u.binary.right = y;
+			break;
+		}
+		x = x->u.binary.right;
+	}
+	return z;
+}
+
 static void compile_grep_patterns_real(struct grep_opt *opt)
 {
 	struct grep_pat *p;
@@ -510,6 +526,9 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
 
 	if (!opt->pattern_expression)
 		opt->pattern_expression = header_expr;
+	else if (opt->all_match)
+		opt->pattern_expression = grep_splice_or(header_expr,
+							 opt->pattern_expression);
 	else
 		opt->pattern_expression = grep_or_expr(opt->pattern_expression,
 						       header_expr);
Michael J Gruber· Sep 14, 2012, 08:14 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 3/6] t7810-grep: test multiple --author with --all-match

Junio C Hamano venit, vidit, dixit 14.09.2012 01:26:
Show 66 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> One possible improvement we can make is to parse the command line in
>> the last example with "--all-match" to
>>
>>     [all-match]
>>     (or
>>      pattern_body<body>commit
>>      (or
>>       pattern_body<body>tag
>>       (or
>>        pattern_head<head 1>Linus
>>        (or
>>         pattern_head<head 0>Junio
>>         true
>>        )
>>       )
>>      )
>>     )
>>
>> so that the backbone becomes
>>
>>  - Mentions commit,
>>  - Mentions tag,
>>  - Committed by Linus,
>>  - Authored by Junio
>>
>> to require that both commit and tag are mentioned in the message.
> 
> And this is an attempt to do exactly that.  Earlier, when we have
> both header expression (which by the way has to be an OR node by
> construction) and pattern expression (which could be anything), we
> created a top-level OR node (again, look at this as if you are
> reading LISP),
> 
>            OR
>         /        \
>        /          \
>    pattern            OR
>      / \           /     \
>     .....    committer    OR
>                          /   \ 
>                      author   TRUE
> 
> in other words, the three elements on the top-level backbone are
> "pattern", "committer" and "author"; when there are more than one
> elements in the "pattern", the top-level node of it is OR, so that
> node is inspected by "all-match", hence the result ends up ignoring
> the "--all-match" given from the command line.
> 
> This patch turns it into
> 
> 	     OR
>           /      \
>          /         \
>         /              OR
>     committer        /    \
>                  author    \
>                            pattern
> 
> when "--all-match" was given from the command line, so that the
> "committer", "author" and elements on the top-level backbone in
> "pattern" form the top-level backbone of the resulting expression to
> be inspected by the "all-match" logic.
> 
> Does this pass the expect-failure test in your [PATCH 5/6]?
Just a quick heads up:

I merged 38ed8ef (log --grep/--author: honor --all-match honored for multiple --grep patterns, 2012-09-13) from pu into my test branch, and this fixes what I had marked as known failure there. Thanks!

[I still have to figure out the logic, but begin to understand that "(OR...) and "(AND...)" are linewise, and all-match is a bufferwise AND or something. Now, what is "*or*" ...]

Show 41 quoted lines
>  grep.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git c/grep.c w/grep.c
> index be15c47..925aa92 100644
> --- c/grep.c
> +++ w/grep.c
> @@ -476,6 +476,22 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
>  	return header_expr;
>  }
>  
> +static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y)
> +{
> +	struct grep_expr *z = x;
> +
> +	while (x) {
> +		assert(x->node == GREP_NODE_OR);
> +		if (x->u.binary.right &&
> +		    x->u.binary.right->node == GREP_NODE_TRUE) {
> +			x->u.binary.right = y;
> +			break;
> +		}
> +		x = x->u.binary.right;
> +	}
> +	return z;
> +}
> +
>  static void compile_grep_patterns_real(struct grep_opt *opt)
>  {
>  	struct grep_pat *p;
> @@ -510,6 +526,9 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
>  
>  	if (!opt->pattern_expression)
>  		opt->pattern_expression = header_expr;
> +	else if (opt->all_match)
> +		opt->pattern_expression = grep_splice_or(header_expr,
> +							 opt->pattern_expression);
>  	else
>  		opt->pattern_expression = grep_or_expr(opt->pattern_expression,
>  						       header_expr);
>
Junio C Hamano· Sep 14, 2012, 15:55 UTC · re: Michael J Gruber · lore

Re: [PATCHv2 3/6] t7810-grep: test multiple --author with --all-match

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 14 quoted lines
> Junio C Hamano venit, vidit, dixit 14.09.2012 01:26:
> ...
>> when "--all-match" was given from the command line, so that the
>> "committer", "author" and elements on the top-level backbone in
>> "pattern" form the top-level backbone of the resulting expression to
>> be inspected by the "all-match" logic.
>> 
>> Does this pass the expect-failure test in your [PATCH 5/6]?
>
> Just a quick heads up:
>
> I merged 38ed8ef (log --grep/--author: honor --all-match honored for
> multiple --grep patterns, 2012-09-13) from pu into my test branch,
> and this fixes what I had marked as known failure there. Thanks!
Thanks for testing.
>
> I still have to figure out the logic, but begin to understand that
> "(OR...) and "(AND...)" are linewise, and all-match is a bufferwise AND

Yes, all-match is about requiring all the top-level nodes to have fired while inspecting the whole file. So between these:

    $ git grep -e foo --and -e bar
    $ git grep --all-match -e foo -e bar

the former shows lines that has both foo and bar, while the latter shows lines that has either foo or bar but only from files that has both in them (on possibly separate lines).

> Now, what is "*or*" ...

That is for to show the token we received from the command line parser; the more interesting part is the parsed expression tree, that is mostly formed as a tree with each node labeled with what kind of operation it represents.

Michael J Gruber· Sep 13, 2012, 14:04 UTC · re: Junio C Hamano · lore

[PATCHv2 4/6] t7810-grep: test interaction of multiple --grep and --author options

There are tests for this interaction already. Restructure slightly and avoid any claims about --all-match.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
Show changes to t/t7810-grep.sh +22 −16
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 9bc63a3..f98f3f6 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -562,16 +562,6 @@ test_expect_success 'log --all-match with multiple --grep uses intersection' '
 	test_cmp expect actual
 '
 
-test_expect_success 'log --grep --author implicitly uses all-match' '
-	# grep matches initial and second but not third
-	# author matches only initial and third
-	git log --author="A U Thor" --grep=s --grep=l --format=%s >actual &&
-	{
-		echo initial
-	} >expect &&
-	test_cmp expect actual
-'
-
 test_expect_success 'log with multiple --author uses union' '
 	git log --author="Thor" --author="Aster" --format=%s >actual &&
 	{
@@ -588,17 +578,33 @@ test_expect_success 'log --all-match with multiple --author still uses union' '
 	test_cmp expect actual
 '
 
-test_expect_success 'log with --grep and multiple --author uses all-match' '
-	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
+test_expect_success 'log --grep --author uses intersection' '
+	# grep matches only third and fourth
+	# author matches only initial and third
+	git log --author="A U Thor" --grep=r --format=%s >actual &&
 	{
-	    echo third && echo initial
+		echo third
 	} >expect &&
 	test_cmp expect actual
 '
 
-test_expect_success 'log with --grep and multiple --author uses all-match' '
-	git log --author="Thor" --author="Night" --grep=q --format=%s >actual &&
-	>expect &&
+test_expect_success 'log --grep --grep --author takes union of greps and intersects with author' '
+	# grep matches initial and second but not third
+	# author matches only initial and third
+	git log --author="A U Thor" --grep=s --grep=l --format=%s >actual &&
+	{
+		echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --grep --author --author takes union of authors and intersects with grep' '
+	# grep matches only initial and third
+	# author matches all but second
+	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
+	{
+	    echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 

-- 
1.7.12.463.gbd9d638
Michael J Gruber· Sep 13, 2012, 14:04 UTC · re: Junio C Hamano · lore

[PATCHv2 5/6] t7810-grep: test --all-match with multiple --grep and --author options

--all-match is ignored with multiple author options on purpose. It is also ignored with multiple --grep options when an --author option is present. Mark this as known failure.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
Show changes to t/t7810-grep.sh +20 −0
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index f98f3f6..fa2845f 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -598,6 +598,16 @@ test_expect_success 'log --grep --grep --author takes union of greps and interse
 	test_cmp expect actual
 '
 
+test_expect_success 'log ---all-match -grep --author --author still takes union of authors and intersects with grep' '
+	# grep matches only initial and third
+	# author matches all but second
+	git log --all-match --author="Thor" --author="Night" --grep=i --format=%s >actual &&
+	{
+	    echo third && echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log --grep --author --author takes union of authors and intersects with grep' '
 	# grep matches only initial and third
 	# author matches all but second
@@ -608,6 +618,16 @@ test_expect_success 'log --grep --author --author takes union of authors and int
 	test_cmp expect actual
 '
 
+test_expect_failure 'log --all-match --grep --grep --author takes intersection' '
+	# grep matches only third
+	# author matches only initial and third
+	git log --all-match --author="A U Thor" --grep=i --grep=r --format=%s >actual &&
+	{
+		echo third
+	} >expect &&
+	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.463.gbd9d638
Michael J Gruber· Sep 13, 2012, 14:04 UTC · re: Junio C Hamano · lore

[PATCHv2 6/6] rev-list/log: document logic with several limiting options

The current behavior is probably as useful as it is confusing. In any case it is going to stay. So, document it.

This does not take into account the issue of 'log --all-match --author=me --grep=foo --grep=bar' not honoring '--all-match' because it is hopefully a corner case (and, even more hopefully, fixed some time soon).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/rev-list-options.txt | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to Documentation/rev-list-options.txt +14 −1
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5436eba..b2dbfb5 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -6,6 +6,19 @@ special notations explained in the description, additional commit
 limiting may be applied. Note that they are applied before commit
 ordering and formatting options, such as '--reverse'.
 
+Different options are ANDed: '--author=bar --grep=foo'
+limits to commits which match both conditions.
+
+Several occurences of the '--grep' option are ORed: '--grep=foo --grep=bar'
+limits to commits matching any of these conditions.
+(If '--all-match' is given, the conditions are ANDed.)
+
+Several occurences of the '--author' and '--committer' options are ORed
+(because there can be only one each per commit).
+
+For all other options, only the last occurence of the same option is
+taken into account: '-n 5 -3' is '-3' and '-n 3 -n 5' is '-n 5'.
+
 --
 
 -n 'number'::
@@ -47,7 +60,7 @@ endif::git-rev-list[]
 
 --all-match::
 	Limit the commits output to ones that match all given --grep,
-	--author and --committer instead of ones that match at least one.
+	instead of ones that match at least one.
 
 -i::
 --regexp-ignore-case::

-- 
1.7.12.463.gbd9d638
Junio C Hamano· Sep 13, 2012, 22:29 UTC · re: Michael J Gruber · lore

Re: [PATCHv2 6/6] rev-list/log: document logic with several limiting options

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 30 quoted lines
> The current behavior is probably as useful as it is confusing. In any
> case it is going to stay. So, document it.
>
> This does not take into account the issue of 'log --all-match
> --author=me --grep=foo --grep=bar' not honoring '--all-match' because it
> is hopefully a corner case (and, even more hopefully, fixed some time
> soon).
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/rev-list-options.txt | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 5436eba..b2dbfb5 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -6,6 +6,19 @@ special notations explained in the description, additional commit
>  limiting may be applied. Note that they are applied before commit
>  ordering and formatting options, such as '--reverse'.
>  
> +Different options are ANDed: '--author=bar --grep=foo'
> +limits to commits which match both conditions.
> +
> +Several occurences of the '--grep' option are ORed: '--grep=foo --grep=bar'
> +limits to commits matching any of these conditions.
> +(If '--all-match' is given, the conditions are ANDed.)
> +
> +Several occurences of the '--author' and '--committer' options are ORed
> +(because there can be only one each per commit).

As I would really want to eventually see the revision command option parser understand the full power of grep expressions in the future, I would really want to avoid a misleading explanation that calls what "--all-match" does as "ANDed".

With such a change, we could say something like
	git log --grep=commit --and --grep=count

to require the log message to have both "commit" and "count" on the same line (in any order). This obviously is different from

	git log --grep="commit.*count"
but more importantly, it is vastly different from
	git log --all-match --grep=commit --grep=count

that requires some line that has "commit", and some line (which may not be the same line) that has "count", in the log message.

Junio C Hamano· Sep 14, 2012, 01:19 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 6/6] rev-list/log: document logic with several limiting options

Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
>> index 5436eba..b2dbfb5 100644
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -6,6 +6,19 @@ special notations explained in the description, additional commit
>>  limiting may be applied. Note that they are applied before commit
>>  ordering and formatting options, such as '--reverse'.
>>  
>> +Different options are ANDed: '--author=bar --grep=foo'
>> +limits to commits which match both conditions.
>> +
>> +Several occurences of the '--grep' option are ORed: '--grep=foo --grep=bar'
>> +limits to commits matching any of these conditions.
>> +(If '--all-match' is given, the conditions are ANDed.)
>> +
>> +Several occurences of the '--author' and '--committer' options are ORed
>> +(because there can be only one each per commit).
>
> As I would really want to eventually see the revision command option
> parser understand the full power of grep expressions in the future,
> I would really want to avoid a misleading explanation that calls
> what "--all-match" does as "ANDed".

Assuming that the patch I posted earlier actually works, I think the description can become vastly simpler, if you stop explaining author and committer in terms of "grep". It is implementation detail that the same grep machinery is handling these two header fields and the end users do not have to even be aware of.

	You can use "--grep=foo" to limit the output to commits that
	mention "foo" in their messages.  If you use more than one
	of them, e.g. "--grep=foo" and "--grep=bar", by default, the
	command shows commits that mention "foo" or "bar" (or
	naturally, both) in their messages.  If you want to limit
	the output to commits that mention both "foo" and "bar" in
	their messages (note that they do not have to appear on the
	same line), you can use "--all-match".
	You can use "--author=Linus" to limit the output to commits
	authored by Linus.  If you use more than one of them,
	e.g. "--author=Linus" and "--author=Junio", the command
	shows commits that are authored by either Linus or Junio.
	As a commit cannot be authored by more than one person,
	there is no option to choose commits that are authored by
	Linus and Junio (and there is no need for such an option).
	To limit the output with the committer information, use
	"--committer=<person>" instead of "--author=<person>".
Junio C Hamano· Sep 14, 2012, 02:04 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 6/6] rev-list/log: document logic with several limiting options

Junio C Hamano <gitster@pobox.com> writes:
> Assuming that the patch I posted earlier actually works, I think the
> description can become vastly simpler, if you stop explaining author
> and committer in terms of "grep".
And here is a replacement in a patch form.  
-- >8 --
Subject: [PATCH] log: document use of multiple commit limiting options

Generally speaking, using more options will further narrow the selection, but there are a few exceptions. Document them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/rev-list-options.txt | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
Show changes to Documentation/rev-list-options.txt +17 −4
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1ae3c89..57d6c90 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -3,7 +3,14 @@ Commit Limiting
 
 Besides specifying a range of commits that should be listed using the
 special notations explained in the description, additional commit
-limiting may be applied. Note that they are applied before commit
+limiting may be applied.
+
+Using more options generally further limits the output (e.g.
+"--since=<date1>" limits to commits newer than <date1>, and using it
+with "--grep=<pattern>" further limits to commits whose log message
+has a line that match <pattern>), unless otherwise noted.
+
+Note that these are applied before commit
 ordering and formatting options, such as '--reverse'.
 
 --
@@ -38,16 +45,22 @@ endif::git-rev-list[]
 --committer=<pattern>::
 
 	Limit the commits output to ones with author/committer
-	header lines that match the specified pattern (regular expression).
+	header lines that match the specified pattern (regular
+	expression).  With more than one `--author=<pattern>`,
+	commits whose author match any of the given patterns are
+	chosen (similarly for multiple `--committer=<pattern>`).
 
 --grep=<pattern>::
 
 	Limit the commits output to ones with log message that
-	matches the specified pattern (regular expression).
+	matches the specified pattern (regular expression).  With
+	more than one `--grep=<pattern>`, commits whose message
+	match any of the given patterns are chosen (but see
+	`--all-match`).
 
 --all-match::
 	Limit the commits output to ones that match all given --grep,
-	--author and --committer instead of ones that match at least one.
+	instead of ones that match at least one.
 
 -i::
 --regexp-ignore-case::

-- 
1.7.12.469.g5235eb6
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 00/11] rev-list/log: document logic with several limiting options

In v3 I've rebased my tests on top of Junio's commits which change --all-match handling and provide a better documentation. Accordingly, 6/6 from v2 is dropped and 5/5 adjusted (expect_failure -> expect_success).

I'm sending out Junio's commits as in pu along with 3 intermixed fixups which you may squash in freely. (I've only marked the more obvious one as fixup!.) The last 5 are my rebased tests as described above.

[This approach seemed to be more convenient; hopefully for everyone ;)]
Junio C Hamano (3):
  grep: teach --debug option to dump the parse tree
  log --grep/--author: honor --all-match honored for multiple --grep
    patterns
  log: document use of multiple commit limiting options
Michael J Gruber (8):
  log: name --debug-grep option like in the commit message
  grep: show --debug output only once
  fixup! log: document use of multiple commit limiting options
  t7810-grep: bring log --grep tests in common form
  t7810-grep: test multiple --grep with and without --all-match
  t7810-grep: test multiple --author with --all-match
  t7810-grep: test interaction of multiple --grep and --author options
  t7810-grep: test --all-match with multiple --grep and --author
    options
 Documentation/rev-list-options.txt |  23 ++++++--
 builtin/grep.c                     |   4 ++
 grep.c                             | 111 ++++++++++++++++++++++++++++++++++++-
 grep.h                             |   1 +
 revision.c                         |   2 +
 t/t7810-grep.sh                    |  90 +++++++++++++++++++++++++-----
 6 files changed, 210 insertions(+), 21 deletions(-)
-- 
1.7.12.592.g41e7905
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 01/11] grep: teach --debug option to dump the parse tree

From: Junio C Hamano <gitster@pobox.com>

Our "grep" allows complex boolean expressions to be formed to match each individual line with operators like --and, '(', ')' and --not. Introduce the "--debug" option to show the parse tree to help people who want to debug and enhance it.

Also "log" learns "--debug-grep" option to do the same. The command line parser to the log family is a lot more limited than the general "git grep" parser, but it has special handling for header matching (e.g. "--author"), and a parse tree is valuable when working on it.

Note that "--all-match" is *not* any individual node in the parse tree. It is an instruction to the evaluator to check all the nodes in the top-level backbone have matched and reject a document as non-matching otherwise.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/grep.c |  3 ++
 grep.c         | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 grep.h         |  1 +
 revision.c     |  2 ++
 4 files changed, 96 insertions(+), 2 deletions(-)
Show changes to 4 files +96 −2

builtin/grep.c, grep.c, grep.h, revision.c

diff --git a/builtin/grep.c b/builtin/grep.c
index fe1726f..8aea00c 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -772,6 +772,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			   "indicate hit with exit status without output"),
 		OPT_BOOLEAN(0, "all-match", &opt.all_match,
 			"show only matches from files that match all patterns"),
+		{ OPTION_SET_INT, 0, "debug", &opt.debug, NULL,
+		  "show parse tree for grep expression",
+		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1 },
 		OPT_GROUP(""),
 		{ OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
 			"pager", "show matching files in the pager",
diff --git a/grep.c b/grep.c
index 04e3ec6..be15c47 100644
--- a/grep.c
+++ b/grep.c
@@ -332,6 +332,87 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)
 	return compile_pattern_or(list);
 }
 
+static void indent(int in)
+{
+	while (in-- > 0)
+		fputc(' ', stderr);
+}
+
+static void dump_grep_pat(struct grep_pat *p)
+{
+	switch (p->token) {
+	case GREP_AND: fprintf(stderr, "*and*"); break;
+	case GREP_OPEN_PAREN: fprintf(stderr, "*(*"); break;
+	case GREP_CLOSE_PAREN: fprintf(stderr, "*)*"); break;
+	case GREP_NOT: fprintf(stderr, "*not*"); break;
+	case GREP_OR: fprintf(stderr, "*or*"); break;
+
+	case GREP_PATTERN: fprintf(stderr, "pattern"); break;
+	case GREP_PATTERN_HEAD: fprintf(stderr, "pattern_head"); break;
+	case GREP_PATTERN_BODY: fprintf(stderr, "pattern_body"); break;
+	}
+
+	switch (p->token) {
+	default: break;
+	case GREP_PATTERN_HEAD:
+		fprintf(stderr, "<head %d>", p->field); break;
+	case GREP_PATTERN_BODY:
+		fprintf(stderr, "<body>"); break;
+	}
+	switch (p->token) {
+	default: break;
+	case GREP_PATTERN_HEAD:
+	case GREP_PATTERN_BODY:
+	case GREP_PATTERN:
+		fprintf(stderr, "%.*s", (int)p->patternlen, p->pattern);
+		break;
+	}
+	fputc('\n', stderr);
+}
+
+static void dump_grep_expression_1(struct grep_expr *x, int in)
+{
+	indent(in);
+	switch (x->node) {
+	case GREP_NODE_TRUE:
+		fprintf(stderr, "true\n");
+		break;
+	case GREP_NODE_ATOM:
+		dump_grep_pat(x->u.atom);
+		break;
+	case GREP_NODE_NOT:
+		fprintf(stderr, "(not\n");
+		dump_grep_expression_1(x->u.unary, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	case GREP_NODE_AND:
+		fprintf(stderr, "(and\n");
+		dump_grep_expression_1(x->u.binary.left, in+1);
+		dump_grep_expression_1(x->u.binary.right, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	case GREP_NODE_OR:
+		fprintf(stderr, "(or\n");
+		dump_grep_expression_1(x->u.binary.left, in+1);
+		dump_grep_expression_1(x->u.binary.right, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	}
+}
+
+void dump_grep_expression(struct grep_opt *opt)
+{
+	struct grep_expr *x = opt->pattern_expression;
+
+	if (opt->all_match)
+		fprintf(stderr, "[all-match]\n");
+	dump_grep_expression_1(x, 0);
+	fflush(NULL);
+}
+
 static struct grep_expr *grep_true_expr(void)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
@@ -395,7 +476,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	return header_expr;
 }
 
-void compile_grep_patterns(struct grep_opt *opt)
+static void compile_grep_patterns_real(struct grep_opt *opt)
 {
 	struct grep_pat *p;
 	struct grep_expr *header_expr = prep_header_patterns(opt);
@@ -415,7 +496,7 @@ void compile_grep_patterns(struct grep_opt *opt)
 
 	if (opt->all_match || header_expr)
 		opt->extended = 1;
-	else if (!opt->extended)
+	else if (!opt->extended && !opt->debug)
 		return;
 
 	p = opt->pattern_list;
@@ -435,6 +516,13 @@ void compile_grep_patterns(struct grep_opt *opt)
 	opt->all_match = 1;
 }
 
+void compile_grep_patterns(struct grep_opt *opt)
+{
+	compile_grep_patterns_real(opt);
+	if (opt->debug)
+		dump_grep_expression(opt);
+}
+
 static void free_pattern_expr(struct grep_expr *x)
 {
 	switch (x->node) {
diff --git a/grep.h b/grep.h
index ed7de6b..bf5be5a 100644
--- a/grep.h
+++ b/grep.h
@@ -90,6 +90,7 @@ struct grep_opt {
 	int word_regexp;
 	int fixed;
 	int all_match;
+	int debug;
 #define GREP_BINARY_DEFAULT	0
 #define GREP_BINARY_NOMATCH	1
 #define GREP_BINARY_TEXT	2
diff --git a/revision.c b/revision.c
index 9a0d9c7..90376e8 100644
--- a/revision.c
+++ b/revision.c
@@ -1578,6 +1578,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
 		return argcount;
+	} else if (!strcmp(arg, "--grep-debug")) {
+		revs->grep_filter.debug = 1;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
 		revs->grep_filter.regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {

-- 
1.7.12.592.g41e7905
Junio C Hamano· Sep 14, 2012, 17:09 UTC · re: Michael J Gruber · lore

Re: [PATCHv3 01/11] grep: teach --debug option to dump the parse tree

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 8 quoted lines
> From: Junio C Hamano <gitster@pobox.com>
>
> Our "grep" allows complex boolean expressions to be formed to match
> each individual line with operators like --and, '(', ')' and --not.
> Introduce the "--debug" option to show the parse tree to help people
> who want to debug and enhance it.
>
> Also "log" learns "--debug-grep" option to do the same.  The command
This was actually a leftover incorrect message.
When we introduce the equivalent of
    $ git grep -e foo --and --not \( -e bar -e baz \)

to the log family in some distant future, we may end up having to disambiguate the "--not" (which means something completely different for the log family) meant for the grep part by doing

    $ git log --grep=foo --grep-and --grep-not \
    	--grep-\( --grep=bar --grep=baz --grep-\)

or something like that, and the code says --grep-debug because that would fit better than --debug-grep.

Other than that, the patch looks good ;-)
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 02/11] log: name --debug-grep option like in the commit message

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 revision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to revision.c +1 −1
diff --git a/revision.c b/revision.c
index 90376e8..fad8040 100644
--- a/revision.c
+++ b/revision.c
@@ -1578,7 +1578,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
 		return argcount;
-	} else if (!strcmp(arg, "--grep-debug")) {
+	} else if (!strcmp(arg, "--debug-grep")) {
 		revs->grep_filter.debug = 1;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
 		revs->grep_filter.regflags |= REG_EXTENDED;

-- 
1.7.12.592.g41e7905
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 03/11] grep: show --debug output only once

When threaded grep is in effect, the patterns are duplicated and recompiled for each thread. Avoid "--debug" output during the recompilation so that the output is given once instead of "1+nthreads" times.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/grep.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to builtin/grep.c +1 −0
diff --git a/builtin/grep.c b/builtin/grep.c
index 8aea00c..a7e8df0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -209,6 +209,7 @@ static void start_threads(struct grep_opt *opt)
 		int err;
 		struct grep_opt *o = grep_opt_dup(opt);
 		o->output = strbuf_out;
+		o->debug = 0;
 		compile_grep_patterns(o);
 		err = pthread_create(&threads[i], NULL, run, o);
 

-- 
1.7.12.592.g41e7905
Junio C Hamano· Sep 14, 2012, 17:11 UTC · re: Michael J Gruber · lore

Re: [PATCHv3 03/11] grep: show --debug output only once

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 21 quoted lines
> When threaded grep is in effect, the patterns are duplicated and
> recompiled for each thread. Avoid "--debug" output during the
> recompilation so that the output is given once instead of "1+nthreads"
> times.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  builtin/grep.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8aea00c..a7e8df0 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -209,6 +209,7 @@ static void start_threads(struct grep_opt *opt)
>  		int err;
>  		struct grep_opt *o = grep_opt_dup(opt);
>  		o->output = strbuf_out;
> +		o->debug = 0;
>  		compile_grep_patterns(o);
>  		err = pthread_create(&threads[i], NULL, run, o);
Makes sense; thanks.
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 04/11] log --grep/--author: honor --all-match honored for multiple --grep patterns

From: Junio C Hamano <gitster@pobox.com>

Earlier, when we have both header expression (which has to be an OR node by construction) and a pattern expression (which could be anything), we created a top-level OR node that looks like this to bind them together:

             OR
        /          \
       /            \
   pattern            OR
     / \           /     \
    .....    committer    OR
                         /   \
                     author   TRUE

In other words, the three elements on the top-level backbone that were inspected by the "all-match" logic are "pattern", "committer" and "author". When there are more than one elements in the "pattern", the top-level node of it is that OR, so that node is inspected by "all-match", hence the result ends up ignoring the "--all-match" given from the command line. A match on either side of the pattern was considered a match, hence

        git log --grep=A --grep=B --author=C --all-match

showed the same "authored by C and has either A or B" with or without --all-match.

This patch turns the grep expression into this form:
              OR
          /       \
         /          \
        /              OR
    committer        /    \
                 author    \
                           pattern

when "--all-match" was given from the command line. This way, the set of nodes on the top-level backbone in the resulting expression consists of "committer", "author", and whatever nodes were on the top-level backbone of the "pattern" expression. The "all-match" logic inspects the same nodes in "pattern" as the case without the author and/or the committer restriction.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 grep.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Show changes to grep.c +19 −0
diff --git a/grep.c b/grep.c
index be15c47..925aa92 100644
--- a/grep.c
+++ b/grep.c
@@ -476,6 +476,22 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	return header_expr;
 }
 
+static struct grep_expr *grep_splice_or(struct grep_expr *x, struct grep_expr *y)
+{
+	struct grep_expr *z = x;
+
+	while (x) {
+		assert(x->node == GREP_NODE_OR);
+		if (x->u.binary.right &&
+		    x->u.binary.right->node == GREP_NODE_TRUE) {
+			x->u.binary.right = y;
+			break;
+		}
+		x = x->u.binary.right;
+	}
+	return z;
+}
+
 static void compile_grep_patterns_real(struct grep_opt *opt)
 {
 	struct grep_pat *p;
@@ -510,6 +526,9 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
 
 	if (!opt->pattern_expression)
 		opt->pattern_expression = header_expr;
+	else if (opt->all_match)
+		opt->pattern_expression = grep_splice_or(header_expr,
+							 opt->pattern_expression);
 	else
 		opt->pattern_expression = grep_or_expr(opt->pattern_expression,
 						       header_expr);

-- 
1.7.12.592.g41e7905
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 05/11] log: document use of multiple commit limiting options

From: Junio C Hamano <gitster@pobox.com>

Generally speaking, using more options will further narrow the selection, but there are a few exceptions. Document them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/rev-list-options.txt | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
Show changes to Documentation/rev-list-options.txt +17 −4
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1ae3c89..57d6c90 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -3,7 +3,14 @@ Commit Limiting
 
 Besides specifying a range of commits that should be listed using the
 special notations explained in the description, additional commit
-limiting may be applied. Note that they are applied before commit
+limiting may be applied.
+
+Using more options generally further limits the output (e.g.
+"--since=<date1>" limits to commits newer than <date1>, and using it
+with "--grep=<pattern>" further limits to commits whose log message
+has a line that match <pattern>), unless otherwise noted.
+
+Note that these are applied before commit
 ordering and formatting options, such as '--reverse'.
 
 --
@@ -38,16 +45,22 @@ endif::git-rev-list[]
 --committer=<pattern>::
 
 	Limit the commits output to ones with author/committer
-	header lines that match the specified pattern (regular expression).
+	header lines that match the specified pattern (regular
+	expression).  With more than one `--author=<pattern>`,
+	commits whose author match any of the given patterns are
+	chosen (similarly for multiple `--committer=<pattern>`).
 
 --grep=<pattern>::
 
 	Limit the commits output to ones with log message that
-	matches the specified pattern (regular expression).
+	matches the specified pattern (regular expression).  With
+	more than one `--grep=<pattern>`, commits whose message
+	match any of the given patterns are chosen (but see
+	`--all-match`).
 
 --all-match::
 	Limit the commits output to ones that match all given --grep,
-	--author and --committer instead of ones that match at least one.
+	instead of ones that match at least one.
 
 -i::
 --regexp-ignore-case::

-- 
1.7.12.592.g41e7905
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 06/11] fixup! log: document use of multiple commit limiting options

Here are a few typo fixes.

There is a mix of single and back ticks already before this patch, i.e. ` vs. ' -- I thought we had guidelines for this but don't find them at the moment.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/rev-list-options.txt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to Documentation/rev-list-options.txt +6 −6
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 57d6c90..c828408 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -6,12 +6,12 @@ special notations explained in the description, additional commit
 limiting may be applied.
 
 Using more options generally further limits the output (e.g.
-"--since=<date1>" limits to commits newer than <date1>, and using it
-with "--grep=<pattern>" further limits to commits whose log message
-has a line that match <pattern>), unless otherwise noted.
+`--since=<date1>` limits to commits newer than `<date1>`, and using it
+with `--grep=<pattern>` further limits to commits whose log message
+has a line that matches `<pattern>`), unless otherwise noted.
 
 Note that these are applied before commit
-ordering and formatting options, such as '--reverse'.
+ordering and formatting options, such as `--reverse`.
 
 --
 
@@ -47,7 +47,7 @@ endif::git-rev-list[]
 	Limit the commits output to ones with author/committer
 	header lines that match the specified pattern (regular
 	expression).  With more than one `--author=<pattern>`,
-	commits whose author match any of the given patterns are
+	commits whose author matches any of the given patterns are
 	chosen (similarly for multiple `--committer=<pattern>`).
 
 --grep=<pattern>::
@@ -55,7 +55,7 @@ endif::git-rev-list[]
 	Limit the commits output to ones with log message that
 	matches the specified pattern (regular expression).  With
 	more than one `--grep=<pattern>`, commits whose message
-	match any of the given patterns are chosen (but see
+	matches any of the given patterns are chosen (but see
 	`--all-match`).
 
 --all-match::

-- 
1.7.12.592.g41e7905
Junio C Hamano· Sep 14, 2012, 17:23 UTC · re: Michael J Gruber · lore

Re: [PATCHv3 06/11] fixup! log: document use of multiple commit limiting options

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 10 quoted lines
> Here are a few typo fixes.
>
> There is a mix of single and back ticks already before this patch,
> i.e. ` vs. ' -- I thought we had guidelines for this but don't find them
> at the moment.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/rev-list-options.txt | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
Thanks, will squash in.
In general:
 - we want to use `exactly this` when writing an item that the user
   has to exactly spell as in the text, e.g. subcommand names.
 - from the older days, we use <angle bracket as placeholder> in
   synopsis section and in "git subcmd -h" output, and in the body
   text, we tend to write like '<this>' to italicise in the
   documentation.

I personally find the <angle> somewhat ugly in the documentation, but we cannot just drop them as the "man -Tascii" would suffer if we did so.

Show 40 quoted lines
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 57d6c90..c828408 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -6,12 +6,12 @@ special notations explained in the description, additional commit
>  limiting may be applied.
>  
>  Using more options generally further limits the output (e.g.
> -"--since=<date1>" limits to commits newer than <date1>, and using it
> -with "--grep=<pattern>" further limits to commits whose log message
> -has a line that match <pattern>), unless otherwise noted.
> +`--since=<date1>` limits to commits newer than `<date1>`, and using it
> +with `--grep=<pattern>` further limits to commits whose log message
> +has a line that matches `<pattern>`), unless otherwise noted.
>  
>  Note that these are applied before commit
> -ordering and formatting options, such as '--reverse'.
> +ordering and formatting options, such as `--reverse`.
>  
>  --
>  
> @@ -47,7 +47,7 @@ endif::git-rev-list[]
>  	Limit the commits output to ones with author/committer
>  	header lines that match the specified pattern (regular
>  	expression).  With more than one `--author=<pattern>`,
> -	commits whose author match any of the given patterns are
> +	commits whose author matches any of the given patterns are
>  	chosen (similarly for multiple `--committer=<pattern>`).
>  
>  --grep=<pattern>::
> @@ -55,7 +55,7 @@ endif::git-rev-list[]
>  	Limit the commits output to ones with log message that
>  	matches the specified pattern (regular expression).  With
>  	more than one `--grep=<pattern>`, commits whose message
> -	match any of the given patterns are chosen (but see
> +	matches any of the given patterns are chosen (but see
>  	`--all-match`).
>  
>  --all-match::
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 07/11] t7810-grep: bring log --grep tests in common form

The log --grep tests generate the expected out in different ways. Make them all use command blocks so that subshells are avoided and the expected output is easier to grasp visually.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
Show changes to t/t7810-grep.sh +18 −6
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 24e9b19..180e998 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -435,31 +435,41 @@ test_expect_success 'log grep setup' '
 
 test_expect_success 'log grep (1)' '
 	git log --author=author --pretty=tformat:%s >actual &&
-	( echo third ; echo initial ) >expect &&
+	{
+		echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (2)' '
 	git log --author=" * " -F --pretty=tformat:%s >actual &&
-	( echo second ) >expect &&
+	{
+		echo second
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (3)' '
 	git log --author="^A U" --pretty=tformat:%s >actual &&
-	( echo third ; echo initial ) >expect &&
+	{
+		echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (4)' '
 	git log --author="frotz\.com>$" --pretty=tformat:%s >actual &&
-	( echo second ) >expect &&
+	{
+		echo second
+	} >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'log grep (5)' '
 	git log --author=Thor -F --pretty=tformat:%s >actual &&
-	( echo third ; echo initial ) >expect &&
+	{
+		echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 
@@ -473,7 +483,9 @@ test_expect_success 'log --grep --author implicitly uses all-match' '
 	# grep matches initial and second but not third
 	# author matches only initial and third
 	git log --author="A U Thor" --grep=s --grep=l --format=%s >actual &&
-	echo initial >expect &&
+	{
+		echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 

-- 
1.7.12.592.g41e7905
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 08/11] t7810-grep: test multiple --grep with and without --all-match

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Show changes to t/t7810-grep.sh +16 −0
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 180e998..b841909 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -479,6 +479,22 @@ test_expect_success 'log grep (6)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log with multiple --grep uses union' '
+	git log --grep=i --grep=r --format=%s >actual &&
+	{
+		echo fourth && echo third && echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --all-match with multiple --grep uses intersection' '
+	git log --all-match --grep=i --grep=r --format=%s >actual &&
+	{
+		echo third
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log --grep --author implicitly uses all-match' '
 	# grep matches initial and second but not third
 	# author matches only initial and third

-- 
1.7.12.592.g41e7905
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 09/11] t7810-grep: test multiple --author with --all-match

--all-match is ignored for author matching on purpose.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/t7810-grep.sh +8 −0
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index b841909..be81d96 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -513,6 +513,14 @@ test_expect_success 'log with multiple --author uses union' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log --all-match with multiple --author still uses union' '
+	git log --all-match --author="Thor" --author="Aster" --format=%s >actual &&
+	{
+	    echo third && echo second && echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log with --grep and multiple --author uses all-match' '
 	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
 	{

-- 
1.7.12.592.g41e7905
Junio C Hamano· Sep 14, 2012, 17:58 UTC · re: Michael J Gruber · lore

Re: [PATCHv3 09/11] t7810-grep: test multiple --author with --all-match

Michael J Gruber <git@drmicha.warpmail.net> writes:
> --all-match is ignored for author matching on purpose.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---

It is more like "--all-match is about --grep and does not interact with --author or --committer at all". At least with the recent fix.

Show 22 quoted lines
>  t/t7810-grep.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index b841909..be81d96 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -513,6 +513,14 @@ test_expect_success 'log with multiple --author uses union' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'log --all-match with multiple --author still uses union' '
> +	git log --all-match --author="Thor" --author="Aster" --format=%s >actual &&
> +	{
> +	    echo third && echo second && echo initial
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'log with --grep and multiple --author uses all-match' '
>  	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
>  	{
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 10/11] t7810-grep: test interaction of multiple --grep and --author options

There are tests for this interaction already. Restructure slightly and avoid any claims about --all-match.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
Show changes to t/t7810-grep.sh +22 −16
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index be81d96..f6edb4d 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -495,16 +495,6 @@ test_expect_success 'log --all-match with multiple --grep uses intersection' '
 	test_cmp expect actual
 '
 
-test_expect_success 'log --grep --author implicitly uses all-match' '
-	# grep matches initial and second but not third
-	# author matches only initial and third
-	git log --author="A U Thor" --grep=s --grep=l --format=%s >actual &&
-	{
-		echo initial
-	} >expect &&
-	test_cmp expect actual
-'
-
 test_expect_success 'log with multiple --author uses union' '
 	git log --author="Thor" --author="Aster" --format=%s >actual &&
 	{
@@ -521,17 +511,33 @@ test_expect_success 'log --all-match with multiple --author still uses union' '
 	test_cmp expect actual
 '
 
-test_expect_success 'log with --grep and multiple --author uses all-match' '
-	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
+test_expect_success 'log --grep --author uses intersection' '
+	# grep matches only third and fourth
+	# author matches only initial and third
+	git log --author="A U Thor" --grep=r --format=%s >actual &&
 	{
-	    echo third && echo initial
+		echo third
 	} >expect &&
 	test_cmp expect actual
 '
 
-test_expect_success 'log with --grep and multiple --author uses all-match' '
-	git log --author="Thor" --author="Night" --grep=q --format=%s >actual &&
-	>expect &&
+test_expect_success 'log --grep --grep --author takes union of greps and intersects with author' '
+	# grep matches initial and second but not third
+	# author matches only initial and third
+	git log --author="A U Thor" --grep=s --grep=l --format=%s >actual &&
+	{
+		echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --grep --author --author takes union of authors and intersects with grep' '
+	# grep matches only initial and third
+	# author matches all but second
+	git log --author="Thor" --author="Night" --grep=i --format=%s >actual &&
+	{
+	    echo third && echo initial
+	} >expect &&
 	test_cmp expect actual
 '
 

-- 
1.7.12.592.g41e7905
Michael J Gruber· Sep 14, 2012, 09:46 UTC · re: Junio C Hamano · lore

[PATCHv3 11/11] t7810-grep: test --all-match with multiple --grep and --author options

--all-match is ignored with multiple author options on purpose but requires all --grep to be matched on some line.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
Show changes to t/t7810-grep.sh +20 −0
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index f6edb4d..b5c488e 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -531,6 +531,16 @@ test_expect_success 'log --grep --grep --author takes union of greps and interse
 	test_cmp expect actual
 '
 
+test_expect_success 'log ---all-match -grep --author --author still takes union of authors and intersects with grep' '
+	# grep matches only initial and third
+	# author matches all but second
+	git log --all-match --author="Thor" --author="Night" --grep=i --format=%s >actual &&
+	{
+	    echo third && echo initial
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'log --grep --author --author takes union of authors and intersects with grep' '
 	# grep matches only initial and third
 	# author matches all but second
@@ -541,6 +551,16 @@ test_expect_success 'log --grep --author --author takes union of authors and int
 	test_cmp expect actual
 '
 
+test_expect_success 'log --all-match --grep --grep --author takes intersection' '
+	# grep matches only third
+	# author matches only initial and third
+	git log --all-match --author="A U Thor" --grep=i --grep=r --format=%s >actual &&
+	{
+		echo third
+	} >expect &&
+	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.592.g41e7905
Junio C Hamano· Sep 14, 2012, 18:01 UTC · re: Michael J Gruber · lore

Re: [PATCHv3 11/11] t7810-grep: test --all-match with multiple --grep and --author options

Michael J Gruber <git@drmicha.warpmail.net> writes:
> --all-match is ignored with multiple author options on purpose but
> requires all --grep to be matched on some line.

It is more like "the behaviour of --all-match to tie more than one --grep used to be broken when --author or --committer is used".

Show 44 quoted lines
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  t/t7810-grep.sh | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index f6edb4d..b5c488e 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -531,6 +531,16 @@ test_expect_success 'log --grep --grep --author takes union of greps and interse
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'log ---all-match -grep --author --author still takes union of authors and intersects with grep' '
> +	# grep matches only initial and third
> +	# author matches all but second
> +	git log --all-match --author="Thor" --author="Night" --grep=i --format=%s >actual &&
> +	{
> +	    echo third && echo initial
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'log --grep --author --author takes union of authors and intersects with grep' '
>  	# grep matches only initial and third
>  	# author matches all but second
> @@ -541,6 +551,16 @@ test_expect_success 'log --grep --author --author takes union of authors and int
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'log --all-match --grep --grep --author takes intersection' '
> +	# grep matches only third
> +	# author matches only initial and third
> +	git log --all-match --author="A U Thor" --grep=i --grep=r --format=%s >actual &&
> +	{
> +		echo third
> +	} >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'grep with CE_VALID file' '
>  	git update-index --assume-unchanged t/t &&
>  	rm t/t &&
Michael J Gruber· Sep 13, 2012, 07:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Junio C Hamano venit, vidit, dixit 12.09.2012 19:25:
Show 50 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> It was introduced in 0ab7befa with a clear meaning (AND everything),
>> then the general logic (without --all-match) was modified in 80235ba7
>> (to take headermatch AND (all greps ORed)), and 5aaeb733 finally made
>> multiple authors resp. committers get ORed among each other. All of this
>> in an attempt to make the standard usage most useful, of course. As a
>> consequence, --all-match does not influence multiple --author options at
>> all (contrary to the doc), e.g.
>>
>> I don't see any of this reflected in the doc, though. I noticed only by
>> reading t/t7810-grep.sh. Before that, I had only gone by my own testing
>> which didn't reveal the multiple author multiple special casing effect.
>>
>> I guess I'll have to wrap my head around the current implementation a
>> few more times before trying to describe the status quo in the
>> documentation...
> 
> This is what I used to use when adding these generalized grep
> boolean expressions.
> 
> With this applied, you can try things like these:
> 
>     $ git log --grep=regexp --grep=nosuch --all-match >/dev/null
>     $ git log --grep=regexp --grep=nosuch --author=Michael >/dev/null
> 
> For example, "--all-match --grep=regexp --author=Michael --author=Linus" turns
> into
> 
>     [all-match]
>     (or
>      pattern_body<body>regexp
>      (or
>       (or
>        pattern_head<head 0>Linus
>        pattern_head<head 0>Michael
>       )
>       true
>      )
>     )
> 
> that says "body must have 'regexp' in it, and authored by either
> Linus or Michael".
> 
> The semantics of "--all-match" is different from "--and" (which,
> together with "--or", ")", and "(", is not availble to rev-list
> command line parser primarily because "--not" is not available).
> After applying all the "or"-ed terms, it checks the top-level nodes
> that are "or"-ed and makes sure all of them fired (possibly and
> usually on different lines).
Thanks for "this" ;)

I'll recheck my understanding and update the doc then. Maybe even putting the above in-tree with a "--debug" option seems inline with things such as "git describe --debug" (and maybe a preparation for exposing a richer interface).

Michael
Junio C Hamano· Sep 13, 2012, 21:21 UTC · re: Michael J Gruber · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Michael J Gruber <git@drmicha.warpmail.net> writes:
> Thanks for "this" ;)

Here is a replacement to "this", that adds the "--debug" option to "git grep" and an equivalent "--debug-grep" to "git log" family.

-- >8 --
Subject: [PATCH] grep: teach --debug option to dump the parse tree

Our "grep" allows complex boolean expressions to be formed to match each individual line with operators like --and, '(', ')' and --not. Introduce the "--debug" option to show the parse tree to help people who want to debug and enhance it.

Also "log" learns "--debug-grep" option to do the same. The command line parser to the log family is a lot more limited than the general "git grep" parser, but it has special handling for header matching (e.g. "--author"), and a parse tree is valuable when working on it.

Note that "--all-match" is *not* any individual node in the parse tree. It is an instruction to the evaluator to check all the nodes in the top-level backbone have matched and reject a document as non-matching otherwise.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/grep.c |  3 ++
 grep.c         | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 grep.h         |  1 +
 revision.c     |  2 ++
 4 files changed, 97 insertions(+), 2 deletions(-)
Show changes to 4 files +97 −2

builtin/grep.c, grep.c, grep.h, revision.c

diff --git a/builtin/grep.c b/builtin/grep.c
index 09ca4c9..1c6b95a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -817,6 +817,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			   N_("indicate hit with exit status without output")),
 		OPT_BOOLEAN(0, "all-match", &opt.all_match,
 			N_("show only matches from files that match all patterns")),
+		{ OPTION_SET_INT, 0, "debug", &opt.debug, NULL,
+		  N_("show parse tree for grep expression"),
+		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1 },
 		OPT_GROUP(""),
 		{ OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
 			N_("pager"), N_("show matching files in the pager"),
diff --git a/grep.c b/grep.c
index 04e3ec6..f896d68 100644
--- a/grep.c
+++ b/grep.c
@@ -332,6 +332,87 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)
 	return compile_pattern_or(list);
 }
 
+static void indent(int in)
+{
+	while (in-- > 0)
+		fputc(' ', stderr);
+}
+
+static void dump_grep_pat(struct grep_pat *p)
+{
+	switch (p->token) {
+	case GREP_AND: fprintf(stderr, "*and*"); break;
+	case GREP_OPEN_PAREN: fprintf(stderr, "*(*"); break;
+	case GREP_CLOSE_PAREN: fprintf(stderr, "*)*"); break;
+	case GREP_NOT: fprintf(stderr, "*not*"); break;
+	case GREP_OR: fprintf(stderr, "*or*"); break;
+
+	case GREP_PATTERN: fprintf(stderr, "pattern"); break;
+	case GREP_PATTERN_HEAD: fprintf(stderr, "pattern_head"); break;
+	case GREP_PATTERN_BODY: fprintf(stderr, "pattern_body"); break;
+	}
+
+	switch (p->token) {
+	default: break;
+	case GREP_PATTERN_HEAD:
+		fprintf(stderr, "<head %d>", p->field); break;
+	case GREP_PATTERN_BODY:
+		fprintf(stderr, "<body>"); break;
+	}
+	switch (p->token) {
+	default: break;
+	case GREP_PATTERN_HEAD:
+	case GREP_PATTERN_BODY:
+	case GREP_PATTERN:
+		fprintf(stderr, "%.*s", (int)p->patternlen, p->pattern);
+		break;
+	}
+	fputc('\n', stderr);
+}
+
+static void dump_grep_expression_1(struct grep_expr *x, int in)
+{
+	indent(in);
+	switch (x->node) {
+	case GREP_NODE_TRUE:
+		fprintf(stderr, "true\n");
+		break;
+	case GREP_NODE_ATOM:
+		dump_grep_pat(x->u.atom);
+		break;
+	case GREP_NODE_NOT:
+		fprintf(stderr, "(not\n");
+		dump_grep_expression_1(x->u.unary, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	case GREP_NODE_AND:
+		fprintf(stderr, "(and\n");
+		dump_grep_expression_1(x->u.binary.left, in+1);
+		dump_grep_expression_1(x->u.binary.right, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	case GREP_NODE_OR:
+		fprintf(stderr, "(or\n");
+		dump_grep_expression_1(x->u.binary.left, in+1);
+		dump_grep_expression_1(x->u.binary.right, in+1);
+		indent(in);
+		fprintf(stderr, ")\n");
+		break;
+	}
+}
+
+void dump_grep_expression(struct grep_opt *opt)
+{
+	struct grep_expr *x = opt->pattern_expression;
+
+	if (opt->all_match)
+		fprintf(stderr, "[all-match]\n");
+	dump_grep_expression_1(x, 0);
+	fflush(NULL);
+}
+
 static struct grep_expr *grep_true_expr(void)
 {
 	struct grep_expr *z = xcalloc(1, sizeof(*z));
@@ -395,7 +476,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	return header_expr;
 }
 
-void compile_grep_patterns(struct grep_opt *opt)
+static void compile_grep_patterns_real(struct grep_opt *opt)
 {
 	struct grep_pat *p;
 	struct grep_expr *header_expr = prep_header_patterns(opt);
@@ -415,7 +496,7 @@ void compile_grep_patterns(struct grep_opt *opt)
 
 	if (opt->all_match || header_expr)
 		opt->extended = 1;
-	else if (!opt->extended)
+	else if (!opt->extended && !opt->debug)
 		return;
 
 	p = opt->pattern_list;
@@ -432,9 +513,17 @@ void compile_grep_patterns(struct grep_opt *opt)
 	else
 		opt->pattern_expression = grep_or_expr(opt->pattern_expression,
 						       header_expr);
+
 	opt->all_match = 1;
 }
 
+void compile_grep_patterns(struct grep_opt *opt)
+{
+	compile_grep_patterns_real(opt);
+	if (opt->debug)
+		dump_grep_expression(opt);
+}
+
 static void free_pattern_expr(struct grep_expr *x)
 {
 	switch (x->node) {
diff --git a/grep.h b/grep.h
index 75afb7b..00d71f7 100644
--- a/grep.h
+++ b/grep.h
@@ -98,6 +98,7 @@ struct grep_opt {
 	int word_regexp;
 	int fixed;
 	int all_match;
+	int debug;
 #define GREP_BINARY_DEFAULT	0
 #define GREP_BINARY_NOMATCH	1
 #define GREP_BINARY_TEXT	2
diff --git a/revision.c b/revision.c
index dc3fecf..9ef9a4c 100644
--- a/revision.c
+++ b/revision.c
@@ -1598,6 +1598,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
 		add_message_grep(revs, optarg);
 		return argcount;
+	} else if (!strcmp(arg, "--debug-grep")) {
+		revs->grep_filter.debug = 1;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
 		revs->grep_filter.regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {

-- 
1.7.12.469.g5235eb6
Michael J Gruber· Sep 14, 2012, 07:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Junio C Hamano venit, vidit, dixit 13.09.2012 23:21:
Show 24 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Thanks for "this" ;)
> 
> Here is a replacement to "this", that adds the "--debug" option to
> "git grep" and an equivalent "--debug-grep" to "git log" family.
> 
> -- >8 --
> Subject: [PATCH] grep: teach --debug option to dump the parse tree
> 
> Our "grep" allows complex boolean expressions to be formed to match
> each individual line with operators like --and, '(', ')' and --not.
> Introduce the "--debug" option to show the parse tree to help people
> who want to debug and enhance it.
> 
> Also "log" learns "--debug-grep" option to do the same.  The command
> line parser to the log family is a lot more limited than the general
> "git grep" parser, but it has special handling for header matching
> (e.g. "--author"), and a parse tree is valuable when working on it.
> 
> Note that "--all-match" is *not* any individual node in the parse
> tree.  It is an instruction to the evaluator to check all the nodes
> in the top-level backbone have matched and reject a document as
> non-matching otherwise.

I haven't read all your responses yet, but the/my main confusion comes from all-match. My understanding is:

--all-match == turn all top-level ORs into ANDs

(unless it's all --author at the top-level; and I'm referring to user supplied ORs, not those added by the implementation)

Seing (OR foo true) being evaluated to false when foo is false and all-match is in effect is terribly confusing, me thinks (debug output).

In fact, I think the behavior described above (if it's correct) is a product of the evolution of grep.c and the current implementation (turning former unions into intersections in some cases, inserting that TRUE node), but not necessarily the intended or preferrable outcome in all corner cases.

We AND different types of limit options in any case (this used to be affected by --all-match[?]), and we OR --author options in any case. So having --all-match turn "--grep=foo --grep=bar" from OR into AND would make the most sense at least to me (and I mean: independent of the presence of an --author option).

Michael
Junio C Hamano· Sep 14, 2012, 07:50 UTC · re: Michael J Gruber · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Michael J Gruber venit, vidit, dixit 14.09.2012 09:46: [snipped, just adding]

...and maybe the meaning of "(or ...)" and "*or*" isn't what I think it is either?

Michael J Gruber· Sep 14, 2012, 08:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list/log: document logic with several limiting options

Oh, my gosh, it's not as early as this indicates, rather coffein depletion already:

> Message-ID: <5052E1C2.1050008@pobox.com>
> Date: Fri, 14 Sep 2012 09:50:26 +0200
> From: Junio C Hamano <gitster@pobox.com>
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120827 Thunderbird/15.0
[no, that Thunderbird user isn't Junio]
> MIME-Version: 1.0
> To: Michael J Gruber <git@drmicha.warpmail.net>
> CC: git@vger.kernel.org
> Subject: Re: [PATCH] rev-list/log: document logic with several limiting options
Show 5 quoted lines
> Michael J Gruber venit, vidit, dixit 14.09.2012 09:46:
> [snipped, just adding]
> 
> ...and maybe the meaning of "(or ...)" and "*or*" isn't what I think it
> is either?
[and that confused persion isn't Junio either]

I didn't mean to imposter Junio. Something went wrong with "virtual identity" choosing the From: address. Terribly sorry.

Michael (really...)

← back to recent threads