Re: [PATCH 5/6] submodule: fixup nested submodules after moving the submodule

[Jonathan Tan <jonathantanmy@google.com>]

On Wed, 28 Mar 2018 10:24:48 -0700
Stefan Beller <sbeller@google.com> wrote:

> +static void connect_wt_gitdir_in_nested(const char *sub_worktree,
> +					const char *sub_gitdir)
> +{
> +	int i;
> +	struct repository subrepo;
> +	struct strbuf sub_wt = STRBUF_INIT;
> +	struct strbuf sub_gd = STRBUF_INIT;
> +
> +	const struct submodule *sub;
> +
> +	if (repo_init(&subrepo, sub_gitdir, sub_worktree))
> +		return;

If repo_init() fails, is it because the working tree doesn't exist on
disk, so we don't need to perform any connections on submodules? I think
a comment should be added to describe this.

> +
> +	if (repo_read_index(&subrepo) < 0)
> +		die("index file corrupt in repo %s", subrepo.gitdir);
> +
> +	for (i = 0; i < subrepo.index->cache_nr; i++) {
> +		const struct cache_entry *ce = subrepo.index->cache[i];
> +
> +		if (!S_ISGITLINK(ce->ce_mode))
> +			continue;
> +
> +		while (i + 1 < subrepo.index->cache_nr &&
> +		       !strcmp(ce->name, subrepo.index->cache[i + 1]->name))
> +			/*
> +			 * Skip entries with the same name in different stages
> +			 * to make sure an entry is returned only once.
> +			 */
> +			i++;
> +
> +		sub = submodule_from_path(&subrepo, &null_oid, ce->name);
> +		if (!sub)
> +			/* submodule not checked out? */
> +			continue;
> +
> +		if (is_submodule_active(&subrepo, ce->name)) {

Optional: This could be combined with the previous "if" block, so that
the following lines don't need to be indented.

> +			strbuf_addf(&sub_wt, "%s/%s", sub_worktree, sub->path);
> +			strbuf_addf(&sub_gd, "%s/modules/%s", sub_gitdir, sub->name);
> +
> +			connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 0);
> +			connect_wt_gitdir_in_nested(sub_wt.buf, sub_gd.buf);

What's the difference between having this call to
connect_wt_gitdir_in_nested() and just passing 1 instead of 0 to
connect_work_tree_and_git_dir()? I see that the latter uses absolute
paths, but that would seem to have the same effect. (If not, I think a
comment is warranted.)

> +
> +			strbuf_reset(&sub_wt);
> +			strbuf_reset(&sub_gd);

I think we normally write the resets before the strbuf_addf(), so that
it's clearer that sub_wt and sub_gd are meant to start from scratch in
every iteration.

Overall, this version of the patch is clearer - thanks.