{"thread":{"id":"49029","subject":"[PATCH] t7406: Make a test_must_fail call fail for the right reason","startedAt":"2018-08-03T23:14:12Z","lastAt":"2018-08-18T20:52:25Z","messageCount":33,"participants":["Elijah Newren","Junio C Hamano","Eric Sunshine","SZEDER Gábor","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"354458","messageId":"20180803231407.10662-1-newren@gmail.com","threadId":"49029","inReplyTo":null,"subject":"[PATCH] t7406: Make a test_must_fail call fail for the right reason","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-03T23:14:07Z","receivedAt":"2018-08-03T23:14:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"A test making use of test_must_fail was failing like this:\n  fatal: ambiguous argument '|': unknown revision or path not in the working tree.\nwhen the intent was to verify that a specific string was not found\nin the output of the git diff command, i.e. that grep returned\nnon-zero.  Fix the test to do that.\n---\n t/t7406-submodule-update.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f604ef7a72..7be8b59569 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -599,7 +599,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t ) &&\n \t git diff --raw | grep \"\tsubmodule\" &&\n \t git submodule update --checkout &&\n-\t test_must_fail git diff --raw \\| grep \"\tsubmodule\" &&\n+\t git diff --raw | test_must_fail grep \"\tsubmodule\" &&\n \t (cd submodule &&\n \t  test_must_fail compare_head\n \t ) &&\n-- \n2.18.0.548.gcd835b18f7\n\n"},{"id":"354459","messageId":"xmqqd0uzuifd.fsf@gitster-ct.c.googlers.com","threadId":"49029","inReplyTo":"20180803231407.10662-1-newren@gmail.com","subject":"Re: [PATCH] t7406: Make a test_must_fail call fail for the right reason","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T23:41:42Z","receivedAt":"2018-08-03T23:41:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> A test making use of test_must_fail was failing like this:\n>   fatal: ambiguous argument '|': unknown revision or path not in the working tree.\n> when the intent was to verify that a specific string was not found\n> in the output of the git diff command, i.e. that grep returned\n> non-zero.  Fix the test to do that.\n> ---\n>  t/t7406-submodule-update.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index f604ef7a72..7be8b59569 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -599,7 +599,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n>  \t ) &&\n>  \t git diff --raw | grep \"\tsubmodule\" &&\n>  \t git submodule update --checkout &&\n> -\t test_must_fail git diff --raw \\| grep \"\tsubmodule\" &&\n> +\t git diff --raw | test_must_fail grep \"\tsubmodule\" &&\n\nGood spotting, but a few comments.\n\n * I've seen \"do not have 'git' upstream on a pipee (it would hide\n   the exit status from an unexpected failure of 'git') recently.\n   We probably want to do the same.\n\n * We do not use test_must_fail for non-git things, as we are not in\n   the business of protecting us from unexpected segfault of system\n   binaries like grep.\n\nSo an immediate improvement for this line would be\n\n\t! git diff --raw | grep \" submodule\" &&\n\nand longer-term clean-up would aim for\n\n\tgit diff --raw >differs &&\n\t! grep \" submodule\" &&\n\nor something like that.  I suspect that --raw may want to be updated\nto --name-only or something, as I do not see the tests using the\nobject names hence no strong need for using --raw format.\n\n\n\n"},{"id":"354460","messageId":"CAPig+cRWxns2t60G5gnQwMGuWyN8BLHq_CLwHznDy=g9haVAMg@mail.gmail.com","threadId":"49029","inReplyTo":"CAPig+cR75Q7HDipttU9dqtNjf23SYNJ98o=0-=JFtPgMMjFxHA@mail.gmail.com","subject":"Re: [PATCH] t7406: Make a test_must_fail call fail for the right reason","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T23:42:05Z","receivedAt":"2018-08-03T23:42:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 3, 2018 at 7:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>     git diff --raw >out &&\n>     ! grep \"<literal-tab>\" out &&\n\n\n> (where <literal-tab> is a literal TAB)\n>\n> Since this script has a number of instances of Git commands upstream\n> pipes, it may not make sense to fix just this one. So, either a\n> preparatory cleanup patch could fix them all at once, and then this\n> patch could come on top or, if you don't want to fix all the pipe\n> cases, you could do this instead:\n>\n>     ! git diff --raw | grep \"<literal-tab>\" &&\n\nOf course, I mean \"<literal-tab>submodule\".\n"},{"id":"354462","messageId":"CAPig+cR75Q7HDipttU9dqtNjf23SYNJ98o=0-=JFtPgMMjFxHA@mail.gmail.com","threadId":"49029","inReplyTo":"20180803231407.10662-1-newren@gmail.com","subject":"Re: [PATCH] t7406: Make a test_must_fail call fail for the right reason","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T23:40:22Z","receivedAt":"2018-08-03T23:46:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 3, 2018 at 7:14 PM Elijah Newren <newren@gmail.com> wrote:\n> A test making use of test_must_fail was failing like this:\n>   fatal: ambiguous argument '|': unknown revision or path not in the working tree.\n> when the intent was to verify that a specific string was not found\n> in the output of the git diff command, i.e. that grep returned\n> non-zero.  Fix the test to do that.\n> ---\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> @@ -599,7 +599,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n>          git diff --raw | grep \"        submodule\" &&\n>          git submodule update --checkout &&\n> -        test_must_fail git diff --raw \\| grep \"        submodule\" &&\n> +        git diff --raw | test_must_fail grep \" submodule\" &&\n\nUnfortunately, this is a mis-use of test_must_fail() which is intended\nonly for Git commands; it does extra checking to ensure that the Git\ncommand failed in a sane way (say, by returning a failing exit status)\nrather than by crashing. It's not intended for use with system\ncommands which are assumed to be bug-free.\n\nHaving a Git command upstream of a pipe is also discouraged since the\npipe swallows its exit status, which means we won't know if the Git\ncommand actually crashed. So, what you really want is this:\n\n    git diff --raw >out &&\n    ! grep \"<literal-tab>\" out &&\n\n(where <literal-tab> is a literal TAB)\n\nSince this script has a number of instances of Git commands upstream\npipes, it may not make sense to fix just this one. So, either a\npreparatory cleanup patch could fix them all at once, and then this\npatch could come on top or, if you don't want to fix all the pipe\ncases, you could do this instead:\n\n    ! git diff --raw | grep \"<literal-tab>\" &&\n"},{"id":"354621","messageId":"20180806152524.27516-2-newren@gmail.com","threadId":"49029","inReplyTo":"20180806152524.27516-1-newren@gmail.com","subject":"[PATCH 1/3] t7406: simplify by using diff --name-only instead of diff --raw","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T15:25:22Z","receivedAt":"2018-08-06T15:25:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We can get rid of some quoted tabs and make a few tests slightly easier\nto read and edit by just asking for the names of the files modified,\nsince that's all these tests were interested in anyway.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f604ef7a72..e2405c96b5 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep submodule &&\n \t git submodule update &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep submodule &&\n \t (cd submodule &&\n \t  compare_head\n \t ) &&\n@@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep submodule &&\n \t git submodule update --checkout &&\n-\t test_must_fail git diff --raw \\| grep \"\tsubmodule\" &&\n+\t test_must_fail git diff --name-only \\| grep submodule &&\n \t (cd submodule &&\n \t  test_must_fail compare_head\n \t ) &&\n-- \n2.18.0.548.gd57a518419\n\n"},{"id":"354622","messageId":"20180806152524.27516-3-newren@gmail.com","threadId":"49029","inReplyTo":"20180806152524.27516-1-newren@gmail.com","subject":"[PATCH 2/3] t7406: avoid having git commands upstream of a pipe","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T15:25:23Z","receivedAt":"2018-08-06T15:25:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When a git command is on the left side of a pipe, the pipe will swallow\nits exit status, preventing us from detecting failures in said commands.\nRestructure the tests to put the output in a temporary file to avoid\nthis problem.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex e2405c96b5..c8971abd07 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches\n \n test_expect_success 'submodule init does not copy command into .git/config' '\n \t(cd super &&\n-\t H=$(git ls-files -s submodule | cut -d\" \" -f2) &&\n+\t git ls-files -s submodule >out &&\n+\t H=$(cut -d\" \" -f2 out) &&\n \t mkdir submodule1 &&\n \t git update-index --add --cacheinfo 160000 $H submodule1 &&\n \t git config -f .gitmodules submodule.submodule1.path submodule1 &&\n@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --name-only | grep submodule &&\n+\t git diff --name-only >out &&\n+\t grep submodule out &&\n \t git submodule update &&\n-\t git diff --name-only | grep submodule &&\n+\t git diff --name-only >out &&\n+\t grep submodule out &&\n \t (cd submodule &&\n \t  compare_head\n \t ) &&\n@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --name-only | grep submodule &&\n+\t git diff --name-only >out &&\n+\t grep submodule out &&\n \t git submodule update --checkout &&\n \t test_must_fail git diff --name-only \\| grep submodule &&\n \t (cd submodule &&\n@@ -885,7 +889,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '\n \t H=$(git rev-parse --short HEAD) &&\n \t git commit -am \"pre move\" &&\n \t H2=$(git rev-parse --short HEAD) &&\n-\t git status | sed \"s/$H/XXX/\" >expect &&\n+\t git status >out &&\n+\t sed \"s/$H/XXX/\" out >expect &&\n \t H=$(cd submodule2 && git rev-parse HEAD) &&\n \t git rm --cached submodule2 &&\n \t rm -rf submodule2 &&\n@@ -894,7 +899,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '\n \t git config -f .gitmodules submodule.submodule2.path \"moved/sub module\" &&\n \t git commit -am \"post move\" &&\n \t git submodule update &&\n-\t git status | sed \"s/$H2/XXX/\" >actual &&\n+\t git status > out &&\n+\t sed \"s/$H2/XXX/\" out >actual &&\n \t test_cmp expect actual\n \t)\n '\n@@ -912,7 +918,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'\n \n test_expect_success 'submodule update clone shallow submodule' '\n \ttest_when_finished \"rm -rf super3\" &&\n-\tfirst=$(git -C cloned submodule status submodule |cut -c2-41) &&\n+\tfirst=$(git -C cloned rev-parse HEAD:submodule) &&\n \tsecond=$(git -C submodule rev-parse HEAD) &&\n \tcommit_count=$(git -C submodule rev-list --count $first^..$second) &&\n \tgit clone cloned super3 &&\n@@ -922,7 +928,7 @@ test_expect_success 'submodule update clone shallow submodule' '\n \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n \t\tgit submodule update --init --depth=$commit_count &&\n-\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t\ttest 1 = $(git -C submodule rev-list --count HEAD)\n \t)\n '\n \n@@ -938,7 +944,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n \t\tgit submodule update --init --depth=1 >actual &&\n-\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t\ttest 1 = $(git -C submodule rev-list --count HEAD)\n \t)\n '\n \n-- \n2.18.0.548.gd57a518419\n\n"},{"id":"354623","messageId":"20180806152524.27516-1-newren@gmail.com","threadId":"49029","inReplyTo":"20180803231407.10662-1-newren@gmail.com","subject":"[PATCH 0/2] Simple fixes to t7406","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T15:25:21Z","receivedAt":"2018-08-06T15:25:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v1:\n  - Simplify multiple tests using diff --name-only instead of diff --raw;\n    these tests are only interested in which file was modified anyway.\n    (Suggested by Junio)\n  - Avoid putting git commands upstream of a pipe, since we don't run under\n    set -o pipefail (Suggested by Eric)\n  - Avoid using test_must_fail for system binaries (Pointed out by\n    both Eric and Junio)\n\nElijah Newren (2):\n  t7406: simplify by using diff --name-only instead of diff --raw\n  t7406: avoid having git commands upstream of a pipe\n  t7406: make a test_must_fail call fail for the right reason\n\n t/t7406-submodule-update.sh | 27 +++++++++++++++++----------\n 1 file changed, 17 insertions(+), 10 deletions(-)\n\n-- \n2.18.0.548.gd57a518419\n\n"},{"id":"354624","messageId":"20180806152524.27516-4-newren@gmail.com","threadId":"49029","inReplyTo":"20180806152524.27516-1-newren@gmail.com","subject":"[PATCH 3/3] t7406: make a test_must_fail call fail for the right reason","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T15:25:24Z","receivedAt":"2018-08-06T15:25:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"A test making use of test_must_fail was failing like this:\n  fatal: ambiguous argument '|': unknown revision or path not in the working tree.\nwhen the intent was to verify that a specific string was not found\nin the output of the git diff command, i.e. that grep returned\nnon-zero.  Fix the test to do that.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex c8971abd07..f65049ec73 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t git diff --name-only >out &&\n \t grep submodule out &&\n \t git submodule update --checkout &&\n-\t test_must_fail git diff --name-only \\| grep submodule &&\n+\t git diff --name-only >out &&\n+\t ! grep submodule out &&\n \t (cd submodule &&\n \t  test_must_fail compare_head\n \t ) &&\n-- \n2.18.0.548.gd57a518419\n\n"},{"id":"354630","messageId":"20180806154814.24585-1-szeder.dev@gmail.com","threadId":"49029","inReplyTo":"20180806152524.27516-3-newren@gmail.com","subject":"Re: [PATCH 2/3] t7406: avoid having git commands upstream of a pipe","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-06T15:48:14Z","receivedAt":"2018-08-06T15:48:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n\n> When a git command is on the left side of a pipe, the pipe will swallow\n> its exit status, preventing us from detecting failures in said commands.\n> Restructure the tests to put the output in a temporary file to avoid\n> this problem.\n> \n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  t/t7406-submodule-update.sh | 24 +++++++++++++++---------\n>  1 file changed, 15 insertions(+), 9 deletions(-)\n> \n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index e2405c96b5..c8971abd07 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n\n> @@ -922,7 +928,7 @@ test_expect_success 'submodule update clone shallow submodule' '\n>  \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n>  \t\tmv -f .gitmodules.tmp .gitmodules &&\n>  \t\tgit submodule update --init --depth=$commit_count &&\n> -\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n> +\t\ttest 1 = $(git -C submodule rev-list --count HEAD)\n>  \t)\n>  '\n> \n> @@ -938,7 +944,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n>  \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n>  \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n>  \t\tgit submodule update --init --depth=1 >actual &&\n> -\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n> +\t\ttest 1 = $(git -C submodule rev-list --count HEAD)\n>  \t)\n>  '\n\nThese two hunks don't have the desired effect, because command\nsubstitutions used like this will hide the exit code anyway.  I'd\nsuggest\n\n  git -C submodule log --oneline >out &&\n  test_line_count = 1 out\n\ninstead, with the additional benefit of a nice error message on\nfailure.\n\n\n"},{"id":"354636","messageId":"CABPp-BEJn3fj8y_K27MOX48t9W9DRS-OOGuJGhq+zuHsMz3SrA@mail.gmail.com","threadId":"49029","inReplyTo":"20180806154814.24585-1-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] t7406: avoid having git commands upstream of a pipe","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T16:09:56Z","receivedAt":"2018-08-06T16:10:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 6, 2018 at 8:48 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> > @@ -922,7 +928,7 @@ test_expect_success 'submodule update clone shallow submodule' '\n> >               sed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n> >               mv -f .gitmodules.tmp .gitmodules &&\n> >               git submodule update --init --depth=$commit_count &&\n> > -             test 1 = $(git -C submodule log --oneline | wc -l)\n> > +             test 1 = $(git -C submodule rev-list --count HEAD)\n> >       )\n> >  '\n> >\n> > @@ -938,7 +944,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n> >               test_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n> >               git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n> >               git submodule update --init --depth=1 >actual &&\n> > -             test 1 = $(git -C submodule log --oneline | wc -l)\n> > +             test 1 = $(git -C submodule rev-list --count HEAD)\n> >       )\n> >  '\n>\n> These two hunks don't have the desired effect, because command\n> substitutions used like this will hide the exit code anyway.  I'd\n> suggest\n>\n>   git -C submodule log --oneline >out &&\n>   test_line_count = 1 out\n>\n> instead, with the additional benefit of a nice error message on\n> failure.\n\nAh, good point...and good suggestion.  I'll wait for further feedback\nthen resend with this change.\n"},{"id":"354710","messageId":"CAN0heSqNFJm7cJ8SpTXLVi_0CFR5fphAT0D34a1k5i8d5T9E_g@mail.gmail.com","threadId":"49029","inReplyTo":"20180806152524.27516-1-newren@gmail.com","subject":"Re: [PATCH 0/2] Simple fixes to t7406","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-08-07T07:50:30Z","receivedAt":"2018-08-07T07:50:33Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 August 2018 at 17:25, Elijah Newren <newren@gmail.com> wrote:\n> Changes since v1:\n>   - Simplify multiple tests using diff --name-only instead of diff --raw;\n>     these tests are only interested in which file was modified anyway.\n>     (Suggested by Junio)\n>   - Avoid putting git commands upstream of a pipe, since we don't run under\n>     set -o pipefail (Suggested by Eric)\n>   - Avoid using test_must_fail for system binaries (Pointed out by\n>     both Eric and Junio)\n>\n> Elijah Newren (2):\n\nI'm not sure what's up with the count of 2 vs 3.\n\n>   t7406: simplify by using diff --name-only instead of diff --raw\n>   t7406: avoid having git commands upstream of a pipe\n>   t7406: make a test_must_fail call fail for the right reason\n\nThe subject of the final patch doesn't quite match its content anymore.\nThe problematic test_must_fail is dropped, so it can no longer fail.\nMaybe something like \"t7406: fix call that was failing for the wrong\nreason\", or just s/test_must_fail// in what you have.\n\nMartin\n"},{"id":"354717","messageId":"20180807090739.5829-1-szeder.dev@gmail.com","threadId":"49029","inReplyTo":"20180806152524.27516-4-newren@gmail.com","subject":"Re: [PATCH 3/3] t7406: make a test_must_fail call fail for the right reason","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-07T09:07:39Z","receivedAt":"2018-08-07T09:07:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> A test making use of test_must_fail was failing like this:\n>   fatal: ambiguous argument '|': unknown revision or path not in the working tree.\n> when the intent was to verify that a specific string was not found\n> in the output of the git diff command, i.e. that grep returned\n> non-zero.  Fix the test to do that.\n> \n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  t/t7406-submodule-update.sh | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index c8971abd07..f65049ec73 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n>  \t git diff --name-only >out &&\n>  \t grep submodule out &&\n>  \t git submodule update --checkout &&\n> -\t test_must_fail git diff --name-only \\| grep submodule &&\n> +\t git diff --name-only >out &&\n> +\t ! grep submodule out &&\n>  \t (cd submodule &&\n>  \t  test_must_fail compare_head\n\nMay I suggest a while-at-it cleanup? :)\nHere 'test_must_fail' is used in front of a shell function, which\nshould be written as '! compare_head', and indeed in all other places\nin this test script it's written that way.\n\n>  \t ) &&\n> --\n> 2.18.0.548.gd57a518419\n> \n> \n"},{"id":"354731","messageId":"CABPp-BGQowPCMA2msTQ8pSQgak+7gig7qeq0dN-ZGU7K+ceCyw@mail.gmail.com","threadId":"49029","inReplyTo":"CAN0heSqNFJm7cJ8SpTXLVi_0CFR5fphAT0D34a1k5i8d5T9E_g@mail.gmail.com","subject":"Re: [PATCH 0/2] Simple fixes to t7406","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T13:40:25Z","receivedAt":"2018-08-07T13:40:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 7, 2018 at 12:50 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n> On 6 August 2018 at 17:25, Elijah Newren <newren@gmail.com> wrote:\n>> Changes since v1:\n>>   - Simplify multiple tests using diff --name-only instead of diff --raw;\n>>     these tests are only interested in which file was modified anyway.\n>>     (Suggested by Junio)\n>>   - Avoid putting git commands upstream of a pipe, since we don't run under\n>>     set -o pipefail (Suggested by Eric)\n>>   - Avoid using test_must_fail for system binaries (Pointed out by\n>>     both Eric and Junio)\n>>\n>> Elijah Newren (2):\n>\n> I'm not sure what's up with the count of 2 vs 3.\n\nI started writing a cover letter, then realized I missed something in\nthe original review.  Fixed it up by adding another patch, but decided\nto just manually \"fix up\" my cover letter to match -- and missed\nsomething.  Oops.\n\n>>   t7406: simplify by using diff --name-only instead of diff --raw\n>>   t7406: avoid having git commands upstream of a pipe\n>>   t7406: make a test_must_fail call fail for the right reason\n>\n> The subject of the final patch doesn't quite match its content anymore.\n> The problematic test_must_fail is dropped, so it can no longer fail.\n> Maybe something like \"t7406: fix call that was failing for the wrong\n> reason\", or just s/test_must_fail// in what you have.\n\nGood point; I'll fix it up.\n"},{"id":"354732","messageId":"CABPp-BEfmUf+TKMb8oX47qJtXJ-X4einLEdEuvJe4=0Pa5D7FA@mail.gmail.com","threadId":"49029","inReplyTo":"20180807090739.5829-1-szeder.dev@gmail.com","subject":"Re: [PATCH 3/3] t7406: make a test_must_fail call fail for the right reason","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T13:46:29Z","receivedAt":"2018-08-07T13:46:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 7, 2018 at 2:07 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>> A test making use of test_must_fail was failing like this:\n>>   fatal: ambiguous argument '|': unknown revision or path not in the working tree.\n>> when the intent was to verify that a specific string was not found\n>> in the output of the git diff command, i.e. that grep returned\n>> non-zero.  Fix the test to do that.\n>>\n>> Signed-off-by: Elijah Newren <newren@gmail.com>\n>> ---\n>>  t/t7406-submodule-update.sh | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>> index c8971abd07..f65049ec73 100755\n>> --- a/t/t7406-submodule-update.sh\n>> +++ b/t/t7406-submodule-update.sh\n>> @@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n>>        git diff --name-only >out &&\n>>        grep submodule out &&\n>>        git submodule update --checkout &&\n>> -      test_must_fail git diff --name-only \\| grep submodule &&\n>> +      git diff --name-only >out &&\n>> +      ! grep submodule out &&\n>>        (cd submodule &&\n>>         test_must_fail compare_head\n>\n> May I suggest a while-at-it cleanup? :)\n\nI think while-at-it-cleanups have become the primary purpose of this\nseries.  :-)\n\n> Here 'test_must_fail' is used in front of a shell function, which\n> should be written as '! compare_head', and indeed in all other places\n> in this test script it's written that way.\n\nGood catch; I'll fix it up.  In fact, there's also one other\ntest_must_fail in front of a non-git command inside t7406, so I'll fix\nthat up while at it too.\n"},{"id":"354749","messageId":"20180807164905.3859-2-newren@gmail.com","threadId":"49029","inReplyTo":"20180807164905.3859-1-newren@gmail.com","subject":"[PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T16:49:01Z","receivedAt":"2018-08-07T16:49:18Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We can get rid of some quoted tabs and make a few tests slightly easier\nto read and edit by just asking for the names of the files modified,\nsince that's all these tests were interested in anyway.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f604ef7a72..e2405c96b5 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep submodule &&\n \t git submodule update &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep submodule &&\n \t (cd submodule &&\n \t  compare_head\n \t ) &&\n@@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep submodule &&\n \t git submodule update --checkout &&\n-\t test_must_fail git diff --raw \\| grep \"\tsubmodule\" &&\n+\t test_must_fail git diff --name-only \\| grep submodule &&\n \t (cd submodule &&\n \t  test_must_fail compare_head\n \t ) &&\n-- \n2.18.0.550.geb414df874.dirty\n\n"},{"id":"354750","messageId":"20180807164905.3859-1-newren@gmail.com","threadId":"49029","inReplyTo":"20180806152524.27516-1-newren@gmail.com","subject":"[PATCHv3 0/5] Simple fixes to t7406","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T16:49:00Z","receivedAt":"2018-08-07T16:49:20Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series started as a simple single-line fix, but folks keep\nnoticing other problems with t7406 while reading my patches.  So, changes\nnoted below and I have a challenge at the end.\n\nChanges since v2:\n  - Two new patches inserted into this series (3/5 and 4/5):\n    - Prefer test_* helper functions to 'test -[feds]' (see patch 3; this\n      is my attempt to remove easy answers from the challenge below)\n    - Fix other places in the testfile using test_must_fail for non-git\n      commands (see patch 4; suggested by SZEDER)\n  - Avoid losing the check of git's exit status in a command substituion,\n    improve potential error message for a test while at it (part of patch 2;\n    suggested by SZEDER)\n  - Update the commit message for the final commit to match the updated\n    code (Pointed out by Martin)\n\nSince folks like to notice other problems with t7406 while reading my\npatches, here's a challenge:\n\n  Find something *else* wrong with t7406 that neither I nor any of the\n  reviewers so far have caught that could be fixed.\n\n    - You get bonus points if that thing is in the context region for\n      one of my five patches.\n    - Extra bonus points if the thing needing fixing was on a line I\n      changed.\n    - You win outright if it's something big enough that I give up and\n      request to just have my series merged as-is and punt your\n      suggested fixes down the road to someone else.\n\n  (Note: If I flubbed something in v3, pointing it out doesn't count\n   as finding \"something else\", but do please still point it out.)\n\n:-)\n\n\nElijah Newren (5):\n  t7406: simplify by using diff --name-only instead of diff --raw\n  t7406: avoid having git commands upstream of a pipe\n  t7406: prefer test_* helper functions to test -[feds]\n  t7406: avoid using test_must_fail for commands other than git\n  t7406: fix call that was failing for the wrong reason\n\n t/t7406-submodule-update.sh | 37 +++++++++++++++++++++++--------------\n 1 file changed, 23 insertions(+), 14 deletions(-)\n\n-- \n2.18.0.550.geb414df874.dirty\n\n"},{"id":"354751","messageId":"20180807164905.3859-4-newren@gmail.com","threadId":"49029","inReplyTo":"20180807164905.3859-1-newren@gmail.com","subject":"[PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T16:49:03Z","receivedAt":"2018-08-07T16:49:20Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"test -e, test -s, etc. do not provide nice error messages when we hit\ntest failures, so use the test_* helper functions from\ntest-lib-functions.sh.\n\nNote: The use of 'test_path_is_file submodule/.git' may look odd, but\nit is a file which is populated with a\n   gitdir: ../.git/modules/submodule\ndirective.  If, in the future, handling of the submodule is changed and\nsubmodule/.git becomes a directory we can change this to\ntest_path_is_dir (or perhaps write a test_path_exists helper function\nthat doesn't care whether the path is a file or a directory).\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex c6b7b59350..ab67e373c5 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '\n \t  git submodule update > ../actual 2> ../actual.err\n \t) &&\n \ttest_i18ncmp expected actual &&\n-\t! test -s actual.err\n+\ttest_must_be_empty actual.err\n '\n \n test_expect_success 'submodule update should fail due to local changes' '\n@@ -619,8 +619,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '\n \tgit clone super cloned &&\n \t(cd cloned &&\n \t git submodule update --init &&\n-\t test -e submodule/.git &&\n-\t test_must_fail test -e none/.git\n+\t test_path_is_file submodule/.git &&\n+\t test_path_is_missing none/.git\n \t)\n '\n \n-- \n2.18.0.550.geb414df874.dirty\n\n"},{"id":"354752","messageId":"20180807164905.3859-6-newren@gmail.com","threadId":"49029","inReplyTo":"20180807164905.3859-1-newren@gmail.com","subject":"[PATCHv3 5/5] t7406: fix call that was failing for the wrong reason","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T16:49:05Z","receivedAt":"2018-08-07T16:49:21Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"A test making use of test_must_fail was failing like this:\n  fatal: ambiguous argument '|': unknown revision or path not in the working tree.\nwhen the intent was to verify that a specific string was not found\nin the output of the git diff command, i.e. that grep returned\nnon-zero.  Fix the test to do that.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 5b42bbe9fa..7dd1c86b02 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t git diff --name-only >out &&\n \t grep submodule out &&\n \t git submodule update --checkout &&\n-\t test_must_fail git diff --name-only \\| grep submodule &&\n+\t git diff --name-only >out &&\n+\t ! grep submodule out &&\n \t (cd submodule &&\n \t  ! compare_head\n \t ) &&\n-- \n2.18.0.550.geb414df874.dirty\n\n"},{"id":"354753","messageId":"20180807164905.3859-5-newren@gmail.com","threadId":"49029","inReplyTo":"20180807164905.3859-1-newren@gmail.com","subject":"[PATCHv3 4/5] t7406: avoid using test_must_fail for commands other than git","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T16:49:04Z","receivedAt":"2018-08-07T16:49:22Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex ab67e373c5..5b42bbe9fa 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -605,7 +605,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t git submodule update --checkout &&\n \t test_must_fail git diff --name-only \\| grep submodule &&\n \t (cd submodule &&\n-\t  test_must_fail compare_head\n+\t  ! compare_head\n \t ) &&\n \t git config --unset submodule.submodule.update\n \t)\n-- \n2.18.0.550.geb414df874.dirty\n\n"},{"id":"354754","messageId":"20180807164905.3859-3-newren@gmail.com","threadId":"49029","inReplyTo":"20180807164905.3859-1-newren@gmail.com","subject":"[PATCHv3 2/5] t7406: avoid having git commands upstream of a pipe","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T16:49:02Z","receivedAt":"2018-08-07T16:49:22Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When a git command is on the left side of a pipe, the pipe will swallow\nits exit status, preventing us from detecting failures in said commands.\nRestructure the tests to put the output in a temporary file to avoid\nthis problem.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex e2405c96b5..c6b7b59350 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches\n \n test_expect_success 'submodule init does not copy command into .git/config' '\n \t(cd super &&\n-\t H=$(git ls-files -s submodule | cut -d\" \" -f2) &&\n+\t git ls-files -s submodule >out &&\n+\t H=$(cut -d\" \" -f2 out) &&\n \t mkdir submodule1 &&\n \t git update-index --add --cacheinfo 160000 $H submodule1 &&\n \t git config -f .gitmodules submodule.submodule1.path submodule1 &&\n@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --name-only | grep submodule &&\n+\t git diff --name-only >out &&\n+\t grep submodule out &&\n \t git submodule update &&\n-\t git diff --name-only | grep submodule &&\n+\t git diff --name-only >out &&\n+\t grep submodule out &&\n \t (cd submodule &&\n \t  compare_head\n \t ) &&\n@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --name-only | grep submodule &&\n+\t git diff --name-only >out &&\n+\t grep submodule out &&\n \t git submodule update --checkout &&\n \t test_must_fail git diff --name-only \\| grep submodule &&\n \t (cd submodule &&\n@@ -885,7 +889,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '\n \t H=$(git rev-parse --short HEAD) &&\n \t git commit -am \"pre move\" &&\n \t H2=$(git rev-parse --short HEAD) &&\n-\t git status | sed \"s/$H/XXX/\" >expect &&\n+\t git status >out &&\n+\t sed \"s/$H/XXX/\" out >expect &&\n \t H=$(cd submodule2 && git rev-parse HEAD) &&\n \t git rm --cached submodule2 &&\n \t rm -rf submodule2 &&\n@@ -894,7 +899,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '\n \t git config -f .gitmodules submodule.submodule2.path \"moved/sub module\" &&\n \t git commit -am \"post move\" &&\n \t git submodule update &&\n-\t git status | sed \"s/$H2/XXX/\" >actual &&\n+\t git status > out &&\n+\t sed \"s/$H2/XXX/\" out >actual &&\n \t test_cmp expect actual\n \t)\n '\n@@ -912,7 +918,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'\n \n test_expect_success 'submodule update clone shallow submodule' '\n \ttest_when_finished \"rm -rf super3\" &&\n-\tfirst=$(git -C cloned submodule status submodule |cut -c2-41) &&\n+\tfirst=$(git -C cloned rev-parse HEAD:submodule) &&\n \tsecond=$(git -C submodule rev-parse HEAD) &&\n \tcommit_count=$(git -C submodule rev-list --count $first^..$second) &&\n \tgit clone cloned super3 &&\n@@ -922,7 +928,8 @@ test_expect_success 'submodule update clone shallow submodule' '\n \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n \t\tgit submodule update --init --depth=$commit_count &&\n-\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t\tgit -C submodule log --oneline >out &&\n+\t\ttest_line_count = 1 out\n \t)\n '\n \n@@ -938,7 +945,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n \t\tgit submodule update --init --depth=1 >actual &&\n-\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t\tgit -C submodule log --oneline >out &&\n+\t\ttest_line_count = 1 out\n \t)\n '\n \n-- \n2.18.0.550.geb414df874.dirty\n\n"},{"id":"354756","messageId":"xmqqo9eekrv0.fsf@gitster-ct.c.googlers.com","threadId":"49029","inReplyTo":"20180807164905.3859-2-newren@gmail.com","subject":"Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T17:29:23Z","receivedAt":"2018-08-07T17:29:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> We can get rid of some quoted tabs and make a few tests slightly easier\n> to read and edit by just asking for the names of the files modified,\n> since that's all these tests were interested in anyway.\n\nTechnically the quoted tab was making sure that we do not mistake\n\"subsubmodule\" (if existed) as \"submodule\" we seek, so a faithful\nreplacement would be to find \"^submodule\", and \"^submodule$\" would\nbe an improvement.  But we do not have paths with confusing names in\nthese tests, so we can leave it as-is, I guess.\n\nI think 0/5 should fix the real bug you are deliberately keeping in\nthis patch, from the point of view of organization.\n\n\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  t/t7406-submodule-update.sh | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index f604ef7a72..e2405c96b5 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '\n>  \t  git checkout master &&\n>  \t  compare_head\n>  \t ) &&\n> -\t git diff --raw | grep \"\tsubmodule\" &&\n> +\t git diff --name-only | grep submodule &&\n>  \t git submodule update &&\n> -\t git diff --raw | grep \"\tsubmodule\" &&\n> +\t git diff --name-only | grep submodule &&\n>  \t (cd submodule &&\n>  \t  compare_head\n>  \t ) &&\n> @@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n>  \t  git checkout master &&\n>  \t  compare_head\n>  \t ) &&\n> -\t git diff --raw | grep \"\tsubmodule\" &&\n> +\t git diff --name-only | grep submodule &&\n>  \t git submodule update --checkout &&\n> -\t test_must_fail git diff --raw \\| grep \"\tsubmodule\" &&\n> +\t test_must_fail git diff --name-only \\| grep submodule &&\n>  \t (cd submodule &&\n>  \t  test_must_fail compare_head\n>  \t ) &&\n"},{"id":"354758","messageId":"xmqqk1p2krlz.fsf@gitster-ct.c.googlers.com","threadId":"49029","inReplyTo":"20180807164905.3859-4-newren@gmail.com","subject":"Re: [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T17:34:48Z","receivedAt":"2018-08-07T17:34:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> test -e, test -s, etc. do not provide nice error messages when we hit\n> test failures, so use the test_* helper functions from\n> test-lib-functions.sh.\n\nGood explanation.\n\n> Note: The use of 'test_path_is_file submodule/.git' may look odd, but\n> it is a file which is populated with a\n>    gitdir: ../.git/modules/submodule\n> directive.  If, in the future, handling of the submodule is changed and\n> submodule/.git becomes a directory we can change this to\n> test_path_is_dir (or perhaps write a test_path_exists helper function\n> that doesn't care whether the path is a file or a directory).\n\nYup, path_exists would be a good direction going forward.  If we\nalready have \"missing\" and use it in this rewrite, it may make sense\nto introduce \"exists\" and use it at the same time here.\n\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  t/t7406-submodule-update.sh | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index c6b7b59350..ab67e373c5 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '\n>  \t  git submodule update > ../actual 2> ../actual.err\n>  \t) &&\n>  \ttest_i18ncmp expected actual &&\n> -\t! test -s actual.err\n> +\ttest_must_be_empty actual.err\n>  '\n>  \n>  test_expect_success 'submodule update should fail due to local changes' '\n> @@ -619,8 +619,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '\n>  \tgit clone super cloned &&\n>  \t(cd cloned &&\n>  \t git submodule update --init &&\n> -\t test -e submodule/.git &&\n> -\t test_must_fail test -e none/.git\n> +\t test_path_is_file submodule/.git &&\n> +\t test_path_is_missing none/.git\n>  \t)\n>  '\n"},{"id":"354759","messageId":"xmqqftzqkri0.fsf@gitster-ct.c.googlers.com","threadId":"49029","inReplyTo":"20180807164905.3859-6-newren@gmail.com","subject":"Re: [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T17:37:11Z","receivedAt":"2018-08-07T17:37:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> A test making use of test_must_fail was failing like this:\n>   fatal: ambiguous argument '|': unknown revision or path not in the working tree.\n> when the intent was to verify that a specific string was not found\n> in the output of the git diff command, i.e. that grep returned\n> non-zero.  Fix the test to do that.\n\nNice.  All other patches and the final shape of this script look\nsensible.\n\nThanks.\n\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  t/t7406-submodule-update.sh | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 5b42bbe9fa..7dd1c86b02 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n>  \t git diff --name-only >out &&\n>  \t grep submodule out &&\n>  \t git submodule update --checkout &&\n> -\t test_must_fail git diff --name-only \\| grep submodule &&\n> +\t git diff --name-only >out &&\n> +\t ! grep submodule out &&\n>  \t (cd submodule &&\n>  \t  ! compare_head\n>  \t ) &&\n"},{"id":"354760","messageId":"CABPp-BGB5dD6UYUzRq2pDSan-2CV7=8xmZKCa0tv7nzY2Mb-aA@mail.gmail.com","threadId":"49029","inReplyTo":"xmqqo9eekrv0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-07T17:40:05Z","receivedAt":"2018-08-07T17:40:19Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 7, 2018 at 10:29 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > We can get rid of some quoted tabs and make a few tests slightly easier\n> > to read and edit by just asking for the names of the files modified,\n> > since that's all these tests were interested in anyway.\n>\n> Technically the quoted tab was making sure that we do not mistake\n> \"subsubmodule\" (if existed) as \"submodule\" we seek, so a faithful\n> replacement would be to find \"^submodule\", and \"^submodule$\" would\n> be an improvement.  But we do not have paths with confusing names in\n> these tests, so we can leave it as-is, I guess.\n\nI knew someone would find additional issues.  I'll add the anchors if\nany other issues come up in review for the series.\n\n> I think 0/5 should fix the real bug you are deliberately keeping in\n> this patch, from the point of view of organization.\n\nYou mean 5/5?  And yeah, it was just a temporary thing for\norganizational purposes.\n"},{"id":"354761","messageId":"xmqqbmaekpke.fsf@gitster-ct.c.googlers.com","threadId":"49029","inReplyTo":"CABPp-BGB5dD6UYUzRq2pDSan-2CV7=8xmZKCa0tv7nzY2Mb-aA@mail.gmail.com","subject":"Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T18:18:57Z","receivedAt":"2018-08-07T18:19:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> I think 0/5 should fix the real bug you are deliberately keeping in\n>> this patch, from the point of view of organization.\n>\n> You mean 5/5?  And yeah, it was just a temporary thing for\n> organizational purposes.\n\nI meant \"a thing that comes before all the other steps\".\n\nAs far as I can tell, everything else this 5-patch series improves\nwould still have caught a new bug in the subsystem being tested\n(i.e. \"submodule update\") even without these improvements.  Surely,\nif we greak \"git diff\", \"git diff --raw\" that sits on the upstream\nof a pipe would not have stopped these tests, but we have tests for\n\"diff\" elsewhere and t7406 are not about catching the breakage of\n\"diff\", so from that point of view, fixing them, although it is\nnecessary and important, is less important.\n\nThe \"\\| grep\" thing was a real bug in that if we broke \"submodule\nupdate\", it would not have helped to catch such a bug, as it wasn't\nlooking for 'submodule' string in the output.\n\nThat is why I felt it would have been a better organization to fix\n\"\\| grep\" in \"a thing that comes before all the other steps\" and\nthen fix the rest as lower priority clean up.\n"},{"id":"354863","messageId":"20180808163107.12329-6-newren@gmail.com","threadId":"49029","inReplyTo":"20180808163107.12329-1-newren@gmail.com","subject":"[PATCHv4 5/5] t7406: avoid using test_must_fail for commands other than git","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-08T16:31:07Z","receivedAt":"2018-08-08T16:31:16Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 24aa732312..815b60cb59 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -606,7 +606,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t git diff --name-only >out &&\n \t ! grep ^submodule$ out &&\n \t (cd submodule &&\n-\t  test_must_fail compare_head\n+\t  ! compare_head\n \t ) &&\n \t git config --unset submodule.submodule.update\n \t)\n-- \n2.18.0.556.g1604670984\n\n"},{"id":"354864","messageId":"20180808163107.12329-3-newren@gmail.com","threadId":"49029","inReplyTo":"20180808163107.12329-1-newren@gmail.com","subject":"[PATCHv4 2/5] t7406: simplify by using diff --name-only instead of diff --raw","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-08T16:31:04Z","receivedAt":"2018-08-08T16:31:16Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We can get rid of some quoted tabs and make a few tests slightly easier\nto read and edit by just asking for the names of the files modified,\nsince that's all these tests were interested in anyway.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex ccdc2f9039..f821b01f15 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep ^submodule$ &&\n \t git submodule update &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep ^submodule$ &&\n \t (cd submodule &&\n \t  compare_head\n \t ) &&\n@@ -597,10 +597,10 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --raw | grep \"\tsubmodule\" &&\n+\t git diff --name-only | grep ^submodule$ &&\n \t git submodule update --checkout &&\n-\t git diff --raw >out &&\n-\t ! grep \"\tsubmodule\" out &&\n+\t git diff --name-only >out &&\n+\t ! grep ^submodule$ out &&\n \t (cd submodule &&\n \t  test_must_fail compare_head\n \t ) &&\n-- \n2.18.0.556.g1604670984\n\n"},{"id":"354865","messageId":"20180808163107.12329-5-newren@gmail.com","threadId":"49029","inReplyTo":"20180808163107.12329-1-newren@gmail.com","subject":"[PATCHv4 4/5] t7406: prefer test_* helper functions to test -[feds]","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-08T16:31:06Z","receivedAt":"2018-08-08T16:31:17Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"test -e, test -s, etc. do not provide nice error messages when we hit\ntest failures, so use the test_* helper functions from\ntest-lib-functions.sh.\n\nAlso, add test_path_exists() to test-lib-function.sh while at it, so\nthat we don't need to worry whether submodule/.git is a file or a\ndirectory.  It currently is a file with contents of the form\n   gitdir: ../.git/modules/submodule\nbut it could be changed in the future to be a directory; this test\nonly really cares that it exists.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 6 +++---\n t/test-lib-functions.sh     | 8 ++++++++\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 2463863a51..24aa732312 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '\n \t  git submodule update > ../actual 2> ../actual.err\n \t) &&\n \ttest_i18ncmp expected actual &&\n-\t! test -s actual.err\n+\ttest_must_be_empty actual.err\n '\n \n test_expect_success 'submodule update should fail due to local changes' '\n@@ -620,8 +620,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '\n \tgit clone super cloned &&\n \t(cd cloned &&\n \t git submodule update --init &&\n-\t test -e submodule/.git &&\n-\t test_must_fail test -e none/.git\n+\t test_path_exists submodule/.git &&\n+\t test_path_is_missing none/.git\n \t)\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2b2181dca0..4207af4077 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -565,6 +565,14 @@ test_path_is_dir () {\n \tfi\n }\n \n+test_path_exists () {\n+\tif ! test -e \"$1\"\n+\tthen\n+\t\techo \"Path $1 doesn't exist. $2\"\n+\t\tfalse\n+\tfi\n+}\n+\n # Check if the directory exists and is empty as expected, barf otherwise.\n test_dir_is_empty () {\n \ttest_path_is_dir \"$1\" &&\n-- \n2.18.0.556.g1604670984\n\n"},{"id":"354866","messageId":"20180808163107.12329-1-newren@gmail.com","threadId":"49029","inReplyTo":"20180807164905.3859-1-newren@gmail.com","subject":"[PATCHv4 0/5] Simple fixes to t7406","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-08T16:31:02Z","receivedAt":"2018-08-08T16:31:19Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v3, all suggested/endorsed by Junio (range-diff at the end):\n  - Moved the actual fix from being last patch in the series to the first\n    (other patches in this series are just test code cleanups)\n  - Anchored regexes to avoid matching another filename as a substring\n  - Add test_path_exists() to test-lib-function.sh and use it (we had\n    test_path_is_dir, test_path_is_file, and test_path_is_missing, but\n    not simple test_path_exists)\n\nElijah Newren (5):\n  t7406: fix call that was failing for the wrong reason\n  t7406: simplify by using diff --name-only instead of diff --raw\n  t7406: avoid having git commands upstream of a pipe\n  t7406: prefer test_* helper functions to test -[feds]\n  t7406: avoid using test_must_fail for commands other than git\n\n t/t7406-submodule-update.sh | 37 +++++++++++++++++++++++--------------\n t/test-lib-functions.sh     |  8 ++++++++\n 2 files changed, 31 insertions(+), 14 deletions(-)\n\n-:  ---------- > 1:  5f257af6c8 t7406: fix call that was failing for the wrong reason\n1:  3c369bf73d ! 2:  9e5400a1ad t7406: simplify by using diff --name-only instead of diff --raw\n    @@ -16,10 +16,10 @@\n      \t  compare_head\n      \t ) &&\n     -\t git diff --raw | grep \"\tsubmodule\" &&\n    -+\t git diff --name-only | grep submodule &&\n    ++\t git diff --name-only | grep ^submodule$ &&\n      \t git submodule update &&\n     -\t git diff --raw | grep \"\tsubmodule\" &&\n    -+\t git diff --name-only | grep submodule &&\n    ++\t git diff --name-only | grep ^submodule$ &&\n      \t (cd submodule &&\n      \t  compare_head\n      \t ) &&\n    @@ -28,10 +28,12 @@\n      \t  compare_head\n      \t ) &&\n     -\t git diff --raw | grep \"\tsubmodule\" &&\n    -+\t git diff --name-only | grep submodule &&\n    ++\t git diff --name-only | grep ^submodule$ &&\n      \t git submodule update --checkout &&\n    --\t test_must_fail git diff --raw \\| grep \"\tsubmodule\" &&\n    -+\t test_must_fail git diff --name-only \\| grep submodule &&\n    +-\t git diff --raw >out &&\n    +-\t ! grep \"\tsubmodule\" out &&\n    ++\t git diff --name-only >out &&\n    ++\t ! grep ^submodule$ out &&\n      \t (cd submodule &&\n      \t  test_must_fail compare_head\n      \t ) &&\n2:  ba50d6b0f3 ! 3:  4e8cdf60f4 t7406: avoid having git commands upstream of a pipe\n    @@ -26,13 +26,13 @@\n      \t  git checkout master &&\n      \t  compare_head\n      \t ) &&\n    --\t git diff --name-only | grep submodule &&\n    +-\t git diff --name-only | grep ^submodule$ &&\n     +\t git diff --name-only >out &&\n    -+\t grep submodule out &&\n    ++\t grep ^submodule$ out &&\n      \t git submodule update &&\n    --\t git diff --name-only | grep submodule &&\n    +-\t git diff --name-only | grep ^submodule$ &&\n     +\t git diff --name-only >out &&\n    -+\t grep submodule out &&\n    ++\t grep ^submodule$ out &&\n      \t (cd submodule &&\n      \t  compare_head\n      \t ) &&\n    @@ -40,12 +40,12 @@\n      \t  git checkout master &&\n      \t  compare_head\n      \t ) &&\n    --\t git diff --name-only | grep submodule &&\n    +-\t git diff --name-only | grep ^submodule$ &&\n     +\t git diff --name-only >out &&\n    -+\t grep submodule out &&\n    ++\t grep ^submodule$ out &&\n      \t git submodule update --checkout &&\n    - \t test_must_fail git diff --name-only \\| grep submodule &&\n    - \t (cd submodule &&\n    +\t git diff --name-only >out &&\n    +\t ! grep ^submodule$ out &&\n     @@\n      \t H=$(git rev-parse --short HEAD) &&\n      \t git commit -am \"pre move\" &&\n3:  42f7b7f225 ! 4:  f171cbcc9a t7406: prefer test_* helper functions to test -[feds]\n    @@ -6,13 +6,12 @@\n         test failures, so use the test_* helper functions from\n         test-lib-functions.sh.\n     \n    -    Note: The use of 'test_path_is_file submodule/.git' may look odd, but\n    -    it is a file which is populated with a\n    +    Also, add test_path_exists() to test-lib-function.sh while at it, so\n    +    that we don't need to worry whether submodule/.git is a file or a\n    +    directory.  It currently is a file with contents of the form\n            gitdir: ../.git/modules/submodule\n    -    directive.  If, in the future, handling of the submodule is changed and\n    -    submodule/.git becomes a directory we can change this to\n    -    test_path_is_dir (or perhaps write a test_path_exists helper function\n    -    that doesn't care whether the path is a file or a directory).\n    +    but it could be changed in the future to be a directory; this test\n    +    only really cares that it exists.\n     \n         Signed-off-by: Elijah Newren <newren@gmail.com>\n     \n    @@ -34,8 +33,27 @@\n      \t git submodule update --init &&\n     -\t test -e submodule/.git &&\n     -\t test_must_fail test -e none/.git\n    -+\t test_path_is_file submodule/.git &&\n    ++\t test_path_exists submodule/.git &&\n     +\t test_path_is_missing none/.git\n      \t)\n      '\n      \n    +\n    +diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n    +--- a/t/test-lib-functions.sh\n    ++++ b/t/test-lib-functions.sh\n    +@@\n    +\tfi\n    + }\n    +\n    ++test_path_exists () {\n    ++\tif ! test -e \"$1\"\n    ++\tthen\n    ++\t\techo \"Path $1 doesn't exist. $2\"\n    ++\t\tfalse\n    ++\tfi\n    ++}\n    ++\n    + # Check if the directory exists and is empty as expected, barf otherwise.\n    + test_dir_is_empty () {\n    +\ttest_path_is_dir \"$1\" &&\n4:  54cf6531ec ! 5:  a44c566321 t7406: avoid using test_must_fail for commands other than git\n    @@ -8,8 +8,8 @@\n     --- a/t/t7406-submodule-update.sh\n     +++ b/t/t7406-submodule-update.sh\n     @@\n    - \t git submodule update --checkout &&\n    - \t test_must_fail git diff --name-only \\| grep submodule &&\n    +\t git diff --name-only >out &&\n    +\t ! grep ^submodule$ out &&\n      \t (cd submodule &&\n     -\t  test_must_fail compare_head\n     +\t  ! compare_head\n5:  3019f2d01c < -:  ---------- t7406: fix call that was failing for the wrong reason\n\n-- \n2.18.0.556.g1604670984\n"},{"id":"354867","messageId":"20180808163107.12329-2-newren@gmail.com","threadId":"49029","inReplyTo":"20180808163107.12329-1-newren@gmail.com","subject":"[PATCHv4 1/5] t7406: fix call that was failing for the wrong reason","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-08T16:31:03Z","receivedAt":"2018-08-08T16:31:20Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"A test making use of test_must_fail was failing like this:\n  fatal: ambiguous argument '|': unknown revision or path not in the working tree.\nwhen the intent was to verify that a specific string was not found\nin the output of the git diff command, i.e. that grep returned\nnon-zero.  Fix the test to do that.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f604ef7a72..ccdc2f9039 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -599,7 +599,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t ) &&\n \t git diff --raw | grep \"\tsubmodule\" &&\n \t git submodule update --checkout &&\n-\t test_must_fail git diff --raw \\| grep \"\tsubmodule\" &&\n+\t git diff --raw >out &&\n+\t ! grep \"\tsubmodule\" out &&\n \t (cd submodule &&\n \t  test_must_fail compare_head\n \t ) &&\n-- \n2.18.0.556.g1604670984\n\n"},{"id":"354868","messageId":"20180808163107.12329-4-newren@gmail.com","threadId":"49029","inReplyTo":"20180808163107.12329-1-newren@gmail.com","subject":"[PATCHv4 3/5] t7406: avoid having git commands upstream of a pipe","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-08T16:31:05Z","receivedAt":"2018-08-08T16:31:21Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When a git command is on the left side of a pipe, the pipe will swallow\nits exit status, preventing us from detecting failures in said commands.\nRestructure the tests to put the output in a temporary file to avoid\nthis problem.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7406-submodule-update.sh | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f821b01f15..2463863a51 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches\n \n test_expect_success 'submodule init does not copy command into .git/config' '\n \t(cd super &&\n-\t H=$(git ls-files -s submodule | cut -d\" \" -f2) &&\n+\t git ls-files -s submodule >out &&\n+\t H=$(cut -d\" \" -f2 out) &&\n \t mkdir submodule1 &&\n \t git update-index --add --cacheinfo 160000 $H submodule1 &&\n \t git config -f .gitmodules submodule.submodule1.path submodule1 &&\n@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --name-only | grep ^submodule$ &&\n+\t git diff --name-only >out &&\n+\t grep ^submodule$ out &&\n \t git submodule update &&\n-\t git diff --name-only | grep ^submodule$ &&\n+\t git diff --name-only >out &&\n+\t grep ^submodule$ out &&\n \t (cd submodule &&\n \t  compare_head\n \t ) &&\n@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou\n \t  git checkout master &&\n \t  compare_head\n \t ) &&\n-\t git diff --name-only | grep ^submodule$ &&\n+\t git diff --name-only >out &&\n+\t grep ^submodule$ out &&\n \t git submodule update --checkout &&\n \t git diff --name-only >out &&\n \t ! grep ^submodule$ out &&\n@@ -886,7 +890,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '\n \t H=$(git rev-parse --short HEAD) &&\n \t git commit -am \"pre move\" &&\n \t H2=$(git rev-parse --short HEAD) &&\n-\t git status | sed \"s/$H/XXX/\" >expect &&\n+\t git status >out &&\n+\t sed \"s/$H/XXX/\" out >expect &&\n \t H=$(cd submodule2 && git rev-parse HEAD) &&\n \t git rm --cached submodule2 &&\n \t rm -rf submodule2 &&\n@@ -895,7 +900,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '\n \t git config -f .gitmodules submodule.submodule2.path \"moved/sub module\" &&\n \t git commit -am \"post move\" &&\n \t git submodule update &&\n-\t git status | sed \"s/$H2/XXX/\" >actual &&\n+\t git status > out &&\n+\t sed \"s/$H2/XXX/\" out >actual &&\n \t test_cmp expect actual\n \t)\n '\n@@ -913,7 +919,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'\n \n test_expect_success 'submodule update clone shallow submodule' '\n \ttest_when_finished \"rm -rf super3\" &&\n-\tfirst=$(git -C cloned submodule status submodule |cut -c2-41) &&\n+\tfirst=$(git -C cloned rev-parse HEAD:submodule) &&\n \tsecond=$(git -C submodule rev-parse HEAD) &&\n \tcommit_count=$(git -C submodule rev-list --count $first^..$second) &&\n \tgit clone cloned super3 &&\n@@ -923,7 +929,8 @@ test_expect_success 'submodule update clone shallow submodule' '\n \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n \t\tgit submodule update --init --depth=$commit_count &&\n-\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t\tgit -C submodule log --oneline >out &&\n+\t\ttest_line_count = 1 out\n \t)\n '\n \n@@ -939,7 +946,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n \t\tgit submodule update --init --depth=1 >actual &&\n-\t\ttest 1 = $(git -C submodule log --oneline | wc -l)\n+\t\tgit -C submodule log --oneline >out &&\n+\t\ttest_line_count = 1 out\n \t)\n '\n \n-- \n2.18.0.556.g1604670984\n\n"},{"id":"355472","messageId":"CAM0VKjn6QAp-hQa3Qp07qZ2unNk20SXYoPwwFbpiLfqqx+KV+A@mail.gmail.com","threadId":"49029","inReplyTo":"20180807164905.3859-1-newren@gmail.com","subject":"Re: [PATCHv3 0/5] Simple fixes to t7406","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-13T20:28:36Z","receivedAt":"2018-08-13T20:28:52Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> wrote:\n\n> Since folks like to notice other problems with t7406 while reading my\n> patches, here's a challenge:\n>\n>   Find something *else* wrong with t7406 that neither I nor any of the\n>   reviewers so far have caught that could be fixed.\n\nWell, I'd hate to be that guy...  but since those who already\ncommented on previous rounds are not explicitly excluded from the\nchallenge, let's see.\n\n- There are still a few command substitutions running git commands,\n  where the exit status of that command is ignored; just look for the\n  '[^=]$(' pattern in the test script.\n\n  (Is not noticing those cases considered as \"flubbing\"?)\n\n- The 'compare_head' helper function defined in this test script looks\n  very similar to the generally available 'test_cmp_rev' function,\n  which has the benefit to provide some visible output on failure\n  (though, IMO, not a particularly useful output, because the diff of\n  two OIDs is not very informative, but at least it's something as\n  opposed to the silence of 'test $this = $that\").\n\n  Now, since 'compare_head' always compares the same two revisions,\n  namely 'master' and HEAD, replacing 'compare_head' with an\n  appropriate 'test_cmp_rev' call would result in repeating 'master'\n  and 'HEAD' arguments all over the test script.  I'm not sure whether\n  that's good or bad.  Anyway, I think that 'compare_head' could be\n  turned into a wrapper around 'test_cmp_rev'.\n\n>     - You get bonus points if that thing is in the context region for\n>       one of my five patches.\n>     - Extra bonus points if the thing needing fixing was on a line I\n>       changed.\n>     - You win outright if it's something big enough that I give up and\n>       request to just have my series merged as-is and punt your\n>       suggested fixes down the road to someone else.\n\nWell, there's always the indentation of the commands run in subshells,\nwhich doesn't conform to our coding style...\n\nGah, now you made me that guy ;)\n\n\n>   (Note: If I flubbed something in v3, pointing it out doesn't count\n>    as finding \"something else\", but do please still point it out.)\n>\n> :-)\n"},{"id":"356006","messageId":"CABPp-BG2q9b1og50auV6-u6hV7Vht8Dxojk8E1802pDpCNnObg@mail.gmail.com","threadId":"49029","inReplyTo":"CAM0VKjn6QAp-hQa3Qp07qZ2unNk20SXYoPwwFbpiLfqqx+KV+A@mail.gmail.com","subject":"Re: [PATCHv3 0/5] Simple fixes to t7406","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-18T20:52:10Z","receivedAt":"2018-08-18T20:52:25Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Mon, Aug 13, 2018 at 1:28 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> > Since folks like to notice other problems with t7406 while reading my\n> > patches, here's a challenge:\n> >\n> >   Find something *else* wrong with t7406 that neither I nor any of the\n> >   reviewers so far have caught that could be fixed.\n>\n> Well, I'd hate to be that guy...  but since those who already\n> commented on previous rounds are not explicitly excluded from the\n> challenge, let's see.\n>\n> - There are still a few command substitutions running git commands,\n>   where the exit status of that command is ignored; just look for the\n>   '[^=]$(' pattern in the test script.\n>\n>   (Is not noticing those cases considered as \"flubbing\"?)\n\nHmm, borderline.\n\n> - The 'compare_head' helper function defined in this test script looks\n>   very similar to the generally available 'test_cmp_rev' function,\n>   which has the benefit to provide some visible output on failure\n>   (though, IMO, not a particularly useful output, because the diff of\n>   two OIDs is not very informative, but at least it's something as\n>   opposed to the silence of 'test $this = $that\").\n>\n>   Now, since 'compare_head' always compares the same two revisions,\n>   namely 'master' and HEAD, replacing 'compare_head' with an\n>   appropriate 'test_cmp_rev' call would result in repeating 'master'\n>   and 'HEAD' arguments all over the test script.  I'm not sure whether\n>   that's good or bad.  Anyway, I think that 'compare_head' could be\n>   turned into a wrapper around 'test_cmp_rev'.\n\nOoh, that does sound better.\n\n> >     - You get bonus points if that thing is in the context region for\n> >       one of my five patches.\n> >     - Extra bonus points if the thing needing fixing was on a line I\n> >       changed.\n> >     - You win outright if it's something big enough that I give up and\n> >       request to just have my series merged as-is and punt your\n> >       suggested fixes down the road to someone else.\n>\n> Well, there's always the indentation of the commands run in subshells,\n> which doesn't conform to our coding style...\n>\n> Gah, now you made me that guy ;)\n\nI read this on Monday and got a really good laugh.  I meant to fix it\nup, but fell asleep too soon the first couple nights...and now this\nseries is in next anyway and there are a couple other git things that\nhave my attention.  You have pointed out a couple additional nice\nfixups, like you always do, but I think at this point I'm just going\nto declare you the winner and label these as #leftoverbits.\n\nThanks for always thoroughly looking over the testcase patches and\nyour constant work to improve the testsuite.\n"}]}