builtin/config.c: prefer `--type=bool` over `--bool`, etc.
PATCH 1/1 builtin/config.c: prefer `--type=bool` over `--bool`, etc.
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.
--- Documentation/git-config.txt | 66 +++++++++++++++++++----------------- builtin/config.c | 25 ++++++++++++++ t/t1300-repo-config.sh | 21 ++++++++++++ 3 files changed, 80 insertions(+), 32 deletions(-)
Documentation/git-config.txt +34 -32
@@ -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::
builtin/config.c +25 -0
@@ -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 );
t/t1300-repo-config.sh +21 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
Quoted 8 lines · Taylor Blau
git confighas long allowed the ability for callers to provide a 'type specifier', which instructsgit configto (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
--colorand--defaultto replace--get-color.
Quoted 7 lines
However, we traditionally use
--colorto mean "colorize this output", instead of "this value should be treated as a color".Currently,
git configdoes not support this kind of colorization, but we should be careful to avoid inhabiting this option too soon, so thatgit configcan support--color(in the traditional sense) in the future, if that is desired.
OK.
Quoted 4 lines
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.
Quoted 16 lines · Taylor Blau
git confighas long allowed the ability for callers to provide a 'type specifier', which instructsgit configto (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
--colorand--defaultto replace--get-color.However, we traditionally use
--colorto mean "colorize this output", instead of "this value should be treated as a color".Currently,
git configdoes not support this kind of colorization, but we should be careful to avoid inhabiting this option too soon, so thatgit configcan support--color(in the traditional sense) in the future, if that is desired.
Quoted 4 lines
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".
Quoted 5 lines
Documentation/git-config.txt +5
-0+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
Documentation/git-config.txt See also <<FILES>>. · line 180
Quoted 10 lines
Documentation/git-config.txt +10
-0+ 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&/ :)
builtin/config.c static char *default_user_config(void) · line 501
Quoted 4 lines
builtin/config.c +4
-0+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"))
builtin/config.c int cmd_config(int argc, const char **argv, const char *prefix) · line 627
Quoted 11 lines
builtin/config.c +7
-0@@ -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
Quoted 16 lines · Jeff King
Quoted 5 lines
Documentation/git-config.txt +5
-0+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, orDocumentation/git-config.txt See also <<FILES>>. · line 180
Quoted 10 lines
Documentation/git-config.txt +10
-0+ 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. ++
Quoted 1 line
Yay. It's nice to have this in only one place now.
Thanks! Agreed :-).
Quoted 1 line
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.
Quoted 8 lines
builtin/config.c static char *default_user_config(void) · line 501
Quoted 4 lines
builtin/config.c +4
-0+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.
Quoted 26 lines
builtin/config.c int cmd_config(int argc, const char **argv, const char *prefix) · line 627
Quoted 11 lines
builtin/config.c +7
-0@@ -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=boolSo 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
Quoted 7 lines
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=boolI 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.
Quoted 2 lines
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.
Quoted 8 lines
I think it would be pretty simple. One of:
- convert
OPT_BIT("bool")intoOPT_CALLBACK("bool")and just assign "bool" to the "type" string, which will then later get parsed intoTYPE_BOOL.or
Quoted 7 lines
- convert
OPT_BIT("bool")intoOPT_SET_INT("bool")to setTYPE_BOOLdirectly. Convert
OPT_STRING("type")intoOPT_CALLBACK(), and have it assign the result oftype_name_to_specifier()directly.I'd probably do the latter, but would be fine with either (and I'd make the
OPT_SET_INTthing 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.
Quoted 3 lines
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
Quoted 6 lines · Taylor Blau
Quoted 3 lines
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(-)
builtin/config.c +72 -65
@@ -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 ]);
Signature · 1 line
2.17.0.rc2.594.gdb94a0ce02
Quoted 7 lines · Jeff King
... 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.
Quoted 23 lines · Junio C Hamano
Quoted 7 lines · Jeff King
... 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.
Quoted 4 lines
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
In reply to Taylor BlauInternally, 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.
--- builtin/config.c | 39 +++++++++++++++++---------------------- t/t1300-repo-config.sh | 11 +++++++++++ 2 files changed, 28 insertions(+), 22 deletions(-)
builtin/config.c +17 -22
@@ -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 *valueif (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); }
t/t1300-repo-config.sh +11 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
PATCH v2 2/2 builtin/config.c: prefer `--type=bool` over `--bool`, etc.
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.
--- Documentation/git-config.txt | 66 +++++++++++++++++++----------------- builtin/config.c | 23 +++++++++++++ t/t1300-repo-config.sh | 18 ++++++++++ 3 files changed, 75 insertions(+), 32 deletions(-)
Documentation/git-config.txt +34 -32
@@ -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::
builtin/config.c +23 -0
@@ -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),
t/t1300-repo-config.sh +18 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
Am 30.03.2018 um 07:28 schrieb Taylor Blau:
Quoted 9 lines
Documentation/git-config.txt +0
-1@@ -160,30 +158,34 @@ See also <<FILES>>. --list:: List all variables set in config file, along with their values. ---bool::
Documentation/git-config.txt See also <<FILES>>. · line 169
Quoted 10 lines
Documentation/git-config.txt +9
-1- '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é
Quoted 11 lines · Taylor Blau
builtin/config.c +11
-0+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 ;
builtin/config.c static int show_origin; · line 84
Quoted 10 lines
builtin/config.c +10
-0+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".
Quoted 10 lines
t/t1300-repo-config.sh +6
-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 +' +
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1641
Quoted 10 lines
t/t1300-repo-config.sh +10
-0+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
In reply to Taylor BlauQuoted 17 lines · Taylor Blau
Internally, we represent
git config's type specifiers as a bitset usingOPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique allows for the representation of multiple type specifiers in theint typesfield, but this multi-representation is left unused.In fact,
git configwill 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_INTto prefer the _last_ mentioned type specifier, so that the above command would instead be valid, and a synonym of:$ git config --bool some.section
Quoted 8 lines
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_INTis more well-suited to this task thanOPT_BITis. (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...
Quoted 9 lines
Where--although the legacy and modern type specifier (
--int, and--type, respectively) do not conflict--we would store the value of--type=in achar *and the--intas a bit in theintbitset.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--intand--type=inta 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.
Quoted 5 lines
Signed-off-by Taylor Blau <[email protected]>--- 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.
Quoted 8 lines
t/t1300-repo-config.sh +0
-0@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' ' test_expect_code 128 nongit git config --local foo.bar '
t/t1300-repo-config.sh test_expect_success '--local requires a repo' ' · line 1623
Quoted 10 lines
t/t1300-repo-config.sh +10
-0+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
v3 cover builtin/config.c: prefer `--type=bool` over `--bool`, etc.
In reply to Taylor BlauHi,
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
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
--- builtin/config.c | 49 +++++++++++++++++++----------------------- t/t1300-repo-config.sh | 11 ++++++++++ 2 files changed, 33 insertions(+), 27 deletions(-)
builtin/config.c +22 -27
@@ -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 5static 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 *valueif (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); }
t/t1300-repo-config.sh +11 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
Re: [PATCH v3 1/2] builtin/config.c: treat type specifiers singularly
Quoted 13 lines · Taylor Blau
[...]
In fact,git configwill 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_INTto prefer the _last_ mentioned type specifier, so that the above command would instead be valid, and a synonym of:$ git config --bool some.section
Quoted 8 lines
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_INTis more well-suited to this task thanOPT_BITis. (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
Quoted 31 lines · Eric Sunshine
Quoted 13 lines · Taylor Blau
[...]
In fact,git configwill 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_INTto prefer the _last_ mentioned type specifier, so that the above command would instead be valid, and a synonym of:$ git config --bool some.sectionQuoted 8 lines
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_INTis more well-suited to this task thanOPT_BITis. (2) a future patch will introduce--type=<type>, and we would like not to complain in the following situation:$ git config --int --type=intI 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.
Quoted 7 lines
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
Quoted 18 lines · Taylor Blau
Quoted 7 lines
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.
Quoted 7 lines
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.
In reply to Taylor Blaugit 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.
--- Documentation/git-config.txt | 66 +++++++++++++++++++----------------- builtin/config.c | 28 +++++++++++++++ t/t1300-repo-config.sh | 18 ++++++++++ 3 files changed, 80 insertions(+), 32 deletions(-)
Documentation/git-config.txt +34 -32
@@ -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::
builtin/config.c +28 -0
@@ -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),
t/t1300-repo-config.sh +18 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
Quoted 16 lines · Taylor Blau
git confighas long allowed the ability for callers to provide a 'type specifier', which instructsgit configto (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
--colorand--defaultto replace--get-color.However, we traditionally use
--colorto mean "colorize this output", instead of "this value should be treated as a color".Currently,
git configdoes not support this kind of colorization, but we should be careful to avoid inhabiting this option too soon, so thatgit configcan support--color(in the traditional sense) in the future, if that is desired.
Quoted 4 lines
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=...".
Documentation/git-config.txt See also <<FILES>>. · line 161
Quoted 8 lines
Signed-off-by Taylor Blau <[email protected]>---
Documentation/git-config.txt +4
-0@@ -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.
Quoted 8 lines
builtin/config.c +6
-0@@ -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 ;
builtin/config.c static int show_origin; · line 76
Quoted 10 lines
builtin/config.c +10
-0+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"?
Quoted 4 lines
builtin/config.c +4
-0+return 1 ; + } +return 0 ; +}
Quoted 6 lines · Eric Sunshine
Quoted 4 lines · Taylor Blau
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.
Quoted 7 lines
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.
Quoted 4 lines
Quoted 4 lines
Signed-off-by Taylor Blau <[email protected]>---
Documentation/git-config.txt +0
-0@@ -160,30 +158,34 @@ See also <<FILES>>.
Quoted 8 lines
Documentation/git-config.txt See also <<FILES>>. · line 164
Quoted 4 lines
Documentation/git-config.txt +4
-0+--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.
Quoted 13 lines
Quoted 13 lines
builtin/config.c +11
-0@@ -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" ))
Quoted 8 lines
builtin/config.c static int show_origin; · line 85
Quoted 5 lines
builtin/config.c +5
-0+ *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
v4 cover builtin/config.c: prefer `--type=bool` over `--bool`, etc.
In reply to Taylor BlauHi,
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
Quoted 5 lines · Taylor Blau
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:
Quoted 7 lines
- 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
Quoted 12 lines · Jeff King
Quoted 10 lines · Taylor Blau
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).
Quoted 7 lines
Quoted 2 lines
- 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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v4 1/2 builtin/config.c: treat type specifiers singularly
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
--- builtin/config.c | 49 +++++++++++++++++++----------------------- t/t1300-repo-config.sh | 11 ++++++++++ 2 files changed, 33 insertions(+), 27 deletions(-)
builtin/config.c +22 -27
@@ -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 5static 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 *valueif (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); }
t/t1300-repo-config.sh +11 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
PATCH v4 2/2 builtin/config.c: prefer `--type=bool` over `--bool`, etc.
In reply to a message not in the archivegit 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=....
--- Documentation/git-config.txt | 66 +++++++++++++++++++----------------- builtin/config.c | 28 +++++++++++++++ t/t1300-repo-config.sh | 18 ++++++++++ 3 files changed, 80 insertions(+), 32 deletions(-)
Documentation/git-config.txt +34 -32
@@ -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::
builtin/config.c +28 -0
@@ -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),
t/t1300-repo-config.sh +18 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1636
Quoted 5 lines · Taylor Blau
t/t1300-repo-config.sh +5
-0+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
Quoted 10 lines · Jeff King
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1636
Quoted 5 lines · Taylor Blau
t/t1300-repo-config.sh +5
-0+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
Quoted 10 lines · Jeff King
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1636
Quoted 5 lines · Taylor Blau
t/t1300-repo-config.sh +5
-0+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 v4 2/2 builtin/config.c: prefer `--type=bool` over `--bool`, etc.
In reply to a message not in the archivegit 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=....
--- Documentation/git-config.txt | 66 +++++++++++++++++++----------------- builtin/config.c | 28 +++++++++++++++ t/t1300-repo-config.sh | 18 ++++++++++ 3 files changed, 80 insertions(+), 32 deletions(-)
Documentation/git-config.txt +34 -32
@@ -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::
builtin/config.c +28 -0
@@ -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),
t/t1300-repo-config.sh +18 -0
@@ -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
Signature · 1 line
2.16.2.440.gc6284da4f
Documentation/git-config.txt existing values that match the regexp are updated or unset. If · line 38
Quoted 11 lines · Taylor Blau
[...]
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 <[email protected]>---
Documentation/git-config.txt +1
-0@@ -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.)
Documentation/git-config.txt See also <<FILES>>. · line 164
Quoted 13 lines
Documentation/git-config.txt +7
-2+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...
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1627
Quoted 7 lines
t/t1300-repo-config.sh +5
-0@@ -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/[email protected]/
[2]: https://public-inbox.org/git/CAPig+cR4uFiC_gFsb2e9JR6SdP-wUQVz-E0MjRHR=vNHd+hvhA@mail.gmail.com/
Quoted 7 lines · Eric Sunshine
Quoted 7 lines
Documentation/git-config.txt +3
-0+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.
Quoted 10 lines
Documentation/git-config.txt See also <<FILES>>. · line 164
Quoted 6 lines
Documentation/git-config.txt +4
-2---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?)
Quoted 4 lines
Quoted 4 lines
t/t1300-repo-config.sh +2
-0@@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' ' +test_expect_success '--no-type unsets type specifiers' ' + echo "10" > expect &&
Quoted 8 lines
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1636
Quoted 3 lines
t/t1300-repo-config.sh +3
-0+ 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
Quoted 20 lines · Jeff King
Quoted 7 lines · Eric Sunshine
Quoted 7 lines
Documentation/git-config.txt +3
-0+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.Quoted 10 lines
Documentation/git-config.txt See also <<FILES>>. · line 164
Quoted 6 lines
Documentation/git-config.txt +4
-2---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...
Quoted 2 lines
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.
Quoted 16 lines
Quoted 4 lines
Quoted 4 lines
t/t1300-repo-config.sh +2
-0@@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' ' +test_expect_success '--no-type unsets type specifiers' ' + echo "10" > expect &&Quoted 8 lines
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1636
Quoted 3 lines
t/t1300-repo-config.sh +3
-0+ 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:
Quoted 6 lines
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
Quoted 17 lines · Eric Sunshine
Documentation/git-config.txt existing values that match the regexp are updated or unset. If · line 38
Quoted 11 lines · Taylor Blau
[...]
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 <[email protected]>---
Documentation/git-config.txt +1
-0@@ -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:
Quoted 1 line
The --type option requests
git configto ...
Thanks; I agree that this is much clearer.
Quoted 5 lines
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.
Quoted 4 lines
Quoted 4 lines
t/t1300-repo-config.sh +2
-0@@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' ' +test_expect_success '--no-type unsets type specifiers' ' + echo "10" > expect &&
Quoted 8 lines
t/t1300-repo-config.sh test_expect_success 'later legacy specifiers are given precedence' ' · line 1636
Quoted 3 lines
t/t1300-repo-config.sh +3
-0+ 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
v6 cover builtin/config.c: support `--type=<type>` as preferred alias for `--type`
In reply to Taylor BlauHi,
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/[email protected]/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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v6 1/2 builtin/config.c: treat type specifiers singularly
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
--- builtin/config.c | 49 +++++++++++++++++++----------------------- t/t1300-repo-config.sh | 11 ++++++++++ 2 files changed, 33 insertions(+), 27 deletions(-)
builtin/config.c +22 -27
@@ -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 5static 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 *valueif (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); }
t/t1300-repo-config.sh +11 -0
@@ -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`
In reply to a message not in the archivegit 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.
--- Documentation/git-config.txt | 73 ++++++++++++++++++++---------------- builtin/config.c | 27 +++++++++++++ t/t1300-repo-config.sh | 18 +++++++++ 3 files changed, 86 insertions(+), 32 deletions(-)
Documentation/git-config.txt +41 -32
@@ -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::
builtin/config.c +27 -0
@@ -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),
t/t1300-repo-config.sh +18 -0
@@ -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`
Quoted 10 lines · Taylor Blau
[...]
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 <[email protected]>---
Documentation/git-config.txt existing values that match the regexp are updated or unset. If · line 43
Quoted 6 lines
Documentation/git-config.txt +6
-0@@ -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`
Quoted 7 lines · Eric Sunshine
Quoted 6 lines · Taylor Blau
[...]
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.
Quoted 15 lines
Documentation/git-config.txt existing values that match the regexp are updated or unset. If · line 43
Quoted 10 lines
Signed-off-by Taylor Blau <[email protected]>---
Documentation/git-config.txt +6
-0@@ -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?
Quoted 3 lines
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`
Quoted 6 lines · Taylor Blau
Quoted 4 lines · Eric Sunshine
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.
Quoted 7 lines
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--typespecifier with--no-type.
This sounds fine. (Maybe s/the existing/an existing/)
Quoted 4 lines
I think documenting that
--no-typeunsets any pre-existing--typespecifier 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.
Quoted 8 lines
Quoted 3 lines
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).
Quoted 1 line
after which I think it will be ready for queueing as-is.
Famous last words ;-)
v7 cover builtin/config.c: support `--type=<type>` as preferred alias for `--type`
In reply to Taylor BlauHi,
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):
Documentation/git-config.txt +4 -6
@@ -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`
Quoted 8 lines · Taylor Blau
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.)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v7 1/2 builtin/config.c: treat type specifiers singularly
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
--- builtin/config.c | 49 +++++++++++++++++++----------------------- t/t1300-repo-config.sh | 11 ++++++++++ 2 files changed, 33 insertions(+), 27 deletions(-)
builtin/config.c +22 -27
@@ -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 5static 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 *valueif (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); }
t/t1300-repo-config.sh +11 -0
@@ -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
Signature · 1 line
2.17.0
Re: [PATCH v7 1/2] builtin/config.c: treat type specifiers singularly
Quoted 17 lines · Taylor Blau
Internally, we represent
git config's type specifiers as a bitset usingOPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique allows for the representation of multiple type specifiers in theint typesfield, but this multi-representation is left unused.In fact,
git configwill 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_INTto prefer the _last_ mentioned type specifier, so that the above command would instead be valid, and a synonym of:$ git config --bool some.section
Quoted 8 lines
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_INTis more well-suited to this task thanOPT_BITis. (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").
Quoted 8 lines
t/t1300-repo-config.sh +0
-0@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' ' test_expect_code 128 nongit git config --local foo.bar '
t/t1300-repo-config.sh test_expect_success '--local requires a repo' ' · line 1623
Quoted 10 lines
t/t1300-repo-config.sh +10
-0+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
Quoted 45 lines · Junio C Hamano
Quoted 17 lines · Taylor Blau
Internally, we represent
git config's type specifiers as a bitset usingOPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique allows for the representation of multiple type specifiers in theint typesfield, but this multi-representation is left unused.In fact,
git configwill 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_INTto prefer the _last_ mentioned type specifier, so that the above command would instead be valid, and a synonym of:$ git config --bool some.sectionQuoted 8 lines
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_INTis more well-suited to this task thanOPT_BITis. (2) a future patch will introduce--type=<type>, and we would like not to complain in the following situation:$ git config --int --type=intThe 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=intIs 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.
Quoted 4 lines
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:
builtin/config.c +5 -5
@@ -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.
Quoted 19 lines
Quoted 8 lines
t/t1300-repo-config.sh +0
-0@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' ' test_expect_code 128 nongit git config --local foo.bar 't/t1300-repo-config.sh test_expect_success '--local requires a repo' ' · line 1623
Quoted 10 lines
t/t1300-repo-config.sh +10
-0+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 +'
Quoted 7 lines
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_INTis more suited to complain when two conflicting things are given thanOPT_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
Quoted 11 lines · Taylor Blau
Quoted 4 lines · Junio C Hamano
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 thatOPT_SET_BITbrings 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).
Quoted 19 lines
I think that
OPT_CMDMODEwould 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),builtin/config.c +5
-5-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
Quoted 3 lines
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`
In reply to a message not in the archivegit 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.
--- Documentation/git-config.txt | 71 ++++++++++++++++++++---------------- builtin/config.c | 27 ++++++++++++++ t/t1300-repo-config.sh | 18 +++++++++ 3 files changed, 84 insertions(+), 32 deletions(-)
Documentation/git-config.txt +39 -32
@@ -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::
builtin/config.c +27 -0
@@ -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),
t/t1300-repo-config.sh +18 -0
@@ -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
Signature · 1 line
2.17.0
Re: [PATCH v7 2/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type`
Quoted 4 lines · Taylor Blau
git confighas long allowed the ability for callers to provide a 'type specifier', which instructsgit configto (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?
Quoted 5 lines
In another series, we propose to extend this functionality with
--type=colorand--defaultto replace--get-color.However, we traditionally use
--colorto mean "colorize this output", instead of "this value should be treated as a color".
Makes sense.
Quoted 4 lines
Currently,
git configdoes not support this kind of colorization, but we should be careful to avoid inhabiting this option too soon, so thatgit configcan 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/?
Quoted 4 lines
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.
Quoted 7 lines
Documentation/git-config.txt +1
-2@@ -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>::
Documentation/git-config.txt See also <<FILES>>. · line 171
Quoted 10 lines
Documentation/git-config.txt +10
-0+ '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`
Quoted 10 lines · Junio C Hamano
Quoted 4 lines · Taylor Blau
git confighas long allowed the ability for callers to provide a 'type specifier', which instructsgit configto (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.
Quoted 13 lines
Quoted 5 lines
In another series, we propose to extend this functionality with
--type=colorand--defaultto replace--get-color.However, we traditionally use
--colorto mean "colorize this output", instead of "this value should be treated as a color".Makes sense.
Quoted 4 lines
Currently,
git configdoes not support this kind of colorization, but we should be careful to avoid inhabiting this option too soon, so thatgit configcan support--type=color(in the traditional sense) in the future, if that is desired.
Quoted 4 lines
Shouldn't the above
--colorinstead? That is, we avoid squatting on--colorbecause 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.
Quoted 29 lines
Quoted 4 lines
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.
Quoted 7 lines
Documentation/git-config.txt +1
-2@@ -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>::Documentation/git-config.txt See also <<FILES>>. · line 171
Quoted 10 lines
Documentation/git-config.txt +10
-0+ '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.
Quoted 8 lines
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
v8 cover builtin/config.c: support `--type=<type>` as preferred alias for `--type`
In reply to Taylor BlauHi,
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):
builtin/config.c +46 -21
@@ -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")),
t/t1300-repo-config.sh +37 -3
@@ -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`
Quoted 34 lines · Taylor Blau
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 --intWe 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.
Quoted 7 lines
This is accomplished by a new locally defined macro called
OPT_CALLBACK_VALUE, which allows us to reuseoption_parse_type()to handle --int as well, by sending it through asopt->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.
builtin/config.c static int show_origin; · line 68
Quoted 13 lines
builtin/config.c +5
-0@@ -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.
Quoted 46 lines
builtin/config.c +24
-15static 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.
builtin/config.c static int show_origin; · line 110
Quoted 10 lines
builtin/config.c +10
-0+* 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)?
Quoted 3 lines
builtin/config.c +0
-0return 0 ; }
Re: [PATCH v8 0/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type`
Quoted 44 lines · Junio C Hamano
Quoted 34 lines · Taylor Blau
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 --intWe 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.
Quoted 7 lines
This is accomplished by a new locally defined macro called
OPT_CALLBACK_VALUE, which allows us to reuseoption_parse_type()to handle --int as well, by sending it through asopt->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.
Quoted 6 lines
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.
Quoted 8 lines
Quoted 8 lines
builtin/config.c +0
-0@@ -61,28 +61,53 @@ static int show_origin;#define TYPE_PATH 4#define TYPE_EXPIRY_DATE 5
Quoted 8 lines
builtin/config.c static int show_origin; · line 68
Quoted 5 lines
builtin/config.c +5
-0+#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 theoption_parse_typefunction 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.
Quoted 50 lines
Quoted 50 lines
builtin/config.c +28
-15static 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'.
Quoted 8 lines
builtin/config.c static int show_origin; · line 109
Quoted 6 lines
builtin/config.c +6
-0+*/ +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")?
builtin/config.c +12 -11
@@ -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`
Quoted 9 lines · Taylor Blau
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+#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 theoption_parse_typefunction to it.
Quoted 4 lines
That's fair. I left this in as an indication that something like this _might_ want to make its way into
parse-options.has a general-purpose utility, but was not yet ready to do so. Thus, I defined it insidebuiltin/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.
builtin/config.c static int option_parse_type(const struct option *opt, const char *arg, · line 76
Quoted 8 lines
builtin/config.c +1
-1@@ -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`
Quoted 16 lines · Junio C Hamano
Quoted 9 lines · Taylor Blau
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+#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 theoption_parse_typefunction to it.Quoted 4 lines
That's fair. I left this in as an indication that something like this _might_ want to make its way into
parse-options.has a general-purpose utility, but was not yet ready to do so. Thus, I defined it insidebuiltin/config.c.
Quoted 4 lines
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.cfile.
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v8 1/2 builtin/config.c: treat type specifiers singularly
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
--- builtin/config.c | 49 +++++++++++++++++++----------------------- t/t1300-repo-config.sh | 11 ++++++++++ 2 files changed, 33 insertions(+), 27 deletions(-)
builtin/config.c +22 -27
@@ -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 5static 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 *valueif (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); }
t/t1300-repo-config.sh +11 -0
@@ -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
Signature · 1 line
2.17.0
PATCH v8 2/2 builtin/config.c: support `--type=<type>` as preferred alias for `--type`
In reply to a message not in the archivegit 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.
--- Documentation/git-config.txt | 71 ++++++++++++++++++++---------------- builtin/config.c | 62 ++++++++++++++++++++++++++++--- t/t1300-repo-config.sh | 58 +++++++++++++++++++++++++++-- 3 files changed, 151 insertions(+), 40 deletions(-)
Documentation/git-config.txt +39 -32
@@ -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::
builtin/config.c +57 -5
74 lines not on this page · raw message
t/t1300-repo-config.sh +55 -3
68 lines not on this page · raw message
Signature · 1 line
2.17.0
v9 cover builtin/config.c: support `--type=<type>` as preferred alias for `--type`
In reply to Taylor BlauHi,
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):
builtin/config.c +13 -12
@@ -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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v9 1/2 builtin/config.c: treat type specifiers singularly
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
--- builtin/config.c | 49 +++++++++++++++++++----------------------- t/t1300-repo-config.sh | 11 ++++++++++ 2 files changed, 33 insertions(+), 27 deletions(-)
builtin/config.c +22 -27
@@ -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 5static 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 *valueif (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); }
t/t1300-repo-config.sh +11 -0
@@ -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
Signature · 1 line
2.17.0
PATCH v9 2/2 builtin/config.c: support `--type=<type>` as preferred alias for `--type`
In reply to a message not in the archivegit 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.
--- Documentation/git-config.txt | 71 ++++++++++++++++++++---------------- builtin/config.c | 63 +++++++++++++++++++++++++++++--- t/t1300-repo-config.sh | 58 +++++++++++++++++++++++++++-- 3 files changed, 152 insertions(+), 40 deletions(-)
Documentation/git-config.txt +39 -32
@@ -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::
builtin/config.c +58 -5
75 lines not on this page · raw message
t/t1300-repo-config.sh +55 -3
68 lines not on this page · raw message
Signature · 1 line
2.17.0
Re: [PATCH v9 2/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type`
Quoted 7 lines · Taylor Blau
builtin/config.c +0
-0@@ -61,6 +61,58 @@ static int show_origin;#define TYPE_PATH 4#define TYPE_EXPIRY_DATE 5
builtin/config.c static int show_origin; · line 72
Quoted 10 lines
builtin/config.c +9
-0+#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.
builtin/config.c static int show_origin; · line 82
Quoted 10 lines
builtin/config.c +10
-0+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 ...
Quoted 6 lines
builtin/config.c +6
-0+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" ))
builtin/config.c static int show_origin; · line 98
Quoted 10 lines
builtin/config.c +10
-0+ 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.
Quoted 17 lines
builtin/config.c +17
-0+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`
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Taylor Blau
builtin/config.c +0
-0@@ -61,6 +61,58 @@ static int show_origin;#define TYPE_PATH 4#define TYPE_EXPIRY_DATE 5
Quoted 11 lines
builtin/config.c static int show_origin; · line 72
Quoted 9 lines
builtin/config.c +9
-0+#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:
builtin/config.c +5 -2
@@ -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