Re: [RFC/PATCH 4/7] grep: make the behavior for \0 in patterns sane

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 2437 bytes --]

On 2019-06-26 at 00:03:26, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 2d27969057..c89fb569e3 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -271,6 +271,23 @@ providing this option will cause it to die.
>  
>  -f <file>::
>  	Read patterns from <file>, one per line.
> ++
> +Passing the pattern via <file> allows for providing a search pattern
> +containing a \0.

In this case, I think it's easier if we write this as "NUL" or "NUL
byte", since I think you mean a literal byte with value 0 and not the
literal string "\0". I certainly find myself a bit confused, at least,
and I expect others will as well.

> diff --git a/grep.c b/grep.c
> index d3e6111c46..261bd3a342 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -368,18 +368,6 @@ static int is_fixed(const char *s, size_t len)
>  	return 1;
>  }
>  
> -static int has_null(const char *s, size_t len)
> -{
> -	/*
> -	 * regcomp cannot accept patterns with NULs so when using it
> -	 * we consider any pattern containing a NUL fixed.
> -	 */
> -	if (memchr(s, 0, len))
> -		return 1;
> -
> -	return 0;
> -}
> -
>  #ifdef USE_LIBPCRE1
>  static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
>  {
> @@ -668,9 +656,7 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>  	 * simple string match using kws.  p->fixed tells us if we
>  	 * want to use kws.
>  	 */
> -	if (opt->fixed ||
> -	    has_null(p->pattern, p->patternlen) ||
> -	    is_fixed(p->pattern, p->patternlen))
> +	if (opt->fixed || is_fixed(p->pattern, p->patternlen))
>  		p->fixed = !p->ignore_case || !has_non_ascii(p->pattern);
>  
>  	if (p->fixed) {
> @@ -678,7 +664,12 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>  		kwsincr(p->kws, p->pattern, p->patternlen);
>  		kwsprep(p->kws);
>  		return;
> -	} else if (opt->fixed) {
> +	}
> +
> +	if (memchr(p->pattern, 0, p->patternlen) && !opt->pcre2)
> +		die(_("given pattern contains NULL byte (via -f <file>). This is only supported with -P under PCRE v2"));

We probably want to write this as "NUL" as well.

Otherwise, I'm okay with this change. I didn't expect Git to handle
literal NULs in patterns and I'm surprised that it ever worked.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]