[PATCH] grep: provide pathspecs/patterns via file or stdin

[PATCH] grep: provide pathspecs/patterns via file or stdin

[Emily Shaffer]

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

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Denton Liu]

Hi Emily,

On Thu, Nov 21, 2019 at 05:16:46PM -0800, Emily Shaffer wrote:
> 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

Out of curiosity, are there any situations that --pathspecs-file handles
better than an xargs invocation? I didn't take a look at the code of
git-secrets but I suspect that that could solve their problem.

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.

> 
> 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
> +'

Please indent these commands in and use `-\EOF` for the here-doc.

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

Is there any reason why these two aren't part of the setup above?

Also, even though 'expect' is the conventional name for the expected
output file, it seems like the rest of the test uses 'expected' so I
guess it's fine to leave it as is.

Thanks,

Denton

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

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Teach 'git grep' how to receive pathspecs via file, and add convenience
> flags for receiving pathspecs or patterns via stdin.

When I invented the terms "pathspec", "refspec", etc. for this
project, I meant them to be collective nouns.  A pathspec is a set
of pathspec elements, each of which is usually given as a separate
argument on the command line.  So "Read pathspec from file" is good
(not "Read pathspecs from file").

Are you aware of the am/pathspec-from-file topic that has been
cooking for a while by now?

I do not necessarily agree with the second rationale why many
commands may want to learn this option given in that series, but do
agree with the first one.  A lot of times running underlying "git"
command multiple times via xargs invocation can mean quite different
thing from giving a large pathspec to a single invocation of "git".
But not with "git grep" ;-)  So I'd say this is a second class
target for the pathspec-from-file theme.

Thanks.

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Junio C Hamano]

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.

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

Sorry for not addressing your sample commands.

The "add", "reset", "checkout" subcommands are fine examples that
driving them with "xargs" is 100% sufficient and there is no need
for the pathspec-from-file thing; adding it to them would probably
not make the system any worse, but it is far far less important than
some other "git" subcommands for which "xargs" does not work as a
replacement.

Thanks.

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Denton Liu]

Hi Junio,

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.

Interesting, I never caught this topic when it went over the list. I
guess I should read your What's Cooking emails more thoroughly instead
of just scanning for my own contributions.

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

In the above, I was talking about the new --pathspecs-file option in
particular. So it looks like you agree with me that the new option
doesn't supercede xargs?

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

We already have `-f` to handle this particular case, no?

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

Anyway, thanks for going into detail about this. It makes things a lot
more clear.

-Denton

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Emily Shaffer]

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

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Emily Shaffer]

On Fri, Nov 22, 2019 at 02:02:41PM -0800, Emily Shaffer wrote:
> 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 fact, as I think further, it could be that the solution is "don't
change anything, just use --pathspec-from-file". But I think my next
question will still be useful to learn the response to anyway. :)

> 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

Re: [PATCH] grep: provide pathspecs/patterns via file or stdin

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

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

If you need to use a particular new feature from a topic (or two),
not depending on 'next' but either building on

 (1) directly that topic, or

 (2) a local merge you create across the 'master' branch and the
     topic(s) you want to depend on

would be good.  Whichever way you go, leaving a note either on the
cover letter or after the three-dash line to say what you did would
always be helpful to the reviewers.

In this particular case, I think (1) would work better for you, as
the topic you want to use builds directly on the latest release so
you won't be missing anything that is not yet on that topic but
already in 'master' at this point.

Thanks.

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

[Emily Shaffer]

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>
---
Refactored to use am/pathspec-from-file. This changes the implementation
significantly since v1, but the testcases mostly remain the same.

This change builds on top of am/pathspec-from-file and is dependent on
at least "parse-options.h: add new options `--pathspec-from-file`,
`--pathspec-file-nul`". I followed the example of "commit: support the
--pathspec-from-file" option and retained the tests from v1 of this
topic.

 Documentation/git-grep.txt | 22 ++++++++++++++++--
 builtin/grep.c             | 35 ++++++++++++++++++++++++-----
 t/t7810-grep.sh            | 46 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 94 insertions(+), 9 deletions(-)

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>
+	   [--pathspec-from-file=<file> [--pathspec-file-nul]]
 	   [--and|--or|--not|(|)|-e <pattern>...]
 	   [--recurse-submodules] [--parent-basename <basename>]
 	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
@@ -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.
 +
 Passing the pattern via <file> allows for providing a search pattern
 containing a \0.
@@ -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).
+
 -e::
 	The next parameter is the pattern. This option has to be
 	used for patterns starting with `-` and should be used in
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 (patterns_from_stdin && pathspec_from_stdin)
+		die(_("cannot specify both patterns and pathspec via stdin"));
+
+	patterns = patterns_from_stdin ? stdin : fopen(arg, "r");
 	if (!patterns)
 		die_errno(_("cannot open '%s'"), arg);
 	while (strbuf_getline(&sb, patterns) == 0) {
@@ -742,7 +746,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset)
 		append_grep_pat(grep_opt, sb.buf, sb.len, arg, ++lno,
 				GREP_PATTERN);
 	}
-	if (!from_stdin)
+	if (!patterns_from_stdin)
 		fclose(patterns);
 	strbuf_release(&sb);
 	return 0;
@@ -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) {
+		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"));
+
+		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"));
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 7d7b396c23..355890a72a 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-from-file, non-existent file' '
+	test_must_fail git grep --pathspec-from-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-from-file, multiple patterns' '
+	git grep --patterns-from-file patterns >actual &&
 	test_cmp expected actual
 '
 
@@ -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

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

[Denton Liu]

Hi Emily,

On Wed, Dec 04, 2019 at 12:39:11PM -0800, Emily Shaffer wrote:
> @@ -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).

Does it make sense to have a corresponding --patterns-file-nul option?
As in, is it possible for patterns to contain inline newlines? If it's
not possible, then that option probably isn't necessary.

> +
>  -e::
>  	The next parameter is the pattern. This option has to be
>  	used for patterns starting with `-` and should be used in

> @@ -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
> +'

Could you please change these here-docs to be <<-\EOF and then indent
the test case?

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

In an earlier email, I was wondering aloud why these two blocks were
outside of the test case above. I think the answer to that is that
you're following the existing style of the test case.

In that case, could I pester you to do some test clean up while you're
at it? I think it'd be nice to move the cats into their respective test
cases (with <<-\EOF) and also rename the files from 'expected' to
'expect'. But otherwise, I think it's fine as is as well.

Thanks,

Denton

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

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

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

>> +--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?

I do not think so.  "grep" is always record oriented and the record
separter is the LF, so patterns file can safely be delimited with
LF.

>> +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?

Good suggestion.

	test_expect_success 'setup ...' '
		cat >excluded-file <<-\EOF &&
		bar
		EOF
		...
		git add ...
	'

If each line in these files consists of a short single token (which
seems to be the case), perhaps consider using test_write_lines?

	test_write_lines >excluded-file bar &&
	test_write_lines >pathspec-file foo bar baz &&
	test_write_lines >unrelated-file xyz &&
	test_write_lines >pathspecs pathspec-file unrelated-file &&

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

[Junio C Hamano]

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.

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

[Alexandr Miloslavskiy]

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

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

[Johannes Schindelin]

Hi Emily,

On Wed, 4 Dec 2019, Emily Shaffer wrote:

> 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[] = {

I need this on top to make it work without problems (I have not yet
triggered the CI build, but I will in a moment):

-- snipsnap --
Subject: [PATCH] fixup??? grep: support the --pathspec-from-file option

We must not use those variables uninitialized, this causes CI build
failures left and right (see e.g.
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=22821)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 54ba991c425..c2aa1aeebd6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -813,8 +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;
+	char *pathspec_from_file = NULL;
+	int pathspec_file_nul = 0;

 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
--
2.24.0.windows.2.611.ge9aced84530

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

[SZEDER Gábor]

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

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

> +		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.

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

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

[Emily Shaffer]

On Wed, Dec 04, 2019 at 02:24:07PM -0800, Junio C Hamano wrote:
> 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 ;-)

At one point Jonathan Nieder had suggested to me that it might be nice
to watch for "--" in the standard input. But I think I wasn't confident
in the idea that all commands which take arbitrary number of arguments,
and pathspec expect their arg list to end in <arg...> -- <pathspec>.

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

Yes, I took them verbatim.

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

Done. I was hesitant to touch the other code; I guess it is fine.

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

Agreed; that's probably a handy thing to put into parse-options.h. I
suggest we think of this next time we are looking for Outreachy
microprojects or similar. I'll write it down somewhere.

> 
> > +	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...

Another reviewer suggested not using a callback and instead waiting
until after the entire argparse to do file reads; this would let us
avoid the weird double check here. I think I will do it.

> 
> > +	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.

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

[Emily Shaffer]

On Thu, Dec 05, 2019 at 12:58:19PM +0100, Alexandr Miloslavskiy wrote:
> 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.

Yeah, I think this is a good idea for the reasons you stated, so I'll do
so.

> 
> > +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 &&

Sure. Thanks.

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

[Emily Shaffer]

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').

Thanks all who caught the OSX/GfW CI failures; the test suite passed for me
in Linux and so I completely missed the uninitialized variables. Not a
great look for me...

Since v2, I got rid of the callback for -f arg and am now running it
directly; this means we can check for stdin in only one place, instead
of two, and get rid of some of the globals which were added elsewhere.
The test suite still passes for me (but still on Linux) so here's hoping
for a more favorable result with the rest of the CI.

 - Emily

 Documentation/git-grep.txt | 22 ++++++++++++++++--
 builtin/grep.c             | 43 ++++++++++++++++++++++++++--------
 t/t7810-grep.sh            | 47 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 98 insertions(+), 14 deletions(-)

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>
+	   [--pathspec-from-file=<file> [--pathspec-file-nul]]
 	   [--and|--or|--not|(|)|-e <pattern>...]
 	   [--recurse-submodules] [--parent-basename <basename>]
 	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
@@ -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.
 +
 Passing the pattern via <file> allows for providing a search pattern
 containing a \0.
@@ -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).
+
 -e::
 	The next parameter is the pattern. This option has to be
 	used for patterns starting with `-` and should be used in
diff --git a/builtin/grep.c b/builtin/grep.c
index 50ce8d9461..28b9f99d4c 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -720,26 +720,23 @@ 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 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)
@@ -809,6 +806,9 @@ 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 *patterns_from_file = NULL;
+	char *pathspec_from_file = NULL;
+	int pathspec_file_nul = 0;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
@@ -896,8 +896,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"),
-			N_("read patterns from file"), file_callback),
+		OPT_STRING('f', "patterns-from-file", &patterns_from_file, N_("file"),
+			N_("read patterns from file")),
+		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,
@@ -949,6 +951,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 (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);
+
+
 	if (use_index && !startup_info->have_repository) {
 		int fallback = 0;
 		git_config_get_bool("grep.fallbacktonoindex", &fallback);
@@ -1062,6 +1073,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	pathspec.recursive = 1;
 	pathspec.recurse_submodules = !!recurse_submodules;
 
+	if (pathspec_from_file) {
+		if (pathspec.nr)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		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"));
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 7d7b396c23..3ace2dfdac 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-from-file, non-existent file' '
+	test_must_fail git grep --pathspec-from-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-from-file, multiple patterns' '
+	git grep --patterns-from-file patterns >actual &&
 	test_cmp expected actual
 '
 
@@ -1125,6 +1129,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 --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 &&
 --
-- 
2.24.1.735.g03f4e72817-goog

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

[Alexandr Miloslavskiy]

Thanks, the patch looks solid to me, just a few code style rants :)

On 13.12.2019 5:12, Emily Shaffer wrote:

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

That looks a lot more solid now, thanks!

> @@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '
>   	test_must_fail git grep -f patterns
>   '

Could also benefit from testing for specific error here.

> +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
> +'

Please use <<-EOF and tabulate the code properly. I suggest that you 
have a look at `t/t7107-reset-pathspec-file.sh`, last test.

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

The general approach nowadays is to write `expect` file in every test. 
Note that the standard name is `expect`, so please remove `ed` from your 
name. I think that this is also reasonable for `pathspecs` file, it's 
only used in two places. Again, please refer to 
`t/t7107-reset-pathspec-file.sh`.

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

[Junio C Hamano]

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.
>
> 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').

Thanks.  I'd probably just queue on top of 'master' as the other
topic has graduated as part of the fifth batch.

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

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?

> +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
> +'

As Alexandr mentioned, the above is harder to read than necessary.


	test_expect_success 'setup pathspec-file tests' '
		test_write_lines >excluded-file bar &&
		test_write_lines >pathspec-file foo bar baz &&
		test_write_lines >unrelated-file xyz &&
		git add ...
	'

perhaps?

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

Also, shouldn't the above also be part of the (sub)setup for these
tests?  IOW, after that addition of three files with "git add", 

		test_write_lines >pathspec pathspec-file unrelated-file &&
		test_write_lines >expect pathspec-file:bar

in the "setup pathspec-file tests".

> +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 &&
>  --

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

[Alexandr Miloslavskiy]

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

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

[Emily Shaffer]

On Fri, Dec 13, 2019 at 09:13:09PM +0100, Alexandr Miloslavskiy wrote:
> 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 :)

Yeah, I'm surprised by it too - and surprised that there wasn't a test
to ensure this behavior.

I can put it back as a callback. I'm going to add a second patch to this
topic to enforce 'git grep -f one -f two' with a testcase.

As for missed benefits, they all have to do with ugly code vs. pretty
code. I don't think it's enough of a reason to lose functionality.

 - Emily