`submodule embedgitdirs` [was: Introduce `submodule interngitdirs`]

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

diff to v1 below.

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

 Documentation/git-submodule.txt   | 14 +++++++
 builtin/submodule--helper.c       | 62 +++++++++++++++++++++++++------
 git-submodule.sh                  |  7 +++-
 git.c                             |  2 +-
 submodule.c                       | 77 +++++++++++++++++++++++++++++++++++---
 submodule.h                       |  2 +
 t/t7412-submodule-embedgitdirs.sh | 78 +++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh           | 20 +++++++---
 8 files changed, 239 insertions(+), 23 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

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

diff to v1:

index 80d55350eb..34791cfc65 100644
@@ -22,7 +22,8 @@ SYNOPSIS
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
-'git submodule' [--quiet] interngitdirs [--] [<path>...]
+'git submodule' [--quiet] embedgitdirs [--] [<path>...]
+
 
 DESCRIPTION
 -----------

@@ -245,18 +246,18 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
-interngitdirs::
+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 the
+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.
+This command is recursive by default.
 
 OPTIONS
 -------

index 747e934df2..b7e124f340 100755
@@ -1131,9 +1131,9 @@ cmd_sync()
 	done
 }
 
-cmd_interngitdirs()
+cmd_embedgitdirs()
 {
-	git submodule--helper intern-git-dir "$@"
+	git submodule--helper embed-git-dirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the

@@ -1145,7 +1145,7 @@ cmd_interngitdirs()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | status | summary | sync | interngitdirs)
+	add | foreach | init | deinit | update | status | summary | sync | embedgitdirs)
 		command=$1
 		;;
 	-q|--quiet)

index efa1059fe0..98dcf6c518 100644
@@ -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 },

index 859026ecfa..e5975d1f3d 100644
@@ -75,5 +75,6 @@ int parallel_submodules(void);
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 
-extern void migrate_submodule_gitdir(const char *path);
+extern void migrate_submodule_gitdir(const char *prefix,
+				     const char *path, int recursive);
 #endif

deleted file mode 100755
index 709359155d..0000000000
@@ -1,40 +0,0 @@
-#!/bin/sh
-
-test_description='Test submodule interngitdirs
-
-This test verifies that `git submodue interngitdirs` 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 'intern the git dir' '
-	git submodule interngitdirs &&
-	test -f sub1/.git &&
-	test -d .git/modules/sub1 &&
-	# check that we did not break the repository:
-	git status
-'
-
-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 'intern the git dir fails for incomplete submodules' '
-	test_must_fail git submodule interngitdirs &&
-	# check that we did not break the repository:
-	git status
-'
-
-test_done

2.11.0.rc2.4.g3396b6f.dirty

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.

---
 submodule.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index 6f7d883de9..66c5ce5a24 100644
@@ -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.4.g3396b6f.dirty

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.

---
 builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------
 git.c                       |  2 +-
 2 files changed, 21 insertions(+), 12 deletions(-)

index 4beeda5f9f..806e29ce4e 100644
@@ -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]);

index efa1059fe0..98dcf6c518 100644
@@ -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.4.g3396b6f.dirty

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.

---
 t/test-lib-functions.sh | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

index fdaeb3a96b..579e812506 100644
@@ -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.4.g3396b6f.dirty

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.

---
 Documentation/git-submodule.txt   | 14 +++++++
 builtin/submodule--helper.c       | 33 ++++++++++++++++-
 git-submodule.sh                  |  7 +++-
 submodule.c                       | 65 ++++++++++++++++++++++++++++++++
 submodule.h                       |  2 +
 t/t7412-submodule-embedgitdirs.sh | 78 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 197 insertions(+), 2 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

index d841573475..34791cfc65 100644
@@ -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::

index 806e29ce4e..e94dd68a0e 100644
@@ -1076,6 +1076,36 @@ 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;
+
+	struct option embed_gitdir_options[] = {
+		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++)
+		migrate_submodule_gitdir(prefix, list.entries[i]->name, 1);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {

@@ -1093,7 +1123,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)

index a024a135d6..2178248287 100755
@@ -1131,6 +1131,11 @@ cmd_sync()
 	done
 }
 
+cmd_embedgitdirs()
+{
+	git submodule--helper --prefix "$wt_prefix" embed-git-dirs "$@"
+}
+
 # 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)

index d9e197a948..e5975d1f3d 100644
@@ -75,4 +75,6 @@ int parallel_submodules(void);
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 
+extern void migrate_submodule_gitdir(const char *prefix,
+				     const char *path, int recursive);
 #endif

2.11.0.rc2.4.g3396b6f.dirty

---

I spoke too early, this needs to be squashed. :(

Stefan

 builtin/submodule--helper.c | 3 +++
 git-submodule.sh            | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

index e94dd68a0e..75cdbf45b8 100644
@@ -1083,6 +1083,9 @@ static int embed_git_dir(int argc, const char **argv, const char *prefix)
 	struct module_list list = MODULE_LIST_INIT;
 
 	struct option embed_gitdir_options[] = {
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
 		OPT_END()
 	};
 

index 2178248287..b7e124f340 100755
@@ -1133,7 +1133,7 @@ cmd_sync()
 
 cmd_embedgitdirs()
 {
-	git submodule--helper --prefix "$wt_prefix" embed-git-dirs "$@"
+	git submodule--helper embed-git-dirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the

2.11.0.rc2.4.g3396b6f.dirty

Yes. :)

That patch is a sensible approach. :)
(By checking all files to not be submodules a worktree would not run into problems like
a127331cd81, mv: allow moving nested submodules)

ok tell me where this neutral place is found?
(I'd prefer to not clobber it into cache.h *the* most neutral place in git) Maybe dir.{c,h} ?

In the non-main working trees you'd also have a .git file linking to the actual git dir and you'd only have to fix them up instead of moving.

I thought that was a long term goal?
(I actually think about reviving the series you sent out a few weeks ago to make worktree and submodules work well together)

So for that we'd want to have at least the object store shared across all worktrees.

We could signal that via one of the flag bits?
(e.g. FIXUP_WORKTREE_SYMLINKS )

I moved the code to dir.{c,h} and renamed it to a more generic "move_gitdir", but then I am thinking further about how to align worktrees and submodules here, specifically the recursive part.

Whenever submodules and recursion is involved so far we spawned off a child process to take care of the issue in the submodule itself. Here we followed the same pattern and called the submodule--helper to embed the git dirs in the submodule recursively.

As this function is not supposed to be submodule specific anymore, I'd rather not want to have the recursive childrens code live inside the submodule helper, so we also need a neutral helper for moving git directories?

So there are a couple ways forward:
* We declare the "recursive" flag to be submodule specific and keep the recursion
in the submodule helper
* the recursive flag is generic enough and we invent a plumbing helper. Analogous to the submodule--helper, that was originally invented as a C aid for the git-submodule shell script, we could have a "git--helper" or just "git plumbing <subcmd>", though that sounds like reinventing the wheel as traditionally we'd just have a top level command to do a specific thing. (side note: Some parts of git-rev-parse could go into such a plumbing command)

For now I'd think to declare recursion in this function to be submodule specific.

Stefan

Yeah, good suggestion (including name; first round used "intern" I had trouble with, then "embed" which was OK-ish, but probably "relocate" is better choice. If anything, what Stefan's series adds is a command to un-embed embedded one).

Thanks two of you for working well together ;-)

Heh, good counter perspective. I thought about embedding it "into the superprojects git directory" whereas your un-embed sounds like you'd assume embedding is targetd towards the working dir.

relocate as a fancy name for move sounds like it expects 2 arguments (source and destination), but maybe we could go with:

    git relocate-git-dir (--into-workingtree|--into-gitdir) \
      [--recurse-submodules] \
      [--only-fix-gitfile-links-and-core-setting-as-I-moved-it-myself-already] \
      [--dryrun] [--verbose] [--unsafe-move]

No need to have a pathspec here IMHO. Later it could have another flag to specify the "main" worktree or such as well.

I am not sure if you meant this as a submodule-specific subcommand or more general helper. "into-workingtree" suggests to me that it is submodule specific, so I'll base my response on that assumption.

Would there ever be a situation where you already have submodule repositories in the right place (according to the more modern practice, to keep them in .git/modules/ of superproject) and want to move them to embed them in worktrees of submodules? I do not think of any.

If there is no such situation, I do not think we want a verb that is direction-neutral (e.g. "move" or "relocate") with two options. Rather we would want "git submodule unembed-git-dir" or something like that.

"Hi, I made a mistake by using submodules. I don't want to use
them any more, I rather want to:
A) make it a separate git repo again and I'll keep them in sync myself
B) ... "

"I abuse submodules for what git-LFS was designed for, and the
submodule is on a different mount point, please keep the git directory also at that mount point".

Not sure I agree these problems and the proposed solutions are beautiful, but that is what people may think of as a fast hack?

So when we want to have a generic function in C ("relocate_gitdir") for both worktree and submodules, the recursive flag is not supposed to invoke a submodule specific helper, but a generic helper.

Alternatively we make the function not as generic and claim the recursive part is submodule specific and we can happily call "git submodule [un]embed-git-dir" recursively.

OK, I can buy that. Thanks.