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

[Kyle Lippincott <spectral@google.com>]

On Fri, May 10, 2024 at 4:26 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Hi,
>
> we have quite a lot of global state in git-config(1). For one, this
> global state is used to track options passed by the user. And second,
> there is a lot of global state that is really only used to pass data
> between caller and callbacks.
>
> This global state makes it quite hard to understand interactions at
> times. This only became harder now with the introduction of subcommands,
> where a whole lot less of the global state is even relevant in the first
> place. As an example, this global state has made it quite easy for a bug
> to sneak into the new subcommands where `check_write()` was called
> before the global data it access is initialized (see patch 6).
>
> This patch series refactors the code such that we have no more global
> state in "builtin/config.c". It's rather long, but most of the patches
> are really quite trivial and only move code around. So overall, I think
> it shouldn't be too bad to review this. But please, let me know if you
> disagree and I'll happily split this series into two.
>
> The patch series depends on 7b91d310ce (builtin/config: display
> subcommand help, 2024-05-06).
>
> 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  | 964 +++++++++++++++++++++++++---------------------
>  config.c          |   4 +-
>  config.h          |   2 +-
>  t/t1300-config.sh |   9 +-
>  4 files changed, 546 insertions(+), 433 deletions(-)

Left a couple small comments, but looks good to me. Thanks!

>
> --
> 2.45.GIT
>