{"thread":{"id":"52697","subject":"[PATCH] grep: ignore --recurse-submodules if --no-index is given","startedAt":"2020-01-26T00:58:23Z","lastAt":"2020-01-30T18:14:48Z","messageCount":4,"participants":["Philippe Blain via GitGitGadget","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"390503","messageId":"pull.540.git.1580000298097.gitgitgadget@gmail.com","threadId":"52697","inReplyTo":null,"subject":"[PATCH] grep: ignore --recurse-submodules if --no-index is given","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-26T00:58:17Z","receivedAt":"2020-01-26T00:58:23Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince grep learned to recurse into submodules in 0281e487fd\n(grep: optionally recurse into submodules, 2016-12-16),\nusing --recurse-submodules along with --no-index makes Git\ndie().\n\nThis is unfortunate because if submodule.recurse is set in a user's\n~/.gitconfig, invoking `git grep --no-index` either inside or outside\na Git repository results in\n\n    fatal: option not supported with --recurse-submodules\n\nLet's allow using these options together, so that setting submodule.recurse\nglobally does not prevent using `git grep --no-index`.\n\nUsing `--recurse-submodules` should not have any effect if `--no-index`\nis used inside a repository, as Git will recurse into the checked out\nsubmodule directories just like into regular directories.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    grep: ignore --recurse-submodules if --no-index is given\n    \n    Since grep learned to recurse into submodules in 0281e487fd (grep:\n    optionally recurse into submodules, 2016-12-16), using\n    --recurse-submodules along with --no-index makes Git die().\n    \n    This is unfortunate because if submodule.recurse is set in a user's\n    ~/.gitconfig, invoking git grep --no-index either inside or outside a\n    Git repository results in\n    \n    fatal: option not supported with --recurse-submodules\n    \n    Let's allow using these options together, so that setting\n    submodule.recurse globally does not prevent using git grep --no-index.\n    \n    Using --recurse-submodules should not have any effect if --no-indexis\n    used inside a repository, as Git will recurse into the checked out\n    submodule directories just like into regular directories.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-540%2Fphil-blain%2Fgrep-no-index-ignore-recurse-submodule-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-540/phil-blain/grep-no-index-ignore-recurse-submodule-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/540\n\n Documentation/git-grep.txt         |  3 ++-\n builtin/grep.c                     |  4 ++--\n t/t7814-grep-recurse-submodules.sh | 11 ++++++++++-\n 3 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex c89fb569e3..ffc3a6efdc 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -96,7 +96,8 @@ OPTIONS\n \tRecursively search in each submodule that has been initialized and\n \tchecked out in the repository.  When used in combination with the\n \t<tree> option the prefix of all submodule output will be the name of\n-\tthe parent project's <tree> object.\n+\tthe parent project's <tree> object. This option has no effect\n+\tif `--no-index` is given.\n \n -a::\n --text::\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 50ce8d9461..d5f089dd41 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1115,8 +1115,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (recurse_submodules && (!use_index || untracked))\n-\t\tdie(_(\"option not supported with --recurse-submodules\"));\n+\tif (recurse_submodules && untracked)\n+\t\tdie(_(\"--untracked not supported with --recurse-submodules\"));\n \n \tif (!show_in_pager && !opt.status_only)\n \t\tsetup_pager();\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 946f91fa57..828cb3ba58 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -345,7 +345,16 @@ test_incompatible_with_recurse_submodules ()\n }\n \n test_incompatible_with_recurse_submodules --untracked\n-test_incompatible_with_recurse_submodules --no-index\n+\n+test_expect_success 'grep --recurse-submodules --no-index ignores --recurse-submodules' '\n+\tgit grep --recurse-submodules --no-index -e \"^(.|.)[\\d]\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)d(3|4)\n+\tsubmodule/a:(1|2)d(3|4)\n+\tsubmodule/sub/a:(1|2)d(3|4)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'grep --recurse-submodules should pass the pattern type along' '\n \t# Fixed\n\nbase-commit: bc7a3d4dc04dd719e7c8c35ebd7a6e6651c5c5b6\n-- \ngitgitgadget\n"},{"id":"390506","messageId":"nycvar.QRO.7.76.6.2001261036570.46@tvgsbejvaqbjf.bet","threadId":"52697","inReplyTo":"pull.540.git.1580000298097.gitgitgadget@gmail.com","subject":"Re: [PATCH] grep: ignore --recurse-submodules if --no-index is given","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-26T09:39:20Z","receivedAt":"2020-01-26T09:39:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Sun, 26 Jan 2020, Philippe Blain via GitGitGadget wrote:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> Since grep learned to recurse into submodules in 0281e487fd\n> (grep: optionally recurse into submodules, 2016-12-16),\n> using --recurse-submodules along with --no-index makes Git\n> die().\n>\n> This is unfortunate because if submodule.recurse is set in a user's\n> ~/.gitconfig, invoking `git grep --no-index` either inside or outside\n> a Git repository results in\n>\n>     fatal: option not supported with --recurse-submodules\n>\n> Let's allow using these options together, so that setting submodule.recurse\n> globally does not prevent using `git grep --no-index`.\n>\n> Using `--recurse-submodules` should not have any effect if `--no-index`\n> is used inside a repository, as Git will recurse into the checked out\n> submodule directories just like into regular directories.\n>\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n\nMy initial reaction to this patch was: \"but why would you try to combine\n`--recurse-submodules` with `--no-index`?\". The `submodule.recurse`\nreference made me think again, though.\n\nAnd then it hit me: by `--no-index`, what we _really_ mean to say is:\nignore that this _might_ be tracked in Git. And that obviously means that\nthere cannot be any submodules to recurse into, as far as `git diff\n--no-index` is concerned.\n\nSo I think your patch makes a ton of sense.\n\nThanks,\nDscho\n\n> ---\n>     grep: ignore --recurse-submodules if --no-index is given\n>\n>     Since grep learned to recurse into submodules in 0281e487fd (grep:\n>     optionally recurse into submodules, 2016-12-16), using\n>     --recurse-submodules along with --no-index makes Git die().\n>\n>     This is unfortunate because if submodule.recurse is set in a user's\n>     ~/.gitconfig, invoking git grep --no-index either inside or outside a\n>     Git repository results in\n>\n>     fatal: option not supported with --recurse-submodules\n>\n>     Let's allow using these options together, so that setting\n>     submodule.recurse globally does not prevent using git grep --no-index.\n>\n>     Using --recurse-submodules should not have any effect if --no-indexis\n>     used inside a repository, as Git will recurse into the checked out\n>     submodule directories just like into regular directories.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-540%2Fphil-blain%2Fgrep-no-index-ignore-recurse-submodule-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-540/phil-blain/grep-no-index-ignore-recurse-submodule-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/540\n>\n>  Documentation/git-grep.txt         |  3 ++-\n>  builtin/grep.c                     |  4 ++--\n>  t/t7814-grep-recurse-submodules.sh | 11 ++++++++++-\n>  3 files changed, 14 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index c89fb569e3..ffc3a6efdc 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -96,7 +96,8 @@ OPTIONS\n>  \tRecursively search in each submodule that has been initialized and\n>  \tchecked out in the repository.  When used in combination with the\n>  \t<tree> option the prefix of all submodule output will be the name of\n> -\tthe parent project's <tree> object.\n> +\tthe parent project's <tree> object. This option has no effect\n> +\tif `--no-index` is given.\n>\n>  -a::\n>  --text::\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 50ce8d9461..d5f089dd41 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1115,8 +1115,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\t}\n>  \t}\n>\n> -\tif (recurse_submodules && (!use_index || untracked))\n> -\t\tdie(_(\"option not supported with --recurse-submodules\"));\n> +\tif (recurse_submodules && untracked)\n> +\t\tdie(_(\"--untracked not supported with --recurse-submodules\"));\n>\n>  \tif (!show_in_pager && !opt.status_only)\n>  \t\tsetup_pager();\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index 946f91fa57..828cb3ba58 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -345,7 +345,16 @@ test_incompatible_with_recurse_submodules ()\n>  }\n>\n>  test_incompatible_with_recurse_submodules --untracked\n> -test_incompatible_with_recurse_submodules --no-index\n> +\n> +test_expect_success 'grep --recurse-submodules --no-index ignores --recurse-submodules' '\n> +\tgit grep --recurse-submodules --no-index -e \"^(.|.)[\\d]\" >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\ta:(1|2)d(3|4)\n> +\tsubmodule/a:(1|2)d(3|4)\n> +\tsubmodule/sub/a:(1|2)d(3|4)\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n>\n>  test_expect_success 'grep --recurse-submodules should pass the pattern type along' '\n>  \t# Fixed\n>\n> base-commit: bc7a3d4dc04dd719e7c8c35ebd7a6e6651c5c5b6\n> --\n> gitgitgadget\n>\n"},{"id":"390792","messageId":"pull.540.v2.git.1580391448318.gitgitgadget@gmail.com","threadId":"52697","inReplyTo":"pull.540.git.1580000298097.gitgitgadget@gmail.com","subject":"[PATCH v2] grep: ignore --recurse-submodules if --no-index is given","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-30T13:37:28Z","receivedAt":"2020-01-30T13:37:34Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince grep learned to recurse into submodules in 0281e487fd\n(grep: optionally recurse into submodules, 2016-12-16),\nusing --recurse-submodules along with --no-index makes Git\ndie().\n\nThis is unfortunate because if submodule.recurse is set in a user's\n~/.gitconfig, invoking `git grep --no-index` either inside or outside\na Git repository results in\n\n    fatal: option not supported with --recurse-submodules\n\nLet's allow using these options together, so that setting submodule.recurse\nglobally does not prevent using `git grep --no-index`.\n\nUsing `--recurse-submodules` should not have any effect if `--no-index`\nis used inside a repository, as Git will recurse into the checked out\nsubmodule directories just like into regular directories.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    grep: ignore --recurse-submodules if --no-index is given\n    \n    Changes since v1:\n    \n     * set recurse_submodules to 0 early in cmd_grep if !use_index. This\n       makes more sense, and eliminates an incompatibility with topic\n       mt/threaded-grep-in-object-store.\n    \n    v1: Since grep learned to recurse into submodules in 0281e487fd (grep:\n    optionally recurse into submodules, 2016-12-16), using\n    --recurse-submodules along with --no-index makes Git die().\n    \n    This is unfortunate because if submodule.recurse is set in a user's\n    ~/.gitconfig, invoking git grep --no-index either inside or outside a\n    Git repository results in\n    \n    fatal: option not supported with --recurse-submodules\n    \n    Let's allow using these options together, so that setting\n    submodule.recurse globally does not prevent using git grep --no-index.\n    \n    Using --recurse-submodules should not have any effect if --no-indexis\n    used inside a repository, as Git will recurse into the checked out\n    submodule directories just like into regular directories.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-540%2Fphil-blain%2Fgrep-no-index-ignore-recurse-submodule-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-540/phil-blain/grep-no-index-ignore-recurse-submodule-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/540\n\nRange-diff vs v1:\n\n 1:  6634266b01 ! 1:  6fc8bf788a grep: ignore --recurse-submodules if --no-index is given\n     @@ -20,6 +20,7 @@\n          is used inside a repository, as Git will recurse into the checked out\n          submodule directories just like into regular directories.\n      \n     +    Helped-by: Junio C Hamano <gitster@pobox.com>\n          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n      \n       diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n     @@ -39,6 +40,16 @@\n       diff --git a/builtin/grep.c b/builtin/grep.c\n       --- a/builtin/grep.c\n       +++ b/builtin/grep.c\n     +@@\n     + \t\t\t/* die the same way as if we did it at the beginning */\n     + \t\t\tsetup_git_directory();\n     + \t}\n     ++\t/* Ignore --recurse-submodules if --no-index is given or implied */\n     ++\tif (!use_index)\n     ++\t\trecurse_submodules = 0;\n     + \n     + \t/*\n     + \t * skip a -- separator; we know it cannot be\n      @@\n       \t\t}\n       \t}\n\n\n Documentation/git-grep.txt         |  3 ++-\n builtin/grep.c                     |  7 +++++--\n t/t7814-grep-recurse-submodules.sh | 11 ++++++++++-\n 3 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex c89fb569e3..ffc3a6efdc 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -96,7 +96,8 @@ OPTIONS\n \tRecursively search in each submodule that has been initialized and\n \tchecked out in the repository.  When used in combination with the\n \t<tree> option the prefix of all submodule output will be the name of\n-\tthe parent project's <tree> object.\n+\tthe parent project's <tree> object. This option has no effect\n+\tif `--no-index` is given.\n \n -a::\n --text::\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 50ce8d9461..ae2d5bbafc 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -958,6 +958,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t/* die the same way as if we did it at the beginning */\n \t\t\tsetup_git_directory();\n \t}\n+\t/* Ignore --recurse-submodules if --no-index is given or implied */\n+\tif (!use_index)\n+\t\trecurse_submodules = 0;\n \n \t/*\n \t * skip a -- separator; we know it cannot be\n@@ -1115,8 +1118,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (recurse_submodules && (!use_index || untracked))\n-\t\tdie(_(\"option not supported with --recurse-submodules\"));\n+\tif (recurse_submodules && untracked)\n+\t\tdie(_(\"--untracked not supported with --recurse-submodules\"));\n \n \tif (!show_in_pager && !opt.status_only)\n \t\tsetup_pager();\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 946f91fa57..828cb3ba58 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -345,7 +345,16 @@ test_incompatible_with_recurse_submodules ()\n }\n \n test_incompatible_with_recurse_submodules --untracked\n-test_incompatible_with_recurse_submodules --no-index\n+\n+test_expect_success 'grep --recurse-submodules --no-index ignores --recurse-submodules' '\n+\tgit grep --recurse-submodules --no-index -e \"^(.|.)[\\d]\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)d(3|4)\n+\tsubmodule/a:(1|2)d(3|4)\n+\tsubmodule/sub/a:(1|2)d(3|4)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'grep --recurse-submodules should pass the pattern type along' '\n \t# Fixed\n\nbase-commit: bc7a3d4dc04dd719e7c8c35ebd7a6e6651c5c5b6\n-- \ngitgitgadget\n"},{"id":"390822","messageId":"xmqq36bwerqo.fsf@gitster-ct.c.googlers.com","threadId":"52697","inReplyTo":"pull.540.v2.git.1580391448318.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] grep: ignore --recurse-submodules if --no-index is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-30T18:14:39Z","receivedAt":"2020-01-30T18:14:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Using `--recurse-submodules` should not have any effect if `--no-index`\n> is used inside a repository, as Git will recurse into the checked out\n> submodule directories just like into regular directories.\n> ...\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 50ce8d9461..ae2d5bbafc 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -958,6 +958,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\t\t/* die the same way as if we did it at the beginning */\n>  \t\t\tsetup_git_directory();\n>  \t}\n> +\t/* Ignore --recurse-submodules if --no-index is given or implied */\n> +\tif (!use_index)\n> +\t\trecurse_submodules = 0;\n\nThis is done quite early in the execution flow.  That makes any and\nall existing checks that says \"if recurse-submodules is set and we\nare in no-index mode, do this\" unneeded.\n\n>  \n>  \t/*\n>  \t * skip a -- separator; we know it cannot be\n\n> @@ -1115,8 +1118,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\t}\n>  \t}\n>  \n> -\tif (recurse_submodules && (!use_index || untracked))\n> -\t\tdie(_(\"option not supported with --recurse-submodules\"));\n> +\tif (recurse_submodules && untracked)\n> +\t\tdie(_(\"--untracked not supported with --recurse-submodules\"));\n\nAnd this is an example of a change to remove a check for such a\nredundant condition.  If recurse_submodules is true (which is the\nonly time the RHS of &&- is evaluated), we know use_index cannot be\nfalse, so the final outcome solely depends on the value of untracked.\n\nLooks good.  And my quick reading of the current builtin/grep.c code\nsuggests that this is the only such combination that can be simplified.\n\nThanks.\n\n>  \tif (!show_in_pager && !opt.status_only)\n>  \t\tsetup_pager();\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index 946f91fa57..828cb3ba58 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -345,7 +345,16 @@ test_incompatible_with_recurse_submodules ()\n>  }\n>  \n>  test_incompatible_with_recurse_submodules --untracked\n> -test_incompatible_with_recurse_submodules --no-index\n> +\n> +test_expect_success 'grep --recurse-submodules --no-index ignores --recurse-submodules' '\n> +\tgit grep --recurse-submodules --no-index -e \"^(.|.)[\\d]\" >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\ta:(1|2)d(3|4)\n> +\tsubmodule/a:(1|2)d(3|4)\n> +\tsubmodule/sub/a:(1|2)d(3|4)\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n>  \n>  test_expect_success 'grep --recurse-submodules should pass the pattern type along' '\n>  \t# Fixed\n>\n> base-commit: bc7a3d4dc04dd719e7c8c35ebd7a6e6651c5c5b6\n"}]}