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

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
`--int`, `--bool`, and etc. This allows the aforementioned other patch
to add `--color` (in the non-traditional sense) via `--type=color`,
instead of `--color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 66 +++++++++++++++++++-----------------
 builtin/config.c             | 25 ++++++++++++++
 t/t1300-repo-config.sh       | 21 ++++++++++++
 3 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..a4a5ffb41 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type] --add name value
+'git config' [<file-option>] [--type] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier may be given as an argument to `--type` to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations are
+performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,34 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type [type]::
+  'git config' will ensure that any input output is valid under the given type
+  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
+  form.
++
+Valid `[type]`'s include:
++
+- 'bool': canonicalize  values as either "true" or "false".
+- 'int': canonicalize  values as simple decimla numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative ate-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..ea7923493 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, types;
+static char *type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -84,6 +85,7 @@ 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_STRING('t', "type", &type, N_("type"), N_("value is given this type")),
 	OPT_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
 	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
 	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
@@ -493,6 +495,21 @@ static char *default_user_config(void)
 	return strbuf_detach(&buf, NULL);
 }
 
+static int type_name_to_specifier(char *name)
+{
+	if (!(strcmp(name, "bool")))
+		return TYPE_BOOL;
+	else if (!(strcmp(name, "int")))
+		return TYPE_INT;
+	else if (!(strcmp(name, "bool-or-int")))
+		return TYPE_BOOL_OR_INT;
+	else if (!(strcmp(name, "path")))
+		return TYPE_PATH;
+	else if (!(strcmp(name, "expiry-date")))
+		return TYPE_EXPIRY_DATE;
+	die(_("unexpected --type argument, %s"), name);
+}
+
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
@@ -601,6 +618,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (type) {
+		if (types != 0) {
+			error("usage of --type is ambiguous");
+			usage_with_options(builtin_config_usage, builtin_config_options);
+		}
+		types = type_name_to_specifier(type);
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fd..12dc94bd2 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,25 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+foo = true
+EOF
+
+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 '--type does not allow for ambiguity' '
+	test_must_fail git config --type=bool --bool core.foo 2>error &&
+	test_i18ngrep "usage of --type is ambiguous" error
+'
+
 test_done
-- 
2.16.2.440.gc6284da4f

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values are satisfiable under that type, and (2) that outgoing values are
> canonicalized under that type.
>
> In another series, we propose to add extend this functionality with
> `--color` and `--default` to replace `--get-color`.
>
> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".
>
> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid inhabiting this option too soon, so that
> `git config` can support `--color` (in the traditional sense) in the
> future, if that is desired.

OK.

> In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> `--int`, `--bool`, and etc. This allows the aforementioned other patch
> to add `--color` (in the non-traditional sense) via `--type=color`,
> instead of `--color`.

OK, it's not just "we prefer" but we add this new "--type" thing, so
that we do not have to worry about having to make --color and other
"types" into double-dash options.  Makes good sense, I would guess.

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

[Jeff King]

On Wed, Mar 28, 2018 at 04:47:19PM -0700, Taylor Blau wrote:

> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values are satisfiable under that type, and (2) that outgoing values are
> canonicalized under that type.
> 
> In another series, we propose to add extend this functionality with
> `--color` and `--default` to replace `--get-color`.
> 
> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".
> 
> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid inhabiting this option too soon, so that
> `git config` can support `--color` (in the traditional sense) in the
> future, if that is desired.
> 
> In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> `--int`, `--bool`, and etc. This allows the aforementioned other patch
> to add `--color` (in the non-traditional sense) via `--type=color`,
> instead of `--color`.

Makes sense. I agree with promoting --type as the correct way going
forward, since it will grow new types, whereas we can stop adding
"--foo" aliases for "--type=foo".

> +Valid `[type]`'s include:
> ++
> +- 'bool': canonicalize  values as either "true" or "false".
> +- 'int': canonicalize  values as simple decimla numbers. An optional suffix of
> +  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
> +  1073741824 prior to output.
> +- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
> +  above.
> +- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
> +  `~user` to the home directory for the specified user. This specifier has no
> +  effect when setting the value (but you can use `git config section.variable
> +  ~/` from the command line to let your shell do the expansion.)
> +- 'expiry-date': canonicalize by converting from a fixed or relative ate-string
> +  to a timestamp. This specifier has no effect when setting the value.
> ++

Yay. It's nice to have this in only one place now.

s/ate-string/d&/ :)

> +static int type_name_to_specifier(char *name)
> +{
> +	if (!(strcmp(name, "bool")))
> +		return TYPE_BOOL;

We'd usually drop the extra level of parentheses, and just write:

  if (!strcmp(name, "bool"))

> @@ -601,6 +618,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		usage_with_options(builtin_config_usage, builtin_config_options);
>  	}
>  
> +	if (type) {
> +		if (types != 0) {
> +			error("usage of --type is ambiguous");
> +			usage_with_options(builtin_config_usage, builtin_config_options);
> +		}
> +		types = type_name_to_specifier(type);
> +	}

This error message left me scratching my head for a minute. Ambiguous
how? I think this is covering the case of:

  git config --int --type=bool

So maybe "--type cannot be used with other type options" or something?

Let's take a step back, though. As part of this, should we convert the
parsing of type options to last-one-wins? The fact that they are all
OPT_BIT() is quite silly, since you cannot have more than one bit set.
So if you do:

  git config --int --bool

you get an error. Whereas normal behavior for most options would be for
--bool to override --int. And that is what happens with:

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

I don't think there are any backwards compatibility issues to deal with
here; we'd be changing a case which is now always an error.

And then after that, you truly can make (and document, if we want) that
"--int" is a true synonym for "--type=int".

I think it would be pretty simple. One of:

  - convert OPT_BIT("bool") into OPT_CALLBACK("bool") and just assign
    "bool" to the "type" string, which will then later get parsed into
    TYPE_BOOL.

or

  - convert OPT_BIT("bool") into OPT_SET_INT("bool") to set TYPE_BOOL
    directly. Convert OPT_STRING("type") into OPT_CALLBACK(), and have
    it assign the result of type_name_to_specifier() directly.

I'd probably do the latter, but would be fine with either (and I'd make
the OPT_SET_INT thing its own preparatory patch).

If you really want to go all-out, I think the ACTION flags could use the
same cleanup. We treat them as bitflags, and then issue an error when
you set more than one, which is just silly.

-Peff

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

[Taylor Blau]

On Thu, Mar 29, 2018 at 06:11:22PM -0400, Jeff King wrote:
> > +Valid `[type]`'s include:
> > ++
> > +- 'bool': canonicalize  values as either "true" or "false".
> > +- 'int': canonicalize  values as simple decimla numbers. An optional suffix of
> > +  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
> > +  1073741824 prior to output.
> > +- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
> > +  above.
> > +- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
> > +  `~user` to the home directory for the specified user. This specifier has no
> > +  effect when setting the value (but you can use `git config section.variable
> > +  ~/` from the command line to let your shell do the expansion.)
> > +- 'expiry-date': canonicalize by converting from a fixed or relative ate-string
> > +  to a timestamp. This specifier has no effect when setting the value.
> > ++
>
> Yay. It's nice to have this in only one place now.

Thanks! Agreed :-).

> s/ate-string/d&/ :)

Ack.

My excuse for this is that I have started using iTerm2.app with Vim in
my terminal using the new graphics acceleration options, and have had
trouble getting Vim to render the underline for misspelled words
consistently.

> > +static int type_name_to_specifier(char *name)
> > +{
> > +	if (!(strcmp(name, "bool")))
> > +		return TYPE_BOOL;
>
> We'd usually drop the extra level of parentheses, and just write:
>
>   if (!strcmp(name, "bool"))

Sounds good; I have adjusted this in the appropriate location in v2.

> > @@ -601,6 +618,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> >  		usage_with_options(builtin_config_usage, builtin_config_options);
> >  	}
> >
> > +	if (type) {
> > +		if (types != 0) {
> > +			error("usage of --type is ambiguous");
> > +			usage_with_options(builtin_config_usage, builtin_config_options);
> > +		}
> > +		types = type_name_to_specifier(type);
> > +	}
>
> This error message left me scratching my head for a minute. Ambiguous
> how? I think this is covering the case of:
>
>   git config --int --type=bool
>
> So maybe "--type cannot be used with other type options" or something?
>
> Let's take a step back, though. As part of this, should we convert the
> parsing of type options to last-one-wins? The fact that they are all
> OPT_BIT() is quite silly, since you cannot have more than one bit set.
> So if you do:
>
>   git config --int --bool
>
> you get an error. Whereas normal behavior for most options would be for
> --bool to override --int. And that is what happens with:
>
>   git config --type=int --type=bool
>
> I don't think there are any backwards compatibility issues to deal with
> here; we'd be changing a case which is now always an error.

Agreed.

> And then after that, you truly can make (and document, if we want) that
> "--int" is a true synonym for "--type=int".

Great point; for me this is the primary motivating factor for making
this change. In addition to simplifying our work when we check:

  if (types != 0 && type_str) { ... }

it would be nice not to have to compare the two in order to ensure that
they are the same, continuing on if so, and causing an error if not.

> I think it would be pretty simple. One of:
>
>   - convert OPT_BIT("bool") into OPT_CALLBACK("bool") and just assign
>     "bool" to the "type" string, which will then later get parsed into
>     TYPE_BOOL.
>
> or
>
>   - convert OPT_BIT("bool") into OPT_SET_INT("bool") to set TYPE_BOOL
>
>     directly. Convert OPT_STRING("type") into OPT_CALLBACK(), and have
>     it assign the result of type_name_to_specifier() directly.
>
> I'd probably do the latter, but would be fine with either (and I'd make
> the OPT_SET_INT thing its own preparatory patch).

I adopted your advice and used the later, converting each `OPT_BIT` into
an `OPT_SET_INT`, and adding a callback for `--type`, which does _not_
complain if `type` is already non-zero.

> If you really want to go all-out, I think the ACTION flags could use the
> same cleanup. We treat them as bitflags, and then issue an error when
> you set more than one, which is just silly.

Agreed, and I think that this is a good candidate for a future patch.
Thoughts? :-).


Thanks,
Taylor

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

[Jeff King]

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

> > If you really want to go all-out, I think the ACTION flags could use the
> > same cleanup. We treat them as bitflags, and then issue an error when
> > you set more than one, which is just silly.
> 
> Agreed, and I think that this is a good candidate for a future patch.
> Thoughts? :-).

I actually worked this up for fun, though I had second thoughts while
writing the commit message.

Besides the cleanup, my primary motivation was following last-one-wins
rules as we often do elsewhere. But actually, last-one-wins applies only
to a _single_ option, not necessarily unrelated ones. Many other
multi-action commands actually have a series of separate boolean flags,
and then complain when more than one of the flags is set.

So maybe it's not such a good idea for the actions (I do still think
it's the right path for the types).

For reference, here's the patch I wrote:

 builtin/config.c | 137 +++++++++++++++++++++++++----------------------
 1 file changed, 72 insertions(+), 65 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd628..5581f48ac8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,35 +25,36 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int types;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
 
-#define ACTION_GET (1<<0)
-#define ACTION_GET_ALL (1<<1)
-#define ACTION_GET_REGEXP (1<<2)
-#define ACTION_REPLACE_ALL (1<<3)
-#define ACTION_ADD (1<<4)
-#define ACTION_UNSET (1<<5)
-#define ACTION_UNSET_ALL (1<<6)
-#define ACTION_RENAME_SECTION (1<<7)
-#define ACTION_REMOVE_SECTION (1<<8)
-#define ACTION_LIST (1<<9)
-#define ACTION_EDIT (1<<10)
-#define ACTION_SET (1<<11)
-#define ACTION_SET_ALL (1<<12)
-#define ACTION_GET_COLOR (1<<13)
-#define ACTION_GET_COLORBOOL (1<<14)
-#define ACTION_GET_URLMATCH (1<<15)
-
+enum config_action {
+	ACTION_NONE = 0,
+	ACTION_GET,
+	ACTION_GET_ALL,
+	ACTION_GET_REGEXP,
+	ACTION_REPLACE_ALL,
+	ACTION_ADD,
+	ACTION_UNSET,
+	ACTION_UNSET_ALL,
+	ACTION_RENAME_SECTION,
+	ACTION_REMOVE_SECTION,
+	ACTION_LIST,
+	ACTION_EDIT,
+	ACTION_SET,
+	ACTION_SET_ALL,
+	ACTION_GET_COLOR,
+	ACTION_GET_COLORBOOL,
+	ACTION_GET_URLMATCH,
+};
 /*
- * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
- * one line of output and which should therefore be paged.
+ * This must be an int and not an enum because we pass it by address
+ * to OPT_SETINT.
  */
-#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
-			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
+static int action;
 
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
@@ -69,20 +70,20 @@ static struct option builtin_config_options[] = {
 	OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),
 	OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")),
 	OPT_GROUP(N_("Action")),
-	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
-	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
-	OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-regex]"), ACTION_GET_REGEXP),
-	OPT_BIT(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
-	OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value_regex]"), ACTION_REPLACE_ALL),
-	OPT_BIT(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
-	OPT_BIT(0, "unset", &actions, N_("remove a variable: name [value-regex]"), ACTION_UNSET),
-	OPT_BIT(0, "unset-all", &actions, N_("remove all matches: name [value-regex]"), ACTION_UNSET_ALL),
-	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
-	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
-	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
-	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
-	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_SET_INT(0, "get", &action, N_("get value: name [value-regex]"), ACTION_GET),
+	OPT_SET_INT(0, "get-all", &action, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
+	OPT_SET_INT(0, "get-regexp", &action, N_("get values for regexp: name-regex [value-regex]"), ACTION_GET_REGEXP),
+	OPT_SET_INT(0, "get-urlmatch", &action, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
+	OPT_SET_INT(0, "replace-all", &action, N_("replace all matching variables: name value [value_regex]"), ACTION_REPLACE_ALL),
+	OPT_SET_INT(0, "add", &action, N_("add a new variable: name value"), ACTION_ADD),
+	OPT_SET_INT(0, "unset", &action, N_("remove a variable: name [value-regex]"), ACTION_UNSET),
+	OPT_SET_INT(0, "unset-all", &action, N_("remove all matches: name [value-regex]"), ACTION_UNSET_ALL),
+	OPT_SET_INT(0, "rename-section", &action, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
+	OPT_SET_INT(0, "remove-section", &action, N_("remove a section: name"), ACTION_REMOVE_SECTION),
+	OPT_SET_INT('l', "list", &action, N_("list all"), ACTION_LIST),
+	OPT_SET_INT('e', "edit", &action, N_("open an editor"), ACTION_EDIT),
+	OPT_SET_INT(0, "get-color", &action, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
+	OPT_SET_INT(0, "get-colorbool", &action, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
 	OPT_GROUP(N_("Type")),
 	OPT_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
 	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
@@ -571,40 +572,46 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if (types &&
+	    (action == ACTION_GET_COLOR ||
+	     action == ACTION_GET_COLORBOOL)) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
-	if (HAS_MULTI_BITS(actions)) {
-		error("only one action at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-	if (actions == 0)
+	if (action == ACTION_NONE) {
 		switch (argc) {
-		case 1: actions = ACTION_GET; break;
-		case 2: actions = ACTION_SET; break;
-		case 3: actions = ACTION_SET_ALL; break;
+		case 1: action = ACTION_GET; break;
+		case 2: action = ACTION_SET; break;
+		case 3: action = ACTION_SET_ALL; break;
 		default:
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
+	}
 	if (omit_values &&
-	    !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) {
+	    !(action == ACTION_LIST ||
+	      action == ACTION_GET_REGEXP)) {
 		error("--name-only is only applicable to --list or --get-regexp");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
-	if (show_origin && !(actions &
-		(ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) {
+	if (show_origin &&
+	    !(action == ACTION_GET ||
+	      action == ACTION_GET_ALL ||
+	      action == ACTION_GET_REGEXP ||
+	      action == ACTION_LIST)) {
 		error("--show-origin is only applicable to --get, --get-all, "
 			  "--get-regexp, and --list.");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
-	if (actions & PAGING_ACTIONS)
+	if (action == ACTION_LIST ||
+	    action == ACTION_GET_ALL ||
+	    action == ACTION_GET_REGEXP ||
+	    action == ACTION_GET_URLMATCH)
 		setup_auto_pager("config", 1);
 
-	if (actions == ACTION_LIST) {
+	if (action == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (config_with_options(show_all_config, NULL,
 					&given_config_source,
@@ -616,7 +623,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 				die("error processing config file(s)");
 		}
 	}
-	else if (actions == ACTION_EDIT) {
+	else if (action == ACTION_EDIT) {
 		char *config_file;
 
 		check_argc(argc, 0, 0);
@@ -644,7 +651,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		launch_editor(config_file, NULL, NULL);
 		free(config_file);
 	}
-	else if (actions == ACTION_SET) {
+	else if (action == ACTION_SET) {
 		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
@@ -656,7 +663,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			"       Use a regexp, --add or --replace-all to change %s."), argv[0]);
 		return ret;
 	}
-	else if (actions == ACTION_SET_ALL) {
+	else if (action == ACTION_SET_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
@@ -664,7 +671,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value, argv[2], 0);
 	}
-	else if (actions == ACTION_ADD) {
+	else if (action == ACTION_ADD) {
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
@@ -673,7 +680,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 							      argv[0], value,
 							      CONFIG_REGEX_NONE, 0);
 	}
-	else if (actions == ACTION_REPLACE_ALL) {
+	else if (action == ACTION_REPLACE_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
@@ -681,27 +688,27 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value, argv[2], 1);
 	}
-	else if (actions == ACTION_GET) {
+	else if (action == ACTION_GET) {
 		check_argc(argc, 1, 2);
 		return get_value(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_GET_ALL) {
+	else if (action == ACTION_GET_ALL) {
 		do_all = 1;
 		check_argc(argc, 1, 2);
 		return get_value(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_GET_REGEXP) {
+	else if (action == ACTION_GET_REGEXP) {
 		show_keys = 1;
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
 		return get_value(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_GET_URLMATCH) {
+	else if (action == ACTION_GET_URLMATCH) {
 		check_argc(argc, 2, 2);
 		return get_urlmatch(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_UNSET) {
+	else if (action == ACTION_UNSET) {
 		check_write();
 		check_argc(argc, 1, 2);
 		if (argc == 2)
@@ -711,13 +718,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			return git_config_set_in_file_gently(given_config_source.file,
 							     argv[0], NULL);
 	}
-	else if (actions == ACTION_UNSET_ALL) {
+	else if (action == ACTION_UNSET_ALL) {
 		check_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], NULL, argv[1], 1);
 	}
-	else if (actions == ACTION_RENAME_SECTION) {
+	else if (action == ACTION_RENAME_SECTION) {
 		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
@@ -728,7 +735,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (ret == 0)
 			die("No such section!");
 	}
-	else if (actions == ACTION_REMOVE_SECTION) {
+	else if (action == ACTION_REMOVE_SECTION) {
 		int ret;
 		check_write();
 		check_argc(argc, 1, 1);
@@ -739,11 +746,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (ret == 0)
 			die("No such section!");
 	}
-	else if (actions == ACTION_GET_COLOR) {
+	else if (action == ACTION_GET_COLOR) {
 		check_argc(argc, 1, 2);
 		get_color(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_GET_COLORBOOL) {
+	else if (action == ACTION_GET_COLORBOOL) {
 		check_argc(argc, 1, 2);
 		if (argc == 2)
 			color_stdout_is_tty = git_config_bool("command line", argv[1]);
-- 
2.17.0.rc2.594.gdb94a0ce02

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... But actually, last-one-wins applies only
> to a _single_ option, not necessarily unrelated ones. Many other
> multi-action commands actually have a series of separate boolean flags,
> and then complain when more than one of the flags is set.
>
> So maybe it's not such a good idea for the actions (I do still think
> it's the right path for the types).

If this were using command verbs (e.g. "git config get foo.bar") as
opposed to command options (e.g. "git config --get foo.bar"), it
wouldn't ahve allowed multiple command verbs from the command line,
and last-one-wins would not have made much sense because there is no
way to trigger it as a desirable "feature".

Just like the topic of the discussion unifies --int/--bool/etc. into
a single --type={int,bool,...}, perhaps the existing command options
--get/--list/etc. can be taken as if they were a mistaken historical
way to spell --action={get,list,...}.  I of course am not recommending
to add a new "--action" option.  I am suggesting it as a thought-aid
to see if actions are all that different from value type options.

I agree that a-bit-per-type that is checked with HAS_MULTI_BITS()
for error at the end does not make much sense.  I also think what
you did in this patch for actions is a good clean-up for the above
reason.

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

[Jeff King]

On Fri, Mar 30, 2018 at 09:00:05AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... But actually, last-one-wins applies only
> > to a _single_ option, not necessarily unrelated ones. Many other
> > multi-action commands actually have a series of separate boolean flags,
> > and then complain when more than one of the flags is set.
> >
> > So maybe it's not such a good idea for the actions (I do still think
> > it's the right path for the types).
> 
> If this were using command verbs (e.g. "git config get foo.bar") as
> opposed to command options (e.g. "git config --get foo.bar"), it
> wouldn't ahve allowed multiple command verbs from the command line,
> and last-one-wins would not have made much sense because there is no
> way to trigger it as a desirable "feature".
> 
> Just like the topic of the discussion unifies --int/--bool/etc. into
> a single --type={int,bool,...}, perhaps the existing command options
> --get/--list/etc. can be taken as if they were a mistaken historical
> way to spell --action={get,list,...}.  I of course am not recommending
> to add a new "--action" option.  I am suggesting it as a thought-aid
> to see if actions are all that different from value type options.
> 
> I agree that a-bit-per-type that is checked with HAS_MULTI_BITS()
> for error at the end does not make much sense.  I also think what
> you did in this patch for actions is a good clean-up for the above
> reason.

I agree the code internally is nicer after the patch. If we throw away
the argument of "last-one-wins is more consistent with other parts of
git" (because I don't really think that it is for this type of option),
I could possibly buy this as a code cleanup. But it does have a
user-visible impact, which makes the question: are we _OK_ with
switching to last-one-wins?

-Peff

[PATCH v2 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Where--although the legacy and modern type specifier (`--int`, and
`--type`, respectively) do not conflict--we would store the value of
`--type=` in a `char *` and the `--int` as a bit in the `int` bitset.

In the above, when error-checking `if (types != && type_str)`, we do not
check additionally whether or not these types are the same, and simply
complain immediately. This change makes `--int` and `--type=int` a true
synonym of each other, and removes the need for additional complexity,
as above in the conditional.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 39 +++++++++++++++++----------------------
 t/t1300-repo-config.sh | 11 +++++++++++
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..fd7b36c41 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -84,11 +84,11 @@ 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_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	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_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")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fd..aa45b9267 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 > expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.16.2.440.gc6284da4f

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
`--int`, `--bool`, and etc. This allows the aforementioned other patch
to add `--color` (in the non-traditional sense) via `--type=color`,
instead of `--color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 66 +++++++++++++++++++-----------------
 builtin/config.c             | 23 +++++++++++++
 t/t1300-repo-config.sh       | 18 ++++++++++
 3 files changed, 75 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..9956b03f7 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type] --add name value
+'git config' [<file-option>] [--type] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier may be given as an argument to `--type` to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations are
+performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,34 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type [type]::
+  'git config' will ensure that any input output is valid under the given type
+  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
+  form.
++
+Valid `[type]`'s include:
++
+- 'bool': canonicalize  values as either "true" or "false".
+- 'int': canonicalize  values as simple decimla numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index fd7b36c41..8833f928a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,28 @@ static int show_origin;
 #define TYPE_PATH (1<<3)
 #define TYPE_EXPIRY_DATE (1<<4)
 
+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;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,6 +106,7 @@ 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),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index aa45b9267..8c5210fdf 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '
 
 cat >.git/config <<-\EOF &&
 [core]
+foo = true
 number = 10
 EOF
 
@@ -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
+'
+
 test_done
-- 
2.16.2.440.gc6284da4f

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

[René Scharfe]

Am 30.03.2018 um 07:28 schrieb Taylor Blau:
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index e09ed5d7d..9956b03f7 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -160,30 +158,34 @@ See also <<FILES>>.
>   --list::
>   	List all variables set in config file, along with their values.
>   
> ---bool::
> -	'git config' will ensure that the output is "true" or "false"
> +--type [type]::
> +  'git config' will ensure that any input output is valid under the given type
> +  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
> +  form.
> ++
> +Valid `[type]`'s include:
> ++
> +- 'bool': canonicalize  values as either "true" or "false".
> +- 'int': canonicalize  values as simple decimla numbers. An optional suffix of

s/ize  val/ize val/
s/decimla/decimal/

René

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

[Jeff King]

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

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

> @@ -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

Re: [PATCH v2 1/2] builtin/config.c: treat type specifiers singularly

[Jeff King]

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

> Internally, we represent `git config`'s type specifiers as a bitset
> using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
> allows for the representation of multiple type specifiers in the `int
> types` field, but this multi-representation is left unused.
> 
> In fact, `git config` will not accept multiple type specifiers at a
> time, as indicated by:
> 
>   $ git config --int --bool some.section
>   error: only one type at a time.
> 
> This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
> specifier, so that the above command would instead be valid, and a
> synonym of:
> 
>   $ git config --bool some.section
> 
> This change is motivated by two urges: (1) it does not make sense to
> represent a singular type specifier internally as a bitset, only to
> complain when there are multiple bits in the set. `OPT_SET_INT` is more
> well-suited to this task than `OPT_BIT` is. (2) a future patch will
> introduce `--type=<type>`, and we would like not to complain in the
> following situation:
> 
>   $ git config --int --type=int

I think you could just end here, since...

> Where--although the legacy and modern type specifier (`--int`, and
> `--type`, respectively) do not conflict--we would store the value of
> `--type=` in a `char *` and the `--int` as a bit in the `int` bitset.
> 
> In the above, when error-checking `if (types != && type_str)`, we do not
> check additionally whether or not these types are the same, and simply
> complain immediately. This change makes `--int` and `--type=int` a true
> synonym of each other, and removes the need for additional complexity,
> as above in the conditional.

None of this type_str stuff exists yet, and you've sufficiently
motivated the change.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/config.c       | 39 +++++++++++++++++----------------------
>  t/t1300-repo-config.sh | 11 +++++++++++
>  2 files changed, 28 insertions(+), 22 deletions(-)

The patch mostly looks good. We probably want to also rewrite the TYPE_*
#defines to be sequential, since somebody may scratch their head
wondering why we use one bit per type when we do not treat them as a
bitfield.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 4f8e6f5fd..aa45b9267 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
>  	test_expect_code 128 nongit git config --local foo.bar
>  '
>  
> +cat >.git/config <<-\EOF &&
> +[core]
> +number = 10
> +EOF
> +
> +test_expect_success 'later legacy specifiers are given precedence' '
> +	git config --bool --int core.number >actual &&
> +	echo 10 > expect &&
> +	test_cmp expect actual
> +'

Minor style nit: we prefer ">expect" with no space. Though t1300 is
certainly a cornucopia of style violations already. I could buy it under
the guise of matching existing style if there wasn't a counter-example
directly above. :)

We also prefer to put that "cat >.git/config" inside a test block,
though I'm pretty sure that _is_ following existing style in the test.

-Peff

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

[Taylor Blau]

Hi,

Here is a v3 of a series to (1) treat type specifiers in `git config` as
singulars rather than a collection of bits, and (2) to prefer --type=<t> over
--<t>.

I have made the following changes since v2:

  - Fix some misspellings in Documentation/git-config.txt (cc: René).
  - Handle --no-type correctly (cc: Peff).
  - No longer prefer (1 << x) style for enumerating type specifiers (cc: Peff).
  - Fix awkward rebase (cc: Peff).

Thanks as always for your review :-).


Thanks,
Taylor

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: prefer `--type=bool` over `--bool`, etc.

 Documentation/git-config.txt | 66 ++++++++++++++++---------------
 builtin/config.c             | 77 +++++++++++++++++++++++-------------
 t/t1300-repo-config.sh       | 29 ++++++++++++++
 3 files changed, 113 insertions(+), 59 deletions(-)

--
2.16.2.440.gc6284da4f

[PATCH v3 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..92fb8d56b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ 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_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	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_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")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fd..24de37d54 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.16.2.440.gc6284da4f

Re: [PATCH v3 1/2] builtin/config.c: treat type specifiers singularly

[Eric Sunshine]

On Wed, Apr 4, 2018 at 2:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> In fact, `git config` will not accept multiple type specifiers at a
> time, as indicated by:
>
>   $ git config --int --bool some.section
>   error: only one type at a time.
>
> This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
> specifier, so that the above command would instead be valid, and a
> synonym of:
>
>   $ git config --bool some.section
>
> This change is motivated by two urges: (1) it does not make sense to
> represent a singular type specifier internally as a bitset, only to
> complain when there are multiple bits in the set. `OPT_SET_INT` is more
> well-suited to this task than `OPT_BIT` is. (2) a future patch will
> introduce `--type=<type>`, and we would like not to complain in the
> following situation:
>
>   $ git config --int --type=int

I can understand "last wins" for related options such as "--verbose
--quiet" or "--verbose=1 --verbose=2", but I have a lot of trouble
buying the "last wins" argument in this context where there is no
semantic relationship between, say, --bool and --expiry-date.
Specifying conflicting options together is likely to be unintentional,
thus reporting it as an error seems sensible, so I'm not convinced
that patch's removal of that error is a good idea.

I understand that you're doing this to avoid complaining about "--int
--type=int", but exactly how that case is supported should be an
implementation detail; it doesn't need to bleed into the UI as an
unnecessary and not-well-justified loosening of an age-old
restriction. There are other ways to support "--int --type=int"
without "last wins". Also, do we really need to support "--int
--type=int"? Is that an expected use-case?

Re: [PATCH v3 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

On Wed, Apr 04, 2018 at 03:57:12AM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 2:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > [...]
> > In fact, `git config` will not accept multiple type specifiers at a
> > time, as indicated by:
> >
> >   $ git config --int --bool some.section
> >   error: only one type at a time.
> >
> > This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
> > specifier, so that the above command would instead be valid, and a
> > synonym of:
> >
> >   $ git config --bool some.section
> >
> > This change is motivated by two urges: (1) it does not make sense to
> > represent a singular type specifier internally as a bitset, only to
> > complain when there are multiple bits in the set. `OPT_SET_INT` is more
> > well-suited to this task than `OPT_BIT` is. (2) a future patch will
> > introduce `--type=<type>`, and we would like not to complain in the
> > following situation:
> >
> >   $ git config --int --type=int
>
> I can understand "last wins" for related options such as "--verbose
> --quiet" or "--verbose=1 --verbose=2", but I have a lot of trouble
> buying the "last wins" argument in this context where there is no
> semantic relationship between, say, --bool and --expiry-date.
> Specifying conflicting options together is likely to be unintentional,
> thus reporting it as an error seems sensible, so I'm not convinced
> that patch's removal of that error is a good idea.
>
> I understand that you're doing this to avoid complaining about "--int
> --type=int", but exactly how that case is supported should be an
> implementation detail; it doesn't need to bleed into the UI as an
> unnecessary and not-well-justified loosening of an age-old
> restriction. There are other ways to support "--int --type=int"
> without "last wins". Also, do we really need to support "--int
> --type=int"? Is that an expected use-case?

This is a fair concern, though I view the problem slightly differently.
I see this change as being motivated by the fact that we are adding
"--type", not removing an age-old restriction.

Peff's motivation for this--as I understand it--is that "--type=int"
should be a _true_ synonym for "--int". Adopting the old-style
"OPT_SET_BIT" for this purpose makes "--type=int" and "--int" _mostly_ a
synonym for one another, except that "--type=bool --type=int" will not
complain, whereas "--bool --int" would.

Loosening this restriction, in my view, brings us closer (1) to the new
semantics of multiple "--type"'s, and (2) to the existing semantics of
"--verbose=1 --verbose=2" as you mentioned above.

I would like to hear Peff's take on this as well, since he suggested the
patch originally, and would likely provide the clearest insight into
this.


Thanks,
Taylor

Re: [PATCH v3 1/2] builtin/config.c: treat type specifiers singularly

[Jeff King]

On Wed, Apr 04, 2018 at 06:53:04PM -0700, Taylor Blau wrote:

> > I understand that you're doing this to avoid complaining about "--int
> > --type=int", but exactly how that case is supported should be an
> > implementation detail; it doesn't need to bleed into the UI as an
> > unnecessary and not-well-justified loosening of an age-old
> > restriction. There are other ways to support "--int --type=int"
> > without "last wins". Also, do we really need to support "--int
> > --type=int"? Is that an expected use-case?
> 
> This is a fair concern, though I view the problem slightly differently.
> I see this change as being motivated by the fact that we are adding
> "--type", not removing an age-old restriction.
> 
> Peff's motivation for this--as I understand it--is that "--type=int"
> should be a _true_ synonym for "--int". Adopting the old-style
> "OPT_SET_BIT" for this purpose makes "--type=int" and "--int" _mostly_ a
> synonym for one another, except that "--type=bool --type=int" will not
> complain, whereas "--bool --int" would.
> 
> Loosening this restriction, in my view, brings us closer (1) to the new
> semantics of multiple "--type"'s, and (2) to the existing semantics of
> "--verbose=1 --verbose=2" as you mentioned above.
> 
> I would like to hear Peff's take on this as well, since he suggested the
> patch originally, and would likely provide the clearest insight into
> this.

I think you've captured it fairly well.  The options _are_ semantically
linked, in that they are all mutually-exclusive types. Obviously we
could continue to flag errors, and even catch "--type=int --type=bool"
in the same way if we really wanted to (by using a custom parse-options
callback).

So I think the primary value here is in the code cleanup. Even without
the new "--type" option, avoiding the bitset makes the code clearer
(IMHO).

I do agree that a user saying "--int --bool" is almost certainly an
error, and they'd be just as happy to see an error message as to get the
last-one-wins behavior. But I also doubt that it comes up very much
either way.

-Peff

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
`--int`, `--bool`, and etc. This allows the aforementioned other patch
to add `--color` (in the non-traditional sense) via `--type=color`,
instead of `--color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 66 +++++++++++++++++++-----------------
 builtin/config.c             | 28 +++++++++++++++
 t/t1300-repo-config.sh       | 18 ++++++++++
 3 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..869999c9b 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type] --add name value
+'git config' [<file-option>] [--type] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier may be given as an argument to `--type` to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations are
+performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,34 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type [type]::
+  'git config' will ensure that any input output is valid under the given type
+  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
+  form.
++
+Valid `[type]`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b..942fcf4bd 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,33 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
 
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	int *type = opt->value;
+
+	if (unset) {
+		*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(_("unexpected --type argument, %s"), arg);
+		return 1;
+	}
+	return 0;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,6 +111,7 @@ 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),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 24de37d54..5f9a83725 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '
 
 cat >.git/config <<-\EOF &&
 [core]
+foo = true
 number = 10
 EOF
 
@@ -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 '--no-type unsets type specifiers' '
+	echo "10" > expect &&
+	git config --type=bool --no-type core.number >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_done
-- 
2.16.2.440.gc6284da4f

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

[Eric Sunshine]

On Wed, Apr 4, 2018 at 2:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values are satisfiable under that type, and (2) that outgoing values are
> canonicalized under that type.
>
> In another series, we propose to add extend this functionality with
> `--color` and `--default` to replace `--get-color`.
>
> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".
>
> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid inhabiting this option too soon, so that
> `git config` can support `--color` (in the traditional sense) in the
> future, if that is desired.
>
> In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> `--int`, `--bool`, and etc. This allows the aforementioned other patch
> to add `--color` (in the non-traditional sense) via `--type=color`,
> instead of `--color`.

I always find this last sentence confusing since it's not clear to
which ilk of "--color" option you refer. Perhaps say instead something
like:

    This normalization will allow the aforementioned upcoming patch to
    support querying a color value with a default via "--type=color
    --default=...".

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -160,30 +158,34 @@ See also <<FILES>>.
> +--type [type]::
> +  'git config' will ensure that any input output is valid under the given type
> +  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
> +  form.

Do the brackets in "[type]" mean that the argument is optional? If so,
what does 'type' default to when not specified? The documentation
should discuss this.

> diff --git a/builtin/config.c b/builtin/config.c
> @@ -61,6 +61,33 @@ static int show_origin;
> +static int option_parse_type(const struct option *opt, const char *arg,
> +                            int unset)
> +{
> +       [...]
> +       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);

"unexpected" doesn't seem like the best word choice since an argument
to --type _is_ expected. Perhaps you mean "unrecognized"?

> +               return 1;
> +       }
> +       return 0;
> +}

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

[Taylor Blau]

On Wed, Apr 04, 2018 at 03:27:48AM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 2:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> > `--int`, `--bool`, and etc. This allows the aforementioned other patch
> > to add `--color` (in the non-traditional sense) via `--type=color`,
> > instead of `--color`.
>
> I always find this last sentence confusing since it's not clear to
> which ilk of "--color" option you refer. Perhaps say instead something
> like:
>
>     This normalization will allow the aforementioned upcoming patch to
>     support querying a color value with a default via "--type=color
>     --default=...".

I agree that this is much clearer. I have amended this patch to include
your wording here.

> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -160,30 +158,34 @@ See also <<FILES>>.
> > +--type [type]::
> > +  'git config' will ensure that any input output is valid under the given type
> > +  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
> > +  form.
>
> Do the brackets in "[type]" mean that the argument is optional? If so,
> what does 'type' default to when not specified? The documentation
> should discuss this.

This is my mistake; I was unaware of the semantic difference between '[]'
and '<>'. If my understanding is correct that '<>' means "required" and
'[]' means "optional", than this is a misspelling of "<type>".

I have addressed this during the re-roll.

> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -61,6 +61,33 @@ static int show_origin;
> > +static int option_parse_type(const struct option *opt, const char *arg,
> > +                            int unset)
> > +{
> > +       [...]
> > +       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);
>
> "unexpected" doesn't seem like the best word choice since an argument
> to --type _is_ expected. Perhaps you mean "unrecognized"?

Sure; I think "unrecognized" is a much better fit. I have updated this
in the re-roll.

Thanks,
Taylor

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

[Taylor Blau]

Hi,

I have attached a fourth re-roll of my series to introduce
"--type=<type>" in "git config", and prefer it to "--<type>".

In particular, since the last update, I have changed the following:

  - Clearer wording in the second patch per Eric's suggestion.

  - Stopped spelling the required argument to "--type=" as "[type]", and
    instead as "<type>" (cc: Eric).

  - Changed "unexpected" to "unrecognized" in the fatal message when we
    don't know how to interpret the argument to "--type".

Thanks,
Taylor

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: prefer `--type=bool` over `--bool`, etc.

 Documentation/git-config.txt | 66 ++++++++++++++++---------------
 builtin/config.c             | 77 +++++++++++++++++++++++-------------
 t/t1300-repo-config.sh       | 29 ++++++++++++++
 3 files changed, 113 insertions(+), 59 deletions(-)

--
2.16.2.440.gc6284da4f

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

[Jeff King]

On Wed, Apr 04, 2018 at 07:00:34PM -0700, Taylor Blau wrote:

> I have attached a fourth re-roll of my series to introduce
> "--type=<type>" in "git config", and prefer it to "--<type>".
> 
> In particular, since the last update, I have changed the following:
> 
>   - Clearer wording in the second patch per Eric's suggestion.
> 
>   - Stopped spelling the required argument to "--type=" as "[type]", and
>     instead as "<type>" (cc: Eric).
> 
>   - Changed "unexpected" to "unrecognized" in the fatal message when we
>     don't know how to interpret the argument to "--type".

This iteration looks good to me, assuming that last-one-wins is still
the direction we want to go. I'm open to the notion that the cleanup is
not worth the change in behavior. It is IMHO, but obviously it's
somewhat subjective.

-Peff

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 05:58:00PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:00:34PM -0700, Taylor Blau wrote:
>
> > I have attached a fourth re-roll of my series to introduce
> > "--type=<type>" in "git config", and prefer it to "--<type>".
> >
> > In particular, since the last update, I have changed the following:
> >
> >   - Clearer wording in the second patch per Eric's suggestion.
> >
> >   - Stopped spelling the required argument to "--type=" as "[type]", and
> >     instead as "<type>" (cc: Eric).
> >
> >   - Changed "unexpected" to "unrecognized" in the fatal message when we
> >     don't know how to interpret the argument to "--type".
>
> This iteration looks good to me, assuming that last-one-wins is still
> the direction we want to go. I'm open to the notion that the cleanup is
> not worth the change in behavior. It is IMHO, but obviously it's
> somewhat subjective.

I am too, unless people on this thread have strong feelings otherwise.

Thanks,
Taylor

[PATCH v4 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..92fb8d56b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ 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_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	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_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")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fd..24de37d54 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.16.2.440.gc6284da4f

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
`--int`, `--bool`, and etc. This allows the aforementioned upcoming
patch to support querying a color value with a default via `--type=color
--default=....`

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 66 +++++++++++++++++++-----------------
 builtin/config.c             | 28 +++++++++++++++
 t/t1300-repo-config.sh       | 18 ++++++++++
 3 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..c7e56e3fd 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type] --add name value
+'git config' [<file-option>] [--type] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier may be given as an argument to `--type` to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations are
+performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,34 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input output is valid under the given type
+  constraint(s), and will canonicalize outgoing values in `<type>`'s canonical
+  form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b..6e1495eac 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,33 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
 
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	int *type = opt->value;
+
+	if (unset) {
+		*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);
+		return 1;
+	}
+	return 0;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,6 +111,7 @@ 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),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 24de37d54..b25ab7b9e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '
 
 cat >.git/config <<-\EOF &&
 [core]
+foo = true
 number = 10
 EOF
 
@@ -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 '--no-type unsets type specifiers' '
+	echo "10" > expect &&
+	git config --type=bool --no-type core.number >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
+'
+
 test_done
-- 
2.16.2.440.gc6284da4f

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

[Eric Sunshine]

On Wed, Apr 4, 2018 at 10:00 PM, Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> `--int`, `--bool`, and etc. This allows the aforementioned upcoming
> patch to support querying a color value with a default via `--type=color
> --default=....`
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
> +A type specifier may be given as an argument to `--type` to make 'git config'

In [1], you said that the argument to --type is required, so use of
"may be given" here is ambiguous; it makes it sound as if the argument
is optional. Perhaps rewrite something like:

    The --type option requests `git config` to ...

Not necessarily worth a re-roll, though. (But if you do need to
re-roll for some reason, it might make sense to combine this series
with the --default series to make it slightly easier to review them
together -- since the one depends upon the other -- and probably ease
the burden on Junio slightly.)

> +ensure that the variable(s) are of the given type and convert the value to the
> +canonical form. If no type specifier is passed, no checks or transformations are
> +performed on the value.
> @@ -160,30 +158,34 @@ See also <<FILES>>.
>  --list::
>         List all variables set in config file, along with their values.
>
> ---bool::
> -       'git config' will ensure that the output is "true" or "false"
> +--type <type>::
> +  'git config' will ensure that any input output is valid under the given type
> +  constraint(s), and will canonicalize outgoing values in `<type>`'s canonical
> +  form.

In response to my question[2] about whether the typesetting "[type]"
meant that it was optional, you responded[1] that it was not, thus
correctly changed the typesetting to "<type>". However...

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' '
> +test_expect_success '--no-type unsets type specifiers' '
> +       echo "10" > expect &&
> +       git config --type=bool --no-type core.number >actual &&
> +       test_cmp expect actual
> +'

What does --no-type mean and why is it being tested? If this is an
explicitly supported user-facing option, should it be documented? If
it's not meant to be user-facing, then why are we enforcing its
presence and behavior via a test?

[1]: https://public-inbox.org/git/20180405014758.GA4671@syl.local/
[2]: https://public-inbox.org/git/CAPig+cR4uFiC_gFsb2e9JR6SdP-wUQVz-E0MjRHR=vNHd+hvhA@mail.gmail.com/

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

[Jeff King]

On Thu, Apr 05, 2018 at 06:29:18PM -0400, Eric Sunshine wrote:

> > +ensure that the variable(s) are of the given type and convert the value to the
> > +canonical form. If no type specifier is passed, no checks or transformations are
> > +performed on the value.
> > @@ -160,30 +158,34 @@ See also <<FILES>>.
> >  --list::
> >         List all variables set in config file, along with their values.
> >
> > ---bool::
> > -       'git config' will ensure that the output is "true" or "false"
> > +--type <type>::
> > +  'git config' will ensure that any input output is valid under the given type
> > +  constraint(s), and will canonicalize outgoing values in `<type>`'s canonical
> > +  form.
> 
> In response to my question[2] about whether the typesetting "[type]"
> meant that it was optional, you responded[1] that it was not, thus
> correctly changed the typesetting to "<type>". However...

Right, "--type" without a specifier means nothing (also, I missed this
in my review, but "input output" in the quoted text?)

> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' '
> > +test_expect_success '--no-type unsets type specifiers' '
> > +       echo "10" > expect &&
> > +       git config --type=bool --no-type core.number >actual &&
> > +       test_cmp expect actual
> > +'
> 
> What does --no-type mean and why is it being tested? If this is an
> explicitly supported user-facing option, should it be documented? If
> it's not meant to be user-facing, then why are we enforcing its
> presence and behavior via a test?

It would be the same as if no --type option had been given. The current
documentation says:

  If no type specifier is passed, no checks or transformations are
  performed on the value.

That's retained in the DESCRIPTION section, but it may be worth a
mention of the "--no-type" behavior in the OPTIONS section, too. I
dunno.

-Peff

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:40:51PM -0400, Jeff King wrote:
> On Thu, Apr 05, 2018 at 06:29:18PM -0400, Eric Sunshine wrote:
>
> > > +ensure that the variable(s) are of the given type and convert the value to the
> > > +canonical form. If no type specifier is passed, no checks or transformations are
> > > +performed on the value.
> > > @@ -160,30 +158,34 @@ See also <<FILES>>.
> > >  --list::
> > >         List all variables set in config file, along with their values.
> > >
> > > ---bool::
> > > -       'git config' will ensure that the output is "true" or "false"
> > > +--type <type>::
> > > +  'git config' will ensure that any input output is valid under the given type
> > > +  constraint(s), and will canonicalize outgoing values in `<type>`'s canonical
> > > +  form.
> >
> > In response to my question[2] about whether the typesetting "[type]"
> > meant that it was optional, you responded[1] that it was not, thus
> > correctly changed the typesetting to "<type>". However...
>
> Right, "--type" without a specifier means nothing (also, I missed this
> in my review, but "input output" in the quoted text?)

I missed this too; thanks for pointing it out. I have amended this in
the forthcoming re-roll.

> > > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > > @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' '
> > > +test_expect_success '--no-type unsets type specifiers' '
> > > +       echo "10" > expect &&
> > > +       git config --type=bool --no-type core.number >actual &&
> > > +       test_cmp expect actual
> > > +'
> >
> > What does --no-type mean and why is it being tested? If this is an
> > explicitly supported user-facing option, should it be documented? If
> > it's not meant to be user-facing, then why are we enforcing its
> > presence and behavior via a test?
>
> It would be the same as if no --type option had been given. The current
> documentation says:
>
>   If no type specifier is passed, no checks or transformations are
>   performed on the value.
>
> That's retained in the DESCRIPTION section, but it may be worth a
> mention of the "--no-type" behavior in the OPTIONS section, too. I
> dunno.

Fair; my inclination is to document it, since it is potentially useful
for scripts (as I mentioned in my mail to Eric).

Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:29:18PM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 10:00 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > [...]
> > In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> > `--int`, `--bool`, and etc. This allows the aforementioned upcoming
> > patch to support querying a color value with a default via `--type=color
> > --default=....`
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
> > +A type specifier may be given as an argument to `--type` to make 'git config'
>
> In [1], you said that the argument to --type is required, so use of
> "may be given" here is ambiguous; it makes it sound as if the argument
> is optional. Perhaps rewrite something like:
>
>     The --type option requests `git config` to ...

Thanks; I agree that this is much clearer.

> Not necessarily worth a re-roll, though. (But if you do need to
> re-roll for some reason, it might make sense to combine this series
> with the --default series to make it slightly easier to review them
> together -- since the one depends upon the other -- and probably ease
> the burden on Junio slightly.)

I am re-rolling for some of the additional feedback, and have included
the above changes in it. That being the case, I think that I would like
to hold off on merging the two together, since they are separate topics
and should be merged as-such.

If this list feels differently, however, I can happily merge the two.

> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > @@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' '
> > +test_expect_success '--no-type unsets type specifiers' '
> > +       echo "10" > expect &&
> > +       git config --type=bool --no-type core.number >actual &&
> > +       test_cmp expect actual
> > +'
>
> What does --no-type mean and why is it being tested? If this is an
> explicitly supported user-facing option, should it be documented? If
> it's not meant to be user-facing, then why are we enforcing its
> presence and behavior via a test?

I think that --no-type should be documented. It means "unset any
previously set --type=<type> or --<type>". For example, "git config
--type=bool --no-type core.var" is a synonym of "git config core.var".

This option is useful for callers that would like to overwrite scripts
that impose a specific type specifier. (For example, my mail script
invokes "git format-patch --thread $@", but I can disable threading if I
run my script with "--no-thread".)

Thanks,
Taylor

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
`--int`, `--bool`, and etc. This allows the aforementioned upcoming
patch to support querying a color value with a default via `--type=color
--default=....`

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 66 +++++++++++++++++++-----------------
 builtin/config.c             | 28 +++++++++++++++
 t/t1300-repo-config.sh       | 18 ++++++++++
 3 files changed, 80 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..c7e56e3fd 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type] --add name value
+'git config' [<file-option>] [--type] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier may be given as an argument to `--type` to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations are
+performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,34 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input output is valid under the given type
+  constraint(s), and will canonicalize outgoing values in `<type>`'s canonical
+  form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b..6e1495eac 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,33 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
 
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	int *type = opt->value;
+
+	if (unset) {
+		*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);
+		return 1;
+	}
+	return 0;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,6 +111,7 @@ 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),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 24de37d54..b25ab7b9e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '
 
 cat >.git/config <<-\EOF &&
 [core]
+foo = true
 number = 10
 EOF
 
@@ -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 '--no-type unsets type specifiers' '
+	echo "10" > expect &&
+	git config --type=bool --no-type core.number >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
+'
+
 test_done
-- 
2.16.2.440.gc6284da4f

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

[Jeff King]

On Wed, Apr 04, 2018 at 07:02:38PM -0700, Taylor Blau wrote:

> +test_expect_success '--no-type unsets type specifiers' '
> +	echo "10" > expect &&
> +	git config --type=bool --no-type core.number >actual &&
> +	test_cmp expect actual
> +'

Actually, one minor nit (not worth a re-roll, but Junio may want to mark
it up): drop the space in "> expect".

-Peff

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:12:02PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:02:38PM -0700, Taylor Blau wrote:
>
> > +test_expect_success '--no-type unsets type specifiers' '
> > +	echo "10" > expect &&
> > +	git config --type=bool --no-type core.number >actual &&
> > +	test_cmp expect actual
> > +'
>
> Actually, one minor nit (not worth a re-roll, but Junio may want to mark
> it up): drop the space in "> expect".

Ack; I thought I picked this one up. I am happy to re-roll it, but maybe
it makes to amend while queueing.

Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:12:02PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:02:38PM -0700, Taylor Blau wrote:
>
> > +test_expect_success '--no-type unsets type specifiers' '
> > +	echo "10" > expect &&
> > +	git config --type=bool --no-type core.number >actual &&
> > +	test_cmp expect actual
> > +'
>
> Actually, one minor nit (not worth a re-roll, but Junio may want to mark
> it up): drop the space in "> expect".

I'm re-rolling for some of the other review this round picked up, so
I've included this in the forthcoming v5.

Thanks,
Taylor

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

[Taylor Blau]

Hi,

Here is v6 of this series. I have sent it in direct response to v4
correctly (since v5 was sent elsewhere by mistake).

It contains responses to Eric's feedback in [1].


Thanks,
Taylor

[1]: https://public-inbox.org/git/20180324005556.8145-1-me@ttaylorr.com/T/#mab6aee239c023445fd72693728b566cad5e0fc66

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 | 73 +++++++++++++++++++---------------
 builtin/config.c             | 76 +++++++++++++++++++++++-------------
 t/t1300-repo-config.sh       | 29 ++++++++++++++
 3 files changed, 119 insertions(+), 59 deletions(-)

--
2.17.0

[PATCH v6 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..92fb8d56b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';

 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)

-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5

 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ 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_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	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_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")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);

-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;

-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}

-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }

 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}

-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fd..24de37d54 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '

+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done
--
2.17.0

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--type=color` (in the traditional sense) in
the future, if that is desired.

In this patch, we support `--type=<int|bool|bool-or-int|...>` in
addition to `--int`, `--bool`, and etc. This allows the aforementioned
upcoming patch to support querying a color value with a default via
`--type=color --default=...`, without squandering `--color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 73 ++++++++++++++++++++----------------
 builtin/config.c             | 27 +++++++++++++
 t/t1300-repo-config.sh       | 18 +++++++++
 3 files changed, 86 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..b7686fcbe 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] --add name value
+'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,12 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).

-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type` option requests 'git config' to ensure that the configured values
+associated with the given variable(s) are of the given type. When given
+`--type`, 'git config' will ensure that the variable(s) are of the given type
+and convert the value to the canonical form. If no type specifier is passed, no
+checks or transformations are performed on the value. Callers may unset any
+pre-existing type specifiers with `--no-type`.

 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +160,39 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.

---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in `<type>`'s
+  canonical form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++

+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
+
+--no-type::
+  Un-sets the previously set type specifier (if one was previously set). This
+  option requests that 'git config' not canonicalize the retrieved variable.
+  `--no-type` has no effect without `--type=<type>` or `--<type>`.

 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b..5c8952a17 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,32 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5

+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	int *type = opt->value;
+
+	if (unset) {
+		*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);
+
+	return 0;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,6 +110,7 @@ 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),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 24de37d54..f5ae80e9a 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '

 cat >.git/config <<-\EOF &&
 [core]
+foo = true
 number = 10
 EOF

@@ -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 '--no-type unsets type specifiers' '
+	echo "10" >expect &&
+	git config --type=bool --no-type core.number >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
+'
+
 test_done
--
2.17.0

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

[Eric Sunshine]

On Fri, Apr 6, 2018 at 2:39 AM, Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> In this patch, we support `--type=<int|bool|bool-or-int|...>` in
> addition to `--int`, `--bool`, and etc. This allows the aforementioned
> upcoming patch to support querying a color value with a default via
> `--type=color --default=...`, without squandering `--color`.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -38,12 +38,12 @@ existing values that match the regexp are updated or unset.  If
> +The `--type` option requests 'git config' to ensure that the configured values
> +associated with the given variable(s) are of the given type. When given
> +`--type`, 'git config' will ensure that the variable(s) are of the given type
> +and convert the value to the canonical form. If no type specifier is passed, no
> +checks or transformations are performed on the value. Callers may unset any
> +pre-existing type specifiers with `--no-type`.

Sorry for being such a stickler, but this is still too mushy. The
first two sentences are saying effectively the same thing. One or the
other should be dropped or they should somehow be combined in a way
that says everything that needs to be said without repetition.

If necessary, iterate over updates of this paragraph here in the email
thread until a polished version is reached rather than re-rolling the
entire series every few minutes.

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

[Taylor Blau]

On Fri, Apr 06, 2018 at 03:04:53AM -0400, Eric Sunshine wrote:
> On Fri, Apr 6, 2018 at 2:39 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > [...]
> > In this patch, we support `--type=<int|bool|bool-or-int|...>` in
> > addition to `--int`, `--bool`, and etc. This allows the aforementioned
> > upcoming patch to support querying a color value with a default via
> > `--type=color --default=...`, without squandering `--color`.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -38,12 +38,12 @@ existing values that match the regexp are updated or unset.  If
> > +The `--type` option requests 'git config' to ensure that the configured values
> > +associated with the given variable(s) are of the given type. When given
> > +`--type`, 'git config' will ensure that the variable(s) are of the given type
> > +and convert the value to the canonical form. If no type specifier is passed, no
> > +checks or transformations are performed on the value. Callers may unset any
> > +pre-existing type specifiers with `--no-type`.
>
> Sorry for being such a stickler, but this is still too mushy. The
> first two sentences are saying effectively the same thing. One or the
> other should be dropped or they should somehow be combined in a way
> that says everything that needs to be said without repetition.

I agree, and please do not apologize for being a stickler. I think that
it's important we reach consensus before landing this, since other
humans will read this.

How do you feel about?

  The `--type=<type>` option instructs 'git config' to ensure that
  incoming and outgoing values are canonicalize-able under the given
  <type>.  If no `--type=<type>` is given, no canonicalization will be
  performed. Callers may unset the existing `--type` specifier with
  `--no-type`.

I think documenting that `--no-type` unsets any pre-existing `--type`
specifier is worthwhile. That said, I also think that there's a way to
combine the last two sentences, but it might be clearer as two smaller
pieces rather than one big one.

If this is still off base, could you provide some pointers on how you
would word it?

> If necessary, iterate over updates of this paragraph here in the email
> thread until a polished version is reached rather than re-rolling the
> entire series every few minutes.

Thanks, and will do :-). I am quite new to this and was unaware of the
situations when re-rolling is and isn't desirable. I am going to wait to
re-roll this series until it has gathered more feedback, or at least
consensus, after which I think it will be ready for queueing as-is.

Thanks for your patience and guidance.


Thanks,
Taylor

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

[Eric Sunshine]

On Fri, Apr 6, 2018 at 8:39 PM, Taylor Blau <me@ttaylorr.com> wrote:
> On Fri, Apr 06, 2018 at 03:04:53AM -0400, Eric Sunshine wrote:
>> Sorry for being such a stickler, but this is still too mushy. The
>> first two sentences are saying effectively the same thing. One or the
>> other should be dropped or they should somehow be combined in a way
>> that says everything that needs to be said without repetition.
>
> How do you feel about?
>
>   The `--type=<type>` option instructs 'git config' to ensure that
>   incoming and outgoing values are canonicalize-able under the given
>   <type>.  If no `--type=<type>` is given, no canonicalization will be
>   performed. Callers may unset the existing `--type` specifier with
>   `--no-type`.

This sounds fine. (Maybe s/the existing/an existing/)

> I think documenting that `--no-type` unsets any pre-existing `--type`
> specifier is worthwhile. That said, I also think that there's a way to
> combine the last two sentences, but it might be clearer as two smaller
> pieces rather than one big one.

Smaller, simpler, more easily digested sentences are a win.

>> If necessary, iterate over updates of this paragraph here in the email
>> thread until a polished version is reached rather than re-rolling the
>> entire series every few minutes.
>
> Thanks, and will do :-). I am quite new to this and was unaware of the
> situations when re-rolling is and isn't desirable. I am going to wait to
> re-roll this series until it has gathered more feedback, or at least
> consensus,

Just as it's a burden for you to repeatedly re-roll, it's also a
burden on reviewers to repeatedly re-read the series. Ideally, we'd
like fewer, rather than more, re-rolls, so it's good to nail down
questionable or ambiguous issues via normal email discussion before
going for a re-roll, and some reviewers try to assist with deciding if
a re-roll is needed by saying whether or not a review comment warrants
one.

Allowing time for others to review and possibly comment on a series
before re-rolling is indeed a good idea.

Another useful tactic is to include, in the cover letter, an interdiff
between the previous and new versions, which gives reviewers a way to
quickly examine the changed parts of the series without necessarily
having to re-review each patch in entirety (an often time-consuming
task).

> after which I think it will be ready for queueing as-is.

Famous last words ;-)

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

[Taylor Blau]

Hi,

Attached is the seventh re-roll of my series to support '--type=<type>'
instead of '--<type>' in 'git-config(1)'.

Since v6, I have changed only the wording in
Documentation/git-config.txt, which Eric and I reached consensus upon in
a sub-thread [1]. Per Eric's suggestion, I have also included an
inter-diff between this re-roll and the one previous for easier
consumption.

I anticipate that since relatively little has changed since the last
re-roll, that this will be the final re-roll of this series. I apologize
that the re-roll count has gotten so high, though I am glad that we
hashed out the important details. I have a better idea of how to discuss
on this list without increasing the re-roll count so much.

Thanks in advance for your review :-).


Thanks,
Taylor

[1]: https://public-inbox.org/git/CAPig+cQXJuLWpxfwhQ98a23wfAAzKZpkOvqWBdXaKKHLiW-7Uw@mail.gmail.com/

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             | 76 +++++++++++++++++++++++-------------
 t/t1300-repo-config.sh       | 29 ++++++++++++++
 3 files changed, 117 insertions(+), 59 deletions(-)

Inter-diff (since v6):

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index b7686fcbe3..a1e3ffe750 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).

-The `--type` option requests 'git config' to ensure that the configured values
-associated with the given variable(s) are of the given type. When given
-`--type`, 'git config' will ensure that the variable(s) are of the given type
-and convert the value to the canonical form. If no type specifier is passed, no
-checks or transformations are performed on the value. Callers may unset any
-pre-existing type specifiers with `--no-type`.
+The `--type=<type>` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given <type>.  If no
+`--type=<type>` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.

 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
--
2.17.0

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

[Eric Sunshine]

On Mon, Apr 9, 2018 at 6:46 PM, Taylor Blau <me@ttaylorr.com> wrote:
> Attached is the seventh re-roll of my series to support '--type=<type>'
> instead of '--<type>' in 'git-config(1)'.
>
> Since v6, I have changed only the wording in
> Documentation/git-config.txt, which Eric and I reached consensus upon in
> a sub-thread [1]. Per Eric's suggestion, I have also included an
> inter-diff between this re-roll and the one previous for easier
> consumption.

Thanks, this iteration seems to address the issues raised by my
earlier reviews, and I didn't find anything else upon which to
comment.

I had meant earlier to say that the subject of 2/2:

    builtin/config.c: support `--type=<type>` as preferred alias for `--type`

should probably be typeset as:

    builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`

but I'd rather not see a re-roll just for that. (Perhaps Junio can
tweak it when queuing.)

[PATCH v7 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd628..92fb8d56b1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ 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_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	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_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")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fde..24de37d544 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.17.0

Re: [PATCH v7 1/2] builtin/config.c: treat type specifiers singularly

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Internally, we represent `git config`'s type specifiers as a bitset
> using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
> allows for the representation of multiple type specifiers in the `int
> types` field, but this multi-representation is left unused.
>
> In fact, `git config` will not accept multiple type specifiers at a
> time, as indicated by:
>
>   $ git config --int --bool some.section
>   error: only one type at a time.
>
> This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
> specifier, so that the above command would instead be valid, and a
> synonym of:
>
>   $ git config --bool some.section
>
> This change is motivated by two urges: (1) it does not make sense to
> represent a singular type specifier internally as a bitset, only to
> complain when there are multiple bits in the set. `OPT_SET_INT` is more
> well-suited to this task than `OPT_BIT` is. (2) a future patch will
> introduce `--type=<type>`, and we would like not to complain in the
> following situation:
>
>   $ git config --int --type=int

The above does not exactly argue for adopting the last-one-wins
semantics, and still leaves it unclear if we want to complain
against

    $ git config --bool --type=int

Is it intentionally left vague if we want to (or not want to)
complain when such a conflicting specification is given?

We could keep the traditional behaviour of "only one type at a time"
error and still move away from the bitset representation that does
not make sense, if we wanted to.  Initialize the "type" variable to
an unset value, and use a callback to ensure either the variable is
set to the unset value, or the value being set is already in the
variable.  I think if you use OPT_CMDMODE(), it would do all of that
for you automatically.

I suspect that it may be OK to switch to last-one-wins, but then we
should give a justification that is a bit stronger than "we want to
avoid complaining against --int --type=int" (i.e. "we want to switch
to last-one-wins for such and such reasons").

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 4f8e6f5fde..24de37d544 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
>  	test_expect_code 128 nongit git config --local foo.bar
>  '
>  
> +cat >.git/config <<-\EOF &&
> +[core]
> +number = 10
> +EOF
> +
> +test_expect_success 'later legacy specifiers are given precedence' '
> +	git config --bool --int core.number >actual &&
> +	echo 10 >expect &&
> +	test_cmp expect actual
> +'

And this expects more than we gave justifications for in the
proposed log message.  I do not think it is necessarily a bad idea
to switch to last-one-wins, but if that is where we really want to
go, the proposed log message is being misleading.  It is true that
OPT_SET_INT is more suited to complain when two conflicting things
are given than OPT_BIT, but this example actually tells us that you
no longer want to catch an error to give conflicting requests.

Re: [PATCH v7 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

On Tue, Apr 10, 2018 at 10:22:25AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Internally, we represent `git config`'s type specifiers as a bitset
> > using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
> > allows for the representation of multiple type specifiers in the `int
> > types` field, but this multi-representation is left unused.
> >
> > In fact, `git config` will not accept multiple type specifiers at a
> > time, as indicated by:
> >
> >   $ git config --int --bool some.section
> >   error: only one type at a time.
> >
> > This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
> > specifier, so that the above command would instead be valid, and a
> > synonym of:
> >
> >   $ git config --bool some.section
> >
> > This change is motivated by two urges: (1) it does not make sense to
> > represent a singular type specifier internally as a bitset, only to
> > complain when there are multiple bits in the set. `OPT_SET_INT` is more
> > well-suited to this task than `OPT_BIT` is. (2) a future patch will
> > introduce `--type=<type>`, and we would like not to complain in the
> > following situation:
> >
> >   $ git config --int --type=int
>
> The above does not exactly argue for adopting the last-one-wins
> semantics, and still leaves it unclear if we want to complain
> against
>
>     $ git config --bool --type=int
>
> Is it intentionally left vague if we want to (or not want to)
> complain when such a conflicting specification is given?
>
> We could keep the traditional behaviour of "only one type at a time"
> error and still move away from the bitset representation that does
> not make sense, if we wanted to.  Initialize the "type" variable to
> an unset value, and use a callback to ensure either the variable is
> set to the unset value, or the value being set is already in the
> variable.  I think if you use OPT_CMDMODE(), it would do all of that
> for you automatically.
>
> I suspect that it may be OK to switch to last-one-wins, but then we
> should give a justification that is a bit stronger than "we want to
> avoid complaining against --int --type=int" (i.e. "we want to switch
> to last-one-wins for such and such reasons").

I think that the major justification is to treat --type=int as a _true_
synonym of --int, such that neither `--type=<t1> --type=<t2>` nor
`--<t1> --<t2>` will complain. This, as well as the fact that
OPT_SET_BIT brings us closer to the semantics of `--verbose=1
--verbose=2`, which is something that Eric had mentioned above.

I think that OPT_CMDMODE would not work quite in the way we desire,
since the error messages would not quite line up with the command typed.
For instance, after applying the following diff:

diff --git a/builtin/config.c b/builtin/config.c
index 5c8952a17c..d9b73b949a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -111,11 +111,11 @@ static struct option builtin_config_options[] = {
     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_CMDMODE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
+    OPT_CMDMODE(0, "int", &type, N_("value is decimal number"), TYPE_INT),
+    OPT_CMDMODE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
+    OPT_CMDMODE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
+    OPT_CMDMODE(0, "expiry-date", &type, N_("value is an expiry date"), 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")),

The following occurs:

  ~/g/git (tb/config-type-specifier-option!) $ ./git-config --type=int --bool foo.bar
  error: option `bool' : incompatible with --int

Whereas I would expect that to say:

  error: option `bool' is incompatible with `--type=int'.

I am not sure whether amending the implementation of OPT_CMDMODE is something
that you're interested in here.

I can amend my patch to include this extra reasoning, if you think that
would be helpful.

> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > index 4f8e6f5fde..24de37d544 100755
> > --- a/t/t1300-repo-config.sh
> > +++ b/t/t1300-repo-config.sh
> > @@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
> >  	test_expect_code 128 nongit git config --local foo.bar
> >  '
> >
> > +cat >.git/config <<-\EOF &&
> > +[core]
> > +number = 10
> > +EOF
> > +
> > +test_expect_success 'later legacy specifiers are given precedence' '
> > +	git config --bool --int core.number >actual &&
> > +	echo 10 >expect &&
> > +	test_cmp expect actual
> > +'
>
> And this expects more than we gave justifications for in the
> proposed log message.  I do not think it is necessarily a bad idea
> to switch to last-one-wins, but if that is where we really want to
> go, the proposed log message is being misleading.  It is true that
> OPT_SET_INT is more suited to complain when two conflicting things
> are given than OPT_BIT, but this example actually tells us that you
> no longer want to catch an error to give conflicting requests.

Yes, since I'd like to be able to insert `--type=` and have it behave
the same way. Since `--type=bool --type=int` will not complain (and
instead adopt `--type=int` given that it comes later in the command
string), neither should `--bool --int`.


Thanks,
Taylor

Re: [PATCH v7 1/2] builtin/config.c: treat type specifiers singularly

[Eric Sunshine]

On Mon, Apr 9, 2018 at 10:12 PM, Taylor Blau <me@ttaylorr.com> wrote:
> On Tue, Apr 10, 2018 at 10:22:25AM +0900, Junio C Hamano wrote:
>> I suspect that it may be OK to switch to last-one-wins, but then we
>> should give a justification that is a bit stronger than "we want to
>> avoid complaining against --int --type=int" (i.e. "we want to switch
>> to last-one-wins for such and such reasons").
>
> I think that the major justification is to treat --type=int as a _true_
> synonym of --int, such that neither `--type=<t1> --type=<t2>` nor
> `--<t1> --<t2>` will complain. This, as well as the fact that
> OPT_SET_BIT brings us closer to the semantics of `--verbose=1
> --verbose=2`, which is something that Eric had mentioned above.

I'm probably being dense, but even after reading this paragraph
several times, I still don't have a good idea as to what it is trying
to say.

As for my earlier reference to '--verbose=1 --verbose=2', that was
cited merely as a "last wins" which I could buy; it was offered in
contrast to '--type=bool --type=int' "last wins" which this patch
tries to sell, and which I have a tough time buying (though I defer to
Junio's and Peff's judgments).

This patch (or perhaps its commit message) seems to conflate two
independent goals. (1) ridding this code of OPT_BIT() since its use in
this context is not very sensible, and (2) trying to sell "last wins"
(for a not well justified argument) to support '--int --type=int'
without complaint.

Goal #1 makes plenty sense; no objection to that. Goal #2 isn't so
obviously desirable (I already raised objections to the more general
'--bool --type=int' "last wins" it implements).

> I think that OPT_CMDMODE would not work quite in the way we desire,
> since the error messages would not quite line up with the command typed.
> For instance, after applying the following diff:
>
>      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_CMDMODE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
> +    OPT_CMDMODE(0, "int", &type, N_("value is decimal number"), TYPE_INT),
> +    OPT_CMDMODE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
> +    OPT_CMDMODE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
> +    OPT_CMDMODE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
>
>   ~/g/git (tb/config-type-specifier-option!) $ ./git-config --type=int --bool foo.bar
>   error: option `bool' : incompatible with --int
>
> Whereas I would expect that to say:
>
>   error: option `bool' is incompatible with `--type=int'.

Couldn't you achieve reasonable output (such as "error: conflicting
types requested: %s and %s") by handling all those --<type>s with a
custom callback? You've already coded such a callback for
--type=<type>, and it doesn't look like it would be much more effort
to refactor it a bit to handle --<type> as well.

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--type=color` (in the traditional sense) in
the future, if that is desired.

In this patch, we support `--type=<int|bool|bool-or-int|...>` in
addition to `--int`, `--bool`, and etc. This allows the aforementioned
upcoming patch to support querying a color value with a default via
`--type=color --default=...`, without squandering `--color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 71 ++++++++++++++++++++----------------
 builtin/config.c             | 27 ++++++++++++++
 t/t1300-repo-config.sh       | 18 +++++++++
 3 files changed, 84 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d5..a1e3ffe750 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] --add name value
+'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=<type>` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given <type>.  If no
+`--type=<type>` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,39 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in `<type>`'s
+  canonical form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
+
+--no-type::
+  Un-sets the previously set type specifier (if one was previously set). This
+  option requests that 'git config' not canonicalize the retrieved variable.
+  `--no-type` has no effect without `--type=<type>` or `--<type>`.
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b1..5c8952a17c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,32 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
 
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	int *type = opt->value;
+
+	if (unset) {
+		*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);
+
+	return 0;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,6 +110,7 @@ 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),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 24de37d544..f5ae80e9ae 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '
 
 cat >.git/config <<-\EOF &&
 [core]
+foo = true
 number = 10
 EOF
 
@@ -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 '--no-type unsets type specifiers' '
+	echo "10" >expect &&
+	git config --type=bool --no-type core.number >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
+'
+
 test_done
-- 
2.17.0

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values are satisfiable under that type, and (2) that outgoing values are
> canonicalized under that type.

Hmm, "satisfiable" is not the first word that comes to my mind to
describe "when you give me a string 'frotz', we cannot take it as an
integer".  s/are satisfiable under/can be interpreted as/ perhaps?

> In another series, we propose to extend this functionality with
> `--type=color` and `--default` to replace `--get-color`.
>
> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".

Makes sense.

> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid inhabiting this option too soon, so that
> `git config` can support `--type=color` (in the traditional sense) in
> the future, if that is desired.

Shouldn't the above `--color` instead?  That is, we avoid squatting
on `--color` because we might later want to use it in the
traditional way.  Also my reading stuttered around "inhabiting".  It
might be just me, but perhaps s/inhabiting/squating on/?

> In this patch, we support `--type=<int|bool|bool-or-int|...>` in
> addition to `--int`, `--bool`, and etc. This allows the aforementioned
> upcoming patch to support querying a color value with a default via
> `--type=color --default=...`, without squandering `--color`.

Good.

> @@ -160,30 +158,39 @@ See also <<FILES>>.
>  --list::
>  	List all variables set in config file, along with their values.
>  
> ---bool::
> -	'git config' will ensure that the output is "true" or "false"
> +--type <type>::
> +  'git config' will ensure that any input or output is valid under the given
> +  type constraint(s), and will canonicalize outgoing values in `<type>`'s
> +  canonical form.
> ++
> +Valid `<type>`'s include:
> ++
> +- 'bool': canonicalize values as either "true" or "false".
> +- 'int': canonicalize values as simple decimal numbers. An optional suffix of
> +  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
> +  1073741824 prior to output.

It is not a new problem introduced by this patch, but these place
too much weight on the output side and may end up giving a flawed
mental model to the readers, I am afraid.

It's not like an 'int' value internally is "2k" and shown it as 2048
when output.  In reality, we internalize it 2048 upon input and we
do not do anything special when showing, no?  A 'bool' value given
with a string 'no' internally becomes 0 upon input and is shown as
'false' upon output.

I suspect s/prior to output/upon input/ may alleviate the issue
quite a bit.

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

[Taylor Blau]

On Tue, Apr 10, 2018 at 10:44:18AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > `git config` has long allowed the ability for callers to provide a 'type
> > specifier', which instructs `git config` to (1) ensure that incoming
> > values are satisfiable under that type, and (2) that outgoing values are
> > canonicalized under that type.
>
> Hmm, "satisfiable" is not the first word that comes to my mind to
> describe "when you give me a string 'frotz', we cannot take it as an
> integer".  s/are satisfiable under/can be interpreted as/ perhaps?

Sure; I think that "can be interpreted as" is clearer. I have amended my
patch locally, and will include this in the next re-roll, should one
exist.

> > In another series, we propose to extend this functionality with
> > `--type=color` and `--default` to replace `--get-color`.
> >
> > However, we traditionally use `--color` to mean "colorize this output",
> > instead of "this value should be treated as a color".
>
> Makes sense.
>
> > Currently, `git config` does not support this kind of colorization, but
> > we should be careful to avoid inhabiting this option too soon, so that
> > `git config` can support `--type=color` (in the traditional sense) in
> > the future, if that is desired.
>
> Shouldn't the above `--color` instead?  That is, we avoid squatting
> on `--color` because we might later want to use it in the
> traditional way.  Also my reading stuttered around "inhabiting".  It
> might be just me, but perhaps s/inhabiting/squating on/?

Ack, yes to both.

> > In this patch, we support `--type=<int|bool|bool-or-int|...>` in
> > addition to `--int`, `--bool`, and etc. This allows the aforementioned
> > upcoming patch to support querying a color value with a default via
> > `--type=color --default=...`, without squandering `--color`.
>
> Good.
>
> > @@ -160,30 +158,39 @@ See also <<FILES>>.
> >  --list::
> >  	List all variables set in config file, along with their values.
> >
> > ---bool::
> > -	'git config' will ensure that the output is "true" or "false"
> > +--type <type>::
> > +  'git config' will ensure that any input or output is valid under the given
> > +  type constraint(s), and will canonicalize outgoing values in `<type>`'s
> > +  canonical form.
> > ++
> > +Valid `<type>`'s include:
> > ++
> > +- 'bool': canonicalize values as either "true" or "false".
> > +- 'int': canonicalize values as simple decimal numbers. An optional suffix of
> > +  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
> > +  1073741824 prior to output.
>
> It is not a new problem introduced by this patch, but these place
> too much weight on the output side and may end up giving a flawed
> mental model to the readers, I am afraid.
>
> It's not like an 'int' value internally is "2k" and shown it as 2048
> when output.  In reality, we internalize it 2048 upon input and we
> do not do anything special when showing, no?  A 'bool' value given
> with a string 'no' internally becomes 0 upon input and is shown as
> 'false' upon output.
>
> I suspect s/prior to output/upon input/ may alleviate the issue
> quite a bit.

I share your concern, and I think that your suggestion would alleviate
the issue entirely.

I have applied these suggestions locally. :-)

Thanks,
Taylor

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

[Taylor Blau]

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

I too am curious.  Personally I do not think your "last one wins"
was necessarily bad--in fact it internally was consistent--I just
thought that the log message did not justify the choice well.  And I
do not think the semantics defined by this one, "once you choose,
stick to it, or explicitly clear the previous choice", is bad,
either.

> 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[];

OK.  I am not sure if OPT_CALLBACK_VALUE() needs to take 'f', as you
always pass the option_parse_type function to it.

>  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;

Does this rely on a file-scope global variable (type)?

>
>  	return 0;
>  }

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

[Taylor Blau]

On Wed, Apr 11, 2018 at 10:24:45AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > 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.
>
> I too am curious.  Personally I do not think your "last one wins"
> was necessarily bad--in fact it internally was consistent--I just
> thought that the log message did not justify the choice well.  And I
> do not think the semantics defined by this one, "once you choose,
> stick to it, or explicitly clear the previous choice", is bad,
> either.

:-). If nothing else, I like that we retain more, stricter behavior from
previous versions.

> > 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[];
>
> OK.  I am not sure if OPT_CALLBACK_VALUE() needs to take 'f', as you
> always pass the option_parse_type function to it.

That's fair. I left this in as an indication that something like this
_might_ want to make its way into parse-options.h as a general-purpose
utility, but was not yet ready to do so. Thus, I defined it inside
builtin/config.c.

> >  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;
>
> Does this rely on a file-scope global variable (type)?

I don't think it does. I think I had conflated the difference between
opt->value and opt->defval while amending this patch. What do you think of the
following (which removes reaching outside the function for "type")?

diff --git a/builtin/config.c b/builtin/config.c
index 7184c09582..53755ca461 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,8 +61,8 @@ 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 | \
+#define OPT_CALLBACK_VALUE(s, l, v, h, f, i) \
+	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
 	PARSE_OPT_NONEG, (f), (i) }

 static struct option builtin_config_options[];
@@ -71,7 +71,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
 {
 	if (unset) {
-		type = 0;
+		*((int *) opt->value) = 0;
 		return 0;
 	}

@@ -95,7 +95,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			die(_("unrecognized --type argument, %s"), arg);
 	}

-	if (type != 0 && type != new_type) {
+	int *to_type = opt->value;
+	if (*to_type && *to_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
@@ -107,7 +108,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
 		usage_with_options(builtin_config_usage,
 			builtin_config_options);
 	}
-	type = new_type;
+	*to_type = new_type;

 	return 0;
 }
@@ -135,12 +136,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", 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_CALLBACK('t', "type", &type, "", N_("value is given this type"), option_parse_type),
+	OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), option_parse_type, TYPE_BOOL),
+	OPT_CALLBACK_VALUE(0, "int", &type, N_("value is decimal number"), option_parse_type, TYPE_INT),
+	OPT_CALLBACK_VALUE(0, "bool-or-int", &type, N_("value is --bool or --int"), option_parse_type, TYPE_BOOL_OR_INT),
+	OPT_CALLBACK_VALUE(0, "path", &type, N_("value is a path (file or directory name)"), option_parse_type, TYPE_PATH),
+	OPT_CALLBACK_VALUE(0, "expiry-date", &type, 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")),


Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> > +#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[];
>>
>> OK.  I am not sure if OPT_CALLBACK_VALUE() needs to take 'f', as you
>> always pass the option_parse_type function to it.
>
> That's fair. I left this in as an indication that something like this
> _might_ want to make its way into parse-options.h as a general-purpose
> utility, but was not yet ready to do so. Thus, I defined it inside
> builtin/config.c.

I understood the reasoning, but as your current verdict is that this
is not yet ready for parse-options.[ch], I think it is probably
preferrable to reduce repeated passing of the same function to the
macro, at least while it is in this builgin/config.c file.

> @@ -71,7 +71,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
>  			     int unset)
>  {
>  	if (unset) {
> -		type = 0;
> +		*((int *) opt->value) = 0;
>  		return 0;
>  	}

Yup.  This (if it works) is exactly what I imagined the function
should look like.

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

[Taylor Blau]

On Wed, Apr 11, 2018 at 12:11:47PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> > +#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[];
> >>
> >> OK.  I am not sure if OPT_CALLBACK_VALUE() needs to take 'f', as you
> >> always pass the option_parse_type function to it.
> >
> > That's fair. I left this in as an indication that something like this
> > _might_ want to make its way into parse-options.h as a general-purpose
> > utility, but was not yet ready to do so. Thus, I defined it inside
> > builtin/config.c.
>
> I understood the reasoning, but as your current verdict is that this
> is not yet ready for parse-options.[ch], I think it is probably
> preferrable to reduce repeated passing of the same function to the
> macro, at least while it is in this builgin/config.c file.

Ah, that seems fair to me. I have removed the duplicate 'f' parameter
and all of the option_parse_type()'s in builtin/config.c within my local
copy, and will happily include these changes in the subsequent re-roll.

I'll wait for more feedback before sending this, however.


Thanks,
Taylor

[PATCH v8 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd628..92fb8d56b1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ 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_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	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_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")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fde..24de37d544 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.17.0

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values can be interpreted as that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid squatting on this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we support `--type=<int|bool|bool-or-int|...>` in
addition to `--int`, `--bool`, and etc. This allows the aforementioned
upcoming patch to support querying a color value with a default via
`--type=color --default=...`, without squandering `--color`.

We retain the historic behavior of complaining when multiple,
legacy-style `--<type>` flags are given, as well as extend this to
conflicting new-style `--type=<type>` flags. `--int --type=int` (and its
commutative pair) does not complain, but `--bool --type=int` (and its
commutative pair) does.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 71 ++++++++++++++++++++----------------
 builtin/config.c             | 62 ++++++++++++++++++++++++++++---
 t/t1300-repo-config.sh       | 58 +++++++++++++++++++++++++++--
 3 files changed, 151 insertions(+), 40 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d5..b759009c75 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] --add name value
+'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=<type>` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given <type>.  If no
+`--type=<type>` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,39 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in `<type>`'s
+  canonical form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
+
+--no-type::
+  Un-sets the previously set type specifier (if one was previously set). This
+  option requests that 'git config' not canonicalize the retrieved variable.
+  `--no-type` has no effect without `--type=<type>` or `--<type>`.
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b1..7184c09582 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,57 @@ 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)
+{
+	if (unset) {
+		type = 0;
+		return 0;
+	}
+
+	/*
+	 * 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;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,11 +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_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 24de37d544..e06af3d337 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,13 +1613,65 @@ test_expect_success '--local requires a repo' '
 
 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 &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-type unsets type specifiers' '
+	echo "10" >expect &&
+	git config --type=bool --no-type core.number >actual &&
+	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
+'
+
 test_done
-- 
2.17.0

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

[Taylor Blau]

Hi,

Attached is my final re-roll of the series to add `--type=<type>` as a
supported replacement for `--<type>`.

Since the last time, I have changed the following:

  * OPT_CALLBACK_VALUE no longer takes a function pointer, and instead
    assumes option_parse_type().

  * No longer rely on 'type' as a global from within
    option_parse_type(), instead pass it through opt->value. (NB: it is
    never NULL, and thus always safe to *(opt->value).)

Thanks to all who reviewed this series in its many iterations. I am very
happy with how it turned out, and was fortunate to receive feedback and
eventual consensus on the interface.


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             | 102 +++++++++++++++++++++++++----------
 t/t1300-repo-config.sh       |  63 ++++++++++++++++++++++
 3 files changed, 177 insertions(+), 59 deletions(-)

Inter-diff (since v8):

diff --git a/builtin/config.c b/builtin/config.c
index 7184c09582..bd7a8d0ce7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,9 +61,9 @@ 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) }
+#define OPT_CALLBACK_VALUE(s, l, v, h, i) \
+	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
+	PARSE_OPT_NONEG, option_parse_type, (i) }

 static struct option builtin_config_options[];

@@ -71,7 +71,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
 {
 	if (unset) {
-		type = 0;
+		*((int *) opt->value) = 0;
 		return 0;
 	}

@@ -95,7 +95,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			die(_("unrecognized --type argument, %s"), arg);
 	}

-	if (type != 0 && type != new_type) {
+	int *to_type = opt->value;
+	if (*to_type && *to_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
@@ -107,7 +108,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
 		usage_with_options(builtin_config_usage,
 			builtin_config_options);
 	}
-	type = new_type;
+	*to_type = new_type;

 	return 0;
 }
@@ -135,12 +136,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", 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_CALLBACK('t', "type", &type, "", N_("value is given this type"), option_parse_type),
+	OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
+	OPT_CALLBACK_VALUE(0, "int", &type, N_("value is decimal number"), TYPE_INT),
+	OPT_CALLBACK_VALUE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
+	OPT_CALLBACK_VALUE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
+	OPT_CALLBACK_VALUE(0, "expiry-date", &type, N_("value is an expiry date"), 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")),


--
2.17.0

[PATCH v9 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd628..92fb8d56b1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ 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_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	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_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")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fde..24de37d544 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.17.0

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

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values can be interpreted as that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid squatting on this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we support `--type=<int|bool|bool-or-int|...>` in
addition to `--int`, `--bool`, and etc. This allows the aforementioned
upcoming patch to support querying a color value with a default via
`--type=color --default=...`, without squandering `--color`.

We retain the historic behavior of complaining when multiple,
legacy-style `--<type>` flags are given, as well as extend this to
conflicting new-style `--type=<type>` flags. `--int --type=int` (and its
commutative pair) does not complain, but `--bool --type=int` (and its
commutative pair) does.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 71 ++++++++++++++++++++----------------
 builtin/config.c             | 63 +++++++++++++++++++++++++++++---
 t/t1300-repo-config.sh       | 58 +++++++++++++++++++++++++++--
 3 files changed, 152 insertions(+), 40 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d5..b759009c75 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] --add name value
+'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=<type>` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given <type>.  If no
+`--type=<type>` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,39 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in `<type>`'s
+  canonical form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
+
+--no-type::
+  Un-sets the previously set type specifier (if one was previously set). This
+  option requests that 'git config' not canonicalize the retrieved variable.
+  `--no-type` has no effect without `--type=<type>` or `--<type>`.
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b1..bd7a8d0ce7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,58 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
 
+#define OPT_CALLBACK_VALUE(s, l, v, h, i) \
+	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
+	PARSE_OPT_NONEG, option_parse_type, (i) }
+
+static struct option builtin_config_options[];
+
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	if (unset) {
+		*((int *) opt->value) = 0;
+		return 0;
+	}
+
+	/*
+	 * 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);
+	}
+
+	int *to_type = opt->value;
+	if (*to_type && *to_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);
+	}
+	*to_type = new_type;
+
+	return 0;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,11 +136,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_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", &type, "", N_("value is given this type"), option_parse_type),
+	OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
+	OPT_CALLBACK_VALUE(0, "int", &type, N_("value is decimal number"), TYPE_INT),
+	OPT_CALLBACK_VALUE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
+	OPT_CALLBACK_VALUE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
+	OPT_CALLBACK_VALUE(0, "expiry-date", &type, N_("value is an expiry date"), 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 24de37d544..e06af3d337 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,13 +1613,65 @@ test_expect_success '--local requires a repo' '
 
 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 &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-type unsets type specifiers' '
+	echo "10" >expect &&
+	git config --type=bool --no-type core.number >actual &&
+	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
+'
+
 test_done
-- 
2.17.0

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> diff --git a/builtin/config.c b/builtin/config.c
> index 92fb8d56b1..bd7a8d0ce7 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -61,6 +61,58 @@ static int show_origin;
>  #define TYPE_PATH		4
>  #define TYPE_EXPIRY_DATE	5
>  
> +#define OPT_CALLBACK_VALUE(s, l, v, h, i) \
> +	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
> +	PARSE_OPT_NONEG, option_parse_type, (i) }
> +
> +static struct option builtin_config_options[];
> +
> +static int option_parse_type(const struct option *opt, const char *arg,
> +			     int unset)
> +{

Declare all local variables here.  We do not accept decl-after-statement.

> +	if (unset) {
> +		*((int *) opt->value) = 0;
> +		return 0;
> +	}
> +
> +	/*
> +	 * To support '--<type>' style flags, begin with new_type equal to
> +	 * opt->defval.
> +	 */
> +	int new_type = opt->defval;

Like this one and ...

> +	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);
> +	}
> +
> +	int *to_type = opt->value;

... this one.

> +	if (*to_type && *to_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);
> +	}
> +	*to_type = new_type;
> +
> +	return 0;
> +}
> +

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

[Taylor Blau]

On Thu, Apr 19, 2018 at 11:47:50AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 92fb8d56b1..bd7a8d0ce7 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -61,6 +61,58 @@ static int show_origin;
> >  #define TYPE_PATH		4
> >  #define TYPE_EXPIRY_DATE	5
> >
> > +#define OPT_CALLBACK_VALUE(s, l, v, h, i) \
> > +	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
> > +	PARSE_OPT_NONEG, option_parse_type, (i) }
> > +
> > +static struct option builtin_config_options[];
> > +
> > +static int option_parse_type(const struct option *opt, const char *arg,
> > +			     int unset)
> > +{
>
> Declare all local variables here.  We do not accept decl-after-statement.

My apologies, I will read Documentation/CodingGuidelines carefully. I
have generated the following patch locally:

diff --git a/builtin/config.c b/builtin/config.c
index bd7a8d0ce7..2f91ef15a4 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -70,6 +70,9 @@ static struct option builtin_config_options[];
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
 {
+	int new_type;
+	int *to_type;
+
 	if (unset) {
 		*((int *) opt->value) = 0;
 		return 0;
@@ -79,7 +82,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
 	 * To support '--<type>' style flags, begin with new_type equal to
 	 * opt->defval.
 	 */
-	int new_type = opt->defval;
+	new_type = opt->defval;
 	if (!new_type) {
 		if (!strcmp(arg, "bool"))
 			new_type = TYPE_BOOL;
@@ -95,7 +98,7 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			die(_("unrecognized --type argument, %s"), arg);
 	}

-	int *to_type = opt->value;
+	*to_type = opt->value;
 	if (*to_type && *to_type != new_type) {
 		/*
 		 * Complain when there is a new type not equal to the old type.

---

And would be happy to apply it locally myself and send it to you via a
re-roll. You are also free to apply it yourself if it would be easier. I
do not have a preference one way or another.


Thanks,
Taylor