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

From: Jeff King <peff@peff.net>
Date: Sep 21, 2021, 01:15 UTC
Message-ID: <YUkyJjaTfDcjakIe@coredump.intra.peff.net>
In-Reply-To: <20210921003050.641393-1-someguy@effective-light.com>

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

Show 30 quoted lines

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

Show 14 quoted lines

>  			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