patchgrep: provide pathspecs/patterns via file or stdin

> Teach 'git grep' how to receive pathspecs via file, and add convenience
> flags for receiving pathspecs or patterns via stdin.
> 
> In some cases, the list of pathspecs to 'git grep' is longer than the
> command line allows, or long enough that a user becomes fatigued reading
> the command.  In other cases, a user may wish to write a script or
> oneliner which generates a list of files to 'git grep' against. In both
> scenarios, a reasonable workaround is to provide the list of pathspecs
> via a file or stdin.
> 
> Before this change, it was already possible to receive patterns via
> stdin by using the argument '-f -'. Now that there are two arguments
> which could both use stdin, help safeguard the user from doing so by
> providing convenience '--stdin-patterns' and '--stdin-pathspecs' which
> are mutually exclusive.  (It is still possible to ask for '-f -
> --pathspecs-file -', though.)
> 
> Finally, in order to match '--pathspecs-file', give '-f' a long form
> '--patterns-file' too.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> Providing too many pathspecs for the argument list to handle is causing
> some gore out in the wild with git-secrets:
> https://github.com/awslabs/git-secrets/issues/78

> 
> A short arg for --pathspecs-file didn't occur to me because grep has
> many short args already, but if anyone has a suggestion I'll happily add
> it.
> 
> The pathspec list is gathered into an argv_array because parse_pathspec
> is only called once, against the entire list of pathspecs. Perhaps it
> makes sense to concatenate the paths given on the command line with the
> paths given via file, but since it was nontrivial to merge two
> argv_arrays and I wasn't certain it was the right choice, I didn't do
> so. I'd probably implement that by adding a utility function to
> argv-array. ('-f somepatterns.txt -e otherpattern' concatenates the
> patterns, by the way.)
> 
>  - Emily
> 
>  Documentation/git-grep.txt | 18 ++++++++++-
>  builtin/grep.c             | 61 +++++++++++++++++++++++++++++++++-----
>  t/t7810-grep.sh            | 52 ++++++++++++++++++++++++++++++--
>  3 files changed, 121 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index c89fb569e3..fb588a3846 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -24,7 +24,8 @@ SYNOPSIS
>  	   [-A <post-context>] [-B <pre-context>] [-C <context>]
>  	   [-W | --function-context]
>  	   [--threads <num>]
> -	   [-f <file>] [-e] <pattern>
> +	   [--pathspecs-file <file>] [--stdin-pathspecs]
> +	   [-f | --patterns-file <file>] [--stdin-patterns] [-e] <pattern>
>  	   [--and|--or|--not|(|)|-e <pattern>...]
>  	   [--recurse-submodules] [--parent-basename <basename>]
>  	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
> @@ -270,7 +271,22 @@ providing this option will cause it to die.
>  	See `grep.threads` in 'CONFIGURATION' for more information.
>  
>  -f <file>::
> +--patterns-file <file>::
>  	Read patterns from <file>, one per line.
> +
> +--stdin-patterns::
> +	Read patterns from stdin, one per line. Equivalent to `-f -`. Cannot be
> +	combined with `--stdin-pathspecs`.
> +
> +--pathspecs-file <file>::
> +	Read pathspecs from <file>, one per line. Pathspecs passed in the
> +	argument list will be ignored in this mode.
> +
> +--stdin-pathspecs::
> +	Read pathspecs from stdin, one per line. Pathspecs passed in the
> +	argument list will be ignored in this mode. Equivalent to
> +	`--pathspecs-file -`.
> +
>  +
>  Passing the pattern via <file> allows for providing a search pattern
>  containing a \0.
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 50ce8d9461..f6f9f84002 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -720,16 +720,38 @@ static int context_callback(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> -static int file_callback(const struct option *opt, const char *arg, int unset)
> +static int pathspec_file(struct argv_array *pathspec_argv, const char *arg)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (!strcmp(arg, "-")) {
> +		while (strbuf_getline(&sb, stdin) != EOF)
> +			argv_array_push(pathspec_argv, sb.buf);
> +	}
> +	else {
> +		if (!strbuf_read_file(&sb, arg, 0))
> +			die_errno(_("cannot open '%s'"), arg);
> +		argv_array_split(pathspec_argv, sb.buf);
> +	}
> +
> +	strbuf_release(&sb);
> +	return 0;
> +}
> +
> +static int pathspec_file_callback(const struct option *opt,
> +				  const char *arg, int unset)
> +{
> +	BUG_ON_OPT_NEG(unset);
> +	return pathspec_file(opt->value, arg);
> +}
> +
> +static int pattern_file(struct grep_opt *grep_opt, const char *arg)
>  {
> -	struct grep_opt *grep_opt = opt->value;
>  	int from_stdin;
>  	FILE *patterns;
>  	int lno = 0;
>  	struct strbuf sb = STRBUF_INIT;
>  
> -	BUG_ON_OPT_NEG(unset);
> -
>  	from_stdin = !strcmp(arg, "-");
>  	patterns = from_stdin ? stdin : fopen(arg, "r");
>  	if (!patterns)
> @@ -748,6 +770,12 @@ static int file_callback(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>  
> +static int pattern_file_callback(const struct option *opt, const char *arg, int unset)
> +{
> +	BUG_ON_OPT_NEG(unset);
> +	return pattern_file(opt->value, arg);
> +}
> +
>  static int not_callback(const struct option *opt, const char *arg, int unset)
>  {
>  	struct grep_opt *grep_opt = opt->value;
> @@ -803,12 +831,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	struct grep_opt opt;
>  	struct object_array list = OBJECT_ARRAY_INIT;
>  	struct pathspec pathspec;
> +	struct argv_array pathspec_argv;
>  	struct string_list path_list = STRING_LIST_INIT_NODUP;
>  	int i;
>  	int dummy;
>  	int use_index = 1;
>  	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
>  	int allow_revs;
> +	int stdin_patterns = 0;
> +	int stdin_pathspecs = 0;
>  
>  	struct option options[] = {
>  		OPT_BOOL(0, "cached", &cached,
> @@ -896,8 +927,14 @@ 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"),
> -			N_("read patterns from file"), file_callback),
> +		OPT_CALLBACK('f', "patterns-file", &opt, N_("file"),
> +			N_("read patterns from file"), pattern_file_callback),
> +		OPT_BOOL(0, "stdin-patterns", &stdin_patterns,
> +			 N_("read patterns from stdin")),
> +		OPT_CALLBACK(0, "pathspecs-file", &pathspec_argv, N_("file"),
> +			N_("read pathspecs from file"), pathspec_file_callback),
> +		OPT_BOOL(0, "stdin-pathspecs", &stdin_pathspecs,
> +			 N_("read pathspecs from stdin")),
>  		{ OPTION_CALLBACK, 'e', NULL, &opt, N_("pattern"),
>  			N_("match <pattern>"), PARSE_OPT_NONEG, pattern_callback },
>  		{ OPTION_CALLBACK, 0, "and", &opt, NULL,
> @@ -949,6 +986,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  			     PARSE_OPT_STOP_AT_NON_OPTION);
>  	grep_commit_pattern_type(pattern_type_arg, &opt);
>  
> +	if (stdin_patterns && stdin_pathspecs)
> +		die(_("cannot use stdin for both patterns and pathspecs"));
> +
> +	if (stdin_patterns)
> +		pattern_file(&opt, "-");
> +
> +	if (stdin_pathspecs)
> +		pathspec_file(&pathspec_argv, "-");
> +
>  	if (use_index && !startup_info->have_repository) {
>  		int fallback = 0;
>  		git_config_get_bool("grep.fallbacktonoindex", &fallback);
> @@ -1057,7 +1103,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	parse_pathspec(&pathspec, 0,
>  		       PATHSPEC_PREFER_CWD |
>  		       (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
> -		       prefix, argv + i);
> +		       prefix,
> +		       (pathspec_argv.argc ? pathspec_argv.argv : argv) + i);
>  	pathspec.max_depth = opt.max_depth;
>  	pathspec.recursive = 1;
>  	pathspec.recurse_submodules = !!recurse_submodules;
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 7d7b396c23..be8e60cfa0 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '
>  	test_must_fail git grep -f patterns
>  '
>  
> +text_expect_success 'grep --pathspec-file, non-existent file' '
> +	test_must_fail git grep --pathspecs-file pathspecs
> +'
> +
>  cat >expected <<EOF
>  file:foo mmap bar
>  file:foo_mmap bar
> @@ -582,8 +586,8 @@ mmap
>  vvv
>  EOF
>  
> -test_expect_success 'grep -f, multiple patterns' '
> -	git grep -f patterns >actual &&
> +test_expect_success 'grep --patterns-file, multiple patterns' '
> +	git grep --patterns-file patterns >actual &&
>  	test_cmp expected actual
>  '
>  
> @@ -621,6 +625,11 @@ test_expect_success 'grep -f, ignore empty lines, read patterns from stdin' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'grep --stdin--patterns' '
> +	git grep --stdin-patterns <patterns >actual &&
> +	test_cmp expected actual
> +'
> +
>  cat >expected <<EOF
>  y:y yy
>  --
> @@ -1125,6 +1134,45 @@ test_expect_success 'grep --no-index descends into repos, but not .git' '
>  	)
>  '
>  
> +test_expect_success 'setup pathspecs-file tests' '
> +cat >excluded-file <<EOF &&
> +bar
> +EOF
> +cat >pathspec-file <<EOF &&
> +foo
> +bar
> +baz
> +EOF
> +cat >unrelated-file <<EOF &&
> +xyz
> +EOF
> +git add excluded-file pathspec-file unrelated-file
> +'

> +
> +cat >pathspecs <<EOF
> +pathspec-file
> +unrelated-file
> +EOF
> +
> +cat >expected <<EOF
> +pathspec-file:bar
> +EOF

> +
> +test_expect_success 'grep --stdin-pathspecs' '
> +	git grep --stdin-pathspecs "bar" <pathspecs >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep --pathspecs-file with file' '
> +	git grep --pathspecs-file pathspecs "bar" >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep --pathspec-file with stdin' '
> +	git grep --pathspecs-file - "bar" <pathspecs >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'setup double-dash tests' '
>  cat >double-dash <<EOF &&
>  --
> -- 
> 2.24.0.432.g9d3f5f5b63-goog
> 

> Denton Liu <liu.denton@gmail.com> writes:
>
>> The reason I ask is because (correct me if I'm wrong) a lot of other git
>> commands (like add, reset and checkout) don't seem to accept pathspecs
>> via stdin and could suffer the same problem. xargs seems like a more
>> general way of solving the problem of long command lines.
>
> There are things "xargs" is sufficient, and there are things that
> absolutely requires a single invocation of "git".  "grep" is a bit
> of both.

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > The reason I ask is because (correct me if I'm wrong) a lot of other git
> > commands (like add, reset and checkout) don't seem to accept pathspecs
> > via stdin and could suffer the same problem. xargs seems like a more
> > general way of solving the problem of long command lines.
> 
> You contributors who are potentially throwing your own topics into
> the cauldron, please be paying a bit more attention to other topics
> already cooking in the pot.  I think am/pathspec-from-file wants to
> go in the general direction.

> 
> There are things "xargs" is sufficient, and there are things that
> absolutely requires a single invocation of "git".  "grep" is a bit
> of both.
> 
>     $ git grep -e "$pattern" -- A B C
> 
> (where A, B and C are hundreds) can be split into three independent
> invocations of "git grep" via "xargs", essentially running
> 
>     $ git grep -e "$pattern" -- A
>     $ git grep -e "$pattern" -- B
>     $ git grep -e "$pattern" -- C
> 
> independently.

> 
> But
> 
>     $ git grep -e P1 -e P2 -e P3 -- A
> 
> (where each of "-e Pn" in reality may be "-e Pn1 -e Pn2 -e Pn3..."
> that has hundreds of patterns) cannot be split into separate
> invocations and keep the same meaning.
> 
>     $ git grep -e P1 -- A
>     $ git grep -e P2 -- A
>     $ git grep -e P3 -- A
> 
> may show the same lines, but (1) lines with both P1 and P2 would be
> shown duplicated, and (2) the order of the output would be different
> from a single invocation looking for all patterns at once.

> 
> Needless to say, the ability to combine patterns with --all-match,
> --and, etc., and negate them would mean the list of patterns must be
> split (even when it makes sense to do so) at the right places.
> 
>     $ git grep -e P1 --and -e P2
> 
> cannot be split into two or more invocations, for example.

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > The reason I ask is because (correct me if I'm wrong) a lot of other git
> > commands (like add, reset and checkout) don't seem to accept pathspecs
> > via stdin and could suffer the same problem. xargs seems like a more
> > general way of solving the problem of long command lines.
> 
> You contributors who are potentially throwing your own topics into
> the cauldron, please be paying a bit more attention to other topics
> already cooking in the pot.  I think am/pathspec-from-file wants to
> go in the general direction.

> On Fri, Nov 22, 2019 at 11:34:17AM +0900, Junio C Hamano wrote:
> > Denton Liu <liu.denton@gmail.com> writes:
> > 
> > > The reason I ask is because (correct me if I'm wrong) a lot of other git
> > > commands (like add, reset and checkout) don't seem to accept pathspecs
> > > via stdin and could suffer the same problem. xargs seems like a more
> > > general way of solving the problem of long command lines.
> > 
> > You contributors who are potentially throwing your own topics into
> > the cauldron, please be paying a bit more attention to other topics
> > already cooking in the pot.  I think am/pathspec-from-file wants to
> > go in the general direction.
> 
> Thanks for pointing it out. It is certainly easy to miss the big picture
> when you spend your time looking at a bug queue instead of a review
> queue; we who stand in different places see different things more
> clearly. I appreciate the reminder to look around a little more.
> 
> am/pathspec-from-file does solve this issue in the way that we discussed
> internally, so I'm excited to base a solution for this issue on that
> branch instead.

> In this situation - where it is not a related fixup for a branch in
> next, but the topic does rely on that branch - should I send a series
> which is based on 'next'? How do I make the dependency clear to you via
> emailed patch?
> 
>  - Emily

> @@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \0 for
>  more search backends, until then we'll die when the pattern type in
>  question doesn't support them.
>  
> +--pathspec-from-file <file>::
> +	Read pathspec from <file> instead of the command line. If `<file>` is
> +	exactly `-` then standard input is used; standard input cannot be used
> +	for both --patterns-from-file and --pathspec-from-file. Pathspec elements
> +	are separated by LF or CR/LF. Pathspec elements can be quoted as
> +	explained for the configuration variable `core.quotePath` (see
> +	linkgit:git-config[1]). See also `--pathspec-file-nul` and global
> +	`--literal-pathspecs`.
> +
> +--pathspec-file-nul::
> +	Only meaningful with `--pathspec-from-file`. Pathspec elements are
> +	separated with NUL character and all other characters are taken
> +	literally (including newlines and quotes).

> @@ -1125,6 +1129,44 @@ test_expect_success 'grep --no-index descends into repos, but not .git' '
>  	)
>  '
>  
> +test_expect_success 'setup pathspecs-file tests' '
> +cat >excluded-file <<EOF &&
> +bar
> +EOF
> +cat >pathspec-file <<EOF &&
> +foo
> +bar
> +baz
> +EOF
> +cat >unrelated-file <<EOF &&
> +xyz
> +EOF
> +git add excluded-file pathspec-file unrelated-file
> +'

> +
> +cat >pathspecs <<EOF
> +pathspec-file
> +unrelated-file
> +EOF
> +
> +cat >expected <<EOF
> +pathspec-file:bar
> +EOF

> +
> +test_expect_success 'grep --pathspec-from-file with file' '
> +	git grep --pathspec-from-file pathspecs "bar" >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep --pathspec-file with stdin' '
> +	git grep --pathspec-from-file - "bar" <pathspecs >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep with two stdin inputs fails' '
> +	test_must_fail git grep --pathspec-from-file - --patterns-from-file - <pathspecs
> +'
> +
>  test_expect_success 'setup double-dash tests' '
>  cat >double-dash <<EOF &&
>  --
> -- 
> 2.24.0.393.g34dc348eaf-goog
> 

>> +--pathspec-file-nul::
>> +	Only meaningful with `--pathspec-from-file`. Pathspec elements are
>> +	separated with NUL character and all other characters are taken
>> +	literally (including newlines and quotes).
>
> Does it make sense to have a corresponding --patterns-file-nul option?

>> +test_expect_success 'setup pathspecs-file tests' '
>> +cat >excluded-file <<EOF &&
>> +bar
>> +EOF
>> ...
>> +git add excluded-file pathspec-file unrelated-file
>> +'
>
> Could you please change these here-docs to be <<-\EOF and then indent
> the test case?

> Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the
> documentation accordingly.
>
> This changes enables 'git grep' to receive the pathspec from a file by
> specifying the path, or from stdin by specifying '-' as the path. This
> matches the previous functionality of '-f', so the documentation of '-f'
> has been expanded to describe this functionality. To let '-f' match the
> new '--pathspec-from-file' option, also teach a '--patterns-from-file'
> long name to '-f'.
>
> Since there are now two arguments which can attempt to read from stdin,
> add a safeguard to check whether the user specified '-' for both of
> them. It is still possible for a user to pass '/dev/stdin' to one or
> both arguments at present; we do not explicitly check.

> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index c89fb569e3..56b1c5a302 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -24,7 +24,8 @@ SYNOPSIS
>  	   [-A <post-context>] [-B <pre-context>] [-C <context>]
>  	   [-W | --function-context]
>  	   [--threads <num>]
> -	   [-f <file>] [-e] <pattern>
> +	   [-f | --patterns-from-file <file>] [-e] <pattern>

> @@ -270,7 +271,10 @@ providing this option will cause it to die.
>  	See `grep.threads` in 'CONFIGURATION' for more information.
>  
>  -f <file>::
> -	Read patterns from <file>, one per line.
> +--patterns-from-file <file>::
> +	Read patterns from <file>, one per line. If `<file>` is exactly `-` then
> +	standard input is used; standard input cannot be used for both
> +	--patterns-from-file and --pathspec-from-file.

> @@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \0 for
>  more search backends, until then we'll die when the pattern type in
>  question doesn't support them.
>  
> +--pathspec-from-file <file>::
> +	Read pathspec from <file> instead of the command line. If `<file>` is
> +	exactly `-` then standard input is used; standard input cannot be used
> +	for both --patterns-from-file and --pathspec-from-file. Pathspec elements
> +	are separated by LF or CR/LF. Pathspec elements can be quoted as
> +	explained for the configuration variable `core.quotePath` (see
> +	linkgit:git-config[1]). See also `--pathspec-file-nul` and global
> +	`--literal-pathspecs`.
> +
> +--pathspec-file-nul::
> +	Only meaningful with `--pathspec-from-file`. Pathspec elements are
> +	separated with NUL character and all other characters are taken
> +	literally (including newlines and quotes).
> +

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 50ce8d9461..54ba991c42 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -31,6 +31,7 @@ static char const * const grep_usage[] = {
>  };
>  
>  static int recurse_submodules;
> +static int patterns_from_stdin, pathspec_from_stdin;
>  
>  #define GREP_NUM_THREADS_DEFAULT 8
>  static int num_threads;
> @@ -723,15 +724,18 @@ static int context_callback(const struct option *opt, const char *arg,
>  static int file_callback(const struct option *opt, const char *arg, int unset)
>  {

>  	struct grep_opt *grep_opt = opt->value;
> -	int from_stdin;
>  	FILE *patterns;
>  	int lno = 0;
>  	struct strbuf sb = STRBUF_INIT;
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	from_stdin = !strcmp(arg, "-");
> -	patterns = from_stdin ? stdin : fopen(arg, "r");
> +	patterns_from_stdin = !strcmp(arg, "-");

> +	if (pathspec_from_file) {
> +		if (pathspec.nr)
> +			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
> +
> +		pathspec_from_stdin = !strcmp(pathspec_from_file, "-");
> +
> +		if (patterns_from_stdin && pathspec_from_stdin)
> +			die(_("cannot specify both patterns and pathspec via stdin"));

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the
> > documentation accordingly.
> >
> > This changes enables 'git grep' to receive the pathspec from a file by
> > specifying the path, or from stdin by specifying '-' as the path. This
> > matches the previous functionality of '-f', so the documentation of '-f'
> > has been expanded to describe this functionality. To let '-f' match the
> > new '--pathspec-from-file' option, also teach a '--patterns-from-file'
> > long name to '-f'.
> >
> > Since there are now two arguments which can attempt to read from stdin,
> > add a safeguard to check whether the user specified '-' for both of
> > them. It is still possible for a user to pass '/dev/stdin' to one or
> > both arguments at present; we do not explicitly check.
> 
> OK.  I guess this is good enough at least for now and possibly
> forever as that falls into the "doctor, it hurts when I do this"
> category.
> 
> > Refactored to use am/pathspec-from-file. This changes the implementation
> > significantly since v1, but the testcases mostly remain the same.
> 
> I am hoping that this topic, and Alexandr's "add" and "checkout"
> stuff, would help establishing a good pattern for other commands to
> follow.
> 
> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> > index c89fb569e3..56b1c5a302 100644
> > --- a/Documentation/git-grep.txt
> > +++ b/Documentation/git-grep.txt
> > @@ -24,7 +24,8 @@ SYNOPSIS
> >  	   [-A <post-context>] [-B <pre-context>] [-C <context>]
> >  	   [-W | --function-context]
> >  	   [--threads <num>]
> > -	   [-f <file>] [-e] <pattern>
> > +	   [-f | --patterns-from-file <file>] [-e] <pattern>
> 
> OK.
> 
> > +	   [--pathspec-from-file=<file> [--pathspec-file-nul]]
> 
> OK.
> 
> > @@ -270,7 +271,10 @@ providing this option will cause it to die.
> >  	See `grep.threads` in 'CONFIGURATION' for more information.
> >  
> >  -f <file>::
> > -	Read patterns from <file>, one per line.
> > +--patterns-from-file <file>::
> > +	Read patterns from <file>, one per line. If `<file>` is exactly `-` then
> > +	standard input is used; standard input cannot be used for both
> > +	--patterns-from-file and --pathspec-from-file.
> 
> Makes sense---otherwise they would compete and we cannot know which
> kind each line in the standard input is ;-)

> 
> > @@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \0 for
> >  more search backends, until then we'll die when the pattern type in
> >  question doesn't support them.
> >  
> > +--pathspec-from-file <file>::
> > +	Read pathspec from <file> instead of the command line. If `<file>` is
> > +	exactly `-` then standard input is used; standard input cannot be used
> > +	for both --patterns-from-file and --pathspec-from-file. Pathspec elements
> > +	are separated by LF or CR/LF. Pathspec elements can be quoted as
> > +	explained for the configuration variable `core.quotePath` (see
> > +	linkgit:git-config[1]). See also `--pathspec-file-nul` and global
> > +	`--literal-pathspecs`.
> > +
> > +--pathspec-file-nul::
> > +	Only meaningful with `--pathspec-from-file`. Pathspec elements are
> > +	separated with NUL character and all other characters are taken
> > +	literally (including newlines and quotes).
> > +
> 
> I think these matches the ones in git-reset.txt from the earlier
> work by Alexandr's, which is good.

> 
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 50ce8d9461..54ba991c42 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -31,6 +31,7 @@ static char const * const grep_usage[] = {
> >  };
> >  
> >  static int recurse_submodules;
> > +static int patterns_from_stdin, pathspec_from_stdin;
> >  
> >  #define GREP_NUM_THREADS_DEFAULT 8
> >  static int num_threads;
> > @@ -723,15 +724,18 @@ static int context_callback(const struct option *opt, const char *arg,
> >  static int file_callback(const struct option *opt, const char *arg, int unset)
> >  {
> 
> Shouldn't this be renamed?  Earlier, the only thing that can be
> taken from a file was the patterns, but now a file can be given
> to specify a set of pathspec elements.  "patterns_file_callback()"
> or something like taht?

> 
> >  	struct grep_opt *grep_opt = opt->value;
> > -	int from_stdin;
> >  	FILE *patterns;
> >  	int lno = 0;
> >  	struct strbuf sb = STRBUF_INIT;
> >  
> >  	BUG_ON_OPT_NEG(unset);
> >  
> > -	from_stdin = !strcmp(arg, "-");
> > -	patterns = from_stdin ? stdin : fopen(arg, "r");
> > +	patterns_from_stdin = !strcmp(arg, "-");
> 
> Totally outside the scope of this patch, but we may want to
> introduce
> 
> 	int is_stdin_cmdline_marker(const char *arg)
> 	{
> 		return !strcmp(arg, "-");
> 	}
> 
> which later can be taught also about "/dev/stdin".  Even outside the
> pathspec-from-file option, I think "diff --no-index" also has some
> special-case provision for treating "-" as "input coming from the
> standard input string", which would benefit from such a helper.

> 
> > +	if (patterns_from_stdin && pathspec_from_stdin)
> > +		die(_("cannot specify both patterns and pathspec via stdin"));
> 
> It's kind of sad that we need to check this in this callback and
> also inside cmd_grep() top-level we will see further down...

> 
> > +	if (pathspec_from_file) {
> > +		if (pathspec.nr)
> > +			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
> > +
> > +		pathspec_from_stdin = !strcmp(pathspec_from_file, "-");
> > +
> > +		if (patterns_from_stdin && pathspec_from_stdin)
> > +			die(_("cannot specify both patterns and pathspec via stdin"));
> 
> ... here.
> 
> Thanks.

>   static int file_callback(const struct option *opt, const char *arg, int unset)
>   {
>   	struct grep_opt *grep_opt = opt->value;
> -	int from_stdin;
>   	FILE *patterns;
>   	int lno = 0;
>   	struct strbuf sb = STRBUF_INIT;
>   
>   	BUG_ON_OPT_NEG(unset);
>   
> -	from_stdin = !strcmp(arg, "-");
> -	patterns = from_stdin ? stdin : fopen(arg, "r");
> +	patterns_from_stdin = !strcmp(arg, "-");
> +
> +	if (patterns_from_stdin && pathspec_from_stdin)

> I'm excited to see someone else join my effort, thanks for continuing my
> effort! Also, less work for me :)
> 
> On 04.12.2019 21:39, Emily Shaffer wrote:
> 
> >   static int file_callback(const struct option *opt, const char *arg, int unset)
> >   {
> >   	struct grep_opt *grep_opt = opt->value;
> > -	int from_stdin;
> >   	FILE *patterns;
> >   	int lno = 0;
> >   	struct strbuf sb = STRBUF_INIT;
> >   	BUG_ON_OPT_NEG(unset);
> > -	from_stdin = !strcmp(arg, "-");
> > -	patterns = from_stdin ? stdin : fopen(arg, "r");
> > +	patterns_from_stdin = !strcmp(arg, "-");
> > +
> > +	if (patterns_from_stdin && pathspec_from_stdin)
> 
> To my understanding, this check will not work as expected. `file_callback`
> will be called at the moment of parsing args. `pathspec_from_stdin` is only
> initialized later.
> 
> Maybe it would be better to convert `file_callback` into a regular function
> and call it after the options were parsed, similar to how
> `pathspec_from_file` is parsed later?
> 
> This will also allow to move global variables into local scope and resolve
> other small issues raised by other reviewers.

> 
> > +test_expect_success 'grep with two stdin inputs fails' '
> > +	test_must_fail git grep --pathspec-from-file - --patterns-from-file - <pathspecs
> > +'
> > +
> 
> It is usually a good idea to test for specific error, like this:
> 
>   test_must_fail git grep --pathspec-from-file - --patterns-from-file -
> <pathspecs 2>err &&
>   test_i18ngrep "cannot specify both patterns and pathspec via stdin" err &&

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 50ce8d9461..54ba991c42 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -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;
>
>  	struct option options[] = {

> @@ -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;

>  
>  	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) {

> +		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);

> +	} 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"));

> +	if (patterns_from_file && pathspec_from_file &&
> +	    !strcmp(pathspec_from_file, "-") &&
> +	    !strcmp(patterns_from_file, "-"))
> +		die(_("cannot specify both patterns and pathspec via stdin"));
> +
> +	if (patterns_from_file)
> +		read_pattern_file(&opt, patterns_from_file);
> +
> +

> +test_expect_success 'setup pathspecs-file tests' '
> +cat >excluded-file <<EOF &&
> +bar
> +EOF
> +cat >pathspec-file <<EOF &&
> +foo
> +bar
> +baz
> +EOF
> +cat >unrelated-file <<EOF &&
> +xyz
> +EOF
> +git add excluded-file pathspec-file unrelated-file
> +'

> +
> +cat >pathspecs <<EOF
> +pathspec-file
> +unrelated-file
> +EOF
> +
> +cat >expected <<EOF
> +pathspec-file:bar
> +EOF

> Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the
> documentation accordingly.
>
> This changes enables 'git grep' to receive the pathspec from a file by
> specifying the path, or from stdin by specifying '-' as the path. This
> matches the previous functionality of '-f', so the documentation of '-f'
> has been expanded to describe this functionality. To let '-f' match the
> new '--pathspec-from-file' option, also teach a '--patterns-from-file'
> long name to '-f'.
>
> Since there are now two arguments which can attempt to read from stdin,
> add a safeguard to check whether the user specified '-' for both of
> them. It is still possible for a user to pass '/dev/stdin' to one or
> both arguments at present; we do not explicitly check.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> I built this on 'next' locally, but it does require 'am/pathspec-from-file'
> (which was merged to 'next').

> -static int file_callback(const struct option *opt, const char *arg, int unset)
> +static int read_pattern_file(struct grep_opt *grep_opt, const char *path)
>  {
> -	struct grep_opt *grep_opt = opt->value;
>  	int from_stdin;
>  	FILE *patterns;
>  	int lno = 0;
>  	struct strbuf sb = STRBUF_INIT;
>  
> -	BUG_ON_OPT_NEG(unset);
> -
> -	from_stdin = !strcmp(arg, "-");
> -	patterns = from_stdin ? stdin : fopen(arg, "r");
> +	from_stdin = !strcmp(path, "-");
> +	patterns = from_stdin ? stdin : fopen(path, "r");
>  	if (!patterns)
> -		die_errno(_("cannot open '%s'"), arg);
> +		die_errno(_("cannot open '%s'"), path);
>  	while (strbuf_getline(&sb, patterns) == 0) {
>  		/* ignore empty line like grep does */
>  		if (sb.len == 0)
>  			continue;
>  
> -		append_grep_pat(grep_opt, sb.buf, sb.len, arg, ++lno,
> +		append_grep_pat(grep_opt, sb.buf, sb.len, path, ++lno,
>  				GREP_PATTERN);
>  	}
>  	if (!from_stdin)

> +test_expect_success 'setup pathspecs-file tests' '
> +cat >excluded-file <<EOF &&
> +bar
> +EOF
> +cat >pathspec-file <<EOF &&
> +foo
> +bar
> +baz
> +EOF
> +cat >unrelated-file <<EOF &&
> +xyz
> +EOF
> +git add excluded-file pathspec-file unrelated-file
> +'

> +
> +cat >pathspecs <<EOF
> +pathspec-file
> +unrelated-file
> +EOF
> +
> +cat >expected <<EOF
> +pathspec-file:bar
> +EOF

> +test_expect_success 'grep --pathspec-from-file with file' '
> +	git grep --pathspec-from-file pathspecs "bar" >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep --pathspec-file with stdin' '
> +	git grep --pathspec-from-file - "bar" <pathspecs >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'grep with two stdin inputs fails' '
> +	test_must_fail git grep --pathspec-from-file - --patterns-from-file - <pathspecs 2>err &&
> +	test_i18ngrep "cannot specify both patterns and pathspec via stdin" err
> +'
> +
>  test_expect_success 'setup double-dash tests' '
>  cat >double-dash <<EOF &&
>  --

> Hmph.  This has nothing to do with "--pathspec-from-file" that was
> advertised on the title of the patch.  It used to be that
> 
> 	git grep -f one -f two
> 
> can be used to read patterns from two sources, but that is no
> longer possible, is it?  Am I missing a larger benefit to accept
> this regression?

> On 13.12.2019 19:26, Junio C Hamano wrote:
> 
> > Hmph.  This has nothing to do with "--pathspec-from-file" that was
> > advertised on the title of the patch.  It used to be that
> > 
> > 	git grep -f one -f two
> > 
> > can be used to read patterns from two sources, but that is no
> > longer possible, is it?  Am I missing a larger benefit to accept
> > this regression?
> 
> Ouch, that comes completely unexpected to me. It's good when someone
> experienced watches over :)