{"thread":{"id":"56550","subject":"[PATCH v6 1/2] grep: refactor next_match() and match_one_pattern() for external use","startedAt":"2021-09-21T00:32:57Z","lastAt":"2021-09-21T03:31:07Z","messageCount":10,"participants":["Hamza Mahfooz","Jeff King"],"isPatch":true,"patchVersion":6,"patchTotal":2},"messages":[{"id":"436478","messageId":"20210921003050.641393-1-someguy@effective-light.com","threadId":"56550","inReplyTo":null,"subject":"[PATCH v6 1/2] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-21T00:30:49Z","receivedAt":"2021-09-21T00:32:57Z","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\nthe format of \"bol\" differs in the context that I require to use\nmatch_one_pattern() in and because next_match() doesn't handle the case of\n\"ctx == GREP_CONTEXT_HEAD\" at all. So, teach each function how to handle\nthe new cases.\n\nSigned-off-by: Hamza Mahfooz <someguy@effective-light.com>\n---\nv5: separate grep changes from pretty changes.\n\nv6: rescope some variables.\n---\n grep.c | 50 +++++++++++++++++++++++++++++++++++---------------\n grep.h |  3 +++\n 2 files changed, 38 insertions(+), 15 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 424a39591b..2901233865 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -956,26 +956,34 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\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\tconst char *end = eol;\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+\t\t\tif (eol == end)\n+\t\t\t\tgoto again;\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tbreak;\n \t\t}\n+\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+\n+\t\tif (strncmp(bol, field, len))\n+\t\t\tgoto restore;\n+\n+\t\tbol += len;\n \t}\n \n  again:\n@@ -1021,12 +1029,17 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \t\t\t\tgoto again;\n \t\t}\n \t}\n-\tif (p->token == GREP_PATTERN_HEAD && saved_ch)\n-\t\t*eol = saved_ch;\n+\n \tif (hit) {\n \t\tpmatch[0].rm_so += bol - start;\n \t\tpmatch[0].rm_eo += bol - start;\n \t}\n+\n+restore:\n+\tif (p->token == GREP_PATTERN_HEAD && saved_ch)\n+\t\t*eol = saved_ch;\n+\n+\n \treturn hit;\n }\n \n@@ -1159,21 +1172,27 @@ static int match_next_pattern(struct grep_pat *p, char *bol, char *eol,\n \treturn 1;\n }\n \n-static int next_match(struct grep_opt *opt, char *bol, char *eol,\n-\t\t      enum grep_context ctx, regmatch_t *pmatch, int eflags)\n+int grep_next_match(struct grep_opt *opt, char *bol, 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@@ -1262,7 +1281,8 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n \t\t\t\tline_color = opt->colors[GREP_COLOR_FUNCTION];\n \t\t}\n \t\t*eol = '\\0';\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 72f82b1e30..d2943e29ea 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -177,6 +177,9 @@ 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, char *buf, unsigned long size);\n+int grep_next_match(struct grep_opt *opt, char *bol, 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":"436479","messageId":"20210921003050.641393-2-someguy@effective-light.com","threadId":"56550","inReplyTo":"20210921003050.641393-1-someguy@effective-light.com","subject":"[PATCH v6 2/2] pretty: colorize pattern matches in commit messages","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-21T00:30:50Z","receivedAt":"2021-09-21T00:32:59Z","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---\n Documentation/config/color.txt |   7 ++-\n pretty.c                       | 107 +++++++++++++++++++++++++++++----\n t/t4202-log.sh                 |  51 ++++++++++++++++\n 3 files changed, 151 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..943a2d2ee2 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -431,6 +431,56 @@ 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 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+\tchar *buf, *eol;\n+\tconst char *line_color, *match_color;\n+\tregmatch_t match;\n+\tint eflags = 0;\n+\n+\tif (!opt || !want_color(color) || opt->invert) {\n+\t\tstrbuf_add(sb, line, linelen);\n+\t\treturn;\n+\t}\n+\n+\tbuf = (char *)line;\n+\teol = buf + linelen;\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\tstrbuf_grow(sb, strlen(line_color) + strlen(match_color) +\n+\t\t\t    (2 * strlen(GIT_COLOR_RESET)));\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_grow(sb, strlen(line_color) + strlen(GIT_COLOR_RESET));\n+\t\tstrbuf_add_with_color(sb, line_color, buf, eol - buf);\n+\t} else\n+\t\tstrbuf_add(sb, buf, eol - buf);\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 +546,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;\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\tstrbuf_init(&id, namelen + maillen + 4);\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\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 +2008,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 +2027,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 +2044,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 +2057,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 +2084,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 +2107,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":"436484","messageId":"YUkyJjaTfDcjakIe@coredump.intra.peff.net","threadId":"56550","inReplyTo":"20210921003050.641393-1-someguy@effective-light.com","subject":"Re: [PATCH v6 1/2] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T01:15:18Z","receivedAt":"2021-09-21T01:17:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 20, 2021 at 08:30:49PM -0400, Hamza Mahfooz wrote:\n\n> diff --git a/grep.c b/grep.c\n> index 424a39591b..2901233865 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -956,26 +956,34 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\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\tconst char *end = eol;\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> +\t\t\tif (eol == end)\n> +\t\t\t\tgoto again;\n\nI'm not sure if this part is right. If we didn't strip any timestamp,\nthen we jump to the \"again\" label, where we actually try to match the\npattern.\n\nBut that means we skip the part you deleted above, which got moved down\nhere:\n\n>  \t\t\tbreak;\n>  \t\tdefault:\n>  \t\t\tbreak;\n>  \t\t}\n> +\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> +\n> +\t\tif (strncmp(bol, field, len))\n> +\t\t\tgoto restore;\n> +\n> +\t\tbol += len;\n>  \t}\n\nAnd so we do not check that we have the right field at all. And as a\nresult, we may return nonsense results. For example, try this in\ngit.git:\n\n  git log -1 --author=junio 1462b67bc893fc845d28e2748c20357cb16a5ce3\n\nIt currently returns no results, because the match is case-sensitive (so\nit does not match \"Junio\" in the author field). But with your patch, it\nprints t hat commit (1462b67bc), because it matches a line buried in the\nmergetag header (\"tag post183-for-junio\").\n\nThat pattern is how I actually stumbled across it, but an even more\nobvious version is:\n\n  git log --author=commit\n\nCurrently that returns one result (somebody who has the word \"commit\" in\ntheir email address). But after your patch, it returns a ton of tag\nmerges (because they all have \"type commit\" in their mergetag headers).\n\n-Peff\n"},{"id":"436486","messageId":"YUk0OEXg36QXrkDm@coredump.intra.peff.net","threadId":"56550","inReplyTo":"20210921003050.641393-2-someguy@effective-light.com","subject":"Re: [PATCH v6 2/2] pretty: colorize pattern matches in commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T01:24:08Z","receivedAt":"2021-09-21T01:26:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 20, 2021 at 08:30:50PM -0400, Hamza Mahfooz wrote:\n\n> Teach the pretty-printer code to highlight matches from the\n> \"--grep=<pattern>\", \"--author=<pattern>\" and \"--committer=<pattern>\"\n> options (to view the last one, you may have to ask for --pretty=fuller).\n> \n> Also, it must be noted that we are effectively greping the content twice,\n> however it only slows down \"git log --author=^H\" on this repository by\n> around 1-2% (compared to v2.33.0), so it should be a small enough slow\n> down to justify the addition of the feature.\n\nThis might or might not be related, but one thing I noticed is that your\nearlier patch causes us to grep a lot more lines than we mean to (even\nif we are looking for \"author\" lines, it greps every header line). That\nmight contribute to the slowdown. Likewise, it calls strip_timestamp()\non every line, even if it does not start with \"author\").\n\n> +static inline void strbuf_add_with_color(struct strbuf *sb, const char *color,\n> +\t\t\t\t\t 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\nYou could take \"buf\" as a \"const char *\" here. That doesn't matter too\nmuch for now, but see below.\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> +\tchar *buf, *eol;\n> +\tconst char *line_color, *match_color;\n> +\tregmatch_t match;\n> +\tint eflags = 0;\n> +\n> +\tif (!opt || !want_color(color) || opt->invert) {\n> +\t\tstrbuf_add(sb, line, linelen);\n> +\t\treturn;\n> +\t}\n> +\n> +\tbuf = (char *)line;\n> +\teol = buf + linelen;\n\nOK, so we got rid of the copy of \"line\", which is nice. But we are\ncasting away const-ness, which is a potential red flag (is somebody\ngoing to modify this string, even though we promised our caller we would\nnot?). We'd probably want a comment to explain why we are doing so, and\nwhy it is OK (e.g., if somebody in the call stack modifies it\ntemporarily).\n\nMore on this in a moment.\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\tstrbuf_grow(sb, strlen(line_color) + strlen(match_color) +\n> +\t\t\t    (2 * strlen(GIT_COLOR_RESET)));\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\nAs Eric mentioned, these strbuf_grow() calls can go away. The whole\npoint of strbuf is that we do not have to clutter the code with manual\nsize computations, because it will do the right thing automatically.\n\nSometimes you can get extra performance by pre-sizing the strbuf, but:\n\n  1. I'd be surprised if we did in this case. We're writing into a\n     strbuf that will receive the whole per-commit output, so any growth\n     cost due to a couple of short strings here would be amortized\n     anyway.\n\n  2. The computation here doesn't represent the needed growth anyway.\n     When we call strbuf_add_with_color(), it's going to add not just\n     the colors but all of the data for the line itself.\n\nSo at best it's doing nothing, and at worst it is making the code harder\nto understand.\n\n> +\tif (eflags) {\n> +\t\tstrbuf_grow(sb, strlen(line_color) + strlen(GIT_COLOR_RESET));\n> +\t\tstrbuf_add_with_color(sb, line_color, buf, eol - buf);\n> +\t} else\n> +\t\tstrbuf_add(sb, buf, eol - buf);\n> +}\n\nDitto here (we grow for the colors, but also end up adding \"eol - buf\"\nbytes).\n\n-Peff\n"},{"id":"436490","messageId":"YUk3zwuse56v76ze@coredump.intra.peff.net","threadId":"56550","inReplyTo":"YUk0OEXg36QXrkDm@coredump.intra.peff.net","subject":"Re: [PATCH v6 2/2] pretty: colorize pattern matches in commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T01:39:27Z","receivedAt":"2021-09-21T01:44:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 20, 2021 at 09:24:08PM -0400, Jeff King wrote:\n\n> > +\tbuf = (char *)line;\n> > +\teol = buf + linelen;\n> \n> OK, so we got rid of the copy of \"line\", which is nice. But we are\n> casting away const-ness, which is a potential red flag (is somebody\n> going to modify this string, even though we promised our caller we would\n> not?). We'd probably want a comment to explain why we are doing so, and\n> why it is OK (e.g., if somebody in the call stack modifies it\n> temporarily).\n> \n> More on this in a moment.\n\nThe root of the issue is that grep_next_match() takes a non-const\nbuffer, and so on. And indeed, it _does_ eventually get modified,\nalthough only temporarily. I think we can clean that up, though.\n\nHere are two patches I prepared on top of your series to show what's\npossible, though I think we should do one of:\n\n  - put them at the front of your series (with the appropriate\n    adjustments) as preparatory cleanup\n\n  - keep them separate. You can put a comment above the cast to mention\n    what's going on and why it's OK for now, and then later when they're\n    both merged, we can remove that cast.\n\nThe second option creates a little extra work for the maintainer (they\nboth touch match_one_patter(), so there will be some textual conflicts).\nBut it does mean we avoid a dependencies; the cleanups don't derail your\nseries, nor does your series hold up the cleanups. So I could go either\nway.\n\n  [1/2]: grep: stop modifying buffer in strip_timestamp\n  [2/2]: grep: mark \"haystack\" buffers as const\n\n grep.c   | 30 ++++++++++++------------------\n grep.h   |  3 ++-\n pretty.c |  6 +++---\n 3 files changed, 17 insertions(+), 22 deletions(-)\n\n-Peff\n"},{"id":"436492","messageId":"YUk4R040RBc0aBIF@coredump.intra.peff.net","threadId":"56550","inReplyTo":"YUk3zwuse56v76ze@coredump.intra.peff.net","subject":"[PATCH 1/2] grep: stop modifying buffer in strip_timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T01:41:27Z","receivedAt":"2021-09-21T01:44:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When grepping for headers in commit objects, we receive individual\nlines (e.g., \"author Name <email> 1234 -0000\"), and then strip off the\ntimestamp to do our match. We do so by writing a NUL byte over the\nwhitespace separator, and then remembering to restore it later.\n\nWe had to do it this way when this was added back in a4d7d2c6db (log\n--author/--committer: really match only with name part, 2008-09-04),\nbecause we fed the result directly to regexec(), which expects a\nNUL-terminated string. But since b7d36ffca0 (regex: use regexec_buf(),\n2016-09-21), we have a function which can match on part of a buffer.\n\nSo instead of modifying the string, we can instead just move the \"eol\"\npointer, and the rest of the code will do the right thing. This will let\nthe next patch make more use of \"const\" in grep functions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think this is fairly safe. It would fail subtly if somebody _did_ rely\non the NUL, but we just call into patmatch(), which calls functions\nwhich handle the length-delimited buffer.\n\n grep.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 35708ce973..3b372ec29d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -922,20 +922,16 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n \treturn hit;\n }\n \n-static int strip_timestamp(char *bol, char **eol_p)\n+static void strip_timestamp(char *bol, char **eol_p)\n {\n \tchar *eol = *eol_p;\n-\tint ch;\n \n \twhile (bol < --eol) {\n \t\tif (*eol != '>')\n \t\t\tcontinue;\n \t\t*eol_p = ++eol;\n-\t\tch = *eol;\n-\t\t*eol = '\\0';\n-\t\treturn ch;\n+\t\tbreak;\n \t}\n-\treturn 0;\n }\n \n static struct {\n@@ -952,7 +948,6 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \t\t\t     regmatch_t *pmatch, int eflags)\n {\n \tint hit = 0;\n-\tint saved_ch = 0;\n \tconst char *start = bol;\n \n \tif ((p->token != GREP_PATTERN) &&\n@@ -968,7 +963,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\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+\t\t\tstrip_timestamp(bol, &eol);\n \t\t\tif (eol == end)\n \t\t\t\tgoto again;\n \t\t\tbreak;\n@@ -981,7 +976,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \t\tlen = header_field[p->field].len;\n \n \t\tif (strncmp(bol, field, len))\n-\t\t\tgoto restore;\n+\t\t\treturn 0;\n \n \t\tbol += len;\n \t}\n@@ -1035,11 +1030,6 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \t\tpmatch[0].rm_eo += bol - start;\n \t}\n \n-restore:\n-\tif (p->token == GREP_PATTERN_HEAD && saved_ch)\n-\t\t*eol = saved_ch;\n-\n-\n \treturn hit;\n }\n \n-- \n2.33.0.1023.gc687d0d3c8\n\n"},{"id":"436493","messageId":"YUk4vnMQHApY99Lb@coredump.intra.peff.net","threadId":"56550","inReplyTo":"YUk3zwuse56v76ze@coredump.intra.peff.net","subject":"[PATCH 2/2] grep: mark \"haystack\" buffers as const","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T01:43:26Z","receivedAt":"2021-09-21T02:20:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're grepping in a buffer, we don't need to modify it. So we can\ntake \"const char *\" buffers, rather than \"char *\". This can avoid some\nawkward casts in our callers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis step should be quite safe, because we're not changing any behavior,\nand the compiler will alert us if we missed any spots.\n\nThere may be further cleanup possible that the compiler doesn't tell us\nabout (i.e., other callers which have \"char *\" but could themselves\ntighten to \"const char *\"), since that implicit conversion is OK without\na cast.\n\n grep.c   | 12 ++++++++----\n grep.h   |  3 ++-\n pretty.c |  6 +++---\n 3 files changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 3b372ec29d..2cb65d191f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -908,7 +908,8 @@ static void show_name(struct grep_opt *opt, const char *name)\n \topt->output(opt, opt->null_following_name ? \"\\0\" : \"\\n\", 1);\n }\n \n-static int patmatch(struct grep_pat *p, char *line, char *eol,\n+static int patmatch(struct grep_pat *p,\n+\t\t    const char *line, const char *eol,\n \t\t    regmatch_t *match, int eflags)\n {\n \tint hit;\n@@ -943,7 +944,8 @@ static struct {\n \t{ \"reflog \", 7 },\n };\n \n-static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\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 \t\t\t     regmatch_t *pmatch, int eflags)\n {\n@@ -1141,7 +1143,8 @@ static int match_line(struct grep_opt *opt, char *bol, char *eol,\n \treturn hit;\n }\n \n-static int match_next_pattern(struct grep_pat *p, char *bol, char *eol,\n+static int match_next_pattern(struct grep_pat *p,\n+\t\t\t      const char *bol, const char *eol,\n \t\t\t      enum grep_context ctx,\n \t\t\t      regmatch_t *pmatch, int eflags)\n {\n@@ -1162,7 +1165,8 @@ static int match_next_pattern(struct grep_pat *p, char *bol, char *eol,\n \treturn 1;\n }\n \n-int grep_next_match(struct grep_opt *opt, char *bol, char *eol,\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 {\ndiff --git a/grep.h b/grep.h\nindex b82e5de982..a1880899ba 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -190,7 +190,8 @@ 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, char *buf, unsigned long size);\n-int grep_next_match(struct grep_opt *opt, char *bol, char *eol,\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 \ndiff --git a/pretty.c b/pretty.c\nindex 943a2d2ee2..be4efd9364 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -432,7 +432,7 @@ const char *show_ident_date(const struct ident_split *ident,\n }\n \n static inline void strbuf_add_with_color(struct strbuf *sb, const char *color,\n-\t\t\t\t\t char *buf, size_t buflen)\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@@ -445,7 +445,7 @@ static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt,\n \t\t\t\t   int color, enum grep_context ctx,\n \t\t\t\t   enum grep_header_field field)\n {\n-\tchar *buf, *eol;\n+\tconst char *buf, *eol;\n \tconst char *line_color, *match_color;\n \tregmatch_t match;\n \tint eflags = 0;\n@@ -455,7 +455,7 @@ static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt,\n \t\treturn;\n \t}\n \n-\tbuf = (char *)line;\n+\tbuf = line;\n \teol = buf + linelen;\n \n \tline_color = opt->colors[GREP_COLOR_SELECTED];\n-- \n2.33.0.1023.gc687d0d3c8\n"},{"id":"436523","messageId":"YUk9+p1j3GNwZ1DL@coredump.intra.peff.net","threadId":"56550","inReplyTo":"YUk4vnMQHApY99Lb@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] grep: mark \"haystack\" buffers as const","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T02:05:46Z","receivedAt":"2021-09-21T02:22:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 20, 2021 at 09:43:26PM -0400, Jeff King wrote:\n\n> When we're grepping in a buffer, we don't need to modify it. So we can\n> take \"const char *\" buffers, rather than \"char *\". This can avoid some\n> awkward casts in our callers.\n\nSorry, this patch should have touched strip_timestamp(), too. I had\noriginally done it in the earlier patch, but because we pass a\npointer-to-pointer, the compiler got mad about the mis-matched type from\nthe caller. So I reverted it there, but forgot to add it back in here.\n\nAt any rate, these are mostly for illustration. They'd need a little\nrebasing if not put on top of your patches anyway.\n\n-Peff\n"},{"id":"436551","messageId":"MBJRZQ.9003XUH26Y7G2@effective-light.com","threadId":"56550","inReplyTo":"YUk3zwuse56v76ze@coredump.intra.peff.net","subject":"Re: [PATCH v6 2/2] pretty: colorize pattern matches in commit messages","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-21T02:38:10Z","receivedAt":"2021-09-21T02:59:05Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"\nOn Mon, Sep 20 2021 at 09:39:27 PM -0400, Jeff King <peff@peff.net> \nwrote:\n> Here are two patches I prepared on top of your series to show what's\n> possible, though I think we should do one of:\n> \n>   - put them at the front of your series (with the appropriate\n>     adjustments) as preparatory cleanup\n> \n>   - keep them separate. You can put a comment above the cast to \n> mention\n>     what's going on and why it's OK for now, and then later when \n> they're\n>     both merged, we can remove that cast.\n\nOption 1 is preferable from my perspective, in that case.\n\n\n"},{"id":"436553","messageId":"YUlOY1IDBL8iAZRm@coredump.intra.peff.net","threadId":"56550","inReplyTo":"MBJRZQ.9003XUH26Y7G2@effective-light.com","subject":"Re: [PATCH v6 2/2] pretty: colorize pattern matches in commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:15:47Z","receivedAt":"2021-09-21T03:31:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 20, 2021 at 10:38:10PM -0400, Hamza Mahfooz wrote:\n\n> \n> On Mon, Sep 20 2021 at 09:39:27 PM -0400, Jeff King <peff@peff.net> wrote:\n> > Here are two patches I prepared on top of your series to show what's\n> > possible, though I think we should do one of:\n> > \n> >   - put them at the front of your series (with the appropriate\n> >     adjustments) as preparatory cleanup\n> > \n> >   - keep them separate. You can put a comment above the cast to mention\n> >     what's going on and why it's OK for now, and then later when they're\n> >     both merged, we can remove that cast.\n> \n> Option 1 is preferable from my perspective, in that case.\n\nOK. The patches I showed were the minimum to get your series working,\nbut there's actually a bit more cleanup we can do. I'll post a new\nseries in a moment, and then you can build on top of that.\n\n-Peff\n"}]}