[RFC,0/5] bypass config.c global state with configset

Message

Ævar Arnfjörð Bjarmason March 17, 2023, 5:01 a.m. UTC

On Thu, Mar 16 2023, Glen Choo via GitGitGadget wrote:

> 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.

That libification url
(https://github.com/git/git/compare/master...chooglen:git:config-lib-parsing)
doesn't work for me, and I didn't find a branch with that name in your
published repo. So maybe you've already done all this
post-libification work...

> = 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.

...in any case. This RFC expands a bit on my comments on the v1 (at
[1] and upthread). It doesn't get all the way there, but with the
small change in 5/5 we've gotten rid of current_config_line(), the
1-4/5 are trivial pre-refactorings to make that diff smaller
(e.g. moving the "struct key_value_info" around in config.h).

Maybe it still makes sense to go for this "the_reader" intermediate
step, but I can't help but think that we could just go for it all in
one leap, and that you've just got stuck on thinking that you needed
to change "config_fn_t" for all its callers.

As the 5/5 here shows we have various orthagonal uses of the
"config_fn_t" in config.c, and can just implement a new callback type
for the edge cases where we need the file & line info.

This still leave the current_config_name() etc, which
e.g. builtin/config.c still uses. In your series you've needed to add
the new "reader" parameter for everything from do_config_from(), but
if we're doing that can't we instead just go straight to passing a
"struct key_value_info *" (perhaps with an added "name" field) all the
way down, replacing "cf->linenr" etc?

Instead you end up extending "the_reader" everywhere, including to
e.g. configset_iter, which I think as the 5/5 here shows isn't needed,
but maybe I've missed something.

Similarly, you mention git_parse_int() wanting to report a filename
and/or line number. I'm aware that it can do that, but it doesn't do
so in the common case, e.g.:

	git -c format.filenameMaxLength=abc log
	fatal: bad numeric config value 'abc' for 'format.filenamemaxlength': invalid unit

And the same goes for writing it to e.g. ~/.gitconfig. It's only if
you use "git config --file" or similar that we'll report a filename.

So just as with the current_config_line() I wonder if you just grepped
for e.g. git_config_int() and thought because we have a lot of users
of it that all of them would require this data, but for e.g. this
log.c caller (and most or all of the others) we'll be reading the
normal config, and aren't getting any useful info from
die_bad_number() that we wouldn't get from an error function that
didn't need the "linenr" etc.

> 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).

I think we can make it even simpler than that, and from playing around
with builtin/config.c a bit after the 5/5 here I got a POC working
(but am not posting it here, didn't have time to clean it up).

We can just make config_set_callback() and configset_iter()
non-static, so e.g. the builtin/config.c caller that implements
"--show-origin" can keep its config_with_options(...) call, but
instead of "streaming" the config, it'll buffer it up into a
configset.

The advantage of that is that with the configset API we'll get a
"struct key_value_info *" for free on the other end. I.e. we'll
configset_iter() with a fn=NULL, but with a defined "config_kvi_fn_t",
which 5/5 is adding.

But I haven't done that work (and am not planning to finish this), but
maybe this helps.

We'll also need to track the equivalent of "cf->linenr" etc. while we
do the actual initial parse. I think it might be simpler to start by
converting those "linenr" to a "struct key_value_info" that's placed
in the "config_source" right away.

I.e. when we pass it to the error handlers we'll need to give them
access to the "linenr", but we don't want to provide e.g. "eof" (which
is internal-only state).

I wonder how much else you're converting here is actually dead code in
the end (or can trivially be made dead). E.g. the
current_config_line() change you make in 1/8 is never going to use the
"cf_global" if combined with the 5/5 change here.

1. https://lore.kernel.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason (5):
  config.h: move up "struct key_value_info"
  config.c: use "enum config_origin_type", not "int"
  config API: add a config_origin_type_name() helper
  config.c: refactor configset_iter()
  config API: add and use a repo_config_kvi()

 builtin/remote.c       | 11 +++----
 config.c               | 67 ++++++++++++++++++++++++++----------------
 config.h               | 29 +++++++++++++-----
 t/helper/test-config.c | 13 ++++----
 t/t5505-remote.sh      |  7 +++--
 5 files changed, 80 insertions(+), 47 deletions(-)

Comments

Junio C Hamano March 17, 2023, 4:21 p.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

> Ævar Arnfjörð Bjarmason (5):
>   config.h: move up "struct key_value_info"
>   config.c: use "enum config_origin_type", not "int"
>   config API: add a config_origin_type_name() helper
>   config.c: refactor configset_iter()
>   config API: add and use a repo_config_kvi()

I haven't reached the end of 5/5 but it is a shame to see that these
obviously good and trivial clean-up patches like [1-4/5] have to be
buried in an RFC patch and benefit of applying them alone becomes
unclear only because it is done in an area that is actively worked
on by others.  The latter is unfortunately inherent to the approach
of "commenting by patches" but hopefully we can see them reappear
when the tree is quiescent.

Thanks.

Glen Choo March 17, 2023, 4:28 p.m. UTC | #2

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> That libification url
> (https://github.com/git/git/compare/master...chooglen:git:config-lib-parsing)
> doesn't work for me, and I didn't find a branch with that name in your
> published repo. So maybe you've already done all this
> post-libification work...

Argh, sorry, I renamed the branch.

  https://github.com/git/git/compare/master...chooglen:git:config/read-without-globals

> ...in any case. This RFC expands a bit on my comments on the v1 (at
> [1] and upthread). It doesn't get all the way there, but with the
> small change in 5/5 we've gotten rid of current_config_line(), the
> 1-4/5 are trivial pre-refactorings to make that diff smaller
> (e.g. moving the "struct key_value_info" around in config.h).

Thanks. I haven't read through it yet, but it sounds promising :)

Glen Choo March 17, 2023, 7:20 p.m. UTC | #3

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> Maybe it still makes sense to go for this "the_reader" intermediate
> step, but I can't help but think that we could just go for it all in
> one leap, and that you've just got stuck on thinking that you needed
> to change "config_fn_t" for all its callers.
>
> As the 5/5 here shows we have various orthagonal uses of the
> "config_fn_t" in config.c, and can just implement a new callback type
> for the edge cases where we need the file & line info.
>
> This still leave the current_config_name() etc, which
> e.g. builtin/config.c still uses. In your series you've needed to add
> the new "reader" parameter for everything from do_config_from(), but
> if we're doing that can't we instead just go straight to passing a
> "struct key_value_info *" (perhaps with an added "name" field) all the
> way down, replacing "cf->linenr" etc?

In the end state, I also think we should be passing "struct
key_value_info *" around instead of "cf", so I think we are seeing
"the_reader" in the same way (as a transitional state).

I considered the "repo_config_kvi() + config_fn_kvi_t" as well, but I
rejected it (before discussion on the list, whoops) because I didn't
want to add _yet another_ set of parallel config APIs, e.g. we already
have repo_config(), git_config(), configset*(),
git_config_from_file*(). Multiplying that by 2 to add *_kvi() seems like
way too much, especially when it seems clear that our current definition
of config_fn_t has some problems.

Maybe we could deprecate the non-*_kvi(), and have both as a
transitional state? It might work, but I think biting the bullet and
changing config_fn_t would be easier actually.

I'll try applying your series on top of my 1/8 (and maybe 7/8, see next
reply) and extending it to cover "cf" (instead of just
current_config_kvi()) to see whether the *_kvi() approach saves a lot of
unnecessary plumbing. I'd still feel very strongly about getting rid of
all of the non *_kvi() versions, though, but maybe that would happen in
a cleanup topic.

> Similarly, you mention git_parse_int() wanting to report a filename
> and/or line number. I'm aware that it can do that, but it doesn't do
> so in the common case, e.g.:
>
> 	git -c format.filenameMaxLength=abc log
> 	fatal: bad numeric config value 'abc' for 'format.filenamemaxlength': invalid unit
>
> And the same goes for writing it to e.g. ~/.gitconfig. It's only if
> you use "git config --file" or similar that we'll report a filename.

That's true, but I think that's a bug, not a feature. See 7/8 [1] where
I addressed it.

1.  https://lore.kernel.org/git/3c83[e-mail]/

> We can just make config_set_callback() and configset_iter()
> non-static, so e.g. the builtin/config.c caller that implements
> "--show-origin" can keep its config_with_options(...) call, but
> instead of "streaming" the config, it'll buffer it up into a
> configset.

Hm, so to extrapolate, we could make it so that nobody outside of
config.c uses the *config_from_file() APIs directly. Instead, all reads
get buffered up into a configset. That might not be a bad idea. It would
definitely help with some of your goals of config API surface reduction.

This would be friendlier in cases where we were already creating custom
configsets (I know we have some of those, but I don't recall where), but
in cases where we were reading the file directly (e.g.
builtin/config.c), we'd be taking a memory and runtime hit. I'm not sure
how I (or others) feel about that yet.

Glen Choo March 17, 2023, 11:32 p.m. UTC | #4

Glen Choo <[e-mail]> writes:

>> This still leave the current_config_name() etc, which
>> e.g. builtin/config.c still uses. In your series you've needed to add
>> the new "reader" parameter for everything from do_config_from(), but
>> if we're doing that can't we instead just go straight to passing a
>> "struct key_value_info *" (perhaps with an added "name" field) all the
>> way down, replacing "cf->linenr" etc?
>
> In the end state, I also think we should be passing "struct
> key_value_info *" around instead of "cf", so I think we are seeing
> "the_reader" in the same way (as a transitional state).
>
> I considered the "repo_config_kvi() + config_fn_kvi_t" as well, but I
> rejected it (before discussion on the list, whoops) because I didn't
> want to add _yet another_ set of parallel config APIs, e.g. we already
> have repo_config(), git_config(), configset*(),
> git_config_from_file*(). Multiplying that by 2 to add *_kvi() seems like
> way too much, especially when it seems clear that our current definition
> of config_fn_t has some problems.
>
> Maybe we could deprecate the non-*_kvi(), and have both as a
> transitional state? It might work, but I think biting the bullet and
> changing config_fn_t would be easier actually.
>
> I'll try applying your series on top of my 1/8 (and maybe 7/8, see next
> reply) and extending it to cover "cf" (instead of just
> current_config_kvi()) to see whether the *_kvi() approach saves a lot of
> unnecessary plumbing. I'd still feel very strongly about getting rid of
> all of the non *_kvi() versions, though, but maybe that would happen in
> a cleanup topic.

As mentioned above, I think having both "config_fn_t" and
config_kvi_fn_t" would make sense if we found a good way to extend it to
the functions that use "cf" (parsing config syntax), not the ones that
use "current_config_kvi" (iterating a config set).

I think technical difficulty is not a barrier here:

- Constructing a "struct key_value_info" out of "cf" is trivial

- Supporting both "config_fn_t" and "config_kvi_fn_t" with one
  implementation is also doable in theory. One approach would be to use
  only *_kvi() internally, and then adapt the external "config_fn_t"
  like:

    struct adapt_nonkvi {
          config_fn_t fn;
          void *data;
    };

    static int adapt_nonkvi_fn(const char *key, const char *value,
                              struct key_value_info *kvi UNUSED, void *cb)
    {
          struct adapt_nonkvi *adapt = cb;
          return adapt->fn(key, value, adapt->data);
    }

The real cost is that there are so many functions we'd need to adapt (I
counted 12 functions that accept config_fn_t in config.h). I think I got
through about 30% of it before thinking that it was too much work to try
to avoid adjusting config_fn_t.

I still strongly believe that we shouldn't have both config_fn_t and
config_kvi_fn_t in the long run, and we should converge on one.
It's plausible that if we support both as an intermediate state, we'll
never do the actual cleanup, so I think the extra cost is not worth it.

Ævar Arnfjörð Bjarmason March 29, 2023, 11:53 a.m. UTC | #5

On Fri, Mar 17 2023, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <[e-mail]> writes:
> [...]
>> Similarly, you mention git_parse_int() wanting to report a filename
>> and/or line number. I'm aware that it can do that, but it doesn't do
>> so in the common case, e.g.:
>>
>> 	git -c format.filenameMaxLength=abc log
>> 	fatal: bad numeric config value 'abc' for 'format.filenamemaxlength': invalid unit
>>
>> And the same goes for writing it to e.g. ~/.gitconfig. It's only if
>> you use "git config --file" or similar that we'll report a filename.
>
> That's true, but I think that's a bug, not a feature. See 7/8 [1] where
> I addressed it.
>
> 1.  https://lore.kernel.org/git/3c83[e-mail]/
>
>> We can just make config_set_callback() and configset_iter()
>> non-static, so e.g. the builtin/config.c caller that implements
>> "--show-origin" can keep its config_with_options(...) call, but
>> instead of "streaming" the config, it'll buffer it up into a
>> configset.
>
> Hm, so to extrapolate, we could make it so that nobody outside of
> config.c uses the *config_from_file() APIs directly. Instead, all reads
> get buffered up into a configset. That might not be a bad idea. It would
> definitely help with some of your goals of config API surface reduction.
>
> This would be friendlier in cases where we were already creating custom
> configsets (I know we have some of those, but I don't recall where), but
> in cases where we were reading the file directly (e.g.
> builtin/config.c), we'd be taking a memory and runtime hit. I'm not sure
> how I (or others) feel about that yet.

I'm pretty sure that in the end this wouldn't matter, i.e. the time it
takes to parse the config is trivial, and the users of these APIs like
"git config -l --show-origin" aren't performance-senitive.

But for the general case & if you're concerned about this a trivial
addition on top of what I suggested would be to pass a streaming
callback to config_set_callback(), i.e. you could get the values you'd
get from configset_iter() as we parse them.