{"thread":{"id":"49575","subject":"[PATCH 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","startedAt":"2018-10-16T18:13:33Z","lastAt":"2018-10-25T21:42:29Z","messageCount":28,"participants":["Stefan Beller","Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"360646","messageId":"20181016181327.107186-1-sbeller@google.com","threadId":"49575","inReplyTo":null,"subject":"[PATCH 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:18Z","receivedAt":"2018-10-16T18:13:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is based on ao/submodule-wo-gitmodules-checked-out.\n\nThis resends origin/sb/submodule-recursive-fetch-gets-the-tip, resolving\nthe issues pointed out via origin/xxx/sb-submodule-recursive-fetch-gets-the-tip-in-pu\nby basing this series on origin/ao/submodule-wo-gitmodules-checked-out\n\nA range-diff below shows how picking a different base changed the patches,\napart from that no further adjustments have been made.\n\nThanks,\nStefan\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: move global changed_submodule_names into fetch submodule\n struct\n submodule.c: do not copy around submodule list\n repository: repo_submodule_init to take a submodule struct\n submodule: fetch in submodules git directory instead of in worktree\n fetch: retry fetching submodules if needed objects were not fetched\n builtin/fetch: check for submodule updates for non branch fetches\n\n Documentation/technical/api-oid-array.txt | 5 +\n builtin/fetch.c | 14 +-\n builtin/grep.c | 17 +-\n builtin/ls-files.c | 12 +-\n repository.c | 27 +-\n repository.h | 11 +-\n sha1-array.c | 17 ++\n sha1-array.h | 3 +\n submodule.c | 275 +++++++++++++++----\n t/helper/test-submodule-nested-repo-config.c | 8 +-\n t/t5526-fetch-submodules.sh | 23 +-\n 11 files changed, 315 insertions(+), 97 deletions(-)\n\ngit range-diff origin/xxx/sb-submodule-recursive-fetch-gets-the-tip-in-pu...\n[...]\n585: ac1f98a0df < -: ---------- doc: move git-rev-parse from porcelain to plumbing\n586: 7cf1a0fbef = 1: a035323c49 sha1-array: provide oid_array_filter\n587: 01077381d0 = 2: 30ed20b4f0 submodule.c: fix indentation\n588: 4b0cdf5899 = 3: cd590ea88d submodule.c: sort changed_submodule_names before searching it\n589: 78e5099ecc ! 4: ce959811ba submodule.c: move global changed_submodule_names into fetch submodule struct\n @@ -12,7 +12,7 @@\n --- a/submodule.c\n +++ b/submodule.c\n @@\n - #include \"commit-reach.h\"\n + #include \"object-store.h\"\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n -static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n590: d813f18bb3 = 5: 151f9a8ad4 submodule.c: do not copy around submodule list\n591: a077d63af7 ! 6: 3a97743fa2 repository: repo_submodule_init to take a submodule struct\n @@ -15,7 +15,6 @@\n Also move its documentation into the header file.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/builtin/grep.c b/builtin/grep.c\n --- a/builtin/grep.c\n @@ -31,12 +30,16 @@\n +\n \tint hit;\n \n - \tif (!is_submodule_active(superproject, path))\n + \t/*\n +@@\n \t\treturn 0;\n + \t}\n \n --\tif (repo_submodule_init(&submodule, superproject, path))\n -+\tif (repo_submodule_init(&subrepo, superproject, sub))\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 @@ -44,9 +47,9 @@\n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\n @@\n + \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n - \tgrep_read_lock();\n -\tadd_to_alternates_memory(submodule.objects->objectdir);\n +\tadd_to_alternates_memory(subrepo.objects->objectdir);\n \tgrep_read_unlock();\n @@ -100,19 +103,6 @@\n \n static void show_ce(struct repository *repo, struct dir_struct *dir,\n \n -diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n ---- a/builtin/submodule--helper.c\n -+++ b/builtin/submodule--helper.c\n -@@\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)) {\n -\n diff --git a/repository.c b/repository.c\n --- a/repository.c\n +++ b/repository.c\n @@ -197,3 +187,32 @@\n void repo_clear(struct repository *repo);\n \n /*\n +\n +diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c\n +--- a/t/helper/test-submodule-nested-repo-config.c\n ++++ b/t/helper/test-submodule-nested-repo-config.c\n +@@\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 + \n592: 780f6c1a92 = 7: 4e8ad61f8d submodule: fetch in submodules git directory instead of in worktree\n593: a530535912 = 8: 24bac00db7 fetch: retry fetching submodules if needed objects were not fetched\n594: a72bde3a8a = 9: e031182e44 builtin/fetch: check for submodule updates for non branch fetches\n[...]\n"},{"id":"360647","messageId":"20181016181327.107186-2-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"[PATCH 1/9] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:19Z","receivedAt":"2018-10-16T18:13:34Z","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 265941fbf4..d505a004bb 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.19.0\n\n"},{"id":"360648","messageId":"20181016181327.107186-3-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"[PATCH 2/9] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:20Z","receivedAt":"2018-10-16T18:13:37Z","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 2b7082b2db..e145ebbb16 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1258,7 +1258,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@@ -1268,8 +1269,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.19.0\n\n"},{"id":"360649","messageId":"20181016181327.107186-4-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-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-10-16T18:13:21Z","receivedAt":"2018-10-16T18:13:39Z","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 e145ebbb16..9fbfcfcfe1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1270,7 +1270,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@@ -1364,6 +1364,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();\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.19.0\n\n"},{"id":"360650","messageId":"20181016181327.107186-5-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"[PATCH 4/9] submodule.c: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:22Z","receivedAt":"2018-10-16T18:13:44Z","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 \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9fbfcfcfe1..6b4cee82bf 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -24,7 +24,7 @@\n #include \"object-store.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+\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@@ -1124,7 +1124,22 @@ 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(void)\n+struct submodule_parallel_fetch {\n+\tint count;\n+\tstruct argv_array args;\n+\tstruct repository *r;\n+\tconst char *prefix;\n+\tint command_line_option;\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, STRING_LIST_INIT_DUP }\n+\n+static void calculate_changed_submodule_paths(\n+\tstruct submodule_parallel_fetch *spf)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n@@ -1162,7 +1177,8 @@ static void calculate_changed_submodule_paths(void)\n \t\t\tcontinue;\n \n \t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&changed_submodule_names, name->string);\n+\t\t\tstring_list_append(&spf->changed_submodule_names,\n+\t\t\t\t\t name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1199,18 +1215,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \treturn ret;\n }\n \n-struct submodule_parallel_fetch {\n-\tint count;\n-\tstruct argv_array args;\n-\tstruct repository *r;\n-\tconst char *prefix;\n-\tint command_line_option;\n-\tint default_option;\n-\tint quiet;\n-\tint result;\n-};\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 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 {\n@@ -1271,7 +1275,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@@ -1363,8 +1367,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();\n-\tstring_list_sort(&changed_submodule_names);\n+\tcalculate_changed_submodule_paths(&spf);\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@@ -1373,7 +1377,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.19.0\n\n"},{"id":"360651","messageId":"20181016181327.107186-6-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"[PATCH 5/9] submodule.c: do not copy around submodule list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:23Z","receivedAt":"2018-10-16T18:13:53Z","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 \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6b4cee82bf..cbefe5f54d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1142,8 +1142,7 @@ static void calculate_changed_submodule_paths(\n \tstruct submodule_parallel_fetch *spf)\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(the_repository, NULL, NULL))\n@@ -1160,9 +1159,9 @@ static void calculate_changed_submodule_paths(\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(&changed_submodules, &argv);\n+\tcollect_changed_submodules(&spf->changed_submodule_names, &argv);\n \n-\tfor_each_string_list_item(name, &changed_submodules) {\n+\tfor_each_string_list_item(name, &spf->changed_submodule_names) {\n \t\tstruct oid_array *commits = name->util;\n \t\tconst struct submodule *submodule;\n \t\tconst char *path = NULL;\n@@ -1176,12 +1175,14 @@ static void calculate_changed_submodule_paths(\n \t\tif (!path)\n \t\t\tcontinue;\n \n-\t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&spf->changed_submodule_names,\n-\t\t\t\t\t name->string);\n+\t\tif (submodule_has_commits(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(&spf->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@@ -1377,7 +1378,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.19.0\n\n"},{"id":"360652","messageId":"20181016181327.107186-7-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-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-10-16T18:13:24Z","receivedAt":"2018-10-16T18:14:02Z","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\nsiutation giving a 'path' argument is not useful. Upgrade the\nrepo_submodule_init function to take a struct submodule instead.\n\nWhile we are at it, overhaul the repo_submodule_init function by renaming\nthe submodule repository struct, which is to be initialized, to a name\nthat is not confused with the struct submodule as easily.\n\nAlso move its documentation into the header file.\n\nSigned-off-by: Stefan Beller \n---\n builtin/grep.c | 17 +++++++-----\n builtin/ls-files.c | 12 +++++----\n repository.c | 27 ++++++++------------\n repository.h | 11 ++++++--\n t/helper/test-submodule-nested-repo-config.c | 8 +++---\n 5 files changed, 41 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7da8fef31a..ba7634258a 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -418,7 +418,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@@ -434,12 +437,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@@ -451,7 +454,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@@ -476,14 +479,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 7f9919a362..4d1649c1b3 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/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..a9c7a5baa5 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -116,9 +116,16 @@ 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.\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.19.0\n\n"},{"id":"360653","messageId":"20181016181327.107186-8-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"[PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:25Z","receivedAt":"2018-10-16T18:14:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch started as a refactoring to make 'get_next_submodule' more\nreadable, but upon doing so, I realized that \"git fetch\" of the submodule\nactually doesn't need to be run in the submodules worktree. So let's run\nit in its git dir instead.\n\nThat should pave the way towards fetching submodules that are currently\nnot checked out.\n\nThis patch leaks the cp->dir in get_next_submodule, as any further\ncallback in run_processes_parallel doesn't have access to the child\nprocess any more. In an early iteration of this patch, the function\nget_submodule_repo_for directly returned the string containing the\ngit directory, which would be a better design choice for this patch.\n\nHowever the next patch both fixes the memory leak of cp->dir and also has\na use case for using the full repository handle of the submodule, so\nit makes sense to introduce the get_submodule_repo_for here already.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 51 +++++++++++++++++++++++++++----------\n t/t5526-fetch-submodules.sh | 7 ++++-\n 2 files changed, 44 insertions(+), 14 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex cbefe5f54d..30c06507e3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -495,6 +495,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@@ -1241,6 +1247,29 @@ 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\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@@ -1248,12 +1277,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@@ -1288,16 +1316,12 @@ 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\tprepare_submodule_repo_env(&cp->env_array);\n+\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\t\tcp->dir = xstrdup(repo->gitdir);\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@@ -1307,10 +1331,11 @@ 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}\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++;\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 6c2f9b2ba2..42692219a1 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -566,7 +566,12 @@ test_expect_success 'fetching submodule into a broken repository' '\n \n \ttest_must_fail git -C dst status &&\n \ttest_must_fail git -C dst diff &&\n-\ttest_must_fail git -C dst fetch --recurse-submodules\n+\n+\t# git-fetch cannot find the git directory of the submodule,\n+\t# so it will do nothing, successfully, as it cannot distinguish between\n+\t# this broken submodule and a submodule that was just set active but\n+\t# not cloned yet\n+\tgit -C dst fetch --recurse-submodules\n '\n \n test_expect_success \"fetch new commits when submodule got renamed\" '\n-- \n2.19.0\n\n"},{"id":"360654","messageId":"20181016181327.107186-9-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"[PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:26Z","receivedAt":"2018-10-16T18:14:20Z","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\nThis works surprisingly well in some workflows (such as using submodules\nas a third party library), while not so well in other scenarios, such\nas in a Gerrit topic-based workflow, that can tie together changes\n(potentially across repositories) on the server side. One of the parts\nof such a Gerrit workflow is to download a change when wanting to examine\nit, and you'd want to have its submodule changes that are in the same\ntopic downloaded as well. However these submodule changes reside in their\nown repository in their own ref (refs/changes/).\n\nRetry fetching a submodule by object name if the object id that the\nsuperproject points to, cannot be found.\n\nThis retrying does not happen when the \"git fetch\" done at the\nsuperproject is not storing the fetched results in remote\ntracking branches (i.e. instead just recording them to\nFETCH_HEAD) in this step. A later patch will fix this.\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\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/fetch.c | 9 +-\n submodule.c | 185 ++++++++++++++++++++++++++++++------\n t/t5526-fetch-submodules.sh | 16 ++++\n 3 files changed, 177 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 61bec5d213..95c44bf6ff 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -700,8 +700,7 @@ 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\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\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@@ -716,8 +715,7 @@ 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\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\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@@ -731,8 +729,7 @@ 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\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\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,\ndiff --git a/submodule.c b/submodule.c\nindex 30c06507e3..7246b776f3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1141,8 +1141,12 @@ struct submodule_parallel_fetch {\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n+\tstruct get_next_submodule_task **retry;\n+\tint retry_nr, retry_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 void calculate_changed_submodule_paths(\n \tstruct submodule_parallel_fetch *spf)\n@@ -1247,6 +1251,56 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+struct get_next_submodule_task {\n+\tstruct repository *repo;\n+\tconst struct submodule *sub;\n+\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n+\tstruct oid_array *commits;\n+};\n+\n+static const struct submodule *get_default_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 get_next_submodule_task *get_next_submodule_task_create(\n+\tstruct repository *r, const char *path)\n+{\n+\tstruct get_next_submodule_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\ttask->free_sub = 1;\n+\t}\n+\n+\treturn task;\n+}\n+\n+static void get_next_submodule_task_release(struct get_next_submodule_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@@ -1273,39 +1327,35 @@ 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 get_next_submodule_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\ttask = get_next_submodule_task_create(spf->r, ce->name);\n+\n+\t\tif (!task->sub) {\n+\t\t\tfree(task);\n+\t\t\tcontinue;\n \t\t}\n \n-\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\trecurse_config = get_fetch_recurse_config(task->sub, spf);\n+\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@@ -1316,12 +1366,12 @@ 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\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n-\t\t\tcp->dir = xstrdup(repo->gitdir);\n+\t\t\tcp->dir = task->repo->gitdir;\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@@ -1330,18 +1380,51 @@ 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}\n-\t\tstrbuf_release(&submodule_prefix);\n-\t\tif (ret) {\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+\t\t\tget_next_submodule_task_release(task);\n+\t\t\tfree(task);\n \t\t}\n \t}\n+\n+\tif (spf->retry_nr) {\n+\t\tstruct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tspf->retry_nr--;\n+\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", 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 s--h */\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\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1349,20 +1432,68 @@ 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 get_next_submodule_task *task = task_cb;\n \n \tspf->result = 1;\n \n+\tget_next_submodule_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 get_next_submodule_task *task = task_cb;\n+\tconst struct submodule *sub;\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)\n+\t\treturn 0;\n+\n+\tsub = task->sub;\n+\tif (!sub)\n+\t\tgoto out;\n+\n+\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n+\tif (!it)\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 that do not exist? */\n+\tif (commits->nr) {\n+\t\t/* We already tried fetching them, do not try again. */\n+\t\tif (task->commits)\n+\t\t\treturn 0;\n+\n+\t\ttask->commits = commits;\n+\t\tALLOC_GROW(spf->retry, spf->retry_nr + 1, spf->retry_alloc);\n+\t\tspf->retry[spf->retry_nr] = task;\n+\t\tspf->retry_nr++;\n+\t\treturn 0;\n+\t}\n+\n+out:\n+\tget_next_submodule_task_release(task);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 42692219a1..af12c50e7d 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -605,4 +605,20 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n+\tgit checkout --detach &&\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+\tgit commit -m \"updated submodule outside of refs/heads\" &&\n+\tD=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/2 $D &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.19.0\n\n"},{"id":"360655","messageId":"20181016181327.107186-10-sbeller@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"[PATCH 9/9] builtin/fetch: check for submodule updates for non branch fetches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T18:13:27Z","receivedAt":"2018-10-16T18:14:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Gerrit, the code review tool, has a different workflow than our mailing\nlist based approach. Usually users upload changes to a Gerrit server and\ncontinuous integration and testing happens by bots. Sometimes however a\nuser wants to checkout a change locally and look at it locally. For this\nuse case, Gerrit offers a command line snippet to copy and paste to your\nterminal, which looks like\n\n git fetch https:///gerrit refs/changes/ &&\n git checkout FETCH_HEAD\n\nFor Gerrit changes that contain changing submodule gitlinks, it would be\neasy to extend both the fetch and checkout with the '--recurse-submodules'\nflag, such that this command line snippet would produce the state of a\nchange locally.\n\nHowever the functionality added in the previous patch, which would\nensure that we fetch the objects in the submodule that the gitlink pointed\nat, only works for remote tracking branches so far, not for FETCH_HEAD.\n\nMake sure that fetching a superproject to its FETCH_HEAD, also respects\nthe existence checks for objects in the submodule recursion.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/fetch.c | 5 ++++-\n t/t5526-fetch-submodules.sh | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 95c44bf6ff..ea6ecd123e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -887,11 +887,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\trc |= update_local_ref(ref, what, rm, ¬e,\n \t\t\t\t\t\t summary_width);\n \t\t\t\tfree(ref);\n-\t\t\t} else\n+\t\t\t} else {\n+\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n \t\t\t\tformat_display(¬e, '*',\n \t\t\t\t\t *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t *what ? what : \"HEAD\",\n \t\t\t\t\t \"FETCH_HEAD\", summary_width);\n+\t\t\t}\n+\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex af12c50e7d..a509eabb04 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -615,7 +615,7 @@ test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n \tgit update-ref refs/changes/2 $D &&\n \t(\n \t\tcd downstream &&\n-\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/2 &&\n \t\tgit -C submodule cat-file -t $C &&\n \t\tgit checkout --recurse-submodules FETCH_HEAD\n \t)\n-- \n2.19.0\n\n"},{"id":"360785","messageId":"20181017212133.195680-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-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-10-17T21:21:33Z","receivedAt":"2018-10-17T21:21:39Z","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\nThis confused me at first, because neither function is mentioned in the\npatch.\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\nIt took me some time to see that you were rejecting the two solutions\nyou listed in the first paragraph, and are instead using a third (that\nyou describe in this paragraph).\n\nThe code itself looks fine.\n\nIn the future, I think that it's better if this type of patch went into\nits own patch set - this seems independent of the concerns of this patch\nset, so splitting up keeps patch sets small.\n"},{"id":"360786","messageId":"20181017212624.196598-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-5-sbeller@google.com","subject":"Re: [PATCH 4/9] submodule.c: move global changed_submodule_names into fetch submodule struct","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-17T21:26:24Z","receivedAt":"2018-10-17T21:26:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> The `changed_submodule_names` are only used for fetching, so let's make it\n> part of the struct that is passed around for fetching submodules.\n\nKeep the titles of commit messages to 50 characters or under.\n\n> +static void calculate_changed_submodule_paths(\n> +\tstruct submodule_parallel_fetch *spf)\n\nInstead of taking the entire struct, could this just take the list of\nchanged submodule names instead?\n"},{"id":"360787","messageId":"20181017214534.199890-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-6-sbeller@google.com","subject":"Re: [PATCH 5/9] submodule.c: do not copy around submodule list","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-17T21:45:34Z","receivedAt":"2018-10-17T21:45:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> By doing so we'll have access to the util pointer for longer that\n> contains the commits that we need to fetch, which will be\n> useful in a later patch.\n\nIt seems that the main point of this patch is so that the OIDs be stored\nin changed_submodule_names, because you need them in a later patch. If\nso, I would have expected a commit title like \"submodule: store OIDs in\nchanged_submodule_names\".\n\n> @@ -1142,8 +1142,7 @@ static void calculate_changed_submodule_paths(\n> \tstruct submodule_parallel_fetch *spf)\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\nPrior to this patch, changed_submodules is here just so that we know\nwhat to add to changed_submodule_names (it will be cleared at the end of\nthe function). So removing it is fine.\n\n> -\tcollect_changed_submodules(&changed_submodules, &argv);\n> +\tcollect_changed_submodules(&spf->changed_submodule_names, &argv);\n> \n> -\tfor_each_string_list_item(name, &changed_submodules) {\n> +\tfor_each_string_list_item(name, &spf->changed_submodule_names) {\n\nWe add all the changed submodules directly to changed_submodule_names,\nand iterate over it. So we use changed_submodule_names as a\nscratchpad...\n\n> -\t\tif (!submodule_has_commits(path, commits))\n> -\t\t\tstring_list_append(&spf->changed_submodule_names,\n> -\t\t\t\t\t name->string);\n> +\t\tif (submodule_has_commits(path, commits)) {\n> +\t\t\toid_array_clear(commits);\n> +\t\t\t*name->string = '\\0';\n> +\t\t}\n\n...but this is fine because previously, we appended the name->string to\nchanged_submodule_names (with no util) whereas now, we make the\nname->string empty in the opposite condition.\n\nBefore this patch, at the end of the loop, we had all the wanted\nsubmodule names with no util in changed_submodule_names. With this\npatch, at the end of the loop, we have all the wanted submodule names\nwith util pointing to an OID array, and also some blanks with util\npointing to a cleared OID array.\n\n> -\tfree_submodules_oids(&changed_submodules);\n> +\tstring_list_remove_empty_items(&spf->changed_submodule_names, 1);\n\nThe local variable changed_submodules no longer exists, so removing that\nline is fine.\n\nAnd we remove all the blanks from changed_submodule_names.\n\n> @@ -1377,7 +1378,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\nAnd because changed_submodule_names now has a non-trivial util pointer,\nwe need to free it properly.\n\nThis patch looks good, except that the commit title and message could\nperhaps be clearer.\n"},{"id":"360788","messageId":"20181017215545.202278-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-7-sbeller@google.com","subject":"Re: [PATCH 6/9] repository: repo_submodule_init to take a submodule struct","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-17T21:55:45Z","receivedAt":"2018-10-17T21:55:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> When constructing a struct repository for a submodule for some revision\n> of the superproject where the submodule is not contained in the index,\n> it may not be present in the working tree currently either. In that\n> siutation giving a 'path' argument is not useful. Upgrade the\n> repo_submodule_init function to take a struct submodule instead.\n\nAre there ways for other code to create a struct submodule without using\nsubmodule_from_path()? If yes, maybe outline them here and say that this\nmakes repo_submodule_init() more useful, since it can now be used with\nthose methods.\n\n> While we are at it, overhaul the repo_submodule_init function by renaming\n> the submodule repository struct, which is to be initialized, to a name\n> that is not confused with the struct submodule as easily.\n\n\"Overhaul\" is probably not the right word for just a rename of a local\nvariable. Also mention the other functions in which you did this rename\n(or just say \"repo_submodule_init() and other functions\").\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.\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\nFrom this description, I would expect \"sub\" to not be allowed to be\nNULL, but from the code I don't think that's the case. Should we\nprohibit NULL (and add checks to all repo_submodule_init()'s callers) or\ndocument that a NULL sub is allowed (and what happens in that case)?\n"},{"id":"360792","messageId":"20181017225811.66554-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-8-sbeller@google.com","subject":"Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-17T22:58:11Z","receivedAt":"2018-10-17T22:58:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> This patch started as a refactoring to make 'get_next_submodule' more\n> readable, but upon doing so, I realized that \"git fetch\" of the submodule\n> actually doesn't need to be run in the submodules worktree. So let's run\n> it in its git dir instead.\n\nThe commit message needs to be updated, I think - this patch does\nsignificantly more than fetching in the gitdir.\n\n> This patch leaks the cp->dir in get_next_submodule, as any further\n> callback in run_processes_parallel doesn't have access to the child\n> process any more.\n\nThe cp->dir is already leaked - probably better to write \"cp->dir in\nget_next_submodule() is still leaked, but this will be fixed in a\nsubsequent patch\".\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\nWhy does GIT_DIR need to be set? Is it to avoid subcommands recursively\nchecking the parent directories in case the CWD is a malformed Git\nrepository? If yes, maybe it's worth adding a comment.\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\treturn NULL;\n> +\t\t}\n> +\t\tstrbuf_release(&gitdir);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\nThis is the significant thing that this patch does more - an unskipped\nsubmodule is now something that either passes the checks in\nrepo_submodule_init() or the checks in repo_init(), which seems to be\nstricter than the current check that \".git\" points to a directory or is\none. This means that we skip certain broken repositories, and this\nnecessitates a change in the test.\n\nI think we should be more particular about what we're allowed to skip -\nin particular, maybe if we're planning to skip this submodule, its\ncorresponding directory in the worktree (if one exists) needs to be\nempty.\n\n> -\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n> -\t\t\tprepare_submodule_repo_env(&cp->env_array);\n> +\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> +\t\t\tcp->dir = xstrdup(repo->gitdir);\n\nHere is where the functionality change (fetch in \".git\") described in\nthe commit message occurs.\n"},{"id":"360799","messageId":"20181018003954.139498-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-9-sbeller@google.com","subject":"Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-18T00:39:54Z","receivedAt":"2018-10-18T00:40:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> 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> This works surprisingly well in some workflows (such as using submodules\n> as a third party library), while not so well in other scenarios, such\n> as in a Gerrit topic-based workflow, that can tie together changes\n> (potentially across repositories) on the server side. One of the parts\n> of such a Gerrit workflow is to download a change when wanting to examine\n> it, and you'd want to have its submodule changes that are in the same\n> topic downloaded as well. However these submodule changes reside in their\n> own repository in their own ref (refs/changes/).\n\nIt seems that the pertinent situation is when, in the superproject, you\nfetch a commit (which may be the target of a ref, or an ancestor of the\ntarget of a ref) that points to a submodule commit that was not fetched\nby the default refspec-less fetch that you describe in the first\nparagraph. I would just describe it as follows:\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\nAnother thing you need to clarify is what happens if the fetch-by-commit\nfails. Right now, it seems that it will make the whole thing fail, which\nmight be a surprising change in behavior.\n\n> Retry fetching a submodule by object name if the object id that the\n> superproject points to, cannot be found.\n\nI don't really think of this as a retry - the first time, you're\nfetching the default refspec, and the second time, with a specific list\nof object IDs. Also, be consistent in the usage of \"object name\" and\n\"object id\" - as far as I know, they are the same thing.\n\n> This retrying 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\nThe test stores the result in a normal branch, not a remote tracking\nbranch. Is storing in a normal branch required?\n\nAlso, do you know why this is required? A naive reading of the patch\nleads me to believe that this should work even if merely fetching to\nFETCH_HEAD.\n\n> +struct get_next_submodule_task {\n> +\tstruct repository *repo;\n> +\tconst struct submodule *sub;\n> +\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n> +\tstruct oid_array *commits;\n> +};\n\nFirstly, I don't think \"commits\" needs to be a pointer.\n\nDocument at least \"commits\". As far as I can tell, if NULL (or empty if\nyou take my suggestion), this means that this task is the first pass for\nthis particular submodule and the fetch needs to be done using the\ndefault refspec. Otherwise, this task is the second pass for this\nparticular submodule and the fetch needs to be done passing the\ncontained OIDs.\n\n> +static const struct submodule *get_default_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\nWhat is a \"default\" submodule and why would you need one?\n\n> +\t\ttask = get_next_submodule_task_create(spf->r, ce->name);\n> +\n> +\t\tif (!task->sub) {\n> +\t\t\tfree(task);\n> +\t\t\tcontinue;\n> \t\t}\n\nWill task->sub ever be NULL?\n\n> +\tif (spf->retry_nr) {\n> +\t\tstruct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n> +\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n> +\t\tspf->retry_nr--;\n> +\n> +\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", 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\nThis means that we need to trust that the last entry in spf->args can\ntake an \"on-demand\" parameter. Could we supply that argument here\nexplicitly instead?\n\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 s--h */\n\nWhat is s--h?\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\nShouldn't the function name be commit_missing_in_sub?\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 get_next_submodule_task *task = task_cb;\n> +\tconst struct submodule *sub;\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)\n> +\t\treturn 0;\n\nWhen will task be NULL?\n\n> +\n> +\tsub = task->sub;\n> +\tif (!sub)\n> +\t\tgoto out;\n\nSame as above - when will sub be NULL?\n\n> +\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n> +\tif (!it)\n> +\t\tgoto out;\n\nAnd \"it\" as well.\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 that do not exist? */\n> +\tif (commits->nr) {\n> +\t\t/* We already tried fetching them, do not try again. */\n> +\t\tif (task->commits)\n> +\t\t\treturn 0;\n\nClearer and more efficient if the check for task->commits was first\nbefore all the filtering.\n\n> +test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n\n\"not reachable\" confused me - they are indeed reachable, just not from\nthe default refspec.\n\n> +\tgit checkout --detach &&\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> +\tgit commit -m \"updated submodule outside of refs/heads\" &&\n> +\tD=$(git rev-parse HEAD) &&\n> +\tgit update-ref refs/changes/2 $D &&\n> +\t(\n> +\t\tcd downstream &&\n> +\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n> +\t\tgit -C submodule cat-file -t $C &&\n> +\t\tgit checkout --recurse-submodules FETCH_HEAD\n> +\t)\n> +'\n\nFor maximum test coverage, I think this test should involve 2\nsubmodules:\n\n B C E F\n \\ / \\ /\n A D\n\nand the upstream superproject having:\n\n G -> H -> I\n\nThe downstream superproject already has G, and is fetching I. In H, the\nsubmodule gitlinks point to B and E respectively, and in I, the\nsubmodule gitlinks point to C and F respectively. This ensures that both\nmultiple submodules work, and that submodule commits are also fetched\nfor interior superproject commits.\n"},{"id":"360800","messageId":"20181018004732.141825-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"20181016181327.107186-10-sbeller@google.com","subject":"Re: [PATCH 9/9] builtin/fetch: check for submodule updates for non branch fetches","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-18T00:47:32Z","receivedAt":"2018-10-18T00:47:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> @@ -887,11 +887,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\t\trc |= update_local_ref(ref, what, rm, ¬e,\n> \t\t\t\t\t\t summary_width);\n> \t\t\t\tfree(ref);\n> -\t\t\t} else\n> +\t\t\t} else {\n> +\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n\nDoes this need to be guarded with a recurse_submodules check, just like\nin update_local_ref()?\n\nAlso, this warrants a comment - this is here because there is some code\nlater that requires the new submodule commits to be registered, and the\nother branch does not require it only because update_local_ref() calls\nit.\n\n> @@ -615,7 +615,7 @@ test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n> \tgit update-ref refs/changes/2 $D &&\n> \t(\n> \t\tcd downstream &&\n> -\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n> +\t\tgit fetch --recurse-submodules origin refs/changes/2 &&\n> \t\tgit -C submodule cat-file -t $C &&\n> \t\tgit checkout --recurse-submodules FETCH_HEAD\n> \t)\n\nI think there should be a new test - we can tell from the code that just\nbecause fetching to FETCH_HEAD works doesn't mean that fetching to a ref\nworks, and vice versa.\n\nAlso, can you make the test fetch 2 refs? So that we know that it works\nwith more than one.\n"},{"id":"360807","messageId":"xmqqbm7svvf8.fsf@gitster-ct.c.googlers.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"Re: [PATCH 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T02:30:03Z","receivedAt":"2018-10-18T02:30:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This is based on ao/submodule-wo-gitmodules-checked-out.\n\nThanks. I had an impression that we were not entirely happy with\nthe topic and are expecting a reroll, but let's hope that the part\nwe expect to be updated won't have much overlaps.\n\n> A range-diff below shows how picking a different base changed the patches,\n> apart from that no further adjustments have been made.\n\nThanks. Let's see how well this plays together with other topics in 'pu'.q\n"},{"id":"360808","messageId":"xmqq7eigvv6r.fsf@gitster-ct.c.googlers.com","threadId":"49575","inReplyTo":"20181017214534.199890-1-jonathantanmy@google.com","subject":"Re: [PATCH 5/9] submodule.c: do not copy around submodule list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T02:35:08Z","receivedAt":"2018-10-18T02:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> By doing so we'll have access to the util pointer for longer that\n>> contains the commits that we need to fetch, which will be\n>> useful in a later patch.\n>\n> It seems that the main point of this patch is so that the OIDs be stored\n> in changed_submodule_names, because you need them in a later patch. If\n> so, I would have expected a commit title like \"submodule: store OIDs in\n> changed_submodule_names\".\n>\n> ...\n> This patch looks good, except that the commit title and message could\n> perhaps be clearer.\n\nThanks for a thoughtful review; not just this step, but for your review\ncomments on all the other steps in the series are very helpful.\n\n"},{"id":"360828","messageId":"xmqqa7nb67a2.fsf@gitster-ct.c.googlers.com","threadId":"49575","inReplyTo":"20181016181327.107186-1-sbeller@google.com","subject":"Re: [PATCH 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T07:30:45Z","receivedAt":"2018-10-18T07:30:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This is based on ao/submodule-wo-gitmodules-checked-out.\n>\n> This resends origin/sb/submodule-recursive-fetch-gets-the-tip, resolving\n> the issues pointed out via origin/xxx/sb-submodule-recursive-fetch-gets-the-tip-in-pu\n> by basing this series on origin/ao/submodule-wo-gitmodules-checked-out\n\nApplying this round to the result of merging ao/submodule-* to\n'master' requires this to work, it seems, as you've introduced a\ncall to repo-init thing in the meantime with another topic.\n\nSubject: [PATCH] fixup! repository: repo_submodule_init to take a submodule struct\n\n---\n builtin/submodule--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5f8a804a6e..015aa1471f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2037,7 +2037,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)) {\n-- \n2.19.1-450-ga4b8ab5363\n\n"},{"id":"360855","messageId":"CAGZ79kapFJh+4rubbD4oNtzfceFc6yMWEixDu48vN7wqJGWMRw@mail.gmail.com","threadId":"49575","inReplyTo":"xmqqa7nb67a2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-18T18:00:59Z","receivedAt":"2018-10-18T18:01:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 18, 2018 at 12:30 AM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > This is based on ao/submodule-wo-gitmodules-checked-out.\n> >\n> > This resends origin/sb/submodule-recursive-fetch-gets-the-tip, resolving\n> > the issues pointed out via origin/xxx/sb-submodule-recursive-fetch-gets-the-tip-in-pu\n> > by basing this series on origin/ao/submodule-wo-gitmodules-checked-out\n>\n> Applying this round to the result of merging ao/submodule-* to\n> 'master' requires this to work, it seems, as you've introduced a\n> call to repo-init thing in the meantime with another topic.\n\nUnfortunately, yes, sb/submodule-update-in-c had one such call.\n\n> > This is based on ao/submodule-wo-gitmodules-checked-out.\n> Thanks. I had an impression that we were not entirely happy with\n> the topic and are expecting a reroll, but let's hope that the part\n> we expect to be updated won't have much overlaps.\n\nI slowly came to the realization that this topic might be ripped up\ninto 2 or more topics, as some of the cleanups seem to be orthogonal.\n"},{"id":"360867","messageId":"CAGZ79kZHhmoBf26z3_Lk-yQEXztpGdWX-FfsRnvaKRCwETZ9XA@mail.gmail.com","threadId":"49575","inReplyTo":"20181017212624.196598-1-jonathantanmy@google.com","subject":"Re: [PATCH 4/9] submodule.c: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-18T19:09:16Z","receivedAt":"2018-10-18T19:09:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 17, 2018 at 2:26 PM Jonathan Tan wrote:\n>\n> > The `changed_submodule_names` are only used for fetching, so let's make it\n> > part of the struct that is passed around for fetching submodules.\n>\n> Keep the titles of commit messages to 50 characters or under.\n\nrenamed\n\n>\n> > +static void calculate_changed_submodule_paths(\n> > + struct submodule_parallel_fetch *spf)\n>\n> Instead of taking the entire struct, could this just take the list of\n> changed submodule names instead?\n\nI think so, done.\n"},{"id":"361308","messageId":"CAGZ79kbunyc02802+4sjGwthBnj_=eS=+z5B_WQSRdbUAHw1tQ@mail.gmail.com","threadId":"49575","inReplyTo":"20181017225811.66554-1-jonathantanmy@google.com","subject":"Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-23T18:26:32Z","receivedAt":"2018-10-23T18:26:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 17, 2018 at 3:58 PM Jonathan Tan wrote:\n>\n> > This patch started as a refactoring to make 'get_next_submodule' more\n> > readable, but upon doing so, I realized that \"git fetch\" of the submodule\n> > actually doesn't need to be run in the submodules worktree. So let's run\n> > it in its git dir instead.\n>\n> The commit message needs to be updated, I think - this patch does\n> significantly more than fetching in the gitdir.\n\nFrom my point of view, it is not significant, but refactoring.\nI'll think how to write a better commit message.\n\n> > This patch leaks the cp->dir in get_next_submodule, as any further\n> > callback in run_processes_parallel doesn't have access to the child\n> > process any more.\n>\n> The cp->dir is already leaked - probably better to write \"cp->dir in\n> get_next_submodule() is still leaked, but this will be fixed in a\n> subsequent patch\".\n\n... which fails to mention the reason why (as it is hard to do given\nthe current design) but is more concise.\n\n> > +static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)\n> > +{\n> > + prepare_submodule_repo_env_no_git_dir(out);\n> > + argv_array_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n>\n> Why does GIT_DIR need to be set? Is it to avoid subcommands recursively\n> checking the parent directories in case the CWD is a malformed Git\n> repository? If yes, maybe it's worth adding a comment.\n\nIt is copying the structure from prepare_submodule_repo_env,\nspecifically 10f5c52656 (submodule: avoid auto-discovery in\nprepare_submodule_repo_env(), 2016-09-01), which sounds\nappealing (and brings real benefits for the working directory),\nbut I have not thought about this protection for the git dir.\n\nMaybe another approach is to not set the cwd for the child process\nand instead point GIT_DIR_ENVIRONMENT only to the right\ndirectory.\n\nThen the use of GIT_DIR_ENVIRONMENT is obvious and\nis not just for protection of corner cases.\n\nHowever I think this protection is really valuable for the\n.git dir as well as the submodule may be broken and we do not\nwant to end up in an infinite loop (as the discovery would find\nthe superproject which then tries to recurse, again, into the\nsubmodule with the broken git dir)\n\nWhen adding the comment here, we'd also want to have\nthe comment in prepare_submodule_repo_env, which\ncould be its own preparation commit.\n\n> > +static struct repository *get_submodule_repo_for(struct repository *r,\n> > + const struct submodule *sub)\n> > +{\n> > + struct repository *ret = xmalloc(sizeof(*ret));\n> > +\n> > + if (repo_submodule_init(ret, r, sub)) {\n> > + /*\n> > + * No entry in .gitmodules? Technically not a submodule,\n> > + * but historically we supported repositories that happen to be\n> > + * in-place where a gitlink is. Keep supporting them.\n> > + */\n> > + struct strbuf gitdir = STRBUF_INIT;\n> > + strbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", sub->path);\n> > + if (repo_init(ret, gitdir.buf, NULL)) {\n> > + strbuf_release(&gitdir);\n> > + return NULL;\n> > + }\n> > + strbuf_release(&gitdir);\n> > + }\n> > +\n> > + return ret;\n> > +}\n>\n> This is the significant thing that this patch does more - an unskipped\n> submodule is now something that either passes the checks in\n> repo_submodule_init() or the checks in repo_init(), which seems to be\n> stricter than the current check that \".git\" points to a directory or is\n> one. This means that we skip certain broken repositories, and this\n> necessitates a change in the test.\n\nI see. However there is no change in function, the check in repo_init\n(or repo_submodule_init) is less strict than the check in the child process.\nSo if there are broken submodule repositories, the difference of this\npatch is the layer at which it is caught, i.e. we would not spawn a child\nthat fails, but skip the submodule.\n\nThinking of that, maybe we need to announce that in get_next_submodule\n\n>\n> I think we should be more particular about what we're allowed to skip -\n> in particular, maybe if we're planning to skip this submodule, its\n> corresponding directory in the worktree (if one exists) needs to be\n> empty.\n\nIf the working tree directory is empty for that submodule, it means\nit is likely not initialized. But why would we use that as a signal to\nskip the submodule?\n\n\n\n> > - cp->dir = strbuf_detach(&submodule_path, NULL);\n> > - prepare_submodule_repo_env(&cp->env_array);\n> > + prepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> > + cp->dir = xstrdup(repo->gitdir);\n>\n> Here is where the functionality change (fetch in \".git\") described in\n> the commit message occurs.\n\nTrue.\n\nThanks for the review, I'll try to split up this commit a bit more and\nexplain each part on its own.\n"},{"id":"361335","messageId":"CAGZ79kZrK5G-EeGRzxyw0xW3ozo9_aPab4r8fn_Jc4hzKDOEwg@mail.gmail.com","threadId":"49575","inReplyTo":"20181018003954.139498-1-jonathantanmy@google.com","subject":"Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-23T22:37:48Z","receivedAt":"2018-10-23T22:38:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 17, 2018 at 5:40 PM Jonathan Tan wrote:\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> > This works surprisingly well in some workflows (such as using submodules\n> > as a third party library), while not so well in other scenarios, such\n> > as in a Gerrit topic-based workflow, that can tie together changes\n> > (potentially across repositories) on the server side. One of the parts\n> > of such a Gerrit workflow is to download a change when wanting to examine\n> > it, and you'd want to have its submodule changes that are in the same\n> > topic downloaded as well. However these submodule changes reside in their\n> > own repository in their own ref (refs/changes/).\n>\n> It seems that the pertinent situation is when, in the superproject, you\n> fetch a commit (which may be the target of a ref, or an ancestor of the\n> target of a ref) that points to a submodule commit that was not fetched\n> by the default refspec-less fetch that you describe in the first\n> paragraph. I would just describe it as follows:\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\nMakes sense.\n\n> Another thing you need to clarify is what happens if the fetch-by-commit\n> fails. Right now, it seems that it will make the whole thing fail, which\n> might be a surprising change in behavior.\n\nBut a positive surprise, I would assume?\n\n> The test stores the result in a normal branch, not a remote tracking\n> branch. Is storing in a normal branch required?\n\nIn the test we fetch from another repository, such that in the\nrepository-under-test this will show up in a remote tracking branch?\n\n> Also, do you know why this is required? A naive reading of the patch\n> leads me to believe that this should work even if merely fetching to\n> FETCH_HEAD.\n\nSee the next patch, check_for_new_submodule_commits() is missing\nfor FETCH_HEAD.\n\n>\n> > +struct get_next_submodule_task {\n> > + struct repository *repo;\n> > + const struct submodule *sub;\n> > + unsigned free_sub : 1; /* Do we need to free the submodule? */\n> > + struct oid_array *commits;\n> > +};\n>\n> Firstly, I don't think \"commits\" needs to be a pointer.\n>\n> Document at least \"commits\". As far as I can tell, if NULL (or empty if\n> you take my suggestion), this means that this task is the first pass for\n> this particular submodule and the fetch needs to be done using the\n> default refspec. Otherwise, this task is the second pass for this\n> particular submodule and the fetch needs to be done passing the\n> contained OIDs.\n\nMakes sense. I think I'll make it a non-pointer, but will introduce another\nflag or counter for the phase.\n\n>\n> > +static const struct submodule *get_default_submodule(const char *path)\n> > +{\n> > + struct submodule *ret = NULL;\n> > + const char *name = default_name_or_path(path);\n> > +\n> > + if (!name)\n> > + return NULL;\n> > +\n> > + ret = xmalloc(sizeof(*ret));\n> > + memset(ret, 0, sizeof(*ret));\n> > + ret->path = name;\n> > + ret->name = name;\n> > +\n> > + return (const struct submodule *) ret;\n> > +}\n>\n> What is a \"default\" submodule and why would you need one?\n\ns/default/artificial/. Such a submodule is a submodule that has no\nconfig in the .gitmodules file and its name == path.\nWe need to keep those around for historic reasons AFAICT, c.f.\nc68f837576 (implement fetching of moved submodules, 2017-10-16)\n\n> > + task = get_next_submodule_task_create(spf->r, ce->name);\n> > +\n> > + if (!task->sub) {\n> > + free(task);\n> > + continue;\n> > }\n>\n> Will task->sub ever be NULL?\n\nYes, if we fall back to these \"default\" submodule and just try if it\ncan be handled\nas a submodule, but it cannot be handled as such,\nget_next_submodule_task_create has\n\n task->sub = submodule_from_path(r, &null_oid, path);\n if (!task->sub) {\n task->sub = get_default_submodule(path);\n\nand get_default_submodule can return NULL.\n\n\n>\n> > + if (spf->retry_nr) {\n> > + struct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n> > + struct strbuf submodule_prefix = STRBUF_INIT;\n> > + spf->retry_nr--;\n> > +\n> > + strbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, task->sub->path);\n> > +\n> > + child_process_init(cp);\n> > + prepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> > + cp->git_cmd = 1;\n> > + cp->dir = task->repo->gitdir;\n> > +\n> > + argv_array_init(&cp->args);\n> > + argv_array_pushv(&cp->args, spf->args.argv);\n> > + argv_array_push(&cp->args, \"on-demand\");\n>\n> This means that we need to trust that the last entry in spf->args can\n> take an \"on-demand\" parameter. Could we supply that argument here\n> explicitly instead?\n>\n> > + argv_array_push(&cp->args, \"--submodule-prefix\");\n> > + argv_array_push(&cp->args, submodule_prefix.buf);\n> > +\n> > + /* NEEDSWORK: have get_default_remote from s--h */\n>\n> What is s--h?\n\nbuiltin/submodule--helper, will clarify\n\n>\n> > +static int commit_exists_in_sub(const struct object_id *oid, void *data)\n> > +{\n> > + struct repository *subrepo = data;\n> > +\n> > + enum object_type type = oid_object_info(subrepo, oid, NULL);\n> > +\n> > + return type != OBJ_COMMIT;\n> > +}\n>\n> Shouldn't the function name be commit_missing_in_sub?\n\nyes.\n\n>\n> > static int fetch_finish(int retvalue, struct strbuf *err,\n> > void *cb, void *task_cb)\n> > {\n> > struct submodule_parallel_fetch *spf = cb;\n> > + struct get_next_submodule_task *task = task_cb;\n> > + const struct submodule *sub;\n> > +\n> > + struct string_list_item *it;\n> > + struct oid_array *commits;\n> >\n> > if (retvalue)\n> > spf->result = 1;\n> >\n> > + if (!task)\n> > + return 0;\n>\n> When will task be NULL?\n>\n> > +\n> > + sub = task->sub;\n> > + if (!sub)\n> > + goto out;\n>\n> Same as above - when will sub be NULL?\n>\n> > + it = string_list_lookup(&spf->changed_submodule_names, sub->name);\n> > + if (!it)\n> > + goto out;\n>\n> And \"it\" as well.\n\nI'll add comments.\n\n>\n> > + commits = it->util;\n> > + oid_array_filter(commits,\n> > + commit_exists_in_sub,\n> > + task->repo);\n> > +\n> > + /* Are there commits that do not exist? */\n> > + if (commits->nr) {\n> > + /* We already tried fetching them, do not try again. */\n> > + if (task->commits)\n> > + return 0;\n>\n> Clearer and more efficient if the check for task->commits was first\n> before all the filtering.\n>\n> > +test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n>\n> \"not reachable\" confused me - they are indeed reachable, just not from\n> the default refspec.\n\nMakes sense\n\n>\n> > + git checkout --detach &&\n> > + C=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n> > + git -C submodule update-ref refs/changes/1 $C &&\n> > + git update-index --cacheinfo 160000 $C submodule &&\n> > + git commit -m \"updated submodule outside of refs/heads\" &&\n> > + D=$(git rev-parse HEAD) &&\n> > + git update-ref refs/changes/2 $D &&\n> > + (\n> > + cd downstream &&\n> > + git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n> > + git -C submodule cat-file -t $C &&\n> > + git checkout --recurse-submodules FETCH_HEAD\n> > + )\n> > +'\n>\n> For maximum test coverage, I think this test should involve 2\n> submodules:\n>\n> B C E F\n> \\ / \\ /\n> A D\n>\n> and the upstream superproject having:\n>\n> G -> H -> I\n>\n> The downstream superproject already has G, and is fetching I. In H, the\n> submodule gitlinks point to B and E respectively, and in I, the\n> submodule gitlinks point to C and F respectively. This ensures that both\n> multiple submodules work, and that submodule commits are also fetched\n> for interior superproject commits.\n\nok.\n"},{"id":"361336","messageId":"20181023225536.61508-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"CAGZ79kbunyc02802+4sjGwthBnj_=eS=+z5B_WQSRdbUAHw1tQ@mail.gmail.com","subject":"Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-23T22:55:36Z","receivedAt":"2018-10-23T22:55:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Why does GIT_DIR need to be set? Is it to avoid subcommands recursively\n> > checking the parent directories in case the CWD is a malformed Git\n> > repository? If yes, maybe it's worth adding a comment.\n> \n> It is copying the structure from prepare_submodule_repo_env,\n> specifically 10f5c52656 (submodule: avoid auto-discovery in\n> prepare_submodule_repo_env(), 2016-09-01), which sounds\n> appealing (and brings real benefits for the working directory),\n> but I have not thought about this protection for the git dir.\n> \n> Maybe another approach is to not set the cwd for the child process\n> and instead point GIT_DIR_ENVIRONMENT only to the right\n> directory.\n> \n> Then the use of GIT_DIR_ENVIRONMENT is obvious and\n> is not just for protection of corner cases.\n> \n> However I think this protection is really valuable for the\n> .git dir as well as the submodule may be broken and we do not\n> want to end up in an infinite loop (as the discovery would find\n> the superproject which then tries to recurse, again, into the\n> submodule with the broken git dir)\n> \n> When adding the comment here, we'd also want to have\n> the comment in prepare_submodule_repo_env, which\n> could be its own preparation commit.\n\nI agree with the protection. As for the preparation commit, I don't\nthink it's always the code author's responsibility to tidy up the\nsurrounding code, but since you're adding an identical comment here,\nit's probably worth it to add the comment there too.\n\n> > This is the significant thing that this patch does more - an unskipped\n> > submodule is now something that either passes the checks in\n> > repo_submodule_init() or the checks in repo_init(), which seems to be\n> > stricter than the current check that \".git\" points to a directory or is\n> > one. This means that we skip certain broken repositories, and this\n> > necessitates a change in the test.\n> \n> I see. However there is no change in function, the check in repo_init\n> (or repo_submodule_init) is less strict than the check in the child process.\n> So if there are broken submodule repositories, the difference of this\n> patch is the layer at which it is caught, i.e. we would not spawn a child\n> that fails, but skip the submodule.\n> \n> Thinking of that, maybe we need to announce that in get_next_submodule\n\nThe consequence of getting caught changes, though. Currently,\nspf->result is set to 1 whenever a child process fails. But in this\npatch, some of these repositories would be entirely skipped, meaning\nthat no child process is run, and spf->result is never modified.\n\n> > I think we should be more particular about what we're allowed to skip -\n> > in particular, maybe if we're planning to skip this submodule, its\n> > corresponding directory in the worktree (if one exists) needs to be\n> > empty.\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\nWhat I meant was: if empty, skip it completely. Otherwise, do the\nrepo_submodule_init() and repo_init() thing, and if they both fail, set\nspf->result to 1, preserving existing behavior.\n"},{"id":"361338","messageId":"CAGZ79kbNXD35ZwevjLZcrGsT=2hNcUPmVUWvP1RjsKSH0Gd3ww@mail.gmail.com","threadId":"49575","inReplyTo":"20181023225536.61508-1-jonathantanmy@google.com","subject":"Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-23T23:01:14Z","receivedAt":"2018-10-23T23:01:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 23, 2018 at 3:55 PM Jonathan Tan wrote:\n> > When adding the comment here, we'd also want to have\n> > the comment in prepare_submodule_repo_env, which\n> > could be its own preparation commit.\n>\n> I agree with the protection. As for the preparation commit, I don't\n> think it's always the code author's responsibility to tidy up the\n> surrounding code, but since you're adding an identical comment here,\n> it's probably worth it to add the comment there too.\n\nAm I the only one who dislikes inconsistent files? ;-)\n(ie. clean in one place, not cleaned up in another)\nI can see your point. Will add a comment\n\n> > Thinking of that, maybe we need to announce that in get_next_submodule\n>\n> The consequence of getting caught changes, though. Currently,\n> spf->result is set to 1 whenever a child process fails. But in this\n> patch, some of these repositories would be entirely skipped, meaning\n> that no child process is run, and spf->result is never modified.\n\nRight.\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\nI did it the other way round:\n\nIf repo_[submodule_]init fails, see if we have a gitlink in tree and\nan empty dir in the FS, to decide if we need to signal failure.\n\nI can switch it around again, but it seemed easier to write as\nthat puts corner cases away into one else {} case.\n"},{"id":"361341","messageId":"20181023233756.190026-1-jonathantanmy@google.com","threadId":"49575","inReplyTo":"CAGZ79kZrK5G-EeGRzxyw0xW3ozo9_aPab4r8fn_Jc4hzKDOEwg@mail.gmail.com","subject":"Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-23T23:37:56Z","receivedAt":"2018-10-23T23:49:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Another thing you need to clarify is what happens if the fetch-by-commit\n> > fails. Right now, it seems that it will make the whole thing fail, which\n> > might be a surprising change in behavior.\n> \n> But a positive surprise, I would assume?\n\nWhether positive or negative, I think that this needs to be mentioned in\nthe commit message.\n\nAs for positive or negative, I tend to agree that it's positive - sure,\nsome previously successful fetches would now fail, but the results of\nthose fetches could not be recursively checked out anyway.\n\n> > The test stores the result in a normal branch, not a remote tracking\n> > branch. Is storing in a normal branch required?\n> \n> In the test we fetch from another repository, such that in the\n> repository-under-test this will show up in a remote tracking branch?\n\nIf that were true, I would expect that when this line:\n\n> git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n\nis replaced by this line:\n\n> git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2 &&\n\nthen things would still work. The tests pass with the first line (after\nI fixed a type mismatch) but not with the second. (Also I don't think a\nremote-tracking branch is generated here - the output printed doesn't\nindicate so, and refs/changes/2 is not a branch anyway.)\n\n> > Also, do you know why this is required? A naive reading of the patch\n> > leads me to believe that this should work even if merely fetching to\n> > FETCH_HEAD.\n> \n> See the next patch, check_for_new_submodule_commits() is missing\n> for FETCH_HEAD.\n\nI see in the next patch that there is an \"if\" branch in\nstore_updated_refs() without update_local_ref() in which\n\"check_for_new_submodule_commits(&rm->old_oid)\" needs to be inserted. I\nthink this is a symptom that maybe check_for_new_submodule_commits()\nneeds to be extracted from update_local_ref() and put into\nstore_updated_refs() instead? In update_local_ref(), it is called on\nref->new_oid, which is actually the same as rm->old_oid anyway (there is\nan oidcpy earlier).\n\n> > > +static const struct submodule *get_default_submodule(const char *path)\n> > > +{\n> > > + struct submodule *ret = NULL;\n> > > + const char *name = default_name_or_path(path);\n> > > +\n> > > + if (!name)\n> > > + return NULL;\n> > > +\n> > > + ret = xmalloc(sizeof(*ret));\n> > > + memset(ret, 0, sizeof(*ret));\n> > > + ret->path = name;\n> > > + ret->name = name;\n> > > +\n> > > + return (const struct submodule *) ret;\n> > > +}\n> >\n> > What is a \"default\" submodule and why would you need one?\n> \n> s/default/artificial/. Such a submodule is a submodule that has no\n> config in the .gitmodules file and its name == path.\n> We need to keep those around for historic reasons AFAICT, c.f.\n> c68f837576 (implement fetching of moved submodules, 2017-10-16)\n\nAh, OK. I would call it a fake submodule then, and copy over the \"No\nentry in .gitmodules?\" comment.\n\n> > Will task->sub ever be NULL?\n> \n> Yes, if we fall back to these \"default\" submodule and just try if it\n> can be handled\n> as a submodule, but it cannot be handled as such,\n> get_next_submodule_task_create has\n> \n> task->sub = submodule_from_path(r, &null_oid, path);\n> if (!task->sub) {\n> task->sub = get_default_submodule(path);\n> \n> and get_default_submodule can return NULL.\n\nAh, yes you're right.\n"},{"id":"361564","messageId":"CAGZ79kY8bXR2OsdmKMyB+OJP67sWCF=N4KfUdUcpKy5ZQwcV9A@mail.gmail.com","threadId":"49575","inReplyTo":"20181023233756.190026-1-jonathantanmy@google.com","subject":"Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T21:42:15Z","receivedAt":"2018-10-25T21:42:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 23, 2018 at 4:38 PM Jonathan Tan wrote:\n>\n> > > Another thing you need to clarify is what happens if the fetch-by-commit\n> > > fails. Right now, it seems that it will make the whole thing fail, which\n> > > might be a surprising change in behavior.\n> >\n> > But a positive surprise, I would assume?\n>\n> Whether positive or negative, I think that this needs to be mentioned in\n> the commit message.\n>\n> As for positive or negative, I tend to agree that it's positive - sure,\n> some previously successful fetches would now fail, but the results of\n> those fetches could not be recursively checked out anyway.\n>\n> > > The test stores the result in a normal branch, not a remote tracking\n> > > branch. Is storing in a normal branch required?\n> >\n> > In the test we fetch from another repository, such that in the\n> > repository-under-test this will show up in a remote tracking branch?\n\nI messed up there. Yes, we need to fetch into a normal branch\nsuch that the logic of check_for_new_submodule_commits triggers\nno matter where it is on the remote.\n\nYour experiment below shows that we cannot fetch into FETCH_HEAD:\n\n> If that were true, I would expect that when this line:\n>\n> > git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n>\n> is replaced by this line:\n>\n> > git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2 &&\n>\n> then things would still work. The tests pass with the first line (after\n> I fixed a type mismatch) but not with the second. (Also I don't think a\n> remote-tracking branch is generated here - the output printed doesn't\n> indicate so, and refs/changes/2 is not a branch anyway.)\n\n> > > Also, do you know why this is required? A naive reading of the patch\n> > > leads me to believe that this should work even if merely fetching to\n> > > FETCH_HEAD.\n> >\n> > See the next patch, check_for_new_submodule_commits() is missing\n> > for FETCH_HEAD.\n>\n> I see in the next patch that there is an \"if\" branch in\n> store_updated_refs() without update_local_ref() in which\n> \"check_for_new_submodule_commits(&rm->old_oid)\" needs to be inserted. I\n> think this is a symptom that maybe check_for_new_submodule_commits()\n> needs to be extracted from update_local_ref() and put into\n> store_updated_refs() instead? In update_local_ref(), it is called on\n> ref->new_oid, which is actually the same as rm->old_oid anyway (there is\n> an oidcpy earlier).\n\nI'll look into that.\n\n> > > What is a \"default\" submodule and why would you need one?\n> >\n> > s/default/artificial/. Such a submodule is a submodule that has no\n> > config in the .gitmodules file and its name == path.\n> > We need to keep those around for historic reasons AFAICT, c.f.\n> > c68f837576 (implement fetching of moved submodules, 2017-10-16)\n>\n> Ah, OK. I would call it a fake submodule then, and copy over the \"No\n> entry in .gitmodules?\" comment.\n\n\"fake submodule\" sounds like\nhttp://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb\nwhich is what I think of when hearing fake submodules.\n"}]}