checkout/reset/read-tree: fix --recurse-submodules in linked worktree

This series fixes the behaviour of git checkout/reset/read-tree --recurse-submodules when they are called in a linked worktree (created by git worktree add).

Although submodules are cloned in $GIT_COMMON_DIR/worktrees/<name>/modules upon issuing git submodule update in the linked worktree, any invocation of git checkout/reset/read-tree --recurse-submodules that changes the state of the submodule(s) will incorrectly operate on the repositories of the submodules in the main worktree, i.e. the ones at $GIT_COMMON_DIR/modules/.

The fourth patch fixes this behaviour by using get_git_dir() instead of git_common_dir() in submodule.c::submodule_move_head and submodule.c::submodule_unset_core_worktree to construct the path to the submodule repository.

The first 3 patches are clean-up patches on t7410-submodule-checkout-to.sh (renamed to t2405-worktree-submodule.sh) to bring it up to date.

Note: in the second test added in the fourth patch I used test_might_fail such that when the test is run on the current master, only the last test_cmp makes the test fail. If we want to be more strict I'll change that to :

index eba17d9e35..31d156cce7 100755
@@ -70,9 +70,9 @@ test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules
 test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
     git -C main/sub config --get core.worktree > expect &&
     git -C checkout-recurse checkout --recurse-submodules first &&
-    test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree > linked-config &&
+    test_expect_code 1 git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree > linked-config &&
     test_must_be_empty linked-config &&
-    test_might_fail git -C main/sub config --get core.worktree > actual &&
+    git -C main/sub config --get core.worktree > actual &&
     test_cmp expect actual
 '
 

Cc:Max Kirillov [email protected] [[email protected]] Brandon Williams [email protected] [[email protected]] Jonathan Tan [email protected] [[email protected]] Stefan Beller [email protected] [[email protected]] Nguyễn Thái Ngọc Duy [email protected] [[email protected]]

Philippe Blain (4):
t7410: rename to t2405-worktree-submodule.sh
t2405: use git -C and test_commit -C instead of subshells
t2405: clarify test descriptions and simplify test submodule.c: use get_git_dir() instead of get_git_common_dir()

 submodule.c                      |  6 +--
 t/t2405-worktree-submodule.sh    | 79 ++++++++++++++++++++++++++++++++
 t/t7410-submodule-checkout-to.sh | 77 -------------------------------
 3 files changed, 82 insertions(+), 80 deletions(-)
 create mode 100755 t/t2405-worktree-submodule.sh
 delete mode 100755 t/t7410-submodule-checkout-to.sh

base-commit: b4615e40a8125477e18490d868f7b65954372b43
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-523%2Fphil-blain%2Fcheckout-recurse-in-linked-worktree-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-523/phil-blain/checkout-recurse-in-linked-worktree-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/523

gitgitgadget

From: Philippe Blain <[email protected]>

This test was added in df56607dff (git-common-dir: make "modules/" per-working-directory directory, 2014-11-30), back when the 'git worktree' command did not exist and 'git checkout --to' was used to create supplementary worktrees.

Since this file contains tests for the interaction of 'git worktree' with submodules, rename it to t2405-worktree-submodule.sh, following the naming scheme for tests checking the behavior of various commands with submodules.

---
 ...410-submodule-checkout-to.sh => t2405-worktree-submodule.sh} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename t/{t7410-submodule-checkout-to.sh => t2405-worktree-submodule.sh} (96%)

similarity index 96%
rename from t/t7410-submodule-checkout-to.sh
rename to t/t2405-worktree-submodule.sh
index f1b492ebc4..f2eee328cc 100755
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='Combination of submodules and multiple workdirs'
+test_description='Combination of submodules and multiple worktrees'
 
 . ./test-lib.sh
 

gitgitgadget

From: Philippe Blain <[email protected]>

The subshells used in the setup phase of this test are unnecessary.

Remove them by using 'git -C' and 'test_commit -C'.

---
 t/t2405-worktree-submodule.sh | 40 +++++++++++------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

index f2eee328cc..b9040c30d4 100755
@@ -6,32 +6,16 @@ test_description='Combination of submodules and multiple worktrees'
 
 base_path=$(pwd -P)
 
-test_expect_success 'setup: make origin'  '
-	mkdir -p origin/sub &&
-	(
-		cd origin/sub && git init &&
-		echo file1 >file1 &&
-		git add file1 &&
-		git commit -m file1
-	) &&
-	mkdir -p origin/main &&
-	(
-		cd origin/main && git init &&
-		git submodule add ../sub &&
-		git commit -m "add sub"
-	) &&
-	(
-		cd origin/sub &&
-		echo file1updated >file1 &&
-		git add file1 &&
-		git commit -m "file1 updated"
-	) &&
+test_expect_success 'setup: create origin repos'  '
+	git init origin/sub &&
+	test_commit -C origin/sub file1 &&
+	git init origin/main &&
+	git -C origin/main submodule add ../sub &&
+	git -C origin/main commit -m "add sub" &&
+	test_commit -C origin/sub "file1-updated" file1 file1updated &&
 	git -C origin/main/sub pull &&
-	(
-		cd origin/main &&
-		git add sub &&
-		git commit -m "sub updated"
-	)
+	git -C origin/main add sub &&
+	git -C origin/main commit -m "sub updated"
 '
 
 test_expect_success 'setup: clone' '

@@ -49,7 +33,7 @@ test_expect_success 'checkout main' '
 
 test_expect_failure 'can see submodule diffs just after checkout' '
 	git -C default_checkout/main diff --submodule master"^!" >out &&
-	grep "file1 updated" out
+	grep "file1-updated" out
 '
 
 test_expect_success 'checkout main and initialize independent clones' '

@@ -60,7 +44,7 @@ test_expect_success 'checkout main and initialize independent clones' '
 
 test_expect_success 'can see submodule diffs after independent cloning' '
 	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
-	grep "file1 updated" out
+	grep "file1-updated" out
 '
 
 test_expect_success 'checkout sub manually' '

@@ -71,7 +55,7 @@ test_expect_success 'checkout sub manually' '
 
 test_expect_success 'can see submodule diffs after manual checkout of linked submodule' '
 	git -C linked_submodule/main diff --submodule master"^!" >out &&
-	grep "file1 updated" out
+	grep "file1-updated" out
 '
 
 test_done

gitgitgadget

The subshells may not be necessary, but the code feels cleaner before this patch is applied since all the added "-C foo/bar" noise hurts readability. So, I'm "meh" on this patch and wouldn't complain if it was dropped (though I don't insist upon it).

I dunno. Each of these subshells did not do much after going into its subdirectory, so repetition of "-C foo/bar" did not bother me that much.

FWIW I had the same reaction on that dash in "file1 updated".

Hi Eric,

The only reason is that I didn’t notice that test_commit accepts a <tag> argument, which defaults to <message> if unset. So when I changed the test to use test_commit and ran it I got errors from ‘git tag’ saying "file1 updated" is not a valid tag name, so I added a dash.

I’ll correct that in v2.

Thanks,
Philippe.

From: Philippe Blain <[email protected]>

When 'checkout --to' functionality was moved to 'worktree add', tests were adapted in f194b1ef6e (tests: worktree: retrofit "checkout --to" tests for "worktree add", 2015-07-06).

The calls were changed to 'worktree add' in this test (then t7410), but the test descriptions were not updated, keeping 'checkout' instead of using the new terminology (linked worktrees).

Clarify the tests by using the right terminology. While at it, remove some unnecessary leading directories such that all superproject worktrees are directly next to one another in the trash directory.

---
 t/t2405-worktree-submodule.sh | 36 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

index b9040c30d4..f1952c70dd 100755
@@ -18,43 +18,39 @@ test_expect_success 'setup: create origin repos'  '
 	git -C origin/main commit -m "sub updated"
 '
 
-test_expect_success 'setup: clone' '
-	mkdir clone &&
-	git -C clone clone --recursive "$base_path/origin/main"
+test_expect_success 'setup: clone superproject to create main worktree' '
+	git clone --recursive "$base_path/origin/main" main
 '
 
 rev1_hash_main=$(git --git-dir=origin/main/.git show --pretty=format:%h -q "HEAD~1")
 rev1_hash_sub=$(git --git-dir=origin/sub/.git show --pretty=format:%h -q "HEAD~1")
 
-test_expect_success 'checkout main' '
-	mkdir default_checkout &&
-	git -C clone/main worktree add "$base_path/default_checkout/main" "$rev1_hash_main"
+test_expect_success 'add superproject worktree' '
+	git -C main worktree add "$base_path/worktree" "$rev1_hash_main"
 '
 
-test_expect_failure 'can see submodule diffs just after checkout' '
-	git -C default_checkout/main diff --submodule master"^!" >out &&
+test_expect_failure 'submodule is checked out just after worktree add' '
+	git -C worktree diff --submodule master"^!" >out &&
 	grep "file1-updated" out
 '
 
-test_expect_success 'checkout main and initialize independent clones' '
-	mkdir fully_cloned_submodule &&
-	git -C clone/main worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main" &&
-	git -C fully_cloned_submodule/main submodule update
+test_expect_success 'add superproject worktree and initialize submodules' '
+	git -C main worktree add "$base_path/worktree-submodule-update" "$rev1_hash_main" &&
+	git -C worktree-submodule-update submodule update
 '
 
-test_expect_success 'can see submodule diffs after independent cloning' '
-	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
+test_expect_success 'submodule is checked out just after submodule update in linked worktree' '
+	git -C worktree-submodule-update diff --submodule master"^!" >out &&
 	grep "file1-updated" out
 '
 
-test_expect_success 'checkout sub manually' '
-	mkdir linked_submodule &&
-	git -C clone/main worktree add "$base_path/linked_submodule/main" "$rev1_hash_main" &&
-	git -C clone/main/sub worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub"
+test_expect_success 'add superproject worktree and manually add submodule worktree' '
+	git -C main worktree add "$base_path/linked_submodule" "$rev1_hash_main" &&
+	git -C main/sub worktree add "$base_path/linked_submodule/sub" "$rev1_hash_sub"
 '
 
-test_expect_success 'can see submodule diffs after manual checkout of linked submodule' '
-	git -C linked_submodule/main diff --submodule master"^!" >out &&
+test_expect_success 'submodule is checked out after manually adding submodule worktree' '
+	git -C linked_submodule diff --submodule master"^!" >out &&
 	grep "file1-updated" out
 '
 

gitgitgadget

The unanswered questions which popped into my head when reading the "While at it..." include:

Why is it desirable for the worktrees to live in this new location rather than their original locations?

Is it safe to relocate the worktrees like this without losing some important aspect of the test?

Why were the worktrees located as they were originally? Was there some hidden or not-so-obvious reason that we're overlooking? (I guess this is really the same as question #2.)

Hi Eric,

I thought it was desirable because the leading directories don’t serve any purpose apart from carrying information about what they are testing, which the worktree directory itself can do instead of all of them being called "main".

After analyzing the test to see what was being tested, making the change and making sure the test behaved the same way, I concluded that it was.

I don’t think there was a reason. The worktrees directories were structured that way since the test was introduced in df56607dff (git-common-dir: make "modules/" per-working-directory directory, 2014-11-30). Maybe one reason was for every superproject worktree to be in a directory called "main", just as every submodule is in a directory called "sub"…

I can explain more this reasoning in the commit message if necessary.

Philippe.

Yes, please.

From: Philippe Blain <[email protected]>

Ever since df56607dff (git-common-dir: make "modules/" per-working-directory directory, 2014-11-30), submodules in linked worktrees are cloned to $GIT_DIR/modules, i.e. $GIT_COMMON_DIR/worktrees/<name>/modules.

However, this convention was not followed when the worktree updater commands checkout, reset and read-tree learned to recurse into submodules. Specifically, submodule.c::submodule_move_head, introduced in 6e3c1595c6 (update submodules:
add submodule_move_head, 2017-03-14) and submodule.c::submodule_unset_core_worktree, (re)introduced in 898c2e65b7 (submodule: unset core.worktree if no working tree is present, 2018-12-14) use get_git_common_dir() instead of get_git_dir() to get the path of the submodule repository.

This means that, for example, 'git checkout --recurse-submodules <branch>' in a linked worktree will correctly checkout <branch>, detach the submodule's HEAD at the commit recorded in <branch> and update the submodule working tree, but the submodule HEAD that will be moved is the one in $GIT_COMMON_DIR/modules/<name>/, i.e. the submodule repository of the main superproject working tree.
It will also rewrite the gitfile in the submodule working tree of the linked worktree to point to $GIT_COMMON_DIR/modules/<name>/.
This leads to an incorrect (and confusing!) state in the submodule working tree of the main superproject worktree.

Additionnally, if switching to a commit where the submodule is not present, submodule_unset_core_worktree will be called and will incorrectly remove 'core.wortree' from the config file of the submodule in the main superproject worktree, $GIT_COMMON_DIR/modules/<name>/config.

Fix this by constructing the path to the submodule repository using get_git_dir() in both submodule_move_head and submodule_unset_core_worktree.

---
 submodule.c                   |  6 +++---
 t/t2405-worktree-submodule.sh | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

index 9da7181321..5d19ec48a6 100644
@@ -1811,7 +1811,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
 void submodule_unset_core_worktree(const struct submodule *sub)
 {
 	char *config_path = xstrfmt("%s/modules/%s/config",
-				    get_git_common_dir(), sub->name);
+				    get_git_dir(), sub->name);
 
 	if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
 		warning(_("Could not unset core.worktree setting in submodule '%s'"),

@@ -1914,7 +1914,7 @@ int submodule_move_head(const char *path,
 					ABSORB_GITDIR_RECURSE_SUBMODULES);
 		} else {
 			char *gitdir = xstrfmt("%s/modules/%s",
-				    get_git_common_dir(), sub->name);
+				    get_git_dir(), sub->name);
 			connect_work_tree_and_git_dir(path, gitdir, 0);
 			free(gitdir);
 

@@ -1924,7 +1924,7 @@ int submodule_move_head(const char *path,
 
 		if (old_head && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {
 			char *gitdir = xstrfmt("%s/modules/%s",
-				    get_git_common_dir(), sub->name);
+				    get_git_dir(), sub->name);
 			connect_work_tree_and_git_dir(path, gitdir, 1);
 			free(gitdir);
 		}

index f1952c70dd..eba17d9e35 100755
@@ -10,6 +10,7 @@ test_expect_success 'setup: create origin repos'  '
 	git init origin/sub &&
 	test_commit -C origin/sub file1 &&
 	git init origin/main &&
+	test_commit -C origin/main first &&
 	git -C origin/main submodule add ../sub &&
 	git -C origin/main commit -m "add sub" &&
 	test_commit -C origin/sub "file1-updated" file1 file1updated &&

@@ -54,4 +55,25 @@ test_expect_success 'submodule is checked out after manually adding submodule wo
 	grep "file1-updated" out
 '
 
+test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules in a linked worktree' '
+	git -C main worktree add "$base_path/checkout-recurse" --detach  &&
+	git -C checkout-recurse submodule update --init &&
+	cat checkout-recurse/sub/.git > expect-gitfile &&
+	git -C main/sub rev-parse HEAD > expect-head-main &&
+	git -C checkout-recurse checkout --recurse-submodules HEAD~1 &&
+	cat checkout-recurse/sub/.git > actual-gitfile &&
+	git -C main/sub rev-parse HEAD > actual-head-main &&
+	test_cmp expect-gitfile actual-gitfile &&
+	test_cmp expect-head-main actual-head-main
+'
+
+test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
+	git -C main/sub config --get core.worktree > expect &&
+	git -C checkout-recurse checkout --recurse-submodules first &&
+	test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree > linked-config &&
+	test_must_be_empty linked-config &&
+	test_might_fail git -C main/sub config --get core.worktree > actual &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

s/Additionnally/Additionally

Hi Stolee,

fixed.

fixed.

I expected that question and answered in the cover letter (since Gitgitgadet can’t (yet, I hope) do in-patch "timely commentaries", [1]), but here is my answer:
I used test_might_fail such that when the test is run on the current master, only the last test_cmp makes the test fail. If we want to be more strict I'll change that to :

index eba17d9e35..31d156cce7 100755

@@ -70,9 +70,9 @@

test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules
test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '

	git -C main/sub config --get core.worktree > expect &&
	git -C checkout-recurse checkout --recurse-submodules first &&

- test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree >linked-config &&
+ test_expect_code 1 git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree >linked-config &&

	test_must_be_empty linked-config &&

- test_might_fail git -C main/sub config --get core.worktree >actual &&
+ git -C main/sub config --get core.worktree >actual &&

test_cmp expect actual
‘
[1] https://github.com/gitgitgadget/gitgitgadget/issues/173

Good idea, thanks. I’ll harden the test in v2.

I’ll also add that.

Thanks for the review!
Philippe.
p.s. Sorry for the re-send, I forgot to CC the list.

These cleanups make sense to me.

Thanks,
-Stolee

Changes since v1:

* revert the addition of a dash in "file1 updated" by correctly using the 'tag' argument of test_commit
* improve the commit message for the 3rd patch, as suggested by Eric
* fix spacing when redirecting into 'expect' and 'actual'
* harden the tests by echoing expected values into expect, as suggested by Stolee (I did that in both tests)
* remove test_might_fail and use test_expect_code
* add the 'git log' test suggested by Stolee

v1: This series fixes the behaviour of git checkout/reset/read-tree --recurse-submodules when they are called in a linked worktree (created by git worktree add).

Although submodules are cloned in $GIT_COMMON_DIR/worktrees/<name>/modules upon issuing git submodule update in the linked worktree, any invocation of git checkout/reset/read-tree --recurse-submodules that changes the state of the submodule(s) will incorrectly operate on the repositories of the submodules in the main worktree, i.e. the ones at $GIT_COMMON_DIR/modules/.

The fourth patch fixes this behaviour by using get_git_dir() instead of git_common_dir() in submodule.c::submodule_move_head and submodule.c::submodule_unset_core_worktree to construct the path to the submodule repository.

The first 3 patches are clean-up patches on t7410-submodule-checkout-to.sh (renamed to t2405-worktree-submodule.sh) to bring it up to date.

Cc:Max Kirillov [email protected] [[email protected]] Brandon Williams [email protected] [[email protected]] Jonathan Tan [email protected] [[email protected]] Stefan Beller [email protected] [[email protected]] Nguyễn Thái Ngọc Duy [email protected] [[email protected]] Eric Sunshine [email protected] [[email protected]] Derrick Stolee [email protected] [[email protected]]

Philippe Blain (4):
t7410: rename to t2405-worktree-submodule.sh
t2405: use git -C and test_commit -C instead of subshells
t2405: clarify test descriptions and simplify test submodule.c: use get_git_dir() instead of get_git_common_dir()

 submodule.c                      |  6 +--
 t/t2405-worktree-submodule.sh    | 90 ++++++++++++++++++++++++++++++++
 t/t7410-submodule-checkout-to.sh | 77 ---------------------------
 3 files changed, 93 insertions(+), 80 deletions(-)
 create mode 100755 t/t2405-worktree-submodule.sh
 delete mode 100755 t/t7410-submodule-checkout-to.sh

base-commit: b4615e40a8125477e18490d868f7b65954372b43
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-523%2Fphil-blain%2Fcheckout-recurse-in-linked-worktree-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-523/phil-blain/checkout-recurse-in-linked-worktree-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/523

Range-diff vs v1:

 1:  ff33339690 = 1:  1a4eae1ef5 t7410: rename to t2405-worktree-submodule.sh
 2:  5060ce3d64 ! 2:  f06d2c4aa5 t2405: use git -C and test_commit -C instead of subshells
     @@ -41,7 +41,7 @@
      +	git init origin/main &&
      +	git -C origin/main submodule add ../sub &&
      +	git -C origin/main commit -m "add sub" &&
     -+	test_commit -C origin/sub "file1-updated" file1 file1updated &&
     ++	test_commit -C origin/sub "file1 updated" file1 file1updated file1updated &&
       	git -C origin/main/sub pull &&
      -	(
      -		cd origin/main &&
     @@ -53,30 +53,3 @@
       '
       
       test_expect_success 'setup: clone' '
     -@@
     - 
     - test_expect_failure 'can see submodule diffs just after checkout' '
     - 	git -C default_checkout/main diff --submodule master"^!" >out &&
     --	grep "file1 updated" out
     -+	grep "file1-updated" out
     - '
     - 
     - test_expect_success 'checkout main and initialize independent clones' '
     -@@
     - 
     - test_expect_success 'can see submodule diffs after independent cloning' '
     - 	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
     --	grep "file1 updated" out
     -+	grep "file1-updated" out
     - '
     - 
     - test_expect_success 'checkout sub manually' '
     -@@
     - 
     - test_expect_success 'can see submodule diffs after manual checkout of linked submodule' '
     - 	git -C linked_submodule/main diff --submodule master"^!" >out &&
     --	grep "file1 updated" out
     -+	grep "file1-updated" out
     - '
     - 
     - test_done

3: 6e0e664026 ! 3: 10727275a2 t2405: clarify test descriptions and simplify test
@@ -10,7 +10,13 @@
descriptions were not updated, keeping 'checkout' instead of using the new terminology (linked worktrees).

- Clarify the tests by using the right terminology. While at it, remove some unnecessary
+ Also, in the test each worktree is created in
+ $TRASH_DIRECTORY/<leading-directory>/main, where the name of <leading-directory>
+ carries some information about what behavior each test verifies. This directory
+ structure is not mandatory for the tests; the worktrees can live next to one
+ another in the trash directory.
+
+ Clarify the tests by using the right terminology, and remove the unnecessary
leading directories such that all superproject worktrees are directly next to one another in the trash directory.

@@ -44,7 +50,7 @@
- git -C default_checkout/main diff --submodule master"^!" >out &&
+test_expect_failure 'submodule is checked out just after worktree add' '
+ git -C worktree diff --submodule master"^!" >out &&
- grep "file1-updated" out
+ grep "file1 updated" out
'

-test_expect_success 'checkout main and initialize independent clones' '
@@ -60,7 +66,7 @@
- git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
+test_expect_success 'submodule is checked out just after submodule update in linked worktree' '
+ git -C worktree-submodule-update diff --submodule master"^!" >out &&
- grep "file1-updated" out
+ grep "file1 updated" out
'

-test_expect_success 'checkout sub manually' '
@@ -76,6 +82,6 @@
- git -C linked_submodule/main diff --submodule master"^!" >out &&
+test_expect_success 'submodule is checked out after manually adding submodule worktree' '
+ git -C linked_submodule diff --submodule master"^!" >out &&
- grep "file1-updated" out
+ grep "file1 updated" out
'

4: 72cdb2f95d ! 4: 614bccd31b submodule.c: use get_git_dir() instead of get_git_common_dir()

     @@ -24,7 +24,7 @@
          This leads to an incorrect (and confusing!) state in the submodule working tree
          of the main superproject worktree.
      
     -    Additionnally, if switching to a commit where the submodule is not present,
     +    Additionally, if switching to a commit where the submodule is not present,
          submodule_unset_core_worktree will be called and will incorrectly remove
          'core.wortree' from the config file of the submodule in the main superproject worktree,
          $GIT_COMMON_DIR/modules/<name>/config.
     @@ -75,30 +75,41 @@
      +	test_commit -C origin/main first &&
       	git -C origin/main submodule add ../sub &&
       	git -C origin/main commit -m "add sub" &&
     - 	test_commit -C origin/sub "file1-updated" file1 file1updated &&
     + 	test_commit -C origin/sub "file1 updated" file1 file1updated file1updated &&
      @@
     - 	grep "file1-updated" out
     + 	grep "file1 updated" out
       '
       
      +test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules in a linked worktree' '
      +	git -C main worktree add "$base_path/checkout-recurse" --detach  &&
      +	git -C checkout-recurse submodule update --init &&
     -+	cat checkout-recurse/sub/.git > expect-gitfile &&
     -+	git -C main/sub rev-parse HEAD > expect-head-main &&
     ++	echo "gitdir: ../../main/.git/worktrees/checkout-recurse/modules/sub" >expect-gitfile &&
     ++	cat checkout-recurse/sub/.git >actual-gitfile &&
     ++	test_cmp expect-gitfile actual-gitfile &&
     ++	git -C main/sub rev-parse HEAD >expect-head-main &&
      +	git -C checkout-recurse checkout --recurse-submodules HEAD~1 &&
     -+	cat checkout-recurse/sub/.git > actual-gitfile &&
     -+	git -C main/sub rev-parse HEAD > actual-head-main &&
     ++	cat checkout-recurse/sub/.git >actual-gitfile &&
     ++	git -C main/sub rev-parse HEAD >actual-head-main &&
      +	test_cmp expect-gitfile actual-gitfile &&
      +	test_cmp expect-head-main actual-head-main
      +'
      +
      +test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
     -+	git -C main/sub config --get core.worktree > expect &&
     ++	echo "../../../sub" >expect-main &&
     ++	git -C main/sub config --get core.worktree >actual-main &&
     ++	test_cmp expect-main actual-main &&
     ++	echo "../../../../../../checkout-recurse/sub" >expect-linked &&
     ++	git -C checkout-recurse/sub config --get core.worktree >actual-linked &&
     ++	test_cmp expect-linked actual-linked &&
      +	git -C checkout-recurse checkout --recurse-submodules first &&
     -+	test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree > linked-config &&
     ++	test_expect_code 1 git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree >linked-config &&
      +	test_must_be_empty linked-config &&
     -+	test_might_fail git -C main/sub config --get core.worktree > actual &&
     -+	test_cmp expect actual
     ++	git -C main/sub config --get core.worktree >actual-main &&
     ++	test_cmp expect-main actual-main
     ++'
     ++
     ++test_expect_success 'unsetting core.worktree does not prevent running commands directly against the submodule repository' '
     ++	git -C main/.git/worktrees/checkout-recurse/modules/sub log
      +'
      +
       test_done

gitgitgadget

From: Philippe Blain <[email protected]>

This test was added in df56607dff (git-common-dir: make "modules/" per-working-directory directory, 2014-11-30), back when the 'git worktree' command did not exist and 'git checkout --to' was used to create supplementary worktrees.

Since this file contains tests for the interaction of 'git worktree' with submodules, rename it to t2405-worktree-submodule.sh, following the naming scheme for tests checking the behavior of various commands with submodules.

---
 ...410-submodule-checkout-to.sh => t2405-worktree-submodule.sh} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename t/{t7410-submodule-checkout-to.sh => t2405-worktree-submodule.sh} (96%)

similarity index 96%
rename from t/t7410-submodule-checkout-to.sh
rename to t/t2405-worktree-submodule.sh
index f1b492ebc4..f2eee328cc 100755
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='Combination of submodules and multiple workdirs'
+test_description='Combination of submodules and multiple worktrees'
 
 . ./test-lib.sh
 

gitgitgadget

From: Philippe Blain <[email protected]>

The subshells used in the setup phase of this test are unnecessary.

Remove them by using 'git -C' and 'test_commit -C'.

---
 t/t2405-worktree-submodule.sh | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

index f2eee328cc..c4e555776a 100755
@@ -6,32 +6,16 @@ test_description='Combination of submodules and multiple worktrees'
 
 base_path=$(pwd -P)
 
-test_expect_success 'setup: make origin'  '
-	mkdir -p origin/sub &&
-	(
-		cd origin/sub && git init &&
-		echo file1 >file1 &&
-		git add file1 &&
-		git commit -m file1
-	) &&
-	mkdir -p origin/main &&
-	(
-		cd origin/main && git init &&
-		git submodule add ../sub &&
-		git commit -m "add sub"
-	) &&
-	(
-		cd origin/sub &&
-		echo file1updated >file1 &&
-		git add file1 &&
-		git commit -m "file1 updated"
-	) &&
+test_expect_success 'setup: create origin repos'  '
+	git init origin/sub &&
+	test_commit -C origin/sub file1 &&
+	git init origin/main &&
+	git -C origin/main submodule add ../sub &&
+	git -C origin/main commit -m "add sub" &&
+	test_commit -C origin/sub "file1 updated" file1 file1updated file1updated &&
 	git -C origin/main/sub pull &&
-	(
-		cd origin/main &&
-		git add sub &&
-		git commit -m "sub updated"
-	)
+	git -C origin/main add sub &&
+	git -C origin/main commit -m "sub updated"
 '
 
 test_expect_success 'setup: clone' '

gitgitgadget

From: Philippe Blain <[email protected]>

When 'checkout --to' functionality was moved to 'worktree add', tests were adapted in f194b1ef6e (tests: worktree: retrofit "checkout --to" tests for "worktree add", 2015-07-06).

The calls were changed to 'worktree add' in this test (then t7410), but the test descriptions were not updated, keeping 'checkout' instead of using the new terminology (linked worktrees).

Also, in the test each worktree is created in $TRASH_DIRECTORY/<leading-directory>/main, where the name of <leading-directory> carries some information about what behavior each test verifies. This directory structure is not mandatory for the tests; the worktrees can live next to one another in the trash directory.

Clarify the tests by using the right terminology, and remove the unnecessary leading directories such that all superproject worktrees are directly next to one another in the trash directory.

---
 t/t2405-worktree-submodule.sh | 36 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

index c4e555776a..d0830058fd 100755
@@ -18,43 +18,39 @@ test_expect_success 'setup: create origin repos'  '
 	git -C origin/main commit -m "sub updated"
 '
 
-test_expect_success 'setup: clone' '
-	mkdir clone &&
-	git -C clone clone --recursive "$base_path/origin/main"
+test_expect_success 'setup: clone superproject to create main worktree' '
+	git clone --recursive "$base_path/origin/main" main
 '
 
 rev1_hash_main=$(git --git-dir=origin/main/.git show --pretty=format:%h -q "HEAD~1")
 rev1_hash_sub=$(git --git-dir=origin/sub/.git show --pretty=format:%h -q "HEAD~1")
 
-test_expect_success 'checkout main' '
-	mkdir default_checkout &&
-	git -C clone/main worktree add "$base_path/default_checkout/main" "$rev1_hash_main"
+test_expect_success 'add superproject worktree' '
+	git -C main worktree add "$base_path/worktree" "$rev1_hash_main"
 '
 
-test_expect_failure 'can see submodule diffs just after checkout' '
-	git -C default_checkout/main diff --submodule master"^!" >out &&
+test_expect_failure 'submodule is checked out just after worktree add' '
+	git -C worktree diff --submodule master"^!" >out &&
 	grep "file1 updated" out
 '
 
-test_expect_success 'checkout main and initialize independent clones' '
-	mkdir fully_cloned_submodule &&
-	git -C clone/main worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main" &&
-	git -C fully_cloned_submodule/main submodule update
+test_expect_success 'add superproject worktree and initialize submodules' '
+	git -C main worktree add "$base_path/worktree-submodule-update" "$rev1_hash_main" &&
+	git -C worktree-submodule-update submodule update
 '
 
-test_expect_success 'can see submodule diffs after independent cloning' '
-	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
+test_expect_success 'submodule is checked out just after submodule update in linked worktree' '
+	git -C worktree-submodule-update diff --submodule master"^!" >out &&
 	grep "file1 updated" out
 '
 
-test_expect_success 'checkout sub manually' '
-	mkdir linked_submodule &&
-	git -C clone/main worktree add "$base_path/linked_submodule/main" "$rev1_hash_main" &&
-	git -C clone/main/sub worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub"
+test_expect_success 'add superproject worktree and manually add submodule worktree' '
+	git -C main worktree add "$base_path/linked_submodule" "$rev1_hash_main" &&
+	git -C main/sub worktree add "$base_path/linked_submodule/sub" "$rev1_hash_sub"
 '
 
-test_expect_success 'can see submodule diffs after manual checkout of linked submodule' '
-	git -C linked_submodule/main diff --submodule master"^!" >out &&
+test_expect_success 'submodule is checked out after manually adding submodule worktree' '
+	git -C linked_submodule diff --submodule master"^!" >out &&
 	grep "file1 updated" out
 '
 

gitgitgadget

From: Philippe Blain <[email protected]>

Ever since df56607dff (git-common-dir: make "modules/" per-working-directory directory, 2014-11-30), submodules in linked worktrees are cloned to $GIT_DIR/modules, i.e. $GIT_COMMON_DIR/worktrees/<name>/modules.

However, this convention was not followed when the worktree updater commands checkout, reset and read-tree learned to recurse into submodules. Specifically, submodule.c::submodule_move_head, introduced in 6e3c1595c6 (update submodules:
add submodule_move_head, 2017-03-14) and submodule.c::submodule_unset_core_worktree, (re)introduced in 898c2e65b7 (submodule: unset core.worktree if no working tree is present, 2018-12-14) use get_git_common_dir() instead of get_git_dir() to get the path of the submodule repository.

This means that, for example, 'git checkout --recurse-submodules <branch>' in a linked worktree will correctly checkout <branch>, detach the submodule's HEAD at the commit recorded in <branch> and update the submodule working tree, but the submodule HEAD that will be moved is the one in $GIT_COMMON_DIR/modules/<name>/, i.e. the submodule repository of the main superproject working tree.
It will also rewrite the gitfile in the submodule working tree of the linked worktree to point to $GIT_COMMON_DIR/modules/<name>/.
This leads to an incorrect (and confusing!) state in the submodule working tree of the main superproject worktree.

Additionally, if switching to a commit where the submodule is not present, submodule_unset_core_worktree will be called and will incorrectly remove 'core.wortree' from the config file of the submodule in the main superproject worktree, $GIT_COMMON_DIR/modules/<name>/config.

Fix this by constructing the path to the submodule repository using get_git_dir() in both submodule_move_head and submodule_unset_core_worktree.

---
 submodule.c                   |  6 +++---
 t/t2405-worktree-submodule.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

index 9da7181321..5d19ec48a6 100644
@@ -1811,7 +1811,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
 void submodule_unset_core_worktree(const struct submodule *sub)
 {
 	char *config_path = xstrfmt("%s/modules/%s/config",
-				    get_git_common_dir(), sub->name);
+				    get_git_dir(), sub->name);
 
 	if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
 		warning(_("Could not unset core.worktree setting in submodule '%s'"),

@@ -1914,7 +1914,7 @@ int submodule_move_head(const char *path,
 					ABSORB_GITDIR_RECURSE_SUBMODULES);
 		} else {
 			char *gitdir = xstrfmt("%s/modules/%s",
-				    get_git_common_dir(), sub->name);
+				    get_git_dir(), sub->name);
 			connect_work_tree_and_git_dir(path, gitdir, 0);
 			free(gitdir);
 

@@ -1924,7 +1924,7 @@ int submodule_move_head(const char *path,
 
 		if (old_head && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {
 			char *gitdir = xstrfmt("%s/modules/%s",
-				    get_git_common_dir(), sub->name);
+				    get_git_dir(), sub->name);
 			connect_work_tree_and_git_dir(path, gitdir, 1);
 			free(gitdir);
 		}

index d0830058fd..e1b2bfd87e 100755
@@ -10,6 +10,7 @@ test_expect_success 'setup: create origin repos'  '
 	git init origin/sub &&
 	test_commit -C origin/sub file1 &&
 	git init origin/main &&
+	test_commit -C origin/main first &&
 	git -C origin/main submodule add ../sub &&
 	git -C origin/main commit -m "add sub" &&
 	test_commit -C origin/sub "file1 updated" file1 file1updated file1updated &&

@@ -54,4 +55,36 @@ test_expect_success 'submodule is checked out after manually adding submodule wo
 	grep "file1 updated" out
 '
 
+test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules in a linked worktree' '
+	git -C main worktree add "$base_path/checkout-recurse" --detach  &&
+	git -C checkout-recurse submodule update --init &&
+	echo "gitdir: ../../main/.git/worktrees/checkout-recurse/modules/sub" >expect-gitfile &&
+	cat checkout-recurse/sub/.git >actual-gitfile &&
+	test_cmp expect-gitfile actual-gitfile &&
+	git -C main/sub rev-parse HEAD >expect-head-main &&
+	git -C checkout-recurse checkout --recurse-submodules HEAD~1 &&
+	cat checkout-recurse/sub/.git >actual-gitfile &&
+	git -C main/sub rev-parse HEAD >actual-head-main &&
+	test_cmp expect-gitfile actual-gitfile &&
+	test_cmp expect-head-main actual-head-main
+'
+
+test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
+	echo "../../../sub" >expect-main &&
+	git -C main/sub config --get core.worktree >actual-main &&
+	test_cmp expect-main actual-main &&
+	echo "../../../../../../checkout-recurse/sub" >expect-linked &&
+	git -C checkout-recurse/sub config --get core.worktree >actual-linked &&
+	test_cmp expect-linked actual-linked &&
+	git -C checkout-recurse checkout --recurse-submodules first &&
+	test_expect_code 1 git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree >linked-config &&
+	test_must_be_empty linked-config &&
+	git -C main/sub config --get core.worktree >actual-main &&
+	test_cmp expect-main actual-main
+'
+
+test_expect_success 'unsetting core.worktree does not prevent running commands directly against the submodule repository' '
+	git -C main/.git/worktrees/checkout-recurse/modules/sub log
+'
+
 test_done

gitgitgadget

Hello,

Here is v2 of my series. I realized that Gitgitgadget failed to parse the Cc: footer in my PR description and thus both v1 and v2 got sent to the list without any Cc:s.

Cheers,

Philippe.

I've read these four patches a few times and did not spot any remaining issues. I however is hardly an expert on (nor a big fan of) the "--recurse-submodule" feature, so a proper review by those who are more familiar with it is surely appreciated.

Thanks.

This is somewhat unreadable wall of names X-< Is it a funny rendering of what is originally in some mark-up format (perhaps HTML???)

Hi Junio,

Yes, Gitgitgadget unfortunately failed to convert this wall of text into a proper CC: list because there was no space between the "Cc:" and "Max". I’ll try to submit a PR for that in GGG.

Cheers,
Philippe.

For the record, https://github.com/gitgitgadget/gitgitgadget/pull/198 has been merged so this should not happen again.

That's good to know. As long as the resulting e-mail addresses are sensible (e.g. if Markdown markups are not cleansed enough and the duplicated addresses inside [] seep through, the result would still not be good), it is OK.

Thanks.