Re: [PATCH v2 00/21] builtin/config: remove global state

[Kyle Lippincott <spectral@google.com>]

On Mon, May 13, 2024 at 3:22 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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:

Looks good to me

>  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
>