{"thread":{"id":"49421","subject":"[PATCH v4 0/9] fetch: make sure submodule oids are fetched","startedAt":"2018-09-25T19:48:06Z","lastAt":"2018-09-26T22:19:18Z","messageCount":14,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":9},"messages":[{"id":"358857","messageId":"20180925194755.105578-1-sbeller@google.com","threadId":"49421","inReplyTo":null,"subject":"[PATCH v4 0/9] fetch: make sure submodule oids are fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:46Z","receivedAt":"2018-09-25T19:48:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v4:\n* Per Ævars comment, moved the docs for oid_array_filter into\n  Documentation/technical/...\n* addressed all outstanding comment as noted in \"What's cooking\" email,\n* below is a range-diff against the currently queued version of the\n  series.\n\nv3:\n* I discovered some issues with v2 after sending,\n  which is why I rewrote the later patches completely\n  and now we pass around a \"task\" struct that contains everything to know\n  about the things to work on and what needs free()ing afterwards.\n* as it is no longer string list based, this drops adding string_list_{pop, last}\n\nv2:\n* extended commit messages,\n* plugged a memory leak\n* rewrote the patch \"sha1-array: provide oid_array_filter\" to be much more like \n  object_array_fiter\n* fixed a typo pointed out by Ramsay.\n\nThe range diff is below.\n  \nThanks,\nStefan\n\nv1:\nCurrently when git-fetch is asked to recurse into submodules, it dispatches\na plain \"git-fetch -C <submodule-dir>\" (and some submodule related options\nsuch as prefix and recusing strategy, but) without any information of the\nremote or the tip that should be fetched.\n\nThis works surprisingly well in some workflows, not so well in others,\nwhich this series aims to fix.\n\nThe first patches provide new basic functionality and do some refactoring;\nthe interesting part is in the two last patches.\n\nThis was discussed in\nhttps://public-inbox.org/git/20180808221752.195419-1-sbeller@google.com/\nand I think I addressed all feedback so far.\n\nStefan Beller (9):\n  sha1-array: provide oid_array_filter\n  submodule.c: fix indentation\n  submodule.c: sort changed_submodule_names before searching it\n  submodule: move global changed_submodule_names into fetch submodule\n    struct\n  submodule.c: do not copy around submodule list\n  repository: repo_submodule_init to take a submodule struct\n  submodule: fetch in submodules git directory instead of in worktree\n  fetch: retry fetching submodules if needed objects were not fetched\n  builtin/fetch: check for submodule updates for non branch fetches\n\n Documentation/technical/api-oid-array.txt |   5 +\n builtin/fetch.c                           |  14 +-\n builtin/grep.c                            |  17 +-\n builtin/ls-files.c                        |  12 +-\n builtin/submodule--helper.c               |   2 +-\n repository.c                              |  27 +--\n repository.h                              |  11 +-\n sha1-array.c                              |  17 ++\n sha1-array.h                              |   3 +\n submodule.c                               | 275 +++++++++++++++++-----\n t/t5526-fetch-submodules.sh               |  23 +-\n 11 files changed, 311 insertions(+), 95 deletions(-)\n\ngit range-diff  origin/sb/submodule-recursive-fetch-gets-the-tip...\n\n  1:  6fecf7cd01a <   -:  ----------- string-list: add string_list_{pop, last} functions\n  2:  7007a318a68 <   -:  ----------- sha1-array: provide oid_array_filter\n\n    [ ... snip ... I rebased onto: ]\n\n  -:  ----------- > 215:  fe8321ec057 Second batch post 2.19\n  -:  ----------- > 216:  a9b49d4cfe9 sha1-array: provide oid_array_filter\n  3:  807429234ac ! 217:  813205700d1 submodule.c: fix indentation\n    @@ -6,7 +6,6 @@\n         Fix it while we are here.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      diff --git a/submodule.c b/submodule.c\n      --- a/submodule.c\n  4:  f6fa5273af9 ! 218:  b4aa77f72ba submodule.c: sort changed_submodule_names before searching it\n    @@ -12,7 +12,6 @@\n         appropriate.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      diff --git a/submodule.c b/submodule.c\n      --- a/submodule.c\n  5:  adf7a2fd203 ! 219:  df4da0e18e4 submodule: move global changed_submodule_names into fetch submodule struct\n    @@ -6,13 +6,12 @@\n         part of the struct that is passed around for fetching submodules.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      diff --git a/submodule.c b/submodule.c\n      --- a/submodule.c\n      +++ b/submodule.c\n     @@\n    - #include \"object-store.h\"\n    + #include \"commit-reach.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  6:  56c9398589a ! 220:  b9f5d06c134 submodule.c: do not copy around submodule list\n    @@ -9,12 +9,11 @@\n         Instead use the result list directly and prune items afterwards\n         using string_list_remove_empty_items.\n     \n    -    By doin so we'll have access to the util pointer for longer that\n    +    By doing so we'll have access to the util pointer for longer that\n         contains the commits that we need to fetch, which will be\n         useful in a later patch.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      diff --git a/submodule.c b/submodule.c\n      --- a/submodule.c\n  -:  ----------- > 221:  e5ff287a0a2 repository: repo_submodule_init to take a submodule struct\n  7:  9f70a5f32c9 ! 222:  8acd734b5f5 submodule: fetch in submodules git directory instead of in worktree\n    @@ -3,14 +3,24 @@\n         submodule: fetch in submodules git directory instead of in worktree\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 actually doesn't\n    -    need to be run in the worktree. So let's run it in the git dir instead.\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     \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    +\n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      diff --git a/submodule.c b/submodule.c\n      --- a/submodule.c\n    @@ -32,24 +42,26 @@\n      \treturn spf->default_option;\n      }\n      \n    -+static const char *get_submodule_git_dir(struct repository *r, const char *path)\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 subrepo;\n    -+\tconst char *ret;\n    ++\tstruct repository *ret = xmalloc(sizeof(*ret));\n     +\n    -+\tif (repo_submodule_init(&subrepo, r, path)) {\n    -+\t\t/* no entry in .gitmodules? */\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\", path);\n    -+\t\tif (repo_init(&subrepo, gitdir.buf, NULL)) {\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    -+\tret = xstrdup(subrepo.gitdir);\n    -+\trepo_clear(&subrepo);\n    -+\n     +\treturn ret;\n     +}\n     +\n    @@ -64,7 +76,13 @@\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 *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     @@\n      \t\t\tcontinue;\n      \t\t}\n    @@ -76,18 +94,23 @@\n     -\t\tif (!git_dir)\n     -\t\t\tgit_dir = submodule_git_dir.buf;\n     -\t\tif (is_directory(git_dir)) {\n    -+\t\tgit_dir = get_submodule_git_dir(spf->r, ce->name);\n    -+\t\tif (git_dir) {\n    ++\t\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\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n    -+\t\t\tcp->dir = git_dir;\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     @@\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}\n     -\t\tstrbuf_release(&submodule_path);\n  8:  bab609b4dc1 ! 223:  5752ba212a7 fetch: retry fetching submodules if sha1 were not fetched\n    @@ -1,9 +1,9 @@\n     Author: Stefan Beller <sbeller@google.com>\n     \n    -    fetch: retry fetching submodules if sha1 were not fetched\n    +    fetch: retry 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 <submodule-dir>\" (and some submodule related options\n    +    a plain \"git-fetch -C <submodule-dir>\" (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    @@ -16,16 +16,22 @@\n         topic downloaded as well. However these submodule changes reside in their\n         own repository in their own ref (refs/changes/<int>).\n     \n    -    Retry fetching a submodule if the object id that the superproject points\n    -    to, cannot be found.\n    +    Retry fetching a submodule by object name if the object id that the\n    +    superproject points to, cannot be found.\n     \n    -    This doesn't support fetching to FETCH_HEAD yet, but only into a local\n    -    branch. To make fetching into FETCH_HEAD work, we need some refactoring\n    -    in builtin/fetch.c to adjust the calls to 'check_for_new_submodule_commits'\n    -    that is coming in the next patch.\n    +    This retrying 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    +\n    +    builtin/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\n    +    needs to be fetched. However to know whether we need to try fetching the\n    +    object ids, we need to identify the object names, which is done in this\n    +    function check_for_new_submodule_commits(), so we'll also run that code\n    +    in case the submodule recursion is set to \"on\".\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      diff --git a/builtin/fetch.c b/builtin/fetch.c\n      --- a/builtin/fetch.c\n    @@ -68,68 +74,181 @@\n      \tint result;\n      \n      \tstruct string_list changed_submodule_names;\n    -+\tstruct string_list retry;\n    ++\tstruct get_next_submodule_task **retry;\n    ++\tint retry_nr, retry_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  STRING_LIST_INIT_NODUP}\n    ++\t\t  NULL, 0, 0}\n      \n      static void calculate_changed_submodule_paths(\n      \tstruct submodule_parallel_fetch *spf)\n     @@\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    ++\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    +@@\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    +-\tint ret = 0;\n      \tstruct submodule_parallel_fetch *spf = data;\n    -+\tstruct string_list_item *it;\n      \n      \tfor (; spf->count < spf->r->index->cache_nr; spf->count++) {\n    +-\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n     +\t\tint recurse_config;\n    - \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n      \t\tconst struct cache_entry *ce = spf->r->index->cache[spf->count];\n    - \t\tconst char *git_dir, *default_argv;\n    -@@\n    - \t\t\t}\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(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    +@@\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     @@\n    - \t\tstrbuf_release(&submodule_prefix);\n    - \t\tif (ret) {\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}\n    +-\t\tstrbuf_release(&submodule_prefix);\n    +-\t\tif (ret) {\n      \t\t\tspf->count++;\n    -+\t\t\tif (submodule != &default_submodule)\n    -+\t\t\t\t/* discard const-ness: */\n    -+\t\t\t\t*task_cb = (void*)submodule;\n    ++\t\t\t*task_cb = task;\n    ++\n    ++\t\t\tstrbuf_release(&submodule_prefix);\n      \t\t\treturn 1;\n    ++\t\t} else {\n    ++\t\t\tget_next_submodule_task_release(task);\n    ++\t\t\tfree(task);\n      \t\t}\n      \t}\n     +\n    -+retry_next:\n    -+\n    -+\tif (spf->retry.nr) {\n    ++\tif (spf->retry_nr) {\n    ++\t\tstruct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n     +\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n    -+\t\tconst struct submodule *sub;\n    ++\t\tspf->retry_nr--;\n     +\n    -+\t\tit = string_list_last(&spf->retry);\n    -+\t\tsub = submodule_from_name(spf->r, &null_oid,\n    -+\t\t\t\t\t  it->string);\n    ++\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, task->sub->path);\n     +\n     +\t\tchild_process_init(cp);\n    -+\t\tcp->dir = get_submodule_git_dir(spf->r, sub->path);\n    -+\t\tif (!cp->dir) {\n    -+\t\t\tstring_list_pop(&spf->retry, 0);\n    -+\t\t\tgoto retry_next;\n    -+\t\t}\n     +\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n     +\t\tcp->git_cmd = 1;\n    ++\t\tcp->dir = task->repo->gitdir;\n     +\n    -+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, sub->path);\n     +\t\targv_array_init(&cp->args);\n     +\t\targv_array_pushv(&cp->args, spf->args.argv);\n     +\t\targv_array_push(&cp->args, \"on-demand\");\n    @@ -138,12 +257,11 @@\n     +\n     +\t\t/* NEEDSWORK: have get_default_remote from s--h */\n     +\t\targv_array_push(&cp->args, \"origin\");\n    -+\t\toid_array_for_each_unique(it->util,\n    ++\t\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 = NULL; /* make sure we do not recurse forever */\n    ++\t\t*task_cb = task;\n     +\t\tstrbuf_release(&submodule_prefix);\n    -+\t\tstring_list_pop(&spf->retry, 0);\n     +\t\treturn 1;\n     +\t}\n     +\n    @@ -151,6 +269,14 @@\n      }\n      \n     @@\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    @@ -167,35 +293,46 @@\n      \t\t\tvoid *cb, void *task_cb)\n      {\n      \tstruct submodule_parallel_fetch *spf = cb;\n    -+\tstruct submodule *sub = task_cb;\n    -+\tstruct repository subrepo;\n    ++\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 (!sub)\n    ++\tif (!task)\n     +\t\treturn 0;\n     +\n    -+\tif (repo_submodule_init(&subrepo, spf->r, sub->path) < 0)\n    -+\t\twarning(_(\"Could not get submodule repository for submodule '%s' in repository '%s'\"),\n    -+\t\t\t  sub->path, spf->r->worktree);\n    -+\telse {\n    -+\t\tstruct string_list_item *it;\n    -+\t\tstruct oid_array *commits;\n    ++\tsub = task->sub;\n    ++\tif (!sub)\n    ++\t\tgoto out;\n     +\n    -+\t\tit = string_list_lookup(&spf->changed_submodule_names, sub->name);\n    -+\t\tif (!it)\n    -+\t\t\treturn 0;\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\tcommits = it->util;\n    -+\t\toid_array_filter(commits,\n    -+\t\t\t\t commit_exists_in_sub,\n    -+\t\t\t\t &subrepo);\n    ++\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\tif (commits->nr)\n    -+\t\t\tstring_list_append(&spf->retry, sub->name)\n    -+\t\t\t\t->util = commits;\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\treturn 0;\n     +\t}\n    ++\n    ++out:\n    ++\tget_next_submodule_task_release(task);\n     +\n      \treturn 0;\n      }\n  9:  c16d21313f6 ! 224:  d02ee9ef485 builtin/fetch: check for submodule updates for non branch fetches\n    @@ -2,11 +2,29 @@\n     \n         builtin/fetch: check for submodule updates for non branch fetches\n     \n    -    For Gerrit users that use submodules the invocation of fetch without a\n    -    branch is their main use case.\n    +    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    +\n    +      git fetch https://<host>/gerrit refs/changes/<id> &&\n    +      git checkout FETCH_HEAD\n    +\n    +    For Gerrit changes that contain changing submodule gitlinks, it would be\n    +    easy to extend both the fetch and checkout with the '--recurse-submodules'\n    +    flag, such that this command line snippet would produce the state of a\n    +    change locally.\n    +\n    +    However the functionality added in the previous patch, which would\n    +    ensure that we fetch the objects in the submodule that the gitlink pointed\n    +    at, only works for remote tracking branches so far, not for FETCH_HEAD.\n    +\n    +    Make sure that fetching a superproject to its FETCH_HEAD, also respects\n    +    the existence checks for objects in the submodule recursion.\n     \n         Signed-off-by: Stefan Beller <sbeller@google.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      diff --git a/builtin/fetch.c b/builtin/fetch.c\n      --- a/builtin/fetch.c\n"},{"id":"358858","messageId":"20180925194755.105578-2-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 1/9] sha1-array: provide oid_array_filter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:47Z","receivedAt":"2018-09-25T19:48:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Helped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\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 9febfb1d528..c97428c2c34 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 b94e0ec0f5e..d922e94e3fc 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 232bf950172..55d016c4bf7 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.605.g01d371f741-goog\n\n"},{"id":"358859","messageId":"20180925194755.105578-3-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 2/9] submodule.c: fix indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:48Z","receivedAt":"2018-09-25T19:48:10Z","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 <sbeller@google.com>\n---\n submodule.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b53cb6e9c47..0de9e2800ad 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1244,7 +1244,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (!submodule) {\n \t\t\tconst char *name = default_name_or_path(ce->name);\n \t\t\tif (name) {\n-\t\t\t\tdefault_submodule.path = default_submodule.name = name;\n+\t\t\t\tdefault_submodule.path = name;\n+\t\t\t\tdefault_submodule.name = name;\n \t\t\t\tsubmodule = &default_submodule;\n \t\t\t}\n \t\t}\n@@ -1254,8 +1255,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.605.g01d371f741-goog\n\n"},{"id":"358860","messageId":"20180925194755.105578-4-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 3/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:49Z","receivedAt":"2018-09-25T19:48:11Z","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\nTo pick which one is more appropriate, we notice the fact\nthat we discover new items more or less in the already\nsorted order.  That makes \"append then sort\" more\nappropriate.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0de9e2800ad..22c64bd8559 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1256,7 +1256,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@@ -1350,6 +1350,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.605.g01d371f741-goog\n\n"},{"id":"358861","messageId":"20180925194755.105578-5-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 4/9] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:50Z","receivedAt":"2018-09-25T19:48:15Z","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 <sbeller@google.com>\n---\n submodule.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 22c64bd8559..17103379ba4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -25,7 +25,7 @@\n #include \"commit-reach.h\"\n \n static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n+\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n@@ -1110,7 +1110,22 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \toid_array_append(&ref_tips_after_fetch, oid);\n }\n \n-static void calculate_changed_submodule_paths(void)\n+struct submodule_parallel_fetch {\n+\tint count;\n+\tstruct argv_array args;\n+\tstruct repository *r;\n+\tconst char *prefix;\n+\tint command_line_option;\n+\tint default_option;\n+\tint quiet;\n+\tint result;\n+\n+\tstruct string_list changed_submodule_names;\n+};\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n+\n+static void calculate_changed_submodule_paths(\n+\tstruct submodule_parallel_fetch *spf)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n@@ -1148,7 +1163,8 @@ static void calculate_changed_submodule_paths(void)\n \t\t\tcontinue;\n \n \t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&changed_submodule_names, name->string);\n+\t\t\tstring_list_append(&spf->changed_submodule_names,\n+\t\t\t\t\t   name->string);\n \t}\n \n \tfree_submodules_oids(&changed_submodules);\n@@ -1185,18 +1201,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n \treturn ret;\n }\n \n-struct submodule_parallel_fetch {\n-\tint count;\n-\tstruct argv_array args;\n-\tstruct repository *r;\n-\tconst char *prefix;\n-\tint command_line_option;\n-\tint default_option;\n-\tint quiet;\n-\tint result;\n-};\n-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}\n-\n static int get_fetch_recurse_config(const struct submodule *submodule,\n \t\t\t\t    struct submodule_parallel_fetch *spf)\n {\n@@ -1257,7 +1261,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@@ -1349,8 +1353,8 @@ int fetch_populated_submodules(struct repository *r,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-\tcalculate_changed_submodule_paths();\n-\tstring_list_sort(&changed_submodule_names);\n+\tcalculate_changed_submodule_paths(&spf);\n+\tstring_list_sort(&spf.changed_submodule_names);\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t       get_next_submodule,\n \t\t\t       fetch_start_failure,\n@@ -1359,7 +1363,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.605.g01d371f741-goog\n\n"},{"id":"358862","messageId":"20180925194755.105578-6-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 5/9] submodule.c: do not copy around submodule list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:51Z","receivedAt":"2018-09-25T19:48:16Z","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 <sbeller@google.com>\n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 17103379ba4..dd478ed70bf 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1128,8 +1128,7 @@ static void calculate_changed_submodule_paths(\n \tstruct submodule_parallel_fetch *spf)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n-\tconst struct string_list_item *name;\n+\tstruct string_list_item *name;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(the_repository, NULL, NULL))\n@@ -1146,9 +1145,9 @@ static void calculate_changed_submodule_paths(\n \t * Collect all submodules (whether checked out or not) for which new\n \t * commits have been recorded upstream in \"changed_submodule_names\".\n \t */\n-\tcollect_changed_submodules(&changed_submodules, &argv);\n+\tcollect_changed_submodules(&spf->changed_submodule_names, &argv);\n \n-\tfor_each_string_list_item(name, &changed_submodules) {\n+\tfor_each_string_list_item(name, &spf->changed_submodule_names) {\n \t\tstruct oid_array *commits = name->util;\n \t\tconst struct submodule *submodule;\n \t\tconst char *path = NULL;\n@@ -1162,12 +1161,14 @@ static void calculate_changed_submodule_paths(\n \t\tif (!path)\n \t\t\tcontinue;\n \n-\t\tif (!submodule_has_commits(path, commits))\n-\t\t\tstring_list_append(&spf->changed_submodule_names,\n-\t\t\t\t\t   name->string);\n+\t\tif (submodule_has_commits(path, commits)) {\n+\t\t\toid_array_clear(commits);\n+\t\t\t*name->string = '\\0';\n+\t\t}\n \t}\n \n-\tfree_submodules_oids(&changed_submodules);\n+\tstring_list_remove_empty_items(&spf->changed_submodule_names, 1);\n+\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n \toid_array_clear(&ref_tips_after_fetch);\n@@ -1363,7 +1364,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.605.g01d371f741-goog\n\n"},{"id":"358863","messageId":"20180925194755.105578-7-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 6/9] repository: repo_submodule_init to take a submodule struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:52Z","receivedAt":"2018-09-25T19:48:19Z","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\nsiutation giving a 'path' argument is not useful. Upgrade the\nrepo_submodule_init function to take a struct submodule instead.\n\nWhile we are at it, overhaul the repo_submodule_init function by renaming\nthe submodule repository struct, which is to be initialized, to a name\nthat is not confused with the struct submodule as easily.\n\nAlso move its documentation into the header file.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\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                | 11 +++++++++--\n 5 files changed, 37 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 601f801158f..81c53c862b1 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -418,16 +418,19 @@ 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 \tif (!is_submodule_active(superproject, path))\n \t\treturn 0;\n \n-\tif (repo_submodule_init(&submodule, superproject, path))\n+\tif (repo_submodule_init(&subrepo, superproject, sub))\n \t\treturn 0;\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@@ -440,7 +443,7 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t * object.\n \t */\n \tgrep_read_lock();\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@@ -465,14 +468,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 7f9919a3623..4d1649c1b3a 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 40844870cfb..1164e0979bd 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 5dd14867181..aabe64ee5d9 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 9f16c42c1ed..a9c7a5baa54 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -116,9 +116,16 @@ 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.\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 /*\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"358864","messageId":"20180925194755.105578-8-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 7/9] submodule: fetch in submodules git directory instead of in worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:53Z","receivedAt":"2018-09-25T19:48:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch started as a refactoring to make 'get_next_submodule' more\nreadable, but upon doing so, I realized that \"git fetch\" of the submodule\nactually doesn't need to be run in the submodules worktree. So let's run\nit in its git dir instead.\n\nThat should pave the way towards fetching submodules that are currently\nnot checked out.\n\nThis patch leaks the cp->dir in get_next_submodule, as any further\ncallback in run_processes_parallel doesn't have access to the child\nprocess any more. In an early iteration of this patch, the function\nget_submodule_repo_for directly returned the string containing the\ngit directory, which would be a better design choice for this patch.\n\nHowever the next patch both fixes the memory leak of cp->dir and also has\na use case for using the full repository handle of the submodule, so\nit makes sense to introduce the get_submodule_repo_for here already.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c                 | 51 +++++++++++++++++++++++++++----------\n t/t5526-fetch-submodules.sh |  7 ++++-\n 2 files changed, 44 insertions(+), 14 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex dd478ed70bf..3f791f22771 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -481,6 +481,12 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)\n+{\n+\tprepare_submodule_repo_env_no_git_dir(out);\n+\targv_array_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n+}\n+\n /* Helper function to display the submodule header line prior to the full\n  * summary output. If it can locate the submodule objects directory it will\n  * attempt to lookup both the left and right commits and put them into the\n@@ -1227,6 +1233,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@@ -1234,12 +1263,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@@ -1274,16 +1302,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\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@@ -1293,10 +1317,11 @@ 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}\n-\t\tstrbuf_release(&submodule_path);\n-\t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 6c2f9b2ba26..42692219a1a 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -566,7 +566,12 @@ test_expect_success 'fetching submodule into a broken repository' '\n \n \ttest_must_fail git -C dst status &&\n \ttest_must_fail git -C dst diff &&\n-\ttest_must_fail git -C dst fetch --recurse-submodules\n+\n+\t# git-fetch cannot find the git directory of the submodule,\n+\t# so it will do nothing, successfully, as it cannot distinguish between\n+\t# this broken submodule and a submodule that was just set active but\n+\t# not cloned yet\n+\tgit -C dst fetch --recurse-submodules\n '\n \n test_expect_success \"fetch new commits when submodule got renamed\" '\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"358865","messageId":"20180925194755.105578-9-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 8/9] fetch: retry fetching submodules if needed objects were not fetched","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:54Z","receivedAt":"2018-09-25T19:48:24Z","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 <submodule-dir>\" (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\nThis works surprisingly well in some workflows (such as using submodules\nas a third party library), while not so well in other scenarios, such\nas in a Gerrit topic-based workflow, that can tie together changes\n(potentially across repositories) on the server side. One of the parts\nof such a Gerrit workflow is to download a change when wanting to examine\nit, and you'd want to have its submodule changes that are in the same\ntopic downloaded as well. However these submodule changes reside in their\nown repository in their own ref (refs/changes/<int>).\n\nRetry fetching a submodule by object name if the object id that the\nsuperproject points to, cannot be found.\n\nThis retrying 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 <sbeller@google.com>\n---\n builtin/fetch.c             |   9 +-\n submodule.c                 | 185 ++++++++++++++++++++++++++++++------\n t/t5526-fetch-submodules.sh |  16 ++++\n 3 files changed, 177 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0696abfc2a1..e3b03ad3bd3 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -707,8 +707,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@@ -723,8 +722,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, &current->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@@ -738,8 +736,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, &current->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 3f791f22771..05799362e05 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1127,8 +1127,12 @@ struct submodule_parallel_fetch {\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 };\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@@ -1233,6 +1237,56 @@ 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+\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@@ -1259,39 +1313,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@@ -1302,12 +1352,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@@ -1316,18 +1366,51 @@ 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}\n-\t\tstrbuf_release(&submodule_prefix);\n-\t\tif (ret) {\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\tget_next_submodule_task_release(task);\n+\t\t\tfree(task);\n \t\t}\n \t}\n+\n+\tif (spf->retry_nr) {\n+\t\tstruct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];\n+\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n+\t\tspf->retry_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\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1335,20 +1418,68 @@ 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->retry, spf->retry_nr + 1, spf->retry_alloc);\n+\t\tspf->retry[spf->retry_nr] = task;\n+\t\tspf->retry_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 42692219a1a..af12c50e7dd 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -605,4 +605,20 @@ test_expect_success \"fetch new commits when submodule got renamed\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n+\tgit checkout --detach &&\n+\tC=$(git -C submodule commit-tree -m \"new change outside refs/heads\" HEAD^{tree}) &&\n+\tgit -C submodule update-ref refs/changes/1 $C &&\n+\tgit update-index --cacheinfo 160000 $C submodule &&\n+\tgit commit -m \"updated submodule outside of refs/heads\" &&\n+\tD=$(git rev-parse HEAD) &&\n+\tgit update-ref refs/changes/2 $D &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n+\t\tgit -C submodule cat-file -t $C &&\n+\t\tgit checkout --recurse-submodules FETCH_HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"358866","messageId":"20180925194755.105578-10-sbeller@google.com","threadId":"49421","inReplyTo":"20180925194755.105578-1-sbeller@google.com","subject":"[PATCH v4 9/9] builtin/fetch: check for submodule updates for non branch fetches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T19:47:55Z","receivedAt":"2018-09-25T19:48:26Z","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://<host>/gerrit refs/changes/<id> &&\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 <sbeller@google.com>\n---\n builtin/fetch.c             | 5 ++++-\n t/t5526-fetch-submodules.sh | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e3b03ad3bd3..f2d9e548bf0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -894,11 +894,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n \t\t\t\t\t\t       summary_width);\n \t\t\t\tfree(ref);\n-\t\t\t} else\n+\t\t\t} else {\n+\t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n \t\t\t\tformat_display(&note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t       *what ? what : \"HEAD\",\n \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n+\t\t\t}\n+\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex af12c50e7dd..a509eabb044 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -615,7 +615,7 @@ test_expect_success \"fetch new commits on-demand when they are not reachable\" '\n \tgit update-ref refs/changes/2 $D &&\n \t(\n \t\tcd downstream &&\n-\t\tgit fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch &&\n+\t\tgit fetch --recurse-submodules origin refs/changes/2 &&\n \t\tgit -C submodule cat-file -t $C &&\n \t\tgit checkout --recurse-submodules FETCH_HEAD\n \t)\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359014","messageId":"xmqq1s9fkisf.fsf@gitster-ct.c.googlers.com","threadId":"49421","inReplyTo":"20180925194755.105578-4-sbeller@google.com","subject":"Re: [PATCH v4 3/9] submodule.c: sort changed_submodule_names before searching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T22:14:56Z","receivedAt":"2018-09-26T22:15:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> We can string_list_insert() to maintain sorted-ness of the\n> list as we find new items, or we can string_list_append() to\n> build an unsorted list and sort it at the end just once.\n>\n> To pick which one is more appropriate, we notice the fact\n> that we discover new items more or less in the already\n> sorted order.  That makes \"append then sort\" more\n> appropriate.\n\nI somehow thought that we agreed that the second paragraph above did\nnot make much sense in the previous review round.\n\n    ... goes and looks ...\n\nhttps://public-inbox.org/git/CAGZ79kbavjVbTqXsmtjW6=jhkq47_p3mc6=92xOp4_mfhqDtvw@mail.gmail.com/\n\nThat was two review cycles ago, I guess.\n"},{"id":"359015","messageId":"xmqqwor7j432.fsf@gitster-ct.c.googlers.com","threadId":"49421","inReplyTo":"20180925194755.105578-2-sbeller@google.com","subject":"Re: [PATCH v4 1/9] sha1-array: provide oid_array_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T22:17:53Z","receivedAt":"2018-09-26T22:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\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\nPerfect ;-)\n\n>\n> diff --git a/Documentation/technical/api-oid-array.txt b/Documentation/technical/api-oid-array.txt\n> index 9febfb1d528..c97428c2c34 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>  \n> diff --git a/sha1-array.c b/sha1-array.c\n> index b94e0ec0f5e..d922e94e3fc 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> +}\n> diff --git a/sha1-array.h b/sha1-array.h\n> index 232bf950172..55d016c4bf7 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"},{"id":"359016","messageId":"xmqqsh1vj42m.fsf@gitster-ct.c.googlers.com","threadId":"49421","inReplyTo":"20180925194755.105578-3-sbeller@google.com","subject":"Re: [PATCH v4 2/9] submodule.c: fix indentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T22:18:09Z","receivedAt":"2018-09-26T22:18:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The submodule subsystem is really bad at staying within 80 characters.\n> Fix it while we are here.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nMakes sense.\n\n>  submodule.c | 9 ++++++---\n>  1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index b53cb6e9c47..0de9e2800ad 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1244,7 +1244,8 @@ static int get_next_submodule(struct child_process *cp,\n>  \t\tif (!submodule) {\n>  \t\t\tconst char *name = default_name_or_path(ce->name);\n>  \t\t\tif (name) {\n> -\t\t\t\tdefault_submodule.path = default_submodule.name = name;\n> +\t\t\t\tdefault_submodule.path = name;\n> +\t\t\t\tdefault_submodule.name = name;\n>  \t\t\t\tsubmodule = &default_submodule;\n>  \t\t\t}\n>  \t\t}\n> @@ -1254,8 +1255,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"},{"id":"359017","messageId":"xmqqo9cjj40u.fsf@gitster-ct.c.googlers.com","threadId":"49421","inReplyTo":"20180925194755.105578-5-sbeller@google.com","subject":"Re: [PATCH v4 4/9] submodule: move global changed_submodule_names into fetch submodule struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T22:19:13Z","receivedAt":"2018-09-26T22:19:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> 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 <sbeller@google.com>\n> ---\n>  submodule.c | 42 +++++++++++++++++++++++-------------------\n>  1 file changed, 23 insertions(+), 19 deletions(-)\n\nYup, the less file-scope static we have and the more of them moved\nto a struct, the closer we get to be able to use multiple of them at\nthe same time, which is a very nice step in the right direction.\n\n>\n> diff --git a/submodule.c b/submodule.c\n> index 22c64bd8559..17103379ba4 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -25,7 +25,7 @@\n>  #include \"commit-reach.h\"\n>  \n>  static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;\n> -static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;\n> +\n>  static int initialized_fetch_ref_tips;\n>  static struct oid_array ref_tips_before_fetch;\n>  static struct oid_array ref_tips_after_fetch;\n> @@ -1110,7 +1110,22 @@ void check_for_new_submodule_commits(struct object_id *oid)\n>  \toid_array_append(&ref_tips_after_fetch, oid);\n>  }\n>  \n> -static void calculate_changed_submodule_paths(void)\n> +struct submodule_parallel_fetch {\n> +\tint count;\n> +\tstruct argv_array args;\n> +\tstruct repository *r;\n> +\tconst char *prefix;\n> +\tint command_line_option;\n> +\tint default_option;\n> +\tint quiet;\n> +\tint result;\n> +\n> +\tstruct string_list changed_submodule_names;\n> +};\n> +#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }\n> +\n> +static void calculate_changed_submodule_paths(\n> +\tstruct submodule_parallel_fetch *spf)\n>  {\n>  \tstruct argv_array argv = ARGV_ARRAY_INIT;\n>  \tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n> @@ -1148,7 +1163,8 @@ static void calculate_changed_submodule_paths(void)\n>  \t\t\tcontinue;\n>  \n>  \t\tif (!submodule_has_commits(path, commits))\n> -\t\t\tstring_list_append(&changed_submodule_names, name->string);\n> +\t\t\tstring_list_append(&spf->changed_submodule_names,\n> +\t\t\t\t\t   name->string);\n>  \t}\n>  \n>  \tfree_submodules_oids(&changed_submodules);\n> @@ -1185,18 +1201,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,\n>  \treturn ret;\n>  }\n>  \n> -struct submodule_parallel_fetch {\n> -\tint count;\n> -\tstruct argv_array args;\n> -\tstruct repository *r;\n> -\tconst char *prefix;\n> -\tint command_line_option;\n> -\tint default_option;\n> -\tint quiet;\n> -\tint result;\n> -};\n> -#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}\n> -\n>  static int get_fetch_recurse_config(const struct submodule *submodule,\n>  \t\t\t\t    struct submodule_parallel_fetch *spf)\n>  {\n> @@ -1257,7 +1261,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> @@ -1349,8 +1353,8 @@ int fetch_populated_submodules(struct repository *r,\n>  \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n>  \t/* default value, \"--submodule-prefix\" and its value are added later */\n>  \n> -\tcalculate_changed_submodule_paths();\n> -\tstring_list_sort(&changed_submodule_names);\n> +\tcalculate_changed_submodule_paths(&spf);\n> +\tstring_list_sort(&spf.changed_submodule_names);\n>  \trun_processes_parallel(max_parallel_jobs,\n>  \t\t\t       get_next_submodule,\n>  \t\t\t       fetch_start_failure,\n> @@ -1359,7 +1363,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"}]}