[PATCH] t7406: Make a test_must_fail call fail for the right reason

[PATCH] t7406: Make a test_must_fail call fail for the right reason

[Elijah Newren]

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(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..7be8b59569 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCH] t7406: Make a test_must_fail call fail for the right reason

[Eric Sunshine]

On Fri, Aug 3, 2018 at 7:14 PM Elijah Newren <newren@gmail.com> wrote:
> 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.
> ---
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> @@ -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" &&

Unfortunately, this is a mis-use of test_must_fail() which is intended
only for Git commands; it does extra checking to ensure that the Git
command failed in a sane way (say, by returning a failing exit status)
rather than by crashing. It's not intended for use with system
commands which are assumed to be bug-free.

Having a Git command upstream of a pipe is also discouraged since the
pipe swallows its exit status, which means we won't know if the Git
command actually crashed. So, what you really want is this:

    git diff --raw >out &&
    ! grep "<literal-tab>" out &&

(where <literal-tab> is a literal TAB)

Since this script has a number of instances of Git commands upstream
pipes, it may not make sense to fix just this one. So, either a
preparatory cleanup patch could fix them all at once, and then this
patch could come on top or, if you don't want to fix all the pipe
cases, you could do this instead:

    ! git diff --raw | grep "<literal-tab>" &&

Re: [PATCH] t7406: Make a test_must_fail call fail for the right reason

[Eric Sunshine]

On Fri, Aug 3, 2018 at 7:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>     git diff --raw >out &&
>     ! grep "<literal-tab>" out &&


> (where <literal-tab> is a literal TAB)
>
> Since this script has a number of instances of Git commands upstream
> pipes, it may not make sense to fix just this one. So, either a
> preparatory cleanup patch could fix them all at once, and then this
> patch could come on top or, if you don't want to fix all the pipe
> cases, you could do this instead:
>
>     ! git diff --raw | grep "<literal-tab>" &&

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

Re: [PATCH] t7406: Make a test_must_fail call fail for the right reason

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> 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(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index f604ef7a72..7be8b59569 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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" &&

Good spotting, but a few comments.

 * I've seen "do not have 'git' upstream on a pipee (it would hide
   the exit status from an unexpected failure of 'git') recently.
   We probably want to do the same.

 * We do not use test_must_fail for non-git things, as we are not in
   the business of protecting us from unexpected segfault of system
   binaries like grep.

So an immediate improvement for this line would be

	! git diff --raw | grep " submodule" &&

and longer-term clean-up would aim for

	git diff --raw >differs &&
	! grep " submodule" &&

or something like that.  I suspect that --raw may want to be updated
to --name-only or something, as I do not see the tests using the
object names hence no strong need for using --raw format.

[PATCH 0/2] Simple fixes to t7406

[Elijah Newren]

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

[PATCH 1/3] t7406: simplify by using diff --name-only instead of diff --raw

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..e2405c96b5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

[PATCH 2/3] t7406: avoid having git commands upstream of a pipe

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e2405c96b5..c8971abd07 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCH 2/3] t7406: avoid having git commands upstream of a pipe

[SZEDER Gábor]

> 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.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t7406-submodule-update.sh | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index e2405c96b5..c8971abd07 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh

> @@ -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)
>  	)
>  '

These two hunks don't have the desired effect, because command
substitutions used like this will hide the exit code anyway.  I'd
suggest

  git -C submodule log --oneline >out &&
  test_line_count = 1 out

instead, with the additional benefit of a nice error message on
failure.

Re: [PATCH 2/3] t7406: avoid having git commands upstream of a pipe

[Elijah Newren]

On Mon, Aug 6, 2018 at 8:48 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > @@ -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)
> >       )
> >  '
>
> These two hunks don't have the desired effect, because command
> substitutions used like this will hide the exit code anyway.  I'd
> suggest
>
>   git -C submodule log --oneline >out &&
>   test_line_count = 1 out
>
> instead, with the additional benefit of a nice error message on
> failure.

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

[PATCH 3/3] t7406: make a test_must_fail call fail for the right reason

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c8971abd07..f65049ec73 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCH 3/3] t7406: make a test_must_fail call fail for the right reason

[SZEDER Gábor]

> 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.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t7406-submodule-update.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c8971abd07..f65049ec73 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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

May I suggest a while-at-it cleanup? :)
Here 'test_must_fail' is used in front of a shell function, which
should be written as '! compare_head', and indeed in all other places
in this test script it's written that way.

>  	 ) &&
> --
> 2.18.0.548.gd57a518419
> 
> 

Re: [PATCH 3/3] t7406: make a test_must_fail call fail for the right reason

[Elijah Newren]

On Tue, Aug 7, 2018 at 2:07 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> 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.
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>>  t/t7406-submodule-update.sh | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index c8971abd07..f65049ec73 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -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
>
> May I suggest a while-at-it cleanup? :)

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

> Here 'test_must_fail' is used in front of a shell function, which
> should be written as '! compare_head', and indeed in all other places
> in this test script it's written that way.

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.

Re: [PATCH 0/2] Simple fixes to t7406

[Martin Ågren]

On 6 August 2018 at 17:25, Elijah Newren <newren@gmail.com> wrote:
> 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):

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

>   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

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

Re: [PATCH 0/2] Simple fixes to t7406

[Elijah Newren]

On Tue, Aug 7, 2018 at 12:50 AM, Martin Ågren <martin.agren@gmail.com> wrote:
> On 6 August 2018 at 17:25, Elijah Newren <newren@gmail.com> wrote:
>> 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):
>
> I'm not sure what's up with the count of 2 vs 3.

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.

>>   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
>
> 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.

Good point; I'll fix it up.

[PATCHv3 0/5] Simple fixes to t7406

[Elijah Newren]

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

[PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..e2405c96b5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> 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.

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.


> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t7406-submodule-update.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index f604ef7a72..e2405c96b5 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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
>  	 ) &&

Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Elijah Newren]

On Tue, Aug 7, 2018 at 10:29 AM Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
> > 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.
>
> 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 knew someone would find additional issues.  I'll add the anchors if
any other issues come up in review for the series.

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

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

Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> I think 0/5 should fix the real bug you are deliberately keeping in
>> this patch, from the point of view of organization.
>
> 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.

[PATCHv3 2/5] t7406: avoid having git commands upstream of a pipe

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e2405c96b5..c6b7b59350 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

[PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]

[Elijah Newren]

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).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c6b7b59350..ab67e373c5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> 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.

Good explanation.

> 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).

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.

>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t7406-submodule-update.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c6b7b59350..ab67e373c5 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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
>  	)
>  '

[PATCHv3 4/5] t7406: avoid using test_must_fail for commands other than git

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index ab67e373c5..5b42bbe9fa 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

[PATCHv3 5/5] t7406: fix call that was failing for the wrong reason

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 5b42bbe9fa..7dd1c86b02 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> 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.

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

Thanks.

>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t7406-submodule-update.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 5b42bbe9fa..7dd1c86b02 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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
>  	 ) &&

[PATCHv4 0/5] Simple fixes to t7406

[Elijah Newren]

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 <newren@gmail.com>
     
    @@ -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

[PATCHv4 1/5] t7406: fix call that was failing for the wrong reason

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..ccdc2f9039 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

[PATCHv4 2/5] t7406: simplify by using diff --name-only instead of diff --raw

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index ccdc2f9039..f821b01f15 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

[PATCHv4 3/5] t7406: avoid having git commands upstream of a pipe

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f821b01f15..2463863a51 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

[PATCHv4 4/5] t7406: prefer test_* helper functions to test -[feds]

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 6 +++---
 t/test-lib-functions.sh     | 8 ++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 2463863a51..24aa732312 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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
 	)
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca0..4207af4077 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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

[PATCHv4 5/5] t7406: avoid using test_must_fail for commands other than git

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 24aa732312..815b60cb59 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCHv3 0/5] Simple fixes to t7406

[SZEDER Gábor]

On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> wrote:

> 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.

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'.

>     - 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.

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 ;)


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

Re: [PATCHv3 0/5] Simple fixes to t7406

[Elijah Newren]

Hi,

On Mon, Aug 13, 2018 at 1:28 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> wrote:
>
> > 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.
>
> 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"?)

Hmm, borderline.

> - 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'.

Ooh, that does sound better.

> >     - 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.
>
> 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 ;)

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.