Re: [PATCH v2 00/21] builtin/config: remove global state
On Mon, May 13, 2024 at 3:22 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 48 quoted lines
>
> Hi,
>
> this is the second version of my patch series that removes global state
> from "builtin/config.c". Changes compared to v1:
>
> - Reinstated a comment in patch 5.
>
> - Fixed a memory leak in patch 9.
>
> - A couple of commit message fixes.
>
> The series continues to build on top of ps/config-subcommands.
>
> Thanks!
>
> Patrick
>
> Patrick Steinhardt (21):
> builtin/config: stop printing full usage on misuse
> builtin/config: move legacy mode into its own function
> builtin/config: move subcommand options into `cmd_config()`
> builtin/config: move legacy options into `cmd_config()`
> builtin/config: move actions into `cmd_config_actions()`
> builtin/config: check for writeability after source is set up
> config: make the config source const
> builtin/config: refactor functions to have common exit paths
> builtin/config: move location options into local variables
> builtin/config: move display options into local variables
> builtin/config: move type options into display options
> builtin/config: move default value into display options
> builtin/config: move `respect_includes_opt` into location options
> builtin/config: convert `do_not_match` to a local variable
> builtin/config: convert `value_pattern` to a local variable
> builtin/config: convert `regexp` to a local variable
> builtin/config: convert `key_regexp` to a local variable
> builtin/config: convert `key` to a local variable
> builtin/config: track "fixed value" option via flags only
> builtin/config: convert flags to a local variable
> builtin/config: pass data between callbacks via local variables
>
> builtin/config.c | 968 ++++++++++++++++++++++++++--------------------
> config.c | 4 +-
> config.h | 2 +-
> t/t1300-config.sh | 9 +-
> 4 files changed, 550 insertions(+), 433 deletions(-)
>
> Range-diff against v1:
Show 84 quoted lines
> 1: 0ba7628126 = 1: 0ba7628126 builtin/config: stop printing full usage on misuse
> 2: 663e1f74f8 = 2: 663e1f74f8 builtin/config: move legacy mode into its own function
> 3: 1239c151d0 = 3: 1239c151d0 builtin/config: move subcommand options into `cmd_config()`
> 4: 82964510c5 = 4: 82964510c5 builtin/config: move legacy options into `cmd_config()`
> 5: 2e308393ed ! 5: 0a6ecae2cc builtin/config: move actions into `cmd_config_actions()`
> @@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, con
> 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);
>
> 6: edfd7caa39 = 6: 7ab99a27c1 builtin/config: check for writeability after source is set up
> 7: bfba68aa1e = 7: 1460d3a36c config: make the config source const
> 8: 6bff0410e9 = 8: 018ed0226b builtin/config: refactor functions to have common exit paths
> 9: a96c122280 ! 9: b5d43b6f85 builtin/config: move location options into local variables
> @@ builtin/config.c: static char *default_user_config(void)
> - given_config_source.file = git_global_config();
> - if (!given_config_source.file)
> + if (opts->use_global_config) {
> -+ opts->source.file = xstrdup_or_null(git_global_config());
> ++ opts->source.file = git_global_config();
> + if (!opts->source.file)
> /*
> * It is unknown if HOME/.gitconfig exists, so
> 10: 06c1e08fc4 = 10: d66e14af30 builtin/config: move display options into local variables
> 11: 9610897662 = 11: 63436c3416 builtin/config: move type options into display options
> 12: eb79462861 = 12: 106b8ac8a2 builtin/config: move default value into display options
> 13: b9ebfbd667 = 13: 8a6b555b58 builtin/config: move `respect_includes_opt` into location options
> 14: 2b40b784fe ! 14: 0dd22bf51a builtin/config: convert `do_not_match` to a local variable
> @@ Commit message
> 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 whteher or not the passed regular expression is negated. It
> + 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.
>
> 15: 71d1b7a51b = 15: b656951f0c builtin/config: convert `value_pattern` to a local variable
> 16: c3b340f119 = 16: b56a07bda0 builtin/config: convert `regexp` to a local variable
> 17: 835cc0acfb = 17: 323cb05120 builtin/config: convert `key_regexp` to a local variable
> 18: 2aee7ec5d8 ! 18: e972e63be8 builtin/config: convert `key` to a local variable
> @@ Commit message
> 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 si
> + - 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.
>
> - Both of these cases don't 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.
> + 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.
>
> 19: 625216a774 ! 19: d83c3d085e builtin/config: track "fixed value" option via flags only
> @@ Commit message
> 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 require callsites anyway, except for `collect_config()`.
> + the required callsites anyway, except for `collect_config()`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> 20: 05254d512b = 20: 294bcd96a4 builtin/config: convert flags to a local variable
> 21: 3a5f059789 = 21: 0496b958e2 builtin/config: pass data between callbacks via local variables
> --
> 2.45.GIT
>Kyle Lippincott <spectral@google.com>