{"thread":{"id":"52729","subject":"[PATCH] clone: use submodules.recurse option for automatically clone submodules","startedAt":"2020-01-31T21:11:37Z","lastAt":"2020-05-01T13:54:51Z","messageCount":11,"participants":["Markus Klein via GitGitGadget","Johannes Schindelin","Junio C Hamano","masmiseim@gmx.de"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"390963","messageId":"pull.695.git.git.1580505092071.gitgitgadget@gmail.com","threadId":"52729","inReplyTo":null,"subject":"[PATCH] clone: use submodules.recurse option for automatically clone submodules","fromName":"Markus Klein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-31T21:11:31Z","receivedAt":"2020-01-31T21:11:37Z","isPatch":true,"sender":{"key":"name:Markus Klein","avatar":null},"body":"From: Markus \n\nSimplify cloning repositories with submodules when the option\nsubmodules.recurse is set by the user. This makes it transparent to the\nuser if submodules are used. The user doesn’t have to know if he has to add\nan extra parameter to get the full project including the used submodules.\nThis makes clone behave identical to other commands like fetch, pull,\ncheckout, ... which include the submodules automatically if this option is\nset.\n\nIt is implemented analog to the pull command by using an own config\nfunction instead of using just the default config. In contrast to the pull\ncommand, the submodule.recurse state is saved as an array of strings as it\ncan take an optionally pathspec argument which describes which submodules\nshould be recursively initialized and cloned. To recursively initialize and\nclone all submodules a pathspec of \".\" has to be used.\nThe regression test is simplified compared to the test for \"git clone\n--recursive\" as the general functionality is already checked there.\n\nSigned-off-by: Markus Klein \n---\n Add the usage of the submodules.recurse parameter on clone\n \n I try to finish the pullrequest #573 from Maddimax. This adds the usage\n of the submodules.recurse parameter on clone\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-695%2FMasmiseim36%2Fdev%2FCloneWithSubmodule-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-695/Masmiseim36/dev/CloneWithSubmodule-v1\nPull-Request: https://github.com/git/git/pull/695\n\n builtin/clone.c | 16 +++++++++++++++-\n t/t7407-submodule-foreach.sh | 11 +++++++++++\n 2 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0fc89ae2b9..21b9d927a2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -26,6 +26,8 @@\n #include \"dir-iterator.h\"\n #include \"iterator.h\"\n #include \"sigchain.h\"\n+#include \"submodule-config.h\"\n+#include \"submodule.h\"\n #include \"branch.h\"\n #include \"remote.h\"\n #include \"run-command.h\"\n@@ -929,6 +931,18 @@ static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+/**\n+ * Read config variables.\n+ */\n+static int git_clone_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n+\t\tstring_list_append(&option_recurse_submodules, \"true\");\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -1103,7 +1117,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_clone_config, NULL);\n \n \tif (option_bare) {\n \t\tif (option_mirror)\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6b2aa917e1..44b32f7b27 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -383,6 +383,17 @@ test_expect_success 'use \"update --recursive nested1\" to checkout all submodules\n \t\tgit rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.git\n \t)\n '\n+test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n+\tgit clone -c submodule.recurse=true super clone7 &&\n+\t(\n+\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t.git\n+\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n \n test_expect_success 'command passed to foreach retains notion of stdin' '\n \t(\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n-- \ngitgitgadget\n"},{"id":"390998","messageId":"nycvar.QRO.7.76.6.2002012218410.46@tvgsbejvaqbjf.bet","threadId":"52729","inReplyTo":"pull.695.git.git.1580505092071.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: use submodules.recurse option for automatically clone submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-01T21:19:38Z","receivedAt":"2020-02-01T21:19:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Markus,\n\n\nOn Fri, 31 Jan 2020, Markus Klein via GitGitGadget wrote:\n\n> From: Markus \n\nThis line should probably match...\n\n>\n> Simplify cloning repositories with submodules when the option\n> submodules.recurse is set by the user. This makes it transparent to the\n> user if submodules are used. The user doesn’t have to know if he has to add\n> an extra parameter to get the full project including the used submodules.\n> This makes clone behave identical to other commands like fetch, pull,\n> checkout, ... which include the submodules automatically if this option is\n> set.\n>\n> It is implemented analog to the pull command by using an own config\n> function instead of using just the default config. In contrast to the pull\n> command, the submodule.recurse state is saved as an array of strings as it\n> can take an optionally pathspec argument which describes which submodules\n> should be recursively initialized and cloned. To recursively initialize and\n> clone all submodules a pathspec of \".\" has to be used.\n> The regression test is simplified compared to the test for \"git clone\n> --recursive\" as the general functionality is already checked there.\n>\n> Signed-off-by: Markus Klein \n\n... this line. I.e. you will want to run `git config --global user.name\n\"Markus Klein\"` and then `git commit --amend --reset-author`.\n\nCiao,\nJohannes\n\n> ---\n> Add the usage of the submodules.recurse parameter on clone\n>\n> I try to finish the pullrequest #573 from Maddimax. This adds the usage\n> of the submodules.recurse parameter on clone\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-695%2FMasmiseim36%2Fdev%2FCloneWithSubmodule-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-695/Masmiseim36/dev/CloneWithSubmodule-v1\n> Pull-Request: https://github.com/git/git/pull/695\n>\n> builtin/clone.c | 16 +++++++++++++++-\n> t/t7407-submodule-foreach.sh | 11 +++++++++++\n> 2 files changed, 26 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 0fc89ae2b9..21b9d927a2 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -26,6 +26,8 @@\n> #include \"dir-iterator.h\"\n> #include \"iterator.h\"\n> #include \"sigchain.h\"\n> +#include \"submodule-config.h\"\n> +#include \"submodule.h\"\n> #include \"branch.h\"\n> #include \"remote.h\"\n> #include \"run-command.h\"\n> @@ -929,6 +931,18 @@ static int path_exists(const char *path)\n> \treturn !stat(path, &sb);\n> }\n>\n> +/**\n> + * Read config variables.\n> + */\n> +static int git_clone_config(const char *var, const char *value, void *cb)\n> +{\n> +\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n> +\t\tstring_list_append(&option_recurse_submodules, \"true\");\n> +\t\treturn 0;\n> +\t}\n> +\treturn git_default_config(var, value, cb);\n> +}\n> +\n> int cmd_clone(int argc, const char **argv, const char *prefix)\n> {\n> \tint is_bundle = 0, is_local;\n> @@ -1103,7 +1117,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>\n> \twrite_config(&option_config);\n>\n> -\tgit_config(git_default_config, NULL);\n> +\tgit_config(git_clone_config, NULL);\n>\n> \tif (option_bare) {\n> \t\tif (option_mirror)\n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 6b2aa917e1..44b32f7b27 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -383,6 +383,17 @@ test_expect_success 'use \"update --recursive nested1\" to checkout all submodules\n> \t\tgit rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.git\n> \t)\n> '\n> +test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n> +\tgit clone -c submodule.recurse=true super clone7 &&\n> +\t(\n> +\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\t.git\n> +\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n> +\t\tEOF\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n>\n> test_expect_success 'command passed to foreach retains notion of stdin' '\n> \t(\n>\n> base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n> --\n> gitgitgadget\n>\n"},{"id":"391127","messageId":"pull.695.v2.git.git.1580851963616.gitgitgadget@gmail.com","threadId":"52729","inReplyTo":"pull.695.git.git.1580505092071.gitgitgadget@gmail.com","subject":"[PATCH v2] clone: use submodules.recurse option for automatically clone submodules","fromName":"Markus Klein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-04T21:32:42Z","receivedAt":"2020-02-04T21:32:49Z","isPatch":true,"sender":{"key":"name:Markus Klein","avatar":null},"body":"From: Markus Klein \n\nSimplify cloning repositories with submodules when the option\nsubmodules.recurse is set by the user. This makes it transparent to the\nuser if submodules are used. The user doesn’t have to know if he has to add\nan extra parameter to get the full project including the used submodules.\nThis makes clone behave identical to other commands like fetch, pull,\ncheckout, ... which include the submodules automatically if this option is\nset.\n\nIt is implemented analog to the pull command by using an own config\nfunction instead of using just the default config. In contrast to the pull\ncommand, the submodule.recurse state is saved as an array of strings as it\ncan take an optionally pathspec argument which describes which submodules\nshould be recursively initialized and cloned. To recursively initialize and\nclone all submodules a pathspec of \".\" has to be used.\nThe regression test is simplified compared to the test for \"git clone\n--recursive\" as the general functionality is already checked there.\n\nChanges since v1:\n* Fixed the commit author to match the Signed-off-by line\n\nSigned-off-by: Markus Klein \n---\n Add the usage of the submodules.recurse parameter on clone\n \n I try to finish the pullrequest #573 from Maddimax. This adds the usage\n of the submodules.recurse parameter on clone\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-695%2FMasmiseim36%2Fdev%2FCloneWithSubmodule-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-695/Masmiseim36/dev/CloneWithSubmodule-v2\nPull-Request: https://github.com/git/git/pull/695\n\nRange-diff vs v1:\n\n 1: 7fa8d19faf ! 1: c75835268a clone: use submodules.recurse option for automatically clone submodules\n @@ -1,4 +1,4 @@\n -Author: Markus \n +Author: Markus Klein \n \n clone: use submodules.recurse option for automatically clone submodules\n \n @@ -19,6 +19,9 @@\n The regression test is simplified compared to the test for \"git clone\n --recursive\" as the general functionality is already checked there.\n \n + Changes since v1:\n + * Fixed the commit author to match the Signed-off-by line\n +\n Signed-off-by: Markus Klein \n \n diff --git a/builtin/clone.c b/builtin/clone.c\n\n\n builtin/clone.c | 16 +++++++++++++++-\n t/t7407-submodule-foreach.sh | 11 +++++++++++\n 2 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0fc89ae2b9..21b9d927a2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -26,6 +26,8 @@\n #include \"dir-iterator.h\"\n #include \"iterator.h\"\n #include \"sigchain.h\"\n+#include \"submodule-config.h\"\n+#include \"submodule.h\"\n #include \"branch.h\"\n #include \"remote.h\"\n #include \"run-command.h\"\n@@ -929,6 +931,18 @@ static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+/**\n+ * Read config variables.\n+ */\n+static int git_clone_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n+\t\tstring_list_append(&option_recurse_submodules, \"true\");\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -1103,7 +1117,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_clone_config, NULL);\n \n \tif (option_bare) {\n \t\tif (option_mirror)\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6b2aa917e1..44b32f7b27 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -383,6 +383,17 @@ test_expect_success 'use \"update --recursive nested1\" to checkout all submodules\n \t\tgit rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.git\n \t)\n '\n+test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n+\tgit clone -c submodule.recurse=true super clone7 &&\n+\t(\n+\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t.git\n+\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n \n test_expect_success 'command passed to foreach retains notion of stdin' '\n \t(\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n-- \ngitgitgadget\n"},{"id":"391158","messageId":"nycvar.QRO.7.76.6.2002051136310.46@tvgsbejvaqbjf.bet","threadId":"52729","inReplyTo":"pull.695.v2.git.git.1580851963616.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] clone: use submodules.recurse option for automatically clone submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-05T10:37:34Z","receivedAt":"2020-02-05T10:37:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Markus,\n\nOn Tue, 4 Feb 2020, Markus Klein via GitGitGadget wrote:\n\n> From: Markus Klein \n>\n> Simplify cloning repositories with submodules when the option\n> submodules.recurse is set by the user. This makes it transparent to the\n> user if submodules are used. The user doesn’t have to know if he has to add\n> an extra parameter to get the full project including the used submodules.\n> This makes clone behave identical to other commands like fetch, pull,\n> checkout, ... which include the submodules automatically if this option is\n> set.\n>\n> It is implemented analog to the pull command by using an own config\n> function instead of using just the default config. In contrast to the pull\n> command, the submodule.recurse state is saved as an array of strings as it\n> can take an optionally pathspec argument which describes which submodules\n> should be recursively initialized and cloned. To recursively initialize and\n> clone all submodules a pathspec of \".\" has to be used.\n> The regression test is simplified compared to the test for \"git clone\n> --recursive\" as the general functionality is already checked there.\n>\n> Changes since v1:\n> * Fixed the commit author to match the Signed-off-by line\n\nThis changelog should go...\n\n>\n> Signed-off-by: Markus Klein \n> ---\n\n... after the `---`. I.e. it should go into the PR description (which is\nthe first comment on the PR) instead of the commit message.\n\nCiao,\nJohannes\n\n> Add the usage of the submodules.recurse parameter on clone\n>\n> I try to finish the pullrequest #573 from Maddimax. This adds the usage\n> of the submodules.recurse parameter on clone\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-695%2FMasmiseim36%2Fdev%2FCloneWithSubmodule-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-695/Masmiseim36/dev/CloneWithSubmodule-v2\n> Pull-Request: https://github.com/git/git/pull/695\n>\n> Range-diff vs v1:\n>\n> 1: 7fa8d19faf ! 1: c75835268a clone: use submodules.recurse option for automatically clone submodules\n> @@ -1,4 +1,4 @@\n> -Author: Markus \n> +Author: Markus Klein \n>\n> clone: use submodules.recurse option for automatically clone submodules\n>\n> @@ -19,6 +19,9 @@\n> The regression test is simplified compared to the test for \"git clone\n> --recursive\" as the general functionality is already checked there.\n>\n> + Changes since v1:\n> + * Fixed the commit author to match the Signed-off-by line\n> +\n> Signed-off-by: Markus Klein \n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n>\n>\n> builtin/clone.c | 16 +++++++++++++++-\n> t/t7407-submodule-foreach.sh | 11 +++++++++++\n> 2 files changed, 26 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 0fc89ae2b9..21b9d927a2 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -26,6 +26,8 @@\n> #include \"dir-iterator.h\"\n> #include \"iterator.h\"\n> #include \"sigchain.h\"\n> +#include \"submodule-config.h\"\n> +#include \"submodule.h\"\n> #include \"branch.h\"\n> #include \"remote.h\"\n> #include \"run-command.h\"\n> @@ -929,6 +931,18 @@ static int path_exists(const char *path)\n> \treturn !stat(path, &sb);\n> }\n>\n> +/**\n> + * Read config variables.\n> + */\n> +static int git_clone_config(const char *var, const char *value, void *cb)\n> +{\n> +\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n> +\t\tstring_list_append(&option_recurse_submodules, \"true\");\n> +\t\treturn 0;\n> +\t}\n> +\treturn git_default_config(var, value, cb);\n> +}\n> +\n> int cmd_clone(int argc, const char **argv, const char *prefix)\n> {\n> \tint is_bundle = 0, is_local;\n> @@ -1103,7 +1117,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>\n> \twrite_config(&option_config);\n>\n> -\tgit_config(git_default_config, NULL);\n> +\tgit_config(git_clone_config, NULL);\n>\n> \tif (option_bare) {\n> \t\tif (option_mirror)\n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 6b2aa917e1..44b32f7b27 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -383,6 +383,17 @@ test_expect_success 'use \"update --recursive nested1\" to checkout all submodules\n> \t\tgit rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.git\n> \t)\n> '\n> +test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n> +\tgit clone -c submodule.recurse=true super clone7 &&\n> +\t(\n> +\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\t.git\n> +\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n> +\t\tEOF\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n>\n> test_expect_success 'command passed to foreach retains notion of stdin' '\n> \t(\n>\n> base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n> --\n> gitgitgadget\n>\n"},{"id":"391261","messageId":"xmqq1rr7fsh3.fsf@gitster-ct.c.googlers.com","threadId":"52729","inReplyTo":"pull.695.v2.git.git.1580851963616.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] clone: use submodules.recurse option for automatically clone submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-06T19:03:52Z","receivedAt":"2020-02-06T19:04:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Markus Klein via GitGitGadget\" writes:\n\n> From: Markus Klein \n>\n> Simplify cloning repositories with submodules when the option\n> submodules.recurse is set by the user. This makes it transparent to the\n> user if submodules are used. The user doesn’t have to know if he has to add\n> an extra parameter to get the full project including the used submodules.\n> This makes clone behave identical to other commands like fetch, pull,\n> checkout, ... which include the submodules automatically if this option is\n> set.\n\nI am not sure if it is even a good idea to make clone behave\nidentically to fetch and pull. We cannot escape from the fact that\nthe initial cloning of the top-level superproject is a special\nevent---we do not even have a place to put the configuration\nspecific to that superproject (e.g. which submodules are good ones\nto clone by default) before that happens.\n\nYou misspelt \"submodule.recurse\" everywhere in the log message, by\nthe way, even though the code seems to react to the right variable.\n\n> It is implemented analog to the pull command by using an own config\n> function instead of using just the default config. \n\nI am not sure if this is worth saying, but it is not incorrect per-se.\n\n> In contrast to the pull\n> command, the submodule.recurse state is saved as an array of strings as it\n> can take an optionally pathspec argument which describes which submodules\n> should be recursively initialized and cloned.\n\nSorry, but I do not think I get this part at all. Your callback\nseems to add a fixed string \"true\" to option_recurse_submodules\nstring list as many times as submodule.recurse variable is defined\nin various configuration files. Does anybody count how many and\nreact differently? You mention \"pathspec\" here, but how does one\nspecify a pathspec beforehand (remember, this is clone and there is\nno superproject repository or its per-repository configuration file\nyet before we clone it)?\n\n> To recursively initialize and\n> clone all submodules a pathspec of \".\" has to be used.\n> The regression test is simplified compared to the test for \"git clone\n> --recursive\" as the general functionality is already checked there.\n\nDocumentation/config/submodule.txt says submodule.recurse says\n\n Specifies if commands recurse into submodules by default. This\n applies to all commands that have a `--recurse-submodules`\n option, except `clone`. Defaults to false.\n\nso I take that the value must be a boolean. So I am lost what\npathspec you are talking about here.\n\n> +/**\n> + * Read config variables.\n> + */\n\nThat's a fairly useless comment that does not say more than what the\nname of the function already tells us X-<.\n\n> +static int git_clone_config(const char *var, const char *value, void *cb)\n> +{\n> +\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n> +\t\tstring_list_append(&option_recurse_submodules, \"true\");\n> +\t\treturn 0;\n\nThe breakage of this is not apparent, but this is misleading. If\nsubmodule.recurse is set to a value that git_config_bool() would say\n\"false\", the if statement is skipped, and you end up calling\ngit_default_config() with \"submodule.recurse\", even though you are\nsupposed to have already dealt with the setting.\n\n\tif (!strcmp(var, \"submodule.recurse\")) {\n\t\tif (git_config_bool(var, value))\n\t\t\t...\n\t\treturn 0; /* done with the variable either way */\n\t}\n\nis more appropriate. I still do not know what this code is trying\nto do by appending \"true\" as many times as submodule.recurse appears\nin the configuration file(s), though.\n\nWhen given from the command line, i.e.\n\n\tgit clone --no-recurse-submodules ...\n\tgit clone --recurse-submodules ...\n\tgit clone --recurse-submodules= ...\n\nrecurse_submodules_cb() reacts to them by\n\n (1) clearing what have been accumulated so far,\n (2) appending the match-all \".\" pathspec, and\n (3) appending the string \n\nto option_recurse_submodules string list. But given that\nsubmodule.recurse is not (and will not be without an involved\ntransition plan) a pathspec but merely a boolean, I would think\nappending hardcoded string constant \"true\" makes little sense.\nAfter sorting the list, these values become values of the\nsubmodule.active configuration variable whose values are pathspec\nelements in cmd_clone(); see the part of the code before it makes a\ncall to init_db().\n\nSo, I would sort-of understand if you pretend --recurse-submodules\nwas given from the command line when submodule.recurse is set to\ntrue (which would mean that you'd append \".\" to the string list).\nBut I do not understand why appending \"true\" is a good thing at all\nhere.\n\nAnother thing I noticed.\n\nIf you have \"[submodule] recurse\" in your $HOME/.gitconfig, you'd\nwant to be able to countermand from the command line with\n\n git clone --no-recurse-submodules ...\n\nso that the clone would not go recursive. And that should be\ntested. \n\nYou'd also want the opposite, i.e. with \"[submodule] recurse=no\" in\nyour $HOME/.gitconfig and running\n\n git clone --recurse-submodules ...\n\nshould countermand the configuration.\n\nThanks.\n\n> +test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n> +\tgit clone -c submodule.recurse=true super clone7 &&\n> +\t(\n> +\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\t.git\n> +\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n> +\t\tEOF\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\n"},{"id":"391333","messageId":"nycvar.QRO.7.76.6.2002071621160.3718@tvgsbejvaqbjf.bet","threadId":"52729","inReplyTo":"xmqq1rr7fsh3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] clone: use submodules.recurse option for automatically clone submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-07T15:45:42Z","receivedAt":"2020-02-07T15:45:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 6 Feb 2020, Junio C Hamano wrote:\n\n> \"Markus Klein via GitGitGadget\" writes:\n>\n> > +static int git_clone_config(const char *var, const char *value, void *cb)\n> > +{\n> > +\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n> > +\t\tstring_list_append(&option_recurse_submodules, \"true\");\n> > +\t\treturn 0;\n>\n> The breakage of this is not apparent, but this is misleading. If\n> submodule.recurse is set to a value that git_config_bool() would say\n> \"false\", the if statement is skipped, and you end up calling\n> git_default_config() with \"submodule.recurse\", even though you are\n> supposed to have already dealt with the setting.\n>\n> \tif (!strcmp(var, \"submodule.recurse\")) {\n> \t\tif (git_config_bool(var, value))\n> \t\t\t...\n> \t\treturn 0; /* done with the variable either way */\n> \t}\n>\n> is more appropriate.\n\nGood catch, and I think you will have to do even more: in the \"else\" case,\nit is possible that the user overrode a `submodule.recurse` from the\nsystem config in their user-wide config, so we must _undo_ the\n`string_list_append().\n\nFurther, it is probably not a good idea to append \"true\" _twice_ if\nmultiple configs in the chain specify `submodule.recurse = true`.\n\n> I still do not know what this code is trying to do by appending \"true\"\n> as many times as submodule.recurse appears in the configuration file(s),\n> though.\n>\n> When given from the command line, i.e.\n>\n> \tgit clone --no-recurse-submodules ...\n> \tgit clone --recurse-submodules ...\n> \tgit clone --recurse-submodules= ...\n>\n> recurse_submodules_cb() reacts to them by\n>\n> (1) clearing what have been accumulated so far,\n> (2) appending the match-all \".\" pathspec, and\n> (3) appending the string\n>\n> to option_recurse_submodules string list. But given that\n> submodule.recurse is not (and will not be without an involved\n> transition plan) a pathspec but merely a boolean, I would think\n> appending hardcoded string constant \"true\" makes little sense.\n> After sorting the list, these values become values of the\n> submodule.active configuration variable whose values are pathspec\n> elements in cmd_clone(); see the part of the code before it makes a\n> call to init_db().\n\nIndeed, I think I even pointed out that \"true\" is not an appropriate value\nto use here: https://github.com/git/git/pull/695/#discussion_r367866708\n\nCiao,\nDscho\n"},{"id":"391340","messageId":"xmqqeev645zy.fsf@gitster-ct.c.googlers.com","threadId":"52729","inReplyTo":"nycvar.QRO.7.76.6.2002071621160.3718@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] clone: use submodules.recurse option for automatically clone submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-07T18:17:05Z","receivedAt":"2020-02-07T18:17:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> \tif (!strcmp(var, \"submodule.recurse\")) {\n>> \t\tif (git_config_bool(var, value))\n>> \t\t\t...\n>> \t\treturn 0; /* done with the variable either way */\n>> \t}\n>>\n>> is more appropriate.\n>\n> Good catch, and I think you will have to do even more: in the \"else\" case,\n> it is possible that the user overrode a `submodule.recurse` from the\n> system config in their user-wide config, so we must _undo_ the\n> `string_list_append().\n\nYeah, I tend to agree that submodule.recurse should not be made into\na multi-valued fields with this change; it should stay to be the\nusual last-one-wins single boolean.\n\n> Further, it is probably not a good idea to append \"true\" _twice_ if\n> multiple configs in the chain specify `submodule.recurse = true`.\n\nThe user of this list in cmd_clone() first sorts and dedups, so\nappending the same is OK, even though it may appear sloppy.\n"},{"id":"391341","messageId":"xmqqa75u457v.fsf@gitster-ct.c.googlers.com","threadId":"52729","inReplyTo":"xmqq1rr7fsh3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] clone: use submodules.recurse option for automatically clone submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-07T18:33:56Z","receivedAt":"2020-02-07T18:34:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So, I would sort-of understand if you pretend --recurse-submodules\n> was given from the command line when submodule.recurse is set to\n> true (which would mean that you'd append \".\" to the string list).\n> But I do not understand why appending \"true\" is a good thing at all\n> here.\n>\n> Another thing I noticed.\n>\n> If you have \"[submodule] recurse\" in your $HOME/.gitconfig, you'd\n> want to be able to countermand from the command line with\n>\n> git clone --no-recurse-submodules ...\n>\n> so that the clone would not go recursive. And that should be\n> tested. \n>\n> You'd also want the opposite, i.e. with \"[submodule] recurse=no\" in\n> your $HOME/.gitconfig and running\n>\n> git clone --recurse-submodules ...\n>\n> should countermand the configuration.\n\n\nTotally untested, but just to illustrate the approach, here is a\nsample patch to implement \"Pretend --recurse-submodules=. is set on\nthe command line when submodule.recurse is set (in the 'last one\nwins' sense) and there is no --recurse-submodules command line\noption.\" It should outline the right interactions between the\ncommand line options and configuration variable, like allowing \"git\nclone --no-recurse-submodules\" to defeat submodule.recurse\nconfiguration.\n\nNot that I agree that \"[submodules] recurse\" set in the\n$HOME/.gitconfig should affect \"git clone\". It is merely to\nillustrate how it could be done, if it were a good idea.\n\n builtin/clone.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0fc89ae2b9..163803d89e 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -32,6 +32,7 @@\n #include \"connected.h\"\n #include \"packfile.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"submodule.h\"\n \n /*\n * Overall FIXMEs:\n@@ -71,6 +72,8 @@ static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_NODUP;\n static int option_remote_submodules;\n \n+static int recurse_submodules_option_given;\n+\n static int recurse_submodules_cb(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -81,7 +84,7 @@ static int recurse_submodules_cb(const struct option *opt,\n \telse\n \t\tstring_list_append((struct string_list *)opt->value,\n \t\t\t\t (const char *)opt->defval);\n-\n+\trecurse_submodules_option_given = 1;\n \treturn 0;\n }\n \n@@ -929,6 +932,13 @@ static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+static int git_clone_config(const char *var, const char *value, void *cb)\n+{\n+\tif (starts_with(var, \"submodule.\"))\n+\t\treturn git_default_submodule_config(var, value, NULL);\n+\treturn git_default_config(var, value, cb);\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -1103,7 +1113,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_clone_config, NULL);\n+\tif (!recurse_submodules_option_given && should_update_submodules())\n+\t\tstring_list_append(&option_recurse_submodules, \".\");\n \n \tif (option_bare) {\n \t\tif (option_mirror)\n"},{"id":"392450","messageId":"011f01d5eb60$6fdf9890$4f9ec9b0$@gmx.de","threadId":"52729","inReplyTo":"xmqq1rr7fsh3.fsf@gitster-ct.c.googlers.com","subject":"AW: [PATCH v2] clone: use submodules.recurse option for automatically clone submodules","fromName":"","fromEmail":"masmiseim@gmx.de","sentAt":"2020-02-24T22:19:09Z","receivedAt":"2020-02-24T22:20:43Z","isPatch":true,"sender":{"key":"masmiseim@gmx.de","avatar":null},"body":"> \"Markus Klein via GitGitGadget\" writes:\n> \n> > From: Markus Klein \n> >\n> > Simplify cloning repositories with submodules when the option \n> > submodules.recurse is set by the user. This makes it transparent to \n> > the user if submodules are used. The user doesn’t have to know if he \n> > has to add an extra parameter to get the full project including the used submodules.\n> > This makes clone behave identical to other commands like fetch, pull, \n> > checkout, ... which include the submodules automatically if this \n> > option is set.\n> \n> I am not sure if it is even a good idea to make clone behave identically to fetch and pull. \n> We cannot escape from the fact that the initial cloning of the top-level superproject is a special\n> event---we do not even have a place to put the configuration specific to that superproject\n> (e.g. which submodules are good ones to clone by default) before that happens.\n\nIt behaves only identical if the option \"submodule.recurse\" is set in the global .gitconfig. So, it is optional for people who know what they do. For people which use submodules heavily, this is very useful.\n\nFor the case where you don't like to get all submodules but have this option set, you can disable it via --no-recurse-submodules\n\n> \n> You misspelt \"submodule.recurse\" everywhere in the log message, by the way, even though the code seems\n> to react to the right variable.\n> \n> > It is implemented analog to the pull command by using an own config \n> > function instead of using just the default config.\n> \n> I am not sure if this is worth saying, but it is not incorrect per-se.\n> \n> > In contrast to the pull\n> > command, the submodule.recurse state is saved as an array of strings \n> > as it can take an optionally pathspec argument which describes which \n> > submodules should be recursively initialized and cloned.\n> \n> Sorry, but I do not think I get this part at all. Your callback seems to add a fixed string \"true\"\n> to option_recurse_submodules string list as many times as submodule.recurse variable is defined in\n> various configuration files. Does anybody count how many and react differently? You mention \"pathspec\"\n> here, but how does one specify a pathspec beforehand (remember, this is clone and there is no superproject\n> repository or its per-repository configuration file yet before we clone it)?\n\nI'm so sorry for the confusing with the true. This is definitely wrong. Johannes already pointed this out to me and I had already fixed it. Shame on me, as I had uploaded an old version :-(\n\n> \n> > To recursively initialize and\n> > clone all submodules a pathspec of \".\" has to be used.\n> > The regression test is simplified compared to the test for \"git clone \n> > --recursive\" as the general functionality is already checked there.\n> \n> Documentation/config/submodule.txt says submodule.recurse says\n> \n> Specifies if commands recurse into submodules by default. This\n> applies to all commands that have a `--recurse-submodules`\n> option, except `clone`. Defaults to false.\n> \n> so I take that the value must be a boolean. So I am lost what pathspec you are talking about here.\n> \n> > +/**\n> > + * Read config variables.\n> > + */\n> \n> That's a fairly useless comment that does not say more than what the name of the function already tells us X-<.\n\nTrue, this was copy'pasted from the pull implementation. So it should be useless there also.\n\n> \n> > +static int git_clone_config(const char *var, const char *value, void \n> > +*cb) {\n> > +\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n> > +\t\tstring_list_append(&option_recurse_submodules, \"true\");\n> > +\t\treturn 0;\n> \n> The breakage of this is not apparent, but this is misleading. If submodule.recurse is set to a value\n> that git_config_bool() would say \"false\", the if statement is skipped, and you end up calling\n> git_default_config() with \"submodule.recurse\", even though you are supposed to have already dealt with\n> the setting.\n> \n> \tif (!strcmp(var, \"submodule.recurse\")) {\n> \t\tif (git_config_bool(var, value))\n> \t\t\t...\n> \t\treturn 0; /* done with the variable either way */\n> \t}\n> \n> is more appropriate. I still do not know what this code is trying to do by appending \"true\" as many\n> times as submodule.recurse appears in the configuration file(s), though.\n> \n> When given from the command line, i.e.\n> \n> \tgit clone --no-recurse-submodules ...\n> \tgit clone --recurse-submodules ...\n> \tgit clone --recurse-submodules= ...\n> \n> recurse_submodules_cb() reacts to them by\n> \n> (1) clearing what have been accumulated so far,\n> (2) appending the match-all \".\" pathspec, and\n> (3) appending the string \n> \n> to option_recurse_submodules string list. But given that submodule.recurse is not (and will not be without\n> an involved transition plan) a pathspec but merely a boolean, I would think appending hardcoded string\n> constant \"true\" makes little sense.\n> After sorting the list, these values become values of the submodule.active configuration variable whose\n> values are pathspec elements in cmd_clone(); see the part of the code before it makes a call to init_db().\n> \n> So, I would sort-of understand if you pretend --recurse-submodules was given from the command line when\n> submodule.recurse is set to true (which would mean that you'd append \".\" to the string list).\n> But I do not understand why appending \"true\" is a good thing at all here.\n> \n> Another thing I noticed.\n> \n> If you have \"[submodule] recurse\" in your $HOME/.gitconfig, you'd want to be able to countermand\n> from the command line with\n> \n> git clone --no-recurse-submodules ...\n> \n> so that the clone would not go recursive. And that should be tested. \n> \n> You'd also want the opposite, i.e. with \"[submodule] recurse=no\" in your $HOME/.gitconfig and running\n> \n> git clone --recurse-submodules ...\n> \n> should countermand the configuration.\n\nThanks for the hint. I added this tests, and it was very helpful, as it pointed out, that the disabling via --no-recurse-submodules was not working.\n\n> \n> Thanks.\n> \n> > +test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n> > +\tgit clone -c submodule.recurse=true super clone7 &&\n> > +\t(\n> > +\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\t.git\n> > +\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n> > +\t\tEOF\n> > +\t\ttest_cmp expect actual\n> > +\t)\n> > +'\n\nThanks for the feedback\n\n\n"},{"id":"392452","messageId":"pull.695.v3.git.git.1582585595108.gitgitgadget@gmail.com","threadId":"52729","inReplyTo":"pull.695.v2.git.git.1580851963616.gitgitgadget@gmail.com","subject":"[PATCH v3] clone: use submodule.recurse option for automatically clone submodules","fromName":"Markus Klein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-24T23:06:34Z","receivedAt":"2020-02-24T23:06:42Z","isPatch":true,"sender":{"key":"name:Markus Klein","avatar":null},"body":"From: Markus Klein \n\nSimplify cloning repositories with submodules when the option\nsubmodule.recurse is set by the user. This makes it transparent to the\nuser if submodules are used. The user doesn’t have to know if he has to add\nan extra parameter to get the full project including the used submodules.\nThis makes clone behave identical to other commands like fetch, pull,\ncheckout, ... which include the submodules automatically if this option is\nset.\n\nIt is implemented analog to the pull command by using an own config\nfunction instead of using just the default config. In contrast to the pull\ncommand, the submodule.recurse state is saved in addition as an array of\nstrings as it can take an optionally pathspec argument which describes\nwhich submodules should be recursively initialized and cloned. To\nrecursively initialize and clone all submodules a pathspec of \".\" has to\nbe used.\nThe regression test is simplified compared to the test for \"git clone\n--recursive\" as the general functionality is already checked there.\n\nSigned-off-by: Markus Klein \n---\n Add the usage of the submodules.recurse parameter on clone\n \n This adds the usage of the submodule.recurse parameter on clone\n \n Changes since v1:\n \n * Fixed the commit author to match the Signed-off-by line\n \n Changes since v2:\n \n * Added additional regression tests for checking the\n --no-recurse-submodules parameter\n * fixed not working --no-recurse-submodules parameter when\n submodule.recurse option is set\n * fixed invalid setting of “true” to the pathspec\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-695%2FMasmiseim36%2Fdev%2FCloneWithSubmodule-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-695/Masmiseim36/dev/CloneWithSubmodule-v3\nPull-Request: https://github.com/git/git/pull/695\n\nRange-diff vs v2:\n\n 1: c75835268a2 ! 1: f71ca264885 clone: use submodules.recurse option for automatically clone submodules\n @@ -1,9 +1,9 @@\n Author: Markus Klein \n \n - clone: use submodules.recurse option for automatically clone submodules\n + clone: use submodule.recurse option for automatically clone submodules\n \n Simplify cloning repositories with submodules when the option\n - submodules.recurse is set by the user. This makes it transparent to the\n + submodule.recurse is set by the user. This makes it transparent to the\n user if submodules are used. The user doesn’t have to know if he has to add\n an extra parameter to get the full project including the used submodules.\n This makes clone behave identical to other commands like fetch, pull,\n @@ -12,16 +12,14 @@\n \n It is implemented analog to the pull command by using an own config\n function instead of using just the default config. In contrast to the pull\n - command, the submodule.recurse state is saved as an array of strings as it\n - can take an optionally pathspec argument which describes which submodules\n - should be recursively initialized and cloned. To recursively initialize and\n - clone all submodules a pathspec of \".\" has to be used.\n + command, the submodule.recurse state is saved in addition as an array of\n + strings as it can take an optionally pathspec argument which describes\n + which submodules should be recursively initialized and cloned. To\n + recursively initialize and clone all submodules a pathspec of \".\" has to\n + be used.\n The regression test is simplified compared to the test for \"git clone\n --recursive\" as the general functionality is already checked there.\n \n - Changes since v1:\n - * Fixed the commit author to match the Signed-off-by line\n -\n Signed-off-by: Markus Klein \n \n diff --git a/builtin/clone.c b/builtin/clone.c\n @@ -36,17 +34,156 @@\n #include \"branch.h\"\n #include \"remote.h\"\n #include \"run-command.h\"\n +@@\n + \tNULL\n + };\n + \n ++struct option_submodules\n ++{\n ++\tstruct string_list option;\n ++\tint status;\n ++};\n ++\n + static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;\n + static int option_local = -1, option_no_hardlinks, option_shared;\n + static int option_no_tags;\n +@@\n + static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;\n + static int option_dissociate;\n + static int max_jobs = -1;\n +-static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;\n ++static struct option_submodules option_recurse_submodules = {STRING_LIST_INIT_NODUP, RECURSE_SUBMODULES_DEFAULT};\n + static struct list_objects_filter_options filter_options;\n + static struct string_list server_options = STRING_LIST_INIT_NODUP;\n + static int option_remote_submodules;\n +@@\n + static int recurse_submodules_cb(const struct option *opt,\n + \t\t\t\t const char *arg, int unset)\n + {\n +-\tif (unset)\n +-\t\tstring_list_clear((struct string_list *)opt->value, 0);\n +-\telse if (arg)\n +-\t\tstring_list_append((struct string_list *)opt->value, arg);\n +-\telse\n +-\t\tstring_list_append((struct string_list *)opt->value,\n +-\t\t\t\t (const char *)opt->defval);\n ++\tstruct option_submodules *list = (struct option_submodules *)(opt->value);\n ++\n ++\tif (unset) {\n ++\t\tstring_list_clear(&list->option, 0);\n ++\t\tlist->status = RECURSE_SUBMODULES_OFF;\n ++\t}\n ++\telse if (arg) {\n ++\t\tstring_list_append(&list->option, arg);\n ++\t\tlist->status = RECURSE_SUBMODULES_ON_DEMAND;\n ++\t}\n ++\telse {\n ++\t\tstring_list_append(&list->option, (const char *)opt->defval);\n ++\t\tlist->status = RECURSE_SUBMODULES_ON;\n ++\t}\n + \n + \treturn 0;\n + }\n +@@\n + static struct option builtin_clone_options[] = {\n + \tOPT__VERBOSITY(&option_verbosity),\n + \tOPT_BOOL(0, \"progress\", &option_progress,\n +-\t\t N_(\"force progress reporting\")),\n ++\t\tN_(\"force progress reporting\")),\n + \tOPT_BOOL('n', \"no-checkout\", &option_no_checkout,\n +-\t\t N_(\"don't create a checkout\")),\n ++\t\tN_(\"don't create a checkout\")),\n + \tOPT_BOOL(0, \"bare\", &option_bare, N_(\"create a bare repository\")),\n + \tOPT_HIDDEN_BOOL(0, \"naked\", &option_bare,\n + \t\t\tN_(\"create a bare repository\")),\n +@@\n + \tOPT_BOOL('l', \"local\", &option_local,\n + \t\tN_(\"to clone from a local repository\")),\n + \tOPT_BOOL(0, \"no-hardlinks\", &option_no_hardlinks,\n +-\t\t N_(\"don't use local hardlinks, always copy\")),\n ++\t\t\tN_(\"don't use local hardlinks, always copy\")),\n + \tOPT_BOOL('s', \"shared\", &option_shared,\n +-\t\t N_(\"setup as shared repository\")),\n ++\t\t\tN_(\"setup as shared repository\")),\n + \tOPT_ALIAS(0, \"recursive\", \"recurse-submodules\"),\n + \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n + \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n + \t PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\n + \tOPT_INTEGER('j', \"jobs\", &max_jobs,\n +-\t\t N_(\"number of submodules cloned in parallel\")),\n ++\t\t\tN_(\"number of submodules cloned in parallel\")),\n + \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n +-\t\t N_(\"directory from which templates will be used\")),\n ++\t\t\tN_(\"directory from which templates will be used\")),\n + \tOPT_STRING_LIST(0, \"reference\", &option_required_reference, N_(\"repo\"),\n + \t\t\tN_(\"reference repository\")),\n + \tOPT_STRING_LIST(0, \"reference-if-able\", &option_optional_reference,\n + \t\t\tN_(\"repo\"), N_(\"reference repository\")),\n + \tOPT_BOOL(0, \"dissociate\", &option_dissociate,\n +-\t\t N_(\"use --reference only while cloning\")),\n ++\t\t\tN_(\"use --reference only while cloning\")),\n + \tOPT_STRING('o', \"origin\", &option_origin, N_(\"name\"),\n +-\t\t N_(\"use instead of 'origin' to track upstream\")),\n ++\t\t\tN_(\"use instead of 'origin' to track upstream\")),\n + \tOPT_STRING('b', \"branch\", &option_branch, N_(\"branch\"),\n +-\t\t N_(\"checkout instead of the remote's HEAD\")),\n ++\t\t\tN_(\"checkout instead of the remote's HEAD\")),\n + \tOPT_STRING('u', \"upload-pack\", &option_upload_pack, N_(\"path\"),\n +-\t\t N_(\"path to git-upload-pack on the remote\")),\n ++\t\t\tN_(\"path to git-upload-pack on the remote\")),\n + \tOPT_STRING(0, \"depth\", &option_depth, N_(\"depth\"),\n +-\t\t N_(\"create a shallow clone of that depth\")),\n ++\t\t\tN_(\"create a shallow clone of that depth\")),\n + \tOPT_STRING(0, \"shallow-since\", &option_since, N_(\"time\"),\n +-\t\t N_(\"create a shallow clone since a specific time\")),\n ++\t\t\tN_(\"create a shallow clone since a specific time\")),\n + \tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"revision\"),\n + \t\t\tN_(\"deepen history of shallow clone, excluding rev\")),\n + \tOPT_BOOL(0, \"single-branch\", &option_single_branch,\n +-\t\t N_(\"clone only one branch, HEAD or --branch\")),\n ++\t\t\tN_(\"clone only one branch, HEAD or --branch\")),\n + \tOPT_BOOL(0, \"no-tags\", &option_no_tags,\n +-\t\t N_(\"don't clone any tags, and make later fetches not to follow them\")),\n ++\t\t\tN_(\"don't clone any tags, and make later fetches not to follow them\")),\n + \tOPT_BOOL(0, \"shallow-submodules\", &option_shallow_submodules,\n +-\t\t N_(\"any cloned submodules will be shallow\")),\n ++\t\t\tN_(\"any cloned submodules will be shallow\")),\n + \tOPT_STRING(0, \"separate-git-dir\", &real_git_dir, N_(\"gitdir\"),\n +-\t\t N_(\"separate git dir from working tree\")),\n ++\t\t\tN_(\"separate git dir from working tree\")),\n + \tOPT_STRING_LIST('c', \"config\", &option_config, N_(\"key=value\"),\n + \t\t\tN_(\"set config inside the new repository\")),\n + \tOPT_STRING_LIST(0, \"server-option\", &server_options,\n +@@\n + \t\t\tTRANSPORT_FAMILY_IPV6),\n + \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n + \tOPT_BOOL(0, \"remote-submodules\", &option_remote_submodules,\n +-\t\t N_(\"any cloned submodules will use their remote-tracking branch\")),\n ++\t\t\tN_(\"any cloned submodules will use their remote-tracking branch\")),\n + \tOPT_BOOL(0, \"sparse\", &option_sparse_checkout,\n +-\t\t N_(\"initialize sparse-checkout file to include only files at root\")),\n ++\t\t\tN_(\"initialize sparse-checkout file to include only files at root\")),\n + \tOPT_END()\n + };\n + \n +@@\n + \terr |= run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n + \t\t\t oid_to_hex(&oid), \"1\", NULL);\n + \n +-\tif (!err && (option_recurse_submodules.nr > 0)) {\n ++\tif (!err && (option_recurse_submodules.option.nr > 0)) {\n + \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n + \t\targv_array_pushl(&args, \"submodule\", \"update\", \"--require-init\", \"--recursive\", NULL);\n + \n @@\n \treturn !stat(path, &sb);\n }\n \n -+/**\n -+ * Read config variables.\n -+ */\n +static int git_clone_config(const char *var, const char *value, void *cb)\n +{\n -+\tif (!strcmp(var, \"submodule.recurse\") && git_config_bool(var, value)) {\n -+\t\tstring_list_append(&option_recurse_submodules, \"true\");\n ++\tif (!strcmp(var, \"submodule.recurse\")){\n ++\t\tif (git_config_bool(var, value) && option_recurse_submodules.status != RECURSE_SUBMODULES_OFF)\n ++\t\t\tstring_list_append(&option_recurse_submodules.option, \".\");\n +\t\treturn 0;\n +\t}\n +\treturn git_default_config(var, value, cb);\n @@ -56,6 +193,30 @@\n {\n \tint is_bundle = 0, is_local;\n @@\n + \t\t\tfprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n + \t}\n + \n +-\tif (option_recurse_submodules.nr > 0) {\n ++\tif (option_recurse_submodules.option.nr > 0) {\n + \t\tstruct string_list_item *item;\n + \t\tstruct strbuf sb = STRBUF_INIT;\n + \n + \t\t/* remove duplicates */\n +-\t\tstring_list_sort(&option_recurse_submodules);\n +-\t\tstring_list_remove_duplicates(&option_recurse_submodules, 0);\n ++\t\tstring_list_sort(&option_recurse_submodules.option);\n ++\t\tstring_list_remove_duplicates(&option_recurse_submodules.option, 0);\n + \n + \t\t/*\n + \t\t * NEEDSWORK: In a multi-working-tree world, this needs to be\n + \t\t * set in the per-worktree config.\n + \t\t */\n +-\t\tfor_each_string_list_item(item, &option_recurse_submodules) {\n ++\t\tfor_each_string_list_item(item, &option_recurse_submodules.option) {\n + \t\t\tstrbuf_addf(&sb, \"submodule.active=%s\",\n + \t\t\t\t item->string);\n + \t\t\tstring_list_append(&option_config,\n +@@\n \n \twrite_config(&option_config);\n \n @@ -82,6 +243,25 @@\n +\t\tEOF\n +\t\ttest_cmp expect actual\n +\t)\n ++'\n ++\n ++test_expect_success 'use \"git clone\" with submodule.recurse=false and --recurse-submodules to checkout all submodules' '\n ++\tgit clone -c submodule.recurse=false --recurse-submodules super clone8 &&\n ++\t(\n ++\t\tgit -C clone8 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n ++\t\tcat >expect <<-EOF &&\n ++\t\t.git\n ++\t\t$(pwd)/clone8/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n ++\t\tEOF\n ++\t\ttest_cmp expect actual\n ++\t)\n ++'\n ++\n ++test_expect_success 'use \"git clone\" with submodule.recurse=true and --no-recurse-submodules to checkout no submodules' '\n ++\tgit clone -c submodule.recurse=true --no-recurse-submodules super clone9 &&\n ++\t(\n ++\t\ttest_path_is_missing clone9/nested1/nested2/nested3/submodule\n ++\t)\n +'\n \n test_expect_success 'command passed to foreach retains notion of stdin' '\n\n\n builtin/clone.c | 89 +++++++++++++++++++++++-------------\n t/t7407-submodule-foreach.sh | 30 ++++++++++++\n 2 files changed, 87 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0fc89ae2b9c..dd45b6bb6a0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -26,6 +26,8 @@\n #include \"dir-iterator.h\"\n #include \"iterator.h\"\n #include \"sigchain.h\"\n+#include \"submodule-config.h\"\n+#include \"submodule.h\"\n #include \"branch.h\"\n #include \"remote.h\"\n #include \"run-command.h\"\n@@ -46,6 +48,12 @@ static const char * const builtin_clone_usage[] = {\n \tNULL\n };\n \n+struct option_submodules\n+{\n+\tstruct string_list option;\n+\tint status;\n+};\n+\n static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;\n static int option_local = -1, option_no_hardlinks, option_shared;\n static int option_no_tags;\n@@ -66,7 +74,7 @@ static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;\n static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;\n static int option_dissociate;\n static int max_jobs = -1;\n-static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;\n+static struct option_submodules option_recurse_submodules = {STRING_LIST_INIT_NODUP, RECURSE_SUBMODULES_DEFAULT};\n static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_NODUP;\n static int option_remote_submodules;\n@@ -74,13 +82,20 @@ static int option_remote_submodules;\n static int recurse_submodules_cb(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n-\tif (unset)\n-\t\tstring_list_clear((struct string_list *)opt->value, 0);\n-\telse if (arg)\n-\t\tstring_list_append((struct string_list *)opt->value, arg);\n-\telse\n-\t\tstring_list_append((struct string_list *)opt->value,\n-\t\t\t\t (const char *)opt->defval);\n+\tstruct option_submodules *list = (struct option_submodules *)(opt->value);\n+\n+\tif (unset) {\n+\t\tstring_list_clear(&list->option, 0);\n+\t\tlist->status = RECURSE_SUBMODULES_OFF;\n+\t}\n+\telse if (arg) {\n+\t\tstring_list_append(&list->option, arg);\n+\t\tlist->status = RECURSE_SUBMODULES_ON_DEMAND;\n+\t}\n+\telse {\n+\t\tstring_list_append(&list->option, (const char *)opt->defval);\n+\t\tlist->status = RECURSE_SUBMODULES_ON;\n+\t}\n \n \treturn 0;\n }\n@@ -88,9 +103,9 @@ static int recurse_submodules_cb(const struct option *opt,\n static struct option builtin_clone_options[] = {\n \tOPT__VERBOSITY(&option_verbosity),\n \tOPT_BOOL(0, \"progress\", &option_progress,\n-\t\t N_(\"force progress reporting\")),\n+\t\tN_(\"force progress reporting\")),\n \tOPT_BOOL('n', \"no-checkout\", &option_no_checkout,\n-\t\t N_(\"don't create a checkout\")),\n+\t\tN_(\"don't create a checkout\")),\n \tOPT_BOOL(0, \"bare\", &option_bare, N_(\"create a bare repository\")),\n \tOPT_HIDDEN_BOOL(0, \"naked\", &option_bare,\n \t\t\tN_(\"create a bare repository\")),\n@@ -99,43 +114,43 @@ static struct option builtin_clone_options[] = {\n \tOPT_BOOL('l', \"local\", &option_local,\n \t\tN_(\"to clone from a local repository\")),\n \tOPT_BOOL(0, \"no-hardlinks\", &option_no_hardlinks,\n-\t\t N_(\"don't use local hardlinks, always copy\")),\n+\t\t\tN_(\"don't use local hardlinks, always copy\")),\n \tOPT_BOOL('s', \"shared\", &option_shared,\n-\t\t N_(\"setup as shared repository\")),\n+\t\t\tN_(\"setup as shared repository\")),\n \tOPT_ALIAS(0, \"recursive\", \"recurse-submodules\"),\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n \t PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\n \tOPT_INTEGER('j', \"jobs\", &max_jobs,\n-\t\t N_(\"number of submodules cloned in parallel\")),\n+\t\t\tN_(\"number of submodules cloned in parallel\")),\n \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n-\t\t N_(\"directory from which templates will be used\")),\n+\t\t\tN_(\"directory from which templates will be used\")),\n \tOPT_STRING_LIST(0, \"reference\", &option_required_reference, N_(\"repo\"),\n \t\t\tN_(\"reference repository\")),\n \tOPT_STRING_LIST(0, \"reference-if-able\", &option_optional_reference,\n \t\t\tN_(\"repo\"), N_(\"reference repository\")),\n \tOPT_BOOL(0, \"dissociate\", &option_dissociate,\n-\t\t N_(\"use --reference only while cloning\")),\n+\t\t\tN_(\"use --reference only while cloning\")),\n \tOPT_STRING('o', \"origin\", &option_origin, N_(\"name\"),\n-\t\t N_(\"use instead of 'origin' to track upstream\")),\n+\t\t\tN_(\"use instead of 'origin' to track upstream\")),\n \tOPT_STRING('b', \"branch\", &option_branch, N_(\"branch\"),\n-\t\t N_(\"checkout instead of the remote's HEAD\")),\n+\t\t\tN_(\"checkout instead of the remote's HEAD\")),\n \tOPT_STRING('u', \"upload-pack\", &option_upload_pack, N_(\"path\"),\n-\t\t N_(\"path to git-upload-pack on the remote\")),\n+\t\t\tN_(\"path to git-upload-pack on the remote\")),\n \tOPT_STRING(0, \"depth\", &option_depth, N_(\"depth\"),\n-\t\t N_(\"create a shallow clone of that depth\")),\n+\t\t\tN_(\"create a shallow clone of that depth\")),\n \tOPT_STRING(0, \"shallow-since\", &option_since, N_(\"time\"),\n-\t\t N_(\"create a shallow clone since a specific time\")),\n+\t\t\tN_(\"create a shallow clone since a specific time\")),\n \tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"revision\"),\n \t\t\tN_(\"deepen history of shallow clone, excluding rev\")),\n \tOPT_BOOL(0, \"single-branch\", &option_single_branch,\n-\t\t N_(\"clone only one branch, HEAD or --branch\")),\n+\t\t\tN_(\"clone only one branch, HEAD or --branch\")),\n \tOPT_BOOL(0, \"no-tags\", &option_no_tags,\n-\t\t N_(\"don't clone any tags, and make later fetches not to follow them\")),\n+\t\t\tN_(\"don't clone any tags, and make later fetches not to follow them\")),\n \tOPT_BOOL(0, \"shallow-submodules\", &option_shallow_submodules,\n-\t\t N_(\"any cloned submodules will be shallow\")),\n+\t\t\tN_(\"any cloned submodules will be shallow\")),\n \tOPT_STRING(0, \"separate-git-dir\", &real_git_dir, N_(\"gitdir\"),\n-\t\t N_(\"separate git dir from working tree\")),\n+\t\t\tN_(\"separate git dir from working tree\")),\n \tOPT_STRING_LIST('c', \"config\", &option_config, N_(\"key=value\"),\n \t\t\tN_(\"set config inside the new repository\")),\n \tOPT_STRING_LIST(0, \"server-option\", &server_options,\n@@ -146,9 +161,9 @@ static struct option builtin_clone_options[] = {\n \t\t\tTRANSPORT_FAMILY_IPV6),\n \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \tOPT_BOOL(0, \"remote-submodules\", &option_remote_submodules,\n-\t\t N_(\"any cloned submodules will use their remote-tracking branch\")),\n+\t\t\tN_(\"any cloned submodules will use their remote-tracking branch\")),\n \tOPT_BOOL(0, \"sparse\", &option_sparse_checkout,\n-\t\t N_(\"initialize sparse-checkout file to include only files at root\")),\n+\t\t\tN_(\"initialize sparse-checkout file to include only files at root\")),\n \tOPT_END()\n };\n \n@@ -811,7 +826,7 @@ static int checkout(int submodule_progress)\n \terr |= run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n \n-\tif (!err && (option_recurse_submodules.nr > 0)) {\n+\tif (!err && (option_recurse_submodules.option.nr > 0)) {\n \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n \t\targv_array_pushl(&args, \"submodule\", \"update\", \"--require-init\", \"--recursive\", NULL);\n \n@@ -929,6 +944,16 @@ static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+static int git_clone_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"submodule.recurse\")){\n+\t\tif (git_config_bool(var, value) && option_recurse_submodules.status != RECURSE_SUBMODULES_OFF)\n+\t\t\tstring_list_append(&option_recurse_submodules.option, \".\");\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -1060,19 +1085,19 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n \t}\n \n-\tif (option_recurse_submodules.nr > 0) {\n+\tif (option_recurse_submodules.option.nr > 0) {\n \t\tstruct string_list_item *item;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* remove duplicates */\n-\t\tstring_list_sort(&option_recurse_submodules);\n-\t\tstring_list_remove_duplicates(&option_recurse_submodules, 0);\n+\t\tstring_list_sort(&option_recurse_submodules.option);\n+\t\tstring_list_remove_duplicates(&option_recurse_submodules.option, 0);\n \n \t\t/*\n \t\t * NEEDSWORK: In a multi-working-tree world, this needs to be\n \t\t * set in the per-worktree config.\n \t\t */\n-\t\tfor_each_string_list_item(item, &option_recurse_submodules) {\n+\t\tfor_each_string_list_item(item, &option_recurse_submodules.option) {\n \t\t\tstrbuf_addf(&sb, \"submodule.active=%s\",\n \t\t\t\t item->string);\n \t\t\tstring_list_append(&option_config,\n@@ -1103,7 +1128,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_clone_config, NULL);\n \n \tif (option_bare) {\n \t\tif (option_mirror)\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6b2aa917e11..e6667d8e9b0 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -383,6 +383,36 @@ test_expect_success 'use \"update --recursive nested1\" to checkout all submodules\n \t\tgit rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.git\n \t)\n '\n+test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n+\tgit clone -c submodule.recurse=true super clone7 &&\n+\t(\n+\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t.git\n+\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'use \"git clone\" with submodule.recurse=false and --recurse-submodules to checkout all submodules' '\n+\tgit clone -c submodule.recurse=false --recurse-submodules super clone8 &&\n+\t(\n+\t\tgit -C clone8 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t.git\n+\t\t$(pwd)/clone8/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'use \"git clone\" with submodule.recurse=true and --no-recurse-submodules to checkout no submodules' '\n+\tgit clone -c submodule.recurse=true --no-recurse-submodules super clone9 &&\n+\t(\n+\t\ttest_path_is_missing clone9/nested1/nested2/nested3/submodule\n+\t)\n+'\n \n test_expect_success 'command passed to foreach retains notion of stdin' '\n \t(\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n-- \ngitgitgadget\n"},{"id":"396745","messageId":"pull.695.v4.git.git.1588341280967.gitgitgadget@gmail.com","threadId":"52729","inReplyTo":"pull.695.v3.git.git.1582585595108.gitgitgadget@gmail.com","subject":"[PATCH v4] clone: use submodule.recurse option for automatically clone submodules","fromName":"Markus Klein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-01T13:54:39Z","receivedAt":"2020-05-01T13:54:51Z","isPatch":true,"sender":{"key":"name:Markus Klein","avatar":null},"body":"From: Markus Klein \n\nSimplify cloning repositories with submodules when the option\nsubmodule.recurse is set by the user. This makes it transparent to the\nuser if submodules are used. The user doesn’t have to know if he has to add\nan extra parameter to get the full project including the used submodules.\nThis makes clone behave identical to other commands like fetch, pull,\ncheckout, ... which include the submodules automatically if this option is\nset.\n\nIt is implemented analog to the pull command by using an own config\nfunction instead of using just the default config. In contrast to the pull\ncommand, the submodule.recurse state is saved in addition as an array of\nstrings as it can take an optionally pathspec argument which describes\nwhich submodules should be recursively initialized and cloned. To\nrecursively initialize and clone all submodules a pathspec of \".\" has to\nbe used.\nThe regression test is simplified compared to the test for \"git clone\n--recursive\" as the general functionality is already checked there.\n\nSigned-off-by: Markus Klein \n---\n Add the usage of the submodules.recurse parameter on clone\n \n This adds the usage of the submodule.recurse parameter on clone\n \n Changes since v1:\n \n * Fixed the commit author to match the Signed-off-by line\n \n Changes since v2:\n \n * Added additional regression tests for checking the\n --no-recurse-submodules parameter\n * fixed not working --no-recurse-submodules parameter when\n submodule.recurse option is set\n * fixed invalid setting of “true” to the pathspec\n \n Changes since v3:\n \n * Using the solution from Junio C Hamano which is tested with the\n regression tests introduced in V3\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-695%2FMasmiseim36%2Fdev%2FCloneWithSubmodule-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-695/Masmiseim36/dev/CloneWithSubmodule-v4\nPull-Request: https://github.com/git/git/pull/695\n\nRange-diff vs v3:\n\n 1: f71ca264885 ! 1: e502f370b04 clone: use submodule.recurse option for automatically clone submodules\n @@ Commit message\n \n ## builtin/clone.c ##\n @@\n - #include \"dir-iterator.h\"\n - #include \"iterator.h\"\n - #include \"sigchain.h\"\n -+#include \"submodule-config.h\"\n + #include \"connected.h\"\n + #include \"packfile.h\"\n + #include \"list-objects-filter-options.h\"\n +#include \"submodule.h\"\n - #include \"branch.h\"\n - #include \"remote.h\"\n - #include \"run-command.h\"\n -@@ builtin/clone.c: static const char * const builtin_clone_usage[] = {\n - \tNULL\n - };\n \n -+struct option_submodules\n -+{\n -+\tstruct string_list option;\n -+\tint status;\n -+};\n -+\n - static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;\n - static int option_local = -1, option_no_hardlinks, option_shared;\n - static int option_no_tags;\n -@@ builtin/clone.c: static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;\n - static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;\n - static int option_dissociate;\n - static int max_jobs = -1;\n --static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;\n -+static struct option_submodules option_recurse_submodules = {STRING_LIST_INIT_NODUP, RECURSE_SUBMODULES_DEFAULT};\n - static struct list_objects_filter_options filter_options;\n + /*\n + * Overall FIXMEs:\n +@@ builtin/clone.c: static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_NODUP;\n static int option_remote_submodules;\n -@@ builtin/clone.c: static int option_remote_submodules;\n + \n ++static int recurse_submodules_option_given;\n ++\n static int recurse_submodules_cb(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n --\tif (unset)\n --\t\tstring_list_clear((struct string_list *)opt->value, 0);\n --\telse if (arg)\n --\t\tstring_list_append((struct string_list *)opt->value, arg);\n --\telse\n --\t\tstring_list_append((struct string_list *)opt->value,\n --\t\t\t\t (const char *)opt->defval);\n -+\tstruct option_submodules *list = (struct option_submodules *)(opt->value);\n -+\n -+\tif (unset) {\n -+\t\tstring_list_clear(&list->option, 0);\n -+\t\tlist->status = RECURSE_SUBMODULES_OFF;\n -+\t}\n -+\telse if (arg) {\n -+\t\tstring_list_append(&list->option, arg);\n -+\t\tlist->status = RECURSE_SUBMODULES_ON_DEMAND;\n -+\t}\n -+\telse {\n -+\t\tstring_list_append(&list->option, (const char *)opt->defval);\n -+\t\tlist->status = RECURSE_SUBMODULES_ON;\n -+\t}\n - \n +@@ builtin/clone.c: static int recurse_submodules_cb(const struct option *opt,\n + \telse\n + \t\tstring_list_append((struct string_list *)opt->value,\n + \t\t\t\t (const char *)opt->defval);\n +-\n ++\trecurse_submodules_option_given = 1;\n \treturn 0;\n }\n -@@ builtin/clone.c: static int recurse_submodules_cb(const struct option *opt,\n - static struct option builtin_clone_options[] = {\n - \tOPT__VERBOSITY(&option_verbosity),\n - \tOPT_BOOL(0, \"progress\", &option_progress,\n --\t\t N_(\"force progress reporting\")),\n -+\t\tN_(\"force progress reporting\")),\n - \tOPT_BOOL('n', \"no-checkout\", &option_no_checkout,\n --\t\t N_(\"don't create a checkout\")),\n -+\t\tN_(\"don't create a checkout\")),\n - \tOPT_BOOL(0, \"bare\", &option_bare, N_(\"create a bare repository\")),\n - \tOPT_HIDDEN_BOOL(0, \"naked\", &option_bare,\n - \t\t\tN_(\"create a bare repository\")),\n -@@ builtin/clone.c: static struct option builtin_clone_options[] = {\n - \tOPT_BOOL('l', \"local\", &option_local,\n - \t\tN_(\"to clone from a local repository\")),\n - \tOPT_BOOL(0, \"no-hardlinks\", &option_no_hardlinks,\n --\t\t N_(\"don't use local hardlinks, always copy\")),\n -+\t\t\tN_(\"don't use local hardlinks, always copy\")),\n - \tOPT_BOOL('s', \"shared\", &option_shared,\n --\t\t N_(\"setup as shared repository\")),\n -+\t\t\tN_(\"setup as shared repository\")),\n - \tOPT_ALIAS(0, \"recursive\", \"recurse-submodules\"),\n - \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n - \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n - \t PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\n - \tOPT_INTEGER('j', \"jobs\", &max_jobs,\n --\t\t N_(\"number of submodules cloned in parallel\")),\n -+\t\t\tN_(\"number of submodules cloned in parallel\")),\n - \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n --\t\t N_(\"directory from which templates will be used\")),\n -+\t\t\tN_(\"directory from which templates will be used\")),\n - \tOPT_STRING_LIST(0, \"reference\", &option_required_reference, N_(\"repo\"),\n - \t\t\tN_(\"reference repository\")),\n - \tOPT_STRING_LIST(0, \"reference-if-able\", &option_optional_reference,\n - \t\t\tN_(\"repo\"), N_(\"reference repository\")),\n - \tOPT_BOOL(0, \"dissociate\", &option_dissociate,\n --\t\t N_(\"use --reference only while cloning\")),\n -+\t\t\tN_(\"use --reference only while cloning\")),\n - \tOPT_STRING('o', \"origin\", &option_origin, N_(\"name\"),\n --\t\t N_(\"use instead of 'origin' to track upstream\")),\n -+\t\t\tN_(\"use instead of 'origin' to track upstream\")),\n - \tOPT_STRING('b', \"branch\", &option_branch, N_(\"branch\"),\n --\t\t N_(\"checkout instead of the remote's HEAD\")),\n -+\t\t\tN_(\"checkout instead of the remote's HEAD\")),\n - \tOPT_STRING('u', \"upload-pack\", &option_upload_pack, N_(\"path\"),\n --\t\t N_(\"path to git-upload-pack on the remote\")),\n -+\t\t\tN_(\"path to git-upload-pack on the remote\")),\n - \tOPT_STRING(0, \"depth\", &option_depth, N_(\"depth\"),\n --\t\t N_(\"create a shallow clone of that depth\")),\n -+\t\t\tN_(\"create a shallow clone of that depth\")),\n - \tOPT_STRING(0, \"shallow-since\", &option_since, N_(\"time\"),\n --\t\t N_(\"create a shallow clone since a specific time\")),\n -+\t\t\tN_(\"create a shallow clone since a specific time\")),\n - \tOPT_STRING_LIST(0, \"shallow-exclude\", &option_not, N_(\"revision\"),\n - \t\t\tN_(\"deepen history of shallow clone, excluding rev\")),\n - \tOPT_BOOL(0, \"single-branch\", &option_single_branch,\n --\t\t N_(\"clone only one branch, HEAD or --branch\")),\n -+\t\t\tN_(\"clone only one branch, HEAD or --branch\")),\n - \tOPT_BOOL(0, \"no-tags\", &option_no_tags,\n --\t\t N_(\"don't clone any tags, and make later fetches not to follow them\")),\n -+\t\t\tN_(\"don't clone any tags, and make later fetches not to follow them\")),\n - \tOPT_BOOL(0, \"shallow-submodules\", &option_shallow_submodules,\n --\t\t N_(\"any cloned submodules will be shallow\")),\n -+\t\t\tN_(\"any cloned submodules will be shallow\")),\n - \tOPT_STRING(0, \"separate-git-dir\", &real_git_dir, N_(\"gitdir\"),\n --\t\t N_(\"separate git dir from working tree\")),\n -+\t\t\tN_(\"separate git dir from working tree\")),\n - \tOPT_STRING_LIST('c', \"config\", &option_config, N_(\"key=value\"),\n - \t\t\tN_(\"set config inside the new repository\")),\n - \tOPT_STRING_LIST(0, \"server-option\", &server_options,\n -@@ builtin/clone.c: static struct option builtin_clone_options[] = {\n - \t\t\tTRANSPORT_FAMILY_IPV6),\n - \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n - \tOPT_BOOL(0, \"remote-submodules\", &option_remote_submodules,\n --\t\t N_(\"any cloned submodules will use their remote-tracking branch\")),\n -+\t\t\tN_(\"any cloned submodules will use their remote-tracking branch\")),\n - \tOPT_BOOL(0, \"sparse\", &option_sparse_checkout,\n --\t\t N_(\"initialize sparse-checkout file to include only files at root\")),\n -+\t\t\tN_(\"initialize sparse-checkout file to include only files at root\")),\n - \tOPT_END()\n - };\n - \n -@@ builtin/clone.c: static int checkout(int submodule_progress)\n - \terr |= run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n - \t\t\t oid_to_hex(&oid), \"1\", NULL);\n - \n --\tif (!err && (option_recurse_submodules.nr > 0)) {\n -+\tif (!err && (option_recurse_submodules.option.nr > 0)) {\n - \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n - \t\targv_array_pushl(&args, \"submodule\", \"update\", \"--require-init\", \"--recursive\", NULL);\n \n @@ builtin/clone.c: static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n @@ builtin/clone.c: static int path_exists(const char *path)\n \n +static int git_clone_config(const char *var, const char *value, void *cb)\n +{\n -+\tif (!strcmp(var, \"submodule.recurse\")){\n -+\t\tif (git_config_bool(var, value) && option_recurse_submodules.status != RECURSE_SUBMODULES_OFF)\n -+\t\t\tstring_list_append(&option_recurse_submodules.option, \".\");\n -+\t\treturn 0;\n -+\t}\n ++\tif (starts_with(var, \"submodule.\"))\n ++\t\treturn git_default_submodule_config(var, value, NULL);\n +\treturn git_default_config(var, value, cb);\n +}\n +\n @@ builtin/clone.c: static int path_exists(const char *path)\n {\n \tint is_bundle = 0, is_local;\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n - \t\t\tfprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n - \t}\n - \n --\tif (option_recurse_submodules.nr > 0) {\n -+\tif (option_recurse_submodules.option.nr > 0) {\n - \t\tstruct string_list_item *item;\n - \t\tstruct strbuf sb = STRBUF_INIT;\n - \n - \t\t/* remove duplicates */\n --\t\tstring_list_sort(&option_recurse_submodules);\n --\t\tstring_list_remove_duplicates(&option_recurse_submodules, 0);\n -+\t\tstring_list_sort(&option_recurse_submodules.option);\n -+\t\tstring_list_remove_duplicates(&option_recurse_submodules.option, 0);\n - \n - \t\t/*\n - \t\t * NEEDSWORK: In a multi-working-tree world, this needs to be\n - \t\t * set in the per-worktree config.\n - \t\t */\n --\t\tfor_each_string_list_item(item, &option_recurse_submodules) {\n -+\t\tfor_each_string_list_item(item, &option_recurse_submodules.option) {\n - \t\t\tstrbuf_addf(&sb, \"submodule.active=%s\",\n - \t\t\t\t item->string);\n - \t\t\tstring_list_append(&option_config,\n -@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \twrite_config(&option_config);\n \n -\tgit_config(git_default_config, NULL);\n +\tgit_config(git_clone_config, NULL);\n ++\tif (!recurse_submodules_option_given && should_update_submodules())\n ++\t\tstring_list_append(&option_recurse_submodules, \".\");\n \n \tif (option_bare) {\n \t\tif (option_mirror)\n\n\n builtin/clone.c | 16 ++++++++++++++--\n t/t7407-submodule-foreach.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0fc89ae2b9c..163803d89e5 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -32,6 +32,7 @@\n #include \"connected.h\"\n #include \"packfile.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"submodule.h\"\n \n /*\n * Overall FIXMEs:\n@@ -71,6 +72,8 @@ static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_NODUP;\n static int option_remote_submodules;\n \n+static int recurse_submodules_option_given;\n+\n static int recurse_submodules_cb(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -81,7 +84,7 @@ static int recurse_submodules_cb(const struct option *opt,\n \telse\n \t\tstring_list_append((struct string_list *)opt->value,\n \t\t\t\t (const char *)opt->defval);\n-\n+\trecurse_submodules_option_given = 1;\n \treturn 0;\n }\n \n@@ -929,6 +932,13 @@ static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+static int git_clone_config(const char *var, const char *value, void *cb)\n+{\n+\tif (starts_with(var, \"submodule.\"))\n+\t\treturn git_default_submodule_config(var, value, NULL);\n+\treturn git_default_config(var, value, cb);\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -1103,7 +1113,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_clone_config, NULL);\n+\tif (!recurse_submodules_option_given && should_update_submodules())\n+\t\tstring_list_append(&option_recurse_submodules, \".\");\n \n \tif (option_bare) {\n \t\tif (option_mirror)\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6b2aa917e11..e6667d8e9b0 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -383,6 +383,36 @@ test_expect_success 'use \"update --recursive nested1\" to checkout all submodules\n \t\tgit rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.git\n \t)\n '\n+test_expect_success 'use \"git clone\" with submodule.recurse=true to checkout all submodules' '\n+\tgit clone -c submodule.recurse=true super clone7 &&\n+\t(\n+\t\tgit -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t.git\n+\t\t$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'use \"git clone\" with submodule.recurse=false and --recurse-submodules to checkout all submodules' '\n+\tgit clone -c submodule.recurse=false --recurse-submodules super clone8 &&\n+\t(\n+\t\tgit -C clone8 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\t.git\n+\t\t$(pwd)/clone8/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'use \"git clone\" with submodule.recurse=true and --no-recurse-submodules to checkout no submodules' '\n+\tgit clone -c submodule.recurse=true --no-recurse-submodules super clone9 &&\n+\t(\n+\t\ttest_path_is_missing clone9/nested1/nested2/nested3/submodule\n+\t)\n+'\n \n test_expect_success 'command passed to foreach retains notion of stdin' '\n \t(\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n-- \ngitgitgadget\n"}]}