Re: [PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 20, 2010, 15:55 UTC
Message-ID: <AANLkTil9jA8Dva_KqW67c1ZgWk9_a5S1rBViui8Jn0Os@mail.gmail.com>
In-Reply-To: <tMTpk3TmiYV54AYDiNJMBdnlXhSooJQQ1gRoAEsSwmcSwJ9ROgOpr75wpEQNx6_kZkqBtD71_QU@cipher.nrlssc.navy.mil>

On Tue, Jul 20, 2010 at 15:24, Brandon Casey <casey@nrlssc.navy.mil> wrote:

> These two lines use the negation '!' operator to negate the result of a
> simple command.  Since these commands do not contain any pipes or other
> complexities, the test_must_fail function can be used and is preferred
> since it will additionally detect termination due to a signal.

Maybe I'm missing something, but unless `git add --dry-run` is special in being killed due to a signal this seems misguided. We actually prefer to use !, from t/README:

 - test_must_fail <git-command>

   Run a git command and ensure it fails in a controlled way.  Use
   this instead of "! <git-command>" to fail when git commands
   segfault.

Show 10 quoted lines

> This was noticed because the second use of '!' does not include a space
> between the '!' and the opening parens.  Ksh interprets this as follows:
>
>   !(pattern-list)
>      Matches anything except one of the given patterns.
>
> Ksh performs a file glob using the pattern-list and then tries to execute
> the first file in the list.  If a space is added between the '!' and the
> open parens, then Ksh will not interpret it as a pattern list, but in this
> case, it is preferred to use test_must_fail, so lets do so.

Isn't this a completely seperate thing? Was this test really the only bit in the test suite that did "!foo" instead of "! foo" ?

Does the test pass for you if you just:

    @@ -281,7 +281,7 @@ add 'track-this'
     EOF

     test_expect_success 'git add --dry-run --ignore-missing of
non-existing file' '
    -       !(git add --dry-run --ignore-missing track-this
ignored-file >actual 2>&1) &&
    +       ! (git add --dry-run --ignore-missing track-this
ignored-file >actual 2>&1) &&
           test_cmp expect actual
     '

Show 35 quoted lines

> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>  t/t3700-add.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 47fbf53..d03495d 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -268,7 +268,7 @@ test_expect_success 'git add --dry-run of existing changed file' "
>
>  test_expect_success 'git add --dry-run of non-existing file' "
>        echo ignored-file >>.gitignore &&
> -       ! (git add --dry-run track-this ignored-file >actual 2>&1) &&
> +       test_must_fail git add --dry-run track-this ignored-file >actual 2>&1 &&
>        echo \"fatal: pathspec 'ignored-file' did not match any files\" | test_cmp - actual
>  "
>
> @@ -281,7 +281,7 @@ add 'track-this'
>  EOF
>
>  test_expect_success 'git add --dry-run --ignore-missing of non-existing file' '
> -       !(git add --dry-run --ignore-missing track-this ignored-file >actual 2>&1) &&
> +       test_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual 2>&1 &&
>        test_cmp expect actual
>  '
>
> --
> 1.6.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>