{"thread":{"id":"52307","subject":"[PATCH] grep: provide pathspecs/patterns via file or stdin","startedAt":"2019-11-22T01:17:12Z","lastAt":"2019-12-17T00:33:47Z","messageCount":23,"participants":["Emily Shaffer","Denton Liu","Junio C Hamano","Alexandr Miloslavskiy","Johannes Schindelin","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"386785","messageId":"20191122011646.218346-1-emilyshaffer@google.com","threadId":"52307","inReplyTo":null,"subject":"[PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-11-22T01:16:46Z","receivedAt":"2019-11-22T01:17:12Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach 'git grep' how to receive pathspecs via file, and add convenience\nflags for receiving pathspecs or patterns via stdin.\n\nIn some cases, the list of pathspecs to 'git grep' is longer than the\ncommand line allows, or long enough that a user becomes fatigued reading\nthe command. In other cases, a user may wish to write a script or\noneliner which generates a list of files to 'git grep' against. In both\nscenarios, a reasonable workaround is to provide the list of pathspecs\nvia a file or stdin.\n\nBefore this change, it was already possible to receive patterns via\nstdin by using the argument '-f -'. Now that there are two arguments\nwhich could both use stdin, help safeguard the user from doing so by\nproviding convenience '--stdin-patterns' and '--stdin-pathspecs' which\nare mutually exclusive. (It is still possible to ask for '-f -\n--pathspecs-file -', though.)\n\nFinally, in order to match '--pathspecs-file', give '-f' a long form\n'--patterns-file' too.\n\nSigned-off-by: Emily Shaffer \n---\nProviding too many pathspecs for the argument list to handle is causing\nsome gore out in the wild with git-secrets:\nhttps://github.com/awslabs/git-secrets/issues/78\n\nA short arg for --pathspecs-file didn't occur to me because grep has\nmany short args already, but if anyone has a suggestion I'll happily add\nit.\n\nThe pathspec list is gathered into an argv_array because parse_pathspec\nis only called once, against the entire list of pathspecs. Perhaps it\nmakes sense to concatenate the paths given on the command line with the\npaths given via file, but since it was nontrivial to merge two\nargv_arrays and I wasn't certain it was the right choice, I didn't do\nso. I'd probably implement that by adding a utility function to\nargv-array. ('-f somepatterns.txt -e otherpattern' concatenates the\npatterns, by the way.)\n\n - Emily\n\n Documentation/git-grep.txt | 18 ++++++++++-\n builtin/grep.c | 61 +++++++++++++++++++++++++++++++++-----\n t/t7810-grep.sh | 52 ++++++++++++++++++++++++++++++--\n 3 files changed, 121 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex c89fb569e3..fb588a3846 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -24,7 +24,8 @@ SYNOPSIS\n \t [-A ] [-B ] [-C ]\n \t [-W | --function-context]\n \t [--threads ]\n-\t [-f ] [-e] \n+\t [--pathspecs-file ] [--stdin-pathspecs]\n+\t [-f | --patterns-file ] [--stdin-patterns] [-e] \n \t [--and|--or|--not|(|)|-e ...]\n \t [--recurse-submodules] [--parent-basename ]\n \t [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n@@ -270,7 +271,22 @@ providing this option will cause it to die.\n \tSee `grep.threads` in 'CONFIGURATION' for more information.\n \n -f ::\n+--patterns-file ::\n \tRead patterns from , one per line.\n+\n+--stdin-patterns::\n+\tRead patterns from stdin, one per line. Equivalent to `-f -`. Cannot be\n+\tcombined with `--stdin-pathspecs`.\n+\n+--pathspecs-file ::\n+\tRead pathspecs from , one per line. Pathspecs passed in the\n+\targument list will be ignored in this mode.\n+\n+--stdin-pathspecs::\n+\tRead pathspecs from stdin, one per line. Pathspecs passed in the\n+\targument list will be ignored in this mode. Equivalent to\n+\t`--pathspecs-file -`.\n+\n +\n Passing the pattern via allows for providing a search pattern\n containing a \\0.\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 50ce8d9461..f6f9f84002 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -720,16 +720,38 @@ static int context_callback(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static int file_callback(const struct option *opt, const char *arg, int unset)\n+static int pathspec_file(struct argv_array *pathspec_argv, const char *arg)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!strcmp(arg, \"-\")) {\n+\t\twhile (strbuf_getline(&sb, stdin) != EOF)\n+\t\t\targv_array_push(pathspec_argv, sb.buf);\n+\t}\n+\telse {\n+\t\tif (!strbuf_read_file(&sb, arg, 0))\n+\t\t\tdie_errno(_(\"cannot open '%s'\"), arg);\n+\t\targv_array_split(pathspec_argv, sb.buf);\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn 0;\n+}\n+\n+static int pathspec_file_callback(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tBUG_ON_OPT_NEG(unset);\n+\treturn pathspec_file(opt->value, arg);\n+}\n+\n+static int pattern_file(struct grep_opt *grep_opt, const char *arg)\n {\n-\tstruct grep_opt *grep_opt = opt->value;\n \tint from_stdin;\n \tFILE *patterns;\n \tint lno = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tBUG_ON_OPT_NEG(unset);\n-\n \tfrom_stdin = !strcmp(arg, \"-\");\n \tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n \tif (!patterns)\n@@ -748,6 +770,12 @@ static int file_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int pattern_file_callback(const struct option *opt, const char *arg, int unset)\n+{\n+\tBUG_ON_OPT_NEG(unset);\n+\treturn pattern_file(opt->value, arg);\n+}\n+\n static int not_callback(const struct option *opt, const char *arg, int unset)\n {\n \tstruct grep_opt *grep_opt = opt->value;\n@@ -803,12 +831,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstruct grep_opt opt;\n \tstruct object_array list = OBJECT_ARRAY_INIT;\n \tstruct pathspec pathspec;\n+\tstruct argv_array pathspec_argv;\n \tstruct string_list path_list = STRING_LIST_INIT_NODUP;\n \tint i;\n \tint dummy;\n \tint use_index = 1;\n \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n \tint allow_revs;\n+\tint stdin_patterns = 0;\n+\tint stdin_pathspecs = 0;\n \n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"cached\", &cached,\n@@ -896,8 +927,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('W', \"function-context\", &opt.funcbody,\n \t\t\tN_(\"show the surrounding function\")),\n \t\tOPT_GROUP(\"\"),\n-\t\tOPT_CALLBACK('f', NULL, &opt, N_(\"file\"),\n-\t\t\tN_(\"read patterns from file\"), file_callback),\n+\t\tOPT_CALLBACK('f', \"patterns-file\", &opt, N_(\"file\"),\n+\t\t\tN_(\"read patterns from file\"), pattern_file_callback),\n+\t\tOPT_BOOL(0, \"stdin-patterns\", &stdin_patterns,\n+\t\t\t N_(\"read patterns from stdin\")),\n+\t\tOPT_CALLBACK(0, \"pathspecs-file\", &pathspec_argv, N_(\"file\"),\n+\t\t\tN_(\"read pathspecs from file\"), pathspec_file_callback),\n+\t\tOPT_BOOL(0, \"stdin-pathspecs\", &stdin_pathspecs,\n+\t\t\t N_(\"read pathspecs from stdin\")),\n \t\t{ OPTION_CALLBACK, 'e', NULL, &opt, N_(\"pattern\"),\n \t\t\tN_(\"match \"), PARSE_OPT_NONEG, pattern_callback },\n \t\t{ OPTION_CALLBACK, 0, \"and\", &opt, NULL,\n@@ -949,6 +986,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \tgrep_commit_pattern_type(pattern_type_arg, &opt);\n \n+\tif (stdin_patterns && stdin_pathspecs)\n+\t\tdie(_(\"cannot use stdin for both patterns and pathspecs\"));\n+\n+\tif (stdin_patterns)\n+\t\tpattern_file(&opt, \"-\");\n+\n+\tif (stdin_pathspecs)\n+\t\tpathspec_file(&pathspec_argv, \"-\");\n+\n \tif (use_index && !startup_info->have_repository) {\n \t\tint fallback = 0;\n \t\tgit_config_get_bool(\"grep.fallbacktonoindex\", &fallback);\n@@ -1057,7 +1103,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_CWD |\n \t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n-\t\t prefix, argv + i);\n+\t\t prefix,\n+\t\t (pathspec_argv.argc ? pathspec_argv.argv : argv) + i);\n \tpathspec.max_depth = opt.max_depth;\n \tpathspec.recursive = 1;\n \tpathspec.recurse_submodules = !!recurse_submodules;\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 7d7b396c23..be8e60cfa0 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '\n \ttest_must_fail git grep -f patterns\n '\n \n+text_expect_success 'grep --pathspec-file, non-existent file' '\n+\ttest_must_fail git grep --pathspecs-file pathspecs\n+'\n+\n cat >expected <actual &&\n+test_expect_success 'grep --patterns-file, multiple patterns' '\n+\tgit grep --patterns-file patterns >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -621,6 +625,11 @@ test_expect_success 'grep -f, ignore empty lines, read patterns from stdin' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep --stdin--patterns' '\n+\tgit grep --stdin-patterns actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected <excluded-file <pathspec-file <unrelated-file <pathspecs <expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'grep --pathspecs-file with file' '\n+\tgit grep --pathspecs-file pathspecs \"bar\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'grep --pathspec-file with stdin' '\n+\tgit grep --pathspecs-file - \"bar\" actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'setup double-dash tests' '\n cat >double-dash < Teach 'git grep' how to receive pathspecs via file, and add convenience\n> flags for receiving pathspecs or patterns via stdin.\n> \n> In some cases, the list of pathspecs to 'git grep' is longer than the\n> command line allows, or long enough that a user becomes fatigued reading\n> the command. In other cases, a user may wish to write a script or\n> oneliner which generates a list of files to 'git grep' against. In both\n> scenarios, a reasonable workaround is to provide the list of pathspecs\n> via a file or stdin.\n> \n> Before this change, it was already possible to receive patterns via\n> stdin by using the argument '-f -'. Now that there are two arguments\n> which could both use stdin, help safeguard the user from doing so by\n> providing convenience '--stdin-patterns' and '--stdin-pathspecs' which\n> are mutually exclusive. (It is still possible to ask for '-f -\n> --pathspecs-file -', though.)\n> \n> Finally, in order to match '--pathspecs-file', give '-f' a long form\n> '--patterns-file' too.\n> \n> Signed-off-by: Emily Shaffer \n> ---\n> Providing too many pathspecs for the argument list to handle is causing\n> some gore out in the wild with git-secrets:\n> https://github.com/awslabs/git-secrets/issues/78\n\nOut of curiosity, are there any situations that --pathspecs-file handles\nbetter than an xargs invocation? I didn't take a look at the code of\ngit-secrets but I suspect that that could solve their problem.\n\nThe reason I ask is because (correct me if I'm wrong) a lot of other git\ncommands (like add, reset and checkout) don't seem to accept pathspecs\nvia stdin and could suffer the same problem. xargs seems like a more\ngeneral way of solving the problem of long command lines.\n\n> \n> A short arg for --pathspecs-file didn't occur to me because grep has\n> many short args already, but if anyone has a suggestion I'll happily add\n> it.\n> \n> The pathspec list is gathered into an argv_array because parse_pathspec\n> is only called once, against the entire list of pathspecs. Perhaps it\n> makes sense to concatenate the paths given on the command line with the\n> paths given via file, but since it was nontrivial to merge two\n> argv_arrays and I wasn't certain it was the right choice, I didn't do\n> so. I'd probably implement that by adding a utility function to\n> argv-array. ('-f somepatterns.txt -e otherpattern' concatenates the\n> patterns, by the way.)\n> \n> - Emily\n> \n> Documentation/git-grep.txt | 18 ++++++++++-\n> builtin/grep.c | 61 +++++++++++++++++++++++++++++++++-----\n> t/t7810-grep.sh | 52 ++++++++++++++++++++++++++++++--\n> 3 files changed, 121 insertions(+), 10 deletions(-)\n> \n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index c89fb569e3..fb588a3846 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -24,7 +24,8 @@ SYNOPSIS\n> \t [-A ] [-B ] [-C ]\n> \t [-W | --function-context]\n> \t [--threads ]\n> -\t [-f ] [-e] \n> +\t [--pathspecs-file ] [--stdin-pathspecs]\n> +\t [-f | --patterns-file ] [--stdin-patterns] [-e] \n> \t [--and|--or|--not|(|)|-e ...]\n> \t [--recurse-submodules] [--parent-basename ]\n> \t [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n> @@ -270,7 +271,22 @@ providing this option will cause it to die.\n> \tSee `grep.threads` in 'CONFIGURATION' for more information.\n> \n> -f ::\n> +--patterns-file ::\n> \tRead patterns from , one per line.\n> +\n> +--stdin-patterns::\n> +\tRead patterns from stdin, one per line. Equivalent to `-f -`. Cannot be\n> +\tcombined with `--stdin-pathspecs`.\n> +\n> +--pathspecs-file ::\n> +\tRead pathspecs from , one per line. Pathspecs passed in the\n> +\targument list will be ignored in this mode.\n> +\n> +--stdin-pathspecs::\n> +\tRead pathspecs from stdin, one per line. Pathspecs passed in the\n> +\targument list will be ignored in this mode. Equivalent to\n> +\t`--pathspecs-file -`.\n> +\n> +\n> Passing the pattern via allows for providing a search pattern\n> containing a \\0.\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 50ce8d9461..f6f9f84002 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -720,16 +720,38 @@ static int context_callback(const struct option *opt, const char *arg,\n> \treturn 0;\n> }\n> \n> -static int file_callback(const struct option *opt, const char *arg, int unset)\n> +static int pathspec_file(struct argv_array *pathspec_argv, const char *arg)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tif (!strcmp(arg, \"-\")) {\n> +\t\twhile (strbuf_getline(&sb, stdin) != EOF)\n> +\t\t\targv_array_push(pathspec_argv, sb.buf);\n> +\t}\n> +\telse {\n> +\t\tif (!strbuf_read_file(&sb, arg, 0))\n> +\t\t\tdie_errno(_(\"cannot open '%s'\"), arg);\n> +\t\targv_array_split(pathspec_argv, sb.buf);\n> +\t}\n> +\n> +\tstrbuf_release(&sb);\n> +\treturn 0;\n> +}\n> +\n> +static int pathspec_file_callback(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tBUG_ON_OPT_NEG(unset);\n> +\treturn pathspec_file(opt->value, arg);\n> +}\n> +\n> +static int pattern_file(struct grep_opt *grep_opt, const char *arg)\n> {\n> -\tstruct grep_opt *grep_opt = opt->value;\n> \tint from_stdin;\n> \tFILE *patterns;\n> \tint lno = 0;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \n> -\tBUG_ON_OPT_NEG(unset);\n> -\n> \tfrom_stdin = !strcmp(arg, \"-\");\n> \tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n> \tif (!patterns)\n> @@ -748,6 +770,12 @@ static int file_callback(const struct option *opt, const char *arg, int unset)\n> \treturn 0;\n> }\n> \n> +static int pattern_file_callback(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tBUG_ON_OPT_NEG(unset);\n> +\treturn pattern_file(opt->value, arg);\n> +}\n> +\n> static int not_callback(const struct option *opt, const char *arg, int unset)\n> {\n> \tstruct grep_opt *grep_opt = opt->value;\n> @@ -803,12 +831,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tstruct grep_opt opt;\n> \tstruct object_array list = OBJECT_ARRAY_INIT;\n> \tstruct pathspec pathspec;\n> +\tstruct argv_array pathspec_argv;\n> \tstruct string_list path_list = STRING_LIST_INIT_NODUP;\n> \tint i;\n> \tint dummy;\n> \tint use_index = 1;\n> \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n> \tint allow_revs;\n> +\tint stdin_patterns = 0;\n> +\tint stdin_pathspecs = 0;\n> \n> \tstruct option options[] = {\n> \t\tOPT_BOOL(0, \"cached\", &cached,\n> @@ -896,8 +927,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL('W', \"function-context\", &opt.funcbody,\n> \t\t\tN_(\"show the surrounding function\")),\n> \t\tOPT_GROUP(\"\"),\n> -\t\tOPT_CALLBACK('f', NULL, &opt, N_(\"file\"),\n> -\t\t\tN_(\"read patterns from file\"), file_callback),\n> +\t\tOPT_CALLBACK('f', \"patterns-file\", &opt, N_(\"file\"),\n> +\t\t\tN_(\"read patterns from file\"), pattern_file_callback),\n> +\t\tOPT_BOOL(0, \"stdin-patterns\", &stdin_patterns,\n> +\t\t\t N_(\"read patterns from stdin\")),\n> +\t\tOPT_CALLBACK(0, \"pathspecs-file\", &pathspec_argv, N_(\"file\"),\n> +\t\t\tN_(\"read pathspecs from file\"), pathspec_file_callback),\n> +\t\tOPT_BOOL(0, \"stdin-pathspecs\", &stdin_pathspecs,\n> +\t\t\t N_(\"read pathspecs from stdin\")),\n> \t\t{ OPTION_CALLBACK, 'e', NULL, &opt, N_(\"pattern\"),\n> \t\t\tN_(\"match \"), PARSE_OPT_NONEG, pattern_callback },\n> \t\t{ OPTION_CALLBACK, 0, \"and\", &opt, NULL,\n> @@ -949,6 +986,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n> \tgrep_commit_pattern_type(pattern_type_arg, &opt);\n> \n> +\tif (stdin_patterns && stdin_pathspecs)\n> +\t\tdie(_(\"cannot use stdin for both patterns and pathspecs\"));\n> +\n> +\tif (stdin_patterns)\n> +\t\tpattern_file(&opt, \"-\");\n> +\n> +\tif (stdin_pathspecs)\n> +\t\tpathspec_file(&pathspec_argv, \"-\");\n> +\n> \tif (use_index && !startup_info->have_repository) {\n> \t\tint fallback = 0;\n> \t\tgit_config_get_bool(\"grep.fallbacktonoindex\", &fallback);\n> @@ -1057,7 +1103,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tparse_pathspec(&pathspec, 0,\n> \t\t PATHSPEC_PREFER_CWD |\n> \t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n> -\t\t prefix, argv + i);\n> +\t\t prefix,\n> +\t\t (pathspec_argv.argc ? pathspec_argv.argv : argv) + i);\n> \tpathspec.max_depth = opt.max_depth;\n> \tpathspec.recursive = 1;\n> \tpathspec.recurse_submodules = !!recurse_submodules;\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index 7d7b396c23..be8e60cfa0 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '\n> \ttest_must_fail git grep -f patterns\n> '\n> \n> +text_expect_success 'grep --pathspec-file, non-existent file' '\n> +\ttest_must_fail git grep --pathspecs-file pathspecs\n> +'\n> +\n> cat >expected < file:foo mmap bar\n> file:foo_mmap bar\n> @@ -582,8 +586,8 @@ mmap\n> vvv\n> EOF\n> \n> -test_expect_success 'grep -f, multiple patterns' '\n> -\tgit grep -f patterns >actual &&\n> +test_expect_success 'grep --patterns-file, multiple patterns' '\n> +\tgit grep --patterns-file patterns >actual &&\n> \ttest_cmp expected actual\n> '\n> \n> @@ -621,6 +625,11 @@ test_expect_success 'grep -f, ignore empty lines, read patterns from stdin' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'grep --stdin--patterns' '\n> +\tgit grep --stdin-patterns actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> cat >expected < y:y yy\n> --\n> @@ -1125,6 +1134,45 @@ test_expect_success 'grep --no-index descends into repos, but not .git' '\n> \t)\n> '\n> \n> +test_expect_success 'setup pathspecs-file tests' '\n> +cat >excluded-file < +bar\n> +EOF\n> +cat >pathspec-file < +foo\n> +bar\n> +baz\n> +EOF\n> +cat >unrelated-file < +xyz\n> +EOF\n> +git add excluded-file pathspec-file unrelated-file\n> +'\n\nPlease indent these commands in and use `-\\EOF` for the here-doc.\n\n> +\n> +cat >pathspecs < +pathspec-file\n> +unrelated-file\n> +EOF\n> +\n> +cat >expected < +pathspec-file:bar\n> +EOF\n\nIs there any reason why these two aren't part of the setup above?\n\nAlso, even though 'expect' is the conventional name for the expected\noutput file, it seems like the rest of the test uses 'expected' so I\nguess it's fine to leave it as is.\n\nThanks,\n\nDenton\n\n> +\n> +test_expect_success 'grep --stdin-pathspecs' '\n> +\tgit grep --stdin-pathspecs \"bar\" actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'grep --pathspecs-file with file' '\n> +\tgit grep --pathspecs-file pathspecs \"bar\" >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'grep --pathspec-file with stdin' '\n> +\tgit grep --pathspecs-file - \"bar\" actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success 'setup double-dash tests' '\n> cat >double-dash < --\n> -- \n> 2.24.0.432.g9d3f5f5b63-goog\n> \n"},{"id":"386788","messageId":"xmqq1ru08x80.fsf@gitster-ct.c.googlers.com","threadId":"52307","inReplyTo":"20191122011646.218346-1-emilyshaffer@google.com","subject":"Re: [PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T02:24:15Z","receivedAt":"2019-11-22T02:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Teach 'git grep' how to receive pathspecs via file, and add convenience\n> flags for receiving pathspecs or patterns via stdin.\n\nWhen I invented the terms \"pathspec\", \"refspec\", etc. for this\nproject, I meant them to be collective nouns. A pathspec is a set\nof pathspec elements, each of which is usually given as a separate\nargument on the command line. So \"Read pathspec from file\" is good\n(not \"Read pathspecs from file\").\n\nAre you aware of the am/pathspec-from-file topic that has been\ncooking for a while by now?\n\nI do not necessarily agree with the second rationale why many\ncommands may want to learn this option given in that series, but do\nagree with the first one. A lot of times running underlying \"git\"\ncommand multiple times via xargs invocation can mean quite different\nthing from giving a large pathspec to a single invocation of \"git\".\nBut not with \"git grep\" ;-) So I'd say this is a second class\ntarget for the pathspec-from-file theme.\n\nThanks.\n"},{"id":"386789","messageId":"xmqqwobs7i6u.fsf@gitster-ct.c.googlers.com","threadId":"52307","inReplyTo":"20191122021419.GA52557@generichostname","subject":"Re: [PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T02:34:17Z","receivedAt":"2019-11-22T02:34:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> The reason I ask is because (correct me if I'm wrong) a lot of other git\n> commands (like add, reset and checkout) don't seem to accept pathspecs\n> via stdin and could suffer the same problem. xargs seems like a more\n> general way of solving the problem of long command lines.\n\nYou contributors who are potentially throwing your own topics into\nthe cauldron, please be paying a bit more attention to other topics\nalready cooking in the pot. I think am/pathspec-from-file wants to\ngo in the general direction.\n\nThere are things \"xargs\" is sufficient, and there are things that\nabsolutely requires a single invocation of \"git\". \"grep\" is a bit\nof both.\n\n $ git grep -e \"$pattern\" -- A B C\n\n(where A, B and C are hundreds) can be split into three independent\ninvocations of \"git grep\" via \"xargs\", essentially running\n\n $ git grep -e \"$pattern\" -- A\n $ git grep -e \"$pattern\" -- B\n $ git grep -e \"$pattern\" -- C\n\nindependently.\n\nBut\n\n $ git grep -e P1 -e P2 -e P3 -- A\n\n(where each of \"-e Pn\" in reality may be \"-e Pn1 -e Pn2 -e Pn3...\"\nthat has hundreds of patterns) cannot be split into separate\ninvocations and keep the same meaning.\n\n $ git grep -e P1 -- A\n $ git grep -e P2 -- A\n $ git grep -e P3 -- A\n\nmay show the same lines, but (1) lines with both P1 and P2 would be\nshown duplicated, and (2) the order of the output would be different\nfrom a single invocation looking for all patterns at once.\n\nNeedless to say, the ability to combine patterns with --all-match,\n--and, etc., and negate them would mean the list of patterns must be\nsplit (even when it makes sense to do so) at the right places.\n\n $ git grep -e P1 --and -e P2\n\ncannot be split into two or more invocations, for example.\n\n\n\n\n\n\n"},{"id":"386795","messageId":"xmqqa78o7edw.fsf@gitster-ct.c.googlers.com","threadId":"52307","inReplyTo":"xmqqwobs7i6u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T03:56:27Z","receivedAt":"2019-11-22T03:56:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Denton Liu writes:\n>\n>> The reason I ask is because (correct me if I'm wrong) a lot of other git\n>> commands (like add, reset and checkout) don't seem to accept pathspecs\n>> via stdin and could suffer the same problem. xargs seems like a more\n>> general way of solving the problem of long command lines.\n>\n> There are things \"xargs\" is sufficient, and there are things that\n> absolutely requires a single invocation of \"git\". \"grep\" is a bit\n> of both.\n\nSorry for not addressing your sample commands.\n\nThe \"add\", \"reset\", \"checkout\" subcommands are fine examples that\ndriving them with \"xargs\" is 100% sufficient and there is no need\nfor the pathspec-from-file thing; adding it to them would probably\nnot make the system any worse, but it is far far less important than\nsome other \"git\" subcommands for which \"xargs\" does not work as a\nreplacement.\n\nThanks.\n"},{"id":"386834","messageId":"20191122185251.GA58965@generichostname","threadId":"52307","inReplyTo":"xmqqwobs7i6u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-11-22T18:52:51Z","receivedAt":"2019-11-22T18:52:57Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Fri, Nov 22, 2019 at 11:34:17AM +0900, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > The reason I ask is because (correct me if I'm wrong) a lot of other git\n> > commands (like add, reset and checkout) don't seem to accept pathspecs\n> > via stdin and could suffer the same problem. xargs seems like a more\n> > general way of solving the problem of long command lines.\n> \n> You contributors who are potentially throwing your own topics into\n> the cauldron, please be paying a bit more attention to other topics\n> already cooking in the pot. I think am/pathspec-from-file wants to\n> go in the general direction.\n\nInteresting, I never caught this topic when it went over the list. I\nguess I should read your What's Cooking emails more thoroughly instead\nof just scanning for my own contributions.\n\n> \n> There are things \"xargs\" is sufficient, and there are things that\n> absolutely requires a single invocation of \"git\". \"grep\" is a bit\n> of both.\n> \n> $ git grep -e \"$pattern\" -- A B C\n> \n> (where A, B and C are hundreds) can be split into three independent\n> invocations of \"git grep\" via \"xargs\", essentially running\n> \n> $ git grep -e \"$pattern\" -- A\n> $ git grep -e \"$pattern\" -- B\n> $ git grep -e \"$pattern\" -- C\n> \n> independently.\n\nIn the above, I was talking about the new --pathspecs-file option in\nparticular. So it looks like you agree with me that the new option\ndoesn't supercede xargs?\n\n> \n> But\n> \n> $ git grep -e P1 -e P2 -e P3 -- A\n> \n> (where each of \"-e Pn\" in reality may be \"-e Pn1 -e Pn2 -e Pn3...\"\n> that has hundreds of patterns) cannot be split into separate\n> invocations and keep the same meaning.\n> \n> $ git grep -e P1 -- A\n> $ git grep -e P2 -- A\n> $ git grep -e P3 -- A\n> \n> may show the same lines, but (1) lines with both P1 and P2 would be\n> shown duplicated, and (2) the order of the output would be different\n> from a single invocation looking for all patterns at once.\n\nWe already have `-f` to handle this particular case, no?\n\n> \n> Needless to say, the ability to combine patterns with --all-match,\n> --and, etc., and negate them would mean the list of patterns must be\n> split (even when it makes sense to do so) at the right places.\n> \n> $ git grep -e P1 --and -e P2\n> \n> cannot be split into two or more invocations, for example.\n\nAnyway, thanks for going into detail about this. It makes things a lot\nmore clear.\n\n-Denton\n"},{"id":"386871","messageId":"20191122220241.GA56020@google.com","threadId":"52307","inReplyTo":"xmqqwobs7i6u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-11-22T22:02:41Z","receivedAt":"2019-11-22T22:02:51Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Nov 22, 2019 at 11:34:17AM +0900, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > The reason I ask is because (correct me if I'm wrong) a lot of other git\n> > commands (like add, reset and checkout) don't seem to accept pathspecs\n> > via stdin and could suffer the same problem. xargs seems like a more\n> > general way of solving the problem of long command lines.\n> \n> You contributors who are potentially throwing your own topics into\n> the cauldron, please be paying a bit more attention to other topics\n> already cooking in the pot. I think am/pathspec-from-file wants to\n> go in the general direction.\n\nThanks for pointing it out. It is certainly easy to miss the big picture\nwhen you spend your time looking at a bug queue instead of a review\nqueue; we who stand in different places see different things more\nclearly. I appreciate the reminder to look around a little more.\n\nam/pathspec-from-file does solve this issue in the way that we discussed\ninternally, so I'm excited to base a solution for this issue on that\nbranch instead.\n\nIn this situation - where it is not a related fixup for a branch in\nnext, but the topic does rely on that branch - should I send a series\nwhich is based on 'next'? How do I make the dependency clear to you via\nemailed patch?\n\n - Emily\n"},{"id":"386872","messageId":"20191122220648.GA101478@google.com","threadId":"52307","inReplyTo":"20191122220241.GA56020@google.com","subject":"Re: [PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-11-22T22:06:48Z","receivedAt":"2019-11-22T22:06:55Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Nov 22, 2019 at 02:02:41PM -0800, Emily Shaffer wrote:\n> On Fri, Nov 22, 2019 at 11:34:17AM +0900, Junio C Hamano wrote:\n> > Denton Liu writes:\n> > \n> > > The reason I ask is because (correct me if I'm wrong) a lot of other git\n> > > commands (like add, reset and checkout) don't seem to accept pathspecs\n> > > via stdin and could suffer the same problem. xargs seems like a more\n> > > general way of solving the problem of long command lines.\n> > \n> > You contributors who are potentially throwing your own topics into\n> > the cauldron, please be paying a bit more attention to other topics\n> > already cooking in the pot. I think am/pathspec-from-file wants to\n> > go in the general direction.\n> \n> Thanks for pointing it out. It is certainly easy to miss the big picture\n> when you spend your time looking at a bug queue instead of a review\n> queue; we who stand in different places see different things more\n> clearly. I appreciate the reminder to look around a little more.\n> \n> am/pathspec-from-file does solve this issue in the way that we discussed\n> internally, so I'm excited to base a solution for this issue on that\n> branch instead.\n\nIn fact, as I think further, it could be that the solution is \"don't\nchange anything, just use --pathspec-from-file\". But I think my next\nquestion will still be useful to learn the response to anyway. :)\n\n> In this situation - where it is not a related fixup for a branch in\n> next, but the topic does rely on that branch - should I send a series\n> which is based on 'next'? How do I make the dependency clear to you via\n> emailed patch?\n> \n> - Emily\n"},{"id":"386883","messageId":"xmqqsgmf3071.fsf@gitster-ct.c.googlers.com","threadId":"52307","inReplyTo":"20191122220241.GA56020@google.com","subject":"Re: [PATCH] grep: provide pathspecs/patterns via file or stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-23T00:28:50Z","receivedAt":"2019-11-23T00:28:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> In this situation - where it is not a related fixup for a branch in\n> next, but the topic does rely on that branch - should I send a series\n> which is based on 'next'? How do I make the dependency clear to you via\n> emailed patch?\n\nIf you need to use a particular new feature from a topic (or two),\nnot depending on 'next' but either building on\n\n (1) directly that topic, or\n\n (2) a local merge you create across the 'master' branch and the\n topic(s) you want to depend on\n\nwould be good. Whichever way you go, leaving a note either on the\ncover letter or after the three-dash line to say what you did would\nalways be helpful to the reviewers.\n\nIn this particular case, I think (1) would work better for you, as\nthe topic you want to use builds directly on the latest release so\nyou won't be missing anything that is not yet on that topic but\nalready in 'master' at this point.\n\nThanks.\n"},{"id":"387509","messageId":"20191204203911.237056-1-emilyshaffer@google.com","threadId":"52307","inReplyTo":"20191122011646.218346-1-emilyshaffer@google.com","subject":"[PATCH v2] grep: support the --pathspec-from-file option","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-04T20:39:11Z","receivedAt":"2019-12-04T20:39:17Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the\ndocumentation accordingly.\n\nThis changes enables 'git grep' to receive the pathspec from a file by\nspecifying the path, or from stdin by specifying '-' as the path. This\nmatches the previous functionality of '-f', so the documentation of '-f'\nhas been expanded to describe this functionality. To let '-f' match the\nnew '--pathspec-from-file' option, also teach a '--patterns-from-file'\nlong name to '-f'.\n\nSince there are now two arguments which can attempt to read from stdin,\nadd a safeguard to check whether the user specified '-' for both of\nthem. It is still possible for a user to pass '/dev/stdin' to one or\nboth arguments at present; we do not explicitly check.\n\nSigned-off-by: Emily Shaffer \n---\nRefactored to use am/pathspec-from-file. This changes the implementation\nsignificantly since v1, but the testcases mostly remain the same.\n\nThis change builds on top of am/pathspec-from-file and is dependent on\nat least \"parse-options.h: add new options `--pathspec-from-file`,\n`--pathspec-file-nul`\". I followed the example of \"commit: support the\n--pathspec-from-file\" option and retained the tests from v1 of this\ntopic.\n\n Documentation/git-grep.txt | 22 ++++++++++++++++--\n builtin/grep.c | 35 ++++++++++++++++++++++++-----\n t/t7810-grep.sh | 46 ++++++++++++++++++++++++++++++++++++--\n 3 files changed, 94 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex c89fb569e3..56b1c5a302 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -24,7 +24,8 @@ SYNOPSIS\n \t [-A ] [-B ] [-C ]\n \t [-W | --function-context]\n \t [--threads ]\n-\t [-f ] [-e] \n+\t [-f | --patterns-from-file ] [-e] \n+\t [--pathspec-from-file= [--pathspec-file-nul]]\n \t [--and|--or|--not|(|)|-e ...]\n \t [--recurse-submodules] [--parent-basename ]\n \t [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n@@ -270,7 +271,10 @@ providing this option will cause it to die.\n \tSee `grep.threads` in 'CONFIGURATION' for more information.\n \n -f ::\n-\tRead patterns from , one per line.\n+--patterns-from-file ::\n+\tRead patterns from , one per line. If `` is exactly `-` then\n+\tstandard input is used; standard input cannot be used for both\n+\t--patterns-from-file and --pathspec-from-file.\n +\n Passing the pattern via allows for providing a search pattern\n containing a \\0.\n@@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \\0 for\n more search backends, until then we'll die when the pattern type in\n question doesn't support them.\n \n+--pathspec-from-file ::\n+\tRead pathspec from instead of the command line. If `` is\n+\texactly `-` then standard input is used; standard input cannot be used\n+\tfor both --patterns-from-file and --pathspec-from-file. Pathspec elements\n+\tare separated by LF or CR/LF. Pathspec elements can be quoted as\n+\texplained for the configuration variable `core.quotePath` (see\n+\tlinkgit:git-config[1]). See also `--pathspec-file-nul` and global\n+\t`--literal-pathspecs`.\n+\n+--pathspec-file-nul::\n+\tOnly meaningful with `--pathspec-from-file`. Pathspec elements are\n+\tseparated with NUL character and all other characters are taken\n+\tliterally (including newlines and quotes).\n+\n -e::\n \tThe next parameter is the pattern. This option has to be\n \tused for patterns starting with `-` and should be used in\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 50ce8d9461..54ba991c42 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -31,6 +31,7 @@ static char const * const grep_usage[] = {\n };\n \n static int recurse_submodules;\n+static int patterns_from_stdin, pathspec_from_stdin;\n \n #define GREP_NUM_THREADS_DEFAULT 8\n static int num_threads;\n@@ -723,15 +724,18 @@ static int context_callback(const struct option *opt, const char *arg,\n static int file_callback(const struct option *opt, const char *arg, int unset)\n {\n \tstruct grep_opt *grep_opt = opt->value;\n-\tint from_stdin;\n \tFILE *patterns;\n \tint lno = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tfrom_stdin = !strcmp(arg, \"-\");\n-\tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n+\tpatterns_from_stdin = !strcmp(arg, \"-\");\n+\n+\tif (patterns_from_stdin && pathspec_from_stdin)\n+\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n+\n+\tpatterns = patterns_from_stdin ? stdin : fopen(arg, \"r\");\n \tif (!patterns)\n \t\tdie_errno(_(\"cannot open '%s'\"), arg);\n \twhile (strbuf_getline(&sb, patterns) == 0) {\n@@ -742,7 +746,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset)\n \t\tappend_grep_pat(grep_opt, sb.buf, sb.len, arg, ++lno,\n \t\t\t\tGREP_PATTERN);\n \t}\n-\tif (!from_stdin)\n+\tif (!patterns_from_stdin)\n \t\tfclose(patterns);\n \tstrbuf_release(&sb);\n \treturn 0;\n@@ -809,6 +813,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tint use_index = 1;\n \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n \tint allow_revs;\n+\tchar *pathspec_from_file;\n+\tint pathspec_file_nul;\n \n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"cached\", &cached,\n@@ -896,8 +902,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('W', \"function-context\", &opt.funcbody,\n \t\t\tN_(\"show the surrounding function\")),\n \t\tOPT_GROUP(\"\"),\n-\t\tOPT_CALLBACK('f', NULL, &opt, N_(\"file\"),\n+\t\tOPT_CALLBACK('f', \"patterns-from-file\", &opt, N_(\"file\"),\n \t\t\tN_(\"read patterns from file\"), file_callback),\n+\t\tOPT_PATHSPEC_FROM_FILE(&pathspec_from_file),\n+\t\tOPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),\n \t\t{ OPTION_CALLBACK, 'e', NULL, &opt, N_(\"pattern\"),\n \t\t\tN_(\"match \"), PARSE_OPT_NONEG, pattern_callback },\n \t\t{ OPTION_CALLBACK, 0, \"and\", &opt, NULL,\n@@ -1062,6 +1070,23 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpathspec.recursive = 1;\n \tpathspec.recurse_submodules = !!recurse_submodules;\n \n+\tif (pathspec_from_file) {\n+\t\tif (pathspec.nr)\n+\t\t\tdie(_(\"--pathspec-from-file is incompatible with pathspec arguments\"));\n+\n+\t\tpathspec_from_stdin = !strcmp(pathspec_from_file, \"-\");\n+\n+\t\tif (patterns_from_stdin && pathspec_from_stdin)\n+\t\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n+\n+\t\tparse_pathspec_file(&pathspec, 0, PATHSPEC_PREFER_CWD |\n+\t\t\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n+\t\t\t\t prefix, pathspec_from_file,\n+\t\t\t\t pathspec_file_nul);\n+\t} else if (pathspec_file_nul) {\n+\t\tdie(_(\"--pathspec-file-nul requires --pathspec-from-file\"));\n+\t}\n+\n \tif (list.nr || cached || show_in_pager) {\n \t\tif (num_threads > 1)\n \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 7d7b396c23..355890a72a 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '\n \ttest_must_fail git grep -f patterns\n '\n \n+text_expect_success 'grep --pathspec-from-file, non-existent file' '\n+\ttest_must_fail git grep --pathspec-from-file pathspecs\n+'\n+\n cat >expected <actual &&\n+test_expect_success 'grep --patterns-from-file, multiple patterns' '\n+\tgit grep --patterns-from-file patterns >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1125,6 +1129,44 @@ test_expect_success 'grep --no-index descends into repos, but not .git' '\n \t)\n '\n \n+test_expect_success 'setup pathspecs-file tests' '\n+cat >excluded-file <pathspec-file <unrelated-file <pathspecs <expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'grep --pathspec-file with stdin' '\n+\tgit grep --pathspec-from-file - \"bar\" actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'grep with two stdin inputs fails' '\n+\ttest_must_fail git grep --pathspec-from-file - --patterns-from-file - double-dash < @@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \\0 for\n> more search backends, until then we'll die when the pattern type in\n> question doesn't support them.\n> \n> +--pathspec-from-file ::\n> +\tRead pathspec from instead of the command line. If `` is\n> +\texactly `-` then standard input is used; standard input cannot be used\n> +\tfor both --patterns-from-file and --pathspec-from-file. Pathspec elements\n> +\tare separated by LF or CR/LF. Pathspec elements can be quoted as\n> +\texplained for the configuration variable `core.quotePath` (see\n> +\tlinkgit:git-config[1]). See also `--pathspec-file-nul` and global\n> +\t`--literal-pathspecs`.\n> +\n> +--pathspec-file-nul::\n> +\tOnly meaningful with `--pathspec-from-file`. Pathspec elements are\n> +\tseparated with NUL character and all other characters are taken\n> +\tliterally (including newlines and quotes).\n\nDoes it make sense to have a corresponding --patterns-file-nul option?\nAs in, is it possible for patterns to contain inline newlines? If it's\nnot possible, then that option probably isn't necessary.\n\n> +\n> -e::\n> \tThe next parameter is the pattern. This option has to be\n> \tused for patterns starting with `-` and should be used in\n\n> @@ -1125,6 +1129,44 @@ test_expect_success 'grep --no-index descends into repos, but not .git' '\n> \t)\n> '\n> \n> +test_expect_success 'setup pathspecs-file tests' '\n> +cat >excluded-file < +bar\n> +EOF\n> +cat >pathspec-file < +foo\n> +bar\n> +baz\n> +EOF\n> +cat >unrelated-file < +xyz\n> +EOF\n> +git add excluded-file pathspec-file unrelated-file\n> +'\n\nCould you please change these here-docs to be <<-\\EOF and then indent\nthe test case?\n\n> +\n> +cat >pathspecs < +pathspec-file\n> +unrelated-file\n> +EOF\n> +\n> +cat >expected < +pathspec-file:bar\n> +EOF\n\nIn an earlier email, I was wondering aloud why these two blocks were\noutside of the test case above. I think the answer to that is that\nyou're following the existing style of the test case.\n\nIn that case, could I pester you to do some test clean up while you're\nat it? I think it'd be nice to move the cats into their respective test\ncases (with <<-\\EOF) and also rename the files from 'expected' to\n'expect'. But otherwise, I think it's fine as is as well.\n\nThanks,\n\nDenton\n\n> +\n> +test_expect_success 'grep --pathspec-from-file with file' '\n> +\tgit grep --pathspec-from-file pathspecs \"bar\" >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'grep --pathspec-file with stdin' '\n> +\tgit grep --pathspec-from-file - \"bar\" actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'grep with two stdin inputs fails' '\n> +\ttest_must_fail git grep --pathspec-from-file - --patterns-from-file - +'\n> +\n> test_expect_success 'setup double-dash tests' '\n> cat >double-dash < --\n> -- \n> 2.24.0.393.g34dc348eaf-goog\n> \n"},{"id":"387515","messageId":"xmqqy2vr3hso.fsf@gitster-ct.c.googlers.com","threadId":"52307","inReplyTo":"20191204210514.GA89300@generichostname","subject":"Re: [PATCH v2] grep: support the --pathspec-from-file option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-04T21:24:55Z","receivedAt":"2019-12-04T21:25:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n>> +--pathspec-file-nul::\n>> +\tOnly meaningful with `--pathspec-from-file`. Pathspec elements are\n>> +\tseparated with NUL character and all other characters are taken\n>> +\tliterally (including newlines and quotes).\n>\n> Does it make sense to have a corresponding --patterns-file-nul option?\n\nI do not think so. \"grep\" is always record oriented and the record\nseparter is the LF, so patterns file can safely be delimited with\nLF.\n\n>> +test_expect_success 'setup pathspecs-file tests' '\n>> +cat >excluded-file <> +bar\n>> +EOF\n>> ...\n>> +git add excluded-file pathspec-file unrelated-file\n>> +'\n>\n> Could you please change these here-docs to be <<-\\EOF and then indent\n> the test case?\n\nGood suggestion.\n\n\ttest_expect_success 'setup ...' '\n\t\tcat >excluded-file <<-\\EOF &&\n\t\tbar\n\t\tEOF\n\t\t...\n\t\tgit add ...\n\t'\n\nIf each line in these files consists of a short single token (which\nseems to be the case), perhaps consider using test_write_lines?\n\n\ttest_write_lines >excluded-file bar &&\n\ttest_write_lines >pathspec-file foo bar baz &&\n\ttest_write_lines >unrelated-file xyz &&\n\ttest_write_lines >pathspecs pathspec-file unrelated-file &&\n"},{"id":"387529","messageId":"xmqq5ziv3f20.fsf@gitster-ct.c.googlers.com","threadId":"52307","inReplyTo":"20191204203911.237056-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] grep: support the --pathspec-from-file option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-04T22:24:07Z","receivedAt":"2019-12-04T22:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the\n> documentation accordingly.\n>\n> This changes enables 'git grep' to receive the pathspec from a file by\n> specifying the path, or from stdin by specifying '-' as the path. This\n> matches the previous functionality of '-f', so the documentation of '-f'\n> has been expanded to describe this functionality. To let '-f' match the\n> new '--pathspec-from-file' option, also teach a '--patterns-from-file'\n> long name to '-f'.\n>\n> Since there are now two arguments which can attempt to read from stdin,\n> add a safeguard to check whether the user specified '-' for both of\n> them. It is still possible for a user to pass '/dev/stdin' to one or\n> both arguments at present; we do not explicitly check.\n\nOK. I guess this is good enough at least for now and possibly\nforever as that falls into the \"doctor, it hurts when I do this\"\ncategory.\n\n> Refactored to use am/pathspec-from-file. This changes the implementation\n> significantly since v1, but the testcases mostly remain the same.\n\nI am hoping that this topic, and Alexandr's \"add\" and \"checkout\"\nstuff, would help establishing a good pattern for other commands to\nfollow.\n\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index c89fb569e3..56b1c5a302 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -24,7 +24,8 @@ SYNOPSIS\n> \t [-A ] [-B ] [-C ]\n> \t [-W | --function-context]\n> \t [--threads ]\n> -\t [-f ] [-e] \n> +\t [-f | --patterns-from-file ] [-e] \n\nOK.\n\n> +\t [--pathspec-from-file= [--pathspec-file-nul]]\n\nOK.\n\n> @@ -270,7 +271,10 @@ providing this option will cause it to die.\n> \tSee `grep.threads` in 'CONFIGURATION' for more information.\n> \n> -f ::\n> -\tRead patterns from , one per line.\n> +--patterns-from-file ::\n> +\tRead patterns from , one per line. If `` is exactly `-` then\n> +\tstandard input is used; standard input cannot be used for both\n> +\t--patterns-from-file and --pathspec-from-file.\n\nMakes sense---otherwise they would compete and we cannot know which\nkind each line in the standard input is ;-)\n\n> @@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \\0 for\n> more search backends, until then we'll die when the pattern type in\n> question doesn't support them.\n> \n> +--pathspec-from-file ::\n> +\tRead pathspec from instead of the command line. If `` is\n> +\texactly `-` then standard input is used; standard input cannot be used\n> +\tfor both --patterns-from-file and --pathspec-from-file. Pathspec elements\n> +\tare separated by LF or CR/LF. Pathspec elements can be quoted as\n> +\texplained for the configuration variable `core.quotePath` (see\n> +\tlinkgit:git-config[1]). See also `--pathspec-file-nul` and global\n> +\t`--literal-pathspecs`.\n> +\n> +--pathspec-file-nul::\n> +\tOnly meaningful with `--pathspec-from-file`. Pathspec elements are\n> +\tseparated with NUL character and all other characters are taken\n> +\tliterally (including newlines and quotes).\n> +\n\nI think these matches the ones in git-reset.txt from the earlier\nwork by Alexandr's, which is good.\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 50ce8d9461..54ba991c42 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -31,6 +31,7 @@ static char const * const grep_usage[] = {\n> };\n> \n> static int recurse_submodules;\n> +static int patterns_from_stdin, pathspec_from_stdin;\n> \n> #define GREP_NUM_THREADS_DEFAULT 8\n> static int num_threads;\n> @@ -723,15 +724,18 @@ static int context_callback(const struct option *opt, const char *arg,\n> static int file_callback(const struct option *opt, const char *arg, int unset)\n> {\n\nShouldn't this be renamed? Earlier, the only thing that can be\ntaken from a file was the patterns, but now a file can be given\nto specify a set of pathspec elements. \"patterns_file_callback()\"\nor something like taht?\n\n> \tstruct grep_opt *grep_opt = opt->value;\n> -\tint from_stdin;\n> \tFILE *patterns;\n> \tint lno = 0;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tfrom_stdin = !strcmp(arg, \"-\");\n> -\tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n> +\tpatterns_from_stdin = !strcmp(arg, \"-\");\n\nTotally outside the scope of this patch, but we may want to\nintroduce\n\n\tint is_stdin_cmdline_marker(const char *arg)\n\t{\n\t\treturn !strcmp(arg, \"-\");\n\t}\n\nwhich later can be taught also about \"/dev/stdin\". Even outside the\npathspec-from-file option, I think \"diff --no-index\" also has some\nspecial-case provision for treating \"-\" as \"input coming from the\nstandard input string\", which would benefit from such a helper.\n\n> +\tif (patterns_from_stdin && pathspec_from_stdin)\n> +\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n\nIt's kind of sad that we need to check this in this callback and\nalso inside cmd_grep() top-level we will see further down...\n\n> +\tif (pathspec_from_file) {\n> +\t\tif (pathspec.nr)\n> +\t\t\tdie(_(\"--pathspec-from-file is incompatible with pathspec arguments\"));\n> +\n> +\t\tpathspec_from_stdin = !strcmp(pathspec_from_file, \"-\");\n> +\n> +\t\tif (patterns_from_stdin && pathspec_from_stdin)\n> +\t\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n\n... here.\n\nThanks.\n"},{"id":"387552","messageId":"6af006eb-6d67-4eb5-c4f5-1677d6cbd120@syntevo.com","threadId":"52307","inReplyTo":"20191204203911.237056-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] grep: support the --pathspec-from-file option","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-12-05T11:58:19Z","receivedAt":"2019-12-05T11:58:34Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"I'm excited to see someone else join my effort, thanks for continuing my \neffort! Also, less work for me :)\n\nOn 04.12.2019 21:39, Emily Shaffer wrote:\n\n> static int file_callback(const struct option *opt, const char *arg, int unset)\n> {\n> \tstruct grep_opt *grep_opt = opt->value;\n> -\tint from_stdin;\n> \tFILE *patterns;\n> \tint lno = 0;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tfrom_stdin = !strcmp(arg, \"-\");\n> -\tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n> +\tpatterns_from_stdin = !strcmp(arg, \"-\");\n> +\n> +\tif (patterns_from_stdin && pathspec_from_stdin)\n\nTo my understanding, this check will not work as expected. \n`file_callback` will be called at the moment of parsing args. \n`pathspec_from_stdin` is only initialized later.\n\nMaybe it would be better to convert `file_callback` into a regular \nfunction and call it after the options were parsed, similar to how \n`pathspec_from_file` is parsed later?\n\nThis will also allow to move global variables into local scope and \nresolve other small issues raised by other reviewers.\n\n> +test_expect_success 'grep with two stdin inputs fails' '\n> +\ttest_must_fail git grep --pathspec-from-file - --patterns-from-file - +'\n> +\n\nIt is usually a good idea to test for specific error, like this:\n\n test_must_fail git grep --pathspec-from-file - --patterns-from-file - \nerr &&\n test_i18ngrep \"cannot specify both patterns and pathspec via stdin\" \nerr &&\n"},{"id":"387601","messageId":"nycvar.QRO.7.76.6.1912061220400.31080@tvgsbejvaqbjf.bet","threadId":"52307","inReplyTo":"20191204203911.237056-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] grep: support the --pathspec-from-file option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-06T11:22:41Z","receivedAt":"2019-12-06T11:23:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Emily,\n\nOn Wed, 4 Dec 2019, Emily Shaffer wrote:\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 50ce8d9461..54ba991c42 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -809,6 +813,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tint use_index = 1;\n> \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n> \tint allow_revs;\n> +\tchar *pathspec_from_file;\n> +\tint pathspec_file_nul;\n>\n> \tstruct option options[] = {\n\nI need this on top to make it work without problems (I have not yet\ntriggered the CI build, but I will in a moment):\n\n-- snipsnap --\nSubject: [PATCH] fixup??? grep: support the --pathspec-from-file option\n\nWe must not use those variables uninitialized, this causes CI build\nfailures left and right (see e.g.\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=22821)\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 54ba991c425..c2aa1aeebd6 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -813,8 +813,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tint use_index = 1;\n \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n \tint allow_revs;\n-\tchar *pathspec_from_file;\n-\tint pathspec_file_nul;\n+\tchar *pathspec_from_file = NULL;\n+\tint pathspec_file_nul = 0;\n\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"cached\", &cached,\n--\n2.24.0.windows.2.611.ge9aced84530\n\n"},{"id":"387602","messageId":"20191206113401.GA2746@szeder.dev","threadId":"52307","inReplyTo":"20191204203911.237056-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] grep: support the --pathspec-from-file option","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-06T11:34:01Z","receivedAt":"2019-12-06T11:34:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Dec 04, 2019 at 12:39:11PM -0800, Emily Shaffer wrote:\n> Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the\n> documentation accordingly.\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 50ce8d9461..54ba991c42 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n\n> @@ -809,6 +813,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tint use_index = 1;\n> \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n> \tint allow_revs;\n> +\tchar *pathspec_from_file;\n> +\tint pathspec_file_nul;\n\nUninitialized variables ...\n> \n> \tstruct option options[] = {\n> \t\tOPT_BOOL(0, \"cached\", &cached,\n> @@ -896,8 +902,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL('W', \"function-context\", &opt.funcbody,\n> \t\t\tN_(\"show the surrounding function\")),\n> \t\tOPT_GROUP(\"\"),\n> -\t\tOPT_CALLBACK('f', NULL, &opt, N_(\"file\"),\n> +\t\tOPT_CALLBACK('f', \"patterns-from-file\", &opt, N_(\"file\"),\n> \t\t\tN_(\"read patterns from file\"), file_callback),\n> +\t\tOPT_PATHSPEC_FROM_FILE(&pathspec_from_file),\n> +\t\tOPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),\n> \t\t{ OPTION_CALLBACK, 'e', NULL, &opt, N_(\"pattern\"),\n> \t\t\tN_(\"match \"), PARSE_OPT_NONEG, pattern_callback },\n> \t\t{ OPTION_CALLBACK, 0, \"and\", &opt, NULL,\n> @@ -1062,6 +1070,23 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tpathspec.recursive = 1;\n> \tpathspec.recurse_submodules = !!recurse_submodules;\n> \n> +\tif (pathspec_from_file) {\n\n... one of which is checked here ...\n\n> +\t\tif (pathspec.nr)\n> +\t\t\tdie(_(\"--pathspec-from-file is incompatible with pathspec arguments\"));\n\n... causing a lot of tests to fail in our OSX CI builds (but,\nstrangely, in none of the Linux builds), either with the above error\nmessage, e.g.:\n\n expecting success of 4014.150 'format-patch --pretty=mboxrd':\n [...]\n ++git format-patch --pretty=mboxrd --stdout -1 01b96447182ff76f30268fb82d3f9e9e09b14e6c~1..01b96447182ff76f30268fb82d3f9e9e09b14e6c\n ++git grep -h --no-index -A11 '^>From could trip up a loose mbox parser' patch\n fatal: --pathspec-from-file is incompatible with pathspec arguments\n error: last command exited with $?=128\n\nor with:\n\n expecting success of 7814.2 'grep correctly finds patterns in a submodule': \n [...]\n ++git grep -e '(3|4)' --recurse-submodules\n fatal: could not open '����?' for reading: No such file or directory\n error: last command exited with $?=128\n\n> +\t\tpathspec_from_stdin = !strcmp(pathspec_from_file, \"-\");\n> +\n> +\t\tif (patterns_from_stdin && pathspec_from_stdin)\n> +\t\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n> +\n> +\t\tparse_pathspec_file(&pathspec, 0, PATHSPEC_PREFER_CWD |\n> +\t\t\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n> +\t\t\t\t prefix, pathspec_from_file,\n> +\t\t\t\t pathspec_file_nul);\n\nThe other uninitialized variable is passed as parameter above and\nchecked below, but I haven't seen any test failures caused by it.\n\n> +\t} else if (pathspec_file_nul) {\n> +\t\tdie(_(\"--pathspec-file-nul requires --pathspec-from-file\"));\n> +\t}\n> +\n> \tif (list.nr || cached || show_in_pager) {\n> \t\tif (num_threads > 1)\n> \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n"},{"id":"388074","messageId":"20191213030710.GA135450@google.com","threadId":"52307","inReplyTo":"xmqq5ziv3f20.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] grep: support the --pathspec-from-file option","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-13T03:07:10Z","receivedAt":"2019-12-13T03:07:24Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Dec 04, 2019 at 02:24:07PM -0800, Junio C Hamano wrote:\n> Emily Shaffer writes:\n> \n> > Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the\n> > documentation accordingly.\n> >\n> > This changes enables 'git grep' to receive the pathspec from a file by\n> > specifying the path, or from stdin by specifying '-' as the path. This\n> > matches the previous functionality of '-f', so the documentation of '-f'\n> > has been expanded to describe this functionality. To let '-f' match the\n> > new '--pathspec-from-file' option, also teach a '--patterns-from-file'\n> > long name to '-f'.\n> >\n> > Since there are now two arguments which can attempt to read from stdin,\n> > add a safeguard to check whether the user specified '-' for both of\n> > them. It is still possible for a user to pass '/dev/stdin' to one or\n> > both arguments at present; we do not explicitly check.\n> \n> OK. I guess this is good enough at least for now and possibly\n> forever as that falls into the \"doctor, it hurts when I do this\"\n> category.\n> \n> > Refactored to use am/pathspec-from-file. This changes the implementation\n> > significantly since v1, but the testcases mostly remain the same.\n> \n> I am hoping that this topic, and Alexandr's \"add\" and \"checkout\"\n> stuff, would help establishing a good pattern for other commands to\n> follow.\n> \n> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> > index c89fb569e3..56b1c5a302 100644\n> > --- a/Documentation/git-grep.txt\n> > +++ b/Documentation/git-grep.txt\n> > @@ -24,7 +24,8 @@ SYNOPSIS\n> > \t [-A ] [-B ] [-C ]\n> > \t [-W | --function-context]\n> > \t [--threads ]\n> > -\t [-f ] [-e] \n> > +\t [-f | --patterns-from-file ] [-e] \n> \n> OK.\n> \n> > +\t [--pathspec-from-file= [--pathspec-file-nul]]\n> \n> OK.\n> \n> > @@ -270,7 +271,10 @@ providing this option will cause it to die.\n> > \tSee `grep.threads` in 'CONFIGURATION' for more information.\n> > \n> > -f ::\n> > -\tRead patterns from , one per line.\n> > +--patterns-from-file ::\n> > +\tRead patterns from , one per line. If `` is exactly `-` then\n> > +\tstandard input is used; standard input cannot be used for both\n> > +\t--patterns-from-file and --pathspec-from-file.\n> \n> Makes sense---otherwise they would compete and we cannot know which\n> kind each line in the standard input is ;-)\n\nAt one point Jonathan Nieder had suggested to me that it might be nice\nto watch for \"--\" in the standard input. But I think I wasn't confident\nin the idea that all commands which take arbitrary number of arguments,\nand pathspec expect their arg list to end in -- .\n\n> \n> > @@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \\0 for\n> > more search backends, until then we'll die when the pattern type in\n> > question doesn't support them.\n> > \n> > +--pathspec-from-file ::\n> > +\tRead pathspec from instead of the command line. If `` is\n> > +\texactly `-` then standard input is used; standard input cannot be used\n> > +\tfor both --patterns-from-file and --pathspec-from-file. Pathspec elements\n> > +\tare separated by LF or CR/LF. Pathspec elements can be quoted as\n> > +\texplained for the configuration variable `core.quotePath` (see\n> > +\tlinkgit:git-config[1]). See also `--pathspec-file-nul` and global\n> > +\t`--literal-pathspecs`.\n> > +\n> > +--pathspec-file-nul::\n> > +\tOnly meaningful with `--pathspec-from-file`. Pathspec elements are\n> > +\tseparated with NUL character and all other characters are taken\n> > +\tliterally (including newlines and quotes).\n> > +\n> \n> I think these matches the ones in git-reset.txt from the earlier\n> work by Alexandr's, which is good.\n\nYes, I took them verbatim.\n\n> \n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index 50ce8d9461..54ba991c42 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -31,6 +31,7 @@ static char const * const grep_usage[] = {\n> > };\n> > \n> > static int recurse_submodules;\n> > +static int patterns_from_stdin, pathspec_from_stdin;\n> > \n> > #define GREP_NUM_THREADS_DEFAULT 8\n> > static int num_threads;\n> > @@ -723,15 +724,18 @@ static int context_callback(const struct option *opt, const char *arg,\n> > static int file_callback(const struct option *opt, const char *arg, int unset)\n> > {\n> \n> Shouldn't this be renamed? Earlier, the only thing that can be\n> taken from a file was the patterns, but now a file can be given\n> to specify a set of pathspec elements. \"patterns_file_callback()\"\n> or something like taht?\n\nDone. I was hesitant to touch the other code; I guess it is fine.\n\n> \n> > \tstruct grep_opt *grep_opt = opt->value;\n> > -\tint from_stdin;\n> > \tFILE *patterns;\n> > \tint lno = 0;\n> > \tstruct strbuf sb = STRBUF_INIT;\n> > \n> > \tBUG_ON_OPT_NEG(unset);\n> > \n> > -\tfrom_stdin = !strcmp(arg, \"-\");\n> > -\tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n> > +\tpatterns_from_stdin = !strcmp(arg, \"-\");\n> \n> Totally outside the scope of this patch, but we may want to\n> introduce\n> \n> \tint is_stdin_cmdline_marker(const char *arg)\n> \t{\n> \t\treturn !strcmp(arg, \"-\");\n> \t}\n> \n> which later can be taught also about \"/dev/stdin\". Even outside the\n> pathspec-from-file option, I think \"diff --no-index\" also has some\n> special-case provision for treating \"-\" as \"input coming from the\n> standard input string\", which would benefit from such a helper.\n\nAgreed; that's probably a handy thing to put into parse-options.h. I\nsuggest we think of this next time we are looking for Outreachy\nmicroprojects or similar. I'll write it down somewhere.\n\n> \n> > +\tif (patterns_from_stdin && pathspec_from_stdin)\n> > +\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n> \n> It's kind of sad that we need to check this in this callback and\n> also inside cmd_grep() top-level we will see further down...\n\nAnother reviewer suggested not using a callback and instead waiting\nuntil after the entire argparse to do file reads; this would let us\navoid the weird double check here. I think I will do it.\n\n> \n> > +\tif (pathspec_from_file) {\n> > +\t\tif (pathspec.nr)\n> > +\t\t\tdie(_(\"--pathspec-from-file is incompatible with pathspec arguments\"));\n> > +\n> > +\t\tpathspec_from_stdin = !strcmp(pathspec_from_file, \"-\");\n> > +\n> > +\t\tif (patterns_from_stdin && pathspec_from_stdin)\n> > +\t\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n> \n> ... here.\n> \n> Thanks.\n"},{"id":"388075","messageId":"20191213040042.GB135450@google.com","threadId":"52307","inReplyTo":"6af006eb-6d67-4eb5-c4f5-1677d6cbd120@syntevo.com","subject":"Re: [PATCH v2] grep: support the --pathspec-from-file option","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-13T04:00:42Z","receivedAt":"2019-12-13T04:01:05Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Dec 05, 2019 at 12:58:19PM +0100, Alexandr Miloslavskiy wrote:\n> I'm excited to see someone else join my effort, thanks for continuing my\n> effort! Also, less work for me :)\n> \n> On 04.12.2019 21:39, Emily Shaffer wrote:\n> \n> > static int file_callback(const struct option *opt, const char *arg, int unset)\n> > {\n> > \tstruct grep_opt *grep_opt = opt->value;\n> > -\tint from_stdin;\n> > \tFILE *patterns;\n> > \tint lno = 0;\n> > \tstruct strbuf sb = STRBUF_INIT;\n> > \tBUG_ON_OPT_NEG(unset);\n> > -\tfrom_stdin = !strcmp(arg, \"-\");\n> > -\tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n> > +\tpatterns_from_stdin = !strcmp(arg, \"-\");\n> > +\n> > +\tif (patterns_from_stdin && pathspec_from_stdin)\n> \n> To my understanding, this check will not work as expected. `file_callback`\n> will be called at the moment of parsing args. `pathspec_from_stdin` is only\n> initialized later.\n> \n> Maybe it would be better to convert `file_callback` into a regular function\n> and call it after the options were parsed, similar to how\n> `pathspec_from_file` is parsed later?\n> \n> This will also allow to move global variables into local scope and resolve\n> other small issues raised by other reviewers.\n\nYeah, I think this is a good idea for the reasons you stated, so I'll do\nso.\n\n> \n> > +test_expect_success 'grep with two stdin inputs fails' '\n> > +\ttest_must_fail git grep --pathspec-from-file - --patterns-from-file - > +'\n> > +\n> \n> It is usually a good idea to test for specific error, like this:\n> \n> test_must_fail git grep --pathspec-from-file - --patterns-from-file -\n> err &&\n> test_i18ngrep \"cannot specify both patterns and pathspec via stdin\" err &&\n\nSure. Thanks.\n"},{"id":"388076","messageId":"20191213041254.13202-1-emilyshaffer@google.com","threadId":"52307","inReplyTo":"20191204203911.237056-1-emilyshaffer@google.com","subject":"[PATCH v3] grep: support the --pathspec-from-file option","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-13T04:12:54Z","receivedAt":"2019-12-13T04:13:04Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the\ndocumentation accordingly.\n\nThis changes enables 'git grep' to receive the pathspec from a file by\nspecifying the path, or from stdin by specifying '-' as the path. This\nmatches the previous functionality of '-f', so the documentation of '-f'\nhas been expanded to describe this functionality. To let '-f' match the\nnew '--pathspec-from-file' option, also teach a '--patterns-from-file'\nlong name to '-f'.\n\nSince there are now two arguments which can attempt to read from stdin,\nadd a safeguard to check whether the user specified '-' for both of\nthem. It is still possible for a user to pass '/dev/stdin' to one or\nboth arguments at present; we do not explicitly check.\n\nSigned-off-by: Emily Shaffer \n---\nI built this on 'next' locally, but it does require 'am/pathspec-from-file'\n(which was merged to 'next').\n\nThanks all who caught the OSX/GfW CI failures; the test suite passed for me\nin Linux and so I completely missed the uninitialized variables. Not a\ngreat look for me...\n\nSince v2, I got rid of the callback for -f arg and am now running it\ndirectly; this means we can check for stdin in only one place, instead\nof two, and get rid of some of the globals which were added elsewhere.\nThe test suite still passes for me (but still on Linux) so here's hoping\nfor a more favorable result with the rest of the CI.\n\n - Emily\n\n Documentation/git-grep.txt | 22 ++++++++++++++++--\n builtin/grep.c | 43 ++++++++++++++++++++++++++--------\n t/t7810-grep.sh | 47 ++++++++++++++++++++++++++++++++++++--\n 3 files changed, 98 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex c89fb569e3..56b1c5a302 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -24,7 +24,8 @@ SYNOPSIS\n \t [-A ] [-B ] [-C ]\n \t [-W | --function-context]\n \t [--threads ]\n-\t [-f ] [-e] \n+\t [-f | --patterns-from-file ] [-e] \n+\t [--pathspec-from-file= [--pathspec-file-nul]]\n \t [--and|--or|--not|(|)|-e ...]\n \t [--recurse-submodules] [--parent-basename ]\n \t [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n@@ -270,7 +271,10 @@ providing this option will cause it to die.\n \tSee `grep.threads` in 'CONFIGURATION' for more information.\n \n -f ::\n-\tRead patterns from , one per line.\n+--patterns-from-file ::\n+\tRead patterns from , one per line. If `` is exactly `-` then\n+\tstandard input is used; standard input cannot be used for both\n+\t--patterns-from-file and --pathspec-from-file.\n +\n Passing the pattern via allows for providing a search pattern\n containing a \\0.\n@@ -289,6 +293,20 @@ In future versions we may learn to support patterns containing \\0 for\n more search backends, until then we'll die when the pattern type in\n question doesn't support them.\n \n+--pathspec-from-file ::\n+\tRead pathspec from instead of the command line. If `` is\n+\texactly `-` then standard input is used; standard input cannot be used\n+\tfor both --patterns-from-file and --pathspec-from-file. Pathspec elements\n+\tare separated by LF or CR/LF. Pathspec elements can be quoted as\n+\texplained for the configuration variable `core.quotePath` (see\n+\tlinkgit:git-config[1]). See also `--pathspec-file-nul` and global\n+\t`--literal-pathspecs`.\n+\n+--pathspec-file-nul::\n+\tOnly meaningful with `--pathspec-from-file`. Pathspec elements are\n+\tseparated with NUL character and all other characters are taken\n+\tliterally (including newlines and quotes).\n+\n -e::\n \tThe next parameter is the pattern. This option has to be\n \tused for patterns starting with `-` and should be used in\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 50ce8d9461..28b9f99d4c 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -720,26 +720,23 @@ static int context_callback(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static int file_callback(const struct option *opt, const char *arg, int unset)\n+static int read_pattern_file(struct grep_opt *grep_opt, const char *path)\n {\n-\tstruct grep_opt *grep_opt = opt->value;\n \tint from_stdin;\n \tFILE *patterns;\n \tint lno = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tBUG_ON_OPT_NEG(unset);\n-\n-\tfrom_stdin = !strcmp(arg, \"-\");\n-\tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n+\tfrom_stdin = !strcmp(path, \"-\");\n+\tpatterns = from_stdin ? stdin : fopen(path, \"r\");\n \tif (!patterns)\n-\t\tdie_errno(_(\"cannot open '%s'\"), arg);\n+\t\tdie_errno(_(\"cannot open '%s'\"), path);\n \twhile (strbuf_getline(&sb, patterns) == 0) {\n \t\t/* ignore empty line like grep does */\n \t\tif (sb.len == 0)\n \t\t\tcontinue;\n \n-\t\tappend_grep_pat(grep_opt, sb.buf, sb.len, arg, ++lno,\n+\t\tappend_grep_pat(grep_opt, sb.buf, sb.len, path, ++lno,\n \t\t\t\tGREP_PATTERN);\n \t}\n \tif (!from_stdin)\n@@ -809,6 +806,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tint use_index = 1;\n \tint pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;\n \tint allow_revs;\n+\tchar *patterns_from_file = NULL;\n+\tchar *pathspec_from_file = NULL;\n+\tint pathspec_file_nul = 0;\n \n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"cached\", &cached,\n@@ -896,8 +896,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('W', \"function-context\", &opt.funcbody,\n \t\t\tN_(\"show the surrounding function\")),\n \t\tOPT_GROUP(\"\"),\n-\t\tOPT_CALLBACK('f', NULL, &opt, N_(\"file\"),\n-\t\t\tN_(\"read patterns from file\"), file_callback),\n+\t\tOPT_STRING('f', \"patterns-from-file\", &patterns_from_file, N_(\"file\"),\n+\t\t\tN_(\"read patterns from file\")),\n+\t\tOPT_PATHSPEC_FROM_FILE(&pathspec_from_file),\n+\t\tOPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),\n \t\t{ OPTION_CALLBACK, 'e', NULL, &opt, N_(\"pattern\"),\n \t\t\tN_(\"match \"), PARSE_OPT_NONEG, pattern_callback },\n \t\t{ OPTION_CALLBACK, 0, \"and\", &opt, NULL,\n@@ -949,6 +951,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \tgrep_commit_pattern_type(pattern_type_arg, &opt);\n \n+\tif (patterns_from_file && pathspec_from_file &&\n+\t !strcmp(pathspec_from_file, \"-\") &&\n+\t !strcmp(patterns_from_file, \"-\"))\n+\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n+\n+\tif (patterns_from_file)\n+\t\tread_pattern_file(&opt, patterns_from_file);\n+\n+\n \tif (use_index && !startup_info->have_repository) {\n \t\tint fallback = 0;\n \t\tgit_config_get_bool(\"grep.fallbacktonoindex\", &fallback);\n@@ -1062,6 +1073,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpathspec.recursive = 1;\n \tpathspec.recurse_submodules = !!recurse_submodules;\n \n+\tif (pathspec_from_file) {\n+\t\tif (pathspec.nr)\n+\t\t\tdie(_(\"--pathspec-from-file is incompatible with pathspec arguments\"));\n+\n+\t\tparse_pathspec_file(&pathspec, 0, PATHSPEC_PREFER_CWD |\n+\t\t\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n+\t\t\t\t prefix, pathspec_from_file,\n+\t\t\t\t pathspec_file_nul);\n+\t} else if (pathspec_file_nul) {\n+\t\tdie(_(\"--pathspec-file-nul requires --pathspec-from-file\"));\n+\t}\n+\n \tif (list.nr || cached || show_in_pager) {\n \t\tif (num_threads > 1)\n \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 7d7b396c23..3ace2dfdac 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '\n \ttest_must_fail git grep -f patterns\n '\n \n+text_expect_success 'grep --pathspec-from-file, non-existent file' '\n+\ttest_must_fail git grep --pathspec-from-file pathspecs\n+'\n+\n cat >expected <actual &&\n+test_expect_success 'grep --patterns-from-file, multiple patterns' '\n+\tgit grep --patterns-from-file patterns >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -1125,6 +1129,45 @@ test_expect_success 'grep --no-index descends into repos, but not .git' '\n \t)\n '\n \n+test_expect_success 'setup pathspecs-file tests' '\n+cat >excluded-file <pathspec-file <unrelated-file <pathspecs <expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'grep --pathspec-file with stdin' '\n+\tgit grep --pathspec-from-file - \"bar\" actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'grep with two stdin inputs fails' '\n+\ttest_must_fail git grep --pathspec-from-file - --patterns-from-file - err &&\n+\ttest_i18ngrep \"cannot specify both patterns and pathspec via stdin\" err\n+'\n+\n test_expect_success 'setup double-dash tests' '\n cat >double-dash < +\tif (patterns_from_file && pathspec_from_file &&\n> +\t !strcmp(pathspec_from_file, \"-\") &&\n> +\t !strcmp(patterns_from_file, \"-\"))\n> +\t\tdie(_(\"cannot specify both patterns and pathspec via stdin\"));\n> +\n> +\tif (patterns_from_file)\n> +\t\tread_pattern_file(&opt, patterns_from_file);\n> +\n> +\n\nThat looks a lot more solid now, thanks!\n\n> @@ -549,6 +549,10 @@ test_expect_success 'grep -f, non-existent file' '\n> \ttest_must_fail git grep -f patterns\n> '\n\nCould also benefit from testing for specific error here.\n\n> +test_expect_success 'setup pathspecs-file tests' '\n> +cat >excluded-file < +bar\n> +EOF\n> +cat >pathspec-file < +foo\n> +bar\n> +baz\n> +EOF\n> +cat >unrelated-file < +xyz\n> +EOF\n> +git add excluded-file pathspec-file unrelated-file\n> +'\n\nPlease use <<-EOF and tabulate the code properly. I suggest that you \nhave a look at `t/t7107-reset-pathspec-file.sh`, last test.\n\n> +\n> +cat >pathspecs < +pathspec-file\n> +unrelated-file\n> +EOF\n> +\n> +cat >expected < +pathspec-file:bar\n> +EOF\n\nThe general approach nowadays is to write `expect` file in every test. \nNote that the standard name is `expect`, so please remove `ed` from your \nname. I think that this is also reasonable for `pathspecs` file, it's \nonly used in two places. Again, please refer to \n`t/t7107-reset-pathspec-file.sh`.\n"},{"id":"388139","messageId":"xmqqpngskrpd.fsf@gitster-ct.c.googlers.com","threadId":"52307","inReplyTo":"20191213041254.13202-1-emilyshaffer@google.com","subject":"Re: [PATCH v3] grep: support the --pathspec-from-file option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-13T18:26:06Z","receivedAt":"2019-12-13T20:40:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Teach 'git grep' to use OPT_PATHSPEC_FROM_FILE and update the\n> documentation accordingly.\n>\n> This changes enables 'git grep' to receive the pathspec from a file by\n> specifying the path, or from stdin by specifying '-' as the path. This\n> matches the previous functionality of '-f', so the documentation of '-f'\n> has been expanded to describe this functionality. To let '-f' match the\n> new '--pathspec-from-file' option, also teach a '--patterns-from-file'\n> long name to '-f'.\n>\n> Since there are now two arguments which can attempt to read from stdin,\n> add a safeguard to check whether the user specified '-' for both of\n> them. It is still possible for a user to pass '/dev/stdin' to one or\n> both arguments at present; we do not explicitly check.\n>\n> Signed-off-by: Emily Shaffer \n> ---\n> I built this on 'next' locally, but it does require 'am/pathspec-from-file'\n> (which was merged to 'next').\n\nThanks. I'd probably just queue on top of 'master' as the other\ntopic has graduated as part of the fifth batch.\n\n> -static int file_callback(const struct option *opt, const char *arg, int unset)\n> +static int read_pattern_file(struct grep_opt *grep_opt, const char *path)\n> {\n> -\tstruct grep_opt *grep_opt = opt->value;\n> \tint from_stdin;\n> \tFILE *patterns;\n> \tint lno = 0;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \n> -\tBUG_ON_OPT_NEG(unset);\n> -\n> -\tfrom_stdin = !strcmp(arg, \"-\");\n> -\tpatterns = from_stdin ? stdin : fopen(arg, \"r\");\n> +\tfrom_stdin = !strcmp(path, \"-\");\n> +\tpatterns = from_stdin ? stdin : fopen(path, \"r\");\n> \tif (!patterns)\n> -\t\tdie_errno(_(\"cannot open '%s'\"), arg);\n> +\t\tdie_errno(_(\"cannot open '%s'\"), path);\n> \twhile (strbuf_getline(&sb, patterns) == 0) {\n> \t\t/* ignore empty line like grep does */\n> \t\tif (sb.len == 0)\n> \t\t\tcontinue;\n> \n> -\t\tappend_grep_pat(grep_opt, sb.buf, sb.len, arg, ++lno,\n> +\t\tappend_grep_pat(grep_opt, sb.buf, sb.len, path, ++lno,\n> \t\t\t\tGREP_PATTERN);\n> \t}\n> \tif (!from_stdin)\n\nHmph. This has nothing to do with \"--pathspec-from-file\" that was\nadvertised on the title of the patch. It used to be that\n\n\tgit grep -f one -f two\n\ncan be used to read patterns from two sources, but that is no\nlonger possible, is it? Am I missing a larger benefit to accept\nthis regression?\n\n> +test_expect_success 'setup pathspecs-file tests' '\n> +cat >excluded-file < +bar\n> +EOF\n> +cat >pathspec-file < +foo\n> +bar\n> +baz\n> +EOF\n> +cat >unrelated-file < +xyz\n> +EOF\n> +git add excluded-file pathspec-file unrelated-file\n> +'\n\nAs Alexandr mentioned, the above is harder to read than necessary.\n\n\n\ttest_expect_success 'setup pathspec-file tests' '\n\t\ttest_write_lines >excluded-file bar &&\n\t\ttest_write_lines >pathspec-file foo bar baz &&\n\t\ttest_write_lines >unrelated-file xyz &&\n\t\tgit add ...\n\t'\n\nperhaps?\n\n> +\n> +cat >pathspecs < +pathspec-file\n> +unrelated-file\n> +EOF\n> +\n> +cat >expected < +pathspec-file:bar\n> +EOF\n\nAlso, shouldn't the above also be part of the (sub)setup for these\ntests? IOW, after that addition of three files with \"git add\", \n\n\t\ttest_write_lines >pathspec pathspec-file unrelated-file &&\n\t\ttest_write_lines >expect pathspec-file:bar\n\nin the \"setup pathspec-file tests\".\n\n> +test_expect_success 'grep --pathspec-from-file with file' '\n> +\tgit grep --pathspec-from-file pathspecs \"bar\" >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'grep --pathspec-file with stdin' '\n> +\tgit grep --pathspec-from-file - \"bar\" actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'grep with two stdin inputs fails' '\n> +\ttest_must_fail git grep --pathspec-from-file - --patterns-from-file - err &&\n> +\ttest_i18ngrep \"cannot specify both patterns and pathspec via stdin\" err\n> +'\n> +\n> test_expect_success 'setup double-dash tests' '\n> cat >double-dash < --\n"},{"id":"388159","messageId":"52cf5a3b-09e4-f4d2-a5d0-36b6ddc442f6@syntevo.com","threadId":"52307","inReplyTo":"xmqqpngskrpd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] grep: support the --pathspec-from-file option","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-12-13T20:13:09Z","receivedAt":"2019-12-13T20:41:18Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"On 13.12.2019 19:26, Junio C Hamano wrote:\n\n> Hmph. This has nothing to do with \"--pathspec-from-file\" that was\n> advertised on the title of the patch. It used to be that\n> \n> \tgit grep -f one -f two\n> \n> can be used to read patterns from two sources, but that is no\n> longer possible, is it? Am I missing a larger benefit to accept\n> this regression?\n\nOuch, that comes completely unexpected to me. It's good when someone \nexperienced watches over :)\n"},{"id":"388318","messageId":"20191217003340.GO135450@google.com","threadId":"52307","inReplyTo":"52cf5a3b-09e4-f4d2-a5d0-36b6ddc442f6@syntevo.com","subject":"Re: [PATCH v3] grep: support the --pathspec-from-file option","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-17T00:33:40Z","receivedAt":"2019-12-17T00:33:47Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Dec 13, 2019 at 09:13:09PM +0100, Alexandr Miloslavskiy wrote:\n> On 13.12.2019 19:26, Junio C Hamano wrote:\n> \n> > Hmph. This has nothing to do with \"--pathspec-from-file\" that was\n> > advertised on the title of the patch. It used to be that\n> > \n> > \tgit grep -f one -f two\n> > \n> > can be used to read patterns from two sources, but that is no\n> > longer possible, is it? Am I missing a larger benefit to accept\n> > this regression?\n> \n> Ouch, that comes completely unexpected to me. It's good when someone\n> experienced watches over :)\n\nYeah, I'm surprised by it too - and surprised that there wasn't a test\nto ensure this behavior.\n\nI can put it back as a callback. I'm going to add a second patch to this\ntopic to enforce 'git grep -f one -f two' with a testcase.\n\nAs for missed benefits, they all have to do with ugly code vs. pretty\ncode. I don't think it's enough of a reason to lose functionality.\n\n - Emily\n"}]}