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

From: René Scharfe <l.s.r@web.de>
Date: Sep 21, 2021, 07:37 UTC
Message-ID: <7d791c04-d122-1eb9-a84c-939294817395@web.de>
In-Reply-To: <YUl+w8Tn3jqfLqt2@carlos-mbp.lan>

Am 21.09.21 um 08:42 schrieb Carlo Marcelo Arenas Belón:

Show 13 quoted lines

> 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.

Show 5 quoted lines

> ------ 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.

Show 41 quoted lines

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

Show 6 quoted lines

>  			break;
> +		}
>  		default:
>  			break;
>  		}
>