Re: [PATCH v2 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

From: Jeff King <peff@peff.net>
Date: Mar 30, 2018, 13:48 UTC
Message-ID: <20180330134828.GD29568@sigill.intra.peff.net>
In-Reply-To: <20180330052830.57251-2-me@ttaylorr.com>

On Fri, Mar 30, 2018 at 01:28:30AM -0400, Taylor Blau wrote:

Show 21 quoted lines

> +static int option_parse_type(const struct option *opt, const char *arg,
> +			     int unset)
> +{
> +	int *type = opt->value;
> +
> +	if (!strcmp(arg, "bool"))
> +		*type = TYPE_BOOL;
> +	else if (!strcmp(arg, "int"))
> +		*type = TYPE_INT;
> +	else if (!strcmp(arg, "bool-or-int"))
> +		*type = TYPE_BOOL_OR_INT;
> +	else if (!strcmp(arg, "path"))
> +		*type = TYPE_PATH;
> +	else if (!strcmp(arg, "expiry-date"))
> +		*type = TYPE_EXPIRY_DATE;
> +	else {
> +		die(_("unexpected --type argument, %s"), arg);
> +		return 1;
> +	}
> +	return 0;
> +}

You need to handle "unset" here, which will trigger when somebody does:

  git config --no-type

In which case you'd probably want to reset it to "0".

Show 20 quoted lines

> @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '--type allows valid type specifiers' '
> +	echo "true" >expect &&
> +	git config --type=bool core.foo >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--type rejects unknown specifiers' '
> +	test_must_fail git config --type=nonsense core.foo 2>error &&
> +	test_i18ngrep "unexpected --type argument" error
> +'
> +
> +test_expect_success 'later legacy specifiers are given precedence' '
> +	git config --bool --int core.number >actual &&
> +	echo 10 > expect &&
> +	test_cmp expect actual
> +'

I think there's some rebasing funkiness with this last test, which already exists from patch 1.

Other than those two minor issues and the typos that René noticed, this looks good to me.

-Peff