{"thread":{"id":"56515","subject":"[PATCH v5 1/2] grep: refactor next_match() and match_one_pattern() for external use","startedAt":"2021-09-16T14:10:10Z","lastAt":"2021-09-17T22:16:02Z","messageCount":6,"participants":["Hamza Mahfooz","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":5,"patchTotal":2},"messages":[{"id":"436122","messageId":"20210916140943.2442184-1-someguy@effective-light.com","threadId":"56515","inReplyTo":null,"subject":"[PATCH v5 1/2] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-16T14:09:42Z","receivedAt":"2021-09-16T14:10:10Z","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---\n grep.c | 53 ++++++++++++++++++++++++++++++++++++-----------------\n grep.h |  3 +++\n 2 files changed, 39 insertions(+), 17 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 424a39591b..e77c8643d2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -951,31 +951,38 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \t\t\t     enum grep_context ctx,\n \t\t\t     regmatch_t *pmatch, int eflags)\n {\n+\tconst char *field;\n+\tsize_t len;\n \tint hit = 0;\n \tint saved_ch = 0;\n \tconst char *start = bol;\n+\tconst char *end = eol;\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\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 +1028,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 +1171,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 +1280,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":"436123","messageId":"20210916140943.2442184-2-someguy@effective-light.com","threadId":"56515","inReplyTo":"20210916140943.2442184-1-someguy@effective-light.com","subject":"[PATCH v5 2/2] pretty: colorize pattern matches in commit messages","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-16T14:09:43Z","receivedAt":"2021-09-16T14:10:13Z","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---\n Documentation/git-log.txt |   8 +++\n pretty.c                  | 110 +++++++++++++++++++++++++++++++++-----\n t/t4202-log.sh            |  55 +++++++++++++++++++\n 3 files changed, 161 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 0498e7bacb..c689f7d235 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -241,6 +241,14 @@ This setting can be disabled by the `--no-notes` option,\n overridden by the `GIT_NOTES_DISPLAY_REF` environment variable,\n and overridden by the `--notes=<ref>` option.\n \n+color.grep.selected::\n+\tDetermines the non matching text (background) color of selected lines,\n+\twhen `--grep`, `--author` or `--committer` are used.\n+\n+color.grep.matchSelected::\n+\tDetermines the matching text (foreground) color of selected lines, when\n+\t`--grep`, `--author` or `--committer` are used.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/pretty.c b/pretty.c\nindex 73b5ead509..77969a623b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -431,15 +431,74 @@ 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_add(sb, color, strlen(color));\n+\tstrbuf_add(sb, buf, buflen);\n+\tif (strlen(color))\n+\t\tstrbuf_add(sb, GIT_COLOR_RESET, strlen(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+\tstruct strbuf tmp_sb;\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+\tstrbuf_init(&tmp_sb, linelen + 1);\n+\tstrbuf_add(&tmp_sb, line, linelen);\n+\n+\tbuf = tmp_sb.buf;\n+\teol = buf + linelen;\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 (buf != line) {\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+\t}\n+\n+\tstrbuf_release(&tmp_sb);\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 {\n+\tstruct strbuf id;\n \tstruct ident_split ident;\n \tchar *line_end;\n \tconst char *mailbuf, *namebuf;\n \tsize_t namelen, maillen;\n \tint max_length = 78; /* per rfc2822 */\n+\tenum grep_header_field field = GREP_HEADER_FIELD_MAX;\n+\tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n \n \tif (pp->fmt == CMIT_FMT_ONELINE)\n \t\treturn;\n@@ -496,9 +555,22 @@ 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\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: %.*s\", what,\n+\t\t\t    (pp->fmt == CMIT_FMT_FULLER) ? 4 : 0, \"    \");\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 +2011,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 +2030,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 +2047,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 +2060,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 +2087,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 +2110,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..943c00e338 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -449,6 +449,61 @@ test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurati\n \t)\n '\n \n+cat > expect << EOF\n+Author: <BOLD;RED>A U<RESET> Thor <author@example.com>\n+EOF\n+\n+test_expect_success 'log --author' '\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+cat > expect << EOF\n+Commit:     C O Mitter <committer@<BOLD;RED>example<RESET>.com>\n+EOF\n+\n+test_expect_success 'log --committer' '\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+cat > expect << EOF\n+    <BOLD;RED>Sec<RESET>ond\n+    <BOLD;RED>sec<RESET>ond\n+EOF\n+\n+test_expect_success 'log -i --grep with color' '\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+cat > expect << EOF\n+    <GREEN>th<RESET><BOLD;RED>ir<RESET><GREEN>d<RESET>\n+EOF\n+\n+test_expect_success '-c color.grep.selected log --grep' '\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+cat > expect << EOF\n+    <BLUE>i<RESET>n<BLUE>i<RESET>t<BLUE>i<RESET>al\n+EOF\n+\n+test_expect_success '-c color.grep.matchSelected log --grep' '\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":"436214","messageId":"CAPig+cRfFZ=GAevJ0b9oBGTR8P3MnSg+R43ujSP0dZJL08riqA@mail.gmail.com","threadId":"56515","inReplyTo":"20210916140943.2442184-2-someguy@effective-light.com","subject":"Re: [PATCH v5 2/2] pretty: colorize pattern matches in commit messages","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-17T07:10:12Z","receivedAt":"2021-09-17T07:10:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 16, 2021 at 10:10 AM Hamza Mahfooz\n<someguy@effective-light.com> 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> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>\n\nA few relatively superficial comments below...\n\n> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\n> @@ -241,6 +241,14 @@ This setting can be disabled by the `--no-notes` option,\n> +color.grep.selected::\n> +       Determines the non matching text (background) color of selected lines,\n> +       when `--grep`, `--author` or `--committer` are used.\n> +\n> +color.grep.matchSelected::\n> +       Determines the matching text (foreground) color of selected lines, when\n> +       `--grep`, `--author` or `--committer` are used.\n\nShould these be documented instead in Documentation/config/color.txt\nas entries in the table under `color.grep.<slot>`?\n\n> diff --git a/pretty.c b/pretty.c\n> @@ -431,15 +431,74 @@ const char *show_ident_date(const struct ident_split *ident,\n> +static inline void strbuf_add_with_color(struct strbuf *sb, const char *color,\n> +                                        char *buf, size_t buflen)\n> +{\n> +       strbuf_add(sb, color, strlen(color));\n\nThere is no need to call strlen() here; instead use strbuf_addstr():\n\n    strbuf_addstr(sb, color);\n\n> +       strbuf_add(sb, buf, buflen);\n> +       if (strlen(color))\n> +               strbuf_add(sb, GIT_COLOR_RESET, strlen(GIT_COLOR_RESET));\n\nLikewise, no need for strlen() in the conditional or when adding the\nRESET color:\n\n    if (*color)\n        strbuf_addstr(sb, GIT_COLOR_RESET);\n\n> +}\n> +\n> +static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt,\n> +                                  const char *line, size_t linelen,\n> +                                  int color, enum grep_context ctx,\n> +                                  enum grep_header_field field)\n> +{\n> +       strbuf_init(&tmp_sb, linelen + 1);\n\nWhat is the +1 for? Is that to account for the NUL byte at the end of\nthe string? If so, there's no need to do so manually since strbuf will\ntake that into account itself.\n\n> +       strbuf_add(&tmp_sb, line, linelen);\n> +\n> +       buf = tmp_sb.buf;\n> +       eol = buf + linelen;\n\n`buf` and `eol` seem like an accident waiting to happen...\n\n> +       line_color = opt->colors[GREP_COLOR_SELECTED];\n> +       match_color = opt->colors[GREP_COLOR_MATCH_SELECTED];\n> +\n> +       while (grep_next_match(opt, buf, eol, ctx, &match, field, eflags)) {\n> +               if (match.rm_so == match.rm_eo)\n> +                       break;\n> +\n> +               strbuf_grow(sb, strlen(line_color) + strlen(match_color) +\n> +                           (2 * strlen(GIT_COLOR_RESET)));\n\n... because strbuf_grow() may reallocate the underlying buffer, which\nmeans that `buf` and `eol` will end up pointing at freed memory, which\nwill be accessed by the next call to grep_next_match().\n\nI also wonder if these manual calls to strbuf_grow() and the \"hint\"\npassed to strbuf_init() are actually helping considering that strbuf\nshould do a pretty good job of managing its underlying buffer growth\nwithout this sort of micromanagement. Have you done performance\ntesting which indicates that such manual management is beneficial (and\nthat this isn't a case of premature optimization)?\n\n> +               strbuf_add_with_color(sb, line_color, buf, match.rm_so);\n> +               strbuf_add_with_color(sb, match_color, buf + match.rm_so,\n> +                                     match.rm_eo - match.rm_so);\n> +               buf += match.rm_eo;\n> +               eflags = REG_NOTBOL;\n> +       }\n> +\n> +       if (buf != line) {\n> +               strbuf_grow(sb, strlen(line_color) + strlen(GIT_COLOR_RESET));\n> +               strbuf_add_with_color(sb, line_color, buf, eol - buf);\n> +       } else {\n> +               strbuf_add(sb, buf, eol - buf);\n> +       }\n\nI'm confused by this. How can `buf` ever equal `line` considering that\nthe above code does:\n\n    strbuf_add(&tmp_sb, line, linelen);\n    ...\n    buf = tmp_sb.buf;\n\nso that the strbuf contains a _copy_ of `line`, and `buf` is pointing\ninto the strbuf.\n\n> +       strbuf_release(&tmp_sb);\n> +}\n> +\n>  void pp_user_info(struct pretty_print_context *pp,\n>                   const char *what, struct strbuf *sb,\n>                   const char *line, const char *encoding)\n>  {\n> +       struct strbuf id;\n> +       enum grep_header_field field = GREP_HEADER_FIELD_MAX;\n> +       struct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n\nThese new variables only ever seem to be used...\n\n>         if (pp->fmt == CMIT_FMT_ONELINE)\n>                 return;\n> @@ -496,9 +555,22 @@ void pp_user_info(struct pretty_print_context *pp,\n>                         strbuf_addch(sb, '\\n');\n>                 strbuf_addf(sb, \" <%.*s>\\n\", (int)maillen, mailbuf);\n>         } else {\n> -               strbuf_addf(sb, \"%s: %.*s%.*s <%.*s>\\n\", what,\n> -                           (pp->fmt == CMIT_FMT_FULLER) ? 4 : 0, \"    \",\n> -                           (int)namelen, namebuf, (int)maillen, mailbuf);\n\n... within this block, so they should be declared here rather than in\nthe outer block in order to reduce their scope.\n\n> +               strbuf_init(&id, namelen + maillen + 4);\n\nAgain, I wonder if this micromanagement of strbuf allocation is a case\nof premature optimization.\n\n> +               if (!strcmp(what, \"Author\"))\n> +                       field = GREP_HEADER_AUTHOR;\n> +               else if (!strcmp(what, \"Commit\"))\n> +                       field = GREP_HEADER_COMMITTER;\n> +\n> +               strbuf_addf(sb, \"%s: %.*s\", what,\n> +                           (pp->fmt == CMIT_FMT_FULLER) ? 4 : 0, \"    \");\n\nIt's subjective, but this may be overly clever and unnecessarily\ncompact. The slightly more verbose:\n\n    strbuf_addf(sb, \"%s: \", what);\n    if (pp->fmt == CMIT_FMT_FULLER)\n        strbuf_addchars(sb, ' ', 4);\n\nis easier to understand at-a-glance, as is this equivalent:\n\n    strbuf_addf(sb, \"%s: \", what);\n    if (pp->fmt == CMIT_FMT_FULLER)\n        strbuf_addstr(sb, \"    \");\n\n> +               strbuf_addf(&id, \"%.*s <%.*s>\", (int)namelen, namebuf,\n> +                           (int)maillen, mailbuf);\n> +\n> +               append_line_with_color(sb, opt, id.buf, id.len, pp->color,\n> +                                      GREP_CONTEXT_HEAD, field);\n> +               strbuf_addch(sb, '\\n');\n> +               strbuf_release(&id);\n>         }\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> @@ -449,6 +449,61 @@ test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurati\n> +cat > expect << EOF\n> +Author: <BOLD;RED>A U<RESET> Thor <author@example.com>\n> +EOF\n> +\n> +test_expect_success 'log --author' '\n> +       git log -1 --color=always --author=\"A U\" >log &&\n> +       grep Author log >actual.raw &&\n> +       test_decode_color <actual.raw >actual &&\n> +       test_cmp expect actual\n> +'\n\nI realize that you're mirroring how this is done in a few existing\ntests in this script, but these days we like to place all code,\nincluding creation of the `expect` file, inside the test body. Also,\nstyle guidelines state that there shouldn't be any whitespace\nfollowing the `>` and `<<` operators. Finally, since no variables need\nto be interpolated into the here-doc content, we use `\\EOF` instead of\n`EOF`, and since we want to indent the here-doc content inside the\ntest body, we use `-\\EOF`. So:\n\n    test_expect_success 'log --author' '\n        cat >expect <<-\\EOF\n        Author: <BOLD;RED>A U<RESET> Thor <author@example.com>\n        EOF\n        ...\n"},{"id":"436260","messageId":"WCKLZQ.YCP2HWKW7YBB2@effective-light.com","threadId":"56515","inReplyTo":"CAPig+cRfFZ=GAevJ0b9oBGTR8P3MnSg+R43ujSP0dZJL08riqA@mail.gmail.com","subject":"Re: [PATCH v5 2/2] pretty: colorize pattern matches in commit messages","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-17T21:14:56Z","receivedAt":"2021-09-17T21:15:08Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"\nOn Fri, Sep 17 2021 at 03:10:12 AM -0400, Eric Sunshine \n<sunshine@sunshineco.com> wrote:\n> `buf` and `eol` seem like an accident waiting to happen...\n> \n>>  +       line_color = opt->colors[GREP_COLOR_SELECTED];\n>>  +       match_color = opt->colors[GREP_COLOR_MATCH_SELECTED];\n>>  +\n>>  +       while (grep_next_match(opt, buf, eol, ctx, &match, field, \n>> eflags)) {\n>>  +               if (match.rm_so == match.rm_eo)\n>>  +                       break;\n>>  +\n>>  +               strbuf_grow(sb, strlen(line_color) + \n>> strlen(match_color) +\n>>  +                           (2 * strlen(GIT_COLOR_RESET)));\n> \n> ... because strbuf_grow() may reallocate the underlying buffer, which\n> means that `buf` and `eol` will end up pointing at freed memory, which\n> will be accessed by the next call to grep_next_match().\n\nI don't see how it's problematic, since `tmp_sb` isn't modified after \n`buf`\nis initialized (until strbuf_release() is called, of course).\n\n\n"},{"id":"436271","messageId":"YUUQA1qNCzY7Vx+j@coredump.intra.peff.net","threadId":"56515","inReplyTo":"WCKLZQ.YCP2HWKW7YBB2@effective-light.com","subject":"Re: [PATCH v5 2/2] pretty: colorize pattern matches in commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-17T22:00:35Z","receivedAt":"2021-09-17T22:00:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 17, 2021 at 05:14:56PM -0400, Hamza Mahfooz wrote:\n\n> \n> On Fri, Sep 17 2021 at 03:10:12 AM -0400, Eric Sunshine\n> <sunshine@sunshineco.com> wrote:\n> > `buf` and `eol` seem like an accident waiting to happen...\n> > \n> > >  +       line_color = opt->colors[GREP_COLOR_SELECTED];\n> > >  +       match_color = opt->colors[GREP_COLOR_MATCH_SELECTED];\n> > >  +\n> > >  +       while (grep_next_match(opt, buf, eol, ctx, &match, field,\n> > > eflags)) {\n> > >  +               if (match.rm_so == match.rm_eo)\n> > >  +                       break;\n> > >  +\n> > >  +               strbuf_grow(sb, strlen(line_color) +\n> > > strlen(match_color) +\n> > >  +                           (2 * strlen(GIT_COLOR_RESET)));\n> > \n> > ... because strbuf_grow() may reallocate the underlying buffer, which\n> > means that `buf` and `eol` will end up pointing at freed memory, which\n> > will be accessed by the next call to grep_next_match().\n> \n> I don't see how it's problematic, since `tmp_sb` isn't modified after `buf`\n> is initialized (until strbuf_release() is called, of course).\n\nYes, you are correct. However, I do think the code would be much clearer\nif you skipped the strbuf entirely, like:\n\n  char *line_as_string;\n  ...\n  line_as_string = xmemdupz(line, linelen);\n  ...\n  buf = line_as_string;\n  eol = buf + linelen;\n  ...\n  free(line_as_string);\n\nwhich makes it much clearer that you don't intend to modify it further\n(especially with all those other calls operating on the _other_ strbuf,\nit's hard to see immediately that this is the case).\n\nThe \"as_string\" name is assuming the purpose is to get a NUL-terminated\nstring. I'm not sure why we need one, though, since we pass the buf/eol\npointers to grep_next_match(). A comment above the xmemdupz() line might\nbe a good place to explain that (which would help especially if the\nreasons change later and we can get rid of it).\n\n-Peff\n"},{"id":"436275","messageId":"CAPig+cTD22MM1vjXMSjVcNXfVWfqDgT7rDp1j-e_Fko33osATA@mail.gmail.com","threadId":"56515","inReplyTo":"YUUQA1qNCzY7Vx+j@coredump.intra.peff.net","subject":"Re: [PATCH v5 2/2] pretty: colorize pattern matches in commit messages","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-17T22:15:46Z","receivedAt":"2021-09-17T22:16:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 17, 2021 at 6:01 PM Jeff King <peff@peff.net> wrote:\n> On Fri, Sep 17, 2021 at 05:14:56PM -0400, Hamza Mahfooz wrote:\n> > On Fri, Sep 17 2021 at 03:10:12 AM -0400, Eric Sunshine\n> > <sunshine@sunshineco.com> wrote:\n> > > `buf` and `eol` seem like an accident waiting to happen...\n> > > ... because strbuf_grow() may reallocate the underlying buffer, which\n> > > means that `buf` and `eol` will end up pointing at freed memory, which\n> > > will be accessed by the next call to grep_next_match().\n> >\n> > I don't see how it's problematic, since `tmp_sb` isn't modified after `buf`\n> > is initialized (until strbuf_release() is called, of course).\n>\n> Yes, you are correct.\n\nIndeed, I got confused by the multiple strbufs with similar names. However...\n\n> However, I do think the code would be much clearer\n> if you skipped the strbuf entirely, like:\n>\n>   line_as_string = xmemdupz(line, linelen);\n>   ...\n>   buf = line_as_string;\n>   eol = buf + linelen;\n>\n> which makes it much clearer that you don't intend to modify it further\n> (especially with all those other calls operating on the _other_ strbuf,\n> it's hard to see immediately that this is the case).\n>\n> The \"as_string\" name is assuming the purpose is to get a NUL-terminated\n> string. I'm not sure why we need one, though, since we pass the buf/eol\n> pointers to grep_next_match(). A comment above the xmemdupz() line might\n> be a good place to explain that (which would help especially if the\n> reasons change later and we can get rid of it).\n\n... as Peff points out, it indeed is unclear why a copy of `line` is\nneeded at all. It seems like a simple:\n\n    buf = line;\n    eol = buf + linelen;\n\nwould more than suffice without making any copies. If you do that, then the:\n\n    if (buf != line) {\n\nlater in the code -- and which I questioned in my review as making no\nsense -- suddenly makes sense.\n"}]}