{"thread":{"id":"55122","subject":"[PATCH] grep: error out if --untracked is used with --cached","startedAt":"2021-02-08T19:45:26Z","lastAt":"2021-02-10T02:46:38Z","messageCount":7,"participants":["Matheus Tavares","Taylor Blau","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"416439","messageId":"5bd9dce8f611c5fe380c9f58dbdfa2dc6d2fd51f.1612813249.git.matheus.bernardino@usp.br","threadId":"55122","inReplyTo":null,"subject":"[PATCH] grep: error out if --untracked is used with --cached","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-08T19:43:28Z","receivedAt":"2021-02-08T19:45:26Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The options --untracked and --cached are not compatible, but if they are\nused together, grep just silently ignores --cached and searches the\nworking tree. Error out, instead, to avoid any potential confusion.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex ca259af441..392acf8cab 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!use_index && (untracked || cached))\n \t\tdie(_(\"--cached or --untracked cannot be used with --no-index\"));\n \n+\tif (untracked && cached)\n+\t\tdie(_(\"--untracked cannot be used with --cached\"));\n+\n \tif (!use_index || untracked) {\n \t\tint use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n \t\thit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n-- \n2.29.2\n\n"},{"id":"416446","messageId":"YCGxos2vB6mgHOTA@nand.local","threadId":"55122","inReplyTo":"5bd9dce8f611c5fe380c9f58dbdfa2dc6d2fd51f.1612813249.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] grep: error out if --untracked is used with --cached","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-08T21:48:18Z","receivedAt":"2021-02-08T21:49:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 08, 2021 at 04:43:28PM -0300, Matheus Tavares wrote:\n> The options --untracked and --cached are not compatible, but if they are\n> used together, grep just silently ignores --cached and searches the\n> working tree. Error out, instead, to avoid any potential confusion.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> builtin/grep.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index ca259af441..392acf8cab 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tif (!use_index && (untracked || cached))\n> \t\tdie(_(\"--cached or --untracked cannot be used with --no-index\"));\n>\n> +\tif (untracked && cached)\n> +\t\tdie(_(\"--untracked cannot be used with --cached\"));\n> +\n\nAre these really incompatible? --untracked says that untracked files are\nsearched in addition to tracked ones in the working tree.\n--cached says that the index is searched instead of tracked files. From\nmy reading, that seems to imply that the combination you're proposing\ngetting rid of would mean: \"search the index,and untracked files\".\n\nThat's a narrow use-case, but I couldn't think of a reason that it\nshouldn't work (but it's been a while since I've looked or thought much\nabout the \"git grep\" code...).\n\nAssuming that they are incompatible, though, a few thoughts:\n\nShould this come before the \"!use_index && (untracked || cached)\" guard?\nright now passing all three options first says you can't combine\n--cached/--untracked with --no-index. Presumably the next invocation\nwould come without --no-index, only to come back that the remaining\noptions are incompatible.\n\nI dunno. I'm thinking out loud, but it feels like this guard should come\nbefore the one above it, not after.\n\nShould this appear in 'git-grep(1)' too? I guess not, since looking for\n\"[iI]ncompatible\" in that file turns up zero results.\n\nThanks,\nTaylor\n"},{"id":"416458","messageId":"20210208232159.100543-1-matheus.bernardino@usp.br","threadId":"55122","inReplyTo":"YCGxos2vB6mgHOTA@nand.local","subject":"Re: [PATCH] grep: error out if --untracked is used with --cached","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-08T23:21:59Z","receivedAt":"2021-02-08T23:22:50Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"\nOn Mon, Feb 8, 2021 at 6:48 PM Taylor Blau wrote:\n>\n> On Mon, Feb 08, 2021 at 04:43:28PM -0300, Matheus Tavares wrote:\n> > The options --untracked and --cached are not compatible, but if they are\n> > used together, grep just silently ignores --cached and searches the\n> > working tree. Error out, instead, to avoid any potential confusion.\n> >\n> > Signed-off-by: Matheus Tavares \n> > ---\n> >  builtin/grep.c | 3 +++\n> >  1 file changed, 3 insertions(+)\n> >\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index ca259af441..392acf8cab 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> >       if (!use_index && (untracked || cached))\n> >               die(_(\"--cached or --untracked cannot be used with --no-index\"));\n> >\n> > +     if (untracked && cached)\n> > +             die(_(\"--untracked cannot be used with --cached\"));\n> > +\n>\n> Are these really incompatible? --untracked says that untracked files are\n> searched in addition to tracked ones in the working tree.\n> --cached says that the index is searched instead of tracked files. From\n> my reading, that seems to imply that the combination you're proposing\n> getting rid of would mean: \"search the index,and untracked files\".\n\nYeah, I agree that there is nothing conceptually wrong with the use case\n\"search the index, and untracked files\". The problem is that git-grep is\ncurrently not able to search both these places in the same execution.\nIn fact, I don't think it can combine working tree and index searches\nin any way (besides with --assume-unchanged paths).\n\nWhen --cached is used with --untracked, git-grep silently ignores\n--cached and searches the working tree only:\n\n$ echo 'cached A' >A\n$ git add A\n$ git commit -m A\n$ echo 'modified A' >A\n$ echo 'untracked' >B\n$ git grep --cached --untracked .\nA:modified A\nB:untracked\n\nPerhaps, instead of erroring out with this currently invalid\ncombination, should we make it valid by teaching git-grep how to search\nthe two places on a single run? I.e. something like:\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex ca259af441..b0e99096ff 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -699,7 +699,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n }\n\n static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t\t int exc_std, int use_index)\n+\t\t\t int exc_std, int use_index, int untracked_only)\n {\n \tstruct dir_struct dir;\n \tint i, hit = 0;\n@@ -712,7 +712,13 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n\n \tfill_directory(&dir, opt->repo->index, pathspec);\n \tfor (i = 0; i < dir.nr; i++) {\n-\t\thit |= grep_file(opt, dir.entries[i]->name);\n+\t\tstruct dir_entry *ent = dir.entries[i];\n+\n+\t\tif (untracked_only &&\n+\t\t !index_name_is_other(opt->repo->index, ent->name, ent->len))\n+\t\t\tcontinue;\n+\n+\t\thit |= grep_file(opt, ent->name);\n \t\tif (hit && opt->status_only)\n \t\t\tbreak;\n \t}\n@@ -1157,9 +1163,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!use_index && (untracked || cached))\n \t\tdie(_(\"--cached or --untracked cannot be used with --no-index\"));\n\n-\tif (!use_index || untracked) {\n+\tif (cached && untracked) {\n+\t\thit = grep_cache(&opt, &pathspec, 1);\n+\t\thit |= grep_directory(&opt, &pathspec, !!opt_exclude, 1, 1);\n+\n+\t} else if (!use_index || untracked) {\n \t\tint use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n-\t\thit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n+\t\thit |= grep_directory(&opt, &pathspec, use_exclude, use_index, 0);\n+\n \t} else if (0 <= opt_exclude) {\n \t\tdie(_(\"--[no-]exclude-standard cannot be used for tracked contents\"));\n \t} else if (!list.nr) {\n\nWith this, the `git grep --cached --untracked .` search from the\nprevious example would produce the following output:\n\nA:cached A\nB:untracked\n\nIn this case, should we also add an 'untracked:' / 'cached:' prefix to\nthe filenames, like we do for revision searches (e.g. 'HEAD:A:cached A') ?\n\n"},{"id":"416488","messageId":"CABPp-BFouwiACwwS5mDdgBRF=YK--=NfqZ9r=qkFouEvyJfnGQ@mail.gmail.com","threadId":"55122","inReplyTo":"20210208232159.100543-1-matheus.bernardino@usp.br","subject":"Re: [PATCH] grep: error out if --untracked is used with --cached","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-02-09T10:06:46Z","receivedAt":"2021-02-09T10:16:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Feb 8, 2021 at 3:27 PM Matheus Tavares\n wrote:\n>\n>\n> On Mon, Feb 8, 2021 at 6:48 PM Taylor Blau wrote:\n> >\n> > On Mon, Feb 08, 2021 at 04:43:28PM -0300, Matheus Tavares wrote:\n> > > The options --untracked and --cached are not compatible, but if they are\n> > > used together, grep just silently ignores --cached and searches the\n> > > working tree. Error out, instead, to avoid any potential confusion.\n> > >\n> > > Signed-off-by: Matheus Tavares \n> > > ---\n> > > builtin/grep.c | 3 +++\n> > > 1 file changed, 3 insertions(+)\n> > >\n> > > diff --git a/builtin/grep.c b/builtin/grep.c\n> > > index ca259af441..392acf8cab 100644\n> > > --- a/builtin/grep.c\n> > > +++ b/builtin/grep.c\n> > > @@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> > > if (!use_index && (untracked || cached))\n> > > die(_(\"--cached or --untracked cannot be used with --no-index\"));\n> > >\n> > > + if (untracked && cached)\n> > > + die(_(\"--untracked cannot be used with --cached\"));\n> > > +\n> >\n> > Are these really incompatible? --untracked says that untracked files are\n> > searched in addition to tracked ones in the working tree.\n> > --cached says that the index is searched instead of tracked files. From\n> > my reading, that seems to imply that the combination you're proposing\n> > getting rid of would mean: \"search the index,and untracked files\".\n>\n> Yeah, I agree that there is nothing conceptually wrong with the use case\n> \"search the index, and untracked files\". The problem is that git-grep is\n> currently not able to search both these places in the same execution.\n> In fact, I don't think it can combine working tree and index searches\n> in any way (besides with --assume-unchanged paths).\n>\n> When --cached is used with --untracked, git-grep silently ignores\n> --cached and searches the working tree only:\n>\n> $ echo 'cached A' >A\n> $ git add A\n> $ git commit -m A\n> $ echo 'modified A' >A\n> $ echo 'untracked' >B\n> $ git grep --cached --untracked .\n> A:modified A\n> B:untracked\n>\n> Perhaps, instead of erroring out with this currently invalid\n> combination, should we make it valid by teaching git-grep how to search\n> the two places on a single run? I.e. something like:\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index ca259af441..b0e99096ff 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -699,7 +699,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n> }\n>\n> static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n> - int exc_std, int use_index)\n> + int exc_std, int use_index, int untracked_only)\n> {\n> struct dir_struct dir;\n> int i, hit = 0;\n> @@ -712,7 +712,13 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n>\n> fill_directory(&dir, opt->repo->index, pathspec);\n> for (i = 0; i < dir.nr; i++) {\n> - hit |= grep_file(opt, dir.entries[i]->name);\n> + struct dir_entry *ent = dir.entries[i];\n> +\n> + if (untracked_only &&\n> + !index_name_is_other(opt->repo->index, ent->name, ent->len))\n> + continue;\n> +\n> + hit |= grep_file(opt, ent->name);\n> if (hit && opt->status_only)\n> break;\n> }\n> @@ -1157,9 +1163,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> if (!use_index && (untracked || cached))\n> die(_(\"--cached or --untracked cannot be used with --no-index\"));\n>\n> - if (!use_index || untracked) {\n> + if (cached && untracked) {\n> + hit = grep_cache(&opt, &pathspec, 1);\n> + hit |= grep_directory(&opt, &pathspec, !!opt_exclude, 1, 1);\n> +\n> + } else if (!use_index || untracked) {\n> int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n> - hit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n> + hit |= grep_directory(&opt, &pathspec, use_exclude, use_index, 0);\n> +\n> } else if (0 <= opt_exclude) {\n> die(_(\"--[no-]exclude-standard cannot be used for tracked contents\"));\n> } else if (!list.nr) {\n>\n> With this, the `git grep --cached --untracked .` search from the\n> previous example would produce the following output:\n>\n> A:cached A\n> B:untracked\n>\n> In this case, should we also add an 'untracked:' / 'cached:' prefix to\n> the filenames, like we do for revision searches (e.g. 'HEAD:A:cached A') ?\n\nIck, no. --untracked was a modifier on working tree searches. I\nthink doing this adds a really weird inconsistency for all the other\noption combinations. I say mark --untracked and --cached as\nincompatible like you did in your original patch, unless you're\nwilling to make grep generally be able to have options to search two\nor more locations from any of (revisions of history, the cache, the\nworking tree), including being able to simultaneously search two\ndifferent revisions of history. Even then, I'd be tempted to say that\n--untracked is only used in combination with a search of the working\ntree.\n"},{"id":"416489","messageId":"CABPp-BFx6HmoVBsS2Aeb+GgZTC8axwGcuWrkpaCFxM-j2SOMww@mail.gmail.com","threadId":"55122","inReplyTo":"5bd9dce8f611c5fe380c9f58dbdfa2dc6d2fd51f.1612813249.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] grep: error out if --untracked is used with --cached","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-02-09T10:07:12Z","receivedAt":"2021-02-09T10:16:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Feb 8, 2021 at 11:47 AM Matheus Tavares\n wrote:\n>\n> The options --untracked and --cached are not compatible, but if they are\n> used together, grep just silently ignores --cached and searches the\n> working tree. Error out, instead, to avoid any potential confusion.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> builtin/grep.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index ca259af441..392acf8cab 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1157,6 +1157,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> if (!use_index && (untracked || cached))\n> die(_(\"--cached or --untracked cannot be used with --no-index\"));\n>\n> + if (untracked && cached)\n> + die(_(\"--untracked cannot be used with --cached\"));\n> +\n> if (!use_index || untracked) {\n> int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n> hit = grep_directory(&opt, &pathspec, use_exclude, use_index);\n> --\n> 2.29.2\n\nReviewed-by: Elijah Newren \n"},{"id":"416552","messageId":"xmqqtuql0yfp.fsf@gitster.c.googlers.com","threadId":"55122","inReplyTo":"CABPp-BFouwiACwwS5mDdgBRF=YK--=NfqZ9r=qkFouEvyJfnGQ@mail.gmail.com","subject":"Re: [PATCH] grep: error out if --untracked is used with --cached","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-09T20:38:02Z","receivedAt":"2021-02-09T20:51:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> ... Even then, I'd be tempted to say that\n> --untracked is only used in combination with a search of the working\n> tree.\n\nI tend to agree. Something like\n\n $ git grep -ne POISON maint master next seen -- t/test-lib.sh\n\nwould be a useful thing, but I do not see how --untracked would\nusefully interact with any of these \"tracked\" contents, be the\nsearch from the index or tree.\n\n"},{"id":"416601","messageId":"YCNIVm3uz25Akl1q@nand.local","threadId":"55122","inReplyTo":"xmqqtuql0yfp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] grep: error out if --untracked is used with --cached","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-10T02:43:34Z","receivedAt":"2021-02-10T02:46:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 09, 2021 at 12:38:02PM -0800, Junio C Hamano wrote:\n> Elijah Newren writes:\n>\n> > ... Even then, I'd be tempted to say that\n> > --untracked is only used in combination with a search of the working\n> > tree.\n>\n> I tend to agree. Something like\n>\n> $ git grep -ne POISON maint master next seen -- t/test-lib.sh\n>\n> would be a useful thing, but I do not see how --untracked would\n> usefully interact with any of these \"tracked\" contents, be the\n> search from the index or tree.\n\nI agree with both you and Elijah. I don't mind banning the pair as\nunimplemented and \"won't possibly work correctly together\", since it\nleaves open the possibility that we might find time to make them\ncompatible, at which point we could revert this patch.\n\nBut what Matheus provided looks good to me.\n\n\nThanks,\nTaylor\n"}]}