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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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