| Message ID | 20181129002756.167615-8-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Resending sb/submodule-recursive-fetch-gets-the-tip | expand |
> We used to recurse into submodules, even if they were broken having > only an objects directory. The child process executed in the submodule > would fail though if the submodule was broken. This is tested via > "fetching submodule into a broken repository" in t5526. > > This patch tightens the check upfront, such that we do not need > to spawn a child process to find out if the submodule is broken. Thanks, patches 4-7 look good to me - I see that you have addressed all my comments. Not sending one email each for patches 4, 5, and 6 - although I have commented on all of them, my comments were minor. My more in-depth review was done on a previous version [1], and I see that my comments have been addressed. Also, Stefan says [2] (and implements in this patch): > > > If the working tree directory is empty for that submodule, it means > > > it is likely not initialized. But why would we use that as a signal to > > > skip the submodule? > > > > What I meant was: if empty, skip it completely. Otherwise, do the > > repo_submodule_init() and repo_init() thing, and if they both fail, set > > spf->result to 1, preserving existing behavior. > > I did it the other way round: > > If repo_[submodule_]init fails, see if we have a gitlink in tree and > an empty dir in the FS, to decide if we need to signal failure. This works too. [1] https://public-inbox.org/git/20181017225811.66554-1-jonathantanmy@google.com/ [2] https://public-inbox.org/git/CAGZ79kbNXD35ZwevjLZcrGsT=2hNcUPmVUWvP1RjsKSH0Gd3ww@mail.gmail.com/
Hi, Stefan Beller wrote: > This patch tightens the check upfront, such that we do not need > to spawn a child process to find out if the submodule is broken. Sounds sensible. [...] > --- a/submodule.c > +++ b/submodule.c [...] > @@ -1319,10 +1338,23 @@ static int get_next_submodule(struct child_process *cp, > 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; > + } else { > + /* > + * An empty directory is normal, > + * the submodule is not initialized > + */ > + if (S_ISGITLINK(ce->ce_mode) && > + !is_empty_dir(ce->name)) { What if the directory is nonempty (e.g. contains build artifacts)? > + spf->result = 1; > + strbuf_addf(err, > + _("Could not access submodule '%s'"), > + ce->name); > + } Should this exit the loop? Otherwise, multiple "Could not access" messages can go in the same err string a big concatenated line. Thanks, Jonathan
diff --git a/submodule.c b/submodule.c index 0c81aca6f2..77ace5e784 100644 --- a/submodule.c +++ b/submodule.c @@ -1253,6 +1253,30 @@ static int get_fetch_recurse_config(const struct submodule *submodule, return spf->default_option; } +static struct repository *get_submodule_repo_for(struct repository *r, + const struct submodule *sub) +{ + struct repository *ret = xmalloc(sizeof(*ret)); + + 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", sub->path); + if (repo_init(ret, gitdir.buf, NULL)) { + strbuf_release(&gitdir); + free(ret); + return NULL; + } + strbuf_release(&gitdir); + } + + return ret; +} + static int get_next_submodule(struct child_process *cp, struct strbuf *err, void *data, void **task_cb) { @@ -1260,12 +1284,11 @@ static int get_next_submodule(struct child_process *cp, struct submodule_parallel_fetch *spf = data; for (; spf->count < spf->r->index->cache_nr; spf->count++) { - struct strbuf submodule_path = STRBUF_INIT; - 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 *default_argv; const struct submodule *submodule; + struct repository *repo; struct submodule default_submodule = SUBMODULE_INIT; if (!S_ISGITLINK(ce->ce_mode)) @@ -1300,15 +1323,11 @@ static int get_next_submodule(struct child_process *cp, continue; } - strbuf_repo_worktree_path(&submodule_path, spf->r, "%s", ce->name); - strbuf_addf(&submodule_git_dir, "%s/.git", submodule_path.buf); strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, ce->name); - git_dir = read_gitfile(submodule_git_dir.buf); - if (!git_dir) - git_dir = submodule_git_dir.buf; - if (is_directory(git_dir)) { + repo = get_submodule_repo_for(spf->r, submodule); + if (repo) { child_process_init(cp); - cp->dir = strbuf_detach(&submodule_path, NULL); + cp->dir = xstrdup(repo->worktree); prepare_submodule_repo_env(&cp->env_array); cp->git_cmd = 1; if (!spf->quiet) @@ -1319,10 +1338,23 @@ static int get_next_submodule(struct child_process *cp, 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; + } else { + /* + * An empty directory is normal, + * the submodule is not initialized + */ + if (S_ISGITLINK(ce->ce_mode) && + !is_empty_dir(ce->name)) { + spf->result = 1; + strbuf_addf(err, + _("Could not access submodule '%s'"), + ce->name); + } } - strbuf_release(&submodule_path); - strbuf_release(&submodule_git_dir); strbuf_release(&submodule_prefix); if (ret) { spf->count++;
We used to recurse into submodules, even if they were broken having only an objects directory. The child process executed in the submodule would fail though if the submodule was broken. This is tested via "fetching submodule into a broken repository" in t5526. This patch tightens the check upfront, such that we do not need to spawn a child process to find out if the submodule is broken. Signed-off-by: Stefan Beller <sbeller@google.com> --- submodule.c | 56 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 12 deletions(-)