[RFC/PATCH 00/10] Support using submodules with worktrees

[RFC/PATCH 00/10] Support using submodules with worktrees

[Nguyễn Thái Ngọc Duy]

Since this topic has come up twice recently, I'll take this opportunity
to gather early comments on this series. It depends nd/config-move-to and
at least has one known problem reported Tomasz. But I'll fix that
later once nd/config-move-to lands.

For more details, head for 01/10. This basically makes it possible
to create secondary worktrees in either supermodules or submodules. The
user will be required to turn on extensions.worktreeConfig and this is
something I'll need to work on, to make the transition smoother.

Note that while it works, it's not really optimal because worktree
clones of the same submodule could be duplicated. But hopefully I could
tackle this one step at a time. Make it work first, shake out bugs, then
think about sharing repos.

Nguyễn Thái Ngọc Duy (10):
  doc: about submodule support with multiple worktrees
  submodule--helper: add missing \n
  submodule add: support multiple worktrees
  submodule init: support multiple worktrees
  submodule update: add tests for multiple worktrees
  submodule sync: support multiple worktrees
  submodule deinit: support multiple worktrees
  submodule clone: use repo_config_set()
  submodule clone: propagate extensions.worktreeConfig
  submodule ensure-core-worktree: write to config.worktree

 Documentation/git-worktree.txt  |   8 ++
 Documentation/gitsubmodules.txt |  17 +++++
 builtin/submodule--helper.c     |  86 ++++++++++++----------
 config.c                        |  10 +++
 config.h                        |   1 +
 git-submodule.sh                |   8 +-
 repository.c                    |  21 ++++--
 repository.h                    |   4 +
 t/t2405-worktree-submodules.sh  | 125 ++++++++++++++++++++++++++++++++
 9 files changed, 228 insertions(+), 52 deletions(-)
 create mode 100755 t/t2405-worktree-submodules.sh

-- 
2.20.0.482.g66447595a7

[PATCH 01/10] doc: about submodule support with multiple worktrees

[Nguyễn Thái Ngọc Duy]

This lays out the very first step of making multiple worktrees and
submodules work together.

The first problem is git-submodule keeps per-worktree config in
$GIT_DIR/config, which is shared for all worktrees. This series makes
git-submodule use extensions.worktreeConfig and write submodule.* to
config.worktree instead.

The rest goes on and on about the remaining problems. But let's talk a
bit more about solving the first problem. Since it relies on the
experimental extensions.worktreeConfig, this support is of course also
experimental. On the other hand, submodules have never really worked
with multi worktrees before, this change can't bite anybody.

Second problem. That is about multiple worktrees at superproject
level. At submodule level, we still can't have multiple worktrees
because git-submodule writes to submodule's "config" file
(again). Fixing this is not particularly hard. Absorbing submodule's
git dir takes some work but is feasible. This could be addressed soon
in the future.

The third problem is a big and complicaed one. Submodule clones
(inside the superproject) are per-worktree. So if you have two
worktrees, and these have one submodule, you need space for _two_
clones. This is definitely not elegant. The tenative plan is to move
clones from $GIT_COMMON_DIR/worktrees/X/modules to
$GIT_COMMON_DIR/common/modules.

The latter directory is shared across all worktrees. Once we keep the
clone in a common place, the submodule's worktree can be created and
managed with git-worktree[1].

Another good point about this approach is we could finally safely
allow "git worktree remove" to work with submodules. With current
solution, removing $GIT_COMMON_DIR/worktrees/X directory means also
removing potentially precious clones inside the "modules" subdir.

But whether we can do this depends on:

- if we need separate ref namespace for submodule on each worktree

- how does submodule's worktrees (remember the second problem)
  interact these worktrees

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt  |  8 ++++++++
 Documentation/gitsubmodules.txt | 17 +++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index aae8e1d8b2..3510fd5331 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -267,6 +267,14 @@ configuration that you do not want to share to all working trees:
  - `core.sparseCheckout` is recommended per working tree, unless you
    are sure you always use sparse checkout for all working trees.
 
+ - Most configuration variables under `submodule` group in superproject
+   should not be shared.
++
+------------
+$ git config --local --move-to --worktree submodule.active
+$ git config --local --move-to-regexp --worktree 'submodule\..*\..*'
+------------
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's
diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index 57999e9f36..d91817b45d 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -222,6 +222,23 @@ submodule active pathspec, which specifies that any submodule
 starting with 'b' except 'baz' are also active, regardless of the
 presence of the .url field.
 
+MULTIPLE WORKING TREE SUPPORT
+-----------------------------
+When you have more than one working tree, created by
+linkgit:git-worktree[1], submodules will not work on any working tree
+until `extensions.worktreeConfig` is enabled. Since this config
+affects more than just submodules, please see "CONFIGURATION FILE"
+section for more information before turning it on.
+
+Once on, submodules can be added in any working tree. The submodule
+itself though cannot have more than one working tree.
+
+When submodules are created in a working tree, their git directory is
+also per-worktree, e.g. inside
+'$GIT_COMMON_DIR/worktrees/<worktree>/modules' and not shared with
+other working trees. This means if you have the same submodule on
+different working trees, you need disk space for multiple clones.
+
 Workflow for a third party library
 ----------------------------------
 
-- 
2.20.0.482.g66447595a7

[PATCH 02/10] submodule--helper: add missing \n

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d38113a31a..232bfaac7f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1297,7 +1297,7 @@ static int add_possible_reference_from_superproject(
 				die(_("submodule '%s' cannot add alternate: %s"),
 				    sas->submodule_name, err.buf);
 			case SUBMODULE_ALTERNATE_ERROR_INFO:
-				fprintf(stderr, _("submodule '%s' cannot add alternate: %s"),
+				fprintf(stderr, _("submodule '%s' cannot add alternate: %s\n"),
 					sas->submodule_name, err.buf);
 			case SUBMODULE_ALTERNATE_ERROR_IGNORE:
 				; /* nothing */
-- 
2.20.0.482.g66447595a7

[PATCH 03/10] submodule add: support multiple worktrees

[Nguyễn Thái Ngọc Duy]

The entire submodule section in the superproject will be per-worktree
and written to $GIT_DIR/config.worktree.

The behavior when you only have one worktree (and not enabled
extensions.worktreeConfig) is the same as before, everything is still
written in $GIT_DIR/config

'submodule-helper config --check-writable' also checks if it's
supported worktree configuration.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c    | 17 +++++++++++++-
 git-submodule.sh               |  8 +++----
 t/t2405-worktree-submodules.sh | 42 ++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 6 deletions(-)
 create mode 100755 t/t2405-worktree-submodules.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 232bfaac7f..7b328ec060 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -18,6 +18,7 @@
 #include "diffcore.h"
 #include "diff.h"
 #include "object-store.h"
+#include "worktree.h"
 
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
@@ -27,6 +28,18 @@
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
 				  void *cb_data);
 
+static void check_multi_worktree_support(void)
+{
+	char *worktree_config = get_worktree_config(the_repository);
+	if (!worktree_config)
+		die(_("submodules cannot be used with multiple "
+		      "working trees unless the config\n"
+		      "extension worktreeConfig is enabled. "
+		      "Please read \"CONFIGURATION FILE\"\n"
+		      "section in \"git help worktree\" for details"));
+	free(worktree_config);
+}
+
 static char *get_default_remote(void)
 {
 	char *dest = NULL, *ret;
@@ -2162,8 +2175,10 @@ static int module_config(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_config_options,
 			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
-	if (argc == 1 && command == CHECK_WRITEABLE)
+	if (argc == 1 && command == CHECK_WRITEABLE) {
+		check_multi_worktree_support();
 		return is_writing_gitmodules_ok() ? 0 : -1;
+	}
 
 	/* Equivalent to ACTION_GET in builtin/config.c */
 	if (argc == 2)
diff --git a/git-submodule.sh b/git-submodule.sh
index 5e608f8bad..695939eff9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -288,7 +288,7 @@ or you are unsure what this means choose another name with the '--name' option."
 			esac
 		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
 	fi
-	git config submodule."$sm_name".url "$realrepo"
+	git config --worktree submodule."$sm_name".url "$realrepo"
 
 	git add --no-warn-embedded-repo $force "$sm_path" ||
 	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
@@ -302,18 +302,16 @@ or you are unsure what this means choose another name with the '--name' option."
 	git add --force .gitmodules ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
 
-	# NEEDSWORK: In a multi-working-tree world, this needs to be
-	# set in the per-worktree config.
 	if git config --get submodule.active >/dev/null
 	then
 		# If the submodule being adding isn't already covered by the
 		# current configured pathspec, set the submodule's active flag
 		if ! git submodule--helper is-active "$sm_path"
 		then
-			git config submodule."$sm_name".active "true"
+			git config --worktree submodule."$sm_name".active "true"
 		fi
 	else
-		git config submodule."$sm_name".active "true"
+		git config --worktree submodule."$sm_name".active "true"
 	fi
 }
 
diff --git a/t/t2405-worktree-submodules.sh b/t/t2405-worktree-submodules.sh
new file mode 100755
index 0000000000..3ee5380e88
--- /dev/null
+++ b/t/t2405-worktree-submodules.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='multiple worktrees as superprojects'
+
+. ./test-lib.sh
+
+test_expect_success 'set up submodule source' '
+	test_create_repo submodsrc &&
+	(
+		cd submodsrc &&
+		test_commit one
+	) &&
+	test_commit initial &&
+	git worktree add -b secondary secondary HEAD &&
+	git config extensions.worktreeConfig true
+'
+
+test_expect_success 'add submodules' '
+	SRC="$(pwd)/submodsrc" &&
+	git submodule add "$SRC" sub1 &&
+	git commit -m sub1 &&
+	git -C secondary submodule add "$SRC" sub2 &&
+	git -C secondary commit -m sub2 &&
+
+	git config --get-regexp "submodule.*" | sort >actual1 &&
+	cat >expected1 <<-EOF &&
+	submodule.sub1.active true
+	submodule.sub1.url $(pwd)/submodsrc
+	EOF
+	test_cmp expected1 actual1 &&
+	test -d .git/modules/sub1 &&
+
+	git -C secondary config --get-regexp "submodule.*" | sort >actual2 &&
+	cat >expected2 <<-EOF &&
+	submodule.sub2.active true
+	submodule.sub2.url $(pwd)/submodsrc
+	EOF
+	test_cmp expected2 actual2 &&
+	test -d .git/worktrees/secondary/modules/sub2
+'
+
+test_done
-- 
2.20.0.482.g66447595a7

[PATCH 04/10] submodule init: support multiple worktrees

[Nguyễn Thái Ngọc Duy]

The entire submodule section in the superproject will be per-worktree
and written to $GIT_DIR/config.worktree.

The behavior when you only have one worktree (and not enabled
extensions.worktreeConfig) is the same as before, everything is still
written in $GIT_DIR/config

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c    | 14 +++++---------
 t/t2405-worktree-submodules.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7b328ec060..6b749b41fb 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -639,15 +639,11 @@ static void init_submodule(const char *path, const char *prefix,
 		die(_("No url found for submodule path '%s' in .gitmodules"),
 			displaypath);
 
-	/*
-	 * NEEDSWORK: In a multi-working-tree world, this needs to be
-	 * set in the per-worktree config.
-	 *
-	 * Set active flag for the submodule being initialized
-	 */
+	check_multi_worktree_support();
+
 	if (!is_submodule_active(the_repository, path)) {
 		strbuf_addf(&sb, "submodule.%s.active", sub->name);
-		git_config_set_gently(sb.buf, "true");
+		repo_config_set_worktree_gently(the_repository, sb.buf, "true");
 		strbuf_reset(&sb);
 	}
 
@@ -672,7 +668,7 @@ static void init_submodule(const char *path, const char *prefix,
 			free(oldurl);
 		}
 
-		if (git_config_set_gently(sb.buf, url))
+		if (repo_config_set_worktree_gently(the_repository, sb.buf, url))
 			die(_("Failed to register url for submodule path '%s'"),
 			    displaypath);
 		if (!(flags & OPT_QUIET))
@@ -693,7 +689,7 @@ static void init_submodule(const char *path, const char *prefix,
 		} else
 			upd = xstrdup(submodule_strategy_to_string(&sub->update_strategy));
 
-		if (git_config_set_gently(sb.buf, upd))
+		if (repo_config_set_worktree_gently(the_repository, sb.buf, upd))
 			die(_("Failed to register update mode for submodule path '%s'"), displaypath);
 	}
 	strbuf_release(&sb);
diff --git a/t/t2405-worktree-submodules.sh b/t/t2405-worktree-submodules.sh
index 3ee5380e88..2ee3e2d039 100755
--- a/t/t2405-worktree-submodules.sh
+++ b/t/t2405-worktree-submodules.sh
@@ -39,4 +39,32 @@ test_expect_success 'add submodules' '
 	test -d .git/worktrees/secondary/modules/sub2
 '
 
+test_expect_success 'init submodules' '
+	git clone --no-local .git cloned &&
+	(
+		cd cloned &&
+		git config extensions.worktreeConfig true &&
+		git submodule init sub1 &&
+		git worktree add secondary origin/secondary &&
+		cd secondary &&
+		git submodule init sub2
+	) &&
+
+	git -C cloned config --get-regexp "submodule.*" | sort >actual1 &&
+	cat >expected1 <<-EOF &&
+	submodule.sub1.active true
+	submodule.sub1.url $(pwd)/submodsrc
+	EOF
+	test_cmp expected1 actual1 &&
+	! test -d cloned/.git/modules/sub1 &&
+
+	git -C cloned/secondary config --get-regexp "submodule.*" | sort >actual2 &&
+	cat >expected2 <<-EOF &&
+	submodule.sub2.active true
+	submodule.sub2.url $(pwd)/submodsrc
+	EOF
+	test_cmp expected2 actual2 &&
+	! test -d cloned/.git/worktrees/secondary/modules/sub2
+'
+
 test_done
-- 
2.20.0.482.g66447595a7

[PATCH 05/10] submodule update: add tests for multiple worktrees

[Nguyễn Thái Ngọc Duy]

There are no changes needed for 'submodule update'. The clones will be
per-worktree and all the support is already in place. Add a test to
make sure it actually works.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2405-worktree-submodules.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t2405-worktree-submodules.sh b/t/t2405-worktree-submodules.sh
index 2ee3e2d039..c1b19ad613 100755
--- a/t/t2405-worktree-submodules.sh
+++ b/t/t2405-worktree-submodules.sh
@@ -67,4 +67,15 @@ test_expect_success 'init submodules' '
 	! test -d cloned/.git/worktrees/secondary/modules/sub2
 '
 
+test_expect_success 'update submodules' '
+	(
+		cd cloned &&
+		git submodule update sub1 &&
+		cd secondary &&
+		git submodule update sub2
+	) &&
+	test -d cloned/.git/modules/sub1 &&
+	test -d cloned/.git/worktrees/secondary/modules/sub2
+'
+
 test_done
-- 
2.20.0.482.g66447595a7

[PATCH 06/10] submodule sync: support multiple worktrees

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c    | 12 +++++++-----
 t/t2405-worktree-submodules.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6b749b41fb..288858af83 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -939,6 +939,7 @@ static void sync_submodule(const char *path, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	char *sub_config_path = NULL;
+	struct repository subrepo;
 
 	if (!is_submodule_active(the_repository, path))
 		return;
@@ -979,7 +980,7 @@ static void sync_submodule(const char *path, const char *prefix,
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "submodule.%s.url", sub->name);
-	if (git_config_set_gently(sb.buf, super_config_url))
+	if (repo_config_set_worktree_gently(the_repository, sb.buf, super_config_url))
 		die(_("failed to register url for submodule path '%s'"),
 		      displaypath);
 
@@ -1000,14 +1001,15 @@ static void sync_submodule(const char *path, const char *prefix,
 	strbuf_strip_suffix(&sb, "\n");
 	remote_key = xstrfmt("remote.%s.url", sb.buf);
 
-	strbuf_reset(&sb);
-	submodule_to_gitdir(&sb, path);
-	strbuf_addstr(&sb, "/config");
+	if (repo_submodule_init(&subrepo, the_repository, path))
+		die(_("could not get a repository handle for submodule '%s'"), path);
 
-	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+	if (repo_config_set_worktree_gently(&subrepo, remote_key, sub_origin_url))
 		die(_("failed to update remote for submodule '%s'"),
 		      path);
 
+	repo_clear(&subrepo);
+
 	if (flags & OPT_RECURSIVE) {
 		struct child_process cpr = CHILD_PROCESS_INIT;
 
diff --git a/t/t2405-worktree-submodules.sh b/t/t2405-worktree-submodules.sh
index c1b19ad613..55daace672 100755
--- a/t/t2405-worktree-submodules.sh
+++ b/t/t2405-worktree-submodules.sh
@@ -78,4 +78,37 @@ test_expect_success 'update submodules' '
 	test -d cloned/.git/worktrees/secondary/modules/sub2
 '
 
+test_expect_success 'sync submodules' '
+	sed s/submodsrc/crsdombus/ .gitmodules >temp &&
+	mv temp .gitmodules &&
+	git submodule sync &&
+	(
+		cd secondary &&
+		sed s/submodsrc/crsdombus/ .gitmodules >temp &&
+		mv temp .gitmodules &&
+		git submodule sync
+	) &&
+
+	git config --get-regexp "submodule.*" | sort >actual1 &&
+	cat >expected1 <<-EOF &&
+	submodule.sub1.active true
+	submodule.sub1.url $(pwd)/crsdombus
+	EOF
+	test_cmp expected1 actual1 &&
+
+	git -C secondary config --get-regexp "submodule.*" | sort >actual2 &&
+	cat >expected2 <<-EOF &&
+	submodule.sub2.active true
+	submodule.sub2.url $(pwd)/crsdombus
+	EOF
+	test_cmp expected2 actual2
+'
+
+test_expect_success 'reset sync submodules' '
+	git checkout -- .gitmodules &&
+	git submodule sync &&
+	git -C secondary checkout -- .gitmodules &&
+	git -C secondary submodule sync
+'
+
 test_done
-- 
2.20.0.482.g66447595a7

[PATCH 07/10] submodule deinit: support multiple worktrees

[Nguyễn Thái Ngọc Duy]

This is the reverse of 'init' or 'add', config from superproject is
removed from config.worktree (or just config in standard single
worktree setup)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c    |  8 ++++++--
 t/t2405-worktree-submodules.sh | 11 +++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 288858af83..8943127ae7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1097,6 +1097,8 @@ static void deinit_submodule(const char *path, const char *prefix,
 	struct strbuf sb_config = STRBUF_INIT;
 	char *sub_git_dir = xstrfmt("%s/.git", path);
 
+	check_multi_worktree_support();
+
 	sub = submodule_from_path(the_repository, &null_oid, path);
 
 	if (!sub || !sub->name)
@@ -1150,21 +1152,23 @@ static void deinit_submodule(const char *path, const char *prefix,
 		      displaypath);
 
 	cp_config.git_cmd = 1;
-	argv_array_pushl(&cp_config.args, "config", "--get-regexp", NULL);
+	argv_array_pushl(&cp_config.args, "config", "--worktree", "--get-regexp", NULL);
 	argv_array_pushf(&cp_config.args, "submodule.%s\\.", sub->name);
 
 	/* remove the .git/config entries (unless the user already did it) */
 	if (!capture_command(&cp_config, &sb_config, 0) && sb_config.len) {
+		char *config_file = get_worktree_config(the_repository);
 		char *sub_key = xstrfmt("submodule.%s", sub->name);
 		/*
 		 * remove the whole section so we have a clean state when
 		 * the user later decides to init this submodule again
 		 */
-		git_config_rename_section_in_file(NULL, sub_key, NULL);
+		git_config_rename_section_in_file(config_file, sub_key, NULL);
 		if (!(flags & OPT_QUIET))
 			printf(_("Submodule '%s' (%s) unregistered for path '%s'\n"),
 				 sub->name, sub->url, displaypath);
 		free(sub_key);
+		free(config_file);
 	}
 
 cleanup:
diff --git a/t/t2405-worktree-submodules.sh b/t/t2405-worktree-submodules.sh
index 55daace672..8d53f639c9 100755
--- a/t/t2405-worktree-submodules.sh
+++ b/t/t2405-worktree-submodules.sh
@@ -111,4 +111,15 @@ test_expect_success 'reset sync submodules' '
 	git -C secondary submodule sync
 '
 
+test_expect_success 'deinit submodules' '
+	git submodule deinit sub1 &&
+	git -C secondary submodule deinit sub2 &&
+	git config --get-regexp "submodule.*" | sort >actual1 &&
+	test_must_be_empty actual1 &&
+	test -d .git/modules/sub1 &&
+	git -C secondary config --get-regexp "submodule.*" | sort >actual2 &&
+	test_must_be_empty actual2 &&
+	test -d .git/worktrees/secondary/modules/sub2
+'
+
 test_done
-- 
2.20.0.482.g66447595a7

[PATCH 08/10] submodule clone: use repo_config_set()

[Nguyễn Thái Ngọc Duy]

Avoid direct access to $SUBMODULE/config and do it through
repo_config_set() instead. Note that repo_submodule_init() cannot be
used because this early in the submodule initialization process, we
may fail to get and parse .gitmodules with submodule_from_path().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c | 24 +++++++-----------------
 config.c                    | 10 ++++++++++
 config.h                    |  1 +
 repository.c                | 21 ++++++++++++++-------
 repository.h                |  4 ++++
 5 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8943127ae7..b5d74cd415 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1366,11 +1366,11 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	const char *name = NULL, *url = NULL, *depth = NULL;
 	int quiet = 0;
 	int progress = 0;
-	char *p, *path = NULL, *sm_gitdir;
+	char *path = NULL, *sm_gitdir;
 	struct strbuf sb = STRBUF_INIT;
 	struct string_list reference = STRING_LIST_INIT_NODUP;
 	int dissociate = 0;
-	char *sm_alternate = NULL, *error_strategy = NULL;
+	struct repository subrepo;
 
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -1443,27 +1443,17 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 
 	connect_work_tree_and_git_dir(path, sm_gitdir, 0);
 
-	p = git_pathdup_submodule(path, "config");
-	if (!p)
-		die(_("could not get submodule directory for '%s'"), path);
+	if (repo_submodule_init_by_name(&subrepo, the_repository, path, name))
+		die(_("could not get a repository handle for submodule '%s'"), path);
 
 	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
-	git_config_get_string("submodule.alternateLocation", &sm_alternate);
-	if (sm_alternate)
-		git_config_set_in_file(p, "submodule.alternateLocation",
-					   sm_alternate);
-	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
-	if (error_strategy)
-		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
-					   error_strategy);
-
-	free(sm_alternate);
-	free(error_strategy);
+	repo_config_copy(&subrepo, the_repository, "submodule.alternateLocation");
+	repo_config_copy(&subrepo, the_repository, "submodule.alternateErrorStrategy");
 
+	repo_clear(&subrepo);
 	strbuf_release(&sb);
 	free(sm_gitdir);
 	free(path);
-	free(p);
 	return 0;
 }
 
diff --git a/config.c b/config.c
index 151d28664e..007436b382 100644
--- a/config.c
+++ b/config.c
@@ -2157,6 +2157,16 @@ void repo_config_set(struct repository *r, const char *key, const char *value)
 		die(_("could not unset '%s'"), key);
 }
 
+void repo_config_copy(struct repository *dst, struct repository *src, const char *key)
+{
+	char *value = NULL;
+
+	repo_config_get_string(src, key, &value);
+	if (value)
+		repo_config_set(dst, key, value);
+	free(value);
+}
+
 int repo_config_set_worktree_gently(struct repository *r,
 				    const char *key, const char *value)
 {
diff --git a/config.h b/config.h
index 62204dc252..22edd96716 100644
--- a/config.h
+++ b/config.h
@@ -105,6 +105,7 @@ extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
 extern int repo_config_set_gently(struct repository *, const char *, const char *);
 extern void repo_config_set(struct repository *, const char *, const char *);
+extern void repo_config_copy(struct repository *dst, struct repository *src, const char *key);
 extern int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
 extern void git_config_set(const char *, const char *);
 extern int git_config_parse_key(const char *, char **, int *);
diff --git a/repository.c b/repository.c
index 5dd1486718..f997bd1629 100644
--- a/repository.c
+++ b/repository.c
@@ -176,16 +176,23 @@ int repo_submodule_init(struct repository *submodule,
 			const char *path)
 {
 	const struct submodule *sub;
+
+	sub = submodule_from_path(superproject, &null_oid, path);
+	if (!sub)
+		return -1;
+	return repo_submodule_init_by_name(submodule, superproject,
+					   path, sub->name);
+}
+
+int repo_submodule_init_by_name(struct repository *submodule,
+				struct repository *superproject,
+				const char *path,
+				const char *name)
+{
 	struct strbuf gitdir = STRBUF_INIT;
 	struct strbuf worktree = STRBUF_INIT;
 	int ret = 0;
 
-	sub = submodule_from_path(superproject, &null_oid, path);
-	if (!sub) {
-		ret = -1;
-		goto out;
-	}
-
 	strbuf_repo_worktree_path(&gitdir, superproject, "%s/.git", path);
 	strbuf_repo_worktree_path(&worktree, superproject, "%s", path);
 
@@ -199,7 +206,7 @@ int repo_submodule_init(struct repository *submodule,
 		 */
 		strbuf_reset(&gitdir);
 		strbuf_repo_git_path(&gitdir, superproject,
-				     "modules/%s", sub->name);
+				     "modules/%s", name);
 
 		if (repo_init(submodule, gitdir.buf, NULL)) {
 			ret = -1;
diff --git a/repository.h b/repository.h
index 9f16c42c1e..d3f0592471 100644
--- a/repository.h
+++ b/repository.h
@@ -119,6 +119,10 @@ int repo_init(struct repository *r, const char *gitdir, const char *worktree);
 int repo_submodule_init(struct repository *submodule,
 			struct repository *superproject,
 			const char *path);
+int repo_submodule_init_by_name(struct repository *submodule,
+				struct repository *superproject,
+				const char *path,
+				const char *name);
 void repo_clear(struct repository *repo);
 
 /*
-- 
2.20.0.482.g66447595a7

[PATCH 09/10] submodule clone: propagate extensions.worktreeConfig

[Nguyễn Thái Ngọc Duy]

If this extension is already enabled on the superproject, the user
likes to use multiple worktrees very much and is already aware of
it. Enable it in submodules too so that when they want to make
secondary submodule worktrees, they will not be worried about enabling
it or migrating per-worktree config away.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b5d74cd415..8a12d2f0ed 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1449,6 +1449,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
 	repo_config_copy(&subrepo, the_repository, "submodule.alternateLocation");
 	repo_config_copy(&subrepo, the_repository, "submodule.alternateErrorStrategy");
+	repo_config_copy(&subrepo, the_repository, "extensions.worktreeConfig");
 
 	repo_clear(&subrepo);
 	strbuf_release(&sb);
-- 
2.20.0.482.g66447595a7

[PATCH 10/10] submodule ensure-core-worktree: write to config.worktree

[Nguyễn Thái Ngọc Duy]

Use repo_config_set_worktree_gently() to write core.worktree. If the
submodule has extensions.worktreeConfig on, the key will be in
config.worktree instead of config file and not affect any new
submodule's worktrees.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/submodule--helper.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8a12d2f0ed..890de5902c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2063,18 +2063,16 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
 		die(_("could not get a repository handle for submodule '%s'"), path);
 
 	if (!repo_config_get_string(&subrepo, "core.worktree", &cw)) {
-		char *cfg_file, *abs_path;
+		char *abs_path;
 		const char *rel_path;
 		struct strbuf sb = STRBUF_INIT;
 
-		cfg_file = repo_git_path(&subrepo, "config");
-
 		abs_path = absolute_pathdup(path);
 		rel_path = relative_path(abs_path, subrepo.gitdir, &sb);
 
-		git_config_set_in_file(cfg_file, "core.worktree", rel_path);
+		if (repo_config_set_worktree_gently(&subrepo, "core.worktree", rel_path))
+			die(_("could not set '%s' to '%s'"), "core.worktree", rel_path);
 
-		free(cfg_file);
 		free(abs_path);
 		strbuf_release(&sb);
 	}
-- 
2.20.0.482.g66447595a7

Re: [PATCH 01/10] doc: about submodule support with multiple worktrees

[Stefan Beller]

> The third problem is a big and complicaed one. Submodule clones

complicated

> (inside the superproject) are per-worktree. So if you have two
> worktrees, and these have one submodule, you need space for _two_
> clones. This is definitely not elegant. The tenative plan is to move

tentative?

> clones from $GIT_COMMON_DIR/worktrees/X/modules to
> $GIT_COMMON_DIR/common/modules.

... from the submodule side of things this is big problem,
as the submodule is usually assumed at $GIT_DIR/modules/<name>
(I think we have not been strict in $GIT_DIR $GIT_COMMON_DIR
in the submodule code)

So the plan is to neither use
$GIT_COMMON_DIR/worktrees/X/modules/Y
$GIT_COMMON_DIR/modules/Y/worktrees/X
but to create a new third location at
$GIT_COMMON_DIR/common/modules
to which either new submodule worktrees
or superproject worktrees that happen to have this submodule
can point to?

>
> The latter directory is shared across all worktrees. Once we keep the
> clone in a common place, the submodule's worktree can be created and
> managed with git-worktree[1].

So even when the user has never heard of worktrees, the internal structure
will be worktree oriented, the common dir in common/modules/Y and in
$GIT_DIR/modules/Y we could just have a worktree git dir?

> Another good point about this approach is we could finally safely
> allow "git worktree remove" to work with submodules. With current
> solution, removing $GIT_COMMON_DIR/worktrees/X directory means also
> removing potentially precious clones inside the "modules" subdir.

yup, very sensible.

I like this approach very much.

Re: [PATCH 03/10] submodule add: support multiple worktrees

[Stefan Beller]

> --- /dev/null
> +++ b/t/t2405-worktree-submodules.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +
> +test_description='multiple worktrees as superprojects'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'set up submodule source' '
> +       test_create_repo submodsrc &&
> +       (
> +               cd submodsrc &&
> +               test_commit one
> +       ) &&

Just like git itself, test_commit now supports the -C <dir>
argument, so you could replace the whole subshell with

    test_commit -C submodsrc one &&


> +       test_commit initial &&
> +       git worktree add -b secondary secondary HEAD &&

Could we have multiple things not named the same?
(i.e. have the the branch and worktree be spelled differently)
That way it is less confusing and we'd catch errors of swapping
these two in the implementation (unlikely, but...)

> +       git config extensions.worktreeConfig true
> +'
> +
> +test_expect_success 'add submodules' '
> +       SRC="$(pwd)/submodsrc" &&
> +       git submodule add "$SRC" sub1 &&

you can also use relative paths in submodules:

    git submodule add ./submodsrc sub1 &&

if you want to.

> +       git commit -m sub1 &&
> +       git -C secondary submodule add "$SRC" sub2 &&

Oh never mind, by having the absolute path we
add the submodule with the same setting.
(When using relative path we'd have to use ../submodusrc
here, but would that matter?)

> +       git -C secondary commit -m sub2 &&
> +
> +       git config --get-regexp "submodule.*" | sort >actual1 &&
> +       cat >expected1 <<-EOF &&
> +       submodule.sub1.active true
> +       submodule.sub1.url $(pwd)/submodsrc
> +       EOF
> +       test_cmp expected1 actual1 &&
> +       test -d .git/modules/sub1 &&
> +
> +       git -C secondary config --get-regexp "submodule.*" | sort >actual2 &&
> +       cat >expected2 <<-EOF &&
> +       submodule.sub2.active true
> +       submodule.sub2.url $(pwd)/submodsrc
> +       EOF
> +       test_cmp expected2 actual2 &&
> +       test -d .git/worktrees/secondary/modules/sub2

This section is very brittle. For example the way how submodules
are active changed not so long ago, and might change again, which
seems unrelated to the thing tested here?

Re: [PATCH 01/10] doc: about submodule support with multiple worktrees

[Duy Nguyen]

On Thu, Jan 17, 2019 at 5:06 AM Stefan Beller <sbeller@google.com> wrote:
> > clones from $GIT_COMMON_DIR/worktrees/X/modules to
> > $GIT_COMMON_DIR/common/modules.
>
> ... from the submodule side of things this is big problem,
> as the submodule is usually assumed at $GIT_DIR/modules/<name>
> (I think we have not been strict in $GIT_DIR $GIT_COMMON_DIR
> in the submodule code)
>
> So the plan is to neither use
>
> $GIT_COMMON_DIR/worktrees/X/modules/Y
> $GIT_COMMON_DIR/modules/Y/worktrees/X
>
> but to create a new third location at
>
> $GIT_COMMON_DIR/common/modules
>
> to which either new submodule worktrees
> or superproject worktrees that happen to have this submodule
> can point to?

Yes. And this is yet another "absorb" operation to move from the
current location to the new one. The code will have to check both
places, just as it has to check if "foo/bar/.git" is a real repo
before it goes to $GIT_DIR/.git/modules/bar (at least this is how I
understand it).

> > The latter directory is shared across all worktrees. Once we keep the
> > clone in a common place, the submodule's worktree can be created and
> > managed with git-worktree[1].
>
> So even when the user has never heard of worktrees, the internal structure
> will be worktree oriented, the common dir in common/modules/Y and in
> $GIT_DIR/modules/Y we could just have a worktree git dir?

I think the .git dir will be in common/modules/Y. $GIT_DIR/modules/Y
is basically replaced by $GIT_COMMON_DIR/common/modules/Y, which
should work even when you don't use git-worktree (in single-worktree
setting, $GIT_COMMON_DIR == $GIT_DIR).
-- 
Duy