Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Oct 17, 2018, 22:58 UTC
- Message-ID
- <20181017225811.66554-1-jonathantanmy@google.com>
- In-Reply-To
- <20181016181327.107186-8-sbeller@google.com>
> This patch started as a refactoring to make 'get_next_submodule' more > 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.
The commit message needs to be updated, I think - this patch does significantly more than fetching in the gitdir.
> 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.
The cp->dir is already leaked - probably better to write "cp->dir in get_next_submodule() is still leaked, but this will be fixed in a subsequent patch".
> +static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
> +{
> + prepare_submodule_repo_env_no_git_dir(out);
> + argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);Why does GIT_DIR need to be set? Is it to avoid subcommands recursively checking the parent directories in case the CWD is a malformed Git repository? If yes, maybe it's worth adding a comment.
Show 22 quoted lines
> +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);
> + return NULL;
> + }
> + strbuf_release(&gitdir);
> + }
> +
> + return ret;
> +}This is the significant thing that this patch does more - an unskipped submodule is now something that either passes the checks in repo_submodule_init() or the checks in repo_init(), which seems to be stricter than the current check that ".git" points to a directory or is one. This means that we skip certain broken repositories, and this necessitates a change in the test.
I think we should be more particular about what we're allowed to skip - in particular, maybe if we're planning to skip this submodule, its corresponding directory in the worktree (if one exists) needs to be empty.
> - 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 = xstrdup(repo->gitdir);
Here is where the functionality change (fetch in ".git") described in the commit message occurs.