git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Philippe Blain <levraiphilippeblain@gmail.com>
Date
Jan 18, 2020, 21:09 UTC
Message-ID
<D28196FE-67A1-4D03-8EFD-34ECC5633062@gmail.com>
In-Reply-To
<b97f7dc2-9cc9-9665-e00e-1cbb8b0996e4@gmail.com>
Hi Stolee,
Show 5 quoted lines
> 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.
Show 5 quoted lines
> 
>> +	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.
Show 16 quoted lines
> 
>> +	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.
Previous: Derrick StoleeNext: Derrick Stolee
Message 13 of 24 in “checkout/reset/read-tree: fix --recurse-submodules in linked worktree”
  1. 0/4 checkout/reset/read-tree: fix --recurse-submodules in linked worktreePhilippe Blain via GitGitGadget, Jan 17, 2020
  2. 1/4 t7410: rename to t2405-worktree-submodule.shPhilippe Blain via GitGitGadget, Jan 17, 2020
  3. 2/4 t2405: use git -C and test_commit -C instead of subshellsPhilippe Blain via GitGitGadget, Jan 17, 2020
  4. Eric SunshineJan 17, 2020
  5. Junio C HamanoJan 17, 2020
  6. Philippe BlainJan 18, 2020
  7. 3/4 t2405: clarify test descriptions and simplify testPhilippe Blain via GitGitGadget, Jan 17, 2020
  8. Eric SunshineJan 17, 2020
  9. Philippe BlainJan 19, 2020
  10. Eric SunshineJan 19, 2020
  11. 4/4 submodule.c: use get_git_dir() instead of get_git_common_dir()Philippe Blain via GitGitGadget, Jan 17, 2020
  12. Derrick StoleeJan 17, 2020
  13. Philippe BlainJan 18, 2020
  14. Derrick StoleeJan 17, 2020
  15. 0/4 checkout/reset/read-tree: fix --recurse-submodules in linked worktreePhilippe Blain via GitGitGadget, Jan 21, 2020
  16. 1/4 t7410: rename to t2405-worktree-submodule.shPhilippe Blain via GitGitGadget, Jan 21, 2020
  17. 2/4 t2405: use git -C and test_commit -C instead of subshellsPhilippe Blain via GitGitGadget, Jan 21, 2020
  18. 3/4 t2405: clarify test descriptions and simplify testPhilippe Blain via GitGitGadget, Jan 21, 2020
  19. 4/4 submodule.c: use get_git_dir() instead of get_git_common_dir()Philippe Blain via GitGitGadget, Jan 21, 2020
  20. Philippe BlainJan 22, 2020
  21. Junio C HamanoJan 22, 2020
  22. Philippe BlainJan 22, 2020
  23. Philippe BlainJan 24, 2020
  24. Junio C HamanoJan 24, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.