{"thread":{"id":"46614","subject":"[RFC PATCH 1/2] implement fetching of moved submodules","startedAt":"2017-08-17T10:53:59Z","lastAt":"2017-09-18T16:49:49Z","messageCount":11,"participants":["Heiko Voigt","Stefan Beller","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"326599","messageId":"20170817105349.GC52233@book.hvoigt.net","threadId":"46614","inReplyTo":null,"subject":"[RFC PATCH 1/2] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-17T10:53:49Z","receivedAt":"2017-08-17T10:53:59Z","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.\n\nWith the change described above we implement 'on-demand' fetching of\nchanges in moved submodules.\n\nNote: This does only work when repositories have a .gitmodules file. In\nother words: It breaks if we do not get a name for a repository.\nIIRC, consensus was that this is a requirement to get nice submodule\nhandling these days?\n\nSigned-off-by: Heiko Voigt \n---\n\nI updated the leftover code from my series implementing recursive fetch\nfor moved submodules[1] to the current master.\n\nThis breaks t5531 and t5545 because they do not use a .gitmodules file.\n\nI also have some code leftover that does fallback on paths in case no\nsubmodule names can be found. But I do not really like it. The question\nhere is how far do we support not using .gitmodules. Is it e.g.\nreasonable to say: \"For --recurse-submodules=on-demand you need a\n.gitmodules file?\"\n\n[1] https://public-inbox.org/git/f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvoigt@hvoigt.net/\n\n submodule.c | 92 +++++++++++++++++++++++++--------------------\n t/t5526-fetch-submodules.sh | 35 +++++++++++++++++\n 2 files changed, 86 insertions(+), 41 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 27de65a..3ed78ac 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -23,7 +23,7 @@\n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n static int parallel_jobs = 1;\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@@ -742,11 +742,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@@ -755,39 +755,34 @@ 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 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+\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\tsubmodule = submodule_from_path(commit_oid, p->two->path);\n+\t\tif (!submodule)\n \t\t\tcontinue;\n-\t\t}\n+\n+\t\tcommits = submodule_commits(changed, submodule->name);\n+\t\toid_array_append(commits, &p->two->oid);\n \t}\n }\n \n@@ -810,11 +805,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@@ -871,6 +869,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n \toid_array_for_each_unique(commits, check_has_commit, &has_commit);\n \n \tif (has_commit) {\n+\n \t\t/*\n \t\t * Even if the submodule is checked out and the commit is\n \t\t * present, make sure it exists in the submodule's object store\n@@ -945,7 +944,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@@ -956,12 +955,16 @@ 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+\n+\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n+\t\tif (!submodule)\n+\t\t\tcontinue;\n \n-\t\tif (submodule_needs_pushing(path, commits))\n-\t\t\tstring_list_insert(needs_pushing, path);\n+\t\tif (submodule_needs_pushing(submodule->path, commits))\n+\t\t\tstring_list_insert(needs_pushing, submodule->path);\n \t}\n \n \tfree_submodules_oids(&submodules);\n@@ -1104,7 +1107,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@@ -1119,16 +1122,20 @@ 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+\n+\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n+\t\tif (!submodule)\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\tif (!submodule_has_commits(submodule->path, commits))\n+\t\t\tstring_list_append(&changed_submodule_names, name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1206,7 +1213,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t\t\tcontinue;\n \t\t\t\tif (submodule->fetch_recurse ==\n \t\t\t\t\t\tRECURSE_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@@ -1215,13 +1223,15 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t\t gitmodules_is_unmerged)\n \t\t\t\t\tcontinue;\n \t\t\t\tif (config_fetch_recurse_submodules == 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@@ -1315,7 +1325,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 162baf1..ce788e9 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -530,4 +530,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.0.0.274.g6b2cd91\n\n"},{"id":"326601","messageId":"20170817110013.GD52233@book.hvoigt.net","threadId":"46614","inReplyTo":"20170817105349.GC52233@book.hvoigt.net","subject":"[RFC PATCH 2/2] submodule: simplify decision tree whether to or not to fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-17T11:00:13Z","receivedAt":"2017-08-17T11:00:22Z","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---\nI am quite sure I replicated the same logic but a few more eyes would be\nappreciated.\n\nCheers Heiko\n\n submodule.c | 55 +++++++++++++++++++++++++++----------------------------\n 1 file changed, 27 insertions(+), 28 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3ed78ac..a1011f4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1171,6 +1171,21 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \treturn ret;\n }\n \n+static int get_fetch_recurse_config(const struct submodule *submodule, int command_line_option)\n+{\n+\tif (command_line_option != RECURSE_SUBMODULES_DEFAULT)\n+\t\treturn command_line_option;\n+\n+\tif (submodule && submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n+\t\t/* local config overrules everything except commandline */\n+\t\treturn submodule->fetch_recurse;\n+\n+\tif (gitmodules_is_unmerged)\n+\t\treturn RECURSE_SUBMODULES_OFF;\n+\n+\treturn config_fetch_recurse_submodules;\n+}\n+\n struct submodule_parallel_fetch {\n \tint count;\n \tstruct argv_array args;\n@@ -1203,37 +1218,21 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (!submodule)\n \t\t\tsubmodule = submodule_from_name(&null_oid, ce->name);\n \n-\t\tdefault_argv = \"yes\";\n-\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n-\t\t\tif (submodule &&\n-\t\t\t submodule->fetch_recurse !=\n-\t\t\t\t\t\tRECURSE_SUBMODULES_NONE) {\n-\t\t\t\tif (submodule->fetch_recurse ==\n-\t\t\t\t\t\tRECURSE_SUBMODULES_OFF)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (submodule->fetch_recurse ==\n-\t\t\t\t\t\tRECURSE_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 ((config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) ||\n-\t\t\t\t gitmodules_is_unmerged)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (config_fetch_recurse_submodules == 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->command_line_option))\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.0.0.274.g6b2cd91\n\n"},{"id":"326608","messageId":"CAGZ79kY41A--4j6bkeKE=vOVVLXDcFc6BhBgz0G-4jQ2bVjyBw@mail.gmail.com","threadId":"46614","inReplyTo":"20170817105349.GC52233@book.hvoigt.net","subject":"Re: [RFC PATCH 1/2] implement fetching of moved submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-17T17:20:13Z","receivedAt":"2017-08-17T17:20:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 17, 2017 at 3:53 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.\n>\n> With the change described above we implement 'on-demand' fetching of\n> changes in moved submodules.\n\nThis sounds as if this would also enable fetching new submodules\neventually?\n\n> Note: This does only work when repositories have a .gitmodules file. In\n> other words: It breaks if we do not get a name for a repository.\n> IIRC, consensus was that this is a requirement to get nice submodule\n> handling these days?\n\nI think that should have been the consensus since ~1.7.8 (since the\nsubmodules git dir can live inside the superprojects\n/module/).\n\nA gitlink entry without corresponding .gitmodules entry is just a gitlink.\nIf we happen to have a repository at that path of the gitlink, we can\nbe nice and pretend like it is a functional submodule, but it really is\nnot. It's just another repo inside the superproject that happen to live\nat the path of a gitlink.\n\n> Signed-off-by: Heiko Voigt \n> ---\n>\n> I updated the leftover code from my series implementing recursive fetch\n> for moved submodules[1] to the current master.\n>\n> This breaks t5531 and t5545 because they do not use a .gitmodules file.\n>\n> I also have some code leftover that does fallback on paths in case no\n> submodule names can be found. But I do not really like it. The question\n> here is how far do we support not using .gitmodules. Is it e.g.\n> reasonable to say: \"For --recurse-submodules=on-demand you need a\n> .gitmodules file?\"\n\nI would not intentionally break users here, but any new functionality can\nsafely assume (a) we have a proper .gitmodules entry or (b) it is not a\nsubmodule, so do nothing/be extra careful.\n\nFor example in recursive diff sort of makes sense to also handle\nnon-submodule gitlinks, but fetch is harder to tell.\n\n(just last night I was rereading\nhttps://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/\nwhich I think is a super cute application of gitlinks. If you happen\nto checkout such\na tree, you don't want to fetch all of the fake submodules)\n\n>\n> [1] https://public-inbox.org/git/f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvoigt@hvoigt.net/\n\nOha, that is from way back in the time. :)\n\n> submodule.c | 92 +++++++++++++++++++++++++--------------------\n> t/t5526-fetch-submodules.sh | 35 +++++++++++++++++\n> 2 files changed, 86 insertions(+), 41 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 27de65a..3ed78ac 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -23,7 +23,7 @@\n> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n> static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n> static int parallel_jobs = 1;\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> @@ -742,11 +742,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> @@ -755,39 +755,34 @@ 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\nHere a comment would be helpful or a more concise variable name.\n(What is changed?)\n\n> + const struct object_id *commit_oid;\n> +};\n> +\n> static void collect_changed_submodules_cb(struct diff_queue_struct *q,\n> struct diff_options *options,\n> void *data)\n> {\n> + struct collect_changed_submodules_cb_data *me = data;\n> + struct string_list *changed = me->changed;\n> + const struct object_id *commit_oid = me->commit_oid;\n> int i;\n> - struct string_list *changed = data;\n>\n> for (i = 0; i < q->nr; i++) {\n> struct diff_filepair *p = q->queue[i];\n> struct oid_array *commits;\n> + const struct submodule *submodule;\n\n(optional style nit, personal opinion, feel free to ignore)\nI personally prefer to not name variables exactly as their type.\nAlso most (all) of the struct submodule uses used 'sub' as\nthe variable name, maybe keep it consistent?\n\n> +\n> if (!S_ISGITLINK(p->two->mode))\n> continue;\n>\n> - if (S_ISGITLINK(p->one->mode)) {\n> - /*\n> - * NEEDSWORK: We should honor the name configured in\n> - * the .gitmodules file of the commit we are examining\n> - * here to be able to correctly follow submodules\n> - * being moved around.\n> - */\n> - commits = submodule_commits(changed, p->two->path);\n> - oid_array_append(commits, &p->two->oid);\n> - } else {\n> - /* Submodule is new or was moved here */\n> - /*\n> - * NEEDSWORK: When the .git directories of submodules\n> - * live inside the superprojects .git directory some\n> - * day we should fetch new submodules directly into\n> - * that location too when config or options request\n> - * that so they can be checked out from there.\n> - */\n> + submodule = submodule_from_path(commit_oid, p->two->path);\n> + if (!submodule)\n> continue;\n> - }\n> +\n> + commits = submodule_commits(changed, submodule->name);\n> + oid_array_append(commits, &p->two->oid);\n> }\n> }\n>\n> @@ -810,11 +805,14 @@ static void collect_changed_submodules(struct string_list *changed,\n>\n> while ((commit = get_revision(&rev))) {\n> struct rev_info diff_rev;\n> + struct collect_changed_submodules_cb_data data;\n> + data.changed = changed;\n> + data.commit_oid = &commit->object.oid;\n>\n> init_revisions(&diff_rev, NULL);\n> diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n> diff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n> - diff_rev.diffopt.format_callback_data = changed;\n> + diff_rev.diffopt.format_callback_data = &data;\n> diff_tree_combined_merge(commit, 1, &diff_rev);\n> }\n>\n> @@ -871,6 +869,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n> oid_array_for_each_unique(commits, check_has_commit, &has_commit);\n>\n> if (has_commit) {\n> +\n\nstray new line\n\n> /*\n> * Even if the submodule is checked out and the commit is\n> * present, make sure it exists in the submodule's object store\n> @@ -945,7 +944,7 @@ int find_unpushed_submodules(struct oid_array *commits,\n> const char *remotes_name, struct string_list *needs_pushing)\n> {\n> struct string_list submodules = STRING_LIST_INIT_DUP;\n> - struct string_list_item *submodule;\n> + struct string_list_item *name;\n> struct argv_array argv = ARGV_ARRAY_INIT;\n>\n> /* argv.argv[0] will be ignored by setup_revisions */\n> @@ -956,12 +955,16 @@ int find_unpushed_submodules(struct oid_array *commits,\n>\n> collect_changed_submodules(&submodules, &argv);\n>\n> - for_each_string_list_item(submodule, &submodules) {\n> - struct oid_array *commits = submodule->util;\n> - const char *path = submodule->string;\n> + for_each_string_list_item(name, &submodules) {\n> + struct oid_array *commits = name->util;\n> + const struct submodule *submodule;\n> +\n> + submodule = submodule_from_name(&null_oid, name->string);\n> + if (!submodule)\n> + continue;\n>\n> - if (submodule_needs_pushing(path, commits))\n> - string_list_insert(needs_pushing, path);\n> + if (submodule_needs_pushing(submodule->path, commits))\n> + string_list_insert(needs_pushing, submodule->path);\n\neventually we can also migrate to name here as well.\nIn a later patch.\n\n> }\n>\n> free_submodules_oids(&submodules);\n> @@ -1104,7 +1107,7 @@ static void calculate_changed_submodule_paths(void)\n> {\n> struct argv_array argv = ARGV_ARRAY_INIT;\n> struct string_list changed_submodules = STRING_LIST_INIT_DUP;\n> - const struct string_list_item *item;\n> + const struct string_list_item *name;\n>\n> /* No need to check if there are no submodules configured */\n> if (!submodule_from_path(NULL, NULL))\n> @@ -1119,16 +1122,20 @@ static void calculate_changed_submodule_paths(void)\n>\n> /*\n> * Collect all submodules (whether checked out or not) for which new\n> - * commits have been recorded upstream in \"changed_submodule_paths\".\n> + * commits have been recorded upstream in \"changed_submodule_names\".\n> */\n> collect_changed_submodules(&changed_submodules, &argv);\n>\n> - for_each_string_list_item(item, &changed_submodules) {\n> - struct oid_array *commits = item->util;\n> - const char *path = item->string;\n> + for_each_string_list_item(name, &changed_submodules) {\n> + struct oid_array *commits = name->util;\n> + const struct submodule *submodule;\n> +\n> + submodule = submodule_from_name(&null_oid, name->string);\n> + if (!submodule)\n> + continue;\n>\n> - if (!submodule_has_commits(path, commits))\n> - string_list_append(&changed_submodule_paths, path);\n> + if (!submodule_has_commits(submodule->path, commits))\n> + string_list_append(&changed_submodule_names, name->string);\n> }\n>\n> free_submodules_oids(&changed_submodules);\n> @@ -1206,7 +1213,8 @@ static int get_next_submodule(struct child_process *cp,\n> continue;\n> if (submodule->fetch_recurse ==\n> RECURSE_SUBMODULES_ON_DEMAND) {\n> - if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n> + if (!unsorted_string_list_lookup(&changed_submodule_names,\n> + submodule->name))\n> continue;\n> default_argv = \"on-demand\";\n> }\n> @@ -1215,13 +1223,15 @@ static int get_next_submodule(struct child_process *cp,\n> gitmodules_is_unmerged)\n> continue;\n> if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {\n> - if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n> + if (!unsorted_string_list_lookup(&changed_submodule_names,\n> + submodule->name))\n> continue;\n> default_argv = \"on-demand\";\n> }\n> }\n> } else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> - if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n> + if (!unsorted_string_list_lookup(&changed_submodule_names,\n> + submodule->name))\n> continue;\n> default_argv = \"on-demand\";\n> }\n> @@ -1315,7 +1325,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n>\n> argv_array_clear(&spf.args);\n> out:\n> - string_list_clear(&changed_submodule_paths, 1);\n> + string_list_clear(&changed_submodule_names, 1);\n> return spf.result;\n> }\n>\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 162baf1..ce788e9 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -530,4 +530,39 @@ test_expect_success 'fetching submodule into a broken repository' '\n> test_must_fail git -C dst fetch --recurse-submodules\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.0.0.274.g6b2cd91\n>\n"},{"id":"326609","messageId":"CAGZ79kZ2oOK_=jdKa9EMMUO8pi+v+f6tn83sSvafO1Jg5YOjew@mail.gmail.com","threadId":"46614","inReplyTo":"20170817110013.GD52233@book.hvoigt.net","subject":"Re: [RFC PATCH 2/2] submodule: simplify decision tree whether to or not to fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-17T17:24:46Z","receivedAt":"2017-08-17T17:24:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 17, 2017 at 4:00 AM, Heiko Voigt wrote:\n> To make extending this logic later easier.\n>\n> Signed-off-by: Heiko Voigt \n> ---\n> I am quite sure I replicated the same logic but a few more eyes would be\n> appreciated.\n\nA code cleanup is appreciated!\n\nI thought Brandon had a series in flight doing a very similar cleanup here,\nbut in master..pu there is nothing to be found.\n\n> Cheers Heiko\n\nThe code looks good to me.\n\nCheers!\nStefan\n\n>\n> submodule.c | 55 +++++++++++++++++++++++++++----------------------------\n> 1 file changed, 27 insertions(+), 28 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 3ed78ac..a1011f4 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1171,6 +1171,21 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n> return ret;\n> }\n>\n> +static int get_fetch_recurse_config(const struct submodule *submodule, int command_line_option)\n> +{\n> + if (command_line_option != RECURSE_SUBMODULES_DEFAULT)\n> + return command_line_option;\n> +\n> + if (submodule && submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n> + /* local config overrules everything except commandline */\n> + return submodule->fetch_recurse;\n> +\n> + if (gitmodules_is_unmerged)\n> + return RECURSE_SUBMODULES_OFF;\n> +\n> + return config_fetch_recurse_submodules;\n> +}\n> +\n> struct submodule_parallel_fetch {\n> int count;\n> struct argv_array args;\n> @@ -1203,37 +1218,21 @@ static int get_next_submodule(struct child_process *cp,\n> if (!submodule)\n> submodule = submodule_from_name(&null_oid, ce->name);\n>\n> - default_argv = \"yes\";\n> - if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> - if (submodule &&\n> - submodule->fetch_recurse !=\n> - RECURSE_SUBMODULES_NONE) {\n> - if (submodule->fetch_recurse ==\n> - RECURSE_SUBMODULES_OFF)\n> - continue;\n> - if (submodule->fetch_recurse ==\n> - RECURSE_SUBMODULES_ON_DEMAND) {\n> - if (!unsorted_string_list_lookup(&changed_submodule_names,\n> - submodule->name))\n> - continue;\n> - default_argv = \"on-demand\";\n> - }\n> - } else {\n> - if ((config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) ||\n> - gitmodules_is_unmerged)\n> - continue;\n> - if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {\n> - if (!unsorted_string_list_lookup(&changed_submodule_names,\n> - submodule->name))\n> - continue;\n> - default_argv = \"on-demand\";\n> - }\n> - }\n> - } else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> - if (!unsorted_string_list_lookup(&changed_submodule_names,\n> + switch (get_fetch_recurse_config(submodule, spf->command_line_option))\n> + {\n> + default:\n> + case RECURSE_SUBMODULES_DEFAULT:\n> + case RECURSE_SUBMODULES_ON_DEMAND:\n> + if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n> submodule->name))\n> continue;\n> default_argv = \"on-demand\";\n> + break;\n> + case RECURSE_SUBMODULES_ON:\n> + default_argv = \"yes\";\n> + break;\n> + case RECURSE_SUBMODULES_OFF:\n> + continue;\n> }\n>\n> strbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n> --\n> 2.0.0.274.g6b2cd91\n>\n"},{"id":"326616","messageId":"20170817175007.GA109680@google.com","threadId":"46614","inReplyTo":"CAGZ79kZ2oOK_=jdKa9EMMUO8pi+v+f6tn83sSvafO1Jg5YOjew@mail.gmail.com","subject":"Re: [RFC PATCH 2/2] submodule: simplify decision tree whether to or not to fetch","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-17T17:50:07Z","receivedAt":"2017-08-17T17:50:22Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/17, Stefan Beller wrote:\n> On Thu, Aug 17, 2017 at 4:00 AM, Heiko Voigt wrote:\n> > To make extending this logic later easier.\n> >\n> > Signed-off-by: Heiko Voigt \n> > ---\n> > I am quite sure I replicated the same logic but a few more eyes would be\n> > appreciated.\n> \n> A code cleanup is appreciated!\n> \n> I thought Brandon had a series in flight doing a very similar cleanup here,\n> but in master..pu there is nothing to be found.\n\nYeah there are 2 series in flight which will probably conflict here.\nbw/grep-recurse-submodules and bw/submodule-config-cleanup\n\n> \n> > Cheers Heiko\n> \n> The code looks good to me.\n> \n> Cheers!\n> Stefan\n> \n> >\n> > submodule.c | 55 +++++++++++++++++++++++++++----------------------------\n> > 1 file changed, 27 insertions(+), 28 deletions(-)\n> >\n> > diff --git a/submodule.c b/submodule.c\n> > index 3ed78ac..a1011f4 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -1171,6 +1171,21 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n> > return ret;\n> > }\n> >\n> > +static int get_fetch_recurse_config(const struct submodule *submodule, int command_line_option)\n> > +{\n> > + if (command_line_option != RECURSE_SUBMODULES_DEFAULT)\n> > + return command_line_option;\n> > +\n> > + if (submodule && submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n> > + /* local config overrules everything except commandline */\n> > + return submodule->fetch_recurse;\n> > +\n> > + if (gitmodules_is_unmerged)\n> > + return RECURSE_SUBMODULES_OFF;\n> > +\n> > + return config_fetch_recurse_submodules;\n> > +}\n> > +\n> > struct submodule_parallel_fetch {\n> > int count;\n> > struct argv_array args;\n> > @@ -1203,37 +1218,21 @@ static int get_next_submodule(struct child_process *cp,\n> > if (!submodule)\n> > submodule = submodule_from_name(&null_oid, ce->name);\n> >\n> > - default_argv = \"yes\";\n> > - if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> > - if (submodule &&\n> > - submodule->fetch_recurse !=\n> > - RECURSE_SUBMODULES_NONE) {\n> > - if (submodule->fetch_recurse ==\n> > - RECURSE_SUBMODULES_OFF)\n> > - continue;\n> > - if (submodule->fetch_recurse ==\n> > - RECURSE_SUBMODULES_ON_DEMAND) {\n> > - if (!unsorted_string_list_lookup(&changed_submodule_names,\n> > - submodule->name))\n> > - continue;\n> > - default_argv = \"on-demand\";\n> > - }\n> > - } else {\n> > - if ((config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) ||\n> > - gitmodules_is_unmerged)\n> > - continue;\n> > - if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {\n> > - if (!unsorted_string_list_lookup(&changed_submodule_names,\n> > - submodule->name))\n> > - continue;\n> > - default_argv = \"on-demand\";\n> > - }\n> > - }\n> > - } else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n> > - if (!unsorted_string_list_lookup(&changed_submodule_names,\n> > + switch (get_fetch_recurse_config(submodule, spf->command_line_option))\n> > + {\n> > + default:\n> > + case RECURSE_SUBMODULES_DEFAULT:\n> > + case RECURSE_SUBMODULES_ON_DEMAND:\n> > + if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n> > submodule->name))\n> > continue;\n> > default_argv = \"on-demand\";\n> > + break;\n> > + case RECURSE_SUBMODULES_ON:\n> > + default_argv = \"yes\";\n> > + break;\n> > + case RECURSE_SUBMODULES_OFF:\n> > + continue;\n> > }\n> >\n> > strbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n> > --\n> > 2.0.0.274.g6b2cd91\n> >\n\n-- \nBrandon Williams\n"},{"id":"326693","messageId":"20170818160640.GC69414@book.hvoigt.net","threadId":"46614","inReplyTo":"20170817175007.GA109680@google.com","subject":"Re: [RFC PATCH 2/2] submodule: simplify decision tree whether to or not to fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-18T16:06:40Z","receivedAt":"2017-08-18T16:20:46Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Aug 17, 2017 at 10:50:07AM -0700, Brandon Williams wrote:\n> On 08/17, Stefan Beller wrote:\n> > On Thu, Aug 17, 2017 at 4:00 AM, Heiko Voigt wrote:\n> > > To make extending this logic later easier.\n> > >\n> > > Signed-off-by: Heiko Voigt \n> > > ---\n> > > I am quite sure I replicated the same logic but a few more eyes would be\n> > > appreciated.\n> > \n> > A code cleanup is appreciated!\n> > \n> > I thought Brandon had a series in flight doing a very similar cleanup here,\n> > but in master..pu there is nothing to be found.\n> \n> Yeah there are 2 series in flight which will probably conflict here.\n> bw/grep-recurse-submodules and bw/submodule-config-cleanup\n\nOk then I will wait until those are in and then see if I can base the\ncleanup on top. I think it is only necessary as a preparation for the\nfully fledged fetch configuration logic mess we will get into once we\nget to the full recursive submodule fetch implementation. Not\nnecessarily needed for the moved submodules.\n\n> > \n> > The code looks good to me.\n\nThanks.\n\nCheers Heiko\n"},{"id":"326696","messageId":"20170818160623.GB69414@book.hvoigt.net","threadId":"46614","inReplyTo":"CAGZ79kY41A--4j6bkeKE=vOVVLXDcFc6BhBgz0G-4jQ2bVjyBw@mail.gmail.com","subject":"Re: [RFC PATCH 1/2] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-18T16:06:23Z","receivedAt":"2017-08-18T16:32:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Aug 17, 2017 at 10:20:13AM -0700, Stefan Beller wrote:\n> On Thu, Aug 17, 2017 at 3:53 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.\n> >\n> > With the change described above we implement 'on-demand' fetching of\n> > changes in moved submodules.\n> \n> This sounds as if this would also enable fetching new submodules\n> eventually?\n\nYes that was the goal when starting with these changes back then. But it\ntook more time than I had back then. So instead of letting these changes\nsit bitrot again lets see if we can get them integrated.\n\nFor new submodules we need to change the iteration somehow. Currently we\nare iterating through the index. But new submodules obviously do not\nhave an index entry (otherwise they would not be new). So instead of the\nindex we will need to create another list that contains \"all\"\nsubmodules. Maybe something like: all submodules from the index plus all\nsubmodules that changed / are new? We could also go further and inspect\nall submodules from all ref tips to handle submodules on other branches\nconfigured to 'yes'. But I think we should leave that for later if need\narises.\n\nSome merge of index and additional submodules is needed, because for\n--recurse-submodules=yes or submodule..fetchRecurseSubmodules=yes\nwe always need to run fetch inside the submodule. That would break if we\nonly looked at submodules that are collected as changed.\n\n> > Note: This does only work when repositories have a .gitmodules file. In\n> > other words: It breaks if we do not get a name for a repository.\n> > IIRC, consensus was that this is a requirement to get nice submodule\n> > handling these days?\n> \n> I think that should have been the consensus since ~1.7.8 (since the\n> submodules git dir can live inside the superprojects\n> /module/).\n\nI agree but since we started without it, we kind of have a mixed state.\n\n> A gitlink entry without corresponding .gitmodules entry is just a gitlink.\n> If we happen to have a repository at that path of the gitlink, we can\n> be nice and pretend like it is a functional submodule, but it really is\n> not. It's just another repo inside the superproject that happen to live\n> at the path of a gitlink.\n\nYeah but at the moment we are handling 'on-demand' fetches and stuff for\nsuch just gitlink submodules. If we were firm on that requirement we\nwould just skip those but that is not the case with the current\nimplementation.\n\n> > Signed-off-by: Heiko Voigt \n> > ---\n> >\n> > I updated the leftover code from my series implementing recursive fetch\n> > for moved submodules[1] to the current master.\n> >\n> > This breaks t5531 and t5545 because they do not use a .gitmodules file.\n> >\n> > I also have some code leftover that does fallback on paths in case no\n> > submodule names can be found. But I do not really like it. The question\n> > here is how far do we support not using .gitmodules. Is it e.g.\n> > reasonable to say: \"For --recurse-submodules=on-demand you need a\n> > .gitmodules file?\"\n> \n> I would not intentionally break users here, but any new functionality can\n> safely assume (a) we have a proper .gitmodules entry or (b) it is not a\n> submodule, so do nothing/be extra careful.\n> \n> For example in recursive diff sort of makes sense to also handle\n> non-submodule gitlinks, but fetch is harder to tell.\n\nWell we have a few different cases for gitlinks without .gitmodule\nentry:\n\n 1. New gitlink: We can not handle since we do not know where to clone\n from.\n\n 2. Removed gitlink: No need to do anything in fetch\n\n 3. Changed (but same name) gitlink: We can / and currently do run fetch\n in it\n\n 4. Renamed: We currently skip those. We could probably do something to\n track the rename and run fetch in case of gitlink changes.\n In my current approach only the ones with a name are\n handled.\n\nSo I guess I will add a fallback to paths for 3. so we do not\nunnecessarily break users using the current implementation.\n\n> (just last night I was rereading\n> https://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/\n> which I think is a super cute application of gitlinks. If you happen\n> to checkout such\n> a tree, you don't want to fetch all of the fake submodules)\n> \n> >\n> > [1] https://public-inbox.org/git/f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvoigt@hvoigt.net/\n> \n> Oha, that is from way back in the time. :)\n\nYeah this code did go through some proper bitrotting :)\n\n> > submodule.c | 92 +++++++++++++++++++++++++--------------------\n> > t/t5526-fetch-submodules.sh | 35 +++++++++++++++++\n> > 2 files changed, 86 insertions(+), 41 deletions(-)\n> >\n> > diff --git a/submodule.c b/submodule.c\n> > index 27de65a..3ed78ac 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -23,7 +23,7 @@\n> > static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n> > static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n> > static int parallel_jobs = 1;\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> > @@ -742,11 +742,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> > @@ -755,39 +755,34 @@ 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> \n> Here a comment would be helpful or a more concise variable name.\n> (What is changed?)\n\nI'll change that to 'changed_submodules' the caller who is passing this\nin called this changed. It is the list of changed submodules to be\nfilled.\n\n> \n> > + const struct object_id *commit_oid;\n\nWhat about this name? It is the commit_oid in the superproject of the\ncurrent revision under investigation. IMO is easy to get confused what\ncommits are referenced superproject or submodule. Maybe\n'super_commit_oid' would be more clear?\n\n> > +};\n> > +\n> > static void collect_changed_submodules_cb(struct diff_queue_struct *q,\n> > struct diff_options *options,\n> > void *data)\n> > {\n> > + struct collect_changed_submodules_cb_data *me = data;\n> > + struct string_list *changed = me->changed;\n> > + const struct object_id *commit_oid = me->commit_oid;\n> > int i;\n> > - struct string_list *changed = data;\n> >\n> > for (i = 0; i < q->nr; i++) {\n> > struct diff_filepair *p = q->queue[i];\n> > struct oid_array *commits;\n> > + const struct submodule *submodule;\n> \n> (optional style nit, personal opinion, feel free to ignore)\n> I personally prefer to not name variables exactly as their type.\n> Also most (all) of the struct submodule uses used 'sub' as\n> the variable name, maybe keep it consistent?\n\nWell I understand that and its similar for me but I personally I do not\nlike abbreviations for variable names since I like to be able to read\ncode natually. So that took precendence over naming submodule\ndifferently than its type here :) Counting submodule vs. sub in\nsubmodule.c I see 4 vs. 5 occurrences... I'll think about it.\n\n> > +\n> > if (!S_ISGITLINK(p->two->mode))\n> > continue;\n> >\n> > - if (S_ISGITLINK(p->one->mode)) {\n> > - /*\n> > - * NEEDSWORK: We should honor the name configured in\n> > - * the .gitmodules file of the commit we are examining\n> > - * here to be able to correctly follow submodules\n> > - * being moved around.\n> > - */\n> > - commits = submodule_commits(changed, p->two->path);\n> > - oid_array_append(commits, &p->two->oid);\n> > - } else {\n> > - /* Submodule is new or was moved here */\n> > - /*\n> > - * NEEDSWORK: When the .git directories of submodules\n> > - * live inside the superprojects .git directory some\n> > - * day we should fetch new submodules directly into\n> > - * that location too when config or options request\n> > - * that so they can be checked out from there.\n> > - */\n> > + submodule = submodule_from_path(commit_oid, p->two->path);\n> > + if (!submodule)\n> > continue;\n> > - }\n> > +\n> > + commits = submodule_commits(changed, submodule->name);\n> > + oid_array_append(commits, &p->two->oid);\n> > }\n> > }\n> >\n> > @@ -810,11 +805,14 @@ static void collect_changed_submodules(struct string_list *changed,\n> >\n> > while ((commit = get_revision(&rev))) {\n> > struct rev_info diff_rev;\n> > + struct collect_changed_submodules_cb_data data;\n> > + data.changed = changed;\n> > + data.commit_oid = &commit->object.oid;\n> >\n> > init_revisions(&diff_rev, NULL);\n> > diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n> > diff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n> > - diff_rev.diffopt.format_callback_data = changed;\n> > + diff_rev.diffopt.format_callback_data = &data;\n> > diff_tree_combined_merge(commit, 1, &diff_rev);\n> > }\n> >\n> > @@ -871,6 +869,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n> > oid_array_for_each_unique(commits, check_has_commit, &has_commit);\n> >\n> > if (has_commit) {\n> > +\n> \n> stray new line\n\nYeah saw that too late. Will fix.\n\n> > /*\n> > * Even if the submodule is checked out and the commit is\n> > * present, make sure it exists in the submodule's object store\n> > @@ -945,7 +944,7 @@ int find_unpushed_submodules(struct oid_array *commits,\n> > const char *remotes_name, struct string_list *needs_pushing)\n> > {\n> > struct string_list submodules = STRING_LIST_INIT_DUP;\n> > - struct string_list_item *submodule;\n> > + struct string_list_item *name;\n> > struct argv_array argv = ARGV_ARRAY_INIT;\n> >\n> > /* argv.argv[0] will be ignored by setup_revisions */\n> > @@ -956,12 +955,16 @@ int find_unpushed_submodules(struct oid_array *commits,\n> >\n> > collect_changed_submodules(&submodules, &argv);\n> >\n> > - for_each_string_list_item(submodule, &submodules) {\n> > - struct oid_array *commits = submodule->util;\n> > - const char *path = submodule->string;\n> > + for_each_string_list_item(name, &submodules) {\n> > + struct oid_array *commits = name->util;\n> > + const struct submodule *submodule;\n> > +\n> > + submodule = submodule_from_name(&null_oid, name->string);\n> > + if (!submodule)\n> > + continue;\n> >\n> > - if (submodule_needs_pushing(path, commits))\n> > - string_list_insert(needs_pushing, path);\n> > + if (submodule_needs_pushing(submodule->path, commits))\n> > + string_list_insert(needs_pushing, submodule->path);\n> \n> eventually we can also migrate to name here as well.\n> In a later patch.\n\nYeah, then it would be possible to also push submodules without a\npopulated worktree.\n\nThere was this other thread about the is-populated-check in\npush_submodules() where it was consensus that it does not make much\nsense but for a maintainer integrating others work it might be useful to\nnot always have all submodules populated.\n\nCheers Heiko\n"},{"id":"328124","messageId":"20170915131853.GA99666@book.hvoigt.net","threadId":"46614","inReplyTo":"20170817105349.GC52233@book.hvoigt.net","subject":"[RFC PATCH v2 1/2] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-09-15T13:18:53Z","receivedAt":"2017-09-15T13:19:03Z","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.\n\nWith the change described above we implement 'on-demand' fetching of\nchanges in moved submodules.\n\nNote: This does only work when repositories have a .gitmodules file. In\nother words: It breaks if we do not get a name for a repository.\nIIRC, consensus was that this is a requirement to get nice submodule\nhandling these days?\n\nNEEDSWORK: This breaks t5531 and t5545 because they do not use a\n.gitmodules file. I will add a fallback to paths to help such users.\n\nSigned-off-by: Heiko Voigt \n---\nThis an update of the previous series[1] to the current master. The\nfallback is still missing but now it should not conflict with any topics\nin flight anymore (hopefully).\n\nCheers Heiko\n\n[1] https://public-inbox.org/git/20170817105349.GC52233@book.hvoigt.net/\n\n submodule.c | 91 +++++++++++++++++++++++++--------------------\n t/t5526-fetch-submodules.sh | 35 +++++++++++++++++\n 2 files changed, 85 insertions(+), 41 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3cea8221e0..38b9905e43 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@@ -667,11 +667,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@@ -680,39 +680,34 @@ 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 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+\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\tsubmodule = submodule_from_path(commit_oid, p->two->path);\n+\t\tif (!submodule)\n \t\t\tcontinue;\n-\t\t}\n+\n+\t\tcommits = submodule_commits(changed, submodule->name);\n+\t\toid_array_append(commits, &p->two->oid);\n \t}\n }\n \n@@ -735,11 +730,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@@ -870,7 +868,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@@ -881,12 +879,16 @@ 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+\n+\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n+\t\tif (!submodule)\n+\t\t\tcontinue;\n \n-\t\tif (submodule_needs_pushing(path, commits))\n-\t\t\tstring_list_insert(needs_pushing, path);\n+\t\tif (submodule_needs_pushing(submodule->path, commits))\n+\t\t\tstring_list_insert(needs_pushing, submodule->path);\n \t}\n \n \tfree_submodules_oids(&submodules);\n@@ -1041,7 +1043,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@@ -1056,16 +1058,20 @@ 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+\n+\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n+\t\tif (!submodule)\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\tif (!submodule_has_commits(submodule->path, commits))\n+\t\t\tstring_list_append(&changed_submodule_names, name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1151,7 +1157,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@@ -1159,13 +1166,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@@ -1258,7 +1267,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 42251f7f3a..22a7358b6f 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -530,4 +530,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":"328126","messageId":"20170915132021.GB99666@book.hvoigt.net","threadId":"46614","inReplyTo":"20170915131853.GA99666@book.hvoigt.net","subject":"[RFC PATCH v2 2/2] submodule: simplify decision tree whether to or not to fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-09-15T13:20:21Z","receivedAt":"2017-09-15T13:20:29Z","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 38b9905e43..fa44fc59f2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1118,6 +1118,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@@ -1137,46 +1162,21 @@ static int get_next_submodule(struct child_process *cp,\n \n \t\tsubmodule = submodule_from_path(&null_oid, ce->name);\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":"328304","messageId":"20170918164709.GB144331@google.com","threadId":"46614","inReplyTo":"20170915132021.GB99666@book.hvoigt.net","subject":"Re: [RFC PATCH v2 2/2] submodule: simplify decision tree whether to or not to fetch","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-09-18T16:47:09Z","receivedAt":"2017-09-18T16:47:16Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/15, Heiko Voigt wrote:\n> To make extending this logic later easier.\n> \n> Signed-off-by: Heiko Voigt \n\nI like the result of this patch, its much cleaner.\n\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 38b9905e43..fa44fc59f2 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1118,6 +1118,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> @@ -1137,46 +1162,21 @@ static int get_next_submodule(struct child_process *cp,\n> \n> \t\tsubmodule = submodule_from_path(&null_oid, ce->name);\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":"328305","messageId":"20170918164940.GC144331@google.com","threadId":"46614","inReplyTo":"20170915131853.GA99666@book.hvoigt.net","subject":"Re: [RFC PATCH v2 1/2] implement fetching of moved submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-09-18T16:49:40Z","receivedAt":"2017-09-18T16:49:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/15, 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.\n> \n> With the change described above we implement 'on-demand' fetching of\n> changes in moved submodules.\n> \n> Note: This does only work when repositories have a .gitmodules file. In\n> other words: It breaks if we do not get a name for a repository.\n> IIRC, consensus was that this is a requirement to get nice submodule\n> handling these days?\n> \n> NEEDSWORK: This breaks t5531 and t5545 because they do not use a\n> .gitmodules file. I will add a fallback to paths to help such users.\n> \n> Signed-off-by: Heiko Voigt \n> ---\n> This an update of the previous series[1] to the current master. The\n> fallback is still missing but now it should not conflict with any topics\n> in flight anymore (hopefully).\n\nSo the idea is to collect changed submodule's name, instead of their\npath, so that if they happened to moved you don't have to worry about\nthe path changing underneath you. This should be good once those tests\nget fixed.\n\nThanks for working on cleaning this up! :)\n\n> \n> Cheers Heiko\n> \n> [1] https://public-inbox.org/git/20170817105349.GC52233@book.hvoigt.net/\n> \n> submodule.c | 91 +++++++++++++++++++++++++--------------------\n> t/t5526-fetch-submodules.sh | 35 +++++++++++++++++\n> 2 files changed, 85 insertions(+), 41 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 3cea8221e0..38b9905e43 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> @@ -667,11 +667,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> @@ -680,39 +680,34 @@ 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> 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> +\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\tsubmodule = submodule_from_path(commit_oid, p->two->path);\n> +\t\tif (!submodule)\n> \t\t\tcontinue;\n> -\t\t}\n> +\n> +\t\tcommits = submodule_commits(changed, submodule->name);\n> +\t\toid_array_append(commits, &p->two->oid);\n> \t}\n> }\n> \n> @@ -735,11 +730,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> @@ -870,7 +868,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> @@ -881,12 +879,16 @@ 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> +\n> +\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n> +\t\tif (!submodule)\n> +\t\t\tcontinue;\n> \n> -\t\tif (submodule_needs_pushing(path, commits))\n> -\t\t\tstring_list_insert(needs_pushing, path);\n> +\t\tif (submodule_needs_pushing(submodule->path, commits))\n> +\t\t\tstring_list_insert(needs_pushing, submodule->path);\n> \t}\n> \n> \tfree_submodules_oids(&submodules);\n> @@ -1041,7 +1043,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> @@ -1056,16 +1058,20 @@ 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> +\n> +\t\tsubmodule = submodule_from_name(&null_oid, name->string);\n> +\t\tif (!submodule)\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\tif (!submodule_has_commits(submodule->path, commits))\n> +\t\t\tstring_list_append(&changed_submodule_names, name->string);\n> \t}\n> \n> \tfree_submodules_oids(&changed_submodules);\n> @@ -1151,7 +1157,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> @@ -1159,13 +1166,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> @@ -1258,7 +1267,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> \n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 42251f7f3a..22a7358b6f 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -530,4 +530,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> -- \n> 2.14.1.145.gb3622a4\n> \n\n-- \nBrandon Williams\n"}]}