# [PATCH 3/8] config: Use parseopt. 19 messages from 2009-02-15 to 2009-02-16. Participants: Felipe Contreras, Johannes Schindelin, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/17798 ## Felipe Contreras, 2009-02-15 09:00 Subject: [PATCH 1/8] config: Trivial rename in preparation for parseopt. Message-ID: <1234688460-9248-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-1-git-send-email-felipe.contreras%40gmail.com ``` As suggested by Johannes Schindelin. Signed-off-by: Felipe Contreras --- builtin-config.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 6937eaf..bf8df58 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -178,6 +178,7 @@ static char *normalize_value(const char *key, const char *value) static int get_color_found; static const char *get_color_slot; +static const char *get_colorbool_slot; static char parsed_color[COLOR_MAXLEN]; static int git_get_color_config(const char *var, const char *value, void *cb) @@ -231,7 +232,7 @@ static int get_diff_color_found; static int git_get_colorbool_config(const char *var, const char *value, void *cb) { - if (!strcmp(var, get_color_slot)) { + if (!strcmp(var, get_colorbool_slot)) { get_colorbool_found = git_config_colorbool(var, value, stdout_is_tty); } @@ -263,11 +264,11 @@ static int get_colorbool(int argc, const char **argv) usage(git_config_set_usage); get_colorbool_found = -1; get_diff_color_found = -1; - get_color_slot = argv[0]; + get_colorbool_slot = argv[0]; git_config(git_get_colorbool_config, NULL); if (get_colorbool_found < 0) { - if (!strcmp(get_color_slot, "color.diff")) + if (!strcmp(get_colorbool_slot, "color.diff")) get_colorbool_found = get_diff_color_found; if (get_colorbool_found < 0) get_colorbool_found = git_use_color_default; -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-15 09:00 Subject: [PATCH 2/8] config: Cleanup config file handling. Message-ID: <1234688460-9248-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1234688460-9248-1-git-send-email-felipe.contreras@gmail.com> ``` As suggested by Johannes Schindelin. Signed-off-by: Felipe Contreras --- builtin-config.c | 20 +++++++++----------- 1 files changed, 9 insertions(+), 11 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index bf8df58..da754e0 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -282,11 +282,11 @@ static int get_colorbool(int argc, const char **argv) } } -int cmd_config(int argc, const char **argv, const char *prefix) +int cmd_config(int argc, const char **argv, const char *unused_prefix) { int nongit; char* value; - const char *file = setup_git_directory_gently(&nongit); + const char *prefix = setup_git_directory_gently(&nongit); config_exclusive_filename = getenv(CONFIG_ENVIRONMENT); @@ -300,10 +300,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) 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 && - file && errno) - die("unable to read config file %s: %s", file, - strerror(errno)); + if (git_config(show_all_config, NULL) < 0) + die("error processing config file(s)"); return 0; } else if (!strcmp(argv[1], "--global")) { @@ -320,12 +318,12 @@ int cmd_config(int argc, const char **argv, const char *prefix) else if (!strcmp(argv[1], "--file") || !strcmp(argv[1], "-f")) { if (argc < 3) usage(git_config_set_usage); - if (!is_absolute_path(argv[2]) && file) - file = prefix_filename(file, strlen(file), - argv[2]); + if (!is_absolute_path(argv[2]) && prefix) + config_exclusive_filename = prefix_filename(prefix, + strlen(prefix), + argv[2]); else - file = argv[2]; - config_exclusive_filename = file; + config_exclusive_filename = argv[2]; argc--; argv++; } -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-15 09:00 Subject: [PATCH 3/8] config: Use parseopt. Message-ID: <1234688460-9248-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1234688460-9248-2-git-send-email-felipe.contreras@gmail.com> ``` Reorganizing the code to use parseopt as suggested by Johannes Schindelin. This patch has benefited from comments by Johannes Schindelin and Junio C Hamano. Signed-off-by: Felipe Contreras --- builtin-config.c | 362 ++++++++++++++++++++++++++--------------------------- 1 files changed, 178 insertions(+), 184 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index da754e0..084222a 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,59 @@ 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) + +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), + 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_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_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("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_) @@ -180,7 +236,6 @@ static int get_color_found; static const char *get_color_slot; static const char *get_colorbool_slot; static char parsed_color[COLOR_MAXLEN]; - static int git_get_color_config(const char *var, const char *value, void *cb) { if (!strcmp(var, get_color_slot)) { @@ -192,29 +247,8 @@ static int git_get_color_config(const char *var, const char *value, void *cb) return 0; } -static int get_color(int argc, const char **argv) +static void get_color(const char *def_color) { - /* - * grab the color setting for the given slot from the configuration, - * or parse the default value if missing, and return ANSI color - * escape sequence. - * - * e.g. - * git config --get-color color.diff.whitespace "blue reverse" - */ - const char *def_color = NULL; - - switch (argc) { - default: - usage(git_config_set_usage); - case 2: - def_color = argv[1]; - /* fallthru */ - case 1: - get_color_slot = argv[0]; - break; - } - get_color_found = 0; parsed_color[0] = '\0'; git_config(git_get_color_config, NULL); @@ -223,7 +257,6 @@ static int get_color(int argc, const char **argv) color_parse(def_color, "command line", parsed_color); fputs(parsed_color, stdout); - return 0; } static int stdout_is_tty; @@ -247,24 +280,10 @@ static int git_get_colorbool_config(const char *var, const char *value, return 0; } -static int get_colorbool(int argc, const char **argv) +static int get_colorbool(int print) { - /* - * git config --get-colorbool [] - * - * returns "true" or "false" depending on how - * is configured. - */ - - if (argc == 2) - stdout_is_tty = git_config_bool("command line", argv[1]); - else if (argc == 1) - stdout_is_tty = isatty(1); - else - usage(git_config_set_usage); get_colorbool_found = -1; get_diff_color_found = -1; - get_colorbool_slot = argv[0]; git_config(git_get_colorbool_config, NULL); if (get_colorbool_found < 0) { @@ -274,12 +293,11 @@ static int get_colorbool(int argc, const char **argv) get_colorbool_found = git_use_color_default; } - if (argc == 1) { - return get_colorbool_found ? 0 : 1; - } else { + if (print) { printf("%s\n", get_colorbool_found ? "true" : "false"); return 0; - } + } else + return get_colorbool_found ? 0 : 1; } int cmd_config(int argc, const char **argv, const char *unused_prefix) @@ -290,149 +308,125 @@ 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) - 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")) { - return get_color(argc-2, argv+2); - } else if (!strcmp(argv[1], "--get-colorbool")) { - return get_colorbool(argc-2, argv+2); - } else if (!strcmp(argv[1], "--edit") || !strcmp(argv[1], "-e")) { - const char *config_filename; - if (argc != 2) - usage(git_config_set_usage); - if (config_exclusive_filename) - config_filename = config_exclusive_filename; - else - config_filename = git_path("config"); - git_config(git_default_config, NULL); - launch_editor(config_filename, NULL, NULL); - return 0; - } else - break; - argc--; - argv++; - } + argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0); - 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); - } else { - value = normalize_value(argv[1], argv[2]); - return git_config_set(argv[1], value); - } - 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); + 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_multivar(argv[1], value, argv[3], 0); + die("$HOME not set"); } - 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); + } + 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 (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_ADD; break; + case 3: actions |= ACTION_REPLACE_ALL; break; + default: + usage_with_options(builtin_config_usage, builtin_config_options); } - case 1: - default: - usage(git_config_set_usage); + + if (actions & ACTION_LIST) { + if (git_config(show_all_config, NULL) < 0) + die("error processing config file(s)"); + } + else if (actions & ACTION_EDIT) { + const char *config_filename; + if (config_exclusive_filename) + config_filename = config_exclusive_filename; + else + config_filename = git_path("config"); + git_config(git_default_config, NULL); + launch_editor(config_filename, NULL, NULL); + } + 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 (get_color_slot) { + get_color(argv[0]); + } + else if (get_colorbool_slot) { + 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 != 1); + } + return 0; } -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-15 09:00 Subject: [PATCH 4/8] config: Improve variable 'type' handling. Message-ID: <1234688460-9248-4-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-4-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1234688460-9248-3-git-send-email-felipe.contreras@gmail.com> ``` So now only either --bool, --int, or --bool-or-int can be used, but not any combination of them. Signed-off-by: Felipe Contreras --- builtin-config.c | 23 +++++++++++++++++++---- 1 files changed, 19 insertions(+), 4 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 084222a..83f8b74 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -23,7 +23,7 @@ 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; @@ -39,6 +39,10 @@ static int end_null; #define ACTION_LIST (1<<9) #define ACTION_EDIT (1<<10) +#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"), @@ -57,9 +61,9 @@ static struct option builtin_config_options[] = { OPT_BIT('l', "list", &actions, "list all", ACTION_LIST), OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT), 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_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("Other"), @@ -336,6 +340,17 @@ 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); + } + switch (types) { + case TYPE_BOOL: type = T_BOOL; break; + case TYPE_INT: type = T_INT; break; + case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break; + default: break; + } + 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-15 09:00 Subject: [PATCH 5/8] config: Disallow multiple config file locations. Message-ID: <1234688460-9248-5-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-5-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1234688460-9248-4-git-send-email-felipe.contreras@gmail.com> ``` Either --global, --system, or --file should be used, but not any combination. Signed-off-by: Felipe Contreras --- builtin-config.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 83f8b74..e744ad8 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0); + { + int config_file_count = use_global_config + use_system_config; + if (given_config_file) + config_file_count++; + if (config_file_count > 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-15 09:00 Subject: [PATCH 6/8] config: Don't allow extra arguments for -e or -l. Message-ID: <1234688460-9248-6-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-6-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1234688460-9248-5-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 e744ad8..3463b1c 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -375,10 +375,12 @@ 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) die("error processing config file(s)"); } else if (actions & ACTION_EDIT) { + check_argc(argc, 0, 0); const char *config_filename; if (config_exclusive_filename) config_filename = config_exclusive_filename; -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-15 09:00 Subject: [PATCH 7/8] config: Don't return negative exit codes. Message-ID: <1234688460-9248-7-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-7-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1234688460-9248-6-git-send-email-felipe.contreras@gmail.com> ``` As suggested by Johannes Schindelin. Signed-off-by: Felipe Contreras --- builtin-config.c | 27 +++++++++++---------------- 1 files changed, 11 insertions(+), 16 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 3463b1c..677ae3f 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -308,6 +308,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) { int nongit; char* value; + int ret = 0; const char *prefix = setup_git_directory_gently(&nongit); config_exclusive_filename = getenv(CONFIG_ENVIRONMENT); @@ -392,55 +393,49 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) 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); + ret = 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); + ret = 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]); + ret = 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]); + ret = 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]); + ret = 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); + ret = git_config_set_multivar(argv[0], NULL, argv[1], 0); else - return git_config_set(argv[0], NULL); + ret = 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); + ret = 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!"); } @@ -452,8 +447,8 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) stdout_is_tty = git_config_bool("command line", argv[0]); else if (argc == 0) stdout_is_tty = isatty(1); - return get_colorbool(argc != 1); + ret = get_colorbool(argc != 1); } - return 0; + return !!ret; } -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-15 09:01 Subject: [PATCH 8/8] config: Codestyle cleanups. Message-ID: <1234688460-9248-8-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1234688460-9248-8-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1234688460-9248-7-git-send-email-felipe.contreras@gmail.com> ``` Signed-off-by: Felipe Contreras --- builtin-config.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 677ae3f..4d1805b 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -87,7 +87,7 @@ static int show_all_config(const char *key_, const char *value_, void *cb) return 0; } -static int show_config(const char* key_, const char* value_, void *cb) +static int show_config(const char *key_, const char *value_, void *cb) { char value[256]; const char *vptr = value; @@ -134,7 +134,7 @@ static int show_config(const char* key_, const char* value_, void *cb) return 0; } -static int get_value(const char* key_, const char* regex_) +static int get_value(const char *key_, const char *regex_) { int ret = -1; char *tl; @@ -307,7 +307,7 @@ static int get_colorbool(int print) int cmd_config(int argc, const char **argv, const char *unused_prefix) { int nongit; - char* value; + char *value; int ret = 0; const char *prefix = setup_git_directory_gently(&nongit); -- 1.6.1.3 ``` ## Johannes Schindelin, 2009-02-15 12:22 Subject: Re: [PATCH 7/8] config: Don't return negative exit codes. Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902151321310.10279%40pacific.mpi-cbg.de In-Reply-To: <1234688460-9248-7-git-send-email-felipe.contreras@gmail.com> ``` Hi, On Sun, 15 Feb 2009, Felipe Contreras wrote: > 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!"); > } You need an "if (ret > 0) ret = 0;" to avoid regressions. > 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!"); > } Likewise. Thanks, Dscho ``` ## Johannes Schindelin, 2009-02-15 12:24 Subject: Re: [PATCH 4/8] config: Improve variable 'type' handling. Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902151323100.10279%40pacific.mpi-cbg.de In-Reply-To: <1234688460-9248-4-git-send-email-felipe.contreras@gmail.com> ``` Hi, On Sun, 15 Feb 2009, Felipe Contreras wrote: > + switch (types) { > + case TYPE_BOOL: type = T_BOOL; break; > + case TYPE_INT: type = T_INT; break; > + case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break; > + default: break; > + } Would it not be nicer to get rid of the variable named "type" and use "types == TYPE_BOOL" and similar statements instead? Ciao, Dscho ``` ## Johannes Schindelin, 2009-02-15 12:26 Subject: Re: [PATCH 5/8] config: Disallow multiple config file locations. Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902151324530.10279%40pacific.mpi-cbg.de In-Reply-To: <1234688460-9248-5-git-send-email-felipe.contreras@gmail.com> ``` Hi, On Sun, 15 Feb 2009, Felipe Contreras wrote: > Either --global, --system, or --file should be used, but not any > combination. > > Signed-off-by: Felipe Contreras > --- > builtin-config.c | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/builtin-config.c b/builtin-config.c > index 83f8b74..e744ad8 100644 > --- a/builtin-config.c > +++ b/builtin-config.c > @@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) > > argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0); > > + { > + int config_file_count = use_global_config + use_system_config; > + if (given_config_file) > + config_file_count++; > + if (config_file_count > 1) { > + error("only one config file at a time."); > + usage_with_options(builtin_config_usage, builtin_config_options); > + } > + } Hmm. Is this a convoluted way to write if (use_global_config + use_system_config + !!given_config_file > 1) or am I misunderstanding anything? Ciao, Dscho ``` ## Felipe Contreras, 2009-02-15 12:39 Subject: Re: [PATCH 7/8] config: Don't return negative exit codes. Message-ID: <94a0d4530902150439peac228ckec79f8c692b84e4e@mail.gmail.com> URL: https://gitlist.dev/e/94a0d4530902150439peac228ckec79f8c692b84e4e%40mail.gmail.com In-Reply-To: ``` On Sun, Feb 15, 2009 at 2:22 PM, Johannes Schindelin wrote: > Hi, > > On Sun, 15 Feb 2009, Felipe Contreras wrote: > > >> 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!"); >> } > > You need an "if (ret > 0) ret = 0;" to avoid regressions. > >> 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!"); >> } > > Likewise. True. Fixed in the attached patch. -- Felipe Contreras From b682131b61d01cac2946e7c7ab6666fef86441a4 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Sun, 15 Feb 2009 10:54:53 +0200 Subject: [PATCH 7/8] config: Don't return negative exit codes. As suggested by Johannes Schindelin. Signed-off-by: Felipe Contreras --- builtin-config.c | 31 +++++++++++++++---------------- 1 files changed, 15 insertions(+), 16 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 5891a4e..0606ada 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -307,6 +307,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) { int nongit; char* value; + int ret = 0; const char *prefix = setup_git_directory_gently(&nongit); config_exclusive_filename = getenv(CONFIG_ENVIRONMENT); @@ -380,57 +381,55 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) 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); + ret = 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); + ret = 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]); + ret = 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]); + ret = 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]); + ret = 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); + ret = git_config_set_multivar(argv[0], NULL, argv[1], 0); else - return git_config_set(argv[0], NULL); + ret = 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); + ret = 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!"); + if (ret > 0) + ret = 0; } 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!"); + if (ret > 0) + ret = 0; } else if (get_color_slot) { get_color(argv[0]); @@ -440,8 +439,8 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) stdout_is_tty = git_config_bool("command line", argv[0]); else if (argc == 0) stdout_is_tty = isatty(1); - return get_colorbool(argc != 1); + ret = get_colorbool(argc != 1); } - return 0; + return !!ret; } -- 1.6.1.3 ``` ## Felipe Contreras, 2009-02-15 12:43 Subject: Re: [PATCH 4/8] config: Improve variable 'type' handling. Message-ID: <94a0d4530902150443t38d9253bi85ec42f3afbf0a1d@mail.gmail.com> URL: https://gitlist.dev/e/94a0d4530902150443t38d9253bi85ec42f3afbf0a1d%40mail.gmail.com In-Reply-To: ``` On Sun, Feb 15, 2009 at 2:24 PM, Johannes Schindelin wrote: > Hi, > > On Sun, 15 Feb 2009, Felipe Contreras wrote: > >> + switch (types) { >> + case TYPE_BOOL: type = T_BOOL; break; >> + case TYPE_INT: type = T_INT; break; >> + case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break; >> + default: break; >> + } > > Would it not be nicer to get rid of the variable named "type" and use > "types == TYPE_BOOL" and similar statements instead? I'm not too sure about that. If you read the code you might think you could actually have multiple types, but the same can be said about actions. Anyway, attached is a patch with that change. -- Felipe Contreras From a2ca08ace97e070d10496eee3e84c8ba2f052b50 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Sun, 15 Feb 2009 10:38:01 +0200 Subject: [PATCH 4/8] config: Improve variable 'type' handling. So now only either --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 084222a..30de93e 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; @@ -39,6 +38,10 @@ static int end_null; #define ACTION_LIST (1<<9) #define ACTION_EDIT (1<<10) +#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"), @@ -57,9 +60,9 @@ static struct option builtin_config_options[] = { OPT_BIT('l', "list", &actions, "list all", ACTION_LIST), OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT), 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_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("Other"), @@ -105,11 +108,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) @@ -208,18 +211,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) @@ -336,6 +339,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 (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-15 12:44 Subject: Re: [PATCH 5/8] config: Disallow multiple config file locations. Message-ID: <94a0d4530902150444t6e4dcd3btda79aaa0efebfe62@mail.gmail.com> URL: https://gitlist.dev/e/94a0d4530902150444t6e4dcd3btda79aaa0efebfe62%40mail.gmail.com In-Reply-To: ``` On Sun, Feb 15, 2009 at 2:26 PM, Johannes Schindelin wrote: > Hi, > > On Sun, 15 Feb 2009, Felipe Contreras wrote: > >> Either --global, --system, or --file should be used, but not any >> combination. >> >> Signed-off-by: Felipe Contreras >> --- >> builtin-config.c | 10 ++++++++++ >> 1 files changed, 10 insertions(+), 0 deletions(-) >> >> diff --git a/builtin-config.c b/builtin-config.c >> index 83f8b74..e744ad8 100644 >> --- a/builtin-config.c >> +++ b/builtin-config.c >> @@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) >> >> argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0); >> >> + { >> + int config_file_count = use_global_config + use_system_config; >> + if (given_config_file) >> + config_file_count++; >> + if (config_file_count > 1) { >> + error("only one config file at a time."); >> + usage_with_options(builtin_config_usage, builtin_config_options); >> + } >> + } > > Hmm. Is this a convoluted way to write > > if (use_global_config + use_system_config + !!given_config_file > 1) > > or am I misunderstanding anything? Ah, much better. (the !!foo trick is new to me) -- Felipe Contreras From 1ef7e251a2ab24f213af3022dd0a98785c8127b7 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Sun, 15 Feb 2009 10:46:03 +0200 Subject: [PATCH 5/8] config: Disallow multiple config file locations. Either --global, --system, or --file should 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 30de93e..cdd8a12 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -313,6 +313,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0); + 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-15 13:30 Subject: Re: [PATCH 7/8] config: Don't return negative exit codes. Message-ID: <20090215133012.GA13801@annwn> URL: https://gitlist.dev/e/20090215133012.GA13801%40annwn In-Reply-To: <94a0d4530902150439peac228ckec79f8c692b84e4e@mail.gmail.com> ``` On Sun, Feb 15, 2009 at 02:39:47PM +0200, Felipe Contreras wrote: > On Sun, Feb 15, 2009 at 2:22 PM, Johannes Schindelin > wrote: > > Hi, > > > > On Sun, 15 Feb 2009, Felipe Contreras wrote: > > > > > >> 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!"); > >> } > > > > You need an "if (ret > 0) ret = 0;" to avoid regressions. > > > >> 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!"); > >> } > > > > Likewise. > > True. Fixed in the attached patch. I'm inlining the patch as Johannes requested: --- builtin-config.c | 31 +++++++++++++++---------------- 1 files changed, 15 insertions(+), 16 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 5891a4e..0606ada 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -307,6 +307,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) { int nongit; char* value; + int ret = 0; const char *prefix = setup_git_directory_gently(&nongit); config_exclusive_filename = getenv(CONFIG_ENVIRONMENT); @@ -380,57 +381,55 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) 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); + ret = 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); + ret = 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]); + ret = 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]); + ret = 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]); + ret = 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); + ret = git_config_set_multivar(argv[0], NULL, argv[1], 0); else - return git_config_set(argv[0], NULL); + ret = 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); + ret = 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!"); + if (ret > 0) + ret = 0; } 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!"); + if (ret > 0) + ret = 0; } else if (get_color_slot) { get_color(argv[0]); @@ -440,8 +439,8 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) stdout_is_tty = git_config_bool("command line", argv[0]); else if (argc == 0) stdout_is_tty = isatty(1); - return get_colorbool(argc != 1); + ret = get_colorbool(argc != 1); } - return 0; + return !!ret; } -- 1.6.1.3 -- Felipe Contreras ``` ## Felipe Contreras, 2009-02-15 13:34 Subject: Re: [PATCH 4/8] config: Improve variable 'type' handling. Message-ID: <20090215133449.GA13810@annwn> URL: https://gitlist.dev/e/20090215133449.GA13810%40annwn In-Reply-To: <94a0d4530902150443t38d9253bi85ec42f3afbf0a1d@mail.gmail.com> ``` On Sun, Feb 15, 2009 at 02:43:15PM +0200, Felipe Contreras wrote: > On Sun, Feb 15, 2009 at 2:24 PM, Johannes Schindelin > wrote: > > Hi, > > > > On Sun, 15 Feb 2009, Felipe Contreras wrote: > > > >> + switch (types) { > >> + case TYPE_BOOL: type = T_BOOL; break; > >> + case TYPE_INT: type = T_INT; break; > >> + case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break; > >> + default: break; > >> + } > > > > Would it not be nicer to get rid of the variable named "type" and use > > "types == TYPE_BOOL" and similar statements instead? > > I'm not too sure about that. If you read the code you might think you > could actually have multiple types, but the same can be said about > actions. > > Anyway, attached is a patch with that change. Sorry, again, but now inlined: --- builtin-config.c | 32 ++++++++++++++++++++------------ 1 files changed, 20 insertions(+), 12 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 084222a..30de93e 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; @@ -39,6 +38,10 @@ static int end_null; #define ACTION_LIST (1<<9) #define ACTION_EDIT (1<<10) +#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"), @@ -57,9 +60,9 @@ static struct option builtin_config_options[] = { OPT_BIT('l', "list", &actions, "list all", ACTION_LIST), OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT), 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_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("Other"), @@ -105,11 +108,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) @@ -208,18 +211,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) @@ -336,6 +339,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 (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 ``` ## Felipe Contreras, 2009-02-15 13:35 Subject: Re: [PATCH 5/8] config: Disallow multiple config file locations. Message-ID: <20090215133557.GB13810@annwn> URL: https://gitlist.dev/e/20090215133557.GB13810%40annwn In-Reply-To: <94a0d4530902150444t6e4dcd3btda79aaa0efebfe62@mail.gmail.com> ``` On Sun, Feb 15, 2009 at 02:44:29PM +0200, Felipe Contreras wrote: > On Sun, Feb 15, 2009 at 2:26 PM, Johannes Schindelin > wrote: > > Hi, > > > > On Sun, 15 Feb 2009, Felipe Contreras wrote: > > > >> Either --global, --system, or --file should be used, but not any > >> combination. > >> > >> Signed-off-by: Felipe Contreras > >> --- > >> builtin-config.c | 10 ++++++++++ > >> 1 files changed, 10 insertions(+), 0 deletions(-) > >> > >> diff --git a/builtin-config.c b/builtin-config.c > >> index 83f8b74..e744ad8 100644 > >> --- a/builtin-config.c > >> +++ b/builtin-config.c > >> @@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) > >> > >> argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0); > >> > >> + { > >> + int config_file_count = use_global_config + use_system_config; > >> + if (given_config_file) > >> + config_file_count++; > >> + if (config_file_count > 1) { > >> + error("only one config file at a time."); > >> + usage_with_options(builtin_config_usage, builtin_config_options); > >> + } > >> + } > > > > Hmm. Is this a convoluted way to write > > > > if (use_global_config + use_system_config + !!given_config_file > 1) > > > > or am I misunderstanding anything? > > Ah, much better. (the !!foo trick is new to me) Now inlined: --- builtin-config.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/builtin-config.c b/builtin-config.c index 30de93e..cdd8a12 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -313,6 +313,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix) argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0); + 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 ``` ## Jeff King, 2009-02-15 20:15 Subject: Re: [PATCH 2/8] config: Cleanup config file handling. Message-ID: <20090215201505.GC26779@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090215201505.GC26779%40coredump.intra.peff.net In-Reply-To: <1234688460-9248-2-git-send-email-felipe.contreras@gmail.com> ``` On Sun, Feb 15, 2009 at 11:00:54AM +0200, Felipe Contreras wrote: > As suggested by Johannes Schindelin. Thank you for splitting this patch up. I have to admit I was too scared to read the original after seeing the diffstat. But part of the nice thing about splitting up is that you can write meaningful commit messages for each individual change. Reading this commit message, I don't know what to expect. Your 1/8, though short, tells me what to expect: a trivial rename. But "clean up config file handling" is quite ambiguous. And after reading the patch, I am left wondering why the filename and errno are not useful as part of the die() message. I think even a single sentence would probably make it clear. And yes, I know I can look elsewhere in the thread to find the discussion between you and Dscho. But think of the poor "git log" user six months from now. -Peff ``` ## Junio C Hamano, 2009-02-16 01:15 Subject: Re: [PATCH 2/8] config: Cleanup config file handling. Message-ID: <7vljs7gp15.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljs7gp15.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1234688460-9248-2-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > As suggested by Johannes Schindelin. > > Signed-off-by: Felipe Contreras Dscho won't be the only person who will read "git log" output. What was suggested, what convinced you it is a good idea, what goodness would this patch add if I take it? > - if (git_config(show_all_config, NULL) < 0 && > - file && errno) > - die("unable to read config file %s: %s", file, > - strerror(errno)); > + if (git_config(show_all_config, NULL) < 0) > + die("error processing config file(s)"); > return 0; This is the only change with substance in this patch. What difference does it make? Earlier we failed to say "die" with message in some situations, and now we die when git_config() signals any error. What are the situations in whoich the command misbehaved? The commit log message is where you are expected to explain that. ```