v4:
* Per Ævars comment, moved the docs for oid_array_filter into
Documentation/technical/...
* addressed all outstanding comment as noted in "What's cooking" email,
* below is a range-diff against the currently queued version of the
series.
v3:
* I discovered some issues with v2 after sending,
which is why I rewrote the later patches completely
and now we pass around a "task" struct that contains everything to know
about the things to work on and what needs free()ing afterwards.
* as it is no longer string list based, this drops adding string_list_{pop, last}
v2:
* extended commit messages,
* plugged a memory leak
* rewrote the patch "sha1-array: provide oid_array_filter" to be much more like
object_array_fiter
* fixed a typo pointed out by Ramsay.
The range diff is below.
Thanks,
Stefan
v1:
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C <submodule-dir>" (and some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
This works surprisingly well in some workflows, not so well in others,
which this series aims to fix.
The first patches provide new basic functionality and do some refactoring;
the interesting part is in the two last patches.
This was discussed in
https://public-inbox.org/git/20180808221752.195419-1-sbeller@xxxxxxxxxx/
and I think I addressed all feedback so far.
Stefan Beller (9):
sha1-array: provide oid_array_filter
submodule.c: fix indentation
submodule.c: sort changed_submodule_names before searching it
submodule: move global changed_submodule_names into fetch submodule
struct
submodule.c: do not copy around submodule list
repository: repo_submodule_init to take a submodule struct
submodule: fetch in submodules git directory instead of in worktree
fetch: retry fetching submodules if needed objects were not fetched
builtin/fetch: check for submodule updates for non branch fetches
Documentation/technical/api-oid-array.txt | 5 +
builtin/fetch.c | 14 +-
builtin/grep.c | 17 +-
builtin/ls-files.c | 12 +-
builtin/submodule--helper.c | 2 +-
repository.c | 27 +--
repository.h | 11 +-
sha1-array.c | 17 ++
sha1-array.h | 3 +
submodule.c | 275 +++++++++++++++++-----
t/t5526-fetch-submodules.sh | 23 +-
11 files changed, 311 insertions(+), 95 deletions(-)
git range-diff origin/sb/submodule-recursive-fetch-gets-the-tip...
1: 6fecf7cd01a < -: ----------- string-list: add string_list_{pop, last} functions
2: 7007a318a68 < -: ----------- sha1-array: provide oid_array_filter
[ ... snip ... I rebased onto: ]
-: ----------- > 215: fe8321ec057 Second batch post 2.19
-: ----------- > 216: a9b49d4cfe9 sha1-array: provide oid_array_filter
3: 807429234ac ! 217: 813205700d1 submodule.c: fix indentation
@@ -6,7 +6,6 @@
Fix it while we are here.
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
4: f6fa5273af9 ! 218: b4aa77f72ba submodule.c: sort changed_submodule_names before searching it
@@ -12,7 +12,6 @@
appropriate.
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
5: adf7a2fd203 ! 219: df4da0e18e4 submodule: move global changed_submodule_names into fetch submodule struct
@@ -6,13 +6,12 @@
part of the struct that is passed around for fetching submodules.
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
+++ b/submodule.c
@@
- #include "object-store.h"
+ #include "commit-reach.h"
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
6: 56c9398589a ! 220: b9f5d06c134 submodule.c: do not copy around submodule list
@@ -9,12 +9,11 @@
Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.
- By doin so we'll have access to the util pointer for longer that
+ By doing so we'll have access to the util pointer for longer that
contains the commits that we need to fetch, which will be
useful in a later patch.
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
-: ----------- > 221: e5ff287a0a2 repository: repo_submodule_init to take a submodule struct
7: 9f70a5f32c9 ! 222: 8acd734b5f5 submodule: fetch in submodules git directory instead of in worktree
@@ -3,14 +3,24 @@
submodule: fetch in submodules git directory instead of in worktree
This patch started as a refactoring to make 'get_next_submodule' more
- readable, but upon doing so, I realized that git-fetch actually doesn't
- need to be run in the worktree. So let's run it in the git dir instead.
+ readable, but upon doing so, I realized that "git fetch" of the submodule
+ actually doesn't need to be run in the submodules worktree. So let's run
+ it in its git dir instead.
That should pave the way towards fetching submodules that are currently
not checked out.
+ This patch leaks the cp->dir in get_next_submodule, as any further
+ callback in run_processes_parallel doesn't have access to the child
+ process any more. In an early iteration of this patch, the function
+ get_submodule_repo_for directly returned the string containing the
+ git directory, which would be a better design choice for this patch.
+
+ However the next patch both fixes the memory leak of cp->dir and also has
+ a use case for using the full repository handle of the submodule, so
+ it makes sense to introduce the get_submodule_repo_for here already.
+
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
@@ -32,24 +42,26 @@
return spf->default_option;
}
-+static const char *get_submodule_git_dir(struct repository *r, const char *path)
++static struct repository *get_submodule_repo_for(struct repository *r,
++ const struct submodule *sub)
+{
-+ struct repository subrepo;
-+ const char *ret;
++ struct repository *ret = xmalloc(sizeof(*ret));
+
-+ if (repo_submodule_init(&subrepo, r, path)) {
-+ /* no entry in .gitmodules? */
++ if (repo_submodule_init(ret, r, sub)) {
++ /*
++ * No entry in .gitmodules? Technically not a submodule,
++ * but historically we supported repositories that happen to be
++ * in-place where a gitlink is. Keep supporting them.
++ */
+ struct strbuf gitdir = STRBUF_INIT;
-+ strbuf_repo_worktree_path(&gitdir, r, "%s/.git", path);
-+ if (repo_init(&subrepo, gitdir.buf, NULL)) {
++ strbuf_repo_worktree_path(&gitdir, r, "%s/.git", sub->path);
++ if (repo_init(ret, gitdir.buf, NULL)) {
+ strbuf_release(&gitdir);
+ return NULL;
+ }
++ strbuf_release(&gitdir);
+ }
+
-+ ret = xstrdup(subrepo.gitdir);
-+ repo_clear(&subrepo);
-+
+ return ret;
+}
+
@@ -64,7 +76,13 @@
- struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
- const char *git_dir, *default_argv;
+- const char *git_dir, *default_argv;
++ const char *default_argv;
+ const struct submodule *submodule;
++ struct repository *repo;
+ struct submodule default_submodule = SUBMODULE_INIT;
+
+ if (!S_ISGITLINK(ce->ce_mode))
@@
continue;
}
@@ -76,18 +94,23 @@
- if (!git_dir)
- git_dir = submodule_git_dir.buf;
- if (is_directory(git_dir)) {
-+ git_dir = get_submodule_git_dir(spf->r, ce->name);
-+ if (git_dir) {
++ repo = get_submodule_repo_for(spf->r, submodule);
++ if (repo) {
child_process_init(cp);
- cp->dir = strbuf_detach(&submodule_path, NULL);
- prepare_submodule_repo_env(&cp->env_array);
+ prepare_submodule_repo_env_in_gitdir(&cp->env_array);
-+ cp->dir = git_dir;
++ cp->dir = xstrdup(repo->gitdir);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@
+ argv_array_push(&cp->args, default_argv);
+ argv_array_push(&cp->args, "--submodule-prefix");
argv_array_push(&cp->args, submodule_prefix.buf);
++
++ repo_clear(repo);
++ free(repo);
ret = 1;
}
- strbuf_release(&submodule_path);
8: bab609b4dc1 ! 223: 5752ba212a7 fetch: retry fetching submodules if sha1 were not fetched
@@ -1,9 +1,9 @@
Author: Stefan Beller <sbeller@xxxxxxxxxx>
- fetch: retry fetching submodules if sha1 were not fetched
+ fetch: retry fetching submodules if needed objects were not fetched
Currently when git-fetch is asked to recurse into submodules, it dispatches
- a plain "git-fetch -C <submodule-dir>" (and some submodule related options
+ a plain "git-fetch -C <submodule-dir>" (with some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
@@ -16,16 +16,22 @@
topic downloaded as well. However these submodule changes reside in their
own repository in their own ref (refs/changes/<int>).
- Retry fetching a submodule if the object id that the superproject points
- to, cannot be found.
+ Retry fetching a submodule by object name if the object id that the
+ superproject points to, cannot be found.
- This doesn't support fetching to FETCH_HEAD yet, but only into a local
- branch. To make fetching into FETCH_HEAD work, we need some refactoring
- in builtin/fetch.c to adjust the calls to 'check_for_new_submodule_commits'
- that is coming in the next patch.
+ This retrying does not happen when the "git fetch" done at the
+ superproject is not storing the fetched results in remote
+ tracking branches (i.e. instead just recording them to
+ FETCH_HEAD) in this step. A later patch will fix this.
+
+ builtin/fetch used to only inspect submodules when they were fetched
+ "on-demand", as in either on/off case it was clear whether the submodule
+ needs to be fetched. However to know whether we need to try fetching the
+ object ids, we need to identify the object names, which is done in this
+ function check_for_new_submodule_commits(), so we'll also run that code
+ in case the submodule recursion is set to "on".
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
diff --git a/builtin/fetch.c b/builtin/fetch.c
--- a/builtin/fetch.c
@@ -68,68 +74,181 @@
int result;
struct string_list changed_submodule_names;
-+ struct string_list retry;
++ struct get_next_submodule_task **retry;
++ int retry_nr, retry_alloc;
};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
+ STRING_LIST_INIT_DUP, \
-+ STRING_LIST_INIT_NODUP}
++ NULL, 0, 0}
static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
@@
+ return spf->default_option;
+ }
+
++struct get_next_submodule_task {
++ struct repository *repo;
++ const struct submodule *sub;
++ unsigned free_sub : 1; /* Do we need to free the submodule? */
++ struct oid_array *commits;
++};
++
++static const struct submodule *get_default_submodule(const char *path)
++{
++ struct submodule *ret = NULL;
++ const char *name = default_name_or_path(path);
++
++ if (!name)
++ return NULL;
++
++ ret = xmalloc(sizeof(*ret));
++ memset(ret, 0, sizeof(*ret));
++ ret->path = name;
++ ret->name = name;
++
++ return (const struct submodule *) ret;
++}
++
++static struct get_next_submodule_task *get_next_submodule_task_create(
++ struct repository *r, const char *path)
++{
++ struct get_next_submodule_task *task = xmalloc(sizeof(*task));
++ memset(task, 0, sizeof(*task));
++
++ task->sub = submodule_from_path(r, &null_oid, path);
++ if (!task->sub) {
++ task->sub = get_default_submodule(path);
++ task->free_sub = 1;
++ }
++
++ return task;
++}
++
++static void get_next_submodule_task_release(struct get_next_submodule_task *p)
++{
++ if (p->free_sub)
++ free((void*)p->sub);
++ p->free_sub = 0;
++ p->sub = NULL;
++
++ if (p->repo)
++ repo_clear(p->repo);
++ FREE_AND_NULL(p->repo);
++}
++
+ static struct repository *get_submodule_repo_for(struct repository *r,
+ const struct submodule *sub)
+ {
+@@
+ static int get_next_submodule(struct child_process *cp,
+ struct strbuf *err, void *data, void **task_cb)
{
- int ret = 0;
+- int ret = 0;
struct submodule_parallel_fetch *spf = data;
-+ struct string_list_item *it;
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
+- struct strbuf submodule_prefix = STRBUF_INIT;
+ int recurse_config;
- struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
- const char *git_dir, *default_argv;
-@@
- }
+ const char *default_argv;
+- const struct submodule *submodule;
+- struct repository *repo;
+- struct submodule default_submodule = SUBMODULE_INIT;
++ struct get_next_submodule_task *task;
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+- submodule = submodule_from_path(spf->r, &null_oid, ce->name);
+- if (!submodule) {
+- const char *name = default_name_or_path(ce->name);
+- if (name) {
+- default_submodule.path = name;
+- default_submodule.name = name;
+- submodule = &default_submodule;
+- }
++ task = get_next_submodule_task_create(spf->r, ce->name);
++
++ if (!task->sub) {
++ free(task);
++ continue;
}
- switch (get_fetch_recurse_config(submodule, spf))
-+ recurse_config = get_fetch_recurse_config(submodule, spf);
++ recurse_config = get_fetch_recurse_config(task->sub, spf);
+
+ switch (recurse_config)
{
default:
case RECURSE_SUBMODULES_DEFAULT:
+ case RECURSE_SUBMODULES_ON_DEMAND:
+- if (!submodule ||
++ if (!task->sub ||
+ !string_list_lookup(
+ &spf->changed_submodule_names,
+- submodule->name))
++ task->sub->name))
+ continue;
+ default_argv = "on-demand";
+ break;
+@@
+ continue;
+ }
+
+- strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, ce->name);
+- repo = get_submodule_repo_for(spf->r, submodule);
+- if (repo) {
++ task->repo = get_submodule_repo_for(spf->r, task->sub);
++ if (task->repo) {
++ struct strbuf submodule_prefix = STRBUF_INIT;
+ child_process_init(cp);
+ prepare_submodule_repo_env_in_gitdir(&cp->env_array);
+- cp->dir = xstrdup(repo->gitdir);
++ cp->dir = task->repo->gitdir;
+ cp->git_cmd = 1;
+ if (!spf->quiet)
+ strbuf_addf(err, "Fetching submodule %s%s\n",
@@
- strbuf_release(&submodule_prefix);
- if (ret) {
+ argv_array_pushv(&cp->args, spf->args.argv);
+ argv_array_push(&cp->args, default_argv);
+ argv_array_push(&cp->args, "--submodule-prefix");
++
++ strbuf_addf(&submodule_prefix, "%s%s/",
++ spf->prefix,
++ task->sub->path);
+ argv_array_push(&cp->args, submodule_prefix.buf);
+
+- repo_clear(repo);
+- free(repo);
+- ret = 1;
+- }
+- strbuf_release(&submodule_prefix);
+- if (ret) {
spf->count++;
-+ if (submodule != &default_submodule)
-+ /* discard const-ness: */
-+ *task_cb = (void*)submodule;
++ *task_cb = task;
++
++ strbuf_release(&submodule_prefix);
return 1;
++ } else {
++ get_next_submodule_task_release(task);
++ free(task);
}
}
+
-+retry_next:
-+
-+ if (spf->retry.nr) {
++ if (spf->retry_nr) {
++ struct get_next_submodule_task *task = spf->retry[spf->retry_nr - 1];
+ struct strbuf submodule_prefix = STRBUF_INIT;
-+ const struct submodule *sub;
++ spf->retry_nr--;
+
-+ it = string_list_last(&spf->retry);
-+ sub = submodule_from_name(spf->r, &null_oid,
-+ it->string);
++ strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, task->sub->path);
+
+ child_process_init(cp);
-+ cp->dir = get_submodule_git_dir(spf->r, sub->path);
-+ if (!cp->dir) {
-+ string_list_pop(&spf->retry, 0);
-+ goto retry_next;
-+ }
+ prepare_submodule_repo_env_in_gitdir(&cp->env_array);
+ cp->git_cmd = 1;
++ cp->dir = task->repo->gitdir;
+
-+ strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, sub->path);
+ argv_array_init(&cp->args);
+ argv_array_pushv(&cp->args, spf->args.argv);
+ argv_array_push(&cp->args, "on-demand");
@@ -138,12 +257,11 @@
+
+ /* NEEDSWORK: have get_default_remote from s--h */
+ argv_array_push(&cp->args, "origin");
-+ oid_array_for_each_unique(it->util,
++ oid_array_for_each_unique(task->commits,
+ append_oid_to_argv, &cp->args);
+
-+ *task_cb = NULL; /* make sure we do not recurse forever */
++ *task_cb = task;
+ strbuf_release(&submodule_prefix);
-+ string_list_pop(&spf->retry, 0);
+ return 1;
+ }
+
@@ -151,6 +269,14 @@
}
@@
+ void *cb, void *task_cb)
+ {
+ struct submodule_parallel_fetch *spf = cb;
++ struct get_next_submodule_task *task = task_cb;
+
+ spf->result = 1;
+
++ get_next_submodule_task_release(task);
return 0;
}
@@ -167,35 +293,46 @@
void *cb, void *task_cb)
{
struct submodule_parallel_fetch *spf = cb;
-+ struct submodule *sub = task_cb;
-+ struct repository subrepo;
++ struct get_next_submodule_task *task = task_cb;
++ const struct submodule *sub;
++
++ struct string_list_item *it;
++ struct oid_array *commits;
if (retvalue)
spf->result = 1;
-+ if (!sub)
++ if (!task)
+ return 0;
+
-+ if (repo_submodule_init(&subrepo, spf->r, sub->path) < 0)
-+ warning(_("Could not get submodule repository for submodule '%s' in repository '%s'"),
-+ sub->path, spf->r->worktree);
-+ else {
-+ struct string_list_item *it;
-+ struct oid_array *commits;
++ sub = task->sub;
++ if (!sub)
++ goto out;
+
-+ it = string_list_lookup(&spf->changed_submodule_names, sub->name);
-+ if (!it)
-+ return 0;
++ it = string_list_lookup(&spf->changed_submodule_names, sub->name);
++ if (!it)
++ goto out;
++
++ commits = it->util;
++ oid_array_filter(commits,
++ commit_exists_in_sub,
++ task->repo);
+
-+ commits = it->util;
-+ oid_array_filter(commits,
-+ commit_exists_in_sub,
-+ &subrepo);
++ /* Are there commits that do not exist? */
++ if (commits->nr) {
++ /* We already tried fetching them, do not try again. */
++ if (task->commits)
++ return 0;
+
-+ if (commits->nr)
-+ string_list_append(&spf->retry, sub->name)
-+ ->util = commits;
++ task->commits = commits;
++ ALLOC_GROW(spf->retry, spf->retry_nr + 1, spf->retry_alloc);
++ spf->retry[spf->retry_nr] = task;
++ spf->retry_nr++;
++ return 0;
+ }
++
++out:
++ get_next_submodule_task_release(task);
+
return 0;
}
9: c16d21313f6 ! 224: d02ee9ef485 builtin/fetch: check for submodule updates for non branch fetches
@@ -2,11 +2,29 @@
builtin/fetch: check for submodule updates for non branch fetches
- For Gerrit users that use submodules the invocation of fetch without a
- branch is their main use case.
+ Gerrit, the code review tool, has a different workflow than our mailing
+ list based approach. Usually users upload changes to a Gerrit server and
+ continuous integration and testing happens by bots. Sometimes however a
+ user wants to checkout a change locally and look at it locally. For this
+ use case, Gerrit offers a command line snippet to copy and paste to your
+ terminal, which looks like
+
+ git fetch https://<host>/gerrit refs/changes/<id> &&
+ git checkout FETCH_HEAD
+
+ For Gerrit changes that contain changing submodule gitlinks, it would be
+ easy to extend both the fetch and checkout with the '--recurse-submodules'
+ flag, such that this command line snippet would produce the state of a
+ change locally.
+
+ However the functionality added in the previous patch, which would
+ ensure that we fetch the objects in the submodule that the gitlink pointed
+ at, only works for remote tracking branches so far, not for FETCH_HEAD.
+
+ Make sure that fetching a superproject to its FETCH_HEAD, also respects
+ the existence checks for objects in the submodule recursion.
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
- Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
diff --git a/builtin/fetch.c b/builtin/fetch.c
--- a/builtin/fetch.c
