From a577d2f1a93ecfafbedd5a30cc15e4180253f04d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:41:38 +0200 Subject: [PATCH 01/21] builtin/config: stop printing full usage on misuse When invoking git-config(1) with a wrong set of arguments we end up calling `usage_builtin_config()` after printing an error message that says what was wrong. As that function ends up printing the full list of options, which is quite long, the actual error message will be buried by a wall of text. This makes it really hard to figure out what exactly caused the error. Furthermore, now that we have recently introduced subcommands, the usage information may actually be misleading as we unconditionally print options of the subcommand-less mode. Fix both of these issues by just not printing the options at all anymore. Instead, we call `usage()` that makes us report in a single line what has gone wrong. This should be way more discoverable for our users and addresses the inconsistency. Furthermore, this change allow us to inline the options into the respective functions that use them to parse the command line. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 28 +++++++++++----------------- t/t1300-config.sh | 3 ++- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 80aa9d8a66..3a71d3253f 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -125,8 +125,6 @@ static const char *comment_arg; { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \ PARSE_OPT_NONEG, option_parse_type, (i) } -static NORETURN void usage_builtin_config(void); - static int option_parse_type(const struct option *opt, const char *arg, int unset) { @@ -171,7 +169,7 @@ static int option_parse_type(const struct option *opt, const char *arg, * --type=int'. */ error(_("only one type at a time")); - usage_builtin_config(); + exit(129); } *to_type = new_type; @@ -187,7 +185,7 @@ static void check_argc(int argc, int min, int max) else error(_("wrong number of arguments, should be from %d to %d"), min, max); - usage_builtin_config(); + exit(129); } static void show_config_origin(const struct key_value_info *kvi, @@ -672,7 +670,7 @@ static void handle_config_location(const char *prefix) use_worktree_config + !!given_config_source.file + !!given_config_source.blob > 1) { error(_("only one config file at a time")); - usage_builtin_config(); + exit(129); } if (!startup_info->have_repository) { @@ -802,11 +800,6 @@ static struct option builtin_config_options[] = { OPT_END(), }; -static NORETURN void usage_builtin_config(void) -{ - usage_with_options(builtin_config_usage, builtin_config_options); -} - static int cmd_config_list(int argc, const char **argv, const char *prefix) { struct option opts[] = { @@ -1110,7 +1103,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) { error(_("--get-color and variable type are incoherent")); - usage_builtin_config(); + exit(129); } if (actions == 0) @@ -1119,30 +1112,31 @@ int cmd_config(int argc, const char **argv, const char *prefix) case 2: actions = ACTION_SET; break; case 3: actions = ACTION_SET_ALL; break; default: - usage_builtin_config(); + error(_("no action specified")); + exit(129); } if (omit_values && !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) { error(_("--name-only is only applicable to --list or --get-regexp")); - usage_builtin_config(); + exit(129); } if (show_origin && !(actions & (ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) { error(_("--show-origin is only applicable to --get, --get-all, " "--get-regexp, and --list")); - usage_builtin_config(); + exit(129); } if (default_value && !(actions & ACTION_GET)) { error(_("--default is only applicable to --get")); - usage_builtin_config(); + exit(129); } if (comment_arg && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) { error(_("--comment is only applicable to add/set/replace operations")); - usage_builtin_config(); + exit(129); } /* check usage of --fixed-value */ @@ -1175,7 +1169,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (!allowed_usage) { error(_("--fixed-value only applies with 'value-pattern'")); - usage_builtin_config(); + exit(129); } flags |= CONFIG_FLAGS_FIXED_VALUE; diff --git a/t/t1300-config.sh b/t/t1300-config.sh index f3c4d28e06..d90a69b29f 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -596,7 +596,8 @@ test_expect_success 'get bool variable with empty value' ' test_expect_success 'no arguments, but no crash' ' test_must_fail git config >output 2>&1 && - test_grep usage output + echo "error: no action specified" >expect && + test_cmp expect output ' cat > .git/config << EOF From 0336d0055c4a15916fe6fabbe8031efa042412c0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:41:43 +0200 Subject: [PATCH 02/21] builtin/config: move legacy mode into its own function In `cmd_config()` we first try to parse the provided arguments as subcommands and, if this is successful, call the respective functions of that subcommand. Otherwise we continue with the "legacy" mode that uses implicit actions and/or flags. Disentangle this by moving the legacy mode into its own function. This allows us to move the options into the respective functions and clearly separates concerns. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 3a71d3253f..f6c7e7a082 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -1069,31 +1069,13 @@ static struct option builtin_subcommand_options[] = { OPT_END(), }; -int cmd_config(int argc, const char **argv, const char *prefix) +static int cmd_config_actions(int argc, const char **argv, const char *prefix) { char *value = NULL, *comment = NULL; int flags = 0; int ret = 0; struct key_value_info default_kvi = KVI_INIT; - given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); - - /* - * This is somewhat hacky: we first parse the command line while - * keeping all args intact in order to determine whether a subcommand - * has been specified. If so, we re-parse it a second time, but this - * time we drop KEEP_ARGV0. This is so that we don't munge the command - * line in case no subcommand was given, which would otherwise confuse - * us when parsing the legacy-style modes that don't use subcommands. - */ - argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage, - PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN_OPT); - if (subcommand) { - argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage, - PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_UNKNOWN_OPT); - return subcommand(argc, argv, prefix); - } - argc = parse_options(argc, argv, prefix, builtin_config_options, builtin_config_usage, PARSE_OPT_STOP_AT_NON_OPTION); @@ -1306,3 +1288,26 @@ int cmd_config(int argc, const char **argv, const char *prefix) free(value); return ret; } + +int cmd_config(int argc, const char **argv, const char *prefix) +{ + given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); + + /* + * This is somewhat hacky: we first parse the command line while + * keeping all args intact in order to determine whether a subcommand + * has been specified. If so, we re-parse it a second time, but this + * time we drop KEEP_ARGV0. This is so that we don't munge the command + * line in case no subcommand was given, which would otherwise confuse + * us when parsing the legacy-style modes that don't use subcommands. + */ + argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage, + PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN_OPT); + if (subcommand) { + argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage, + PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_UNKNOWN_OPT); + return subcommand(argc, argv, prefix); + } + + return cmd_config_actions(argc, argv, prefix); +} From 8b908f9dcf0b76345f1c17303ab55aa55e924d92 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:41:47 +0200 Subject: [PATCH 03/21] builtin/config: move subcommand options into `cmd_config()` Move the subcommand options as well as the `subcommand` variable into `cmd_config()`. This reduces our reliance on global state. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index f6c7e7a082..58c8b0abda 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -75,7 +75,6 @@ static char delim = '='; static char key_delim = ' '; static char term = '\n'; -static parse_opt_subcommand_fn *subcommand; static int use_global_config, use_system_config, use_local_config; static int use_worktree_config; static struct git_config_source given_config_source; @@ -1058,17 +1057,6 @@ static int cmd_config_edit(int argc, const char **argv, const char *prefix) return show_editor(); } -static struct option builtin_subcommand_options[] = { - OPT_SUBCOMMAND("list", &subcommand, cmd_config_list), - OPT_SUBCOMMAND("get", &subcommand, cmd_config_get), - OPT_SUBCOMMAND("set", &subcommand, cmd_config_set), - OPT_SUBCOMMAND("unset", &subcommand, cmd_config_unset), - OPT_SUBCOMMAND("rename-section", &subcommand, cmd_config_rename_section), - OPT_SUBCOMMAND("remove-section", &subcommand, cmd_config_remove_section), - OPT_SUBCOMMAND("edit", &subcommand, cmd_config_edit), - OPT_END(), -}; - static int cmd_config_actions(int argc, const char **argv, const char *prefix) { char *value = NULL, *comment = NULL; @@ -1291,6 +1279,18 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) int cmd_config(int argc, const char **argv, const char *prefix) { + parse_opt_subcommand_fn *subcommand = NULL; + struct option subcommand_opts[] = { + OPT_SUBCOMMAND("list", &subcommand, cmd_config_list), + OPT_SUBCOMMAND("get", &subcommand, cmd_config_get), + OPT_SUBCOMMAND("set", &subcommand, cmd_config_set), + OPT_SUBCOMMAND("unset", &subcommand, cmd_config_unset), + OPT_SUBCOMMAND("rename-section", &subcommand, cmd_config_rename_section), + OPT_SUBCOMMAND("remove-section", &subcommand, cmd_config_remove_section), + OPT_SUBCOMMAND("edit", &subcommand, cmd_config_edit), + OPT_END(), + }; + given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); /* @@ -1301,10 +1301,10 @@ int cmd_config(int argc, const char **argv, const char *prefix) * line in case no subcommand was given, which would otherwise confuse * us when parsing the legacy-style modes that don't use subcommands. */ - argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage, + argc = parse_options(argc, argv, prefix, subcommand_opts, builtin_config_usage, PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN_OPT); if (subcommand) { - argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage, + argc = parse_options(argc, argv, prefix, subcommand_opts, builtin_config_usage, PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_UNKNOWN_OPT); return subcommand(argc, argv, prefix); } From 7d5387e26348ebb517fbe3880a4299e2c23d4bff Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:41:52 +0200 Subject: [PATCH 04/21] builtin/config: move legacy options into `cmd_config()` Move the legacy options as well some of the variables it references into `cmd_config_action()`. This reduces our reliance on global state. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 60 ++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 58c8b0abda..e9956574fe 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -78,7 +78,7 @@ static char term = '\n'; static int use_global_config, use_system_config, use_local_config; static int use_worktree_config; static struct git_config_source given_config_source; -static int actions, type; +static int type; static char *default_value; static int end_nul; static int respect_includes_opt = -1; @@ -86,7 +86,6 @@ static struct config_options config_options; static int show_origin; static int show_scope; static int fixed_value; -static const char *comment_arg; #define ACTION_GET (1<<0) #define ACTION_GET_ALL (1<<1) @@ -772,33 +771,6 @@ static void handle_nul(void) { OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), \ OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")) -static struct option builtin_config_options[] = { - CONFIG_LOCATION_OPTIONS, - OPT_GROUP(N_("Action")), - OPT_CMDMODE(0, "get", &actions, N_("get value: name []"), ACTION_GET), - OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key []"), ACTION_GET_ALL), - OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex []"), ACTION_GET_REGEXP), - OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH), - OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value []"), ACTION_REPLACE_ALL), - OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD), - OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name []"), ACTION_UNSET), - OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name []"), ACTION_UNSET_ALL), - OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION), - OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION), - OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST), - OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT), - OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot []"), ACTION_GET_COLOR), - OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot []"), ACTION_GET_COLORBOOL), - CONFIG_TYPE_OPTIONS, - CONFIG_DISPLAY_OPTIONS, - OPT_GROUP(N_("Other")), - OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), - OPT_STRING(0, "comment", &comment_arg, N_("value"), N_("human-readable comment string (# will be prepended as needed)")), - OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")), - OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), - OPT_END(), -}; - static int cmd_config_list(int argc, const char **argv, const char *prefix) { struct option opts[] = { @@ -1059,12 +1031,40 @@ static int cmd_config_edit(int argc, const char **argv, const char *prefix) static int cmd_config_actions(int argc, const char **argv, const char *prefix) { + const char *comment_arg = NULL; + int actions = 0; + struct option opts[] = { + CONFIG_LOCATION_OPTIONS, + OPT_GROUP(N_("Action")), + OPT_CMDMODE(0, "get", &actions, N_("get value: name []"), ACTION_GET), + OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key []"), ACTION_GET_ALL), + OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex []"), ACTION_GET_REGEXP), + OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH), + OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value []"), ACTION_REPLACE_ALL), + OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD), + OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name []"), ACTION_UNSET), + OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name []"), ACTION_UNSET_ALL), + OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION), + OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION), + OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST), + OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT), + OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot []"), ACTION_GET_COLOR), + OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot []"), ACTION_GET_COLORBOOL), + CONFIG_TYPE_OPTIONS, + CONFIG_DISPLAY_OPTIONS, + OPT_GROUP(N_("Other")), + OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), + OPT_STRING(0, "comment", &comment_arg, N_("value"), N_("human-readable comment string (# will be prepended as needed)")), + OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")), + OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), + OPT_END(), + }; char *value = NULL, *comment = NULL; int flags = 0; int ret = 0; struct key_value_info default_kvi = KVI_INIT; - argc = parse_options(argc, argv, prefix, builtin_config_options, + argc = parse_options(argc, argv, prefix, opts, builtin_config_usage, PARSE_OPT_STOP_AT_NON_OPTION); From 9cab5e8078c314957395ddb4c198c3320c4e5a91 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:41:57 +0200 Subject: [PATCH 05/21] builtin/config: move actions into `cmd_config_actions()` We only use actions in the legacy mode. Convert them to an enum and move them into `cmd_config_actions()` to clearly demonstrate that they are not used anywhere else. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 48 +++++++++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index e9956574fe..0842e4f198 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -87,30 +87,6 @@ static int show_origin; static int show_scope; static int fixed_value; -#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) - -/* - * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than - * one line of output and which should therefore be paged. - */ -#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \ - ACTION_GET_REGEXP | ACTION_GET_URLMATCH) - #define TYPE_BOOL 1 #define TYPE_INT 2 #define TYPE_BOOL_OR_INT 3 @@ -1031,6 +1007,24 @@ static int cmd_config_edit(int argc, const char **argv, const char *prefix) static int cmd_config_actions(int argc, const char **argv, const char *prefix) { + enum { + ACTION_GET = (1<<0), + ACTION_GET_ALL = (1<<1), + ACTION_GET_REGEXP = (1<<2), + ACTION_REPLACE_ALL = (1<<3), + ACTION_ADD = (1<<4), + ACTION_UNSET = (1<<5), + ACTION_UNSET_ALL = (1<<6), + ACTION_RENAME_SECTION = (1<<7), + ACTION_REMOVE_SECTION = (1<<8), + ACTION_LIST = (1<<9), + ACTION_EDIT = (1<<10), + ACTION_SET = (1<<11), + ACTION_SET_ALL = (1<<12), + ACTION_GET_COLOR = (1<<13), + ACTION_GET_COLORBOOL = (1<<14), + ACTION_GET_URLMATCH = (1<<15), + }; const char *comment_arg = NULL; int actions = 0; struct option opts[] = { @@ -1147,7 +1141,11 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) comment = git_config_prepare_comment_string(comment_arg); - if (actions & PAGING_ACTIONS) + /* + * The following actions may produce more than one line of output and + * should therefore be paged. + */ + if (actions & (ACTION_LIST | ACTION_GET_ALL | ACTION_GET_REGEXP | ACTION_GET_URLMATCH)) setup_auto_pager("config", 1); if (actions == ACTION_LIST) { From e44b018c5299f2632fcbb079bead00a529546763 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:02 +0200 Subject: [PATCH 06/21] builtin/config: check for writeability after source is set up The `check_write()` function verifies that we do not try to write to a config source that cannot be written to, like for example stdin. But while the new subcommands do call this function, they do so before calling `handle_config_location()`. Consequently, we only end up checking the default config location for writeability, not the location that was actually specified by the caller of git-config(1). Fix this by calling `check_write()` after `handle_config_location()`. We will further clarify the relationship between those two functions in a subsequent commit where we remove the global state that both implicitly rely on. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 10 +++++----- t/t1300-config.sh | 6 ++++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 0842e4f198..9866d1a055 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -843,7 +843,6 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, opts, builtin_config_set_usage, PARSE_OPT_STOP_AT_NON_OPTION); - check_write(); check_argc(argc, 2, 2); if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern) @@ -856,6 +855,7 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix) comment = git_config_prepare_comment_string(comment_arg); handle_config_location(prefix); + check_write(); value = normalize_value(argv[0], argv[1], &default_kvi); @@ -891,13 +891,13 @@ static int cmd_config_unset(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, opts, builtin_config_unset_usage, PARSE_OPT_STOP_AT_NON_OPTION); - check_write(); check_argc(argc, 1, 1); if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern) die(_("--fixed-value only applies with 'value-pattern'")); handle_config_location(prefix); + check_write(); if ((flags & CONFIG_FLAGS_MULTI_REPLACE) || value_pattern) return git_config_set_multivar_in_file_gently(given_config_source.file, @@ -918,10 +918,10 @@ static int cmd_config_rename_section(int argc, const char **argv, const char *pr argc = parse_options(argc, argv, prefix, opts, builtin_config_rename_section_usage, PARSE_OPT_STOP_AT_NON_OPTION); - check_write(); check_argc(argc, 2, 2); handle_config_location(prefix); + check_write(); ret = git_config_rename_section_in_file(given_config_source.file, argv[0], argv[1]); @@ -943,10 +943,10 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr argc = parse_options(argc, argv, prefix, opts, builtin_config_remove_section_usage, PARSE_OPT_STOP_AT_NON_OPTION); - check_write(); check_argc(argc, 1, 1); handle_config_location(prefix); + check_write(); ret = git_config_rename_section_in_file(given_config_source.file, argv[0], NULL); @@ -997,10 +997,10 @@ static int cmd_config_edit(int argc, const char **argv, const char *prefix) }; argc = parse_options(argc, argv, prefix, opts, builtin_config_edit_usage, 0); - check_write(); check_argc(argc, 0, 0); handle_config_location(prefix); + check_write(); return show_editor(); } diff --git a/t/t1300-config.sh b/t/t1300-config.sh index d90a69b29f..9de2d95f06 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2835,6 +2835,12 @@ test_expect_success 'specifying multiple modes causes failure' ' test_cmp expect err ' +test_expect_success 'writing to stdin is rejected' ' + echo "fatal: writing to stdin is not supported" >expect && + test_must_fail git config ${mode_set} --file - foo.bar baz 2>err && + test_cmp expect err +' + done test_done From 12b23068307d512de7de8f551102f5928ab8b88a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:06 +0200 Subject: [PATCH 07/21] config: make the config source const The `struct git_config_source` passed to `config_with_options()` is never modified. Let's mark it as `const` to clarify. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- config.c | 4 ++-- config.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config.c b/config.c index 13cf9eeb16..bc0c005039 100644 --- a/config.c +++ b/config.c @@ -125,7 +125,7 @@ struct config_include_data { config_fn_t fn; void *data; const struct config_options *opts; - struct git_config_source *config_source; + const struct git_config_source *config_source; struct repository *repo; /* @@ -2105,7 +2105,7 @@ static int do_git_config_sequence(const struct config_options *opts, } int config_with_options(config_fn_t fn, void *data, - struct git_config_source *config_source, + const struct git_config_source *config_source, struct repository *repo, const struct config_options *opts) { diff --git a/config.h b/config.h index db8b608064..e66c84520b 100644 --- a/config.h +++ b/config.h @@ -232,7 +232,7 @@ void git_config(config_fn_t fn, void *); * sets `opts.respect_includes` to `1` by default. */ int config_with_options(config_fn_t fn, void *, - struct git_config_source *config_source, + const struct git_config_source *config_source, struct repository *repo, const struct config_options *opts); From 999425cb12face7e8e67c87d255003d40a3d3cf8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:11 +0200 Subject: [PATCH 08/21] builtin/config: refactor functions to have common exit paths Refactor functions to have a single exit path. This will make it easier in subsequent commits to add common cleanup code. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 64 ++++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 9866d1a055..155564b832 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -798,6 +798,7 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) OPT_STRING(0, "default", &default_value, N_("value"), N_("use default value when missing entry")), OPT_END(), }; + int ret; argc = parse_options(argc, argv, prefix, opts, builtin_config_get_usage, PARSE_OPT_STOP_AT_NON_OPTION); @@ -816,8 +817,11 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) setup_auto_pager("config", 1); if (url) - return get_urlmatch(argv[0], url); - return get_value(argv[0], value_pattern, flags); + ret = get_urlmatch(argv[0], url); + else + ret = get_value(argv[0], value_pattern, flags); + + return ret; } static int cmd_config_set(int argc, const char **argv, const char *prefix) @@ -888,6 +892,7 @@ static int cmd_config_unset(int argc, const char **argv, const char *prefix) OPT_BIT(0, "fixed-value", &flags, N_("use string equality when comparing values to value pattern"), CONFIG_FLAGS_FIXED_VALUE), OPT_END(), }; + int ret; argc = parse_options(argc, argv, prefix, opts, builtin_config_unset_usage, PARSE_OPT_STOP_AT_NON_OPTION); @@ -900,12 +905,14 @@ static int cmd_config_unset(int argc, const char **argv, const char *prefix) check_write(); if ((flags & CONFIG_FLAGS_MULTI_REPLACE) || value_pattern) - return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], NULL, value_pattern, - NULL, flags); + ret = git_config_set_multivar_in_file_gently(given_config_source.file, + argv[0], NULL, value_pattern, + NULL, flags); else - return git_config_set_in_file_gently(given_config_source.file, argv[0], - NULL, NULL); + ret = git_config_set_in_file_gently(given_config_source.file, argv[0], + NULL, NULL); + + return ret; } static int cmd_config_rename_section(int argc, const char **argv, const char *prefix) @@ -926,11 +933,13 @@ static int cmd_config_rename_section(int argc, const char **argv, const char *pr ret = git_config_rename_section_in_file(given_config_source.file, argv[0], argv[1]); if (ret < 0) - return ret; + goto out; else if (!ret) die(_("no such section: %s"), argv[0]); + ret = 0; - return 0; +out: + return ret; } static int cmd_config_remove_section(int argc, const char **argv, const char *prefix) @@ -951,11 +960,13 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr ret = git_config_rename_section_in_file(given_config_source.file, argv[0], NULL); if (ret < 0) - return ret; + goto out; else if (!ret) die(_("no such section: %s"), argv[0]); + ret = 0; - return 0; +out: + return ret; } static int show_editor(void) @@ -1199,41 +1210,41 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); - return get_value(argv[0], argv[1], flags); + ret = get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_ALL) { do_all = 1; check_argc(argc, 1, 2); - return get_value(argv[0], argv[1], flags); + ret = get_value(argv[0], argv[1], flags); } 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], flags); + ret = get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_URLMATCH) { check_argc(argc, 2, 2); - return get_urlmatch(argv[0], argv[1]); + ret = get_urlmatch(argv[0], argv[1]); } else if (actions == ACTION_UNSET) { check_write(); check_argc(argc, 1, 2); if (argc == 2) - return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], NULL, argv[1], - NULL, flags); + ret = git_config_set_multivar_in_file_gently(given_config_source.file, + argv[0], NULL, argv[1], + NULL, flags); else - return git_config_set_in_file_gently(given_config_source.file, - argv[0], NULL, NULL); + ret = git_config_set_in_file_gently(given_config_source.file, + argv[0], NULL, NULL); } else if (actions == 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], - NULL, flags | CONFIG_FLAGS_MULTI_REPLACE); + ret = git_config_set_multivar_in_file_gently(given_config_source.file, + argv[0], NULL, argv[1], + NULL, flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_RENAME_SECTION) { check_write(); @@ -1241,7 +1252,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) ret = git_config_rename_section_in_file(given_config_source.file, argv[0], argv[1]); if (ret < 0) - return ret; + goto out; else if (!ret) die(_("no such section: %s"), argv[0]); else @@ -1253,7 +1264,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) ret = git_config_rename_section_in_file(given_config_source.file, argv[0], NULL); if (ret < 0) - return ret; + goto out; else if (!ret) die(_("no such section: %s"), argv[0]); else @@ -1267,9 +1278,10 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) check_argc(argc, 1, 2); if (argc == 2) color_stdout_is_tty = git_config_bool("command line", argv[1]); - return get_colorbool(argv[0], argc == 2); + ret = get_colorbool(argv[0], argc == 2); } +out: free(comment); free(value); return ret; From ddb103c2c7047ed4de0f00a3aeb371245532c2f9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:16 +0200 Subject: [PATCH 09/21] builtin/config: move location options into local variables The location options are tracked via a set of global variables. Move them into a self-contained structure so that we can easily parse all relevant options and hand them over to the various functions that require them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 313 ++++++++++++++++++++++++++--------------------- 1 file changed, 176 insertions(+), 137 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 155564b832..ec36d7289a 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -62,6 +62,26 @@ static const char *const builtin_config_edit_usage[] = { NULL }; +#define CONFIG_LOCATION_OPTIONS(opts) \ + OPT_GROUP(N_("Config file location")), \ + OPT_BOOL(0, "global", &opts.use_global_config, N_("use global config file")), \ + OPT_BOOL(0, "system", &opts.use_system_config, N_("use system config file")), \ + OPT_BOOL(0, "local", &opts.use_local_config, N_("use repository config file")), \ + OPT_BOOL(0, "worktree", &opts.use_worktree_config, N_("use per-worktree config file")), \ + OPT_STRING('f', "file", &opts.source.file, N_("file"), N_("use given config file")), \ + OPT_STRING(0, "blob", &opts.source.blob, N_("blob-id"), N_("read config from given blob object")) + +struct config_location_options { + struct git_config_source source; + struct config_options options; + char *file_to_free; + int use_global_config; + int use_system_config; + int use_local_config; + int use_worktree_config; +}; +#define CONFIG_LOCATION_OPTIONS_INIT {0} + static char *key; static regex_t *key_regexp; static const char *value_pattern; @@ -75,14 +95,10 @@ static char delim = '='; static char key_delim = ' '; static char term = '\n'; -static int use_global_config, use_system_config, use_local_config; -static int use_worktree_config; -static struct git_config_source given_config_source; static int type; static char *default_value; static int end_nul; static int respect_includes_opt = -1; -static struct config_options config_options; static int show_origin; static int show_scope; static int fixed_value; @@ -298,7 +314,8 @@ static int collect_config(const char *key_, const char *value_, return format_config(&values->items[values->nr++], key_, value_, kvi); } -static int get_value(const char *key_, const char *regex_, unsigned flags) +static int get_value(const struct config_location_options *opts, + const char *key_, const char *regex_, unsigned flags) { int ret = CONFIG_GENERIC_ERROR; struct strbuf_list values = {NULL}; @@ -353,8 +370,8 @@ static int get_value(const char *key_, const char *regex_, unsigned flags) } config_with_options(collect_config, &values, - &given_config_source, the_repository, - &config_options); + &opts->source, the_repository, + &opts->options); if (!values.nr && default_value) { struct key_value_info kvi = KVI_INIT; @@ -464,14 +481,15 @@ static int git_get_color_config(const char *var, const char *value, return 0; } -static void get_color(const char *var, const char *def_color) +static void get_color(const struct config_location_options *opts, + const char *var, const char *def_color) { get_color_slot = var; get_color_found = 0; parsed_color[0] = '\0'; config_with_options(git_get_color_config, NULL, - &given_config_source, the_repository, - &config_options); + &opts->source, the_repository, + &opts->options); if (!get_color_found && def_color) { if (color_parse(def_color, parsed_color) < 0) @@ -497,15 +515,16 @@ static int git_get_colorbool_config(const char *var, const char *value, return 0; } -static int get_colorbool(const char *var, int print) +static int get_colorbool(const struct config_location_options *opts, + const char *var, int print) { get_colorbool_slot = var; get_colorbool_found = -1; get_diff_color_found = -1; get_color_ui_found = -1; config_with_options(git_get_colorbool_config, NULL, - &given_config_source, the_repository, - &config_options); + &opts->source, the_repository, + &opts->options); if (get_colorbool_found < 0) { if (!strcmp(get_colorbool_slot, "color.diff")) @@ -527,15 +546,15 @@ static int get_colorbool(const char *var, int print) return get_colorbool_found ? 0 : 1; } -static void check_write(void) +static void check_write(const struct git_config_source *source) { - if (!given_config_source.file && !startup_info->have_repository) + if (!source->file && !startup_info->have_repository) die(_("not in a git directory")); - if (given_config_source.use_stdin) + if (source->use_stdin) die(_("writing to stdin is not supported")); - if (given_config_source.blob) + if (source->blob) die(_("writing config blobs is not supported")); } @@ -572,7 +591,8 @@ static int urlmatch_collect_fn(const char *var, const char *value, return 0; } -static int get_urlmatch(const char *var, const char *url) +static int get_urlmatch(const struct config_location_options *opts, + const char *var, const char *url) { int ret; char *section_tail; @@ -599,8 +619,8 @@ static int get_urlmatch(const char *var, const char *url) } config_with_options(urlmatch_config_entry, &config, - &given_config_source, the_repository, - &config_options); + &opts->source, the_repository, + &opts->options); ret = !values.nr; @@ -638,34 +658,39 @@ static char *default_user_config(void) return strbuf_detach(&buf, NULL); } -static void handle_config_location(const char *prefix) +static void location_options_init(struct config_location_options *opts, + const char *prefix) { - if (use_global_config + use_system_config + use_local_config + - use_worktree_config + - !!given_config_source.file + !!given_config_source.blob > 1) { + if (!opts->source.file) + opts->source.file = opts->file_to_free = + xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); + + if (opts->use_global_config + opts->use_system_config + + opts->use_local_config + opts->use_worktree_config + + !!opts->source.file + !!opts->source.blob > 1) { error(_("only one config file at a time")); exit(129); } if (!startup_info->have_repository) { - if (use_local_config) + if (opts->use_local_config) die(_("--local can only be used inside a git repository")); - if (given_config_source.blob) + if (opts->source.blob) die(_("--blob can only be used inside a git repository")); - if (use_worktree_config) + if (opts->use_worktree_config) die(_("--worktree can only be used inside a git repository")); } - if (given_config_source.file && - !strcmp(given_config_source.file, "-")) { - given_config_source.file = NULL; - given_config_source.use_stdin = 1; - given_config_source.scope = CONFIG_SCOPE_COMMAND; + if (opts->source.file && + !strcmp(opts->source.file, "-")) { + opts->source.file = NULL; + opts->source.use_stdin = 1; + opts->source.scope = CONFIG_SCOPE_COMMAND; } - if (use_global_config) { - given_config_source.file = git_global_config(); - if (!given_config_source.file) + if (opts->use_global_config) { + opts->source.file = opts->file_to_free = git_global_config(); + if (!opts->source.file) /* * It is unknown if HOME/.gitconfig exists, so * we do not know if we should write to XDG @@ -673,17 +698,18 @@ static void handle_config_location(const char *prefix) * is set and points at a sane location. */ die(_("$HOME not set")); - given_config_source.scope = CONFIG_SCOPE_GLOBAL; - } else if (use_system_config) { - given_config_source.file = git_system_config(); - given_config_source.scope = CONFIG_SCOPE_SYSTEM; - } else if (use_local_config) { - given_config_source.file = git_pathdup("config"); - given_config_source.scope = CONFIG_SCOPE_LOCAL; - } else if (use_worktree_config) { + opts->source.scope = CONFIG_SCOPE_GLOBAL; + } else if (opts->use_system_config) { + opts->source.file = opts->file_to_free = git_system_config(); + opts->source.scope = CONFIG_SCOPE_SYSTEM; + } else if (opts->use_local_config) { + opts->source.file = opts->file_to_free = git_pathdup("config"); + opts->source.scope = CONFIG_SCOPE_LOCAL; + } else if (opts->use_worktree_config) { struct worktree **worktrees = get_worktrees(); if (the_repository->repository_format_worktree_config) - given_config_source.file = git_pathdup("config.worktree"); + opts->source.file = opts->file_to_free = + git_pathdup("config.worktree"); else if (worktrees[0] && worktrees[1]) die(_("--worktree cannot be used with multiple " "working trees unless the config\n" @@ -691,28 +717,34 @@ static void handle_config_location(const char *prefix) "Please read \"CONFIGURATION FILE\"\n" "section in \"git help worktree\" for details")); else - given_config_source.file = git_pathdup("config"); - given_config_source.scope = CONFIG_SCOPE_LOCAL; + opts->source.file = opts->file_to_free = + git_pathdup("config"); + opts->source.scope = CONFIG_SCOPE_LOCAL; free_worktrees(worktrees); - } else if (given_config_source.file) { - if (!is_absolute_path(given_config_source.file) && prefix) - given_config_source.file = - prefix_filename(prefix, given_config_source.file); - given_config_source.scope = CONFIG_SCOPE_COMMAND; - } else if (given_config_source.blob) { - given_config_source.scope = CONFIG_SCOPE_COMMAND; + } else if (opts->source.file) { + if (!is_absolute_path(opts->source.file) && prefix) + opts->source.file = opts->file_to_free = + prefix_filename(prefix, opts->source.file); + opts->source.scope = CONFIG_SCOPE_COMMAND; + } else if (opts->source.blob) { + opts->source.scope = CONFIG_SCOPE_COMMAND; } if (respect_includes_opt == -1) - config_options.respect_includes = !given_config_source.file; + opts->options.respect_includes = !opts->source.file; else - config_options.respect_includes = respect_includes_opt; + opts->options.respect_includes = respect_includes_opt; if (startup_info->have_repository) { - config_options.commondir = get_git_common_dir(); - config_options.git_dir = get_git_dir(); + opts->options.commondir = get_git_common_dir(); + opts->options.git_dir = get_git_dir(); } } +static void location_options_release(struct config_location_options *opts) +{ + free(opts->file_to_free); +} + static void handle_nul(void) { if (end_nul) { term = '\0'; @@ -721,15 +753,6 @@ static void handle_nul(void) { } } -#define CONFIG_LOCATION_OPTIONS \ - OPT_GROUP(N_("Config file location")), \ - OPT_BOOL(0, "global", &use_global_config, N_("use global config file")), \ - OPT_BOOL(0, "system", &use_system_config, N_("use system config file")), \ - OPT_BOOL(0, "local", &use_local_config, N_("use repository config file")), \ - OPT_BOOL(0, "worktree", &use_worktree_config, N_("use per-worktree config file")), \ - 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")) - #define CONFIG_TYPE_OPTIONS \ OPT_GROUP(N_("Type")), \ OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type), \ @@ -749,8 +772,9 @@ static void handle_nul(void) { static int cmd_config_list(int argc, const char **argv, const char *prefix) { + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), CONFIG_DISPLAY_OPTIONS, OPT_GROUP(N_("Other")), OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), @@ -760,30 +784,32 @@ static int cmd_config_list(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, opts, builtin_config_list_usage, 0); check_argc(argc, 0, 0); - handle_config_location(prefix); + location_options_init(&location_opts, prefix); handle_nul(); setup_auto_pager("config", 1); if (config_with_options(show_all_config, NULL, - &given_config_source, the_repository, - &config_options) < 0) { - if (given_config_source.file) + &location_opts.source, the_repository, + &location_opts.options) < 0) { + if (location_opts.source.file) die_errno(_("unable to read config file '%s'"), - given_config_source.file); + location_opts.source.file); else die(_("error processing config file(s)")); } + location_options_release(&location_opts); return 0; } static int cmd_config_get(int argc, const char **argv, const char *prefix) { + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; const char *value_pattern = NULL, *url = NULL; int flags = 0; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), CONFIG_TYPE_OPTIONS, OPT_GROUP(N_("Filter options")), OPT_BOOL(0, "all", &do_all, N_("return all values for multi-valued config options")), @@ -811,26 +837,28 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) if (url && (do_all || use_key_regexp || value_pattern)) die(_("--url= cannot be used with --all, --regexp or --value")); - handle_config_location(prefix); + location_options_init(&location_opts, prefix); handle_nul(); setup_auto_pager("config", 1); if (url) - ret = get_urlmatch(argv[0], url); + ret = get_urlmatch(&location_opts, argv[0], url); else - ret = get_value(argv[0], value_pattern, flags); + ret = get_value(&location_opts, argv[0], value_pattern, flags); + location_options_release(&location_opts); return ret; } static int cmd_config_set(int argc, const char **argv, const char *prefix) { + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; const char *value_pattern = NULL, *comment_arg = NULL; char *comment = NULL; int flags = 0, append = 0; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), CONFIG_TYPE_OPTIONS, OPT_GROUP(N_("Filter")), OPT_BIT(0, "all", &flags, N_("replace multi-valued config option with new value"), CONFIG_FLAGS_MULTI_REPLACE), @@ -858,23 +886,24 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix) comment = git_config_prepare_comment_string(comment_arg); - handle_config_location(prefix); - check_write(); + location_options_init(&location_opts, prefix); + check_write(&location_opts.source); value = normalize_value(argv[0], argv[1], &default_kvi); if ((flags & CONFIG_FLAGS_MULTI_REPLACE) || value_pattern) { - ret = git_config_set_multivar_in_file_gently(given_config_source.file, + ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], value, value_pattern, comment, flags); } else { - ret = git_config_set_in_file_gently(given_config_source.file, + ret = git_config_set_in_file_gently(location_opts.source.file, argv[0], comment, value); if (ret == CONFIG_NOTHING_SET) error(_("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s."), argv[0]); } + location_options_release(&location_opts); free(comment); free(value); return ret; @@ -882,10 +911,11 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix) static int cmd_config_unset(int argc, const char **argv, const char *prefix) { + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; const char *value_pattern = NULL; int flags = 0; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), OPT_GROUP(N_("Filter")), OPT_BIT(0, "all", &flags, N_("replace multi-valued config option with new value"), CONFIG_FLAGS_MULTI_REPLACE), OPT_STRING(0, "value", &value_pattern, N_("pattern"), N_("show config with values matching the pattern")), @@ -901,24 +931,26 @@ static int cmd_config_unset(int argc, const char **argv, const char *prefix) if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern) die(_("--fixed-value only applies with 'value-pattern'")); - handle_config_location(prefix); - check_write(); + location_options_init(&location_opts, prefix); + check_write(&location_opts.source); if ((flags & CONFIG_FLAGS_MULTI_REPLACE) || value_pattern) - ret = git_config_set_multivar_in_file_gently(given_config_source.file, + ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], NULL, value_pattern, NULL, flags); else - ret = git_config_set_in_file_gently(given_config_source.file, argv[0], + ret = git_config_set_in_file_gently(location_opts.source.file, argv[0], NULL, NULL); + location_options_release(&location_opts); return ret; } static int cmd_config_rename_section(int argc, const char **argv, const char *prefix) { + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), OPT_END(), }; int ret; @@ -927,10 +959,10 @@ static int cmd_config_rename_section(int argc, const char **argv, const char *pr PARSE_OPT_STOP_AT_NON_OPTION); check_argc(argc, 2, 2); - handle_config_location(prefix); - check_write(); + location_options_init(&location_opts, prefix); + check_write(&location_opts.source); - ret = git_config_rename_section_in_file(given_config_source.file, + ret = git_config_rename_section_in_file(location_opts.source.file, argv[0], argv[1]); if (ret < 0) goto out; @@ -939,13 +971,15 @@ static int cmd_config_rename_section(int argc, const char **argv, const char *pr ret = 0; out: + location_options_release(&location_opts); return ret; } static int cmd_config_remove_section(int argc, const char **argv, const char *prefix) { + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), OPT_END(), }; int ret; @@ -954,10 +988,10 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr PARSE_OPT_STOP_AT_NON_OPTION); check_argc(argc, 1, 1); - handle_config_location(prefix); - check_write(); + location_options_init(&location_opts, prefix); + check_write(&location_opts.source); - ret = git_config_rename_section_in_file(given_config_source.file, + ret = git_config_rename_section_in_file(location_opts.source.file, argv[0], NULL); if (ret < 0) goto out; @@ -966,24 +1000,25 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr ret = 0; out: + location_options_release(&location_opts); return ret; } -static int show_editor(void) +static int show_editor(struct config_location_options *opts) { char *config_file; - if (!given_config_source.file && !startup_info->have_repository) + if (!opts->source.file && !startup_info->have_repository) die(_("not in a git directory")); - if (given_config_source.use_stdin) + if (opts->source.use_stdin) die(_("editing stdin is not supported")); - if (given_config_source.blob) + if (opts->source.blob) die(_("editing blobs is not supported")); git_config(git_default_config, NULL); - config_file = given_config_source.file ? - xstrdup(given_config_source.file) : + config_file = opts->source.file ? + xstrdup(opts->source.file) : git_pathdup("config"); - if (use_global_config) { + if (opts->use_global_config) { int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666); if (fd >= 0) { char *content = default_user_config(); @@ -1002,18 +1037,22 @@ static int show_editor(void) static int cmd_config_edit(int argc, const char **argv, const char *prefix) { + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), OPT_END(), }; + int ret; argc = parse_options(argc, argv, prefix, opts, builtin_config_edit_usage, 0); check_argc(argc, 0, 0); - handle_config_location(prefix); - check_write(); + location_options_init(&location_opts, prefix); + check_write(&location_opts.source); - return show_editor(); + ret = show_editor(&location_opts); + location_options_release(&location_opts); + return ret; } static int cmd_config_actions(int argc, const char **argv, const char *prefix) @@ -1036,10 +1075,11 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) ACTION_GET_COLORBOOL = (1<<14), ACTION_GET_URLMATCH = (1<<15), }; + struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; const char *comment_arg = NULL; int actions = 0; struct option opts[] = { - CONFIG_LOCATION_OPTIONS, + CONFIG_LOCATION_OPTIONS(location_opts), OPT_GROUP(N_("Action")), OPT_CMDMODE(0, "get", &actions, N_("get value: name []"), ACTION_GET), OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key []"), ACTION_GET_ALL), @@ -1073,7 +1113,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) builtin_config_usage, PARSE_OPT_STOP_AT_NON_OPTION); - handle_config_location(prefix); + location_options_init(&location_opts, prefix); handle_nul(); if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) { @@ -1162,94 +1202,94 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) if (actions == ACTION_LIST) { check_argc(argc, 0, 0); if (config_with_options(show_all_config, NULL, - &given_config_source, the_repository, - &config_options) < 0) { - if (given_config_source.file) + &location_opts.source, the_repository, + &location_opts.options) < 0) { + if (location_opts.source.file) die_errno(_("unable to read config file '%s'"), - given_config_source.file); + location_opts.source.file); else die(_("error processing config file(s)")); } } else if (actions == ACTION_EDIT) { - ret = show_editor(); + ret = show_editor(&location_opts); } else if (actions == ACTION_SET) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1], &default_kvi); - ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value); + ret = git_config_set_in_file_gently(location_opts.source.file, argv[0], comment, value); if (ret == CONFIG_NOTHING_SET) error(_("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s."), argv[0]); } else if (actions == ACTION_SET_ALL) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1], &default_kvi); - ret = git_config_set_multivar_in_file_gently(given_config_source.file, + ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], value, argv[2], comment, flags); } else if (actions == ACTION_ADD) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1], &default_kvi); - ret = git_config_set_multivar_in_file_gently(given_config_source.file, + ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], value, CONFIG_REGEX_NONE, comment, flags); } else if (actions == ACTION_REPLACE_ALL) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1], &default_kvi); - ret = git_config_set_multivar_in_file_gently(given_config_source.file, + ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], value, argv[2], comment, flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); - ret = get_value(argv[0], argv[1], flags); + ret = get_value(&location_opts, argv[0], argv[1], flags); } else if (actions == ACTION_GET_ALL) { do_all = 1; check_argc(argc, 1, 2); - ret = get_value(argv[0], argv[1], flags); + ret = get_value(&location_opts, argv[0], argv[1], flags); } else if (actions == ACTION_GET_REGEXP) { show_keys = 1; use_key_regexp = 1; do_all = 1; check_argc(argc, 1, 2); - ret = get_value(argv[0], argv[1], flags); + ret = get_value(&location_opts, argv[0], argv[1], flags); } else if (actions == ACTION_GET_URLMATCH) { check_argc(argc, 2, 2); - ret = get_urlmatch(argv[0], argv[1]); + ret = get_urlmatch(&location_opts, argv[0], argv[1]); } else if (actions == ACTION_UNSET) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 1, 2); if (argc == 2) - ret = git_config_set_multivar_in_file_gently(given_config_source.file, + ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], NULL, argv[1], NULL, flags); else - ret = git_config_set_in_file_gently(given_config_source.file, + ret = git_config_set_in_file_gently(location_opts.source.file, argv[0], NULL, NULL); } else if (actions == ACTION_UNSET_ALL) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 1, 2); - ret = git_config_set_multivar_in_file_gently(given_config_source.file, + ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], NULL, argv[1], NULL, flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_RENAME_SECTION) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 2, 2); - ret = git_config_rename_section_in_file(given_config_source.file, + ret = git_config_rename_section_in_file(location_opts.source.file, argv[0], argv[1]); if (ret < 0) goto out; @@ -1259,9 +1299,9 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) ret = 0; } else if (actions == ACTION_REMOVE_SECTION) { - check_write(); + check_write(&location_opts.source); check_argc(argc, 1, 1); - ret = git_config_rename_section_in_file(given_config_source.file, + ret = git_config_rename_section_in_file(location_opts.source.file, argv[0], NULL); if (ret < 0) goto out; @@ -1272,16 +1312,17 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) } else if (actions == ACTION_GET_COLOR) { check_argc(argc, 1, 2); - get_color(argv[0], argv[1]); + get_color(&location_opts, argv[0], argv[1]); } else if (actions == ACTION_GET_COLORBOOL) { check_argc(argc, 1, 2); if (argc == 2) color_stdout_is_tty = git_config_bool("command line", argv[1]); - ret = get_colorbool(argv[0], argc == 2); + ret = get_colorbool(&location_opts, argv[0], argc == 2); } out: + location_options_release(&location_opts); free(comment); free(value); return ret; @@ -1301,8 +1342,6 @@ int cmd_config(int argc, const char **argv, const char *prefix) OPT_END(), }; - given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); - /* * This is somewhat hacky: we first parse the command line while * keeping all args intact in order to determine whether a subcommand From c0c1e263264fcb60296e2540f39799e925822d6c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:21 +0200 Subject: [PATCH 10/21] builtin/config: move display options into local variables The display options are tracked via a set of global variables. Move them into a self-contained structure so that we can easily parse all relevant options and hand them over to the various functions that require them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 171 ++++++++++++++++++++++++++++------------------- 1 file changed, 101 insertions(+), 70 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index ec36d7289a..1a67a6caef 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -82,25 +82,42 @@ struct config_location_options { }; #define CONFIG_LOCATION_OPTIONS_INIT {0} +#define CONFIG_DISPLAY_OPTIONS(opts) \ + OPT_GROUP(N_("Display options")), \ + OPT_BOOL('z', "null", &opts.end_nul, N_("terminate values with NUL byte")), \ + OPT_BOOL(0, "name-only", &opts.omit_values, N_("show variable names only")), \ + OPT_BOOL(0, "show-origin", &opts.show_origin, N_("show origin of config (file, standard input, blob, command line)")), \ + OPT_BOOL(0, "show-scope", &opts.show_scope, N_("show scope of config (worktree, local, global, system, command)")), \ + OPT_BOOL(0, "show-names", &opts.show_keys, N_("show config keys in addition to their values")) + +struct config_display_options { + int end_nul; + int omit_values; + int show_origin; + int show_scope; + int show_keys; + /* Populated via `display_options_init()`. */ + int term; + int delim; + int key_delim; +}; +#define CONFIG_DISPLAY_OPTIONS_INIT { \ + .term = '\n', \ + .delim = '=', \ + .key_delim = ' ', \ +} + static char *key; static regex_t *key_regexp; static const char *value_pattern; static regex_t *regexp; -static int show_keys; -static int omit_values; static int use_key_regexp; static int do_all; static int do_not_match; -static char delim = '='; -static char key_delim = ' '; -static char term = '\n'; static int type; static char *default_value; -static int end_nul; static int respect_includes_opt = -1; -static int show_origin; -static int show_scope; static int fixed_value; #define TYPE_BOOL 1 @@ -178,24 +195,26 @@ static void check_argc(int argc, int min, int max) exit(129); } -static void show_config_origin(const struct key_value_info *kvi, +static void show_config_origin(const struct config_display_options *opts, + const struct key_value_info *kvi, struct strbuf *buf) { - const char term = end_nul ? '\0' : '\t'; + const char term = opts->end_nul ? '\0' : '\t'; strbuf_addstr(buf, config_origin_type_name(kvi->origin_type)); strbuf_addch(buf, ':'); - if (end_nul) + if (opts->end_nul) strbuf_addstr(buf, kvi->filename ? kvi->filename : ""); else quote_c_style(kvi->filename ? kvi->filename : "", buf, NULL, 0); strbuf_addch(buf, term); } -static void show_config_scope(const struct key_value_info *kvi, +static void show_config_scope(const struct config_display_options *opts, + const struct key_value_info *kvi, struct strbuf *buf) { - const char term = end_nul ? '\0' : '\t'; + const char term = opts->end_nul ? '\0' : '\t'; const char *scope = config_scope_name(kvi->scope); strbuf_addstr(buf, N_(scope)); @@ -204,24 +223,25 @@ static void show_config_scope(const struct key_value_info *kvi, static int show_all_config(const char *key_, const char *value_, const struct config_context *ctx, - void *cb UNUSED) + void *cb) { + const struct config_display_options *opts = cb; const struct key_value_info *kvi = ctx->kvi; - if (show_origin || show_scope) { + if (opts->show_origin || opts->show_scope) { struct strbuf buf = STRBUF_INIT; - if (show_scope) - show_config_scope(kvi, &buf); - if (show_origin) - show_config_origin(kvi, &buf); + if (opts->show_scope) + show_config_scope(opts, kvi, &buf); + if (opts->show_origin) + show_config_origin(opts, kvi, &buf); /* Use fwrite as "buf" can contain \0's if "end_null" is set. */ fwrite(buf.buf, 1, buf.len, stdout); strbuf_release(&buf); } - if (!omit_values && value_) - printf("%s%c%s%c", key_, delim, value_, term); + if (!opts->omit_values && value_) + printf("%s%c%s%c", key_, opts->delim, value_, opts->term); else - printf("%s%c", key_, term); + printf("%s%c", key_, opts->term); return 0; } @@ -231,18 +251,19 @@ struct strbuf_list { int alloc; }; -static int format_config(struct strbuf *buf, const char *key_, +static int format_config(const struct config_display_options *opts, + struct strbuf *buf, const char *key_, const char *value_, const struct key_value_info *kvi) { - if (show_scope) - show_config_scope(kvi, buf); - if (show_origin) - show_config_origin(kvi, buf); - if (show_keys) + if (opts->show_scope) + show_config_scope(opts, kvi, buf); + if (opts->show_origin) + show_config_origin(opts, kvi, buf); + if (opts->show_keys) strbuf_addstr(buf, key_); - if (!omit_values) { - if (show_keys) - strbuf_addch(buf, key_delim); + if (!opts->omit_values) { + if (opts->show_keys) + strbuf_addch(buf, opts->key_delim); if (type == TYPE_INT) strbuf_addf(buf, "%"PRId64, @@ -284,18 +305,24 @@ static int format_config(struct strbuf *buf, const char *key_, strbuf_addstr(buf, value_); } else { /* Just show the key name; back out delimiter */ - if (show_keys) + if (opts->show_keys) strbuf_setlen(buf, buf->len - 1); } } - strbuf_addch(buf, term); + strbuf_addch(buf, opts->term); return 0; } +struct collect_config_data { + const struct config_display_options *display_opts; + struct strbuf_list *values; +}; + static int collect_config(const char *key_, const char *value_, const struct config_context *ctx, void *cb) { - struct strbuf_list *values = cb; + struct collect_config_data *data = cb; + struct strbuf_list *values = data->values; const struct key_value_info *kvi = ctx->kvi; if (!use_key_regexp && strcmp(key_, key)) @@ -311,14 +338,20 @@ static int collect_config(const char *key_, const char *value_, ALLOC_GROW(values->items, values->nr + 1, values->alloc); strbuf_init(&values->items[values->nr], 0); - return format_config(&values->items[values->nr++], key_, value_, kvi); + return format_config(data->display_opts, &values->items[values->nr++], + key_, value_, kvi); } static int get_value(const struct config_location_options *opts, + const struct config_display_options *display_opts, const char *key_, const char *regex_, unsigned flags) { int ret = CONFIG_GENERIC_ERROR; struct strbuf_list values = {NULL}; + struct collect_config_data data = { + .display_opts = display_opts, + .values = &values, + }; int i; if (use_key_regexp) { @@ -369,7 +402,7 @@ static int get_value(const struct config_location_options *opts, } } - config_with_options(collect_config, &values, + config_with_options(collect_config, &data, &opts->source, the_repository, &opts->options); @@ -381,7 +414,7 @@ static int get_value(const struct config_location_options *opts, ALLOC_GROW(values.items, values.nr + 1, values.alloc); item = &values.items[values.nr++]; strbuf_init(item, 0); - if (format_config(item, key_, default_value, &kvi) < 0) + if (format_config(display_opts, item, key_, default_value, &kvi) < 0) die(_("failed to format default config value: %s"), default_value); } @@ -592,10 +625,12 @@ static int urlmatch_collect_fn(const char *var, const char *value, } static int get_urlmatch(const struct config_location_options *opts, + const struct config_display_options *_display_opts, const char *var, const char *url) { int ret; char *section_tail; + struct config_display_options display_opts = *_display_opts; struct string_list_item *item; struct urlmatch_config config = URLMATCH_CONFIG_INIT; struct string_list values = STRING_LIST_INIT_DUP; @@ -612,10 +647,10 @@ static int get_urlmatch(const struct config_location_options *opts, if (section_tail) { *section_tail = '\0'; config.key = section_tail + 1; - show_keys = 0; + display_opts.show_keys = 0; } else { config.key = NULL; - show_keys = 1; + display_opts.show_keys = 1; } config_with_options(urlmatch_config_entry, &config, @@ -628,7 +663,7 @@ static int get_urlmatch(const struct config_location_options *opts, struct urlmatch_current_candidate_value *matched = item->util; struct strbuf buf = STRBUF_INIT; - format_config(&buf, item->string, + format_config(&display_opts, &buf, item->string, matched->value_is_null ? NULL : matched->value.buf, &matched->kvi); fwrite(buf.buf, 1, buf.len, stdout); @@ -745,11 +780,12 @@ static void location_options_release(struct config_location_options *opts) free(opts->file_to_free); } -static void handle_nul(void) { - if (end_nul) { - term = '\0'; - delim = '\n'; - key_delim = '\n'; +static void display_options_init(struct config_display_options *opts) +{ + if (opts->end_nul) { + opts->term = '\0'; + opts->delim = '\n'; + opts->key_delim = '\n'; } } @@ -763,19 +799,13 @@ static void handle_nul(void) { 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) -#define CONFIG_DISPLAY_OPTIONS \ - OPT_GROUP(N_("Display options")), \ - OPT_BOOL('z', "null", &end_nul, N_("terminate values with NUL byte")), \ - OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), \ - OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), \ - OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")) - static int cmd_config_list(int argc, const char **argv, const char *prefix) { struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; + struct config_display_options display_opts = CONFIG_DISPLAY_OPTIONS_INIT; struct option opts[] = { CONFIG_LOCATION_OPTIONS(location_opts), - CONFIG_DISPLAY_OPTIONS, + CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), OPT_END(), @@ -785,11 +815,11 @@ static int cmd_config_list(int argc, const char **argv, const char *prefix) check_argc(argc, 0, 0); location_options_init(&location_opts, prefix); - handle_nul(); + display_options_init(&display_opts); setup_auto_pager("config", 1); - if (config_with_options(show_all_config, NULL, + if (config_with_options(show_all_config, &display_opts, &location_opts.source, the_repository, &location_opts.options) < 0) { if (location_opts.source.file) @@ -806,6 +836,7 @@ static int cmd_config_list(int argc, const char **argv, const char *prefix) static int cmd_config_get(int argc, const char **argv, const char *prefix) { struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; + struct config_display_options display_opts = CONFIG_DISPLAY_OPTIONS_INIT; const char *value_pattern = NULL, *url = NULL; int flags = 0; struct option opts[] = { @@ -817,8 +848,7 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) OPT_STRING(0, "value", &value_pattern, N_("pattern"), N_("show config with values matching the pattern")), OPT_BIT(0, "fixed-value", &flags, N_("use string equality when comparing values to value pattern"), CONFIG_FLAGS_FIXED_VALUE), OPT_STRING(0, "url", &url, N_("URL"), N_("show config matching the given URL")), - CONFIG_DISPLAY_OPTIONS, - OPT_BOOL(0, "show-names", &show_keys, N_("show config keys in addition to their values")), + CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), OPT_STRING(0, "default", &default_value, N_("value"), N_("use default value when missing entry")), @@ -838,14 +868,14 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) die(_("--url= cannot be used with --all, --regexp or --value")); location_options_init(&location_opts, prefix); - handle_nul(); + display_options_init(&display_opts); setup_auto_pager("config", 1); if (url) - ret = get_urlmatch(&location_opts, argv[0], url); + ret = get_urlmatch(&location_opts, &display_opts, argv[0], url); else - ret = get_value(&location_opts, argv[0], value_pattern, flags); + ret = get_value(&location_opts, &display_opts, argv[0], value_pattern, flags); location_options_release(&location_opts); return ret; @@ -1076,6 +1106,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) ACTION_GET_URLMATCH = (1<<15), }; struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; + struct config_display_options display_opts = CONFIG_DISPLAY_OPTIONS_INIT; const char *comment_arg = NULL; int actions = 0; struct option opts[] = { @@ -1096,7 +1127,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot []"), ACTION_GET_COLOR), OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot []"), ACTION_GET_COLORBOOL), CONFIG_TYPE_OPTIONS, - CONFIG_DISPLAY_OPTIONS, + CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), OPT_STRING(0, "comment", &comment_arg, N_("value"), N_("human-readable comment string (# will be prepended as needed)")), @@ -1114,7 +1145,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) PARSE_OPT_STOP_AT_NON_OPTION); location_options_init(&location_opts, prefix); - handle_nul(); + display_options_init(&display_opts); if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) { error(_("--get-color and variable type are incoherent")); @@ -1130,13 +1161,13 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) error(_("no action specified")); exit(129); } - if (omit_values && + if (display_opts.omit_values && !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) { error(_("--name-only is only applicable to --list or --get-regexp")); exit(129); } - if (show_origin && !(actions & + if (display_opts.show_origin && !(actions & (ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) { error(_("--show-origin is only applicable to --get, --get-all, " "--get-regexp, and --list")); @@ -1201,7 +1232,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) if (actions == ACTION_LIST) { check_argc(argc, 0, 0); - if (config_with_options(show_all_config, NULL, + if (config_with_options(show_all_config, &display_opts, &location_opts.source, the_repository, &location_opts.options) < 0) { if (location_opts.source.file) @@ -1250,23 +1281,23 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); - ret = get_value(&location_opts, argv[0], argv[1], flags); + ret = get_value(&location_opts, &display_opts, argv[0], argv[1], flags); } else if (actions == ACTION_GET_ALL) { do_all = 1; check_argc(argc, 1, 2); - ret = get_value(&location_opts, argv[0], argv[1], flags); + ret = get_value(&location_opts, &display_opts, argv[0], argv[1], flags); } else if (actions == ACTION_GET_REGEXP) { - show_keys = 1; + display_opts.show_keys = 1; use_key_regexp = 1; do_all = 1; check_argc(argc, 1, 2); - ret = get_value(&location_opts, argv[0], argv[1], flags); + ret = get_value(&location_opts, &display_opts, argv[0], argv[1], flags); } else if (actions == ACTION_GET_URLMATCH) { check_argc(argc, 2, 2); - ret = get_urlmatch(&location_opts, argv[0], argv[1]); + ret = get_urlmatch(&location_opts, &display_opts, argv[0], argv[1]); } else if (actions == ACTION_UNSET) { check_write(&location_opts.source); From 94c4693079efd107ddb941550f68c98601d26123 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:25 +0200 Subject: [PATCH 11/21] builtin/config: move type options into display options The type options are tracked via a global variable. Move it into the display options instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 60 +++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 1a67a6caef..ff111d83a9 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -82,13 +82,24 @@ struct config_location_options { }; #define CONFIG_LOCATION_OPTIONS_INIT {0} +#define CONFIG_TYPE_OPTIONS(type) \ + OPT_GROUP(N_("Type")), \ + OPT_CALLBACK('t', "type", &type, N_("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, "bool-or-str", &type, N_("value is --bool or string"), TYPE_BOOL_OR_STR), \ + 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) + #define CONFIG_DISPLAY_OPTIONS(opts) \ OPT_GROUP(N_("Display options")), \ OPT_BOOL('z', "null", &opts.end_nul, N_("terminate values with NUL byte")), \ OPT_BOOL(0, "name-only", &opts.omit_values, N_("show variable names only")), \ OPT_BOOL(0, "show-origin", &opts.show_origin, N_("show origin of config (file, standard input, blob, command line)")), \ OPT_BOOL(0, "show-scope", &opts.show_scope, N_("show scope of config (worktree, local, global, system, command)")), \ - OPT_BOOL(0, "show-names", &opts.show_keys, N_("show config keys in addition to their values")) + OPT_BOOL(0, "show-names", &opts.show_keys, N_("show config keys in addition to their values")), \ + CONFIG_TYPE_OPTIONS(opts.type) struct config_display_options { int end_nul; @@ -96,6 +107,7 @@ struct config_display_options { int show_origin; int show_scope; int show_keys; + int type; /* Populated via `display_options_init()`. */ int term; int delim; @@ -114,8 +126,6 @@ static regex_t *regexp; static int use_key_regexp; static int do_all; static int do_not_match; - -static int type; static char *default_value; static int respect_includes_opt = -1; static int fixed_value; @@ -265,13 +275,13 @@ static int format_config(const struct config_display_options *opts, if (opts->show_keys) strbuf_addch(buf, opts->key_delim); - if (type == TYPE_INT) + if (opts->type == TYPE_INT) strbuf_addf(buf, "%"PRId64, git_config_int64(key_, value_ ? value_ : "", kvi)); - else if (type == TYPE_BOOL) + else if (opts->type == TYPE_BOOL) strbuf_addstr(buf, git_config_bool(key_, value_) ? "true" : "false"); - else if (type == TYPE_BOOL_OR_INT) { + else if (opts->type == TYPE_BOOL_OR_INT) { int is_bool, v; v = git_config_bool_or_int(key_, value_, kvi, &is_bool); @@ -279,24 +289,24 @@ static int format_config(const struct config_display_options *opts, strbuf_addstr(buf, v ? "true" : "false"); else strbuf_addf(buf, "%d", v); - } else if (type == TYPE_BOOL_OR_STR) { + } else if (opts->type == TYPE_BOOL_OR_STR) { int v = git_parse_maybe_bool(value_); if (v < 0) strbuf_addstr(buf, value_); else strbuf_addstr(buf, v ? "true" : "false"); - } else if (type == TYPE_PATH) { + } else if (opts->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 (type == TYPE_EXPIRY_DATE) { + } else if (opts->type == TYPE_EXPIRY_DATE) { timestamp_t t; if (git_config_expiry_date(&t, key_, value_) < 0) return -1; strbuf_addf(buf, "%"PRItime, t); - } else if (type == TYPE_COLOR) { + } else if (opts->type == TYPE_COLOR) { char v[COLOR_MAXLEN]; if (git_config_color(v, key_, value_) < 0) return -1; @@ -444,7 +454,7 @@ free_strings: } static char *normalize_value(const char *key, const char *value, - struct key_value_info *kvi) + int type, struct key_value_info *kvi) { if (!value) return NULL; @@ -789,16 +799,6 @@ static void display_options_init(struct config_display_options *opts) } } -#define CONFIG_TYPE_OPTIONS \ - OPT_GROUP(N_("Type")), \ - OPT_CALLBACK('t', "type", &type, N_("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, "bool-or-str", &type, N_("value is --bool or string"), TYPE_BOOL_OR_STR), \ - 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) - static int cmd_config_list(int argc, const char **argv, const char *prefix) { struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; @@ -841,7 +841,6 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) int flags = 0; struct option opts[] = { CONFIG_LOCATION_OPTIONS(location_opts), - CONFIG_TYPE_OPTIONS, OPT_GROUP(N_("Filter options")), OPT_BOOL(0, "all", &do_all, N_("return all values for multi-valued config options")), OPT_BOOL(0, "regexp", &use_key_regexp, N_("interpret the name as a regular expression")), @@ -886,10 +885,10 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix) struct config_location_options location_opts = CONFIG_LOCATION_OPTIONS_INIT; const char *value_pattern = NULL, *comment_arg = NULL; char *comment = NULL; - int flags = 0, append = 0; + int flags = 0, append = 0, type = 0; struct option opts[] = { CONFIG_LOCATION_OPTIONS(location_opts), - CONFIG_TYPE_OPTIONS, + CONFIG_TYPE_OPTIONS(type), OPT_GROUP(N_("Filter")), OPT_BIT(0, "all", &flags, N_("replace multi-valued config option with new value"), CONFIG_FLAGS_MULTI_REPLACE), OPT_STRING(0, "value", &value_pattern, N_("pattern"), N_("show config with values matching the pattern")), @@ -919,7 +918,7 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix) location_options_init(&location_opts, prefix); check_write(&location_opts.source); - value = normalize_value(argv[0], argv[1], &default_kvi); + value = normalize_value(argv[0], argv[1], type, &default_kvi); if ((flags & CONFIG_FLAGS_MULTI_REPLACE) || value_pattern) { ret = git_config_set_multivar_in_file_gently(location_opts.source.file, @@ -1126,7 +1125,6 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT), OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot []"), ACTION_GET_COLOR), OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot []"), ACTION_GET_COLORBOOL), - CONFIG_TYPE_OPTIONS, CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), @@ -1147,7 +1145,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) location_options_init(&location_opts, prefix); display_options_init(&display_opts); - if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) { + if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && display_opts.type) { error(_("--get-color and variable type are incoherent")); exit(129); } @@ -1248,7 +1246,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) else if (actions == ACTION_SET) { check_write(&location_opts.source); check_argc(argc, 2, 2); - value = normalize_value(argv[0], argv[1], &default_kvi); + value = normalize_value(argv[0], argv[1], display_opts.type, &default_kvi); ret = git_config_set_in_file_gently(location_opts.source.file, argv[0], comment, value); if (ret == CONFIG_NOTHING_SET) error(_("cannot overwrite multiple values with a single value\n" @@ -1257,7 +1255,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) else if (actions == ACTION_SET_ALL) { check_write(&location_opts.source); check_argc(argc, 2, 3); - value = normalize_value(argv[0], argv[1], &default_kvi); + value = normalize_value(argv[0], argv[1], display_opts.type, &default_kvi); ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], value, argv[2], comment, flags); @@ -1265,7 +1263,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) else if (actions == ACTION_ADD) { check_write(&location_opts.source); check_argc(argc, 2, 2); - value = normalize_value(argv[0], argv[1], &default_kvi); + value = normalize_value(argv[0], argv[1], display_opts.type, &default_kvi); ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], value, CONFIG_REGEX_NONE, @@ -1274,7 +1272,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) else if (actions == ACTION_REPLACE_ALL) { check_write(&location_opts.source); check_argc(argc, 2, 3); - value = normalize_value(argv[0], argv[1], &default_kvi); + value = normalize_value(argv[0], argv[1], display_opts.type, &default_kvi); ret = git_config_set_multivar_in_file_gently(location_opts.source.file, argv[0], value, argv[2], comment, flags | CONFIG_FLAGS_MULTI_REPLACE); From 4090a9c948eaf755be203d313dd78b1189828f8b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:30 +0200 Subject: [PATCH 12/21] builtin/config: move default value into display options The default value is tracked via a global variable. Move it into the display options instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index ff111d83a9..8a327e770d 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -108,6 +108,7 @@ struct config_display_options { int show_scope; int show_keys; int type; + char *default_value; /* Populated via `display_options_init()`. */ int term; int delim; @@ -126,7 +127,6 @@ static regex_t *regexp; static int use_key_regexp; static int do_all; static int do_not_match; -static char *default_value; static int respect_includes_opt = -1; static int fixed_value; @@ -416,7 +416,7 @@ static int get_value(const struct config_location_options *opts, &opts->source, the_repository, &opts->options); - if (!values.nr && default_value) { + if (!values.nr && display_opts->default_value) { struct key_value_info kvi = KVI_INIT; struct strbuf *item; @@ -424,9 +424,10 @@ static int get_value(const struct config_location_options *opts, ALLOC_GROW(values.items, values.nr + 1, values.alloc); item = &values.items[values.nr++]; strbuf_init(item, 0); - if (format_config(display_opts, item, key_, default_value, &kvi) < 0) + if (format_config(display_opts, item, key_, + display_opts->default_value, &kvi) < 0) die(_("failed to format default config value: %s"), - default_value); + display_opts->default_value); } ret = !values.nr; @@ -850,7 +851,8 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), - OPT_STRING(0, "default", &default_value, N_("value"), N_("use default value when missing entry")), + OPT_STRING(0, "default", &display_opts.default_value, + N_("value"), N_("use default value when missing entry")), OPT_END(), }; int ret; @@ -861,7 +863,7 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern) die(_("--fixed-value only applies with 'value-pattern'")); - if (default_value && (do_all || url)) + if (display_opts.default_value && (do_all || url)) die(_("--default= cannot be used with --all or --url=")); if (url && (do_all || use_key_regexp || value_pattern)) die(_("--url= cannot be used with --all, --regexp or --value")); @@ -1127,7 +1129,8 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot []"), ACTION_GET_COLORBOOL), CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), - OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), + OPT_STRING(0, "default", &display_opts.default_value, + N_("value"), N_("with --get, use default value when missing entry")), OPT_STRING(0, "comment", &comment_arg, N_("value"), N_("human-readable comment string (# will be prepended as needed)")), OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")), OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), @@ -1172,7 +1175,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) exit(129); } - if (default_value && !(actions & ACTION_GET)) { + if (display_opts.default_value && !(actions & ACTION_GET)) { error(_("--default is only applicable to --get")); exit(129); } From 8c86981228912e06b6da1fd76076b306fb27d2b5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:35 +0200 Subject: [PATCH 13/21] builtin/config: move `respect_includes_opt` into location options The variable tracking whether or not we want to honor includes is tracked via a global variable. Move it into the location options instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 8a327e770d..c54bdcbcdb 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -79,8 +79,11 @@ struct config_location_options { int use_system_config; int use_local_config; int use_worktree_config; + int respect_includes_opt; }; -#define CONFIG_LOCATION_OPTIONS_INIT {0} +#define CONFIG_LOCATION_OPTIONS_INIT { \ + .respect_includes_opt = -1, \ +} #define CONFIG_TYPE_OPTIONS(type) \ OPT_GROUP(N_("Type")), \ @@ -127,7 +130,6 @@ static regex_t *regexp; static int use_key_regexp; static int do_all; static int do_not_match; -static int respect_includes_opt = -1; static int fixed_value; #define TYPE_BOOL 1 @@ -776,10 +778,10 @@ static void location_options_init(struct config_location_options *opts, opts->source.scope = CONFIG_SCOPE_COMMAND; } - if (respect_includes_opt == -1) + if (opts->respect_includes_opt == -1) opts->options.respect_includes = !opts->source.file; else - opts->options.respect_includes = respect_includes_opt; + opts->options.respect_includes = opts->respect_includes_opt; if (startup_info->have_repository) { opts->options.commondir = get_git_common_dir(); opts->options.git_dir = get_git_dir(); @@ -808,7 +810,8 @@ static int cmd_config_list(int argc, const char **argv, const char *prefix) CONFIG_LOCATION_OPTIONS(location_opts), CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), - OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), + OPT_BOOL(0, "includes", &location_opts.respect_includes_opt, + N_("respect include directives on lookup")), OPT_END(), }; @@ -850,7 +853,8 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) OPT_STRING(0, "url", &url, N_("URL"), N_("show config matching the given URL")), CONFIG_DISPLAY_OPTIONS(display_opts), OPT_GROUP(N_("Other")), - OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), + OPT_BOOL(0, "includes", &location_opts.respect_includes_opt, + N_("respect include directives on lookup")), OPT_STRING(0, "default", &display_opts.default_value, N_("value"), N_("use default value when missing entry")), OPT_END(), @@ -1133,7 +1137,8 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) N_("value"), N_("with --get, use default value when missing entry")), OPT_STRING(0, "comment", &comment_arg, N_("value"), N_("human-readable comment string (# will be prepended as needed)")), OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")), - OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), + OPT_BOOL(0, "includes", &location_opts.respect_includes_opt, + N_("respect include directives on lookup")), OPT_END(), }; char *value = NULL, *comment = NULL; From 65d197cffc7ad52c61398c2579efa988779cc7ab Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:39 +0200 Subject: [PATCH 14/21] builtin/config: convert `do_not_match` to a local variable The `do_not_match` variable is used by the `format_config()` callback as an indicator whether or not the passed regular expression is negated. It is only ever set up by its only caller, `collect_config()` and can thus easily be moved into the `collect_config_data` structure. Do so to remove our reliance on global state. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index c54bdcbcdb..bc80fd293a 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -129,7 +129,6 @@ static const char *value_pattern; static regex_t *regexp; static int use_key_regexp; static int do_all; -static int do_not_match; static int fixed_value; #define TYPE_BOOL 1 @@ -328,6 +327,7 @@ static int format_config(const struct config_display_options *opts, struct collect_config_data { const struct config_display_options *display_opts; struct strbuf_list *values; + int do_not_match; }; static int collect_config(const char *key_, const char *value_, @@ -344,7 +344,7 @@ static int collect_config(const char *key_, const char *value_, if (fixed_value && strcmp(value_pattern, (value_?value_:""))) return 0; if (regexp != NULL && - (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0))) + (data->do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0))) return 0; ALLOC_GROW(values->items, values->nr + 1, values->alloc); @@ -401,7 +401,7 @@ static int get_value(const struct config_location_options *opts, value_pattern = regex_; else if (regex_) { if (regex_[0] == '!') { - do_not_match = 1; + data.do_not_match = 1; regex_++; } From bfe45f83e7a7815cbaea04f380cd807c18c088ea Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:44 +0200 Subject: [PATCH 15/21] builtin/config: convert `value_pattern` to a local variable The `value_pattern` variable is used by the `format_config()` callback when `CONFIG_FLAGS_FIXED_VALUE` is used. It is only ever set up by its only caller, `collect_config()` and can thus easily be moved into the `collect_config_data` structure. Do so to remove our reliance on global state. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index bc80fd293a..aac5f7b976 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -125,7 +125,6 @@ struct config_display_options { static char *key; static regex_t *key_regexp; -static const char *value_pattern; static regex_t *regexp; static int use_key_regexp; static int do_all; @@ -327,6 +326,7 @@ static int format_config(const struct config_display_options *opts, struct collect_config_data { const struct config_display_options *display_opts; struct strbuf_list *values; + const char *value_pattern; int do_not_match; }; @@ -341,7 +341,7 @@ static int collect_config(const char *key_, const char *value_, return 0; if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0)) return 0; - if (fixed_value && strcmp(value_pattern, (value_?value_:""))) + if (fixed_value && strcmp(data->value_pattern, (value_?value_:""))) return 0; if (regexp != NULL && (data->do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0))) @@ -398,7 +398,7 @@ static int get_value(const struct config_location_options *opts, } if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE)) - value_pattern = regex_; + data.value_pattern = regex_; else if (regex_) { if (regex_[0] == '!') { data.do_not_match = 1; From 4ff8feb307432e20b2b79e7fc0fde3f2e282b5a6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:49 +0200 Subject: [PATCH 16/21] builtin/config: convert `regexp` to a local variable The `regexp` variable is used by the `format_config()` callback when `CONFIG_FLAGS_FIXED_VALUE` is not set. It is only ever set up by its only caller, `collect_config()` and can thus easily be moved into the `collect_config_data` structure. Do so to remove our reliance on global state. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index aac5f7b976..ae609c9b97 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -125,7 +125,6 @@ struct config_display_options { static char *key; static regex_t *key_regexp; -static regex_t *regexp; static int use_key_regexp; static int do_all; static int fixed_value; @@ -327,6 +326,7 @@ struct collect_config_data { const struct config_display_options *display_opts; struct strbuf_list *values; const char *value_pattern; + regex_t *regexp; int do_not_match; }; @@ -343,8 +343,8 @@ static int collect_config(const char *key_, const char *value_, return 0; if (fixed_value && strcmp(data->value_pattern, (value_?value_:""))) return 0; - if (regexp != NULL && - (data->do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0))) + if (data->regexp && + (data->do_not_match ^ !!regexec(data->regexp, (value_?value_:""), 0, NULL, 0))) return 0; ALLOC_GROW(values->items, values->nr + 1, values->alloc); @@ -405,10 +405,10 @@ static int get_value(const struct config_location_options *opts, regex_++; } - regexp = (regex_t*)xmalloc(sizeof(regex_t)); - if (regcomp(regexp, regex_, REG_EXTENDED)) { + data.regexp = (regex_t*)xmalloc(sizeof(regex_t)); + if (regcomp(data.regexp, regex_, REG_EXTENDED)) { error(_("invalid pattern: %s"), regex_); - FREE_AND_NULL(regexp); + FREE_AND_NULL(data.regexp); ret = CONFIG_INVALID_PATTERN; goto free_strings; } @@ -448,9 +448,9 @@ free_strings: regfree(key_regexp); free(key_regexp); } - if (regexp) { - regfree(regexp); - free(regexp); + if (data.regexp) { + regfree(data.regexp); + free(data.regexp); } return ret; From fdfaaa1b68f61eccd7423da558e9c69e3c7bb908 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:53 +0200 Subject: [PATCH 17/21] builtin/config: convert `key_regexp` to a local variable The `key_regexp` variable is used by the `format_config()` callback when `use_key_regexp` is set. It is only ever set up by its only caller, `collect_config()` and can thus easily be moved into the `collect_config_data` structure. Do so to remove our reliance on global state. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index ae609c9b97..08a11b7999 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -124,7 +124,6 @@ struct config_display_options { } static char *key; -static regex_t *key_regexp; static int use_key_regexp; static int do_all; static int fixed_value; @@ -327,6 +326,7 @@ struct collect_config_data { struct strbuf_list *values; const char *value_pattern; regex_t *regexp; + regex_t *key_regexp; int do_not_match; }; @@ -339,7 +339,7 @@ static int collect_config(const char *key_, const char *value_, if (!use_key_regexp && strcmp(key_, key)) return 0; - if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0)) + if (use_key_regexp && regexec(data->key_regexp, key_, 0, NULL, 0)) return 0; if (fixed_value && strcmp(data->value_pattern, (value_?value_:""))) return 0; @@ -383,10 +383,10 @@ static int get_value(const struct config_location_options *opts, for (tl = key; *tl && *tl != '.'; tl++) *tl = tolower(*tl); - key_regexp = (regex_t*)xmalloc(sizeof(regex_t)); - if (regcomp(key_regexp, key, REG_EXTENDED)) { + data.key_regexp = (regex_t*)xmalloc(sizeof(regex_t)); + if (regcomp(data.key_regexp, key, REG_EXTENDED)) { error(_("invalid key pattern: %s"), key_); - FREE_AND_NULL(key_regexp); + FREE_AND_NULL(data.key_regexp); ret = CONFIG_INVALID_PATTERN; goto free_strings; } @@ -444,9 +444,9 @@ static int get_value(const struct config_location_options *opts, free_strings: free(key); - if (key_regexp) { - regfree(key_regexp); - free(key_regexp); + if (data.key_regexp) { + regfree(data.key_regexp); + free(data.key_regexp); } if (data.regexp) { regfree(data.regexp); From 040b141df39a952af936abf73f4e2fa5fc9954b5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:42:58 +0200 Subject: [PATCH 18/21] builtin/config: convert `key` to a local variable The `key` variable is used by the `get_value()` function for two purposes: - It is used to store the result of `git_config_parse_key()`, which is then passed on to `collect_config()`. - It is used as a store to convert the provided key to an all-lowercase key when `use_key_regexp` is set. Neither of these cases warrant a global variable at all. In the former case we can pass the key via `struct collect_config_data`. And in the latter case we really only want to have it as a temporary local variable such that we can free associated memory. Refactor the code accordingly to reduce our reliance on global state. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 08a11b7999..5a03dbb452 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -123,7 +123,6 @@ struct config_display_options { .key_delim = ' ', \ } -static char *key; static int use_key_regexp; static int do_all; static int fixed_value; @@ -325,6 +324,7 @@ struct collect_config_data { const struct config_display_options *display_opts; struct strbuf_list *values; const char *value_pattern; + const char *key; regex_t *regexp; regex_t *key_regexp; int do_not_match; @@ -337,7 +337,7 @@ static int collect_config(const char *key_, const char *value_, struct strbuf_list *values = data->values; const struct key_value_info *kvi = ctx->kvi; - if (!use_key_regexp && strcmp(key_, key)) + if (!use_key_regexp && strcmp(key_, data->key)) return 0; if (use_key_regexp && regexec(data->key_regexp, key_, 0, NULL, 0)) return 0; @@ -364,6 +364,7 @@ static int get_value(const struct config_location_options *opts, .display_opts = display_opts, .values = &values, }; + char *key = NULL; int i; if (use_key_regexp) { @@ -395,6 +396,8 @@ static int get_value(const struct config_location_options *opts, ret = CONFIG_INVALID_KEY; goto free_strings; } + + data.key = key; } if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE)) From ab8bac8bb6d89f3b3eca2972ad468ca863c9f492 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:43:02 +0200 Subject: [PATCH 19/21] builtin/config: track "fixed value" option via flags only We track the "fixed value" option via two separate bits: once via the global variable `fixed_value`, and once via the CONFIG_FLAGS_FIXED_VALUE bit in `flags`. This is confusing and may easily lead to issues when one is not aware that this is tracked via two separate mechanisms. Refactor the code to use the flag exclusively. We already pass it to all the required callsites anyway, except for `collect_config()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 5a03dbb452..1f673ebee7 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -125,7 +125,6 @@ struct config_display_options { static int use_key_regexp; static int do_all; -static int fixed_value; #define TYPE_BOOL 1 #define TYPE_INT 2 @@ -328,6 +327,7 @@ struct collect_config_data { regex_t *regexp; regex_t *key_regexp; int do_not_match; + unsigned flags; }; static int collect_config(const char *key_, const char *value_, @@ -341,7 +341,8 @@ static int collect_config(const char *key_, const char *value_, return 0; if (use_key_regexp && regexec(data->key_regexp, key_, 0, NULL, 0)) return 0; - if (fixed_value && strcmp(data->value_pattern, (value_?value_:""))) + if ((data->flags & CONFIG_FLAGS_FIXED_VALUE) && + strcmp(data->value_pattern, (value_?value_:""))) return 0; if (data->regexp && (data->do_not_match ^ !!regexec(data->regexp, (value_?value_:""), 0, NULL, 0))) @@ -363,6 +364,7 @@ static int get_value(const struct config_location_options *opts, struct collect_config_data data = { .display_opts = display_opts, .values = &values, + .flags = flags, }; char *key = NULL; int i; @@ -1117,6 +1119,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) struct config_display_options display_opts = CONFIG_DISPLAY_OPTIONS_INIT; const char *comment_arg = NULL; int actions = 0; + unsigned flags = 0; struct option opts[] = { CONFIG_LOCATION_OPTIONS(location_opts), OPT_GROUP(N_("Action")), @@ -1139,13 +1142,12 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) OPT_STRING(0, "default", &display_opts.default_value, N_("value"), N_("with --get, use default value when missing entry")), OPT_STRING(0, "comment", &comment_arg, N_("value"), N_("human-readable comment string (# will be prepended as needed)")), - OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")), + OPT_BIT(0, "fixed-value", &flags, N_("use string equality when comparing values to value pattern"), CONFIG_FLAGS_FIXED_VALUE), OPT_BOOL(0, "includes", &location_opts.respect_includes_opt, N_("respect include directives on lookup")), OPT_END(), }; char *value = NULL, *comment = NULL; - int flags = 0; int ret = 0; struct key_value_info default_kvi = KVI_INIT; @@ -1195,7 +1197,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) } /* check usage of --fixed-value */ - if (fixed_value) { + if (flags & CONFIG_FLAGS_FIXED_VALUE) { int allowed_usage = 0; switch (actions) { @@ -1226,8 +1228,6 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) error(_("--fixed-value only applies with 'value-pattern'")); exit(129); } - - flags |= CONFIG_FLAGS_FIXED_VALUE; } comment = git_config_prepare_comment_string(comment_arg); From 35a7cfda56f85c8fccfbf7e8b5bae9a356dd0d45 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:43:07 +0200 Subject: [PATCH 20/21] builtin/config: convert flags to a local variable Both the `do_all` and `use_key_regexp` bits essentially act like flags to `get_value()`. Let's convert them to actual flags so that we can get rid of the last two remaining global variables that track options. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 48 +++++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 1f673ebee7..3b588df8cf 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -123,9 +123,6 @@ struct config_display_options { .key_delim = ' ', \ } -static int use_key_regexp; -static int do_all; - #define TYPE_BOOL 1 #define TYPE_INT 2 #define TYPE_BOOL_OR_INT 3 @@ -319,6 +316,9 @@ static int format_config(const struct config_display_options *opts, return 0; } +#define GET_VALUE_ALL (1 << 0) +#define GET_VALUE_KEY_REGEXP (1 << 1) + struct collect_config_data { const struct config_display_options *display_opts; struct strbuf_list *values; @@ -327,6 +327,7 @@ struct collect_config_data { regex_t *regexp; regex_t *key_regexp; int do_not_match; + unsigned get_value_flags; unsigned flags; }; @@ -337,9 +338,11 @@ static int collect_config(const char *key_, const char *value_, struct strbuf_list *values = data->values; const struct key_value_info *kvi = ctx->kvi; - if (!use_key_regexp && strcmp(key_, data->key)) + if (!(data->get_value_flags & GET_VALUE_KEY_REGEXP) && + strcmp(key_, data->key)) return 0; - if (use_key_regexp && regexec(data->key_regexp, key_, 0, NULL, 0)) + if ((data->get_value_flags & GET_VALUE_KEY_REGEXP) && + regexec(data->key_regexp, key_, 0, NULL, 0)) return 0; if ((data->flags & CONFIG_FLAGS_FIXED_VALUE) && strcmp(data->value_pattern, (value_?value_:""))) @@ -357,19 +360,21 @@ static int collect_config(const char *key_, const char *value_, static int get_value(const struct config_location_options *opts, const struct config_display_options *display_opts, - const char *key_, const char *regex_, unsigned flags) + const char *key_, const char *regex_, + unsigned get_value_flags, unsigned flags) { int ret = CONFIG_GENERIC_ERROR; struct strbuf_list values = {NULL}; struct collect_config_data data = { .display_opts = display_opts, .values = &values, + .get_value_flags = get_value_flags, .flags = flags, }; char *key = NULL; int i; - if (use_key_regexp) { + if (get_value_flags & GET_VALUE_KEY_REGEXP) { char *tl; /* @@ -441,7 +446,7 @@ static int get_value(const struct config_location_options *opts, for (i = 0; i < values.nr; i++) { struct strbuf *buf = values.items + i; - if (do_all || i == values.nr - 1) + if ((get_value_flags & GET_VALUE_ALL) || i == values.nr - 1) fwrite(buf->buf, 1, buf->len, stdout); strbuf_release(buf); } @@ -848,11 +853,12 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) struct config_display_options display_opts = CONFIG_DISPLAY_OPTIONS_INIT; const char *value_pattern = NULL, *url = NULL; int flags = 0; + unsigned get_value_flags = 0; struct option opts[] = { CONFIG_LOCATION_OPTIONS(location_opts), OPT_GROUP(N_("Filter options")), - OPT_BOOL(0, "all", &do_all, N_("return all values for multi-valued config options")), - OPT_BOOL(0, "regexp", &use_key_regexp, N_("interpret the name as a regular expression")), + OPT_BIT(0, "all", &get_value_flags, N_("return all values for multi-valued config options"), GET_VALUE_ALL), + OPT_BIT(0, "regexp", &get_value_flags, N_("interpret the name as a regular expression"), GET_VALUE_KEY_REGEXP), OPT_STRING(0, "value", &value_pattern, N_("pattern"), N_("show config with values matching the pattern")), OPT_BIT(0, "fixed-value", &flags, N_("use string equality when comparing values to value pattern"), CONFIG_FLAGS_FIXED_VALUE), OPT_STRING(0, "url", &url, N_("URL"), N_("show config matching the given URL")), @@ -872,9 +878,12 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern) die(_("--fixed-value only applies with 'value-pattern'")); - if (display_opts.default_value && (do_all || url)) + if (display_opts.default_value && + ((get_value_flags & GET_VALUE_ALL) || url)) die(_("--default= cannot be used with --all or --url=")); - if (url && (do_all || use_key_regexp || value_pattern)) + if (url && ((get_value_flags & GET_VALUE_ALL) || + (get_value_flags & GET_VALUE_KEY_REGEXP) || + value_pattern)) die(_("--url= cannot be used with --all, --regexp or --value")); location_options_init(&location_opts, prefix); @@ -885,7 +894,8 @@ static int cmd_config_get(int argc, const char **argv, const char *prefix) if (url) ret = get_urlmatch(&location_opts, &display_opts, argv[0], url); else - ret = get_value(&location_opts, &display_opts, argv[0], value_pattern, flags); + ret = get_value(&location_opts, &display_opts, argv[0], value_pattern, + get_value_flags, flags); location_options_release(&location_opts); return ret; @@ -1290,19 +1300,19 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix) } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); - ret = get_value(&location_opts, &display_opts, argv[0], argv[1], flags); + ret = get_value(&location_opts, &display_opts, argv[0], argv[1], + 0, flags); } else if (actions == ACTION_GET_ALL) { - do_all = 1; check_argc(argc, 1, 2); - ret = get_value(&location_opts, &display_opts, argv[0], argv[1], flags); + ret = get_value(&location_opts, &display_opts, argv[0], argv[1], + GET_VALUE_ALL, flags); } else if (actions == ACTION_GET_REGEXP) { display_opts.show_keys = 1; - use_key_regexp = 1; - do_all = 1; check_argc(argc, 1, 2); - ret = get_value(&location_opts, &display_opts, argv[0], argv[1], flags); + ret = get_value(&location_opts, &display_opts, argv[0], argv[1], + GET_VALUE_ALL|GET_VALUE_KEY_REGEXP, flags); } else if (actions == ACTION_GET_URLMATCH) { check_argc(argc, 2, 2); From 9c62534377f988f36b7565a83a276d5b9099951c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:43:12 +0200 Subject: [PATCH 21/21] builtin/config: pass data between callbacks via local variables We use several global variables to pass data between callers and callbacks in `get_color()` and `get_colorbool()`. Convert those to use callback data structures instead. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/config.c | 90 ++++++++++++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 38 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 3b588df8cf..c38264c999 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -518,21 +518,24 @@ static char *normalize_value(const char *key, const char *value, BUG("cannot normalize type %d", type); } -static int get_color_found; -static const char *get_color_slot; -static const char *get_colorbool_slot; -static char parsed_color[COLOR_MAXLEN]; +struct get_color_config_data { + int get_color_found; + const char *get_color_slot; + char parsed_color[COLOR_MAXLEN]; +}; static int git_get_color_config(const char *var, const char *value, const struct config_context *ctx UNUSED, - void *cb UNUSED) + void *cb) { - if (!strcmp(var, get_color_slot)) { + struct get_color_config_data *data = cb; + + if (!strcmp(var, data->get_color_slot)) { if (!value) config_error_nonbool(var); - if (color_parse(value, parsed_color) < 0) + if (color_parse(value, data->parsed_color) < 0) return -1; - get_color_found = 1; + data->get_color_found = 1; } return 0; } @@ -540,66 +543,77 @@ static int git_get_color_config(const char *var, const char *value, static void get_color(const struct config_location_options *opts, const char *var, const char *def_color) { - get_color_slot = var; - get_color_found = 0; - parsed_color[0] = '\0'; - config_with_options(git_get_color_config, NULL, + struct get_color_config_data data = { + .get_color_slot = var, + .parsed_color[0] = '\0', + }; + + config_with_options(git_get_color_config, &data, &opts->source, the_repository, &opts->options); - if (!get_color_found && def_color) { - if (color_parse(def_color, parsed_color) < 0) + if (!data.get_color_found && def_color) { + if (color_parse(def_color, data.parsed_color) < 0) die(_("unable to parse default color value")); } - fputs(parsed_color, stdout); + fputs(data.parsed_color, stdout); } -static int get_colorbool_found; -static int get_diff_color_found; -static int get_color_ui_found; +struct get_colorbool_config_data { + int get_colorbool_found; + int get_diff_color_found; + int get_color_ui_found; + const char *get_colorbool_slot; +}; + static int git_get_colorbool_config(const char *var, const char *value, const struct config_context *ctx UNUSED, - void *data UNUSED) + void *cb) { - if (!strcmp(var, get_colorbool_slot)) - get_colorbool_found = git_config_colorbool(var, value); + struct get_colorbool_config_data *data = cb; + + if (!strcmp(var, data->get_colorbool_slot)) + data->get_colorbool_found = git_config_colorbool(var, value); else if (!strcmp(var, "diff.color")) - get_diff_color_found = git_config_colorbool(var, value); + data->get_diff_color_found = git_config_colorbool(var, value); else if (!strcmp(var, "color.ui")) - get_color_ui_found = git_config_colorbool(var, value); + data->get_color_ui_found = git_config_colorbool(var, value); return 0; } static int get_colorbool(const struct config_location_options *opts, const char *var, int print) { - get_colorbool_slot = var; - get_colorbool_found = -1; - get_diff_color_found = -1; - get_color_ui_found = -1; - config_with_options(git_get_colorbool_config, NULL, + struct get_colorbool_config_data data = { + .get_colorbool_slot = var, + .get_colorbool_found = -1, + .get_diff_color_found = -1, + .get_color_ui_found = -1, + }; + + config_with_options(git_get_colorbool_config, &data, &opts->source, the_repository, &opts->options); - if (get_colorbool_found < 0) { - if (!strcmp(get_colorbool_slot, "color.diff")) - get_colorbool_found = get_diff_color_found; - if (get_colorbool_found < 0) - get_colorbool_found = get_color_ui_found; + if (data.get_colorbool_found < 0) { + if (!strcmp(data.get_colorbool_slot, "color.diff")) + data.get_colorbool_found = data.get_diff_color_found; + if (data.get_colorbool_found < 0) + data.get_colorbool_found = data.get_color_ui_found; } - if (get_colorbool_found < 0) + if (data.get_colorbool_found < 0) /* default value if none found in config */ - get_colorbool_found = GIT_COLOR_AUTO; + data.get_colorbool_found = GIT_COLOR_AUTO; - get_colorbool_found = want_color(get_colorbool_found); + data.get_colorbool_found = want_color(data.get_colorbool_found); if (print) { - printf("%s\n", get_colorbool_found ? "true" : "false"); + printf("%s\n", data.get_colorbool_found ? "true" : "false"); return 0; } else - return get_colorbool_found ? 0 : 1; + return data.get_colorbool_found ? 0 : 1; } static void check_write(const struct git_config_source *source)