Note to Junio: 8/8 (which renames "cs" -> "set") conflicts with ab/config-multi-and-nonbool. I previously said that I'd rebase this, but presumably a remerge-diff is more ergonomic + flexible (let me know if I'm mistaken), so I'll send a remerge-diff in a reply (I don't trust GGG not to mangle the patch :/). After sending this out, I'll see if cocci can make it easy enough to change config_fn_t. If so, I'll probably go with that approach for the future 'libified' patches, which should also line up nicely with what Ævar suggested. = Changes in v3 * Adjust the *_push() function to be unconditional (like the *_pop() function) * Various commit message and comment cleanups = 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. = Description This series prepares for config.[ch] to be libified as as part of the libification effort that Emily described in [1]. 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 [2], and we didn't do this back then out of concern for the churn (just like in v1). [1] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@xxxxxxxxxxxxxx [2] https://lore.kernel.org/git/CAPc5daV6bdUKS-ExHmpT4Ppy2S832NXoyPw7aOLP7fG=WrBPgg@xxxxxxxxxxxxxx/ 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 | 584 ++++++++++++++++++++++++----------------- config.h | 1 + t/helper/test-config.c | 17 ++ t/t1308-config-set.sh | 9 + 4 files changed, 368 insertions(+), 243 deletions(-) base-commit: dadc8e6dacb629f46aee39bde90b6f09b73722eb Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1463%2Fchooglen%2Fconfig%2Fstructify-reading-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1463/chooglen/config/structify-reading-v3 Pull-Request: https://github.com/git/git/pull/1463 Range-diff vs v2: 1: 75d0f0efb79 = 1: 75d0f0efb79 config.c: plumb config_source through static fns 2: 7555da0b0e0 ! 2: 39db7d8596a config.c: don't assign to "cf_global" directly @@ config.c: static struct key_value_info *current_config_kvi; +static inline void config_reader_push_source(struct config_source *top) +{ -+ if (cf_global) -+ top->prev = cf_global; ++ top->prev = cf_global; + cf_global = top; +} + 3: 4347896f0a4 ! 3: 72774fd08f3 config.c: create config_reader and the_reader @@ config.c: static struct key_value_info *current_config_kvi; +static inline void config_reader_push_source(struct config_reader *reader, + struct config_source *top) { -- if (cf_global) -- top->prev = cf_global; +- top->prev = cf_global; - cf_global = top; -+ if (reader->source) -+ top->prev = reader->source; ++ top->prev = reader->source; + reader->source = top; + /* FIXME remove this when cf_global is removed. */ + cf_global = reader->source; @@ config.c: static struct key_value_info *current_config_kvi; - cf_global = cf_global->prev; + ret = reader->source; + reader->source = reader->source->prev; -+ /* FIXME remove this when cf is removed. */ ++ /* FIXME remove this when cf_global is removed. */ + cf_global = reader->source; return ret; } 4: 22b69971749 ! 4: e02dddd560f config.c: plumb the_reader through callbacks @@ config.c: static struct config_reader the_reader; /* @@ config.c: static inline void config_reader_push_source(struct config_reader *reader, - if (reader->source) - top->prev = reader->source; + { + top->prev = reader->source; reader->source = top; - /* FIXME remove this when cf_global is removed. */ - cf_global = reader->source; @@ config.c: static inline struct config_source *config_reader_pop_source(struct co BUG("tried to pop config source, but we weren't reading config"); ret = reader->source; reader->source = reader->source->prev; -- /* FIXME remove this when cf is removed. */ +- /* FIXME remove this when cf_global is removed. */ - cf_global = reader->source; return ret; } 5: afb6e3e318d ! 5: c79eaf74f89 config.c: remove current_config_kvi @@ config.c: static enum config_scope current_parsing_scope; { + if (reader->config_kvi) + BUG("source should not be set while iterating a config set"); - if (reader->source) - top->prev = reader->source; + top->prev = reader->source; reader->source = top; + } @@ config.c: static inline struct config_source *config_reader_pop_source(struct config_reade return ret; } 6: a57e35163ae = 6: 05d9ffa21f6 config.c: remove current_parsing_scope 7: 3c83d9535a0 ! 7: eb843e6f08d config: report cached filenames in die_bad_number() @@ Commit message Fix this by refactoring the current_config_* functions into variants that don't BUG() when we aren't reading config, and using the resulting - functions in die_bad_number(). Refactoring is needed because "git config - --get[-regexp] --type=int" parses the int value _after_ parsing the - config file, which will run into the BUG(). + functions in die_bad_number(). "git config --get[-regexp] --type=int" + cannot use the non-refactored version because it parses the int value + _after_ parsing the config file, which would run into the BUG(). - Also, plumb "struct config_reader" into the new functions. This isn't - necessary per se, but this generalizes better, so it might help us avoid - yet another refactor. + Since the refactored functions aren't public, they use "struct + config_reader". 1. https://lore.kernel.org/git/20160518223712.GA18317@xxxxxxxxxxxxxxxxxxxxx/ 8: 9aec9092fdf = 8: ab800aa104c config.c: rename "struct config_source cf" -- gitgitgadget
