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

[Philippe Blain <levraiphilippeblain@gmail.com>]

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.