Re: [RFC PATCH 0/5] bypass config.c global state with configset

From: Glen Choo <chooglen@google.com>
Date: Mar 17, 2023, 23:32 UTC
Message-ID: <kl6ljzzfdkd6.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <kl6la60buquj.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 29 quoted lines

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