Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree
flat view
From: Jonathan Tan <hidden>
Date: 2018-10-17 22:58:18
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".
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
- 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.