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

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

[Michael J Gruber]

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(+)

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

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

[Junio C Hamano]

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.

>
>  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"?

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 11.09.2012 18:22:
> 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).

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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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(-)

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

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

[Michael J Gruber]

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

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

[Michael J Gruber]

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(-)

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

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

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

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

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

[Michael J Gruber]

--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(+)

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

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

[Junio C Hamano]

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.

>  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 &&
>  	{

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

[Junio C Hamano]

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]?

 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);

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 14.09.2012 01:26:
> 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*" ...]

>  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);
> 

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

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

[Michael J Gruber]

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(-)

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

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

[Michael J Gruber]

--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(+)

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

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

[Michael J Gruber]

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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

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(-)

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

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

[Michael J Gruber]

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

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

[Michael J Gruber]

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(-)

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

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> 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 ;-)

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

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 revision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Michael J Gruber]

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);
 
-- 
1.7.12.592.g41e7905

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

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

[Michael J Gruber]

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(+)

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

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

[Michael J Gruber]

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(-)

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

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

[Michael J Gruber]

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(-)

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

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

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

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

[Michael J Gruber]

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(-)

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

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

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7810-grep.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

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

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

[Michael J Gruber]

--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(+)

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

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

[Junio C Hamano]

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.


>  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 &&
>  	{

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

[Michael J Gruber]

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(-)

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

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

[Michael J Gruber]

--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(+)

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

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

Thanks for an update.  Queued with minimum modification.

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 12.09.2012 19:25:
> 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

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

[Junio C Hamano]

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(-)

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 13.09.2012 23:21:
> 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

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

[Junio C Hamano]

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?

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

[Michael J Gruber]

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

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