Hi Stolee, > Le 17 janv. 2020 à 08:24, Derrick Stolee <stolee@xxxxxxxxx> 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.
