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

From: Denton Liu <liu.denton@gmail.com>
Date: Jun 30, 2020, 23:39 UTC
Message-ID: <20200630233939.GA1301999@generichostname>
In-Reply-To: <CAPig+cSNK1MDitZyh7Ax-eRAh6NjG_QsoF0feEo4475GjZ5ezw@mail.gmail.com>

Hi Eric,

On Tue, Jun 30, 2020 at 04:56:22PM -0400, Eric Sunshine wrote:

Show 13 quoted lines

> > 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.

There are no cases of nested envs. When I was writing this, I wanted to be as permissive as possible in case someone wrote something like this. Now that you bring it up, though, I don't think it makes sense to support this use case because I can't come up with any legitimate reason to have nested envs. (If something comes up in the future, we can reinstate this behaviour.)

Show 8 quoted lines

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

Show 13 quoted lines

> 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).

This is caught by the

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

part. It only emits

	test_must_fail: only 'git' is allowed:

if this happens but it's probably fine as I don't think we should do much hand-holding for this case.

And I'll use the remainder of your suggestions.

Thanks,

Denton