Re: [PATCH 0/6] [RFC] config.c: use struct for config reading state

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Mar 01 2023, Glen Choo via GitGitGadget wrote:
>
>> This series extracts the global config reading state into "struct
>> config_reader" and plumbs it through the config reading machinery. It's 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 agree with the overall direction, but don't think that rationale in
> 3/6 is sufficient to go in this "the_reader" direction, as opposed to
> sticking with and extending "the_repository" approach.
>
> For orthagonal reasons (getting rid of some of the API duplication) I've
> been carrying a patch to get rid of the "configset" part of the *public*
> API, i.e. to have API users always use the "repo_config_*()" or
> "git_config_*()" variants, that patch is at:
> https://github.com/avar/git/commit/0233297a359bbda43a902dd0213aacdca82faa34

Those patches are probably worth sending, even if only as RFC. I found
it pretty hard to draft a substantial response without effectively doing
a full review of the patch.

> It's a bit distasteful, but that change argues that just mocking up a
> "struct repository" with a "config" member pointing to a new configset
> is better than maintaining an entirely different API just for those
> cases where we need to parse a one-off file or whatever.
>
> I think that going in that direction neatly solves the issues you're
> noting here and in your 3/6, i.e. we'd always have this in the "repo"
> object, so we'd just stick the persistent "reader" variables in the
> "struct repository"'s "config" member.

If I understand your proposal correctly, we would move the config
variables to the_repository. Then, any time a caller would like to work
with an individual file, it would init a new "struct repository" with a
clean set of config members (using repo_init_repo_blank_config() or
something) and reuse the repo_config_* API?

It is a workable solution, e.g. that approach would work around the
failures in test-tool and scalar that I observed. In the spirit of
libification, this feels like a kludge, though, since we'd be reverting
to using "struct repository" for more things instead of using more
well-scoped interfaces. IMO a better future for the config_set API would
be to move it into configset.c or something, where only users who want
the low level API would use it and everyone else would just pretend it
doesn't exist. This would be a little like libgit2's organization, where
'general config', 'config parsing' and 'in-memory config value
representations' are separate files, e.g.

  https://github.com/libgit2/libgit2/blob/main/src/libgit2/config.h
  https://github.com/libgit2/libgit2/blob/main/src/libgit2/config_parse.h
  https://github.com/libgit2/libgit2/blob/main/src/libgit2/config_entries.h

I also hesitate to put the config variables on the_repository, because
in the long term, I think "struct config_reader" can and should be
purely internal to config.c. But if we start advertising its existence
via the_repository, that might be an invitation to (ab)use that API and
make that transition harder.