[PATCH 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[PATCH 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Philippe Blain via GitGitGadget]

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 :

diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
index eba17d9e35..31d156cce7 100755
--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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 max@max630.net [max@max630.net] Brandon Williams 
bwilliams.eng@gmail.com [bwilliams.eng@gmail.com] Jonathan Tan 
jonathantanmy@google.com [jonathantanmy@google.com] Stefan Beller 
stefanbeller@gmail.com [stefanbeller@gmail.com] Nguyễn Thái Ngọc Duy 
pclouds@gmail.com [pclouds@gmail.com]

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

[PATCH 1/4] t7410: rename to t2405-worktree-submodule.sh

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 ...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%)

diff --git a/t/t7410-submodule-checkout-to.sh b/t/t2405-worktree-submodule.sh
similarity index 96%
rename from t/t7410-submodule-checkout-to.sh
rename to t/t2405-worktree-submodule.sh
index f1b492ebc4..f2eee328cc 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

[PATCH 2/4] t2405: use git -C and test_commit -C instead of subshells

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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

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

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 t/t2405-worktree-submodule.sh | 40 +++++++++++------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
index f2eee328cc..b9040c30d4 100755
--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

Re: [PATCH 2/4] t2405: use git -C and test_commit -C instead of subshells

[Eric Sunshine]

On Fri, Jan 17, 2020 at 7:24 AM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The subshells used in the setup phase of this test are unnecessary.
> Remove them by using 'git -C' and 'test_commit -C'.

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

> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
> @@ -6,32 +6,16 @@ test_description='Combination of submodules and multiple worktrees'
> -               git commit -m "file1 updated"
> +       git -C origin/main commit -m "add sub" &&
> +       test_commit -C origin/sub "file1-updated" file1 file1updated &&
> @@ -49,7 +33,7 @@ test_expect_success 'checkout main' '
> -       grep "file1 updated" out
> +       grep "file1-updated" out

Why this change? Is it because test_commit() mishandles the whitespace
in the commit message? If so, it might deserve mention in the commit
message of this patch. (Even better would be to fix test_commit(), if
that is the case.)

Re: [PATCH 2/4] t2405: use git -C and test_commit -C instead of subshells

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Jan 17, 2020 at 7:24 AM Philippe Blain via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> The subshells used in the setup phase of this test are unnecessary.
>> Remove them by using 'git -C' and 'test_commit -C'.
>
> 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.

>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>> diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
>> @@ -6,32 +6,16 @@ test_description='Combination of submodules and multiple worktrees'
>> -               git commit -m "file1 updated"
>> +       git -C origin/main commit -m "add sub" &&
>> +       test_commit -C origin/sub "file1-updated" file1 file1updated &&
>> @@ -49,7 +33,7 @@ test_expect_success 'checkout main' '
>> -       grep "file1 updated" out
>> +       grep "file1-updated" out
>
> Why this change? Is it because test_commit() mishandles the whitespace
> in the commit message? If so, it might deserve mention in the commit
> message of this patch. (Even better would be to fix test_commit(), if
> that is the case.)

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

Re: [PATCH 2/4] t2405: use git -C and test_commit -C instead of subshells

[Philippe Blain]

Hi Eric,

>> -       grep "file1 updated" out
>> +       grep "file1-updated" out
> 
> Why this change? Is it because test_commit() mishandles the whitespace
> in the commit message? If so, it might deserve mention in the commit
> message of this patch. (Even better would be to fix test_commit(), if
> that is the case.)

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.

[PATCH 3/4] t2405: clarify test descriptions and simplify test

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 t/t2405-worktree-submodule.sh | 36 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
index b9040c30d4..f1952c70dd 100755
--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

Re: [PATCH 3/4] t2405: clarify test descriptions and simplify test

[Eric Sunshine]

On Fri, Jan 17, 2020 at 7:25 AM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.

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

Re: [PATCH 3/4] t2405: clarify test descriptions and simplify test

[Philippe Blain]

Hi Eric,
> Le 17 janv. 2020 à 08:56, Eric Sunshine <sunshine@sunshineco.com> a écrit :
> 
>> While at it, remove some unnecessary
>> leading directories such that all superproject worktrees are directly next to one
>> another in the trash directory.
> 
> 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?
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". 
> 
>    Is it safe to relocate the worktrees like this without losing some
>    important aspect of the test?
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.
> 
>    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.)
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.

Re: [PATCH 3/4] t2405: clarify test descriptions and simplify test

[Eric Sunshine]

On Sat, Jan 18, 2020 at 7:21 PM Philippe Blain
<levraiphilippeblain@gmail.com> wrote:
> > Le 17 janv. 2020 à 08:56, Eric Sunshine <sunshine@sunshineco.com> a écrit :
> > The unanswered questions which popped into my head when reading the
> > "While at it..." include:
> > [...]
> I can explain more this reasoning in the commit message if necessary.

Yes, please.

[PATCH 4/4] submodule.c: use get_git_dir() instead of get_git_common_dir()

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 submodule.c                   |  6 +++---
 t/t2405-worktree-submodule.sh | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index 9da7181321..5d19ec48a6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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);
 		}
diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
index f1952c70dd..eba17d9e35 100755
--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

Re: [PATCH 4/4] submodule.c: use get_git_dir() instead of get_git_common_dir()

[Derrick Stolee]

On 1/17/2020 7:23 AM, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> 
> 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,

s/Additionnally/Additionally

> 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.
> 
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  submodule.c                   |  6 +++---
>  t/t2405-worktree-submodule.sh | 22 ++++++++++++++++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 9da7181321..5d19ec48a6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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);
>  		}
> diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
> index f1952c70dd..eba17d9e35 100755
> --- a/t/t2405-worktree-submodule.sh
> +++ b/t/t2405-worktree-submodule.sh
> @@ -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 &&

Here, and the rest of these tests, please drop the space between the ">" and
the output file: ">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 &&

Why test_might_fail here, and below? Because the config may not exist, which
would return an error code? Should we _expect_ that failure with test_must_fail?

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

This tests that core.wortkree didn't change throughout the process, but
could we instead confirm an exact value by echoing into "expect" and
comparing both config outputs against that value?

Perhaps it is worth checking the success of the command that was failing
in submodules that still had core.worktree=true before 898c2e6? For your
test, it would be:

	git -C main/.git/worktrees/checkout-recurse/modules/sub log

Thanks,
-Stolee

Re: [PATCH 4/4] submodule.c: use get_git_dir() instead of get_git_common_dir()

[Philippe Blain]

Hi Stolee,

> Le 17 janv. 2020 à 08:24, Derrick Stolee <stolee@gmail.com> a écrit :
> 
>> Additionnally, if switching to a commit where the submodule is not present,
> 
> s/Additionnally/Additionally
fixed.
> 
>> +	cat checkout-recurse/sub/.git > expect-gitfile &&
> 
> Here, and the rest of these tests, please drop the space between the ">" and
> the output file: ">expect-gitfile ».
fixed.
> 
>> +	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 &&
> 
> Why test_might_fail here, and below? Because the config may not exist, which
> would return an error code? Should we _expect_ that failure with test_must_fail?
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 :

diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh

index eba17d9e35..31d156cce7 100755

--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

> 
>> +	test_must_be_empty linked-config &&
>> +	test_might_fail git -C main/sub config --get core.worktree > actual &&
>> +	test_cmp expect actual
> 
> This tests that core.wortkree didn't change throughout the process, but
> could we instead confirm an exact value by echoing into "expect" and
> comparing both config outputs against that value?
Good idea, thanks. I’ll harden the test in v2.
> 
> Perhaps it is worth checking the success of the command that was failing
> in submodules that still had core.worktree=true before 898c2e6? For your
> test, it would be:
> 
> 	git -C main/.git/worktrees/checkout-recurse/modules/sub log
I’ll also add that.
> 
> Thanks,
> -Stolee
Thanks for the review!
Philippe.
p.s. Sorry for the re-send, I forgot to CC the list.

Re: [PATCH 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Derrick Stolee]

On 1/17/2020 7:23 AM, Philippe Blain via GitGitGadget wrote:
> 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.

These cleanups make sense to me.

Thanks,
-Stolee

[PATCH v2 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Philippe Blain via GitGitGadget]

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 max@max630.net [max@max630.net] Brandon Williams 
bwilliams.eng@gmail.com [bwilliams.eng@gmail.com] Jonathan Tan 
jonathantanmy@google.com [jonathantanmy@google.com] Stefan Beller 
stefanbeller@gmail.com [stefanbeller@gmail.com] Nguyễn Thái Ngọc Duy 
pclouds@gmail.com [pclouds@gmail.com] Eric Sunshine sunshine@sunshineco.com
[sunshine@sunshineco.com] Derrick Stolee stolee@gmail.com [stolee@gmail.com]

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

[PATCH v2 1/4] t7410: rename to t2405-worktree-submodule.sh

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 ...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%)

diff --git a/t/t7410-submodule-checkout-to.sh b/t/t2405-worktree-submodule.sh
similarity index 96%
rename from t/t7410-submodule-checkout-to.sh
rename to t/t2405-worktree-submodule.sh
index f1b492ebc4..f2eee328cc 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

[PATCH v2 2/4] t2405: use git -C and test_commit -C instead of subshells

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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

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

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 t/t2405-worktree-submodule.sh | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
index f2eee328cc..c4e555776a 100755
--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

[PATCH v2 3/4] t2405: clarify test descriptions and simplify test

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 t/t2405-worktree-submodule.sh | 36 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
index c4e555776a..d0830058fd 100755
--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

[PATCH v2 4/4] submodule.c: use get_git_dir() instead of get_git_common_dir()

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

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.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 submodule.c                   |  6 +++---
 t/t2405-worktree-submodule.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index 9da7181321..5d19ec48a6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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);
 		}
diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
index d0830058fd..e1b2bfd87e 100755
--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -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

Re: [PATCH v2 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Philippe Blain]

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.

> Le 21 janv. 2020 à 10:01, Philippe Blain via GitGitGadget <gitgitgadget@gmail.com> a écrit :
> 
> 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 max@max630.net [max@max630.net] Brandon Williams 
> bwilliams.eng@gmail.com [bwilliams.eng@gmail.com] Jonathan Tan 
> jonathantanmy@google.com [jonathantanmy@google.com] Stefan Beller 
> stefanbeller@gmail.com [stefanbeller@gmail.com] Nguyễn Thái Ngọc Duy 
> pclouds@gmail.com [pclouds@gmail.com] Eric Sunshine sunshine@sunshineco.com
> [sunshine@sunshineco.com] Derrick Stolee stolee@gmail.com [stolee@gmail.com]
> 
> 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

Re: [PATCH v2 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Junio C Hamano]

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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.

> Cc:Max Kirillov max@max630.net [max@max630.net] Brandon Williams 
> bwilliams.eng@gmail.com [bwilliams.eng@gmail.com] Jonathan Tan 
> jonathantanmy@google.com [jonathantanmy@google.com] Stefan Beller 
> stefanbeller@gmail.com [stefanbeller@gmail.com] Nguyễn Thái Ngọc Duy 
> pclouds@gmail.com [pclouds@gmail.com] Eric Sunshine sunshine@sunshineco.com
> [sunshine@sunshineco.com] Derrick Stolee stolee@gmail.com [stolee@gmail.com]

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

Re: [PATCH v2 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Philippe Blain]

Hi Junio, 

> Le 22 janv. 2020 à 17:10, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

>> Cc:Max Kirillov max@max630.net [max@max630.net] Brandon Williams 
>> bwilliams.eng@gmail.com [bwilliams.eng@gmail.com] Jonathan Tan 
>> jonathantanmy@google.com [jonathantanmy@google.com] Stefan Beller 
>> stefanbeller@gmail.com [stefanbeller@gmail.com] Nguyễn Thái Ngọc Duy 
>> pclouds@gmail.com [pclouds@gmail.com] Eric Sunshine sunshine@sunshineco.com
>> [sunshine@sunshineco.com] Derrick Stolee stolee@gmail.com [stolee@gmail.com]
> 
> This is somewhat unreadable wall of names X-< Is it a funny
> rendering of what is originally in some mark-up format (perhaps
> HTML???)

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. 

Re: [PATCH v2 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Philippe Blain]

> Le 22 janv. 2020 à 17:25, Philippe Blain <levraiphilippeblain@gmail.com> a écrit :
> 
> Hi Junio, 
> 
>> Le 22 janv. 2020 à 17:10, Junio C Hamano <gitster@pobox.com> a écrit :
>> 
>> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>>> Cc:Max Kirillov max@max630.net [max@max630.net] Brandon Williams 
>>> bwilliams.eng@gmail.com [bwilliams.eng@gmail.com] Jonathan Tan 
>>> jonathantanmy@google.com [jonathantanmy@google.com] Stefan Beller 
>>> stefanbeller@gmail.com [stefanbeller@gmail.com] Nguyễn Thái Ngọc Duy 
>>> pclouds@gmail.com [pclouds@gmail.com] Eric Sunshine sunshine@sunshineco.com
>>> [sunshine@sunshineco.com] Derrick Stolee stolee@gmail.com [stolee@gmail.com]
>> 
>> This is somewhat unreadable wall of names X-< Is it a funny
>> rendering of what is originally in some mark-up format (perhaps
>> HTML???)
> 
> 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. 
For the record, https://github.com/gitgitgadget/gitgitgadget/pull/198 has been merged so this should not happen again.

Re: [PATCH v2 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

>>>> Cc:Max Kirillov max@max630.net [max@max630.net] Brandon Williams 
>>>> bwilliams.eng@gmail.com [bwilliams.eng@gmail.com] Jonathan Tan 
>>>> jonathantanmy@google.com [jonathantanmy@google.com] Stefan Beller 
>>>> stefanbeller@gmail.com [stefanbeller@gmail.com] Nguyễn Thái Ngọc Duy 
>>>> pclouds@gmail.com [pclouds@gmail.com] Eric Sunshine sunshine@sunshineco.com
>>>> [sunshine@sunshineco.com] Derrick Stolee stolee@gmail.com [stolee@gmail.com]
>>> 
>>> This is somewhat unreadable wall of names X-< Is it a funny
>>> rendering of what is originally in some mark-up format (perhaps
>>> HTML???)
>> 
>> 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. 
> 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.