{"thread":{"id":"24870","subject":"Regression in git log with multiple authors","startedAt":"2010-08-26T17:39:45Z","lastAt":"2010-09-13T18:11:40Z","messageCount":6,"participants":["Emil Sit","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"149073","messageId":"AANLkTikONxneEgF5m+m6100pwzThTnaiAB+OFzYufcC2@mail.gmail.com","threadId":"24870","inReplyTo":null,"subject":"Regression in git log with multiple authors","fromName":"Emil Sit","fromEmail":"sit@emilsit.net","sentAt":"2010-08-26T17:39:45Z","receivedAt":"2010-08-26T17:39:45Z","isPatch":false,"sender":{"key":"sit@emilsit.net","avatar":"https://gravatar.com/avatar/21981e3a66a89f6107940c70470f0791955004b68d192eee304104c2e2534d48?d=mp&s=160"},"body":"Commit 80235ba79ef43349f455cce869397b3e726f4058 introduced a\nregression in a corner case for git log --author when multiple authors\nare specified.  Prior to 1.7.0.3, if I wanted to find all commits done\nby a series of authors, I could simply specify \"git log --author=a1\n--author=a2\" to get all commits done by a1 and a2.  However, in the\nlatest releases, this finds nothing.\n\nHere's a simple test case that demonstrates this:\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 023f225..587069c 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -372,6 +372,14 @@ test_expect_success 'log --grep --author\nimplicitly uses all-match' '\n        test_cmp expect actual\n '\n\n+test_expect_success 'log --author --author matches both authors' '\n+       # author matches only initial and third\n+       # frotz matches only second\n+       git log --author=\"A U Thor\" --author=\"frotz\\.com>$\"\n--format=%s >actual &&\n+        ( echo third ; echo second ; echo initial ) >expect &&\n+       test_cmp expect actual\n+'\n+\n test_expect_success 'grep with CE_VALID file' '\n        git update-index --assume-unchanged t/t &&\n        rm t/t &&\n\nThis fails against master, but if you revert 80235ba, this will pass\n(whereas obviously 'log --grep --author implicitly uses all-match'\nwill then fail).\n\nIt doesn't seem like I can work-around this with 'git log --author a1\n--or --author a2'.  Is there some other way to find commits by a set\nof authors? I don't think it makes sense to treat multiple --author\nflags with \"and' logic since a commit can only have one author.  So\nmaybe all --authors should be grouped with ors and then anded against\nall --committers?\n\n-- \nEmil Sit / http://www.emilsit.net/\n"},{"id":"149080","messageId":"7veidlkxdb.fsf@alter.siamese.dyndns.org","threadId":"24870","inReplyTo":"AANLkTikONxneEgF5m+m6100pwzThTnaiAB+OFzYufcC2@mail.gmail.com","subject":"Re: Regression in git log with multiple authors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-26T19:05:52Z","receivedAt":"2010-08-26T19:05:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emil Sit <sit@emilsit.net> writes:\n\n> Commit 80235ba79ef43349f455cce869397b3e726f4058 introduced a\n> regression in a corner case for git log --author when multiple authors\n> are specified.  Prior to 1.7.0.3, if I wanted to find all commits done\n> by a series of authors, I could simply specify \"git log --author=a1\n> --author=a2\" to get all commits done by a1 and a2.  However, in the\n> latest releases, this finds nothing.\n\nThat is more or less deliberate, not in the sense that the patch wanted to\nforbid looking for multiple authors but in the sense that the patch wanted\nto apply the \"grep\" terms as intersection, not as union.\n\nIn the olden days,\n\n    log --author=me --committer=him --grep=this --grep=that\n\nused to be turned into:\n\n    (OR (HEADER-AUTHOR me)\n        (HEADER-COMMITTER him)\n        (PATTERN this)\n        (PATTERN that))\n\nshowing my patches that do not have any \"this\" nor \"that\", which was\ntotally bogus and useless.\n\n80235ba (\"log --author=me --grep=it\" should find intersection, not union,\n2010-01-17) improved it greatly to turn the same into:\n\n    (all-match (HEADER-AUTHOR me)\n\t       (HEADER-COMMITTER him)\n\t       (OR\n\t         (PATTERN this)\n                 (PATTERN that)))\n\nThat is, \"show only patches by me committed by him that have either this\nor that\", which is a lot more natural thing to ask.  So simply reverting\nthe commit is out of question.\n\nBut I do not think it is a bad idea if you turned\n\n    log --author=me --author=her --committer=him --committer=you --grep=this\n\ninto\n\n    (all-match (OR\n\t\t(HEADER-AUTHOR me)\n\t\t(HEADER-AUTHOR her))\n               (OR\n\t        (HEADER-COMMITTER him)\n\t        (HEADER-COMMITTER you))\n\t       (OR\n\t         (PATTERN this)))\n\nas it is obvious that with multiple authors (or committers) the command\nline is asking for union among them.\n"},{"id":"150575","messageId":"7vsk1eawmx.fsf_-_@alter.siamese.dyndns.org","threadId":"24870","inReplyTo":"7veidlkxdb.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] grep: move logic to compile header pattern into a separate helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-13T08:13:58Z","receivedAt":"2010-09-13T08:13:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The callers should be queuing only GREP_PATTERN_HEAD elements to the\nheader_list queue; simplify the switch and guard it with an assert.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is just a clean-up before the real fun.\n\n grep.c |   43 +++++++++++++++++++++----------------------\n 1 files changed, 21 insertions(+), 22 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 82fb349..718a3c2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -189,29 +189,31 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)\n \treturn compile_pattern_or(list);\n }\n \n-void compile_grep_patterns(struct grep_opt *opt)\n+static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n {\n \tstruct grep_pat *p;\n-\tstruct grep_expr *header_expr = NULL;\n-\n-\tif (opt->header_list) {\n-\t\tp = opt->header_list;\n-\t\theader_expr = compile_pattern_expr(&p);\n-\t\tif (p)\n-\t\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n-\t\tfor (p = opt->header_list; p; p = p->next) {\n-\t\t\tswitch (p->token) {\n-\t\t\tcase GREP_PATTERN: /* atom */\n-\t\t\tcase GREP_PATTERN_HEAD:\n-\t\t\tcase GREP_PATTERN_BODY:\n-\t\t\t\tcompile_regexp(p, opt);\n-\t\t\t\tbreak;\n-\t\t\tdefault:\n-\t\t\t\topt->extended = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n+\tstruct grep_expr *header_expr;\n+\n+\tif (!opt->header_list)\n+\t\treturn NULL;\n+\tp = opt->header_list;\n+\theader_expr = compile_pattern_expr(&p);\n+\tif (p)\n+\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n+\tfor (p = opt->header_list; p; p = p->next) {\n+\t\tif (p->token != GREP_PATTERN_HEAD)\n+\t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n+\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n+\t\t\tdie(\"bug: unknown header field %d\", p->field);\n+\t\tcompile_regexp(p, opt);\n \t}\n+\treturn header_expr;\n+}\n+\n+void compile_grep_patterns(struct grep_opt *opt)\n+{\n+\tstruct grep_pat *p;\n+\tstruct grep_expr *header_expr = prep_header_patterns(opt);\n \n \tfor (p = opt->pattern_list; p; p = p->next) {\n \t\tswitch (p->token) {\n@@ -231,9 +233,6 @@ void compile_grep_patterns(struct grep_opt *opt)\n \telse if (!opt->extended)\n \t\treturn;\n \n-\t/* Then bundle them up in an expression.\n-\t * A classic recursive descent parser would do.\n-\t */\n \tp = opt->pattern_list;\n \tif (p)\n \t\topt->pattern_expression = compile_pattern_expr(&p);\n-- \n1.7.3.rc1.227.gee5c7b\n"},{"id":"150583","messageId":"7vmxrmawg0.fsf_-_@alter.siamese.dyndns.org","threadId":"24870","inReplyTo":"7veidlkxdb.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] log --author: take union of multiple \"author\" requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-13T08:18:07Z","receivedAt":"2010-09-13T08:18:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In the olden days,\n\n    log --author=me --committer=him --grep=this --grep=that\n\nused to be turned into:\n\n    (OR (HEADER-AUTHOR me)\n        (HEADER-COMMITTER him)\n        (PATTERN this)\n        (PATTERN that))\n\nshowing my patches that do not have any \"this\" nor \"that\", which was\ntotally useless.\n\n80235ba (\"log --author=me --grep=it\" should find intersection, not union,\n2010-01-17) improved it greatly to turn the same into:\n\n    (ALL-MATCH\n      (HEADER-AUTHOR me)\n      (HEADER-COMMITTER him)\n      (OR (PATTERN this) (PATTERN that)))\n\nThat is, \"show only patches by me and committed by him, that have either\nthis or that\", which is a lot more natural thing to ask.\n\nWe however need to be a bit more clever when the user asks more than one\n\"author\" (or \"committer\"); because a commit has only one author (and one\ncommitter), they ought to be interpreted as asking for union to be useful.\nThe current implementation simply added another author/committer pattern\nat the same top-level for ALL-MATCH to insist on matching all, finding\nnothing.\n\nTurn\n\n    log --author=me --author=her \\\n    \t--committer=him --committer=you \\\n\t--grep=this --grep=that\n\ninto\n\n    (ALL-MATCH\n      (OR (HEADER-AUTHOR me) (HEADER-AUTHOR her))\n      (OR (HEADER-COMMITTER him) (HEADER-COMMITTER you))\n      (OR (PATTERN this) (PATTERN that)))\n\ninstead.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n grep.c          |   65 ++++++++++++++++++++++++++++++++++++++++++++----------\n grep.h          |    2 +\n t/t7810-grep.sh |   29 +++++++++++++++++++++++-\n 3 files changed, 83 insertions(+), 13 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 718a3c2..63c4280 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -189,17 +189,32 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)\n \treturn compile_pattern_or(list);\n }\n \n+static struct grep_expr *grep_true_expr(void)\n+{\n+\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n+\tz->node = GREP_NODE_TRUE;\n+\treturn z;\n+}\n+\n+static struct grep_expr *grep_or_expr(struct grep_expr *left, struct grep_expr *right)\n+{\n+\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n+\tz->node = GREP_NODE_OR;\n+\tz->u.binary.left = left;\n+\tz->u.binary.right = right;\n+\treturn z;\n+}\n+\n static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n {\n \tstruct grep_pat *p;\n \tstruct grep_expr *header_expr;\n+\tstruct grep_expr *(header_group[GREP_HEADER_FIELD_MAX]);\n+\tenum grep_header_field fld;\n \n \tif (!opt->header_list)\n \t\treturn NULL;\n \tp = opt->header_list;\n-\theader_expr = compile_pattern_expr(&p);\n-\tif (p)\n-\t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n \tfor (p = opt->header_list; p; p = p->next) {\n \t\tif (p->token != GREP_PATTERN_HEAD)\n \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n@@ -207,6 +222,33 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n \t\t\tdie(\"bug: unknown header field %d\", p->field);\n \t\tcompile_regexp(p, opt);\n \t}\n+\n+\tfor (fld = 0; fld < GREP_HEADER_FIELD_MAX; fld++)\n+\t\theader_group[fld] = NULL;\n+\n+\tfor (p = opt->header_list; p; p = p->next) {\n+\t\tstruct grep_expr *h;\n+\t\tstruct grep_pat *pp = p;\n+\n+\t\th = compile_pattern_atom(&pp);\n+\t\tif (!h || pp != p->next)\n+\t\t\tdie(\"bug: malformed header expr\");\n+\t\tif (!header_group[p->field]) {\n+\t\t\theader_group[p->field] = h;\n+\t\t\tcontinue;\n+\t\t}\n+\t\theader_group[p->field] = grep_or_expr(h, header_group[p->field]);\n+\t}\n+\n+\theader_expr = NULL;\n+\n+\tfor (fld = 0; fld < GREP_HEADER_FIELD_MAX; fld++) {\n+\t\tif (!header_group[fld])\n+\t\t\tcontinue;\n+\t\tif (!header_expr)\n+\t\t\theader_expr = grep_true_expr();\n+\t\theader_expr = grep_or_expr(header_group[fld], header_expr);\n+\t}\n \treturn header_expr;\n }\n \n@@ -242,22 +284,18 @@ void compile_grep_patterns(struct grep_opt *opt)\n \tif (!header_expr)\n \t\treturn;\n \n-\tif (opt->pattern_expression) {\n-\t\tstruct grep_expr *z;\n-\t\tz = xcalloc(1, sizeof(*z));\n-\t\tz->node = GREP_NODE_OR;\n-\t\tz->u.binary.left = opt->pattern_expression;\n-\t\tz->u.binary.right = header_expr;\n-\t\topt->pattern_expression = z;\n-\t} else {\n+\tif (!opt->pattern_expression)\n \t\topt->pattern_expression = header_expr;\n-\t}\n+\telse\n+\t\topt->pattern_expression = grep_or_expr(opt->pattern_expression,\n+\t\t\t\t\t\t       header_expr);\n \topt->all_match = 1;\n }\n \n static void free_pattern_expr(struct grep_expr *x)\n {\n \tswitch (x->node) {\n+\tcase GREP_NODE_TRUE:\n \tcase GREP_NODE_ATOM:\n \t\tbreak;\n \tcase GREP_NODE_NOT:\n@@ -486,6 +524,9 @@ static int match_expr_eval(struct grep_expr *x, char *bol, char *eol,\n \tif (!x)\n \t\tdie(\"Not a valid grep expression\");\n \tswitch (x->node) {\n+\tcase GREP_NODE_TRUE:\n+\t\th = 1;\n+\t\tbreak;\n \tcase GREP_NODE_ATOM:\n \t\th = match_one_pattern(x->u.atom, bol, eol, ctx, &match, 0);\n \t\tbreak;\ndiff --git a/grep.h b/grep.h\nindex efa8cff..06621fe 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -22,6 +22,7 @@ enum grep_header_field {\n \tGREP_HEADER_AUTHOR = 0,\n \tGREP_HEADER_COMMITTER\n };\n+#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1)\n \n struct grep_pat {\n \tstruct grep_pat *next;\n@@ -41,6 +42,7 @@ enum grep_expr_node {\n \tGREP_NODE_ATOM,\n \tGREP_NODE_NOT,\n \tGREP_NODE_AND,\n+\tGREP_NODE_TRUE,\n \tGREP_NODE_OR\n };\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 8a63227..dc5c085 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -324,8 +324,13 @@ test_expect_success 'log grep setup' '\n \n \techo a >>file &&\n \ttest_tick &&\n-\tgit commit -a -m \"third\"\n+\tgit commit -a -m \"third\" &&\n \n+\techo a >>file &&\n+\ttest_tick &&\n+\tGIT_AUTHOR_NAME=\"Night Fall\" \\\n+\tGIT_AUTHOR_EMAIL=\"nitfol@frobozz.com\" \\\n+\tgit commit -a -m \"fourth\"\n '\n \n test_expect_success 'log grep (1)' '\n@@ -372,6 +377,28 @@ test_expect_success 'log --grep --author implicitly uses all-match' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log with multiple --author uses union' '\n+\tgit log --author=\"Thor\" --author=\"Aster\" --format=%s >actual &&\n+\t{\n+\t    echo third && echo second && echo initial\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log with --grep and multiple --author uses all-match' '\n+\tgit log --author=\"Thor\" --author=\"Night\" --grep=i --format=%s >actual &&\n+\t{\n+\t    echo third && echo initial\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log with --grep and multiple --author uses all-match' '\n+\tgit log --author=\"Thor\" --author=\"Night\" --grep=q --format=%s >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'grep with CE_VALID file' '\n \tgit update-index --assume-unchanged t/t &&\n \trm t/t &&\n-- \n1.7.3.rc1.227.gee5c7b\n"},{"id":"150603","messageId":"AANLkTinaj4AsPE9j-gS2-0Cn8jx7a1uYYGtmq5oC=YVB@mail.gmail.com","threadId":"24870","inReplyTo":"7vmxrmawg0.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] log --author: take union of multiple \"author\" requests","fromName":"Emil Sit","fromEmail":"sit@emilsit.net","sentAt":"2010-09-13T16:17:26Z","receivedAt":"2010-09-13T16:17:26Z","isPatch":true,"sender":{"key":"sit@emilsit.net","avatar":"https://gravatar.com/avatar/21981e3a66a89f6107940c70470f0791955004b68d192eee304104c2e2534d48?d=mp&s=160"},"body":"On Mon, Sep 13, 2010 at 4:18 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>    log --author=me --author=her \\\n>        --committer=him --committer=you \\\n>        --grep=this --grep=that\n>\n> into\n>\n>    (ALL-MATCH\n>      (OR (HEADER-AUTHOR me) (HEADER-AUTHOR her))\n>      (OR (HEADER-COMMITTER him) (HEADER-COMMITTER you))\n>      (OR (PATTERN this) (PATTERN that)))\n\nBoth patches look good to me.  Tested fine with my use cases. Thanks\nfor doing this.\n\nI'm a little confused about the implementation with regards to\n--all-match; does there still need to be an all-match flag?  Seems\nlike it has now been deprecated (to being, essentially, the default).\nIn any case, there should also probably something going along with\nthis patch series to update Documentation/rev-list-options.txt.\n\nThanks again.\n\n-- \nEmil Sit / http://www.emilsit.net/\n"},{"id":"150607","messageId":"7vfwxdbjj7.fsf@alter.siamese.dyndns.org","threadId":"24870","inReplyTo":"AANLkTinaj4AsPE9j-gS2-0Cn8jx7a1uYYGtmq5oC=YVB@mail.gmail.com","subject":"Re: [PATCH 2/2] log --author: take union of multiple \"author\" requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-13T18:11:40Z","receivedAt":"2010-09-13T18:11:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emil Sit <sit@emilsit.net> writes:\n\n> I'm a little confused about the implementation with regards to\n> --all-match; does there still need to be an all-match flag?\n\nWhen used in the context of \"git log --grep/--author/--committer\" (as\nopposed to more flexible \"git grep\"), in conjunction with either of the\n\"header match\" element (--author/committer), all-match is implied.\n\nThe implementation of the all-match rewriting gets a bit trickier than\nnecessary, as our internal representation of nodes does not have n-ary\nALL-MATCH (nor n-ary OR/AND) node.\n\nAn (ALL-MATCH 1 2 3 4) node is instead represented by this grep_expr\nbinary tree (rooted at the leftmost OR node):\n\n      OR--OR--OR--4\n      |   |   |\n      1   2   3\n\nand requiring the top-level terms of backbone OR chain (i.e. 1 2 3 4) to\nall match.\n\nIn order to represent\n\n    (ALL-MATCH\n     (PATTERN this)\n     (OR (AUTHOR A) (AUTHOR B)))\n\nwe cannot simply do\n\n     OR--------------OR-----------author B\n     |               |\n     pattern \"this\"  author A\n\nbecause this requires both (AUTHOR A) and (AUTHOR B) to match, in addition\nto \"this\".  We instead need to do something like:\n\n\n     OR--------------OR---TRUE\n     |               |\n     pattern \"this\"  OR---author B\n                     |\n                     author A\n\nto say \"this\" must match and (OR (author A) (author B)) must match (IOW\nthe terms on the backbone OR chain are (PATTERN this), (OR (AUTHOR A/B))\nand TRUE and they all have to match).\n"}]}