git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Jeff King" <peff@peff.net>
Subject: Re: [PATCH v3 2/8] env--helper: new undocumented builtin wrapping git_env_*()
Date: Fri, 21 Jun 2019 10:07:21 -0700	[thread overview]
Message-ID: <xmqqr27mkgty.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190621101812.27300-3-avarab@gmail.com> ("Ævar Arnfjörð Bjarmason"'s message of "Fri, 21 Jun 2019 12:18:06 +0200")

Æ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 &&


  reply	other threads:[~2019-06-21 17:07 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-19  9:46 [PATCH] fetch: only run 'gc' once when fetching multiple remotes Nguyễn Thái Ngọc Duy
2019-06-19 10:26 ` [RFC/PATCH] gc: run more pre-detach operations under lock Ævar Arnfjörð Bjarmason
2019-06-19 12:51   ` Duy Nguyen
2019-06-19 18:01     ` Ævar Arnfjörð Bjarmason
2019-06-19 19:10       ` Jeff King
2019-06-19 22:49         ` Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 0/6] Change <non-empty?> GIT_TEST_* variables to <boolean> Ævar Arnfjörð Bjarmason
2019-06-20 18:13             ` Junio C Hamano
2019-06-20 21:00               ` Ævar Arnfjörð Bjarmason
2019-06-20 20:03             ` Junio C Hamano
2019-06-20 21:09             ` [PATCH v2 0/8] " Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 1/8] config tests: simplify include cycle test Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 2/8] env--helper: new undocumented builtin wrapping git_env_*() Ævar Arnfjörð Bjarmason
2019-06-21 17:07                 ` Junio C Hamano [this message]
2019-06-21 10:18               ` [PATCH v3 3/8] config.c: refactor die_bad_number() to not call gettext() early Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 4/8] t6040 test: stop using global "script" variable Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean Ævar Arnfjörð Bjarmason
2019-06-24 16:47                 ` Junio C Hamano
2019-06-21 10:18               ` [PATCH v3 6/8] tests README: re-flow a previously changed paragraph Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 7/8] tests: replace test_tristate with "git env--helper" Ævar Arnfjörð Bjarmason
2019-09-06 12:13                 ` [PATCH 1/2] t/lib-git-svn.sh: check GIT_TEST_SVN_HTTPD when running SVN HTTP tests SZEDER Gábor
2019-09-06 12:13                   ` [PATCH 2/2] ci: restore running httpd tests SZEDER Gábor
2019-09-06 17:03                     ` Junio C Hamano
2019-09-06 19:13                     ` Jeff King
2019-09-07 10:16                       ` SZEDER Gábor
2019-11-22 13:14                         ` [PATCH 0/2] tests: catch non-bool GIT_TEST_* values SZEDER Gábor
2019-11-22 13:14                           ` [PATCH 1/2] tests: add 'test_bool_env' to " SZEDER Gábor
2019-11-25 13:50                             ` Jeff King
2019-11-22 13:14                           ` [PATCH 2/2] t5608-clone-2gb.sh: turn GIT_TEST_CLONE_2GB into a bool SZEDER Gábor
2019-11-25 13:53                             ` Jeff King
2019-06-21 10:18               ` [PATCH v3 8/8] tests: make GIT_TEST_FAIL_PREREQS a boolean Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 1/8] config tests: simplify include cycle test Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 2/8] env--helper: new undocumented builtin wrapping git_env_*() Ævar Arnfjörð Bjarmason
2019-06-20 22:11               ` Junio C Hamano
2019-06-20 22:21                 ` Junio C Hamano
2019-06-21  8:11                   ` Ævar Arnfjörð Bjarmason
2019-06-21 15:04                     ` Junio C Hamano
2019-06-20 21:09             ` [PATCH v2 3/8] config.c: refactor die_bad_number() to not call gettext() early Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 4/8] t6040 test: stop using global "script" variable Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 6/8] tests README: re-flow a previously changed paragraph Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 7/8] tests: replace test_tristate with "git env--helper" Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 8/8] tests: make GIT_TEST_FAIL_PREREQS a boolean Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 1/6] env--helper: new undocumented builtin wrapping git_env_*() Ævar Arnfjörð Bjarmason
2019-06-20 19:25             ` Junio C Hamano
2019-06-19 23:30           ` [PATCH 2/6] t6040 test: stop using global "script" variable Ævar Arnfjörð Bjarmason
2019-06-20 19:54             ` Junio C Hamano
2019-06-19 23:30           ` [PATCH 3/6] tests: make GIT_TEST_GETTEXT_POISON a boolean Ævar Arnfjörð Bjarmason
2019-06-20 20:00             ` Junio C Hamano
2019-06-19 23:30           ` [PATCH 4/6] tests README: re-flow a previously changed paragraph Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 5/6] tests: replace test_tristate with "git env--helper" Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 6/6] tests: make GIT_TEST_FAIL_PREREQS a boolean Ævar Arnfjörð Bjarmason
2019-06-20 10:26           ` [RFC/PATCH] gc: run more pre-detach operations under lock Duy Nguyen
2019-06-20 21:49             ` Ævar Arnfjörð Bjarmason
2019-06-20 10:43           ` Jeff King
2019-06-20 18:55         ` Junio C Hamano
2019-06-19 19:08     ` Jeff King
2019-06-19 18:59 ` [PATCH] fetch: only run 'gc' once when fetching multiple remotes Jeff King
2019-06-20 10:11   ` Duy Nguyen
2019-06-20 10:21     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqr27mkgty.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).