{"thread":{"id":"49321","subject":"[PATCH 0/9] fetch: make sure submodule oids are fetched","startedAt":"2018-09-11T23:49:59Z","lastAt":"2018-09-21T22:08:27Z","messageCount":33,"participants":["Stefan Beller","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"357929","messageId":"20180911234951.14129-1-sbeller@google.com","threadId":"49321","inReplyTo":null,"subject":"[PATCH 0/9] fetch: make sure submodule oids are fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:42Z","receivedAt":"2018-09-11T23:49:59Z","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 \" (and 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, not so well in others,\nwhich this series aims to fix.\n\nThe first patches provide new basic functionality and do some refactoring;\nthe interesting part is in the two last patches.\n\nThis was discussed in\nhttps://public-inbox.org/git/20180808221752.195419-1-sbeller@google.com/\nand I think I addressed all feedback so far.\n\nThanks,\nStefan\n\nStefan Beller (9):\n string-list: add string_list_{pop, last} functions\n sha1-array: provide oid_array_filter\n submodule.c: fix indentation\n submodule.c: sort changed_submodule_names before searching it\n submodule: move global changed_submodule_names into fetch submodule\n struct\n submodule.c: do not copy around submodule list\n submodule: fetch in submodules git directory instead of in worktree\n fetch: retry fetching submodules if sha1 were not fetched\n builtin/fetch: check for submodule updates for non branch fetches\n\n builtin/fetch.c | 14 +--\n sha1-array.c | 18 ++++\n sha1-array.h | 5 +\n string-list.c | 14 +++\n string-list.h | 11 +++\n submodule.c | 189 ++++++++++++++++++++++++++++--------\n t/t5526-fetch-submodules.sh | 23 ++++-\n 7 files changed, 227 insertions(+), 47 deletions(-)\n\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357930","messageId":"20180911234951.14129-2-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCH 1/9] string-list: add string_list_{pop, last} functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:43Z","receivedAt":"2018-09-11T23:50:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Add a few functions to allow a string-list to be used as a stack:\n\n - string_list_last() lets a caller peek the string_list_item at the\n end of the string list. The caller needs to be aware that it is\n borrowing a pointer, which can become invalid if/when the\n string_list is resized.\n\n - string_list_pop() removes the string_list_item at the end of\n the string list.\n\n - _pop usually has a friend _push. This role is taken by\n string_list_append already, as they are not symmetrical\n in our code base: _append returns the pointer, such that\n adding a util is easy, but _pop doesn't return such a pointer.\n\nYou can use them in this pattern:\n\n while (list.nr) {\n struct string_list_item *item = string_list_last(&list);\n\n work_on(item);\n string_list_pop(&list);\n }\n\nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n string-list.c | 14 ++++++++++++++\n string-list.h | 11 +++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/string-list.c b/string-list.c\nindex 771c4550980..04db2b537c0 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char *string,\n \t}\n }\n \n+void string_list_pop(struct string_list *list, int free_util)\n+{\n+\tif (list->nr == 0)\n+\t\tBUG(\"tried to remove an item from empty string list\");\n+\n+\tif (list->strdup_strings)\n+\t\tfree(list->items[list->nr - 1].string);\n+\n+\tif (free_util)\n+\t\tfree(list->items[list->nr - 1].util);\n+\n+\tlist->nr--;\n+}\n+\n int string_list_has_string(const struct string_list *list, const char *string)\n {\n \tint exact_match;\ndiff --git a/string-list.h b/string-list.h\nindex ff8f6094a33..ce2528bbe15 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -191,6 +191,17 @@ extern void string_list_remove(struct string_list *list, const char *string,\n */\n struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n \n+/**\n+ * Removes the last item from the list.\n+ * The caller must ensure that the list is not empty.\n+ */\n+void string_list_pop(struct string_list *list, int free_util);\n+\n+static inline struct string_list_item *string_list_last(struct string_list *list)\n+{\n+\treturn &list->items[list->nr - 1];\n+}\n+\n /*\n * Remove all but the first of consecutive entries with the same\n * string value. If free_util is true, call free() on the util\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357931","messageId":"20180911234951.14129-3-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCH 2/9] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:44Z","receivedAt":"2018-09-11T23:50:04Z","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 \n---\n sha1-array.c | 18 ++++++++++++++++++\n sha1-array.h | 5 +++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/sha1-array.c b/sha1-array.c\nindex 265941fbf40..76323935dd7 100644\n--- a/sha1-array.c\n+++ b/sha1-array.c\n@@ -77,3 +77,21 @@ int oid_array_for_each_unique(struct oid_array *array,\n \t}\n \treturn 0;\n }\n+\n+int oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn fn,\n+\t\t void *cbdata)\n+{\n+\tint src, dst;\n+\n+\tfor (src = dst = 0; src < array->nr; src++) {\n+\t\tif (fn(&array->oid[src], cbdata)) {\n+\t\t\tif (dst < src)\n+\t\t\t\toidcpy(&array->oid[dst], &array->oid[src]);\n+\t\t\tdst++;\n+\t\t}\n+\t}\n+\tarray->nr = dst;\n+\n+\treturn 0;\n+}\ndiff --git a/sha1-array.h b/sha1-array.h\nindex 232bf950172..a2d7c210835 100644\n--- a/sha1-array.h\n+++ b/sha1-array.h\n@@ -23,4 +23,9 @@ 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 \n+/* Call fn for each oid, and remove it if fn returns 0, retain it otherwise */\n+int oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn fn,\n+\t\t void *cbdata);\n+\n #endif /* SHA1_ARRAY_H */\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357932","messageId":"20180911234951.14129-4-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCH 3/9] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:45Z","receivedAt":"2018-09-11T23:50:07Z","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 \n---\n submodule.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex a2b266fbfae..d29dfa3d1f5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1244,7 +1244,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@@ -1254,8 +1255,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.397.gdd90340f6a-goog\n\n"},{"id":"357933","messageId":"20180911234951.14129-5-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCH 4/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:46Z","receivedAt":"2018-09-11T23:50:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We can string_list_insert() to maintain sorted-ness of the\nlist as we find new items, or we can string_list_append() to\nbuild an unsorted list and sort it at the end just once.\n\nTo pick which one is more appropriate, we notice the fact\nthat we discover new items more or less in the already\nsorted order. That makes \"append then sort\" more\nappropriate.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d29dfa3d1f5..c6eff7699f3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1256,7 +1256,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@@ -1350,6 +1350,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.397.gdd90340f6a-goog\n\n"},{"id":"357934","messageId":"20180911234951.14129-6-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCH 5/9] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:47Z","receivedAt":"2018-09-11T23:50:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `changed_submodule_names` are only used for fetching, so let's make it\npart of the struct that is passed around for fetching submodules.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex c6eff7699f3..3520dd76bdf 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@@ -1110,7 +1110,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@@ -1148,7 +1163,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@@ -1185,18 +1201,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@@ -1257,7 +1261,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@@ -1349,8 +1353,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@@ -1359,7 +1363,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.397.gdd90340f6a-goog\n\n"},{"id":"357935","messageId":"20180911234951.14129-7-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCH 6/9] submodule.c: do not copy around submodule list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:48Z","receivedAt":"2018-09-11T23:50:15Z","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 doin 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 \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3520dd76bdf..00a9a3c6b12 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1128,8 +1128,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@@ -1146,9 +1145,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@@ -1162,12 +1161,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@@ -1363,7 +1364,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.397.gdd90340f6a-goog\n\n"},{"id":"357936","messageId":"20180911234951.14129-8-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-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-09-11T23:49:49Z","receivedAt":"2018-09-11T23:50:19Z","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 actually doesn't\nneed to be run in the worktree. So let's run it in the git dir instead.\n\nThat should pave the way towards fetching submodules that are currently\nnot checked out.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 43 ++++++++++++++++++++++++++-----------\n t/t5526-fetch-submodules.sh | 7 +++++-\n 2 files changed, 37 insertions(+), 13 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 00a9a3c6b12..1e6781504f0 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -481,6 +481,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@@ -1227,6 +1233,27 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+static const char *get_submodule_git_dir(struct repository *r, const char *path)\n+{\n+\tstruct repository subrepo;\n+\tconst char *ret;\n+\n+\tif (repo_submodule_init(&subrepo, r, path)) {\n+\t\t/* no entry in .gitmodules? */\n+\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", path);\n+\t\tif (repo_init(&subrepo, gitdir.buf, NULL)) {\n+\t\t\tstrbuf_release(&gitdir);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\n+\tret = xstrdup(subrepo.gitdir);\n+\trepo_clear(&subrepo);\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@@ -1234,8 +1261,6 @@ 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@@ -1274,16 +1299,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\tgit_dir = get_submodule_git_dir(spf->r, ce->name);\n+\t\tif (git_dir) {\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 = git_dir;\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@@ -1295,8 +1316,6 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\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 6c2f9b2ba26..42692219a1a 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.397.gdd90340f6a-goog\n\n"},{"id":"357937","messageId":"20180911234951.14129-9-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCH 8/9] fetch: retry fetching submodules if sha1 were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T23:49:50Z","receivedAt":"2018-09-11T23:50:21Z","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 \" (and 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 if the object id that the superproject points\nto, cannot be found.\n\nThis doesn't support fetching to FETCH_HEAD yet, but only into a local\nbranch. To make fetching into FETCH_HEAD work, we need some refactoring\nin builtin/fetch.c to adjust the calls to 'check_for_new_submodule_commits'\nthat is coming in the next patch.\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 9 ++--\n submodule.c | 87 ++++++++++++++++++++++++++++++++++++-\n t/t5526-fetch-submodules.sh | 16 +++++++\n 3 files changed, 104 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 61bec5d213d..95c44bf6ffa 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 1e6781504f0..a75146e89cf 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1127,8 +1127,11 @@ struct submodule_parallel_fetch {\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n+\tstruct string_list retry;\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 STRING_LIST_INIT_NODUP}\n \n static void calculate_changed_submodule_paths(\n \tstruct submodule_parallel_fetch *spf)\n@@ -1259,8 +1262,10 @@ static int get_next_submodule(struct child_process *cp,\n {\n \tint ret = 0;\n \tstruct submodule_parallel_fetch *spf = data;\n+\tstruct string_list_item *it;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n+\t\tint recurse_config;\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@@ -1280,7 +1285,9 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t}\n \t\t}\n \n-\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\trecurse_config = get_fetch_recurse_config(submodule, spf);\n+\n+\t\tswitch (recurse_config)\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n@@ -1319,9 +1326,50 @@ static int get_next_submodule(struct child_process *cp,\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\n+\t\t\tif (submodule != &default_submodule)\n+\t\t\t\t/* discard const-ness: */\n+\t\t\t\t*task_cb = (void*)submodule;\n \t\t\treturn 1;\n \t\t}\n \t}\n+\n+retry_next:\n+\n+\tif (spf->retry.nr) {\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tconst struct submodule *sub;\n+\n+\t\tit = string_list_last(&spf->retry);\n+\t\tsub = submodule_from_name(spf->r, &null_oid,\n+\t\t\t\t\t it->string);\n+\n+\t\tchild_process_init(cp);\n+\t\tcp->dir = get_submodule_git_dir(spf->r, sub->path);\n+\t\tif (!cp->dir) {\n+\t\t\tstring_list_pop(&spf->retry, 0);\n+\t\t\tgoto retry_next;\n+\t\t}\n+\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tcp->git_cmd = 1;\n+\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, sub->path);\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(it->util,\n+\t\t\t\t\t append_oid_to_argv, &cp->args);\n+\n+\t\t*task_cb = NULL; /* make sure we do not recurse forever */\n+\t\tstrbuf_release(&submodule_prefix);\n+\t\tstring_list_pop(&spf->retry, 0);\n+\t\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1335,14 +1383,49 @@ static int fetch_start_failure(struct strbuf *err,\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 submodule *sub = task_cb;\n+\tstruct repository subrepo;\n \n \tif (retvalue)\n \t\tspf->result = 1;\n \n+\tif (!sub)\n+\t\treturn 0;\n+\n+\tif (repo_submodule_init(&subrepo, spf->r, sub->path) < 0)\n+\t\twarning(_(\"Could not get submodule repository for submodule '%s' in repository '%s'\"),\n+\t\t\t sub->path, spf->r->worktree);\n+\telse {\n+\t\tstruct string_list_item *it;\n+\t\tstruct oid_array *commits;\n+\n+\t\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n+\t\tif (!it)\n+\t\t\treturn 0;\n+\n+\t\tcommits = it->util;\n+\t\toid_array_filter(commits,\n+\t\t\t\t commit_exists_in_sub,\n+\t\t\t\t &subrepo);\n+\n+\t\tif (commits->nr)\n+\t\t\tstring_list_append(&spf->retry, sub->name)\n+\t\t\t\t->util = commits;\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 42692219a1a..af12c50e7dd 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.397.gdd90340f6a-goog\n\n"},{"id":"357938","messageId":"20180911234951.14129-10-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-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-09-11T23:49:51Z","receivedAt":"2018-09-11T23:50:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"For Gerrit users that use submodules the invocation of fetch without a\nbranch is their main use case.\n\nSigned-off-by: Stefan Beller \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 95c44bf6ffa..ea6ecd123e7 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 af12c50e7dd..a509eabb044 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.397.gdd90340f6a-goog\n\n"},{"id":"357940","messageId":"d6668ef0-2a9c-d591-fa4c-b8777576f334@ramsayjones.plus.com","threadId":"49321","inReplyTo":"20180911234951.14129-2-sbeller@google.com","subject":"Re: [PATCH 1/9] string-list: add string_list_{pop, last} functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-12T02:24:59Z","receivedAt":"2018-09-12T02:25:06Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 12/09/18 00:49, Stefan Beller wrote:\n> Add a few functions to allow a string-list to be used as a stack:\n> \n> - string_list_last() lets a caller peek the string_list_item at the\n> end of the string list. The caller needs to be aware that it is\n> borrowing a pointer, which can become invalid if/when the\n> string_list is resized.\n> \n> - string_list_pop() removes the string_list_item at the end of\n> the string list.\n> \n> - _pop usually has a friend _push. This role is taken by\n> string_list_append already, as they are not symmetrical\n> in our code base: _append returns the pointer, such that\n> adding a util is easy, but _pop doesn't return such a pointer.\n> \n> You can use them in this pattern:\n> \n> while (list.nr) {\n> struct string_list_item *item = string_list_last(&list);\n> \n> work_on(item);\n> string_list_pop(&list);\n\nstring_list_pop() takes a second int parameter (free_util).\n\nATB,\nRamsay Jones\n\n"},{"id":"357941","messageId":"b16af8c0-0435-0de4-ed6c-53888d6190af@ramsayjones.plus.com","threadId":"49321","inReplyTo":"20180911234951.14129-7-sbeller@google.com","subject":"Re: [PATCH 6/9] submodule.c: do not copy around submodule list","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-12T02:25:52Z","receivedAt":"2018-09-12T02:25:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 12/09/18 00:49, Stefan Beller wrote:\n> 'calculate_changed_submodule_paths' uses a local list to compute the\n> changed submodules, and then produces the result by copying appropriate\n> items into the result list.\n> \n> Instead use the result list directly and prune items afterwards\n> using string_list_remove_empty_items.\n> \n> By doin so we'll have access to the util pointer for longer that\n\ns/doin/doing/\n\nATB,\nRamsay Jones\n"},{"id":"357987","messageId":"xmqq7ejqmwmh.fsf@gitster-ct.c.googlers.com","threadId":"49321","inReplyTo":"20180911234951.14129-2-sbeller@google.com","subject":"Re: [PATCH 1/9] string-list: add string_list_{pop, last} functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T17:52:22Z","receivedAt":"2018-09-12T17:52:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Add a few functions to allow a string-list to be used as a stack:\n>\n> - string_list_last() lets a caller peek the string_list_item at the\n> end of the string list. The caller needs to be aware that it is\n> borrowing a pointer, which can become invalid if/when the\n> string_list is resized.\n>\n> - string_list_pop() removes the string_list_item at the end of\n> the string list.\n>\n> - _pop usually has a friend _push. This role is taken by\n> string_list_append already, as they are not symmetrical\n> in our code base: _append returns the pointer, such that\n> adding a util is easy, but _pop doesn't return such a pointer.\n\nThis third-item somehow is indented one place too deeply.\n\n\"our pop() does not return a pointer, so we won't introduce push()\nthat does return a pointer\"? Am I reading the third item right?\n\npop() and push() usually go together, but they are opposite\noperations, they are expected to behave oppositely, and they are\nexpected to be interfaced differently. pop() that does not return a\npointer is no better or no worse match to push() that returns a\npointer, no?\n\nLack of something_push(), when something_pop() exists, would always\nbe surprising to anybody new to the \"something\" API, and no amount\nof mumbling would justify it, I would think, but this third item\nsounds like it is trying to make a lame excuse when there is no need\nto.\n\nWouldn't it be sufficient to say \"there is no string_list_push();\nstring_list_append() can be used in its place\" and stop there?\n\n> You can use them in this pattern:\n>\n> while (list.nr) {\n> struct string_list_item *item = string_list_last(&list);\n>\n> work_on(item);\n> string_list_pop(&list);\n> }\n\n\"free_util\" as the second argument to this sample call is missing.\n\n> string-list.c | 14 ++++++++++++++\n> string-list.h | 11 +++++++++++\n> 2 files changed, 25 insertions(+)\n\n> diff --git a/string-list.c b/string-list.c\n> index 771c4550980..04db2b537c0 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char *string,\n> \t}\n> }\n> \n> +void string_list_pop(struct string_list *list, int free_util)\n> +{\n> +\tif (list->nr == 0)\n> +\t\tBUG(\"tried to remove an item from empty string list\");\n> +\n> +\tif (list->strdup_strings)\n> +\t\tfree(list->items[list->nr - 1].string);\n> +\n> +\tif (free_util)\n> +\t\tfree(list->items[list->nr - 1].util);\n> +\n> +\tlist->nr--;\n> +}\n> +\n> int string_list_has_string(const struct string_list *list, const char *string)\n> {\n> \tint exact_match;\n> diff --git a/string-list.h b/string-list.h\n> index ff8f6094a33..ce2528bbe15 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -191,6 +191,17 @@ extern void string_list_remove(struct string_list *list, const char *string,\n> */\n> struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n> \n> +/**\n> + * Removes the last item from the list.\n> + * The caller must ensure that the list is not empty.\n> + */\n> +void string_list_pop(struct string_list *list, int free_util);\n> +\n> +static inline struct string_list_item *string_list_last(struct string_list *list)\n\nWe may want to warn users that pop(), append(), etc. shouldn't be\ndone while using the returned value from this function in an in-code\ncomment or docstring.\n\n> +{\n> +\treturn &list->items[list->nr - 1];\n> +}\n> +\n\nOther than that, nicely done.\n\n> /*\n> * Remove all but the first of consecutive entries with the same\n> * string value. If free_util is true, call free() on the util\n"},{"id":"357988","messageId":"xmqqzhwmlhlj.fsf@gitster-ct.c.googlers.com","threadId":"49321","inReplyTo":"20180911234951.14129-3-sbeller@google.com","subject":"Re: [PATCH 2/9] sha1-array: provide oid_array_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T18:02:16Z","receivedAt":"2018-09-12T18:02:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Helped-by: Junio C Hamano \n> Signed-off-by: Stefan Beller \n> ---\n> sha1-array.c | 18 ++++++++++++++++++\n> sha1-array.h | 5 +++++\n> 2 files changed, 23 insertions(+)\n>\n> diff --git a/sha1-array.c b/sha1-array.c\n> index 265941fbf40..76323935dd7 100644\n> --- a/sha1-array.c\n> +++ b/sha1-array.c\n> @@ -77,3 +77,21 @@ int oid_array_for_each_unique(struct oid_array *array,\n> \t}\n> \treturn 0;\n> }\n> +\n> +int oid_array_filter(struct oid_array *array,\n> +\t\t for_each_oid_fn fn,\n\nIt probably makes sense to call it \"want\" instead of \"fn\" to match\nobject_array_filter().\n\n> +\t\t void *cbdata)\n> +{\n> +\tint src, dst;\n> +\n> +\tfor (src = dst = 0; src < array->nr; src++) {\n> +\t\tif (fn(&array->oid[src], cbdata)) {\n> +\t\t\tif (dst < src)\n> +\t\t\t\toidcpy(&array->oid[dst], &array->oid[src]);\n> +\t\t\tdst++;\n> +\t\t}\n\nIn fact, matching the implementation of object_array_fiter() may\nalso make sense, as I do not see a strong reason why the resulting\ncode would become better by rewriting \"dst != src\" over there to\n\"dst < src\" here.\n\n> +\t}\n> +\tarray->nr = dst;\n> +\n> +\treturn 0;\n> +}\n> diff --git a/sha1-array.h b/sha1-array.h\n> index 232bf950172..a2d7c210835 100644\n> --- a/sha1-array.h\n> +++ b/sha1-array.h\n> @@ -23,4 +23,9 @@ 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> \n> +/* Call fn for each oid, and remove it if fn returns 0, retain it otherwise */\n\nAlso perhaps mimic the wording of object_array_filter()'s comment?\nI find it easier that the latter says \"retaining only if X\" instead\nof saying \"remove if !X, retain otherwise\"; it's both shorter and\nmore to the point. It also is nicer that it notes that the order is\npreserved.\n\n> +int oid_array_filter(struct oid_array *array,\n> +\t\t for_each_oid_fn fn,\n> +\t\t void *cbdata);\n> +\n\nOther than that, the function makes sense very much, and the\ncallsite we see in patch 8/9 does, too.\n\nThanks.\n\n> #endif /* SHA1_ARRAY_H */\n"},{"id":"357989","messageId":"xmqqva7alhkw.fsf@gitster-ct.c.googlers.com","threadId":"49321","inReplyTo":"20180911234951.14129-4-sbeller@google.com","subject":"Re: [PATCH 3/9] submodule.c: fix indentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T18:02:39Z","receivedAt":"2018-09-12T18:02:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The submodule subsystem is really bad at staying within 80 characters.\n> Fix it while we are here.\n\nMakes sense. Thanks.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index a2b266fbfae..d29dfa3d1f5 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1244,7 +1244,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> @@ -1254,8 +1255,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"},{"id":"357990","messageId":"xmqqr2hylgv9.fsf@gitster-ct.c.googlers.com","threadId":"49321","inReplyTo":"20180911234951.14129-5-sbeller@google.com","subject":"Re: [PATCH 4/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T18:18:02Z","receivedAt":"2018-09-12T18:18:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> We can string_list_insert() to maintain sorted-ness of the\n> list as we find new items, or we can string_list_append() to\n> build an unsorted list and sort it at the end just once.\n>\n> To pick which one is more appropriate, we notice the fact\n> that we discover new items more or less in the already\n> sorted order. That makes \"append then sort\" more\n> appropriate.\n\nSorry, but I still do not get the math you are implying in the\nsecond paragraph. Are you saying that append-then-sort is efficient\nwhen items being appended is already sorted? That depends on the\nsorting algorithm used, so the logic is incomplete unless you say\n\"given that we use X for sorting,...\", I think.\n\nDo we really discover new items in sorted order, by the way? In a\nsingle diff invocation made inside collect_changed_submodules() for\none commit in the superproject's history, we will grab changed paths\nin the pathname order (i.e. sorted); if the superproject's tip commit\ntouches the submodules at paths A and Z, we will discover these two\npaths in sorted order.\n\nBut because we are walking the superproject's history to collect all\npaths that have been affected in that function, and repeatedly\ncalling diff as we discover commit in the superproject's history, I\nam not sure how well the resulting set of paths would be sorted.\n\nThe tip commit in superproject's history may have modified the\nsubmodule at path X, the parent of that commit may have touched the\nsubmodule at path M, and its parent may have touched the submodule\nat path A. Don't we end up grabbing these paths in that discoverd\norder, i.e. X, M and A?\n\nI still think changing it from \"insert as we find an item, keeping\nthe list sorted\" to \"append all and then sort before we start\nlooking things up from the result\" makes sense, but I do not think\nthe \"we find things in sorted order\" is either true, or it would\naffect the choice between the two. A justification to choose the\nlatter I can think of that makes sense is that we don't have to pay\ncost to keep the list sorted while building it because we do not do\nany look-up while building the list.\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index d29dfa3d1f5..c6eff7699f3 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1256,7 +1256,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> @@ -1350,6 +1350,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"},{"id":"357992","messageId":"xmqqlg86lg06.fsf@gitster-ct.c.googlers.com","threadId":"49321","inReplyTo":"20180911234951.14129-8-sbeller@google.com","subject":"Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T18:36:41Z","receivedAt":"2018-09-12T18:36:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\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 actually doesn't\n> need to be run in the worktree. So let's run it in the git dir instead.\n\nIt may be clear to the author but not clear to the reader of the\nabove paragraph that \"worktree\", \"fetch\" and \"git dir\" all refer to\nthe recursively invoked operation that updates the submodules\nrepository. s/git-fetch/\"git fetch\" for the submodule/ should be\nsufficient to help the readers.\n\n> That should pave the way towards fetching submodules that are currently\n> not checked out.\n\nVery good.\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> @@ -1227,6 +1233,27 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n> \treturn spf->default_option;\n> }\n> \n> +static const char *get_submodule_git_dir(struct repository *r, const char *path)\n> +{\n> +\tstruct repository subrepo;\n> +\tconst char *ret;\n> +\n> +\tif (repo_submodule_init(&subrepo, r, path)) {\n> +\t\t/* no entry in .gitmodules? */\n> +\t\tstruct strbuf gitdir = STRBUF_INIT;\n> +\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", path);\n> +\t\tif (repo_init(&subrepo, gitdir.buf, NULL)) {\n> +\t\t\tstrbuf_release(&gitdir);\n> +\t\t\treturn NULL;\n> +\t\t}\n\nThis is for the modern \"absorbed\" layout? Do we get a notice and\nencouragement to migrate from the historical layout, or there is no\nneed to (e.g. the migration happens automatically in some other\ncodepaths)?\n\n> +\t}\n> +\n> +\tret = xstrdup(subrepo.gitdir);\n> +\trepo_clear(&subrepo);\n> +\n> +\treturn ret;\n> +}\n\nReturned value from this function is xstrdup()'ed so the caller\nowns, not borrows. There is no need to return \"const char *\" from\nthis function. Also the caller needs to free it once done.\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> @@ -1234,8 +1261,6 @@ 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> @@ -1274,16 +1299,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\nIn the old code, git_dir came from read_gitfile() which borrowed.\n\n> +\t\tgit_dir = get_submodule_git_dir(spf->r, ce->name);\n\nIn the new code, we own it, so we'd eventually need to get rid of\nit. How does it happen?\n\n> +\t\tif (git_dir) {\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 = git_dir;\n\nDoes cp now own it and cp->dir gets freed once run_processes_parallel()\nis done with this task? Or is cp->dir simply leaking? The old code\ngave the result of strbuf_detach(), so even if cp->dir is leaking,\nthe leak is not new in this patch.\n\n> \t\t\tcp->git_cmd = 1;\n> \t\t\tif (!spf->quiet)\n> \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n> @@ -1295,8 +1316,6 @@ static int get_next_submodule(struct child_process *cp,\n> \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n> \t\t\tret = 1;\n> \t\t}\n> -\t\tstrbuf_release(&submodule_path);\n> -\t\tstrbuf_release(&submodule_git_dir);\n\nBut if it is a leak, it is easily plugged by freeing git_dir here, I\nthink.\n\nThanks.\n\n\n> \t\tstrbuf_release(&submodule_prefix);\n> \t\tif (ret) {\n> \t\t\tspf->count++;\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 6c2f9b2ba26..42692219a1a 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"},{"id":"357995","messageId":"xmqqftyeleqv.fsf@gitster-ct.c.googlers.com","threadId":"49321","inReplyTo":"20180911234951.14129-9-sbeller@google.com","subject":"Re: [PATCH 8/9] fetch: retry fetching submodules if sha1 were not fetched","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T19:03:52Z","receivedAt":"2018-09-12T19:03:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Retry fetching a submodule if the object id that the superproject points\n> to, cannot be found.\n\nBy object name? By attempting to fetch all refs? Or by doing\nsomething else? Fetching by the exact object name is the most\nefficient approach, but the server side may not be prepared to\nserve such a request, and that is why spelling it out here would\nhelp the readers.\n\n> This doesn't support fetching to FETCH_HEAD yet, but only into a local\n> branch.\n\nIt is not clear if this sentence is talking about the fetch done at\nthe superproject level, or what happens in a submodule repository\nwhen this \"retrying\" happens. Assuming that it is the former,\nperhaps\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.\n\nwould help the readers understand what you are trying to say.\n\n> To make fetching into FETCH_HEAD work, we need some refactoring\n> in builtin/fetch.c to adjust the calls to 'check_for_new_submodule_commits'\n> that is coming in the next patch.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/fetch.c | 9 ++--\n> submodule.c | 87 ++++++++++++++++++++++++++++++++++++-\n> t/t5526-fetch-submodules.sh | 16 +++++++\n> 3 files changed, 104 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 61bec5d213d..95c44bf6ffa 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,\n\nAll of these used to react to any value set to recurse-submodules\nthat is not off or on (i.e. on-demand, default, none), but now\nunless the value is explicitly set to off, we call into the check.\nIt is not immediately clear how that change is linked to the\nretrying. \"When set to 'on', we did not check for new commits, but\nnow we do\" can be read from the patch text but not the reasoning\nbehind it.\n\nWhat was the reason why we didn't call \"check-for-new\" when recurse\nis set to \"on\"? Is it because \"we are going to recurse anyway, so\nthere is no need to check to decide if we need to fetch in\nsubmodule\"? And the reason why we now need to call when we are set\nto recurse anyway is because check-for-new now learns much more than\njust \"do we need to cd there and run git-fetch? yes/no?\"\n\nThe answers to these two questions would help readers in the log\nmessage.\n\n> diff --git a/submodule.c b/submodule.c\n> index 1e6781504f0..a75146e89cf 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1127,8 +1127,11 @@ struct submodule_parallel_fetch {\n> \tint result;\n> \n> \tstruct string_list changed_submodule_names;\n> +\tstruct string_list retry;\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 STRING_LIST_INIT_NODUP}\n> \n> static void calculate_changed_submodule_paths(\n> \tstruct submodule_parallel_fetch *spf)\n> @@ -1259,8 +1262,10 @@ static int get_next_submodule(struct child_process *cp,\n> {\n> \tint ret = 0;\n> \tstruct submodule_parallel_fetch *spf = data;\n> +\tstruct string_list_item *it;\n> \n> \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n> +\t\tint recurse_config;\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> @@ -1280,7 +1285,9 @@ static int get_next_submodule(struct child_process *cp,\n> \t\t\t}\n> \t\t}\n> \n> -\t\tswitch (get_fetch_recurse_config(submodule, spf))\n> +\t\trecurse_config = get_fetch_recurse_config(submodule, spf);\n> +\n> +\t\tswitch (recurse_config)\n> \t\t{\n> \t\tdefault:\n> \t\tcase RECURSE_SUBMODULES_DEFAULT:\n> @@ -1319,9 +1326,50 @@ static int get_next_submodule(struct child_process *cp,\n> \t\tstrbuf_release(&submodule_prefix);\n> \t\tif (ret) {\n> \t\t\tspf->count++;\n> +\t\t\tif (submodule != &default_submodule)\n> +\t\t\t\t/* discard const-ness: */\n> +\t\t\t\t*task_cb = (void*)submodule;\n> \t\t\treturn 1;\n> \t\t}\n> \t}\n> +\n> +retry_next:\n> +\n> +\tif (spf->retry.nr) {\n> +\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n> +\t\tconst struct submodule *sub;\n> +\n> +\t\tit = string_list_last(&spf->retry);\n> +\t\tsub = submodule_from_name(spf->r, &null_oid,\n> +\t\t\t\t\t it->string);\n> +\n> +\t\tchild_process_init(cp);\n> +\t\tcp->dir = get_submodule_git_dir(spf->r, sub->path);\n> +\t\tif (!cp->dir) {\n> +\t\t\tstring_list_pop(&spf->retry, 0);\n> +\t\t\tgoto retry_next;\n> +\t\t}\n> +\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> +\t\tcp->git_cmd = 1;\n> +\n> +\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, sub->path);\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(it->util,\n> +\t\t\t\t\t append_oid_to_argv, &cp->args);\n> +\n> +\t\t*task_cb = NULL; /* make sure we do not recurse forever */\n> +\t\tstrbuf_release(&submodule_prefix);\n> +\t\tstring_list_pop(&spf->retry, 0);\n> +\t\treturn 1;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -1335,14 +1383,49 @@ static int fetch_start_failure(struct strbuf *err,\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\nIs this checking if the 'oid' exists as a comit in the submodule\nrepository? It smells to me that it is checking the opposite.\nShouldn't the function be named \"commit_missing_from_submodule()\" or\nsomething like that?\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 submodule *sub = task_cb;\n> +\tstruct repository subrepo;\n> \n> \tif (retvalue)\n> \t\tspf->result = 1;\n> \n> +\tif (!sub)\n> +\t\treturn 0;\n> +\n> +\tif (repo_submodule_init(&subrepo, spf->r, sub->path) < 0)\n> +\t\twarning(_(\"Could not get submodule repository for submodule '%s' in repository '%s'\"),\n> +\t\t\t sub->path, spf->r->worktree);\n> +\telse {\n> +\t\tstruct string_list_item *it;\n> +\t\tstruct oid_array *commits;\n> +\n> +\t\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n> +\t\tif (!it)\n> +\t\t\treturn 0;\n\nOK, so after a fetch, if we have missing commits, we append to the\nspf->retry list, which will be looked at in the function we looked\nat earlier.\n\n> +\t\tcommits = it->util;\n> +\t\toid_array_filter(commits,\n> +\t\t\t\t commit_exists_in_sub,\n> +\t\t\t\t &subrepo);\n> +\n> +\t\tif (commits->nr)\n> +\t\t\tstring_list_append(&spf->retry, sub->name)\n> +\t\t\t\t->util = commits;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 42692219a1a..af12c50e7dd 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"},{"id":"357996","messageId":"CAGZ79kbavjVbTqXsmtjW6=jhkq47_p3mc6=92xOp4_mfhqDtvw@mail.gmail.com","threadId":"49321","inReplyTo":"xmqqr2hylgv9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-12T19:06:36Z","receivedAt":"2018-09-12T19:06:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 12, 2018 at 11:18 AM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > We can string_list_insert() to maintain sorted-ness of the\n> > list as we find new items, or we can string_list_append() to\n> > build an unsorted list and sort it at the end just once.\n> >\n> > To pick which one is more appropriate, we notice the fact\n> > that we discover new items more or less in the already\n> > sorted order. That makes \"append then sort\" more\n> > appropriate.\n>\n> Sorry, but I still do not get the math you are implying in the\n> second paragraph. Are you saying that append-then-sort is efficient\n> when items being appended is already sorted? That depends on the\n> sorting algorithm used, so the logic is incomplete unless you say\n> \"given that we use X for sorting,...\", I think.\n>\n> Do we really discover new items in sorted order, by the way? In a\n> single diff invocation made inside collect_changed_submodules() for\n> one commit in the superproject's history, we will grab changed paths\n> in the pathname order (i.e. sorted); if the superproject's tip commit\n> touches the submodules at paths A and Z, we will discover these two\n> paths in sorted order.\n>\n> But because we are walking the superproject's history to collect all\n> paths that have been affected in that function, and repeatedly\n> calling diff as we discover commit in the superproject's history, I\n> am not sure how well the resulting set of paths would be sorted.\n>\n> The tip commit in superproject's history may have modified the\n> submodule at path X, the parent of that commit may have touched the\n> submodule at path M, and its parent may have touched the submodule\n> at path A. Don't we end up grabbing these paths in that discoverd\n> order, i.e. X, M and A?\n\nThat is true.\n\n>\n> I still think changing it from \"insert as we find an item, keeping\n> the list sorted\" to \"append all and then sort before we start\n> looking things up from the result\" makes sense, but I do not think\n> the \"we find things in sorted order\" is either true, or it would\n> affect the choice between the two. A justification to choose the\n> latter I can think of that makes sense is that we don't have to pay\n> cost to keep the list sorted while building it because we do not do\n> any look-up while building the list.\n\nok.\n\nThanks,\nStefan\n"},{"id":"357999","messageId":"xmqqbm92ldyi.fsf@gitster-ct.c.googlers.com","threadId":"49321","inReplyTo":"20180911234951.14129-10-sbeller@google.com","subject":"Re: [PATCH 9/9] builtin/fetch: check for submodule updates for non branch fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T19:20:53Z","receivedAt":"2018-09-12T19:20:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> For Gerrit users that use submodules the invocation of fetch without a\n> branch is their main use case.\n\nThat's way under explains this commit. It is totally unclear how\nthat statement of fact relates to the problem this patch is trying\nto address; it does not even make it clear what problem is being\naddressed by the patch.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/fetch.c | 5 ++++-\n> t/t5526-fetch-submodules.sh | 2 +-\n> 2 files changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 95c44bf6ffa..ea6ecd123e7 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\nThis happens when there is no \"ref\", which is set only when\nrm->peer_ref exists, which is set only when we are using remote\ntracking branch (or more generally storing the fetched rev somewhere\nin our refs/ hierarchy), e.g. the rev is recorded only in FETCH_HEAD.\n\nWhat does rm->old_oid have in such a case? Is this the tip of the\nsuperproject history we just fetched?\n\nWhen we keep record of what we saw in the previous attempt to fetch,\nwe can tell \"we have seen their history up to this old commit\nbefore, and now we fetched their history up to this new commit\" and\nthe question \"during that time, which submodules have been modified\nin the history of the superproject\" becomes answerable. When we are\nnot keeping the record of previous fetch, how would we answer that\nquestion without going through the whole history?\n\n\tThe answer is that check-for-new does not even do the \"old\n\tbranch tip was X and new branch tip is Y, so we can look\n\tonly at X..Y\"; it only cares about the new branch tip of the\n\tsuperproject, and excludes the existing tips of all branches\n\tin the superproject (i.e. computing something akin to \"Y\n\t--not --all\" instead of \"X..Y\").\n\nSo, I guess this is probably reasonable. But does the call to\n\"check-for-new submodule\" need to be unconditional? In this\ncodepath, do we know when we are not doing a recursive fetch in a\nsuperproject? If so, perhaps we can omit the cost of going through\nall the refs to populate ref_tips_before_fetch array in such a case.\n\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\"),\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index af12c50e7dd..a509eabb044 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"},{"id":"358088","messageId":"CAGZ79kZKKf9N8yx9EuCRZhrZS_mA2218PouEG7aHDhK2bJGEdA@mail.gmail.com","threadId":"49321","inReplyTo":"xmqqlg86lg06.fsf@gitster-ct.c.googlers.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-09-13T19:29:45Z","receivedAt":"2018-09-13T19:30:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 12, 2018 at 11:36 AM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\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 actually doesn't\n> > need to be run in the worktree. So let's run it in the git dir instead.\n>\n> It may be clear to the author but not clear to the reader of the\n> above paragraph that \"worktree\", \"fetch\" and \"git dir\" all refer to\n> the recursively invoked operation that updates the submodules\n> repository. s/git-fetch/\"git fetch\" for the submodule/ should be\n> sufficient to help the readers.\n>\n> > That should pave the way towards fetching submodules that are currently\n> > not checked out.\n>\n> Very good.\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> > +\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> > @@ -1227,6 +1233,27 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n> > return spf->default_option;\n> > }\n> >\n> > +static const char *get_submodule_git_dir(struct repository *r, const char *path)\n> > +{\n> > + struct repository subrepo;\n> > + const char *ret;\n> > +\n> > + if (repo_submodule_init(&subrepo, r, path)) {\n> > + /* no entry in .gitmodules? */\n> > + struct strbuf gitdir = STRBUF_INIT;\n> > + strbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", path);\n> > + if (repo_init(&subrepo, gitdir.buf, NULL)) {\n> > + strbuf_release(&gitdir);\n> > + return NULL;\n> > + }\n>\n> This is for the modern \"absorbed\" layout? Do we get a notice and\n> encouragement to migrate from the historical layout, or there is no\n> need to (e.g. the migration happens automatically in some other\n> codepaths)?\n\nNo, the absorbed would also be handled by repo_submodule_init.\nI wrote a patch once to migrate repo_submodule_init to take a\n\"struct *submodule\" instead of a path as the third argument, which\nwould fall in line with this patch as well, I'll dig it up.\n\nHistorically git-fetch supported repositories that are not submodules\n(but have a gitlink and a working tree in place) as well. That is covered\nhere. (see comment /* no entry in .gitmodules? */)\n\n> > - strbuf_release(&submodule_path);\n> > - strbuf_release(&submodule_git_dir);\n>\n> But if it is a leak, it is easily plugged by freeing git_dir here, I\n> think.\n\nThanks.\n"},{"id":"358338","messageId":"20180917213559.126404-1-sbeller@google.com","threadId":"49321","inReplyTo":"20180911234951.14129-1-sbeller@google.com","subject":"[PATCHv2 0/9] fetch: make sure submodule oids are fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:35:50Z","receivedAt":"2018-09-17T21:36:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\n* extended commit messages,\n* plugged a memory leak\n* rewrote the patch \"sha1-array: provide oid_array_filter\" to be much more like \n object_array_fiter\n* fixed a typo pointed out by Ramsay.\n\nThe range diff is below.\n \nThanks,\nStefan\n\nv1:\nCurrently when git-fetch is asked to recurse into submodules, it dispatches\na plain \"git-fetch -C \" (and 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, not so well in others,\nwhich this series aims to fix.\n\nThe first patches provide new basic functionality and do some refactoring;\nthe interesting part is in the two last patches.\n\nThis was discussed in\nhttps://public-inbox.org/git/20180808221752.195419-1-sbeller@google.com/\nand I think I addressed all feedback so far.\n\nStefan Beller (9):\n string-list: add string_list_{pop, last} functions\n sha1-array: provide oid_array_filter\n submodule.c: fix indentation\n submodule.c: sort changed_submodule_names before searching it\n submodule: move global changed_submodule_names into fetch submodule\n struct\n submodule.c: do not copy around submodule list\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 builtin/fetch.c | 14 +--\n sha1-array.c | 17 ++++\n sha1-array.h | 9 ++\n string-list.c | 14 +++\n string-list.h | 15 +++\n submodule.c | 191 ++++++++++++++++++++++++++++--------\n t/t5526-fetch-submodules.sh | 23 ++++-\n 7 files changed, 236 insertions(+), 47 deletions(-)\n\ngit range-diff origin/sb/submodule-recursive-fetch-gets-the-tip...HEAD \n\n 1: 6fecf7cd01a ! 1: 54d31d90734 string-list: add string_list_{pop, last} functions\n @@ -12,8 +12,8 @@\n - string_list_pop() removes the string_list_item at the end of\n the string list.\n \n - - string_list_push() is not added, but string_list_append() can be\n - used in its place.\n + - there is no string_list_push(); string_list_append() can be used\n + in its place.\n \n You can use them in this pattern:\n \n @@ -26,7 +26,6 @@\n \n Helped-by: Junio C Hamano \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/string-list.c b/string-list.c\n --- a/string-list.c\n @@ -66,6 +65,10 @@\n + */\n +void string_list_pop(struct string_list *list, int free_util);\n +\n ++/*\n ++ * Returns the last item of the list. As it returns the raw access, do not\n ++ * modify the list while holding onto the returned pointer.\n ++ */\n +static inline struct string_list_item *string_list_last(struct string_list *list)\n +{\n +\treturn &list->items[list->nr - 1];\n 2: 7007a318a68 < -: ----------- sha1-array: provide oid_array_filter\n -: ----------- > 2: a2bd6ef2bf0 sha1-array: provide oid_array_filter\n 3: 807429234ac ! 3: 0300c27cbd7 submodule.c: fix indentation\n @@ -6,7 +6,6 @@\n Fix it while we are here.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n 4: f6fa5273af9 ! 4: 80cf0221bbe submodule.c: sort changed_submodule_names before searching it\n @@ -12,7 +12,6 @@\n appropriate.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n 5: adf7a2fd203 ! 5: 7ddb448b748 submodule: move global changed_submodule_names into fetch submodule struct\n @@ -6,7 +6,6 @@\n part of the struct that is passed around for fetching submodules.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n 6: 56c9398589a ! 6: 7975a7f1e3b submodule.c: do not copy around submodule list\n @@ -9,12 +9,11 @@\n Instead use the result list directly and prune items afterwards\n using string_list_remove_empty_items.\n \n - By doin so we'll have access to the util pointer for longer that\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 Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n 7: 9f70a5f32c9 ! 7: 29bc2868f26 submodule: fetch in submodules git directory instead of in worktree\n @@ -3,14 +3,14 @@\n submodule: fetch in submodules git directory instead of in worktree\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 actually doesn't\n - need to be run in the worktree. So let's run it in the git dir instead.\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 That should pave the way towards fetching submodules that are currently\n not checked out.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n @@ -32,7 +32,7 @@\n \treturn spf->default_option;\n }\n \n -+static const char *get_submodule_git_dir(struct repository *r, const char *path)\n ++static char *get_submodule_git_dir(struct repository *r, const char *path)\n +{\n +\tstruct repository subrepo;\n +\tconst char *ret;\n @@ -87,7 +87,11 @@\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n @@\n + \t\t\targv_array_push(&cp->args, default_argv);\n + \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n ++\n ++\t\t\tfree(git_dir);\n \t\t\tret = 1;\n \t\t}\n -\t\tstrbuf_release(&submodule_path);\n 8: bab609b4dc1 ! 8: f837c4a0789 fetch: retry fetching submodules if sha1 were not fetched\n @@ -1,9 +1,9 @@\n Author: Stefan Beller \n \n - fetch: retry fetching submodules if sha1 were not fetched\n + fetch: retry fetching submodules if needed objects were not fetched\n \n Currently when git-fetch is asked to recurse into submodules, it dispatches\n - a plain \"git-fetch -C \" (and some submodule related options\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 @@ -16,16 +16,22 @@\n topic downloaded as well. However these submodule changes reside in their\n own repository in their own ref (refs/changes/).\n \n - Retry fetching a submodule if the object id that the superproject points\n - to, cannot be found.\n + Retry fetching a submodule by object name if the object id that the\n + superproject points to, cannot be found.\n \n - This doesn't support fetching to FETCH_HEAD yet, but only into a local\n - branch. To make fetching into FETCH_HEAD work, we need some refactoring\n - in builtin/fetch.c to adjust the calls to 'check_for_new_submodule_commits'\n - that is coming in the next patch.\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 +\n + builtin/fetch used to only inspect submodules when they were fetched\n + \"on-demand\", as in either on/off case it was clear whether the submodule\n + needs to be fetched. However to know whether we need to try fetching the\n + object ids, we need to identify the object names, which is done in this\n + function check_for_new_submodule_commits(), so we'll also run that code\n + in case the submodule recursion is set to \"on\".\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/builtin/fetch.c b/builtin/fetch.c\n --- a/builtin/fetch.c\n 9: c16d21313f6 ! 9: 71f2bb035b1 builtin/fetch: check for submodule updates for non branch fetches\n @@ -2,11 +2,29 @@\n \n builtin/fetch: check for submodule updates for non branch fetches\n \n - For Gerrit users that use submodules the invocation of fetch without a\n - branch is their main use case.\n + Gerrit, the code review tool, has a different workflow than our mailing\n + list based approach. Usually users upload changes to a Gerrit server and\n + continuous integration and testing happens by bots. Sometimes however a\n + user wants to checkout a change locally and look at it locally. For this\n + use case, Gerrit offers a command line snippet to copy and paste to your\n + terminal, which looks like\n +\n + git fetch https:///gerrit refs/changes/ &&\n + git checkout FETCH_HEAD\n +\n + For Gerrit changes that contain changing submodule gitlinks, it would be\n + easy to extend both the fetch and checkout with the '--recurse-submodules'\n + flag, such that this command line snippet would produce the state of a\n + change locally.\n +\n + However the functionality added in the previous patch, which would\n + ensure that we fetch the objects in the submodule that the gitlink pointed\n + at, only works for remote tracking branches so far, not for FETCH_HEAD.\n +\n + Make sure that fetching a superproject to its FETCH_HEAD, also respects\n + the existence checks for objects in the submodule recursion.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/builtin/fetch.c b/builtin/fetch.c\n --- a/builtin/fetch.c\n"},{"id":"358339","messageId":"20180917213559.126404-2-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-1-sbeller@google.com","subject":"[PATCH 1/9] string-list: add string_list_{pop, last} functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:35:51Z","receivedAt":"2018-09-17T21:36:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Add a few functions to allow a string-list to be used as a stack:\n\n - string_list_last() lets a caller peek the string_list_item at the\n end of the string list. The caller needs to be aware that it is\n borrowing a pointer, which can become invalid if/when the\n string_list is resized.\n\n - string_list_pop() removes the string_list_item at the end of\n the string list.\n\n - there is no string_list_push(); string_list_append() can be used\n in its place.\n\nYou can use them in this pattern:\n\n while (list.nr) {\n struct string_list_item *item = string_list_last(&list);\n\n work_on(item);\n string_list_pop(&list, free_util);\n }\n\nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n string-list.c | 14 ++++++++++++++\n string-list.h | 15 +++++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/string-list.c b/string-list.c\nindex 771c4550980..04db2b537c0 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char *string,\n \t}\n }\n \n+void string_list_pop(struct string_list *list, int free_util)\n+{\n+\tif (list->nr == 0)\n+\t\tBUG(\"tried to remove an item from empty string list\");\n+\n+\tif (list->strdup_strings)\n+\t\tfree(list->items[list->nr - 1].string);\n+\n+\tif (free_util)\n+\t\tfree(list->items[list->nr - 1].util);\n+\n+\tlist->nr--;\n+}\n+\n int string_list_has_string(const struct string_list *list, const char *string)\n {\n \tint exact_match;\ndiff --git a/string-list.h b/string-list.h\nindex ff8f6094a33..15f2936e337 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -191,6 +191,21 @@ extern void string_list_remove(struct string_list *list, const char *string,\n */\n struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n \n+/**\n+ * Removes the last item from the list.\n+ * The caller must ensure that the list is not empty.\n+ */\n+void string_list_pop(struct string_list *list, int free_util);\n+\n+/*\n+ * Returns the last item of the list. As it returns the raw access, do not\n+ * modify the list while holding onto the returned pointer.\n+ */\n+static inline struct string_list_item *string_list_last(struct string_list *list)\n+{\n+\treturn &list->items[list->nr - 1];\n+}\n+\n /*\n * Remove all but the first of consecutive entries with the same\n * string value. If free_util is true, call free() on the util\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"358340","messageId":"20180917213559.126404-3-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-1-sbeller@google.com","subject":"[PATCH 2/9] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:35:52Z","receivedAt":"2018-09-17T21:36:11Z","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 \n---\n sha1-array.c | 17 +++++++++++++++++\n sha1-array.h | 9 +++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/sha1-array.c b/sha1-array.c\nindex 265941fbf40..67db5eeec9a 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->oids;\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 232bf950172..ae059ca0431 100644\n--- a/sha1-array.h\n+++ b/sha1-array.h\n@@ -23,4 +23,13 @@ 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 \n+/*\n+ * Apply want to each entry in array, retaining only the entries for\n+ * which the function returns true. Preserve the order of the entries\n+ * that are retained.\n+ */\n+int 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.397.gdd90340f6a-goog\n\n"},{"id":"358341","messageId":"20180917213559.126404-4-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-1-sbeller@google.com","subject":"[PATCH 3/9] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:35:53Z","receivedAt":"2018-09-17T21:36:12Z","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 \n---\n submodule.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex a2b266fbfae..d29dfa3d1f5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1244,7 +1244,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@@ -1254,8 +1255,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.397.gdd90340f6a-goog\n\n"},{"id":"358342","messageId":"20180917213559.126404-5-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-1-sbeller@google.com","subject":"[PATCH 4/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:35:54Z","receivedAt":"2018-09-17T21:36:14Z","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\nTo pick which one is more appropriate, we notice the fact\nthat we discover new items more or less in the already\nsorted order. That makes \"append then sort\" more\nappropriate.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d29dfa3d1f5..c6eff7699f3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1256,7 +1256,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@@ -1350,6 +1350,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.397.gdd90340f6a-goog\n\n"},{"id":"358343","messageId":"20180917213559.126404-6-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-1-sbeller@google.com","subject":"[PATCH 5/9] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:35:55Z","receivedAt":"2018-09-17T21:36:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `changed_submodule_names` are only used for fetching, so let's make it\npart of the struct that is passed around for fetching submodules.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex c6eff7699f3..3520dd76bdf 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@@ -1110,7 +1110,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@@ -1148,7 +1163,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@@ -1185,18 +1201,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@@ -1257,7 +1261,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@@ -1349,8 +1353,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@@ -1359,7 +1363,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.397.gdd90340f6a-goog\n\n"},{"id":"358344","messageId":"20180917213559.126404-7-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-1-sbeller@google.com","subject":"[PATCH 6/9] submodule.c: do not copy around submodule list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:35:56Z","receivedAt":"2018-09-17T21:36:20Z","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 \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3520dd76bdf..00a9a3c6b12 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1128,8 +1128,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@@ -1146,9 +1145,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@@ -1162,12 +1161,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@@ -1363,7 +1364,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.397.gdd90340f6a-goog\n\n"},{"id":"358345","messageId":"20180917213559.126404-8-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-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-09-17T21:35:57Z","receivedAt":"2018-09-17T21:36:21Z","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\nSigned-off-by: Stefan Beller \n---\n submodule.c | 45 +++++++++++++++++++++++++++----------\n t/t5526-fetch-submodules.sh | 7 +++++-\n 2 files changed, 39 insertions(+), 13 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 00a9a3c6b12..88bce534268 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -481,6 +481,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@@ -1227,6 +1233,27 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+static char *get_submodule_git_dir(struct repository *r, const char *path)\n+{\n+\tstruct repository subrepo;\n+\tconst char *ret;\n+\n+\tif (repo_submodule_init(&subrepo, r, path)) {\n+\t\t/* no entry in .gitmodules? */\n+\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", path);\n+\t\tif (repo_init(&subrepo, gitdir.buf, NULL)) {\n+\t\t\tstrbuf_release(&gitdir);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\n+\tret = xstrdup(subrepo.gitdir);\n+\trepo_clear(&subrepo);\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@@ -1234,8 +1261,6 @@ 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@@ -1274,16 +1299,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\tgit_dir = get_submodule_git_dir(spf->r, ce->name);\n+\t\tif (git_dir) {\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 = git_dir;\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@@ -1293,10 +1314,10 @@ 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\tfree(git_dir);\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 6c2f9b2ba26..42692219a1a 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.397.gdd90340f6a-goog\n\n"},{"id":"358346","messageId":"20180917213559.126404-9-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-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-09-17T21:35:58Z","receivedAt":"2018-09-17T21:36:24Z","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 \n---\n builtin/fetch.c | 9 ++--\n submodule.c | 87 ++++++++++++++++++++++++++++++++++++-\n t/t5526-fetch-submodules.sh | 16 +++++++\n 3 files changed, 104 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 61bec5d213d..95c44bf6ffa 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 88bce534268..7d59e56171f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1127,8 +1127,11 @@ struct submodule_parallel_fetch {\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n+\tstruct string_list retry;\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 STRING_LIST_INIT_NODUP}\n \n static void calculate_changed_submodule_paths(\n \tstruct submodule_parallel_fetch *spf)\n@@ -1259,8 +1262,10 @@ static int get_next_submodule(struct child_process *cp,\n {\n \tint ret = 0;\n \tstruct submodule_parallel_fetch *spf = data;\n+\tstruct string_list_item *it;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n+\t\tint recurse_config;\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@@ -1280,7 +1285,9 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t}\n \t\t}\n \n-\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\trecurse_config = get_fetch_recurse_config(submodule, spf);\n+\n+\t\tswitch (recurse_config)\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n@@ -1321,9 +1328,50 @@ static int get_next_submodule(struct child_process *cp,\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\n+\t\t\tif (submodule != &default_submodule)\n+\t\t\t\t/* discard const-ness: */\n+\t\t\t\t*task_cb = (void*)submodule;\n \t\t\treturn 1;\n \t\t}\n \t}\n+\n+retry_next:\n+\n+\tif (spf->retry.nr) {\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tconst struct submodule *sub;\n+\n+\t\tit = string_list_last(&spf->retry);\n+\t\tsub = submodule_from_name(spf->r, &null_oid,\n+\t\t\t\t\t it->string);\n+\n+\t\tchild_process_init(cp);\n+\t\tcp->dir = get_submodule_git_dir(spf->r, sub->path);\n+\t\tif (!cp->dir) {\n+\t\t\tstring_list_pop(&spf->retry, 0);\n+\t\t\tgoto retry_next;\n+\t\t}\n+\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tcp->git_cmd = 1;\n+\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, sub->path);\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(it->util,\n+\t\t\t\t\t append_oid_to_argv, &cp->args);\n+\n+\t\t*task_cb = NULL; /* make sure we do not recurse forever */\n+\t\tstrbuf_release(&submodule_prefix);\n+\t\tstring_list_pop(&spf->retry, 0);\n+\t\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1337,14 +1385,49 @@ static int fetch_start_failure(struct strbuf *err,\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 submodule *sub = task_cb;\n+\tstruct repository subrepo;\n \n \tif (retvalue)\n \t\tspf->result = 1;\n \n+\tif (!sub)\n+\t\treturn 0;\n+\n+\tif (repo_submodule_init(&subrepo, spf->r, sub->path) < 0)\n+\t\twarning(_(\"Could not get submodule repository for submodule '%s' in repository '%s'\"),\n+\t\t\t sub->path, spf->r->worktree);\n+\telse {\n+\t\tstruct string_list_item *it;\n+\t\tstruct oid_array *commits;\n+\n+\t\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n+\t\tif (!it)\n+\t\t\treturn 0;\n+\n+\t\tcommits = it->util;\n+\t\toid_array_filter(commits,\n+\t\t\t\t commit_exists_in_sub,\n+\t\t\t\t &subrepo);\n+\n+\t\tif (commits->nr)\n+\t\t\tstring_list_append(&spf->retry, sub->name)\n+\t\t\t\t->util = commits;\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 42692219a1a..af12c50e7dd 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.397.gdd90340f6a-goog\n\n"},{"id":"358347","messageId":"20180917213559.126404-10-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-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-09-17T21:35:59Z","receivedAt":"2018-09-17T21:36:26Z","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 \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 95c44bf6ffa..ea6ecd123e7 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 af12c50e7dd..a509eabb044 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.397.gdd90340f6a-goog\n\n"},{"id":"358348","messageId":"CAGZ79kYtR4BgzFDfHookB-ax1wN0TVH1Qam3vC99ypjwr=PHgQ@mail.gmail.com","threadId":"49321","inReplyTo":"20180917213559.126404-3-sbeller@google.com","subject":"Re: [PATCH 2/9] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T21:42:54Z","receivedAt":"2018-09-17T21:43:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 17, 2018 at 2:36 PM Stefan Beller wrote:\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: Stefan Beller \n> ---\n> sha1-array.c | 17 +++++++++++++++++\n> sha1-array.h | 9 +++++++++\n> 2 files changed, 26 insertions(+)\n>\n> diff --git a/sha1-array.c b/sha1-array.c\n> index 265941fbf40..67db5eeec9a 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> }\n> return 0;\n> }\n> +\n> +void oid_array_filter(struct oid_array *array,\n> + for_each_oid_fn want,\n> + void *cb_data)\n> +{\n> + unsigned nr = array->nr, src, dst;\n> + struct object_id *oids = array->oids;\n\nBlargh :-(\n\nI made this last minute \"pull oids out to be more like\nthe object_array_filter\" and typo'd without compile checking.\n\nPlease discard this series.\n\nStefan\n"},{"id":"358664","messageId":"20180921220820.49446-1-sbeller@google.com","threadId":"49321","inReplyTo":"20180917213559.126404-2-sbeller@google.com","subject":"[PATCH] string-list: add string_list_{pop, last} functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-21T22:08:20Z","receivedAt":"2018-09-21T22:08:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Add a few functions to allow a string-list to be used as a stack:\n\n - string_list_last() lets a caller peek the string_list_item at the\n end of the string list. The caller needs to be aware that it is\n borrowing a pointer, which can become invalid if/when the\n string_list is resized.\n\n - string_list_pop() removes the string_list_item at the end of\n the string list.\n\n - there is no string_list_push(); string_list_append() can be used\n in its place.\n\nYou can use them in this pattern:\n\n while (list.nr) {\n struct string_list_item *item = string_list_last(&list);\n\n work_on(item);\n string_list_pop(&list, free_util);\n }\n\nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n\nI will be resending this series shortly, but it will not contain this patch,\nas I will have no need for these any longer.\n\nFor the record, this is the final state of this patch.\nIt may be useful on its own.\n\nThanks,\nStefan\n\n string-list.c | 14 ++++++++++++++\n string-list.h | 15 +++++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/string-list.c b/string-list.c\nindex 771c4550980..04db2b537c0 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char *string,\n \t}\n }\n \n+void string_list_pop(struct string_list *list, int free_util)\n+{\n+\tif (list->nr == 0)\n+\t\tBUG(\"tried to remove an item from empty string list\");\n+\n+\tif (list->strdup_strings)\n+\t\tfree(list->items[list->nr - 1].string);\n+\n+\tif (free_util)\n+\t\tfree(list->items[list->nr - 1].util);\n+\n+\tlist->nr--;\n+}\n+\n int string_list_has_string(const struct string_list *list, const char *string)\n {\n \tint exact_match;\ndiff --git a/string-list.h b/string-list.h\nindex ff8f6094a33..15f2936e337 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -191,6 +191,21 @@ extern void string_list_remove(struct string_list *list, const char *string,\n */\n struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n \n+/**\n+ * Removes the last item from the list.\n+ * The caller must ensure that the list is not empty.\n+ */\n+void string_list_pop(struct string_list *list, int free_util);\n+\n+/*\n+ * Returns the last item of the list. As it returns the raw access, do not\n+ * modify the list while holding onto the returned pointer.\n+ */\n+static inline struct string_list_item *string_list_last(struct string_list *list)\n+{\n+\treturn &list->items[list->nr - 1];\n+}\n+\n /*\n * Remove all but the first of consecutive entries with the same\n * string value. If free_util is true, call free() on the util\n-- \n2.19.0.444.g18242da7ef-goog\n\n"}]}