| Message ID | cover.1715339393.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | builtin/config: remove global state | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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. Long overdue. It is very nice to see this happen finally. Even though I somehow feel that the clean-up should have done before the "subcommand" series, it may probably not worth the churn to flip the order of the series around. Will take a look.
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 >