Re: [PATCH 1/6] env--helper: new undocumented builtin wrapping git_env_*()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2019, 19:25 UTC
Message-ID: <xmqqa7ecnjot.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190619233046.27503-2-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 5 quoted lines

> +	struct option opts[] = {
> +		OPT_CMDMODE(0, "mode-bool", &cmdmode,
> +			    N_("invoke git_env_bool(...)"), ENV_HELPER_BOOL),
> +		OPT_CMDMODE(0, "mode-ulong", &cmdmode,
> +			    N_("invoke git_env_ulong(...)"), ENV_HELPER_ULONG),

It may be a fairly useless nitpick, but is there a reason why we should not just reuse "--type=<bool|int|...>" that is understood by "git config"? In the longer term, it might make sense to expose this subcommand as "git env" that sits next to "git config", and at that point we would regret if we miss the opportunity for obvious parallel between the two.

Show 16 quoted lines

> +		OPT_STRING(0, "variable", &env_variable, N_("name"),
> +			   N_("which environment variable to ask git_env_*(...) about")),
> +		OPT_STRING(0, "default", &env_default, N_("value"),
> +			   N_("what default value does git_env_*(...) fall back on?")),
> +		OPT_BOOL(0, "exit-code", &exit_code,
> +			 N_("exit code determined by truth of the git_env_*() function")),
> +		OPT_BOOL(0, "quiet", &quiet,
> +			 N_("don't print the git_env_*() return value")),
> +		OPT_END(),
> +	};
> +
> +	if (parse_options(argc, argv, prefix, opts, env__helper_usage, 0))
> +		usage_with_options(env__helper_usage, opts);
> +	if (!env_variable || !env_default ||
> +	    !*env_variable || !*env_default)
> +		usage_with_options(env__helper_usage, opts);

The default must be supplied? That makes it smell like it should not be a command line "option", as it is not optional at all. Off the top of my head without the benefit of insight you gained by working on the remainder of the series (read: this is merely my knee jerk reaction, and it is very probable that there is sound rationale why your design is not like what I'll outline), I would imagine an interface that look more like:

 $ git env --type=bool --default=yes GIT_TEST_FOO

    Says "true"/"false" on the standard output and exits with
    success status if $GIT_TEST_FOO is set to the usual
    truth/falsehood values, and gives "true" on the exits with
    success status if $GIT_TEST_FOO is unset.

 $ git env --type=bool GIT_TEST_FOO

    The same as above when $GIT_TEST_FOO is set; exits with failure
    when GIT_TEST_FOO is not exported.

 $ git env --type=bool [--default=yes] --exit-code GIT_TEST_FOO

    Similar to the above two, but the standard output is silent, and
    true/false/failure are given via the exit status (perhaps 0, 1
    and 125 or something like that).

Show 5 quoted lines

> +	switch (cmdmode) {
> +	case ENV_HELPER_BOOL:
> +		tmp_int = strtol(env_default, (char **)&env_default, 10);
> +		if (*env_default) {
> +			error(_("option `--default' expects a numerical value with `--mode-bool`"));

No kidding.  "--mode-bool" does not like "--default=false"?