Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp

On Tue, Sep 21, 2021 at 01:24:41AM -0400, Eric Sunshine wrote:

On Tue, Sep 21, 2021 at 1:18 AM Carlo Arenas [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 9:09 PM Jeff King [off-list ref] wrote:

quoted

@@ -971,7 +966,7 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
                switch (p->field) {
                case GREP_HEADER_AUTHOR:
                case GREP_HEADER_COMMITTER:
-                       saved_ch = strip_timestamp(bol, &eol);
+                       strip_timestamp(bol, &eol);

Why not something like (plus added error handling, even if it seems
the original didn't have them)?

  eol = strrchr(bol, '>');

strrchr() would search backward from the NUL, not from `eol`, thus
would not be a faithful conversion (and might not be safe, though I
didn't dig through all the callers).

Right. The point is that we should be respecting "eol" here in the first
place. Plus the final result is the character _after_ the '>'. Plus when
it returns NULL, you'd want to leave "eol" untouched (stripping
nothing).

So yeah, I'm sure it could be rewritten around memrchr() or something,
but I doubt it would be much shorter, and the chance of introducing an
off-by-one seems non-trivial. :)

-Peff