# [PATCH B v4 2/5] git config: don't allow multiple config file locations 13 messages from 2009-02-21 to 2009-03-07. Participants: Felipe Contreras, Junio C Hamano. Thread: https://gitlist.dev/t/17931 ## Felipe Contreras, 2009-02-21 00:49 Subject: [PATCH B v4 0/5] git config: reorganization Message-ID: <1235177369-2727-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1235177369-2727-1-git-send-email-felipe.contreras%40gmail.com ``` This patch series is the first of a two parts reorganization for more maintainability and user friendliness. The main change is the switch to use parseopt which provides a nicer help usage dialog. The rest of the changes take advantage of the initial reorganization to do more strict option parsing. Felipe Contreras (5): git config: reorganize to use parseopt git config: don't allow multiple config file locations git config: don't allow multiple variable types git config: don't allow extra arguments for -e or -l. git config: don't allow --get-color* and variable type builtin-config.c | 385 +++++++++++++++++++++++++++++++----------------------- 1 files changed, 223 insertions(+), 162 deletions(-) ``` ## Felipe Contreras, 2009-02-21 00:49 Subject: [PATCH B v4 1/5] git config: reorganize to use parseopt Message-ID: <1235177369-2727-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1235177369-2727-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1235177369-2727-1-git-send-email-felipe.contreras@gmail.com> ``` This patch has benefited from comments by Johannes Schindelin and Junio C Hamano. Signed-off-by: Felipe Contreras --- builtin-config.c | 349 ++++++++++++++++++++++++++++++------------------------ 1 files changed, 195 insertions(+), 154 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 0836880..302846e 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -1,9 +1,12 @@ #include "builtin.h" #include "cache.h" #include "color.h" +#include "parse-options.h" -static const char git_config_set_usage[] = -"git config [ --global | --system | [ -f | --file ] config-file ] [ --bool | --int | --bool-or-int ] [ -z | --null ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list | --get-color var [default] | --get-colorbool name [stdout-is-tty] | --edit | -e ]"; +static const char *const builtin_config_usage[] = { + "git config [options]", + NULL +}; static char *key; static regex_t *key_regexp; @@ -18,6 +21,63 @@ static char key_delim = ' '; static char term = '\n'; static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW; +static int use_global_config, use_system_config; +static const char *given_config_file; +static int actions; +static const char *get_color_slot, *get_colorbool_slot; +static int end_null; + +#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) + +static struct option builtin_config_options[] = { + OPT_GROUP("Config file location"), + OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"), + OPT_BOOLEAN(0, "system", &use_system_config, "use system config file"), + OPT_STRING('f', "file", &given_config_file, "FILE", "use given config file"), + OPT_GROUP("Action"), + OPT_BIT(0, "get", &actions, "get value: name [value-regex]", ACTION_GET), + OPT_BIT(0, "get-all", &actions, "get all values: key [value-regex]", ACTION_GET_ALL), + OPT_BIT(0, "get-regexp", &actions, "get values for regexp: name-regex [value-regex]", ACTION_GET_REGEXP), + OPT_BIT(0, "replace-all", &actions, "replace all matching variables: name [value [value_regex]", ACTION_REPLACE_ALL), + OPT_BIT(0, "add", &actions, "adds a new variable: name value", ACTION_ADD), + OPT_BIT(0, "unset", &actions, "removes a variable: name [value-regex]", ACTION_UNSET), + OPT_BIT(0, "unset-all", &actions, "removes all matches: name [value-regex]", ACTION_UNSET_ALL), + OPT_BIT(0, "rename-section", &actions, "rename section: old-name new-name", ACTION_RENAME_SECTION), + OPT_BIT(0, "remove-section", &actions, "remove a section: name", ACTION_REMOVE_SECTION), + OPT_BIT('l', "list", &actions, "list all", ACTION_LIST), + OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT), + OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"), + OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"), + OPT_GROUP("Type"), + OPT_SET_INT(0, "bool", &type, "value is \"true\" or \"false\"", T_BOOL), + OPT_SET_INT(0, "int", &type, "value is decimal number", T_INT), + OPT_SET_INT(0, "bool-or-int", &type, NULL, T_BOOL_OR_INT), + OPT_GROUP("Other"), + OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"), + OPT_END(), +}; + +static void check_argc(int argc, int min, int max) { + if (argc >= min && argc <= max) + return; + error("wrong number of arguments"); + usage_with_options(builtin_config_usage, builtin_config_options); +} + static int show_all_config(const char *key_, const char *value_, void *cb) { if (value_) @@ -252,162 +312,143 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) config_exclusive_filename = getenv(CONFIG_ENVIRONMENT); - while (1 < argc) { - if (!strcmp(argv[1], "--int")) - type = T_INT; - else if (!strcmp(argv[1], "--bool")) - type = T_BOOL; - else if (!strcmp(argv[1], "--bool-or-int")) - type = T_BOOL_OR_INT; - else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) { - if (argc != 2) - usage(git_config_set_usage); - if (git_config(show_all_config, NULL) < 0) { - if (config_exclusive_filename) - die("unable to read config file %s: %s", - config_exclusive_filename, strerror(errno)); - else - die("error processing config file(s)"); - } - return 0; - } - else if (!strcmp(argv[1], "--global")) { - char *home = getenv("HOME"); - if (home) { - char *user_config = xstrdup(mkpath("%s/.gitconfig", home)); - config_exclusive_filename = user_config; - } else { - die("$HOME not set"); - } - } - else if (!strcmp(argv[1], "--system")) - config_exclusive_filename = git_etc_gitconfig(); - else if (!strcmp(argv[1], "--file") || !strcmp(argv[1], "-f")) { - if (argc < 3) - usage(git_config_set_usage); - if (!is_absolute_path(argv[2]) && prefix) - config_exclusive_filename = prefix_filename(prefix, - strlen(prefix), - argv[2]); - else - config_exclusive_filename = argv[2]; - argc--; - argv++; - } - else if (!strcmp(argv[1], "--null") || !strcmp(argv[1], "-z")) { - term = '\0'; - delim = '\n'; - key_delim = '\n'; - } - else if (!strcmp(argv[1], "--rename-section")) { - int ret; - if (argc != 4) - usage(git_config_set_usage); - ret = git_config_rename_section(argv[2], argv[3]); - if (ret < 0) - return ret; - if (ret == 0) { - fprintf(stderr, "No such section!\n"); - return 1; - } - return 0; - } - else if (!strcmp(argv[1], "--remove-section")) { - int ret; - if (argc != 3) - usage(git_config_set_usage); - ret = git_config_rename_section(argv[2], NULL); - if (ret < 0) - return ret; - if (ret == 0) { - fprintf(stderr, "No such section!\n"); - return 1; - } - return 0; - } else if (!strcmp(argv[1], "--get-color")) { - if (argc > 4 || argc < 3) - usage(git_config_set_usage); - get_color_slot = argv[2]; - get_color(argv[3]); - return 0; - } else if (!strcmp(argv[1], "--get-colorbool")) { - if (argc == 4) - stdout_is_tty = git_config_bool("command line", argv[3]); - else if (argc == 3) - stdout_is_tty = isatty(1); - else - usage(git_config_set_usage); - get_colorbool_slot = argv[2]; - return get_colorbool(argc != 3); - } else if (!strcmp(argv[1], "--edit") || !strcmp(argv[1], "-e")) { - if (argc != 2) - usage(git_config_set_usage); - git_config(git_default_config, NULL); - launch_editor(config_exclusive_filename ? - config_exclusive_filename : git_path("config"), - NULL, NULL); - return 0; - } else - break; - argc--; - argv++; - } - - switch (argc) { - case 2: - return get_value(argv[1], NULL); - case 3: - if (!strcmp(argv[1], "--unset")) - return git_config_set(argv[2], NULL); - else if (!strcmp(argv[1], "--unset-all")) - return git_config_set_multivar(argv[2], NULL, NULL, 1); - else if (!strcmp(argv[1], "--get")) - return get_value(argv[2], NULL); - else if (!strcmp(argv[1], "--get-all")) { - do_all = 1; - return get_value(argv[2], NULL); - } else if (!strcmp(argv[1], "--get-regexp")) { - show_keys = 1; - use_key_regexp = 1; - do_all = 1; - return get_value(argv[2], NULL); + argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, + PARSE_OPT_STOP_AT_NON_OPTION); + + if (use_global_config) { + char *home = getenv("HOME"); + if (home) { + char *user_config = xstrdup(mkpath("%s/.gitconfig", home)); + config_exclusive_filename = user_config; } else { - value = normalize_value(argv[1], argv[2]); - return git_config_set(argv[1], value); + die("$HOME not set"); } - case 4: - if (!strcmp(argv[1], "--unset")) - return git_config_set_multivar(argv[2], NULL, argv[3], 0); - else if (!strcmp(argv[1], "--unset-all")) - return git_config_set_multivar(argv[2], NULL, argv[3], 1); - else if (!strcmp(argv[1], "--get")) - return get_value(argv[2], argv[3]); - else if (!strcmp(argv[1], "--get-all")) { - do_all = 1; - return get_value(argv[2], argv[3]); - } else if (!strcmp(argv[1], "--get-regexp")) { - show_keys = 1; - use_key_regexp = 1; - do_all = 1; - return get_value(argv[2], argv[3]); - } else if (!strcmp(argv[1], "--add")) { - value = normalize_value(argv[2], argv[3]); - return git_config_set_multivar(argv[2], value, "^$", 0); - } else if (!strcmp(argv[1], "--replace-all")) { - value = normalize_value(argv[2], argv[3]); - return git_config_set_multivar(argv[2], value, NULL, 1); - } else { - value = normalize_value(argv[1], argv[2]); - return git_config_set_multivar(argv[1], value, argv[3], 0); + } + else if (use_system_config) + config_exclusive_filename = git_etc_gitconfig(); + else if (given_config_file) { + if (!is_absolute_path(given_config_file) && prefix) + config_exclusive_filename = prefix_filename(prefix, + strlen(prefix), + argv[2]); + else + config_exclusive_filename = given_config_file; + } + + if (end_null) { + term = '\0'; + delim = '\n'; + key_delim = '\n'; + } + + if (get_color_slot) + actions |= ACTION_GET_COLOR; + if (get_colorbool_slot) + actions |= ACTION_GET_COLORBOOL; + + if (HAS_MULTI_BITS(actions)) { + error("only one action at a time."); + usage_with_options(builtin_config_usage, builtin_config_options); + } + if (actions == 0) + switch (argc) { + case 1: actions = ACTION_GET; break; + case 2: actions = ACTION_SET; break; + case 3: actions = ACTION_SET_ALL; break; + default: + usage_with_options(builtin_config_usage, builtin_config_options); } - case 5: - if (!strcmp(argv[1], "--replace-all")) { - value = normalize_value(argv[2], argv[3]); - return git_config_set_multivar(argv[2], value, argv[4], 1); + + if (actions == ACTION_LIST) { + if (git_config(show_all_config, NULL) < 0) { + if (config_exclusive_filename) + die("unable to read config file %s: %s", + config_exclusive_filename, strerror(errno)); + else + die("error processing config file(s)"); } - case 1: - default: - usage(git_config_set_usage); } + else if (actions == ACTION_EDIT) { + git_config(git_default_config, NULL); + launch_editor(config_exclusive_filename ? + config_exclusive_filename : git_path("config"), + NULL, NULL); + } + else if (actions == ACTION_SET) { + check_argc(argc, 2, 2); + value = normalize_value(argv[0], argv[1]); + return git_config_set(argv[0], value); + } + else if (actions == ACTION_SET_ALL) { + check_argc(argc, 2, 3); + value = normalize_value(argv[0], argv[1]); + return git_config_set_multivar(argv[0], value, argv[2], 0); + } + else if (actions == ACTION_ADD) { + check_argc(argc, 2, 2); + value = normalize_value(argv[0], argv[1]); + return git_config_set_multivar(argv[0], value, "^$", 0); + } + else if (actions == ACTION_REPLACE_ALL) { + check_argc(argc, 2, 3); + value = normalize_value(argv[0], argv[1]); + return git_config_set_multivar(argv[0], value, argv[2], 1); + } + else if (actions == ACTION_GET) { + check_argc(argc, 1, 2); + return get_value(argv[0], argv[1]); + } + else if (actions == ACTION_GET_ALL) { + do_all = 1; + check_argc(argc, 1, 2); + return get_value(argv[0], argv[1]); + } + else if (actions == 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_UNSET) { + check_argc(argc, 1, 2); + if (argc == 2) + return git_config_set_multivar(argv[0], NULL, argv[1], 0); + else + return git_config_set(argv[0], NULL); + } + else if (actions == ACTION_UNSET_ALL) { + check_argc(argc, 1, 2); + return git_config_set_multivar(argv[0], NULL, argv[1], 1); + } + else if (actions == ACTION_RENAME_SECTION) { + int ret; + check_argc(argc, 2, 2); + ret = git_config_rename_section(argv[0], argv[1]); + if (ret < 0) + return ret; + if (ret == 0) + die("No such section!"); + } + else if (actions == ACTION_REMOVE_SECTION) { + int ret; + check_argc(argc, 1, 1); + ret = git_config_rename_section(argv[0], NULL); + if (ret < 0) + return ret; + if (ret == 0) + die("No such section!"); + } + else if (actions == ACTION_GET_COLOR) { + get_color(argv[0]); + } + else if (actions == ACTION_GET_COLORBOOL) { + if (argc == 1) + stdout_is_tty = git_config_bool("command line", argv[0]); + else if (argc == 0) + stdout_is_tty = isatty(1); + return get_colorbool(argc != 0); + } + return 0; } -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-21 00:49 Subject: [PATCH B v4 2/5] git config: don't allow multiple config file locations Message-ID: <1235177369-2727-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1235177369-2727-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1235177369-2727-2-git-send-email-felipe.contreras@gmail.com> ``` Either --global, --system, or --file can be used, but not any combination. Signed-off-by: Felipe Contreras --- builtin-config.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 302846e..876eb0e 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -315,6 +315,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, PARSE_OPT_STOP_AT_NON_OPTION); + if (use_global_config + use_system_config + !!given_config_file > 1) { + error("only one config file at a time."); + usage_with_options(builtin_config_usage, builtin_config_options); + } + if (use_global_config) { char *home = getenv("HOME"); if (home) { -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-21 00:49 Subject: [PATCH B v4 3/5] git config: don't allow multiple variable types Message-ID: <1235177369-2727-4-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1235177369-2727-4-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1235177369-2727-3-git-send-email-felipe.contreras@gmail.com> ``` Only --bool, --int, or --bool-or-int can be used, but not any combination of them. Signed-off-by: Felipe Contreras --- builtin-config.c | 32 ++++++++++++++++++++------------ 1 files changed, 20 insertions(+), 12 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 876eb0e..c2a30f2 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -19,11 +19,10 @@ static int seen; static char delim = '='; static char key_delim = ' '; static char term = '\n'; -static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW; static int use_global_config, use_system_config; static const char *given_config_file; -static int actions; +static int actions, types; static const char *get_color_slot, *get_colorbool_slot; static int end_null; @@ -43,6 +42,10 @@ static int end_null; #define ACTION_GET_COLOR (1<<13) #define ACTION_GET_COLORBOOL (1<<14) +#define TYPE_BOOL (1<<0) +#define TYPE_INT (1<<1) +#define TYPE_BOOL_OR_INT (1<<2) + static struct option builtin_config_options[] = { OPT_GROUP("Config file location"), OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"), @@ -63,9 +66,9 @@ static struct option builtin_config_options[] = { OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"), OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"), OPT_GROUP("Type"), - OPT_SET_INT(0, "bool", &type, "value is \"true\" or \"false\"", T_BOOL), - OPT_SET_INT(0, "int", &type, "value is decimal number", T_INT), - OPT_SET_INT(0, "bool-or-int", &type, NULL, T_BOOL_OR_INT), + OPT_BIT(0, "bool", &types, "value is \"true\" or \"false\"", TYPE_BOOL), + OPT_BIT(0, "int", &types, "value is decimal number", TYPE_INT), + OPT_BIT(0, "bool-or-int", &types, NULL, TYPE_BOOL_OR_INT), OPT_GROUP("Other"), OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"), OPT_END(), @@ -109,11 +112,11 @@ static int show_config(const char *key_, const char *value_, void *cb) } if (seen && !do_all) dup_error = 1; - if (type == T_INT) + if (types == TYPE_INT) sprintf(value, "%d", git_config_int(key_, value_?value_:"")); - else if (type == T_BOOL) + else if (types == TYPE_BOOL) vptr = git_config_bool(key_, value_) ? "true" : "false"; - else if (type == T_BOOL_OR_INT) { + else if (types == TYPE_BOOL_OR_INT) { int is_bool, v; v = git_config_bool_or_int(key_, value_, &is_bool); if (is_bool) @@ -212,18 +215,18 @@ static char *normalize_value(const char *key, const char *value) if (!value) return NULL; - if (type == T_RAW) + if (types == 0) normalized = xstrdup(value); else { normalized = xmalloc(64); - if (type == T_INT) { + if (types == TYPE_INT) { int v = git_config_int(key, value); sprintf(normalized, "%d", v); } - else if (type == T_BOOL) + else if (types == TYPE_BOOL) sprintf(normalized, "%s", git_config_bool(key, value) ? "true" : "false"); - else if (type == T_BOOL_OR_INT) { + else if (types == TYPE_BOOL_OR_INT) { int is_bool, v; v = git_config_bool_or_int(key, value, &is_bool); if (!is_bool) @@ -346,6 +349,11 @@ int cmd_config(int argc, const char **argv, const char *unused_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 (get_color_slot) actions |= ACTION_GET_COLOR; if (get_colorbool_slot) -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-21 00:49 Subject: [PATCH B v4 4/5] git config: don't allow extra arguments for -e or -l. Message-ID: <1235177369-2727-5-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1235177369-2727-5-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1235177369-2727-4-git-send-email-felipe.contreras@gmail.com> ``` As suggested by Johannes Schindelin. Signed-off-by: Felipe Contreras --- builtin-config.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index c2a30f2..8045926 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -373,6 +373,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) } if (actions == ACTION_LIST) { + check_argc(argc, 0, 0); if (git_config(show_all_config, NULL) < 0) { if (config_exclusive_filename) die("unable to read config file %s: %s", @@ -382,6 +383,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) } } else if (actions == ACTION_EDIT) { + check_argc(argc, 0, 0); git_config(git_default_config, NULL); launch_editor(config_exclusive_filename ? config_exclusive_filename : git_path("config"), -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-21 00:49 Subject: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type Message-ID: <1235177369-2727-6-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1235177369-2727-6-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1235177369-2727-5-git-send-email-felipe.contreras@gmail.com> ``` Doing so would be incoherent since --get-color would pick a color slot and ignore the variable type option (e.g. --bool), and the type would require a variable name. Suggested by Junio C Hamano. Signed-off-by: Felipe Contreras --- builtin-config.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 8045926..9930568 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) if (get_colorbool_slot) actions |= ACTION_GET_COLORBOOL; + if ((get_color_slot || get_colorbool_slot) && types) { + 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); -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-21 00:53 Subject: Re: [PATCH B v4 0/5] git config: reorganization Message-ID: <94a0d4530902201653p694bcad6wd79579536f285571@mail.gmail.com> URL: https://gitlist.dev/e/94a0d4530902201653p694bcad6wd79579536f285571%40mail.gmail.com In-Reply-To: <1235177369-2727-1-git-send-email-felipe.contreras@gmail.com> ``` On Sat, Feb 21, 2009 at 2:49 AM, Felipe Contreras wrote: > This patch series is the first of a two parts reorganization for more > maintainability and user friendliness. Err, the second. -- Felipe Contreras ``` ## Junio C Hamano, 2009-02-22 16:48 Subject: Re: [PATCH B v4 1/5] git config: reorganize to use parseopt Message-ID: <7vskm62z9w.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vskm62z9w.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1235177369-2727-2-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > + else if (actions == ACTION_GET_COLORBOOL) { > + if (argc == 1) > + stdout_is_tty = git_config_bool("command line", argv[0]); > + else if (argc == 0) > + stdout_is_tty = isatty(1); > + return get_colorbool(argc != 0); > + } I see you fixed this from the last series... ``` ## Junio C Hamano, 2009-02-22 16:48 Subject: Re: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type Message-ID: <7vmyce2z9q.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmyce2z9q.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1235177369-2727-6-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > Doing so would be incoherent since --get-color would pick a color slot > and ignore the variable type option (e.g. --bool), and the type would > require a variable name. > > Suggested by Junio C Hamano. > > Signed-off-by: Felipe Contreras > --- > builtin-config.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/builtin-config.c b/builtin-config.c > index 8045926..9930568 100644 > --- a/builtin-config.c > +++ b/builtin-config.c > @@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) > if (get_colorbool_slot) > actions |= ACTION_GET_COLORBOOL; > > + if ((get_color_slot || get_colorbool_slot) && types) { > + error("--get-color and variable type are incoherent"); > + usage_with_options(builtin_config_usage, builtin_config_options); > + } > + I do not think I suggested anything like this, so I'd decline to take credit for this patch. Strictly speaking, "--bool --get-colorbool diff.color 1" shouldn't error out, don't you think? And it certainly shouldn't say "--get-color". ``` ## Felipe Contreras, 2009-02-22 17:12 Subject: Re: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type Message-ID: <94a0d4530902220912r94e00d6ha7592bc397baa57c@mail.gmail.com> URL: https://gitlist.dev/e/94a0d4530902220912r94e00d6ha7592bc397baa57c%40mail.gmail.com In-Reply-To: <7vmyce2z9q.fsf@gitster.siamese.dyndns.org> ``` On Sun, Feb 22, 2009 at 6:48 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> Doing so would be incoherent since --get-color would pick a color slot >> and ignore the variable type option (e.g. --bool), and the type would >> require a variable name. >> >> Suggested by Junio C Hamano. >> >> Signed-off-by: Felipe Contreras >> --- >>  builtin-config.c |    5 +++++ >>  1 files changed, 5 insertions(+), 0 deletions(-) >> >> diff --git a/builtin-config.c b/builtin-config.c >> index 8045926..9930568 100644 >> --- a/builtin-config.c >> +++ b/builtin-config.c >> @@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) >>       if (get_colorbool_slot) >>           actions |= ACTION_GET_COLORBOOL; >> >> +     if ((get_color_slot || get_colorbool_slot) && types) { >> +             error("--get-color and variable type are incoherent"); >> +             usage_with_options(builtin_config_usage, builtin_config_options); >> +     } >> + > > I do not think I suggested anything like this, so I'd decline to take > credit for this patch. > > Strictly speaking, "--bool --get-colorbool diff.color 1" shouldn't error > out, don't you think?  And it certainly shouldn't say "--get-color". Huh? I misinterpreted: I see "git config --bool --get-color diff.color.whitespace" is still allowed, which you might want to tighten further. I don't really understand the --get-color* options, so please drop the patch. -- Felipe Contreras ``` ## Felipe Contreras, 2009-02-22 17:19 Subject: Re: [PATCH B v4 1/5] git config: reorganize to use parseopt Message-ID: <94a0d4530902220919t729915dleed932b010ee76b@mail.gmail.com> URL: https://gitlist.dev/e/94a0d4530902220919t729915dleed932b010ee76b%40mail.gmail.com In-Reply-To: <7vskm62z9w.fsf@gitster.siamese.dyndns.org> ``` On Sun, Feb 22, 2009 at 6:48 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> +     else if (actions == ACTION_GET_COLORBOOL) { >> +             if (argc == 1) >> +                     stdout_is_tty = git_config_bool("command line", argv[0]); >> +             else if (argc == 0) >> +                     stdout_is_tty = isatty(1); >> +             return get_colorbool(argc != 0); >> +     } > > I see you fixed this from the last series... Yes, argc != 1 was wrong. Now I've made sure all the tests pass. -- Felipe Contreras ``` ## Junio C Hamano, 2009-02-22 17:35 Subject: Re: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type Message-ID: <7vmycez86y.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmycez86y.fsf%40gitster.siamese.dyndns.org In-Reply-To: <94a0d4530902220912r94e00d6ha7592bc397baa57c@mail.gmail.com> ``` Felipe Contreras writes: >>> diff --git a/builtin-config.c b/builtin-config.c >>> index 8045926..9930568 100644 >>> --- a/builtin-config.c >>> +++ b/builtin-config.c >>> @@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) >>>       if (get_colorbool_slot) >>>           actions |= ACTION_GET_COLORBOOL; >>> >>> +     if ((get_color_slot || get_colorbool_slot) && types) { >>> +             error("--get-color and variable type are incoherent"); >>> +             usage_with_options(builtin_config_usage, builtin_config_options); >>> +     } >>> + >> >> I do not think I suggested anything like this, so I'd decline to take >> credit for this patch. >> >> Strictly speaking, "--bool --get-colorbool diff.color 1" shouldn't error >> out, don't you think?  And it certainly shouldn't say "--get-color". > > Huh? I misinterpreted: > I see "git config --bool --get-color diff.color.whitespace" is still > allowed, which you might want to tighten further. "--get-color" gets, escape sequence to throw at the terminal to get the color configured. It does not make sense to ask for bool (or int) for that action. "--get-colorbool" asks if it is appropriate to use such escape sequence (e.g. when the output is tty and config says "auto", you would get "please use color"). In other words, its type is always bool, so using it as --int does not make sense but we cannot really say using it with --bool is nonsense. The two use formats with and without is_tty are meant to be used in scripts this way: # without $is_tty if git config --get-colorbool color.diff then use_color=true else use_color=false fi # with $is_tty if test -t 1 then is_tty=1 else is_tty=0 fi use_color=$(git config --get-colorbool color.diff $is_tty) In the latter form, output from the command is captured by $() and does not see if the stdout of the calling script is a tty, hence the script feeds that information to the command as an extra parameter. ``` ## Felipe Contreras, 2009-03-07 09:47 Subject: Re: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type Message-ID: <94a0d4530903070147v1128e482n9522d60292d206a0@mail.gmail.com> URL: https://gitlist.dev/e/94a0d4530903070147v1128e482n9522d60292d206a0%40mail.gmail.com In-Reply-To: <7vmycez86y.fsf@gitster.siamese.dyndns.org> ``` On Sun, Feb 22, 2009 at 7:35 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >>>> diff --git a/builtin-config.c b/builtin-config.c >>>> index 8045926..9930568 100644 >>>> --- a/builtin-config.c >>>> +++ b/builtin-config.c >>>> @@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) >>>>       if (get_colorbool_slot) >>>>           actions |= ACTION_GET_COLORBOOL; >>>> >>>> +     if ((get_color_slot || get_colorbool_slot) && types) { >>>> +             error("--get-color and variable type are incoherent"); >>>> +             usage_with_options(builtin_config_usage, builtin_config_options); >>>> +     } >>>> + >>> >>> I do not think I suggested anything like this, so I'd decline to take >>> credit for this patch. >>> >>> Strictly speaking, "--bool --get-colorbool diff.color 1" shouldn't error >>> out, don't you think?  And it certainly shouldn't say "--get-color". >> >> Huh? I misinterpreted: >> I see "git config --bool --get-color diff.color.whitespace" is still >> allowed, which you might want to tighten further. > > "--get-color" gets, escape sequence to throw at the terminal to get the > color configured.  It does not make sense to ask for bool (or int) for > that action. > > "--get-colorbool" asks if it is appropriate to use such escape sequence > (e.g. when the output is tty and config says "auto", you would get "please > use color").  In other words, its type is always bool, so using it as > --int does not make sense but we cannot really say using it with --bool is > nonsense. Now I get it, but why would somebody want to do '--bool --get-colorbool'? That is redundant. I think --bool should be used only for boolean variables, not color ones. Please let me know if the original patch is ok and I'll resend it with '--get-color*'. -- Felipe Contreras ```