{"thread":{"id":"56560","subject":"[PATCH v7 1/2] grep: refactor next_match() and match_one_pattern() for external use","startedAt":"2021-09-21T21:13:38Z","lastAt":"2021-09-24T12:04:40Z","messageCount":4,"participants":["Hamza Mahfooz","Junio C Hamano"],"isPatch":true,"patchVersion":7,"patchTotal":2},"messages":[{"id":"436668","messageId":"20210921211324.1426938-1-someguy@effective-light.com","threadId":"56560","inReplyTo":null,"subject":"[PATCH v7 1/2] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-21T21:13:23Z","receivedAt":"2021-09-21T21:13:38Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"These changes are made in preparation of, the colorization support for the\n\"git log\" subcommands that, rely on regex functionality (i.e. \"--author\",\n\"--committer\" and \"--grep\"). These changes are necessary primarily because\nmatch_one_pattern() expects header lines to be prefixed, however, in\npretty, the prefixes are stripped from the lines because the name-email\npairs needs to go through additional parsing, before they can be printed\nand because next_match() doesn't handle the case of\n\"ctx == GREP_CONTEXT_HEAD\" at all. So, teach next_match() how to handle the\nnew case, move header_field[] so it can be used by pretty to reappend\nrelevant prefixes and teach match_one_pattern() how to handle subsequent\nheader line match attempts.\n\nSigned-off-by: Hamza Mahfooz <someguy@effective-light.com>\n---\nv5: separate grep changes from pretty changes.\n\nv6: rescope some variables.\n\nv7: export header_field[] and allow for subsequent matches on header lines\n    in match_one_pattern().\n---\n grep.c | 53 ++++++++++++++++++++++++++++-------------------------\n grep.h | 13 +++++++++++++\n 2 files changed, 41 insertions(+), 25 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 14fe8a0fd2..f4126011c5 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -935,15 +935,6 @@ static void strip_timestamp(const char *bol, const char **eol_p)\n \t}\n }\n \n-static struct {\n-\tconst char *field;\n-\tsize_t len;\n-} header_field[] = {\n-\t{ \"author \", 7 },\n-\t{ \"committer \", 10 },\n-\t{ \"reflog \", 7 },\n-};\n-\n static int match_one_pattern(struct grep_pat *p,\n \t\t\t     const char *bol, const char *eol,\n \t\t\t     enum grep_context ctx,\n@@ -953,18 +944,23 @@ static int match_one_pattern(struct grep_pat *p,\n \tconst char *start = bol;\n \n \tif ((p->token != GREP_PATTERN) &&\n-\t    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)))\n+\t    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)) &&\n+\t    ((p->token == GREP_PATTERN_BODY) != (ctx == GREP_CONTEXT_BODY)))\n \t\treturn 0;\n \n \tif (p->token == GREP_PATTERN_HEAD) {\n-\t\tconst char *field;\n-\t\tsize_t len;\n-\t\tassert(p->field < ARRAY_SIZE(header_field));\n-\t\tfield = header_field[p->field].field;\n-\t\tlen = header_field[p->field].len;\n-\t\tif (strncmp(bol, field, len))\n-\t\t\treturn 0;\n-\t\tbol += len;\n+\t\tif (!(eflags & REG_NOTBOL)) {\n+\t\t\tconst char *field;\n+\t\t\tsize_t len;\n+\n+\t\t\tassert(p->field < ARRAY_SIZE(grep_header_fields));\n+\t\t\tfield = grep_header_fields[p->field].field;\n+\t\t\tlen = grep_header_fields[p->field].len;\n+\t\t\tif (strncmp(bol, field, len))\n+\t\t\t\treturn 0;\n+\t\t\tbol += len;\n+\t\t}\n+\n \t\tswitch (p->field) {\n \t\tcase GREP_HEADER_AUTHOR:\n \t\tcase GREP_HEADER_COMMITTER:\n@@ -1158,22 +1154,28 @@ static int match_next_pattern(struct grep_pat *p,\n \treturn 1;\n }\n \n-static int next_match(struct grep_opt *opt,\n-\t\t      const char *bol, const char *eol,\n-\t\t      enum grep_context ctx, regmatch_t *pmatch, int eflags)\n+int grep_next_match(struct grep_opt *opt,\n+\t\t    const char *bol, const char *eol,\n+\t\t    enum grep_context ctx, regmatch_t *pmatch,\n+\t\t    enum grep_header_field field, int eflags)\n {\n \tstruct grep_pat *p;\n \tint hit = 0;\n \n \tpmatch->rm_so = pmatch->rm_eo = -1;\n \tif (bol < eol) {\n-\t\tfor (p = opt->pattern_list; p; p = p->next) {\n+\t\tfor (p = ((ctx == GREP_CONTEXT_HEAD)\n+\t\t\t   ? opt->header_list : opt->pattern_list);\n+\t\t\t  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\thit |= match_next_pattern(p, bol, eol, ctx,\n-\t\t\t\t\t\t\t  pmatch, eflags);\n+\t\t\t\tif ((field == GREP_HEADER_FIELD_MAX) ||\n+\t\t\t\t    (p->field == field))\n+\t\t\t\t\thit |= match_next_pattern(p, bol, eol,\n+\t\t\t\t\t\t\t\t  ctx, pmatch,\n+\t\t\t\t\t\t\t\t  eflags);\n \t\t\t\tbreak;\n \t\t\tdefault:\n \t\t\t\tbreak;\n@@ -1261,7 +1263,8 @@ static void show_line(struct grep_opt *opt,\n \t\t\telse if (sign == '=')\n \t\t\t\tline_color = opt->colors[GREP_COLOR_FUNCTION];\n \t\t}\n-\t\twhile (next_match(opt, bol, eol, ctx, &match, eflags)) {\n+\t\twhile (grep_next_match(opt, bol, eol, ctx, &match,\n+\t\t\t\t       GREP_HEADER_FIELD_MAX, eflags)) {\n \t\t\tif (match.rm_so == match.rm_eo)\n \t\t\t\tbreak;\n \ndiff --git a/grep.h b/grep.h\nindex 3cb8a83ae8..4847c37280 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -23,6 +23,15 @@ typedef int pcre2_general_context;\n #include \"thread-utils.h\"\n #include \"userdiff.h\"\n \n+static const struct {\n+\tconst char *field;\n+\tsize_t len;\n+} grep_header_fields[] = {\n+\t{ \"author \", 7 },\n+\t{ \"committer \", 10 },\n+\t{ \"reflog \", 7 },\n+};\n+\n struct repository;\n \n enum grep_pat_token {\n@@ -190,6 +199,10 @@ void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const\n void compile_grep_patterns(struct grep_opt *opt);\n void free_grep_patterns(struct grep_opt *opt);\n int grep_buffer(struct grep_opt *opt, const char *buf, unsigned long size);\n+int grep_next_match(struct grep_opt *opt,\n+\t\t    const char *bol, const char *eol,\n+\t\t    enum grep_context ctx, regmatch_t *pmatch,\n+\t\t    enum grep_header_field field, int eflags);\n \n struct grep_source {\n \tchar *name;\n-- \n2.33.0\n\n"},{"id":"436669","messageId":"20210921211324.1426938-2-someguy@effective-light.com","threadId":"56560","inReplyTo":"20210921211324.1426938-1-someguy@effective-light.com","subject":"[PATCH v7 2/2] pretty: colorize pattern matches in commit messages","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-21T21:13:24Z","receivedAt":"2021-09-21T21:13:38Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"The \"git log\" command limits its output to the commits that contain strings\nmatched by a pattern when the \"--grep=<pattern>\" option is used, but unlike\noutput from \"git grep -e <pattern>\", the matches are not highlighted,\nmaking them harder to spot.\n\nTeach the pretty-printer code to highlight matches from the\n\"--grep=<pattern>\", \"--author=<pattern>\" and \"--committer=<pattern>\"\noptions (to view the last one, you may have to ask for --pretty=fuller).\n\nAlso, it must be noted that we are effectively greping the content twice,\nhowever it only slows down \"git log --author=^H\" on this repository by\naround 1-2% (compared to v2.33.0), so it should be a small enough slow\ndown to justify the addition of the feature.\n\nSigned-off-by: Hamza Mahfooz <someguy@effective-light.com>\n---\nv2: make the commit message whole (add the missing ingredients), rename\n    append_matched_line() to append_line_with_color(), use\n    colors[GREP_COLOR_MATCH_SELECTED] instead of\n    colors[GREP_COLOR_MATCH_CONTEXT], allow the background color to be\n    customized, don't copy strings to a buffer when not coloring in\n    append_line_with_color(), rename next_match() to grep_next_match(),\n    repurpose grep_next_match()/match_one_pattern() for use in\n    append_line_with_color() (allowing us to remove duplicated matching\n    code in append_line_with_color()), document how to customize the\n    feature and modify some of the tests to fit the feature better.\n\nv3: fix a formatting issue with the added documentation.\n\nv4: add strbuf_add_with_color(), use the correct color code scheme in the\n    unit tests and add more unit tests.\n\nv5: separate grep changes from pretty changes and add some performance\n    analysis in the commit message.\n\nv6: put the documentation in the correct place, cleanup pretty.c and\n    format the unit tests according to the current convention.\n\nv7: get rid of all manual strbuf management, constify where now appropriate\n    and fix the header line prefix issue properly.\n---\n Documentation/config/color.txt |   7 +-\n pretty.c                       | 113 +++++++++++++++++++++++++++++----\n t/t4202-log.sh                 |  51 +++++++++++++++\n 3 files changed, 157 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config/color.txt b/Documentation/config/color.txt\nindex e05d520a86..91d9a9da32 100644\n--- a/Documentation/config/color.txt\n+++ b/Documentation/config/color.txt\n@@ -104,9 +104,12 @@ color.grep.<slot>::\n `matchContext`;;\n \tmatching text in context lines\n `matchSelected`;;\n-\tmatching text in selected lines\n+\tmatching text in selected lines. Also, used to customize the following\n+\tlinkgit:git-log[1] subcommands: `--grep`, `--author` and `--committer`.\n `selected`;;\n-\tnon-matching text in selected lines\n+\tnon-matching text in selected lines. Also, used to customize the\n+\tfollowing linkgit:git-log[1] subcommands: `--grep`, `--author` and\n+\t`--committer`.\n `separator`;;\n \tseparators between fields on a line (`:`, `-`, and `=`)\n \tand between hunks (`--`)\ndiff --git a/pretty.c b/pretty.c\nindex 73b5ead509..2e5b02081e 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -431,6 +431,62 @@ const char *show_ident_date(const struct ident_split *ident,\n \treturn show_date(date, tz, mode);\n }\n \n+static inline void strbuf_add_with_color(struct strbuf *sb, const char *color,\n+\t\t\t\t\t const char *buf, size_t buflen)\n+{\n+\tstrbuf_addstr(sb, color);\n+\tstrbuf_add(sb, buf, buflen);\n+\tif (*color)\n+\t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n+}\n+\n+static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt,\n+\t\t\t\t   const char *line, size_t linelen,\n+\t\t\t\t   int color, enum grep_context ctx,\n+\t\t\t\t   enum grep_header_field field)\n+{\n+\tconst char *buf, *eol, *line_color, *match_color;\n+\tregmatch_t match;\n+\tint eflags = 0;\n+\n+\tbuf = line;\n+\teol = buf + linelen;\n+\n+\tif (!opt || !want_color(color) || opt->invert)\n+\t\tgoto end;\n+\n+\tline_color = opt->colors[GREP_COLOR_SELECTED];\n+\tmatch_color = opt->colors[GREP_COLOR_MATCH_SELECTED];\n+\n+\twhile (grep_next_match(opt, buf, eol, ctx, &match, field, eflags)) {\n+\t\tif (match.rm_so == match.rm_eo)\n+\t\t\tbreak;\n+\n+\t\tif (!eflags && (field < GREP_HEADER_FIELD_MAX)) {\n+\t\t\tint len = grep_header_fields[field].len;\n+\n+\t\t\tbuf += len;\n+\t\t\tmatch.rm_so -= len;\n+\t\t\tmatch.rm_eo -= len;\n+\t\t}\n+\n+\t\tstrbuf_add_with_color(sb, line_color, buf, match.rm_so);\n+\t\tstrbuf_add_with_color(sb, match_color, buf + match.rm_so,\n+\t\t\t\t      match.rm_eo - match.rm_so);\n+\t\tbuf += match.rm_eo;\n+\t\teflags = REG_NOTBOL;\n+\t}\n+\n+\tif (eflags)\n+\t\tstrbuf_add_with_color(sb, line_color, buf, eol - buf);\n+\telse {\n+end:\n+\t\tif (field < GREP_HEADER_FIELD_MAX)\n+\t\t\tbuf += grep_header_fields[field].len;\n+\t\tstrbuf_add(sb, buf, eol - buf);\n+\t}\n+}\n+\n void pp_user_info(struct pretty_print_context *pp,\n \t\t  const char *what, struct strbuf *sb,\n \t\t  const char *line, const char *encoding)\n@@ -496,9 +552,28 @@ void pp_user_info(struct pretty_print_context *pp,\n \t\t\tstrbuf_addch(sb, '\\n');\n \t\tstrbuf_addf(sb, \" <%.*s>\\n\", (int)maillen, mailbuf);\n \t} else {\n-\t\tstrbuf_addf(sb, \"%s: %.*s%.*s <%.*s>\\n\", what,\n-\t\t\t    (pp->fmt == CMIT_FMT_FULLER) ? 4 : 0, \"    \",\n-\t\t\t    (int)namelen, namebuf, (int)maillen, mailbuf);\n+\t\tstruct strbuf id = STRBUF_INIT;\n+\t\tenum grep_header_field field = GREP_HEADER_FIELD_MAX;\n+\t\tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n+\n+\t\tif (!strcmp(what, \"Author\"))\n+\t\t\tfield = GREP_HEADER_AUTHOR;\n+\t\telse if (!strcmp(what, \"Commit\"))\n+\t\t\tfield = GREP_HEADER_COMMITTER;\n+\n+\t\tstrbuf_addf(sb, \"%s: \", what);\n+\t\tif (pp->fmt == CMIT_FMT_FULLER)\n+\t\t\tstrbuf_addchars(sb, ' ', 4);\n+\n+\t\tif (field < GREP_HEADER_FIELD_MAX)\n+\t\t\tstrbuf_addstr(&id, grep_header_fields[field].field);\n+\t\tstrbuf_addf(&id, \"%.*s <%.*s>\", (int)namelen, namebuf,\n+\t\t\t    (int)maillen, mailbuf);\n+\n+\t\tappend_line_with_color(sb, opt, id.buf, id.len, pp->color,\n+\t\t\t\t       GREP_CONTEXT_HEAD, field);\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\tstrbuf_release(&id);\n \t}\n \n \tswitch (pp->fmt) {\n@@ -1939,8 +2014,9 @@ static int pp_utf8_width(const char *start, const char *end)\n \treturn width;\n }\n \n-static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n-\t\t\t\t const char *line, int linelen)\n+static void strbuf_add_tabexpand(struct strbuf *sb, struct grep_opt *opt,\n+\t\t\t\t int color, int tabwidth, const char *line,\n+\t\t\t\t int linelen)\n {\n \tconst char *tab;\n \n@@ -1957,7 +2033,9 @@ static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n \t\t\tbreak;\n \n \t\t/* Output the data .. */\n-\t\tstrbuf_add(sb, line, tab - line);\n+\t\tappend_line_with_color(sb, opt, line, tab - line, color,\n+\t\t\t\t       GREP_CONTEXT_BODY,\n+\t\t\t\t       GREP_HEADER_FIELD_MAX);\n \n \t\t/* .. and the de-tabified tab */\n \t\tstrbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));\n@@ -1972,7 +2050,8 @@ static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n \t * worrying about width - there's nothing more to\n \t * align.\n \t */\n-\tstrbuf_add(sb, line, linelen);\n+\tappend_line_with_color(sb, opt, line, linelen, color, GREP_CONTEXT_BODY,\n+\t\t\t       GREP_HEADER_FIELD_MAX);\n }\n \n /*\n@@ -1984,11 +2063,16 @@ static void pp_handle_indent(struct pretty_print_context *pp,\n \t\t\t     struct strbuf *sb, int indent,\n \t\t\t     const char *line, int linelen)\n {\n+\tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n+\n \tstrbuf_addchars(sb, ' ', indent);\n \tif (pp->expand_tabs_in_log)\n-\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log, line, linelen);\n+\t\tstrbuf_add_tabexpand(sb, opt, pp->color, pp->expand_tabs_in_log,\n+\t\t\t\t     line, linelen);\n \telse\n-\t\tstrbuf_add(sb, line, linelen);\n+\t\tappend_line_with_color(sb, opt, line, linelen, pp->color,\n+\t\t\t\t       GREP_CONTEXT_BODY,\n+\t\t\t\t       GREP_HEADER_FIELD_MAX);\n }\n \n static int is_mboxrd_from(const char *line, int len)\n@@ -2006,7 +2090,9 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t  struct strbuf *sb,\n \t\t  int indent)\n {\n+\tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n \tint first = 1;\n+\n \tfor (;;) {\n \t\tconst char *line = *msg_p;\n \t\tint linelen = get_one_line(line);\n@@ -2027,14 +2113,17 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\tif (indent)\n \t\t\tpp_handle_indent(pp, sb, indent, line, linelen);\n \t\telse if (pp->expand_tabs_in_log)\n-\t\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n-\t\t\t\t\t     line, linelen);\n+\t\t\tstrbuf_add_tabexpand(sb, opt, pp->color,\n+\t\t\t\t\t     pp->expand_tabs_in_log, line,\n+\t\t\t\t\t     linelen);\n \t\telse {\n \t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n \t\t\t\t\tis_mboxrd_from(line, linelen))\n \t\t\t\tstrbuf_addch(sb, '>');\n \n-\t\t\tstrbuf_add(sb, line, linelen);\n+\t\t\tappend_line_with_color(sb, opt, line, linelen,\n+\t\t\t\t\t       pp->color, GREP_CONTEXT_BODY,\n+\t\t\t\t\t       GREP_HEADER_FIELD_MAX);\n \t\t}\n \t\tstrbuf_addch(sb, '\\n');\n \t}\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 9dfead936b..3d240bba57 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -449,6 +449,57 @@ test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurati\n \t)\n '\n \n+test_expect_success 'log --author' '\n+\tcat >expect <<-\\EOF &&\n+\tAuthor: <BOLD;RED>A U<RESET> Thor <author@example.com>\n+\tEOF\n+\tgit log -1 --color=always --author=\"A U\" >log &&\n+\tgrep Author log >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --committer' '\n+\tcat >expect <<-\\EOF &&\n+\tCommit:     C O Mitter <committer@<BOLD;RED>example<RESET>.com>\n+\tEOF\n+\tgit log -1 --color=always --pretty=fuller --committer=\"example\" >log &&\n+\tgrep \"Commit:\" log >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log -i --grep with color' '\n+\tcat >expect <<-\\EOF &&\n+\t    <BOLD;RED>Sec<RESET>ond\n+\t    <BOLD;RED>sec<RESET>ond\n+\tEOF\n+\tgit log --color=always -i --grep=^sec >log &&\n+\tgrep -i sec log >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '-c color.grep.selected log --grep' '\n+\tcat >expect <<-\\EOF &&\n+\t    <GREEN>th<RESET><BOLD;RED>ir<RESET><GREEN>d<RESET>\n+\tEOF\n+\tgit -c color.grep.selected=\"green\" log --color=always --grep=ir >log &&\n+\tgrep ir log >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '-c color.grep.matchSelected log --grep' '\n+\tcat >expect <<-\\EOF &&\n+\t    <BLUE>i<RESET>n<BLUE>i<RESET>t<BLUE>i<RESET>al\n+\tEOF\n+\tgit -c color.grep.matchSelected=\"blue\" log --color=always --grep=i >log &&\n+\tgrep al log >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat > expect <<EOF\n * Second\n * sixth\n-- \n2.33.0\n\n"},{"id":"436873","messageId":"xmqq8rzn2ohj.fsf@gitster.g","threadId":"56560","inReplyTo":"20210921211324.1426938-1-someguy@effective-light.com","subject":"Re: [PATCH v7 1/2] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T17:25:28Z","receivedAt":"2021-09-23T17:25:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hamza Mahfooz <someguy@effective-light.com> writes:\n\n> These changes are made in preparation of, the colorization support for the\n> \"git log\" subcommands that, rely on regex functionality (i.e. \"--author\",\n> \"--committer\" and \"--grep\"). These changes are necessary primarily because\n> match_one_pattern() expects header lines to be prefixed, however, in\n> pretty, the prefixes are stripped from the lines because the name-email\n> pairs needs to go through additional parsing, before they can be printed\n> and because next_match() doesn't handle the case of\n> \"ctx == GREP_CONTEXT_HEAD\" at all. So, teach next_match() how to handle the\n> new case, move header_field[] so it can be used by pretty to reappend\n> relevant prefixes and teach match_one_pattern() how to handle subsequent\n> header line match attempts.\n>\n> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>\n> ---\n> v5: separate grep changes from pretty changes.\n>\n> v6: rescope some variables.\n>\n> v7: export header_field[] and allow for subsequent matches on header lines\n>     in match_one_pattern().\n> ---\n>  grep.c | 53 ++++++++++++++++++++++++++++-------------------------\n>  grep.h | 13 +++++++++++++\n>  2 files changed, 41 insertions(+), 25 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 14fe8a0fd2..f4126011c5 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -935,15 +935,6 @@ static void strip_timestamp(const char *bol, const char **eol_p)\n>  \t}\n>  }\n>  \n> -static struct {\n> -\tconst char *field;\n> -\tsize_t len;\n> -} header_field[] = {\n> -\t{ \"author \", 7 },\n> -\t{ \"committer \", 10 },\n> -\t{ \"reflog \", 7 },\n> -};\n> -\n>  static int match_one_pattern(struct grep_pat *p,\n>  \t\t\t     const char *bol, const char *eol,\n>  \t\t\t     enum grep_context ctx,\n> @@ -953,18 +944,23 @@ static int match_one_pattern(struct grep_pat *p,\n>  \tconst char *start = bol;\n>  \n>  \tif ((p->token != GREP_PATTERN) &&\n> -\t    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)))\n> +\t    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)) &&\n> +\t    ((p->token == GREP_PATTERN_BODY) != (ctx == GREP_CONTEXT_BODY)))\n>  \t\treturn 0;\n>  \n>  \tif (p->token == GREP_PATTERN_HEAD) {\n> -\t\tconst char *field;\n> -\t\tsize_t len;\n> -\t\tassert(p->field < ARRAY_SIZE(header_field));\n> -\t\tfield = header_field[p->field].field;\n> -\t\tlen = header_field[p->field].len;\n> -\t\tif (strncmp(bol, field, len))\n> -\t\t\treturn 0;\n> -\t\tbol += len;\n> +\t\tif (!(eflags & REG_NOTBOL)) {\n> +\t\t\tconst char *field;\n> +\t\t\tsize_t len;\n> +\n> +\t\t\tassert(p->field < ARRAY_SIZE(grep_header_fields));\n> +\t\t\tfield = grep_header_fields[p->field].field;\n> +\t\t\tlen = grep_header_fields[p->field].len;\n> +\t\t\tif (strncmp(bol, field, len))\n> +\t\t\t\treturn 0;\n> +\t\t\tbol += len;\n> +\t\t}\n> +\n>  \t\tswitch (p->field) {\n>  \t\tcase GREP_HEADER_AUTHOR:\n>  \t\tcase GREP_HEADER_COMMITTER:\n>\t\t\tsaved_ch = strip_timestamp(bol, &eol);\n\nLet's first see if we correctly understand how the original was\ndesigned to be used.  It is called once per line, with \"bol\" set\nto the true beginning of the line.\n\nFor a line in the header (e.g. \"author A U Thor <au@th.or> 12345678\n+0000\"), therefore, we ensure we are looking at the right type of\nthe header (e.g. when looking for an \"author\" line, the line must\nbegin with \"author \"), or we return 0 (i.e. does not match).  And\nthen, for \"author\" and \"committer\" line, we adjust the eol to\nexclude the timestamp part from pattern the matching by calling\nstrip_timetamp().\n\nBoth of these special case used to be unconditional because we _knew_\nthat the caller would call this function with \"bol\" pointing at the\ntrue beginning of the line.\n\nThe patch adds a new !(eflags & REG_NOTBOL) guard, presumably\nbecause new callers will start calling this function with \"bol\" set\nto point in the middle of a line?  So we only do the \"check the kind\nof header and skip a line that records the commiter when we are\nlooking for an author\" part when the guard passes, i.e. we know that\nthe caller called us with the true beginning of the line in bol.\n\nBut how would the new caller that points \"bol\" at middle of a line\nmake sure that we are looking at the right kind of header?  If the\npattern p is set to match only for an author line, the first call\nwith \"bol\" set to the true beginning of the line will correctly\nreject a \"committer\" header, but because you lost the sanity check\nabove, the second and subsequent one will go ahead and scan for the\npattern p on the line, even if p->field asks for author line and the\nline records the committer.  You'd end up finding a commit object\nthat is committed by (but not authored by) the person when you are\nlooking for a commit that was authored by somebody, no?  \n\nIf you ask for commits by somebody (e.g. \"--author=Hazma\") with an\noutput format that shows both the author and the committer\n(e.g. \"log --pretty=fuller\"), wouldn't your \"hit coloring\" code\nshow Hazma on the committer name as a grep hit, too?\n\nAlso, because the function is designed to be called with bol and eol\nset to true beginning and the end of the line, the strip_timestamp()\nside of the logic should also be done only once, but that is not\nwhat is happening in the patch.  Chomping the line immediately after\nthe last occurrence of '>' is idempotent, so it is OK to have the\nlogic outside the new !(eflags & REG_NOTBOL) guard, but discarding\nthe updated eol from the first call and not reusing the result of\nthe strip_timestamp() in the second and subsequent call smells like\na waste.\n\nI am having a feeling that you'd need to make the caller of this\nfunction (either the direct callers, or you may want to introduce\nanother layer between the current direct callers and this function)\nperform these massaging for the prefix and trailing parts of the\nline for GREP_PATTERN_HEAD patterns, so that this function, if you\nwere to change it to be callable many times starting at a middle of\nthe line, only needs to care about matching the pattern in the\nportion between (bol, eol) and nothing else.\n\n"},{"id":"436944","messageId":"HJTXZQ.ONLOCDR1B7O51@effective-light.com","threadId":"56560","inReplyTo":"xmqq8rzn2ohj.fsf@gitster.g","subject":"Re: [PATCH v7 1/2] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-24T12:04:29Z","receivedAt":"2021-09-24T12:04:40Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"\nOn Thu, Sep 23 2021 at 10:25:28 AM -0700, Junio C Hamano \n<gitster@pobox.com> wrote:\n> But how would the new caller that points \"bol\" at middle of a line\n> make sure that we are looking at the right kind of header?  If the\n> pattern p is set to match only for an author line, the first call\n> with \"bol\" set to the true beginning of the line will correctly\n> reject a \"committer\" header, but because you lost the sanity check\n> above, the second and subsequent one will go ahead and scan for the\n> pattern p on the line, even if p->field asks for author line and the\n> line records the committer.  You'd end up finding a commit object\n> that is committed by (but not authored by) the person when you are\n> looking for a commit that was authored by somebody, no?\n> \n> If you ask for commits by somebody (e.g. \"--author=Hazma\") with an\n> output format that shows both the author and the committer\n> (e.g. \"log --pretty=fuller\"), wouldn't your \"hit coloring\" code\n> show Hazma on the committer name as a grep hit, too?\n\nActually, this issue doesn't arise because I filter away the irrelevant\n(header) patterns in grep_next_match(). However, maybe it's a better \nidea\nto handle that in match_one_pattern().\n\n\n"}]}