diff mbox series

[v3,2/8] env--helper: new undocumented builtin wrapping git_env_*()

Message ID 20190621101812.27300-3-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series Change <non-empty?> GIT_TEST_* variables to <boolean> | expand

Commit Message

Ævar Arnfjörð Bjarmason June 21, 2019, 10:18 a.m. UTC 
We have many GIT_TEST_* variables that accept a <boolean> because
they're implemented in C, and then some that take <non-empty?> because
they're implemented at least partially in shellscript.

Add a helper that wraps git_env_bool() and git_env_ulong() as the
first step in fixing this. This isn't being added as a test-tool mode
because some of these are used outside the test suite.

Part of what this tool does can be done via a trick with "git config"
added in 83d842dc8c ("tests: turn on network daemon tests by default",
2014-02-10) for test_tristate(), i.e.:

    git -c magic.variable="$1" config --bool magic.variable 2>/dev/null

But as subsequent changes will show being able to pass along the
default value makes all the difference, and we'll be able to replace
test_tristate() itself with that.

The --type=bool option will be used by subsequent patches, but not
--type=ulong. I figured it was easy enough to add it & test for it so
I left it in so we'd have wrappers for both git_env_*() functions, and
to have a template to make it obvious how we'd add --type=int etc. if
it's needed in the future.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 builtin.h             |  1 +
 builtin/env--helper.c | 95 +++++++++++++++++++++++++++++++++++++++++++
 git.c                 |  1 +
 t/t0017-env-helper.sh | 83 +++++++++++++++++++++++++++++++++++++
 6 files changed, 182 insertions(+)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0017-env-helper.sh

Comments

Junio C Hamano June 21, 2019, 5:07 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The --type=bool option will be used by subsequent patches, but not
> --type=ulong. I figured it was easy enough to add it & test for it so
> I left it in so we'd have wrappers for both git_env_*() functions, and
> to have a template to make it obvious how we'd add --type=int etc. if
> it's needed in the future.

Yup, it is fine to start small/minimal.  In addition, obviously we
would eventually want "string" type (in fact, it would probably be
the reasonable default when --type=<type> option is missing, once
this becomes a real command and not a test helper).

> +static char const * const env__helper_usage[] = {
> +	N_("git env--helper --type=[bool|ulong] <options> <env-var>"),

This makes it look as if --type=<type> is not among options, which I
am not sure is the impression we would want to give, as I doubt we
would want to keep it mandatory in the long run, but I'd say it is
OK for two reasons (1) as we start minimal, it is OK to keep this
mandatory, and (2) this gives us a place to enumerate available
types.

The output from "git env--helper -h" may redundantly list the
"--type" option, though.

> +	argc = parse_options(argc, argv, prefix, opts, env__helper_usage,
> +			     PARSE_OPT_KEEP_UNKNOWN);

Why KEEP_UNKNOWN?

> +	if (env_default && !*env_default)
> +		usage_with_options(env__helper_usage, opts);
> +	if (!cmdmode)
> +		usage_with_options(env__helper_usage, opts);
> +	if (argc != 1)
> +		usage_with_options(env__helper_usage, opts);
> +	env_variable = argv[0];
> +
> +	switch (cmdmode) {
> +	case ENV_HELPER_TYPE_BOOL:
> +		if (env_default) {
> +			default_int = git_parse_maybe_bool(env_default);
> +			if (default_int == -1) {
> +				error(_("option `--default' expects a boolean value with `--type=bool`, not `%s`"),
> +				      env_default);
> +				usage_with_options(env__helper_usage, opts);
> +			}

OK, that more-or-less is equivalent to what git_config_bool_or_int()
does, except that I would say "if (default_int < 0)" to match the
original more closely if I were doing this.

> +		} else {
> +			default_int = 0;
> +		}
> +		ret_int = git_env_bool(env_variable, default_int);
> +		if (!exit_code)
> +			puts(ret_int ? "true" : "false");
> +		ret = ret_int;
> +		break;

I do not think we want to assign 0 to 'ret' if we are not doing
"exit_code".  "We successfully found that the variable's value is
false, said that to standard output, and signal success by exiting
with 0 status" is what we would want, no?

> +	case ENV_HELPER_TYPE_ULONG:
> +		if (env_default) {
> +			if (!git_parse_ulong(env_default, &default_ulong)) {
> +				error(_("option `--default' expects an unsigned long value with `--type=ulong`, not `%s`"),
> +				      env_default);
> +				usage_with_options(env__helper_usage, opts);
> +			}
> +		} else {
> +			default_ulong = 0;
> +		}
> +		ret_ulong = git_env_ulong(env_variable, default_ulong);
> +		if (!exit_code)
> +			printf("%lu\n", ret_ulong);
> +		ret = ret_ulong;

Likewise.

> +		break;
> +	default:
> +		BUG("unknown <type> value");
> +		break;
> +	}
> +
> +	return !ret;
> +}
> diff --git a/git.c b/git.c
> index c2eec470c9..a43e1dd98e 100644
> --- a/git.c
> +++ b/git.c
> @@ -500,6 +500,7 @@ static struct cmd_struct commands[] = {
>  	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
>  	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
>  	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
> +	{ "env--helper", cmd_env__helper },
>  	{ "fast-export", cmd_fast_export, RUN_SETUP },
>  	{ "fetch", cmd_fetch, RUN_SETUP },
>  	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
> diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
> new file mode 100755
> index 0000000000..709bbbd275
> --- /dev/null
> +++ b/t/t0017-env-helper.sh
> @@ -0,0 +1,83 @@
> +#!/bin/sh
> +
> +test_description='test env--helper'
> +
> +. ./test-lib.sh
> +
> +
> +test_expect_success 'env--helper usage' '
> +	test_must_fail git env--helper &&
> +	test_must_fail git env--helper --type=bool &&
> +	test_must_fail git env--helper --type=ulong &&
> +	test_must_fail git env--helper --type=bool &&
> +	test_must_fail git env--helper --type=bool --default &&
> +	test_must_fail git env--helper --type=bool --default= &&
> +	test_must_fail git env--helper --defaultxyz
> +'
> +
> +test_expect_success 'env--helper bad default values' '

I think

	sane_unset MISSING &&

is needed, as I do not think test-lib.sh filters that variable from
the tester's environment from being seen by us.

> +	test_must_fail git env--helper --type=bool --default=1xyz MISSING &&
> +	test_must_fail git env--helper --type=ulong --default=1xyz MISSING

Both I and you know that the current implementation of env--helper
happens to notice and bail for failing to parse --default without
even consulting the variable, but we do not have to rely on such an
implementation detail.

Speaking of that particular implementation detail, if the variable
is not missing, should we even parse and complain what is in --default?
I think the answer is "yes, for catching bugs in the script, that is
more useful behaviour".

If that is the design principle, we should also test cases where an
existing variable is given with a --default that is unparseable and
make sure that the command fails.

> +'
> +
> +test_expect_success 'env--helper --type=bool' '

Ditto for sane_unset MISSING upfront.

> +	# Test various --default bool values
> +	echo true >expected &&
> +	git env--helper --type=bool --default=1 MISSING >actual &&
> +	test_cmp expected actual &&
> +	git env--helper --type=bool --default=yes MISSING >actual &&
> +	test_cmp expected actual &&
> +	git env--helper --type=bool --default=true MISSING >actual &&
> +	test_cmp expected actual &&
> +	echo false >expected &&
> +	test_must_fail git env--helper --type=bool --default=0 MISSING >actual &&
> +	test_cmp expected actual &&

As I said already, it is a horrible calling convention to make the
mode that produces textual output to fail when the variable is set
to false.  The above two should read more like

    echo true >expect &&
    git env--helper --type=bool --default=true MISSING >actual &&
    test_cmp expect actual &&
    echo false >expect &&
    git env--helper --type=bool --default=0 MISSING >actual &&
    test_cmp expect actual &&
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index 4470d7cfc0..1f7a83fb3c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -58,6 +58,7 @@ 
 /git-difftool
 /git-difftool--helper
 /git-describe
+/git-env--helper
 /git-fast-export
 /git-fast-import
 /git-fetch
diff --git a/Makefile b/Makefile
index f58bf14c7b..f2cfc8d812 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,7 @@  BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
 BUILTIN_OBJS += builtin/difftool.o
+BUILTIN_OBJS += builtin/env--helper.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index ec7e0954c4..93bd49fe4f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -160,6 +160,7 @@  int cmd_diff_index(int argc, const char **argv, const char *prefix);
 int cmd_diff(int argc, const char **argv, const char *prefix);
 int cmd_diff_tree(int argc, const char **argv, const char *prefix);
 int cmd_difftool(int argc, const char **argv, const char *prefix);
+int cmd_env__helper(int argc, const char **argv, const char *prefix);
 int cmd_fast_export(int argc, const char **argv, const char *prefix);
 int cmd_fetch(int argc, const char **argv, const char *prefix);
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/env--helper.c b/builtin/env--helper.c
new file mode 100644
index 0000000000..1083c0f707
--- /dev/null
+++ b/builtin/env--helper.c
@@ -0,0 +1,95 @@ 
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+
+static char const * const env__helper_usage[] = {
+	N_("git env--helper --type=[bool|ulong] <options> <env-var>"),
+	NULL
+};
+
+enum {
+	ENV_HELPER_TYPE_BOOL = 1,
+	ENV_HELPER_TYPE_ULONG
+} cmdmode = 0;
+
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	if (!strcmp(arg, "bool"))
+		cmdmode = ENV_HELPER_TYPE_BOOL;
+	else if (!strcmp(arg, "ulong"))
+		cmdmode = ENV_HELPER_TYPE_ULONG;
+	else
+		die(_("unrecognized --type argument, %s"), arg);
+
+	return 0;
+}
+
+int cmd_env__helper(int argc, const char **argv, const char *prefix)
+{
+	int exit_code = 0;
+	const char *env_variable = NULL;
+	const char *env_default = NULL;
+	int ret;
+	int ret_int, default_int;
+	unsigned long ret_ulong, default_ulong;
+	struct option opts[] = {
+		OPT_CALLBACK_F(0, "type", &cmdmode, N_("type"),
+			       N_("value is given this type"), PARSE_OPT_NONEG,
+			       option_parse_type),
+		OPT_STRING(0, "default", &env_default, N_("value"),
+			   N_("default for git_env_*(...) to fall back on")),
+		OPT_BOOL(0, "exit-code", &exit_code,
+			 N_("be quiet only use git_env_*() value as exit code")),
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, prefix, opts, env__helper_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (env_default && !*env_default)
+		usage_with_options(env__helper_usage, opts);
+	if (!cmdmode)
+		usage_with_options(env__helper_usage, opts);
+	if (argc != 1)
+		usage_with_options(env__helper_usage, opts);
+	env_variable = argv[0];
+
+	switch (cmdmode) {
+	case ENV_HELPER_TYPE_BOOL:
+		if (env_default) {
+			default_int = git_parse_maybe_bool(env_default);
+			if (default_int == -1) {
+				error(_("option `--default' expects a boolean value with `--type=bool`, not `%s`"),
+				      env_default);
+				usage_with_options(env__helper_usage, opts);
+			}
+		} else {
+			default_int = 0;
+		}
+		ret_int = git_env_bool(env_variable, default_int);
+		if (!exit_code)
+			puts(ret_int ? "true" : "false");
+		ret = ret_int;
+		break;
+	case ENV_HELPER_TYPE_ULONG:
+		if (env_default) {
+			if (!git_parse_ulong(env_default, &default_ulong)) {
+				error(_("option `--default' expects an unsigned long value with `--type=ulong`, not `%s`"),
+				      env_default);
+				usage_with_options(env__helper_usage, opts);
+			}
+		} else {
+			default_ulong = 0;
+		}
+		ret_ulong = git_env_ulong(env_variable, default_ulong);
+		if (!exit_code)
+			printf("%lu\n", ret_ulong);
+		ret = ret_ulong;
+		break;
+	default:
+		BUG("unknown <type> value");
+		break;
+	}
+
+	return !ret;
+}
diff --git a/git.c b/git.c
index c2eec470c9..a43e1dd98e 100644
--- a/git.c
+++ b/git.c
@@ -500,6 +500,7 @@  static struct cmd_struct commands[] = {
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
+	{ "env--helper", cmd_env__helper },
 	{ "fast-export", cmd_fast_export, RUN_SETUP },
 	{ "fetch", cmd_fetch, RUN_SETUP },
 	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
new file mode 100755
index 0000000000..709bbbd275
--- /dev/null
+++ b/t/t0017-env-helper.sh
@@ -0,0 +1,83 @@ 
+#!/bin/sh
+
+test_description='test env--helper'
+
+. ./test-lib.sh
+
+
+test_expect_success 'env--helper usage' '
+	test_must_fail git env--helper &&
+	test_must_fail git env--helper --type=bool &&
+	test_must_fail git env--helper --type=ulong &&
+	test_must_fail git env--helper --type=bool &&
+	test_must_fail git env--helper --type=bool --default &&
+	test_must_fail git env--helper --type=bool --default= &&
+	test_must_fail git env--helper --defaultxyz
+'
+
+test_expect_success 'env--helper bad default values' '
+	test_must_fail git env--helper --type=bool --default=1xyz MISSING &&
+	test_must_fail git env--helper --type=ulong --default=1xyz MISSING
+'
+
+test_expect_success 'env--helper --type=bool' '
+	# Test various --default bool values
+	echo true >expected &&
+	git env--helper --type=bool --default=1 MISSING >actual &&
+	test_cmp expected actual &&
+	git env--helper --type=bool --default=yes MISSING >actual &&
+	test_cmp expected actual &&
+	git env--helper --type=bool --default=true MISSING >actual &&
+	test_cmp expected actual &&
+	echo false >expected &&
+	test_must_fail git env--helper --type=bool --default=0 MISSING >actual &&
+	test_cmp expected actual &&
+	test_must_fail git env--helper --type=bool --default=no MISSING >actual &&
+	test_cmp expected actual &&
+	test_must_fail git env--helper --type=bool --default=false MISSING >actual &&
+	test_cmp expected actual &&
+
+	# No output with --exit-code
+	git env--helper --type=bool --default=true --exit-code MISSING >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+	test_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+
+	# Existing variable
+	EXISTS=true git env--helper --type=bool --default=false --exit-code EXISTS >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+	test_must_fail \
+		env EXISTS=false \
+		git env--helper --type=bool --default=true --exit-code EXISTS >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err
+'
+
+test_expect_success 'env--helper --type=ulong' '
+	echo 1234567890 >expected &&
+	git env--helper --type=ulong --default=1234567890 MISSING >actual.out 2>actual.err &&
+	test_cmp expected actual.out &&
+	test_must_be_empty actual.err &&
+
+	echo 0 >expected &&
+	test_must_fail git env--helper --type=ulong --default=0 MISSING >actual &&
+	test_cmp expected actual &&
+
+	git env--helper --type=ulong --default=1234567890 --exit-code MISSING >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+
+	EXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS --exit-code >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+
+	echo 1234567890 >expected &&
+	EXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS >actual.out 2>actual.err &&
+	test_cmp expected actual.out &&
+	test_must_be_empty actual.err
+'
+
+test_done