[v2,1/6] t4041, t4060: modernize test style

Commit Message

Calvin Wan Feb. 28, 2023, 6:56 p.m. UTC

From: Josh Steadmon <[e-mail]>

In preparation for later changes, move setup code into test_expect
blocks. Smaller sections are moved into existing blocks, while larger
sections with a standalone purpose are given their own new blocks.

This makes sure that later changes that may break the test setup are
easier to diagnose, because errors will caught immediately rather than
in later unrelated test_expect blocks.

While at it, have tests clean up after themselves with
test_when_finished and fix other minor style issues.

Signed-off-by: Josh Steadmon <[e-mail]>
Signed-off-by: Calvin Wan <[e-mail]>
---
 t/t4041-diff-submodule-option.sh             |  79 ++++++++-------
 t/t4060-diff-submodule-option-diff-format.sh | 101 +++++++++----------
 2 files changed, 89 insertions(+), 91 deletions(-)

Comments

Glen Choo March 6, 2023, 7:32 p.m. UTC | #1

Calvin Wan <[e-mail]> writes:

> In preparation for later changes, move setup code into test_expect
> blocks. Smaller sections are moved into existing blocks, while larger
> sections with a standalone purpose are given their own new blocks.

The changes where we moved lines outside of blocks into blocks without
changing them look good to me.

> While at it, have tests clean up after themselves with
> test_when_finished

I believe this came about as part of the discussion in

  https://lore.kernel.org/git/xmqqedqtbbf4.fsf@gitster.g

I think it's good to have tests clean up after themselves, but I'm not
sure if that's what we're doing in all of these cases, see below.

I'm leaving the diff header in place, since the two files have very
confusingly similar tests.

> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
>  test_expect_success 'typechanged submodule(submodule->blob)' '
> +	test_when_finished rm -rf sm1 &&
>  	git diff --submodule=log >actual &&
>  	cat >expected <<-EOF &&
>  	diff --git a/sm1 b/sm1

This hunk and the next...

> @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' '
>  	test_cmp expected actual
>  '
>  
> -rm -rf sm1 &&
> -git checkout-index sm1
>  test_expect_success 'typechanged submodule(submodule->blob)' '
> +	test_when_finished rm -f sm1 &&
> +	git checkout-index sm1 &&
>  	git diff-index -p --submodule=log HEAD >actual &&
>  	cat >expected <<-EOF &&
>  	Submodule sm1 $head4...0000000 (submodule deleted)

were changed so that the "rm -rf" happens in the clean up phase of the
earlier test (test 14) instead of set up phase of the later test (test
15). But, the "rm -rf" actually results in a _different_ state from
before 14, so it isn't actually cleaning up, it really is preparation
for 15's git checkout-index.

You can observe this by running

  ./t4041-diff-submodule-option.sh --run=1-13,15

which fails as expected. On the other hand, it passes if we move the "rm
-rf" into test 15.

Nearly all of the other test_when_finished here have the same problem,
where they 'clean up' state that wasn't changed in the same test body. I
believe they will show similar dependency issues, though I didn't go
through and test them all.

> @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' '
>  	diff_cmp expected actual
>  '
>  
> -rm -rf sm1
>  test_expect_success 'deleted submodule' '
>  	git diff-index -p --submodule=diff HEAD >actual &&
>  	cat >expected <<-EOF &&

This one is fairly obvious, since the test says 'deleted submodule', but
we no longer delete the submodule in the setup.

> @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' '
>  	diff_cmp expected actual
>  '
>  
> -mv sm2 sm2-bak
> -
>  test_expect_success 'deleted submodule with .git file' '
> +	mv sm2 sm2-bak &&
>  	git diff-index -p --submodule=diff HEAD >actual &&
>  	cat >expected <<-EOF &&
>  	Submodule sm1 $head7...0000000 (submodule deleted)
> @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' '
>  	diff_cmp expected actual
>  '
>  
> -echo submodule-to-blob>sm2
> -
>  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
> +	test_when_finished "rm sm2 && mv sm2-bak sm2" &&
> +	echo submodule-to-blob>sm2 &&
>  	git diff-index -p --submodule=diff HEAD >actual &&
>  	cat >expected <<-EOF &&
>  	Submodule sm1 $head7...0000000 (submodule deleted)
> @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
>  	diff_cmp expected actual
>  '
>  
> -rm sm2
> -mv sm2-bak sm2

This is the original case that Junio flagged, which I think is an almost
correct use of test_when_finished, since we do get back to an earlier
state before this string of tests, but not to the state before the
actual test with the test_when_finished.

If we want to use test_when_finished here (which I think we do), we
should add another test_when_finished to remove the dependency between
the two tests. like so:

  test_expect_success 'deleted submodule with .git file' '
  +	test_when_finished "mv sm2-bak sm2" &&
  	mv sm2 sm2-bak &&
    git diff-index -p --submodule=diff HEAD >actual &&

...

 test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
	test_when_finished "rm sm2 && mv sm2-bak sm2" &&
+ mv sm2 sm2-bak &&

Currently, they're still dependent because one creates sm2-bak and the
other moves it back, but if we have each test restore sm2, there will be
no more dependency.

Calvin Wan March 6, 2023, 8:40 p.m. UTC | #2

On Mon, Mar 6, 2023 at 11:32 AM Glen Choo <[e-mail]> wrote:
>
> Calvin Wan <[e-mail]> writes:
>
> > In preparation for later changes, move setup code into test_expect
> > blocks. Smaller sections are moved into existing blocks, while larger
> > sections with a standalone purpose are given their own new blocks.
>
> The changes where we moved lines outside of blocks into blocks without
> changing them look good to me.
>
> > While at it, have tests clean up after themselves with
> > test_when_finished
>
> I believe this came about as part of the discussion in
>
>   https://lore.kernel.org/git/xmqqedqtbbf4.fsf@gitster.g
>
> I think it's good to have tests clean up after themselves, but I'm not
> sure if that's what we're doing in all of these cases, see below.
>
> I'm leaving the diff header in place, since the two files have very
> confusingly similar tests.
>
> > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> >  test_expect_success 'typechanged submodule(submodule->blob)' '
> > +     test_when_finished rm -rf sm1 &&
> >       git diff --submodule=log >actual &&
> >       cat >expected <<-EOF &&
> >       diff --git a/sm1 b/sm1
>
> This hunk and the next...
>
> > @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' '
> >       test_cmp expected actual
> >  '
> >
> > -rm -rf sm1 &&
> > -git checkout-index sm1
> >  test_expect_success 'typechanged submodule(submodule->blob)' '
> > +     test_when_finished rm -f sm1 &&
> > +     git checkout-index sm1 &&
> >       git diff-index -p --submodule=log HEAD >actual &&
> >       cat >expected <<-EOF &&
> >       Submodule sm1 $head4...0000000 (submodule deleted)
>
> were changed so that the "rm -rf" happens in the clean up phase of the
> earlier test (test 14) instead of set up phase of the later test (test
> 15). But, the "rm -rf" actually results in a _different_ state from
> before 14, so it isn't actually cleaning up, it really is preparation
> for 15's git checkout-index.
>
> You can observe this by running
>
>   ./t4041-diff-submodule-option.sh --run=1-13,15
>
> which fails as expected. On the other hand, it passes if we move the "rm
> -rf" into test 15.
>
> Nearly all of the other test_when_finished here have the same problem,
> where they 'clean up' state that wasn't changed in the same test body. I
> believe they will show similar dependency issues, though I didn't go
> through and test them all.

Good catch. I'll go thru the rest of them and remove the dependency
issues.

>
> > @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' '
> >       diff_cmp expected actual
> >  '
> >
> > -rm -rf sm1
> >  test_expect_success 'deleted submodule' '
> >       git diff-index -p --submodule=diff HEAD >actual &&
> >       cat >expected <<-EOF &&
>
> This one is fairly obvious, since the test says 'deleted submodule', but
> we no longer delete the submodule in the setup.
>
> > @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' '
> >       diff_cmp expected actual
> >  '
> >
> > -mv sm2 sm2-bak
> > -
> >  test_expect_success 'deleted submodule with .git file' '
> > +     mv sm2 sm2-bak &&
> >       git diff-index -p --submodule=diff HEAD >actual &&
> >       cat >expected <<-EOF &&
> >       Submodule sm1 $head7...0000000 (submodule deleted)
> > @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' '
> >       diff_cmp expected actual
> >  '
> >
> > -echo submodule-to-blob>sm2
> > -
> >  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
> > +     test_when_finished "rm sm2 && mv sm2-bak sm2" &&
> > +     echo submodule-to-blob>sm2 &&
> >       git diff-index -p --submodule=diff HEAD >actual &&
> >       cat >expected <<-EOF &&
> >       Submodule sm1 $head7...0000000 (submodule deleted)
> > @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
> >       diff_cmp expected actual
> >  '
> >
> > -rm sm2
> > -mv sm2-bak sm2
>
> This is the original case that Junio flagged, which I think is an almost
> correct use of test_when_finished, since we do get back to an earlier
> state before this string of tests, but not to the state before the
> actual test with the test_when_finished.
>
> If we want to use test_when_finished here (which I think we do), we
> should add another test_when_finished to remove the dependency between
> the two tests. like so:
>
>   test_expect_success 'deleted submodule with .git file' '
>   +     test_when_finished "mv sm2-bak sm2" &&
>         mv sm2 sm2-bak &&
>     git diff-index -p --submodule=diff HEAD >actual &&
>
> ...
>
>  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
>         test_when_finished "rm sm2 && mv sm2-bak sm2" &&
> + mv sm2 sm2-bak &&
>
> Currently, they're still dependent because one creates sm2-bak and the
> other moves it back, but if we have each test restore sm2, there will be
> no more dependency.
>

That all makes sense. Thanks for the recommendations

Patch

diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 0c1502d4b0..2aa12243bd 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -42,11 +42,12 @@  commit_file () {
 	git commit "$@" -m "Commit $*" >/dev/null
 }
 
-test_create_repo sm1 &&
-add_file . foo >/dev/null
-
-head1=$(add_file sm1 foo1 foo2)
-fullhead1=$(cd sm1; git rev-parse --verify HEAD)
+test_expect_success 'setup' '
+	test_create_repo sm1 &&
+	add_file . foo &&
+	head1=$(add_file sm1 foo1 foo2) &&
+	fullhead1=$(git -C sm1 rev-parse --verify HEAD)
+'
 
 test_expect_success 'added submodule' '
 	git add sm1 &&
@@ -99,10 +100,9 @@  test_expect_success 'diff.submodule does not affect plumbing' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
-head2=$(add_file sm1 foo3)
-
 test_expect_success 'modified submodule(forward)' '
+	commit_file sm1 &&
+	head2=$(add_file sm1 foo3) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head1..$head2:
@@ -129,8 +129,8 @@  test_expect_success 'modified submodule(forward) --submodule' '
 	test_cmp expected actual
 '
 
-fullhead2=$(cd sm1; git rev-parse --verify HEAD)
 test_expect_success 'modified submodule(forward) --submodule=short' '
+	fullhead2=$(git -C sm1 rev-parse --verify HEAD) &&
 	git diff --submodule=short >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -144,14 +144,14 @@  test_expect_success 'modified submodule(forward) --submodule=short' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
-head3=$(
-	cd sm1 &&
-	git reset --hard HEAD~2 >/dev/null &&
-	git rev-parse --short --verify HEAD
-)
 
 test_expect_success 'modified submodule(backward)' '
+	commit_file sm1 &&
+	head3=$(
+		cd sm1 &&
+		git reset --hard HEAD~2 >/dev/null &&
+		git rev-parse --short --verify HEAD
+	) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2..$head3 (rewind):
@@ -161,8 +161,8 @@  test_expect_success 'modified submodule(backward)' '
 	test_cmp expected actual
 '
 
-head4=$(add_file sm1 foo4 foo5)
 test_expect_success 'modified submodule(backward and forward)' '
+	head4=$(add_file sm1 foo4 foo5) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2...$head4:
@@ -174,13 +174,15 @@  test_expect_success 'modified submodule(backward and forward)' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
-mv sm1 sm1-bak &&
-echo sm1 >sm1 &&
-head5=$(git hash-object sm1 | cut -c1-7) &&
-git add sm1 &&
-rm -f sm1 &&
-mv sm1-bak sm1
+test_expect_success 'setup - change sm1 to a blob' '
+	commit_file sm1 &&
+	mv sm1 sm1-bak &&
+	echo sm1 >sm1 &&
+	head5=$(git hash-object sm1 | cut -c1-7) &&
+	git add sm1 &&
+	rm -f sm1 &&
+	mv sm1-bak sm1
+'
 
 test_expect_success 'typechanged submodule(submodule->blob), --cached' '
 	git diff --submodule=log --cached >actual &&
@@ -198,6 +200,7 @@  test_expect_success 'typechanged submodule(submodule->blob), --cached' '
 '
 
 test_expect_success 'typechanged submodule(submodule->blob)' '
+	test_when_finished rm -rf sm1 &&
 	git diff --submodule=log >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -212,9 +215,9 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	test_cmp expected actual
 '
 
-rm -rf sm1 &&
-git checkout-index sm1
 test_expect_success 'typechanged submodule(submodule->blob)' '
+	test_when_finished rm -f sm1 &&
+	git checkout-index sm1 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...0000000 (submodule deleted)
@@ -229,11 +232,10 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	test_cmp expected actual
 '
 
-rm -f sm1 &&
-test_create_repo sm1 &&
-head6=$(add_file sm1 foo6 foo7)
-fullhead6=$(cd sm1; git rev-parse --verify HEAD)
 test_expect_success 'nonexistent commit' '
+	test_create_repo sm1 &&
+	head6=$(add_file sm1 foo6 foo7) &&
+	fullhead6=$(git -C sm1 rev-parse --verify HEAD) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...$head6 (commits not present)
@@ -241,8 +243,8 @@  test_expect_success 'nonexistent commit' '
 	test_cmp expected actual
 '
 
-commit_file
 test_expect_success 'typechanged submodule(blob->submodule)' '
+	commit_file &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -257,8 +259,8 @@  test_expect_success 'typechanged submodule(blob->submodule)' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
 test_expect_success 'submodule is up to date' '
+	commit_file sm1 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	test_must_be_empty actual
 '
@@ -313,13 +315,13 @@  test_expect_success 'submodule contains untracked and modified content (dirty ig
 '
 
 test_expect_success 'submodule contains untracked and modified content (all ignored)' '
+	test_when_finished rm -f sm1/new-file &&
 	echo new > sm1/foo6 &&
 	git diff-index -p --ignore-submodules --submodule=log HEAD >actual &&
 	test_must_be_empty actual
 '
 
 test_expect_success 'submodule contains modified content' '
-	rm -f sm1/new-file &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
@@ -327,9 +329,9 @@  test_expect_success 'submodule contains modified content' '
 	test_cmp expected actual
 '
 
-(cd sm1; git commit -mchange foo6 >/dev/null) &&
-head8=$(cd sm1; git rev-parse --short --verify HEAD) &&
 test_expect_success 'submodule is modified' '
+	(cd sm1 && git commit -mchange foo6 >/dev/null) &&
+	head8=$(cd sm1 && git rev-parse --short --verify HEAD) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head6..$head8:
@@ -406,13 +408,14 @@  test_expect_success 'modified submodule contains untracked and modified content
 '
 
 test_expect_success 'modified submodule contains untracked and modified content (all ignored)' '
+	test_when_finished rm -f sm1/new-file &&
 	echo modification >> sm1/foo6 &&
 	git diff-index -p --ignore-submodules --submodule=log HEAD >actual &&
 	test_must_be_empty actual
 '
 
 test_expect_success 'modified submodule contains modified content' '
-	rm -f sm1/new-file &&
+	test_when_finished rm -rf sm1 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
@@ -422,7 +425,6 @@  test_expect_success 'modified submodule contains modified content' '
 	test_cmp expected actual
 '
 
-rm -rf sm1
 test_expect_success 'deleted submodule' '
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
@@ -454,8 +456,8 @@  test_expect_success 'path filter' '
 	test_cmp expected actual
 '
 
-commit_file sm2
 test_expect_success 'given commit' '
+	commit_file sm2 &&
 	git diff-index -p --submodule=log HEAD^ >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head6...0000000 (submodule deleted)
@@ -473,9 +475,8 @@  test_expect_success 'given commit --submodule' '
 	test_cmp expected actual
 '
 
-fullhead7=$(cd sm2; git rev-parse --verify HEAD)
-
 test_expect_success 'given commit --submodule=short' '
+	fullhead7=$(git -C sm2 rev-parse --verify HEAD) &&
 	git diff-index -p --submodule=short HEAD^ >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
index 97c6424cd5..a760ed5eb6 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -138,10 +138,9 @@  test_expect_success 'diff.submodule does not affect plumbing' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
-head2=$(add_file sm1 foo3)
-
 test_expect_success 'modified submodule(forward)' '
+	commit_file sm1 &&
+	head2=$(add_file sm1 foo3) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head1..$head2:
@@ -180,8 +179,8 @@  test_expect_success 'modified submodule(forward) --submodule' '
 	diff_cmp expected actual
 '
 
-fullhead2=$(cd sm1; git rev-parse --verify HEAD)
 test_expect_success 'modified submodule(forward) --submodule=short' '
+	fullhead2=$(git -C sm1 rev-parse --verify HEAD) &&
 	git diff --submodule=short >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -195,14 +194,12 @@  test_expect_success 'modified submodule(forward) --submodule=short' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
-head3=$(
-	cd sm1 &&
-	git reset --hard HEAD~2 >/dev/null &&
-	git rev-parse --short --verify HEAD
-)
-
 test_expect_success 'modified submodule(backward)' '
+	commit_file sm1 &&
+	head3=$(
+		git -C sm1 reset --hard HEAD~2 >/dev/null &&
+		git -C sm1 rev-parse --short --verify HEAD
+	) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2..$head3 (rewind):
@@ -224,8 +221,8 @@  test_expect_success 'modified submodule(backward)' '
 	diff_cmp expected actual
 '
 
-head4=$(add_file sm1 foo4 foo5)
 test_expect_success 'modified submodule(backward and forward)' '
+	head4=$(add_file sm1 foo4 foo5) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2...$head4:
@@ -261,13 +258,15 @@  test_expect_success 'modified submodule(backward and forward)' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
-mv sm1 sm1-bak &&
-echo sm1 >sm1 &&
-head5=$(git hash-object sm1 | cut -c1-7) &&
-git add sm1 &&
-rm -f sm1 &&
-mv sm1-bak sm1
+test_expect_success 'setup - change sm1 to a blob' '
+	commit_file sm1 &&
+	mv sm1 sm1-bak &&
+	echo sm1 >sm1 &&
+	head5=$(git hash-object sm1 | cut -c1-7) &&
+	git add sm1 &&
+	rm -f sm1 &&
+	mv sm1-bak sm1
+'
 
 test_expect_success 'typechanged submodule(submodule->blob), --cached' '
 	git diff --submodule=diff --cached >actual &&
@@ -306,6 +305,7 @@  test_expect_success 'typechanged submodule(submodule->blob), --cached' '
 '
 
 test_expect_success 'typechanged submodule(submodule->blob)' '
+	test_when_finished rm -rf sm1 &&
 	git diff --submodule=diff >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -341,9 +341,9 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	diff_cmp expected actual
 '
 
-rm -rf sm1 &&
-git checkout-index sm1
 test_expect_success 'typechanged submodule(submodule->blob)' '
+	test_when_finished rm -f sm1 &&
+	git checkout-index sm1 &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...0000000 (submodule deleted)
@@ -358,10 +358,9 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	diff_cmp expected actual
 '
 
-rm -f sm1 &&
-test_create_repo sm1 &&
-head6=$(add_file sm1 foo6 foo7)
 test_expect_success 'nonexistent commit' '
+	test_create_repo sm1 &&
+	head6=$(add_file sm1 foo6 foo7) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...$head6 (commits not present)
@@ -369,8 +368,8 @@  test_expect_success 'nonexistent commit' '
 	diff_cmp expected actual
 '
 
-commit_file
 test_expect_success 'typechanged submodule(blob->submodule)' '
+	commit_file &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -399,8 +398,8 @@  test_expect_success 'typechanged submodule(blob->submodule)' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
 test_expect_success 'submodule is up to date' '
+	commit_file sm1 &&
 	head7=$(git -C sm1 rev-parse --short --verify HEAD) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	test_must_be_empty actual
@@ -471,13 +470,13 @@  test_expect_success 'submodule contains untracked and modified content (dirty ig
 '
 
 test_expect_success 'submodule contains untracked and modified content (all ignored)' '
+	test_when_finished rm -f sm1/new-file &&
 	echo new > sm1/foo6 &&
 	git diff-index -p --ignore-submodules --submodule=diff HEAD >actual &&
 	test_must_be_empty actual
 '
 
 test_expect_success 'submodule contains modified content' '
-	rm -f sm1/new-file &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
@@ -492,9 +491,9 @@  test_expect_success 'submodule contains modified content' '
 	diff_cmp expected actual
 '
 
-(cd sm1; git commit -mchange foo6 >/dev/null) &&
-head8=$(cd sm1; git rev-parse --short --verify HEAD) &&
 test_expect_success 'submodule is modified' '
+	(git -C sm1 commit -mchange foo6 >/dev/null) &&
+	head8=$(git -C sm1 rev-parse --short --verify HEAD) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7..$head8:
@@ -616,6 +615,7 @@  test_expect_success 'modified submodule contains untracked and modified content
 '
 
 test_expect_success 'modified submodule contains untracked and modified content (all ignored)' '
+	test_when_finished rm -f sm1/new-file &&
 	echo modification >> sm1/foo6 &&
 	git diff-index -p --ignore-submodules --submodule=diff HEAD >actual &&
 	test_must_be_empty actual
@@ -623,7 +623,7 @@  test_expect_success 'modified submodule contains untracked and modified content
 
 # NOT OK
 test_expect_success 'modified submodule contains modified content' '
-	rm -f sm1/new-file &&
+	test_when_finished rm -rf sm1 &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
@@ -643,7 +643,6 @@  test_expect_success 'modified submodule contains modified content' '
 	diff_cmp expected actual
 '
 
-rm -rf sm1
 test_expect_success 'deleted submodule' '
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
@@ -703,13 +702,15 @@  test_expect_success 'path filter' '
 	diff_cmp expected actual
 '
 
-cat >.gitmodules <<-EOF
-[submodule "sm2"]
-	path = sm2
-	url = bogus_url
-EOF
-git add .gitmodules
-commit_file sm2 .gitmodules
+test_expect_success 'setup - construct .gitmodules' '
+	cat >.gitmodules <<-EOF &&
+	[submodule "sm2"]
+		path = sm2
+		url = bogus_url
+	EOF
+	git add .gitmodules &&
+	commit_file sm2 .gitmodules
+'
 
 test_expect_success 'given commit' '
 	git diff-index -p --submodule=diff HEAD^ >actual &&
@@ -779,9 +780,8 @@  test_expect_success 'diff --submodule=diff with .git file' '
 	diff_cmp expected actual
 '
 
-mv sm2 sm2-bak
-
 test_expect_success 'deleted submodule with .git file' '
+	mv sm2 sm2-bak &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7...0000000 (submodule deleted)
@@ -804,9 +804,9 @@  test_expect_success 'deleted submodule with .git file' '
 	diff_cmp expected actual
 '
 
-echo submodule-to-blob>sm2
-
 test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
+	test_when_finished "rm sm2 && mv sm2-bak sm2" &&
+	echo submodule-to-blob>sm2 &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7...0000000 (submodule deleted)
@@ -836,9 +836,6 @@  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
 	diff_cmp expected actual
 '
 
-rm sm2
-mv sm2-bak sm2
-
 test_expect_success 'setup nested submodule' '
 	git -c protocol.file.allow=always -C sm2 submodule add ../sm2 nested &&
 	git -C sm2 commit -a -m "nested sub" &&
@@ -910,13 +907,11 @@  test_expect_success 'diff --submodule=diff recurses into nested submodules' '
 	diff_cmp expected actual
 '
 
-(cd sm2; commit_file nested)
-commit_file sm2
-head12=$(cd sm2; git rev-parse --short --verify HEAD)
-
-mv sm2 sm2-bak
-
 test_expect_success 'diff --submodule=diff recurses into deleted nested submodules' '
+	(cd sm2 && commit_file nested) &&
+	commit_file sm2 &&
+	head12=$(git -C sm2 rev-parse --short --verify HEAD) &&
+	mv sm2 sm2-bak &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7...0000000 (submodule deleted)
 	Submodule sm2 $head12...0000000 (submodule deleted)
@@ -971,6 +966,8 @@  test_expect_success 'diff --submodule=diff recurses into deleted nested submodul
 	diff_cmp expected actual
 '
 
-mv sm2-bak sm2
+test_expect_success 'submodule cleanup' '
+	mv sm2-bak sm2
+'
 
 test_done