[PATCHv5 0/5] submodule embedgitdirs

[PATCHv5 0/5] submodule embedgitdirs

[Stefan Beller]

v5:
* Add another layer of abstraction, i.e. the relocate_git_dir is only about 
  moving a git dir of one repository. The submodule specific stuff (e.g.
  recursion into nested submodules) is in submodule.{c,h}
  
  This was motivated by reviews on the series of checkout aware of submodules
  building on top of this series, as we want to directly call the embed-git-dirs
  function without the overhead of spawning a child process.
  
Thanks,
Stefan
 
v4:
* rebuilt on top of nd/worktree-list-fixup
* fix and test behavior for un-init submodules (don't crash, rather do nothing)
* incorporated a "static" as pointed out by Ramsay
* use internal functions instead of duplicating code in worktree.c
  (use get_common_dir_noenv for the submodule to actually get the common dir)
* fixed a memory leak in relocate_gitdir

v3:
* have a slightly more generic function "relocate_gitdir".
  The recursion is strictly related to submodules, though.
* bail out if a submodule is using worktrees.
  This also lays the groundwork for later doing the proper thing,
  as worktree.h offers a function `get_submodule_worktrees(path)`
* nit by duy: use git_path instead of git_common_dir

v2:
* fixed commit message for patch:
 "submodule: use absolute path for computing relative path connecting"
* a new patch "submodule helper: support super prefix"
* redid the final patch with more tests and fixing bugs along the way
* "test-lib-functions.sh: teach test_commit -C <dir>" unchanged

v1:
The discussion of the submodule checkout series revealed to me that a command
is needed to move the git directory from the submodules working tree to be
embedded into the superprojects git directory.

So I wrote the code to intern the submodules git dir into the superproject,
but whilst writing the code I realized this could be valueable for our use
in testing too. So I exposed it via the submodule--helper. But as the
submodule helper ought to be just an internal API, we could also
offer it via the proper submodule command.

The command as it is has little value to the end user for now, but
breaking it out of the submodule checkout series hopefully makes review easier.

Thanks,
Stefan

Stefan Beller (5):
  submodule: use absolute path for computing relative path connecting
  submodule helper: support super prefix
  test-lib-functions.sh: teach test_commit -C <dir>
  worktree: get worktrees from submodules
  submodule: add embed-git-dir function

 Documentation/git-submodule.txt   |  14 +++++
 builtin/submodule--helper.c       |  68 ++++++++++++++++----
 dir.c                             |  27 ++++++++
 dir.h                             |   3 +
 git-submodule.sh                  |   7 ++-
 git.c                             |   2 +-
 submodule.c                       | 127 ++++++++++++++++++++++++++++++++++++--
 submodule.h                       |   7 +++
 t/t7412-submodule-embedgitdirs.sh | 101 ++++++++++++++++++++++++++++++
 t/test-lib-functions.sh           |  20 ++++--
 worktree.c                        |  47 +++++++++++---
 worktree.h                        |   6 ++
 12 files changed, 396 insertions(+), 33 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh
 
diff to v4:

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 10df69c86a..321c9e250a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1106,31 +1106,8 @@ static int embed_git_dir(int argc, const char **argv, const char *prefix)
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
 
-	for (i = 0; i < list.nr; i++) {
-		const char *path = list.entries[i]->name, *sub_git_dir, *v;
-		char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
-		struct strbuf gitdir = STRBUF_INIT;
-
-		strbuf_addf(&gitdir, "%s/.git", path);
-		sub_git_dir = resolve_gitdir(gitdir.buf);
-
-		/* not populated? */
-		if (!sub_git_dir)
-			goto free_and_continue;
-
-		/* 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), NULL)
-			goto free_and_continue;
-
-		relocate_gitdir(prefix, path, flags);
-
-free_and_continue:
-		strbuf_release(&gitdir);
-		free(real_sub_git_dir);
-		free(real_common_git_dir);
-	}
+	for (i = 0; i < list.nr; i++)
+		submodule_embed_git_dir(prefix, list.entries[i]->name, flags);
 
 	return 0;
 }
diff --git a/dir.c b/dir.c
index d2f60b5abf..e023b04407 100644
--- a/dir.c
+++ b/dir.c
@@ -15,9 +15,6 @@
 #include "utf8.h"
 #include "varint.h"
 #include "ewah/ewok.h"
-#include "submodule-config.h"
-#include "run-command.h"
-#include "worktree.h"
 
 struct path_simplify {
 	int len;
@@ -2753,79 +2750,28 @@ void untracked_cache_add_to_index(struct index_state *istate,
 }
 
 /*
- * Migrate the given submodule (and all its submodules recursively) from
- * having its git directory within the working tree to the git dir nested
- * in its superprojects git dir under modules/.
+ * 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.
  */
-void relocate_gitdir(const char *prefix, const char *path, unsigned flags)
+int relocate_gitdir(const char *path, const char *old_git_dir,
+		    const char *new_git_dir, const char *displaypath,
+		    struct strbuf *err)
 {
-	char *old_git_dir;
-	const char *new_git_dir;
-	const struct submodule *sub;
-	struct worktree **worktrees;
-	int i;
-
-	worktrees = get_submodule_worktrees(path, 0);
-	if (worktrees) {
-		for (i = 0; worktrees[i]; i++)
-			;
-		free_worktrees(worktrees);
-		if (i > 1)
-			die(_("relocate_gitdir for submodule with more than one worktree not supported"));
-	}
-
-	old_git_dir = xstrfmt("%s/.git", path);
-	if (read_gitfile(old_git_dir))
-		/* If it is an actual gitfile, it doesn't need migration. */
-		goto out;
-
-	sub = submodule_from_path(null_sha1, path);
-	if (!sub)
-		die(_("Could not lookup name for submodule '%s'"),
-		      path);
+	int ret = 0;
 
-	new_git_dir = git_path("modules/%s", sub->name);
-	if (safe_create_leading_directories_const(new_git_dir) < 0)
-		die(_("could not create directory '%s'"), new_git_dir);
+	printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
+		displaypath, old_git_dir, new_git_dir);
 
-	if (!prefix)
-		prefix = get_super_prefix();
-	printf("Migrating git directory of %s%s from\n'%s' to\n'%s'\n",
-		prefix ? prefix : "", path,
-		real_path(old_git_dir), new_git_dir);
-
-	if (rename(old_git_dir, new_git_dir) < 0)
-		die_errno(_("Could not migrate git directory from '%s' to '%s'"),
+	if (rename(old_git_dir, new_git_dir) < 0) {
+		ret = errno;
+		strbuf_addf(err,
+			_("could not migrate git directory from '%s' to '%s'"),
 			old_git_dir, new_git_dir);
+		return ret;
+	}
 
 	connect_work_tree_and_git_dir(path, new_git_dir);
 
-out:
-	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 migrate git directory in submodule '%s'"),
-			    path);
-
-		strbuf_release(&sb);
-	}
-
-	free(old_git_dir);
+	return ret;
 }
diff --git a/dir.h b/dir.h
index 0b5e99b21d..bf06729a86 100644
--- a/dir.h
+++ b/dir.h
@@ -335,8 +335,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
 void add_untracked_cache(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
-
-#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
-extern void relocate_gitdir(const char *prefix, const char *path, unsigned flags);
-
+extern int relocate_gitdir(const char *path, const char *old_git_dir,
+			   const char *new_git_dir, const char *displaypath,
+			   struct strbuf *err);
 #endif
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..67a91275b8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -14,6 +14,7 @@
 #include "blob.h"
 #include "thread-utils.h"
 #include "quote.h"
+#include "worktree.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int parallel_jobs = 1;
@@ -1263,3 +1264,117 @@ void prepare_submodule_repo_env(struct argv_array *out)
 	}
 	argv_array_push(out, "GIT_DIR=.git");
 }
+
+/* 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);
+	}
+
+	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;
+
+	real_old_git_dir = xstrdup(real_path(old_git_dir));
+
+	sub = submodule_from_path(null_sha1, path);
+	if (!sub)
+		die(_("could not lookup name for submodule '%s'"), path);
+
+	new_git_dir = git_path("modules/%s", sub->name);
+	if (safe_create_leading_directories_const(new_git_dir) < 0)
+		die(_("could not create directory '%s'"), new_git_dir);
+	real_new_git_dir = xstrdup(real_path(new_git_dir));
+
+	if (!prefix)
+		prefix = get_super_prefix();
+	strbuf_addf(&pathbuf, "%s%s", prefix ? prefix : "", path);
+
+	code = relocate_gitdir(path, real_old_git_dir, real_new_git_dir,
+			       pathbuf.buf, &errbuf);
+	if (code) {
+		errno = code;
+		die_errno("%s\n", errbuf.buf);
+	}
+
+	free(old_git_dir);
+	free(real_old_git_dir);
+	free(real_new_git_dir);
+	strbuf_release(&pathbuf);
+}
+
+/*
+ * 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/.
+ */
+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;
+
+
+	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))
+		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);
+	}
+
+out:
+	strbuf_release(&gitdir);
+	free(real_sub_git_dir);
+	free(real_common_git_dir);
+	return 0;
+}
diff --git a/submodule.h b/submodule.h
index d9e197a948..922cfd258f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -75,4 +75,11 @@ int parallel_submodules(void);
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 
+/*
+ * Embed a git dir of the submodule given by path.
+ */
+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
+extern int submodule_embed_git_dir(const char *prefix,
+				   const char *path,
+				   unsigned flags);
 #endif

 
-- 
2.11.0.rc2.28.g2af45f1.dirty

[PATCHv5 1/5] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

The current caller of connect_work_tree_and_git_dir passes
an absolute path for the `git_dir` parameter. In the future patch
we will also pass in relative path for `git_dir`. Extend the functionality
of connect_work_tree_and_git_dir to take relative paths for parameters.

We could work around this in the future patch by computing the absolute
path for the git_dir in the calling site, however accepting relative
paths for either parameter makes the API for this function much harder
to misuse.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6f7d883de9..66c5ce5a24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 {
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
-	const char *real_work_tree = xstrdup(real_path(work_tree));
+	char *real_git_dir = xstrdup(real_path(git_dir));
+	char *real_work_tree = xstrdup(real_path(work_tree));
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
 	write_file(file_name.buf, "gitdir: %s",
-		   relative_path(git_dir, real_work_tree, &rel_path));
+		   relative_path(real_git_dir, real_work_tree, &rel_path));
 
 	/* Update core.worktree setting */
 	strbuf_reset(&file_name);
-	strbuf_addf(&file_name, "%s/config", git_dir);
+	strbuf_addf(&file_name, "%s/config", real_git_dir);
 	git_config_set_in_file(file_name.buf, "core.worktree",
-			       relative_path(real_work_tree, git_dir,
+			       relative_path(real_work_tree, real_git_dir,
 					     &rel_path));
 
 	strbuf_release(&file_name);
 	strbuf_release(&rel_path);
-	free((void *)real_work_tree);
+	free(real_work_tree);
+	free(real_git_dir);
 }
 
 int parallel_submodules(void)
-- 
2.11.0.rc2.28.g2af45f1.dirty

[PATCHv5 2/5] submodule helper: support super prefix

[Stefan Beller]

Just like main commands in Git, the submodule helper needs
access to the superproject prefix. Enable this in the git.c
but have its own fuse in the helper code by having a flag to
turn on the super prefix.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------
 git.c                       |  2 +-
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..806e29ce4e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
+	int option;
 };
 
 static struct cmd_struct commands[] = {
-	{"list", module_list},
-	{"name", module_name},
-	{"clone", module_clone},
-	{"update-clone", update_clone},
-	{"relative-path", resolve_relative_path},
-	{"resolve-relative-url", resolve_relative_url},
-	{"resolve-relative-url-test", resolve_relative_url_test},
-	{"init", module_init},
-	{"remote-branch", resolve_remote_submodule_branch}
+	{"list", module_list, 0},
+	{"name", module_name, 0},
+	{"clone", module_clone, 0},
+	{"update-clone", update_clone, 0},
+	{"relative-path", resolve_relative_path, 0},
+	{"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}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 		die(_("submodule--helper subcommand must be "
 		      "called with a subcommand"));
 
-	for (i = 0; i < ARRAY_SIZE(commands); i++)
-		if (!strcmp(argv[1], commands[i].cmd))
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		if (!strcmp(argv[1], commands[i].cmd)) {
+			if (get_super_prefix() &&
+			    !(commands[i].option & SUPPORT_SUPER_PREFIX))
+				die("%s doesn't support --super-prefix",
+				    commands[i].cmd);
 			return commands[i].fn(argc - 1, argv + 1, prefix);
+		}
+	}
 
 	die(_("'%s' is not a valid submodule--helper "
 	      "subcommand"), argv[1]);
diff --git a/git.c b/git.c
index efa1059fe0..98dcf6c518 100644
--- a/git.c
+++ b/git.c
@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
 	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
-	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP },
+	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
 	{ "tag", cmd_tag, RUN_SETUP },
 	{ "unpack-file", cmd_unpack_file, RUN_SETUP },
-- 
2.11.0.rc2.28.g2af45f1.dirty

[PATCHv5 3/5] test-lib-functions.sh: teach test_commit -C <dir>

[Stefan Beller]

Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C <dir>" similar to gits -C parameter
that will perform the operation in the given directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib-functions.sh | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
 	 GIT_TEST_GDB=1 "$@"
 }
 
-# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
+# Call test_commit with the arguments
+# [-C <directory>] <message> [<file> [<contents> [<tag>]]]"
 #
 # This will commit a file with the given contents and the given commit
 # message, and tag the resulting commit with the given tag name.
 #
 # <file>, <contents>, and <tag> all default to <message>.
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
 
 test_commit () {
 	notick= &&
 	signoff= &&
+	indir= &&
 	while test $# != 0
 	do
 		case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
 		--signoff)
 			signoff="$1"
 			;;
+		-C)
+			indir="$2"
+			shift
+			;;
 		*)
 			break
 			;;
 		esac
 		shift
 	done &&
+	indir=${indir:+"$indir"/} &&
 	file=${2:-"$1.t"} &&
-	echo "${3-$1}" > "$file" &&
-	git add "$file" &&
+	echo "${3-$1}" > "$indir$file" &&
+	git ${indir:+ -C "$indir"} add "$file" &&
 	if test -z "$notick"
 	then
 		test_tick
 	fi &&
-	git commit $signoff -m "$1" &&
-	git tag "${4:-$1}"
+	git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+	git ${indir:+ -C "$indir"} tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments "<message> <commit>", where <commit>
-- 
2.11.0.rc2.28.g2af45f1.dirty

[PATCHv5 4/5] worktree: get worktrees from submodules

[Stefan Beller]

In a later patch we want to move around the the git directory of
a submodule. Both submodules as well as worktrees are involved in
placing git directories at unusual places, so their functionality
may collide. To react appropriately to situations where worktrees
in submodules are in use, offer a new function to query the
worktrees for submodules.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 worktree.c | 47 +++++++++++++++++++++++++++++++++++++----------
 worktree.h |  6 ++++++
 2 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/worktree.c b/worktree.c
index eb6121263b..fa2b6dfa9a 100644
--- a/worktree.c
+++ b/worktree.c
@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
 /**
  * get the main worktree
  */
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(const char *git_common_dir)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)
 	int is_bare = 0;
 	int is_detached = 0;
 
-	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+	strbuf_add_absolute_path(&worktree_path, git_common_dir);
 	is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
 	if (is_bare)
 		strbuf_strip_suffix(&worktree_path, "/.");
 
-	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+	strbuf_addf(&path, "%s/HEAD", git_common_dir);
 
 	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
 	return worktree;
 }
 
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *git_common_dir,
+					    const char *id)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -112,7 +113,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	if (!id)
 		die("Missing linked worktree name");
 
-	strbuf_git_common_path(&path, "worktrees/%s/gitdir", id);
+	strbuf_addf(&path, "%s/worktrees/%s/gitdir", git_common_dir, id);
 	if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
 		/* invalid gitdir file */
 		goto done;
@@ -125,7 +126,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	}
 
 	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+	strbuf_addf(&path, "%s/worktrees/%s/HEAD", git_common_dir, id);
 
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
@@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)
 	return fspathcmp((*a)->path, (*b)->path);
 }
 
-struct worktree **get_worktrees(unsigned flags)
+static struct worktree **get_worktrees_internal(const char *git_common_dir,
+						unsigned flags)
 {
 	struct worktree **list = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -177,9 +179,9 @@ struct worktree **get_worktrees(unsigned flags)
 
 	list = xmalloc(alloc * sizeof(struct worktree *));
 
-	list[counter++] = get_main_worktree();
+	list[counter++] = get_main_worktree(git_common_dir);
 
-	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+	strbuf_addf(&path, "%s/worktrees", git_common_dir);
 	dir = opendir(path.buf);
 	strbuf_release(&path);
 	if (dir) {
@@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)
 			if (is_dot_or_dotdot(d->d_name))
 				continue;
 
-			if ((linked = get_linked_worktree(d->d_name))) {
+			if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {
 				ALLOC_GROW(list, counter + 1, alloc);
 				list[counter++] = linked;
 			}
@@ -209,6 +211,31 @@ struct worktree **get_worktrees(unsigned flags)
 	return list;
 }
 
+struct worktree **get_worktrees(unsigned flags)
+{
+	return get_worktrees_internal(get_git_common_dir(), flags);
+}
+
+struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
+{
+	char *submodule_gitdir;
+	const char *submodule_common_dir;
+	struct strbuf sb = STRBUF_INIT;
+	struct worktree **ret;
+
+	submodule_gitdir = git_pathdup_submodule(path, "%s", "");
+	if (!submodule_gitdir)
+		return NULL;
+
+	/* the env would be set for the superproject */
+	get_common_dir_noenv(&sb, submodule_gitdir);
+	submodule_common_dir = strbuf_detach(&sb, NULL);
+	ret = get_worktrees_internal(submodule_common_dir, flags);
+
+	free(submodule_gitdir);
+	return ret;
+}
+
 const char *get_worktree_git_dir(const struct worktree *wt)
 {
 	if (!wt)
diff --git a/worktree.h b/worktree.h
index d59ce1fee8..157fbc4a66 100644
--- a/worktree.h
+++ b/worktree.h
@@ -27,6 +27,12 @@ struct worktree {
  */
 extern struct worktree **get_worktrees(unsigned flags);
 
+/*
+ * Get the worktrees of a submodule named by `path`.
+ */
+extern struct worktree **get_submodule_worktrees(const char *path,
+						 unsigned flags);
+
 /*
  * Return git dir of the worktree. Note that the path may be relative.
  * If wt is NULL, git dir of current worktree is returned.
-- 
2.11.0.rc2.28.g2af45f1.dirty

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

[Stefan Beller]

When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.

Add functionality to migrate the git directory to be embedded
into the superprojects git directory.

The newly added code in this patch is structured such that
other areas of Git can also make use of it. The code in the
submodule--helper is a mere wrapper and option parser
for the function `submodule_embed_git_dir_for_path`, that
takes care of embedding the submodules git directory into
the superprojects git dir. That function makes use of the
more abstract function for this use case `relocate_gitdir`,
which can be used by e.g. the worktree code eventually to
move around a git directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt   |  14 +++++
 builtin/submodule--helper.c       |  39 ++++++++++++-
 dir.c                             |  27 +++++++++
 dir.h                             |   3 +
 git-submodule.sh                  |   7 ++-
 submodule.c                       | 115 ++++++++++++++++++++++++++++++++++++++
 submodule.h                       |   7 +++
 t/t7412-submodule-embedgitdirs.sh | 101 +++++++++++++++++++++++++++++++++
 8 files changed, 311 insertions(+), 2 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..34791cfc65 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
+'git submodule' [--quiet] embedgitdirs [--] [<path>...]
 
 
 DESCRIPTION
@@ -245,6 +246,19 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+embedgitdirs::
+	Move the git directory of submodules into its superprojects
+	`$GIT_DIR/modules` path and then connect the git directory and
+	its working directory by setting the `core.worktree` and adding
+	a .git file pointing to the git directory interned into the
+	superproject.
++
+A repository that was cloned independently and later added as a submodule or
+old setups have the submodules git directory inside the submodule instead of
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
 OPTIONS
 -------
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 806e29ce4e..321c9e250a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,42 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+static int embed_git_dir(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	unsigned flags = RELOCATE_GITDIR_RECURSE_SUBMODULES;
+
+	struct option embed_gitdir_options[] = {
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
+		OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
+			RELOCATE_GITDIR_RECURSE_SUBMODULES),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper embed-git-dir [<path>...]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, embed_gitdir_options,
+			     git_submodule_helper_usage, 0);
+
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	for (i = 0; i < list.nr; i++)
+		submodule_embed_git_dir(prefix, list.entries[i]->name, flags);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -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}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
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);
+
+	if (rename(old_git_dir, new_git_dir) < 0) {
+		ret = errno;
+		strbuf_addf(err,
+			_("could not migrate git directory from '%s' to '%s'"),
+			old_git_dir, new_git_dir);
+		return ret;
+	}
+
+	connect_work_tree_and_git_dir(path, new_git_dir);
+
+	return ret;
+}
diff --git a/dir.h b/dir.h
index 97c83bb383..bf06729a86 100644
--- a/dir.h
+++ b/dir.h
@@ -335,4 +335,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
 void add_untracked_cache(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
+extern int relocate_gitdir(const char *path, const char *old_git_dir,
+			   const char *new_git_dir, const char *displaypath,
+			   struct strbuf *err);
 #endif
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..b7e124f340 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
 	done
 }
 
+cmd_embedgitdirs()
+{
+	git submodule--helper embed-git-dirs --prefix "$wt_prefix" "$@"
+}
+
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
 # options are primarily done by the subcommand implementations.
@@ -1140,7 +1145,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | status | summary | sync)
+	add | foreach | init | deinit | update | status | summary | sync | embedgitdirs)
 		command=$1
 		;;
 	-q|--quiet)
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..67a91275b8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -14,6 +14,7 @@
 #include "blob.h"
 #include "thread-utils.h"
 #include "quote.h"
+#include "worktree.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int parallel_jobs = 1;
@@ -1263,3 +1264,117 @@ void prepare_submodule_repo_env(struct argv_array *out)
 	}
 	argv_array_push(out, "GIT_DIR=.git");
 }
+
+/* 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);
+	}
+
+	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;
+
+	real_old_git_dir = xstrdup(real_path(old_git_dir));
+
+	sub = submodule_from_path(null_sha1, path);
+	if (!sub)
+		die(_("could not lookup name for submodule '%s'"), path);
+
+	new_git_dir = git_path("modules/%s", sub->name);
+	if (safe_create_leading_directories_const(new_git_dir) < 0)
+		die(_("could not create directory '%s'"), new_git_dir);
+	real_new_git_dir = xstrdup(real_path(new_git_dir));
+
+	if (!prefix)
+		prefix = get_super_prefix();
+	strbuf_addf(&pathbuf, "%s%s", prefix ? prefix : "", path);
+
+	code = relocate_gitdir(path, real_old_git_dir, real_new_git_dir,
+			       pathbuf.buf, &errbuf);
+	if (code) {
+		errno = code;
+		die_errno("%s\n", errbuf.buf);
+	}
+
+	free(old_git_dir);
+	free(real_old_git_dir);
+	free(real_new_git_dir);
+	strbuf_release(&pathbuf);
+}
+
+/*
+ * 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/.
+ */
+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;
+
+
+	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))
+		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);
+	}
+
+out:
+	strbuf_release(&gitdir);
+	free(real_sub_git_dir);
+	free(real_common_git_dir);
+	return 0;
+}
diff --git a/submodule.h b/submodule.h
index d9e197a948..922cfd258f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -75,4 +75,11 @@ int parallel_submodules(void);
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 
+/*
+ * Embed a git dir of the submodule given by path.
+ */
+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
+extern int submodule_embed_git_dir(const char *prefix,
+				   const char *path,
+				   unsigned flags);
 #endif
diff --git a/t/t7412-submodule-embedgitdirs.sh b/t/t7412-submodule-embedgitdirs.sh
new file mode 100755
index 0000000000..a02e7c447a
--- /dev/null
+++ b/t/t7412-submodule-embedgitdirs.sh
@@ -0,0 +1,101 @@
+#!/bin/sh
+
+test_description='Test submodule embedgitdirs
+
+This test verifies that `git submodue embedgitdirs` moves a submodules git
+directory into the superproject.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a real submodule' '
+	git init sub1 &&
+	test_commit -C sub1 first &&
+	git submodule add ./sub1 &&
+	test_tick &&
+	git commit -m superproject
+'
+
+test_expect_success 'embed the git dir' '
+	>expect.1 &&
+	>expect.2 &&
+	>actual.1 &&
+	>actual.2 &&
+	git status >expect.1 &&
+	git -C sub1 rev-parse HEAD >expect.2 &&
+	git submodule embedgitdirs &&
+	git fsck &&
+	test -f sub1/.git &&
+	test -d .git/modules/sub1 &&
+	git status >actual.1 &&
+	git -C sub1 rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+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 &&
+	test -d sub1
+'
+
+test_expect_success 'setup nested submodule' '
+	git init sub1/nested &&
+	test_commit -C sub1/nested first_nested &&
+	git -C sub1 submodule add ./nested &&
+	test_tick &&
+	git -C sub1 commit -m "add nested" &&
+	git add sub1 &&
+	git commit -m "sub1 to include nested submodule"
+'
+
+test_expect_success 'embed the git dir in a nested submodule' '
+	git status >expect.1 &&
+	git -C sub1/nested rev-parse HEAD >expect.2 &&
+	git submodule embedgitdirs &&
+	test -f sub1/nested/.git &&
+	test -d .git/modules/sub1/modules/nested &&
+	git status >actual.1 &&
+	git -C sub1/nested rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup a gitlink with missing .gitmodules entry' '
+	git init sub2 &&
+	test_commit -C sub2 first &&
+	git add sub2 &&
+	git commit -m superproject
+'
+
+test_expect_success 'embedding the git dir fails for incomplete submodules' '
+	git status >expect.1 &&
+	git -C sub2 rev-parse HEAD >expect.2 &&
+	test_must_fail git submodule embedgitdirs &&
+	git -C sub2 fsck &&
+	test -d sub2/.git &&
+	git status >actual &&
+	git -C sub2 rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup a submodule with multiple worktrees' '
+	# first create another unembedded git dir in a new submodule
+	git init sub3 &&
+	test_commit -C sub3 first &&
+	git submodule add ./sub3 &&
+	test_tick &&
+	git commit -m "add another submodule" &&
+	git -C sub3 worktree add ../sub3_second_work_tree
+'
+
+test_expect_success 'embed a submodule with multiple worktrees' '
+	test_must_fail git submodule embedgitdirs sub3 2>error &&
+	test_i18ngrep "not supported" error
+'
+
+test_done
-- 
2.11.0.rc2.28.g2af45f1.dirty

Re: [PATCHv5 0/5] submodule embedgitdirs

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> v5:
> * Add another layer of abstraction, i.e. the relocate_git_dir is only about 
>   moving a git dir of one repository. The submodule specific stuff (e.g.
>   recursion into nested submodules) is in submodule.{c,h}
>   
>   This was motivated by reviews on the series of checkout aware of submodules
>   building on top of this series, as we want to directly call the embed-git-dirs
>   function without the overhead of spawning a child process.

OK.  Comparing the last steps between this round and the previous
one, I do think the separation of the responsibility among helpers
is much more reasonable in this version, where:

 - submodule_embed_git_dir() is given a single path and is
   responsible for that submodule itself, which is done by calling
   submodule_embed_git_dir_for_path() on itself, and its
   sub-submodules, which is done by spawning the helper recursively
   with appropriate super-prefix;

 - submodule_embed_git_dir_for_path() computes where the given path
   needs to be moved to using the knowledge specific to the
   submodule subsystem, and asks relocate_gitdir() to perform the
   actual relocation;

 - relocate_gitdir() used to do quite a lot more, but now it is only
   about moving an existing .git directory elsewhere and pointing to
   the new location with .git file placed in the old location.

I would have called the second helper submodule_embed_one_git_dir(),
but that is a minor detail.

Very nicely done.

Re: [PATCHv5 4/5] worktree: get worktrees from submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +	submodule_common_dir = strbuf_detach(&sb, NULL);
> +	ret = get_worktrees_internal(submodule_common_dir, flags);
> +
> +	free(submodule_gitdir);

This sequence felt somewhat unusual.  I would have written this
without an extra variable, i.e.

	ret = get_worktrees_internal(sb.buf, flags);
	strbuf_release(&sb);

Re: [PATCHv5 4/5] worktree: get worktrees from submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +     submodule_common_dir = strbuf_detach(&sb, NULL);
>> +     ret = get_worktrees_internal(submodule_common_dir, flags);
>> +
>> +     free(submodule_gitdir);
>
> This sequence felt somewhat unusual.  I would have written this
> without an extra variable, i.e.
>
>         ret = get_worktrees_internal(sb.buf, flags);
>         strbuf_release(&sb);

Yours is cleaner; I don't remember what I was thinking.

Feel free to squash it in; in case a resend is needed I will do that.

Thanks,
Stefan

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

[Junio C Hamano]

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.

Re: [PATCHv5 0/5] submodule embedgitdirs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> v5:
>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about 
>>   moving a git dir of one repository. The submodule specific stuff (e.g.
>>   recursion into nested submodules) is in submodule.{c,h}
>>   
>>   This was motivated by reviews on the series of checkout aware of submodules
>>   building on top of this series, as we want to directly call the embed-git-dirs
>>   function without the overhead of spawning a child process.
>
> OK.  Comparing the last steps between this round and the previous
> one, I do think the separation of the responsibility among helpers
> is much more reasonable in this version, where:
>
>  - submodule_embed_git_dir() is given a single path and is
>    responsible for that submodule itself, which is done by calling
>    submodule_embed_git_dir_for_path() on itself, and its
>    sub-submodules, which is done by spawning the helper recursively
>    with appropriate super-prefix;
>
>  - submodule_embed_git_dir_for_path() computes where the given path
>    needs to be moved to using the knowledge specific to the
>    submodule subsystem, and asks relocate_gitdir() to perform the
>    actual relocation;
>
>  - relocate_gitdir() used to do quite a lot more, but now it is only
>    about moving an existing .git directory elsewhere and pointing to
>    the new location with .git file placed in the old location.
>
> I would have called the second helper submodule_embed_one_git_dir(),
> but that is a minor detail.
>
> Very nicely done.

One thing that is not so nice from the code organization point of
view is that "dir.c" now needs to include "submodule.h" because it
wants to call connect_work_tree_and_git_dir().

I wonder if that function belongs to dir.c or worktree.c not
submodule.c; I do not see anything that is submodule specific about
what the function does.

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

[Stefan Beller]

On Wed, Dec 7, 2016 at 3:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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).

Ok, thanks for pointing out!

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

Before sending it out to the list I had a version with another flag
that controlled whether we die on error or just print a warning.
That was not fully true however as the connect_work_tree_and_git_dir
would die nevertheless, we'd only warn for the failed rename().

It turns out we do not need a fully functional non-die()ing version
for the checkout series, so I ripped that out again and the version sent out
just dies in case of failure in relocate_gitdir.

That said, I think the printing of the migration message ought to go
into the caller and to stderr as you note.

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

ok, I'll add that helper as its own patch to the worktree code earlier
in the series.

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

If we have 2 worktrees, all bets are off (in my non-understanding).
The git file here *could* point to git directory living inside the
other working tree, which then would need migration from that other
working tree to some embedded place.

I don't think that is how worktrees work (or have ever worked?)
but I am unfamiliar with worktrees, so I erred on the safe side.

>  I am
> wondering if the "ah, we don't do a repository that has secondary
> worktrees" check should come after this one.

Maybe Duy can answer that? (Where are git directories
located in a worktree setup, even historically? Were they
ever inside one of the submodules? The other working tree may
not be a submodule, but a standalone thing as well?)

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

Oh right we had that discussion what the embedding actually means.
I asked around for English language help here:
What about "absorbGitDir" ? (absorb as in eat/consume) for the external
UI and internally I can call it relocate_submodule_git_dir_into_superproject
(or embed_git_dir_into_superproject)

>> +
>> +
>
> Lose the extra blank line here?

ok

>> +     if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
>
> Yeah, checking for NULL-ness with !skip_prefix() helps ;-)

I think you are referring to the interdiff to the previous patches..
Yes we should just use it as it is here.

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

The problem here is that we do not know about the names of
the nested submodules.  We could do a "git -C path submodule--helper list"
and then use the run_processes_parallel for

    git -C <submodule> embed-git-dir <nested-sub1>
    git -C <submodule> embed-git-dir <nested-sub2>
    git -C <submodule> embed-git-dir <nested-sub3>

etc. As a first approach I considered

    git -C <submodule> embed-git-dir <no-pathspec>

to be fast enough as the functionality is not implemented is only
a filesystem-local rename() (fast regardless of directory size).

So if we want to make that relocate git dir aware of
non-atomic-cross-filesystem moves, we want to revisit the decision
to run it in parallel?

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

This mainly ought to test that the new call doesn't crash or segfault
but accepts this as a valid state.

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

However the -e is the correct thing to do.

Re: [PATCHv5 0/5] submodule embedgitdirs

[Stefan Beller]

On Wed, Dec 7, 2016 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> v5:
>>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about
>>>   moving a git dir of one repository. The submodule specific stuff (e.g.
>>>   recursion into nested submodules) is in submodule.{c,h}
>>>
>>>   This was motivated by reviews on the series of checkout aware of submodules
>>>   building on top of this series, as we want to directly call the embed-git-dirs
>>>   function without the overhead of spawning a child process.
>>
>> OK.  Comparing the last steps between this round and the previous
>> one, I do think the separation of the responsibility among helpers
>> is much more reasonable in this version, where:
>>
>>  - submodule_embed_git_dir() is given a single path and is
>>    responsible for that submodule itself, which is done by calling
>>    submodule_embed_git_dir_for_path() on itself, and its
>>    sub-submodules, which is done by spawning the helper recursively
>>    with appropriate super-prefix;
>>
>>  - submodule_embed_git_dir_for_path() computes where the given path
>>    needs to be moved to using the knowledge specific to the
>>    submodule subsystem, and asks relocate_gitdir() to perform the
>>    actual relocation;
>>
>>  - relocate_gitdir() used to do quite a lot more, but now it is only
>>    about moving an existing .git directory elsewhere and pointing to
>>    the new location with .git file placed in the old location.
>>
>> I would have called the second helper submodule_embed_one_git_dir(),
>> but that is a minor detail.
>>
>> Very nicely done.
>
> One thing that is not so nice from the code organization point of
> view is that "dir.c" now needs to include "submodule.h" because it
> wants to call connect_work_tree_and_git_dir().
>
> I wonder if that function belongs to dir.c or worktree.c not
> submodule.c; I do not see anything that is submodule specific about
> what the function does.

Right, it just happened historically for that function to live
in submodule area. I'll move the connect_work_tree_and_git_dir
to dir.c as well.

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

[Brandon Williams]

On 12/07, Stefan Beller wrote:
> +		argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
> +					    "submodule--helper",
> +					   "embed-git-dirs", NULL);

check the spacing on these lines, looks like there is an extra space
before "submodule--helper"

-- 
Brandon Williams

Re: [PATCHv5 4/5] worktree: get worktrees from submodules

[Brandon Williams]

On 12/07, Stefan Beller wrote:
> On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Stefan Beller <sbeller@google.com> writes:
> >
> >> +     submodule_common_dir = strbuf_detach(&sb, NULL);
> >> +     ret = get_worktrees_internal(submodule_common_dir, flags);
> >> +
> >> +     free(submodule_gitdir);
> >
> > This sequence felt somewhat unusual.  I would have written this
> > without an extra variable, i.e.
> >
> >         ret = get_worktrees_internal(sb.buf, flags);
> >         strbuf_release(&sb);
> 
> Yours is cleaner; I don't remember what I was thinking.
> 
> Feel free to squash it in; in case a resend is needed I will do that.

Just make sure to leave that free in as it refers to another variable
(submodule_gitdir).  It actually turns out there is a memory leak in the
original code because submodule_common_dir is never freed after being
detached from the strbuf.

-- 
Brandon Williams

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

[Duy Nguyen]

On Thu, Dec 8, 2016 at 4:01 AM, Stefan Beller <sbeller@google.com> wrote:
> +/*
> + * 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);

I'm going to nag you about _() until your fingers automatically type
"_(" after "(" :-D
-- 
Duy