Re: [PATCH 1/2] tests: add 'test_bool_env' to catch non-bool GIT_TEST_* values

[Jeff King <peff@peff.net>]

On Fri, Nov 22, 2019 at 02:14:36PM +0100, SZEDER Gábor wrote:

> Let's be more careful about what the test suite accepts as bool values
> in GIT_TEST_* environment variables, and error out loud and clear on
> invalid values instead of simply skipping tests.  Add the
> 'test_bool_env' helper function to encapsulate the invocation of 'git
> env--helper' and the verification of its exit code, and replace all
> invocations of that command in our test framework and test suite with
> a call to this new helper (except in 't0017-env-helper.sh', of
> course).
> 
>   $ GIT_TEST_GIT_DAEMON=YesPlease ./t5570-git-daemon.sh
>   fatal: bad numeric config value 'YesPlease' for 'GIT_TEST_GIT_DAEMON': invalid unit
>   error: test_bool_env requires bool values both for $GIT_TEST_GIT_DAEMON and for the default fallback

This patch looks good to me. A few musings below, but I'm not sure if
they're worth acting on.

> +test_bool_env () {
> +	if test $# != 2
> +	then
> +		BUG "test_bool_env requires two parameters (variable name and default value)"
> +	fi
> +
> +	git env--helper --type=bool --default="$2" --exit-code "$1"
> +	ret=$?
> +	case $ret in
> +	0|1)	# unset or valid bool value
> +		;;
> +	*)	# invalid bool value or something unexpected
> +		error >&7 "test_bool_env requires bool values both for \$$1 and for the default fallback"
> +		;;
> +	esac
> +	return $ret
> +}

The magic of exit code "1" is undocumented, but we have to rely on it
here. I suggested earlier that we could do:

  if ! val=$(git env--helper --type=bool --default="$2" "$1")
    error ...
  fi
  test "$val" = "true"

but as you noted, we exit with code 1 for "false" even without
--exit-code. IMHO this is a mis-design in the interface of env--helper.

I think it would be an option to change it. It's an undocumented
double-dashed internal helper, so I don't think we need to worry about
breaking compatibility. There's only one other caller that you didn't
touch in this patch, and it uses --exit-code (more on that in a second).

> +test_expect_success 'test_bool_env' '

These tests make sense. In fact, they're much more interesting than the
ones in t0017, since these cover a superset of the code that's actually
used in practice. t0017 covers non-exit-code and --ulong invocations,
but nobody uses them!

I'm wondering if this whole env--helper thing is kind of
over-engineered. Should it actually be a test-tool helper instead of a
shipped builtin? The only call outside of the test suite is this one in
git-sh-i18n:

  # First decide what scheme to use...
  GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
  if test -n "$GIT_TEST_GETTEXT_POISON" &&
              git env--helper --type=bool --default=0 --exit-code \
                  GIT_TEST_GETTEXT_POISON
  then
          GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
  elif test -n "@@USE_GETTEXT_SCHEME@@"
  ...

which suffers from the same problem your patch is fixing. But since this
is again a test-suite thing, it seems like it would be simpler for the
test suite to just set GIT_INTERNAL_GETTEXT_SH_SCHEME=poison itself
(with a little rearranging here to let that override the "fallthrough"
case).

That would make the remaining --exit-code problem go away, remove some
test cruft from production code, and remove the last non-test-suite
caller of env--helper.

At that point we could make it a test-tool builtin. Or even implement it
purely in shell, saving some processes (that would require duplicating
the internal bool logic, but that's way shorter than the boilerplate
needed to expose it via env--helper).

I do think env--helper _could_ be useful for user scripts. But then I
think we'd need to document and rename it to make it clear that it's
part of Git's plumbing that you can depend on.

-Peff