{"thread":{"id":"49684","subject":"[PATCH 00/10] Resending sb/submodule-recursive-fetch-gets-the-tip","startedAt":"2018-10-25T23:32:39Z","lastAt":"2018-11-29T00:30:50Z","messageCount":19,"participants":["Stefan Beller","Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"361568","messageId":"20181025233231.102245-1-sbeller@google.com","threadId":"49684","inReplyTo":null,"subject":"[PATCH 00/10] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:21Z","receivedAt":"2018-10-25T23:32:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks Jonathan for your thoughtful comments,\nhere is the product of the discussion:\n* I split up the patch to fetch in the worktree to be 2 patches,\n each giving motivation on its own.\n* the last patch is vastly simplified in code, but takes an extra test\n* in [1], you remark \"commits\" ought not to be a pointer, but I decided against\n that, as we keep the pointed-at value around for the same time span (until\n we're done with that submodule) and we don't need to copy over the pointed-at\n value into the new struct.\n\n[1] https://public-inbox.org/git/20181018003954.139498-1-jonathantanmy@google.com/\n\nThis is still based on ao/submodule-wo-gitmodules-checked-out.\nprevious version\nhttps://public-inbox.org/git/20181016181327.107186-1-sbeller@google.com/\n\nStefan Beller (10):\n sha1-array: provide oid_array_filter\n submodule.c: fix indentation\n submodule.c: sort changed_submodule_names before searching it\n submodule.c: tighten scope of changed_submodule_names struct\n submodule: store OIDs in changed_submodule_names\n repository: repo_submodule_init to take a submodule struct\n submodule: migrate get_next_submodule to use repository structs\n submodule.c: fetch in submodules git directory instead of in worktree\n fetch: try fetching submodules if needed objects were not fetched\n builtin/fetch: check for submodule updates in any ref update\n\n Documentation/technical/api-oid-array.txt | 5 +\n builtin/fetch.c | 11 +-\n builtin/grep.c | 17 +-\n builtin/ls-files.c | 12 +-\n builtin/submodule--helper.c | 2 +-\n repository.c | 27 +-\n repository.h | 12 +-\n sha1-array.c | 17 ++\n sha1-array.h | 3 +\n submodule.c | 265 ++++++++++++++++---\n t/helper/test-submodule-nested-repo-config.c | 8 +-\n t/t5526-fetch-submodules.sh | 55 ++++\n 12 files changed, 346 insertions(+), 88 deletions(-)\n\n git range-diff origin/sb/submodule-recursive-fetch-gets-the-tip... \n\n1: 3fbb06aedd ! 1: 0fdb0e2ad9 submodule.c: move global changed_submodule_names into fetch submodule struct\n @@ -1,12 +1,11 @@\n Author: Stefan Beller \n \n - submodule.c: move global changed_submodule_names into fetch submodule struct\n + submodule.c: tighten scope of changed_submodule_names struct\n \n The `changed_submodule_names` are only used for fetching, so let's make it\n part of the struct that is passed around for fetching submodules.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n @@ -16,7 +15,6 @@\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n -static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n -+\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n @@ -25,22 +23,8 @@\n }\n \n -static void calculate_changed_submodule_paths(void)\n -+struct submodule_parallel_fetch {\n -+\tint count;\n -+\tstruct argv_array args;\n -+\tstruct repository *r;\n -+\tconst char *prefix;\n -+\tint command_line_option;\n -+\tint default_option;\n -+\tint quiet;\n -+\tint result;\n -+\n -+\tstruct string_list changed_submodule_names;\n -+};\n -+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n -+\n +static void calculate_changed_submodule_paths(\n -+\tstruct submodule_parallel_fetch *spf)\n ++\tstruct string_list *changed_submodule_names)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n @@ -49,30 +33,23 @@\n \n \t\tif (!submodule_has_commits(path, commits))\n -\t\t\tstring_list_append(&changed_submodule_names, name->string);\n -+\t\t\tstring_list_append(&spf->changed_submodule_names,\n ++\t\t\tstring_list_append(changed_submodule_names,\n +\t\t\t\t\t name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n @@\n - \treturn ret;\n - }\n - \n --struct submodule_parallel_fetch {\n --\tint count;\n --\tstruct argv_array args;\n --\tstruct repository *r;\n --\tconst char *prefix;\n --\tint command_line_option;\n --\tint default_option;\n --\tint quiet;\n --\tint result;\n --};\n + \tint default_option;\n + \tint quiet;\n + \tint result;\n ++\n ++\tstruct string_list changed_submodule_names;\n + };\n -#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}\n --\n ++#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n + \n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t struct submodule_parallel_fetch *spf)\n - {\n @@\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!submodule ||\n @@ -88,7 +65,7 @@\n \n -\tcalculate_changed_submodule_paths();\n -\tstring_list_sort(&changed_submodule_names);\n -+\tcalculate_changed_submodule_paths(&spf);\n ++\tcalculate_changed_submodule_paths(&spf.changed_submodule_names);\n +\tstring_list_sort(&spf.changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n2: a64a8322a1 ! 2: a11e6e39a2 submodule.c: do not copy around submodule list\n @@ -1,6 +1,6 @@\n Author: Stefan Beller \n \n - submodule.c: do not copy around submodule list\n + submodule: store OIDs in changed_submodule_names\n \n 'calculate_changed_submodule_paths' uses a local list to compute the\n changed submodules, and then produces the result by copying appropriate\n @@ -14,13 +14,13 @@\n useful in a later patch.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n + Reviewed-by: Jonathan Tan \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n +++ b/submodule.c\n @@\n - \tstruct submodule_parallel_fetch *spf)\n + \tstruct string_list *changed_submodule_names)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n -\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n @@ -34,10 +34,10 @@\n \t * commits have been recorded upstream in \"changed_submodule_names\".\n \t */\n -\tcollect_changed_submodules(&changed_submodules, &argv);\n -+\tcollect_changed_submodules(&spf->changed_submodule_names, &argv);\n ++\tcollect_changed_submodules(changed_submodule_names, &argv);\n \n -\tfor_each_string_list_item(name, &changed_submodules) {\n -+\tfor_each_string_list_item(name, &spf->changed_submodule_names) {\n ++\tfor_each_string_list_item(name, changed_submodule_names) {\n \t\tstruct oid_array *commits = name->util;\n \t\tconst struct submodule *submodule;\n \t\tconst char *path = NULL;\n @@ -46,7 +46,7 @@\n \t\t\tcontinue;\n \n -\t\tif (!submodule_has_commits(path, commits))\n --\t\t\tstring_list_append(&spf->changed_submodule_names,\n +-\t\t\tstring_list_append(changed_submodule_names,\n -\t\t\t\t\t name->string);\n +\t\tif (submodule_has_commits(path, commits)) {\n +\t\t\toid_array_clear(commits);\n @@ -55,7 +55,7 @@\n \t}\n \n -\tfree_submodules_oids(&changed_submodules);\n -+\tstring_list_remove_empty_items(&spf->changed_submodule_names, 1);\n ++\tstring_list_remove_empty_items(changed_submodule_names, 1);\n +\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n3: 9341b92c83 ! 3: 3f4e0d4b8d repository: repo_submodule_init to take a submodule struct\n @@ -5,17 +5,19 @@\n When constructing a struct repository for a submodule for some revision\n of the superproject where the submodule is not contained in the index,\n it may not be present in the working tree currently either. In that\n - siutation giving a 'path' argument is not useful. Upgrade the\n + situation giving a 'path' argument is not useful. Upgrade the\n repo_submodule_init function to take a struct submodule instead.\n + The submodule struct can be obtained via submodule_from_{path, name} or\n + an artificial submodule struct can be passed in.\n \n - While we are at it, overhaul the repo_submodule_init function by renaming\n - the submodule repository struct, which is to be initialized, to a name\n - that is not confused with the struct submodule as easily.\n + While we are at it, rename the repository struct in the repo_submodule_init\n + function, which is to be initialized, to a name that is not confused with\n + the struct submodule as easily. Perform such renames in similar functions\n + as well.\n \n Also move its documentation into the header file.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/builtin/grep.c b/builtin/grep.c\n --- a/builtin/grep.c\n @@ -104,6 +106,19 @@\n \n static void show_ce(struct repository *repo, struct dir_struct *dir,\n \n +diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n +--- a/builtin/submodule--helper.c\n ++++ b/builtin/submodule--helper.c\n +@@\n + \tif (!sub)\n + \t\tBUG(\"We could get the submodule handle before?\");\n + \n +-\tif (repo_submodule_init(&subrepo, the_repository, path))\n ++\tif (repo_submodule_init(&subrepo, the_repository, sub))\n + \t\tdie(_(\"could not get a repository handle for submodule '%s'\"), path);\n + \n + \tif (!repo_config_get_string(&subrepo, \"core.worktree\", &cw)) {\n +\n diff --git a/repository.c b/repository.c\n --- a/repository.c\n +++ b/repository.c\n @@ -178,7 +193,8 @@\n +/*\n + * Initialize the repository 'subrepo' as the submodule given by the\n + * struct submodule 'sub' in parent repository 'superproject'.\n -+ * Return 0 upon success and a non-zero value upon failure.\n ++ * Return 0 upon success and a non-zero value upon failure, which may happen\n ++ * if the submodule is not found, or 'sub' is NULL.\n + */\n +struct submodule;\n +int repo_submodule_init(struct repository *subrepo,\n4: cea909cbd4 ! 4: b1566069e7 submodule: fetch in submodules git directory instead of in worktree\n @@ -1,44 +1,19 @@\n Author: Stefan Beller \n \n - submodule: fetch in submodules git directory instead of in worktree\n + submodule: migrate get_next_submodule to use repository structs\n \n - This patch started as a refactoring to make 'get_next_submodule' more\n - readable, but upon doing so, I realized that \"git fetch\" of the submodule\n - actually doesn't need to be run in the submodules worktree. So let's run\n - it in its git dir instead.\n + We used to recurse into submodules, even if they were broken having\n + only an objects directory. The child process executed in the submodule\n + would fail though if the submodule was broken.\n \n - That should pave the way towards fetching submodules that are currently\n - not checked out.\n -\n - This patch leaks the cp->dir in get_next_submodule, as any further\n - callback in run_processes_parallel doesn't have access to the child\n - process any more. In an early iteration of this patch, the function\n - get_submodule_repo_for directly returned the string containing the\n - git directory, which would be a better design choice for this patch.\n -\n - However the next patch both fixes the memory leak of cp->dir and also has\n - a use case for using the full repository handle of the submodule, so\n - it makes sense to introduce the get_submodule_repo_for here already.\n + This patch tightens the check upfront, such that we do not need\n + to spawn a child process to find out if the submodule is broken.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n +++ b/submodule.c\n -@@\n - \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n - }\n - \n -+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)\n -+{\n -+\tprepare_submodule_repo_env_no_git_dir(out);\n -+\targv_array_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n -+}\n -+\n - /* Helper function to display the submodule header line prior to the full\n - * summary output. If it can locate the submodule objects directory it will\n - * attempt to lookup both the left and right commits and put them into the\n @@\n \treturn spf->default_option;\n }\n @@ -99,9 +74,8 @@\n +\t\tif (repo) {\n \t\t\tchild_process_init(cp);\n -\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n --\t\t\tprepare_submodule_repo_env(&cp->env_array);\n -+\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n -+\t\t\tcp->dir = xstrdup(repo->gitdir);\n + \t\t\tprepare_submodule_repo_env(&cp->env_array);\n ++\t\t\tcp->dir = xstrdup(repo->worktree);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n @@ -113,27 +87,17 @@\n +\t\t\trepo_clear(repo);\n +\t\t\tfree(repo);\n \t\t\tret = 1;\n ++\t\t} else {\n ++\t\t\t/*\n ++\t\t\t * An empty directory is normal,\n ++\t\t\t * the submodule is not initialized\n ++\t\t\t */\n ++\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n ++\t\t\t !is_empty_dir(ce->name))\n ++\t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n \t\t}\n -\t\tstrbuf_release(&submodule_path);\n -\t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\n -\n -diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n ---- a/t/t5526-fetch-submodules.sh\n -+++ b/t/t5526-fetch-submodules.sh\n -@@\n - \n - \ttest_must_fail git -C dst status &&\n - \ttest_must_fail git -C dst diff &&\n --\ttest_must_fail git -C dst fetch --recurse-submodules\n -+\n -+\t# git-fetch cannot find the git directory of the submodule,\n -+\t# so it will do nothing, successfully, as it cannot distinguish between\n -+\t# this broken submodule and a submodule that was just set active but\n -+\t# not cloned yet\n -+\tgit -C dst fetch --recurse-submodules\n - '\n - \n - test_expect_success \"fetch new commits when submodule got renamed\" '\n-: ---------- > 5: 2d98ff1201 submodule.c: fetch in submodules git directory instead of in worktree\n5: 9ad0125310 ! 6: 092b9cbcba fetch: retry fetching submodules if needed objects were not fetched\n @@ -1,25 +1,23 @@\n Author: Stefan Beller \n \n - fetch: retry fetching submodules if needed objects were not fetched\n + fetch: try fetching submodules if needed objects were not fetched\n \n Currently when git-fetch is asked to recurse into submodules, it dispatches\n a plain \"git-fetch -C \" (with some submodule related options\n such as prefix and recusing strategy, but) without any information of the\n remote or the tip that should be fetched.\n \n - This works surprisingly well in some workflows (such as using submodules\n - as a third party library), while not so well in other scenarios, such\n - as in a Gerrit topic-based workflow, that can tie together changes\n - (potentially across repositories) on the server side. One of the parts\n - of such a Gerrit workflow is to download a change when wanting to examine\n - it, and you'd want to have its submodule changes that are in the same\n - topic downloaded as well. However these submodule changes reside in their\n - own repository in their own ref (refs/changes/).\n + But this default fetch is not sufficient, as a newly fetched commit in\n + the superproject could point to a commit in the submodule that is not\n + in the default refspec. This is common in workflows like Gerrit's.\n + When fetching a Gerrit change under review (from refs/changes/??), the\n + commits in that change likely point to submodule commits that have not\n + been merged to a branch yet.\n \n - Retry fetching a submodule by object name if the object id that the\n + Try fetching a submodule by object id if the object id that the\n superproject points to, cannot be found.\n \n - This retrying does not happen when the \"git fetch\" done at the\n + The try does not happen when the \"git fetch\" done at the\n superproject is not storing the fetched results in remote\n tracking branches (i.e. instead just recording them to\n FETCH_HEAD) in this step. A later patch will fix this.\n @@ -32,7 +30,6 @@\n in case the submodule recursion is set to \"on\".\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/builtin/fetch.c b/builtin/fetch.c\n --- a/builtin/fetch.c\n @@ -75,16 +72,16 @@\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n -+\tstruct get_next_submodule_task **retry;\n -+\tint retry_nr, retry_alloc;\n ++\tstruct get_next_submodule_task **fetch_specific_oids;\n ++\tint fetch_specific_oids_nr, fetch_specific_oids_alloc;\n };\n -#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n +#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n +\t\t STRING_LIST_INIT_DUP, \\\n +\t\t NULL, 0, 0}\n \n - static void calculate_changed_submodule_paths(\n - \tstruct submodule_parallel_fetch *spf)\n + static int get_fetch_recurse_config(const struct submodule *submodule,\n + \t\t\t\t struct submodule_parallel_fetch *spf)\n @@\n \treturn spf->default_option;\n }\n @@ -93,6 +90,8 @@\n +\tstruct repository *repo;\n +\tconst struct submodule *sub;\n +\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n ++\n ++\t/* fetch specific oids if set, otherwise fetch default refspec */\n +\tstruct oid_array *commits;\n +};\n +\n @@ -224,24 +223,29 @@\n -\t\t\trepo_clear(repo);\n -\t\t\tfree(repo);\n -\t\t\tret = 1;\n --\t\t}\n --\t\tstrbuf_release(&submodule_prefix);\n --\t\tif (ret) {\n - \t\t\tspf->count++;\n ++\t\t\tspf->count++;\n +\t\t\t*task_cb = task;\n +\n +\t\t\tstrbuf_release(&submodule_prefix);\n - \t\t\treturn 1;\n -+\t\t} else {\n ++\t\t\treturn 1;\n + \t\t} else {\n ++\n +\t\t\tget_next_submodule_task_release(task);\n +\t\t\tfree(task);\n ++\n + \t\t\t/*\n + \t\t\t * An empty directory is normal,\n + \t\t\t * the submodule is not initialized\n +@@\n + \t\t\t !is_empty_dir(ce->name))\n + \t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n \t\t}\n - \t}\n ++\t}\n +\n -+\tif (spf->retry_nr) {\n -+\t\tstruct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n ++\tif (spf->fetch_specific_oids_nr) {\n ++\t\tstruct get_next_submodule_task *task = spf->fetch_specific_oids[spf->fetch_specific_oids_nr - 1];\n +\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n -+\t\tspf->retry_nr--;\n ++\t\tspf->fetch_specific_oids_nr--;\n +\n +\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, task->sub->path);\n +\n @@ -262,9 +266,13 @@\n +\t\t\t\t\t append_oid_to_argv, &cp->args);\n +\n +\t\t*task_cb = task;\n -+\t\tstrbuf_release(&submodule_prefix);\n + \t\tstrbuf_release(&submodule_prefix);\n +-\t\tif (ret) {\n +-\t\t\tspf->count++;\n +-\t\t\treturn 1;\n +-\t\t}\n +\t\treturn 1;\n -+\t}\n + \t}\n +\n \treturn 0;\n }\n @@ -326,9 +334,11 @@\n +\t\t\treturn 0;\n +\n +\t\ttask->commits = commits;\n -+\t\tALLOC_GROW(spf->retry, spf->retry_nr + 1, spf->retry_alloc);\n -+\t\tspf->retry[spf->retry_nr] = task;\n -+\t\tspf->retry_nr++;\n ++\t\tALLOC_GROW(spf->fetch_specific_oids,\n ++\t\t\t spf->fetch_specific_oids_nr + 1,\n ++\t\t\t spf->fetch_specific_oids_alloc);\n ++\t\tspf->fetch_specific_oids[spf->fetch_specific_oids_nr] = task;\n ++\t\tspf->fetch_specific_oids_nr++;\n +\t\treturn 0;\n +\t}\n +\n @@ -346,18 +356,33 @@\n \ttest_cmp expect actual\n '\n \n -+test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n ++test_expect_success \"fetch new submodule commits on-demand outside standard refspec\" '\n ++\t# add a second submodule and ensure it is around in downstream first\n ++\tgit clone submodule sub1 &&\n ++\tgit submodule add ./sub1 &&\n ++\tgit commit -m \"adding a second submodule\" &&\n ++\tgit -C downstream pull &&\n ++\tgit -C downstream submodule update --init --recursive &&\n ++\n +\tgit checkout --detach &&\n ++\n +\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n +\tgit -C submodule update-ref refs/changes/1 $C &&\n +\tgit update-index --cacheinfo 160000 $C submodule &&\n -+\tgit commit -m \"updated submodule outside of refs/heads\" &&\n -+\tD=$(git rev-parse HEAD) &&\n -+\tgit update-ref refs/changes/2 $D &&\n ++\ttest_tick &&\n ++\n ++\tD=$(git -C sub1 commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n ++\tgit -C sub1 update-ref refs/changes/2 $D &&\n ++\tgit update-index --cacheinfo 160000 $D sub1 &&\n ++\n ++\tgit commit -m \"updated submodules outside of refs/heads\" &&\n ++\tE=$(git rev-parse HEAD) &&\n ++\tgit update-ref refs/changes/2 $E &&\n +\t(\n +\t\tcd downstream &&\n -+\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n ++\t\tgit fetch --recurse-submodules origin refs/changes/2:refs/heads/my_branch &&\n +\t\tgit -C submodule cat-file -t $C &&\n ++\t\tgit -C sub1 cat-file -t $D &&\n +\t\tgit checkout --recurse-submodules FETCH_HEAD\n +\t)\n +'\n6: b8db3b45bf < -: ---------- builtin/fetch: check for submodule updates for non branch fetches\n7: 905a4f0d4f < -: ---------- fixup! repository: repo_submodule_init to take a submodule struct\n-: ---------- > 7: 11bf819782 builtin/fetch: check for submodule updates in any ref update\n"},{"id":"361569","messageId":"20181025233231.102245-2-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 01/10] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:22Z","receivedAt":"2018-10-25T23:32:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Helped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/api-oid-array.txt | 5 +++++\n sha1-array.c | 17 +++++++++++++++++\n sha1-array.h | 3 +++\n 3 files changed, 25 insertions(+)\n\ndiff --git a/Documentation/technical/api-oid-array.txt b/Documentation/technical/api-oid-array.txt\nindex 9febfb1d52..c97428c2c3 100644\n--- a/Documentation/technical/api-oid-array.txt\n+++ b/Documentation/technical/api-oid-array.txt\n@@ -48,6 +48,11 @@ Functions\n \tis not sorted, this function has the side effect of sorting\n \tit.\n \n+`oid_array_filter`::\n+\tApply the callback function `want` to each entry in the array,\n+\tretaining only the entries for which the function returns true.\n+\tPreserve the order of the entries that are retained.\n+\n Examples\n --------\n \ndiff --git a/sha1-array.c b/sha1-array.c\nindex 265941fbf4..d505a004bb 100644\n--- a/sha1-array.c\n+++ b/sha1-array.c\n@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,\n \t}\n \treturn 0;\n }\n+\n+void oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn want,\n+\t\t void *cb_data)\n+{\n+\tunsigned nr = array->nr, src, dst;\n+\tstruct object_id *oids = array->oid;\n+\n+\tfor (src = dst = 0; src < nr; src++) {\n+\t\tif (want(&oids[src], cb_data)) {\n+\t\t\tif (src != dst)\n+\t\t\t\toidcpy(&oids[dst], &oids[src]);\n+\t\t\tdst++;\n+\t\t}\n+\t}\n+\tarray->nr = dst;\n+}\ndiff --git a/sha1-array.h b/sha1-array.h\nindex 232bf95017..55d016c4bf 100644\n--- a/sha1-array.h\n+++ b/sha1-array.h\n@@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,\n int oid_array_for_each_unique(struct oid_array *array,\n \t\t\t for_each_oid_fn fn,\n \t\t\t void *data);\n+void oid_array_filter(struct oid_array *array,\n+\t\t for_each_oid_fn want,\n+\t\t void *cbdata);\n \n #endif /* SHA1_ARRAY_H */\n-- \n2.19.0\n\n"},{"id":"361570","messageId":"20181025233231.102245-3-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 02/10] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:23Z","receivedAt":"2018-10-25T23:32:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The submodule subsystem is really bad at staying within 80 characters.\nFix it while we are here.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2b7082b2db..e145ebbb16 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1258,7 +1258,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (!submodule) {\n \t\t\tconst char *name = default_name_or_path(ce->name);\n \t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = default_submodule.name = name;\n+\t\t\t\tdefault_submodule.path = name;\n+\t\t\t\tdefault_submodule.name = name;\n \t\t\t\tsubmodule = &default_submodule;\n \t\t\t}\n \t\t}\n@@ -1268,8 +1269,10 @@ static int get_next_submodule(struct child_process *cp,\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,\n-\t\t\t\t\t\t\t submodule->name))\n+\t\t\tif (!submodule ||\n+\t\t\t !unsorted_string_list_lookup(\n+\t\t\t\t\t&changed_submodule_names,\n+\t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\n-- \n2.19.0\n\n"},{"id":"361571","messageId":"20181025233231.102245-4-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 03/10] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:24Z","receivedAt":"2018-10-25T23:32:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We can string_list_insert() to maintain sorted-ness of the\nlist as we find new items, or we can string_list_append() to\nbuild an unsorted list and sort it at the end just once.\n\nAs we do not rely on the sortedness while building the\nlist, we pick the \"append and sort at the end\" as it\nhas better worst case execution times.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex e145ebbb16..9fbfcfcfe1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1270,7 +1270,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!submodule ||\n-\t\t\t !unsorted_string_list_lookup(\n+\t\t\t !string_list_lookup(\n \t\t\t\t\t&changed_submodule_names,\n \t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n@@ -1364,6 +1364,7 @@ int fetch_populated_submodules(struct repository *r,\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n \tcalculate_changed_submodule_paths();\n+\tstring_list_sort(&changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n \t\t\t fetch_start_failure,\n-- \n2.19.0\n\n"},{"id":"361572","messageId":"20181025233231.102245-5-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 04/10] submodule.c: tighten scope of changed_submodule_names struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:25Z","receivedAt":"2018-10-25T23:32:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `changed_submodule_names` are only used for fetching, so let's make it\npart of the struct that is passed around for fetching submodules.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9fbfcfcfe1..6fb0b9d783 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -24,7 +24,6 @@\n #include \"object-store.h\"\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n 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@@ -1124,7 +1123,8 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \toid_array_append(&ref_tips_after_fetch, oid);\n }\n \n-static void calculate_changed_submodule_paths(void)\n+static void calculate_changed_submodule_paths(\n+\tstruct string_list *changed_submodule_names)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n@@ -1162,7 +1162,8 @@ static void calculate_changed_submodule_paths(void)\n \t\t\tcontinue;\n \n \t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&changed_submodule_names, name->string);\n+\t\t\tstring_list_append(changed_submodule_names,\n+\t\t\t\t\t name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1208,8 +1209,10 @@ struct submodule_parallel_fetch {\n \tint default_option;\n \tint quiet;\n \tint result;\n+\n+\tstruct string_list changed_submodule_names;\n };\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n \n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t struct submodule_parallel_fetch *spf)\n@@ -1271,7 +1274,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!submodule ||\n \t\t\t !string_list_lookup(\n-\t\t\t\t\t&changed_submodule_names,\n+\t\t\t\t\t&spf->changed_submodule_names,\n \t\t\t\t\tsubmodule->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n@@ -1363,8 +1366,8 @@ int fetch_populated_submodules(struct repository *r,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-\tcalculate_changed_submodule_paths();\n-\tstring_list_sort(&changed_submodule_names);\n+\tcalculate_changed_submodule_paths(&spf.changed_submodule_names);\n+\tstring_list_sort(&spf.changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\n \t\t\t fetch_start_failure,\n@@ -1373,7 +1376,7 @@ int fetch_populated_submodules(struct repository *r,\n \n \targv_array_clear(&spf.args);\n out:\n-\tstring_list_clear(&changed_submodule_names, 1);\n+\tstring_list_clear(&spf.changed_submodule_names, 1);\n \treturn spf.result;\n }\n \n-- \n2.19.0\n\n"},{"id":"361573","messageId":"20181025233231.102245-6-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 05/10] submodule: store OIDs in changed_submodule_names","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:26Z","receivedAt":"2018-10-25T23:32:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"'calculate_changed_submodule_paths' uses a local list to compute the\nchanged submodules, and then produces the result by copying appropriate\nitems into the result list.\n\nInstead use the result list directly and prune items afterwards\nusing string_list_remove_empty_items.\n\nBy doing so we'll have access to the util pointer for longer that\ncontains the commits that we need to fetch, which will be\nuseful in a later patch.\n\nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Tan \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6fb0b9d783..e4b494af7b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1127,8 +1127,7 @@ static void calculate_changed_submodule_paths(\n \tstruct string_list *changed_submodule_names)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n-\tconst struct string_list_item *name;\n+\tstruct string_list_item *name;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(the_repository, NULL, NULL))\n@@ -1145,9 +1144,9 @@ static void calculate_changed_submodule_paths(\n \t * Collect all submodules (whether checked out or not) for which new\n \t * commits have been recorded upstream in \"changed_submodule_names\".\n \t */\n-\tcollect_changed_submodules(&changed_submodules, &argv);\n+\tcollect_changed_submodules(changed_submodule_names, &argv);\n \n-\tfor_each_string_list_item(name, &changed_submodules) {\n+\tfor_each_string_list_item(name, changed_submodule_names) {\n \t\tstruct oid_array *commits = name->util;\n \t\tconst struct submodule *submodule;\n \t\tconst char *path = NULL;\n@@ -1161,12 +1160,14 @@ static void calculate_changed_submodule_paths(\n \t\tif (!path)\n \t\t\tcontinue;\n \n-\t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(changed_submodule_names,\n-\t\t\t\t\t name->string);\n+\t\tif (submodule_has_commits(path, commits)) {\n+\t\t\toid_array_clear(commits);\n+\t\t\t*name->string = '\\0';\n+\t\t}\n \t}\n \n-\tfree_submodules_oids(&changed_submodules);\n+\tstring_list_remove_empty_items(changed_submodule_names, 1);\n+\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n \toid_array_clear(&ref_tips_after_fetch);\n@@ -1376,7 +1377,7 @@ int fetch_populated_submodules(struct repository *r,\n \n \targv_array_clear(&spf.args);\n out:\n-\tstring_list_clear(&spf.changed_submodule_names, 1);\n+\tfree_submodules_oids(&spf.changed_submodule_names);\n \treturn spf.result;\n }\n \n-- \n2.19.0\n\n"},{"id":"361574","messageId":"20181025233231.102245-7-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 06/10] repository: repo_submodule_init to take a submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:27Z","receivedAt":"2018-10-25T23:32:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When constructing a struct repository for a submodule for some revision\nof the superproject where the submodule is not contained in the index,\nit may not be present in the working tree currently either. In that\nsituation giving a 'path' argument is not useful. Upgrade the\nrepo_submodule_init function to take a struct submodule instead.\nThe submodule struct can be obtained via submodule_from_{path, name} or\nan artificial submodule struct can be passed in.\n\nWhile we are at it, rename the repository struct in the repo_submodule_init\nfunction, which is to be initialized, to a name that is not confused with\nthe struct submodule as easily. Perform such renames in similar functions\nas well.\n\nAlso move its documentation into the header file.\n\nSigned-off-by: Stefan Beller \n---\n builtin/grep.c | 17 +++++++-----\n builtin/ls-files.c | 12 +++++----\n builtin/submodule--helper.c | 2 +-\n repository.c | 27 ++++++++------------\n repository.h | 12 +++++++--\n t/helper/test-submodule-nested-repo-config.c | 8 +++---\n 6 files changed, 43 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7da8fef31a..ba7634258a 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -418,7 +418,10 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t\t\t const struct object_id *oid,\n \t\t\t const char *filename, const char *path)\n {\n-\tstruct repository submodule;\n+\tstruct repository subrepo;\n+\tconst struct submodule *sub = submodule_from_path(superproject,\n+\t\t\t\t\t\t\t &null_oid, path);\n+\n \tint hit;\n \n \t/*\n@@ -434,12 +437,12 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t\treturn 0;\n \t}\n \n-\tif (repo_submodule_init(&submodule, superproject, path)) {\n+\tif (repo_submodule_init(&subrepo, superproject, sub)) {\n \t\tgrep_read_unlock();\n \t\treturn 0;\n \t}\n \n-\trepo_read_gitmodules(&submodule);\n+\trepo_read_gitmodules(&subrepo);\n \n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\n@@ -451,7 +454,7 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_to_alternates_memory(submodule.objects->objectdir);\n+\tadd_to_alternates_memory(subrepo.objects->objectdir);\n \tgrep_read_unlock();\n \n \tif (oid) {\n@@ -476,14 +479,14 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \n \t\tinit_tree_desc(&tree, data, size);\n \t\thit = grep_tree(opt, pathspec, &tree, &base, base.len,\n-\t\t\t\tobject->type == OBJ_COMMIT, &submodule);\n+\t\t\t\tobject->type == OBJ_COMMIT, &subrepo);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-\t\thit = grep_cache(opt, &submodule, pathspec, 1);\n+\t\thit = grep_cache(opt, &subrepo, pathspec, 1);\n \t}\n \n-\trepo_clear(&submodule);\n+\trepo_clear(&subrepo);\n \treturn hit;\n }\n \ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 7f9919a362..4d1649c1b3 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -206,17 +206,19 @@ static void show_files(struct repository *repo, struct dir_struct *dir);\n static void show_submodule(struct repository *superproject,\n \t\t\t struct dir_struct *dir, const char *path)\n {\n-\tstruct repository submodule;\n+\tstruct repository subrepo;\n+\tconst struct submodule *sub = submodule_from_path(superproject,\n+\t\t\t\t\t\t\t &null_oid, path);\n \n-\tif (repo_submodule_init(&submodule, superproject, path))\n+\tif (repo_submodule_init(&subrepo, superproject, sub))\n \t\treturn;\n \n-\tif (repo_read_index(&submodule) < 0)\n+\tif (repo_read_index(&subrepo) < 0)\n \t\tdie(\"index file corrupt\");\n \n-\tshow_files(&submodule, dir);\n+\tshow_files(&subrepo, dir);\n \n-\trepo_clear(&submodule);\n+\trepo_clear(&subrepo);\n }\n \n static void show_ce(struct repository *repo, struct dir_struct *dir,\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5f8a804a6e..015aa1471f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2037,7 +2037,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \tif (!sub)\n \t\tBUG(\"We could get the submodule handle before?\");\n \n-\tif (repo_submodule_init(&subrepo, the_repository, path))\n+\tif (repo_submodule_init(&subrepo, the_repository, sub))\n \t\tdie(_(\"could not get a repository handle for submodule '%s'\"), path);\n \n \tif (!repo_config_get_string(&subrepo, \"core.worktree\", &cw)) {\ndiff --git a/repository.c b/repository.c\nindex 5dd1486718..aabe64ee5d 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -166,30 +166,23 @@ int repo_init(struct repository *repo,\n \treturn -1;\n }\n \n-/*\n- * Initialize 'submodule' as the submodule given by 'path' in parent repository\n- * 'superproject'.\n- * Return 0 upon success and a non-zero value upon failure.\n- */\n-int repo_submodule_init(struct repository *submodule,\n+int repo_submodule_init(struct repository *subrepo,\n \t\t\tstruct repository *superproject,\n-\t\t\tconst char *path)\n+\t\t\tconst struct submodule *sub)\n {\n-\tconst struct submodule *sub;\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstruct strbuf worktree = STRBUF_INIT;\n \tint ret = 0;\n \n-\tsub = submodule_from_path(superproject, &null_oid, path);\n \tif (!sub) {\n \t\tret = -1;\n \t\tgoto out;\n \t}\n \n-\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", path);\n-\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", path);\n+\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", sub->path);\n+\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", sub->path);\n \n-\tif (repo_init(submodule, gitdir.buf, worktree.buf)) {\n+\tif (repo_init(subrepo, gitdir.buf, worktree.buf)) {\n \t\t/*\n \t\t * If initilization fails then it may be due to the submodule\n \t\t * not being populated in the superproject's worktree. Instead\n@@ -201,16 +194,16 @@ int repo_submodule_init(struct repository *submodule,\n \t\tstrbuf_repo_git_path(&gitdir, superproject,\n \t\t\t\t \"modules/%s\", sub->name);\n \n-\t\tif (repo_init(submodule, gitdir.buf, NULL)) {\n+\t\tif (repo_init(subrepo, gitdir.buf, NULL)) {\n \t\t\tret = -1;\n \t\t\tgoto out;\n \t\t}\n \t}\n \n-\tsubmodule->submodule_prefix = xstrfmt(\"%s%s/\",\n-\t\t\t\t\t superproject->submodule_prefix ?\n-\t\t\t\t\t superproject->submodule_prefix :\n-\t\t\t\t\t \"\", path);\n+\tsubrepo->submodule_prefix = xstrfmt(\"%s%s/\",\n+\t\t\t\t\t superproject->submodule_prefix ?\n+\t\t\t\t\t superproject->submodule_prefix :\n+\t\t\t\t\t \"\", sub->path);\n \n out:\n \tstrbuf_release(&gitdir);\ndiff --git a/repository.h b/repository.h\nindex 9f16c42c1e..0e482b7d49 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -116,9 +116,17 @@ void repo_set_worktree(struct repository *repo, const char *path);\n void repo_set_hash_algo(struct repository *repo, int algo);\n void initialize_the_repository(void);\n int repo_init(struct repository *r, const char *gitdir, const char *worktree);\n-int repo_submodule_init(struct repository *submodule,\n+\n+/*\n+ * Initialize the repository 'subrepo' as the submodule given by the\n+ * struct submodule 'sub' in parent repository 'superproject'.\n+ * Return 0 upon success and a non-zero value upon failure, which may happen\n+ * if the submodule is not found, or 'sub' is NULL.\n+ */\n+struct submodule;\n+int repo_submodule_init(struct repository *subrepo,\n \t\t\tstruct repository *superproject,\n-\t\t\tconst char *path);\n+\t\t\tconst struct submodule *sub);\n void repo_clear(struct repository *repo);\n \n /*\ndiff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c\nindex a31e2a9bea..bc97929bbc 100644\n--- a/t/helper/test-submodule-nested-repo-config.c\n+++ b/t/helper/test-submodule-nested-repo-config.c\n@@ -10,19 +10,21 @@ static void die_usage(int argc, const char **argv, const char *msg)\n \n int cmd__submodule_nested_repo_config(int argc, const char **argv)\n {\n-\tstruct repository submodule;\n+\tstruct repository subrepo;\n+\tconst struct submodule *sub;\n \n \tif (argc < 3)\n \t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n \n \tsetup_git_directory();\n \n-\tif (repo_submodule_init(&submodule, the_repository, argv[1])) {\n+\tsub = submodule_from_path(the_repository, &null_oid, argv[1]);\n+\tif (repo_submodule_init(&subrepo, the_repository, sub)) {\n \t\tdie_usage(argc, argv, \"Submodule not found.\");\n \t}\n \n \t/* Read the config of _child_ submodules. */\n-\tprint_config_from_gitmodules(&submodule, argv[2]);\n+\tprint_config_from_gitmodules(&subrepo, argv[2]);\n \n \tsubmodule_free(the_repository);\n \n-- \n2.19.0\n\n"},{"id":"361575","messageId":"20181025233231.102245-8-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 07/10] submodule: migrate get_next_submodule to use repository structs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:28Z","receivedAt":"2018-10-25T23:32:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We used to recurse into submodules, even if they were broken having\nonly an objects directory. The child process executed in the submodule\nwould fail though if the submodule was broken.\n\nThis patch tightens the check upfront, such that we do not need\nto spawn a child process to find out if the submodule is broken.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 51 +++++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 39 insertions(+), 12 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex e4b494af7b..a1a32cab7d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1240,6 +1240,29 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+static struct repository *get_submodule_repo_for(struct repository *r,\n+\t\t\t\t\t\t const struct submodule *sub)\n+{\n+\tstruct repository *ret = xmalloc(sizeof(*ret));\n+\n+\tif (repo_submodule_init(ret, r, sub)) {\n+\t\t/*\n+\t\t * No entry in .gitmodules? Technically not a submodule,\n+\t\t * but historically we supported repositories that happen to be\n+\t\t * in-place where a gitlink is. Keep supporting them.\n+\t\t */\n+\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", sub->path);\n+\t\tif (repo_init(ret, gitdir.buf, NULL)) {\n+\t\t\tstrbuf_release(&gitdir);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tstrbuf_release(&gitdir);\n+\t}\n+\n+\treturn ret;\n+}\n+\n static int get_next_submodule(struct child_process *cp,\n \t\t\t struct strbuf *err, void *data, void **task_cb)\n {\n@@ -1247,12 +1270,11 @@ static int get_next_submodule(struct child_process *cp,\n \tstruct submodule_parallel_fetch *spf = data;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n-\t\tstruct strbuf submodule_path = STRBUF_INIT;\n-\t\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n-\t\tconst char *git_dir, *default_argv;\n+\t\tconst char *default_argv;\n \t\tconst struct submodule *submodule;\n+\t\tstruct repository *repo;\n \t\tstruct submodule default_submodule = SUBMODULE_INIT;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n@@ -1287,16 +1309,12 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tstrbuf_repo_worktree_path(&submodule_path, spf->r, \"%s\", ce->name);\n-\t\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", submodule_path.buf);\n \t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n-\t\tgit_dir = read_gitfile(submodule_git_dir.buf);\n-\t\tif (!git_dir)\n-\t\t\tgit_dir = submodule_git_dir.buf;\n-\t\tif (is_directory(git_dir)) {\n+\t\trepo = get_submodule_repo_for(spf->r, submodule);\n+\t\tif (repo) {\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n \t\t\tprepare_submodule_repo_env(&cp->env_array);\n+\t\t\tcp->dir = xstrdup(repo->worktree);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n@@ -1306,10 +1324,19 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\n+\t\t\trepo_clear(repo);\n+\t\t\tfree(repo);\n \t\t\tret = 1;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * An empty directory is normal,\n+\t\t\t * the submodule is not initialized\n+\t\t\t */\n+\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n+\t\t\t !is_empty_dir(ce->name))\n+\t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n \t\t}\n-\t\tstrbuf_release(&submodule_path);\n-\t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\n-- \n2.19.0\n\n"},{"id":"361576","messageId":"20181025233231.102245-9-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 08/10] submodule.c: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:29Z","receivedAt":"2018-10-25T23:32:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Keep the properties introduced in 10f5c52656 (submodule: avoid\nauto-discovery in prepare_submodule_repo_env(), 2016-09-01), by fixating\nthe git directory of the submodule.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex a1a32cab7d..67813fbe78 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -494,6 +494,12 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)\n+{\n+\tprepare_submodule_repo_env_no_git_dir(out);\n+\targv_array_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n+}\n+\n /* Helper function to display the submodule header line prior to the full\n * summary output. If it can locate the submodule objects directory it will\n * attempt to lookup both the left and right commits and put them into the\n@@ -1313,8 +1319,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\trepo = get_submodule_repo_for(spf->r, submodule);\n \t\tif (repo) {\n \t\t\tchild_process_init(cp);\n-\t\t\tprepare_submodule_repo_env(&cp->env_array);\n-\t\t\tcp->dir = xstrdup(repo->worktree);\n+\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\t\tcp->dir = xstrdup(repo->gitdir);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n-- \n2.19.0\n\n"},{"id":"361577","messageId":"20181025233231.102245-10-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 09/10] fetch: try fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:30Z","receivedAt":"2018-10-25T23:33:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently when git-fetch is asked to recurse into submodules, it dispatches\na plain \"git-fetch -C \" (with some submodule related options\nsuch as prefix and recusing strategy, but) without any information of the\nremote or the tip that should be fetched.\n\nBut this default fetch is not sufficient, as a newly fetched commit in\nthe superproject could point to a commit in the submodule that is not\nin the default refspec. This is common in workflows like Gerrit's.\nWhen fetching a Gerrit change under review (from refs/changes/??), the\ncommits in that change likely point to submodule commits that have not\nbeen merged to a branch yet.\n\nTry fetching a submodule by object id if the object id that the\nsuperproject points to, cannot be found.\n\nThe try does not happen when the \"git fetch\" done at the\nsuperproject is not storing the fetched results in remote\ntracking branches (i.e. instead just recording them to\nFETCH_HEAD) in this step. A later patch will fix this.\n\nbuiltin/fetch used to only inspect submodules when they were fetched\n\"on-demand\", as in either on/off case it was clear whether the submodule\nneeds to be fetched. However to know whether we need to try fetching the\nobject ids, we need to identify the object names, which is done in this\nfunction check_for_new_submodule_commits(), so we'll also run that code\nin case the submodule recursion is set to \"on\".\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 9 +-\n submodule.c | 192 ++++++++++++++++++++++++++++++------\n t/t5526-fetch-submodules.sh | 31 ++++++\n 3 files changed, 198 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 61bec5d213..95c44bf6ff 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -700,8 +700,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n+\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n \t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(msg, ref, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n@@ -716,8 +715,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n+\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n \t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"fast-forward\", ref, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n@@ -731,8 +729,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n+\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n \t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"forced-update\", ref, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\ndiff --git a/submodule.c b/submodule.c\nindex 67813fbe78..c978a38c81 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1218,8 +1218,12 @@ struct submodule_parallel_fetch {\n \tint result;\n \n \tstruct string_list changed_submodule_names;\n+\tstruct get_next_submodule_task **fetch_specific_oids;\n+\tint fetch_specific_oids_nr, fetch_specific_oids_alloc;\n };\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n+\t\t STRING_LIST_INIT_DUP, \\\n+\t\t NULL, 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@@ -1246,6 +1250,58 @@ static int get_fetch_recurse_config(const struct submodule *submodule,\n \treturn spf->default_option;\n }\n \n+struct get_next_submodule_task {\n+\tstruct repository *repo;\n+\tconst struct submodule *sub;\n+\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n+\n+\t/* fetch specific oids if set, otherwise fetch default refspec */\n+\tstruct oid_array *commits;\n+};\n+\n+static const struct submodule *get_default_submodule(const char *path)\n+{\n+\tstruct submodule *ret = NULL;\n+\tconst char *name = default_name_or_path(path);\n+\n+\tif (!name)\n+\t\treturn NULL;\n+\n+\tret = xmalloc(sizeof(*ret));\n+\tmemset(ret, 0, sizeof(*ret));\n+\tret->path = name;\n+\tret->name = name;\n+\n+\treturn (const struct submodule *) ret;\n+}\n+\n+static struct get_next_submodule_task *get_next_submodule_task_create(\n+\tstruct repository *r, const char *path)\n+{\n+\tstruct get_next_submodule_task *task = xmalloc(sizeof(*task));\n+\tmemset(task, 0, sizeof(*task));\n+\n+\ttask->sub = submodule_from_path(r, &null_oid, path);\n+\tif (!task->sub) {\n+\t\ttask->sub = get_default_submodule(path);\n+\t\ttask->free_sub = 1;\n+\t}\n+\n+\treturn task;\n+}\n+\n+static void get_next_submodule_task_release(struct get_next_submodule_task *p)\n+{\n+\tif (p->free_sub)\n+\t\tfree((void*)p->sub);\n+\tp->free_sub = 0;\n+\tp->sub = NULL;\n+\n+\tif (p->repo)\n+\t\trepo_clear(p->repo);\n+\tFREE_AND_NULL(p->repo);\n+}\n+\n static struct repository *get_submodule_repo_for(struct repository *r,\n \t\t\t\t\t\t const struct submodule *sub)\n {\n@@ -1272,39 +1328,35 @@ static struct repository *get_submodule_repo_for(struct repository *r,\n static int get_next_submodule(struct child_process *cp,\n \t\t\t struct strbuf *err, void *data, void **task_cb)\n {\n-\tint ret = 0;\n \tstruct submodule_parallel_fetch *spf = data;\n \n \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n-\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tint recurse_config;\n \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n \t\tconst char *default_argv;\n-\t\tconst struct submodule *submodule;\n-\t\tstruct repository *repo;\n-\t\tstruct submodule default_submodule = SUBMODULE_INIT;\n+\t\tstruct get_next_submodule_task *task;\n \n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n-\t\tsubmodule = submodule_from_path(spf->r, &null_oid, ce->name);\n-\t\tif (!submodule) {\n-\t\t\tconst char *name = default_name_or_path(ce->name);\n-\t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = name;\n-\t\t\t\tdefault_submodule.name = name;\n-\t\t\t\tsubmodule = &default_submodule;\n-\t\t\t}\n+\t\ttask = get_next_submodule_task_create(spf->r, ce->name);\n+\n+\t\tif (!task->sub) {\n+\t\t\tfree(task);\n+\t\t\tcontinue;\n \t\t}\n \n-\t\tswitch (get_fetch_recurse_config(submodule, spf))\n+\t\trecurse_config = get_fetch_recurse_config(task->sub, spf);\n+\n+\t\tswitch (recurse_config)\n \t\t{\n \t\tdefault:\n \t\tcase RECURSE_SUBMODULES_DEFAULT:\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tif (!submodule ||\n+\t\t\tif (!task->sub ||\n \t\t\t !string_list_lookup(\n \t\t\t\t\t&spf->changed_submodule_names,\n-\t\t\t\t\tsubmodule->name))\n+\t\t\t\t\ttask->sub->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t\tbreak;\n@@ -1315,12 +1367,12 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n-\t\trepo = get_submodule_repo_for(spf->r, submodule);\n-\t\tif (repo) {\n+\t\ttask->repo = get_submodule_repo_for(spf->r, task->sub);\n+\t\tif (task->repo) {\n+\t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\n \t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n-\t\t\tcp->dir = xstrdup(repo->gitdir);\n+\t\t\tcp->dir = task->repo->gitdir;\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n@@ -1329,12 +1381,22 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\targv_array_pushv(&cp->args, spf->args.argv);\n \t\t\targv_array_push(&cp->args, default_argv);\n \t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\n+\t\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\",\n+\t\t\t\t\t\t spf->prefix,\n+\t\t\t\t\t\t task->sub->path);\n \t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n \n-\t\t\trepo_clear(repo);\n-\t\t\tfree(repo);\n-\t\t\tret = 1;\n+\t\t\tspf->count++;\n+\t\t\t*task_cb = task;\n+\n+\t\t\tstrbuf_release(&submodule_prefix);\n+\t\t\treturn 1;\n \t\t} else {\n+\n+\t\t\tget_next_submodule_task_release(task);\n+\t\t\tfree(task);\n+\n \t\t\t/*\n \t\t\t * An empty directory is normal,\n \t\t\t * the submodule is not initialized\n@@ -1343,12 +1405,36 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t !is_empty_dir(ce->name))\n \t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n \t\t}\n+\t}\n+\n+\tif (spf->fetch_specific_oids_nr) {\n+\t\tstruct get_next_submodule_task *task = spf->fetch_specific_oids[spf->fetch_specific_oids_nr - 1];\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tspf->fetch_specific_oids_nr--;\n+\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, task->sub->path);\n+\n+\t\tchild_process_init(cp);\n+\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tcp->git_cmd = 1;\n+\t\tcp->dir = task->repo->gitdir;\n+\n+\t\targv_array_init(&cp->args);\n+\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\targv_array_push(&cp->args, \"on-demand\");\n+\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\n+\t\t/* NEEDSWORK: have get_default_remote from s--h */\n+\t\targv_array_push(&cp->args, \"origin\");\n+\t\toid_array_for_each_unique(task->commits,\n+\t\t\t\t\t append_oid_to_argv, &cp->args);\n+\n+\t\t*task_cb = task;\n \t\tstrbuf_release(&submodule_prefix);\n-\t\tif (ret) {\n-\t\t\tspf->count++;\n-\t\t\treturn 1;\n-\t\t}\n+\t\treturn 1;\n \t}\n+\n \treturn 0;\n }\n \n@@ -1356,20 +1442,70 @@ static int fetch_start_failure(struct strbuf *err,\n \t\t\t void *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct get_next_submodule_task *task = task_cb;\n \n \tspf->result = 1;\n \n+\tget_next_submodule_task_release(task);\n \treturn 0;\n }\n \n+static int commit_exists_in_sub(const struct object_id *oid, void *data)\n+{\n+\tstruct repository *subrepo = data;\n+\n+\tenum object_type type = oid_object_info(subrepo, oid, NULL);\n+\n+\treturn type != OBJ_COMMIT;\n+}\n+\n static int fetch_finish(int retvalue, struct strbuf *err,\n \t\t\tvoid *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n+\tstruct get_next_submodule_task *task = task_cb;\n+\tconst struct submodule *sub;\n+\n+\tstruct string_list_item *it;\n+\tstruct oid_array *commits;\n \n \tif (retvalue)\n \t\tspf->result = 1;\n \n+\tif (!task)\n+\t\treturn 0;\n+\n+\tsub = task->sub;\n+\tif (!sub)\n+\t\tgoto out;\n+\n+\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n+\tif (!it)\n+\t\tgoto out;\n+\n+\tcommits = it->util;\n+\toid_array_filter(commits,\n+\t\t\t commit_exists_in_sub,\n+\t\t\t task->repo);\n+\n+\t/* Are there commits that do not exist? */\n+\tif (commits->nr) {\n+\t\t/* We already tried fetching them, do not try again. */\n+\t\tif (task->commits)\n+\t\t\treturn 0;\n+\n+\t\ttask->commits = commits;\n+\t\tALLOC_GROW(spf->fetch_specific_oids,\n+\t\t\t spf->fetch_specific_oids_nr + 1,\n+\t\t\t spf->fetch_specific_oids_alloc);\n+\t\tspf->fetch_specific_oids[spf->fetch_specific_oids_nr] = task;\n+\t\tspf->fetch_specific_oids_nr++;\n+\t\treturn 0;\n+\t}\n+\n+out:\n+\tget_next_submodule_task_release(task);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 6c2f9b2ba2..5a75b57852 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -600,4 +600,35 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"fetch new submodule commits on-demand outside standard refspec\" '\n+\t# add a second submodule and ensure it is around in downstream first\n+\tgit clone submodule sub1 &&\n+\tgit submodule add ./sub1 &&\n+\tgit commit -m \"adding a second submodule\" &&\n+\tgit -C downstream pull &&\n+\tgit -C downstream submodule update --init --recursive &&\n+\n+\tgit checkout --detach &&\n+\n+\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/1 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/2 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/2 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/2:refs/heads/my_branch &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.19.0\n\n"},{"id":"361578","messageId":"20181025233231.102245-11-sbeller@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"[PATCH 10/10] builtin/fetch: check for submodule updates in any ref update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T23:32:31Z","receivedAt":"2018-10-25T23:33:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Gerrit, the code review tool, has a different workflow than our mailing\nlist based approach. Usually users upload changes to a Gerrit server and\ncontinuous integration and testing happens by bots. Sometimes however a\nuser wants to checkout a change locally and look at it locally. For this\nuse case, Gerrit offers a command line snippet to copy and paste to your\nterminal, which looks like\n\n git fetch https:///gerrit refs/changes/ &&\n git checkout FETCH_HEAD\n\nFor Gerrit changes that contain changing submodule gitlinks, it would be\neasy to extend both the fetch and checkout with the '--recurse-submodules'\nflag, such that this command line snippet would produce the state of a\nchange locally.\n\nHowever the functionality added in the previous patch, which would\nensure that we fetch the objects in the submodule that the gitlink pointed\nat, only works for remote tracking branches so far, not for FETCH_HEAD.\n\nMake sure that fetching a superproject to its FETCH_HEAD, also respects\nthe existence checks for objects in the submodule recursion.\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 8 ++------\n t/t5526-fetch-submodules.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 95c44bf6ff..f39012d7c2 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -700,8 +700,6 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(msg, ref, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n@@ -715,8 +713,6 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"fast-forward\", ref, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n@@ -729,8 +725,6 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n-\t\t\tcheck_for_new_submodule_commits(&ref->new_oid);\n \t\tr = s_update_ref(\"forced-update\", ref, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n@@ -826,6 +820,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tref->force = rm->peer_ref->force;\n \t\t\t}\n \n+\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n+\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n \n \t\t\tif (!strcmp(rm->name, \"HEAD\")) {\n \t\t\t\tkind = \"\";\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 5a75b57852..799785783f 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -631,4 +631,28 @@ test_expect_success \"fetch new submodule commits on-demand outside standard refs\n \t)\n '\n \n+test_expect_success 'fetch new submodule commits on-demand in FETCH_HEAD' '\n+\t# depends on the previous test for setup\n+\n+\tC=$(git -C submodule commit-tree -m \"another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/1 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\ttest_tick &&\n+\n+\tD=$(git -C sub1 commit-tree -m \"another change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C sub1 update-ref refs/changes/2 $D &&\n+\tgit update-index --cacheinfo 160000 $D sub1 &&\n+\n+\tgit commit -m \"updated submodules outside of refs/heads\" &&\n+\tE=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/2 $E &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/2 &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit -C sub1 cat-file -t $D &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.19.0\n\n"},{"id":"361631","messageId":"20181026185747.259713-1-jonathantanmy@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-6-sbeller@google.com","subject":"Re: [PATCH 05/10] submodule: store OIDs in changed_submodule_names","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-26T18:57:47Z","receivedAt":"2018-10-26T18:57:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Reviewed-by: Jonathan Tan \n\nProbably better not to include such lines, especially since the review\nby me is not yet complete.\n\nHaving said that, patches 1-5 look good to me. Patches 1-3 are identical\nto the previous version, which I have already reviewed. In patch 4,\nStefan made the code change I suggested [1].\n\nIn this patch, compared to the previous version which I have already\nreviewed [2], the code is unchanged aside from some variable renaming. I\nsuggested a commit title change which Stefan has done.\n\n[1] https://public-inbox.org/git/20181017212624.196598-1-jonathantanmy@google.com/\n[2] https://public-inbox.org/git/20181017214534.199890-1-jonathantanmy@google.com/\n"},{"id":"361633","messageId":"20181026191553.108916-1-jonathantanmy@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-7-sbeller@google.com","subject":"Re: [PATCH 06/10] repository: repo_submodule_init to take a submodule struct","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-26T19:15:53Z","receivedAt":"2018-10-26T19:16:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 7da8fef31a..ba7634258a 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -418,7 +418,10 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n> \t\t\t const struct object_id *oid,\n> \t\t\t const char *filename, const char *path)\n> {\n> -\tstruct repository submodule;\n> +\tstruct repository subrepo;\n> +\tconst struct submodule *sub = submodule_from_path(superproject,\n> +\t\t\t\t\t\t\t &null_oid, path);\n\n[snip]\n\n> -\tif (repo_submodule_init(&submodule, superproject, path)) {\n> +\tif (repo_submodule_init(&subrepo, superproject, sub)) {\n\nThe last argument to repo_submodule_init is now\n\"submodule_from_path(superproject, &null_oid, path)\" instead of \"path\",\nand looking forward into the patch, we do not need a NULL check because\nrepo_submodule_init() tolerates NULL in that argument.\n\nLet's see if the rest of the code follows the pattern - a call to\nsubmodule_from_path() with the 3 expected arguments (repo, null OID,\npath).\n\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 7f9919a362..4d1649c1b3 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -206,17 +206,19 @@ static void show_files(struct repository *repo, struct dir_struct *dir);\n> static void show_submodule(struct repository *superproject,\n> \t\t\t struct dir_struct *dir, const char *path)\n> {\n> -\tstruct repository submodule;\n> +\tstruct repository subrepo;\n> +\tconst struct submodule *sub = submodule_from_path(superproject,\n> +\t\t\t\t\t\t\t &null_oid, path);\n> \n> -\tif (repo_submodule_init(&submodule, superproject, path))\n> +\tif (repo_submodule_init(&subrepo, superproject, sub))\n\nSo far so good.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 5f8a804a6e..015aa1471f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2037,7 +2037,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n> \tif (!sub)\n> \t\tBUG(\"We could get the submodule handle before?\");\n> \n> -\tif (repo_submodule_init(&subrepo, the_repository, path))\n> +\tif (repo_submodule_init(&subrepo, the_repository, sub))\n\nThe definition of \"sub\" is not quoted here in this e-mail, but it is\nindeed \"submodule_from_path(the_repository, &null_oid, path)\".\n(\"the_repository\" in the invocation to submodule_from_path() is correct\nbecause the 2nd argument to the invocation of repo_submodule_init() is\n\"the_repository\".)\n\n> -int repo_submodule_init(struct repository *submodule,\n> +int repo_submodule_init(struct repository *subrepo,\n> \t\t\tstruct repository *superproject,\n> -\t\t\tconst char *path)\n> +\t\t\tconst struct submodule *sub)\n> {\n> -\tconst struct submodule *sub;\n> \tstruct strbuf gitdir = STRBUF_INIT;\n> \tstruct strbuf worktree = STRBUF_INIT;\n> \tint ret = 0;\n> \n> -\tsub = submodule_from_path(superproject, &null_oid, path);\n\nAs expected, this line is removed.\n\n> \tif (!sub) {\n> \t\tret = -1;\n> \t\tgoto out;\n> \t}\n> \n> -\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", path);\n> -\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", path);\n> +\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", sub->path);\n> +\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", sub->path);\n\npath and sub->path are the same, so this is fine. (This can be seen from\ncache_put_path() and cache_lookup_path() in submodule-config.c.)\n\n> -\tsubmodule->submodule_prefix = xstrfmt(\"%s%s/\",\n> -\t\t\t\t\t superproject->submodule_prefix ?\n> -\t\t\t\t\t superproject->submodule_prefix :\n> -\t\t\t\t\t \"\", path);\n> +\tsubrepo->submodule_prefix = xstrfmt(\"%s%s/\",\n> +\t\t\t\t\t superproject->submodule_prefix ?\n> +\t\t\t\t\t superproject->submodule_prefix :\n> +\t\t\t\t\t \"\", sub->path);\n\nLikewise.\n\n> +/*\n> + * Initialize the repository 'subrepo' as the submodule given by the\n> + * struct submodule 'sub' in parent repository 'superproject'.\n> + * Return 0 upon success and a non-zero value upon failure, which may happen\n> + * if the submodule is not found, or 'sub' is NULL.\n> + */\n> +struct submodule;\n> +int repo_submodule_init(struct repository *subrepo,\n> \t\t\tstruct repository *superproject,\n> -\t\t\tconst char *path);\n> +\t\t\tconst struct submodule *sub);\n\nHere is where it says that the last argument can be NULL.\n\n> diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c\n> index a31e2a9bea..bc97929bbc 100644\n> --- a/t/helper/test-submodule-nested-repo-config.c\n> +++ b/t/helper/test-submodule-nested-repo-config.c\n> @@ -10,19 +10,21 @@ static void die_usage(int argc, const char **argv, const char *msg)\n> \n> int cmd__submodule_nested_repo_config(int argc, const char **argv)\n> {\n> -\tstruct repository submodule;\n> +\tstruct repository subrepo;\n> +\tconst struct submodule *sub;\n> \n> \tif (argc < 3)\n> \t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n> \n> \tsetup_git_directory();\n> \n> -\tif (repo_submodule_init(&submodule, the_repository, argv[1])) {\n> +\tsub = submodule_from_path(the_repository, &null_oid, argv[1]);\n> +\tif (repo_submodule_init(&subrepo, the_repository, sub)) {\n\nThe expected pattern.\n\nThis patch looks good to me.\n"},{"id":"361635","messageId":"20181026192621.111450-1-jonathantanmy@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-8-sbeller@google.com","subject":"Re: [PATCH 07/10] submodule: migrate get_next_submodule to use repository structs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-26T19:26:21Z","receivedAt":"2018-10-26T19:26:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> We used to recurse into submodules, even if they were broken having\n> only an objects directory. The child process executed in the submodule\n> would fail though if the submodule was broken.\n> \n> This patch tightens the check upfront, such that we do not need\n> to spawn a child process to find out if the submodule is broken.\n\nThanks for the clear commit message. Also mention which tests currently\nexercise this.\n\n> +static struct repository *get_submodule_repo_for(struct repository *r,\n> +\t\t\t\t\t\t const struct submodule *sub)\n> +{\n> +\tstruct repository *ret = xmalloc(sizeof(*ret));\n> +\n> +\tif (repo_submodule_init(ret, r, sub)) {\n> +\t\t/*\n> +\t\t * No entry in .gitmodules? Technically not a submodule,\n> +\t\t * but historically we supported repositories that happen to be\n> +\t\t * in-place where a gitlink is. Keep supporting them.\n> +\t\t */\n> +\t\tstruct strbuf gitdir = STRBUF_INIT;\n> +\t\tstrbuf_repo_worktree_path(&gitdir, r, \"%s/.git\", sub->path);\n> +\t\tif (repo_init(ret, gitdir.buf, NULL)) {\n> +\t\t\tstrbuf_release(&gitdir);\n\nYou should also free ret here.\n\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t\tstrbuf_release(&gitdir);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\nThe above is the rest of get_submodule_repo_for(), so that we can see\nthat gitdir is indeed freed.\n\n> -\t\tif (!git_dir)\n> -\t\t\tgit_dir = submodule_git_dir.buf;\n> -\t\tif (is_directory(git_dir)) {\n> +\t\trepo = get_submodule_repo_for(spf->r, submodule);\n> +\t\tif (repo) {\n> \t\t\tchild_process_init(cp);\n> -\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n> \t\t\tprepare_submodule_repo_env(&cp->env_array);\n> +\t\t\tcp->dir = xstrdup(repo->worktree);\n\nMove the cp->dir one line up for a cleaner diff.\n\n[snip]\n\n> +\t\t\trepo_clear(repo);\n> +\t\t\tfree(repo);\n> \t\t\tret = 1;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * An empty directory is normal,\n> +\t\t\t * the submodule is not initialized\n> +\t\t\t */\n> +\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n> +\t\t\t !is_empty_dir(ce->name))\n> +\t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n\nPreviously, a failed fetch would just set spf->result = 1 (in\nfetch_finish()), allowing other fetches to still proceed. This sounds\nlike a better idea to me instead of die-ing outright. (Also remember to\nprint a warning message - since we no longer spawn a child process to\ntry a fetch that will fail, we need to print a message ourselves.)\n"},{"id":"361645","messageId":"20181026204106.132296-1-jonathantanmy@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-10-sbeller@google.com","subject":"Re: [PATCH 09/10] fetch: try fetching submodules if needed objects were not fetched","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-26T20:41:06Z","receivedAt":"2018-10-26T20:41:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> But this default fetch is not sufficient, as a newly fetched commit in\n> the superproject could point to a commit in the submodule that is not\n> in the default refspec. This is common in workflows like Gerrit's.\n> When fetching a Gerrit change under review (from refs/changes/??), the\n> commits in that change likely point to submodule commits that have not\n> been merged to a branch yet.\n> \n> Try fetching a submodule by object id if the object id that the\n> superproject points to, cannot be found.\n\nI see that these suggestions of mine (from [1]) was implemented, but not\nothers. If you disagree, that's fine, but I think they should be\ndiscussed.\n\n[1] https://public-inbox.org/git/20181018003954.139498-1-jonathantanmy@google.com/\n\n> The try does not happen when the \"git fetch\" done at the\n> superproject is not storing the fetched results in remote\n> tracking branches (i.e. instead just recording them to\n> FETCH_HEAD) in this step. A later patch will fix this.\n\nE.g. here, I said that there was no remote tracking branch involved.\n\n> -\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n> -\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n> +\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n\nI think the next patch should be squashed into this patch. Then you can\nsay that these are now redundant and can be removed.\n\n> @@ -1218,8 +1218,12 @@ struct submodule_parallel_fetch {\n> \tint result;\n> \n> \tstruct string_list changed_submodule_names;\n> +\tstruct get_next_submodule_task **fetch_specific_oids;\n> +\tint fetch_specific_oids_nr, fetch_specific_oids_alloc;\n> };\n\nAdd documentation for fetch_specific_oids. Also, it might be better to\ncall these oid_fetch_tasks and the struct, \"struct fetch_task\".\n\nHere, struct get_next_submodule_task is used for 2 different things:\n (1) After the first round fetch, fetch_finish() uses it to determine if\n a second round is needed.\n (2) In submodule_parallel_fetch.fetch_specific_oids, to tell the\n parallel runner (through get_next_submodule_task()) to do this\n fetch.\n\nI think that it's better to have 2 different structs for these 2\ndifferent uses. (Note that task_cb can be NULL for the second round.\nUnless we plan to recheck the OIDs? Currently we recheck them, but we\ndon't do anything either way.)\n\n> +static const struct submodule *get_default_submodule(const char *path)\n> +{\n> +\tstruct submodule *ret = NULL;\n> +\tconst char *name = default_name_or_path(path);\n> +\n> +\tif (!name)\n> +\t\treturn NULL;\n> +\n> +\tret = xmalloc(sizeof(*ret));\n> +\tmemset(ret, 0, sizeof(*ret));\n> +\tret->path = name;\n> +\tret->name = name;\n> +\n> +\treturn (const struct submodule *) ret;\n> +}\n\nI think that this is best described as the submodule that has no entry\nin .gitmodules? Maybe call it \"get_non_gitmodules_submodule\" and\ndocument it with a similar comment as in get_submodule_repo_for().\n\n> +\n> +static struct get_next_submodule_task *get_next_submodule_task_create(\n> +\tstruct repository *r, const char *path)\n> +{\n> +\tstruct get_next_submodule_task *task = xmalloc(sizeof(*task));\n> +\tmemset(task, 0, sizeof(*task));\n> +\n> +\ttask->sub = submodule_from_path(r, &null_oid, path);\n> +\tif (!task->sub) {\n> +\t\ttask->sub = get_default_submodule(path);\n> +\t\ttask->free_sub = 1;\n> +\t}\n> +\n> +\treturn task;\n> +}\n\nClearer to me would be to make get_next_submodule_task_create() return\nNULL if submodule_from_path() returns NULL.\n\n> +\tif (spf->fetch_specific_oids_nr) {\n> +\t\tstruct get_next_submodule_task *task = spf->fetch_specific_oids[spf->fetch_specific_oids_nr - 1];\n\nBreak lines to 80.\n\n> +\t\targv_array_pushv(&cp->args, spf->args.argv);\n> +\t\targv_array_push(&cp->args, \"on-demand\");\n\nSame comment about \"on-demand\" as in my previous e-mail.\n\n> +\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n> +\t\targv_array_push(&cp->args, submodule_prefix.buf);\n> +\n> +\t\t/* NEEDSWORK: have get_default_remote from s--h */\n\nSame comment about \"s--h\" as in my previous e-mail.\n\n> +\tcommits = it->util;\n> +\toid_array_filter(commits,\n> +\t\t\t commit_exists_in_sub,\n> +\t\t\t task->repo);\n> +\n> +\t/* Are there commits that do not exist? */\n> +\tif (commits->nr) {\n> +\t\t/* We already tried fetching them, do not try again. */\n> +\t\tif (task->commits)\n> +\t\t\treturn 0;\n\nSame comment about \"task->commits\" as in my previous e-mail.\n\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 6c2f9b2ba2..5a75b57852 100755\n\nOne more thing to test is the case where a submodule doesn't have a\n.gitmodules entry.\n"},{"id":"361646","messageId":"20181026204249.132969-1-jonathantanmy@google.com","threadId":"49684","inReplyTo":"20181025233231.102245-11-sbeller@google.com","subject":"Re: [PATCH 10/10] builtin/fetch: check for submodule updates in any ref update","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-26T20:42:49Z","receivedAt":"2018-10-26T20:42:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Gerrit, the code review tool, has a different workflow than our mailing\n> list based approach. Usually users upload changes to a Gerrit server and\n> continuous integration and testing happens by bots. Sometimes however a\n> user wants to checkout a change locally and look at it locally. For this\n> use case, Gerrit offers a command line snippet to copy and paste to your\n> terminal, which looks like\n\nAs I said in my review of the previous patch, I think this should be\nsquashed into the previous patch.\n\nAlso, remember to add the version when formatting the e-mail (\"[PATCH\nv? ??/??]\").\n"},{"id":"361650","messageId":"CAGZ79kYK57rtmYuKUfWsX0_AAMZJqu7UeL2_P=iKN-9LPRWnfg@mail.gmail.com","threadId":"49684","inReplyTo":"20181026191553.108916-1-jonathantanmy@google.com","subject":"Re: [PATCH 06/10] repository: repo_submodule_init to take a submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-26T22:01:01Z","receivedAt":"2018-10-26T22:01:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 26, 2018 at 12:15 PM Jonathan Tan wrote:\n\n [snip]\n\n> The expected pattern.\n>\n> This patch looks good to me.\n\nI'll take this as a \"Reviewed-by\"?\n\nThanks,\nStefan\n"},{"id":"361866","messageId":"xmqqo9bd76xh.fsf@gitster-ct.c.googlers.com","threadId":"49684","inReplyTo":"20181025233231.102245-1-sbeller@google.com","subject":"Re: [PATCH 00/10] Resending sb/submodule-recursive-fetch-gets-the-tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-29T03:44:42Z","receivedAt":"2018-10-29T03:44:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Thanks Jonathan for your thoughtful comments,\n> here is the product of the discussion:\n\nCan you use v$n in the subject (if these rerolls are meant for\napplication and not primarily for discussion, that is)? It quickly\ngets confusing to see many messages with the same subject from\ndifferent attempts.\n\n> This is still based on ao/submodule-wo-gitmodules-checked-out.\n\n... which just got updated but I think only one test script changed,\nso I expect there won't be anything unexpected when I queue this on\ntop of a merge of that topic and master.\n\nIt seems that the first three preparatory steps haven't changed,\npatch #4-#7 have moderate amount of changes, and the previous #8 got\nupdated quite a lot. Let's see how well this and updated base topic\nwill play with other topics in flight.\n\nThanks.\n\n> previous version\n> https://public-inbox.org/git/20181016181327.107186-1-sbeller@google.com/\n>\n> Stefan Beller (10):\n> sha1-array: provide oid_array_filter\n> submodule.c: fix indentation\n> submodule.c: sort changed_submodule_names before searching it\n> submodule.c: tighten scope of changed_submodule_names struct\n> submodule: store OIDs in changed_submodule_names\n> repository: repo_submodule_init to take a submodule struct\n> submodule: migrate get_next_submodule to use repository structs\n> submodule.c: fetch in submodules git directory instead of in worktree\n> fetch: try fetching submodules if needed objects were not fetched\n> builtin/fetch: check for submodule updates in any ref update\n>\n> Documentation/technical/api-oid-array.txt | 5 +\n> builtin/fetch.c | 11 +-\n> builtin/grep.c | 17 +-\n> builtin/ls-files.c | 12 +-\n> builtin/submodule--helper.c | 2 +-\n> repository.c | 27 +-\n> repository.h | 12 +-\n> sha1-array.c | 17 ++\n> sha1-array.h | 3 +\n> submodule.c | 265 ++++++++++++++++---\n> t/helper/test-submodule-nested-repo-config.c | 8 +-\n> t/t5526-fetch-submodules.sh | 55 ++++\n> 12 files changed, 346 insertions(+), 88 deletions(-)\n>\n> git range-diff origin/sb/submodule-recursive-fetch-gets-the-tip... \n>\n> 1: 3fbb06aedd ! 1: 0fdb0e2ad9 submodule.c: move global changed_submodule_names into fetch submodule struct\n> @@ -1,12 +1,11 @@\n> Author: Stefan Beller \n> \n> - submodule.c: move global changed_submodule_names into fetch submodule struct\n> + submodule.c: tighten scope of changed_submodule_names struct\n> \n> The `changed_submodule_names` are only used for fetching, so let's make it\n> part of the struct that is passed around for fetching submodules.\n> \n> Signed-off-by: Stefan Beller \n> - Signed-off-by: Junio C Hamano \n> \n> diff --git a/submodule.c b/submodule.c\n> --- a/submodule.c\n> @@ -16,7 +15,6 @@\n> \n> static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n> -static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n> -+\n> static int initialized_fetch_ref_tips;\n> static struct oid_array ref_tips_before_fetch;\n> static struct oid_array ref_tips_after_fetch;\n> @@ -25,22 +23,8 @@\n> }\n> \n> -static void calculate_changed_submodule_paths(void)\n> -+struct submodule_parallel_fetch {\n> -+\tint count;\n> -+\tstruct argv_array args;\n> -+\tstruct repository *r;\n> -+\tconst char *prefix;\n> -+\tint command_line_option;\n> -+\tint default_option;\n> -+\tint quiet;\n> -+\tint result;\n> -+\n> -+\tstruct string_list changed_submodule_names;\n> -+};\n> -+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n> -+\n> +static void calculate_changed_submodule_paths(\n> -+\tstruct submodule_parallel_fetch *spf)\n> ++\tstruct string_list *changed_submodule_names)\n> {\n> \tstruct argv_array argv = ARGV_ARRAY_INIT;\n> \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n> @@ -49,30 +33,23 @@\n> \n> \t\tif (!submodule_has_commits(path, commits))\n> -\t\t\tstring_list_append(&changed_submodule_names, name->string);\n> -+\t\t\tstring_list_append(&spf->changed_submodule_names,\n> ++\t\t\tstring_list_append(changed_submodule_names,\n> +\t\t\t\t\t name->string);\n> \t}\n> \n> \tfree_submodules_oids(&changed_submodules);\n> @@\n> - \treturn ret;\n> - }\n> - \n> --struct submodule_parallel_fetch {\n> --\tint count;\n> --\tstruct argv_array args;\n> --\tstruct repository *r;\n> --\tconst char *prefix;\n> --\tint command_line_option;\n> --\tint default_option;\n> --\tint quiet;\n> --\tint result;\n> --};\n> + \tint default_option;\n> + \tint quiet;\n> + \tint result;\n> ++\n> ++\tstruct string_list changed_submodule_names;\n> + };\n> -#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}\n> --\n> ++#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n> + \n> static int get_fetch_recurse_config(const struct submodule *submodule,\n> \t\t\t\t struct submodule_parallel_fetch *spf)\n> - {\n> @@\n> \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n> \t\t\tif (!submodule ||\n> @@ -88,7 +65,7 @@\n> \n> -\tcalculate_changed_submodule_paths();\n> -\tstring_list_sort(&changed_submodule_names);\n> -+\tcalculate_changed_submodule_paths(&spf);\n> ++\tcalculate_changed_submodule_paths(&spf.changed_submodule_names);\n> +\tstring_list_sort(&spf.changed_submodule_names);\n> \trun_processes_parallel(max_parallel_jobs,\n> \t\t\t get_next_submodule,\n> 2: a64a8322a1 ! 2: a11e6e39a2 submodule.c: do not copy around submodule list\n> @@ -1,6 +1,6 @@\n> Author: Stefan Beller \n> \n> - submodule.c: do not copy around submodule list\n> + submodule: store OIDs in changed_submodule_names\n> \n> 'calculate_changed_submodule_paths' uses a local list to compute the\n> changed submodules, and then produces the result by copying appropriate\n> @@ -14,13 +14,13 @@\n> useful in a later patch.\n> \n> Signed-off-by: Stefan Beller \n> - Signed-off-by: Junio C Hamano \n> + Reviewed-by: Jonathan Tan \n> \n> diff --git a/submodule.c b/submodule.c\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@\n> - \tstruct submodule_parallel_fetch *spf)\n> + \tstruct string_list *changed_submodule_names)\n> {\n> \tstruct argv_array argv = ARGV_ARRAY_INIT;\n> -\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n> @@ -34,10 +34,10 @@\n> \t * commits have been recorded upstream in \"changed_submodule_names\".\n> \t */\n> -\tcollect_changed_submodules(&changed_submodules, &argv);\n> -+\tcollect_changed_submodules(&spf->changed_submodule_names, &argv);\n> ++\tcollect_changed_submodules(changed_submodule_names, &argv);\n> \n> -\tfor_each_string_list_item(name, &changed_submodules) {\n> -+\tfor_each_string_list_item(name, &spf->changed_submodule_names) {\n> ++\tfor_each_string_list_item(name, changed_submodule_names) {\n> \t\tstruct oid_array *commits = name->util;\n> \t\tconst struct submodule *submodule;\n> \t\tconst char *path = NULL;\n> @@ -46,7 +46,7 @@\n> \t\t\tcontinue;\n> \n> -\t\tif (!submodule_has_commits(path, commits))\n> --\t\t\tstring_list_append(&spf->changed_submodule_names,\n> +-\t\t\tstring_list_append(changed_submodule_names,\n> -\t\t\t\t\t name->string);\n> +\t\tif (submodule_has_commits(path, commits)) {\n> +\t\t\toid_array_clear(commits);\n> @@ -55,7 +55,7 @@\n> \t}\n> \n> -\tfree_submodules_oids(&changed_submodules);\n> -+\tstring_list_remove_empty_items(&spf->changed_submodule_names, 1);\n> ++\tstring_list_remove_empty_items(changed_submodule_names, 1);\n> +\n> \targv_array_clear(&argv);\n> \toid_array_clear(&ref_tips_before_fetch);\n> 3: 9341b92c83 ! 3: 3f4e0d4b8d repository: repo_submodule_init to take a submodule struct\n> @@ -5,17 +5,19 @@\n> When constructing a struct repository for a submodule for some revision\n> of the superproject where the submodule is not contained in the index,\n> it may not be present in the working tree currently either. In that\n> - siutation giving a 'path' argument is not useful. Upgrade the\n> + situation giving a 'path' argument is not useful. Upgrade the\n> repo_submodule_init function to take a struct submodule instead.\n> + The submodule struct can be obtained via submodule_from_{path, name} or\n> + an artificial submodule struct can be passed in.\n> \n> - While we are at it, overhaul the repo_submodule_init function by renaming\n> - the submodule repository struct, which is to be initialized, to a name\n> - that is not confused with the struct submodule as easily.\n> + While we are at it, rename the repository struct in the repo_submodule_init\n> + function, which is to be initialized, to a name that is not confused with\n> + the struct submodule as easily. Perform such renames in similar functions\n> + as well.\n> \n> Also move its documentation into the header file.\n> \n> Signed-off-by: Stefan Beller \n> - Signed-off-by: Junio C Hamano \n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> --- a/builtin/grep.c\n> @@ -104,6 +106,19 @@\n> \n> static void show_ce(struct repository *repo, struct dir_struct *dir,\n> \n> +diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> +--- a/builtin/submodule--helper.c\n> ++++ b/builtin/submodule--helper.c\n> +@@\n> + \tif (!sub)\n> + \t\tBUG(\"We could get the submodule handle before?\");\n> + \n> +-\tif (repo_submodule_init(&subrepo, the_repository, path))\n> ++\tif (repo_submodule_init(&subrepo, the_repository, sub))\n> + \t\tdie(_(\"could not get a repository handle for submodule '%s'\"), path);\n> + \n> + \tif (!repo_config_get_string(&subrepo, \"core.worktree\", &cw)) {\n> +\n> diff --git a/repository.c b/repository.c\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -178,7 +193,8 @@\n> +/*\n> + * Initialize the repository 'subrepo' as the submodule given by the\n> + * struct submodule 'sub' in parent repository 'superproject'.\n> -+ * Return 0 upon success and a non-zero value upon failure.\n> ++ * Return 0 upon success and a non-zero value upon failure, which may happen\n> ++ * if the submodule is not found, or 'sub' is NULL.\n> + */\n> +struct submodule;\n> +int repo_submodule_init(struct repository *subrepo,\n> 4: cea909cbd4 ! 4: b1566069e7 submodule: fetch in submodules git directory instead of in worktree\n> @@ -1,44 +1,19 @@\n> Author: Stefan Beller \n> \n> - submodule: fetch in submodules git directory instead of in worktree\n> + submodule: migrate get_next_submodule to use repository structs\n> \n> - This patch started as a refactoring to make 'get_next_submodule' more\n> - readable, but upon doing so, I realized that \"git fetch\" of the submodule\n> - actually doesn't need to be run in the submodules worktree. So let's run\n> - it in its git dir instead.\n> + We used to recurse into submodules, even if they were broken having\n> + only an objects directory. The child process executed in the submodule\n> + would fail though if the submodule was broken.\n> \n> - That should pave the way towards fetching submodules that are currently\n> - not checked out.\n> -\n> - This patch leaks the cp->dir in get_next_submodule, as any further\n> - callback in run_processes_parallel doesn't have access to the child\n> - process any more. In an early iteration of this patch, the function\n> - get_submodule_repo_for directly returned the string containing the\n> - git directory, which would be a better design choice for this patch.\n> -\n> - However the next patch both fixes the memory leak of cp->dir and also has\n> - a use case for using the full repository handle of the submodule, so\n> - it makes sense to introduce the get_submodule_repo_for here already.\n> + This patch tightens the check upfront, such that we do not need\n> + to spawn a child process to find out if the submodule is broken.\n> \n> Signed-off-by: Stefan Beller \n> - Signed-off-by: Junio C Hamano \n> \n> diff --git a/submodule.c b/submodule.c\n> --- a/submodule.c\n> +++ b/submodule.c\n> -@@\n> - \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n> - }\n> - \n> -+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)\n> -+{\n> -+\tprepare_submodule_repo_env_no_git_dir(out);\n> -+\targv_array_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n> -+}\n> -+\n> - /* Helper function to display the submodule header line prior to the full\n> - * summary output. If it can locate the submodule objects directory it will\n> - * attempt to lookup both the left and right commits and put them into the\n> @@\n> \treturn spf->default_option;\n> }\n> @@ -99,9 +74,8 @@\n> +\t\tif (repo) {\n> \t\t\tchild_process_init(cp);\n> -\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n> --\t\t\tprepare_submodule_repo_env(&cp->env_array);\n> -+\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> -+\t\t\tcp->dir = xstrdup(repo->gitdir);\n> + \t\t\tprepare_submodule_repo_env(&cp->env_array);\n> ++\t\t\tcp->dir = xstrdup(repo->worktree);\n> \t\t\tcp->git_cmd = 1;\n> \t\t\tif (!spf->quiet)\n> \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n> @@ -113,27 +87,17 @@\n> +\t\t\trepo_clear(repo);\n> +\t\t\tfree(repo);\n> \t\t\tret = 1;\n> ++\t\t} else {\n> ++\t\t\t/*\n> ++\t\t\t * An empty directory is normal,\n> ++\t\t\t * the submodule is not initialized\n> ++\t\t\t */\n> ++\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n> ++\t\t\t !is_empty_dir(ce->name))\n> ++\t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n> \t\t}\n> -\t\tstrbuf_release(&submodule_path);\n> -\t\tstrbuf_release(&submodule_git_dir);\n> \t\tstrbuf_release(&submodule_prefix);\n> \t\tif (ret) {\n> \t\t\tspf->count++;\n> -\n> -diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> ---- a/t/t5526-fetch-submodules.sh\n> -+++ b/t/t5526-fetch-submodules.sh\n> -@@\n> - \n> - \ttest_must_fail git -C dst status &&\n> - \ttest_must_fail git -C dst diff &&\n> --\ttest_must_fail git -C dst fetch --recurse-submodules\n> -+\n> -+\t# git-fetch cannot find the git directory of the submodule,\n> -+\t# so it will do nothing, successfully, as it cannot distinguish between\n> -+\t# this broken submodule and a submodule that was just set active but\n> -+\t# not cloned yet\n> -+\tgit -C dst fetch --recurse-submodules\n> - '\n> - \n> - test_expect_success \"fetch new commits when submodule got renamed\" '\n> -: ---------- > 5: 2d98ff1201 submodule.c: fetch in submodules git directory instead of in worktree\n> 5: 9ad0125310 ! 6: 092b9cbcba fetch: retry fetching submodules if needed objects were not fetched\n> @@ -1,25 +1,23 @@\n> Author: Stefan Beller \n> \n> - fetch: retry fetching submodules if needed objects were not fetched\n> + fetch: try fetching submodules if needed objects were not fetched\n> \n> Currently when git-fetch is asked to recurse into submodules, it dispatches\n> a plain \"git-fetch -C \" (with some submodule related options\n> such as prefix and recusing strategy, but) without any information of the\n> remote or the tip that should be fetched.\n> \n> - This works surprisingly well in some workflows (such as using submodules\n> - as a third party library), while not so well in other scenarios, such\n> - as in a Gerrit topic-based workflow, that can tie together changes\n> - (potentially across repositories) on the server side. One of the parts\n> - of such a Gerrit workflow is to download a change when wanting to examine\n> - it, and you'd want to have its submodule changes that are in the same\n> - topic downloaded as well. However these submodule changes reside in their\n> - own repository in their own ref (refs/changes/).\n> + But this default fetch is not sufficient, as a newly fetched commit in\n> + the superproject could point to a commit in the submodule that is not\n> + in the default refspec. This is common in workflows like Gerrit's.\n> + When fetching a Gerrit change under review (from refs/changes/??), the\n> + commits in that change likely point to submodule commits that have not\n> + been merged to a branch yet.\n> \n> - Retry fetching a submodule by object name if the object id that the\n> + Try fetching a submodule by object id if the object id that the\n> superproject points to, cannot be found.\n> \n> - This retrying does not happen when the \"git fetch\" done at the\n> + The try does not happen when the \"git fetch\" done at the\n> superproject is not storing the fetched results in remote\n> tracking branches (i.e. instead just recording them to\n> FETCH_HEAD) in this step. A later patch will fix this.\n> @@ -32,7 +30,6 @@\n> in case the submodule recursion is set to \"on\".\n> \n> Signed-off-by: Stefan Beller \n> - Signed-off-by: Junio C Hamano \n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> --- a/builtin/fetch.c\n> @@ -75,16 +72,16 @@\n> \tint result;\n> \n> \tstruct string_list changed_submodule_names;\n> -+\tstruct get_next_submodule_task **retry;\n> -+\tint retry_nr, retry_alloc;\n> ++\tstruct get_next_submodule_task **fetch_specific_oids;\n> ++\tint fetch_specific_oids_nr, fetch_specific_oids_alloc;\n> };\n> -#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n> +#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \\\n> +\t\t STRING_LIST_INIT_DUP, \\\n> +\t\t NULL, 0, 0}\n> \n> - static void calculate_changed_submodule_paths(\n> - \tstruct submodule_parallel_fetch *spf)\n> + static int get_fetch_recurse_config(const struct submodule *submodule,\n> + \t\t\t\t struct submodule_parallel_fetch *spf)\n> @@\n> \treturn spf->default_option;\n> }\n> @@ -93,6 +90,8 @@\n> +\tstruct repository *repo;\n> +\tconst struct submodule *sub;\n> +\tunsigned free_sub : 1; /* Do we need to free the submodule? */\n> ++\n> ++\t/* fetch specific oids if set, otherwise fetch default refspec */\n> +\tstruct oid_array *commits;\n> +};\n> +\n> @@ -224,24 +223,29 @@\n> -\t\t\trepo_clear(repo);\n> -\t\t\tfree(repo);\n> -\t\t\tret = 1;\n> --\t\t}\n> --\t\tstrbuf_release(&submodule_prefix);\n> --\t\tif (ret) {\n> - \t\t\tspf->count++;\n> ++\t\t\tspf->count++;\n> +\t\t\t*task_cb = task;\n> +\n> +\t\t\tstrbuf_release(&submodule_prefix);\n> - \t\t\treturn 1;\n> -+\t\t} else {\n> ++\t\t\treturn 1;\n> + \t\t} else {\n> ++\n> +\t\t\tget_next_submodule_task_release(task);\n> +\t\t\tfree(task);\n> ++\n> + \t\t\t/*\n> + \t\t\t * An empty directory is normal,\n> + \t\t\t * the submodule is not initialized\n> +@@\n> + \t\t\t !is_empty_dir(ce->name))\n> + \t\t\t\tdie(_(\"Could not access submodule '%s'\"), ce->name);\n> \t\t}\n> - \t}\n> ++\t}\n> +\n> -+\tif (spf->retry_nr) {\n> -+\t\tstruct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n> ++\tif (spf->fetch_specific_oids_nr) {\n> ++\t\tstruct get_next_submodule_task *task = spf->fetch_specific_oids[spf->fetch_specific_oids_nr - 1];\n> +\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n> -+\t\tspf->retry_nr--;\n> ++\t\tspf->fetch_specific_oids_nr--;\n> +\n> +\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, task->sub->path);\n> +\n> @@ -262,9 +266,13 @@\n> +\t\t\t\t\t append_oid_to_argv, &cp->args);\n> +\n> +\t\t*task_cb = task;\n> -+\t\tstrbuf_release(&submodule_prefix);\n> + \t\tstrbuf_release(&submodule_prefix);\n> +-\t\tif (ret) {\n> +-\t\t\tspf->count++;\n> +-\t\t\treturn 1;\n> +-\t\t}\n> +\t\treturn 1;\n> -+\t}\n> + \t}\n> +\n> \treturn 0;\n> }\n> @@ -326,9 +334,11 @@\n> +\t\t\treturn 0;\n> +\n> +\t\ttask->commits = commits;\n> -+\t\tALLOC_GROW(spf->retry, spf->retry_nr + 1, spf->retry_alloc);\n> -+\t\tspf->retry[spf->retry_nr] = task;\n> -+\t\tspf->retry_nr++;\n> ++\t\tALLOC_GROW(spf->fetch_specific_oids,\n> ++\t\t\t spf->fetch_specific_oids_nr + 1,\n> ++\t\t\t spf->fetch_specific_oids_alloc);\n> ++\t\tspf->fetch_specific_oids[spf->fetch_specific_oids_nr] = task;\n> ++\t\tspf->fetch_specific_oids_nr++;\n> +\t\treturn 0;\n> +\t}\n> +\n> @@ -346,18 +356,33 @@\n> \ttest_cmp expect actual\n> '\n> \n> -+test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n> ++test_expect_success \"fetch new submodule commits on-demand outside standard refspec\" '\n> ++\t# add a second submodule and ensure it is around in downstream first\n> ++\tgit clone submodule sub1 &&\n> ++\tgit submodule add ./sub1 &&\n> ++\tgit commit -m \"adding a second submodule\" &&\n> ++\tgit -C downstream pull &&\n> ++\tgit -C downstream submodule update --init --recursive &&\n> ++\n> +\tgit checkout --detach &&\n> ++\n> +\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n> +\tgit -C submodule update-ref refs/changes/1 $C &&\n> +\tgit update-index --cacheinfo 160000 $C submodule &&\n> -+\tgit commit -m \"updated submodule outside of refs/heads\" &&\n> -+\tD=$(git rev-parse HEAD) &&\n> -+\tgit update-ref refs/changes/2 $D &&\n> ++\ttest_tick &&\n> ++\n> ++\tD=$(git -C sub1 commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n> ++\tgit -C sub1 update-ref refs/changes/2 $D &&\n> ++\tgit update-index --cacheinfo 160000 $D sub1 &&\n> ++\n> ++\tgit commit -m \"updated submodules outside of refs/heads\" &&\n> ++\tE=$(git rev-parse HEAD) &&\n> ++\tgit update-ref refs/changes/2 $E &&\n> +\t(\n> +\t\tcd downstream &&\n> -+\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n> ++\t\tgit fetch --recurse-submodules origin refs/changes/2:refs/heads/my_branch &&\n> +\t\tgit -C submodule cat-file -t $C &&\n> ++\t\tgit -C sub1 cat-file -t $D &&\n> +\t\tgit checkout --recurse-submodules FETCH_HEAD\n> +\t)\n> +'\n> 6: b8db3b45bf < -: ---------- builtin/fetch: check for submodule updates for non branch fetches\n> 7: 905a4f0d4f < -: ---------- fixup! repository: repo_submodule_init to take a submodule struct\n> -: ---------- > 7: 11bf819782 builtin/fetch: check for submodule updates in any ref update\n"},{"id":"364276","messageId":"CAGZ79kYAyf9-9dNaaok-z-7WT9E-dC7RX4Sxd-9HBc2Qvr5mgw@mail.gmail.com","threadId":"49684","inReplyTo":"20181026204106.132296-1-jonathantanmy@google.com","subject":"Re: [PATCH 09/10] fetch: try fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T00:30:34Z","receivedAt":"2018-11-29T00:30:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 26, 2018 at 1:41 PM Jonathan Tan wrote:\n>\n> > But this default fetch is not sufficient, as a newly fetched commit in\n> > the superproject could point to a commit in the submodule that is not\n> > in the default refspec. This is common in workflows like Gerrit's.\n> > When fetching a Gerrit change under review (from refs/changes/??), the\n> > commits in that change likely point to submodule commits that have not\n> > been merged to a branch yet.\n> >\n> > Try fetching a submodule by object id if the object id that the\n> > superproject points to, cannot be found.\n>\n> I see that these suggestions of mine (from [1]) was implemented, but not\n> others. If you disagree, that's fine, but I think they should be\n> discussed.\n\nok.\n\n>\n> > - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n> > - (recurse_submodules != RECURSE_SUBMODULES_ON))\n> > + if (recurse_submodules != RECURSE_SUBMODULES_OFF)\n>\n> I think the next patch should be squashed into this patch. Then you can\n> say that these are now redundant and can be removed.\n\nok.\n\n>\n> > @@ -1218,8 +1218,12 @@ struct submodule_parallel_fetch {\n> > int result;\n> >\n> > struct string_list changed_submodule_names;\n> > + struct get_next_submodule_task **fetch_specific_oids;\n> > + int fetch_specific_oids_nr, fetch_specific_oids_alloc;\n> > };\n>\n> Add documentation for fetch_specific_oids. Also, it might be better to\n> call these oid_fetch_tasks and the struct, \"struct fetch_task\".\n\nok.\n\n> Here, struct get_next_submodule_task is used for 2 different things:\n> (1) After the first round fetch, fetch_finish() uses it to determine if\n> a second round is needed.\n> (2) In submodule_parallel_fetch.fetch_specific_oids, to tell the\n> parallel runner (through get_next_submodule_task()) to do this\n> fetch.\n>\n> I think that it's better to have 2 different structs for these 2\n> different uses. (Note that task_cb can be NULL for the second round.\n> Unless we plan to recheck the OIDs? Currently we recheck them, but we\n> don't do anything either way.)\n\nI think it is easier to only have one struct until we have substantially\nmore to communicate. (1) is a boolean answer, for which (non-)NULL\nis sufficient.\n\n\n> I think that this is best described as the submodule that has no entry\n> in .gitmodules? Maybe call it \"get_non_gitmodules_submodule\" and\n> document it with a similar comment as in get_submodule_repo_for().\n\ndone.\n\n>\n> > +\n> > +static struct get_next_submodule_task *get_next_submodule_task_create(\n> > + struct repository *r, const char *path)\n> > +{\n> > + struct get_next_submodule_task *task = xmalloc(sizeof(*task));\n> > + memset(task, 0, sizeof(*task));\n> > +\n> > + task->sub = submodule_from_path(r, &null_oid, path);\n> > + if (!task->sub) {\n> > + task->sub = get_default_submodule(path);\n> > + task->free_sub = 1;\n> > + }\n> > +\n> > + return task;\n> > +}\n>\n> Clearer to me would be to make get_next_submodule_task_create() return\n> NULL if submodule_from_path() returns NULL.\n\nI doubled down on this one and return NULL when get_default_submodule\n(now renamed to get_non_gitmodules_submodule) returns NULL, as then we\ncan move the free() from get_next_submodule here and there we'll just have\n\n task = fetch_task_create(spf->r, ce->name);\n if (!task)\n continue;\n\nwhich helps get_next_submodule to stay readable.\n\n\n> Same comment about \"on-demand\" as in my previous e-mail.\n\nI'd want to push back on refactoring and defer that to a later series.\n\n> Break lines to 80.\n[...]\n> Same comment about \"s--h\" as in my previous e-mail.\n\ndone\n\n> > + /* Are there commits that do not exist? */\n> > + if (commits->nr) {\n> > + /* We already tried fetching them, do not try again. */\n> > + if (task->commits)\n> > + return 0;\n>\n> Same comment about \"task->commits\" as in my previous e-mail.\n\nGood call. I reordered the function read easier and added a comment\non any early return how it could happen.\n\n>\n> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> > index 6c2f9b2ba2..5a75b57852 100755\n>\n> One more thing to test is the case where a submodule doesn't have a\n> .gitmodules entry.\n\nadded a test.\n\nI just resent the series.\n\nStefan\n"}]}