Re: [PATCH v2] grep: support the --pathspec-from-file option

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Dec 6, 2019, 11:34 UTC
Message-ID: <20191206113401.GA2746@szeder.dev>
In-Reply-To: <20191204203911.237056-1-emilyshaffer@google.com>

On Wed, Dec 04, 2019 at 12:39:11PM -0800, Emily Shaffer wrote:

> Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the
> documentation accordingly.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 50ce8d9461..54ba991c42 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c

Show 6 quoted lines

> @@ -809,6 +813,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	int use_index = 1;
>  	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
>  	int allow_revs;
> +	char *pathspec_from_file;
> +	int pathspec_file_nul;

Uninitialized variables ...

Show 20 quoted lines

>  
>  	struct option options[] = {
>  		OPT_BOOL(0, "cached", &cached,
> @@ -896,8 +902,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL('W', "function-context", &opt.funcbody,
>  			N_("show the surrounding function")),
>  		OPT_GROUP(""),
> -		OPT_CALLBACK('f', NULL, &opt, N_("file"),
> +		OPT_CALLBACK('f', "patterns-from-file", &opt, N_("file"),
>  			N_("read patterns from file"), file_callback),
> +		OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
> +		OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
>  		{ OPTION_CALLBACK, 'e', NULL, &opt, N_("pattern"),
>  			N_("match <pattern>"), PARSE_OPT_NONEG, pattern_callback },
>  		{ OPTION_CALLBACK, 0, "and", &opt, NULL,
> @@ -1062,6 +1070,23 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	pathspec.recursive = 1;
>  	pathspec.recurse_submodules = !!recurse_submodules;
>  
> +	if (pathspec_from_file) {

... one of which is checked here ...

> +		if (pathspec.nr)
> +			die(_("--pathspec-from-file is incompatible with pathspec arguments"));

... causing a lot of tests to fail in our OSX CI builds (but, strangely, in none of the Linux builds), either with the above error message, e.g.:

  expecting success of 4014.150 'format-patch --pretty=mboxrd':
  [...]
  ++git format-patch --pretty=mboxrd --stdout -1 01b96447182ff76f30268fb82d3f9e9e09b14e6c~1..01b96447182ff76f30268fb82d3f9e9e09b14e6c
  ++git grep -h --no-index -A11 '^>From could trip up a loose mbox parser' patch
  fatal: --pathspec-from-file is incompatible with pathspec arguments
  error: last command exited with $?=128

or with:

  expecting success of 7814.2 'grep correctly finds patterns in a submodule': 
  [...]
  ++git grep -e '(3|4)' --recurse-submodules
  fatal: could not open '����?' for reading: No such file or directory
  error: last command exited with $?=128

Show 9 quoted lines

> +		pathspec_from_stdin = !strcmp(pathspec_from_file, "-");
> +
> +		if (patterns_from_stdin && pathspec_from_stdin)
> +			die(_("cannot specify both patterns and pathspec via stdin"));
> +
> +		parse_pathspec_file(&pathspec, 0, PATHSPEC_PREFER_CWD |
> +				    (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
> +				    prefix, pathspec_from_file,
> +				    pathspec_file_nul);

The other uninitialized variable is passed as parameter above and checked below, but I haven't seen any test failures caused by it.

Show 7 quoted lines

> +	} else if (pathspec_file_nul) {
> +		die(_("--pathspec-file-nul requires --pathspec-from-file"));
> +	}
> +
>  	if (list.nr || cached || show_in_pager) {
>  		if (num_threads > 1)
>  			warning(_("invalid option combination, ignoring --threads"));