[PATCH v8 0/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type`

[Taylor Blau <me@ttaylorr.com>]

Hi,

Attached is the eighth re-roll of my series to add `--type=<type>` as
the preferred alternative for `--<type>`.

The main changes since v7 concern handling degenerate cases, such as:

  * git config --type=int --type=bool
  * git config --type=int --int

We have previously had discussion about whether we should (1) retain the
error in previous versions when confronted with multiple, conflicting
type specifiers, (2) ignore the error, in favor of making --<type> and
--type=<type> true synonyms, or (3) some combination of the two.

I have thought some more about my argument that it would be favorable to
make "--type=int" and "--int" behave in the same way, and I am no
longer convinced that my argument makes sense. It's based on the premise
that "--type=<type>" must _necessarily_ allow multiple invocations, such
as '--type=int --type=bool', and therefore "--int --bool" should be
updated to behave the same way.

We are not constrained to this behavior, so in v8, I have taught Git the
following:

  1. Allow multiple non-conflicting types, such as '--int --int',
     '--type=int --int', and '--int --type=int'.

  2. Disallow multiple conflicting types, such as '--int --bool',
     '--type=int --bool', and '--int --type=bool'.

  3. Allow conflicting types following --no-type, such as '--int
     --no-type --bool', '--type=int --no-type --bool', and '--int
     --no-type --type=bool'. Note that this does _not_ introduce options
     such as '--no-int' and whatnot.

This is accomplished by a new locally defined macro called
OPT_CALLBACK_VALUE, which allows us to reuse option_parse_type() to
handle --int as well, by sending it through as opt->defval.

I think that the above is the best-of-all-worlds choice, but I am
curious to hear everyone else's thoughts. Thanks in advance for your
review.


Thanks,
Taylor

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: support `--type=<type>` as preferred alias for
    `--type`

 Documentation/git-config.txt |  71 +++++++++++++-----------
 builtin/config.c             | 101 +++++++++++++++++++++++++----------
 t/t1300-repo-config.sh       |  63 ++++++++++++++++++++++
 3 files changed, 176 insertions(+), 59 deletions(-)

Inter-diff (since v7):

diff --git a/builtin/config.c b/builtin/config.c
index 5c8952a17c..7184c09582 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,28 +61,53 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5

+#define OPT_CALLBACK_VALUE(s, l, h, f, i) \
+	{ OPTION_CALLBACK, (s), (l), NULL, NULL, (h), PARSE_OPT_NOARG | \
+	PARSE_OPT_NONEG, (f), (i) }
+
+static struct option builtin_config_options[];
+
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
 {
-	int *type = opt->value;
-
 	if (unset) {
-		*type = 0;
+		type = 0;
 		return 0;
 	}

-	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(_("unrecognized --type argument, %s"), arg);
+	/*
+	 * To support '--<type>' style flags, begin with new_type equal to
+	 * opt->defval.
+	 */
+	int new_type = opt->defval;
+	if (!new_type) {
+		if (!strcmp(arg, "bool"))
+			new_type = TYPE_BOOL;
+		else if (!strcmp(arg, "int"))
+			new_type = TYPE_INT;
+		else if (!strcmp(arg, "bool-or-int"))
+			new_type = TYPE_BOOL_OR_INT;
+		else if (!strcmp(arg, "path"))
+			new_type = TYPE_PATH;
+		else if (!strcmp(arg, "expiry-date"))
+			new_type = TYPE_EXPIRY_DATE;
+		else
+			die(_("unrecognized --type argument, %s"), arg);
+	}
+
+	if (type != 0 && type != new_type) {
+		/*
+		 * Complain when there is a new type not equal to the old type.
+		 * This allows for combinations like '--int --type=int' and
+		 * '--type=int --type=int', but disallows ones like '--type=bool
+		 * --int' and '--type=bool
+		 * --type=int'.
+		 */
+		error("only one type at a time.");
+		usage_with_options(builtin_config_usage,
+			builtin_config_options);
+	}
+	type = new_type;

 	return 0;
 }
@@ -110,12 +135,12 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
 	OPT_GROUP(N_("Type")),
-	OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type),
-	OPT_SET_INT(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_SET_INT(0, "int", &type, N_("value is decimal number"), TYPE_INT),
-	OPT_SET_INT(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_SET_INT(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_SET_INT(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	OPT_CALLBACK('t', "type", NULL, "", N_("value is given this type"), option_parse_type),
+	OPT_CALLBACK_VALUE(0, "bool", N_("value is \"true\" or \"false\""), option_parse_type, TYPE_BOOL),
+	OPT_CALLBACK_VALUE(0, "int", N_("value is decimal number"), option_parse_type, TYPE_INT),
+	OPT_CALLBACK_VALUE(0, "bool-or-int", N_("value is --bool or --int"), option_parse_type, TYPE_BOOL_OR_INT),
+	OPT_CALLBACK_VALUE(0, "path", N_("value is a path (file or directory name)"), option_parse_type, TYPE_PATH),
+	OPT_CALLBACK_VALUE(0, "expiry-date", N_("value is an expiry date"), option_parse_type, TYPE_EXPIRY_DATE),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index f5ae80e9ae..e06af3d337 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1615,14 +1615,42 @@ cat >.git/config <<-\EOF &&
 [core]
 foo = true
 number = 10
+big = 1M
 EOF

-test_expect_success 'later legacy specifiers are given precedence' '
-	git config --bool --int core.number >actual &&
-	echo 10 >expect &&
+test_expect_success 'identical modern --type specifiers are allowed' '
+	git config --type=int --type=int core.big >actual &&
+	echo 1048576 >expect &&
 	test_cmp expect actual
 '

+test_expect_success 'identical legacy --type specifiers are allowed' '
+	git config --int --int core.big >actual &&
+	echo 1048576 >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'identical mixed --type specifiers are allowed' '
+	git config --int --type=int core.big >actual &&
+	echo 1048576 >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'non-identical modern --type specifiers are not allowed' '
+	test_must_fail git config --type=int --type=bool core.big 2>error &&
+	test_i18ngrep "only one type at a time" error
+'
+
+test_expect_success 'non-identical legacy --type specifiers are not allowed' '
+	test_must_fail git config --int --bool core.big 2>error &&
+	test_i18ngrep "only one type at a time" error
+'
+
+test_expect_success 'non-identical mixed --type specifiers are not allowed' '
+	test_must_fail git config --type=int --bool core.big 2>error &&
+	test_i18ngrep "only one type at a time" error
+'
+
 test_expect_success '--type allows valid type specifiers' '
 	echo "true" >expect &&
 	git config --type=bool core.foo >actual &&
@@ -1635,6 +1663,12 @@ test_expect_success '--no-type unsets type specifiers' '
 	test_cmp expect actual
 '

+test_expect_success 'unset type specifiers may be reset to conflicting ones' '
+	echo 1048576 >expect &&
+	git config --type=bool --no-type --type=int core.big >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 "unrecognized --type argument" error
--
2.17.0