git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
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
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.


  reply index

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

  List information: http://vger.kernel.org/majordomo-info.html

* 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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git