t7406: Make a test_must_fail call fail for the right reason

A test making use of test_must_fail was failing like this:
fatal: ambiguous argument '|': unknown revision or path not in the working tree.
when the intent was to verify that a specific string was not found
in the output of the git diff command, i.e. that grep returned non-zero. Fix the test to do that.

---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index f604ef7a72..7be8b59569 100755
@@ -599,7 +599,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	 ) &&
 	 git diff --raw | grep "	submodule" &&
 	 git submodule update --checkout &&
-	 test_must_fail git diff --raw \| grep "	submodule" &&
+	 git diff --raw | test_must_fail grep "	submodule" &&
 	 (cd submodule &&
 	  test_must_fail compare_head
 	 ) &&

2.18.0.548.gcd835b18f7

Of course, I mean "<literal-tab>submodule".

Changes since v1:
- Simplify multiple tests using diff --name-only instead of diff --raw; these tests are only interested in which file was modified anyway. (Suggested by Junio)
- Avoid putting git commands upstream of a pipe, since we don't run under set -o pipefail (Suggested by Eric)
- Avoid using test_must_fail for system binaries (Pointed out by both Eric and Junio)

Elijah Newren (2):
t7406: simplify by using diff --name-only instead of diff --raw
t7406: avoid having git commands upstream of a pipe
t7406: make a test_must_fail call fail for the right reason

 t/t7406-submodule-update.sh | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

2.18.0.548.gd57a518419

We can get rid of some quoted tabs and make a few tests slightly easier to read and edit by just asking for the names of the files modified, since that's all these tests were interested in anyway.

---
 t/t7406-submodule-update.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index f604ef7a72..e2405c96b5 100755
@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep submodule &&
 	 git submodule update &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep submodule &&
 	 (cd submodule &&
 	  compare_head
 	 ) &&

@@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep submodule &&
 	 git submodule update --checkout &&
-	 test_must_fail git diff --raw \| grep "	submodule" &&
+	 test_must_fail git diff --name-only \| grep submodule &&
 	 (cd submodule &&
 	  test_must_fail compare_head
 	 ) &&

2.18.0.548.gd57a518419

When a git command is on the left side of a pipe, the pipe will swallow its exit status, preventing us from detecting failures in said commands. Restructure the tests to put the output in a temporary file to avoid this problem.

---
 t/t7406-submodule-update.sh | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

index e2405c96b5..c8971abd07 100755
@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches
 
 test_expect_success 'submodule init does not copy command into .git/config' '
 	(cd super &&
-	 H=$(git ls-files -s submodule | cut -d" " -f2) &&
+	 git ls-files -s submodule >out &&
+	 H=$(cut -d" " -f2 out) &&
 	 mkdir submodule1 &&
 	 git update-index --add --cacheinfo 160000 $H submodule1 &&
 	 git config -f .gitmodules submodule.submodule1.path submodule1 &&

@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --name-only | grep submodule &&
+	 git diff --name-only >out &&
+	 grep submodule out &&
 	 git submodule update &&
-	 git diff --name-only | grep submodule &&
+	 git diff --name-only >out &&
+	 grep submodule out &&
 	 (cd submodule &&
 	  compare_head
 	 ) &&

@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --name-only | grep submodule &&
+	 git diff --name-only >out &&
+	 grep submodule out &&
 	 git submodule update --checkout &&
 	 test_must_fail git diff --name-only \| grep submodule &&
 	 (cd submodule &&

@@ -885,7 +889,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
 	 H=$(git rev-parse --short HEAD) &&
 	 git commit -am "pre move" &&
 	 H2=$(git rev-parse --short HEAD) &&
-	 git status | sed "s/$H/XXX/" >expect &&
+	 git status >out &&
+	 sed "s/$H/XXX/" out >expect &&
 	 H=$(cd submodule2 && git rev-parse HEAD) &&
 	 git rm --cached submodule2 &&
 	 rm -rf submodule2 &&

@@ -894,7 +899,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
 	 git config -f .gitmodules submodule.submodule2.path "moved/sub module" &&
 	 git commit -am "post move" &&
 	 git submodule update &&
-	 git status | sed "s/$H2/XXX/" >actual &&
+	 git status > out &&
+	 sed "s/$H2/XXX/" out >actual &&
 	 test_cmp expect actual
 	)
 '

@@ -912,7 +918,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'
 
 test_expect_success 'submodule update clone shallow submodule' '
 	test_when_finished "rm -rf super3" &&
-	first=$(git -C cloned submodule status submodule |cut -c2-41) &&
+	first=$(git -C cloned rev-parse HEAD:submodule) &&
 	second=$(git -C submodule rev-parse HEAD) &&
 	commit_count=$(git -C submodule rev-list --count $first^..$second) &&
 	git clone cloned super3 &&

@@ -922,7 +928,7 @@ test_expect_success 'submodule update clone shallow submodule' '
 		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 		mv -f .gitmodules.tmp .gitmodules &&
 		git submodule update --init --depth=$commit_count &&
-		test 1 = $(git -C submodule log --oneline | wc -l)
+		test 1 = $(git -C submodule rev-list --count HEAD)
 	)
 '
 

@@ -938,7 +944,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
 		test_i18ngrep "Direct fetching of that commit failed." actual &&
 		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
 		git submodule update --init --depth=1 >actual &&
-		test 1 = $(git -C submodule log --oneline | wc -l)
+		test 1 = $(git -C submodule rev-list --count HEAD)
 	)
 '
 

2.18.0.548.gd57a518419

Ah, good point...and good suggestion. I'll wait for further feedback then resend with this change.

A test making use of test_must_fail was failing like this:
fatal: ambiguous argument '|': unknown revision or path not in the working tree.
when the intent was to verify that a specific string was not found
in the output of the git diff command, i.e. that grep returned non-zero. Fix the test to do that.

---
 t/t7406-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index c8971abd07..f65049ec73 100755
@@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	 git diff --name-only >out &&
 	 grep submodule out &&
 	 git submodule update --checkout &&
-	 test_must_fail git diff --name-only \| grep submodule &&
+	 git diff --name-only >out &&
+	 ! grep submodule out &&
 	 (cd submodule &&
 	  test_must_fail compare_head
 	 ) &&

2.18.0.548.gd57a518419

I think while-at-it-cleanups have become the primary purpose of this series. :-)

Good catch; I'll fix it up. In fact, there's also one other test_must_fail in front of a non-git command inside t7406, so I'll fix that up while at it too.

I'm not sure what's up with the count of 2 vs 3.

The subject of the final patch doesn't quite match its content anymore. The problematic test_must_fail is dropped, so it can no longer fail. Maybe something like "t7406: fix call that was failing for the wrong reason", or just s/test_must_fail// in what you have.

Martin

I started writing a cover letter, then realized I missed something in the original review. Fixed it up by adding another patch, but decided to just manually "fix up" my cover letter to match -- and missed something. Oops.

Good point; I'll fix it up.

This series started as a simple single-line fix, but folks keep noticing other problems with t7406 while reading my patches. So, changes noted below and I have a challenge at the end.

Changes since v2:
- Two new patches inserted into this series (3/5 and 4/5):
- Prefer test_* helper functions to 'test -[feds]' (see patch 3; this is my attempt to remove easy answers from the challenge below)
- Fix other places in the testfile using test_must_fail for non-git commands (see patch 4; suggested by SZEDER)
- Avoid losing the check of git's exit status in a command substituion, improve potential error message for a test while at it (part of patch 2; suggested by SZEDER)
- Update the commit message for the final commit to match the updated code (Pointed out by Martin)

Since folks like to notice other problems with t7406 while reading my patches, here's a challenge:

Find something *else* wrong with t7406 that neither I nor any of the reviewers so far have caught that could be fixed.

- You get bonus points if that thing is in the context region for one of my five patches.
- Extra bonus points if the thing needing fixing was on a line I changed.
- You win outright if it's something big enough that I give up and request to just have my series merged as-is and punt your suggested fixes down the road to someone else.

(Note: If I flubbed something in v3, pointing it out doesn't count
as finding "something else", but do please still point it out.)

:-)

Elijah Newren (5):
t7406: simplify by using diff --name-only instead of diff --raw
t7406: avoid having git commands upstream of a pipe
t7406: prefer test_* helper functions to test -[feds]
t7406: avoid using test_must_fail for commands other than git
t7406: fix call that was failing for the wrong reason

 t/t7406-submodule-update.sh | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

2.18.0.550.geb414df874.dirty

We can get rid of some quoted tabs and make a few tests slightly easier to read and edit by just asking for the names of the files modified, since that's all these tests were interested in anyway.

---
 t/t7406-submodule-update.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index f604ef7a72..e2405c96b5 100755
@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep submodule &&
 	 git submodule update &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep submodule &&
 	 (cd submodule &&
 	  compare_head
 	 ) &&

@@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep submodule &&
 	 git submodule update --checkout &&
-	 test_must_fail git diff --raw \| grep "	submodule" &&
+	 test_must_fail git diff --name-only \| grep submodule &&
 	 (cd submodule &&
 	  test_must_fail compare_head
 	 ) &&

2.18.0.550.geb414df874.dirty

Technically the quoted tab was making sure that we do not mistake "subsubmodule" (if existed) as "submodule" we seek, so a faithful replacement would be to find "^submodule", and "^submodule$" would be an improvement. But we do not have paths with confusing names in these tests, so we can leave it as-is, I guess.

I think 0/5 should fix the real bug you are deliberately keeping in this patch, from the point of view of organization.

I knew someone would find additional issues. I'll add the anchors if any other issues come up in review for the series.

You mean 5/5? And yeah, it was just a temporary thing for organizational purposes.

I meant "a thing that comes before all the other steps".

As far as I can tell, everything else this 5-patch series improves would still have caught a new bug in the subsystem being tested (i.e. "submodule update") even without these improvements. Surely, if we greak "git diff", "git diff --raw" that sits on the upstream of a pipe would not have stopped these tests, but we have tests for "diff" elsewhere and t7406 are not about catching the breakage of "diff", so from that point of view, fixing them, although it is necessary and important, is less important.

The "\| grep" thing was a real bug in that if we broke "submodule update", it would not have helped to catch such a bug, as it wasn't looking for 'submodule' string in the output.

That is why I felt it would have been a better organization to fix "\| grep" in "a thing that comes before all the other steps" and then fix the rest as lower priority clean up.

When a git command is on the left side of a pipe, the pipe will swallow its exit status, preventing us from detecting failures in said commands. Restructure the tests to put the output in a temporary file to avoid this problem.

---
 t/t7406-submodule-update.sh | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

index e2405c96b5..c6b7b59350 100755
@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches
 
 test_expect_success 'submodule init does not copy command into .git/config' '
 	(cd super &&
-	 H=$(git ls-files -s submodule | cut -d" " -f2) &&
+	 git ls-files -s submodule >out &&
+	 H=$(cut -d" " -f2 out) &&
 	 mkdir submodule1 &&
 	 git update-index --add --cacheinfo 160000 $H submodule1 &&
 	 git config -f .gitmodules submodule.submodule1.path submodule1 &&

@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --name-only | grep submodule &&
+	 git diff --name-only >out &&
+	 grep submodule out &&
 	 git submodule update &&
-	 git diff --name-only | grep submodule &&
+	 git diff --name-only >out &&
+	 grep submodule out &&
 	 (cd submodule &&
 	  compare_head
 	 ) &&

@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --name-only | grep submodule &&
+	 git diff --name-only >out &&
+	 grep submodule out &&
 	 git submodule update --checkout &&
 	 test_must_fail git diff --name-only \| grep submodule &&
 	 (cd submodule &&

@@ -885,7 +889,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
 	 H=$(git rev-parse --short HEAD) &&
 	 git commit -am "pre move" &&
 	 H2=$(git rev-parse --short HEAD) &&
-	 git status | sed "s/$H/XXX/" >expect &&
+	 git status >out &&
+	 sed "s/$H/XXX/" out >expect &&
 	 H=$(cd submodule2 && git rev-parse HEAD) &&
 	 git rm --cached submodule2 &&
 	 rm -rf submodule2 &&

@@ -894,7 +899,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
 	 git config -f .gitmodules submodule.submodule2.path "moved/sub module" &&
 	 git commit -am "post move" &&
 	 git submodule update &&
-	 git status | sed "s/$H2/XXX/" >actual &&
+	 git status > out &&
+	 sed "s/$H2/XXX/" out >actual &&
 	 test_cmp expect actual
 	)
 '

@@ -912,7 +918,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'
 
 test_expect_success 'submodule update clone shallow submodule' '
 	test_when_finished "rm -rf super3" &&
-	first=$(git -C cloned submodule status submodule |cut -c2-41) &&
+	first=$(git -C cloned rev-parse HEAD:submodule) &&
 	second=$(git -C submodule rev-parse HEAD) &&
 	commit_count=$(git -C submodule rev-list --count $first^..$second) &&
 	git clone cloned super3 &&

@@ -922,7 +928,8 @@ test_expect_success 'submodule update clone shallow submodule' '
 		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 		mv -f .gitmodules.tmp .gitmodules &&
 		git submodule update --init --depth=$commit_count &&
-		test 1 = $(git -C submodule log --oneline | wc -l)
+		git -C submodule log --oneline >out &&
+		test_line_count = 1 out
 	)
 '
 

@@ -938,7 +945,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
 		test_i18ngrep "Direct fetching of that commit failed." actual &&
 		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
 		git submodule update --init --depth=1 >actual &&
-		test 1 = $(git -C submodule log --oneline | wc -l)
+		git -C submodule log --oneline >out &&
+		test_line_count = 1 out
 	)
 '
 

2.18.0.550.geb414df874.dirty

test -e, test -s, etc. do not provide nice error messages when we hit test failures, so use the test_* helper functions from test-lib-functions.sh.

Note: The use of 'test_path_is_file submodule/.git' may look odd, but it is a file which is populated with a
gitdir: ../.git/modules/submodule
directive. If, in the future, handling of the submodule is changed and submodule/.git becomes a directory we can change this to test_path_is_dir (or perhaps write a test_path_exists helper function that doesn't care whether the path is a file or a directory).

---
 t/t7406-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index c6b7b59350..ab67e373c5 100755
@@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '
 	  git submodule update > ../actual 2> ../actual.err
 	) &&
 	test_i18ncmp expected actual &&
-	! test -s actual.err
+	test_must_be_empty actual.err
 '
 
 test_expect_success 'submodule update should fail due to local changes' '

@@ -619,8 +619,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
 	git clone super cloned &&
 	(cd cloned &&
 	 git submodule update --init &&
-	 test -e submodule/.git &&
-	 test_must_fail test -e none/.git
+	 test_path_is_file submodule/.git &&
+	 test_path_is_missing none/.git
 	)
 '
 

2.18.0.550.geb414df874.dirty

Good explanation.

Yup, path_exists would be a good direction going forward. If we already have "missing" and use it in this rewrite, it may make sense to introduce "exists" and use it at the same time here.

---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ab67e373c5..5b42bbe9fa 100755
@@ -605,7 +605,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	 git submodule update --checkout &&
 	 test_must_fail git diff --name-only \| grep submodule &&
 	 (cd submodule &&
-	  test_must_fail compare_head
+	  ! compare_head
 	 ) &&
 	 git config --unset submodule.submodule.update
 	)

2.18.0.550.geb414df874.dirty

A test making use of test_must_fail was failing like this:
fatal: ambiguous argument '|': unknown revision or path not in the working tree.
when the intent was to verify that a specific string was not found
in the output of the git diff command, i.e. that grep returned non-zero. Fix the test to do that.

---
 t/t7406-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 5b42bbe9fa..7dd1c86b02 100755
@@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	 git diff --name-only >out &&
 	 grep submodule out &&
 	 git submodule update --checkout &&
-	 test_must_fail git diff --name-only \| grep submodule &&
+	 git diff --name-only >out &&
+	 ! grep submodule out &&
 	 (cd submodule &&
 	  ! compare_head
 	 ) &&

2.18.0.550.geb414df874.dirty

Nice. All other patches and the final shape of this script look sensible.

Thanks.

Changes since v3, all suggested/endorsed by Junio (range-diff at the end):
- Moved the actual fix from being last patch in the series to the first (other patches in this series are just test code cleanups)
- Anchored regexes to avoid matching another filename as a substring
- Add test_path_exists() to test-lib-function.sh and use it (we had test_path_is_dir, test_path_is_file, and test_path_is_missing, but not simple test_path_exists)

Elijah Newren (5):
t7406: fix call that was failing for the wrong reason
t7406: simplify by using diff --name-only instead of diff --raw
t7406: avoid having git commands upstream of a pipe
t7406: prefer test_* helper functions to test -[feds]
t7406: avoid using test_must_fail for commands other than git

 t/t7406-submodule-update.sh | 37 +++++++++++++++++++++++--------------
 t/test-lib-functions.sh     |  8 ++++++++
 2 files changed, 31 insertions(+), 14 deletions(-)

-:  ---------- > 1:  5f257af6c8 t7406: fix call that was failing for the wrong reason
1:  3c369bf73d ! 2:  9e5400a1ad t7406: simplify by using diff --name-only instead of diff --raw
    @@ -16,10 +16,10 @@
      	  compare_head
      	 ) &&
     -	 git diff --raw | grep "	submodule" &&
    -+	 git diff --name-only | grep submodule &&
    ++	 git diff --name-only | grep ^submodule$ &&
      	 git submodule update &&
     -	 git diff --raw | grep "	submodule" &&
    -+	 git diff --name-only | grep submodule &&
    ++	 git diff --name-only | grep ^submodule$ &&
      	 (cd submodule &&
      	  compare_head
      	 ) &&
    @@ -28,10 +28,12 @@
      	  compare_head
      	 ) &&
     -	 git diff --raw | grep "	submodule" &&
    -+	 git diff --name-only | grep submodule &&
    ++	 git diff --name-only | grep ^submodule$ &&
      	 git submodule update --checkout &&
    --	 test_must_fail git diff --raw \| grep "	submodule" &&
    -+	 test_must_fail git diff --name-only \| grep submodule &&
    +-	 git diff --raw >out &&
    +-	 ! grep "	submodule" out &&
    ++	 git diff --name-only >out &&
    ++	 ! grep ^submodule$ out &&
      	 (cd submodule &&
      	  test_must_fail compare_head
      	 ) &&
2:  ba50d6b0f3 ! 3:  4e8cdf60f4 t7406: avoid having git commands upstream of a pipe
    @@ -26,13 +26,13 @@
      	  git checkout master &&
      	  compare_head
      	 ) &&
    --	 git diff --name-only | grep submodule &&
    +-	 git diff --name-only | grep ^submodule$ &&
     +	 git diff --name-only >out &&
    -+	 grep submodule out &&
    ++	 grep ^submodule$ out &&
      	 git submodule update &&
    --	 git diff --name-only | grep submodule &&
    +-	 git diff --name-only | grep ^submodule$ &&
     +	 git diff --name-only >out &&
    -+	 grep submodule out &&
    ++	 grep ^submodule$ out &&
      	 (cd submodule &&
      	  compare_head
      	 ) &&
    @@ -40,12 +40,12 @@
      	  git checkout master &&
      	  compare_head
      	 ) &&
    --	 git diff --name-only | grep submodule &&
    +-	 git diff --name-only | grep ^submodule$ &&
     +	 git diff --name-only >out &&
    -+	 grep submodule out &&
    ++	 grep ^submodule$ out &&
      	 git submodule update --checkout &&
    - 	 test_must_fail git diff --name-only \| grep submodule &&
    - 	 (cd submodule &&
    +	 git diff --name-only >out &&
    +	 ! grep ^submodule$ out &&
     @@
      	 H=$(git rev-parse --short HEAD) &&
      	 git commit -am "pre move" &&
3:  42f7b7f225 ! 4:  f171cbcc9a t7406: prefer test_* helper functions to test -[feds]
    @@ -6,13 +6,12 @@
         test failures, so use the test_* helper functions from
         test-lib-functions.sh.
     
    -    Note: The use of 'test_path_is_file submodule/.git' may look odd, but
    -    it is a file which is populated with a
    +    Also, add test_path_exists() to test-lib-function.sh while at it, so
    +    that we don't need to worry whether submodule/.git is a file or a
    +    directory.  It currently is a file with contents of the form
            gitdir: ../.git/modules/submodule
    -    directive.  If, in the future, handling of the submodule is changed and
    -    submodule/.git becomes a directory we can change this to
    -    test_path_is_dir (or perhaps write a test_path_exists helper function
    -    that doesn't care whether the path is a file or a directory).
    +    but it could be changed in the future to be a directory; this test
    +    only really cares that it exists.
     
         Signed-off-by: Elijah Newren <[email protected]>
     
    @@ -34,8 +33,27 @@
      	 git submodule update --init &&
     -	 test -e submodule/.git &&
     -	 test_must_fail test -e none/.git
    -+	 test_path_is_file submodule/.git &&
    ++	 test_path_exists submodule/.git &&
     +	 test_path_is_missing none/.git
      	)
      '
      
    +
    +diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
    +--- a/t/test-lib-functions.sh
    ++++ b/t/test-lib-functions.sh
    +@@
    +	fi
    + }
    +
    ++test_path_exists () {
    ++	if ! test -e "$1"
    ++	then
    ++		echo "Path $1 doesn't exist. $2"
    ++		false
    ++	fi
    ++}
    ++
    + # Check if the directory exists and is empty as expected, barf otherwise.
    + test_dir_is_empty () {
    +	test_path_is_dir "$1" &&
4:  54cf6531ec ! 5:  a44c566321 t7406: avoid using test_must_fail for commands other than git
    @@ -8,8 +8,8 @@
     --- a/t/t7406-submodule-update.sh
     +++ b/t/t7406-submodule-update.sh
     @@
    - 	 git submodule update --checkout &&
    - 	 test_must_fail git diff --name-only \| grep submodule &&
    +	 git diff --name-only >out &&
    +	 ! grep ^submodule$ out &&
      	 (cd submodule &&
     -	  test_must_fail compare_head
     +	  ! compare_head

5: 3019f2d01c < -: ---------- t7406: fix call that was failing for the wrong reason

2.18.0.556.g1604670984

A test making use of test_must_fail was failing like this:
fatal: ambiguous argument '|': unknown revision or path not in the working tree.
when the intent was to verify that a specific string was not found
in the output of the git diff command, i.e. that grep returned non-zero. Fix the test to do that.

---
 t/t7406-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index f604ef7a72..ccdc2f9039 100755
@@ -599,7 +599,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	 ) &&
 	 git diff --raw | grep "	submodule" &&
 	 git submodule update --checkout &&
-	 test_must_fail git diff --raw \| grep "	submodule" &&
+	 git diff --raw >out &&
+	 ! grep "	submodule" out &&
 	 (cd submodule &&
 	  test_must_fail compare_head
 	 ) &&

2.18.0.556.g1604670984

We can get rid of some quoted tabs and make a few tests slightly easier to read and edit by just asking for the names of the files modified, since that's all these tests were interested in anyway.

---
 t/t7406-submodule-update.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index ccdc2f9039..f821b01f15 100755
@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep ^submodule$ &&
 	 git submodule update &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep ^submodule$ &&
 	 (cd submodule &&
 	  compare_head
 	 ) &&

@@ -597,10 +597,10 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --raw | grep "	submodule" &&
+	 git diff --name-only | grep ^submodule$ &&
 	 git submodule update --checkout &&
-	 git diff --raw >out &&
-	 ! grep "	submodule" out &&
+	 git diff --name-only >out &&
+	 ! grep ^submodule$ out &&
 	 (cd submodule &&
 	  test_must_fail compare_head
 	 ) &&

2.18.0.556.g1604670984

When a git command is on the left side of a pipe, the pipe will swallow its exit status, preventing us from detecting failures in said commands. Restructure the tests to put the output in a temporary file to avoid this problem.

---
 t/t7406-submodule-update.sh | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

index f821b01f15..2463863a51 100755
@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches
 
 test_expect_success 'submodule init does not copy command into .git/config' '
 	(cd super &&
-	 H=$(git ls-files -s submodule | cut -d" " -f2) &&
+	 git ls-files -s submodule >out &&
+	 H=$(cut -d" " -f2 out) &&
 	 mkdir submodule1 &&
 	 git update-index --add --cacheinfo 160000 $H submodule1 &&
 	 git config -f .gitmodules submodule.submodule1.path submodule1 &&

@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --name-only | grep ^submodule$ &&
+	 git diff --name-only >out &&
+	 grep ^submodule$ out &&
 	 git submodule update &&
-	 git diff --name-only | grep ^submodule$ &&
+	 git diff --name-only >out &&
+	 grep ^submodule$ out &&
 	 (cd submodule &&
 	  compare_head
 	 ) &&

@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	  git checkout master &&
 	  compare_head
 	 ) &&
-	 git diff --name-only | grep ^submodule$ &&
+	 git diff --name-only >out &&
+	 grep ^submodule$ out &&
 	 git submodule update --checkout &&
 	 git diff --name-only >out &&
 	 ! grep ^submodule$ out &&

@@ -886,7 +890,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
 	 H=$(git rev-parse --short HEAD) &&
 	 git commit -am "pre move" &&
 	 H2=$(git rev-parse --short HEAD) &&
-	 git status | sed "s/$H/XXX/" >expect &&
+	 git status >out &&
+	 sed "s/$H/XXX/" out >expect &&
 	 H=$(cd submodule2 && git rev-parse HEAD) &&
 	 git rm --cached submodule2 &&
 	 rm -rf submodule2 &&

@@ -895,7 +900,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
 	 git config -f .gitmodules submodule.submodule2.path "moved/sub module" &&
 	 git commit -am "post move" &&
 	 git submodule update &&
-	 git status | sed "s/$H2/XXX/" >actual &&
+	 git status > out &&
+	 sed "s/$H2/XXX/" out >actual &&
 	 test_cmp expect actual
 	)
 '

@@ -913,7 +919,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'
 
 test_expect_success 'submodule update clone shallow submodule' '
 	test_when_finished "rm -rf super3" &&
-	first=$(git -C cloned submodule status submodule |cut -c2-41) &&
+	first=$(git -C cloned rev-parse HEAD:submodule) &&
 	second=$(git -C submodule rev-parse HEAD) &&
 	commit_count=$(git -C submodule rev-list --count $first^..$second) &&
 	git clone cloned super3 &&

@@ -923,7 +929,8 @@ test_expect_success 'submodule update clone shallow submodule' '
 		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 		mv -f .gitmodules.tmp .gitmodules &&
 		git submodule update --init --depth=$commit_count &&
-		test 1 = $(git -C submodule log --oneline | wc -l)
+		git -C submodule log --oneline >out &&
+		test_line_count = 1 out
 	)
 '
 

@@ -939,7 +946,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
 		test_i18ngrep "Direct fetching of that commit failed." actual &&
 		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
 		git submodule update --init --depth=1 >actual &&
-		test 1 = $(git -C submodule log --oneline | wc -l)
+		git -C submodule log --oneline >out &&
+		test_line_count = 1 out
 	)
 '
 

2.18.0.556.g1604670984

test -e, test -s, etc. do not provide nice error messages when we hit test failures, so use the test_* helper functions from test-lib-functions.sh.

Also, add test_path_exists() to test-lib-function.sh while at it, so that we don't need to worry whether submodule/.git is a file or a directory. It currently is a file with contents of the form
gitdir: ../.git/modules/submodule
but it could be changed in the future to be a directory; this test only really cares that it exists.

---
 t/t7406-submodule-update.sh | 6 +++---
 t/test-lib-functions.sh     | 8 ++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

index 2463863a51..24aa732312 100755
@@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '
 	  git submodule update > ../actual 2> ../actual.err
 	) &&
 	test_i18ncmp expected actual &&
-	! test -s actual.err
+	test_must_be_empty actual.err
 '
 
 test_expect_success 'submodule update should fail due to local changes' '

@@ -620,8 +620,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
 	git clone super cloned &&
 	(cd cloned &&
 	 git submodule update --init &&
-	 test -e submodule/.git &&
-	 test_must_fail test -e none/.git
+	 test_path_exists submodule/.git &&
+	 test_path_is_missing none/.git
 	)
 '
 

index 2b2181dca0..4207af4077 100644
@@ -565,6 +565,14 @@ test_path_is_dir () {
 	fi
 }
 
+test_path_exists () {
+	if ! test -e "$1"
+	then
+		echo "Path $1 doesn't exist. $2"
+		false
+	fi
+}
+
 # Check if the directory exists and is empty as expected, barf otherwise.
 test_dir_is_empty () {
 	test_path_is_dir "$1" &&

2.18.0.556.g1604670984

---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 24aa732312..815b60cb59 100755
@@ -606,7 +606,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
 	 git diff --name-only >out &&
 	 ! grep ^submodule$ out &&
 	 (cd submodule &&
-	  test_must_fail compare_head
+	  ! compare_head
 	 ) &&
 	 git config --unset submodule.submodule.update
 	)

2.18.0.556.g1604670984

Well, I'd hate to be that guy... but since those who already commented on previous rounds are not explicitly excluded from the challenge, let's see.

- There are still a few command substitutions running git commands, where the exit status of that command is ignored; just look for the '[^=]$(' pattern in the test script.

(Is not noticing those cases considered as "flubbing"?)

- The 'compare_head' helper function defined in this test script looks very similar to the generally available 'test_cmp_rev' function, which has the benefit to provide some visible output on failure (though, IMO, not a particularly useful output, because the diff of two OIDs is not very informative, but at least it's something as opposed to the silence of 'test $this = $that").

Now, since 'compare_head' always compares the same two revisions, namely 'master' and HEAD, replacing 'compare_head' with an appropriate 'test_cmp_rev' call would result in repeating 'master' and 'HEAD' arguments all over the test script. I'm not sure whether that's good or bad. Anyway, I think that 'compare_head' could be turned into a wrapper around 'test_cmp_rev'.

Well, there's always the indentation of the commands run in subshells, which doesn't conform to our coding style...

Gah, now you made me that guy ;)

Hi,

Hmm, borderline.

Ooh, that does sound better.

I read this on Monday and got a really good laugh. I meant to fix it up, but fell asleep too soon the first couple nights...and now this series is in next anyway and there are a couple other git things that have my attention. You have pointed out a couple additional nice fixups, like you always do, but I think at this point I'm just going to declare you the winner and label these as #leftoverbits.

Thanks for always thoroughly looking over the testcase patches and your constant work to improve the testsuite.