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 gits...@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_bodybodycommit
  (or
   pattern_bodybodytag
   (or
pattern_headhead 1Linus
(or
 pattern_headhead 0Junio
 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
 /\
/  \
patternOR
  / \   / \
 .committerOR
  /   \ 
  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);
 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Michael J Gruber g...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 g...@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

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

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

[Junio C Hamano]

Michael J Gruber g...@drmicha.warpmail.net writes:

 --all-match is ignored for author matching on purpose.

 Signed-off-by: Michael J Gruber g...@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_bodybodyJunio
 pattern_bodybodyLinus
)

but when you say

git log --author=Linus --author=Junio

you will get

[all-match]
(or
 (or
  pattern_headhead 0Linus
  pattern_headhead 0Junio
 )
 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_bodybodycommit
 (or
  (or
   pattern_headhead 0Linus
   pattern_headhead 0Junio
  )
  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_headhead 1Linus
 (or
  pattern_headhead 0Junio
  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_bodybodycommit
 (or
  pattern_headhead 1Linus
  (or
   pattern_headhead 0Junio
   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_bodybodycommit
  pattern_bodybodytag
 )
 (or
  pattern_headhead 1Linus
  (or
   pattern_headhead 0Junio
   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_bodybodycommit
 (or
  pattern_bodybodytag
  (or
   pattern_headhead 1Linus
   (or
pattern_headhead 0Junio
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 
   {
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Junio C Hamano gits...@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_bodybodycommit
  (or
   pattern_bodybodytag
   (or
pattern_headhead 1Linus
(or
 pattern_headhead 0Junio
 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
/\
   /  \
   patternOR
 / \   / \
.committerOR
 /   \ 
 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);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html