Re: [PATCHv5 5/5] submodule: add embed-git-dir function

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2016, 23:03 UTC
Message-ID: <xmqqlgvruyt2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161207210157.18932-6-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 8 quoted lines

> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
>  	{"resolve-relative-url", resolve_relative_url, 0},
>  	{"resolve-relative-url-test", resolve_relative_url_test, 0},
>  	{"init", module_init, 0},
> -	{"remote-branch", resolve_remote_submodule_branch, 0}
> +	{"remote-branch", resolve_remote_submodule_branch, 0},
> +	{"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
>  };

If you want to avoid patch noise like this, your 2/5 can add a trailing comma after the entry for remote-branch. It is OK to end elements in an array literal with trailing comma, even though we avoid doing similar in enum definition (which is only allowed in newer C standards).

Show 22 quoted lines

> diff --git a/dir.c b/dir.c
> index bfa8c8a9a5..e023b04407 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state *istate,
>  {
>  	untracked_cache_invalidate_path(istate, path);
>  }
> +
> +/*
> + * Migrate the git directory of the given `path` from `old_git_dir` to
> + * `new_git_dir`. If an error occurs, append it to `err` and return the
> + * error code.
> + */
> +int relocate_gitdir(const char *path, const char *old_git_dir,
> +		    const char *new_git_dir, const char *displaypath,
> +		    struct strbuf *err)
> +{
> +	int ret = 0;
> +
> +	printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
> +		displaypath, old_git_dir, new_git_dir);

By using "strbuf err", it looks like that the calling convention of this function wants to cater to callers who want to have tight control over their output and an unconditional printing to the standard output looks somewhat out of place.

Besides, does it belong to the standard output? It looks more like a progress-bar eye candy that we send out to the standard error stream (and only when we are talking to a tty).

Show 21 quoted lines

> +/* Embeds a single submodule, non recursively. */
> +static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)
> +{
> +	struct worktree **worktrees;
> +	struct strbuf pathbuf = STRBUF_INIT;
> +	struct strbuf errbuf = STRBUF_INIT;
> +	char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
> +	const char *new_git_dir;
> +	const struct submodule *sub;
> +	int code;
> +
> +	worktrees = get_submodule_worktrees(path, 0);
> +	if (worktrees) {
> +		int i;
> +		for (i = 0; worktrees[i]; i++)
> +			;
> +		free_worktrees(worktrees);
> +		if (i > 1)
> +			die(_("relocate_gitdir for submodule '%s' with "
> +			    "more than one worktree not supported"), path);
> +	}

We may benefit from "does this have secondary worktrees?" boolean helper function, perhaps?

> +	old_git_dir = xstrfmt("%s/.git", path);
> +	if (read_gitfile(old_git_dir))
> +		/* If it is an actual gitfile, it doesn't need migration. */
> +		return;

Isn't this "no-op return" a valid thing to do, even when the submodule has secondary worktrees that borrow from it? I am wondering if the "ah, we don't do a repository that has secondary worktrees" check should come after this one.

> +	real_old_git_dir = xstrdup(real_path(old_git_dir));
> +...
> +}

Show 5 quoted lines

> +/*
> + * Migrate the git directory of the submodule given by path from
> + * having its git directory within the working tree to the git dir nested
> + * in its superprojects git dir under modules/.
> + */

I think that this operation removes the git directories that are embedded in the working tree of the superproject and storing them away to safer place, i.e. unembedding.

Show 9 quoted lines

> +int submodule_embed_git_dir(const char *prefix,
> +			    const char *path,
> +			    unsigned flags)
> +{
> +	const char *sub_git_dir, *v;
> +	char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
> +	struct strbuf gitdir = STRBUF_INIT;
> +
> +

Lose the extra blank line here?

Show 11 quoted lines

> +	strbuf_addf(&gitdir, "%s/.git", path);
> +	sub_git_dir = resolve_gitdir(gitdir.buf);
> +
> +	/* Not populated? */
> +	if (!sub_git_dir)
> +		goto out;
> +
> +	/* Is it already embedded? */
> +	real_sub_git_dir = xstrdup(real_path(sub_git_dir));
> +	real_common_git_dir = xstrdup(real_path(get_git_common_dir()));
> +	if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))

Yeah, checking for NULL-ness with !skip_prefix() helps ;-)

Show 25 quoted lines

> +		submodule_embed_git_dir_for_path(prefix, path);
> +
> +	if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +		struct strbuf sb = STRBUF_INIT;
> +
> +		if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)
> +			die("BUG: we don't know how to pass the flags down?");
> +
> +		if (get_super_prefix())
> +			strbuf_addstr(&sb, get_super_prefix());
> +		strbuf_addstr(&sb, path);
> +		strbuf_addch(&sb, '/');
> +
> +		cp.dir = path;
> +		cp.git_cmd = 1;
> +		cp.no_stdin = 1;
> +		argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
> +					    "submodule--helper",
> +					   "embed-git-dirs", NULL);
> +		prepare_submodule_repo_env(&cp.env_array);
> +		if (run_command(&cp))
> +			die(_("could not recurse into submodule '%s'"), path);
> +		strbuf_release(&sb);
> +	}

Hmph. We cannot use run_processes_parallel() thing here? Is its API too hard to use to be worth it?

Show 6 quoted lines

> +test_expect_success 'embedding does not fail for deinitalized submodules' '
> +	test_when_finished "git submodule update --init" &&
> +	git submodule deinit --all &&
> +	git submodule embedgitdirs &&
> +	test -d .git/modules/sub1 &&
> +	! test -f sub1/.git &&

Does this expect "sub1/.git is not a regular file (we want directory instead)"? Or "there is no filesystem entity at sub1/.git"?

If the former, write "test -d sub1/.git"; if the latter, you probably want "! test -e sub1/.git" instead.