Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function

[Junio C Hamano <gitster@xxxxxxxxx>]

Johannes Schindelin <johannes.schindelin@xxxxxx> writes:

> @@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
>  		 * caller early.
>  		 */
>  		return;
> -	/* Yuck -- line ought to be "const char *"! */
> -	hold = line[len];
> -	line[len] = '\0';
> -	data->hit = !regexec(data->regexp, line + 1, 1, &regmatch, 0);
> -	line[len] = hold;
> +	data->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,
> +				 &regmatch, 0);

This is an unexpected happy surprise.  It really feels good to see
that "Yuck" line go.

> @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
>  			len--;
>  	}
>  
> -	line_buffer = xstrndup(line, len); /* make NUL terminated */
> -
>  	for (i = 0; i < regs->nr; i++) {
>  		struct ff_reg *reg = regs->array + i;
> -		if (!regexec(&reg->re, line_buffer, 2, pmatch, 0)) {
> +		if (!regexec_buf(&reg->re, line, len, 2, pmatch, 0)) {

So is this hunk.  Removing unnecessary copying is a very good thing.

Please give these three patches a common prefix, e.g.

	regex: -G<pattern> feeds a non NUL-terminated string to	regexec() and fails
        regex: add regexec_buf() that can work on a non NUL-terminated string
	regex: use regexec_buf()

or something like that.

Also I agree with Peff that a test with an embedded NUL would be a
good thing.

This round is so close to perfect.