git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage
Date: Tue, 30 Jun 2020 16:56:22 -0400
Message-ID: <CAPig+cSNK1MDitZyh7Ax-eRAh6NjG_QsoF0feEo4475GjZ5ezw@mail.gmail.com> (raw)
In-Reply-To: <01e29450fe51a4ba13e07c611d8795ffd0282b9e.1593529394.git.liu.denton@gmail.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

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 15:03 [PATCH 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-06-30 15:03 ` [PATCH 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-06-30 21:48   ` Eric Sunshine
2020-06-30 22:06     ` Junio C Hamano
2020-06-30 15:03 ` [PATCH 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-06-30 15:03 ` [PATCH 3/5] t7107: don't use test_must_fail() Denton Liu
2020-06-30 15:03 ` [PATCH 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-06-30 15:03 ` [PATCH 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-06-30 20:56   ` Eric Sunshine [this message]
2020-06-30 21:59     ` Eric Sunshine
2020-06-30 23:39     ` Denton Liu
2020-07-01  4:27 ` [PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-07-01  4:27   ` [PATCH v2 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-07-01  4:27   ` [PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-07-01  4:27   ` [PATCH v2 3/5] t7107: don't use test_must_fail() Denton Liu
2020-07-01  4:27   ` [PATCH v2 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-07-01  4:27   ` [PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cSNK1MDitZyh7Ax-eRAh6NjG_QsoF0feEo4475GjZ5ezw@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git