From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 4/4] submodule.c: use get_git_dir() instead of get_git_common_dir()
Date: Sat, 18 Jan 2020 16:09:47 -0500 [thread overview]
Message-ID: <D28196FE-67A1-4D03-8EFD-34ECC5633062@gmail.com> (raw)
In-Reply-To: <b97f7dc2-9cc9-9665-e00e-1cbb8b0996e4@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.
next prev parent reply other threads:[~2020-01-18 21:09 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-17 12:23 [PATCH 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree Philippe Blain via GitGitGadget
2020-01-17 12:23 ` [PATCH 1/4] t7410: rename to t2405-worktree-submodule.sh Philippe Blain via GitGitGadget
2020-01-17 12:23 ` [PATCH 2/4] t2405: use git -C and test_commit -C instead of subshells Philippe Blain via GitGitGadget
2020-01-17 13:45 ` Eric Sunshine
2020-01-17 20:32 ` Junio C Hamano
2020-01-18 19:50 ` Philippe Blain
2020-01-17 12:23 ` [PATCH 3/4] t2405: clarify test descriptions and simplify test Philippe Blain via GitGitGadget
2020-01-17 13:56 ` Eric Sunshine
2020-01-19 0:21 ` Philippe Blain
2020-01-19 1:41 ` Eric Sunshine
2020-01-17 12:23 ` [PATCH 4/4] submodule.c: use get_git_dir() instead of get_git_common_dir() Philippe Blain via GitGitGadget
2020-01-17 13:24 ` Derrick Stolee
2020-01-18 21:09 ` Philippe Blain [this message]
2020-01-17 13:24 ` [PATCH 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree Derrick Stolee
2020-01-21 15:01 ` [PATCH v2 " Philippe Blain via GitGitGadget
2020-01-21 15:01 ` [PATCH v2 1/4] t7410: rename to t2405-worktree-submodule.sh Philippe Blain via GitGitGadget
2020-01-21 15:01 ` [PATCH v2 2/4] t2405: use git -C and test_commit -C instead of subshells Philippe Blain via GitGitGadget
2020-01-21 15:01 ` [PATCH v2 3/4] t2405: clarify test descriptions and simplify test Philippe Blain via GitGitGadget
2020-01-21 15:01 ` [PATCH v2 4/4] submodule.c: use get_git_dir() instead of get_git_common_dir() Philippe Blain via GitGitGadget
2020-01-22 12:55 ` [PATCH v2 0/4] checkout/reset/read-tree: fix --recurse-submodules in linked worktree Philippe Blain
2020-01-22 22:10 ` Junio C Hamano
2020-01-22 22:25 ` Philippe Blain
2020-01-24 23:00 ` Philippe Blain
2020-01-24 23:47 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D28196FE-67A1-4D03-8EFD-34ECC5633062@gmail.com \
--to=levraiphilippeblain@gmail.com \
--cc=git@vger.kernel.org \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).