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

[Matheus Tavares <matheus.bernardino@usp.br>]

On Mon, Feb 8, 2021 at 6:48 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Feb 08, 2021 at 04:43:28PM -0300, Matheus Tavares wrote:
> > 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 <matheus.bernardino@usp.br>
> > ---
> >  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') ?