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

[Elijah Newren <newren@gmail.com>]

On Mon, Feb 8, 2021 at 3:27 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
>
> 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') ?

Ick, no.  --untracked was a modifier on working tree searches.  I
think doing this adds a really weird inconsistency for all the other
option combinations.  I say mark --untracked and --cached as
incompatible like you did in your original patch, unless you're
willing to make grep generally be able to have options to search two
or more locations from any of (revisions of history, the cache, the
working tree), including being able to simultaneously search two
different revisions of history.  Even then, I'd be tempted to say that
--untracked is only used in combination with a search of the working
tree.