{"thread":{"id":"45667","subject":"[PATCH 0/2] clone: record submodule alternates when not recursing","startedAt":"2017-04-11T19:46:36Z","lastAt":"2017-04-20T22:28:09Z","messageCount":9,"participants":["Stefan Beller","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"316628","messageId":"20170411194616.4963-1-sbeller@google.com","threadId":"45667","inReplyTo":null,"subject":"[PATCH 0/2] clone: record submodule alternates when not recursing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T19:46:14Z","receivedAt":"2017-04-11T19:46:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The meat is in the second patch:\n The commit 31224cbdc7 (clone: recursive and reference option triggers\n submodule alternates, 2016-08-17) argued for any further `submodule update`\n to respect the initial setup. This is not the case if you only pass\n '--reference[-if-able]' to the initial clone without instructing to\n recurse into submodules.\n \n If there are submodules however the user is likely to later run\n a 'submodule update --init' to obtain the submodules. In this case we\n also want to have the references available.\n\nThe first patch produces a nice helper function and some refactoring.\n\nThanks,\nStefan\n\nStefan Beller (2):\n submodule.c: add has_submodules to check if we have any submodules\n clone: remember references for submodules even when not recursing\n\n builtin/checkout.c | 2 +-\n builtin/clone.c | 8 +++--\n builtin/fetch.c | 2 +-\n builtin/read-tree.c | 2 +-\n builtin/submodule--helper.c | 6 ++--\n submodule.c | 78 +++++++++++++++++++++++++++++++++++----------\n submodule.h | 8 ++++-\n unpack-trees.c | 2 +-\n 8 files changed, 82 insertions(+), 26 deletions(-)\n\n-- \n2.12.2.575.gb14f27f917\n\n"},{"id":"316629","messageId":"20170411194616.4963-2-sbeller@google.com","threadId":"45667","inReplyTo":"20170411194616.4963-1-sbeller@google.com","subject":"[PATCH 1/2] submodule.c: add has_submodules to check if we have any submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T19:46:15Z","receivedAt":"2017-04-11T19:46:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When submodules are involved, it often slows down the process, as most\nsubmodule related handling is either done via a child process or by\niterating over the index finding all gitlinks.\n\nFor most commands that may interact with submodules, we need have a\nquick check if we do have any submodules at all, such that we can\nbe fast in the case when no submodules are in use. For this quick\ncheck introduce a function that checks with different heuristics if\nwe do have submodules around, checking if\n* anything related to submodules is configured,\n* absorbed git dirs for submodules are present,\n* the '.gitmodules' file exists\n* gitlinks are recorded in the index.\n\nEach heuristic has advantages and disadvantages.\nFor example in a later patch, when we first use this function in\ngit-clone, we'll just check for the existence of the '.gitmodules'\nfile, because at the time of running the clone command there will\nbe no absorbed git dirs or submodule configuration around.\n\nChecking for any configuration related to submodules would be useful\nin a later stage (after cloning) to see if the submodules are actually\nin use.\n\nChecking for absorbed git directories is good to see if the user has\nactually cloned submodules already (i.e. not just initialized them by\nconfiguring them).\n\nThe heuristic for checking the configuration requires this patch\nto have have a global state, whether the submodule config has already\nbeen read, and if there were any submodule related keys. Make\n'submodule_config' private to the submodule code, and introduce\n'load_submodule_config' that will take care of this global state.\n\nSigned-off-by: Stefan Beller \n---\n builtin/checkout.c | 2 +-\n builtin/fetch.c | 2 +-\n builtin/read-tree.c | 2 +-\n builtin/submodule--helper.c | 6 ++--\n submodule.c | 78 +++++++++++++++++++++++++++++++++++----------\n submodule.h | 8 ++++-\n unpack-trees.c | 2 +-\n 7 files changed, 76 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3ecc47bec0..cd5a2fd75a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1216,7 +1216,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n-\t\tgit_config(submodule_config, NULL);\n+\t\tload_submodule_config();\n \t\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n \t\t\tset_config_update_recurse_submodules(recurse_submodules);\n \t}\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5ad09d046..0bf3aa4458 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1342,7 +1342,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tset_config_fetch_recurse_submodules(arg);\n \t\t}\n \t\tgitmodules_config();\n-\t\tgit_config(submodule_config, NULL);\n+\t\tload_submodule_config();\n \t}\n \n \tif (all) {\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 23e212ee8c..fe2ec60a51 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -177,7 +177,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \n \tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {\n \t\tgitmodules_config();\n-\t\tgit_config(submodule_config, NULL);\n+\t\tload_submodule_config();\n \t\tset_config_update_recurse_submodules(RECURSE_SUBMODULES_ON);\n \t}\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 85aafe46a4..770a95ca14 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1015,7 +1015,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \n \t/* Overlay the parsed .gitmodules file with .git/config */\n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n+\tload_submodule_config();\n \n \tif (max_jobs < 0)\n \t\tmax_jobs = parallel_submodules();\n@@ -1058,7 +1058,7 @@ static const char *remote_submodule_branch(const char *path)\n {\n \tconst struct submodule *sub;\n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n+\tload_submodule_config();\n \n \tsub = submodule_from_path(null_sha1, path);\n \tif (!sub)\n@@ -1130,7 +1130,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t\t\t git_submodule_helper_usage, 0);\n \n \tgitmodules_config();\n-\tgit_config(submodule_config, NULL);\n+\tload_submodule_config();\n \n \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n \t\treturn 1;\ndiff --git a/submodule.c b/submodule.c\nindex c52d6634c5..da508dc257 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -24,6 +24,12 @@ static int initialized_fetch_ref_tips;\n static struct sha1_array ref_tips_before_fetch;\n static struct sha1_array ref_tips_after_fetch;\n \n+static enum {\n+\tSUBMODULE_CONFIG_NOT_READ = 0,\n+\tSUBMODULE_CONFIG_NO_CONFIG,\n+\tSUBMODULE_CONFIG_EXISTS,\n+} submodule_config_reading;\n+\n /*\n * The following flag is set if the .gitmodules file is unmerged. We then\n * disable recursion for all submodules where .git/config doesn't have a\n@@ -83,6 +89,62 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n \treturn 0;\n }\n \n+static int submodule_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n+\t\tsubmodule_config_reading = SUBMODULE_CONFIG_EXISTS;\n+\t\tparallel_jobs = git_config_int(var, value);\n+\t\tif (parallel_jobs < 0)\n+\t\t\tdie(_(\"negative values not allowed for submodule.fetchJobs\"));\n+\t\treturn 0;\n+\t} else if (starts_with(var, \"submodule.\")) {\n+\t\tsubmodule_config_reading = SUBMODULE_CONFIG_EXISTS;\n+\t\treturn parse_submodule_config_option(var, value);\n+\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n+\t\tsubmodule_config_reading = SUBMODULE_CONFIG_EXISTS;\n+\t\tconfig_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n+\t\treturn 0;\n+\t}\n+\treturn 0;\n+}\n+\n+void load_submodule_config(void)\n+{\n+\tsubmodule_config_reading = SUBMODULE_CONFIG_NO_CONFIG;\n+\tgit_config(submodule_config, NULL);\n+}\n+\n+int has_submodules(unsigned what_to_check)\n+{\n+\tif (what_to_check & SUBMODULE_CHECK_ANY_CONFIG) {\n+\t\tif (submodule_config_reading == SUBMODULE_CONFIG_NOT_READ)\n+\t\t\tload_submodule_config();\n+\t\tif (submodule_config_reading == SUBMODULE_CONFIG_EXISTS)\n+\t\t\treturn 1;\n+\t}\n+\n+\tif ((what_to_check & SUBMODULE_CHECK_ABSORBED_GIT_DIRS) &&\n+\t file_exists(git_path(\"modules\")))\n+\t\treturn 1;\n+\n+\tif ((what_to_check & SUBMODULE_CHECK_GITMODULES_IN_WT) &&\n+\t (!is_bare_repository() && file_exists(\".gitmodules\")))\n+\t\treturn 1;\n+\n+\tif (what_to_check & SUBMODULE_CHECK_GITLINKS_IN_TREE) {\n+\t\tint i;\n+\n+\t\tif (read_cache() < 0)\n+\t\t\tdie(_(\"index file corrupt\"));\n+\n+\t\tfor (i = 0; i < active_nr; i++)\n+\t\t\tif (S_ISGITLINK(active_cache[i]->ce_mode))\n+\t\t\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n /*\n * Try to remove the \"submodule.\" section from .gitmodules where the given\n * path is configured. Return 0 only if a .gitmodules file was found, a section\n@@ -152,22 +214,6 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t}\n }\n \n-int submodule_config(const char *var, const char *value, void *cb)\n-{\n-\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n-\t\tparallel_jobs = git_config_int(var, value);\n-\t\tif (parallel_jobs < 0)\n-\t\t\tdie(_(\"negative values not allowed for submodule.fetchJobs\"));\n-\t\treturn 0;\n-\t} else if (starts_with(var, \"submodule.\"))\n-\t\treturn parse_submodule_config_option(var, value);\n-\telse if (!strcmp(var, \"fetch.recursesubmodules\")) {\n-\t\tconfig_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n-\t\treturn 0;\n-\t}\n-\treturn 0;\n-}\n-\n void gitmodules_config(void)\n {\n \tconst char *work_tree = get_git_work_tree();\ndiff --git a/submodule.h b/submodule.h\nindex 8a8bc49dc9..5ec72fbb16 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -1,6 +1,12 @@\n #ifndef SUBMODULE_H\n #define SUBMODULE_H\n \n+#define SUBMODULE_CHECK_ANY_CONFIG\t\t(1<<0)\n+#define SUBMODULE_CHECK_ABSORBED_GIT_DIRS\t(1<<1)\n+#define SUBMODULE_CHECK_GITMODULES_IN_WT\t(1<<2)\n+#define SUBMODULE_CHECK_GITLINKS_IN_TREE \t(1<<3)\n+int has_submodules(unsigned what_to_check);\n+\n struct diff_options;\n struct argv_array;\n struct sha1_array;\n@@ -37,7 +43,7 @@ extern int remove_path_from_gitmodules(const char *path);\n extern void stage_updated_gitmodules(void);\n extern void set_diffopt_flags_from_submodule_config(struct diff_options *,\n \t\tconst char *path);\n-extern int submodule_config(const char *var, const char *value, void *cb);\n+extern void load_submodule_config(void);\n extern void gitmodules_config(void);\n extern void gitmodules_config_sha1(const unsigned char *commit_sha1);\n extern int is_submodule_initialized(const char *path);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8333da2cc9..85af680cc9 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -288,7 +288,7 @@ static void reload_gitmodules_file(struct index_state *index,\n \t\t\t\tsubmodule_free();\n \t\t\t\tcheckout_entry(ce, state, NULL);\n \t\t\t\tgitmodules_config();\n-\t\t\t\tgit_config(submodule_config, NULL);\n+\t\t\t\tload_submodule_config();\n \t\t\t} else\n \t\t\t\tbreak;\n \t\t}\n-- \n2.12.2.575.gb14f27f917\n\n"},{"id":"316630","messageId":"20170411194616.4963-3-sbeller@google.com","threadId":"45667","inReplyTo":"20170411194616.4963-1-sbeller@google.com","subject":"[PATCH 2/2] clone: remember references for submodules even when not recursing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T19:46:16Z","receivedAt":"2017-04-11T19:46:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The commit 31224cbdc7 (clone: recursive and reference option triggers\nsubmodule alternates, 2016-08-17) argued for any further `submodule update`\nto respect the initial setup. This is not the case if you only pass\n'--reference[-if-able]' to the initial clone without instructing to\nrecurse into submodules.\n\nIf there are submodules however the user is likely to later run\na 'submodule update --init' to obtain the submodules. In this case we\nalso want to have the references available.\n\nReported-by: Maxime Viargues \nSigned-off-by: Stefan Beller \n---\n builtin/clone.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex de85b85254..5f022e39e9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -24,6 +24,7 @@\n #include \"remote.h\"\n #include \"run-command.h\"\n #include \"connected.h\"\n+#include \"submodule.h\"\n \n /*\n * Overall FIXMEs:\n@@ -993,11 +994,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tstring_list_append(&option_config,\n \t\t\t\t\t strbuf_detach(&sb, NULL));\n \t\t}\n+\t}\n \n+\tif (option_recurse_submodules.nr ||\n+\t has_submodules(SUBMODULE_CHECK_GITMODULES_IN_WT)) {\n \t\tif (option_required_reference.nr &&\n \t\t option_optional_reference.nr)\n-\t\t\tdie(_(\"clone --recursive is not compatible with \"\n-\t\t\t \"both --reference and --reference-if-able\"));\n+\t\t\tdie(_(\"submodules are incompatible with both \"\n+\t\t\t \"--reference and --reference-if-able\"));\n \t\telse if (option_required_reference.nr) {\n \t\t\tstring_list_append(&option_config,\n \t\t\t\t\"submodule.alternateLocation=superproject\");\n-- \n2.12.2.575.gb14f27f917\n\n"},{"id":"317266","messageId":"CAGZ79kZMLVR-G1C3WL5sdo101oURuvhtnx43z=mT_u7x5XoZSw@mail.gmail.com","threadId":"45667","inReplyTo":"20170411194616.4963-1-sbeller@google.com","subject":"Re: [PATCH 0/2] clone: record submodule alternates when not recursing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-19T19:52:03Z","receivedAt":"2017-04-19T19:52:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+ Junio, Jacob\n\nOn Tue, Apr 11, 2017 at 12:46 PM, Stefan Beller wrote:\n> The meat is in the second patch:\n> The commit 31224cbdc7 (clone: recursive and reference option triggers\n> submodule alternates, 2016-08-17) argued for any further `submodule update`\n> to respect the initial setup. This is not the case if you only pass\n> '--reference[-if-able]' to the initial clone without instructing to\n> recurse into submodules.\n>\n> If there are submodules however the user is likely to later run\n> a 'submodule update --init' to obtain the submodules. In this case we\n> also want to have the references available.\n>\n> The first patch produces a nice helper function and some refactoring.\n>\n> Thanks,\n> Stefan\n>\n> Stefan Beller (2):\n> submodule.c: add has_submodules to check if we have any submodules\n> clone: remember references for submodules even when not recursing\n>\n> builtin/checkout.c | 2 +-\n> builtin/clone.c | 8 +++--\n> builtin/fetch.c | 2 +-\n> builtin/read-tree.c | 2 +-\n> builtin/submodule--helper.c | 6 ++--\n> submodule.c | 78 +++++++++++++++++++++++++++++++++++----------\n> submodule.h | 8 ++++-\n> unpack-trees.c | 2 +-\n> 8 files changed, 82 insertions(+), 26 deletions(-)\n>\n> --\n> 2.12.2.575.gb14f27f917\n>\n"},{"id":"317428","messageId":"20170420220736.GH142567@google.com","threadId":"45667","inReplyTo":"20170411194616.4963-2-sbeller@google.com","subject":"Re: [PATCH 1/2] submodule.c: add has_submodules to check if we have any submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-20T22:07:36Z","receivedAt":"2017-04-20T22:07:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/11, Stefan Beller wrote:\n> +int has_submodules(unsigned what_to_check)\n> +{\n> +\tif (what_to_check & SUBMODULE_CHECK_ANY_CONFIG) {\n> +\t\tif (submodule_config_reading == SUBMODULE_CONFIG_NOT_READ)\n> +\t\t\tload_submodule_config();\n> +\t\tif (submodule_config_reading == SUBMODULE_CONFIG_EXISTS)\n> +\t\t\treturn 1;\n> +\t}\n> +\n> +\tif ((what_to_check & SUBMODULE_CHECK_ABSORBED_GIT_DIRS) &&\n> +\t file_exists(git_path(\"modules\")))\n> +\t\treturn 1;\n> +\n> +\tif ((what_to_check & SUBMODULE_CHECK_GITMODULES_IN_WT) &&\n> +\t (!is_bare_repository() && file_exists(\".gitmodules\")))\n> +\t\treturn 1;\n> +\n> +\tif (what_to_check & SUBMODULE_CHECK_GITLINKS_IN_TREE) {\n> +\t\tint i;\n> +\n> +\t\tif (read_cache() < 0)\n> +\t\t\tdie(_(\"index file corrupt\"));\n> +\n> +\t\tfor (i = 0; i < active_nr; i++)\n> +\t\t\tif (S_ISGITLINK(active_cache[i]->ce_mode))\n> +\t\t\t\treturn 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nIt may be a good idea to rearrange these by order to correctness.\nCorrectness may not be the best way to describe it, but which is the\nstrongest indicator that there is a submodule or that a repo 'has a\nsubmodule'. That way in the future we could have a #define that is\nSUBMODULE_CHECK_ANY or ALL or something....Now that I'm thinking harder\nabout that we may not want that, and just require explicitly stating\nwhich check you want done.\n\nAnyways good looking patch, and I like the idea of consolidating the\nchecks into a single function.\n\n> diff --git a/submodule.h b/submodule.h\n> index 8a8bc49dc9..5ec72fbb16 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -1,6 +1,12 @@\n> #ifndef SUBMODULE_H\n> #define SUBMODULE_H\n> \n> +#define SUBMODULE_CHECK_ANY_CONFIG\t\t(1<<0)\n> +#define SUBMODULE_CHECK_ABSORBED_GIT_DIRS\t(1<<1)\n> +#define SUBMODULE_CHECK_GITMODULES_IN_WT\t(1<<2)\n> +#define SUBMODULE_CHECK_GITLINKS_IN_TREE \t(1<<3)\n> +int has_submodules(unsigned what_to_check);\n\n-- \nBrandon Williams\n"},{"id":"317429","messageId":"20170420221255.GI142567@google.com","threadId":"45667","inReplyTo":"20170411194616.4963-3-sbeller@google.com","subject":"Re: [PATCH 2/2] clone: remember references for submodules even when not recursing","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-20T22:12:55Z","receivedAt":"2017-04-20T22:13:05Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/11, Stefan Beller wrote:\n> The commit 31224cbdc7 (clone: recursive and reference option triggers\n> submodule alternates, 2016-08-17) argued for any further `submodule update`\n> to respect the initial setup. This is not the case if you only pass\n> '--reference[-if-able]' to the initial clone without instructing to\n> recurse into submodules.\n> \n> If there are submodules however the user is likely to later run\n> a 'submodule update --init' to obtain the submodules. In this case we\n> also want to have the references available.\n> \n\nSo the idea is to keep the references around even if the user doesn't\nwant to recurse immediately?\n\n-- \nBrandon Williams\n"},{"id":"317431","messageId":"CAGZ79kZeJp3nvjSJGy7P_dojjNbFtx3N11AOkGc-fdKkBOPFUg@mail.gmail.com","threadId":"45667","inReplyTo":"20170420220736.GH142567@google.com","subject":"Re: [PATCH 1/2] submodule.c: add has_submodules to check if we have any submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-20T22:20:48Z","receivedAt":"2017-04-20T22:20:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 20, 2017 at 3:07 PM, Brandon Williams wrote:\n> On 04/11, Stefan Beller wrote:\n>> +int has_submodules(unsigned what_to_check)\n>> +{\n>> + if (what_to_check & SUBMODULE_CHECK_ANY_CONFIG) {\n>> + if (submodule_config_reading == SUBMODULE_CONFIG_NOT_READ)\n>> + load_submodule_config();\n>> + if (submodule_config_reading == SUBMODULE_CONFIG_EXISTS)\n>> + return 1;\n>> + }\n>> +\n>> + if ((what_to_check & SUBMODULE_CHECK_ABSORBED_GIT_DIRS) &&\n>> + file_exists(git_path(\"modules\")))\n>> + return 1;\n>> +\n>> + if ((what_to_check & SUBMODULE_CHECK_GITMODULES_IN_WT) &&\n>> + (!is_bare_repository() && file_exists(\".gitmodules\")))\n>> + return 1;\n>> +\n>> + if (what_to_check & SUBMODULE_CHECK_GITLINKS_IN_TREE) {\n>> + int i;\n>> +\n>> + if (read_cache() < 0)\n>> + die(_(\"index file corrupt\"));\n>> +\n>> + for (i = 0; i < active_nr; i++)\n>> + if (S_ISGITLINK(active_cache[i]->ce_mode))\n>> + return 1;\n>> + }\n>> +\n>> + return 0;\n>> +}\n>\n> It may be a good idea to rearrange these by order to correctness.\n\nI arranged by estimated speed, i.e. from fastest to slowest:\n* The first check just returns a value from memory in the standard case\n Though the first one may take a hit in performance for the very first time\n as it may need to load the config.\n* The next two are an actual stat system call, each having a different\n 'defect'. (We may have non-absorbed submodules or non-bare repos)\n -> We could have a check for in-tree as well, not sure if that is desired.\n\n> Correctness may not be the best way to describe it, but which is the\n> strongest indicator that there is a submodule or that a repo 'has a\n> submodule'.\n\nThese indicators differ in strength for different scenarios IMO.\n(Just after clone: check for a .gitmodules file instead of a config;\nlater: rather check for a config as it is fastest and actually catches\nactive submodules; maybe we do not care about inactive submodules)\n\n> That way in the future we could have a #define that is\n> SUBMODULE_CHECK_ANY or ALL or something....Now that I'm thinking harder\n> about that we may not want that, and just require explicitly stating\n> which check you want done.\n>\n> Anyways good looking patch, and I like the idea of consolidating the\n> checks into a single function.\n\nThanks,\nStefan\n"},{"id":"317433","messageId":"20170420222510.GL142567@google.com","threadId":"45667","inReplyTo":"CAGZ79kZeJp3nvjSJGy7P_dojjNbFtx3N11AOkGc-fdKkBOPFUg@mail.gmail.com","subject":"Re: [PATCH 1/2] submodule.c: add has_submodules to check if we have any submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-20T22:25:10Z","receivedAt":"2017-04-20T22:25:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/20, Stefan Beller wrote:\n> On Thu, Apr 20, 2017 at 3:07 PM, Brandon Williams wrote:\n> > On 04/11, Stefan Beller wrote:\n> >> +int has_submodules(unsigned what_to_check)\n> >> +{\n> >> + if (what_to_check & SUBMODULE_CHECK_ANY_CONFIG) {\n> >> + if (submodule_config_reading == SUBMODULE_CONFIG_NOT_READ)\n> >> + load_submodule_config();\n> >> + if (submodule_config_reading == SUBMODULE_CONFIG_EXISTS)\n> >> + return 1;\n> >> + }\n> >> +\n> >> + if ((what_to_check & SUBMODULE_CHECK_ABSORBED_GIT_DIRS) &&\n> >> + file_exists(git_path(\"modules\")))\n> >> + return 1;\n> >> +\n> >> + if ((what_to_check & SUBMODULE_CHECK_GITMODULES_IN_WT) &&\n> >> + (!is_bare_repository() && file_exists(\".gitmodules\")))\n> >> + return 1;\n> >> +\n> >> + if (what_to_check & SUBMODULE_CHECK_GITLINKS_IN_TREE) {\n> >> + int i;\n> >> +\n> >> + if (read_cache() < 0)\n> >> + die(_(\"index file corrupt\"));\n> >> +\n> >> + for (i = 0; i < active_nr; i++)\n> >> + if (S_ISGITLINK(active_cache[i]->ce_mode))\n> >> + return 1;\n> >> + }\n> >> +\n> >> + return 0;\n> >> +}\n> >\n> > It may be a good idea to rearrange these by order to correctness.\n> \n> I arranged by estimated speed, i.e. from fastest to slowest:\n> * The first check just returns a value from memory in the standard case\n> Though the first one may take a hit in performance for the very first time\n> as it may need to load the config.\n> * The next two are an actual stat system call, each having a different\n> 'defect'. (We may have non-absorbed submodules or non-bare repos)\n> -> We could have a check for in-tree as well, not sure if that is desired.\n\nFair enough, I'm fine with the ordering then.\n\n> \n> > Correctness may not be the best way to describe it, but which is the\n> > strongest indicator that there is a submodule or that a repo 'has a\n> > submodule'.\n> \n> These indicators differ in strength for different scenarios IMO.\n> (Just after clone: check for a .gitmodules file instead of a config;\n> later: rather check for a config as it is fastest and actually catches\n> active submodules; maybe we do not care about inactive submodules)\n\nThis is why I went back on my thinking :) I realized that it really\ndepends on the scenario you are in.\n\n> \n> > That way in the future we could have a #define that is\n> > SUBMODULE_CHECK_ANY or ALL or something....Now that I'm thinking harder\n> > about that we may not want that, and just require explicitly stating\n> > which check you want done.\n> >\n> > Anyways good looking patch, and I like the idea of consolidating the\n> > checks into a single function.\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"317435","messageId":"CAGZ79kYug=5vggaU9qruKK=N7efrMm7B98xjn-hNgghZUzsAiQ@mail.gmail.com","threadId":"45667","inReplyTo":"20170420221255.GI142567@google.com","subject":"Re: [PATCH 2/2] clone: remember references for submodules even when not recursing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-20T22:28:03Z","receivedAt":"2017-04-20T22:28:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 20, 2017 at 3:12 PM, Brandon Williams wrote:\n> On 04/11, Stefan Beller wrote:\n>> The commit 31224cbdc7 (clone: recursive and reference option triggers\n>> submodule alternates, 2016-08-17) argued for any further `submodule update`\n>> to respect the initial setup. This is not the case if you only pass\n>> '--reference[-if-able]' to the initial clone without instructing to\n>> recurse into submodules.\n>>\n>> If there are submodules however the user is likely to later run\n>> a 'submodule update --init' to obtain the submodules. In this case we\n>> also want to have the references available.\n>>\n>\n> So the idea is to keep the references around even if the user doesn't\n> want to recurse immediately?\n\nYes. This patch is a bug fix response for\nhttps://public-inbox.org/git/35343b75-0aa7-3477-888b-3af5024ae7dd@serato.com/\n\nNote that this breaks the test suite\nt7407-submodule-foreach.sh\n 15: test \"update --recursive\" with a flag with spaces\nbecause the reference is recorded but not all submodules can\nbe referenced; the nested submodules are not populated.\n\nA couple of thoughts on that\n* A test for \"update --recursive\" ought to live in a test other than\n t7407-submodule-foreach.sh (Maybe in t7406-submodule-update.sh?)\n* The test checks for white space issues in path names and the breakage\n shows an unintentional side effect of --reference: it may error out in\n more cases (not all submodules populated -> error)\n* Maybe \"git submodule update\" should learn the --reference-if-able\n flag, just like git-clone did, to improve the submodule situation?\n (I put it on my todo list)\n\nThanks,\nStefan\n"}]}