diff mbox series

[v2,5/6] tests: remove duplicate .gitmodules path

Message ID 20230228185642.2357806-5-calvinwan@google.com (mailing list archive)
State New, archived
Headers show
Series add: block invalid submodules | expand

Commit Message

Calvin Wan Feb. 28, 2023, 6:56 p.m. UTC 
Swapping `git add <submodule>` to `git submodule add <submodule>`
in a previous patch created a .gitmodules file with multiple
submodules pointing to the same path in certain tests. Fix tests
so that they are run on the original added submodule rather than
a separate manually configured submodule.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 t/t4027-diff-submodule.sh |  41 ++++++--------
 t/t7508-status.sh         | 114 +++++++++++++++-----------------------
 2 files changed, 62 insertions(+), 93 deletions(-)

Comments

Junio C Hamano Feb. 28, 2023, 11:35 p.m. UTC | #1 
Calvin Wan <calvinwan@google.com> writes:

> Swapping `git add <submodule>` to `git submodule add <submodule>`
> in a previous patch created a .gitmodules file with multiple
> submodules pointing to the same path in certain tests. Fix tests
> so that they are run on the original added submodule rather than
> a separate manually configured submodule.

Doesn't "git submodule add" have a way to give a specific name other
than the default taken from the path?  If "git add sub" is converted
to "git submodule add --name subname ./sub", wouldn't these changes
become unnecessary?
Calvin Wan March 2, 2023, 11:09 p.m. UTC | #2 
On Tue, Feb 28, 2023 at 3:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Calvin Wan <calvinwan@google.com> writes:
>
> > Swapping `git add <submodule>` to `git submodule add <submodule>`
> > in a previous patch created a .gitmodules file with multiple
> > submodules pointing to the same path in certain tests. Fix tests
> > so that they are run on the original added submodule rather than
> > a separate manually configured submodule.
>
> Doesn't "git submodule add" have a way to give a specific name other
> than the default taken from the path?  If "git add sub" is converted
> to "git submodule add --name subname ./sub", wouldn't these changes
> become unnecessary?
>

If we converted to "git submodule add --name subname ./sub", we would
instead have a different set of problems. For example, instances of
git config --add -f .gitmodules submodule.subname.path sub
git config -f .gitmodules submodule.subname.path sub
and other similar lines would still need to be removed to prevent duplicate
paths. That, however, seems like a better alternative than my current
patch which replaces those removals with different ones.
Glen Choo March 7, 2023, 12:51 a.m. UTC | #3 
Calvin Wan <calvinwan@google.com> writes:

> Swapping `git add <submodule>` to `git submodule add <submodule>`
> in a previous patch created a .gitmodules file with multiple
> submodules pointing to the same path in certain tests. Fix tests
> so that they are run on the original added submodule rather than
> a separate manually configured submodule.

Without reading the diff, I thought that having multiple configured
submodules was a bug that needed to be fixed (I also wasn't sure which
previous patch was being referenced). But after reading the diff, this
seems much more like a test simplification.

I think this is might be better squashed with the previous patch (the
combined diff of both patches looks ok to me) with some extra
clarification, e.g.:

  In some of the tests, we modify .gitmodules to check whether the
  value of "submodule.subname.ignore" is respected. When we were using
  "git add", we also had to temporarily turn the gitlink into a
  submodule by setting "submodule.subname.path", but since "git
  submodule add" handles that for us, let's use the real submodule
  instead.

> @@ -194,27 +191,24 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
>  '
>  
>  test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
> -	git config --add -f .gitmodules submodule.subname.ignore all &&
> -	git config --add -f .gitmodules submodule.subname.path sub &&
> +	git config --add -f .gitmodules submodule.sub.ignore all &&
>  	git commit -m "Update .gitmodules" .gitmodules &&
>  	git diff HEAD >actual2 &&
>  	test_must_be_empty actual2 &&
> -	git config -f .gitmodules submodule.subname.ignore untracked &&
> +	git config -f .gitmodules submodule.sub.ignore untracked &&
>  	git commit -m "Update .gitmodules" .gitmodules &&
>  	git diff HEAD >actual3 &&
>  	test_must_be_empty actual3 &&
> -	git config -f .gitmodules submodule.subname.ignore dirty &&
> +	git config -f .gitmodules submodule.sub.ignore dirty &&
>  	git commit -m "Update .gitmodules" .gitmodules &&
>  	git diff HEAD >actual4 &&
>  	test_must_be_empty actual4 &&
> -	git config submodule.subname.ignore none &&
> -	git config submodule.subname.path sub &&
> +	git config submodule.sub.ignore none &&
>  	git diff HEAD >actual &&
>  	sed -e "1,/^@@/d" actual >actual.body &&
>  	expect_from_to >expect.body $subprev $subprev-dirty &&
>  	test_cmp expect.body actual.body &&
> -	git config --remove-section submodule.subname &&
> -	git config --remove-section -f .gitmodules submodule.subname &&
> +	git config --unset submodule.sub.ignore &&
>  	git reset --hard pristine-gitmodules
>  '

Not the fault of this patch, but I think that this diff would have been
easier to read if the latter part were moved into "test_when_finished",
and it would be clear that we are just undoing what we are setting up a
few lines earlier (instead of a whole test block earlier).
diff mbox series

Patch

diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 2ee9f18b38..ce335534b9 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -127,36 +127,33 @@  test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)
 	git config diff.ignoreSubmodules dirty &&
 	git diff HEAD >actual &&
 	test_must_be_empty actual &&
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sub &&
+	git config --add -f .gitmodules submodule.sub.ignore none &&
 	git commit -m "Update .gitmodules" .gitmodules &&
 	git diff HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subprev $subprev-dirty &&
 	test_cmp expect.body actual.body &&
-	git config -f .gitmodules submodule.subname.ignore all &&
-	git config -f .gitmodules submodule.subname.path sub &&
+	git config -f .gitmodules submodule.sub.ignore all &&
 	git commit -m "Update .gitmodules" .gitmodules &&
 	git diff HEAD >actual2 &&
 	test_must_be_empty actual2 &&
-	git config -f .gitmodules submodule.subname.ignore untracked &&
+	git config -f .gitmodules submodule.sub.ignore untracked &&
 	git commit -m "Update .gitmodules" .gitmodules &&
 	git diff HEAD >actual3 &&
 	sed -e "1,/^@@/d" actual3 >actual3.body &&
 	expect_from_to >expect.body $subprev $subprev-dirty &&
 	test_cmp expect.body actual3.body &&
-	git config -f .gitmodules submodule.subname.ignore dirty &&
+	git config -f .gitmodules submodule.sub.ignore dirty &&
 	git commit -m "Update .gitmodules" .gitmodules &&
 	git diff HEAD >actual4 &&
 	test_must_be_empty actual4 &&
-	git config submodule.subname.ignore none &&
-	git config submodule.subname.path sub &&
+	git config submodule.sub.ignore none &&
+	git config submodule.sub.path sub &&
 	git diff HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subprev $subprev-dirty &&
 	test_cmp expect.body actual.body &&
-	git config --remove-section submodule.subname &&
-	git config --remove-section -f .gitmodules submodule.subname &&
+	git config --unset submodule.sub.ignore &&
 	git config --unset diff.ignoreSubmodules &&
 	git reset --hard pristine-gitmodules
 '
@@ -194,27 +191,24 @@  test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
 '
 
 test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
-	git config --add -f .gitmodules submodule.subname.ignore all &&
-	git config --add -f .gitmodules submodule.subname.path sub &&
+	git config --add -f .gitmodules submodule.sub.ignore all &&
 	git commit -m "Update .gitmodules" .gitmodules &&
 	git diff HEAD >actual2 &&
 	test_must_be_empty actual2 &&
-	git config -f .gitmodules submodule.subname.ignore untracked &&
+	git config -f .gitmodules submodule.sub.ignore untracked &&
 	git commit -m "Update .gitmodules" .gitmodules &&
 	git diff HEAD >actual3 &&
 	test_must_be_empty actual3 &&
-	git config -f .gitmodules submodule.subname.ignore dirty &&
+	git config -f .gitmodules submodule.sub.ignore dirty &&
 	git commit -m "Update .gitmodules" .gitmodules &&
 	git diff HEAD >actual4 &&
 	test_must_be_empty actual4 &&
-	git config submodule.subname.ignore none &&
-	git config submodule.subname.path sub &&
+	git config submodule.sub.ignore none &&
 	git diff HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subprev $subprev-dirty &&
 	test_cmp expect.body actual.body &&
-	git config --remove-section submodule.subname &&
-	git config --remove-section -f .gitmodules submodule.subname &&
+	git config --unset submodule.sub.ignore &&
 	git reset --hard pristine-gitmodules
 '
 
@@ -236,22 +230,19 @@  test_expect_success 'git diff between submodule commits [.gitmodules]' '
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subtip $subprev &&
 	test_cmp expect.body actual.body &&
-	git config --add -f .gitmodules submodule.subname.ignore dirty &&
-	git config --add -f .gitmodules submodule.subname.path sub &&
+	git config --add -f .gitmodules submodule.sub.ignore dirty &&
 	git diff HEAD^..HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subtip $subprev &&
 	test_cmp expect.body actual.body &&
-	git config -f .gitmodules submodule.subname.ignore all &&
+	git config -f .gitmodules submodule.sub.ignore all &&
 	git diff HEAD^..HEAD >actual &&
 	test_must_be_empty actual &&
-	git config submodule.subname.ignore dirty &&
-	git config submodule.subname.path sub &&
+	git config submodule.sub.ignore dirty &&
 	git diff  HEAD^..HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subtip $subprev &&
-	git config --remove-section submodule.subname &&
-	git config --remove-section -f .gitmodules submodule.subname &&
+	git config --unset submodule.sub.ignore &&
 	git reset --hard pristine-gitmodules
 '
 
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 5808339997..3d934bfb86 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1154,8 +1154,7 @@  test_expect_success '.gitmodules ignore=untracked suppresses submodules with unt
 	test_config diff.ignoreSubmodules dirty &&
 	git status >output &&
 	test_cmp expect output &&
-	git config --add -f .gitmodules submodule.subname.ignore untracked &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore untracked &&
 	cat > expect-modified-gitmodules << EOF &&
 On branch main
 Your branch and '\''upstream'\'' have diverged,
@@ -1187,18 +1186,16 @@  Untracked files:
 EOF
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success '.git/config ignore=untracked suppresses submodules with untracked content' '
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore untracked &&
-	git config --add submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore none &&
+	git config --add submodule.sm.ignore untracked &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config --remove-section submodule.subname &&
-	git config --remove-section -f .gitmodules submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success '--ignore-submodules=dirty suppresses submodules with untracked content' '
@@ -1210,22 +1207,19 @@  test_expect_success '.gitmodules ignore=dirty suppresses submodules with untrack
 	test_config diff.ignoreSubmodules dirty &&
 	git status >output &&
 	! test -s actual &&
-	git config --add -f .gitmodules submodule.subname.ignore dirty &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore dirty &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success '.git/config ignore=dirty suppresses submodules with untracked content' '
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore dirty &&
-	git config --add submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore none &&
+	git config --add submodule.sm.ignore dirty &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config --remove-section submodule.subname &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success '--ignore-submodules=dirty suppresses submodules with modified content' '
@@ -1235,22 +1229,19 @@  test_expect_success '--ignore-submodules=dirty suppresses submodules with modifi
 '
 
 test_expect_success '.gitmodules ignore=dirty suppresses submodules with modified content' '
-	git config --add -f .gitmodules submodule.subname.ignore dirty &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore dirty &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success '.git/config ignore=dirty suppresses submodules with modified content' '
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore dirty &&
-	git config --add submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore none &&
+	git config --add submodule.sm.ignore dirty &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config --remove-section submodule.subname &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success "--ignore-submodules=untracked doesn't suppress submodules with modified content" '
@@ -1289,8 +1280,7 @@  EOF
 '
 
 test_expect_success ".gitmodules ignore=untracked doesn't suppress submodules with modified content" '
-	git config --add -f .gitmodules submodule.subname.ignore untracked &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore untracked &&
 	cat > expect-modified-gitmodules << EOF &&
 On branch main
 Your branch and '\''upstream'\'' have diverged,
@@ -1324,18 +1314,16 @@  Untracked files:
 EOF
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success ".git/config ignore=untracked doesn't suppress submodules with modified content" '
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore untracked &&
-	git config --add submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore none &&
+	git config --add submodule.sm.ignore untracked &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config --remove-section submodule.subname &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 head2=$(cd sm && git commit -q -m "2nd commit" foo && git rev-parse --short=7 --verify HEAD)
@@ -1415,22 +1403,19 @@  Untracked files:
 	untracked
 
 EOF
-	git config --add -f .gitmodules submodule.subname.ignore untracked &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore untracked &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success ".git/config ignore=untracked doesn't suppress submodule summary" '
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore untracked &&
-	git config --add submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore none &&
+	git config --add submodule.sm.ignore untracked &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config --remove-section submodule.subname &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success "--ignore-submodules=dirty doesn't suppress submodule summary" '
@@ -1438,22 +1423,19 @@  test_expect_success "--ignore-submodules=dirty doesn't suppress submodule summar
 	test_cmp expect output
 '
 test_expect_success ".gitmodules ignore=dirty doesn't suppress submodule summary" '
-	git config --add -f .gitmodules submodule.subname.ignore dirty &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore dirty &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success ".git/config ignore=dirty doesn't suppress submodule summary" '
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore dirty &&
-	git config --add submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore none &&
+	git config --add submodule.sm.ignore dirty &&
 	git status >output &&
 	test_cmp expect-modified-gitmodules output &&
-	git config --remove-section submodule.subname &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 cat > expect << EOF
@@ -1552,22 +1534,19 @@  Untracked files:
 	untracked
 
 EOF
-	git config --add -f .gitmodules submodule.subname.ignore all &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore all &&
 	git status > output &&
 	test_cmp expect output &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success '.git/config ignore=all suppresses unstaged submodule summary' '
-	git config --add -f .gitmodules submodule.subname.ignore none &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore all &&
-	git config --add submodule.subname.path sm &&
+	git config --add -f .gitmodules submodule.sm.ignore none &&
+	git config --add submodule.sm.ignore all &&
 	git status > output &&
 	test_cmp expect output &&
-	git config --remove-section submodule.subname &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success 'setup of test environment' '
@@ -1633,9 +1612,8 @@  test_expect_success 'Restore default test environment' '
 '
 
 test_expect_success 'git commit will commit a staged but ignored submodule' '
-	git config --add -f .gitmodules submodule.subname.ignore all &&
-	git config --add -f .gitmodules submodule.subname.path sm &&
-	git config --add submodule.subname.ignore all &&
+	git config --add -f .gitmodules submodule.sm.ignore all &&
+	git config --add submodule.sm.ignore all &&
 	git status -s --ignore-submodules=dirty >output &&
 	test_i18ngrep "^M. sm" output &&
 	GIT_EDITOR="echo hello >>\"\$1\"" &&
@@ -1676,8 +1654,8 @@  test_expect_success 'git commit -m will commit a staged but ignored submodule' '
 	git commit -uno -m message &&
 	git status -s --ignore-submodules=dirty >output &&
 	test_i18ngrep ! "^M. sm" output &&
-	git config --remove-section submodule.subname &&
-	git config -f .gitmodules  --remove-section submodule.subname
+	git config --unset submodule.sm.ignore &&
+	git config -f .gitmodules --unset submodule.sm.ignore
 '
 
 test_expect_success 'show stash info with "--show-stash"' '