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

From: Jeff King <peff@peff.net>
Date: Nov 25, 2019, 13:50 UTC
Message-ID: <20191125135016.GA632@sigill.intra.peff.net>
In-Reply-To: <20191122131437.25849-2-szeder.dev@gmail.com>

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

Show 12 quoted lines

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

Show 17 quoted lines

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