{"thread":{"id":"56553","subject":"[PATCH 0/5] const-correctness in grep.c","startedAt":"2021-09-21T03:45:48Z","lastAt":"2021-09-24T04:22:10Z","messageCount":32,"participants":["Jeff King","Taylor Blau","Carlo Arenas","Eric Sunshine","Carlo Marcelo Arenas Belón","René Scharfe","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"436556","messageId":"YUlVZk1xXulAqdef@coredump.intra.peff.net","threadId":"56553","inReplyTo":null,"subject":"[PATCH 0/5] const-correctness in grep.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:45:42Z","receivedAt":"2021-09-21T03:45:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While discussing [1], I noticed that the grep code mostly takes\nnon-const buffers, even though it is conceptually a read-only operation\nto search in them. The culprit is a handful of spots that temporarily\ntie off NUL-terminated strings by overwriting a byte of the buffer and\nthen restoring it. But I think we no longer need to do so these days,\nnow that we have a regexec_buf() that can take a ptr/size pair.\n\nThe first three patches are a bit repetitive, but I broke them up\nindividually because they're the high-risk part. I.e., if my assumptions\nabout needing the NUL are wrong, it could introduce a bug. But based on\nmy reading of the code, plus running the test suite with ASan/UBSan, I\nfeel reasonably confident.\n\nThe last two are the bigger cleanups, but should obviously avoid any\nbehavior changes.\n\n  [1/5]: grep: stop modifying buffer in strip_timestamp\n  [2/5]: grep: stop modifying buffer in show_line()\n  [3/5]: grep: stop modifying buffer in grep_source_1()\n  [4/5]: grep: mark \"haystack\" buffers as const\n  [5/5]: grep: store grep_source buffer as const\n\n grep.c | 87 +++++++++++++++++++++++++++++-----------------------------\n grep.h |  4 +--\n 2 files changed, 45 insertions(+), 46 deletions(-)\n\n-Peff\n\n[1] https://lore.kernel.org/git/YUk3zwuse56v76ze@coredump.intra.peff.net/\n"},{"id":"436557","messageId":"YUlVsLkFGRfRqpKG@coredump.intra.peff.net","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"[PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:46:56Z","receivedAt":"2021-09-21T03:46:59Z","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 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\nfurther patches mark more buffers as \"const\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n grep.c | 13 +++----------\n 1 file changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 79598f245f..5b1f2da4d3 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@@ -971,7 +966,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\tbreak;\n \t\tdefault:\n \t\t\tbreak;\n@@ -1021,8 +1016,6 @@ 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 \tif (hit) {\n \t\tpmatch[0].rm_so += bol - start;\n \t\tpmatch[0].rm_eo += bol - start;\n-- \n2.33.0.1023.gc687d0d3c8\n\n"},{"id":"436558","messageId":"YUlV+RPMHGGfk25d@coredump.intra.peff.net","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"[PATCH 2/5] grep: stop modifying buffer in show_line()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:48:09Z","receivedAt":"2021-09-21T03:48:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When showing lines via grep (or looking for funcnames), we call\nshow_line() on a multi-line buffer. It finds the end of line and marks\nit with a NUL. However, we don't need to do so, as the resulting line is\nonly used along with its \"eol\" marker:\n\n - we pass both to next_match(), which takes care to look at only the\n   bytes we specified\n\n - we pass the line to output_color() without its matching eol marker.\n   However, we do use the \"match\" struct we got from next_match() to\n   tell it how many bytes to look at (which can never exceed the string\n   we passed it).\n\nSo we can stop setting and restoring this NUL marker. That makes the\ncode simpler, and will allow us to take a const buffer in a future\npatch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n grep.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 5b1f2da4d3..70af01d1c1 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1239,7 +1239,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n \tif (opt->color || opt->only_matching) {\n \t\tregmatch_t match;\n \t\tenum grep_context ctx = GREP_CONTEXT_BODY;\n-\t\tint ch = *eol;\n \t\tint eflags = 0;\n \n \t\tif (opt->color) {\n@@ -1254,7 +1253,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n \t\t\telse if (sign == '=')\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\t\tif (match.rm_so == match.rm_eo)\n \t\t\t\tbreak;\n@@ -1272,7 +1270,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n \t\t\trest -= match.rm_eo;\n \t\t\teflags = REG_NOTBOL;\n \t\t}\n-\t\t*eol = ch;\n \t}\n \tif (!opt->only_matching) {\n \t\toutput_color(opt, bol, rest, line_color);\n-- \n2.33.0.1023.gc687d0d3c8\n\n"},{"id":"436559","messageId":"YUlWHASpQDJehiZH@coredump.intra.peff.net","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"[PATCH 3/5] grep: stop modifying buffer in grep_source_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:48:44Z","receivedAt":"2021-09-21T03:48:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We find the end of each matching line of a buffer, and then temporarily\nwrite a NUL to turn it into a regular C string. But we don't need to do\nso, because the only thing we do in the interim is pass the line and its\nlength (via an \"eol\" pointer) to match_line(). And that function should\nonly look at the bytes we passed it, whether it has a terminating NUL or\nnot.\n\nWe can drop this temporary write in order to simplify the code and make\nit easier to use const buffers in more of grep.c.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n grep.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 70af01d1c1..32c4641443 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1616,7 +1616,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tbol = gs->buf;\n \tleft = gs->size;\n \twhile (left) {\n-\t\tchar *eol, ch;\n+\t\tchar *eol;\n \t\tint hit;\n \t\tssize_t cno;\n \t\tssize_t col = -1, icol = -1;\n@@ -1637,14 +1637,11 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t    && look_ahead(opt, &left, &lno, &bol))\n \t\t\tbreak;\n \t\teol = end_of_line(bol, &left);\n-\t\tch = *eol;\n-\t\t*eol = 0;\n \n \t\tif ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))\n \t\t\tctx = GREP_CONTEXT_BODY;\n \n \t\thit = match_line(opt, bol, eol, &col, &icol, ctx, collect_hits);\n-\t\t*eol = ch;\n \n \t\tif (collect_hits)\n \t\t\tgoto next_line;\n-- \n2.33.0.1023.gc687d0d3c8\n\n"},{"id":"436560","messageId":"YUlWXRYv1KtZrxZ6@coredump.intra.peff.net","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"[PATCH 4/5] grep: mark \"haystack\" buffers as const","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:49:49Z","receivedAt":"2021-09-21T03:49:53Z","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, and make our expectations more clear (we\nwill not take ownership of the memory, nor will we ever write to it).\n\nThese spots don't all necessarily have to be converted at the same time,\nbut some of them are dependent on others, because we pass\npointers-to-pointers in a few cases. And none of this should change any\nbehavior, since we're just adding \"const\" qualifiers (and likewise, the\ncompiler will let us know if we missed any spots). So it's relatively\nlow-risk to just do this all at once.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n grep.c | 61 +++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 35 insertions(+), 26 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 32c4641443..9603ac119d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -867,7 +867,7 @@ void free_grep_patterns(struct grep_opt *opt)\n \tfree_pattern_expr(opt->pattern_expression);\n }\n \n-static char *end_of_line(char *cp, unsigned long *left)\n+static const char *end_of_line(const char *cp, unsigned long *left)\n {\n \tunsigned long l = *left;\n \twhile (l && *cp != '\\n') {\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@@ -922,9 +923,9 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n \treturn hit;\n }\n \n-static void strip_timestamp(char *bol, char **eol_p)\n+static void strip_timestamp(const char *bol, const char **eol_p)\n {\n-\tchar *eol = *eol_p;\n+\tconst char *eol = *eol_p;\n \n \twhile (bol < --eol) {\n \t\tif (*eol != '>')\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@@ -1023,8 +1025,9 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \treturn hit;\n }\n \n-static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x, char *bol,\n-\t\t\t   char *eol, enum grep_context ctx, ssize_t *col,\n+static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n+\t\t\t   const char *bol, const char *eol,\n+\t\t\t   enum grep_context ctx, ssize_t *col,\n \t\t\t   ssize_t *icol, int collect_hits)\n {\n \tint h = 0;\n@@ -1091,15 +1094,17 @@ static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x, char *bol,\n \treturn h;\n }\n \n-static int match_expr(struct grep_opt *opt, char *bol, char *eol,\n+static int match_expr(struct grep_opt *opt,\n+\t\t      const char *bol, const char *eol,\n \t\t      enum grep_context ctx, ssize_t *col,\n \t\t      ssize_t *icol, int collect_hits)\n {\n \tstruct grep_expr *x = opt->pattern_expression;\n \treturn match_expr_eval(opt, x, bol, eol, ctx, col, icol, collect_hits);\n }\n \n-static int match_line(struct grep_opt *opt, char *bol, char *eol,\n+static int match_line(struct grep_opt *opt,\n+\t\t      const char *bol, const char *eol,\n \t\t      ssize_t *col, ssize_t *icol,\n \t\t      enum grep_context ctx, int collect_hits)\n {\n@@ -1131,7 +1136,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@@ -1152,7 +1158,8 @@ 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+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 {\n \tstruct grep_pat *p;\n@@ -1208,7 +1215,8 @@ static void show_line_header(struct grep_opt *opt, const char *name,\n \t}\n }\n \n-static void show_line(struct grep_opt *opt, char *bol, char *eol,\n+static void show_line(struct grep_opt *opt,\n+\t\t      const char *bol, const char *eol,\n \t\t      const char *name, unsigned lno, ssize_t cno, char sign)\n {\n \tint rest = eol - bol;\n@@ -1297,7 +1305,8 @@ static inline void grep_attr_unlock(void)\n \t\tpthread_mutex_unlock(&grep_attr_mutex);\n }\n \n-static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)\n+static int match_funcname(struct grep_opt *opt, struct grep_source *gs,\n+\t\t\t  const char *bol, const char *eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\n \tif (xecfg && !xecfg->find_func) {\n@@ -1324,10 +1333,10 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo\n }\n \n static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n-\t\t\t       char *bol, unsigned lno)\n+\t\t\t       const char *bol, unsigned lno)\n {\n \twhile (bol > gs->buf) {\n-\t\tchar *eol = --bol;\n+\t\tconst char *eol = --bol;\n \n \t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n@@ -1346,7 +1355,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n static int is_empty_line(const char *bol, const char *eol);\n \n static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,\n-\t\t\t     char *bol, char *end, unsigned lno)\n+\t\t\t     const char *bol, const char *end, unsigned lno)\n {\n \tunsigned cur = lno, from = 1, funcname_lno = 0, orig_from;\n \tint funcname_needed = !!opt->funcname, comment_needed = 0;\n@@ -1366,8 +1375,8 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,\n \n \t/* Rewind. */\n \twhile (bol > gs->buf && cur > from) {\n-\t\tchar *next_bol = bol;\n-\t\tchar *eol = --bol;\n+\t\tconst char *next_bol = bol;\n+\t\tconst char *eol = --bol;\n \n \t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n@@ -1398,7 +1407,7 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,\n \n \t/* Back forward. */\n \twhile (cur < lno) {\n-\t\tchar *eol = bol, sign = (cur == funcname_lno) ? '=' : '-';\n+\t\tconst char *eol = bol, sign = (cur == funcname_lno) ? '=' : '-';\n \n \t\twhile (*eol != '\\n')\n \t\t\teol++;\n@@ -1426,12 +1435,12 @@ static int should_lookahead(struct grep_opt *opt)\n static int look_ahead(struct grep_opt *opt,\n \t\t      unsigned long *left_p,\n \t\t      unsigned *lno_p,\n-\t\t      char **bol_p)\n+\t\t      const char **bol_p)\n {\n \tunsigned lno = *lno_p;\n-\tchar *bol = *bol_p;\n+\tconst char *bol = *bol_p;\n \tstruct grep_pat *p;\n-\tchar *sp, *last_bol;\n+\tconst char *sp, *last_bol;\n \tregoff_t earliest = -1;\n \n \tfor (p = opt->pattern_list; p; p = p->next) {\n@@ -1533,8 +1542,8 @@ static int is_empty_line(const char *bol, const char *eol)\n \n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n {\n-\tchar *bol;\n-\tchar *peek_bol = NULL;\n+\tconst char *bol;\n+\tconst char *peek_bol = NULL;\n \tunsigned long left;\n \tunsigned lno = 1;\n \tunsigned last_hit = 0;\n@@ -1616,7 +1625,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tbol = gs->buf;\n \tleft = gs->size;\n \twhile (left) {\n-\t\tchar *eol;\n+\t\tconst char *eol;\n \t\tint hit;\n \t\tssize_t cno;\n \t\tssize_t col = -1, icol = -1;\n@@ -1700,7 +1709,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t}\n \t\tif (show_function && (!peek_bol || peek_bol < bol)) {\n \t\t\tunsigned long peek_left = left;\n-\t\t\tchar *peek_eol = eol;\n+\t\t\tconst char *peek_eol = eol;\n \n \t\t\t/*\n \t\t\t * Trailing empty lines are not interesting.\n-- \n2.33.0.1023.gc687d0d3c8\n\n"},{"id":"436561","messageId":"YUlWwOY7MD3KUyWh@coredump.intra.peff.net","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"[PATCH 5/5] grep: store grep_source buffer as const","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T03:51:28Z","receivedAt":"2021-09-21T03:51:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our grep_buffer() function takes a non-const buffer, which is confusing:\nwe don't take ownership of nor write to the buffer.\n\nThis mostly comes from the fact that the underlying grep_source struct\nin which we store the buffer uses non-const pointer. The memory pointed\nto by the struct is sometimes owned by us (for FILE or OID sources), and\nsometimes not (for BUF sources).\n\nLet's store it as const, which lets us err on the side of caution (i.e.,\nthe compiler will warn us if any of our code writes to or tries to free\nit).\n\nAs a result, we must annotate the one place where we do free it by\ncasting away the constness. But that's a small price to pay for the\nextra safety and clarity elsewhere (and indeed, it already had a comment\nexplaining why GREP_SOURCE_BUF _didn't_ free it).\n\nAnd then we can mark grep_buffer() as taking a const buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n grep.c | 9 ++++++---\n grep.h | 4 ++--\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 9603ac119d..14fe8a0fd2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1820,9 +1820,10 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs)\n \n \treturn grep_source_1(opt, gs, 0);\n }\n \n-static void grep_source_init_buf(struct grep_source *gs, char *buf,\n+static void grep_source_init_buf(struct grep_source *gs,\n+\t\t\t\t const char *buf,\n \t\t\t\t unsigned long size)\n {\n \tgs->type = GREP_SOURCE_BUF;\n \tgs->name = NULL;\n@@ -1832,9 +1833,9 @@ static void grep_source_init_buf(struct grep_source *gs, char *buf,\n \tgs->driver = NULL;\n \tgs->identifier = NULL;\n }\n \n-int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n+int grep_buffer(struct grep_opt *opt, const char *buf, unsigned long size)\n {\n \tstruct grep_source gs;\n \tint r;\n \n@@ -1884,9 +1885,11 @@ void grep_source_clear_data(struct grep_source *gs)\n {\n \tswitch (gs->type) {\n \tcase GREP_SOURCE_FILE:\n \tcase GREP_SOURCE_OID:\n-\t\tFREE_AND_NULL(gs->buf);\n+\t\t/* these types own the buffer */\n+\t\tfree((char *)gs->buf);\n+\t\tgs->buf = NULL;\n \t\tgs->size = 0;\n \t\tbreak;\n \tcase GREP_SOURCE_BUF:\n \t\t/* leave user-provided buf intact */\ndiff --git a/grep.h b/grep.h\nindex 128007db65..3cb8a83ae8 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -188,9 +188,9 @@ void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const\n void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);\n void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);\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_buffer(struct grep_opt *opt, const char *buf, unsigned long size);\n \n struct grep_source {\n \tchar *name;\n \n@@ -201,9 +201,9 @@ struct grep_source {\n \t} type;\n \tvoid *identifier;\n \tstruct repository *repo; /* if GREP_SOURCE_OID */\n \n-\tchar *buf;\n+\tconst char *buf;\n \tunsigned long size;\n \n \tchar *path; /* for attribute lookups */\n \tstruct userdiff_driver *driver;\n-- \n2.33.0.1023.gc687d0d3c8\n"},{"id":"436563","messageId":"YUleFU4QrKb28bDz@nand.local","threadId":"56553","inReplyTo":"YUlV+RPMHGGfk25d@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] grep: stop modifying buffer in show_line()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T04:22:45Z","receivedAt":"2021-09-21T04:22:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 11:48:09PM -0400, Jeff King wrote:\n> When showing lines via grep (or looking for funcnames), we call\n> show_line() on a multi-line buffer. It finds the end of line and marks\n> it with a NUL. However, we don't need to do so, as the resulting line is\n> only used along with its \"eol\" marker:\n>\n>  - we pass both to next_match(), which takes care to look at only the\n>    bytes we specified\n\nThinking aloud, next_match() calls match_next_pattern() which takes eol\nas non-const and passes it to match_one_pattern(). And that calls\nstrip_timestamp(), which would be non-const, were it not the previous\npatch. So I think this conversion is safe.\n\n>  - we pass the line to output_color() without its matching eol marker.\n>    However, we do use the \"match\" struct we got from next_match() to\n>    tell it how many bytes to look at (which can never exceed the string\n>    we passed it\n\nYep, makes sense. The patch looks good and matches your description\nhere.\n\nThanks,\nTaylor\n"},{"id":"436564","messageId":"YUlf8HyBWC7aDF8n@nand.local","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] const-correctness in grep.c","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T04:30:40Z","receivedAt":"2021-09-21T04:30:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 11:45:42PM -0400, Jeff King wrote:\n> While discussing [1], I noticed that the grep code mostly takes\n> non-const buffers, even though it is conceptually a read-only operation\n> to search in them. The culprit is a handful of spots that temporarily\n> tie off NUL-terminated strings by overwriting a byte of the buffer and\n> then restoring it. But I think we no longer need to do so these days,\n> now that we have a regexec_buf() that can take a ptr/size pair.\n\nThis all looks very reasonable to me. I appreciated the way you broke up\neach spot that unnecessarily modified a buffer into its own patch with\nits own explanation.\n\nI looked through each of the three spots you mentioned with a close eye\nand concurred with your reasoning. (To the extent it was possible, I\ntried to ignore most of your commentary until I had generated my own\nunderstanding while searching through my copy of grep.c).\n\nThanks for an enjoyable set of patches to read.\n\n    Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\n> [1] https://lore.kernel.org/git/YUk3zwuse56v76ze@coredump.intra.peff.net/\n\nThanks,\nTaylor\n"},{"id":"436565","messageId":"YUlinQ7hPokFLgfm@coredump.intra.peff.net","threadId":"56553","inReplyTo":"YUleFU4QrKb28bDz@nand.local","subject":"Re: [PATCH 2/5] grep: stop modifying buffer in show_line()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T04:42:05Z","receivedAt":"2021-09-21T04:42:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 12:22:45AM -0400, Taylor Blau wrote:\n\n> On Mon, Sep 20, 2021 at 11:48:09PM -0400, Jeff King wrote:\n> > When showing lines via grep (or looking for funcnames), we call\n> > show_line() on a multi-line buffer. It finds the end of line and marks\n> > it with a NUL. However, we don't need to do so, as the resulting line is\n> > only used along with its \"eol\" marker:\n> >\n> >  - we pass both to next_match(), which takes care to look at only the\n> >    bytes we specified\n> \n> Thinking aloud, next_match() calls match_next_pattern() which takes eol\n> as non-const and passes it to match_one_pattern(). And that calls\n> strip_timestamp(), which would be non-const, were it not the previous\n> patch. So I think this conversion is safe.\n\nTo be a little nit-picky: this move would be OK even without the change\nto strip_timestamp(). The question is whether any of those sub-calls\nactually looks past the \"eol\" pointer we give it.\n\nI didn't dig all the way down through the complete call-stack in an\nexhaustive way. But after having looked at the related functions when\nchanging their const-ness elsewhere in the series, they'd have to be\nignoring the \"eol\" parameter for it to be a problem. The irony, of\ncourse, is that they did ignore this parameter at one point! Because\nthey'd all eventually end in regexec(), which would happily read past\nour \"eol\".\n\nSo really, all of this is predicated on those sub-functions behaving\nsensibly. I think they do. But if we found that they didn't, I'd much\nrather know that and fix them than live with this \"this NUL may or may\nnot be important\" state forever.\n\n> >  - we pass the line to output_color() without its matching eol marker.\n> >    However, we do use the \"match\" struct we got from next_match() to\n> >    tell it how many bytes to look at (which can never exceed the string\n> >    we passed it\n> \n> Yep, makes sense. The patch looks good and matches your description\n> here.\n\nThanks for looking it over (my nitpick aside). :)\n\n-Peff\n"},{"id":"436566","messageId":"YUljZU5YpclFn56t@nand.local","threadId":"56553","inReplyTo":"YUlinQ7hPokFLgfm@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] grep: stop modifying buffer in show_line()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T04:45:25Z","receivedAt":"2021-09-21T04:45:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 12:42:05AM -0400, Jeff King wrote:\n> On Tue, Sep 21, 2021 at 12:22:45AM -0400, Taylor Blau wrote:\n>\n> > On Mon, Sep 20, 2021 at 11:48:09PM -0400, Jeff King wrote:\n> > > When showing lines via grep (or looking for funcnames), we call\n> > > show_line() on a multi-line buffer. It finds the end of line and marks\n> > > it with a NUL. However, we don't need to do so, as the resulting line is\n> > > only used along with its \"eol\" marker:\n> > >\n> > >  - we pass both to next_match(), which takes care to look at only the\n> > >    bytes we specified\n> >\n> > Thinking aloud, next_match() calls match_next_pattern() which takes eol\n> > as non-const and passes it to match_one_pattern(). And that calls\n> > strip_timestamp(), which would be non-const, were it not the previous\n> > patch. So I think this conversion is safe.\n>\n> To be a little nit-picky: this move would be OK even without the change\n> to strip_timestamp(). The question is whether any of those sub-calls\n> actually looks past the \"eol\" pointer we give it.\n\nRight, I wasn't implying that this move was unsafe without the previous\npatch. Just that without the previous patch, we couldn't make\nshow_line() take a const-pointer to eol.\n\nThanks,\nTaylor\n"},{"id":"436569","messageId":"CAPUEsphSyZB-vtubjYhN_5Gy3Zv0HQ=fH=+G8kMYzJyrOLXPxQ@mail.gmail.com","threadId":"56553","inReplyTo":"YUlVsLkFGRfRqpKG@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-21T05:18:25Z","receivedAt":"2021-09-21T05:18:41Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Sep 20, 2021 at 9:09 PM Jeff King <peff@peff.net> wrote:\n> @@ -971,7 +966,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n>                 switch (p->field) {\n>                 case GREP_HEADER_AUTHOR:\n>                 case GREP_HEADER_COMMITTER:\n> -                       saved_ch = strip_timestamp(bol, &eol);\n> +                       strip_timestamp(bol, &eol);\n\nWhy not something like (plus added error handling, even if it seems\nthe original didn't have them)?\n\n  eol = strrchr(bol, '>');\n\nCarlo\n"},{"id":"436570","messageId":"CAPig+cQnC1LLPtuC0qVX7EQ_ki4pev6scRox3utA45XeLHfGig@mail.gmail.com","threadId":"56553","inReplyTo":"CAPUEsphSyZB-vtubjYhN_5Gy3Zv0HQ=fH=+G8kMYzJyrOLXPxQ@mail.gmail.com","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-21T05:24:41Z","receivedAt":"2021-09-21T05:24:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 21, 2021 at 1:18 AM Carlo Arenas <carenas@gmail.com> wrote:\n> On Mon, Sep 20, 2021 at 9:09 PM Jeff King <peff@peff.net> wrote:\n> > @@ -971,7 +966,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n> >                 switch (p->field) {\n> >                 case GREP_HEADER_AUTHOR:\n> >                 case GREP_HEADER_COMMITTER:\n> > -                       saved_ch = strip_timestamp(bol, &eol);\n> > +                       strip_timestamp(bol, &eol);\n>\n> Why not something like (plus added error handling, even if it seems\n> the original didn't have them)?\n>\n>   eol = strrchr(bol, '>');\n\nstrrchr() would search backward from the NUL, not from `eol`, thus\nwould not be a faithful conversion (and might not be safe, though I\ndidn't dig through all the callers).\n"},{"id":"436573","messageId":"CAPUEsphR5AYUz9eLjexfH0jZWiqgpU06x1ZD9rKb7PBU3z6pJQ@mail.gmail.com","threadId":"56553","inReplyTo":"CAPig+cQnC1LLPtuC0qVX7EQ_ki4pev6scRox3utA45XeLHfGig@mail.gmail.com","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-21T05:40:48Z","receivedAt":"2021-09-21T05:41:02Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Sep 20, 2021 at 10:24 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Tue, Sep 21, 2021 at 1:18 AM Carlo Arenas <carenas@gmail.com> wrote:\n> > On Mon, Sep 20, 2021 at 9:09 PM Jeff King <peff@peff.net> wrote:\n> > > @@ -971,7 +966,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n> > >                 switch (p->field) {\n> > >                 case GREP_HEADER_AUTHOR:\n> > >                 case GREP_HEADER_COMMITTER:\n> > > -                       saved_ch = strip_timestamp(bol, &eol);\n> > > +                       strip_timestamp(bol, &eol);\n> >\n> > Why not something like (plus added error handling, even if it seems\n> > the original didn't have them)?\n> >\n> >   eol = strrchr(bol, '>');\n>\n> strrchr() would search backward from the NUL, not from `eol`, thus\n> would not be a faithful conversion (and might not be safe, though I\n> didn't dig through all the callers).\n\nof course; I meant memrchr, which I guess will need to have also a\ncompat version[1]\nthis is the only caller for strip_timestamp()\n\n[1] https://www.gnu.org/software/gnulib/manual/html_node/memrchr.html\n"},{"id":"436575","messageId":"YUlw6V7AL8l6mbSh@coredump.intra.peff.net","threadId":"56553","inReplyTo":"CAPig+cQnC1LLPtuC0qVX7EQ_ki4pev6scRox3utA45XeLHfGig@mail.gmail.com","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T05:43:05Z","receivedAt":"2021-09-21T05:43:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 01:24:41AM -0400, Eric Sunshine wrote:\n\n> On Tue, Sep 21, 2021 at 1:18 AM Carlo Arenas <carenas@gmail.com> wrote:\n> > On Mon, Sep 20, 2021 at 9:09 PM Jeff King <peff@peff.net> wrote:\n> > > @@ -971,7 +966,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n> > >                 switch (p->field) {\n> > >                 case GREP_HEADER_AUTHOR:\n> > >                 case GREP_HEADER_COMMITTER:\n> > > -                       saved_ch = strip_timestamp(bol, &eol);\n> > > +                       strip_timestamp(bol, &eol);\n> >\n> > Why not something like (plus added error handling, even if it seems\n> > the original didn't have them)?\n> >\n> >   eol = strrchr(bol, '>');\n> \n> strrchr() would search backward from the NUL, not from `eol`, thus\n> would not be a faithful conversion (and might not be safe, though I\n> didn't dig through all the callers).\n\nRight. The point is that we should be respecting \"eol\" here in the first\nplace. Plus the final result is the character _after_ the '>'. Plus when\nit returns NULL, you'd want to leave \"eol\" untouched (stripping\nnothing).\n\nSo yeah, I'm sure it could be rewritten around memrchr() or something,\nbut I doubt it would be much shorter, and the chance of introducing an\noff-by-one seems non-trivial. :)\n\n-Peff\n"},{"id":"436576","messageId":"YUl+w8Tn3jqfLqt2@carlos-mbp.lan","threadId":"56553","inReplyTo":"YUlw6V7AL8l6mbSh@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-21T06:42:11Z","receivedAt":"2021-09-21T06:42:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Sep 21, 2021 at 01:43:05AM -0400, Jeff King wrote:\n> \n> So yeah, I'm sure it could be rewritten around memrchr() or something,\n> but I doubt it would be much shorter, and the chance of introducing an\n> off-by-one seems non-trivial. :)\n\nConsidering I am writing it, it is most likely warranted ;)\n\nbut it doesn't look that bad IMHO\n\nCarlo\n\nPS. I tested it in macOS with the compatibility layer that will be needed\n------ 8> -------\nSubject: [PATCH] grep: retire strip_timestamp()\n\nAfter recent changes, the name is no longer valid, as the function\ndoesn't strip anything.\n\nHaving the code in the main function also helps with readability\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n grep.c | 19 +++++--------------\n 1 file changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 5b1f2da4d3..56fd86a7d8 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -922,18 +922,6 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n \treturn hit;\n }\n \n-static void strip_timestamp(char *bol, char **eol_p)\n-{\n-\tchar *eol = *eol_p;\n-\n-\twhile (bol < --eol) {\n-\t\tif (*eol != '>')\n-\t\t\tcontinue;\n-\t\t*eol_p = ++eol;\n-\t\tbreak;\n-\t}\n-}\n-\n static struct {\n \tconst char *field;\n \tsize_t len;\n@@ -965,9 +953,12 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n \t\tbol += len;\n \t\tswitch (p->field) {\n \t\tcase GREP_HEADER_AUTHOR:\n-\t\tcase GREP_HEADER_COMMITTER:\n-\t\t\tstrip_timestamp(bol, &eol);\n+\t\tcase GREP_HEADER_COMMITTER: {\n+\t\t\tchar *em = memrchr(bol, '>', eol - bol);\n+\t\t\tif (em)\n+\t\t\t\teol = em + 1;\n \t\t\tbreak;\n+\t\t}\n \t\tdefault:\n \t\t\tbreak;\n \t\t}\n-- \n2.33.0.911.gbe391d4e11\n\n"},{"id":"436578","messageId":"7d791c04-d122-1eb9-a84c-939294817395@web.de","threadId":"56553","inReplyTo":"YUl+w8Tn3jqfLqt2@carlos-mbp.lan","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-21T07:37:23Z","receivedAt":"2021-09-21T07:38:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.09.21 um 08:42 schrieb Carlo Marcelo Arenas Belón:\n> On Tue, Sep 21, 2021 at 01:43:05AM -0400, Jeff King wrote:\n>>\n>> So yeah, I'm sure it could be rewritten around memrchr() or something,\n>> but I doubt it would be much shorter, and the chance of introducing an\n>> off-by-one seems non-trivial. :)\n>\n> Considering I am writing it, it is most likely warranted ;)\n>\n> but it doesn't look that bad IMHO\n>\n> Carlo\n>\n> PS. I tested it in macOS with the compatibility layer that will be needed\n\nRight; memrchr is a GNU extension.  We'd need a compat/ implementation to\nbe able to use it.\n\n> ------ 8> -------\n> Subject: [PATCH] grep: retire strip_timestamp()\n>\n> After recent changes, the name is no longer valid, as the function\n> doesn't strip anything.\n\nIt still does; the input string slice between bol and eol contains a\ntrailing timestamp and the output slice doesn't.\n\n>\n> Having the code in the main function also helps with readability\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  grep.c | 19 +++++--------------\n>  1 file changed, 5 insertions(+), 14 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 5b1f2da4d3..56fd86a7d8 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -922,18 +922,6 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n>  \treturn hit;\n>  }\n>\n> -static void strip_timestamp(char *bol, char **eol_p)\n> -{\n> -\tchar *eol = *eol_p;\n> -\n> -\twhile (bol < --eol) {\n> -\t\tif (*eol != '>')\n> -\t\t\tcontinue;\n> -\t\t*eol_p = ++eol;\n> -\t\tbreak;\n> -\t}\n> -}\n> -\n>  static struct {\n>  \tconst char *field;\n>  \tsize_t len;\n> @@ -965,9 +953,12 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n>  \t\tbol += len;\n>  \t\tswitch (p->field) {\n>  \t\tcase GREP_HEADER_AUTHOR:\n> -\t\tcase GREP_HEADER_COMMITTER:\n> -\t\t\tstrip_timestamp(bol, &eol);\n> +\t\tcase GREP_HEADER_COMMITTER: {\n> +\t\t\tchar *em = memrchr(bol, '>', eol - bol);\n> +\t\t\tif (em)\n> +\t\t\t\teol = em + 1;\n\nThe old code documents the intent via the function name.  The new one\ngoes into the nitty-gritty without further explanation, which I find\nharder to read.\n\n>  \t\t\tbreak;\n> +\t\t}\n>  \t\tdefault:\n>  \t\t\tbreak;\n>  \t\t}\n>\n\n"},{"id":"436597","messageId":"87h7ee9lsf.fsf@evledraar.gmail.com","threadId":"56553","inReplyTo":"YUlWXRYv1KtZrxZ6@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] grep: mark \"haystack\" buffers as const","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T12:04:12Z","receivedAt":"2021-09-21T12:05:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Jeff King wrote:\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\nnit; not worth a re-roll, over-line-wrapping here? You usually stay at\ncolumn 79. In this case both line and eol would fit on the patmatch()\nlin, which is being changed anyway...\n"},{"id":"436598","messageId":"87czp29l2c.fsf@evledraar.gmail.com","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] const-correctness in grep.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T12:07:08Z","receivedAt":"2021-09-21T12:20:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Jeff King wrote:\n\n> While discussing [1], I noticed that the grep code mostly takes\n> non-const buffers, even though it is conceptually a read-only operation\n> to search in them. The culprit is a handful of spots that temporarily\n> tie off NUL-terminated strings by overwriting a byte of the buffer and\n> then restoring it. But I think we no longer need to do so these days,\n> now that we have a regexec_buf() that can take a ptr/size pair.\n>\n> The first three patches are a bit repetitive, but I broke them up\n> individually because they're the high-risk part. I.e., if my assumptions\n> about needing the NUL are wrong, it could introduce a bug. But based on\n> my reading of the code, plus running the test suite with ASan/UBSan, I\n> feel reasonably confident.\n>\n> The last two are the bigger cleanups, but should obviously avoid any\n> behavior changes.\n>\n>   [1/5]: grep: stop modifying buffer in strip_timestamp\n>   [2/5]: grep: stop modifying buffer in show_line()\n>   [3/5]: grep: stop modifying buffer in grep_source_1()\n>   [4/5]: grep: mark \"haystack\" buffers as const\n>   [5/5]: grep: store grep_source buffer as const\n>\n>  grep.c | 87 +++++++++++++++++++++++++++++-----------------------------\n>  grep.h |  4 +--\n>  2 files changed, 45 insertions(+), 46 deletions(-)\n>\n> -Peff\n>\n> [1] https://lore.kernel.org/git/YUk3zwuse56v76ze@coredump.intra.peff.net/\n\nThis whole thing looks good to me. I only found a small whitespace nit\nin one of the patches. Did you consider following-up by having this code\ntake const char*/const size_t pairs. E.g. starting with something like\nthe below.\n\nWhen this API is called it's called like that, and the regex functions\nat the bottom expect that, but we have all the bol/eol twiddling in the\nmiddle, which is often confusing because some functions pass the\npointers along as-is, and some modify them. So not for now, but I think\nrolling with what I started here below would make sense for this file\neventually:\n\ndiff --git a/grep.c b/grep.c\nindex 14fe8a0fd23..f55ec5c0e09 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -436,7 +436,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t}\n }\n \n-static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n+static int pcre2match(struct grep_pat *p, const char *line, const size_t len,\n \t\tregmatch_t *match, int eflags)\n {\n \tint ret, flags = 0;\n@@ -448,11 +448,11 @@ static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n \n \tif (p->pcre2_jit_on)\n \t\tret = pcre2_jit_match(p->pcre2_pattern, (unsigned char *)line,\n-\t\t\t\t      eol - line, 0, flags, p->pcre2_match_data,\n+\t\t\t\t      len, 0, flags, p->pcre2_match_data,\n \t\t\t\t      NULL);\n \telse\n \t\tret = pcre2_match(p->pcre2_pattern, (unsigned char *)line,\n-\t\t\t\t  eol - line, 0, flags, p->pcre2_match_data,\n+\t\t\t\t  len, 0, flags, p->pcre2_match_data,\n \t\t\t\t  NULL);\n \n \tif (ret < 0 && ret != PCRE2_ERROR_NOMATCH) {\n@@ -909,15 +909,15 @@ static void show_name(struct grep_opt *opt, const char *name)\n }\n \n static int patmatch(struct grep_pat *p,\n-\t\t    const char *line, const char *eol,\n+\t\t    const char *line, const size_t len,\n \t\t    regmatch_t *match, int eflags)\n {\n \tint hit;\n \n \tif (p->pcre2_pattern)\n-\t\thit = !pcre2match(p, line, eol, match, eflags);\n+\t\thit = !pcre2match(p, line, len, match, eflags);\n \telse\n-\t\thit = !regexec_buf(&p->regexp, line, eol - line, 1, match,\n+\t\thit = !regexec_buf(&p->regexp, line, len, 1, match,\n \t\t\t\t   eflags);\n \n \treturn hit;\n@@ -976,7 +976,7 @@ static int match_one_pattern(struct grep_pat *p,\n \t}\n \n  again:\n-\thit = patmatch(p, bol, eol, pmatch, eflags);\n+\thit = patmatch(p, bol, eol - bol, pmatch, eflags);\n \n \tif (hit && p->word_regexp) {\n \t\tif ((pmatch[0].rm_so < 0) ||\n@@ -1447,7 +1447,7 @@ static int look_ahead(struct grep_opt *opt,\n \t\tint hit;\n \t\tregmatch_t m;\n \n-\t\thit = patmatch(p, bol, bol + *left_p, &m, 0);\n+\t\thit = patmatch(p, bol, *left_p, &m, 0);\n \t\tif (!hit || m.rm_so < 0 || m.rm_eo < 0)\n \t\t\tcontinue;\n \t\tif (earliest < 0 || m.rm_so < earliest)\n\n\n"},{"id":"436599","messageId":"patch-1.1-c317e6e125e-20210921T124416Z-avarab@gmail.com","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"[PATCH 6/5] grep.c: mark eol/bol and derived as \"const char * const\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T12:45:16Z","receivedAt":"2021-09-21T12:45:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The control flow of how \"bol\" and \"eol\" are passed through the various\nfunctions in grep.c can be hard to follow, because in some functions\nwe want e.g. an \"eol pointer to const char\", but in others we can do\nwith a \"const pointer to const char\", likewise for the \"const char **\"\ncase.\n\nI think that it would be good to eventually change this code to mostly\ntake a \"const char *const\"/\"const size_t\" pair, as that's what both\nregexec_buf() and the equivalent PCRE function consume[1], now we\nconvert to length with \"eol - bol\" in several places.\n\nFor any such future conversion, and for reading the code, it'll be\nmuch easier if we're at a starting point of knowing what pointers we\nmodify where, so let's have the compiler help us with that.\n\nThis change was made by converting these to the strictest possible\n\"const\" forms and seeing where we had errors, note that the lone\n\"const char ** const\" can only be that strict, it can't be a \"const\nchar * const * const\".\n\nThis change doesn't matter for the compiler's optimization, both gcc\nand clang generate practically (only address differences) the same\ncode under both -O0 and -O3.\n\n1. https://lore.kernel.org/git/87czp29l2c.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nA suggested patch either on top of Jeff King's\n<YUlVZk1xXulAqdef@coredump.intra.peff.net> or even for squashing into\nhis series.\n\nI think that generally git's codebase could use going beyond just\n\"const char *\" when a \"const char * const\" would suffice, for some\nreason we seem to mostly use it for the static usage variables.\n\nIn this particular case I think it makes the flow of grep.c much\neasier to reason about. You can immediately see which functions are\ntwiddling the bol/eof pointers and which aren't.\n\n grep.c | 41 +++++++++++++++++++++--------------------\n 1 file changed, 21 insertions(+), 20 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 14fe8a0fd23..4e266769931 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -206,7 +206,7 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o\n \t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n }\n \n-static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,\n+static struct grep_pat *create_grep_pat(const char *const pat, size_t patlen,\n \t\t\t\t\tconst char *origin, int no,\n \t\t\t\t\tenum grep_pat_token t,\n \t\t\t\t\tenum grep_header_field field)\n@@ -436,8 +436,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t}\n }\n \n-static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n-\t\tregmatch_t *match, int eflags)\n+static int pcre2match(struct grep_pat *p, const char * const line,\n+\t\t      const char * const eol, regmatch_t *match, int eflags)\n {\n \tint ret, flags = 0;\n \tPCRE2_SIZE *ovector;\n@@ -489,8 +489,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tdie(\"cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE\");\n }\n \n-static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n-\t\tregmatch_t *match, int eflags)\n+static int pcre2match(struct grep_pat *p,\n+\t\t      const char * const line, const char * const eol,\n+\t\t      regmatch_t *match, int eflags)\n {\n \treturn 1;\n }\n@@ -909,7 +910,7 @@ static void show_name(struct grep_opt *opt, const char *name)\n }\n \n static int patmatch(struct grep_pat *p,\n-\t\t    const char *line, const char *eol,\n+\t\t    const char * const line, const char * const eol,\n \t\t    regmatch_t *match, int eflags)\n {\n \tint hit;\n@@ -923,7 +924,7 @@ static int patmatch(struct grep_pat *p,\n \treturn hit;\n }\n \n-static void strip_timestamp(const char *bol, const char **eol_p)\n+static void strip_timestamp(const char * const bol, const char ** const eol_p)\n {\n \tconst char *eol = *eol_p;\n \n@@ -1026,7 +1027,7 @@ static int match_one_pattern(struct grep_pat *p,\n }\n \n static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n-\t\t\t   const char *bol, const char *eol,\n+\t\t\t   const char *bol, const char * const eol,\n \t\t\t   enum grep_context ctx, ssize_t *col,\n \t\t\t   ssize_t *icol, int collect_hits)\n {\n@@ -1095,7 +1096,7 @@ static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n }\n \n static int match_expr(struct grep_opt *opt,\n-\t\t      const char *bol, const char *eol,\n+\t\t      const char *bol, const char * const eol,\n \t\t      enum grep_context ctx, ssize_t *col,\n \t\t      ssize_t *icol, int collect_hits)\n {\n@@ -1104,7 +1105,7 @@ static int match_expr(struct grep_opt *opt,\n }\n \n static int match_line(struct grep_opt *opt,\n-\t\t      const char *bol, const char *eol,\n+\t\t      const char *bol, const char * const eol,\n \t\t      ssize_t *col, ssize_t *icol,\n \t\t      enum grep_context ctx, int collect_hits)\n {\n@@ -1137,7 +1138,7 @@ static int match_line(struct grep_opt *opt,\n }\n \n static int match_next_pattern(struct grep_pat *p,\n-\t\t\t      const char *bol, const char *eol,\n+\t\t\t      const char * const bol, const char * const eol,\n \t\t\t      enum grep_context ctx,\n \t\t\t      regmatch_t *pmatch, int eflags)\n {\n@@ -1159,7 +1160,7 @@ static int match_next_pattern(struct grep_pat *p,\n }\n \n static int next_match(struct grep_opt *opt,\n-\t\t      const char *bol, const char *eol,\n+\t\t      const char * const bol, const char * const eol,\n \t\t      enum grep_context ctx, regmatch_t *pmatch, int eflags)\n {\n \tstruct grep_pat *p;\n@@ -1216,7 +1217,7 @@ static void show_line_header(struct grep_opt *opt, const char *name,\n }\n \n static void show_line(struct grep_opt *opt,\n-\t\t      const char *bol, const char *eol,\n+\t\t      const char *bol, const char * const eol,\n \t\t      const char *name, unsigned lno, ssize_t cno, char sign)\n {\n \tint rest = eol - bol;\n@@ -1306,7 +1307,7 @@ static inline void grep_attr_unlock(void)\n }\n \n static int match_funcname(struct grep_opt *opt, struct grep_source *gs,\n-\t\t\t  const char *bol, const char *eol)\n+\t\t\t  const char * const bol, const char * const eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\n \tif (xecfg && !xecfg->find_func) {\n@@ -1336,7 +1337,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n \t\t\t       const char *bol, unsigned lno)\n {\n \twhile (bol > gs->buf) {\n-\t\tconst char *eol = --bol;\n+\t\tconst char * const eol = --bol;\n \n \t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n@@ -1352,7 +1353,7 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n \t}\n }\n \n-static int is_empty_line(const char *bol, const char *eol);\n+static int is_empty_line(const char * const bol, const char * const eol);\n \n static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,\n \t\t\t     const char *bol, const char *end, unsigned lno)\n@@ -1375,8 +1376,8 @@ static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,\n \n \t/* Rewind. */\n \twhile (bol > gs->buf && cur > from) {\n-\t\tconst char *next_bol = bol;\n-\t\tconst char *eol = --bol;\n+\t\tconst char * const next_bol = bol;\n+\t\tconst char * const eol = --bol;\n \n \t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n@@ -1438,7 +1439,7 @@ static int look_ahead(struct grep_opt *opt,\n \t\t      const char **bol_p)\n {\n \tunsigned lno = *lno_p;\n-\tconst char *bol = *bol_p;\n+\tconst char * const bol = *bol_p;\n \tstruct grep_pat *p;\n \tconst char *sp, *last_bol;\n \tregoff_t earliest = -1;\n@@ -1533,7 +1534,7 @@ static int fill_textconv_grep(struct repository *r,\n \treturn 0;\n }\n \n-static int is_empty_line(const char *bol, const char *eol)\n+static int is_empty_line(const char *bol, const char * const eol)\n {\n \twhile (bol < eol && isspace(*bol))\n \t\tbol++;\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436618","messageId":"YUnrI10DXLElenKZ@coredump.intra.peff.net","threadId":"56553","inReplyTo":"7d791c04-d122-1eb9-a84c-939294817395@web.de","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T14:24:35Z","receivedAt":"2021-09-21T14:24:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 09:37:23AM +0200, René Scharfe wrote:\n\n> > @@ -965,9 +953,12 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n> >  \t\tbol += len;\n> >  \t\tswitch (p->field) {\n> >  \t\tcase GREP_HEADER_AUTHOR:\n> > -\t\tcase GREP_HEADER_COMMITTER:\n> > -\t\t\tstrip_timestamp(bol, &eol);\n> > +\t\tcase GREP_HEADER_COMMITTER: {\n> > +\t\t\tchar *em = memrchr(bol, '>', eol - bol);\n> > +\t\t\tif (em)\n> > +\t\t\t\teol = em + 1;\n> \n> The old code documents the intent via the function name.  The new one\n> goes into the nitty-gritty without further explanation, which I find\n> harder to read.\n\nAgreed. I do think the conversion is functionally correct, but it\ndoesn't strike me as worth the change.\n\n-Peff\n"},{"id":"436619","messageId":"YUnry6IgLyf5LI6g@coredump.intra.peff.net","threadId":"56553","inReplyTo":"87h7ee9lsf.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/5] grep: mark \"haystack\" buffers as const","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T14:27:23Z","receivedAt":"2021-09-21T14:27:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 02:04:12PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Mon, Sep 20 2021, Jeff King wrote:\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> nit; not worth a re-roll, over-line-wrapping here? You usually stay at\n> column 79. In this case both line and eol would fit on the patmatch()\n> lin, which is being changed anyway...\n\nTrue. I had to wrap all the others (and IMHO it is worth keeping \"line\"\nand \"eol\" together), but I didn't calculate that this one can actually\nfit the whole thing.\n\nI dunno that it matters. The wrapping is consistent with the others,\neven if it is not maximally filling the line.\n\n-Peff\n"},{"id":"436620","messageId":"YUnxAawpNjyb0z3o@coredump.intra.peff.net","threadId":"56553","inReplyTo":"87czp29l2c.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] const-correctness in grep.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T14:49:37Z","receivedAt":"2021-09-21T14:49:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 02:07:08PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This whole thing looks good to me. I only found a small whitespace nit\n> in one of the patches. Did you consider following-up by having this code\n> take const char*/const size_t pairs. E.g. starting with something like\n> the below.\n\nI do generally find ptr/len pairs to be easier to read, but they also\nmake it really easy to introduce subtle bugs. E.g., if you consume part\nof a buffer, you have to tweak both the ptr and the len. So the current:\n\n\twhile (word_char(bol[-1]) && bol < eol)\n\t\tbol++;\n\nhas to become:\n\n\twhile (word_char(bol[-1] && len > 0) {\n\t\tbol++;\n\t\tlen--;\n\t}\n\nSo I'd be hesitant to churn battle-tested code in such a way for what I\nconsider to be a pretty minor benefit.\n\nI did notice the ugly use of \"unsigned long\" here in a few places\n(rather than size_t). I do think it is worth fixing, but it seemed a\nlittle too far to try to cram into this series (it's obviously touching\nthe same lines, but it's quite orthogonal semantically).\n\nThe other hesitation I had is that the source of this \"unsigned long\"\npattern is almost certainly the object code (which is much more\nimportant to convert, as it blocks people from having >4GB objects on\nWindows). So we might want to just wait for a larger conversion there.\nOTOH, I don't think there is any downside to a partial conversion here\nin the meantime (because size_t will always be at least as long as\n\"unsigned long\" in practice).\n\n> -static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n> +static int pcre2match(struct grep_pat *p, const char *line, const size_t len,\n>  \t\tregmatch_t *match, int eflags)\n>  {\n>  \tint ret, flags = 0;\n> @@ -448,11 +448,11 @@ static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n>  \n>  \tif (p->pcre2_jit_on)\n>  \t\tret = pcre2_jit_match(p->pcre2_pattern, (unsigned char *)line,\n> -\t\t\t\t      eol - line, 0, flags, p->pcre2_match_data,\n> +\t\t\t\t      len, 0, flags, p->pcre2_match_data,\n>  \t\t\t\t      NULL);\n>  \telse\n>  \t\tret = pcre2_match(p->pcre2_pattern, (unsigned char *)line,\n> -\t\t\t\t  eol - line, 0, flags, p->pcre2_match_data,\n> +\t\t\t\t  len, 0, flags, p->pcre2_match_data,\n>  \t\t\t\t  NULL);\n\nNot related to your point, but these casts are funny now. They are meant\nto cast to \"unsigned char\" pointers to match pcre's signature, but now\nthey are casting away const-ness, too. That might be worth fixing as\npart of this series.\n\nThough should they really be casting to PCRE2_SPTR? The types are opaque\nin their API because of the weird multi-width thing, though I find it\nhard to imagine us ever using the wider versions of the library.\n\n-Peff\n"},{"id":"436621","messageId":"YUnx7gt0KQNRlhuZ@coredump.intra.peff.net","threadId":"56553","inReplyTo":"patch-1.1-c317e6e125e-20210921T124416Z-avarab@gmail.com","subject":"Re: [PATCH 6/5] grep.c: mark eol/bol and derived as \"const char * const\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T14:53:34Z","receivedAt":"2021-09-21T14:53:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 02:45:16PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I think that generally git's codebase could use going beyond just\n> \"const char *\" when a \"const char * const\" would suffice, for some\n> reason we seem to mostly use it for the static usage variables.\n\nI didn't dig up the references in the list archive, but I feel like\nwe've had this discussion long ago. One of the reasons not to do so is\nthat it pollutes the function's interface with internal details. The\ncaller does not care whether the function is going to modify the pointer\nitself, because it is passed by value. You could apply the same logic\nthat we should be passing \"const int\", and so on.\n\n-Peff\n"},{"id":"436629","messageId":"874kadapq1.fsf@evledraar.gmail.com","threadId":"56553","inReplyTo":"YUnx7gt0KQNRlhuZ@coredump.intra.peff.net","subject":"Re: [PATCH 6/5] grep.c: mark eol/bol and derived as \"const char * const\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T15:17:57Z","receivedAt":"2021-09-21T15:56:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 21 2021, Jeff King wrote:\n\n> On Tue, Sep 21, 2021 at 02:45:16PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I think that generally git's codebase could use going beyond just\n>> \"const char *\" when a \"const char * const\" would suffice, for some\n>> reason we seem to mostly use it for the static usage variables.\n>\n> I didn't dig up the references in the list archive, but I feel like\n> we've had this discussion long ago. One of the reasons not to do so is\n> that it pollutes the function's interface with internal details.[...]\n\nAre there cases in my conversion where the caller has to do anything\nspecial that they didn't before? These are also all static functions, so\nit's all internal details exported to nobody.\n\n> The caller does not care whether the function is going to modify the\n> pointer itself, because it is passed by value.\n\nSure, it's for increased clarity of reading te function in question, not\nits although in one case we pass a ** so you can see what exactly we\nmodify both from the callers and function perspective.\n\n> You could apply the same logic that we should be passing \"const int\",\n> and so on.\n\nSure, in general I think churn like that isn't worth it, and \"const char\n*\" is usually good enough, even if it could be \"const char * const\" or\nwhatever.\n\nI think it makes senes in this specific case where you need to read one\nfunction after another with \"bol\" and \"eol\" variables, with some\ntreating their copy as immutable, others not etc. Particularly if we'd\nconvert it to some other style (e.g. str/len) we can see if an entire\nchain of functions can all be safely changed over.\n\n"},{"id":"436659","messageId":"YUowCkmWqKgIldNz@coredump.intra.peff.net","threadId":"56553","inReplyTo":"874kadapq1.fsf@evledraar.gmail.com","subject":"Re: [PATCH 6/5] grep.c: mark eol/bol and derived as \"const char * const\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-21T19:18:34Z","receivedAt":"2021-09-21T19:18:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 05:17:57PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Tue, Sep 21 2021, Jeff King wrote:\n> \n> > On Tue, Sep 21, 2021 at 02:45:16PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> I think that generally git's codebase could use going beyond just\n> >> \"const char *\" when a \"const char * const\" would suffice, for some\n> >> reason we seem to mostly use it for the static usage variables.\n> >\n> > I didn't dig up the references in the list archive, but I feel like\n> > we've had this discussion long ago. One of the reasons not to do so is\n> > that it pollutes the function's interface with internal details.[...]\n> \n> Are there cases in my conversion where the caller has to do anything\n> special that they didn't before? These are also all static functions, so\n> it's all internal details exported to nobody.\n\nNo, they don't have to do anything differently. I just meant that it\nclutters the interface when a human is reading it.\n\n-Peff\n"},{"id":"436667","messageId":"87v92t8wlp.fsf@evledraar.gmail.com","threadId":"56553","inReplyTo":"YUnrI10DXLElenKZ@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T21:02:31Z","receivedAt":"2021-09-21T21:09:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 21 2021, Jeff King wrote:\n\n> On Tue, Sep 21, 2021 at 09:37:23AM +0200, René Scharfe wrote:\n>\n>> > @@ -965,9 +953,12 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n>> >  \t\tbol += len;\n>> >  \t\tswitch (p->field) {\n>> >  \t\tcase GREP_HEADER_AUTHOR:\n>> > -\t\tcase GREP_HEADER_COMMITTER:\n>> > -\t\t\tstrip_timestamp(bol, &eol);\n>> > +\t\tcase GREP_HEADER_COMMITTER: {\n>> > +\t\t\tchar *em = memrchr(bol, '>', eol - bol);\n>> > +\t\t\tif (em)\n>> > +\t\t\t\teol = em + 1;\n>> \n>> The old code documents the intent via the function name.  The new one\n>> goes into the nitty-gritty without further explanation, which I find\n>> harder to read.\n>\n> Agreed. I do think the conversion is functionally correct, but it\n> doesn't strike me as worth the change.\n\nAs far as some general improvement in thish area it seems to me that\nthis whole subthread is losing the forest for the trees.\n\nWe're operating a buffer that has \"\\n\"'s in it, and then we loop and\nsplit it up one line at a time, often just to match author OR\ncommitter.\n\nWe then have things like commit_rewrite_person() in revision.c that's\npreparing a \"fake\" commit just for grep.c's use, just for it to strip\nthose headers down again.\n\nA real improvement in this area IMO would be trying to bridge the gap\nbetween parse_commit_buffer() and grep.c's match_one_pattern().\n\nI.e. we've even parsed this once before in commit.c's\nparse_commit_date() by the time it gets to grep.c, now we're just doing\nit again.\n\nIt probably makes sense to split up that commit.c code into something\nthat can give you structured output, i.e. headers with types and\nstart/end points for interesting data, then in grep.c we won't need a\nstrip_anything(), or strrchr() or memrchr() or whatever.\n\nIt would also be a lot faster for grepping if we could offload more of\nthis work to the regex engine, particularly if we've got a more capable\nengine like PCREv2.\n\nIn many cases we're splitting lines ourselves, when we could have the\nengine work in a multi-line mode, or translate the user's --author match\ninto something that can match the raw commit header. So have an implicit\n/^author /m anchor if we're matching author headers, instead of having\ngrep.c string-twiddle that around one line at a time.\n\nJust my 0.02.\n"},{"id":"436752","messageId":"xmqqlf3o780a.fsf@gitster.g","threadId":"56553","inReplyTo":"YUlVZk1xXulAqdef@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] const-correctness in grep.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T18:57:57Z","receivedAt":"2021-09-22T18:58:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> While discussing [1], I noticed that the grep code mostly takes\n> non-const buffers, even though it is conceptually a read-only operation\n> to search in them. The culprit is a handful of spots that temporarily\n> tie off NUL-terminated strings by overwriting a byte of the buffer and\n> then restoring it. But I think we no longer need to do so these days,\n> now that we have a regexec_buf() that can take a ptr/size pair.\n\nYes, the haystack has not been read-only exactly because we didn't\nhave <ptr,size> based regexec variant when the grep machinery was\nwritten, and there is no reason why we want to use the \"temporarily\nterminate by swapping the byte with a NUL\" trick.\n\nIt always is a pleasure to read such a concise and to-the-point\nsummary.  With a clear summary like that, a reader almost does not\nhave to see the patch to guess how the rest of the story goes ;-)\n"},{"id":"436753","messageId":"xmqqh7ec77s3.fsf@gitster.g","threadId":"56553","inReplyTo":"YUnx7gt0KQNRlhuZ@coredump.intra.peff.net","subject":"Re: [PATCH 6/5] grep.c: mark eol/bol and derived as \"const char * const\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T19:02:52Z","receivedAt":"2021-09-22T19:02:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Sep 21, 2021 at 02:45:16PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I think that generally git's codebase could use going beyond just\n>> \"const char *\" when a \"const char * const\" would suffice, for some\n>> reason we seem to mostly use it for the static usage variables.\n>\n> I didn't dig up the references in the list archive, but I feel like\n> we've had this discussion long ago. One of the reasons not to do so is\n> that it pollutes the function's interface with internal details. The\n> caller does not care whether the function is going to modify the pointer\n> itself, because it is passed by value. You could apply the same logic\n> that we should be passing \"const int\", and so on.\n\nYes.  \"This pointer is not modified\" is a good thing to have inside\nan implementation, but the callers should not have to care.\n\nThanks.\n"},{"id":"436774","messageId":"YUuQJUSl/jrnQR7n@coredump.intra.peff.net","threadId":"56553","inReplyTo":"87v92t8wlp.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T20:20:53Z","receivedAt":"2021-09-22T20:20:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 21, 2021 at 11:02:31PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Tue, Sep 21 2021, Jeff King wrote:\n> \n> > On Tue, Sep 21, 2021 at 09:37:23AM +0200, René Scharfe wrote:\n> >\n> >> > @@ -965,9 +953,12 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n> >> >  \t\tbol += len;\n> >> >  \t\tswitch (p->field) {\n> >> >  \t\tcase GREP_HEADER_AUTHOR:\n> >> > -\t\tcase GREP_HEADER_COMMITTER:\n> >> > -\t\t\tstrip_timestamp(bol, &eol);\n> >> > +\t\tcase GREP_HEADER_COMMITTER: {\n> >> > +\t\t\tchar *em = memrchr(bol, '>', eol - bol);\n> >> > +\t\t\tif (em)\n> >> > +\t\t\t\teol = em + 1;\n> >> \n> >> The old code documents the intent via the function name.  The new one\n> >> goes into the nitty-gritty without further explanation, which I find\n> >> harder to read.\n> >\n> > Agreed. I do think the conversion is functionally correct, but it\n> > doesn't strike me as worth the change.\n> \n> As far as some general improvement in thish area it seems to me that\n> this whole subthread is losing the forest for the trees.\n\nI'd definitely agree with that. :)\n\n> It probably makes sense to split up that commit.c code into something\n> that can give you structured output, i.e. headers with types and\n> start/end points for interesting data, then in grep.c we won't need a\n> strip_anything(), or strrchr() or memrchr() or whatever.\n\nI don't disagree with any of this, either, but I think it's a separate\n(and much more complicated) topic than what this series is dealing with.\nSo my preference would be to take this as an immediate improvement, and\nlet anything like that get built on top.\n\n> It would also be a lot faster for grepping if we could offload more of\n> this work to the regex engine, particularly if we've got a more capable\n> engine like PCREv2.\n> \n> In many cases we're splitting lines ourselves, when we could have the\n> engine work in a multi-line mode, or translate the user's --author match\n> into something that can match the raw commit header. So have an implicit\n> /^author /m anchor if we're matching author headers, instead of having\n> grep.c string-twiddle that around one line at a time.\n\nDitto here. Multi-line matching may make things a lot more efficient,\nbut I think is out of scope for this series. That seems like an\ninteresting area for future work.\n\n-Peff\n"},{"id":"436807","messageId":"87y27o5cxx.fsf@evledraar.gmail.com","threadId":"56553","inReplyTo":"YUuQJUSl/jrnQR7n@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T00:53:15Z","receivedAt":"2021-09-23T00:54:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 22 2021, Jeff King wrote:\n\n> On Tue, Sep 21, 2021 at 11:02:31PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> \n>> On Tue, Sep 21 2021, Jeff King wrote:\n>> \n>> > On Tue, Sep 21, 2021 at 09:37:23AM +0200, René Scharfe wrote:\n>> >\n>> >> > @@ -965,9 +953,12 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,\n>> >> >  \t\tbol += len;\n>> >> >  \t\tswitch (p->field) {\n>> >> >  \t\tcase GREP_HEADER_AUTHOR:\n>> >> > -\t\tcase GREP_HEADER_COMMITTER:\n>> >> > -\t\t\tstrip_timestamp(bol, &eol);\n>> >> > +\t\tcase GREP_HEADER_COMMITTER: {\n>> >> > +\t\t\tchar *em = memrchr(bol, '>', eol - bol);\n>> >> > +\t\t\tif (em)\n>> >> > +\t\t\t\teol = em + 1;\n>> >> \n>> >> The old code documents the intent via the function name.  The new one\n>> >> goes into the nitty-gritty without further explanation, which I find\n>> >> harder to read.\n>> >\n>> > Agreed. I do think the conversion is functionally correct, but it\n>> > doesn't strike me as worth the change.\n>> \n>> As far as some general improvement in thish area it seems to me that\n>> this whole subthread is losing the forest for the trees.\n>\n> I'd definitely agree with that. :)\n>\n>> It probably makes sense to split up that commit.c code into something\n>> that can give you structured output, i.e. headers with types and\n>> start/end points for interesting data, then in grep.c we won't need a\n>> strip_anything(), or strrchr() or memrchr() or whatever.\n>\n> I don't disagree with any of this, either, but I think it's a separate\n> (and much more complicated) topic than what this series is dealing with.\n> So my preference would be to take this as an immediate improvement, and\n> let anything like that get built on top.\n>\n>> It would also be a lot faster for grepping if we could offload more of\n>> this work to the regex engine, particularly if we've got a more capable\n>> engine like PCREv2.\n>> \n>> In many cases we're splitting lines ourselves, when we could have the\n>> engine work in a multi-line mode, or translate the user's --author match\n>> into something that can match the raw commit header. So have an implicit\n>> /^author /m anchor if we're matching author headers, instead of having\n>> grep.c string-twiddle that around one line at a time.\n>\n> Ditto here. Multi-line matching may make things a lot more efficient,\n> but I think is out of scope for this series. That seems like an\n> interesting area for future work.\n\nIndeed, *way* outside this series. I'm I think the change you suggested\nhere makes sense for this change, just pointing out that for any\nfollow-up changes it's much more worthwhile to consider a few more\nstackframes than just the 1-2 that involve those strings in that\nparticular form.\n"},{"id":"436855","messageId":"874kab5r63.fsf@evledraar.gmail.com","threadId":"56553","inReplyTo":"YUowCkmWqKgIldNz@coredump.intra.peff.net","subject":"Re: [PATCH 6/5] grep.c: mark eol/bol and derived as \"const char * const\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T13:56:21Z","receivedAt":"2021-09-23T13:59:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 21 2021, Jeff King wrote:\n\n> On Tue, Sep 21, 2021 at 05:17:57PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> \n>> On Tue, Sep 21 2021, Jeff King wrote:\n>> \n>> > On Tue, Sep 21, 2021 at 02:45:16PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> >\n>> >> I think that generally git's codebase could use going beyond just\n>> >> \"const char *\" when a \"const char * const\" would suffice, for some\n>> >> reason we seem to mostly use it for the static usage variables.\n>> >\n>> > I didn't dig up the references in the list archive, but I feel like\n>> > we've had this discussion long ago. One of the reasons not to do so is\n>> > that it pollutes the function's interface with internal details.[...]\n>> \n>> Are there cases in my conversion where the caller has to do anything\n>> special that they didn't before? These are also all static functions, so\n>> it's all internal details exported to nobody.\n>\n> No, they don't have to do anything differently. I just meant that it\n> clutters the interface when a human is reading it.\n\nFair enough, to this human reading it (and I'm not new to the grep.c\ncode) I find myself trying and failing to mentally track what gets\nmodified where, but anyway, I think that point is understood and I won't\nargue for it ad nauseum.\n\nJust replied to say to Junio's <xmqqh7ec77s3.fsf@gitster.g> in the\nside-thread:\n\n> Yes.  \"This pointer is not modified\" is a good thing to have inside\n> an implementation, but the callers should not have to care.\n\nThat I just don't understand, i.e. how the variable is defined in this\n6/5 pertains to how it gets used /inside/ the function in this case, the\ncaller code doesn't have to care, but perhaps I'm missing something...\n"},{"id":"436920","messageId":"xmqq4kaay55j.fsf@gitster.g","threadId":"56553","inReplyTo":"874kab5r63.fsf@evledraar.gmail.com","subject":"Re: [PATCH 6/5] grep.c: mark eol/bol and derived as \"const char * const\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-24T04:22:00Z","receivedAt":"2021-09-24T04:22:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> That I just don't understand, i.e. how the variable is defined in this\n> 6/5 pertains to how it gets used /inside/ the function in this case, the\n> caller code doesn't have to care, but perhaps I'm missing something...\n\n\n-static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,\n+static struct grep_pat *create_grep_pat(const char *const pat, size_t patlen,\n \t\t\t\t\tconst char *origin, int no,\n \t\t\t\t\tenum grep_pat_token t,\n \t\t\t\t\tenum grep_header_field field)\n\nBecause the function signature is visible to the caller of this\nfunction, it sees that the callee, as part of its implementation\ndetail, keeps \"pat\" pointer pointing at the same location.  That is\nnot something the caller needs to care, even though the caller may\ndeeply care that the memory locations \"pat\" points at will not be\nchanged via the pointer by calling this function.\n\nOn the other hand\n\n@@ -1438,7 +1439,7 @@ static int look_ahead(struct grep_opt *opt,\n \t\t      const char **bol_p)\n {\n \tunsigned lno = *lno_p;\n-\tconst char *bol = *bol_p;\n+\tconst char * const bol = *bol_p;\n \tstruct grep_pat *p;\n \tconst char *sp, *last_bol;\n \tregoff_t earliest = -1;\n\nThis one is totally invisible to the caller of the function, and is\na good documentation for those who read the implementation of it.\n\nThanks.\n"}]}