diff mbox series

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

Message ID 72cdb2f95d8c03c7b0324e8132e04e3a10248432.1579263809.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/4] t7410: rename to t2405-worktree-submodule.sh | expand

Commit Message

Linus Arver via GitGitGadget Jan. 17, 2020, 12:23 p.m. UTC 
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(-)

Comments

Derrick Stolee Jan. 17, 2020, 1:24 p.m. UTC | #1 
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
Philippe Blain Jan. 18, 2020, 9:09 p.m. UTC | #2 
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.
diff mbox series

Patch

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