| Message ID | 20191213041254.13202-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] grep: support the --pathspec-from-file option | expand |
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`.
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 && > --
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 :)
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
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 && --
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(-)