> 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.
> +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.
