[PATCH 00/14] [RFC] config: remove global state from config iteration
- From
Glen Choo via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Apr 21, 2023, 19:13 UTC
- Message-ID
- <pull.1497.git.git.1682104398.gitgitgadget@gmail.com>
= Description
This series removes all global state from config iteration, i.e. parsing config and iterating configsets, by passing config metadata as a "struct key_value_info" arg to the "config_fn_t" callbacks. This allows us to get rid of:
* "the_reader" (formerly "cf", "current_parsing_scope",
"current_config_kvi"), and the config.c machinery that maintained it.
This only needed to be global because it was read by...
* The "current_*" functions that read metadata about the 'current' config
value ("current_config_scope", "current_config_name", etc), which are
replaced by reading values from the "struct key_value_info" passed to the
callbacks.As a result:
* Config iterating code can be moved to into its own library with few modifications. C.f. libification efforts [1]. * The config iterating code can be safely called in parallel. * We expose and fix instances where the "current_*" functions were being called outside of config callbacks.
We've had this idea of doing this "config_fn_t" refactor for a long time [2], but we've never attempted it because we wanted to avoid churn. After attempting it, though, I'm quite convinced that this is the right way forward for config, since the lack of global state makes things much easier to reason about. The churn is also quite manageable:
* The vast majority of changes can be handled with cocci. * The few cases that aren't covered by cocci have obvious fixes. * The change is simple enough for in-flight topics to perform and conflicts will be caught at build time anyway. * Anecdotally, we don't change config functions often anyway. This merges cleanly with 'seen' and the result passes CI.
= Patch overview
I've arranged the patches in a way that makes them readable, but is unsuitable for merging, hence the RFC tag. The "Post-RFC cleanups" section describes all the changes I'll make prior to submitting this as non-RFC.
1-5/14 converts the config.c machinery to set and make use of "config_reader.config_kvi" instead of "config_reader.source" and "config_reader.scope", which makes it easier to pass "struct key_value_info" to config callbacks. To minimize churn, I opted to 'de-plumb' "struct config_reader" here instead of plumbing it and removing it later.
6-10/14 actually changes "config_fn_t"'s signature. The earlier patches are minor refactors that make the cocci application + adjustment easier. Post RFC, cocci application + adjustment will be a single patch, but for ease of reading I've kept them separate.
11-14/14 teach the config callbacks to use the "kvi" parameter, replace the now-obsolete "current_*" functions, and remove the now-obsolete "struct config_reader". This requires plumbing "kvi" through some more config.c machinery and fixing a sneaky config.c-like code path in trace2/.
= Post-RFC cleanups
* To make future refactors easier, I'm strongly considering replacing the "struct key_value_info" arg with something more extensible so that we can modify that without changing 100+ function signatures. An alternative would be to replace all of the args to config_fn_t with a single struct, but that would very significantly increase churn because we'd need to touch all the config_fn_t function bodies too.
* Rearrange the cocci patches to avoid breaking CI in the middle of the series.
* Fix style issues (e.g. spacing around "kvi", line wrapping, "{ 0 }"
instead of dedicated initializer)= Alternatives considered
Ævar suggested in [3] that we might be able to do the refactor incrementally by having both the old "config_fn_t" and the new "config_fn_t with kvi", which lets us convert some of config iterating (e.g. configsets) without touching the others (e.g. config parsing). I experimented with that for a bit, and it turned out that doing it all at once is actually less work because we don't have to worry about the case where the same "config_fn_t" is used in both git_config() and git_config_from_file().
Jonathan Tan suggested in [4] that to reduce churn, we might be able to convert many of the config_fn_t-s to the config set API before attempting this refactor. But (hopefully), these patches show that the churn is manageable even without this preparatory step.
[1] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com/ [2] https://lore.kernel.org/git/CAPc5daV6bdUKS-ExHmpT4Ppy2S832NXoyPw7aOLP7fG=WrBPgg@mail.gmail.com/ [3] https://lore.kernel.org/git/RFC-patch-5.5-2b80d293c83-20230317T042408Z-avarab@gmail.com [4] https://lore.kernel.org/git/20230306195756.3399115-1-jonathantanmy@google.com/
Glen Choo (14): config.c: introduce kvi_fn(), use it for configsets config.c: use kvi for CLI config config: use kvi for config files config: add kvi.path, use it to evaluate includes config: pass source to config_parser_event_fn_t config: inline git_color_default_config urlmatch.h: use config_fn_t type (RFC-only) config: add kvi arg to config_fn_t (RFC-only) config: apply cocci to config_fn_t implementations (RFC-only) config: finish config_fn_t refactor config: remove current_config_(line|name|origin_type) config: remove current_config_scope() config: pass kvi to die_bad_number() config: remove config_reader from configset_add_value
alias.c | 3 +- archive-tar.c | 5 +- archive-zip.c | 1 + builtin/add.c | 5 +- builtin/blame.c | 5 +- builtin/branch.c | 8 +- builtin/cat-file.c | 5 +- builtin/checkout.c | 7 +- builtin/clean.c | 9 +- builtin/clone.c | 10 +- builtin/column.c | 3 +- builtin/commit-graph.c | 3 +- builtin/commit.c | 20 +- builtin/config.c | 59 +- builtin/difftool.c | 5 +- builtin/fetch.c | 12 +- builtin/fsmonitor--daemon.c | 11 +- builtin/grep.c | 12 +- builtin/help.c | 5 +- builtin/index-pack.c | 9 +- builtin/log.c | 12 +- builtin/merge.c | 7 +- builtin/multi-pack-index.c | 1 + builtin/pack-objects.c | 19 +- builtin/patch-id.c | 5 +- builtin/pull.c | 5 +- builtin/push.c | 5 +- builtin/read-tree.c | 5 +- builtin/rebase.c | 5 +- builtin/receive-pack.c | 15 +- builtin/reflog.c | 7 +- builtin/remote.c | 12 +- builtin/repack.c | 5 +- builtin/reset.c | 5 +- builtin/send-pack.c | 5 +- builtin/show-branch.c | 8 +- builtin/stash.c | 5 +- builtin/submodule--helper.c | 3 +- builtin/tag.c | 9 +- builtin/var.c | 5 +- builtin/worktree.c | 5 +- bundle-uri.c | 9 +- color.c | 8 - color.h | 6 +- compat/mingw.c | 3 +- compat/mingw.h | 4 +- config.c | 540 +++++++----------- config.h | 56 +- connect.c | 4 +- .../coccinelle/config_fn_kvi.pending.cocci | 146 +++++ contrib/coccinelle/git_config_number.cocci | 27 + convert.c | 4 +- credential.c | 1 + delta-islands.c | 3 +- diff.c | 19 +- diff.h | 7 +- fetch-pack.c | 5 +- fmt-merge-msg.c | 7 +- fmt-merge-msg.h | 3 +- fsck.c | 11 +- fsck.h | 4 +- git-compat-util.h | 2 + gpg-interface.c | 6 +- grep.c | 7 +- grep.h | 4 +- help.c | 10 +- http.c | 15 +- ident.c | 3 +- ident.h | 4 +- imap-send.c | 7 +- ll-merge.c | 1 + ls-refs.c | 2 +- mailinfo.c | 5 +- notes-utils.c | 3 +- notes.c | 3 +- pager.c | 5 +- pretty.c | 1 + promisor-remote.c | 4 +- remote.c | 7 +- revision.c | 3 +- scalar.c | 3 +- sequencer.c | 28 +- setup.c | 17 +- submodule-config.c | 31 +- submodule-config.h | 3 +- t/helper/test-config.c | 21 +- t/helper/test-userdiff.c | 4 +- trace2.c | 4 +- trace2.h | 3 +- trace2/tr2_cfg.c | 12 +- trace2/tr2_sysenv.c | 3 +- trace2/tr2_tgt.h | 4 +- trace2/tr2_tgt_event.c | 4 +- trace2/tr2_tgt_normal.c | 4 +- trace2/tr2_tgt_perf.c | 4 +- trailer.c | 2 + upload-pack.c | 18 +- urlmatch.c | 7 +- urlmatch.h | 8 +- worktree.c | 2 +- xdiff-interface.c | 5 +- xdiff-interface.h | 5 +- 102 files changed, 855 insertions(+), 641 deletions(-) create mode 100644 contrib/coccinelle/config_fn_kvi.pending.cocci create mode 100644 contrib/coccinelle/git_config_number.cocci
base-commit: 9857273be005833c71e2d16ba48e193113e12276 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1497%2Fchooglen%2Fconfig%2Fno-global-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1497/chooglen/config/no-global-v1 Pull-Request: https://github.com/git/git/pull/1497
-- gitgitgadget