{"thread":{"id":"49915","subject":"[PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","startedAt":"2018-11-29T00:28:03Z","lastAt":"2019-02-02T01:58:22Z","messageCount":21,"participants":["Stefan Beller","Jonathan Tan","Junio C Hamano","Josh Steadmon","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"364266","messageId":"20181129002756.167615-1-sbeller@google.com","threadId":"49915","inReplyTo":null,"subject":"[PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:47Z","receivedAt":"2018-11-29T00:28:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a resend of sb/submodule-recursive-fetch-gets-the-tip,\nwith all feedback addressed. As it took some time, I'll send it\nwithout range-diff, but would ask for full review.\n\nI plan on resending after the next release as this got delayed quite a bit,\nwhich is why I also rebased it to master.\n\nThanks,\nStefan\n\nPrevious round:\nhttps://public-inbox.org/git/20181016181327.107186-1-sbeller@google.com/\n\nStefan Beller (9):\n sha1-array: provide oid_array_filter\n submodule.c: fix indentation\n submodule.c: sort changed_submodule_names before searching it\n submodule.c: tighten scope of changed_submodule_names struct\n submodule: store OIDs in changed_submodule_names\n repository: repo_submodule_init to take a submodule struct\n submodule: migrate get_next_submodule to use repository structs\n submodule.c: fetch in submodules git directory instead of in worktree\n fetch: try fetching submodules if needed objects were not fetched\n\n Documentation/technical/api-oid-array.txt | 5 +\n builtin/fetch.c | 11 +-\n builtin/grep.c | 17 +-\n builtin/ls-files.c | 12 +-\n builtin/submodule--helper.c | 2 +-\n repository.c | 27 +-\n repository.h | 12 +-\n sha1-array.c | 17 ++\n sha1-array.h | 3 +\n submodule.c | 284 ++++++++++++++++---\n t/helper/test-submodule-nested-repo-config.c | 8 +-\n t/t5526-fetch-submodules.sh | 86 ++++++\n 12 files changed, 395 insertions(+), 89 deletions(-)\n\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364267","messageId":"20181129002756.167615-2-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 1/9] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:48Z","receivedAt":"2018-11-29T00:28:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Helped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/api-oid-array.txt | 5 +++++\n sha1-array.c | 17 +++++++++++++++++\n sha1-array.h | 3 +++\n 3 files changed, 25 insertions(+)\n\ndiff --git a/Documentation/technical/api-oid-array.txt b/Documentation/technical/api-oid-array.txt\nindex 9febfb1d52..c97428c2c3 100644\n--- a/Documentation/technical/api-oid-array.txt\n+++ b/Documentation/technical/api-oid-array.txt\n@@ -48,6 +48,11 @@ Functions\n \tis not sorted, this function has the side effect of sorting\n \tit.\n \n+`oid_array_filter`::\n+\tApply the callback function `want` to each entry in the array,\n+\tretaining only the entries for which the function returns true.\n+\tPreserve the order of the entries that are retained.\n+\n Examples\n --------\n \ndiff --git a/sha1-array.c b/sha1-array.c\nindex b94e0ec0f5..d922e94e3f 100644\n--- a/sha1-array.c\n+++ b/sha1-array.c\n@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,\n \t}\n \treturn 0;\n }\n+\n+void oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn want,\n+\t\t void *cb_data)\n+{\n+\tunsigned nr = array->nr, src, dst;\n+\tstruct object_id *oids = array->oid;\n+\n+\tfor (src = dst = 0; src < nr; src++) {\n+\t\tif (want(&oids[src], cb_data)) {\n+\t\t\tif (src != dst)\n+\t\t\t\toidcpy(&oids[dst], &oids[src]);\n+\t\t\tdst++;\n+\t\t}\n+\t}\n+\tarray->nr = dst;\n+}\ndiff --git a/sha1-array.h b/sha1-array.h\nindex 232bf95017..55d016c4bf 100644\n--- a/sha1-array.h\n+++ b/sha1-array.h\n@@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,\n int oid_array_for_each_unique(struct oid_array *array,\n \t\t\t for_each_oid_fn fn,\n \t\t\t void *data);\n+void oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn want,\n+\t\t void *cbdata);\n \n #endif /* SHA1_ARRAY_H */\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364268","messageId":"20181129002756.167615-3-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 2/9] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:49Z","receivedAt":"2018-11-29T00:28:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The submodule subsystem is really bad at staying within 80 characters.\nFix it while we are here.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6415cc5580..bc48ea3b68 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1271,7 +1271,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (!submodule) {\n \t\t\tconst char *name = default_name_or_path(ce->name);\n \t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = default_submodule.name = name;\n+\t\t\t\tdefault_submodule.path = name;\n+\t\t\t\tdefault_submodule.name = name;\n \t\t\t\tsubmodule = &default_submodule;\n \t\t\t}\n \t\t}\n@@ -1281,8 +1282,10 @@ static int get_next_submodule(struct child_process *cp,\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n-\t\t\t\t\t\t\t submodule->name))\n+\t\t\tif (!submodule ||\n+\t\t\t !unsorted_string_list_lookup(\n+\t\t\t\t\t&changed_submodule_names,\n+\t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364269","messageId":"20181129002756.167615-4-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 3/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:50Z","receivedAt":"2018-11-29T00:28:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We can string_list_insert() to maintain sorted-ness of the\nlist as we find new items, or we can string_list_append() to\nbuild an unsorted list and sort it at the end just once.\n\nAs we do not rely on the sortedness while building the\nlist, we pick the \"append and sort at the end\" as it\nhas better worst case execution times.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex bc48ea3b68..3c388f85cc 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1283,7 +1283,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!submodule ||\n-\t\t\t !unsorted_string_list_lookup(\n+\t\t\t !string_list_lookup(\n \t\t\t\t\t&changed_submodule_names,\n \t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n@@ -1377,6 +1377,7 @@ int fetch_populated_submodules(struct repository *r,\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n \tcalculate_changed_submodule_paths(r);\n+\tstring_list_sort(&changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n \t\t\t fetch_start_failure,\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364270","messageId":"20181129002756.167615-5-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 4/9] submodule.c: tighten scope of changed_submodule_names struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:51Z","receivedAt":"2018-11-29T00:28:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `changed_submodule_names` are only used for fetching, so let's make it\npart of the struct that is passed around for fetching submodules.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3c388f85cc..f93f0aff82 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -25,7 +25,6 @@\n #include \"commit-reach.h\"\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n@@ -1136,7 +1135,8 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \toid_array_append(&ref_tips_after_fetch, oid);\n }\n \n-static void calculate_changed_submodule_paths(struct repository *r)\n+static void calculate_changed_submodule_paths(struct repository *r,\n+\t\tstruct string_list *changed_submodule_names)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n@@ -1174,7 +1174,8 @@ static void calculate_changed_submodule_paths(struct repository *r)\n \t\t\tcontinue;\n \n \t\tif (!submodule_has_commits(r, path, commits))\n-\t\t\tstring_list_append(&changed_submodule_names, name->string);\n+\t\t\tstring_list_append(changed_submodule_names,\n+\t\t\t\t\t name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1221,8 +1222,10 @@ struct submodule_parallel_fetch {\n \tint default_option;\n \tint quiet;\n \tint result;\n+\n+\tstruct string_list changed_submodule_names;\n };\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n \n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t struct submodule_parallel_fetch *spf)\n@@ -1284,7 +1287,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!submodule ||\n \t\t\t !string_list_lookup(\n-\t\t\t\t\t&changed_submodule_names,\n+\t\t\t\t\t&spf->changed_submodule_names,\n \t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n@@ -1376,8 +1379,8 @@ int fetch_populated_submodules(struct repository *r,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-\tcalculate_changed_submodule_paths(r);\n-\tstring_list_sort(&changed_submodule_names);\n+\tcalculate_changed_submodule_paths(r, &spf.changed_submodule_names);\n+\tstring_list_sort(&spf.changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n \t\t\t fetch_start_failure,\n@@ -1386,7 +1389,7 @@ int fetch_populated_submodules(struct repository *r,\n \n \targv_array_clear(&spf.args);\n out:\n-\tstring_list_clear(&changed_submodule_names, 1);\n+\tstring_list_clear(&spf.changed_submodule_names, 1);\n \treturn spf.result;\n }\n \n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364271","messageId":"20181129002756.167615-6-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 5/9] submodule: store OIDs in changed_submodule_names","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:52Z","receivedAt":"2018-11-29T00:28:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"'calculate_changed_submodule_paths' uses a local list to compute the\nchanged submodules, and then produces the result by copying appropriate\nitems into the result list.\n\nInstead use the result list directly and prune items afterwards\nusing string_list_remove_empty_items.\n\nBy doing so we'll have access to the util pointer for longer that\ncontains the commits that we need to fetch, which will be\nuseful in a later patch.\n\nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Tan \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex f93f0aff82..0c81aca6f2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1139,8 +1139,7 @@ static void calculate_changed_submodule_paths(struct repository *r,\n \t\tstruct string_list *changed_submodule_names)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n-\tconst struct string_list_item *name;\n+\tstruct string_list_item *name;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(r, NULL, NULL))\n@@ -1157,9 +1156,9 @@ static void calculate_changed_submodule_paths(struct repository *r,\n \t * Collect all submodules (whether checked out or not) for which new\n \t * commits have been recorded upstream in \"changed_submodule_names\".\n \t */\n-\tcollect_changed_submodules(r, &changed_submodules, &argv);\n+\tcollect_changed_submodules(r, changed_submodule_names, &argv);\n \n-\tfor_each_string_list_item(name, &changed_submodules) {\n+\tfor_each_string_list_item(name, changed_submodule_names) {\n \t\tstruct oid_array *commits = name->util;\n \t\tconst struct submodule *submodule;\n \t\tconst char *path = NULL;\n@@ -1173,12 +1172,14 @@ static void calculate_changed_submodule_paths(struct repository *r,\n \t\tif (!path)\n \t\t\tcontinue;\n \n-\t\tif (!submodule_has_commits(r, path, commits))\n-\t\t\tstring_list_append(changed_submodule_names,\n-\t\t\t\t\t name->string);\n+\t\tif (submodule_has_commits(r, path, commits)) {\n+\t\t\toid_array_clear(commits);\n+\t\t\t*name->string = '\\0';\n+\t\t}\n \t}\n \n-\tfree_submodules_oids(&changed_submodules);\n+\tstring_list_remove_empty_items(changed_submodule_names, 1);\n+\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n \toid_array_clear(&ref_tips_after_fetch);\n@@ -1389,7 +1390,7 @@ int fetch_populated_submodules(struct repository *r,\n \n \targv_array_clear(&spf.args);\n out:\n-\tstring_list_clear(&spf.changed_submodule_names, 1);\n+\tfree_submodules_oids(&spf.changed_submodule_names);\n \treturn spf.result;\n }\n \n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364272","messageId":"20181129002756.167615-7-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 6/9] repository: repo_submodule_init to take a submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:53Z","receivedAt":"2018-11-29T00:28:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When constructing a struct repository for a submodule for some revision\nof the superproject where the submodule is not contained in the index,\nit may not be present in the working tree currently either. In that\nsituation giving a 'path' argument is not useful. Upgrade the\nrepo_submodule_init function to take a struct submodule instead.\nThe submodule struct can be obtained via submodule_from_{path, name} or\nan artificial submodule struct can be passed in.\n\nWhile we are at it, rename the repository struct in the repo_submodule_init\nfunction, which is to be initialized, to a name that is not confused with\nthe struct submodule as easily. Perform such renames in similar functions\nas well.\n\nAlso move its documentation into the header file.\n\nReviewed-by: Jonathan Tan \nSigned-off-by: Stefan Beller \n---\n builtin/grep.c | 17 +++++++-----\n builtin/ls-files.c | 12 +++++----\n builtin/submodule--helper.c | 2 +-\n repository.c | 27 ++++++++------------\n repository.h | 12 +++++++--\n t/helper/test-submodule-nested-repo-config.c | 8 +++---\n 6 files changed, 43 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 71df52a333..d6bd887b2d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -404,7 +404,10 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t\t\t const struct object_id *oid,\n \t\t\t const char *filename, const char *path)\n {\n-\tstruct repository submodule;\n+\tstruct repository subrepo;\n+\tconst struct submodule *sub = submodule_from_path(superproject,\n+\t\t\t\t\t\t\t &null_oid, path);\n+\n \tint hit;\n \n \t/*\n@@ -420,12 +423,12 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t\treturn 0;\n \t}\n \n-\tif (repo_submodule_init(&submodule, superproject, path)) {\n+\tif (repo_submodule_init(&subrepo, superproject, sub)) {\n \t\tgrep_read_unlock();\n \t\treturn 0;\n \t}\n \n-\trepo_read_gitmodules(&submodule);\n+\trepo_read_gitmodules(&subrepo);\n \n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\n@@ -437,7 +440,7 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_to_alternates_memory(submodule.objects->objectdir);\n+\tadd_to_alternates_memory(subrepo.objects->objectdir);\n \tgrep_read_unlock();\n \n \tif (oid) {\n@@ -462,14 +465,14 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \n \t\tinit_tree_desc(&tree, data, size);\n \t\thit = grep_tree(opt, pathspec, &tree, &base, base.len,\n-\t\t\t\tobject->type == OBJ_COMMIT, &submodule);\n+\t\t\t\tobject->type == OBJ_COMMIT, &subrepo);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-\t\thit = grep_cache(opt, &submodule, pathspec, 1);\n+\t\thit = grep_cache(opt, &subrepo, pathspec, 1);\n \t}\n \n-\trepo_clear(&submodule);\n+\trepo_clear(&subrepo);\n \treturn hit;\n }\n \ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex c70a9c7158..583a0e1ca2 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -206,17 +206,19 @@ static void show_files(struct repository *repo, struct dir_struct *dir);\n static void show_submodule(struct repository *superproject,\n \t\t\t struct dir_struct *dir, const char *path)\n {\n-\tstruct repository submodule;\n+\tstruct repository subrepo;\n+\tconst struct submodule *sub = submodule_from_path(superproject,\n+\t\t\t\t\t\t\t &null_oid, path);\n \n-\tif (repo_submodule_init(&submodule, superproject, path))\n+\tif (repo_submodule_init(&subrepo, superproject, sub))\n \t\treturn;\n \n-\tif (repo_read_index(&submodule) < 0)\n+\tif (repo_read_index(&subrepo) < 0)\n \t\tdie(\"index file corrupt\");\n \n-\tshow_files(&submodule, dir);\n+\tshow_files(&subrepo, dir);\n \n-\trepo_clear(&submodule);\n+\trepo_clear(&subrepo);\n }\n \n static void show_ce(struct repository *repo, struct dir_struct *dir,\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d38113a31a..4eceb8f040 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2053,7 +2053,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \tif (!sub)\n \t\tBUG(\"We could get the submodule handle before?\");\n \n-\tif (repo_submodule_init(&subrepo, the_repository, path))\n+\tif (repo_submodule_init(&subrepo, the_repository, sub))\n \t\tdie(_(\"could not get a repository handle for submodule '%s'\"), path);\n \n \tif (!repo_config_get_string(&subrepo, \"core.worktree\", &cw)) {\ndiff --git a/repository.c b/repository.c\nindex 5dd1486718..aabe64ee5d 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -166,30 +166,23 @@ int repo_init(struct repository *repo,\n \treturn -1;\n }\n \n-/*\n- * Initialize 'submodule' as the submodule given by 'path' in parent repository\n- * 'superproject'.\n- * Return 0 upon success and a non-zero value upon failure.\n- */\n-int repo_submodule_init(struct repository *submodule,\n+int repo_submodule_init(struct repository *subrepo,\n \t\t\tstruct repository *superproject,\n-\t\t\tconst char *path)\n+\t\t\tconst struct submodule *sub)\n {\n-\tconst struct submodule *sub;\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstruct strbuf worktree = STRBUF_INIT;\n \tint ret = 0;\n \n-\tsub = submodule_from_path(superproject, &null_oid, path);\n \tif (!sub) {\n \t\tret = -1;\n \t\tgoto out;\n \t}\n \n-\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", path);\n-\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", path);\n+\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", sub->path);\n+\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", sub->path);\n \n-\tif (repo_init(submodule, gitdir.buf, worktree.buf)) {\n+\tif (repo_init(subrepo, gitdir.buf, worktree.buf)) {\n \t\t/*\n \t\t * If initilization fails then it may be due to the submodule\n \t\t * not being populated in the superproject's worktree. Instead\n@@ -201,16 +194,16 @@ int repo_submodule_init(struct repository *submodule,\n \t\tstrbuf_repo_git_path(&gitdir, superproject,\n \t\t\t\t \"modules/%s\", sub->name);\n \n-\t\tif (repo_init(submodule, gitdir.buf, NULL)) {\n+\t\tif (repo_init(subrepo, gitdir.buf, NULL)) {\n \t\t\tret = -1;\n \t\t\tgoto out;\n \t\t}\n \t}\n \n-\tsubmodule->submodule_prefix = xstrfmt(\"%s%s/\",\n-\t\t\t\t\t superproject->submodule_prefix ?\n-\t\t\t\t\t superproject->submodule_prefix :\n-\t\t\t\t\t \"\", path);\n+\tsubrepo->submodule_prefix = xstrfmt(\"%s%s/\",\n+\t\t\t\t\t superproject->submodule_prefix ?\n+\t\t\t\t\t superproject->submodule_prefix :\n+\t\t\t\t\t \"\", sub->path);\n \n out:\n \tstrbuf_release(&gitdir);\ndiff --git a/repository.h b/repository.h\nindex 9f16c42c1e..0e482b7d49 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -116,9 +116,17 @@ void repo_set_worktree(struct repository *repo, const char *path);\n void repo_set_hash_algo(struct repository *repo, int algo);\n void initialize_the_repository(void);\n int repo_init(struct repository *r, const char *gitdir, const char *worktree);\n-int repo_submodule_init(struct repository *submodule,\n+\n+/*\n+ * Initialize the repository 'subrepo' as the submodule given by the\n+ * struct submodule 'sub' in parent repository 'superproject'.\n+ * Return 0 upon success and a non-zero value upon failure, which may happen\n+ * if the submodule is not found, or 'sub' is NULL.\n+ */\n+struct submodule;\n+int repo_submodule_init(struct repository *subrepo,\n \t\t\tstruct repository *superproject,\n-\t\t\tconst char *path);\n+\t\t\tconst struct submodule *sub);\n void repo_clear(struct repository *repo);\n \n /*\ndiff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c\nindex a31e2a9bea..bc97929bbc 100644\n--- a/t/helper/test-submodule-nested-repo-config.c\n+++ b/t/helper/test-submodule-nested-repo-config.c\n@@ -10,19 +10,21 @@ static void die_usage(int argc, const char **argv, const char *msg)\n \n int cmd__submodule_nested_repo_config(int argc, const char **argv)\n {\n-\tstruct repository submodule;\n+\tstruct repository subrepo;\n+\tconst struct submodule *sub;\n \n \tif (argc < 3)\n \t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n \n \tsetup_git_directory();\n \n-\tif (repo_submodule_init(&submodule, the_repository, argv[1])) {\n+\tsub = submodule_from_path(the_repository, &null_oid, argv[1]);\n+\tif (repo_submodule_init(&subrepo, the_repository, sub)) {\n \t\tdie_usage(argc, argv, \"Submodule not found.\");\n \t}\n \n \t/* Read the config of _child_ submodules. */\n-\tprint_config_from_gitmodules(&submodule, argv[2]);\n+\tprint_config_from_gitmodules(&subrepo, argv[2]);\n \n \tsubmodule_free(the_repository);\n \n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364273","messageId":"20181129002756.167615-8-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 7/9] submodule: migrate get_next_submodule to use repository structs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:54Z","receivedAt":"2018-11-29T00:28:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We used to recurse into submodules, even if they were broken having\nonly an objects directory. The child process executed in the submodule\nwould fail though if the submodule was broken. This is tested via\n\"fetching submodule into a broken repository\" in t5526.\n\nThis patch tightens the check upfront, such that we do not need\nto spawn a child process to find out if the submodule is broken.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 56 +++++++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 44 insertions(+), 12 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0c81aca6f2..77ace5e784 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1253,6 +1253,30 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+static struct repository *get_submodule_repo_for(struct repository *r,\n+\t\t\t\t\t\t const struct submodule *sub)\n+{\n+\tstruct repository *ret = xmalloc(sizeof(*ret));\n+\n+\tif (repo_submodule_init(ret, r, sub)) {\n+\t\t/*\n+\t\t * No entry in .gitmodules? Technically not a submodule,\n+\t\t * but historically we supported repositories that happen to be\n+\t\t * in-place where a gitlink is. Keep supporting them.\n+\t\t */\n+\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", sub->path);\n+\t\tif (repo_init(ret, gitdir.buf, NULL)) {\n+\t\t\tstrbuf_release(&gitdir);\n+\t\t\tfree(ret);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tstrbuf_release(&gitdir);\n+\t}\n+\n+\treturn ret;\n+}\n+\n static int get_next_submodule(struct child_process *cp,\n \t\t\t struct strbuf *err, void *data, void **task_cb)\n {\n@@ -1260,12 +1284,11 @@ static int get_next_submodule(struct child_process *cp,\n \tstruct submodule_parallel_fetch *spf = data;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n-\t\tstruct strbuf submodule_path = STRBUF_INIT;\n-\t\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n-\t\tconst char *git_dir, *default_argv;\n+\t\tconst char *default_argv;\n \t\tconst struct submodule *submodule;\n+\t\tstruct repository *repo;\n \t\tstruct submodule default_submodule = SUBMODULE_INIT;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n@@ -1300,15 +1323,11 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tstrbuf_repo_worktree_path(&submodule_path, spf->r, \"%s\", ce->name);\n-\t\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", submodule_path.buf);\n \t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n-\t\tgit_dir = read_gitfile(submodule_git_dir.buf);\n-\t\tif (!git_dir)\n-\t\t\tgit_dir = submodule_git_dir.buf;\n-\t\tif (is_directory(git_dir)) {\n+\t\trepo = get_submodule_repo_for(spf->r, submodule);\n+\t\tif (repo) {\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n+\t\t\tcp->dir = xstrdup(repo->worktree);\n \t\t\tprepare_submodule_repo_env(&cp->env_array);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n@@ -1319,10 +1338,23 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\n+\t\t\trepo_clear(repo);\n+\t\t\tfree(repo);\n \t\t\tret = 1;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * An empty directory is normal,\n+\t\t\t * the submodule is not initialized\n+\t\t\t */\n+\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n+\t\t\t !is_empty_dir(ce->name)) {\n+\t\t\t\tspf->result = 1;\n+\t\t\t\tstrbuf_addf(err,\n+\t\t\t\t\t _(\"Could not access submodule '%s'\"),\n+\t\t\t\t\t ce->name);\n+\t\t\t}\n \t\t}\n-\t\tstrbuf_release(&submodule_path);\n-\t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364274","messageId":"20181129002756.167615-9-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 8/9] submodule.c: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:55Z","receivedAt":"2018-11-29T00:28:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Keep the properties introduced in 10f5c52656 (submodule: avoid\nauto-discovery in prepare_submodule_repo_env(), 2016-09-01), by fixating\nthe git directory of the submodule.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 77ace5e784..d1b6646f42 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -494,6 +494,12 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)\n+{\n+\tprepare_submodule_repo_env_no_git_dir(out);\n+\targv_array_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n+}\n+\n /* Helper function to display the submodule header line prior to the full\n * summary output. If it can locate the submodule objects directory it will\n * attempt to lookup both the left and right commits and put them into the\n@@ -1327,8 +1333,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\trepo = get_submodule_repo_for(spf->r, submodule);\n \t\tif (repo) {\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = xstrdup(repo->worktree);\n-\t\t\tprepare_submodule_repo_env(&cp->env_array);\n+\t\t\tcp->dir = xstrdup(repo->gitdir);\n+\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364275","messageId":"20181129002756.167615-10-sbeller@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"[PATCH 9/9] fetch: try fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:27:56Z","receivedAt":"2018-11-29T00:28:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently when git-fetch is asked to recurse into submodules, it dispatches\na plain \"git-fetch -C \" (with some submodule related options\nsuch as prefix and recusing strategy, but) without any information of the\nremote or the tip that should be fetched.\n\nBut this default fetch is not sufficient, as a newly fetched commit in\nthe superproject could point to a commit in the submodule that is not\nin the default refspec. This is common in workflows like Gerrit's.\nWhen fetching a Gerrit change under review (from refs/changes/??), the\ncommits in that change likely point to submodule commits that have not\nbeen merged to a branch yet.\n\nTry fetching a submodule by object id if the object id that the\nsuperproject points to, cannot be found.\n\nbuiltin/fetch used to only inspect submodules when they were fetched\n\"on-demand\", as in either on/off case it was clear whether the submodule\nneeds to be fetched. However to know whether we need to try fetching the\nobject ids, we need to identify the object names, which is done in this\nfunction check_for_new_submodule_commits(), so we'll also run that code\nin case the submodule recursion is set to \"on\".\n\nThe submodule checks were done only when a ref in the superproject\nchanged, these checks were extended to also be performed when fetching\ninto FETCH_HEAD for completeness, and add a test for that too.\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 11 +-\n submodule.c | 206 +++++++++++++++++++++++++++++++-----\n t/t5526-fetch-submodules.sh | 86 +++++++++++++++\n 3 files changed, 265 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e0140327aa..91f9b7d9c8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -763,9 +763,6 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(msg, ref, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n@@ -779,9 +776,6 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"fast-forward\", ref, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n@@ -794,9 +788,6 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"forced-update\", ref, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n@@ -892,6 +883,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tref->force = rm->peer_ref->force;\n \t\t\t}\n \n+\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n+\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n \n \t\t\tif (!strcmp(rm->name, \"HEAD\")) {\n \t\t\t\tkind = \"\";\ndiff --git a/submodule.c b/submodule.c\nindex d1b6646f42..1ce944a737 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1231,8 +1231,14 @@ struct submodule_parallel_fetch {\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n+\n+\t/* The submodules to fetch in */\n+\tstruct fetch_task **oid_fetch_tasks;\n+\tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n };\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n+\t\t STRING_LIST_INIT_DUP, \\\n+\t\t NULL, 0, 0}\n \n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t struct submodule_parallel_fetch *spf)\n@@ -1259,6 +1265,73 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+struct fetch_task {\n+\tstruct repository *repo;\n+\tconst struct submodule *sub;\n+\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n+\n+\t/* fetch specific oids if set, otherwise fetch default refspec */\n+\tstruct oid_array *commits;\n+};\n+\n+/**\n+ * When a submodule is not defined in .gitmodules, we cannot access it\n+ * via the regular submodule-config. Create a fake submodule, which we can\n+ * work on.\n+ */\n+static const struct submodule *get_non_gitmodules_submodule(const char *path)\n+{\n+\tstruct submodule *ret = NULL;\n+\tconst char *name = default_name_or_path(path);\n+\n+\tif (!name)\n+\t\treturn NULL;\n+\n+\tret = xmalloc(sizeof(*ret));\n+\tmemset(ret, 0, sizeof(*ret));\n+\tret->path = name;\n+\tret->name = name;\n+\n+\treturn (const struct submodule *) ret;\n+}\n+\n+static struct fetch_task *fetch_task_create(struct repository *r,\n+\t\t\t\t\t const char *path)\n+{\n+\tstruct fetch_task *task = xmalloc(sizeof(*task));\n+\tmemset(task, 0, sizeof(*task));\n+\n+\ttask->sub = submodule_from_path(r, &null_oid, path);\n+\tif (!task->sub) {\n+\t\t/*\n+\t\t * No entry in .gitmodules? Technically not a submodule,\n+\t\t * but historically we supported repositories that happen to be\n+\t\t * in-place where a gitlink is. Keep supporting them.\n+\t\t */\n+\t\ttask->sub = get_non_gitmodules_submodule(path);\n+\t\tif (!task->sub) {\n+\t\t\tfree(task);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\ttask->free_sub = 1;\n+\t}\n+\n+\treturn task;\n+}\n+\n+static void fetch_task_release(struct fetch_task *p)\n+{\n+\tif (p->free_sub)\n+\t\tfree((void*)p->sub);\n+\tp->free_sub = 0;\n+\tp->sub = NULL;\n+\n+\tif (p->repo)\n+\t\trepo_clear(p->repo);\n+\tFREE_AND_NULL(p->repo);\n+}\n+\n static struct repository *get_submodule_repo_for(struct repository *r,\n \t\t\t\t\t\t const struct submodule *sub)\n {\n@@ -1286,39 +1359,32 @@ static struct repository *get_submodule_repo_for(struct repository *r,\n static int get_next_submodule(struct child_process *cp,\n \t\t\t struct strbuf *err, void *data, void **task_cb)\n {\n-\tint ret = 0;\n \tstruct submodule_parallel_fetch *spf = data;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n-\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tint recurse_config;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n \t\tconst char *default_argv;\n-\t\tconst struct submodule *submodule;\n-\t\tstruct repository *repo;\n-\t\tstruct submodule default_submodule = SUBMODULE_INIT;\n+\t\tstruct fetch_task *task;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n-\t\tsubmodule = submodule_from_path(spf->r, &null_oid, ce->name);\n-\t\tif (!submodule) {\n-\t\t\tconst char *name = default_name_or_path(ce->name);\n-\t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = name;\n-\t\t\t\tdefault_submodule.name = name;\n-\t\t\t\tsubmodule = &default_submodule;\n-\t\t\t}\n-\t\t}\n+\t\ttask = fetch_task_create(spf->r, ce->name);\n+\t\tif (!task)\n+\t\t\tcontinue;\n+\n+\t\trecurse_config = get_fetch_recurse_config(task->sub, spf);\n \n-\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\tswitch (recurse_config)\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule ||\n+\t\t\tif (!task->sub ||\n \t\t\t !string_list_lookup(\n \t\t\t\t\t&spf->changed_submodule_names,\n-\t\t\t\t\tsubmodule->name))\n+\t\t\t\t\ttask->sub->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\n@@ -1329,11 +1395,11 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n-\t\trepo = get_submodule_repo_for(spf->r, submodule);\n-\t\tif (repo) {\n+\t\ttask->repo = get_submodule_repo_for(spf->r, task->sub);\n+\t\tif (task->repo) {\n+\t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = xstrdup(repo->gitdir);\n+\t\t\tcp->dir = task->repo->gitdir;\n \t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n@@ -1343,12 +1409,22 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_pushv(&cp->args, spf->args.argv);\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\n+\t\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\",\n+\t\t\t\t\t\t spf->prefix,\n+\t\t\t\t\t\t task->sub->path);\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n \n-\t\t\trepo_clear(repo);\n-\t\t\tfree(repo);\n-\t\t\tret = 1;\n+\t\t\tspf->count++;\n+\t\t\t*task_cb = task;\n+\n+\t\t\tstrbuf_release(&submodule_prefix);\n+\t\t\treturn 1;\n \t\t} else {\n+\n+\t\t\tfetch_task_release(task);\n+\t\t\tfree(task);\n+\n \t\t\t/*\n \t\t\t * An empty directory is normal,\n \t\t\t * the submodule is not initialized\n@@ -1361,12 +1437,38 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t\t\t ce->name);\n \t\t\t}\n \t\t}\n+\t}\n+\n+\tif (spf->oid_fetch_tasks_nr) {\n+\t\tstruct fetch_task *task =\n+\t\t\tspf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tspf->oid_fetch_tasks_nr--;\n+\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\",\n+\t\t\t spf->prefix, task->sub->path);\n+\n+\t\tchild_process_init(cp);\n+\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tcp->git_cmd = 1;\n+\t\tcp->dir = task->repo->gitdir;\n+\n+\t\targv_array_init(&cp->args);\n+\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\targv_array_push(&cp->args, \"on-demand\");\n+\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\n+\t\t/* NEEDSWORK: have get_default_remote from submodule--helper */\n+\t\targv_array_push(&cp->args, \"origin\");\n+\t\toid_array_for_each_unique(task->commits,\n+\t\t\t\t\t append_oid_to_argv, &cp->args);\n+\n+\t\t*task_cb = task;\n \t\tstrbuf_release(&submodule_prefix);\n-\t\tif (ret) {\n-\t\t\tspf->count++;\n-\t\t\treturn 1;\n-\t\t}\n+\t\treturn 1;\n \t}\n+\n \treturn 0;\n }\n \n@@ -1374,20 +1476,66 @@ static int fetch_start_failure(struct strbuf *err,\n \t\t\t void *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct fetch_task *task = task_cb;\n \n \tspf->result = 1;\n \n+\tfetch_task_release(task);\n \treturn 0;\n }\n \n+static int commit_exists_in_sub(const struct object_id *oid, void *data)\n+{\n+\tstruct repository *subrepo = data;\n+\n+\tenum object_type type = oid_object_info(subrepo, oid, NULL);\n+\n+\treturn type != OBJ_COMMIT;\n+}\n+\n static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t\tvoid *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct fetch_task *task = task_cb;\n+\n+\tstruct string_list_item *it;\n+\tstruct oid_array *commits;\n \n \tif (retvalue)\n \t\tspf->result = 1;\n \n+\tif (!task || !task->sub)\n+\t\tBUG(\"callback cookie bogus\");\n+\n+\t/* Is this the second time we process this submodule? */\n+\tif (task->commits)\n+\t\treturn 0;\n+\n+\tit = string_list_lookup(&spf->changed_submodule_names, task->sub->name);\n+\tif (!it)\n+\t\t/* Could be an unchanged submodule, not contained in the list */\n+\t\tgoto out;\n+\n+\tcommits = it->util;\n+\toid_array_filter(commits,\n+\t\t\t commit_exists_in_sub,\n+\t\t\t task->repo);\n+\n+\t/* Are there commits we want, but do not exist? */\n+\tif (commits->nr) {\n+\t\ttask->commits = commits;\n+\t\tALLOC_GROW(spf->oid_fetch_tasks,\n+\t\t\t spf->oid_fetch_tasks_nr + 1,\n+\t\t\t spf->oid_fetch_tasks_alloc);\n+\t\tspf->oid_fetch_tasks[spf->oid_fetch_tasks_nr] = task;\n+\t\tspf->oid_fetch_tasks_nr++;\n+\t\treturn 0;\n+\t}\n+\n+out:\n+\tfetch_task_release(task);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 6c2f9b2ba2..8a016272bc 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -600,4 +600,90 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"fetch new submodule commits on-demand outside standard refspec\" '\n+\t# add a second submodule and ensure it is around in downstream first\n+\tgit clone submodule sub1 &&\n+\tgit submodule add ./sub1 &&\n+\tgit commit -m \"adding a second submodule\" &&\n+\tgit -C downstream pull &&\n+\tgit -C downstream submodule update --init --recursive &&\n+\n+\tgit checkout --detach &&\n+\n+\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/1 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/2 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/3 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/3:refs/heads/my_branch &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success 'fetch new submodule commits on-demand in FETCH_HEAD' '\n+\t# depends on the previous test for setup\n+\n+\tC=$(git -C submodule commit-tree -m \"another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/4 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/5 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/6 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/6 &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success 'fetch new submodule commits on-demand without .gitmodules entry' '\n+\t# depends on the previous test for setup\n+\n+\tgit config -f .gitmodules --remove-section submodule.sub1 &&\n+\tgit add .gitmodules &&\n+\tgit commit -m \"delete gitmodules file\" &&\n+\tgit checkout -B master &&\n+\tgit -C downstream fetch &&\n+\tgit -C downstream checkout origin/master &&\n+\n+\tC=$(git -C submodule commit-tree -m \"yet another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/7 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"yet another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/8 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/9 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/9 &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364584","messageId":"20181205001246.71899-1-jonathantanmy@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-4-sbeller@google.com","subject":"Re: [PATCH 3/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-05T00:12:46Z","receivedAt":"2018-12-05T00:12:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> We can string_list_insert() to maintain sorted-ness of the\n> list as we find new items, or we can string_list_append() to\n> build an unsorted list and sort it at the end just once.\n> \n> As we do not rely on the sortedness while building the\n> list, we pick the \"append and sort at the end\" as it\n> has better worst case execution times.\n\nI would write this entire commit message as:\n\n Instead of using unsorted_string_list_lookup(), sort\n changed_submodule_names before performing any lookups so that we can\n use the faster string_list_lookup() instead.\n\nThe code in this patch is fine, and patches 1-2 are fine too.\n"},{"id":"364585","messageId":"20181205001736.72764-1-jonathantanmy@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-8-sbeller@google.com","subject":"Re: [PATCH 7/9] submodule: migrate get_next_submodule to use repository structs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-05T00:17:36Z","receivedAt":"2018-12-05T00:17:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> We used to recurse into submodules, even if they were broken having\n> only an objects directory. The child process executed in the submodule\n> would fail though if the submodule was broken. This is tested via\n> \"fetching submodule into a broken repository\" in t5526.\n> \n> This patch tightens the check upfront, such that we do not need\n> to spawn a child process to find out if the submodule is broken.\n\nThanks, patches 4-7 look good to me - I see that you have addressed all\nmy comments. Not sending one email each for patches 4, 5, and 6 -\nalthough I have commented on all of them, my comments were minor.\n\nMy more in-depth review was done on a previous version [1], and I see\nthat my comments have been addressed. Also, Stefan says [2] (and implements\nin this patch):\n\n> > > If the working tree directory is empty for that submodule, it means\n> > > it is likely not initialized. But why would we use that as a signal to\n> > > skip the submodule?\n> >\n> > What I meant was: if empty, skip it completely. Otherwise, do the\n> > repo_submodule_init() and repo_init() thing, and if they both fail, set\n> > spf->result to 1, preserving existing behavior.\n> \n> I did it the other way round:\n> \n> If repo_[submodule_]init fails, see if we have a gitlink in tree and\n> an empty dir in the FS, to decide if we need to signal failure.\n\nThis works too.\n\n[1] https://public-inbox.org/git/20181017225811.66554-1-jonathantanmy@google.com/\n[2] https://public-inbox.org/git/CAGZ79kbNXD35ZwevjLZcrGsT=2hNcUPmVUWvP1RjsKSH0Gd3ww@mail.gmail.com/\n"},{"id":"364586","messageId":"20181205003825.79274-1-jonathantanmy@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-9-sbeller@google.com","subject":"Re: [PATCH 8/9] submodule.c: fetch in submodules git directory instead of in worktree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-05T00:38:25Z","receivedAt":"2018-12-05T00:38:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Keep the properties introduced in 10f5c52656 (submodule: avoid\n> auto-discovery in prepare_submodule_repo_env(), 2016-09-01), by fixating\n> the git directory of the submodule.\n\nThis is to avoid the autodetection of the Git repository, making it less\nerror-prone; looks good to me.\n"},{"id":"364587","messageId":"20181205010704.84790-1-jonathantanmy@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-10-sbeller@google.com","subject":"Re: [PATCH 9/9] fetch: try fetching submodules if needed objects were not fetched","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-05T01:07:04Z","receivedAt":"2018-12-05T01:07:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Try fetching a submodule by object id if the object id that the\n> superproject points to, cannot be found.\n\nMention here the consequences of what happens when this attempt to fetch\nfails. Also, this seems to be a case of \"do or do not, there is no try\"\n- maybe it's better to say \"Fetch commits by ID from the submodule's\norigin if the submodule doesn't already contain the commit that the\nsuperproject references\" (note that there is no \"Try\" word, since the\nconsequence is to fail the entire command).\n\nAlso mention that this fetch is always from the origin.\n\n> builtin/fetch used to only inspect submodules when they were fetched\n> \"on-demand\", as in either on/off case it was clear whether the submodule\n> needs to be fetched. However to know whether we need to try fetching the\n> object ids, we need to identify the object names, which is done in this\n> function check_for_new_submodule_commits(), so we'll also run that code\n> in case the submodule recursion is set to \"on\".\n> \n> The submodule checks were done only when a ref in the superproject\n> changed, these checks were extended to also be performed when fetching\n> into FETCH_HEAD for completeness, and add a test for that too.\n\n\"inspect submodules\" and \"submodule checks\" are unnecessarily vague to\nme - might be better to just say \"A list of new submodule commits are\nalready generated in certain conditions (by\ncheck_for_new_submodule_commits()); this new feature invokes that\nfunction in more situations\".\n\n> -\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n> -\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n> -\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n> \t\tr = s_update_ref(msg, ref, 0);\n> \t\tformat_display(display, r ? '!' : '*', what,\n> \t\t\t r ? _(\"unable to update local ref\") : NULL,\n> @@ -779,9 +776,6 @@ static int update_local_ref(struct ref *ref,\n> \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n> \t\tstrbuf_addstr(&quickref, \"..\");\n> \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n> -\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n> -\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n> -\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n> \t\tr = s_update_ref(\"fast-forward\", ref, 1);\n> \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n> \t\t\t r ? _(\"unable to update local ref\") : NULL,\n> @@ -794,9 +788,6 @@ static int update_local_ref(struct ref *ref,\n> \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n> \t\tstrbuf_addstr(&quickref, \"...\");\n> \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n> -\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n> -\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n> -\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n> \t\tr = s_update_ref(\"forced-update\", ref, 1);\n> \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n> \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n> @@ -892,6 +883,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\t\tref->force = rm->peer_ref->force;\n> \t\t\t}\n> \n> +\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n> +\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n\nAs discussed above, indeed, check_for_new_submodule_commits() is now\ninvoked in more situations (not just when there is a local ref, and also\nwhen recurse_submodules is _ON).\n\n> @@ -1231,8 +1231,14 @@ struct submodule_parallel_fetch {\n> \tint result;\n> \n> \tstruct string_list changed_submodule_names;\n> +\n> +\t/* The submodules to fetch in */\n> +\tstruct fetch_task **oid_fetch_tasks;\n> +\tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n> };\n\nBetter to document as \"Pending fetches by OIDs\", I think. (These are not\nfetches by default refspec, and are not already in progress.)\n\n> +struct fetch_task {\n> +\tstruct repository *repo;\n> +\tconst struct submodule *sub;\n> +\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n> +\n> +\t/* fetch specific oids if set, otherwise fetch default refspec */\n> +\tstruct oid_array *commits;\n> +};\n\nI would document this as \"Fetch in progress (if callback data) or\npending (if in oid_fetch_tasks in struct submodule_parallel_fetch)\".\nThis potential confusion is why I wanted 2 separate types, as I wrote in\n[1].\n\n[1] https://public-inbox.org/git/20181026204106.132296-1-jonathantanmy@google.com/\n\n> +/**\n> + * When a submodule is not defined in .gitmodules, we cannot access it\n> + * via the regular submodule-config. Create a fake submodule, which we can\n> + * work on.\n> + */\n> +static const struct submodule *get_non_gitmodules_submodule(const char *path)\n\nThanks, this is a good explanation.\n\n> static int get_next_submodule(struct child_process *cp,\n> \t\t\t struct strbuf *err, void *data, void **task_cb)\n> {\n> -\tint ret = 0;\n> \tstruct submodule_parallel_fetch *spf = data;\n> \n> \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n> -\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n> +\t\tint recurse_config;\n\nUnnecessary local variable recurse_config, but that's not a big deal.\n\n[snip the part where we use a heap-allocated task instead of a few\nvariables on the stack]\n\n> -\t\t\trepo_clear(repo);\n> -\t\t\tfree(repo);\n> -\t\t\tret = 1;\n> +\t\t\tspf->count++;\n> +\t\t\t*task_cb = task;\n\nAnd here we see why we need the heap-allocated task - it needs to go\ninto the callback data.\n\n> +\tif (spf->oid_fetch_tasks_nr) {\n> +\t\tstruct fetch_task *task =\n> +\t\t\tspf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];\n> +\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n> +\t\tspf->oid_fetch_tasks_nr--;\n> +\n> +\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\",\n> +\t\t\t spf->prefix, task->sub->path);\n> +\n> +\t\tchild_process_init(cp);\n> +\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> +\t\tcp->git_cmd = 1;\n> +\t\tcp->dir = task->repo->gitdir;\n> +\n> +\t\targv_array_init(&cp->args);\n> +\t\targv_array_pushv(&cp->args, spf->args.argv);\n> +\t\targv_array_push(&cp->args, \"on-demand\");\n> +\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n> +\t\targv_array_push(&cp->args, submodule_prefix.buf);\n> +\n> +\t\t/* NEEDSWORK: have get_default_remote from submodule--helper */\n> +\t\targv_array_push(&cp->args, \"origin\");\n> +\t\toid_array_for_each_unique(task->commits,\n> +\t\t\t\t\t append_oid_to_argv, &cp->args);\n> +\n> +\t\t*task_cb = task;\n> \t\tstrbuf_release(&submodule_prefix);\n> -\t\tif (ret) {\n> -\t\t\tspf->count++;\n> -\t\t\treturn 1;\n> -\t\t}\n> +\t\treturn 1;\n> \t}\n\nAnd if we ran out of submodules but have pending fetch-by-OID tasks, we\nexecute them.\n\n> +static int commit_exists_in_sub(const struct object_id *oid, void *data)\n> +{\n> +\tstruct repository *subrepo = data;\n> +\n> +\tenum object_type type = oid_object_info(subrepo, oid, NULL);\n> +\n> +\treturn type != OBJ_COMMIT;\n> +}\n\nShould be commit_missing_in_sub.\n\n> +\t/* Is this the second time we process this submodule? */\n> +\tif (task->commits)\n> +\t\treturn 0;\n\nShould be goto out, to clean up properly?\n\n> +test_expect_success \"fetch new submodule commits on-demand outside standard refspec\" '\n> +\t# add a second submodule and ensure it is around in downstream first\n> +\tgit clone submodule sub1 &&\n> +\tgit submodule add ./sub1 &&\n> +\tgit commit -m \"adding a second submodule\" &&\n> +\tgit -C downstream pull &&\n> +\tgit -C downstream submodule update --init --recursive &&\n> +\n> +\tgit checkout --detach &&\n> +\n> +\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n> +\tgit -C submodule update-ref refs/changes/1 $C &&\n> +\tgit update-index --cacheinfo 160000 $C submodule &&\n> +\ttest_tick &&\n> +\n> +\tD=$(git -C sub1 commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n> +\tgit -C sub1 update-ref refs/changes/2 $D &&\n> +\tgit update-index --cacheinfo 160000 $D sub1 &&\n> +\n> +\tgit commit -m \"updated submodules outside of refs/heads\" &&\n> +\tE=$(git rev-parse HEAD) &&\n> +\tgit update-ref refs/changes/3 $E &&\n> +\t(\n> +\t\tcd downstream &&\n> +\t\tgit fetch --recurse-submodules origin refs/changes/3:refs/heads/my_branch &&\n> +\t\tgit -C submodule cat-file -t $C &&\n> +\t\tgit -C sub1 cat-file -t $D &&\n> +\t\tgit checkout --recurse-submodules FETCH_HEAD\n> +\t)\n> +'\n\nIt would be nicer if all these tests started from scratch (that is, an\nempty repository), but they are understandable - thanks.\n\nIn addition to the tests here, I would like a test that checks that\nsubmodule commits pointed to by interior superproject commits also work.\nFor example:\n\n submodule:\n B C\n \\ /\n A\n\n superproject:\n\n current HEAD -> a commit -> the ref we're fetching\n gitlink=A gitlink=B gitlink=C\n\nWhen fetching recursively in the superproject, we should make sure that\nboth B and C are fetched in the submodule.\n"},{"id":"364591","messageId":"xmqqa7lkeki3.fsf@gitster-ct.c.googlers.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"Re: [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T03:10:44Z","receivedAt":"2018-12-05T03:10:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This is a resend of sb/submodule-recursive-fetch-gets-the-tip,\n> with all feedback addressed. As it took some time, I'll send it\n> without range-diff, but would ask for full review.\n\nIs that a \"resend\" or reroll/update (or whatever word that does not\nimply \"just sending the same thing again\")?\n\nFWIW, here is the range diff between 104f939f27..@{-1} and master..\nafter replacing the topic with this round.\n\n\n 3: 304b2dab29 ! 3: 08a297bd49 submodule.c: sort changed_submodule_names before searching it\n @@ -28,7 +28,7 @@\n @@\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n - \tcalculate_changed_submodule_paths();\n + \tcalculate_changed_submodule_paths(r);\n +\tstring_list_sort(&changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n\nJust the call nearby in the context has become repository-aware; no\nchange in this series.\n\n 4: f7345dad6d ! 4: 16dd6fe133 submodule.c: tighten scope of changed_submodule_names struct\n...\n ++\tcalculate_changed_submodule_paths(r, &spf.changed_submodule_names);\n +\tstring_list_sort(&spf.changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n\nI do recall having to do these adjustments while merging, so not\nhaving to do so anymore with rebasing is a welcome change ;-)\n\n 5: 5613d81d1e ! 5: bcd7337243 submodule: store OIDs in changed_submodule_names\n...\nLikewise.\n\n 7: e2419f7e30 ! 7: 26f80ccfc1 submodule: migrate get_next_submodule to use repository structs\n @@ -4,7 +4,8 @@\n \n We used to recurse into submodules, even if they were broken having\n only an objects directory. The child process executed in the submodule\n - would fail though if the submodule was broken.\n + would fail though if the submodule was broken. This is tested via\n + \"fetching submodule into a broken repository\" in t5526.\n \n This patch tightens the check upfront, such that we do not need\n to spawn a child process to find out if the submodule is broken.\n @@ -34,6 +35,7 @@\n +\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", sub->path);\n +\t\tif (repo_init(ret, gitdir.buf, NULL)) {\n +\t\t\tstrbuf_release(&gitdir);\n ++\t\t\tfree(ret);\n +\t\t\treturn NULL;\n\nLeakfix? Good.\n\n +\t\t}\n +\t\tstrbuf_release(&gitdir);\n @@ -75,11 +77,10 @@\n +\t\tif (repo) {\n \t\t\tchild_process_init(cp);\n -\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n - \t\t\tprepare_submodule_repo_env(&cp->env_array);\n +\t\t\tcp->dir = xstrdup(repo->worktree);\n + \t\t\tprepare_submodule_repo_env(&cp->env_array);\n\nHmph, I offhand do not see there would be any difference if you\nassigned to cp->dir before or after preparing the repo env, but is\nthere a reason these two must be done in this updated order that I\nam missing? Very similar changes appear multiple times in this\nrange-diff.\n\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n - \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n @@\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n @@ -94,8 +95,12 @@\n +\t\t\t * the submodule is not initialized\n +\t\t\t */\n +\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n -+\t\t\t !is_empty_dir(ce->name))\n -+\t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n ++\t\t\t !is_empty_dir(ce->name)) {\n ++\t\t\t\tspf->result = 1;\n ++\t\t\t\tstrbuf_addf(err,\n ++\t\t\t\t\t _(\"Could not access submodule '%s'\"),\n ++\t\t\t\t\t ce->name);\n ++\t\t\t}\n\nOK, not dying but returning to the caller to handle the error.\n\n 9: 7454fe5cb6 ! 9: 04eb06607b fetch: try fetching submodules if needed objects were not fetched\n @@ -17,11 +17,6 @@\n Try fetching a submodule by object id if the object id that the\n superproject points to, cannot be found.\n \n - The try does not happen when the \"git fetch\" done at the\n - superproject is not storing the fetched results in remote\n - tracking branches (i.e. instead just recording them to\n - FETCH_HEAD) in this step. A later patch will fix this.\n -\n builtin/fetch used to only inspect submodules when they were fetched\n \"on-demand\", as in either on/off case it was clear whether the submodule\n needs to be fetched. However to know whether we need to try fetching the\n @@ -29,6 +24,10 @@\n function check_for_new_submodule_commits(), so we'll also run that code\n in case the submodule recursion is set to \"on\".\n \n + The submodule checks were done only when a ref in the superproject\n + changed, these checks were extended to also be performed when fetching\n + into FETCH_HEAD for completeness, and add a test for that too.\n +\n\nOK.\n\n Signed-off-by: Stefan Beller \n Signed-off-by: Junio C Hamano \n \n @@ -41,30 +40,39 @@\n \n...\n\nThe range-diff output for this step is unreadble for me, but the\ncode around this area does not seem to appear in the comparison\nbetween the result of applying these directly to master and the\nresult of merging the previous round to master, so perhaps this is\njust an indication that later follow-up fix has been squashed into\nthis step or something, which I shouldn't have to worry about.\n\n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n @@ -73,8 +81,10 @@\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n -+\tstruct get_next_submodule_task **fetch_specific_oids;\n -+\tint fetch_specific_oids_nr, fetch_specific_oids_alloc;\n ++\n ++\t/* The submodules to fetch in */\n ++\tstruct fetch_task **oid_fetch_tasks;\n ++\tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n\nOK. The task struct has been renamed and the new name makes more\nsense (\"getting the next submodule\" is less important than \"what we\nare going to do to that submodule\").\n\n... \n -+struct get_next_submodule_task {\n ++struct fetch_task {\n +\tstruct repository *repo;\n +\tconst struct submodule *sub;\n +\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n...\n +\treturn (const struct submodule *) ret;\n +}\n +\n -+static struct get_next_submodule_task *get_next_submodule_task_create(\n -+\tstruct repository *r, const char *path)\n ++static struct fetch_task *fetch_task_create(struct repository *r,\n ++\t\t\t\t\t const char *path)\n +{\n -+\tstruct get_next_submodule_task *task = xmalloc(sizeof(*task));\n ++\tstruct fetch_task *task = xmalloc(sizeof(*task));\n +\tmemset(task, 0, sizeof(*task));\n +\n +\ttask->sub = submodule_from_path(r, &null_oid, path);\n +\tif (!task->sub) {\n -+\t\ttask->sub = get_default_submodule(path);\n ++\t\t/*\n ++\t\t * No entry in .gitmodules? Technically not a submodule,\n ++\t\t * but historically we supported repositories that happen to be\n ++\t\t * in-place where a gitlink is. Keep supporting them.\n ++\t\t */\n ++\t\ttask->sub = get_non_gitmodules_submodule(path);\n ++\t\tif (!task->sub) {\n ++\t\t\tfree(task);\n ++\t\t\treturn NULL;\n ++\t\t}\n ++\n +\t\ttask->free_sub = 1;\n\nOK.\n\n -+\t\tif (!task->sub) {\n -+\t\t\tfree(task);\n +-\t\t}\n ++\t\ttask = fetch_task_create(spf->r, ce->name);\n ++\t\tif (!task)\n +\t\t\tcontinue;\n - \t\t}\n\nOK, so the code used to signal the need to work with the presense of\ntask->sub but now task's NULLness is used, so no need to free.\n\n @@ -231,24 +253,26 @@\n +\t\t\treturn 1;\n \t\t} else {\n +\n -+\t\t\tget_next_submodule_task_release(task);\n ++\t\t\tfetch_task_release(task);\n +\t\t\tfree(task);\n +\n\nOK.\n\n -+\t\t/* NEEDSWORK: have get_default_remote from s--h */\n ++\t\t/* NEEDSWORK: have get_default_remote from submodule--helper */\n\n;-)\n\n"},{"id":"364708","messageId":"20181206212655.145586-1-sbeller@google.com","threadId":"49915","inReplyTo":"20181205010704.84790-1-jonathantanmy@google.com","subject":"[PATCH] fetch: ensure submodule objects fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-06T21:26:55Z","receivedAt":"2018-12-06T21:27:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently when git-fetch is asked to recurse into submodules, it dispatches\na plain \"git-fetch -C \" (with some submodule related options\nsuch as prefix and recusing strategy, but) without any information of the\nremote or the tip that should be fetched.\n\nBut this default fetch is not sufficient, as a newly fetched commit in\nthe superproject could point to a commit in the submodule that is not\nin the default refspec. This is common in workflows like Gerrit's.\nWhen fetching a Gerrit change under review (from refs/changes/??), the\ncommits in that change likely point to submodule commits that have not\nbeen merged to a branch yet.\n\nFetch a submodule object by id if the object that the superproject\npoints to, cannot be found. For now this object is fetched from the\n'origin' remote as we defer getting the default remote to a later patch.\n\nA list of new submodule commits are already generated in certain\nconditions (by check_for_new_submodule_commits()); this new feature\ninvokes that function in more situations.\n\nThe submodule checks were done only when a ref in the superproject\nchanged, these checks were extended to also be performed when fetching\ninto FETCH_HEAD for completeness, and add a test for that too.\n\nSigned-off-by: Stefan Beller \n---\n\nThanks Jonathan for the review!\nSo it looks like only the last patch needs some improvements,\nwhich is why I'd only resend the last patch here.\nAlso note the test with interious superproject commits.\n\nAll suggestions sounded sensible, addressing them all,\nhere is a range-diff to the currently queued version:\n\nRange-diff:\n1: 04eb06607b ! 1: ac6558cbc9 fetch: try fetching submodules if needed objects were not fetched\n @@ -1,6 +1,6 @@\n Author: Stefan Beller \n \n - fetch: try fetching submodules if needed objects were not fetched\n + fetch: ensure submodule objects fetched\n \n Currently when git-fetch is asked to recurse into submodules, it dispatches\n a plain \"git-fetch -C \" (with some submodule related options\n @@ -14,22 +14,19 @@\n commits in that change likely point to submodule commits that have not\n been merged to a branch yet.\n \n - Try fetching a submodule by object id if the object id that the\n - superproject points to, cannot be found.\n + Fetch a submodule object by id if the object that the superproject\n + points to, cannot be found. For now this object is fetched from the\n + 'origin' remote as we defer getting the default remote to a later patch.\n \n - builtin/fetch used to only inspect submodules when they were fetched\n - \"on-demand\", as in either on/off case it was clear whether the submodule\n - needs to be fetched. However to know whether we need to try fetching the\n - object ids, we need to identify the object names, which is done in this\n - function check_for_new_submodule_commits(), so we'll also run that code\n - in case the submodule recursion is set to \"on\".\n + A list of new submodule commits are already generated in certain\n + conditions (by check_for_new_submodule_commits()); this new feature\n + invokes that function in more situations.\n \n The submodule checks were done only when a ref in the superproject\n changed, these checks were extended to also be performed when fetching\n into FETCH_HEAD for completeness, and add a test for that too.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/builtin/fetch.c b/builtin/fetch.c\n --- a/builtin/fetch.c\n @@ -82,7 +79,7 @@\n \n \tstruct string_list changed_submodule_names;\n +\n -+\t/* The submodules to fetch in */\n ++\t/* Pending fetches by OIDs */\n +\tstruct fetch_task **oid_fetch_tasks;\n +\tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n };\n @@ -97,13 +94,16 @@\n \treturn spf->default_option;\n }\n \n ++/*\n ++ * Fetch in progress (if callback data) or\n ++ * pending (if in oid_fetch_tasks in struct submodule_parallel_fetch)\n ++ */\n +struct fetch_task {\n +\tstruct repository *repo;\n +\tconst struct submodule *sub;\n +\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n +\n -+\t/* fetch specific oids if set, otherwise fetch default refspec */\n -+\tstruct oid_array *commits;\n ++\tstruct oid_array *commits; /* Ensure these commits are fetched */\n +};\n +\n +/**\n @@ -176,7 +176,6 @@\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n -\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n -+\t\tint recurse_config;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n \t\tconst char *default_argv;\n -\t\tconst struct submodule *submodule;\n @@ -199,11 +198,9 @@\n +\t\ttask = fetch_task_create(spf->r, ce->name);\n +\t\tif (!task)\n +\t\t\tcontinue;\n -+\n -+\t\trecurse_config = get_fetch_recurse_config(task->sub, spf);\n \n -\t\tswitch (get_fetch_recurse_config(submodule, spf))\n -+\t\tswitch (recurse_config)\n ++\t\tswitch (get_fetch_recurse_config(task->sub, spf))\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n @@ -314,7 +311,7 @@\n \treturn 0;\n }\n \n -+static int commit_exists_in_sub(const struct object_id *oid, void *data)\n ++static int commit_missing_in_sub(const struct object_id *oid, void *data)\n +{\n +\tstruct repository *subrepo = data;\n +\n @@ -340,7 +337,7 @@\n +\n +\t/* Is this the second time we process this submodule? */\n +\tif (task->commits)\n -+\t\treturn 0;\n ++\t\tgoto out;\n +\n +\tit = string_list_lookup(&spf->changed_submodule_names, task->sub->name);\n +\tif (!it)\n @@ -349,7 +346,7 @@\n +\n +\tcommits = it->util;\n +\toid_array_filter(commits,\n -+\t\t\t commit_exists_in_sub,\n ++\t\t\t commit_missing_in_sub,\n +\t\t\t task->repo);\n +\n +\t/* Are there commits we want, but do not exist? */\n @@ -408,7 +405,7 @@\n +\t)\n +'\n +\n -+test_expect_success 'fetch new submodule commits on-demand in FETCH_HEAD' '\n ++test_expect_success 'fetch new submodule commit on-demand in FETCH_HEAD' '\n +\t# depends on the previous test for setup\n +\n +\tC=$(git -C submodule commit-tree -m \"another change outside refs/heads\" HEAD^{tree}) &&\n @@ -462,5 +459,36 @@\n +\t\tgit checkout --recurse-submodules FETCH_HEAD\n +\t)\n +'\n ++\n ++test_expect_success 'fetch new submodule commit intermittently referenced by superproject' '\n ++\t# depends on the previous test for setup\n ++\n ++\tD=$(git -C sub1 commit-tree -m \"change 10 outside refs/heads\" HEAD^{tree}) &&\n ++\tE=$(git -C sub1 commit-tree -m \"change 11 outside refs/heads\" HEAD^{tree}) &&\n ++\tF=$(git -C sub1 commit-tree -m \"change 12 outside refs/heads\" HEAD^{tree}) &&\n ++\n ++\tgit -C sub1 update-ref refs/changes/10 $D &&\n ++\tgit update-index --cacheinfo 160000 $D sub1 &&\n ++\tgit commit -m \"updated submodules outside of refs/heads\" &&\n ++\n ++\tgit -C sub1 update-ref refs/changes/11 $E &&\n ++\tgit update-index --cacheinfo 160000 $E sub1 &&\n ++\tgit commit -m \"updated submodules outside of refs/heads\" &&\n ++\n ++\tgit -C sub1 update-ref refs/changes/12 $F &&\n ++\tgit update-index --cacheinfo 160000 $F sub1 &&\n ++\tgit commit -m \"updated submodules outside of refs/heads\" &&\n ++\n ++\tG=$(git rev-parse HEAD) &&\n ++\tgit update-ref refs/changes/13 $G &&\n ++\t(\n ++\t\tcd downstream &&\n ++\t\tgit fetch --recurse-submodules origin refs/changes/13 &&\n ++\n ++\t\tgit -C sub1 cat-file -t $D &&\n ++\t\tgit -C sub1 cat-file -t $E &&\n ++\t\tgit -C sub1 cat-file -t $F\n ++\t)\n ++'\n +\n test_done\n\n builtin/fetch.c | 11 +-\n submodule.c | 206 +++++++++++++++++++++++++++++++-----\n t/t5526-fetch-submodules.sh | 117 ++++++++++++++++++++\n 3 files changed, 296 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e0140327aa..91f9b7d9c8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -763,9 +763,6 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(msg, ref, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n@@ -779,9 +776,6 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"fast-forward\", ref, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n@@ -794,9 +788,6 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"forced-update\", ref, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n@@ -892,6 +883,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tref->force = rm->peer_ref->force;\n \t\t\t}\n \n+\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n+\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n \n \t\t\tif (!strcmp(rm->name, \"HEAD\")) {\n \t\t\t\tkind = \"\";\ndiff --git a/submodule.c b/submodule.c\nindex d1b6646f42..b88343d977 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1231,8 +1231,14 @@ struct submodule_parallel_fetch {\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n+\n+\t/* Pending fetches by OIDs */\n+\tstruct fetch_task **oid_fetch_tasks;\n+\tint oid_fetch_tasks_nr, oid_fetch_tasks_alloc;\n };\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n+\t\t STRING_LIST_INIT_DUP, \\\n+\t\t NULL, 0, 0}\n \n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t struct submodule_parallel_fetch *spf)\n@@ -1259,6 +1265,76 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+/*\n+ * Fetch in progress (if callback data) or\n+ * pending (if in oid_fetch_tasks in struct submodule_parallel_fetch)\n+ */\n+struct fetch_task {\n+\tstruct repository *repo;\n+\tconst struct submodule *sub;\n+\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n+\n+\tstruct oid_array *commits; /* Ensure these commits are fetched */\n+};\n+\n+/**\n+ * When a submodule is not defined in .gitmodules, we cannot access it\n+ * via the regular submodule-config. Create a fake submodule, which we can\n+ * work on.\n+ */\n+static const struct submodule *get_non_gitmodules_submodule(const char *path)\n+{\n+\tstruct submodule *ret = NULL;\n+\tconst char *name = default_name_or_path(path);\n+\n+\tif (!name)\n+\t\treturn NULL;\n+\n+\tret = xmalloc(sizeof(*ret));\n+\tmemset(ret, 0, sizeof(*ret));\n+\tret->path = name;\n+\tret->name = name;\n+\n+\treturn (const struct submodule *) ret;\n+}\n+\n+static struct fetch_task *fetch_task_create(struct repository *r,\n+\t\t\t\t\t const char *path)\n+{\n+\tstruct fetch_task *task = xmalloc(sizeof(*task));\n+\tmemset(task, 0, sizeof(*task));\n+\n+\ttask->sub = submodule_from_path(r, &null_oid, path);\n+\tif (!task->sub) {\n+\t\t/*\n+\t\t * No entry in .gitmodules? Technically not a submodule,\n+\t\t * but historically we supported repositories that happen to be\n+\t\t * in-place where a gitlink is. Keep supporting them.\n+\t\t */\n+\t\ttask->sub = get_non_gitmodules_submodule(path);\n+\t\tif (!task->sub) {\n+\t\t\tfree(task);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\ttask->free_sub = 1;\n+\t}\n+\n+\treturn task;\n+}\n+\n+static void fetch_task_release(struct fetch_task *p)\n+{\n+\tif (p->free_sub)\n+\t\tfree((void*)p->sub);\n+\tp->free_sub = 0;\n+\tp->sub = NULL;\n+\n+\tif (p->repo)\n+\t\trepo_clear(p->repo);\n+\tFREE_AND_NULL(p->repo);\n+}\n+\n static struct repository *get_submodule_repo_for(struct repository *r,\n \t\t\t\t\t\t const struct submodule *sub)\n {\n@@ -1286,39 +1362,29 @@ static struct repository *get_submodule_repo_for(struct repository *r,\n static int get_next_submodule(struct child_process *cp,\n \t\t\t struct strbuf *err, void *data, void **task_cb)\n {\n-\tint ret = 0;\n \tstruct submodule_parallel_fetch *spf = data;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n-\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n \t\tconst char *default_argv;\n-\t\tconst struct submodule *submodule;\n-\t\tstruct repository *repo;\n-\t\tstruct submodule default_submodule = SUBMODULE_INIT;\n+\t\tstruct fetch_task *task;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n-\t\tsubmodule = submodule_from_path(spf->r, &null_oid, ce->name);\n-\t\tif (!submodule) {\n-\t\t\tconst char *name = default_name_or_path(ce->name);\n-\t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = name;\n-\t\t\t\tdefault_submodule.name = name;\n-\t\t\t\tsubmodule = &default_submodule;\n-\t\t\t}\n-\t\t}\n+\t\ttask = fetch_task_create(spf->r, ce->name);\n+\t\tif (!task)\n+\t\t\tcontinue;\n \n-\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\tswitch (get_fetch_recurse_config(task->sub, spf))\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule ||\n+\t\t\tif (!task->sub ||\n \t\t\t !string_list_lookup(\n \t\t\t\t\t&spf->changed_submodule_names,\n-\t\t\t\t\tsubmodule->name))\n+\t\t\t\t\ttask->sub->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\n@@ -1329,11 +1395,11 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n-\t\trepo = get_submodule_repo_for(spf->r, submodule);\n-\t\tif (repo) {\n+\t\ttask->repo = get_submodule_repo_for(spf->r, task->sub);\n+\t\tif (task->repo) {\n+\t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = xstrdup(repo->gitdir);\n+\t\t\tcp->dir = task->repo->gitdir;\n \t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n@@ -1343,12 +1409,22 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_pushv(&cp->args, spf->args.argv);\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\n+\t\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\",\n+\t\t\t\t\t\t spf->prefix,\n+\t\t\t\t\t\t task->sub->path);\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n \n-\t\t\trepo_clear(repo);\n-\t\t\tfree(repo);\n-\t\t\tret = 1;\n+\t\t\tspf->count++;\n+\t\t\t*task_cb = task;\n+\n+\t\t\tstrbuf_release(&submodule_prefix);\n+\t\t\treturn 1;\n \t\t} else {\n+\n+\t\t\tfetch_task_release(task);\n+\t\t\tfree(task);\n+\n \t\t\t/*\n \t\t\t * An empty directory is normal,\n \t\t\t * the submodule is not initialized\n@@ -1361,12 +1437,38 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t\t\t ce->name);\n \t\t\t}\n \t\t}\n+\t}\n+\n+\tif (spf->oid_fetch_tasks_nr) {\n+\t\tstruct fetch_task *task =\n+\t\t\tspf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tspf->oid_fetch_tasks_nr--;\n+\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\",\n+\t\t\t spf->prefix, task->sub->path);\n+\n+\t\tchild_process_init(cp);\n+\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tcp->git_cmd = 1;\n+\t\tcp->dir = task->repo->gitdir;\n+\n+\t\targv_array_init(&cp->args);\n+\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\targv_array_push(&cp->args, \"on-demand\");\n+\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\n+\t\t/* NEEDSWORK: have get_default_remote from submodule--helper */\n+\t\targv_array_push(&cp->args, \"origin\");\n+\t\toid_array_for_each_unique(task->commits,\n+\t\t\t\t\t append_oid_to_argv, &cp->args);\n+\n+\t\t*task_cb = task;\n \t\tstrbuf_release(&submodule_prefix);\n-\t\tif (ret) {\n-\t\t\tspf->count++;\n-\t\t\treturn 1;\n-\t\t}\n+\t\treturn 1;\n \t}\n+\n \treturn 0;\n }\n \n@@ -1374,20 +1476,66 @@ static int fetch_start_failure(struct strbuf *err,\n \t\t\t void *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct fetch_task *task = task_cb;\n \n \tspf->result = 1;\n \n+\tfetch_task_release(task);\n \treturn 0;\n }\n \n+static int commit_missing_in_sub(const struct object_id *oid, void *data)\n+{\n+\tstruct repository *subrepo = data;\n+\n+\tenum object_type type = oid_object_info(subrepo, oid, NULL);\n+\n+\treturn type != OBJ_COMMIT;\n+}\n+\n static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t\tvoid *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct fetch_task *task = task_cb;\n+\n+\tstruct string_list_item *it;\n+\tstruct oid_array *commits;\n \n \tif (retvalue)\n \t\tspf->result = 1;\n \n+\tif (!task || !task->sub)\n+\t\tBUG(\"callback cookie bogus\");\n+\n+\t/* Is this the second time we process this submodule? */\n+\tif (task->commits)\n+\t\tgoto out;\n+\n+\tit = string_list_lookup(&spf->changed_submodule_names, task->sub->name);\n+\tif (!it)\n+\t\t/* Could be an unchanged submodule, not contained in the list */\n+\t\tgoto out;\n+\n+\tcommits = it->util;\n+\toid_array_filter(commits,\n+\t\t\t commit_missing_in_sub,\n+\t\t\t task->repo);\n+\n+\t/* Are there commits we want, but do not exist? */\n+\tif (commits->nr) {\n+\t\ttask->commits = commits;\n+\t\tALLOC_GROW(spf->oid_fetch_tasks,\n+\t\t\t spf->oid_fetch_tasks_nr + 1,\n+\t\t\t spf->oid_fetch_tasks_alloc);\n+\t\tspf->oid_fetch_tasks[spf->oid_fetch_tasks_nr] = task;\n+\t\tspf->oid_fetch_tasks_nr++;\n+\t\treturn 0;\n+\t}\n+\n+out:\n+\tfetch_task_release(task);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 6c2f9b2ba2..9f8c744eb5 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -600,4 +600,121 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"fetch new submodule commits on-demand outside standard refspec\" '\n+\t# add a second submodule and ensure it is around in downstream first\n+\tgit clone submodule sub1 &&\n+\tgit submodule add ./sub1 &&\n+\tgit commit -m \"adding a second submodule\" &&\n+\tgit -C downstream pull &&\n+\tgit -C downstream submodule update --init --recursive &&\n+\n+\tgit checkout --detach &&\n+\n+\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/1 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/2 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/3 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/3:refs/heads/my_branch &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success 'fetch new submodule commit on-demand in FETCH_HEAD' '\n+\t# depends on the previous test for setup\n+\n+\tC=$(git -C submodule commit-tree -m \"another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/4 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/5 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/6 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/6 &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success 'fetch new submodule commits on-demand without .gitmodules entry' '\n+\t# depends on the previous test for setup\n+\n+\tgit config -f .gitmodules --remove-section submodule.sub1 &&\n+\tgit add .gitmodules &&\n+\tgit commit -m \"delete gitmodules file\" &&\n+\tgit checkout -B master &&\n+\tgit -C downstream fetch &&\n+\tgit -C downstream checkout origin/master &&\n+\n+\tC=$(git -C submodule commit-tree -m \"yet another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/7 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"yet another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/8 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/9 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/9 &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success 'fetch new submodule commit intermittently referenced by superproject' '\n+\t# depends on the previous test for setup\n+\n+\tD=$(git -C sub1 commit-tree -m \"change 10 outside refs/heads\" HEAD^{tree}) &&\n+\tE=$(git -C sub1 commit-tree -m \"change 11 outside refs/heads\" HEAD^{tree}) &&\n+\tF=$(git -C sub1 commit-tree -m \"change 12 outside refs/heads\" HEAD^{tree}) &&\n+\n+\tgit -C sub1 update-ref refs/changes/10 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\n+\tgit -C sub1 update-ref refs/changes/11 $E &&\n+\tgit update-index --cacheinfo 160000 $E sub1 &&\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\n+\tgit -C sub1 update-ref refs/changes/12 $F &&\n+\tgit update-index --cacheinfo 160000 $F sub1 &&\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\n+\tG=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/13 $G &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/13 &&\n+\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit -C sub1 cat-file -t $E &&\n+\t\tgit -C sub1 cat-file -t $F\n+\t)\n+'\n+\n test_done\n-- \n2.20.0.rc2.230.gc28305e538\n\n"},{"id":"364711","messageId":"CAGZ79kZaw_Pn8mneHYeUebB2vZ+N0FoJBr9NCWp8atfpjLat4g@mail.gmail.com","threadId":"49915","inReplyTo":"xmqqa7lkeki3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-06T21:59:57Z","receivedAt":"2018-12-06T22:00:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 4, 2018 at 7:10 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > This is a resend of sb/submodule-recursive-fetch-gets-the-tip,\n> > with all feedback addressed. As it took some time, I'll send it\n> > without range-diff, but would ask for full review.\n>\n> Is that a \"resend\" or reroll/update (or whatever word that does not\n> imply \"just sending the same thing again\")?\n\nAs you noticed, it is an actual update. I started to use resend\nas DScho seems very unhappy about the word reroll claiming we'd\nbe the only Software community that uses the term reroll for\nan iteration of a change.\n\nI see how resend could sound like retransmission without change.\n\n\n> child_process_init(cp);\n> - cp->dir = strbuf_detach(&submodule_path, NULL);\n> - prepare_submodule_repo_env(&cp->env_array);\n> + cp->dir = xstrdup(repo->worktree);\n> + prepare_submodule_repo_env(&cp->env_array);\n>\n> Hmph, I offhand do not see there would be any difference if you\n> assigned to cp->dir before or after preparing the repo env, but is\n> there a reason these two must be done in this updated order that I\n> am missing? Very similar changes appear multiple times in this\n> range-diff.\n\nJonathan Tan asked for it to be \"diff friendly\". This -of course- is\nrange-diff unfriendly.\n\n> [...]\n\nyou seem to be OK with a lot of the changes, I did not find an\nactionable suggestion.\n\nThanks for still queuing topics during -rc time,\nStefan\n"},{"id":"364728","messageId":"20181207002531.GA37614@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-1-sbeller@google.com","subject":"Re: [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-07T00:25:31Z","receivedAt":"2018-12-07T00:25:40Z","isPatch":false,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.28 16:27, Stefan Beller wrote:\n> This is a resend of sb/submodule-recursive-fetch-gets-the-tip,\n> with all feedback addressed. As it took some time, I'll send it\n> without range-diff, but would ask for full review.\n> \n> I plan on resending after the next release as this got delayed quite a bit,\n> which is why I also rebased it to master.\n> \n> Thanks,\n> Stefan\n\nI am not very familiar with most of the submodule code, but for what\nit's worth, this entire series looks good to me. I'll note that most of\nthe commits caused some style complaints, but I'll leave it up to your\njudgement as to whether they're valid or not.\n\nReviewed-by: Josh Steadmon \n"},{"id":"364833","messageId":"xmqq1s6r5unb.fsf@gitster-ct.c.googlers.com","threadId":"49915","inReplyTo":"20181206212655.145586-1-sbeller@google.com","subject":"Re: [PATCH] fetch: ensure submodule objects fetched","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-09T01:57:44Z","receivedAt":"2018-12-09T01:57:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Currently when git-fetch is asked to recurse into submodules, it dispatches\n> a plain \"git-fetch -C \" (with some submodule related options\n> such as prefix and recusing strategy, but) without any information of the\n> remote or the tip that should be fetched.\n>\n> But this default fetch is not sufficient, as a newly fetched commit in\n> the superproject could point to a commit in the submodule that is not\n> in the default refspec. This is common in workflows like Gerrit's.\n> When fetching a Gerrit change under review (from refs/changes/??), the\n> commits in that change likely point to submodule commits that have not\n> been merged to a branch yet.\n>\n> Fetch a submodule object by id if the object that the superproject\n> points to, cannot be found. For now this object is fetched from the\n> 'origin' remote as we defer getting the default remote to a later patch.\n>\n> A list of new submodule commits are already generated in certain\n> conditions (by check_for_new_submodule_commits()); this new feature\n> invokes that function in more situations.\n>\n> The submodule checks were done only when a ref in the superproject\n> changed, these checks were extended to also be performed when fetching\n> into FETCH_HEAD for completeness, and add a test for that too.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Thanks Jonathan for the review!\n> So it looks like only the last patch needs some improvements,\n> which is why I'd only resend the last patch here.\n> Also note the test with interious superproject commits.\n\nSorry, can't parse the last sentence.\n\nAnyway, will replace the last step with this. Thanks.\n\n"},{"id":"366699","messageId":"20190115013808.GL162110@google.com","threadId":"49915","inReplyTo":"20181207002531.GA37614@google.com","subject":"Re: [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-01-15T01:38:08Z","receivedAt":"2019-01-15T01:38:13Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Josh Steadmon wrote:\n\n> I am not very familiar with most of the submodule code, but for what\n> it's worth, this entire series looks good to me. I'll note that most of\n> the commits caused some style complaints, but I'll leave it up to your\n> judgement as to whether they're valid or not.\n>\n> Reviewed-by: Josh Steadmon \n\nFor what it's worth, we've been running with this at Google (using the\nsb/submodule-recursive-fetch-gets-the-tip branch from \"pu\") for more\nthan a month, with no user complaints.\n\nTested-by: Jonathan Nieder \n"},{"id":"368397","messageId":"20190202015817.GA241226@google.com","threadId":"49915","inReplyTo":"20181129002756.167615-8-sbeller@google.com","subject":"Re: [PATCH 7/9] submodule: migrate get_next_submodule to use repository structs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-02-02T01:58:17Z","receivedAt":"2019-02-02T01:58:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> This patch tightens the check upfront, such that we do not need\n> to spawn a child process to find out if the submodule is broken.\n\nSounds sensible.\n\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n[...]\n> @@ -1319,10 +1338,23 @@ static int get_next_submodule(struct child_process *cp,\n> \t\t\targv_array_push(&cp->args, default_argv);\n> \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n> \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n> +\n> +\t\t\trepo_clear(repo);\n> +\t\t\tfree(repo);\n> \t\t\tret = 1;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * An empty directory is normal,\n> +\t\t\t * the submodule is not initialized\n> +\t\t\t */\n> +\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n> +\t\t\t !is_empty_dir(ce->name)) {\n\nWhat if the directory is nonempty (e.g. contains build artifacts)?\n\n> +\t\t\t\tspf->result = 1;\n> +\t\t\t\tstrbuf_addf(err,\n> +\t\t\t\t\t _(\"Could not access submodule '%s'\"),\n> +\t\t\t\t\t ce->name);\n> +\t\t\t}\n\nShould this exit the loop? Otherwise, multiple \"Could not access\"\nmessages can go in the same err string a big concatenated line.\n\nThanks,\nJonathan\n"}]}