{"thread":{"id":"46931","subject":"[RFC PATCH v3 0/4] implement fetching of moved submodules","startedAt":"2017-10-06T22:25:53Z","lastAt":"2017-10-12T16:17:55Z","messageCount":18,"participants":["Heiko Voigt","Stefan Beller","Junio C Hamano","Josh Triplett","Brandon Williams"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"329943","messageId":"20171006222544.GA26642@sandbox","threadId":"46931","inReplyTo":null,"subject":"[RFC PATCH v3 0/4] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-06T22:25:44Z","receivedAt":"2017-10-06T22:25:53Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"The last iteration can be found here:\n\nhttps://public-inbox.org/git/20170817105349.GC52233@book.hvoigt.net/\n\nThis is mainly a status update and to let people know that I am still\nworking on this.\n\nI struggled quite a bit with reviving my original test for the path\nbased recursive fetch (first patch). The behavior seems to haved changed\nand simply setting the submodule configuration in .git/config without\none in .gitmodules does not work anymore. I did not have time to\ninvestigate whether this was a deliberate change or a maybe a bug?\n\nSo the solution for now is that I write my fake configuration (to avoid\nskipping submodule handling altogether) into a .gitmodules file.\n\nThe second patch (cleanup of a submodule push testcase) was written\nbecause that currently is the only test failing. It is not meant for\ninclusion but rather as a demonstration of what might be happening when\nwe cleanup testcases: Because of the behavioral change above, on first\nsight, it seemed like there was a shortcut in fetch and so on-demand\nfetch without submodule configuration would not be supported anymore.\n\nIIRC there were a lot more tests failing before when I implemented my\npatch without the fallback on paths. So my guess is that some tests have\nbeen cleaned up to use proper (.gitmodules) submodule setup.\n\nSo the thing here is: If we want to make sure that we stay backwards\ncompatible by supporting the setup with gitlinks without configuration.\nThen we also should keep tests around that have the plain manual setup\nwithout .gitmodules files. Just something, I think, we should keep in\nmind.\n\nApart from the tests nothing has been added in this iteration. Since I\nfinally have a working test now I will continue with reviving the\nfallback to paths.\n\nCheers Heiko\n\nHeiko Voigt (4):\n  fetch: add test to make sure we stay backwards compatible\n  change submodule push test to use proper repository setup\n  implement fetching of moved submodules\n  submodule: simplify decision tree whether to or not to fetch\n\n submodule.c                    | 155 ++++++++++++++++++++++-------------------\n t/t5526-fetch-submodules.sh    |  77 +++++++++++++++++++-\n t/t5531-deep-submodule-push.sh |  29 ++++----\n 3 files changed, 174 insertions(+), 87 deletions(-)\n\n-- \n2.10.0.129.g35f6318\n\n"},{"id":"329944","messageId":"20171006223047.GB26642@sandbox","threadId":"46931","inReplyTo":"20171006222544.GA26642@sandbox","subject":"[RFC PATCH 1/4] fetch: add test to make sure we stay backwards compatible","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-06T22:30:47Z","receivedAt":"2017-10-06T22:30:55Z","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 <hvoigt@hvoigt.net>\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 42251f7..43a22f6 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.10.0.129.g35f6318\n\n"},{"id":"329945","messageId":"20171006223234.GC26642@sandbox","threadId":"46931","inReplyTo":"20171006222544.GA26642@sandbox","subject":"[RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-06T22:32:34Z","receivedAt":"2017-10-06T22:32:44Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"NOTE: The argument in this message is not correct, see description in\ncover letter.\n\nThe setup of the repositories in this test is using gitlinks without the\n.gitmodules infrastructure. It is however testing convenience features\nlike --recurse-submodules=on-demand. These features are already not\nsupported by fetch without a .gitmodules file. This leads us to the\nconclusion that it is not really used here as well.\n\nLet's use the usual submodule commands to setup the repository in a\ntypical way. This also has the advantage that we are testing with a\nrepository structure that is more similar to one we could expect on a\nusers setup.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n\nAs mentioned in the cover letter. This seems to be the only test that\nensures that we stay compatible with setups without .gitmodules. Maybe\nwe should add/revive some?\n\nCheers Heiko\n\n t/t5531-deep-submodule-push.sh | 29 ++++++++++++++++-------------\n 1 file changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 39cb2c1..a4a2c6a 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -8,22 +8,26 @@ test_expect_success setup '\n \tmkdir pub.git &&\n \tGIT_DIR=pub.git git init --bare &&\n \tGIT_DIR=pub.git git config receive.fsckobjects true &&\n+\tmkdir submodule &&\n+\t(\n+\t\tcd submodule &&\n+\t\tgit init &&\n+\t\tgit config push.default matching &&\n+\t\t>junk &&\n+\t\tgit add junk &&\n+\t\tgit commit -m \"Initial junk\"\n+\t) &&\n+\tgit clone --bare submodule submodule.git &&\n \tmkdir work &&\n \t(\n \t\tcd work &&\n \t\tgit init &&\n \t\tgit config push.default matching &&\n-\t\tmkdir -p gar/bage &&\n-\t\t(\n-\t\t\tcd gar/bage &&\n-\t\t\tgit init &&\n-\t\t\tgit config push.default matching &&\n-\t\t\t>junk &&\n-\t\t\tgit add junk &&\n-\t\t\tgit commit -m \"Initial junk\"\n-\t\t) &&\n-\t\tgit add gar/bage &&\n+\t\tmkdir gar &&\n+\t\tgit submodule add ../submodule.git gar/bage &&\n \t\tgit commit -m \"Initial superproject\"\n+\t\tcd gar/bage &&\n+\t\tgit remote rm origin\n \t)\n '\n \n@@ -51,11 +55,10 @@ test_expect_success 'push if submodule has no remote' '\n \n test_expect_success 'push fails if submodule commit not on remote' '\n \t(\n-\t\tcd work/gar &&\n-\t\tgit clone --bare bage ../../submodule.git &&\n-\t\tcd bage &&\n+\t\tcd work/gar/bage &&\n \t\tgit remote add origin ../../../submodule.git &&\n \t\tgit fetch &&\n+\t\tgit push --set-upstream origin master &&\n \t\t>junk3 &&\n \t\tgit add junk3 &&\n \t\tgit commit -m \"Third junk\"\n-- \n2.10.0.129.g35f6318\n\n"},{"id":"329946","messageId":"20171006223509.GE26642@sandbox","threadId":"46931","inReplyTo":"20171006222544.GA26642@sandbox","subject":"[RFC PATCH v3 4/4] submodule: simplify decision tree whether to or not to fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-06T22:35:09Z","receivedAt":"2017-10-06T22:35:16Z","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 <hvoigt@hvoigt.net>\n---\n\nThis should also be the same as in the previous version.\n\n submodule.c | 74 ++++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 37 insertions(+), 37 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0c586a0..c7b32c6 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1142,6 +1142,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@@ -1161,46 +1186,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.10.0.129.g35f6318\n\n"},{"id":"329947","messageId":"20171006223400.GD26642@sandbox","threadId":"46931","inReplyTo":"20171006222544.GA26642@sandbox","subject":"[RFC PATCH v3 3/4] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-06T22:34:00Z","receivedAt":"2017-10-06T22:50:08Z","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: We now require a name for a submodule repository.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n\nNo changes from the previous version here.\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 63e7094..0c586a0 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,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@@ -742,11 +737,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 +892,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,12 +903,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@@ -1065,7 +1067,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 +1082,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@@ -1175,7 +1181,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 +1190,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 +1291,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 43a22f6..a552ad4 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.10.0.129.g35f6318\n\n"},{"id":"329949","messageId":"CAGZ79kZofg3jS+g0weTdco+PGo_p-_Hd-NScZ=q2UfB7tF2GPA@mail.gmail.com","threadId":"46931","inReplyTo":"20171006222544.GA26642@sandbox","subject":"Re: [RFC PATCH v3 0/4] implement fetching of moved submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-06T22:57:19Z","receivedAt":"2017-10-06T22:57:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 6, 2017 at 3:25 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> The last iteration can be found here:\n>\n> https://public-inbox.org/git/20170817105349.GC52233@book.hvoigt.net/\n>\n> This is mainly a status update and to let people know that I am still\n> working on this.\n\nCool. :)\n\n> I struggled quite a bit with reviving my original test for the path\n> based recursive fetch (first patch). The behavior seems to haved changed\n> and simply setting the submodule configuration in .git/config without\n> one in .gitmodules does not work anymore. I did not have time to\n> investigate whether this was a deliberate change or a maybe a bug?\n\nI think it is deliberate. (We wrote this man page \"gitsubmodules\"[1] and there\nwas so much discussion on \"What is a submodule?\". Key take away is this:\n* a gitlink alone is not a submodule.\n* a submodule consists of at least\n  -> the gitlink in the superproject\n  -> a mapping of path -> name via\n      $(git config -f .gitmodules submodule.<name>.path)\n  -> Depending on config in .git/config or the existence of its git directory,\n      it may be [in]active or [de]initialized.\n\n[1] not to be confused with \"gitmodules\" or \"git-submodule\"\n\nSometimes we accept a plain git-link without the config in .gitmodules,\n(a) due to historic reasons or (b) because it seems sane even for\na repo \"that just happens to exist at the gitlinks location\"\n(example git-diff)\n\n> So the solution for now is that I write my fake configuration (to avoid\n> skipping submodule handling altogether) into a .gitmodules file.\n\nI'll try to spot what is fake about the config.\n\n> The second patch (cleanup of a submodule push testcase) was written\n> because that currently is the only test failing. It is not meant for\n> inclusion but rather as a demonstration of what might be happening when\n> we cleanup testcases: Because of the behavioral change above, on first\n> sight, it seemed like there was a shortcut in fetch and so on-demand\n> fetch without submodule configuration would not be supported anymore.\n>\n> IIRC there were a lot more tests failing before when I implemented my\n> patch without the fallback on paths. So my guess is that some tests have\n> been cleaned up to use proper (.gitmodules) submodule setup.\n\nI don't remember any large recent activity for submodule things lately.\n\n> So the thing here is: If we want to make sure that we stay backwards\n> compatible by supporting the setup with gitlinks without configuration.\n> Then we also should keep tests around that have the plain manual setup\n> without .gitmodules files. Just something, I think, we should keep in\n> mind.\n\nBut do we want this?\n\nWithout the name<->path mapping, we can only have the \"old style\"\nsubmodules, that have their git repo inside its tree instead of inside\nthe superprojects git dir.\nSo renaming/moving \"old style with no name<->path mapping\" will not\nwork. (That may be an acceptable trade off. But then again, just providing\nthe mapping, such that the superproject can absorb the git directory\nof the submodule for this use case, doesn't seem like a big deal to me.\nSomething you want to have anyway, for ease of use of the superproject\nw.r.t. cloning for example)\n\nSo while I do not try to deliberately break these old behaviors, I'd rather\nwant us to go forward with a saner model than \"if we happen to have\nenough data around, the operation succeeds\", i.e. ignore anything\nthat is not following the rather strict definition of a submodule.\n\nFYI: Once upon a time I found \"fake submodules\"\nhttp://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb\nLast time this was discussed on list, this was considered a bug not\nworth fixing instead of a feature IIRC. (Personally I think this is\na rather cool hack, which we may want to abuse ourselves for\nthings like \"convert a subtree into a submodule and back again\",\nbut we could also go without this hack)\n\n> Apart from the tests nothing has been added in this iteration. Since I\n> finally have a working test now I will continue with reviving the\n> fallback to paths.\n\nI'll have a look.\n\nCheers,\nStefan\n"},{"id":"329955","messageId":"xmqqa813vjfk.fsf@gitster.mtv.corp.google.com","threadId":"46931","inReplyTo":"20171006222544.GA26642@sandbox","subject":"Re: [RFC PATCH v3 0/4] implement fetching of moved submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-07T01:24:47Z","receivedAt":"2017-10-07T01:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> So the thing here is: If we want to make sure that we stay backwards\n> compatible by supporting the setup with gitlinks without configuration.\n> Then we also should keep tests around that have the plain manual setup\n> without .gitmodules files. Just something, I think, we should keep in\n> mind.\n>\n> Apart from the tests nothing has been added in this iteration. Since I\n> finally have a working test now I will continue with reviving the\n> fallback to paths.\n\nThanks for an update.\n"},{"id":"330047","messageId":"CAGZ79kZqaC-hFAa3dc7_j8Ah94Ua0+sAjcDUYBL0N-C_J4Bx4A@mail.gmail.com","threadId":"46931","inReplyTo":"20171006223234.GC26642@sandbox","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-09T18:20:51Z","receivedAt":"2017-10-09T18:20:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 6, 2017 at 3:32 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> NOTE: The argument in this message is not correct, see description in\n> cover letter.\n>\n> The setup of the repositories in this test is using gitlinks without the\n> .gitmodules infrastructure. It is however testing convenience features\n> like --recurse-submodules=on-demand. These features are already not\n> supported by fetch without a .gitmodules file. This leads us to the\n> conclusion that it is not really used here as well.\n>\n> Let's use the usual submodule commands to setup the repository in a\n> typical way. This also has the advantage that we are testing with a\n> repository structure that is more similar to one we could expect on a\n> users setup.\n>\n> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n> ---\n>\n> As mentioned in the cover letter. This seems to be the only test that\n> ensures that we stay compatible with setups without .gitmodules. Maybe\n> we should add/revive some?\n\nAn interesting discussion covering this topic is found at\nhttps://public-inbox.org/git/20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net/\n"},{"id":"330111","messageId":"20171010130335.GB75189@book.hvoigt.net","threadId":"46931","inReplyTo":"CAGZ79kZqaC-hFAa3dc7_j8Ah94Ua0+sAjcDUYBL0N-C_J4Bx4A@mail.gmail.com","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-10T13:03:35Z","receivedAt":"2017-10-10T13:03:45Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Mon, Oct 09, 2017 at 11:20:51AM -0700, Stefan Beller wrote:\n> On Fri, Oct 6, 2017 at 3:32 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> > NOTE: The argument in this message is not correct, see description in\n> > cover letter.\n> >\n> > The setup of the repositories in this test is using gitlinks without the\n> > .gitmodules infrastructure. It is however testing convenience features\n> > like --recurse-submodules=on-demand. These features are already not\n> > supported by fetch without a .gitmodules file. This leads us to the\n> > conclusion that it is not really used here as well.\n> >\n> > Let's use the usual submodule commands to setup the repository in a\n> > typical way. This also has the advantage that we are testing with a\n> > repository structure that is more similar to one we could expect on a\n> > users setup.\n> >\n> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n> > ---\n> >\n> > As mentioned in the cover letter. This seems to be the only test that\n> > ensures that we stay compatible with setups without .gitmodules. Maybe\n> > we should add/revive some?\n> \n> An interesting discussion covering this topic is found at\n> https://public-inbox.org/git/20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net/\n\nThanks for that pointer. So in that discussion Junio said that the\nrecursive operations should succeed if we have everything necessary at\nhand. I kind of agree because why should we limit usage when not\nnecessary. On the other hand we want git to be easy to use. And that\nexample from Peff is perfect as a demonstration of a incosistency we\ncurrently have:\n\ngit clone git://some.where.git/submodule.git\ngit add submodule\n\nis an operation I remember, I did, when first getting in contact with\nsubmodules (many years back), since that is one intuitive way. And the\nthing is: It works, kind of... Only later I discovered that one actually\nneeds to us a special submodule command to get everything approriately\nsetup to work together with others.\n\nIf everyone agrees that submodules are the default way of handling\nrepositories insided repositories, IMO, 'git add' should also alter\n.gitmodules by default. We could provide a switch to avoid doing that.\n\nAn intermediate solution would be to warn but in the long run my goal\nfor submodules is and always was: Make them behave as close to files as\npossible. And why should a 'git add submodule' not magically do\neverything it can to make submodules just work? I can look into a patch\nfor that if people agree here...\n\nRegarding handling of gitlinks with or without .gitmodules:\n\nCurrently we are actually in some intermediate state:\n\n * If there is no .gitmodules file: No submodule processing on any\n   gitlinks (AFAIK)\n * If there is a .gitmodules files with some submodule configured: Do\n   recursive fetch and push as far as possible on gitlinks.\n\nSo I am not sure whether there are actually many users (knowingly)\nusing a mix of some submodules configured and some not and then relying\non the submodule infrastructure.\n\nI would rather expect two sorts of users:\n\n  1. Those that do use .gitmodules\n\n  2. Those that do *not* use .gitmodules\n\nUsers that do not use any .gitmodules file will currently (AFAIK) not\nget any submodule handling. So the question is are there really many\n\"mixed users\"? My guess would be no.\nBecause without those using this mixed we could switch to saying: \"You\nneed to have a .gitmodules file for submodule handling\" without much\nfallout from breaking users use cases.\n\nMaybe we can test this out somehow? My patch series would be ready in\nthat case, just had to drop the first patch and adjust the commit\nmessage of this one.\n\nCheers Heiko\n"},{"id":"330131","messageId":"CAGZ79kZFtMxD8wf59SViOOc_mrhwTVr6v0ucAePp+-8hg_im-Q@mail.gmail.com","threadId":"46931","inReplyTo":"20171010130335.GB75189@book.hvoigt.net","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-10T18:39:21Z","receivedAt":"2017-10-10T18:39:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n\n>> > As mentioned in the cover letter. This seems to be the only test that\n>> > ensures that we stay compatible with setups without .gitmodules. Maybe\n>> > we should add/revive some?\n>>\n>> An interesting discussion covering this topic is found at\n>> https://public-inbox.org/git/20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net/\n>\n> Thanks for that pointer. So in that discussion Junio said that the\n> recursive operations should succeed if we have everything necessary at\n> hand. I kind of agree because why should we limit usage when not\n> necessary. On the other hand we want git to be easy to use. And that\n> example from Peff is perfect as a demonstration of a incosistency we\n> currently have:\n>\n> git clone git://some.where.git/submodule.git\n> git add submodule\n>\n> is an operation I remember, I did, when first getting in contact with\n> submodules (many years back), since that is one intuitive way. And the\n> thing is: It works, kind of... Only later I discovered that one actually\n> needs to us a special submodule command to get everything approriately\n> setup to work together with others.\n\nI agree that we ought to not block off users \"because we can\", but rather\nperform the operation if possible with the data at hand.\n\nNote that the result of the discussion `jk/warn-add-gitlink actually`\nwarns about adding a raw gitlink now, such that we hint at using\n\"git submodule add\", directly.\n\nSo you propose to make git-add behave like \"git submodule add\"\n(i.e. also add the .gitmodules entry for name/path/URL), which I\nlike from a submodule perspective.\n\nHowever other users of gitlinks might be confused[1], which is why\nI refrained from \"making every gitlink into a submodule\". Specifically\nthe more powerful a submodule operation is (the more fluff adds),\nthe harder it should be for people to mis-use it.\n\n[1] https://github.com/git-series/git-series/blob/master/INTERNALS.md\n     \"git-series uses gitlinks to store pointer to commits in its own repo.\"\n\n> If everyone agrees that submodules are the default way of handling\n> repositories insided repositories, IMO, 'git add' should also alter\n> .gitmodules by default. We could provide a switch to avoid doing that.\n\nI wonder if that switch should be default-on (i.e. not treat a gitlink as\na submodule initially, behavior as-is, and then eventually we will\ndie() on unconfigured repos, expecting the user to make the decision)\n\n> An intermediate solution would be to warn\n\nThat is already implemented by Peff.\n\n> but in the long run my goal\n> for submodules is and always was: Make them behave as close to files as\n> possible. And why should a 'git add submodule' not magically do\n> everything it can to make submodules just work? I can look into a patch\n> for that if people agree here...\n\nI'd love to see this implemented. I cc'd Josh (the author of git-series), who\nmay disagree with this, or has some good input how to go forward without\nbreaking git-series.\n\n> Regarding handling of gitlinks with or without .gitmodules:\n>\n> Currently we are actually in some intermediate state:\n>\n>  * If there is no .gitmodules file: No submodule processing on any\n>    gitlinks (AFAIK)\n\nAFAIK this is true.\n\n>  * If there is a .gitmodules files with some submodule configured: Do\n>    recursive fetch and push as far as possible on gitlinks.\n\n* If submodule.recurse is set, then we also treat submodules like files\n  for checkout, reset, read-tree.\n\n> So I am not sure whether there are actually many users (knowingly)\n> using a mix of some submodules configured and some not and then relying\n> on the submodule infrastructure.\n>\n> I would rather expect two sorts of users:\n>\n>   1. Those that do use .gitmodules\n\nThose want to reap all benefits of good submodules.\n\n>\n>   2. Those that do *not* use .gitmodules\n\nAs said above, we don't know if those users are\n\"holding submodules wrong\" or are using gitlinks for\nmagic tricks (unrelated to submodules).\n\n>\n> Users that do not use any .gitmodules file will currently (AFAIK) not\n> get any submodule handling. So the question is are there really many\n> \"mixed users\"? My guess would be no.\n\nI hope that there are few (if any) users of these mixed setups.\n\n> Because without those using this mixed we could switch to saying: \"You\n> need to have a .gitmodules file for submodule handling\" without much\n> fallout from breaking users use cases.\n\nThat seems reasonable to me, actually.\n\n> Maybe we can test this out somehow? My patch series would be ready in\n> that case, just had to drop the first patch and adjust the commit\n> message of this one.\n\nI wonder how we would test this, though? Do you have any idea\n(even vague) how we'd accomplish such a measurement?\nI fear we'll have to go this way blindly.\n\nCheers,\nStefan\n\n>\n> Cheers Heiko\n"},{"id":"330140","messageId":"xmqq7ew2pokm.fsf@gitster.mtv.corp.google.com","threadId":"46931","inReplyTo":"CAGZ79kZFtMxD8wf59SViOOc_mrhwTVr6v0ucAePp+-8hg_im-Q@mail.gmail.com","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T23:31:37Z","receivedAt":"2017-10-10T23:31:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> So you propose to make git-add behave like \"git submodule add\"\n> (i.e. also add the .gitmodules entry for name/path/URL), which I\n> like from a submodule perspective.\n>\n> However other users of gitlinks might be confused[1], which is why\n> I refrained from \"making every gitlink into a submodule\". Specifically\n> the more powerful a submodule operation is (the more fluff adds),\n> the harder it should be for people to mis-use it.\n\nA few questions that come to mind are:\n\n - Does \"git add sub/\" have enough information to populate\n   .gitmodules?  If we have reasonable \"default\" values for\n   .gitmodules entries (e.g. missing URL means we won't fetch when\n   asked to go recursively fetch), perhaps we can leave everything\n   other than \"submodule.$name.path\" undefined.\n\n - Can't we help those who have gitlinks without .gitmodules entries\n   exactly the same way as above, i.e. when we see a gitlink and try\n   to treat it as a submodule, we'd first try to look it up from\n   .gitmodules (by going from path to name and then to\n   submodule.$name.$var); the above \"'git add sub/' would add an\n   entry for .gitmodules\" wish is based on the assumption that there\n   are reasonable \"default\" values for each of these $var--so by\n   basing on the same assumption, we can \"pretend\" as if these\n   submodule.$name.$var were in .gitmodules file when we see\n   gitlinks without .gitmodules entries.  IOW, if \"git add sub/\" can\n   add .gitmodules to help people without having to type \"git\n   submodule add sub/\", then we can give exactly the same degree of\n   help without even modifying .gitmodules when \"git add sub/\" is\n   run.\n\n - Even if we could solve it with \"git add sub/\" that adds to\n   .gitmodules, is it a good solution, when we can solve the same\n   thing without having to do so?\n\n\n\n"},{"id":"330141","messageId":"CAGZ79kaqAi2-2KfQqqW1TvBvmHb_13gjZSycY2GsVgakLWcxFw@mail.gmail.com","threadId":"46931","inReplyTo":"xmqq7ew2pokm.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-10T23:41:22Z","receivedAt":"2017-10-10T23:41:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 10, 2017 at 4:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> So you propose to make git-add behave like \"git submodule add\"\n>> (i.e. also add the .gitmodules entry for name/path/URL), which I\n>> like from a submodule perspective.\n>>\n>> However other users of gitlinks might be confused[1], which is why\n>> I refrained from \"making every gitlink into a submodule\". Specifically\n>> the more powerful a submodule operation is (the more fluff adds),\n>> the harder it should be for people to mis-use it.\n>\n> A few questions that come to mind are:\n>\n>  - Does \"git add sub/\" have enough information to populate\n>    .gitmodules?  If we have reasonable \"default\" values for\n>    .gitmodules entries (e.g. missing URL means we won't fetch when\n>    asked to go recursively fetch), perhaps we can leave everything\n>    other than \"submodule.$name.path\" undefined.\n\nI think we would want to populate path and URL only.\n\n>\n>  - Can't we help those who have gitlinks without .gitmodules entries\n>    exactly the same way as above, i.e. when we see a gitlink and try\n>    to treat it as a submodule, we'd first try to look it up from\n>    .gitmodules (by going from path to name and then to\n>    submodule.$name.$var); the above \"'git add sub/' would add an\n>    entry for .gitmodules\" wish is based on the assumption that there\n>    are reasonable \"default\" values for each of these $var--so by\n>    basing on the same assumption, we can \"pretend\" as if these\n>    submodule.$name.$var were in .gitmodules file when we see\n>    gitlinks without .gitmodules entries.  IOW, if \"git add sub/\" can\n>    add .gitmodules to help people without having to type \"git\n>    submodule add sub/\", then we can give exactly the same degree of\n>    help without even modifying .gitmodules when \"git add sub/\" is\n>    run.\n\nI do not understand the gist of this paragraph, other then:\n\n  \"When git-add <repository> encounters a section submodule.<name>.*,\n   do not modify it; We can assume it is sane already.\"\n\n>  - Even if we could solve it with \"git add sub/\" that adds to\n>    .gitmodules, is it a good solution, when we can solve the same\n>    thing without having to do so?\n\nI am confused even more.\n\nSo you suggest that \"git add [--gitlink=submodule]\" taking on the\nresponsibilities of \"git submodule add\" is a bad idea?\n\nI thought we had the same transition from \"git remote update\" to\n\"git fetch\", which eventually superseded the former.\n"},{"id":"330142","messageId":"xmqq376qpmcn.fsf@gitster.mtv.corp.google.com","threadId":"46931","inReplyTo":"xmqq7ew2pokm.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-11T00:19:36Z","receivedAt":"2017-10-11T00:19:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> A few questions that come to mind are:\n>\n>  - Does \"git add sub/\" have enough information to populate\n>    .gitmodules?  If we have reasonable \"default\" values for\n>    .gitmodules entries (e.g. missing URL means we won't fetch when\n>    asked to go recursively fetch), perhaps we can leave everything\n>    other than \"submodule.$name.path\" undefined.\n> ...\n>  - ...  IOW, if \"git add sub/\" can\n>    add .gitmodules to help people without having to type \"git\n>    submodule add sub/\", then we can give exactly the same degree of\n>    help without even modifying .gitmodules when \"git add sub/\" is\n>    run.\n\nAnswering my own questions (aka correcting my own stupidity), there\nis a big leap/gap between the two that came from my forgetting an\nimportant point: a local repository has a lot richer information\nthan others that are clones of it.\n\n\"git add sub/\" could look at sub/.git/config and use that\ninformation when considering what values to populate .gitmodules\nwith.  It can learn where its origin remote is, for example.\n\nAnd while this can do that at look-up time locally (i.e. removing\nthe need to do .gitmodules), those who pull from this local\nrepository, of those who pull from a shared central repository this\nlocal repository pushes into, will not have the same information\navailable to them, _unless_ this local repository records it in the\n.gitmodules file for them to use.\n\nSo, I think \"git add sub/\" that adds to .gitmodules would work\n(unless the sub/ repository originates locally without pushing\nout--in which case, submodule.$name.url cannot be populated with a\nvalue suitable for other people, and we should continue warning),\nwhile doing the same at look-up time would not be a good solution.\n"},{"id":"330180","messageId":"20171011145212.GA85076@book.hvoigt.net","threadId":"46931","inReplyTo":"CAGZ79kZFtMxD8wf59SViOOc_mrhwTVr6v0ucAePp+-8hg_im-Q@mail.gmail.com","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-11T14:52:12Z","receivedAt":"2017-10-11T14:52:22Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Oct 10, 2017 at 11:39:21AM -0700, Stefan Beller wrote:\n> So you propose to make git-add behave like \"git submodule add\"\n> (i.e. also add the .gitmodules entry for name/path/URL), which I\n> like from a submodule perspective.\n\nWell more like: clone and add will behave like \"git submodule add\" but\nbasically yes.\n\n> However other users of gitlinks might be confused[1], which is why\n> I refrained from \"making every gitlink into a submodule\". Specifically\n> the more powerful a submodule operation is (the more fluff adds),\n> the harder it should be for people to mis-use it.\n> \n> [1] https://github.com/git-series/git-series/blob/master/INTERNALS.md\n>      \"git-series uses gitlinks to store pointer to commits in its own repo.\"\n\nBut would those users use\n\n    git add\n\nto add a gitlink? From the description in that file I read that it\npoints to commits in its own repository. Will there also be files\nchecked out like submodules at that location?\n\nOtherwise I would propose that 'git add' could detect whether a gitlink\nis a submodule by trying to read its git configuration. If we do not\nfind that we simply do not do anything.\n\n> > If everyone agrees that submodules are the default way of handling\n> > repositories insided repositories, IMO, 'git add' should also alter\n> > .gitmodules by default. We could provide a switch to avoid doing that.\n> \n> I wonder if that switch should be default-on (i.e. not treat a gitlink as\n> a submodule initially, behavior as-is, and then eventually we will\n> die() on unconfigured repos, expecting the user to make the decision)\n> \n> > An intermediate solution would be to warn\n> \n> That is already implemented by Peff.\n\nAh ok, thanks I suspected so when I realized that this discussion was\nolder.\n\n> > but in the long run my goal\n> > for submodules is and always was: Make them behave as close to files as\n> > possible. And why should a 'git add submodule' not magically do\n> > everything it can to make submodules just work? I can look into a patch\n> > for that if people agree here...\n> \n> I'd love to see this implemented. I cc'd Josh (the author of git-series), who\n> may disagree with this, or has some good input how to go forward without\n> breaking git-series.\n\nYeah, lets see if, as described above, that actually would break\ngit-series.\n\n> > Regarding handling of gitlinks with or without .gitmodules:\n> >\n> > Currently we are actually in some intermediate state:\n> >\n> >  * If there is no .gitmodules file: No submodule processing on any\n> >    gitlinks (AFAIK)\n> \n> AFAIK this is true.\n> \n> >  * If there is a .gitmodules files with some submodule configured: Do\n> >    recursive fetch and push as far as possible on gitlinks.\n> \n> * If submodule.recurse is set, then we also treat submodules like files\n>   for checkout, reset, read-tree.\n\nTo clarify: If submodule.recurse is set but there is no .gitmodules file\nwe do submodule processing for the above commands?\n\n> > So I am not sure whether there are actually many users (knowingly)\n> > using a mix of some submodules configured and some not and then relying\n> > on the submodule infrastructure.\n> >\n> > I would rather expect two sorts of users:\n> >\n> >   1. Those that do use .gitmodules\n> \n> Those want to reap all benefits of good submodules.\n> \n> >\n> >   2. Those that do *not* use .gitmodules\n> \n> As said above, we don't know if those users are\n> \"holding submodules wrong\" or are using gitlinks for\n> magic tricks (unrelated to submodules).\n\nI did not want to say that they are \"holding submodules wrong\" but\nrather that if they do not use .gitmodules they do that knowingly and\nthus consistently not use .gitmodules for any gitlink.\n\n> > Users that do not use any .gitmodules file will currently (AFAIK) not\n> > get any submodule handling. So the question is are there really many\n> > \"mixed users\"? My guess would be no.\n> \n> I hope that there are few (if any) users of these mixed setups.\n\nThat sounds promising.\n\n> > Because without those using this mixed we could switch to saying: \"You\n> > need to have a .gitmodules file for submodule handling\" without much\n> > fallout from breaking users use cases.\n> \n> That seems reasonable to me, actually.\n\nNice.\n\n> > Maybe we can test this out somehow? My patch series would be ready in\n> > that case, just had to drop the first patch and adjust the commit\n> > message of this one.\n> \n> I wonder how we would test this, though? Do you have any idea\n> (even vague) how we'd accomplish such a measurement?\n> I fear we'll have to go this way blindly.\n\nOne idea would be to expose this somewhere to a limited amount of users.\nI remember Jonathan was suggesting, back when Jens was working on the\nrecursive checkout, that he could add the series to the debian package\nand see what happens. Or we could use Junios next branch? Something like\nthat. If we get complaints we know the assumption was wrong and we need\na fallback.\n\nCheers Heiko\n"},{"id":"330181","messageId":"20171011145657.GB85076@book.hvoigt.net","threadId":"46931","inReplyTo":"xmqq7ew2pokm.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-10-11T14:56:57Z","receivedAt":"2017-10-11T14:57:07Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Oct 11, 2017 at 08:31:37AM +0900, Junio C Hamano wrote:\n> Stefan Beller <sbeller@google.com> writes:\n> \n> > So you propose to make git-add behave like \"git submodule add\"\n> > (i.e. also add the .gitmodules entry for name/path/URL), which I\n> > like from a submodule perspective.\n> >\n> > However other users of gitlinks might be confused[1], which is why\n> > I refrained from \"making every gitlink into a submodule\". Specifically\n> > the more powerful a submodule operation is (the more fluff adds),\n> > the harder it should be for people to mis-use it.\n> \n> A few questions that come to mind are:\n> \n>  - Does \"git add sub/\" have enough information to populate\n>    .gitmodules?  If we have reasonable \"default\" values for\n>    .gitmodules entries (e.g. missing URL means we won't fetch when\n>    asked to go recursively fetch), perhaps we can leave everything\n>    other than \"submodule.$name.path\" undefined.\n\nMy suggestion would be: If we do not have them we do not populate them.\nWe could even go further and say: If we do not have the set \"git\nsubmodule add\" would populate then we do not add anything to .gitmodules\nand warn the user.\n\n>  - Can't we help those who have gitlinks without .gitmodules entries\n>    exactly the same way as above, i.e. when we see a gitlink and try\n>    to treat it as a submodule, we'd first try to look it up from\n>    .gitmodules (by going from path to name and then to\n>    submodule.$name.$var); the above \"'git add sub/' would add an\n>    entry for .gitmodules\" wish is based on the assumption that there\n>    are reasonable \"default\" values for each of these $var--so by\n>    basing on the same assumption, we can \"pretend\" as if these\n>    submodule.$name.$var were in .gitmodules file when we see\n>    gitlinks without .gitmodules entries.  IOW, if \"git add sub/\" can\n>    add .gitmodules to help people without having to type \"git\n>    submodule add sub/\", then we can give exactly the same degree of\n>    help without even modifying .gitmodules when \"git add sub/\" is\n>    run.\n\nThis \"default\" value thing got me thinking in a different direction. We\ncould use a scheme like that to get names (and values) for submodules\nthat are missing from the .gitmodules file. If we decide that we need to\nhandle them.\n\nCheers Heiko\n"},{"id":"330182","messageId":"20171011151021.o6f4l7kcd3azdmiu@x","threadId":"46931","inReplyTo":"CAGZ79kZFtMxD8wf59SViOOc_mrhwTVr6v0ucAePp+-8hg_im-Q@mail.gmail.com","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2017-10-11T15:10:23Z","receivedAt":"2017-10-11T15:10:36Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Tue, Oct 10, 2017 at 11:39:21AM -0700, Stefan Beller wrote:\n> On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> > but in the long run my goal\n> > for submodules is and always was: Make them behave as close to files as\n> > possible. And why should a 'git add submodule' not magically do\n> > everything it can to make submodules just work? I can look into a patch\n> > for that if people agree here...\n> \n> I'd love to see this implemented. I cc'd Josh (the author of git-series), who\n> may disagree with this, or has some good input how to go forward without\n> breaking git-series.\n\ngit-series doesn't use the git-submodule command at all, nor does it\nconstruct series trees using git-add or any other git command-line tool;\nit constructs gitlinks directly. Most of the time, it doesn't even make\nsense to `git checkout` a series branch. Modifying commands like git-add\nand similar to automatically manage .gitmodules won't cause any issue at\nall, as long as git itself doesn't start rejecting or complaining about\nrepositories that have gitlinks without a .gitmodules file.\n\n- Josh Triplett\n"},{"id":"330216","messageId":"xmqq7ew143f0.fsf@gitster.mtv.corp.google.com","threadId":"46931","inReplyTo":"20171011145657.GB85076@book.hvoigt.net","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T00:26:27Z","receivedAt":"2017-10-12T00:26:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> This \"default\" value thing got me thinking in a different direction. We\n> could use a scheme like that to get names (and values) for submodules\n> that are missing from the .gitmodules file. If we decide that we need to\n> handle them.\n\nYes, I suspect that would improve things quite a bit in a repository\nwhere it added a new submodule by filling the gap between the time\nwhen a gitlink is added and an entry in .gitmodules is added.  The\nlatter needs to happen if the result of the work done in that\nrepository is pushed out elsewhere---otherwise it won't usable by\nother people.\n"},{"id":"330290","messageId":"20171012161729.GA169880@google.com","threadId":"46931","inReplyTo":"20171011151021.o6f4l7kcd3azdmiu@x","subject":"Re: [RFC PATCH 2/4] change submodule push test to use proper repository setup","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-12T16:17:29Z","receivedAt":"2017-10-12T16:17:55Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 10/11, Josh Triplett wrote:\n> On Tue, Oct 10, 2017 at 11:39:21AM -0700, Stefan Beller wrote:\n> > On Tue, Oct 10, 2017 at 6:03 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> > > but in the long run my goal\n> > > for submodules is and always was: Make them behave as close to files as\n> > > possible. And why should a 'git add submodule' not magically do\n> > > everything it can to make submodules just work? I can look into a patch\n> > > for that if people agree here...\n> > \n> > I'd love to see this implemented. I cc'd Josh (the author of git-series), who\n> > may disagree with this, or has some good input how to go forward without\n> > breaking git-series.\n> \n> git-series doesn't use the git-submodule command at all, nor does it\n> construct series trees using git-add or any other git command-line tool;\n> it constructs gitlinks directly. Most of the time, it doesn't even make\n> sense to `git checkout` a series branch. Modifying commands like git-add\n> and similar to automatically manage .gitmodules won't cause any issue at\n> all, as long as git itself doesn't start rejecting or complaining about\n> repositories that have gitlinks without a .gitmodules file.\n\nThat's good to know!  And from what I remember, with the commands we've\nbegun teaching to understand submodules we have been requiring a\n.gitmodules entry for a submodule in order to do the recursion, and a\ngitlink without a .gitmodules entry would simply be ignored or skipped.\n\n-- \nBrandon Williams\n"}]}