Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:
> In previous commits, we removed the usage of test_must_fail() for most
> commands except for a set of pre-approved commands. Since that's done,
> only allow test_must_fail() to run those pre-approved commands.
>
> Obviously, we should allow `git`.
>
> We allow `__git*` as some completion functions return an error code that
> comes from a git invocation. It's good to avoid using test_must_fail
> unnecessarily but it wouldn't hurt to err on the side of caution when
> we're potentially wrapping a git command (like in these case).

s/case/cases/

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> +# Returns success if the arguments indicate that a command should be
> +# accepted by test_must_fail(). If the command is run with env, the env
> +# and its corresponding variable settings will be stripped before we
> +# test the command being run.
> +test_must_fail_acceptable () {
> +       while test "$1" = "env"

I was surprised to see a 'while' loop for stripping 'env'. Did you
actually run across cases in the test suite in which 'env' was
invoking 'env'? If so, were such cases legitimate (as opposed to
accidental)? Perhaps the commit message or an in-code comment could
help readers understand why it needs to strip multiple 'env's.

> +       do
> +               shift
> +               while test $# -gt 0
> +               do
> +                       case "$1" in *?=*) ;; *) break ;; esac
> +                       shift
> +               done
> +       done

Isn't '*?=*' the same as '?=', or am I misunderstanding the intention?
Also, I wonder how important it is to insist that there must be at
least one character before the '=' sign. (It doesn't necessarily hurt,
but I'm curious if it is protecting against legitimate weird cases.)

This logic would be easier to follow written this way:

    case "$1" in
        =) shift ;;
        *) break ;;
    esac

That is, place the 'shift' in the appropriate case-arm rather than
suspending it below all cases.

> +       case "$1" in
> +       git|__git*|test-tool|test-svn-fe|test_terminal)
> +               return 0
> +               ;;
> +       *)
> +               return 1
> +               ;;
> +       esac
> +}

Would it make sense to error out if "$1" has no value? That is, if the
author wrote:

    test_must_fail &&

or

    test_must_fail env foo=bar &&

then that surely is a programmer error, which could be diagnosed here
(though the original 'test_must_fail' didn't bother diagnosing that
problem so it may be overkill and outside the scope of this series to
do so here).

> @@ -817,6 +842,15 @@ list_contains () {
> +# Do not use this to run anything but "git" and other specific testable
> +# commands (see test_must_fail_acceptable()).  We are not in the
> +# business of vetting system supplied commands -- in other words, this
> +# is wrong:
> +#
> +#    test_must_fail grep pattern output
> +#
> +# Just use '!' instead.

I find this somewhat ambiguous; it's not clear at first sight what I'm
supposed to do with '!'. t/README is slightly clearer by saying "use
'! cmd' instead". It might be even clearer to spell it out explicitly
with an example:

    Instead use '!':

        ! grep pattern output