{"thread":{"id":"56425","subject":"[PATCH] pretty: colorize pattern matches in commit messages","startedAt":"2021-09-01T12:16:37Z","lastAt":"2021-09-01T23:26:40Z","messageCount":3,"participants":["Hamza Mahfooz","Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"434426","messageId":"20210901121616.2109658-1-someguy@effective-light.com","threadId":"56425","inReplyTo":null,"subject":"[PATCH] pretty: colorize pattern matches in commit messages","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-09-01T12:16:16Z","receivedAt":"2021-09-01T12:16:37Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"Currently, for example when\n\n  git log --grep=pattern\n\nis executed, the outputted commits that are matched by the pattern do not\nhave the relevant substring matches highlighted.\n\nSigned-off-by: Hamza Mahfooz <someguy@effective-light.com>\n---\n pretty.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 98 insertions(+), 11 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 9631529c10..2886916ae6 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -431,15 +431,80 @@ const char *show_ident_date(const struct ident_split *ident,\n \treturn show_date(date, tz, mode);\n }\n \n+static void append_matched_line(struct grep_opt *opt, const char *line,\n+\t\t\t\tsize_t linelen, enum grep_pat_token token,\n+\t\t\t\tint field, struct strbuf *sb)\n+{\n+\tstruct grep_pat *pat;\n+\tstruct strbuf tmp_sb;\n+\tregmatch_t tmp_match, match;\n+\tchar *buf, *eol, *color;\n+\tint cflags = 0;\n+\n+\tstrbuf_init(&tmp_sb, linelen + 1);\n+\tstrbuf_add(&tmp_sb, line, linelen);\n+\tbuf = tmp_sb.buf;\n+\teol = buf + linelen;\n+\n+\tif (!opt || !want_color(opt->color))\n+\t\tgoto skip;\n+\n+\tcolor = opt->colors[GREP_COLOR_MATCH_CONTEXT];\n+\n+\tfor (;;) {\n+\t\tmatch.rm_so = match.rm_eo = -1;\n+\n+\t\tfor (pat = (token == GREP_PATTERN_HEAD ?\n+\t\t\t    opt->header_list : opt->pattern_list);\n+\t\t     pat; pat = pat->next) {\n+\t\t\tif (pat->token == token &&\n+\t\t\t    (field == -1 || pat->field == field) &&\n+\t\t\t    !regexec(&pat->regexp, buf, 1, &tmp_match,\n+\t\t\t\t     cflags)) {\n+\n+\t\t\t\tif ((match.rm_so >= 0 && match.rm_eo >= 0) &&\n+\t\t\t\t    (tmp_match.rm_so > match.rm_so ||\n+\t\t\t\t     (tmp_match.rm_so == match.rm_so &&\n+\t\t\t\t      tmp_match.rm_eo < match.rm_eo)))\n+\t\t\t\t\tcontinue;\n+\n+\t\t\t\tmatch.rm_so = tmp_match.rm_so;\n+\t\t\t\tmatch.rm_eo = tmp_match.rm_eo;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (match.rm_so == match.rm_eo)\n+\t\t\tbreak;\n+\n+\t\tstrbuf_grow(sb, strlen(color) + strlen(GIT_COLOR_RESET));\n+\t\tstrbuf_add(sb, buf, match.rm_so);\n+\t\tstrbuf_add(sb, color, strlen(color));\n+\t\tstrbuf_add(sb, buf + match.rm_so,\n+\t\t\t   match.rm_eo - match.rm_so);\n+\t\tstrbuf_add(sb, GIT_COLOR_RESET,\n+\t\t\t   strlen(GIT_COLOR_RESET));\n+\t\tbuf += match.rm_eo;\n+\t\tcflags = REG_NOTBOL;\n+\t}\n+\n+skip:\n+\tstrbuf_add(sb, buf, eol - buf);\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+\tint field = -1;\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 +561,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_matched_line(opt, id.buf, id.len,\n+\t\t\t\t    GREP_PATTERN_HEAD, field, sb);\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\tstrbuf_release(&id);\n \t}\n \n \tswitch (pp->fmt) {\n@@ -1855,6 +1933,7 @@ static void pp_header(struct pretty_print_context *pp,\n \t}\n }\n \n+\n void pp_title_line(struct pretty_print_context *pp,\n \t\t   const char **msg_p,\n \t\t   struct strbuf *sb,\n@@ -1935,8 +2014,8 @@ 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 grep_opt *opt, struct strbuf *sb,\n+\t\t\t\t int tabwidth, const char *line, int linelen)\n {\n \tconst char *tab;\n \n@@ -1953,7 +2032,8 @@ 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_matched_line(opt, line, tab - line, GREP_PATTERN_BODY,\n+\t\t\t\t    -1, sb);\n \n \t\t/* .. and the de-tabified tab */\n \t\tstrbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));\n@@ -1968,7 +2048,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_matched_line(opt, line, linelen,\n+\t\t\t    GREP_PATTERN_BODY, -1, sb);\n }\n \n /*\n@@ -1980,11 +2061,14 @@ 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(opt, sb, pp->expand_tabs_in_log, line, linelen);\n \telse\n-\t\tstrbuf_add(sb, line, linelen);\n+\t\tappend_matched_line(opt, line, linelen, GREP_PATTERN_BODY, -1,\n+\t\t\t\t    sb);\n }\n \n static int is_mboxrd_from(const char *line, int len)\n@@ -2002,7 +2086,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@@ -2023,14 +2109,15 @@ 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\tstrbuf_add_tabexpand(opt, sb, pp->expand_tabs_in_log,\n \t\t\t\t\t     line, 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_matched_line(opt, line, linelen,\n+\t\t\t\t\t    GREP_PATTERN_BODY, -1, sb);\n \t\t}\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n-- \n2.33.0\n\n"},{"id":"434457","messageId":"612fb5b96b69a_35b832088a@natae.notmuch","threadId":"56425","inReplyTo":"20210901121616.2109658-1-someguy@effective-light.com","subject":"RE: [PATCH] pretty: colorize pattern matches in commit messages","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-09-01T17:17:45Z","receivedAt":"2021-09-01T17:17:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hamza Mahfooz wrote:\n> Currently, for example when\n> \n>   git log --grep=pattern\n> \n> is executed, the outputted commits that are matched by the pattern do not\n> have the relevant substring matches highlighted.\n> \n> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>\n> ---\n>  pretty.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++------\n\nCan you add some tests?\n\n-- \nFelipe Contreras\n"},{"id":"434480","messageId":"xmqqwnnznahe.fsf@gitster.g","threadId":"56425","inReplyTo":"20210901121616.2109658-1-someguy@effective-light.com","subject":"Re: [PATCH] pretty: colorize pattern matches in commit messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T23:26:37Z","receivedAt":"2021-09-01T23:26:40Z","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> Currently, for example when\n>\n>   git log --grep=pattern\n>\n> is executed, the outputted commits that are matched by the pattern do not\n> have the relevant substring matches highlighted.\n\nA proposed log message that stops after a description of the current\nstatus alone invites a \"so what?\" response.  While it may be so\nobvious to you why the current state is undesirable (after all, it\nmotivated you enough to write a patch to improve the situation), the\njob of the proposed message is to explain and convince others to\nagree what is wrong in the current state and what is a reasonable\ndesign to improve it.  Among these three ingredients, the latter two\nare missing from the above.\n\nBecause it is our convention to talk about the current status in\npresent tense first, you do not have to start the description with\n\"Currently,\".\n\nTaking the above two paragraphs together, perhaps:\n\n    The \"git log\" command limits its output to the commits that\n    contain strings that matched the \"--grep=<pattern>\" option, but\n    unlike output from \"git grep -e <pattern>\", the matches are not\n    highlighted, making them harder to spot.\n\n    Teach the pretty-printer code to highlight matches from the\n    \"--grep=<pattern>\", \"--author=<pattern>\" and\n    \"--committer=<pattern>\" options (to view the last one, you may\n    have to ask for --pretty=fuller).\n\nOr something like that.\n\n>  pretty.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++------\n>  1 file changed, 98 insertions(+), 11 deletions(-)\n\nThis new feature deserves to be tested.  I am surprised that we do\nnot have \"git log --grep\" tests whose expected output we need to\nadjust for this change.  Perhaps it is becuase we are not otherwise\nshowing colors at all in \"git log\" tests?\n\nIt also needs a documentation update, if only to tell readers how to\ncustomize the color used to paint the hits.\n\n> +static void append_matched_line(struct grep_opt *opt, const char *line,\n> +\t\t\t\tsize_t linelen, enum grep_pat_token token,\n> +\t\t\t\tint field, struct strbuf *sb)\n> +{\n> +\tstruct grep_pat *pat;\n> +\tstruct strbuf tmp_sb;\n> +\tregmatch_t tmp_match, match;\n> +\tchar *buf, *eol, *color;\n> +\tint cflags = 0;\n> +\n> +\tstrbuf_init(&tmp_sb, linelen + 1);\n> +\tstrbuf_add(&tmp_sb, line, linelen);\n> +\tbuf = tmp_sb.buf;\n> +\teol = buf + linelen;\n\nThis copy of the whole line is wasted when ...\n\n> +\tif (!opt || !want_color(opt->color))\n> +\t\tgoto skip;\n\n... we are not doing any coloring.  Can we avoid it?\n\n> +\tcolor = opt->colors[GREP_COLOR_MATCH_CONTEXT];\n\nWhy is the context (as opposed to selected) color used here?  \n\nIn general, when you allow the foreground color to be customized,\nyou must also make the background color customizable, so that the\nend user can avoid low contrast combinations.  If we look at how\nopt->color is handled in grep.c::show_line(), we can tell how both\nmatch_color (foregroud) and line_color (background) are taken from\nthe palette that the end user can customize.  We should do the same\nhere, without assuming that 'color' here will have a good contrast\nagainst the COLOR_RESET backdrop.\n\n> +\tfor (;;) {\n> +\t\tmatch.rm_so = match.rm_eo = -1;\n> +\n> +\t\tfor (pat = (token == GREP_PATTERN_HEAD ?\n> +\t\t\t    opt->header_list : opt->pattern_list);\n> +\t\t     pat; pat = pat->next) {\n> +\t\t\tif (pat->token == token &&\n> +\t\t\t    (field == -1 || pat->field == field) &&\n> +\t\t\t    !regexec(&pat->regexp, buf, 1, &tmp_match,\n> +\t\t\t\t     cflags)) {\n> +\n> +\t\t\t\tif ((match.rm_so >= 0 && match.rm_eo >= 0) &&\n> +\t\t\t\t    (tmp_match.rm_so > match.rm_so ||\n> +\t\t\t\t     (tmp_match.rm_so == match.rm_so &&\n> +\t\t\t\t      tmp_match.rm_eo < match.rm_eo)))\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tmatch.rm_so = tmp_match.rm_so;\n> +\t\t\t\tmatch.rm_eo = tmp_match.rm_eo;\n> +\t\t\t}\n> +\t\t}\n\nFor the current commit to come this far, \"git log --grep=<pattern>\"\nmust have done the above exact regexec() to decide if we need to\ncall this function in the first place, right?\n\nWe must be redoing the same computation here, which is unfortunate\nfor two reasons.  Performance and maintainability.\n\nHow much extra cycles are we looking at with this additional code?\nDepending on how inefficient this code makes, we may need to make it\nan optional feature, turned off by default.\n\nWorse yet, this can easily become a source of future bugs, since the\nabove matching logic must be kept in sync with the existing matching\ncode elsewhere.  I would not be surprised if the above logic is\nalready broken when various options that affects how pattern\nmatching works with \"log --grep\" (e.g. \"--invert-grep\", \"-E\", \"-i\")\nare in use.\n\nAlso, this function is misnamed.  It is not a function that appends\nmatched line.  From the caller's point of view, it is used to append\neach and every line it wants to add to sb [*], and they do not even\nneed or want to know how the callee decides which parts of the line\nto paint in different colors.  Perhaps append_line_with_color() or\nsomething?\n\n\tSide note.  By the way, why is the sb the last parameter to\n\tthis function?  Usually functions that operate on a strbuf\n\ttake it as the first argument.\n\n> +\t\tif (match.rm_so == match.rm_eo)\n> +\t\t\tbreak;\n> +\n> +\t\tstrbuf_grow(sb, strlen(color) + strlen(GIT_COLOR_RESET));\n> +\t\tstrbuf_add(sb, buf, match.rm_so);\n> +\t\tstrbuf_add(sb, color, strlen(color));\n> +\t\tstrbuf_add(sb, buf + match.rm_so,\n> +\t\t\t   match.rm_eo - match.rm_so);\n> +\t\tstrbuf_add(sb, GIT_COLOR_RESET,\n> +\t\t\t   strlen(GIT_COLOR_RESET));\n> +\t\tbuf += match.rm_eo;\n> +\t\tcflags = REG_NOTBOL;\n> +\t}\n> +\n> +skip:\n> +\tstrbuf_add(sb, buf, eol - buf);\n> +\n> +\tstrbuf_release(&tmp_sb);\n> +}\n> +\n\nI like what the new feature wants to do, but I am not sure if this\nis the best execution of the idea (yet).\n\nThanks.\n"}]}