Hi Eric,
On Tue, Jun 30, 2020 at 04:56:22PM -0400, Eric Sunshine wrote:
> > 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.)
> > + do
> > + shift
> > + while test $# -gt 0
> > + do
> > + case "$1" in *?=*) ;; *) break ;; esac
> > + shift
> > + done
> > + done
> 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
