[PATCH] grep: error out if --untracked is used with --cached

The options --untracked and --cached are not compatible, but if they are
used together, grep just silently ignores --cached and searches the
working tree. Error out, instead, to avoid any potential confusion.

Signed-off-by: Matheus Tavares <redacted>
---
 builtin/grep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index ca259af441..392acf8cab 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!use_index && (untracked || cached))
 		die(_("--cached or --untracked cannot be used with --no-index"));

+	if (untracked && cached)
+		die(_("--untracked cannot be used with --cached"));
+

On Mon, Feb 8, 2021 at 6:48 PM Taylor Blau [off-list ref] wrote:

quoted

On Mon, Feb 08, 2021 at 04:43:28PM -0300, Matheus Tavares wrote:

quoted

The options --untracked and --cached are not compatible, but if they are
used together, grep just silently ignores --cached and searches the
working tree. Error out, instead, to avoid any potential confusion.

Signed-off-by: Matheus Tavares <redacted>
---
 builtin/grep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index ca259af441..392acf8cab 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
      if (!use_index && (untracked || cached))
              die(_("--cached or --untracked cannot be used with --no-index"));

+     if (untracked && cached)
+             die(_("--untracked cannot be used with --cached"));
+

Are these really incompatible? --untracked says that untracked files are
searched in addition to tracked ones in the working tree.
--cached says that the index is searched instead of tracked files. From
my reading, that seems to imply that the combination you're proposing
getting rid of would mean: "search the index,and untracked files".

Yeah, I agree that there is nothing conceptually wrong with the use case
"search the index, and untracked files". The problem is that git-grep is
currently not able to search both these places in the same execution.
In fact, I don't think it can combine working tree and index searches
in any way (besides with --assume-unchanged paths).

When --cached is used with --untracked, git-grep silently ignores
--cached and searches the working tree only:

$ echo 'cached A' >A
$ git add A
$ git commit -m A
$ echo 'modified A' >A
$ echo 'untracked' >B
$ git grep --cached --untracked .
A:modified A
B:untracked

Perhaps, instead of erroring out with this currently invalid
combination, should we make it valid by teaching git-grep how to search
the two places on a single run? I.e. something like:

diff --git a/builtin/grep.c b/builtin/grep.c
index ca259af441..b0e99096ff 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -699,7 +699,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
 }

 static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
-                         int exc_std, int use_index)
+                         int exc_std, int use_index, int untracked_only)
 {
        struct dir_struct dir;
        int i, hit = 0;

@@ -712,7 +712,13 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,

        fill_directory(&dir, opt->repo->index, pathspec);
        for (i = 0; i < dir.nr; i++) {
-               hit |= grep_file(opt, dir.entries[i]->name);
+               struct dir_entry *ent = dir.entries[i];
+
+               if (untracked_only &&
+                   !index_name_is_other(opt->repo->index, ent->name, ent->len))
+                       continue;
+
+               hit |= grep_file(opt, ent->name);
                if (hit && opt->status_only)
                        break;
        }

@@ -1157,9 +1163,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
        if (!use_index && (untracked || cached))
                die(_("--cached or --untracked cannot be used with --no-index"));

-       if (!use_index || untracked) {
+       if (cached && untracked) {
+               hit = grep_cache(&opt, &pathspec, 1);
+               hit |= grep_directory(&opt, &pathspec, !!opt_exclude, 1, 1);
+
+       } else if (!use_index || untracked) {
                int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
-               hit = grep_directory(&opt, &pathspec, use_exclude, use_index);
+               hit |= grep_directory(&opt, &pathspec, use_exclude, use_index, 0);
+
        } else if (0 <= opt_exclude) {
                die(_("--[no-]exclude-standard cannot be used for tracked contents"));
        } else if (!list.nr) {

With this, the `git grep --cached --untracked .` search from the
previous example would produce the following output:

A:cached A
B:untracked

In this case, should we also add an 'untracked:' / 'cached:' prefix to
the filenames, like we do for revision searches (e.g. 'HEAD:A:cached A') ?

The options --untracked and --cached are not compatible, but if they are
used together, grep just silently ignores --cached and searches the
working tree. Error out, instead, to avoid any potential confusion.

Signed-off-by: Matheus Tavares <redacted>
---
 builtin/grep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index ca259af441..392acf8cab 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
        if (!use_index && (untracked || cached))
                die(_("--cached or --untracked cannot be used with --no-index"));

+       if (untracked && cached)
+               die(_("--untracked cannot be used with --cached"));
+
        if (!use_index || untracked) {
                int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
                hit = grep_directory(&opt, &pathspec, use_exclude, use_index);
--

2.29.2