{"thread":{"id":"49278","subject":"[PATCH 00/11] fetch: make sure submodule oids are fetched","startedAt":"2018-09-04T23:01:59Z","lastAt":"2018-09-11T21:03:34Z","messageCount":39,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"357390","messageId":"20180904230149.180332-1-sbeller@google.com","threadId":"49278","inReplyTo":null,"subject":"[PATCH 00/11] fetch: make sure submodule oids are fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:38Z","receivedAt":"2018-09-04T23:01:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a resend of [1] and was rebased to origin/master and all review comments\nhave been addressed.\n\nThanks,\nStefan\n\n[1] https://public-inbox.org/git/20180808221752.195419-1-sbeller@google.com/\n\nStefan Beller (11):\n string_list: print_string_list to use trace_printf\n string-list.h: add string_list_{pop, last} functions\n sha1-array: provide oid_array_filter\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 builtin/fetch: check for submodule updates for non branch fetches\n\n builtin/fetch.c | 14 +--\n entry.c | 6 +-\n sha1-array.c | 18 ++++\n sha1-array.h | 5 +\n string-list.c | 20 +++-\n string-list.h | 15 ++-\n submodule.c | 200 ++++++++++++++++++++++++++++--------\n submodule.h | 2 +-\n t/t5526-fetch-submodules.sh | 23 ++++-\n unpack-trees.c | 13 +--\n 10 files changed, 246 insertions(+), 70 deletions(-)\n\n-- \n2.19.0.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357391","messageId":"20180904230149.180332-2-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 01/11] string_list: print_string_list to use trace_printf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:39Z","receivedAt":"2018-09-04T23:02:01Z","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 \n---\n string-list.c | 6 +++---\n string-list.h | 4 ++--\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 771c4550980..1ebbe1f56ea 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -196,13 +196,13 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n }\n \n \n-void print_string_list(const struct string_list *p, const char *text)\n+void trace_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,\ndiff --git a/string-list.h b/string-list.h\nindex ff8f6094a33..5b22560cf19 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -114,12 +114,12 @@ void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t want, void *cb_data);\n \n /**\n- * Dump a string_list to stdout, useful mainly for debugging\n+ * Dump a string_list using the trace_print, 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-void print_string_list(const struct string_list *p, const char *text);\n+void trace_print_string_list(const struct string_list *p, const char *text);\n \n /**\n * Free a string_list. The `string` pointer of the items will be freed\n-- \n2.19.0.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357392","messageId":"20180904230149.180332-3-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 02/11] string-list.h: add string_list_{pop, last} functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:40Z","receivedAt":"2018-09-04T23:02:04Z","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\nIn an earlier iteration of this patch it was suggested to return the last\nelement or NULL (if empty), to enable a pattern of\n\n while ((item = string_list_pop(&list))\n\twork_on(item);\n\nBut it turns out that this pattern is error-prone as the memory model\nof the returned item is unclear. The caller would need to free the\nitem->string if the string list is a duplicating list; and more importantly\nif there are further calls to insert items into the list, a memory\nreallocation might occur, invalidating the returned item at some time in\nthe future.\n\nThat is why the implementation doesn't return any item and the caller\nneeds to be worded as\n\n while (list.nr) {\n work_on(last(&list]));\n string_list_pop(&list, free_util);\n }\n\nAlso provide the function to access the last item in the list.\n\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 1ebbe1f56ea..21559f222a7 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 5b22560cf19..d7cdf38e57a 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+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.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357393","messageId":"20180904230149.180332-4-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 03/11] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:41Z","receivedAt":"2018-09-04T23:02:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Helped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \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..7eada4d1811 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..b516ec4bffc 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 retain it if fn returns 0, remove 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.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357394","messageId":"20180904230149.180332-5-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 04/11] submodule.c: convert submodule_move_head new argument to object id","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:42Z","receivedAt":"2018-09-04T23:02:08Z","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 \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 2a2ab6c8394..0b676f997b2 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@@ -439,10 +439,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 50cbf5f13ed..da2ed8696f5 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 7d476cefa7e..ef34d5a7ca8 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -124,7 +124,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n #define SUBMODULE_MOVE_HEAD_FORCE (1<<1)\n int submodule_move_head(const char *path,\n \t\t\tconst char *old,\n-\t\t\tconst char *new_head,\n+\t\t\tconst struct object_id *new_oid,\n \t\t\tunsigned flags);\n \n void submodule_unset_core_worktree(const struct submodule *sub);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f25089b878a..75d1b294ade 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@@ -1517,7 +1517,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@@ -1591,8 +1591,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@@ -1836,8 +1835,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@@ -1865,8 +1863,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.19.0.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357395","messageId":"20180904230149.180332-6-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 05/11] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:43Z","receivedAt":"2018-09-04T23:02:11Z","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 | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex da2ed8696f5..8345d423fda 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.19.0.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357396","messageId":"20180904230149.180332-7-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 06/11] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:44Z","receivedAt":"2018-09-04T23:02:13Z","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 \n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 8345d423fda..8bee455137a 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.19.0.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357397","messageId":"20180904230149.180332-8-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 07/11] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:45Z","receivedAt":"2018-09-04T23:02:16Z","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 8bee455137a..582c0263b91 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.19.0.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357398","messageId":"20180904230149.180332-9-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 08/11] submodule.c: do not copy around submodule list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:46Z","receivedAt":"2018-09-04T23:02:18Z","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 \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 582c0263b91..0efe6711a8c 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.19.0.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357399","messageId":"20180904230149.180332-10-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 09/11] submodule: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:47Z","receivedAt":"2018-09-04T23:02: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 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 0efe6711a8c..8b67f086895 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 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.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357400","messageId":"20180904230149.180332-11-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 10/11] fetch: retry fetching submodules if sha1 were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:48Z","receivedAt":"2018-09-04T23:02:23Z","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\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 \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 8b67f086895..702cba138be 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@@ -1318,9 +1325,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@@ -1334,14 +1382,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.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357401","messageId":"20180904230149.180332-12-sbeller@google.com","threadId":"49278","inReplyTo":"20180904230149.180332-1-sbeller@google.com","subject":"[PATCH 11/11] builtin/fetch: check for submodule updates for non branch fetches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T23:01:49Z","receivedAt":"2018-09-04T23:02:25Z","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.rc1.350.ge57e33dbd1-goog\n\n"},{"id":"357546","messageId":"xmqqh8j236wj.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-2-sbeller@google.com","subject":"Re: [PATCH 01/11] string_list: print_string_list to use trace_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T16:52:28Z","receivedAt":"2018-09-06T16:52:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It is a debugging aid, so it should print to the debugging channel.\n\n... and rename it with trace_ prefix.\n\nUse of trace_printf() is nice, as we can control its behavior at\nruntime ;-)\n\n> Signed-off-by: Stefan Beller \n> ---\n> string-list.c | 6 +++---\n> string-list.h | 4 ++--\n> 2 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/string-list.c b/string-list.c\n> index 771c4550980..1ebbe1f56ea 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -196,13 +196,13 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n> }\n> \n> \n> -void print_string_list(const struct string_list *p, const char *text)\n> +void trace_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> diff --git a/string-list.h b/string-list.h\n> index ff8f6094a33..5b22560cf19 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -114,12 +114,12 @@ void filter_string_list(struct string_list *list, int free_util,\n> \t\t\tstring_list_each_func_t want, void *cb_data);\n> \n> /**\n> - * Dump a string_list to stdout, useful mainly for debugging\n> + * Dump a string_list using the trace_print, 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> -void print_string_list(const struct string_list *p, const char *text);\n> +void trace_print_string_list(const struct string_list *p, const char *text);\n> \n> /**\n> * Free a string_list. The `string` pointer of the items will be freed\n"},{"id":"357547","messageId":"20180906165602.GA19367@sigill.intra.peff.net","threadId":"49278","inReplyTo":"xmqqh8j236wj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 01/11] string_list: print_string_list to use trace_printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-06T16:56:03Z","receivedAt":"2018-09-06T16:56:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 06, 2018 at 09:52:28AM -0700, Junio C Hamano wrote:\n\n> Stefan Beller writes:\n> \n> > It is a debugging aid, so it should print to the debugging channel.\n> \n> ... and rename it with trace_ prefix.\n> \n> Use of trace_printf() is nice, as we can control its behavior at\n> runtime ;-)\n\nYes, though...\n\n> > -void print_string_list(const struct string_list *p, const char *text)\n> > +void trace_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\nIt seems funny that we'd iterate through the list checking over and over\nwhether tracing is enabled.\n\nShould this do:\n\n if (!trace_want(&trace_default_key))\n\treturn;\n\nat the top? (Or possibly even take a trace key from the caller, so that\nit can use whatever context makes sense for this particular list?)\n\n-Peff\n"},{"id":"357549","messageId":"xmqqwory1rhx.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-3-sbeller@google.com","subject":"Re: [PATCH 02/11] string-list.h: add string_list_{pop, last} functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T17:10:34Z","receivedAt":"2018-09-06T17:10:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A string list can be used as a stack, but should we?\n\nverb missing. \"We can use a string list as ..., but should we?\" is\nreadable. \"A string list can be ..., but should it be?\" also is.\n\n> A later patch shows\n> how useful this will be.\n\nInstead of all of the above, how about being more direct, i.e. e.g.\n\n\tAdd a few functions to allow a string-list to be used as a\n\tstack:\n\n\t- string_list_last() lets a caller peek the string_list_item\n at the end of the string list. The caller needs to be\n aware that it is borrowing a pointer, which can become\n invalid if/when the string_list is resized.\n\n\t- string_list_pop() removes the string_list_item at the end\n of the string list.\n\n\tYou can use them in this pattern:\n\n\t\twhile (list.nr) {\n\t\t\tstruct string_list_item *item = string_list_last(&list);\n\n\t\t\twork_on(item);\n\t\t\tstring_list_pop(&list);\n\t\t}\n\nThen the readers would immediately have enough information to judge\nif the new API is useful without blindly trusting what you promise.\n\nTwo things to note in the above suggested rewrite are:\n\n - It is curious that _pop() is not accompanied by _push(). It\n probably is a good idea to add the third bullet point that says\n \"existing string_list_append() can be used in place of the\n missing string_list_push().\"\n\n - As it already lays out the memory ownership issue, it probably\n makes the description we see below unnecessary (besides, the code\n snippet in there were buggy in at least two counts).\n\n> In an earlier iteration of this patch it was suggested to return the last\n> ...\n> Also provide the function to access the last item in the list.\n>\n> Signed-off-by: Stefan Beller \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 1ebbe1f56ea..21559f222a7 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\nConceptually, this allows string_list_clear() to be implemented in\nterms of this function, i.e.\n\n\tstring_list_clear(struct string_list *list, int free_util)\n\t{\n\t\twhile (list->nr)\n\t\t\tstring_list_pop(list, free_util);\n\t\tfree(list->items);\n\t\tlist->items = NULL;\n\t\tlist->nr = list->alloc = 0;\n\t}\n\nIt is unfortunate that string_list has become such a kitchen-sink in\nthat a string list whose util field is used in such a way that\nstring_list_clear_func() needs to be used to clear the list cannot\nbe used as a stack. Ideally each of these \"features\" (like\n\"sorted/unsorted\", \"owning/borrowing util\", \"owning/borrowing\nstring\", etc.) ought to be orthogonal, but they are not.\n\n> diff --git a/string-list.h b/string-list.h\n> index 5b22560cf19..d7cdf38e57a 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> +inline struct string_list_item *string_list_last(struct string_list *list)\n> +{\n> +\treturn &list->items[list->nr - 1];\n> +}\n\ninline functions declared in a header file must be marked \"static\ninline\", I think.\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":"357550","messageId":"xmqqsh2m1r1d.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-4-sbeller@google.com","subject":"Re: [PATCH 03/11] sha1-array: provide oid_array_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T17:20:30Z","receivedAt":"2018-09-06T17:20:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +/* Call fn for each oid, and retain it if fn returns 0, remove 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\nComparing this with object_array_filter(), which I think this was\nmodeled after, I notice that this is much harder to remember how to\nuse it correctly.\n\n void object_array_filter(struct object_array *array,\n object_array_each_func_t want, void *cb_data)\n\nIt is clear to a potential user of this function who needs to\nimplement the callback function what the function should return,\nbecause it calls it \"want\" (as opposed to \"fn\"). The function must\nanswer \"I want to keep this? Yes/no?\".\n\nAlso polarity of oid_array_filter()'s callback is different. It\nretains elements for which \"fn\" returns false.\n\nSo one step of improvement may be to rename \"fn\" to \"reject\", but as\nwe do not have any caller of this new function yet, perhaps match\nthe polarity and call it \"want\"?\n\nI think \"x_filter\" is perhaps trying to match other language's\nfilter(), e.g. Python's\n\n\t>>> filter(lambda x: x % 2 == 0, range(0,10))\n\t[0, 2, 4, 6, 8]\n\nso \"say yes if you want to keep it\" may be more familiar than \"say\nyes if you want to filter it out\".\n\n"},{"id":"357551","messageId":"xmqqmusu1qj9.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-5-sbeller@google.com","subject":"Re: [PATCH 04/11] submodule.c: convert submodule_move_head new argument to object id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T17:31:22Z","receivedAt":"2018-09-06T17:31:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller 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\nI would think this is a good change even without \"as it will turn\nout...\" which readers do not have enough information to judge for\nthemselves at this point.\n\n> diff --git a/submodule.c b/submodule.c\n> index 50cbf5f13ed..da2ed8696f5 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\nThe new calling convention feels somewhat uneven, though, that \"new\"\n(does it mean \"switching to this commit object\"?) takes an object\nid, but \"old\" (\"switching from this commit object\"?) still wants a\ntextual representation.\n\nSo, I no longer am sure that this is a good change by itself. It\nwould be a good change by itself if we deferred _both_ to keep them\nin sync (otherwise those who write (or read) callers will be forced\nto wonder which one takes the object id and which one takes the\ntextual representation and why they need to remember the\ndifference).\n\nPerhaps not all callers use oid_to_hex() to come up with old_head,\nand some may even use branch names, etc., which is passed through\nfrom this function to its callee, to convey more information than\nmere object names? If that is the case, then converting old_head to\nold_oid would be a bad move as it can lose information and we'd need\nto reconsider the strategy used here (e.g. perhaps we may be better\noff sending both textual name and object id down the callchain, and\na caller without a meaningful textual name may indicate that by\npassing NULL, or something like that).\n\n> @@ -1865,8 +1863,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"},{"id":"357552","messageId":"xmqqin3i1qes.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-6-sbeller@google.com","subject":"Re: [PATCH 05/11] submodule.c: fix indentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T17:34:03Z","receivedAt":"2018-09-06T17:34:07Z","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>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index da2ed8696f5..8345d423fda 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\nBesides indentation, it is good to avoid A = B = C assignment.\nWritten this way, it is more obvious that these two fields share the\nsame pointer.\n\nGood.\n\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\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\nwould be easier to see what is going on.\n"},{"id":"357554","messageId":"xmqqbm9a1p1p.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-7-sbeller@google.com","subject":"Re: [PATCH 06/11] submodule.c: sort changed_submodule_names before searching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T18:03:30Z","receivedAt":"2018-09-06T18:03:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Instead of sorting it after we created an unsorted list, we could insert\n> correctly into the list.\n\nIt is unclear what problem you are solving, especially with\nsubjunctive \"could\" there. We are creating an unsorted list and\nthen sorting it and you see it as a problem because it is just as\neasy and efficient to do the insertion sort while building up the\nlist? (don't react and answer without reading all the way to the\nend; I think I know what is going on).\n\n> As the unsorted append is in order of cache entry\n> names, this is already sorted if names were equal to paths for submodules.\n\nThat may be a statement of a fact, but it is unclear how that fact\nrelates to what the code is doing before this patch, or what the\ncode updated by this patch is doing.\n\n> As submodule names are often the same as their path, the input is sorted\n> pretty well already, so let's just do the sort afterwards.\n\nIt is unclear what (performance?) trade-off this senttence is trying\nto make. It sounds as if it is claiming this:\n\n\tWe can string_list_insert() to maintain sorted-ness of the\n\tlist as we find new items, or we can string_list_append() to\n\tbuild an unsorted list and sort it at the end just once.\n\n\tTo pick which one is more appropriate, we notice the fact\n\tthat we discover new items more or less in the already\n\tsorted order. That makes \"append then sort\" more\n\tappropriate.\n\nBut is that reasoning sensible? \n\nI'd imagine that append-then-sort would always be more efficient\nthan insert-at-the-right-place-as-we-go, and the reason why we\nsometimes would need to do the latter is when we need to look up\nelements in the middle of building the list (e.g. we may want to\nde-dup, which requires us to look up a name from the ones we\ncollected so far).\n\nAnd in this application, calculate_changed_submodule_paths()\ndiscover paths by calling collect_changed_submodules() which finds a\nmapping into a list sorted by\nsubmodule name, and then goes through that list and builds a list of\nsubmodules paths (which could be different from submodule names) by\nappending. Only after this list is fully built, get_next_submodule()\ngets called, so making the latter use string_list_lookup() that assumes\na sorted list is safe if we built the list by append-then-sort (iow,\nsortedness while building the list does not matter).\n\nHaving analysed all that, I find it somewhat iffy that _append() is\nused there in the calculate_changed_submodule_paths() function. It\nwould cause the resulting changed_submodule_names list to hold the\nsame name twice (or more), but I do not know if that would pose a\nproblem to the consumer of the list. Using \"accumulate then sort\nbefore calling look-up\" would not change it as string_list_sort()\nwould not dedup, so I do not think this patch would introduce a new\nproblem, though.\n\n\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 8345d423fda..8bee455137a 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"},{"id":"357555","messageId":"xmqq7ejy1p00.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-8-sbeller@google.com","subject":"Re: [PATCH 07/11] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T18:04:31Z","receivedAt":"2018-09-06T18:04:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The `changed_submodule_names` are only used for fetching, so let's make it\n> part of the struct that is passed around for fetching submodules.\n\nYay.\n"},{"id":"357557","messageId":"xmqq36um1o8z.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180904230149.180332-9-sbeller@google.com","subject":"Re: [PATCH 08/11] submodule.c: do not copy around submodule list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T18:20:44Z","receivedAt":"2018-09-06T18:20:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\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\nThat may describe what the patch does, but does not explain why we\nwould even want to do so in the first place. \n\nIt may be a safe thing to do to munge the list in place as there is\nnobody that looks at the list after we are done in the current code,\nbut the above description does not tell that to readers and fails to\ngive readers warm and fuzzy feeling that the safety likely will stay\nwith us in the future.\n\n> As a side effect, we'll have access to the util pointer for longer that\n> contains the commits that we need to fetch.\n\nIf this patch does not move free-submodules-oids call to out: label\n(i.e. instead do so after the call to string-list-remove-empty-items\nis made, perhaps), then the list of oids will have the same lifespan\nas the original code, no? Then it is not a \"side effect\" but is\ndeliberate change of behaviour this patch makes. We can access the\nlist for longer time than before, which may be a good thing, in\nwhich case we'd want to explain the potential benefit we could reap\nwith future changes, no?\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 19 ++++++++++---------\n> 1 file changed, 10 insertions(+), 9 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 582c0263b91..0efe6711a8c 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"},{"id":"357588","messageId":"CAGZ79kbRyVZ_1QpX0pPigN+2Pt2dV1eYvLyoWeSWejbLxko4YQ@mail.gmail.com","threadId":"49278","inReplyTo":"20180906165602.GA19367@sigill.intra.peff.net","subject":"Re: [PATCH 01/11] string_list: print_string_list to use trace_printf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-06T22:16:21Z","receivedAt":"2018-09-06T22:16:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 6, 2018 at 9:56 AM Jeff King wrote:\n>\n> On Thu, Sep 06, 2018 at 09:52:28AM -0700, Junio C Hamano wrote:\n>\n> > Stefan Beller writes:\n> >\n> > > It is a debugging aid, so it should print to the debugging channel.\n> >\n> > ... and rename it with trace_ prefix.\n> >\n> > Use of trace_printf() is nice, as we can control its behavior at\n> > runtime ;-)\n>\n> Yes, though...\n>\n> > > -void print_string_list(const struct string_list *p, const char *text)\n> > > +void trace_print_string_list(const struct string_list *p, const char *text)\n> > > {\n> > > int i;\n> > > if ( text )\n> > > - printf(\"%s\\n\", text);\n> > > + trace_printf(\"%s\\n\", text);\n> > > for (i = 0; i < p->nr; i++)\n> > > - printf(\"%s:%p\\n\", p->items[i].string, p->items[i].util);\n> > > + trace_printf(\"%s:%p\\n\", p->items[i].string, p->items[i].util);\n> > > }\n>\n> It seems funny that we'd iterate through the list checking over and over\n> whether tracing is enabled.\n>\n> Should this do:\n>\n> if (!trace_want(&trace_default_key))\n> return;\n>\n> at the top? (Or possibly even take a trace key from the caller, so that\n> it can use whatever context makes sense for this particular list?)\n\nI added this check as well as rewording the commit message\nto recite Junios understanding of the patch as well.\n\nHowever I would want to not derail this patch any further.\nThis function was used as an aid by me some time ago, so I\nam willing to share the modifications needed for efficient\nprintf debugging here, but I do not want to be dragged into\na rabbit hole.\n\nFor example taking the trace key is much overkill IMHO\nfor a pure debugging aid (and so is the shortcutting return\nthat you proposed, but I added that already for the resend),\nso if anyone needs this function outside of printf-debugging,\nI would recommend patches on top.\n\nThanks,\nStefan\n"},{"id":"357589","messageId":"CAGZ79kYDxC9krHiwHRWGpAfAys-NaQpS5py6-A=rurm96iN9=A@mail.gmail.com","threadId":"49278","inReplyTo":"xmqqwory1rhx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 02/11] string-list.h: add string_list_{pop, last} functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-06T22:29:47Z","receivedAt":"2018-09-06T22:30:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> > A later patch shows\n> > how useful this will be.\n>\n> Instead of all of the above, how about being more direct, i.e. e.g.\n\n\nok.\n\n\n> Conceptually, this allows string_list_clear() to be implemented in\n> terms of this function, i.e.\n>\n> string_list_clear(struct string_list *list, int free_util)\n> {\n> while (list->nr)\n> string_list_pop(list, free_util);\n> free(list->items);\n> list->items = NULL;\n> list->nr = list->alloc = 0;\n> }\n>\n> It is unfortunate that string_list has become such a kitchen-sink in\n> that a string list whose util field is used in such a way that\n> string_list_clear_func() needs to be used to clear the list cannot\n> be used as a stack. Ideally each of these \"features\" (like\n> \"sorted/unsorted\", \"owning/borrowing util\", \"owning/borrowing\n> string\", etc.) ought to be orthogonal, but they are not.\n\nOh, another neat string list feature I overlooked. :/\n"},{"id":"357593","messageId":"20180907000423.GA16764@sigill.intra.peff.net","threadId":"49278","inReplyTo":"CAGZ79kbRyVZ_1QpX0pPigN+2Pt2dV1eYvLyoWeSWejbLxko4YQ@mail.gmail.com","subject":"Re: [PATCH 01/11] string_list: print_string_list to use trace_printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-07T00:04:24Z","receivedAt":"2018-09-07T00:04:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 06, 2018 at 03:16:21PM -0700, Stefan Beller wrote:\n\n> > It seems funny that we'd iterate through the list checking over and over\n> > whether tracing is enabled.\n> >\n> > Should this do:\n> >\n> > if (!trace_want(&trace_default_key))\n> > return;\n> >\n> > at the top? (Or possibly even take a trace key from the caller, so that\n> > it can use whatever context makes sense for this particular list?)\n> \n> I added this check as well as rewording the commit message\n> to recite Junios understanding of the patch as well.\n> \n> However I would want to not derail this patch any further.\n> This function was used as an aid by me some time ago, so I\n> am willing to share the modifications needed for efficient\n> printf debugging here, but I do not want to be dragged into\n> a rabbit hole.\n> \n> For example taking the trace key is much overkill IMHO\n> for a pure debugging aid (and so is the shortcutting return\n> that you proposed, but I added that already for the resend),\n> so if anyone needs this function outside of printf-debugging,\n> I would recommend patches on top.\n\nI guess the question is: is this a thing we would want to make available\nto code to leave in all the time? Or is it just for sticking in\ntemporarily for a quick dump?\n\nIf the former, then I think it needs the early-return at the least (and\nprobably _should_ have the key parameter).\n\nBut I get the feeling that you really just want the latter, and are only\ngrudgingly being pushed into the former by Junio's suggestion.\n\n-Peff\n"},{"id":"357616","messageId":"xmqqo9d9y6oy.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180907000423.GA16764@sigill.intra.peff.net","subject":"Re: [PATCH 01/11] string_list: print_string_list to use trace_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-07T09:53:33Z","receivedAt":"2018-09-07T09:53:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guess the question is: is this a thing we would want to make available\n> to code to leave in all the time? Or is it just for sticking in\n> temporarily for a quick dump?\n>\n> If the former, then I think it needs the early-return at the least (and\n> probably _should_ have the key parameter).\n>\n> But I get the feeling that you really just want the latter, and are only\n> grudgingly being pushed into the former by Junio's suggestion.\n\nWell, if that is the case, I'd change \"my suggestion\" (although I\ndidn't mean to suggest anything concrete). If this was needed\nand/or was useful only during the defvelopment of the remainder of\nthe series, and if this is known to be a half-hearted change that is\nnot meant to be useful as a general solution, then let's not take\nthe main part of the series a hostage to this step; rather, we\nshould drop this change and leave it to another series that is\nwilling to do it right. It would save both author's and reviewers'\ntime if we did so.\n\n"},{"id":"357636","messageId":"CAGZ79kbObDXyaRLADQpvMMSitOLCDwb030Q6UBiX-7Y-XWnqog@mail.gmail.com","threadId":"49278","inReplyTo":"xmqqo9d9y6oy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 01/11] string_list: print_string_list to use trace_printf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-07T17:21:57Z","receivedAt":"2018-09-07T17:22:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 7, 2018 at 2:53 AM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > I guess the question is: is this a thing we would want to make available\n> > to code to leave in all the time? Or is it just for sticking in\n> > temporarily for a quick dump?\n> >\n> > If the former, then I think it needs the early-return at the least (and\n> > probably _should_ have the key parameter).\n> >\n> > But I get the feeling that you really just want the latter, and are only\n> > grudgingly being pushed into the former by Junio's suggestion.\n\nThat is not quite the case. I did take all but the one suggestion\nas I think they are good suggestions and I appreciate it.\n\nHowever having the trace key is not useful I would think.\n(I did not need it for debugging, but presented an API\nof what I would have found useful)\n\nIf the key is really that useful, I would just think we can add it on top.\n\n> Well, if that is the case, I'd change \"my suggestion\" (although I\n> didn't mean to suggest anything concrete). If this was needed\n> and/or was useful only during the defvelopment of the remainder of\n> the series, and if this is known to be a half-hearted change that is\n> not meant to be useful as a general solution, then let's not take\n> the main part of the series a hostage to this step; rather, we\n> should drop this change and leave it to another series that is\n> willing to do it right. It would save both author's and reviewers'\n> time if we did so.\n\nIn that case, would we be interested in a separate patch\nthat kills off that function instead?\n\nStefan\n"},{"id":"357834","messageId":"20180910215831.17608-1-sbeller@google.com","threadId":"49278","inReplyTo":"CAGZ79kbObDXyaRLADQpvMMSitOLCDwb030Q6UBiX-7Y-XWnqog@mail.gmail.com","subject":"[PATCH 1/2] trace: add trace_print_string_list_key","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-10T21:58:30Z","receivedAt":"2018-09-10T21:58:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Similar to trace_strbuf or trace_argv_printf, we might want to output\na string list in tracing. Add such a function.\n\nSigned-off-by: Stefan Beller \n---\n\nI separated this from the other series, making it into 2 patches:\nThis first patch adds tracing for string lists and the next patch that\nremoves the unused function from the string list API.\nThat way we can decide on these two patches separately if needed.\n\n Documentation/technical/api-trace.txt | 8 ++++++\n trace.c | 39 +++++++++++++++++++++++++++\n trace.h | 16 +++++++++++\n 3 files changed, 63 insertions(+)\n\ndiff --git a/Documentation/technical/api-trace.txt b/Documentation/technical/api-trace.txt\nindex fadb5979c48..ad0ea99d930 100644\n--- a/Documentation/technical/api-trace.txt\n+++ b/Documentation/technical/api-trace.txt\n@@ -62,6 +62,14 @@ Functions\n \tPrints the strbuf, without additional formatting (i.e. doesn't\n \tchoke on `%` or even `\\0`).\n \n+`void trace_print_string_list_key(struct trace_key *key, const struct string_list *p, const char *text)::\n+\n+\tPrint the string-pointer pairs of the string_list,\n+\teach one in its own line.\n+\tIt takes an optional key and header argument.\n+\tIf the tracing key is not given, use the default key.\n+\tThe header text is printed before the list itself.\n+\n `uint64_t getnanotime(void)`::\n \n \tReturns nanoseconds since the epoch (01/01/1970), typically used\ndiff --git a/trace.c b/trace.c\nindex fc623e91fdd..e3a12a092f9 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -176,6 +176,38 @@ void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n \tstrbuf_release(&buf);\n }\n \n+void trace_print_string_list_key_fl(const char *file, int line,\n+\t\t\t\t struct trace_key *key,\n+\t\t\t\t const struct string_list *p,\n+\t\t\t\t const char *text)\n+{\n+\tint i, buf_prefix;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!key)\n+\t\tkey = &trace_default_key;\n+\n+\tif (!prepare_trace_line(file, line, key, &buf))\n+\t\treturn;\n+\n+\tbuf_prefix = buf.len;\n+\n+\tif (text) {\n+\t\tstrbuf_addstr(&buf, text);\n+\t\tprint_trace_line(key, &buf);\n+\t}\n+\n+\tfor (i = 0; i < p->nr; i++) {\n+\t\tstrbuf_setlen(&buf, buf_prefix);\n+\t\tstrbuf_addf(&buf, \"%s:%p\\n\",\n+\t\t\t p->items[i].string,\n+\t\t\t p->items[i].util);\n+\t\tprint_trace_line(key, &buf);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static void trace_performance_vprintf_fl(const char *file, int line,\n \t\t\t\t\t uint64_t nanos, const char *format,\n \t\t\t\t\t va_list ap)\n@@ -227,6 +259,13 @@ void trace_strbuf(struct trace_key *key, const struct strbuf *data)\n \ttrace_strbuf_fl(NULL, 0, key, data);\n }\n \n+void trace_print_string_list_key(struct trace_key *key,\n+\t\t\t\t const struct string_list *p,\n+\t\t\t\t const char *text)\n+{\n+\ttrace_print_string_list_key_fl(NULL, 0, key, p, list);\n+}\n+\n void trace_performance(uint64_t nanos, const char *format, ...)\n {\n \tva_list ap;\ndiff --git a/trace.h b/trace.h\nindex 2b6a1bc17c2..0e083891e1f 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -37,6 +37,10 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n \n extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);\n \n+extern void trace_print_string_list_key(struct trace_key *key,\n+\t\t\t\t\tconst struct string_list *p,\n+\t\t\t\t\tconst char *text);\n+\n /* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */\n __attribute__((format (printf, 2, 3)))\n extern void trace_performance(uint64_t nanos, const char *format, ...);\n@@ -103,6 +107,13 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n \t\t\ttrace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\\\n \t} while (0)\n \n+#define trace_print_string_list_key(key, list, text) \t\t\t \\\n+\tdo {\t\t\t\t\t\t\t\t \\\n+\t\tif (trace_pass_fl(key))\t\t\t\t\t \\\n+\t\t\ttrace_print_string_list_key_fl(TRACE_CONTEXT,\t \\\n+\t\t\t\t\t\t __LINE__, key, data);\\\n+\t} while (0)\n+\n #define trace_performance(nanos, ...)\t\t\t\t\t \\\n \tdo {\t\t\t\t\t\t\t\t \\\n \t\tif (trace_pass_fl(&trace_perf_key))\t\t\t \\\n@@ -127,6 +138,11 @@ extern void trace_argv_printf_fl(const char *file, int line, const char **argv,\n \t\t\t\t const char *format, ...);\n extern void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n \t\t\t const struct strbuf *data);\n+extern void trace_print_string_list_key_fl(const char *file, int line,\n+\t\t\t\t\t struct trace_key *key,\n+\t\t\t\t\t const struct string_list *p,\n+\t\t\t\t\t const char *text);\n+\n __attribute__((format (printf, 4, 5)))\n extern void trace_performance_fl(const char *file, int line,\n \t\t\t\t uint64_t nanos, const char *fmt, ...);\n-- \n2.19.0.rc2.392.g5ba43deb5a-goog\n\n"},{"id":"357835","messageId":"20180910215831.17608-2-sbeller@google.com","threadId":"49278","inReplyTo":"20180910215831.17608-1-sbeller@google.com","subject":"[PATCH 2/2] string-list: remove unused function print_string_list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-10T21:58:31Z","receivedAt":"2018-09-10T21:58:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n string-list.c | 10 ----------\n string-list.h | 8 --------\n 2 files changed, 18 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 771c4550980..1f6063f2a27 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -195,16 +195,6 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n \tlist->nr = list->alloc = 0;\n }\n \n-\n-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-\tfor (i = 0; i < p->nr; i++)\n-\t\tprintf(\"%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 \t\t\t\t\t\t char *string)\n {\ndiff --git a/string-list.h b/string-list.h\nindex ff8f6094a33..18c718c12ce 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -113,14 +113,6 @@ typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t want, void *cb_data);\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-void print_string_list(const struct string_list *p, const char *text);\n-\n /**\n * Free a string_list. The `string` pointer of the items will be freed\n * in case the `strdup_strings` member of the string_list is set. The\n-- \n2.19.0.rc2.392.g5ba43deb5a-goog\n\n"},{"id":"357839","messageId":"xmqq36uhrnjk.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180910215831.17608-1-sbeller@google.com","subject":"Re: [PATCH 1/2] trace: add trace_print_string_list_key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-10T22:32:47Z","receivedAt":"2018-09-10T22:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I separated this from the other series, making it into 2 patches:\n> This first patch adds tracing for string lists and the next patch that\n> removes the unused function from the string list API.\n> That way we can decide on these two patches separately if needed.\n\nOf course, even though these are 1/2 and 2/2, only one of them and\nnot both would apply.\n\nThanks for sticking to the topic. \n\nGiven how simple that \"dump them to standard output\" code is, I am\ninclined to say that anybody who needs to inspect the contents of\nstring list at various points in the code under development can\ncreate one from scratch even if we did not have this implementation,\nso perhaps 2/2 is a better choice between the two.\n\nIt is not costing us much to leave it in the code. It's not like\nthe function costed a lot of maintenance burden since it was added\nin 8fd2cb40 (\"Extract helper bits from c-merge-recursive work\",\n2006-07-25), so the alternative #3 might be to do nothing.\n\nI have no strong preference between #2 and #3. The benefit of #2\ncompared to #3 is that, if we remove it today, there will not be\nsomebody else in the future to come and propose removing the\notherwise unused function, which would cost us time to review and\ndiscuss, so unless somebody stops me, I am inclined to say we'd take\n2/2 ;-)\n\nThanks.\n\n\n\n\n"},{"id":"357840","messageId":"CAGZ79kY2XpB4nJOJc0LLueqsutpncgwdN8uC=Wj0Uxu9Yjgd8A@mail.gmail.com","threadId":"49278","inReplyTo":"xmqq36uhrnjk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] trace: add trace_print_string_list_key","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-10T22:38:49Z","receivedAt":"2018-09-10T22:39:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 10, 2018 at 3:32 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > I separated this from the other series, making it into 2 patches:\n> > This first patch adds tracing for string lists and the next patch that\n> > removes the unused function from the string list API.\n> > That way we can decide on these two patches separately if needed.\n>\n> Of course, even though these are 1/2 and 2/2, only one of them and\n> not both would apply.\n\nOr you could squash them once we reach consensus that both are good.\n\n> Thanks for sticking to the topic.\n>\n> Given how simple that \"dump them to standard output\" code is, I am\n> inclined to say that anybody who needs to inspect the contents of\n> string list at various points in the code under development can\n> create one from scratch even if we did not have this implementation,\n> so perhaps 2/2 is a better choice between the two.\n\nThis sounds like the consensus is not to take both but only 2/2,\nwhich I'd be happy with, too.\n\n> It is not costing us much to leave it in the code. It's not like\n> the function costed a lot of maintenance burden since it was added\n> in 8fd2cb40 (\"Extract helper bits from c-merge-recursive work\",\n> 2006-07-25), so the alternative #3 might be to do nothing.\n\nTrue, but ...\n\n> somebody else in the future to propose removing\n\nis what is actually happening here already, see\n\nhttps://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n\n> I am inclined to say we'd take\n> 2/2 ;-)\n>\n> Thanks.\n\nFeel free to take Alexanders patch from 2015 instead.\n\nThanks,\nStefan\n"},{"id":"357842","messageId":"xmqqy3c8rh3d.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"CAGZ79kY2XpB4nJOJc0LLueqsutpncgwdN8uC=Wj0Uxu9Yjgd8A@mail.gmail.com","subject":"Re: [PATCH 1/2] trace: add trace_print_string_list_key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T00:52:06Z","receivedAt":"2018-09-11T00:52:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Of course, even though these are 1/2 and 2/2, only one of them and\n>> not both would apply.\n>\n> Or you could squash them once we reach consensus that both are good.\n\nAh, sorry, I completely misread the first one. I thought that it\nwas extending the implementation of existing unused function by\nusing trace API, which explains why I mistook them as an either-or\nchoice. I did not realize 1/2 was adding yet another unused one\nwithout doing anything to the existing unused one.\n\nSo the choice being offered are:\n\n (0) take 2/2 only, keeping zero unused helper.\n (1) take 1/2 only, keeping two unused helpers.\n (2) do nothing, keeping the simple unused helper we had from the\n beginning of time.\n (3) take 1/2 and 2/2, replacing one simple unused helper with\n another unused helper that is more complex and capable.\n\nAre you planning to, or do you know somebody who plans to, use one\nor the other if available in a near future? If so, it would be OK\nto take choice (2) or (3), and it probably is preferrable to take\n(3) between them. A more complex and capable one would require\nmaintenance over time (trace API is being updated with the trace2 in\nanother topic that will start flying soon, so it would be expected a\nuser of trace API may need update), but as long as that is actually\nused and help developers, that maintenance cost is worth paying.\n\nIf not, I would say that the option (1) or (3) are worse choices\nthan either (0) or (2). It would be better to minimize maintenance\ncost for unused helper(s), and the simpler one is likely to stay\nmaintenance free for longer than the more complex and capable one,\nso (1) and (3) do not make much sense unless we plan to start using\nthese real soon.\n\n>> It is not costing us much to leave it in the code. It's not like\n>> the function costed a lot of maintenance burden since it was added\n>> in 8fd2cb40 (\"Extract helper bits from c-merge-recursive work\",\n>> 2006-07-25), so the alternative #3 might be to do nothing.\n>\n> True, but ...\n>\n>> somebody else in the future to propose removing\n>\n> is what is actually happening here already, see\n>\n> https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n>\n>> I am inclined to say we'd take\n>> 2/2 ;-)\n>>\n>> Thanks.\n>\n> Feel free to take Alexanders patch from 2015 instead.\n\nI prefer to take 2/2 over the one from 2015, especially if we can\nexplain the removal better. We had three extra years that the\nhelper stayed unused and unchanged, which gives us a better\nindication that it won't be missed.\n\nThanks.\n"},{"id":"357847","messageId":"CAGZ79kbRrPnQnPs0FX7_TeiekpmmLzDoSsQpi2fDjc-WkqBf_A@mail.gmail.com","threadId":"49278","inReplyTo":"xmqqy3c8rh3d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] trace: add trace_print_string_list_key","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T03:08:35Z","receivedAt":"2018-09-11T03:08:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 10, 2018 at 5:52 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> >> Of course, even though these are 1/2 and 2/2, only one of them and\n> >> not both would apply.\n> >\n> > Or you could squash them once we reach consensus that both are good.\n>\n> Ah, sorry, I completely misread the first one. I thought that it\n> was extending the implementation of existing unused function by\n> using trace API, which explains why I mistook them as an either-or\n> choice. I did not realize 1/2 was adding yet another unused one\n> without doing anything to the existing unused one.\n>\n> So the choice being offered are:\n>\n> (0) take 2/2 only, keeping zero unused helper.\n> (1) take 1/2 only, keeping two unused helpers.\n> (2) do nothing, keeping the simple unused helper we had from the\n> beginning of time.\n> (3) take 1/2 and 2/2, replacing one simple unused helper with\n> another unused helper that is more complex and capable.\n>\n> Are you planning to, or do you know somebody who plans to, use one\n> or the other if available in a near future? If so, it would be OK\n> to take choice (2) or (3), and it probably is preferrable to take\n> (3) between them. A more complex and capable one would require\n> maintenance over time (trace API is being updated with the trace2 in\n> another topic that will start flying soon, so it would be expected a\n> user of trace API may need update), but as long as that is actually\n> used and help developers, that maintenance cost is worth paying.\n>\n> If not, I would say that the option (1) or (3) are worse choices\n> than either (0) or (2). It would be better to minimize maintenance\n> cost for unused helper(s), and the simpler one is likely to stay\n> maintenance free for longer than the more complex and capable one,\n> so (1) and (3) do not make much sense unless we plan to start using\n> these real soon.\n\nYes, I think (0) is the way to go, actually.\n\nI wrote patch 1/2 to show Peff and you to prove otherwise that I am\nnot contributing \"only grudgingly\".\n\nIf the current unused function would be actually helpful in debugging\nI would not remove it, but actually use it.\n\n>\n> >> It is not costing us much to leave it in the code. It's not like\n> >> the function costed a lot of maintenance burden since it was added\n> >> in 8fd2cb40 (\"Extract helper bits from c-merge-recursive work\",\n> >> 2006-07-25), so the alternative #3 might be to do nothing.\n> >\n> > True, but ...\n> >\n> >> somebody else in the future to propose removing\n> >\n> > is what is actually happening here already, see\n> >\n> > https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n> >\n> >> I am inclined to say we'd take\n> >> 2/2 ;-)\n> >>\n> >> Thanks.\n> >\n> > Feel free to take Alexanders patch from 2015 instead.\n>\n> I prefer to take 2/2 over the one from 2015, especially if we can\n> explain the removal better. We had three extra years that the\n> helper stayed unused and unchanged, which gives us a better\n> indication that it won't be missed.\n\nWill send a patch with better reasons tomorrow,\n\nThanks,\nStefan\n"},{"id":"357889","messageId":"CAGZ79kZ4UtcojkDGjAbxORV6+dfSNmQYOx_Loe=B=-Z3JrQ5dg@mail.gmail.com","threadId":"49278","inReplyTo":"xmqqbm9a1p1p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 06/11] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T18:31:55Z","receivedAt":"2018-09-11T18:32:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 6, 2018 at 11:03 AM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > Instead of sorting it after we created an unsorted list, we could insert\n> > correctly into the list.\n>\n> It is unclear what problem you are solving, especially with\n> subjunctive \"could\" there. We are creating an unsorted list and\n> then sorting it and you see it as a problem because it is just as\n> easy and efficient to do the insertion sort while building up the\n> list? (don't react and answer without reading all the way to the\n> end; I think I know what is going on).\n>\n> > As the unsorted append is in order of cache entry\n> > names, this is already sorted if names were equal to paths for submodules.\n>\n> That may be a statement of a fact, but it is unclear how that fact\n> relates to what the code is doing before this patch, or what the\n> code updated by this patch is doing.\n>\n> > As submodule names are often the same as their path, the input is sorted\n> > pretty well already, so let's just do the sort afterwards.\n>\n> It is unclear what (performance?) trade-off this senttence is trying\n> to make. It sounds as if it is claiming this:\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> But is that reasoning sensible?\n>\n> I'd imagine that append-then-sort would always be more efficient\n> than insert-at-the-right-place-as-we-go, and the reason why we\n> sometimes would need to do the latter is when we need to look up\n> elements in the middle of building the list (e.g. we may want to\n> de-dup, which requires us to look up a name from the ones we\n> collected so far).\n\nIf we come across a (mostly) sorted list, then the insert-at-the-right-place\nwe'd come across the best case of insertion sort which is O(n), which\nsounds better than append-then-sort as our sorting is a merge sort,\nwhich has O(n log n) even in its best case (and needs to copy stuff\ninto a temp buffer and back).\n\nBy having the submodules named after its path, I strongly suspect\nwe have a mostly sorted list in nearly all cases except some really\ninteresting corner cases out there.\n\n> And in this application, calculate_changed_submodule_paths()\n> discover paths by calling collect_changed_submodules() which finds a\n> mapping into a list sorted by\n> submodule name, and then goes through that list and builds a list of\n> submodules paths (which could be different from submodule names) by\n> appending. Only after this list is fully built, get_next_submodule()\n> gets called, so making the latter use string_list_lookup() that assumes\n> a sorted list is safe if we built the list by append-then-sort (iow,\n> sortedness while building the list does not matter).\n>\n> Having analysed all that, I find it somewhat iffy that _append() is\n> used there in the calculate_changed_submodule_paths() function.\n\nNote that this is fixed in the later patch\n\"submodule.c: do not copy around submodule list\"\n\n> It\n> would cause the resulting changed_submodule_names list to hold the\n> same name twice (or more),\n\nThis would be possible if there is a submodule at path A and another\nsubmodule (at a different path) named \"A\", as we'll try hard to collect\nnames, but are also okay with path as we want to keep supporting the\nhistorical use case of submodules.\n\n> but I do not know if that would pose a\n> problem to the consumer of the list. Using \"accumulate then sort\n> before calling look-up\" would not change it as string_list_sort()\n> would not dedup, so I do not think this patch would introduce a new\n> problem, though.\n\nYes, that is true, so we'd want to extend the message above to\nmention the potential duplicates.\n\nThanks,\nStefan\n"},{"id":"357891","messageId":"20180911184850.24891-1-sbeller@google.com","threadId":"49278","inReplyTo":"xmqqy3c8rh3d.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] string-list: remove unused function print_string_list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T18:48:50Z","receivedAt":"2018-09-11T18:48:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The helper function stayed unused for 3 years. A removal of that function\nwas proposed before[1], but now time has proven we really do not need the\nfunction.\n\n[1] https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\nSigned-off-by: Stefan Beller \n---\n string-list.c | 10 ----------\n string-list.h | 8 --------\n 2 files changed, 18 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 771c4550980..1f6063f2a27 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -195,16 +195,6 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n \tlist->nr = list->alloc = 0;\n }\n \n-\n-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-\tfor (i = 0; i < p->nr; i++)\n-\t\tprintf(\"%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 \t\t\t\t\t\t char *string)\n {\ndiff --git a/string-list.h b/string-list.h\nindex ff8f6094a33..18c718c12ce 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -113,14 +113,6 @@ typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t want, void *cb_data);\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-void print_string_list(const struct string_list *p, const char *text);\n-\n /**\n * Free a string_list. The `string` pointer of the items will be freed\n * in case the `strdup_strings` member of the string_list is set. The\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357897","messageId":"xmqq1s9zq1gm.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"20180911184850.24891-1-sbeller@google.com","subject":"Re: [PATCH] string-list: remove unused function print_string_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T19:27:21Z","receivedAt":"2018-09-11T19:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The helper function stayed unused for 3 years. A removal of that function\n\nI think it stayed unused for more than that before the previous\nproposal to remove it was written (I do not bother going back to my\nearlier message that identified which exact commit this was\nintroduced at). It has stayed that way for 3 more years since then.\n\n> was proposed before[1], but now time has proven we really do not need the\n> function.\n>\n> [1] https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n> Signed-off-by: Stefan Beller \n\nI'll add a blank line before the sign-off. Is this an example that\nour \"where is the existing trailer?\" code misbehaving?\n\n> ---\n> string-list.c | 10 ----------\n> string-list.h | 8 --------\n> 2 files changed, 18 deletions(-)\n>\n> diff --git a/string-list.c b/string-list.c\n> index 771c4550980..1f6063f2a27 100644\n> --- a/string-list.c\n> +++ b/string-list.c\n> @@ -195,16 +195,6 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n> \tlist->nr = list->alloc = 0;\n> }\n> \n> -\n> -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> -\tfor (i = 0; i < p->nr; i++)\n> -\t\tprintf(\"%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> \t\t\t\t\t\t char *string)\n> {\n> diff --git a/string-list.h b/string-list.h\n> index ff8f6094a33..18c718c12ce 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -113,14 +113,6 @@ typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n> void filter_string_list(struct string_list *list, int free_util,\n> \t\t\tstring_list_each_func_t want, void *cb_data);\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> -void print_string_list(const struct string_list *p, const char *text);\n> -\n> /**\n> * Free a string_list. The `string` pointer of the items will be freed\n> * in case the `strdup_strings` member of the string_list is set. The\n"},{"id":"357898","messageId":"xmqqworromq4.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"xmqq1s9zq1gm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] string-list: remove unused function print_string_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T19:30:59Z","receivedAt":"2018-09-11T19:31:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> The helper function stayed unused for 3 years. A removal of that function\n>\n> I think it stayed unused for more than that before the previous\n> proposal to remove it was written (I do not bother going back to my\n> earlier message that identified which exact commit this was\n> introduced at). It has stayed that way for 3 more years since then.\n>\n>> was proposed before[1], but now time has proven we really do not need the\n>> function.\n>>\n>> [1] https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n>> Signed-off-by: Stefan Beller \n>\n> I'll add a blank line before the sign-off. Is this an example that\n> our \"where is the existing trailer?\" code misbehaving?\n\nI am still curious about this one.\n\nIn any case, I've reworded the above around \"3 years\".\n\n string-list: remove unused function print_string_list\n \n A removal of this helper function was proposed 3 years ago [1]; the\n function was never used since it was introduced in 2006 back then,\n and there is no new callers since. Now time has proven we really do\n not need the function.\n \n [1] https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n \n Signed-off-by: Stefan Beller \n Signed-off-by: Junio C Hamano \n\nThanks.\n"},{"id":"357899","messageId":"CAGZ79kZZwH1sORNsBZORF_c5V0U2NVLQa9JBY-Z5ksRS3yBT7w@mail.gmail.com","threadId":"49278","inReplyTo":"xmqqworromq4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] string-list: remove unused function print_string_list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T19:47:11Z","receivedAt":"2018-09-11T19:47:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 11, 2018 at 12:31 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Stefan Beller writes:\n> >\n> >> The helper function stayed unused for 3 years. A removal of that function\n> >\n> > I think it stayed unused for more than that before the previous\n> > proposal to remove it was written (I do not bother going back to my\n> > earlier message that identified which exact commit this was\n> > introduced at). It has stayed that way for 3 more years since then.\n> >\n> >> was proposed before[1], but now time has proven we really do not need the\n> >> function.\n> >>\n> >> [1] https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n> >> Signed-off-by: Stefan Beller \n> >\n> > I'll add a blank line before the sign-off. Is this an example that\n> > our \"where is the existing trailer?\" code misbehaving?\n>\n> I am still curious about this one.\n\nNo, it's me who is misbehaving. ;-)\n\nWhile I do have format.signoff set, such that I never forget to sign off on\na patch, I have the habit to sign it off manually, in git-gui while editing the\ncommit message so I can see the whole message.\n\n> In any case, I've reworded the above around \"3 years\".\n>\n> string-list: remove unused function print_string_list\n>\n> A removal of this helper function was proposed 3 years ago [1]; the\n> function was never used since it was introduced in 2006 back then,\n> and there is no new callers since. Now time has proven we really do\n> not need the function.\n>\n> [1] https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n>\n> Thanks.\n\nThanks for rewording it.\nStefan\n"},{"id":"357913","messageId":"xmqq8t47oiwn.fsf@gitster-ct.c.googlers.com","threadId":"49278","inReplyTo":"CAGZ79kZZwH1sORNsBZORF_c5V0U2NVLQa9JBY-Z5ksRS3yBT7w@mail.gmail.com","subject":"Re: [PATCH] string-list: remove unused function print_string_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T20:53:28Z","receivedAt":"2018-09-11T20:53:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> >>\n>> >> [1] https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/\n>> >> Signed-off-by: Stefan Beller \n>> >\n>> > I'll add a blank line before the sign-off. Is this an example that\n>> > our \"where is the existing trailer?\" code misbehaving?\n>>\n>> I am still curious about this one.\n>\n> No, it's me who is misbehaving. ;-)\n\nWhew. That lets me worried about one fewer things ;-) Thanks.\n\n"},{"id":"357916","messageId":"20180911210330.GA3825@sigill.intra.peff.net","threadId":"49278","inReplyTo":"CAGZ79kbRrPnQnPs0FX7_TeiekpmmLzDoSsQpi2fDjc-WkqBf_A@mail.gmail.com","subject":"Re: [PATCH 1/2] trace: add trace_print_string_list_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-11T21:03:30Z","receivedAt":"2018-09-11T21:03:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 08:08:35PM -0700, Stefan Beller wrote:\n\n> > So the choice being offered are:\n> >\n> > (0) take 2/2 only, keeping zero unused helper.\n> > (1) take 1/2 only, keeping two unused helpers.\n> > (2) do nothing, keeping the simple unused helper we had from the\n> > beginning of time.\n> > (3) take 1/2 and 2/2, replacing one simple unused helper with\n> > another unused helper that is more complex and capable.\n> [...]\n> \n> Yes, I think (0) is the way to go, actually.\n> \n> I wrote patch 1/2 to show Peff and you to prove otherwise that I am\n> not contributing \"only grudgingly\".\n\nI am perfectly happy with 0. But for reference, your new trace helper\nlooks completely reasonable from a quick view. Perhaps we can let it\nlive on in the list archive, and somebody may find a good use for it in\nthe future (though there is a significant chance that they would not\nthink to search the archive -- it could even be of value to commit and\nrevert it so that they find it in \"git log\", but that may be getting\npretty hypothetical).\n\n-Peff\n"}]}