{"thread":{"id":"49080","subject":"[RFC PATCH 00/10] fetch: make sure submodule oids are fetched","startedAt":"2018-08-08T22:18:07Z","lastAt":"2018-08-09T22:10:35Z","messageCount":27,"participants":["Stefan Beller","Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"354932","messageId":"20180808221752.195419-1-sbeller@google.com","threadId":"49080","inReplyTo":null,"subject":"[RFC PATCH 00/10] fetch: make sure submodule oids are fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:42Z","receivedAt":"2018-08-08T22:18:07Z","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 <submodule-dir>\" (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\nThanks,\nStefan\n\nStefan Beller (10):\n  string_list: print_string_list to use trace_printf\n  string-list.h: add string_list_pop function.\n  sha1-array: provide oid_array_remove_if\n  submodule.c: convert submodule_move_head new argument to object id\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\n builtin/fetch.c             |   9 +-\n entry.c                     |   6 +-\n sha1-array.c                |  39 ++++++++\n sha1-array.h                |   3 +\n string-list.c               |  12 ++-\n string-list.h               |   6 ++\n submodule.c                 | 194 +++++++++++++++++++++++++++---------\n submodule.h                 |   2 +-\n t/t5526-fetch-submodules.sh |  23 ++++-\n unpack-trees.c              |  13 +--\n 10 files changed, 241 insertions(+), 66 deletions(-)\n\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354933","messageId":"20180808221752.195419-2-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 01/10] string_list: print_string_list to use trace_printf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:43Z","receivedAt":"2018-08-08T22:18:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It is a debugging aid, so it should print to the debugging channel.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n string-list.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 771c4550980..9f651bb4294 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -200,9 +200,9 @@ void print_string_list(const struct string_list *p, const char *text)\n {\n \tint i;\n \tif ( text )\n-\t\tprintf(\"%s\\n\", text);\n+\t\ttrace_printf(\"%s\\n\", text);\n \tfor (i = 0; i < p->nr; i++)\n-\t\tprintf(\"%s:%p\\n\", p->items[i].string, p->items[i].util);\n+\t\ttrace_printf(\"%s:%p\\n\", p->items[i].string, p->items[i].util);\n }\n \n struct string_list_item *string_list_append_nodup(struct string_list *list,\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354934","messageId":"20180808221752.195419-3-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 02/10] string-list.h: add string_list_pop function.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:44Z","receivedAt":"2018-08-08T22:18:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A string list can be used as a stack, but should we? A later patch shows\nhow useful this will be.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n string-list.c | 8 ++++++++\n string-list.h | 6 ++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/string-list.c b/string-list.c\nindex 9f651bb4294..ea80afc8a0c 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -80,6 +80,14 @@ void string_list_remove(struct string_list *list, const char *string,\n \t}\n }\n \n+struct string_list_item *string_list_pop(struct string_list *list)\n+{\n+\tif (list->nr == 0)\n+\t\treturn 0;\n+\n+\treturn &list->items[--list->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..a1a41bc961a 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -191,6 +191,12 @@ 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+ * Returns the last item inserted and removes it from the list.\n+ * If the list is empty, return NULL.\n+ */\n+struct string_list_item *string_list_pop(struct string_list *list);\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.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354935","messageId":"20180808221752.195419-4-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 03/10] sha1-array: provide oid_array_remove_if","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:45Z","receivedAt":"2018-08-08T22:18:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n sha1-array.c | 39 +++++++++++++++++++++++++++++++++++++++\n sha1-array.h |  3 +++\n 2 files changed, 42 insertions(+)\n\ndiff --git a/sha1-array.c b/sha1-array.c\nindex 265941fbf40..10eb08b425e 100644\n--- a/sha1-array.c\n+++ b/sha1-array.c\n@@ -77,3 +77,42 @@ int oid_array_for_each_unique(struct oid_array *array,\n \t}\n \treturn 0;\n }\n+\n+int oid_array_remove_if(struct oid_array *array,\n+\t\t\tfor_each_oid_fn fn,\n+\t\t\tvoid *data)\n+{\n+\tint i, j;\n+\tchar *to_remove = xcalloc(array->nr, sizeof(char));\n+\n+\t/* No oid_array_sort() here! See the api-oid-array.txt docs! */\n+\n+\tfor (i = 0; i < array->nr; i++) {\n+\t\tint ret = fn(array->oid + i, data);\n+\t\tif (ret)\n+\t\t\tto_remove[i] = 1;\n+\t}\n+\n+\ti = 0, j = 0;\n+\twhile (i < array->nr && j < array->nr) {\n+\t\twhile (i < array->nr && !to_remove[i])\n+\t\t\ti++;\n+\t\t/* i at first marked for deletion or out */\n+\t\tif (j < i)\n+\t\t\tj = i;\n+\t\twhile (j < array->nr && to_remove[j])\n+\t\t\tj++;\n+\t\t/* j > i; j at first valid after first deletion range or out */\n+\t\tif (i < array->nr && j < array->nr)\n+\t\t\toidcpy(&array->oid[i], &array->oid[j]);\n+\t\telse if (i >= array->nr)\n+\t\t\tassert(j >= array->nr);\n+\t\t\t/* no pruning happened, keep original array->nr */\n+\t\telse if (j >= array->nr)\n+\t\t\tarray->nr = i;\n+\t}\n+\n+\tfree(to_remove);\n+\n+\treturn 0;\n+}\ndiff --git a/sha1-array.h b/sha1-array.h\nindex 232bf950172..151c7ad7f30 100644\n--- a/sha1-array.h\n+++ b/sha1-array.h\n@@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,\n int oid_array_for_each_unique(struct oid_array *array,\n \t\t\t      for_each_oid_fn fn,\n \t\t\t      void *data);\n+int oid_array_remove_if(struct oid_array *array,\n+\t\t\tfor_each_oid_fn fn,\n+\t\t\tvoid *data);\n \n #endif /* SHA1_ARRAY_H */\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354936","messageId":"20180808221752.195419-5-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 04/10] submodule.c: convert submodule_move_head new argument to object id","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:46Z","receivedAt":"2018-08-08T22:18:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All callers use oid_to_hex to convert the desired oid to a string before\ncalling submodule_move_head. Defer the conversion to the\nsubmodule_move_head as it will turn out to be useful in a bit.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n entry.c        |  6 +++---\n submodule.c    | 12 ++++++------\n submodule.h    |  2 +-\n unpack-trees.c | 13 +++++--------\n 4 files changed, 15 insertions(+), 18 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex b5d1d3cf231..4b34dfd30df 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -358,7 +358,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tsub = submodule_from_ce(ce);\n \t\tif (sub)\n \t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\tNULL, oid_to_hex(&ce->oid),\n+\t\t\t\tNULL, &ce->oid,\n \t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\tbreak;\n \n@@ -438,10 +438,10 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\t\t\tunlink_or_warn(ce->name);\n \n \t\t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\t\tNULL, oid_to_hex(&ce->oid), 0);\n+\t\t\t\t\tNULL, &ce->oid, 0);\n \t\t\t} else\n \t\t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\t\t\"HEAD\", oid_to_hex(&ce->oid),\n+\t\t\t\t\t\"HEAD\", &ce->oid,\n \t\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\t}\n \ndiff --git a/submodule.c b/submodule.c\nindex 6e14547e9e0..5b4e5227d90 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1597,9 +1597,9 @@ static void submodule_reset_index(const char *path)\n  * pass NULL for old or new respectively.\n  */\n int submodule_move_head(const char *path,\n-\t\t\t const char *old_head,\n-\t\t\t const char *new_head,\n-\t\t\t unsigned flags)\n+\t\t\tconst char *old_head,\n+\t\t\tconst struct object_id *new_oid,\n+\t\t\tunsigned flags)\n {\n \tint ret = 0;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1679,7 +1679,7 @@ int submodule_move_head(const char *path,\n \tif (!(flags & SUBMODULE_MOVE_HEAD_FORCE))\n \t\targv_array_push(&cp.args, old_head ? old_head : empty_tree_oid_hex());\n \n-\targv_array_push(&cp.args, new_head ? new_head : empty_tree_oid_hex());\n+\targv_array_push(&cp.args, new_oid ? oid_to_hex(new_oid) : empty_tree_oid_hex());\n \n \tif (run_command(&cp)) {\n \t\tret = error(_(\"Submodule '%s' could not be updated.\"), path);\n@@ -1687,7 +1687,7 @@ int submodule_move_head(const char *path,\n \t}\n \n \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n-\t\tif (new_head) {\n+\t\tif (new_oid) {\n \t\t\tchild_process_init(&cp);\n \t\t\t/* also set the HEAD accordingly */\n \t\t\tcp.git_cmd = 1;\n@@ -1696,7 +1696,7 @@ int submodule_move_head(const char *path,\n \n \t\t\tprepare_submodule_repo_env(&cp.env_array);\n \t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\",\n-\t\t\t\t\t \"--no-deref\", new_head, NULL);\n+\t\t\t\t\t \"--no-deref\", oid_to_hex(new_oid), NULL);\n \n \t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\ndiff --git a/submodule.h b/submodule.h\nindex 4644683e6cb..d1cceabb9b7 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -118,7 +118,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n #define SUBMODULE_MOVE_HEAD_FORCE   (1<<1)\n extern int submodule_move_head(const char *path,\n \t\t\t       const char *old,\n-\t\t\t       const char *new_head,\n+\t\t\t       const struct object_id *new_oid,\n \t\t\t       unsigned flags);\n \n void submodule_unset_core_worktree(const struct submodule *sub);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f9efee0836a..6c76bd6162a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -256,7 +256,7 @@ static void display_error_msgs(struct unpack_trees_options *o)\n \n static int check_submodule_move_head(const struct cache_entry *ce,\n \t\t\t\t     const char *old_id,\n-\t\t\t\t     const char *new_id,\n+\t\t\t\t     const struct object_id *new_id,\n \t\t\t\t     struct unpack_trees_options *o)\n {\n \tunsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;\n@@ -1513,7 +1513,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n \n \t\tif (submodule_from_ce(ce)) {\n \t\t\tint r = check_submodule_move_head(ce,\n-\t\t\t\t\"HEAD\", oid_to_hex(&ce->oid), o);\n+\t\t\t\t\"HEAD\", &ce->oid, o);\n \t\t\tif (r)\n \t\t\t\treturn o->gently ? -1 :\n \t\t\t\t\tadd_rejected_path(o, error_type, ce->name);\n@@ -1576,8 +1576,7 @@ static int verify_clean_submodule(const char *old_sha1,\n \tif (!submodule_from_ce(ce))\n \t\treturn 0;\n \n-\treturn check_submodule_move_head(ce, old_sha1,\n-\t\t\t\t\t oid_to_hex(&ce->oid), o);\n+\treturn check_submodule_move_head(ce, old_sha1, &ce->oid, o);\n }\n \n static int verify_clean_subdirectory(const struct cache_entry *ce,\n@@ -1821,8 +1820,7 @@ static int merged_entry(const struct cache_entry *ce,\n \n \t\tif (submodule_from_ce(ce)) {\n \t\t\tint ret = check_submodule_move_head(ce, NULL,\n-\t\t\t\t\t\t\t    oid_to_hex(&ce->oid),\n-\t\t\t\t\t\t\t    o);\n+\t\t\t\t\t\t\t    &ce->oid, o);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n@@ -1850,8 +1848,7 @@ static int merged_entry(const struct cache_entry *ce,\n \n \t\tif (submodule_from_ce(ce)) {\n \t\t\tint ret = check_submodule_move_head(ce, oid_to_hex(&old->oid),\n-\t\t\t\t\t\t\t    oid_to_hex(&ce->oid),\n-\t\t\t\t\t\t\t    o);\n+\t\t\t\t\t\t\t    &ce->oid, o);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354937","messageId":"20180808221752.195419-6-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 05/10] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:47Z","receivedAt":"2018-08-08T22:18:22Z","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 <sbeller@google.com>\n---\n submodule.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5b4e5227d90..bceeba13217 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,9 @@ 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 || !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.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354938","messageId":"20180808221752.195419-7-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 06/10] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:48Z","receivedAt":"2018-08-08T22:18:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of sorting it after we created an unsorted list, we could insert\ncorrectly into the list.  As the unsorted append is in order of cache entry\nnames, this is already sorted if names were equal to paths for submodules.\n\nAs submodule names are often the same as their path, the input is sorted\npretty well already, so let's just do the sort afterwards.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex bceeba13217..89a46b8af50 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1255,7 +1255,7 @@ 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(\n+\t\t\tif (!submodule || !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@@ -1349,6 +1349,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.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354939","messageId":"20180808221752.195419-8-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 07/10] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:49Z","receivedAt":"2018-08-08T22:18:27Z","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 <sbeller@google.com>\n---\n submodule.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 89a46b8af50..92988239f6b 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@@ -1256,7 +1260,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 || !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@@ -1348,8 +1352,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@@ -1358,7 +1362,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.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354940","messageId":"20180808221752.195419-9-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 08/10] submodule.c: do not copy around submodule list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:50Z","receivedAt":"2018-08-08T22:18:30Z","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\nAs a side effect, we'll have access to the util pointer for longer that\ncontains the commits that we need to fetch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 92988239f6b..21757e32908 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@@ -1362,7 +1363,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.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354941","messageId":"20180808221752.195419-10-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 09/10] submodule: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:51Z","receivedAt":"2018-08-08T22:18:32Z","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 <sbeller@google.com>\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 21757e32908..ec7ea6f8c2d 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@@ -1273,16 +1298,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@@ -1294,8 +1315,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 0f730d77815..4437cb17698 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -567,7 +567,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.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354942","messageId":"20180808221752.195419-11-sbeller@google.com","threadId":"49080","inReplyTo":"20180808221752.195419-1-sbeller@google.com","subject":"[PATCH 10/10] fetch: retry fetching submodules if sha1 were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T22:17:52Z","receivedAt":"2018-08-08T22:18:35Z","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 <submodule-dir>\" (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 on ref (refs/changes/<int>).\n\nRetry fetching a submodule if the object id that the superproject points\nto, cannot be found.\n\nNote: This is an RFC and doesn't support fetching to FETCH_HEAD yet, but\nonly into a local branch. To make fetching into FETCH_HEAD work, we need\nsome refactoring in builtin/fetch.c to adjust the calls to\n'check_for_new_submodule_commits'.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/fetch.c             |  9 ++---\n submodule.c                 | 79 ++++++++++++++++++++++++++++++++++++-\n t/t5526-fetch-submodules.sh | 16 ++++++++\n 3 files changed, 97 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 34d2bd123b3..9396e6a44c6 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, &current->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, &current->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 ec7ea6f8c2d..6cbd0b1a470 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1127,6 +1127,7 @@ 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 \n@@ -1259,8 +1260,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 *retry_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 +1283,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@@ -1318,9 +1323,46 @@ 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+\tretry_it = string_list_pop(&spf->retry);\n+\tif (retry_it) {\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tconst struct submodule *sub =\n+\t\t\t\tsubmodule_from_name(spf->r,\n+\t\t\t\t\t\t    &null_oid,\n+\t\t\t\t\t\t    retry_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\tgoto retry_next;\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(retry_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\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1334,14 +1376,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_remove_if(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 4437cb17698..ade4bf6d0bc 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -606,4 +606,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.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354969","messageId":"CAN0heSqjazimYu4de=xCQ9epSxezVyKPQQ16amZZvoaBh_SKdQ@mail.gmail.com","threadId":"49080","inReplyTo":"20180808221752.195419-3-sbeller@google.com","subject":"Re: [PATCH 02/10] string-list.h: add string_list_pop function.","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-08-09T07:35:51Z","receivedAt":"2018-08-09T07:35:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:\n> A string list can be used as a stack, but should we? A later patch shows\n> how useful this will be.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  string-list.c | 8 ++++++++\n>  string-list.h | 6 ++++++\n>  2 files changed, 14 insertions(+)\n>\n> diff --git a/string-list.c b/string-list.c\n> index 9f651bb4294..ea80afc8a0c 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -80,6 +80,14 @@ void string_list_remove(struct string_list *list, const char *string,\n>         }\n>  }\n>\n> +struct string_list_item *string_list_pop(struct string_list *list)\n> +{\n> +       if (list->nr == 0)\n> +               return 0;\n\nreturn NULL, not 0.\n\n> +\n> +       return &list->items[--list->nr];\n> +}\n> +\n\n> +/**\n> + * Returns the last item inserted and removes it from the list.\n> + * If the list is empty, return NULL.\n> + */\n> +struct string_list_item *string_list_pop(struct string_list *list);\n> +\n\nThe memory ownership is now with the caller. That is, the caller needs\nto check/know `list->strdup_strings` and know `free_util` to be able to\nproperly free all memory.\n\nOTOH, the pointer returned by this function is only guaranteed to be\nvalid until you start inserting into the list (well, you can do one\ninsertion per pop without worrying, but that's quite detailed\nimplementation knowledge).\n\nMaybe these caveats should be documented, or is a hint that this\n`_pop()` is not so nice to have after all, but let's see what happens in\nthe later patches...\n\nMartin\n"},{"id":"354970","messageId":"CAN0heSqwGoFwn34nRp3i09ExnjDPSY8WD=81M9jN5OW-ccpiTg@mail.gmail.com","threadId":"49080","inReplyTo":"20180808221752.195419-4-sbeller@google.com","subject":"Re: [PATCH 03/10] sha1-array: provide oid_array_remove_if","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-08-09T07:39:39Z","receivedAt":"2018-08-09T07:39:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:\n> +int oid_array_remove_if(struct oid_array *array,\n> +                       for_each_oid_fn fn,\n> +                       void *data)\n> +{\n> +       int i, j;\n> +       char *to_remove = xcalloc(array->nr, sizeof(char));\n\nDo you really need this scratch space? Let's see..\n\n> +       /* No oid_array_sort() here! See the api-oid-array.txt docs! */\n> +\n> +       for (i = 0; i < array->nr; i++) {\n> +               int ret = fn(array->oid + i, data);\n> +               if (ret)\n> +                       to_remove[i] = 1;\n> +       }\n> +\n> +       i = 0, j = 0;\n> +       while (i < array->nr && j < array->nr) {\n> +               while (i < array->nr && !to_remove[i])\n> +                       i++;\n> +               /* i at first marked for deletion or out */\n> +               if (j < i)\n> +                       j = i;\n> +               while (j < array->nr && to_remove[j])\n> +                       j++;\n> +               /* j > i; j at first valid after first deletion range or out */\n> +               if (i < array->nr && j < array->nr)\n> +                       oidcpy(&array->oid[i], &array->oid[j]);\n> +               else if (i >= array->nr)\n> +                       assert(j >= array->nr);\n> +                       /* no pruning happened, keep original array->nr */\n> +               else if (j >= array->nr)\n> +                       array->nr = i;\n> +       }\n> +\n> +       free(to_remove);\n> +\n> +       return 0;\n> +}\n\nI can't entirely follow this index-fiddling, but then I haven't had my\nmorning coffee yet, so please forgive me if this is nonsense. Would it\nsuffice to let i point out where to place items (starting at the first\nitem not to keep) and j where to take them from (i.e., the items to\nkeep, after the initial run)?\n\n> diff --git a/sha1-array.h b/sha1-array.h\n> index 232bf950172..151c7ad7f30 100644\n> --- a/sha1-array.h\n> +++ b/sha1-array.h\n> @@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,\n>  int oid_array_for_each_unique(struct oid_array *array,\n>                               for_each_oid_fn fn,\n>                               void *data);\n> +int oid_array_remove_if(struct oid_array *array,\n> +                       for_each_oid_fn fn,\n> +                       void *data);\n\nMaybe some documentation here, but this seems to be following suit. ;-)\n\nMartin\n"},{"id":"354971","messageId":"CAN0heSpsbYWzujzyteWuhto9DTXzvAkP+vt++d7ar3ob6Zx=Gg@mail.gmail.com","threadId":"49080","inReplyTo":"20180808221752.195419-11-sbeller@google.com","subject":"Re: [PATCH 10/10] fetch: retry fetching submodules if sha1 were not fetched","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-08-09T07:50:02Z","receivedAt":"2018-08-09T07:50:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:\n> Currently when git-fetch is asked to recurse into submodules, it dispatches\n> a plain \"git-fetch -C <submodule-dir>\" (and some submodule related options\n> such as prefix and recusing strategy, but) without any information of the\n> remote or the tip that should be fetched.\n>\n> This works surprisingly well in some workflows (such as using submodules\n> as a third party library), while not so well in other scenarios, such\n> as in a Gerrit topic-based workflow, that can tie together changes\n> (potentially across repositories) on the server side. One of the parts\n> of such a Gerrit workflow is to download a change when wanting to examine\n> it, and you'd want to have its submodule changes that are in the same\n> topic downloaded as well. However these submodule changes reside in their\n> own repository in their on ref (refs/changes/<int>).\n\ns/on/own/\n\n> Retry fetching a submodule if the object id that the superproject points\n> to, cannot be found.\n>\n> Note: This is an RFC and doesn't support fetching to FETCH_HEAD yet, but\n> only into a local branch. To make fetching into FETCH_HEAD work, we need\n> some refactoring in builtin/fetch.c to adjust the calls to\n> 'check_for_new_submodule_commits'.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\n> diff --git a/submodule.c b/submodule.c\n> index ec7ea6f8c2d..6cbd0b1a470 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1127,6 +1127,7 @@ struct submodule_parallel_fetch {\n>         int result;\n>\n>         struct string_list changed_submodule_names;\n> +       struct string_list retry;\n>  };\n>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n\n`retry` will effectively be `STRING_LIST_INIT_NODUP`, but making that\nexplicit would be better and the next addition to the struct would be\neasier to get right.\n\n> +retry_next:\n> +       retry_it = string_list_pop(&spf->retry);\n> +       if (retry_it) {\n> +               struct strbuf submodule_prefix = STRBUF_INIT;\n> +               const struct submodule *sub =\n> +                               submodule_from_name(spf->r,\n> +                                                   &null_oid,\n> +                                                   retry_it->string);\n> +\n> +               child_process_init(cp);\n> +               cp->dir = get_submodule_git_dir(spf->r, sub->path);\n> +               if (!cp->dir)\n> +                       goto retry_next;\n\nSo here you just drop the string list item. Since it's NODUP, and since\nthe `util` pointers are owned elsewhere(?), this seems fine. Other uses\nof `string_list_pop()` might not be so straightforward.\n\nJust a thought, but rather than pop+if+goto, maybe\n\nwhile ((retry_it = )) {\n        ...\n        if (!cp->dir) continue;\n        ...\n        return 1;\n}\n\nI haven't commented on any of the submodule stuff, which is probably\nwhere you'd be most interested in comments. I don't use submodules, nor\ndo I know the code that runs them.. I guess my comments are more \"if\nthose who know something about submodules find this series worthwhile,\nyou might want to consider my comments as well\".\n\nMartin\n"},{"id":"355012","messageId":"CAGZ79kYtKZJ0Dm0ejxrotnoaNERtR0J6=DMCgweCObdJr0K6-w@mail.gmail.com","threadId":"49080","inReplyTo":"CAN0heSqwGoFwn34nRp3i09ExnjDPSY8WD=81M9jN5OW-ccpiTg@mail.gmail.com","subject":"Re: [PATCH 03/10] sha1-array: provide oid_array_remove_if","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-09T17:25:52Z","receivedAt":"2018-08-09T17:26:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 9, 2018 at 12:39 AM Martin Ågren <martin.agren@gmail.com> wrote:\n>\n> On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:\n> > +int oid_array_remove_if(struct oid_array *array,\n> > +                       for_each_oid_fn fn,\n> > +                       void *data)\n> > +{\n> > +       int i, j;\n> > +       char *to_remove = xcalloc(array->nr, sizeof(char));\n>\n> Do you really need this scratch space?\n\nI don't think so, when we reorder the items while iterating over them.\n\nI though reordering them later would be easier, but I am not sure anymore.\n\n>\n> > +       /* No oid_array_sort() here! See the api-oid-array.txt docs! */\n> > +\n> > +       for (i = 0; i < array->nr; i++) {\n> > +               int ret = fn(array->oid + i, data);\n> > +               if (ret)\n> > +                       to_remove[i] = 1;\n> > +       }\n> > +\n> > +       i = 0, j = 0;\n> > +       while (i < array->nr && j < array->nr) {\n> > +               while (i < array->nr && !to_remove[i])\n> > +                       i++;\n> > +               /* i at first marked for deletion or out */\n> > +               if (j < i)\n> > +                       j = i;\n> > +               while (j < array->nr && to_remove[j])\n> > +                       j++;\n> > +               /* j > i; j at first valid after first deletion range or out */\n> > +               if (i < array->nr && j < array->nr)\n> > +                       oidcpy(&array->oid[i], &array->oid[j]);\n> > +               else if (i >= array->nr)\n> > +                       assert(j >= array->nr);\n> > +                       /* no pruning happened, keep original array->nr */\n> > +               else if (j >= array->nr)\n> > +                       array->nr = i;\n> > +       }\n> > +\n> > +       free(to_remove);\n> > +\n> > +       return 0;\n> > +}\n>\n> I can't entirely follow this index-fiddling, but then I haven't had my\n> morning coffee yet, so please forgive me if this is nonsense. Would it\n> suffice to let i point out where to place items (starting at the first\n> item not to keep) and j where to take them from (i.e., the items to\n> keep, after the initial run)?\n\nI thought this is what happens, just after the actual loop of calls.\n\n> > +int oid_array_remove_if(struct oid_array *array,\n> > +                       for_each_oid_fn fn,\n> > +                       void *data);\n>\n> Maybe some documentation here, but this seems to be following suit. ;-)\n\nWorth mentioning: the order is kept stable. (c.f. shrink_potential_moved_blocks\nin diff.c which also \"compacts an array\", but without stable order).\n\nThanks for the review. I'll try to rewrite this to be more legible.\n\nThanks,\nStefan\n"},{"id":"355019","messageId":"CAGZ79kbkLDhiqYrd6epPVP7dLLR7AFApyas=_mgeCucngJONMg@mail.gmail.com","threadId":"49080","inReplyTo":"CAN0heSpsbYWzujzyteWuhto9DTXzvAkP+vt++d7ar3ob6Zx=Gg@mail.gmail.com","subject":"Re: [PATCH 10/10] fetch: retry fetching submodules if sha1 were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-09T17:42:34Z","receivedAt":"2018-08-09T17:42:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 9, 2018 at 12:50 AM Martin Ågren <martin.agren@gmail.com> wrote:\n>\n> On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:\n> > Currently when git-fetch is asked to recurse into submodules, it dispatches\n> > a plain \"git-fetch -C <submodule-dir>\" (and some submodule related options\n> > such as prefix and recusing strategy, but) without any information of the\n> > remote or the tip that should be fetched.\n> >\n> > This works surprisingly well in some workflows (such as using submodules\n> > as a third party library), while not so well in other scenarios, such\n> > as in a Gerrit topic-based workflow, that can tie together changes\n> > (potentially across repositories) on the server side. One of the parts\n> > of such a Gerrit workflow is to download a change when wanting to examine\n> > it, and you'd want to have its submodule changes that are in the same\n> > topic downloaded as well. However these submodule changes reside in their\n> > own repository in their on ref (refs/changes/<int>).\n>\n> s/on/own/\n>\n> > Retry fetching a submodule if the object id that the superproject points\n> > to, cannot be found.\n> >\n> > Note: This is an RFC and doesn't support fetching to FETCH_HEAD yet, but\n> > only into a local branch. To make fetching into FETCH_HEAD work, we need\n> > some refactoring in builtin/fetch.c to adjust the calls to\n> > 'check_for_new_submodule_commits'.\n> >\n> > Signed-off-by: Stefan Beller <sbeller@google.com>\n> > ---\n>\n> > diff --git a/submodule.c b/submodule.c\n> > index ec7ea6f8c2d..6cbd0b1a470 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -1127,6 +1127,7 @@ struct submodule_parallel_fetch {\n> >         int result;\n> >\n> >         struct string_list changed_submodule_names;\n> > +       struct string_list retry;\n> >  };\n> >  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n>\n> `retry` will effectively be `STRING_LIST_INIT_NODUP`, but making that\n> explicit would be better and the next addition to the struct would be\n> easier to get right.\n>\n> > +retry_next:\n> > +       retry_it = string_list_pop(&spf->retry);\n> > +       if (retry_it) {\n> > +               struct strbuf submodule_prefix = STRBUF_INIT;\n> > +               const struct submodule *sub =\n> > +                               submodule_from_name(spf->r,\n> > +                                                   &null_oid,\n> > +                                                   retry_it->string);\n> > +\n> > +               child_process_init(cp);\n> > +               cp->dir = get_submodule_git_dir(spf->r, sub->path);\n> > +               if (!cp->dir)\n> > +                       goto retry_next;\n>\n> So here you just drop the string list item. Since it's NODUP, and since\n> the `util` pointers are owned elsewhere(?), this seems fine. Other uses\n> of `string_list_pop()` might not be so straightforward.\n>\n> Just a thought, but rather than pop+if+goto, maybe\n>\n> while ((retry_it = )) {\n>         ...\n>         if (!cp->dir) continue;\n>         ...\n>         return 1;\n> }\n\nI really want to keep the retry list short and pruned, as this\nfunction is called O(n) times with n the number of submodules\nand the retry list will also be up to n.\nAnd with that we'd run O(n^2) times into \"if (!..) continue;\".\n\nWhen we use the 'pop-no-work items' logic, then we're still in O(n).\n\n> I haven't commented on any of the submodule stuff, which is probably\n> where you'd be most interested in comments. I don't use submodules, nor\n> do I know the code that runs them.. I guess my comments are more \"if\n> those who know something about submodules find this series worthwhile,\n> you might want to consider my comments as well\".\n\nThanks for your comments! I'll try to think of another way to\nrepresent this more easily in code.\n\nThanks,\nStefan\n"},{"id":"355025","messageId":"20180809192454.GB32376@sigill.intra.peff.net","threadId":"49080","inReplyTo":"CAGZ79kYtKZJ0Dm0ejxrotnoaNERtR0J6=DMCgweCObdJr0K6-w@mail.gmail.com","subject":"Re: [PATCH 03/10] sha1-array: provide oid_array_remove_if","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T19:24:54Z","receivedAt":"2018-08-09T19:24:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 10:25:52AM -0700, Stefan Beller wrote:\n\n> On Thu, Aug 9, 2018 at 12:39 AM Martin Ågren <martin.agren@gmail.com> wrote:\n> >\n> > On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:\n> > > +int oid_array_remove_if(struct oid_array *array,\n> > > +                       for_each_oid_fn fn,\n> > > +                       void *data)\n> > > +{\n> > > +       int i, j;\n> > > +       char *to_remove = xcalloc(array->nr, sizeof(char));\n> >\n> > Do you really need this scratch space?\n> \n> I don't think so, when we reorder the items while iterating over them.\n\nEven with keeping the order this can be done in a single linear pass.\nSee filter_string_list() for an example.\n\nThe one twist here is that you cannot:\n\n  oidcpy(array->oid[i], array->oid[j]);\n\nwhen i==j, because of memcpy restrictions. With the current\nimplementation it would suffice to use struct assignment (and really,\nevery oidcpy() could just be struct assignment these days). But you\ncould also just do:\n\n  if (i != j)\n    oidcpy(array->oid[i], array->oid[j]);\n\n> > I can't entirely follow this index-fiddling, but then I haven't had my\n> > morning coffee yet, so please forgive me if this is nonsense. Would it\n> > suffice to let i point out where to place items (starting at the first\n> > item not to keep) and j where to take them from (i.e., the items to\n> > keep, after the initial run)?\n> \n> I thought this is what happens, just after the actual loop of calls.\n\nI think the point is that we can just maintain those meanings during the\nsingle walk through the array. The result is simpler to read and more\nefficient.\n\n-Peff\n"},{"id":"355039","messageId":"xmqq7ekzb5qj.fsf@gitster-ct.c.googlers.com","threadId":"49080","inReplyTo":"20180808221752.195419-2-sbeller@google.com","subject":"Re: [PATCH 01/10] string_list: print_string_list to use trace_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T21:16:36Z","receivedAt":"2018-08-09T21:16:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> It is a debugging aid, so it should print to the debugging channel.\n\nWho says?  In-tree code may say so, and I do not think any in-flight\ntopic up to 'pu' uses this to produce non-debugging output, but I do\nnot think it is healthy attitude to assume that you can take over an\nexisting function and change what it does unilaterally.\n\nAs there is no in-tree or in-flight user, I think it makes sense if\nthe proposed change were to rename it to trace_string_list().  If\nthere weren't any print_string_list() and we were adding a debugging\naid to use trace_printf() to dump these, that would be the name we\nwould use anyway.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  string-list.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/string-list.c b/string-list.c\n> index 771c4550980..9f651bb4294 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -200,9 +200,9 @@ void print_string_list(const struct string_list *p, const char *text)\n>  {\n>  \tint i;\n>  \tif ( text )\n> -\t\tprintf(\"%s\\n\", text);\n> +\t\ttrace_printf(\"%s\\n\", text);\n>  \tfor (i = 0; i < p->nr; i++)\n> -\t\tprintf(\"%s:%p\\n\", p->items[i].string, p->items[i].util);\n> +\t\ttrace_printf(\"%s:%p\\n\", p->items[i].string, p->items[i].util);\n>  }\n>  \n>  struct string_list_item *string_list_append_nodup(struct string_list *list,\n"},{"id":"355042","messageId":"xmqqzhxv9qkj.fsf@gitster-ct.c.googlers.com","threadId":"49080","inReplyTo":"CAN0heSqjazimYu4de=xCQ9epSxezVyKPQQ16amZZvoaBh_SKdQ@mail.gmail.com","subject":"Re: [PATCH 02/10] string-list.h: add string_list_pop function.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T21:29:32Z","receivedAt":"2018-08-09T21:29:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:\n>> A string list can be used as a stack, but should we? A later patch shows\n>> how useful this will be.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  string-list.c | 8 ++++++++\n>>  string-list.h | 6 ++++++\n>>  2 files changed, 14 insertions(+)\n>>\n>> diff --git a/string-list.c b/string-list.c\n>> index 9f651bb4294..ea80afc8a0c 100644\n>> --- a/string-list.c\n>> +++ b/string-list.c\n>> @@ -80,6 +80,14 @@ void string_list_remove(struct string_list *list, const char *string,\n>>         }\n>>  }\n>>\n>> +struct string_list_item *string_list_pop(struct string_list *list)\n>> +{\n>> +       if (list->nr == 0)\n>> +               return 0;\n>\n> return NULL, not 0.\n\nIt is OK to return NULL, which may make the caller a bit simpler,\ni.e.\n\n\twhile (item = list_pop(list))\n\t\twork_on(item);\n\nbut if we consider it a good discipline for the caller to see if\nthere are still things on the stack before attempting to pop, then\ninstead of returning NULL, we can have BUG(\"\") there, which requires\nthe caller to be more like this:\n\n\twhile (list->nr)\n\t\twork_on(list_pop(list));\n\nwhich is not so bad.\n\n>> +/**\n>> + * Returns the last item inserted and removes it from the list.\n>> + * If the list is empty, return NULL.\n>> + */\n>> +struct string_list_item *string_list_pop(struct string_list *list);\n>> +\n>\n> The memory ownership is now with the caller. That is, the caller needs\n> to check/know `list->strdup_strings` and know `free_util` to be able to\n> properly free all memory.\n\n> OTOH, the pointer returned by this function is only guaranteed to be\n> valid until you start inserting into the list (well, you can do one\n> insertion per pop without worrying, but that's quite detailed\n> implementation knowledge).\n\nYes, it is a more grave limitation when using string_list as a\nstack.  A single realloc() and you are dead X-<.\n\n"},{"id":"355045","messageId":"CAGZ79kZ-0R4qnZ7LMhncenOeoec3z+kNmt2_VqBKRiDXxKm6fQ@mail.gmail.com","threadId":"49080","inReplyTo":"xmqq7ekzb5qj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 01/10] string_list: print_string_list to use trace_printf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-09T21:40:45Z","receivedAt":"2018-08-09T21:40:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 9, 2018 at 2:16 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > It is a debugging aid, so it should print to the debugging channel.\n>\n> Who says?\n\nThe comment in the header file?\n\n  /**\n   * Dump a string_list to stdout, useful mainly for debugging\n   * purposes. It can take an optional header argument and it writes out\n   * the string-pointer pairs of the string_list, each one in its own\n   * line.\n   */\n\n>  In-tree code may say so, and I do not think any in-flight\n> topic up to 'pu' uses this to produce non-debugging output, but I do\n> not think it is healthy attitude to assume that you can take over an\n> existing function and change what it does unilaterally.\n\nThat is reasonable, as usual, and given the recent fallout of the\nobject store refactoring I have these concerns on my mind, too.\n\nBut for this instance, I do not see any risk of accidental collisions with\nother series in flight:\n\n$ git log -S print_string_list origin/pu --oneline\n4f665f2cf33 string-list.h: move documentation from Documentation/api/\ninto header\nd10cb3dfab9 help.c: rename function \"pretty_print_string_list\"\nc455c87c5cd Rename path_list to string_list\n1eb056905a2 include $PATH in generating list of commands for \"help -a\"\n70827b15bfb Split up builtin commands into separate files from git.c\n27dedf0c3b7 (tag: v1.0rc3, tag: v0.99.9j) GIT 0.99.9j aka 1.0rc3\n8e49d503882 C implementation of the 'git' program, take two.\n\n> As there is no in-tree or in-flight user, I think it makes sense if\n> the proposed change were to rename it to trace_string_list().  If\n> there weren't any print_string_list() and we were adding a debugging\n> aid to use trace_printf() to dump these, that would be the name we\n> would use anyway.\n\nGood call. I'll rename and send separately (or might even drop this\npatch completely; this patch is not used in the patches to follow;\nit is more a \"FYI: here is a thing that I found helpful for debugging,\nunlike the existing function, which I found unhelpful and actively\nharmful\")\n"},{"id":"355047","messageId":"20180809214148.GD11342@sigill.intra.peff.net","threadId":"49080","inReplyTo":"xmqqzhxv9qkj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 02/10] string-list.h: add string_list_pop function.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T21:41:48Z","receivedAt":"2018-08-09T21:41:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 02:29:32PM -0700, Junio C Hamano wrote:\n\n> >> +struct string_list_item *string_list_pop(struct string_list *list)\n> >> +{\n> >> +       if (list->nr == 0)\n> >> +               return 0;\n> >\n> > return NULL, not 0.\n> \n> It is OK to return NULL, which may make the caller a bit simpler,\n> i.e.\n> \n> \twhile (item = list_pop(list))\n> \t\twork_on(item);\n> \n> but if we consider it a good discipline for the caller to see if\n> there are still things on the stack before attempting to pop, then\n> instead of returning NULL, we can have BUG(\"\") there, which requires\n> the caller to be more like this:\n> \n> \twhile (list->nr)\n> \t\twork_on(list_pop(list));\n> \n> which is not so bad.\n\nIn many cases you can just do:\n\n  while (list->nr) {\n\twork_on(list->items[list->nr - 1]);\n\tlist_remove(list, list->nr - 1);\n  }\n\nand then all of those memory ownership issues like:\n\n> > The memory ownership is now with the caller. That is, the caller needs\n> > to check/know `list->strdup_strings` and know `free_util` to be able to\n> > properly free all memory.\n> \n> > OTOH, the pointer returned by this function is only guaranteed to be\n> > valid until you start inserting into the list (well, you can do one\n> > insertion per pop without worrying, but that's quite detailed\n> > implementation knowledge).\n> \n> Yes, it is a more grave limitation when using string_list as a\n> stack.  A single realloc() and you are dead X-<.\n\njust go away. :)\n\nWhere that falls down is if you really need work_on() to put more items\non the stack, but only after you've removed the current top. But then\nwriting it out may still be nicer, because it makes it clear you have to\ndo:\n\n  const char *cur_string = xstrdup(list->items[list->nr-1].string);\n\nif you want the data to live past the removal.\n\n-Peff\n"},{"id":"355048","messageId":"xmqqsh3n9pvv.fsf@gitster-ct.c.googlers.com","threadId":"49080","inReplyTo":"20180808221752.195419-4-sbeller@google.com","subject":"Re: [PATCH 03/10] sha1-array: provide oid_array_remove_if","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T21:44:20Z","receivedAt":"2018-08-09T21:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  sha1-array.c | 39 +++++++++++++++++++++++++++++++++++++++\n>  sha1-array.h |  3 +++\n>  2 files changed, 42 insertions(+)\n>\n> diff --git a/sha1-array.c b/sha1-array.c\n> index 265941fbf40..10eb08b425e 100644\n> --- a/sha1-array.c\n> +++ b/sha1-array.c\n> @@ -77,3 +77,42 @@ int oid_array_for_each_unique(struct oid_array *array,\n>  \t}\n>  \treturn 0;\n>  }\n> +\n> +int oid_array_remove_if(struct oid_array *array,\n> +\t\t\tfor_each_oid_fn fn,\n> +\t\t\tvoid *data)\n> +{\n> +\tint i, j;\n> +\tchar *to_remove = xcalloc(array->nr, sizeof(char));\n> +\n> +\t/* No oid_array_sort() here! See the api-oid-array.txt docs! */\n> +\n> +\tfor (i = 0; i < array->nr; i++) {\n> +\t\tint ret = fn(array->oid + i, data);\n> +\t\tif (ret)\n> +\t\t\tto_remove[i] = 1;\n> +\t}\n\nDoing the same without this secondary array and loop, i.e.\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        array->nr = dst;\n\nwould be no less efficient.  The only reason why you might want to\nmeasure move-span by a secondary array and preliminary counting loop\nlike your version does is that moving contiguous area of memory may\nbe more efficient than moving only by a single oid sized chunks, but\nas far as I can tell you are not doing that \"optimization\", either.\n\nI doubt that remove_if() is particularly a good name.  A version of\nthis function, for which polarity of fn() is reversed, can be called\n\"grep\", or \"filter\", I think, and would be more understandable.\n\n"},{"id":"355050","messageId":"xmqqo9eb9ps5.fsf@gitster-ct.c.googlers.com","threadId":"49080","inReplyTo":"20180809192454.GB32376@sigill.intra.peff.net","subject":"Re: [PATCH 03/10] sha1-array: provide oid_array_remove_if","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T21:46:34Z","receivedAt":"2018-08-09T21:46:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Even with keeping the order this can be done in a single linear pass.\n> See filter_string_list() for an example.\n\nHeh, I just wasted a few minutes saying the same; I should have\npointed him at these two lines ;-)\n"},{"id":"355051","messageId":"CAGZ79kZK=5gftetj3XLTbu-n3WKwRUQQDj12yxDBV0WEN8gg9Q@mail.gmail.com","threadId":"49080","inReplyTo":"20180809214148.GD11342@sigill.intra.peff.net","subject":"Re: [PATCH 02/10] string-list.h: add string_list_pop function.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-09T21:52:29Z","receivedAt":"2018-08-09T21:52:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 9, 2018 at 2:41 PM Jeff King <peff@peff.net> wrote:\n\n> >\n> >       while (list->nr)\n> >               work_on(list_pop(list));\n> >\n> > which is not so bad.\n>\n> In many cases you can just do:\n>\n>   while (list->nr) {\n>         work_on(list->items[list->nr - 1]);\n>         list_remove(list, list->nr - 1);\n>   }\n>\n> and then all of those memory ownership issues like:\n\n[...]\n>\n> just go away. :)\n\nThe only complication here is the lack of list_remove(index),\nwe do have list_remove(string), which internally searches the\nitem and removes it. Hence I did not want to use it.\n\nAnother idea I had was to keep the list immutable (except amending,\njust like a constitution ;-) and store an index of how far we got in that\nlist already. That wastes memory for keeping entries around, but is safe\nfor memory due to its nature.\n\n> Where that falls down is if you really need work_on() to put more items\n> on the stack, but only after you've removed the current top. But then\n> writing it out may still be nicer, because it makes it clear you have to\n> do:\n>\n>   const char *cur_string = xstrdup(list->items[list->nr-1].string);\n\nAnother way would be to use\n\n  string_list_pop(&list, &string_dst, &util_dst);\ni.e.\n  /* Returns 0 if the dst was filled */\n  int (struct string_list *, char **, void**)\n\nas then we do not expose the internals and would not have issues\nwith reallocs.\n\n> if you want the data to live past the removal.\n\nIn the code proposed there are no additions (hence no reallocs)\nand the need for the data is short lived.\n\nBut I can see how the design was just fitting my purpose and\nwe could come up with some better API.\n\nThanks,\nStefan\n"},{"id":"355053","messageId":"20180809215637.GA12441@sigill.intra.peff.net","threadId":"49080","inReplyTo":"CAGZ79kZK=5gftetj3XLTbu-n3WKwRUQQDj12yxDBV0WEN8gg9Q@mail.gmail.com","subject":"Re: [PATCH 02/10] string-list.h: add string_list_pop function.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T21:56:37Z","receivedAt":"2018-08-09T21:56:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 02:52:29PM -0700, Stefan Beller wrote:\n\n> > In many cases you can just do:\n> >\n> >   while (list->nr) {\n> >         work_on(list->items[list->nr - 1]);\n> >         list_remove(list, list->nr - 1);\n> >   }\n> >\n> > and then all of those memory ownership issues like:\n> \n> [...]\n> >\n> > just go away. :)\n> \n> The only complication here is the lack of list_remove(index),\n> we do have list_remove(string), which internally searches the\n> item and removes it. Hence I did not want to use it.\n\nHeh, I almost dug into that more.\n\nI think you could have helpers to spell the two lines above even more\nnicely:\n\n  while (list->nr) {\n        work_on(list_top(list));\n\tlist_pop(list); /* note this doesn't return anything! */\n  }\n\nBut yes, it's not possible with the current functions.\n\n> Another idea I had was to keep the list immutable (except amending,\n> just like a constitution ;-) and store an index of how far we got in that\n> list already. That wastes memory for keeping entries around, but is safe\n> for memory due to its nature.\n\nYou can also use a list.h linked-list. Then removal from the list and\nfreeing are two separate operations (but it exercises your malloc a lot\nmore if you're constantly pushing and popping).\n\n> > Where that falls down is if you really need work_on() to put more items\n> > on the stack, but only after you've removed the current top. But then\n> > writing it out may still be nicer, because it makes it clear you have to\n> > do:\n> >\n> >   const char *cur_string = xstrdup(list->items[list->nr-1].string);\n> \n> Another way would be to use\n> \n>   string_list_pop(&list, &string_dst, &util_dst);\n> i.e.\n>   /* Returns 0 if the dst was filled */\n>   int (struct string_list *, char **, void**)\n> \n> as then we do not expose the internals and would not have issues\n> with reallocs.\n\nYes, I almost suggested that, but there's the question of memory\nownership of string_dst. Does it need freed or not? Is that answer\ndependent on the strdup_strings flag?\n\n> > if you want the data to live past the removal.\n> \n> In the code proposed there are no additions (hence no reallocs)\n> and the need for the data is short lived.\n> \n> But I can see how the design was just fitting my purpose and\n> we could come up with some better API.\n\nYeah, I didn't actually dig into your use case. I just want to make sure\nwe don't add a crappy function to our API. ;)\n\n-Peff\n"},{"id":"355055","messageId":"xmqqk1oz9p5j.fsf@gitster-ct.c.googlers.com","threadId":"49080","inReplyTo":"20180808221752.195419-5-sbeller@google.com","subject":"Re: [PATCH 04/10] submodule.c: convert submodule_move_head new argument to object id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T22:00:08Z","receivedAt":"2018-08-09T22:00:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> All callers use oid_to_hex to convert the desired oid to a string before\n> calling submodule_move_head. Defer the conversion to the\n> submodule_move_head as it will turn out to be useful in a bit.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  entry.c        |  6 +++---\n>  submodule.c    | 12 ++++++------\n>  submodule.h    |  2 +-\n>  unpack-trees.c | 13 +++++--------\n>  4 files changed, 15 insertions(+), 18 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index b5d1d3cf231..4b34dfd30df 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -358,7 +358,7 @@ static int write_entry(struct cache_entry *ce,\n>  \t\tsub = submodule_from_ce(ce);\n>  \t\tif (sub)\n>  \t\t\treturn submodule_move_head(ce->name,\n> -\t\t\t\tNULL, oid_to_hex(&ce->oid),\n> +\t\t\t\tNULL, &ce->oid,\n>  \t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n>  \t\tbreak;\n\nNice.\n\n"},{"id":"355058","messageId":"CAGZ79kaDSkwUqfbtRGtfuLKMQs+JcfA75T2cHNxK52hYP+cF-w@mail.gmail.com","threadId":"49080","inReplyTo":"20180809215637.GA12441@sigill.intra.peff.net","subject":"Re: [PATCH 02/10] string-list.h: add string_list_pop function.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-09T22:10:21Z","receivedAt":"2018-08-09T22:10:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 9, 2018 at 2:56 PM Jeff King <peff@peff.net> wrote:\n\n>\n> I think you could have helpers to spell the two lines above even more\n> nicely:\n>\n>   while (list->nr) {\n>         work_on(list_top(list));\n>         list_pop(list); /* note this doesn't return anything! */\n>   }\n>\n> But yes, it's not possible with the current functions.\n\nI like this one most, and as we manage our own memory via the\nalloc variable we also would not see a lot of memory churn for constant\npush/pop traffic.\n\n> You can also use a list.h linked-list. Then removal from the list and\n> freeing are two separate operations (but it exercises your malloc a lot\n> more if you're constantly pushing and popping).\n\nFor that I'd have to define my own type derived from list.h to carry\nthe string and the util pointer, which looks very similar to the string_list\nwe already have from a users POV.\n\n> > > Where that falls down is if you really need work_on() to put more items\n> > > on the stack, but only after you've removed the current top. But then\n> > > writing it out may still be nicer, because it makes it clear you have to\n> > > do:\n> > >\n> > >   const char *cur_string = xstrdup(list->items[list->nr-1].string);\n> >\n> > Another way would be to use\n> >\n> >   string_list_pop(&list, &string_dst, &util_dst);\n> > i.e.\n> >   /* Returns 0 if the dst was filled */\n> >   int (struct string_list *, char **, void**)\n> >\n> > as then we do not expose the internals and would not have issues\n> > with reallocs.\n>\n> Yes, I almost suggested that, but there's the question of memory\n> ownership of string_dst. Does it need freed or not? Is that answer\n> dependent on the strdup_strings flag?\n\nSure. But as the caller, you should know?\nYou constructed that string_list.\n"}]}