Note to Junio: 8/8 (which renames "cs" -> "set") conflicts with
ab/config-multi-and-nonbool. The resolution is to rename "cs" -> "set" in
the conflicted hunks. This leaves a few "struct config_set cs" in
unconflicted hunks, but since 8/8 might be controversial, I don't think it's
worth your time to fix these. I'll reroll this and base it on top of
ab/config-multi-and-nonbool instead (since that is pretty stable and close
to being merged).

Thanks for the thoughtful review on v1, all. I'm sending this as a regular
series, not an RFC.

After reflecting on Ævar's responses on v1, I'm fairly convinced that
"struct config_reader" shouldn't exist in the long term. I've written my
thoughts on a good long term direction in the "Leftover bits" section. Based
on that, I've also updated my WIP libification patches [1] to remove "struct
config_reader" from the library interface, and think it looks a lot better
as a result.

= Changes in v2

 * To reduce churn, don't rename "struct config_source cf" to "cs" early in
   the series. Instead, rename the global "cf" to "cf_global", and leave the
   existing "cf"s untouched.

   Introduce 8/8 to get rid of the confusing acronym "struct config_source
   cf", but I don't mind ejecting it if it's too much churn.

 * Adjust 5/8 so to pass "struct config_reader" through args instead of
   "*data". v1 made the mistake of thinking "*data" was being passed to a
   callback, but it wasn't.

 * Add a 7/8 to fix a bug in die_bad_number(). I included this because it
   overlaps a little bit with the refactor here, but I don't mind ejecting
   this either.

 * Assorted BUG() message clarifications.

As a result of moving the rename, the range-diff is quite noisy. The diff
between the final commits is might be helpful instead [2] (I'll also send a
diff to the ML).

= Description

This series prepares for config.[ch] to be libified as as part of the
libification effort that Emily described in [3]. One of the first goals is
to read config from a file, but the trouble with how config.c is written
today is that all reading operations rely on global state, so before turning
that into a library, we'd want to make that state non-global.

This series doesn't remove all of the global state, but it gets us closer to
that goal by extracting the global config reading state into "struct
config_reader" and plumbing it through the config reading machinery. This
makes it possible to reuse the config machinery without global state, and to
enforce some constraints on "struct config_reader", which makes it more
predictable and easier to remove in the long run.

This process is very similar to how we've plumbed "struct repository" and
other 'context objects' in the past, except:

 * The global state (named "the_reader") for the git process lives in a
   config.c static variable, and not on "the_repository". See 3/6 for the
   rationale.

 * I've stopped short of adding "struct config_reader" to config.h public
   functions, since that would affect non-config.c callers.

Additionally, I've included a bugfix for die_bad_number() that became clear
as I did this refactor.

= Leftover bits

We still need a global "the_reader" because config callbacks are reading
auxiliary information about the config (e.g. line number, file name) via
global functions (e.g. current_config_line(), current_config_name()). This
is either because the callback uses this info directly (like
builtin/config.c printing the filename and scope of the value) or for error
reporting (like git_parse_int() reporting the filename of the value it
failed to parse).

If we had a way to plumb the state from "struct config_reader" to the config
callback functions, we could initialize "struct config_reader" in the config
machinery whenever we read config (instead of asking the caller to
initialize "struct config_reader" themselves), and config reading could
become a thread-safe operation. There isn't an obvious way to plumb this
state to config callbacks without adding an additional arg to config_fn_t
and incurring a lot of churn, but if we start replacing "config_fn_t" with
the configset API (which we've independently wanted for some time), this may
become feasible.

And if we do this, "struct config_reader" itself will probably become
obsolete, because we'd be able to plumb only the relevant state for the
current operation, e.g. if we are parsing a config file, we'd pass only the
config file parsing state, instead of "struct config_reader", which also
contains config set iterating state. In such a scenario, we'd probably want
to pass "struct key_value_info" to the config callback, since that's all the
callback should be interested in anyway. Interestingly, this was proposed by
Junio back in [4], and we didn't do this back then out of concern for the
churn (just like in v1).

[1]
https://github.com/git/git/compare/master...chooglen:git:config-lib-parsing
[2]
https://github.com/gitgitgadget/git/compare/pr-git-1463/chooglen/config/structify-reading-v1..chooglen:git:config/structify-reading
[3]
https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com
[4]
https://lore.kernel.org/git/CAPc5daV6bdUKS-ExHmpT4Ppy2S832NXoyPw7aOLP7fG=WrBPgg@mail.gmail.com/

Glen Choo (8):
  config.c: plumb config_source through static fns
  config.c: don't assign to "cf_global" directly
  config.c: create config_reader and the_reader
  config.c: plumb the_reader through callbacks
  config.c: remove current_config_kvi
  config.c: remove current_parsing_scope
  config: report cached filenames in die_bad_number()
  config.c: rename "struct config_source cf"

 config.c               | 585 ++++++++++++++++++++++++-----------------
 config.h               |   1 +
 t/helper/test-config.c |  17 ++
 t/t1308-config-set.sh  |   9 +
 4 files changed, 369 insertions(+), 243 deletions(-)

base-commit: dadc8e6dacb629f46aee39bde90b6f09b73722eb

Submitted-As: https://lore.kernel.org/git/pull.1463.v2.git.git.1678925506.gitgitgadget@gmail.com
In-Reply-To: https://lore.kernel.org/git/pull.1463.git.git.1677631097.gitgitgadget@gmail.com