{"thread":{"id":"46976","subject":"[PATCH v4 0/3] implement fetching of moved submodules","startedAt":"2017-10-16T13:56:38Z","lastAt":"2017-10-24T00:54:39Z","messageCount":24,"participants":["Heiko Voigt","Junio C Hamano","Stefan Beller","Brandon Williams"],"isPatch":true,"patchVersion":4,"patchTotal":3},"messages":[{"id":"330455","messageId":"20171016135623.GA12756@book.hvoigt.net","threadId":"46976","inReplyTo":null,"subject":"[PATCH v4 0/3] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-16T13:56:23Z","receivedAt":"2017-10-16T13:56:38Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"The previous RFC iteration can be found here:\n\nhttps://public-inbox.org/git/20171006222544.GA26642@sandbox/\n\nThis should now be in a state ready for review for inclusion.\n\nThe main difference from last iteration is that we now also support\nunconfigured gitlinks for push and fetch for backwards compatibility.\n\nTo implement this compatibility we construct a default name for gitlinks\nif there is a repository found at their location in the worktree.\n\nCheers Heiko\n\nHeiko Voigt (3):\n fetch: add test to make sure we stay backwards compatible\n implement fetching of moved submodules\n submodule: simplify decision tree whether to or not to fetch\n\n submodule-config.h | 3 +\n submodule.c | 200 +++++++++++++++++++++++++++++---------------\n t/t5526-fetch-submodules.sh | 77 ++++++++++++++++-\n 3 files changed, 210 insertions(+), 70 deletions(-)\n\n-- \n2.14.1.145.gb3622a4\n\n"},{"id":"330456","messageId":"20171016135715.GB12756@book.hvoigt.net","threadId":"46976","inReplyTo":"20171016135623.GA12756@book.hvoigt.net","subject":"[PATCH v4 1/3] fetch: add test to make sure we stay backwards compatible","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-16T13:57:15Z","receivedAt":"2017-10-16T13:57:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"The current implementation of submodules supports on-demand fetch if\nthere is no .gitmodules entry for a submodule. Let's add a test to\ndocument this behavior.\n\nSigned-off-by: Heiko Voigt \n---\n t/t5526-fetch-submodules.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 42251f7f3a..43a22f680f 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -478,7 +478,47 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n \t! test -s actual.out &&\n-\ttest_i18ncmp expect.err actual.err\n+\ttest_i18ncmp expect.err actual.err &&\n+\t(\n+\t\tcd submodule &&\n+\t\tgit checkout -q master\n+\t)\n+'\n+\n+test_expect_success \"'fetch.recurseSubmodules=on-demand' works also without .gitmodule entry\" '\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules\n+\t) &&\n+\tadd_upstream_commit &&\n+\thead1=$(git rev-parse --short HEAD) &&\n+\tgit add submodule &&\n+\tgit rm .gitmodules &&\n+\tgit commit -m \"new submodule without .gitmodules\" &&\n+\tprintf \"\" >expect.out &&\n+\thead2=$(git rev-parse --short HEAD) &&\n+\techo \"From $pwd/.\" >expect.err.2 &&\n+\techo \" $head1..$head2 master -> origin/master\" >>expect.err.2 &&\n+\thead -3 expect.err >>expect.err.2 &&\n+\t(\n+\t\tcd downstream &&\n+\t\trm .gitmodules &&\n+\t\tgit config fetch.recurseSubmodules on-demand &&\n+\t\t# fake submodule configuration to avoid skipping submodule handling\n+\t\tgit config -f .gitmodules submodule.fake.path fake &&\n+\t\tgit config -f .gitmodules submodule.fake.url fakeurl &&\n+\t\tgit add .gitmodules &&\n+\t\tgit config --unset submodule.submodule.url &&\n+\t\tgit fetch >../actual.out 2>../actual.err &&\n+\t\t# cleanup\n+\t\tgit config --unset fetch.recurseSubmodules &&\n+\t\tgit reset --hard\n+\t) &&\n+\ttest_i18ncmp expect.out actual.out &&\n+\ttest_i18ncmp expect.err.2 actual.err &&\n+\tgit checkout HEAD^ -- .gitmodules &&\n+\tgit add .gitmodules &&\n+\tgit commit -m \"new submodule restored .gitmodules\"\n '\n \n test_expect_success 'fetching submodules respects parallel settings' '\n-- \n2.14.1.145.gb3622a4\n\n"},{"id":"330457","messageId":"20171016135905.GD12756@book.hvoigt.net","threadId":"46976","inReplyTo":"20171016135623.GA12756@book.hvoigt.net","subject":"[PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-16T13:59:05Z","receivedAt":"2017-10-16T13:59:19Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"To make extending this logic later easier.\n\nSigned-off-by: Heiko Voigt \n---\n submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 37 insertions(+), 37 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 71d1773e2e..82d206eb65 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {\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+\tif (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)\n+\t\treturn spf->command_line_option;\n+\n+\tif (submodule) {\n+\t\tchar *key;\n+\t\tconst char *value;\n+\n+\t\tint fetch_recurse = submodule->fetch_recurse;\n+\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n+\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n+\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n+\t\t}\n+\t\tfree(key);\n+\n+\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE)\n+\t\t\t/* local config overrules everything except commandline */\n+\t\t\treturn fetch_recurse;\n+\t}\n+\n+\treturn spf->default_option;\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@@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t}\n \t\t}\n \n-\t\tdefault_argv = \"yes\";\n-\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n-\t\t\tint fetch_recurse = RECURSE_SUBMODULES_NONE;\n-\n-\t\t\tif (submodule) {\n-\t\t\t\tchar *key;\n-\t\t\t\tconst char *value;\n-\n-\t\t\t\tfetch_recurse = submodule->fetch_recurse;\n-\t\t\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n-\t\t\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n-\t\t\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n-\t\t\t\t}\n-\t\t\t\tfree(key);\n-\t\t\t}\n-\n-\t\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n-\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n-\t\t\t\t\t\t\t\t\t submodule->name))\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\tdefault_argv = \"on-demand\";\n-\t\t\t\t}\n-\t\t\t} else {\n-\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_OFF)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n-\t\t\t\t\t\t\t\t\t submodule->name))\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\tdefault_argv = \"on-demand\";\n-\t\t\t\t}\n-\t\t\t}\n-\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n+\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\t{\n+\t\tdefault:\n+\t\tcase RECURSE_SUBMODULES_DEFAULT:\n+\t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n+\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n \t\t\t\t\t\t\t submodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n+\t\t\tbreak;\n+\t\tcase RECURSE_SUBMODULES_ON:\n+\t\t\tdefault_argv = \"yes\";\n+\t\t\tbreak;\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\t\tcontinue;\n \t\t}\n \n \t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n-- \n2.14.1.145.gb3622a4\n\n"},{"id":"330458","messageId":"20171016135827.GC12756@book.hvoigt.net","threadId":"46976","inReplyTo":"20171016135623.GA12756@book.hvoigt.net","subject":"[PATCH v4 2/3] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-16T13:58:27Z","receivedAt":"2017-10-16T14:04:02Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"We store the changed submodules paths to calculate which submodule needs\nfetching. This does not work for moved submodules since their paths do\nnot stay the same in case of a moved submodules. In case of new\nsubmodules we do not have a path in the current checkout, since they\njust appeared in this fetch.\n\nIt is more general to collect the submodule names for changes instead of\ntheir paths to include the above cases. If we do not have a\nconfiguration for a gitlink we rely on constructing a default name from\nthe path if a git repository can be found at its path. We skip\nnon-configured gitlinks whose default name collides with a configured\none.\n\nWith the change described above we implement 'on-demand' fetching of\nchanges in moved submodules.\n\nSigned-off-by: Heiko Voigt \n---\n submodule-config.h | 3 +\n submodule.c | 138 ++++++++++++++++++++++++++++++++------------\n t/t5526-fetch-submodules.sh | 35 +++++++++++\n 3 files changed, 138 insertions(+), 38 deletions(-)\n\ndiff --git a/submodule-config.h b/submodule-config.h\nindex e3845831f6..a5503a5d17 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -22,6 +22,9 @@ struct submodule {\n \tint recommend_shallow;\n };\n \n+#define SUBMODULE_INIT { NULL, NULL, NULL, RECURSE_SUBMODULES_NONE, \\\n+\tNULL, NULL, SUBMODULE_UPDATE_STRATEGY_INIT, {0}, -1 };\n+\n struct submodule_cache;\n struct repository;\n \ndiff --git a/submodule.c b/submodule.c\nindex 63e7094e16..71d1773e2e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -21,7 +21,7 @@\n #include \"parse-options.h\"\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n-static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;\n+static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n@@ -674,11 +674,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)\n }\n \n static struct oid_array *submodule_commits(struct string_list *submodules,\n-\t\t\t\t\t const char *path)\n+\t\t\t\t\t const char *name)\n {\n \tstruct string_list_item *item;\n \n-\titem = string_list_insert(submodules, path);\n+\titem = string_list_insert(submodules, name);\n \tif (item->util)\n \t\treturn (struct oid_array *) item->util;\n \n@@ -687,39 +687,67 @@ static struct oid_array *submodule_commits(struct string_list *submodules,\n \treturn (struct oid_array *) item->util;\n }\n \n+struct collect_changed_submodules_cb_data {\n+\tstruct string_list *changed;\n+\tconst struct object_id *commit_oid;\n+};\n+\n+/*\n+ * this would normally be two functions: default_name_from_path() and\n+ * path_from_default_name(). Since the default name is the same as\n+ * the submodule path we can get away with just one function which only\n+ * checks whether there is a submodule in the working directory at that\n+ * location.\n+ */\n+static const char *default_name_or_path(const char *path_or_name)\n+{\n+\tint error_code;\n+\n+\tif (!is_submodule_populated_gently(path_or_name, &error_code))\n+\t\treturn NULL;\n+\n+\treturn path_or_name;\n+}\n+\n static void collect_changed_submodules_cb(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n {\n+\tstruct collect_changed_submodules_cb_data *me = data;\n+\tstruct string_list *changed = me->changed;\n+\tconst struct object_id *commit_oid = me->commit_oid;\n \tint i;\n-\tstruct string_list *changed = data;\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tstruct oid_array *commits;\n+\t\tconst struct submodule *submodule;\n+\t\tconst char *name;\n+\n \t\tif (!S_ISGITLINK(p->two->mode))\n \t\t\tcontinue;\n \n-\t\tif (S_ISGITLINK(p->one->mode)) {\n-\t\t\t/*\n-\t\t\t * NEEDSWORK: We should honor the name configured in\n-\t\t\t * the .gitmodules file of the commit we are examining\n-\t\t\t * here to be able to correctly follow submodules\n-\t\t\t * being moved around.\n-\t\t\t */\n-\t\t\tcommits = submodule_commits(changed, p->two->path);\n-\t\t\toid_array_append(commits, &p->two->oid);\n-\t\t} else {\n-\t\t\t/* Submodule is new or was moved here */\n-\t\t\t/*\n-\t\t\t * NEEDSWORK: When the .git directories of submodules\n-\t\t\t * live inside the superprojects .git directory some\n-\t\t\t * day we should fetch new submodules directly into\n-\t\t\t * that location too when config or options request\n-\t\t\t * that so they can be checked out from there.\n-\t\t\t */\n-\t\t\tcontinue;\n+\t\tsubmodule = submodule_from_path(commit_oid, p->two->path);\n+\t\tif (submodule)\n+\t\t\tname = submodule->name;\n+\t\telse {\n+\t\t\tname = default_name_or_path(p->two->path);\n+\t\t\t/* make sure name does not collide with existing one */\n+\t\t\tsubmodule = submodule_from_name(commit_oid, name);\n+\t\t\tif (submodule) {\n+\t\t\t\twarning(\"Submodule in commit %s at path: \"\n+\t\t\t\t\t\"'%s' collides with a submodule named \"\n+\t\t\t\t\t\"the same. Skipping it.\",\n+\t\t\t\t\toid_to_hex(commit_oid), name);\n+\t\t\t\tname = NULL;\n+\t\t\t}\n \t\t}\n+\n+\t\tif (!name)\n+\t\t\tcontinue;\n+\n+\t\tcommits = submodule_commits(changed, name);\n+\t\toid_array_append(commits, &p->two->oid);\n \t}\n }\n \n@@ -742,11 +770,14 @@ static void collect_changed_submodules(struct string_list *changed,\n \n \twhile ((commit = get_revision(&rev))) {\n \t\tstruct rev_info diff_rev;\n+\t\tstruct collect_changed_submodules_cb_data data;\n+\t\tdata.changed = changed;\n+\t\tdata.commit_oid = &commit->object.oid;\n \n \t\tinit_revisions(&diff_rev, NULL);\n \t\tdiff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \t\tdiff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n-\t\tdiff_rev.diffopt.format_callback_data = changed;\n+\t\tdiff_rev.diffopt.format_callback_data = &data;\n \t\tdiff_tree_combined_merge(commit, 1, &diff_rev);\n \t}\n \n@@ -894,7 +925,7 @@ int find_unpushed_submodules(struct oid_array *commits,\n \t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n \tstruct string_list submodules = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *submodule;\n+\tstruct string_list_item *name;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \n \t/* argv.argv[0] will be ignored by setup_revisions */\n@@ -905,9 +936,19 @@ int find_unpushed_submodules(struct oid_array *commits,\n \n \tcollect_changed_submodules(&submodules, &argv);\n \n-\tfor_each_string_list_item(submodule, &submodules) {\n-\t\tstruct oid_array *commits = submodule->util;\n-\t\tconst char *path = submodule->string;\n+\tfor_each_string_list_item(name, &submodules) {\n+\t\tstruct oid_array *commits = name->util;\n+\t\tconst struct submodule *submodule;\n+\t\tconst char *path = NULL;\n+\n+\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n+\t\tif (submodule)\n+\t\t\tpath = submodule->path;\n+\t\telse\n+\t\t\tpath = default_name_or_path(name->string);\n+\n+\t\tif (!path)\n+\t\t\tcontinue;\n \n \t\tif (submodule_needs_pushing(path, commits))\n \t\t\tstring_list_insert(needs_pushing, path);\n@@ -1065,7 +1106,7 @@ static void calculate_changed_submodule_paths(void)\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 *item;\n+\tconst struct string_list_item *name;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(NULL, NULL))\n@@ -1080,16 +1121,26 @@ static void calculate_changed_submodule_paths(void)\n \n \t/*\n \t * Collect all submodules (whether checked out or not) for which new\n-\t * commits have been recorded upstream in \"changed_submodule_paths\".\n+\t * commits have been recorded upstream in \"changed_submodule_names\".\n \t */\n \tcollect_changed_submodules(&changed_submodules, &argv);\n \n-\tfor_each_string_list_item(item, &changed_submodules) {\n-\t\tstruct oid_array *commits = item->util;\n-\t\tconst char *path = item->string;\n+\tfor_each_string_list_item(name, &changed_submodules) {\n+\t\tstruct oid_array *commits = name->util;\n+\t\tconst struct submodule *submodule;\n+\t\tconst char *path = NULL;\n+\n+\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n+\t\tif (submodule)\n+\t\t\tpath = submodule->path;\n+\t\telse\n+\t\t\tpath = default_name_or_path(name->string);\n+\n+\t\tif (!path)\n+\t\t\tcontinue;\n \n \t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&changed_submodule_paths, path);\n+\t\t\tstring_list_append(&changed_submodule_names, name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1149,11 +1200,19 @@ static int get_next_submodule(struct child_process *cp,\n \t\tconst struct cache_entry *ce = active_cache[spf->count];\n \t\tconst char *git_dir, *default_argv;\n \t\tconst struct submodule *submodule;\n+\t\tstruct submodule default_submodule = SUBMODULE_INIT;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n \t\tsubmodule = submodule_from_path(&null_oid, ce->name);\n+\t\tif (!submodule) {\n+\t\t\tconst char *name = default_name_or_path(ce->name);\n+\t\t\tif (name) {\n+\t\t\t\tdefault_submodule.path = default_submodule.name = name;\n+\t\t\t\tsubmodule = &default_submodule;\n+\t\t\t}\n+\t\t}\n \n \t\tdefault_argv = \"yes\";\n \t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n@@ -1175,7 +1234,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n \t\t\t\t\tcontinue;\n \t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n+\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n+\t\t\t\t\t\t\t\t\t submodule->name))\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n@@ -1183,13 +1243,15 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_OFF)\n \t\t\t\t\tcontinue;\n \t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n+\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n+\t\t\t\t\t\t\t\t\t submodule->name))\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n \t\t\t}\n \t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n+\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n+\t\t\t\t\t\t\t submodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t}\n@@ -1282,7 +1344,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n \targv_array_clear(&spf.args);\n out:\n-\tstring_list_clear(&changed_submodule_paths, 1);\n+\tstring_list_clear(&changed_submodule_names, 1);\n \treturn spf.result;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 43a22f680f..a552ad4ead 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -570,4 +570,39 @@ test_expect_success 'fetching submodule into a broken repository' '\n \ttest_must_fail git -C dst fetch --recurse-submodules\n '\n \n+test_expect_success \"fetch new commits when submodule got renamed\" '\n+\tgit clone . downstream_rename &&\n+\t(\n+\t\tcd downstream_rename &&\n+\t\tgit submodule update --init &&\n+# NEEDSWORK: we omitted --recursive for the submodule update here since\n+# that does not work. See test 7001 for mv \"moving nested submodules\"\n+# for details. Once that is fixed we should add the --recursive option\n+# here.\n+\t\tgit checkout -b rename &&\n+\t\tgit mv submodule submodule_renamed &&\n+\t\t(\n+\t\t\tcd submodule_renamed &&\n+\t\t\tgit checkout -b rename_sub &&\n+\t\t\techo a >a &&\n+\t\t\tgit add a &&\n+\t\t\tgit commit -ma &&\n+\t\t\tgit push origin rename_sub &&\n+\t\t\tgit rev-parse HEAD >../../expect\n+\t\t) &&\n+\t\tgit add submodule_renamed &&\n+\t\tgit commit -m \"update renamed submodule\" &&\n+\t\tgit push origin rename\n+\t) &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules=on-demand &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\tgit rev-parse origin/rename_sub >../../actual\n+\t\t)\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.14.1.145.gb3622a4\n\n"},{"id":"330502","messageId":"xmqq7evur1ab.fsf@gitster.mtv.corp.google.com","threadId":"46976","inReplyTo":"20171016135623.GA12756@book.hvoigt.net","subject":"Re: [PATCH v4 0/3] implement fetching of moved submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T01:49:48Z","receivedAt":"2017-10-17T01:49:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> The previous RFC iteration can be found here:\n>\n> https://public-inbox.org/git/20171006222544.GA26642@sandbox/\n>\n> This should now be in a state ready for review for inclusion.\n>\n> The main difference from last iteration is that we now also support\n> unconfigured gitlinks for push and fetch for backwards compatibility.\n>\n> To implement this compatibility we construct a default name for gitlinks\n> if there is a repository found at their location in the worktree.\n\nI do not remember the details of the patch in the previous round\nthat corresponds to PATCH 2/3 here, so I cannot comment on the\nincremental improvement between the two, but the fallback in 2/3\nlooks like a sensible thing to do.\n\nLet's see what others, especially those who are interested in the\n\"--recurse-submodules\" area, say.\n\nThanks.\n"},{"id":"330561","messageId":"CAGZ79kZsQoU8wJk+i5aJOxFtsD=EWu_ycEPLM1KhTaOCWD7Y2w@mail.gmail.com","threadId":"46976","inReplyTo":"20171016135827.GC12756@book.hvoigt.net","subject":"Re: [PATCH v4 2/3] implement fetching of moved submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-17T17:47:28Z","receivedAt":"2017-10-17T17:47:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 16, 2017 at 6:58 AM, Heiko Voigt wrote:\n> We store the changed submodules paths to calculate which submodule needs\n> fetching. This does not work for moved submodules since their paths do\n> not stay the same in case of a moved submodules. In case of new\n> submodules we do not have a path in the current checkout, since they\n> just appeared in this fetch.\n>\n> It is more general to collect the submodule names for changes instead of\n> their paths to include the above cases. If we do not have a\n> configuration for a gitlink we rely on constructing a default name from\n> the path if a git repository can be found at its path. We skip\n> non-configured gitlinks whose default name collides with a configured\n> one.\n\nThanks for working on this!\n\nAs detailed below, I wonder if it is easier (in maintenance, explaining\ncorrectness, reviewing) if we'd rather keep two lists around. One for\nbased on names, and if we cannot lookup a name for a submodule, we\nrather use the second path based list as a fallback. That would avoid\npotential namespace collisions between names and paths, as well as\nnot having the confusion of mapping back and forth.\n\nMost functions would then need to operate on path, as the name->path\nmapping can be looked up for the first list, but the path->name mapping\ncannot be looked up for the second list.\n\nCheers,\nStefan\n\n> With the change described above we implement 'on-demand' fetching of\n> changes in moved submodules.\n>\n> Signed-off-by: Heiko Voigt \n\n> ---\n> submodule-config.h | 3 +\n> submodule.c | 138 ++++++++++++++++++++++++++++++++------------\n> t/t5526-fetch-submodules.sh | 35 +++++++++++\n> 3 files changed, 138 insertions(+), 38 deletions(-)\n>\n> diff --git a/submodule-config.h b/submodule-config.h\n> index e3845831f6..a5503a5d17 100644\n> --- a/submodule-config.h\n> +++ b/submodule-config.h\n> @@ -22,6 +22,9 @@ struct submodule {\n> int recommend_shallow;\n> };\n>\n> +#define SUBMODULE_INIT { NULL, NULL, NULL, RECURSE_SUBMODULES_NONE, \\\n> + NULL, NULL, SUBMODULE_UPDATE_STRATEGY_INIT, {0}, -1 };\n> +\n> struct submodule_cache;\n> struct repository;\n>\n> diff --git a/submodule.c b/submodule.c\n> index 63e7094e16..71d1773e2e 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -21,7 +21,7 @@\n> #include \"parse-options.h\"\n>\n> static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n> -static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;\n> +static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n> static int initialized_fetch_ref_tips;\n> static struct oid_array ref_tips_before_fetch;\n> static struct oid_array ref_tips_after_fetch;\n> @@ -674,11 +674,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)\n> }\n>\n> static struct oid_array *submodule_commits(struct string_list *submodules,\n> - const char *path)\n> + const char *name)\n> {\n> struct string_list_item *item;\n>\n> - item = string_list_insert(submodules, path);\n> + item = string_list_insert(submodules, name);\n> if (item->util)\n> return (struct oid_array *) item->util;\n>\n> @@ -687,39 +687,67 @@ static struct oid_array *submodule_commits(struct string_list *submodules,\n> return (struct oid_array *) item->util;\n> }\n>\n> +struct collect_changed_submodules_cb_data {\n> + struct string_list *changed;\n> + const struct object_id *commit_oid;\n> +};\n> +\n> +/*\n> + * this would normally be two functions: default_name_from_path() and\n\nPlease start the comment capitalised. (minor nit)\n\n> + * path_from_default_name(). Since the default name is the same as\n> + * the submodule path we can get away with just one function which only\n> + * checks whether there is a submodule in the working directory at that\n> + * location.\n\nThis is an interesting comment, as it hints that we ought to keep it that way.\nEarlier I was wondering if we want to make the name distinctively different\nthan its path, as that will confuse users *less* IMHO. (I just remember\nsomeone asking on the mailing list why their \"rename\" did not work, as\nthey just renamed everything in the .gitmodules that looked like the path)\n\nAs the path/name is confusing, I'd wish we'd be super concise, such that\nerrors are harder to sneak into. For example, the arguments name should\nbe \"path\" as that is the only thing we can look up using is_sub_pop_gently,\nif a \"name\" is given, than it just works because the chosen default name\nwas its path.\n\n> +static const char *default_name_or_path(const char *path_or_name)\n> +{\n> + int error_code;\n> +\n> + if (!is_submodule_populated_gently(path_or_name, &error_code))\n> + return NULL;\n> +\n> + return path_or_name;\n> +}\n> +\n\n\n> + if (submodule)\n> + name = submodule->name;\n> + else {\n> + name = default_name_or_path(p->two->path);\n\nHere we use the path, as expected. So ideally we'd use\n\"default_name_for_path\".\n\n\n> + /* make sure name does not collide with existing one */\n> + submodule = submodule_from_name(commit_oid, name);\n> + if (submodule) {\n> + warning(\"Submodule in commit %s at path: \"\n> + \"'%s' collides with a submodule named \"\n> + \"the same. Skipping it.\",\n> + oid_to_hex(commit_oid), name);\n> + name = NULL;\n> + }\n\nThis is the ugly part of using one string list and storing names or\npath in it. I wonder if we could omit this warning if we had 2 string lists?\nOne for names (which will then be used for renamed and new submodules)\nand the \"fall back\" path based list. In such a world we would not need\nto map back and forth between names and path.\n\n> + submodule = submodule_from_name(&null_oid, name->string);\n> + if (submodule)\n> + path = submodule->path;\n> + else\n> + path = default_name_or_path(name->string);\n> +\n> + if (!path)\n> + continue;\n\n\n> + submodule = submodule_from_name(&null_oid, name->string);\n> + if (submodule)\n> + path = submodule->path;\n> + else\n> + path = default_name_or_path(name->string);\n> +\n> + if (!path)\n> + continue;\n\n\n> submodule = submodule_from_path(&null_oid, ce->name);\n> + if (!submodule) {\n> + const char *name = default_name_or_path(ce->name);\n> + if (name) {\n> + default_submodule.path = default_submodule.name = name;\n> + submodule = &default_submodule;\n> + }\n> + }\n>\n\n>\n> +test_expect_success \"fetch new commits when submodule got renamed\" '\n> + git clone . downstream_rename &&\n> + (\n> + cd downstream_rename &&\n> + git submodule update --init &&\n> +# NEEDSWORK: we omitted --recursive for the submodule update here since\n> +# that does not work. See test 7001 for mv \"moving nested submodules\"\n> +# for details. Once that is fixed we should add the --recursive option\n> +# here.\n> + git checkout -b rename &&\n> + git mv submodule submodule_renamed &&\n> + (\n> + cd submodule_renamed &&\n> + git checkout -b rename_sub &&\n> + echo a >a &&\n> + git add a &&\n> + git commit -ma &&\n> + git push origin rename_sub &&\n> + git rev-parse HEAD >../../expect\n> + ) &&\n> + git add submodule_renamed &&\n> + git commit -m \"update renamed submodule\" &&\n> + git push origin rename\n> + ) &&\n> + (\n> + cd downstream &&\n> + git fetch --recurse-submodules=on-demand &&\n> + (\n> + cd submodule &&\n> + git rev-parse origin/rename_sub >../../actual\n> + )\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.14.1.145.gb3622a4\n>\n"},{"id":"330563","messageId":"CAGZ79kaA6myLpDcN2H4sdbMKvkuVRp4Zud==k=p1BNfWn95a4Q@mail.gmail.com","threadId":"46976","inReplyTo":"20171016135715.GB12756@book.hvoigt.net","subject":"Re: [PATCH v4 1/3] fetch: add test to make sure we stay backwards compatible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-17T17:56:24Z","receivedAt":"2017-10-17T17:56:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 16, 2017 at 6:57 AM, Heiko Voigt wrote:\n> The current implementation of submodules supports on-demand fetch if\n> there is no .gitmodules entry for a submodule. Let's add a test to\n> document this behavior.\n>\n> Signed-off-by: Heiko Voigt \n> ---\n> t/t5526-fetch-submodules.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 41 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 42251f7f3a..43a22f680f 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -478,7 +478,47 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n> git fetch >../actual.out 2>../actual.err\n> ) &&\n> ! test -s actual.out &&\n> - test_i18ncmp expect.err actual.err\n> + test_i18ncmp expect.err actual.err &&\n> + (\n> + cd submodule &&\n> + git checkout -q master\n> + )\n\nFor few instructions inside another repo, I tend to use the\n-C option:\n\n git -C submodule checkout -q master\n\nThat saves a shell, which is noticeable cost on Windows I was told.\n(also fewer lines to type).\n\nOh, I see, that is consistent with the rest of the file. Oh well.\n(Otherwise I would have lobbied to even move it further up and\nput it inside a test_when_finished \"\"\n\n\n> +'\n> +\n> +test_expect_success \"'fetch.recurseSubmodules=on-demand' works also without .gitmodule entry\" '\n> + (\n> + cd downstream &&\n> + git fetch --recurse-submodules\n> + ) &&\n\nThis is consistent with the rest of the file as well, so I shall\nrefrain from complaining. ;)\n\n> + add_upstream_commit &&\n> + head1=$(git rev-parse --short HEAD) &&\n> + git add submodule &&\n> + git rm .gitmodules &&\n> + git commit -m \"new submodule without .gitmodules\" &&\n> + printf \"\" >expect.out &&\n\nThis could be just\n\n : >expect.out\n\nno need to invoke a function to print nothing.\n\n> + head2=$(git rev-parse --short HEAD) &&\n> + echo \"From $pwd/.\" >expect.err.2 &&\n> + echo \" $head1..$head2 master -> origin/master\" >>expect.err.2 &&\n> + head -3 expect.err >>expect.err.2 &&\n> + (\n> + cd downstream &&\n> + rm .gitmodules &&\n> + git config fetch.recurseSubmodules on-demand &&\n> + # fake submodule configuration to avoid skipping submodule handling\n> + git config -f .gitmodules submodule.fake.path fake &&\n> + git config -f .gitmodules submodule.fake.url fakeurl &&\n> + git add .gitmodules &&\n> + git config --unset submodule.submodule.url &&\n> + git fetch >../actual.out 2>../actual.err &&\n> + # cleanup\n> + git config --unset fetch.recurseSubmodules &&\n> + git reset --hard\n> + ) &&\n> + test_i18ncmp expect.out actual.out &&\n> + test_i18ncmp expect.err.2 actual.err &&\n> + git checkout HEAD^ -- .gitmodules &&\n> + git add .gitmodules &&\n> + git commit -m \"new submodule restored .gitmodules\"\n> '\n\nThanks for writing this test.\nWith or without the nits addressed, this is\n\nReviewed-by: Stefan Beller \n"},{"id":"330565","messageId":"CAGZ79kaOqynvWsWxReKT=c33+EA2FSAbBicY7vsHuvAxOnAwZA@mail.gmail.com","threadId":"46976","inReplyTo":"20171016135905.GD12756@book.hvoigt.net","subject":"Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-17T18:22:20Z","receivedAt":"2017-10-17T18:22:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 16, 2017 at 6:59 AM, Heiko Voigt wrote:\n> To make extending this logic later easier.\n>\n> Signed-off-by: Heiko Voigt \n\nThanks for this readability fix!\n\nStefan\n"},{"id":"330582","messageId":"xmqqfuahmif9.fsf@gitster.mtv.corp.google.com","threadId":"46976","inReplyTo":"CAGZ79kZsQoU8wJk+i5aJOxFtsD=EWu_ycEPLM1KhTaOCWD7Y2w@mail.gmail.com","subject":"Re: [PATCH v4 2/3] implement fetching of moved submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-18T00:03:06Z","receivedAt":"2017-10-18T00:03:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> + /* make sure name does not collide with existing one */\n>> + submodule = submodule_from_name(commit_oid, name);\n>> + if (submodule) {\n>> + warning(\"Submodule in commit %s at path: \"\n>> + \"'%s' collides with a submodule named \"\n>> + \"the same. Skipping it.\",\n>> + oid_to_hex(commit_oid), name);\n>> + name = NULL;\n>> + }\n>\n> This is the ugly part of using one string list and storing names or\n> path in it. I wonder if we could omit this warning if we had 2 string lists?\n\nWe are keying off of 'name', because that is what will give a module\nits identity. If we have a gitlink whose path is not in .gitmodules\nin the same tree, then we are seeing an unregistered submodule. If\nwe were to \"git add\" it, then we'd use its path as the default name,\nbut if we already have a submodule with that name (the most likely\nexplanation for its existence is because it started its life there\nand then later moved), and the submodule is bound to a different\npath, then that is a different submodule. Skipping and warning both\nare sensible thing to do.\n\nI do not know what you see as ugly here, and more importantly, I am\nnot sure how having two lists would help.\n"},{"id":"330583","messageId":"xmqqbml5mieh.fsf@gitster.mtv.corp.google.com","threadId":"46976","inReplyTo":"CAGZ79kaOqynvWsWxReKT=c33+EA2FSAbBicY7vsHuvAxOnAwZA@mail.gmail.com","subject":"Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-18T00:03:34Z","receivedAt":"2017-10-18T00:03:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, Oct 16, 2017 at 6:59 AM, Heiko Voigt wrote:\n>> To make extending this logic later easier.\n>>\n>> Signed-off-by: Heiko Voigt \n>\n> Thanks for this readability fix!\n>\n> Stefan\n\nThanks, both.\n"},{"id":"330603","messageId":"CAGZ79kaTXC9Eius3jMZGefZioJtS-uuf+ar5zt=WSEWQJxdcwQ@mail.gmail.com","threadId":"46976","inReplyTo":"xmqqfuahmif9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/3] implement fetching of moved submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-18T17:56:58Z","receivedAt":"2017-10-18T17:57:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 17, 2017 at 5:03 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> + /* make sure name does not collide with existing one */\n>>> + submodule = submodule_from_name(commit_oid, name);\n>>> + if (submodule) {\n>>> + warning(\"Submodule in commit %s at path: \"\n>>> + \"'%s' collides with a submodule named \"\n>>> + \"the same. Skipping it.\",\n>>> + oid_to_hex(commit_oid), name);\n>>> + name = NULL;\n>>> + }\n>>\n>> This is the ugly part of using one string list and storing names or\n>> path in it. I wonder if we could omit this warning if we had 2 string lists?\n>\n> We are keying off of 'name', because that is what will give a module\n> its identity. If we have a gitlink whose path is not in .gitmodules\n> in the same tree, then we are seeing an unregistered submodule.\n\nRight, so it has no submodule specific identity and we chose to \"fake it\"\nby pretending its path is its name. However this requires checking as\nthere might be overlap in the name-namespace and the path-namespace.\n\n\n> If\n> we were to \"git add\" it, then we'd use its path as the default name,\n\nI presume \"git submodule add\"\n\n> but if we already have a submodule with that name (the most likely\n> explanation for its existence is because it started its life there\n> and then later moved), and the submodule is bound to a different\n> path, then that is a different submodule. Skipping and warning both\n> are sensible thing to do.\n\nSkipping and warning is sensible once we decide to go this way.\n\nI propose to take a step back and not throw away the information\nwhether the given string is a name or path, as then we do not have\nto warn&skip, but we can treat both correctly.\n\nAs we only need to store an additional boolean (is it path or name?),\nI had suggested to just use two lists, one for key-by-name and one\nkey-by-path, where we intend to use the key-by-name for submodules\nand the by-path only for those with no name (i.e. lone gitlinks), hence\nmaking this a \"fallback list\"\n\n>\n> I do not know what you see as ugly here,\n\nthe necessity of warn&skip instead of having a solution that\nworks in corner cases just fine.\n\n> and more importantly, I am\n> not sure how having two lists would help.\n\nThe current situation is that we use the path of the submodules only,\nwhich makes it work without warn&skip, but it has other disadvantages\n(i.e. new & moved submodules are not detected), which we want to fix.\n\nWe can add this functionality without caving in to skip the corner case\nby storing an additional bit of information. The renaming is detected by\nhaving a constant name before and after, just the path changed.\nSo we could continue to use by-path logic and only have the name\nfor rename detection. However that seems to be ugly, too. So we\nseem to think that the by-name is better (as it is more in line with what\nwe think should happen, it is easier to explain, review and maintain(?)).\n\nSo we could have by-name keys, with the extra information of whether the\nkey is genuine or a \"fake\" key, which is t be resolved to a path instead.\nAnd as that is just one bit, I proposed two lists for that.\n\nDo I miss an essential part here?\n\nThanks,\nStefan\n"},{"id":"330604","messageId":"20171018180322.GA155019@google.com","threadId":"46976","inReplyTo":"20171016135905.GD12756@book.hvoigt.net","subject":"Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-18T18:03:22Z","receivedAt":"2017-10-18T18:03:33Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 10/16, Heiko Voigt wrote:\n> To make extending this logic later easier.\n\nThis makes things so much clearer, thanks!\n\n> \n> Signed-off-by: Heiko Voigt \n> ---\n> submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------\n> 1 file changed, 37 insertions(+), 37 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 71d1773e2e..82d206eb65 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {\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> +\tif (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)\n> +\t\treturn spf->command_line_option;\n> +\n> +\tif (submodule) {\n> +\t\tchar *key;\n> +\t\tconst char *value;\n> +\n> +\t\tint fetch_recurse = submodule->fetch_recurse;\n> +\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> +\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n> +\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> +\t\t}\n> +\t\tfree(key);\n> +\n> +\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE)\n> +\t\t\t/* local config overrules everything except commandline */\n> +\t\t\treturn fetch_recurse;\n> +\t}\n> +\n> +\treturn spf->default_option;\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> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,\n> \t\t\t}\n> \t\t}\n> \n> -\t\tdefault_argv = \"yes\";\n> -\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> -\t\t\tint fetch_recurse = RECURSE_SUBMODULES_NONE;\n> -\n> -\t\t\tif (submodule) {\n> -\t\t\t\tchar *key;\n> -\t\t\t\tconst char *value;\n> -\n> -\t\t\t\tfetch_recurse = submodule->fetch_recurse;\n> -\t\t\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> -\t\t\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n> -\t\t\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> -\t\t\t\t}\n> -\t\t\t\tfree(key);\n> -\t\t\t}\n> -\n> -\t\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n> -\t\t\t\t\tcontinue;\n> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> -\t\t\t\t\t\t\t\t\t submodule->name))\n> -\t\t\t\t\t\tcontinue;\n> -\t\t\t\t\tdefault_argv = \"on-demand\";\n> -\t\t\t\t}\n> -\t\t\t} else {\n> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_OFF)\n> -\t\t\t\t\tcontinue;\n> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> -\t\t\t\t\t\t\t\t\t submodule->name))\n> -\t\t\t\t\t\tcontinue;\n> -\t\t\t\t\tdefault_argv = \"on-demand\";\n> -\t\t\t\t}\n> -\t\t\t}\n> -\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> -\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> +\t\tswitch (get_fetch_recurse_config(submodule, spf))\n> +\t\t{\n> +\t\tdefault:\n> +\t\tcase RECURSE_SUBMODULES_DEFAULT:\n> +\t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n> +\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n> \t\t\t\t\t\t\t submodule->name))\n> \t\t\t\tcontinue;\n> \t\t\tdefault_argv = \"on-demand\";\n> +\t\t\tbreak;\n> +\t\tcase RECURSE_SUBMODULES_ON:\n> +\t\t\tdefault_argv = \"yes\";\n> +\t\t\tbreak;\n> +\t\tcase RECURSE_SUBMODULES_OFF:\n> +\t\t\tcontinue;\n> \t\t}\n> \n> \t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n> -- \n> 2.14.1.145.gb3622a4\n> \n\n-- \nBrandon Williams\n"},{"id":"330615","messageId":"xmqqwp3sj7ov.fsf@gitster.mtv.corp.google.com","threadId":"46976","inReplyTo":"CAGZ79kaTXC9Eius3jMZGefZioJtS-uuf+ar5zt=WSEWQJxdcwQ@mail.gmail.com","subject":"Re: [PATCH v4 2/3] implement fetching of moved submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-19T00:35:28Z","receivedAt":"2017-10-19T00:35:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> but if we already have a submodule with that name (the most likely\n>> explanation for its existence is because it started its life there\n>> and then later moved), and the submodule is bound to a different\n>> path, then that is a different submodule. Skipping and warning both\n>> are sensible thing to do.\n>\n> Skipping and warning is sensible once we decide to go this way.\n>\n> I propose to take a step back and not throw away the information\n> whether the given string is a name or path, as then we do not have\n> to warn&skip, but we can treat both correctly.\n\nNow either one of us is utterly confused, and I suspect it is me, as\nI do not see how \"treat both correctly\" could possibly work in the\ncase this code warns and skips.\n\nAt this point in the flow, we already know that it is not name,\nbecause we asked and got a \"Nah, there is no submodule registered in\n.gitmodules at that path\" from submodule_from_path(). Then we ask\nsubmodule_from_name() if there is any submodule registered under the\nname it would have got if it were added there, and we indeed find\none. And that is definitely *not* a submodule we are looking for,\nbecause if it were, its .path would have pointed at the path we were\nusing to ask in the first place. The one we originally found at\npath and are interested in finding out the details is not known to\n.gitmodules, and the one under that name is not the one that we are\nintereted in, so fetching from the repository the other one that\nhappens to have the same name but is different from the submodule we\nare interested in would simply be wrong.\n\nIf we only have path without any .gitmodules entry (hence there is\nnot even URL), how would we proceed from that point on?\n\n"},{"id":"330616","messageId":"xmqqshegj7mo.fsf@gitster.mtv.corp.google.com","threadId":"46976","inReplyTo":"20171018180322.GA155019@google.com","subject":"Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-19T00:36:47Z","receivedAt":"2017-10-19T00:37:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> On 10/16, Heiko Voigt wrote:\n>> To make extending this logic later easier.\n>\n> This makes things so much clearer, thanks!\n\nI agree that it is clear to see what the code after the patch does,\nbut the code before the patch is so convoluted to follow that it is\na bit hard to see if the code before and after are doing the same\nthing, though ;-)\n\n>\n>> \n>> Signed-off-by: Heiko Voigt \n>> ---\n>> submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------\n>> 1 file changed, 37 insertions(+), 37 deletions(-)\n>> \n>> diff --git a/submodule.c b/submodule.c\n>> index 71d1773e2e..82d206eb65 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {\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>> +\tif (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)\n>> +\t\treturn spf->command_line_option;\n>> +\n>> +\tif (submodule) {\n>> +\t\tchar *key;\n>> +\t\tconst char *value;\n>> +\n>> +\t\tint fetch_recurse = submodule->fetch_recurse;\n>> +\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n>> +\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n>> +\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n>> +\t\t}\n>> +\t\tfree(key);\n>> +\n>> +\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE)\n>> +\t\t\t/* local config overrules everything except commandline */\n>> +\t\t\treturn fetch_recurse;\n>> +\t}\n>> +\n>> +\treturn spf->default_option;\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>> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,\n>> \t\t\t}\n>> \t\t}\n>> \n>> -\t\tdefault_argv = \"yes\";\n>> -\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n>> -\t\t\tint fetch_recurse = RECURSE_SUBMODULES_NONE;\n>> -\n>> -\t\t\tif (submodule) {\n>> -\t\t\t\tchar *key;\n>> -\t\t\t\tconst char *value;\n>> -\n>> -\t\t\t\tfetch_recurse = submodule->fetch_recurse;\n>> -\t\t\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n>> -\t\t\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n>> -\t\t\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n>> -\t\t\t\t}\n>> -\t\t\t\tfree(key);\n>> -\t\t\t}\n>> -\n>> -\t\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n>> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n>> -\t\t\t\t\tcontinue;\n>> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n>> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n>> -\t\t\t\t\t\t\t\t\t submodule->name))\n>> -\t\t\t\t\t\tcontinue;\n>> -\t\t\t\t\tdefault_argv = \"on-demand\";\n>> -\t\t\t\t}\n>> -\t\t\t} else {\n>> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_OFF)\n>> -\t\t\t\t\tcontinue;\n>> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {\n>> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n>> -\t\t\t\t\t\t\t\t\t submodule->name))\n>> -\t\t\t\t\t\tcontinue;\n>> -\t\t\t\t\tdefault_argv = \"on-demand\";\n>> -\t\t\t\t}\n>> -\t\t\t}\n>> -\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n>> -\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n>> +\t\tswitch (get_fetch_recurse_config(submodule, spf))\n>> +\t\t{\n>> +\t\tdefault:\n>> +\t\tcase RECURSE_SUBMODULES_DEFAULT:\n>> +\t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n>> +\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n>> \t\t\t\t\t\t\t submodule->name))\n>> \t\t\t\tcontinue;\n>> \t\t\tdefault_argv = \"on-demand\";\n>> +\t\t\tbreak;\n>> +\t\tcase RECURSE_SUBMODULES_ON:\n>> +\t\t\tdefault_argv = \"yes\";\n>> +\t\t\tbreak;\n>> +\t\tcase RECURSE_SUBMODULES_OFF:\n>> +\t\t\tcontinue;\n>> \t\t}\n>> \n>> \t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n>> -- \n>> 2.14.1.145.gb3622a4\n>> \n"},{"id":"330640","messageId":"20171019153844.GA41283@book.hvoigt.net","threadId":"46976","inReplyTo":"xmqqshegj7mo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-19T15:38:44Z","receivedAt":"2017-10-19T15:40:36Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Oct 19, 2017 at 09:36:47AM +0900, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > On 10/16, Heiko Voigt wrote:\n> >> To make extending this logic later easier.\n> >\n> > This makes things so much clearer, thanks!\n> \n> I agree that it is clear to see what the code after the patch does,\n> but the code before the patch is so convoluted to follow that it is\n> a bit hard to see if the code before and after are doing the same\n> thing, though ;-)\n\nThat is why I would appreciate some extra pairs of eyes on this :) I\ntried to be as careful as possible when refactoring this, but since it\nis quite convoluted something might have slipped through. The testsuite\ndoes not show anything, but there might be corner cases that are not\ntested I guess.\n\nWill hopefully have time to look into the comments to the main patch of\nthis series tomorrow. Did not get around to properly do that yet.\n\nCheers Heiko\n\n> >> Signed-off-by: Heiko Voigt \n> >> ---\n> >> submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------\n> >> 1 file changed, 37 insertions(+), 37 deletions(-)\n> >> \n> >> diff --git a/submodule.c b/submodule.c\n> >> index 71d1773e2e..82d206eb65 100644\n> >> --- a/submodule.c\n> >> +++ b/submodule.c\n> >> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {\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> >> +\tif (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)\n> >> +\t\treturn spf->command_line_option;\n> >> +\n> >> +\tif (submodule) {\n> >> +\t\tchar *key;\n> >> +\t\tconst char *value;\n> >> +\n> >> +\t\tint fetch_recurse = submodule->fetch_recurse;\n> >> +\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> >> +\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n> >> +\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> >> +\t\t}\n> >> +\t\tfree(key);\n> >> +\n> >> +\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE)\n> >> +\t\t\t/* local config overrules everything except commandline */\n> >> +\t\t\treturn fetch_recurse;\n> >> +\t}\n> >> +\n> >> +\treturn spf->default_option;\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> >> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,\n> >> \t\t\t}\n> >> \t\t}\n> >> \n> >> -\t\tdefault_argv = \"yes\";\n> >> -\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> >> -\t\t\tint fetch_recurse = RECURSE_SUBMODULES_NONE;\n> >> -\n> >> -\t\t\tif (submodule) {\n> >> -\t\t\t\tchar *key;\n> >> -\t\t\t\tconst char *value;\n> >> -\n> >> -\t\t\t\tfetch_recurse = submodule->fetch_recurse;\n> >> -\t\t\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> >> -\t\t\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n> >> -\t\t\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> >> -\t\t\t\t}\n> >> -\t\t\t\tfree(key);\n> >> -\t\t\t}\n> >> -\n> >> -\t\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n> >> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n> >> -\t\t\t\t\tcontinue;\n> >> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n> >> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> >> -\t\t\t\t\t\t\t\t\t submodule->name))\n> >> -\t\t\t\t\t\tcontinue;\n> >> -\t\t\t\t\tdefault_argv = \"on-demand\";\n> >> -\t\t\t\t}\n> >> -\t\t\t} else {\n> >> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_OFF)\n> >> -\t\t\t\t\tcontinue;\n> >> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> >> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> >> -\t\t\t\t\t\t\t\t\t submodule->name))\n> >> -\t\t\t\t\t\tcontinue;\n> >> -\t\t\t\t\tdefault_argv = \"on-demand\";\n> >> -\t\t\t\t}\n> >> -\t\t\t}\n> >> -\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> >> -\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> >> +\t\tswitch (get_fetch_recurse_config(submodule, spf))\n> >> +\t\t{\n> >> +\t\tdefault:\n> >> +\t\tcase RECURSE_SUBMODULES_DEFAULT:\n> >> +\t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n> >> +\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n> >> \t\t\t\t\t\t\t submodule->name))\n> >> \t\t\t\tcontinue;\n> >> \t\t\tdefault_argv = \"on-demand\";\n> >> +\t\t\tbreak;\n> >> +\t\tcase RECURSE_SUBMODULES_ON:\n> >> +\t\t\tdefault_argv = \"yes\";\n> >> +\t\t\tbreak;\n> >> +\t\tcase RECURSE_SUBMODULES_OFF:\n> >> +\t\t\tcontinue;\n> >> \t\t}\n> >> \n> >> \t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n> >> -- \n> >> 2.14.1.145.gb3622a4\n> >> \n"},{"id":"330656","messageId":"20171019181109.27792-1-sbeller@google.com","threadId":"46976","inReplyTo":"xmqqwp3sj7ov.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/2] t5526: check for name/path collision in submodule fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T18:11:08Z","receivedAt":"2017-10-19T18:11:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nThis is just to test the corner case we're discussing.\nApplies on top of origin/hv/fetch-moved-submodules-on-demand.\n\n\n t/t5526-fetch-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex a552ad4ead..c82d519e06 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -571,6 +571,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n '\n \n test_expect_success \"fetch new commits when submodule got renamed\" '\n+\ttest_when_finished \"rm -rf downstream_rename\" &&\n \tgit clone . downstream_rename &&\n \t(\n \t\tcd downstream_rename &&\n@@ -605,4 +606,45 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"warn on submodule name/path clash, but new commits fetched in renamed\" '\n+\ttest_when_finished \"rm -rf downstream_rename\" &&\n+\tgit clone . downstream_rename &&\n+\t(\n+\t\tcd downstream_rename &&\n+\t\tgit submodule update --init &&\n+# NEEDSWORK: we omitted --recursive for the submodule update here since\n+# that does not work. See test 7001 for mv \"moving nested submodules\"\n+# for details. Once that is fixed we should add the --recursive option\n+# here.\n+\t\tgit checkout -b rename &&\n+\t\tgit mv submodule submodule_renamed &&\n+\t\t(\n+\t\t\tcd submodule_renamed &&\n+\t\t\tgit checkout -b rename_sub &&\n+\t\t\techo a >a &&\n+\t\t\tgit add a &&\n+\t\t\tgit commit -ma &&\n+\t\t\tgit push origin rename_sub &&\n+\t\t\tgit rev-parse HEAD >../../expect\n+\t\t) &&\n+\t\tgit add submodule_renamed &&\n+\t\tgit commit -m \"update renamed submodule\" &&\n+\t\t# produce collision, note that we use no submodule command\n+\t\tgit clone ../submodule submodule &&\n+\t\tgit add submodule &&\n+\t\tgit commit -m \"have new submodule at old path \" &&\n+\t\tgit push origin rename\n+\t) &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules=on-demand 2>err &&\n+\t\tgrep \"collides with a submodule named\" err &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\tgit rev-parse origin/rename_sub >../../actual\n+\t\t)\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"330657","messageId":"20171019181109.27792-2-sbeller@google.com","threadId":"46976","inReplyTo":"20171019181109.27792-1-sbeller@google.com","subject":"[PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T18:11:09Z","receivedAt":"2017-10-19T18:11:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently when fetching we collect the names of submodules to be fetched\nin a list. As we also want to support fetching 'gitlinks, that happen to\nhave a repo checked out at the right place', we'll just pretend that these\nare submodules. We do that by assuming their path is their name. This in\nturn can yield collisions between the name-namespace and the\npath-namespace. (See the previous test for a demonstration.)\n\nThis patch rewrites the code such that we treat the 'real submodule' case\ndifferently from the 'gitlink, but ok' case. This introduces a bit\nof code duplication, but gets rid of the confusing mapping between names\nand paths.\n\nThe test is incomplete as the long term vision is not achieved yet.\n(which would be fetching both the renamed submodule as well as\nthe gitlink thing, putting them in place via e.g. git-pull)\n\nSigned-off-by: Stefan Beller \n---\n\n Heiko,\n Junio,\n\n I assumed the code would ease up a lot more, but now I am undecided if\n I want to keep arguing as the code is not stopping to be ugly. :)\n \n The idea is to treat submodule and gitlinks separately, with submodules\n supporting renames, and gitlinks as a historic artefact.\n \n Sorry for the noise about code ugliness.\n \n Thanks,\n Stefan\n \n\n submodule.c | 168 +++++++++++++++++++++-----------------------\n t/t5526-fetch-submodules.sh | 1 -\n 2 files changed, 81 insertions(+), 88 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 82d206eb65..115df82f32 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -22,6 +22,7 @@\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n+static struct string_list changed_gitlink_paths = STRING_LIST_INIT_DUP;\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n@@ -674,11 +675,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)\n }\n \n static struct oid_array *submodule_commits(struct string_list *submodules,\n-\t\t\t\t\t const char *name)\n+\t\t\t\t\t const char *key)\n {\n \tstruct string_list_item *item;\n \n-\titem = string_list_insert(submodules, name);\n+\titem = string_list_insert(submodules, key);\n \tif (item->util)\n \t\treturn (struct oid_array *) item->util;\n \n@@ -688,33 +689,20 @@ static struct oid_array *submodule_commits(struct string_list *submodules,\n }\n \n struct collect_changed_submodules_cb_data {\n-\tstruct string_list *changed;\n-\tconst struct object_id *commit_oid;\n-};\n+\t/* used for submodules, supports renames: */\n+\tstruct string_list *changed_by_name;\n \n-/*\n- * this would normally be two functions: default_name_from_path() and\n- * path_from_default_name(). Since the default name is the same as\n- * the submodule path we can get away with just one function which only\n- * checks whether there is a submodule in the working directory at that\n- * location.\n- */\n-static const char *default_name_or_path(const char *path_or_name)\n-{\n-\tint error_code;\n+\t/* support old 'gitlink' with repo in-place, no rename support*/\n+\tstruct string_list *changed_by_path;\n \n-\tif (!is_submodule_populated_gently(path_or_name, &error_code))\n-\t\treturn NULL;\n-\n-\treturn path_or_name;\n-}\n+\tconst struct object_id *commit_oid;\n+};\n \n static void collect_changed_submodules_cb(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n {\n \tstruct collect_changed_submodules_cb_data *me = data;\n-\tstruct string_list *changed = me->changed;\n \tconst struct object_id *commit_oid = me->commit_oid;\n \tint i;\n \n@@ -722,42 +710,35 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tstruct oid_array *commits;\n \t\tconst struct submodule *submodule;\n-\t\tconst char *name;\n \n \t\tif (!S_ISGITLINK(p->two->mode))\n \t\t\tcontinue;\n \n \t\tsubmodule = submodule_from_path(commit_oid, p->two->path);\n-\t\tif (submodule)\n-\t\t\tname = submodule->name;\n-\t\telse {\n-\t\t\tname = default_name_or_path(p->two->path);\n-\t\t\t/* make sure name does not collide with existing one */\n-\t\t\tsubmodule = submodule_from_name(commit_oid, name);\n-\t\t\tif (submodule) {\n-\t\t\t\twarning(\"Submodule in commit %s at path: \"\n-\t\t\t\t\t\"'%s' collides with a submodule named \"\n-\t\t\t\t\t\"the same. Skipping it.\",\n-\t\t\t\t\toid_to_hex(commit_oid), name);\n-\t\t\t\tname = NULL;\n-\t\t\t}\n+\t\tif (submodule) {\n+\t\t\tcommits = submodule_commits(me->changed_by_name, submodule->name);\n+\t\t\toid_array_append(commits, &p->two->oid);\n+\t\t} else {\n+\t\t\tcommits = submodule_commits(me->changed_by_path, p->two->path);\n+\t\t\toid_array_append(commits, &p->two->oid);\n \t\t}\n-\n-\t\tif (!name)\n-\t\t\tcontinue;\n-\n-\t\tcommits = submodule_commits(changed, name);\n-\t\toid_array_append(commits, &p->two->oid);\n \t}\n }\n \n /*\n- * Collect the paths of submodules in 'changed' which have changed based on\n- * the revisions as specified in 'argv'. Each entry in 'changed' will also\n- * have a corresponding 'struct oid_array' (in the 'util' field) which lists\n- * what the submodule pointers were updated to during the change.\n+ * Collect the paths of submodules in 'changed_by_{name, path}' which have\n+ * changed based on the revisions as specified in 'argv'.\n+ *\n+ * Each gitlink/submodule will occur in only one of the list. We'll prefer\n+ * to give it by_name as that allows rename detection. We'll fall back to\n+ * by_path to support gitlinks with no entry in '.gitmodules'.\n+ *\n+ * Each entry in 'changed_*' will also have a corresponding 'struct oid_array'\n+ * (in the 'util' field) which lists what the submodule pointers were updated\n+ * to during the change.\n */\n-static void collect_changed_submodules(struct string_list *changed,\n+static void collect_changed_submodules(struct string_list *changed_by_name,\n+\t\t\t\t struct string_list *changed_by_path,\n \t\t\t\t struct argv_array *argv)\n {\n \tstruct rev_info rev;\n@@ -771,7 +752,8 @@ static void collect_changed_submodules(struct string_list *changed,\n \twhile ((commit = get_revision(&rev))) {\n \t\tstruct rev_info diff_rev;\n \t\tstruct collect_changed_submodules_cb_data data;\n-\t\tdata.changed = changed;\n+\t\tdata.changed_by_name = changed_by_name;\n+\t\tdata.changed_by_path = changed_by_path;\n \t\tdata.commit_oid = &commit->object.oid;\n \n \t\tinit_revisions(&diff_rev, NULL);\n@@ -924,8 +906,9 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)\n int find_unpushed_submodules(struct oid_array *commits,\n \t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n-\tstruct string_list submodules = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *name;\n+\tstruct string_list submodules_by_name = STRING_LIST_INIT_DUP;\n+\tstruct string_list gitlinks_by_path = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \n \t/* argv.argv[0] will be ignored by setup_revisions */\n@@ -934,27 +917,33 @@ int find_unpushed_submodules(struct oid_array *commits,\n \targv_array_push(&argv, \"--not\");\n \targv_array_pushf(&argv, \"--remotes=%s\", remotes_name);\n \n-\tcollect_changed_submodules(&submodules, &argv);\n+\tcollect_changed_submodules(&submodules_by_name, &gitlinks_by_path, &argv);\n \n-\tfor_each_string_list_item(name, &submodules) {\n-\t\tstruct oid_array *commits = name->util;\n+\tfor_each_string_list_item(item, &submodules_by_name) {\n+\t\tstruct oid_array *commits = item->util;\n+\t\tconst char *name = item->string;\n \t\tconst struct submodule *submodule;\n-\t\tconst char *path = NULL;\n+\t\tconst char *path;\n \n-\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n-\t\tif (submodule)\n-\t\t\tpath = submodule->path;\n-\t\telse\n-\t\t\tpath = default_name_or_path(name->string);\n+\t\tsubmodule = submodule_from_name(&null_oid, name);\n+\t\tif (!submodule)\n+\t\t\tBUG(\"submodule name/path mapping corrupt\");\n+\t\tpath = submodule->path;\n \n-\t\tif (!path)\n-\t\t\tcontinue;\n+\t\tif (submodule_needs_pushing(path, commits))\n+\t\t\tstring_list_insert(needs_pushing, path);\n+\t}\n+\n+\tfor_each_string_list_item(item, &gitlinks_by_path) {\n+\t\tstruct oid_array *commits = item->util;\n+\t\tconst char *path = item->string;\n \n \t\tif (submodule_needs_pushing(path, commits))\n \t\t\tstring_list_insert(needs_pushing, path);\n \t}\n \n-\tfree_submodules_oids(&submodules);\n+\tfree_submodules_oids(&submodules_by_name);\n+\tfree_submodules_oids(&gitlinks_by_path);\n \targv_array_clear(&argv);\n \n \treturn needs_pushing->nr;\n@@ -1106,7 +1095,8 @@ static void calculate_changed_submodule_paths(void)\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 changed_gitlinks = STRING_LIST_INIT_DUP;\n+\tconst struct string_list_item *item;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(NULL, NULL))\n@@ -1123,27 +1113,32 @@ static void calculate_changed_submodule_paths(void)\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(&changed_submodules, &changed_gitlinks, &argv);\n \n-\tfor_each_string_list_item(name, &changed_submodules) {\n-\t\tstruct oid_array *commits = name->util;\n-\t\tconst struct submodule *submodule;\n-\t\tconst char *path = NULL;\n+\tfor_each_string_list_item(item, &changed_submodules) {\n+\t\tstruct oid_array *commits = item->util;\n+\t\tconst char *name = item->string;\n+\t\tconst struct submodule *sub =\n+\t\t\tsubmodule_from_name(&null_oid, name);\n \n-\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n-\t\tif (submodule)\n-\t\t\tpath = submodule->path;\n-\t\telse\n-\t\t\tpath = default_name_or_path(name->string);\n+\t\tif (!sub)\n+\t\t\tBUG(\"cannot lookup submodule, but we could before?\");\n \n-\t\tif (!path)\n-\t\t\tcontinue;\n+\t\tif (!submodule_has_commits(sub->path, commits))\n+\t\t\tstring_list_append(&changed_submodule_names, name);\n+\t}\n+\n+\t/* the same for gitnlinks, stored in 'changed_gitlink_paths' */\n+\tfor_each_string_list_item(item, &changed_gitlinks) {\n+\t\tconst char *path = item->string;\n+\t\tstruct oid_array *commits = item->util;\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(&changed_gitlink_paths, path);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n+\tfree_submodules_oids(&changed_gitlinks);\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n \toid_array_clear(&ref_tips_after_fetch);\n@@ -1154,6 +1149,7 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \t\t\t struct object_id *incl_oid)\n {\n \tstruct string_list subs = STRING_LIST_INIT_DUP;\n+\tstruct string_list gitlinks = STRING_LIST_INIT_DUP;\n \tstruct argv_array args = ARGV_ARRAY_INIT;\n \tint ret;\n \n@@ -1166,8 +1162,8 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \targv_array_push(&args, \"--not\");\n \targv_array_push(&args, oid_to_hex(excl_oid));\n \n-\tcollect_changed_submodules(&subs, &args);\n-\tret = subs.nr;\n+\tcollect_changed_submodules(&subs, &gitlinks, &args);\n+\tret = subs.nr + gitlinks.nr;\n \n \targv_array_clear(&args);\n \n@@ -1225,27 +1221,25 @@ static int get_next_submodule(struct child_process *cp,\n \t\tconst struct cache_entry *ce = active_cache[spf->count];\n \t\tconst char *git_dir, *default_argv;\n \t\tconst struct submodule *submodule;\n-\t\tstruct submodule default_submodule = SUBMODULE_INIT;\n+\t\tint found = 0;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n \t\tsubmodule = submodule_from_path(&null_oid, ce->name);\n-\t\tif (!submodule) {\n-\t\t\tconst char *name = default_name_or_path(ce->name);\n-\t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = default_submodule.name = name;\n-\t\t\t\tsubmodule = &default_submodule;\n-\t\t\t}\n-\t\t}\n \n \t\tswitch (get_fetch_recurse_config(submodule, spf))\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n-\t\t\t\t\t\t\t submodule->name))\n+\n+\t\t\tif (submodule)\n+\t\t\t\tfound |= !!unsorted_string_list_lookup(&changed_submodule_names, submodule->name);\n+\n+\t\t\tfound |= !!unsorted_string_list_lookup(&changed_gitlink_paths, ce->name);\n+\n+\t\t\tif (!found)\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex c82d519e06..d6a6d6a4e1 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -638,7 +638,6 @@ test_expect_success \"warn on submodule name/path clash, but new commits fetched\n \t(\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules=on-demand 2>err &&\n-\t\tgrep \"collides with a submodule named\" err &&\n \t\t(\n \t\t\tcd submodule &&\n \t\t\tgit rev-parse origin/rename_sub >../../actual\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"330662","messageId":"20171019191638.GA84767@google.com","threadId":"46976","inReplyTo":"20171019153844.GA41283@book.hvoigt.net","subject":"Re: [PATCH v4 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-19T19:16:38Z","receivedAt":"2017-10-19T19:16:47Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 10/19, Heiko Voigt wrote:\n> On Thu, Oct 19, 2017 at 09:36:47AM +0900, Junio C Hamano wrote:\n> > Brandon Williams writes:\n> > \n> > > On 10/16, Heiko Voigt wrote:\n> > >> To make extending this logic later easier.\n> > >\n> > > This makes things so much clearer, thanks!\n> > \n> > I agree that it is clear to see what the code after the patch does,\n> > but the code before the patch is so convoluted to follow that it is\n> > a bit hard to see if the code before and after are doing the same\n> > thing, though ;-)\n> \n> That is why I would appreciate some extra pairs of eyes on this :) I\n> tried to be as careful as possible when refactoring this, but since it\n> is quite convoluted something might have slipped through. The testsuite\n> does not show anything, but there might be corner cases that are not\n> tested I guess.\n> \n> Will hopefully have time to look into the comments to the main patch of\n> this series tomorrow. Did not get around to properly do that yet.\n> \n> Cheers Heiko\n> \n> > >> Signed-off-by: Heiko Voigt \n> > >> ---\n> > >> submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------\n> > >> 1 file changed, 37 insertions(+), 37 deletions(-)\n> > >> \n> > >> diff --git a/submodule.c b/submodule.c\n> > >> index 71d1773e2e..82d206eb65 100644\n> > >> --- a/submodule.c\n> > >> +++ b/submodule.c\n> > >> @@ -1187,6 +1187,31 @@ struct submodule_parallel_fetch {\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> > >> +\tif (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)\n> > >> +\t\treturn spf->command_line_option;\n> > >> +\n> > >> +\tif (submodule) {\n> > >> +\t\tchar *key;\n> > >> +\t\tconst char *value;\n> > >> +\n> > >> +\t\tint fetch_recurse = submodule->fetch_recurse;\n> > >> +\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> > >> +\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n> > >> +\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> > >> +\t\t}\n> > >> +\t\tfree(key);\n> > >> +\n> > >> +\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE)\n> > >> +\t\t\t/* local config overrules everything except commandline */\n> > >> +\t\t\treturn fetch_recurse;\n> > >> +\t}\n> > >> +\n> > >> +\treturn spf->default_option;\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> > >> @@ -1214,46 +1239,21 @@ static int get_next_submodule(struct child_process *cp,\n> > >> \t\t\t}\n> > >> \t\t}\n> > >> \n> > >> -\t\tdefault_argv = \"yes\";\n> > >> -\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> > >> -\t\t\tint fetch_recurse = RECURSE_SUBMODULES_NONE;\n> > >> -\n> > >> -\t\t\tif (submodule) {\n> > >> -\t\t\t\tchar *key;\n> > >> -\t\t\t\tconst char *value;\n> > >> -\n> > >> -\t\t\t\tfetch_recurse = submodule->fetch_recurse;\n> > >> -\t\t\t\tkey = xstrfmt(\"submodule.%s.fetchRecurseSubmodules\", submodule->name);\n> > >> -\t\t\t\tif (!repo_config_get_string_const(the_repository, key, &value)) {\n> > >> -\t\t\t\t\tfetch_recurse = parse_fetch_recurse_submodules_arg(key, value);\n> > >> -\t\t\t\t}\n> > >> -\t\t\t\tfree(key);\n> > >> -\t\t\t}\n> > >> -\n> > >> -\t\t\tif (fetch_recurse != RECURSE_SUBMODULES_NONE) {\n> > >> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_OFF)\n> > >> -\t\t\t\t\tcontinue;\n> > >> -\t\t\t\tif (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {\n> > >> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> > >> -\t\t\t\t\t\t\t\t\t submodule->name))\n> > >> -\t\t\t\t\t\tcontinue;\n> > >> -\t\t\t\t\tdefault_argv = \"on-demand\";\n> > >> -\t\t\t\t}\n> > >> -\t\t\t} else {\n> > >> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_OFF)\n> > >> -\t\t\t\t\tcontinue;\n> > >> -\t\t\t\tif (spf->default_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> > >> -\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> > >> -\t\t\t\t\t\t\t\t\t submodule->name))\n> > >> -\t\t\t\t\t\tcontinue;\n> > >> -\t\t\t\t\tdefault_argv = \"on-demand\";\n> > >> -\t\t\t\t}\n> > >> -\t\t\t}\n> > >> -\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> > >> -\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names,\n> > >> +\t\tswitch (get_fetch_recurse_config(submodule, spf))\n> > >> +\t\t{\n\nI looked through this one more time and I was able to convince myself\nagain that it's doing the same thing. Instead of repeating the same\nlogic over and over again (via copy and paste of code) in deeply nested\nif's, you are first determining what the value of fetch_recurse is and\nthen based on that doing a set of specific things.\n\nOnly nit would be to move this brace onto the previous line :)\n\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\t\tcontinue;\n> > >> \t\t\tdefault_argv = \"on-demand\";\n> > >> +\t\t\tbreak;\n> > >> +\t\tcase RECURSE_SUBMODULES_ON:\n> > >> +\t\t\tdefault_argv = \"yes\";\n> > >> +\t\t\tbreak;\n> > >> +\t\tcase RECURSE_SUBMODULES_OFF:\n> > >> +\t\t\tcontinue;\n> > >> \t\t}\n> > >> \n> > >> \t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n> > >> -- \n> > >> 2.14.1.145.gb3622a4\n> > >> \n\n-- \nBrandon Williams\n"},{"id":"330699","messageId":"CAGZ79kaisP5fGMOvyscz31XOf9HxN6og2MjsZKO_Dpkq5cy=Mg@mail.gmail.com","threadId":"46976","inReplyTo":"xmqqwp3sj7ov.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/3] implement fetching of moved submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T23:34:16Z","receivedAt":"2017-10-19T23:34:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 18, 2017 at 5:35 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> but if we already have a submodule with that name (the most likely\n>>> explanation for its existence is because it started its life there\n>>> and then later moved), and the submodule is bound to a different\n>>> path, then that is a different submodule. Skipping and warning both\n>>> are sensible thing to do.\n>>\n>> Skipping and warning is sensible once we decide to go this way.\n>>\n>> I propose to take a step back and not throw away the information\n>> whether the given string is a name or path, as then we do not have\n>> to warn&skip, but we can treat both correctly.\n>\n> Now either one of us is utterly confused, and I suspect it is me, as\n> I do not see how \"treat both correctly\" could possibly work in the\n> case this code warns and skips.\n>\n> At this point in the flow, we already know that it is not name,\n> because we asked and got a \"Nah, there is no submodule registered in\n> .gitmodules at that path\" from submodule_from_path(). Then we ask\n> submodule_from_name() if there is any submodule registered under the\n> name it would have got if it were added there, and we indeed find\n> one. And that is definitely *not* a submodule we are looking for,\n> because if it were, its .path would have pointed at the path we were\n> using to ask in the first place. The one we originally found at\n> path and are interested in finding out the details is not known to\n> .gitmodules, and the one under that name is not the one that we are\n> intereted in, so fetching from the repository the other one that\n> happens to have the same name but is different from the submodule we\n> are interested in would simply be wrong.\n\nEventually we'd want to also init new submodules on fetch\n(if you use submodule.active to specify the interesting submodules),\nand in that case I would imagine to fetch both submodules.\n\nAs I wrote the code to further improve this series,\nI realized that this is maybe \"good enough\" for now,\nso assume that I have reviewed this series and found it good.\n\n> If we only have path without any .gitmodules entry (hence there is\n> not even URL), how would we proceed from that point on?\n\nOh well, right. We can only offer to keep the existing behavior\nwhich means supporting existing repos in place at that path.\n\nStefan\n\n>\n"},{"id":"330835","messageId":"20171023141259.GB85043@book.hvoigt.net","threadId":"46976","inReplyTo":"20171019181109.27792-2-sbeller@google.com","subject":"Re: [PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-23T14:12:59Z","receivedAt":"2017-10-23T14:13:10Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Oct 19, 2017 at 11:11:09AM -0700, Stefan Beller wrote:\n> Currently when fetching we collect the names of submodules to be fetched\n> in a list. As we also want to support fetching 'gitlinks, that happen to\n> have a repo checked out at the right place', we'll just pretend that these\n> are submodules. We do that by assuming their path is their name. This in\n> turn can yield collisions between the name-namespace and the\n> path-namespace. (See the previous test for a demonstration.)\n> \n> This patch rewrites the code such that we treat the 'real submodule' case\n> differently from the 'gitlink, but ok' case. This introduces a bit\n> of code duplication, but gets rid of the confusing mapping between names\n> and paths.\n> \n> The test is incomplete as the long term vision is not achieved yet.\n> (which would be fetching both the renamed submodule as well as\n> the gitlink thing, putting them in place via e.g. git-pull)\n> \n> Signed-off-by: Stefan Beller \n> ---\n> \n> Heiko,\n> Junio,\n> \n> I assumed the code would ease up a lot more, but now I am undecided if\n> I want to keep arguing as the code is not stopping to be ugly. :)\n\nSo we are basically coming to the same conclusion? :) My previous\nfallback approach basically did the same but with the old architecture\n(without parallel fetch, ...) and was already ugly.\n\nWith the fallback on submodule default names approach we can keep most\nof the old functionality and keep the code that handles that minimal.\n\nSince there is only a small (IMO quite unlikely) cornercase that could\nbreak peoples expectations I would like to have a look whether anyone\neven notices the behavioral change on next or master. If there are\ncomplaints we can still extend and add the two lists.\n\n> The idea is to treat submodule and gitlinks separately, with submodules\n> supporting renames, and gitlinks as a historic artefact.\n> \n> Sorry for the noise about code ugliness.\n\nWhy sorry? For me it is actually interesting to see you basically coming\nto the same conclusions.\n\nCheers Heiko\n"},{"id":"330836","messageId":"20171023141644.GC85043@book.hvoigt.net","threadId":"46976","inReplyTo":"20171019181109.27792-1-sbeller@google.com","subject":"Re: [PATCH 1/2] t5526: check for name/path collision in submodule fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-23T14:16:44Z","receivedAt":"2017-10-23T14:16:54Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Oct 19, 2017 at 11:11:08AM -0700, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> \n> This is just to test the corner case we're discussing.\n> Applies on top of origin/hv/fetch-moved-submodules-on-demand.\n> \n> \n> t/t5526-fetch-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 42 insertions(+)\n> \n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index a552ad4ead..c82d519e06 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -571,6 +571,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n> '\n> \n> test_expect_success \"fetch new commits when submodule got renamed\" '\n> +\ttest_when_finished \"rm -rf downstream_rename\" &&\n> \tgit clone . downstream_rename &&\n> \t(\n> \t\tcd downstream_rename &&\n> @@ -605,4 +606,45 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success \"warn on submodule name/path clash, but new commits fetched in renamed\" '\n> +\ttest_when_finished \"rm -rf downstream_rename\" &&\n> +\tgit clone . downstream_rename &&\n> +\t(\n> +\t\tcd downstream_rename &&\n> +\t\tgit submodule update --init &&\n> +# NEEDSWORK: we omitted --recursive for the submodule update here since\n> +# that does not work. See test 7001 for mv \"moving nested submodules\"\n> +# for details. Once that is fixed we should add the --recursive option\n> +# here.\n> +\t\tgit checkout -b rename &&\n> +\t\tgit mv submodule submodule_renamed &&\n> +\t\t(\n> +\t\t\tcd submodule_renamed &&\n> +\t\t\tgit checkout -b rename_sub &&\n> +\t\t\techo a >a &&\n> +\t\t\tgit add a &&\n> +\t\t\tgit commit -ma &&\n> +\t\t\tgit push origin rename_sub &&\n> +\t\t\tgit rev-parse HEAD >../../expect\n> +\t\t) &&\n> +\t\tgit add submodule_renamed &&\n> +\t\tgit commit -m \"update renamed submodule\" &&\n> +\t\t# produce collision, note that we use no submodule command\n> +\t\tgit clone ../submodule submodule &&\n> +\t\tgit add submodule &&\n\nA small note even though this is not meant for inclusion: This would\nbreak when I start working on teaching 'git add' to set default values\nin .gitmodules when available.\n\nBut I guess I will discover a few other places, when starting that, that\nwill break in the tests anyway.\n\nCheers Heiko\n"},{"id":"330845","messageId":"CAGZ79kaFcPUUx0+tCnBzpssaN0c09eqjiDSbCTWLAVayaw1FOw@mail.gmail.com","threadId":"46976","inReplyTo":"20171023141644.GC85043@book.hvoigt.net","subject":"Re: [PATCH 1/2] t5526: check for name/path collision in submodule fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-23T17:58:21Z","receivedAt":"2017-10-23T17:58:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">> + git add submodule &&\n>\n> A small note even though this is not meant for inclusion: This would\n> break when I start working on teaching 'git add' to set default values\n> in .gitmodules when available.\n\nYes. I should have used something like:\n\n git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub\n\nas that is the real plumbing command to change a gitlink.\n\n>\n> But I guess I will discover a few other places, when starting that, that\n> will break in the tests anyway.\n\nYes, I have the same suspicion. Thanks for reminding\nme to use more plumbing!\n\nStefan\n"},{"id":"330846","messageId":"CAGZ79kYcvcEe2-K94BSB0j3ig-fgr9xvTg5q4H6vak1H6LkE+Q@mail.gmail.com","threadId":"46976","inReplyTo":"20171023141259.GB85043@book.hvoigt.net","subject":"Re: [PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-23T18:05:16Z","receivedAt":"2017-10-23T18:05:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 23, 2017 at 7:12 AM, Heiko Voigt wrote:\n> On Thu, Oct 19, 2017 at 11:11:09AM -0700, Stefan Beller wrote:\n>> Currently when fetching we collect the names of submodules to be fetched\n>> in a list. As we also want to support fetching 'gitlinks, that happen to\n>> have a repo checked out at the right place', we'll just pretend that these\n>> are submodules. We do that by assuming their path is their name. This in\n>> turn can yield collisions between the name-namespace and the\n>> path-namespace. (See the previous test for a demonstration.)\n>>\n>> This patch rewrites the code such that we treat the 'real submodule' case\n>> differently from the 'gitlink, but ok' case. This introduces a bit\n>> of code duplication, but gets rid of the confusing mapping between names\n>> and paths.\n>>\n>> The test is incomplete as the long term vision is not achieved yet.\n>> (which would be fetching both the renamed submodule as well as\n>> the gitlink thing, putting them in place via e.g. git-pull)\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> Heiko,\n>> Junio,\n>>\n>> I assumed the code would ease up a lot more, but now I am undecided if\n>> I want to keep arguing as the code is not stopping to be ugly. :)\n>\n> So we are basically coming to the same conclusion? :) My previous\n> fallback approach basically did the same but with the old architecture\n> (without parallel fetch, ...) and was already ugly.\n\nIt depends on the conclusion you drew. ;)\nHere is my conclusion:\n* It would really be nice to have this fallback separated out.\n* However for the current state the ugliness of such code trumps the\n more maintainable, long term oriented thing with path/names not\n clashing. I could not spend more time polishing these patches,\n so I could not ask you to do it either\n-> I think your patches are fine as is for inclusion\n-> We may have #leftoverbits here to clear up the confusion around\n path/names, as well as making the code more pleasant to read.\n\n> With the fallback on submodule default names approach we can keep most\n> of the old functionality and keep the code that handles that minimal.\n>\n> Since there is only a small (IMO quite unlikely) cornercase that could\n> break peoples expectations I would like to have a look whether anyone\n> even notices the behavioral change on next or master. If there are\n> complaints we can still extend and add the two lists.\n\nThat sounds good to me.\n\n>\n>> The idea is to treat submodule and gitlinks separately, with submodules\n>> supporting renames, and gitlinks as a historic artefact.\n>>\n>> Sorry for the noise about code ugliness.\n>\n> Why sorry? For me it is actually interesting to see you basically coming\n> to the same conclusions.\n\nI thought I might come off awkwardly criticizing code for ugliness without\nhaving a better alternative to show.\n\nThanks for working on this,\nStefan\n\n>\n> Cheers Heiko\n"},{"id":"330878","messageId":"xmqq7evlbc1o.fsf@gitster.mtv.corp.google.com","threadId":"46976","inReplyTo":"20171023141259.GB85043@book.hvoigt.net","subject":"Re: [PATCH 2/2] fetch, push: keep separate lists of submodules and gitlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-24T00:54:27Z","receivedAt":"2017-10-24T00:54:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> Why sorry? For me it is actually interesting to see you basically coming\n> to the same conclusions.\n\nI find it also assuring to see that two people not constantly\nworking together closely come to the same conclusion. Thanks.\n"}]}