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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> @@ -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).

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

> +/* 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));
> +...
> +}

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

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

> +	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 ;-)

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

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