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:
>>
>> 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.
>
> Having the code in the main function also helps with readability
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx>
> ---
> 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;
> }
>
