Git Mailing List Archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/14] [RFC] config: remove global state from config iteration
@ 2023-04-21 19:13 Glen Choo via GitGitGadget
  2023-04-21 19:13 ` [PATCH 01/14] config.c: introduce kvi_fn(), use it for configsets Glen Choo via GitGitGadget
                   ` (14 more replies)
  0 siblings, 15 replies; 115+ messages in thread
From: Glen Choo via GitGitGadget @ 2023-04-21 19:13 UTC (permalink / raw)
  To: git
  Cc: Jonathan Tan, Ævar Arnfjörð Bjarmason,
	Emily Shaffer, Glen Choo

= 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

^ permalink raw reply	[flat|nested] 115+ messages in thread

end of thread, other threads:[~2023-07-11 18:41 UTC | newest]

Thread overview: 115+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-21 19:13 [PATCH 00/14] [RFC] config: remove global state from config iteration Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 01/14] config.c: introduce kvi_fn(), use it for configsets Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 02/14] config.c: use kvi for CLI config Glen Choo via GitGitGadget
2023-05-01 11:06   ` Ævar Arnfjörð Bjarmason
2023-04-21 19:13 ` [PATCH 03/14] config: use kvi for config files Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 04/14] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 05/14] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 06/14] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 07/14] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 08/14] (RFC-only) config: add kvi arg to config_fn_t Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 09/14] (RFC-only) config: apply cocci to config_fn_t implementations Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 10/14] (RFC-only) config: finish config_fn_t refactor Glen Choo via GitGitGadget
2023-05-01 11:19   ` Ævar Arnfjörð Bjarmason
2023-05-05 21:07     ` Jonathan Tan
2023-05-09 22:46       ` Glen Choo
2023-05-11 16:21         ` Jonathan Tan
2023-05-08 21:00     ` Glen Choo
2023-04-21 19:13 ` [PATCH 11/14] config: remove current_config_(line|name|origin_type) Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 12/14] config: remove current_config_scope() Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 13/14] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 14/14] config: remove config_reader from configset_add_value Glen Choo via GitGitGadget
2023-05-30 18:41 ` [PATCH v2 00/14] [RFC] config: remove global state from config iteration Glen Choo via GitGitGadget
2023-05-30 18:41   ` [PATCH v2 01/14] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-05-30 18:42   ` [PATCH v2 02/14] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-05-30 18:42   ` [PATCH v2 03/14] (RFC-only) config: add kvi arg to config_fn_t Glen Choo via GitGitGadget
2023-06-01  9:50     ` Phillip Wood
2023-06-01 16:22       ` Glen Choo
2023-06-02  9:54         ` Phillip Wood
2023-06-02 16:46           ` Glen Choo
2023-06-05  9:38             ` Phillip Wood
2023-06-09 23:19               ` Glen Choo
2023-05-30 18:42   ` [PATCH v2 04/14] (RFC-only) config: apply cocci to config_fn_t implementations Glen Choo via GitGitGadget
2023-05-30 18:42   ` [PATCH v2 05/14] (RFC-only) config: finish config_fn_t refactor Glen Choo via GitGitGadget
2023-06-01 22:17     ` Jonathan Tan
2023-05-30 18:42   ` [PATCH v2 06/14] config.c: pass kvi in configsets Glen Choo via GitGitGadget
2023-06-01 22:21     ` Jonathan Tan
2023-05-30 18:42   ` [PATCH v2 07/14] config: provide kvi with config files Glen Choo via GitGitGadget
2023-06-01 22:41     ` Jonathan Tan
2023-06-01 23:54     ` Jonathan Tan
2023-05-30 18:42   ` [PATCH v2 08/14] builtin/config.c: test misuse of format_config() Glen Choo via GitGitGadget
2023-05-30 18:42   ` [PATCH v2 09/14] config.c: provide kvi with CLI config Glen Choo via GitGitGadget
2023-06-01 23:35     ` Jonathan Tan
2023-06-02 17:26       ` Glen Choo
2023-05-30 18:42   ` [PATCH v2 10/14] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-01 23:38     ` Jonathan Tan
2023-05-30 18:42   ` [PATCH v2 11/14] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-01 23:48     ` Jonathan Tan
2023-06-02 17:23       ` Glen Choo
2023-05-30 18:42   ` [PATCH v2 12/14] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-05-30 18:42   ` [PATCH v2 13/14] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-02  0:06     ` Jonathan Tan
2023-05-30 18:42   ` [PATCH v2 14/14] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-02  0:08     ` Jonathan Tan
2023-06-02 17:20       ` Glen Choo
2023-06-20 19:43   ` [PATCH v3 00/12] config: remove global state from config iteration Glen Choo via GitGitGadget
2023-06-20 19:43     ` [PATCH v3 01/12] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-06-20 21:01       ` Junio C Hamano
2023-06-20 19:43     ` [PATCH v3 02/12] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-06-20 21:02       ` Junio C Hamano
2023-06-20 19:43     ` [PATCH v3 03/12] config: add ctx arg to config_fn_t Glen Choo via GitGitGadget
2023-06-20 19:43     ` [PATCH v3 04/12] config.c: pass ctx in configsets Glen Choo via GitGitGadget
2023-06-20 21:19       ` Junio C Hamano
2023-06-20 19:43     ` [PATCH v3 05/12] config: pass ctx with config files Glen Choo via GitGitGadget
2023-06-20 19:43     ` [PATCH v3 06/12] builtin/config.c: test misuse of format_config() Glen Choo via GitGitGadget
2023-06-20 21:35       ` Junio C Hamano
2023-06-20 23:06         ` Glen Choo
2023-06-23 20:32       ` Jonathan Tan
2023-06-24  1:31         ` Jeff King
2023-06-28 17:28         ` Glen Choo
2023-06-20 19:43     ` [PATCH v3 07/12] config.c: pass ctx with CLI config Glen Choo via GitGitGadget
2023-06-23 20:35       ` Jonathan Tan
2023-06-23 21:41         ` Glen Choo
2023-06-20 19:43     ` [PATCH v3 08/12] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-23 20:40       ` Jonathan Tan
2023-06-20 19:43     ` [PATCH v3 09/12] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-20 19:43     ` [PATCH v3 10/12] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-06-23 20:57       ` Jonathan Tan
2023-06-23 21:33         ` Junio C Hamano
2023-06-20 19:43     ` [PATCH v3 11/12] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-20 19:43     ` [PATCH v3 12/12] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-20 21:46       ` Junio C Hamano
2023-06-21 21:46     ` [PATCH v3 00/12] config: remove global state from config iteration Junio C Hamano
2023-06-21 23:06       ` Glen Choo
2023-06-23 21:02         ` Jonathan Tan
2023-06-23 21:33           ` Junio C Hamano
2023-06-23 21:45           ` Glen Choo
2023-06-26 18:11     ` [PATCH v4 " Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 01/12] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 02/12] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 03/12] config: add ctx arg to config_fn_t Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 04/12] config.c: pass ctx in configsets Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 05/12] config: pass ctx with config files Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 06/12] builtin/config.c: test misuse of format_config() Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 07/12] config.c: pass ctx with CLI config Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 08/12] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 09/12] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 10/12] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 11/12] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-26 18:11       ` [PATCH v4 12/12] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-26 20:45       ` [PATCH v4 00/12] config: remove global state from config iteration Junio C Hamano
2023-06-28 19:26       ` [PATCH v5 00/11] " Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 01/11] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 02/11] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 03/11] config: add ctx arg to config_fn_t Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 04/11] config.c: pass ctx in configsets Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 05/11] config: pass ctx with config files Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 06/11] config.c: pass ctx with CLI config Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 07/11] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 08/11] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 09/11] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 10/11] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-28 19:26         ` [PATCH v5 11/11] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-28 22:23         ` [PATCH v5 00/11] config: remove global state from config iteration Jonathan Tan
2023-06-28 22:47           ` Junio C Hamano
2023-07-11 18:41         ` Phillip Wood

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).