Re: [PATCH 1/5] grep: stop modifying buffer in strip_timestamp
From: René Scharfe <hidden>
Date: 2021-09-21 07:38:05
Am 21.09.21 um 08:42 schrieb Carlo Marcelo Arenas Belón:
On Tue, Sep 21, 2021 at 01:43:05AM -0400, Jeff King wrote:quoted
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. :)Considering I am writing it, it is most likely warranted ;) but it doesn't look that bad IMHO Carlo PS. I tested it in macOS with the compatibility layer that will be needed
Right; memrchr is a GNU extension. We'd need a compat/ implementation to be able to use it.
------ 8> ------- Subject: [PATCH] grep: retire strip_timestamp() After recent changes, the name is no longer valid, as the function doesn't strip anything.
It still does; the input string slice between bol and eol contains a trailing timestamp and the output slice doesn't.
quoted hunk ↗ jump to hunk
Having the code in the main function also helps with readability Signed-off-by: Carlo Marcelo Arenas Belón <redacted> --- grep.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)diff --git a/grep.c b/grep.c index 5b1f2da4d3..56fd86a7d8 100644 --- a/grep.c +++ b/grep.c@@ -922,18 +922,6 @@ static int patmatch(struct grep_pat *p, char *line, char *eol, return hit; } -static void strip_timestamp(char *bol, char **eol_p) -{ - char *eol = *eol_p; - - while (bol < --eol) { - if (*eol != '>') - continue; - *eol_p = ++eol; - break; - } -} - static struct { const char *field; size_t len;@@ -965,9 +953,12 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol, bol += len; switch (p->field) { case GREP_HEADER_AUTHOR: - case GREP_HEADER_COMMITTER: - strip_timestamp(bol, &eol); + case GREP_HEADER_COMMITTER: { + char *em = memrchr(bol, '>', eol - bol); + if (em) + eol = em + 1;
The old code documents the intent via the function name. The new one goes into the nitty-gritty without further explanation, which I find harder to read.
break; + } default: break; }