From: Glen Choo <chooglen@google.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: vdye@github.com, johannes.schindelin@gmx.de, newren@gmail.com,
peff@peff.net, gitster@pobox.com,
Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 0/6] [RFC] Lazy-loaded default Git config
Date: Thu, 08 Jun 2023 11:19:16 -0700 [thread overview]
Message-ID: <kl6lbkhpzujf.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <pull.1539.git.1685716420.gitgitgadget@gmail.com>
Hi Stolee!
Thanks for coming to the Libification discussion today, it was nice to
be able to discuss this idea with a bigger group.
As is custom, I'll repost my own thoughts here. If folks are interested
in a summary of the whole discussion, you can find the notes at
https://docs.google.com/document/d/1mw_qPPgfQUv1gfKMwmvZjpYaOOzxcNH2N8bRbvBWfIQ/edit#heading=h.fcm7uhwlk55z
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> * Is this a worthwhile category of bug to protect against?
If we look past the concrete bugs and consider the general problem of
process-wide state being hard to use correctly, I think this is
definitely worth solving.
And in case anyone doubts the the current interface is hard to use
correctly: the state lives in poorly scoped global variables that need
to be manually initialized by calling git_config() at the right time
with git_default_config. It's also hard to remember to do this for your
builtin because some builtins call git_default_config in _their own_
outer config_fn_t, and others call git_config(git_default_config).
> * Is wrapping global state in accessor methods the right way to protect
> against that class of bugs?
Lazy-loading via accessor methods seem slightly excessive. The crux of
the problem is that we haven't initialized the values before they are
read, so I think we can get away with plain fields in a struct as long
as they are always initialized (e.g. the builtin author doesn't need to
remember to do this). We could initialize all of the fields during the
setup process, where you placed declare_config_available(), at which
point config should be available.
If we use config hash lookups to intialize the values we want,
pre-initializing like this shouldn't be noticeably more wasteful
compared to lazy-loading, since in either case, we are already parsing
all of the config into the in-memory config sets and looking them up
with hash lookups. Pre-initializing does a small bit more work upfront
by assigning to the fields, but it means that accessing the settings
later can be faster since we can avoid the "getter method" calls.
> * Are there other benefits to doing this?
Yes! I'm generally excited about encapsulating loose global variables
into an appropriate abstraction, e.g. it was something I was thinking
about this when I was working on protected config (which is loose global
state, but also sort of a collection of repo-wide settings). It'll be
extremely relevant for libification, since this paves the way to
eventually encapsulate the process-wide state in a way that makes it
separable from the library code.
> * Are there reasons why this cure is worse than the disease?
This seems quite similar to what we're doing with repo-settings.c (which
also has its own problems with initialization), and I'd like to avoid
having two similar-looking-but-different systems in the long run. For
this to go forward, I'd like to see either an explicit goal to deprecate
repo-settings.c (e.g. this new system handles repository settings in
addition to process-wide settings), or a reasoned separation between
them.
prev parent reply other threads:[~2023-06-08 18:19 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 1/6] config: create new global config helpers Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 2/6] config: add trust_executable_bit to global config Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 3/6] config: move trust_ctime " Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 4/6] config: move quote_path_fully " Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 5/6] config: move has_symlinks " Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 6/6] config: move ignore_case " Derrick Stolee via GitGitGadget
2023-06-08 18:19 ` Glen Choo [this message]
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=kl6lbkhpzujf.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=newren@gmail.com \
--cc=peff@peff.net \
--cc=vdye@github.com \
/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.
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).