git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Glen Choo <chooglen@google.com>
Cc: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>,
	Emily Shaffer <nasamuffin@google.com>, Jeff King <peff@peff.net>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 0/6] [RFC] config.c: use struct for config reading state
Date: Tue, 07 Mar 2023 19:36:58 +0100	[thread overview]
Message-ID: <230307.86jzzsz8x3.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <kl6lfsagifpc.fsf@chooglen-macbookpro.roam.corp.google.com>


On Tue, Mar 07 2023, Glen Choo wrote:

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

Yes, sorry. It's part of some changes on top of my outstanding config
API changes (just re-rolled at
https://lore.kernel.org/git/cover-v6-0.9-00000000000-20230307T180516Z-avarab@gmail.com/). Hopefully
those will land soon after the upcoming release, I'll try to submit this
(along with related changes) soon afterwards.

>> 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's certainly a hack, but so is introducing a new "the_reader"
singleton whose lifetime we need to manage seperately from
"the_repository" in the common case :)

I think a better argument for this is probably that if you try to change
repository.h so that we define "struct repository" thusly (The
"hash_algo" field being still there due to a very common macro):

	struct repository {
	        const struct git_hash_algo *hash_algo;
	        struct config_set *config;
	};

And then try to:

	make config.o

You'll get:
	
	$ make config.o
	    CC config.o
	config.c: In function ‘include_by_branch’:
	config.c:311:46: error: ‘struct repository’ has no member named ‘gitdir’
	  311 |         const char *refname = !the_repository->gitdir ?
	      |                                              ^~
	config.c: In function ‘repo_read_config’:
	config.c:2523:30: error: ‘struct repository’ has no member named ‘commondir’
	 2523 |         opts.commondir = repo->commondir;
	      |                              ^~
	config.c:2524:28: error: ‘struct repository’ has no member named ‘gitdir’
	 2524 |         opts.git_dir = repo->gitdir;
	      |                            ^~
	make: *** [Makefile:2719: config.o] Error 1

I.e. almost all of the config code doesn't care about the repository at
all, it just needs the "struct config_set" that's in the repository
struct.

With the linked-to change I'm arguing that just mocking it up sucks less
than carrying a duplicate set of API functions just for those rare cases
where we need to one-off read a config file.

But...

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

...yes it's a kludge, but I'd think if you're interested in more
generally fixing it that a better use of time would be to narrowly focus
on those cases where we don't have a "repository" now, i.e. the
configset API users my linked-to patch shows & amends.

Everything else that uses git_config_get_*(), repo_config_get_*() will
either use an already set up "the_repository", or lazily init it, see
the git_config_check_init() API users.

Or, we could have repo_config() and friends not take a "struct
repository", but another config-specific object which has the subset of
the three fields from that struct which it actually needs (config,
gitdir & commondir).

The mocking function I added in the linked-to commit was just a way of
getting to that via the shortest means.


  reply	other threads:[~2023-03-07 19:14 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-01  0:38 [PATCH 0/6] [RFC] config.c: use struct for config reading state Glen Choo via GitGitGadget
2023-03-01  0:38 ` [PATCH 1/6] config.c: plumb config_source through static fns Glen Choo via GitGitGadget
2023-03-03 18:02   ` Junio C Hamano
2023-03-01  0:38 ` [PATCH 2/6] config.c: don't assign to "cf" directly Glen Choo via GitGitGadget
2023-03-01  0:38 ` [PATCH 3/6] config.c: create config_reader and the_reader Glen Choo via GitGitGadget
2023-03-03 18:05   ` Junio C Hamano
2023-03-01  0:38 ` [PATCH 4/6] config.c: plumb the_reader through callbacks Glen Choo via GitGitGadget
2023-03-08  9:54   ` Ævar Arnfjörð Bjarmason
2023-03-08 18:00     ` Glen Choo
2023-03-08 18:07       ` Junio C Hamano
2023-03-01  0:38 ` [PATCH 5/6] config.c: remove current_config_kvi Glen Choo via GitGitGadget
2023-03-06 20:12   ` Calvin Wan
2023-03-01  0:38 ` [PATCH 6/6] config.c: remove current_parsing_scope Glen Choo via GitGitGadget
2023-03-06 19:57 ` [PATCH 0/6] [RFC] config.c: use struct for config reading state Jonathan Tan
2023-03-06 21:45   ` Glen Choo
2023-03-06 22:38     ` Jonathan Tan
2023-03-08 10:32       ` Ævar Arnfjörð Bjarmason
2023-03-08 23:09         ` Glen Choo
2023-03-07 11:57 ` Ævar Arnfjörð Bjarmason
2023-03-07 18:22   ` Glen Choo
2023-03-07 18:36     ` Ævar Arnfjörð Bjarmason [this message]
2023-03-07 19:36     ` Junio C Hamano
2023-03-07 22:53       ` Glen Choo
2023-03-08  9:17         ` Ævar Arnfjörð Bjarmason
2023-03-08 23:18           ` Glen Choo
2023-03-16  0:11 ` [PATCH v2 0/8] " Glen Choo via GitGitGadget
2023-03-16  0:11   ` [PATCH v2 1/8] config.c: plumb config_source through static fns Glen Choo via GitGitGadget
2023-03-16 21:16     ` Jonathan Tan
2023-03-16  0:11   ` [PATCH v2 2/8] config.c: don't assign to "cf_global" directly Glen Choo via GitGitGadget
2023-03-16 21:18     ` Jonathan Tan
2023-03-16 21:31       ` Junio C Hamano
2023-03-16 22:56       ` Glen Choo
2023-03-16  0:11   ` [PATCH v2 3/8] config.c: create config_reader and the_reader Glen Choo via GitGitGadget
2023-03-16 21:22     ` Jonathan Tan
2023-03-16  0:11   ` [PATCH v2 4/8] config.c: plumb the_reader through callbacks Glen Choo via GitGitGadget
2023-03-16  0:11   ` [PATCH v2 5/8] config.c: remove current_config_kvi Glen Choo via GitGitGadget
2023-03-16  0:11   ` [PATCH v2 6/8] config.c: remove current_parsing_scope Glen Choo via GitGitGadget
2023-03-16  0:11   ` [PATCH v2 7/8] config: report cached filenames in die_bad_number() Glen Choo via GitGitGadget
2023-03-16 22:22     ` Jonathan Tan
2023-03-16 23:05       ` Glen Choo
2023-03-16  0:11   ` [PATCH v2 8/8] config.c: rename "struct config_source cf" Glen Choo via GitGitGadget
2023-03-16  0:15   ` [PATCH v2 0/8] config.c: use struct for config reading state Glen Choo
2023-03-16 22:29   ` Jonathan Tan
2023-03-17  5:01   ` [RFC PATCH 0/5] bypass config.c global state with configset Ævar Arnfjörð Bjarmason
2023-03-17  5:01     ` [RFC PATCH 1/5] config.h: move up "struct key_value_info" Ævar Arnfjörð Bjarmason
2023-03-17  5:01     ` [RFC PATCH 2/5] config.c: use "enum config_origin_type", not "int" Ævar Arnfjörð Bjarmason
2023-03-17  5:01     ` [RFC PATCH 3/5] config API: add a config_origin_type_name() helper Ævar Arnfjörð Bjarmason
2023-03-17  5:01     ` [RFC PATCH 4/5] config.c: refactor configset_iter() Ævar Arnfjörð Bjarmason
2023-03-17  5:01     ` [RFC PATCH 5/5] config API: add and use a repo_config_kvi() Ævar Arnfjörð Bjarmason
2023-03-17 17:17       ` Junio C Hamano
2023-03-17 20:59       ` Jonathan Tan
2023-03-17 16:21     ` [RFC PATCH 0/5] bypass config.c global state with configset Junio C Hamano
2023-03-17 16:28     ` Glen Choo
2023-03-17 19:20     ` Glen Choo
2023-03-17 23:32       ` Glen Choo
2023-03-29 11:53       ` Ævar Arnfjörð Bjarmason
2023-03-28 17:51   ` [PATCH v3 0/8] config.c: use struct for config reading state Glen Choo via GitGitGadget
2023-03-28 17:51     ` [PATCH v3 1/8] config.c: plumb config_source through static fns Glen Choo via GitGitGadget
2023-03-28 17:51     ` [PATCH v3 2/8] config.c: don't assign to "cf_global" directly Glen Choo via GitGitGadget
2023-03-28 17:51     ` [PATCH v3 3/8] config.c: create config_reader and the_reader Glen Choo via GitGitGadget
2023-03-29 10:41       ` Ævar Arnfjörð Bjarmason
2023-03-29 18:57         ` Junio C Hamano
2023-03-29 20:02           ` Glen Choo
2023-03-30 17:51         ` Glen Choo
2023-03-28 17:51     ` [PATCH v3 4/8] config.c: plumb the_reader through callbacks Glen Choo via GitGitGadget
2023-03-28 17:51     ` [PATCH v3 5/8] config.c: remove current_config_kvi Glen Choo via GitGitGadget
2023-03-28 17:51     ` [PATCH v3 6/8] config.c: remove current_parsing_scope Glen Choo via GitGitGadget
2023-03-28 17:51     ` [PATCH v3 7/8] config: report cached filenames in die_bad_number() Glen Choo via GitGitGadget
2023-03-28 17:51     ` [PATCH v3 8/8] config.c: rename "struct config_source cf" Glen Choo via GitGitGadget
2023-03-28 18:00     ` [PATCH v3 0/8] config.c: use struct for config reading state Glen Choo
2023-03-28 20:14       ` Junio C Hamano
2023-03-28 21:24         ` Glen Choo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=230307.86jzzsz8x3.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=chooglen@google.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@google.com \
    --cc=nasamuffin@google.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).