git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>
Cc: Jonathan Tan <jonathantanmy@google.com>,
	git@vger.kernel.org, Emily Shaffer <nasamuffin@google.com>,
	Jeff King <peff@peff.net>,
	Derrick Stolee <derrickstolee@github.com>,
	Glen Choo <chooglen@google.com>
Subject: Re: [PATCH 0/6] [RFC] config.c: use struct for config reading state
Date: Mon,  6 Mar 2023 11:57:56 -0800	[thread overview]
Message-ID: <20230306195756.3399115-1-jonathantanmy@google.com> (raw)
In-Reply-To: <pull.1463.git.git.1677631097.gitgitgadget@gmail.com>

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>  * We could add "struct config_reader" to "config_fn_t", i.e.
>    
>    -typedef int (*config_fn_t)(const char *var, const char *val, void
>    *data); +typedef int (*config_fn_t)(const struct config_reader *reader,
>    const char *var, const char *val, void *data);
>    
>    which isn't complex at all, except that there are ~100 config_fn_t
>    implementations [3] and a good number of them may never reference
>    "reader". If the churn is tolerable, I think this a good way forward.
> 
>  * We could create a new kind of "config_fn_t" that accepts "struct
>    config_reader", e.g.
>    
>    typedef int (*config_fn_t)(const char *var, const char *val, void *data);
>    +typedef int (*config_state_fn_t)(const struct config_reader *reader,
>    const char *var, const char *val, void *data);
>    
>    and only adjust the callers that would actually reference "reader". This
>    is less churn, but I couldn't find a great way to do this kind of
>    'switching between config callback types' elegantly.

To reduce churn, one thing that could be done alongside is to convert
config-using code (which is...practically the rest of Git) to start
using the configset interface (we seem to be using configsets internally
anyway, as evidenced by repo_config()). That way, we would reduce the
number of implementations of config_fn_t.

>  * We could smuggle "struct config_reader" to callback functions in a way
>    that interested callers could see it, but uninterested callers could
>    ignore. One trick that Jonathan Tan came up with (though not necessarily
>    endorsed) would be to allocate a struct for the config value + "struct
>    config_reader", then, interested callers could use "offset_of" to recover
>    the "struct config_reader". It's a little hacky, but it's low-churn.

Indeed, although we should probably use this as a last resort.

> = Questions
> 
>  * Is this worth merging without the extra work? There are some cleanups in
>    this series that could make it valuable, but there are also some hacks
>    (see 4/6) that aren't so great.

I'm leaning towards merging it now, but can go either way, since the
cost of churn is limited to one single file, but so are the benefits.
If it was up to me to decide, I would merge it now, because this opens
up a lot of work that other contributors could individually do (in
particular, converting individual config code paths so that we don't
need to reference the_reader as a global anymore).

I don't see 4/6 as a hack. It is true that the nature of the config_fn_t
callback could change so that passing the reader would end up being
done in yet another different way, but firstly, I don't think that will
happen for quite some time, and secondly, it might not happen at all
(right now, I think what's most likely to happen is that the rest of the
Git code moves to configsets and only a fraction of the Git code would
need to do low-level parsing, which would not have a problem passing the
reader through the data object since they would probably need to pass
other context anyway).

>  * Is the extra work even worth it?

Depends on which extra work, but I think that eliminating the the_reader
global would really be useful (and, as far as I know, the whole reason
for this effort). From the Git codebase's perspective, doing this would
(as far as I know) eliminate the need for pushing and popping cf, and
make multithreaded multi-repo operations less error-prone (e.g. we
can spawn a thread operating on a submodule and that thread can itself
read the configs of nested submodules without worrying about clobbering
global state...well, there is thread-local storage, but as far as I know
this is not portable).

>  * Do any of the ideas seem more promising than the others? Are there other
>    ideas I'm missing?

Hopefully I answered this in my answers to the other questions.

  parent reply	other threads:[~2023-03-06 19:59 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 ` Jonathan Tan [this message]
2023-03-06 21:45   ` [PATCH 0/6] [RFC] config.c: use struct for config reading state 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
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=20230306195756.3399115-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=chooglen@google.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.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).