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

gitgitgadget · 2020-01-17T13:26:08Z

On the Git mailing list, Derrick Stolee wrote (reply to this):

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

On the Git mailing list, Philippe Blain wrote (reply to this):

Hi Stolee, > Le 17 janv. 2020 Ã 08:24, Derrick Stolee <[email protected]> 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.

gitgitgadget · 2020-01-17T13:47:15Z

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Fri, Jan 17, 2020 at 7:24 AM Philippe Blain via GitGitGadget <[email protected]> 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 <[email protected]> > --- > 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.)

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <[email protected]> writes: > On Fri, Jan 17, 2020 at 7:24 AM Philippe Blain via GitGitGadget > <[email protected]> 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 <[email protected]> >> --- >> 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".

On the Git mailing list, Philippe Blain wrote (reply to this):

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.

gitgitgadget · 2020-01-17T13:59:13Z

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Fri, Jan 17, 2020 at 7:25 AM Philippe Blain via GitGitGadget <[email protected]> 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.)

On the Git mailing list, Philippe Blain wrote (reply to this):

Hi Eric, > Le 17 janv. 2020 Ã 08:56, Eric Sunshine <[email protected]> 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.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sat, Jan 18, 2020 at 7:21 PM Philippe Blain <[email protected]> wrote: > > Le 17 janv. 2020 à 08:56, Eric Sunshine <[email protected]> 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.

-Original file line number
+Diff line change
@@ -0,0 +1,90 @@
+    #!/bin/sh
+    test_description='Combination of submodules and multiple worktrees'
+    . ./test-lib.sh
+    base_path=$(pwd -P)
+    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 &&
+    	git -C origin/main/sub pull &&
+    	git -C origin/main add sub &&
+    	git -C origin/main commit -m "sub updated"
+    '
+    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 'add superproject worktree' '
+    	git -C main worktree add "$base_path/worktree" "$rev1_hash_main"
+    '
+    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 '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 '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 '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 'submodule is checked out after manually adding submodule worktree' '
+    	git -C linked_submodule diff --submodule master"^!" >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 &&
+    	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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Jan 17, 2020

Uh oh!

gitgitgadget bot Jan 18, 2020

Uh oh!

gitgitgadget bot Jan 17, 2020

Uh oh!

gitgitgadget bot Jan 17, 2020

Uh oh!

gitgitgadget bot Jan 18, 2020

Uh oh!

gitgitgadget bot Jan 17, 2020

Uh oh!

gitgitgadget bot Jan 19, 2020

Uh oh!

gitgitgadget bot Jan 19, 2020

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up @@
     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'"),
@@ Expand Down Expand Up / @@ -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);
@@ Expand All / @@ -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);
     		}
@@ Expand Down @@

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

Uh oh!

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

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!