Re: [PATCH] don't use test_must_fail with grep

[Pranit Bauva]

Hey Junio,

On Sun, Jan 8, 2017 at 2:48 AM, Junio C Hamano  wrote:
> I see v3 that has 2>&1 but according to Luke's comment ("the message
> comes from cat"), it shouldn't be there?  I am behind clearing the
> backlog in my mailbox and I could tweak it out from v3 while
> queuing, or I may forget about it after looking at other topics ;-)
> in which case you may want to send v4 with the fix?

Yeah sure! No problem! :)

Regards,
Pranit Bauva

Re: [PATCH] don't use test_must_fail with grep

[Junio C Hamano]

Pranit Bauva  writes:

> Hey Johannes,
>
> On Sun, Jan 1, 2017 at 8:20 PM, Johannes Sixt  wrote:
>> which makes me wonder: Is the message that we do expect not to occur
>> actually printed on stdout? It sounds much more like an error message, i.e.,
>> text that is printed on stderr. Wouldn't we need this?
>>
>> git p4 commit >actual 2>&1 &&
>> ! grep "git author.*does not match" actual &&
>>
>> -- Hannes
>
> This seems better! Since I am at it, I can remove the traces of pipes
> in an another patch.
>
> Regards,
> Pranit Bauva

I see v3 that has 2>&1 but according to Luke's comment ("the message
comes from cat"), it shouldn't be there?  I am behind clearing the
backlog in my mailbox and I could tweak it out from v3 while
queuing, or I may forget about it after looking at other topics ;-)
in which case you may want to send v4 with the fix?

Re: [PATCH] don't use test_must_fail with grep

[Stefan Beller]

On Sat, Dec 31, 2016 at 3:44 AM, Pranit Bauva  wrote:
> test_must_fail should only be used for testing git commands. To test the
> failure of other commands use `!`.
>
> Reported-by: Stefan Beller 
> Signed-off-by: Pranit Bauva 

Thanks for writing up such a patch!
I had put it on my todo list, but you
were faster on actually going through.

Thanks,
Stefan

Re: [PATCH] don't use test_must_fail with grep

[Pranit Bauva]

Hey Johannes,

On Sun, Jan 1, 2017 at 8:20 PM, Johannes Sixt  wrote:
> which makes me wonder: Is the message that we do expect not to occur
> actually printed on stdout? It sounds much more like an error message, i.e.,
> text that is printed on stderr. Wouldn't we need this?
>
> git p4 commit >actual 2>&1 &&
> ! grep "git author.*does not match" actual &&
>
> -- Hannes

This seems better! Since I am at it, I can remove the traces of pipes
in an another patch.

Regards,
Pranit Bauva

Re: [PATCH] don't use test_must_fail with grep

[Luke Diamand]

On 1 January 2017 at 14:50, Johannes Sixt  wrote:
> Am 01.01.2017 um 15:23 schrieb Luke Diamand:
>>
>> On 31 December 2016 at 11:44, Pranit Bauva  wrote:
>>>
>>> diff --git a/t/t9813-git-p4-preserve-users.sh
>>> b/t/t9813-git-p4-preserve-users.sh
>>> index 0fe231280..2384535a7 100755
>>> --- a/t/t9813-git-p4-preserve-users.sh
>>> +++ b/t/t9813-git-p4-preserve-users.sh
>>> @@ -126,13 +126,13 @@ test_expect_success 'not preserving user with mixed
>>> authorship' '
>>> grep "git author char...@example.com does not match" &&
>>>
>>> make_change_by_user usernamefile3 alice al...@example.com
>>> &&
>>> -   git p4 commit |\
>>> -   test_must_fail grep "git author.*does not match" &&
>>> +   ! git p4 commit |\
>>> +   grep "git author.*does not match" &&
>>
>>
>> Would it be clearer to use this?
>>
>> git p4 commit |\
>> grep -q -v "git author.*does not match" &&
>>
>> With your original change, I think that if "git p4 commit" fails, then
>> that expression will be treated as a pass.
>
>
> No. The exit code of the upstream in a pipe is ignored. For this reason,
> having a git invocation as the upstream of a pipe *anywhere* in the test
> suite is frowned upon. Hence, a better rewrite would be
>
> git p4 commit >actual &&
> ! grep "git author.*does not match" actual &&
>
> which makes me wonder: Is the message that we do expect not to occur
> actually printed on stdout? It sounds much more like an error message, i.e.,
> text that is printed on stderr. Wouldn't we need this?
>
> git p4 commit >actual 2>&1 &&
> ! grep "git author.*does not match" actual &&

The message is actually part of a template presented to the user via
their chosen editor. For this test, we set the editor to be "cat", so
it comes out on stdout.

Your first suggestion would therefore be fine (and similarly for the
other cases).

Re: [PATCH] don't use test_must_fail with grep

[Johannes Sixt]

Am 01.01.2017 um 15:23 schrieb Luke Diamand:

On 31 December 2016 at 11:44, Pranit Bauva  wrote:

diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 0fe231280..2384535a7 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -126,13 +126,13 @@ test_expect_success 'not preserving user with mixed 
authorship' '
grep "git author char...@example.com does not match" &&

make_change_by_user usernamefile3 alice al...@example.com &&
-   git p4 commit |\
-   test_must_fail grep "git author.*does not match" &&
+   ! git p4 commit |\
+   grep "git author.*does not match" &&


Would it be clearer to use this?

git p4 commit |\
grep -q -v "git author.*does not match" &&

With your original change, I think that if "git p4 commit" fails, then
that expression will be treated as a pass.


No. The exit code of the upstream in a pipe is ignored. For this reason, 
having a git invocation as the upstream of a pipe *anywhere* in the test 
suite is frowned upon. Hence, a better rewrite would be


git p4 commit >actual &&
! grep "git author.*does not match" actual &&

which makes me wonder: Is the message that we do expect not to occur 
actually printed on stdout? It sounds much more like an error message, 
i.e., text that is printed on stderr. Wouldn't we need this?


git p4 commit >actual 2>&1 &&
! grep "git author.*does not match" actual &&

-- Hannes

Re: [PATCH] don't use test_must_fail with grep

[Luke Diamand]

On 31 December 2016 at 11:44, Pranit Bauva  wrote:
> test_must_fail should only be used for testing git commands. To test the
> failure of other commands use `!`.
>
> Reported-by: Stefan Beller 
> Signed-off-by: Pranit Bauva 
> ---
>  t/t3510-cherry-pick-sequence.sh  |  6 +++---
>  t/t5504-fetch-receive-strict.sh  |  2 +-
>  t/t5516-fetch-push.sh|  2 +-
>  t/t5601-clone.sh |  2 +-
>  t/t6030-bisect-porcelain.sh  |  2 +-
>  t/t7610-mergetool.sh |  2 +-
>  t/t9001-send-email.sh|  2 +-
>  t/t9117-git-svn-init-clone.sh| 12 ++--
>  t/t9813-git-p4-preserve-users.sh |  8 
>  t/t9814-git-p4-rename.sh |  6 +++---
>  10 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 372307c21..0acf4b146 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
> git cat-file commit HEAD~1 >picked_msg &&
> git cat-file commit HEAD~2 >unrelatedpick_msg &&
> git cat-file commit HEAD~3 >initial_msg &&
> -   test_must_fail grep "cherry picked from" initial_msg &&
> +   ! grep "cherry picked from" initial_msg &&
> grep "cherry picked from" unrelatedpick_msg &&
> grep "cherry picked from" picked_msg &&
> grep "cherry picked from" anotherpick_msg
> @@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically 
> propagated to resolved conflict'
> git cat-file commit HEAD~1 >picked_msg &&
> git cat-file commit HEAD~2 >unrelatedpick_msg &&
> git cat-file commit HEAD~3 >initial_msg &&
> -   test_must_fail grep "Signed-off-by:" initial_msg &&
> +   ! grep "Signed-off-by:" initial_msg &&
> grep "Signed-off-by:" unrelatedpick_msg &&
> -   test_must_fail grep "Signed-off-by:" picked_msg &&
> +   ! grep "Signed-off-by:" picked_msg &&
> grep "Signed-off-by:" anotherpick_msg
>  '
>
> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> index 9b19cff72..49d3621a9 100755
> --- a/t/t5504-fetch-receive-strict.sh
> +++ b/t/t5504-fetch-receive-strict.sh
> @@ -152,7 +152,7 @@ test_expect_success 'push with 
> receive.fsck.missingEmail=warn' '
> git --git-dir=dst/.git config --add \
> receive.fsck.badDate warn &&
> git push --porcelain dst bogus >act 2>&1 &&
> -   test_must_fail grep "missingEmail" act
> +   ! grep "missingEmail" act
>  '
>
>  test_expect_success \
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 26b2cafc4..0fc5a7c59 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
>  test_expect_success 'push --porcelain bad url' '
> mk_empty testrepo &&
> test_must_fail git push >.git/bar --porcelain asdfasdfasd 
> refs/heads/master:refs/remotes/origin/master &&
> -   test_must_fail grep -q Done .git/bar
> +   ! grep -q Done .git/bar
>  '
>
>  test_expect_success 'push --porcelain rejected' '
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index a43339420..4241ea5b3 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' 
> '
> git clone --mirror src mirror2 &&
> (cd mirror2 &&
>  git show-ref 2> clone.err > clone.out) &&
> -   test_must_fail grep Duplicate mirror2/clone.err &&
> +   ! grep Duplicate mirror2/clone.err &&
> grep some-tag mirror2/clone.out
>
>  '
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 5e5370feb..8c2c6eaef 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad 
> are siblings' '
> test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
> grep $HASH4 my_bisect_log.txt &&
> git bisect good > my_bisect_log.txt &&
> -   test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
> +   ! grep "merge base must be tested" my_bisect_log.txt &&
> grep $HASH6 my_bisect_log.txt &&
> git bisect reset
>  '
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 63d36fb28..0fe7e58cf 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used 
> with mergetool.writeToT
> test_config mergetool.myecho.trustExitCode true &&
> test_must_fail git merge master &&
> git mergetool --no-prompt --tool myecho -- both >actual &&
> -   test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
> +   ! grep ^\./both_LOCAL_ actual >/dev/null &&
> grep /both_LOCAL_