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

From: Glen Choo <chooglen@google.com>
Date: Mar 7, 2023, 18:22 UTC
Message-ID: <kl6lfsagifpc.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <230307.86wn3szrzu.gmgdl@evledraar.gmail.com>

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

Show 20 quoted lines

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

Show 9 quoted lines

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