Re: [PATCH v6 1/2] grep: refactor next_match() and match_one_pattern() for external use

[Jeff King <peff@peff.net>]

On Mon, Sep 20, 2021 at 08:30:49PM -0400, Hamza Mahfooz wrote:

> diff --git a/grep.c b/grep.c
> index 424a39591b..2901233865 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -956,26 +956,34 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
>  	const char *start = bol;
>  
>  	if ((p->token != GREP_PATTERN) &&
> -	    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)))
> +	    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)) &&
> +	    ((p->token == GREP_PATTERN_BODY) != (ctx == GREP_CONTEXT_BODY)))
>  		return 0;
>  
>  	if (p->token == GREP_PATTERN_HEAD) {
>  		const char *field;
>  		size_t len;
> -		assert(p->field < ARRAY_SIZE(header_field));
> -		field = header_field[p->field].field;
> -		len = header_field[p->field].len;
> -		if (strncmp(bol, field, len))
> -			return 0;
> -		bol += len;
> +		const char *end = eol;
> +
>  		switch (p->field) {
>  		case GREP_HEADER_AUTHOR:
>  		case GREP_HEADER_COMMITTER:
>  			saved_ch = strip_timestamp(bol, &eol);
> +			if (eol == end)
> +				goto again;

I'm not sure if this part is right. If we didn't strip any timestamp,
then we jump to the "again" label, where we actually try to match the
pattern.

But that means we skip the part you deleted above, which got moved down
here:

>  			break;
>  		default:
>  			break;
>  		}
> +
> +		assert(p->field < ARRAY_SIZE(header_field));
> +		field = header_field[p->field].field;
> +		len = header_field[p->field].len;
> +
> +		if (strncmp(bol, field, len))
> +			goto restore;
> +
> +		bol += len;
>  	}

And so we do not check that we have the right field at all. And as a
result, we may return nonsense results. For example, try this in
git.git:

  git log -1 --author=junio 1462b67bc893fc845d28e2748c20357cb16a5ce3

It currently returns no results, because the match is case-sensitive (so
it does not match "Junio" in the author field). But with your patch, it
prints t hat commit (1462b67bc), because it matches a line buried in the
mergetag header ("tag post183-for-junio").

That pattern is how I actually stumbled across it, but an even more
obvious version is:

  git log --author=commit

Currently that returns one result (somebody who has the word "commit" in
their email address). But after your patch, it returns a ton of tag
merges (because they all have "type commit" in their mergetag headers).

-Peff