Merge branch 'jk/grep-no-index-fix'
authorJunio C Hamano <gitster@pobox.com>
Mon, 27 Feb 2017 21:57:15 +0000 (13:57 -0800)
committerJunio C Hamano <gitster@pobox.com>
Mon, 27 Feb 2017 21:57:16 +0000 (13:57 -0800)
The code to parse the command line "git grep <patterns>... <rev>
[[--] <pathspec>...]" has been cleaned up, and a handful of bugs
have been fixed (e.g. we used to check "--" if it is a rev).

* jk/grep-no-index-fix:
grep: treat revs the same for --untracked as for --no-index
grep: do not diagnose misspelt revs with --no-index
grep: avoid resolving revision names in --no-index case
grep: fix "--" rev/pathspec disambiguation
grep: re-order rev-parsing loop
grep: do not unnecessarily query repo for "--"
grep: move thread initialization a little lower

builtin/grep.c
t/t7810-grep.sh
index 2c727ef499c0fa7b44c0cf1890cc44427dc8921f..9304c33e75051baa65ee02d1daa03c361e25339e 100644 (file)
@@ -967,6 +967,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
        int dummy;
        int use_index = 1;
        int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
+       int allow_revs;
 
        struct option options[] = {
                OPT_BOOL(0, "cached", &cached,
@@ -1149,26 +1150,69 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
        compile_grep_patterns(&opt);
 
-       /* Check revs and then paths */
+       /*
+        * We have to find "--" in a separate pass, because its presence
+        * influences how we will parse arguments that come before it.
+        */
+       for (i = 0; i < argc; i++) {
+               if (!strcmp(argv[i], "--")) {
+                       seen_dashdash = 1;
+                       break;
+               }
+       }
+
+       /*
+        * Resolve any rev arguments. If we have a dashdash, then everything up
+        * to it must resolve as a rev. If not, then we stop at the first
+        * non-rev and assume everything else is a path.
+        */
+       allow_revs = use_index && !untracked;
        for (i = 0; i < argc; i++) {
                const char *arg = argv[i];
                unsigned char sha1[20];
                struct object_context oc;
-               /* Is it a rev? */
-               if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
-                       struct object *object = parse_object_or_die(sha1, arg);
-                       if (!seen_dashdash)
-                               verify_non_filename(prefix, arg);
-                       add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
-                       continue;
-               }
+               struct object *object;
+
                if (!strcmp(arg, "--")) {
                        i++;
-                       seen_dashdash = 1;
+                       break;
                }
-               break;
+
+               if (!allow_revs) {
+                       if (seen_dashdash)
+                               die(_("--no-index or --untracked cannot be used with revs"));
+                       break;
+               }
+
+               if (get_sha1_with_context(arg, 0, sha1, &oc)) {
+                       if (seen_dashdash)
+                               die(_("unable to resolve revision: %s"), arg);
+                       break;
+               }
+
+               object = parse_object_or_die(sha1, arg);
+               if (!seen_dashdash)
+                       verify_non_filename(prefix, arg);
+               add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
        }
 
+       /*
+        * Anything left over is presumed to be a path. But in the non-dashdash
+        * "do what I mean" case, we verify and complain when that isn't true.
+        */
+       if (!seen_dashdash) {
+               int j;
+               for (j = i; j < argc; j++)
+                       verify_filename(prefix, argv[j], j == i && allow_revs);
+       }
+
+       parse_pathspec(&pathspec, 0,
+                      PATHSPEC_PREFER_CWD |
+                      (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+                      prefix, argv + i);
+       pathspec.max_depth = opt.max_depth;
+       pathspec.recursive = 1;
+
 #ifndef NO_PTHREADS
        if (list.nr || cached || show_in_pager)
                num_threads = 0;
@@ -1190,20 +1234,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
        }
 #endif
 
-       /* The rest are paths */
-       if (!seen_dashdash) {
-               int j;
-               for (j = i; j < argc; j++)
-                       verify_filename(prefix, argv[j], j == i);
-       }
-
-       parse_pathspec(&pathspec, 0,
-                      PATHSPEC_PREFER_CWD |
-                      (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
-                      prefix, argv + i);
-       pathspec.max_depth = opt.max_depth;
-       pathspec.recursive = 1;
-
        if (recurse_submodules) {
                gitmodules_config();
                compile_submodule_options(&opt, &pathspec, cached, untracked,
@@ -1245,8 +1275,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
        if (!use_index || untracked) {
                int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
-               if (list.nr)
-                       die(_("--no-index or --untracked cannot be used with revs."));
                hit = grep_directory(&opt, &pathspec, use_exclude, use_index);
        } else if (0 <= opt_exclude) {
                die(_("--[no-]exclude-standard cannot be used for tracked contents."));
index 19f0108f8a2c67c518a92d996be7a335dbb69af2..cee42097b077378f42910666a7895347b0787b1a 100755 (executable)
@@ -982,6 +982,72 @@ test_expect_success 'grep -e -- -- path' '
        test_cmp expected actual
 '
 
+test_expect_success 'dashdash disambiguates rev as rev' '
+       test_when_finished "rm -f master" &&
+       echo content >master &&
+       echo master:hello.c >expect &&
+       git grep -l o master -- hello.c >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success 'dashdash disambiguates pathspec as pathspec' '
+       test_when_finished "git rm -f master" &&
+       echo content >master &&
+       git add master &&
+       echo master:content >expect &&
+       git grep o -- master >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success 'report bogus arg without dashdash' '
+       test_must_fail git grep o does-not-exist
+'
+
+test_expect_success 'report bogus rev with dashdash' '
+       test_must_fail git grep o hello.c --
+'
+
+test_expect_success 'allow non-existent path with dashdash' '
+       # We need a real match so grep exits with success.
+       tree=$(git ls-tree HEAD |
+              sed s/hello.c/not-in-working-tree/ |
+              git mktree) &&
+       git grep o "$tree" -- not-in-working-tree
+'
+
+test_expect_success 'grep --no-index pattern -- path' '
+       rm -fr non &&
+       mkdir -p non/git &&
+       (
+               GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+               export GIT_CEILING_DIRECTORIES &&
+               cd non/git &&
+               echo hello >hello &&
+               echo goodbye >goodbye &&
+               echo hello:hello >expect &&
+               git grep --no-index o -- hello >actual &&
+               test_cmp expect actual
+       )
+'
+
+test_expect_success 'grep --no-index complains of revs' '
+       test_must_fail git grep --no-index o master -- 2>err &&
+       test_i18ngrep "cannot be used with revs" err
+'
+
+test_expect_success 'grep --no-index prefers paths to revs' '
+       test_when_finished "rm -f master" &&
+       echo content >master &&
+       echo master:content >expect &&
+       git grep --no-index o master >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success 'grep --no-index does not "diagnose" revs' '
+       test_must_fail git grep --no-index o :1:hello.c 2>err &&
+       test_i18ngrep ! -i "did you mean" err
+'
+
 cat >expected <<EOF
 hello.c:int main(int argc, const char **argv)
 hello.c:       printf("Hello world.\n");