From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Max Gautier <max.gautier@redhat.com>,
Jeff King <peff@peff.net>
Subject: Re: [PATCH] gpg-interface: lazily initialize and read the configuration
Date: Thu, 09 Feb 2023 01:17:39 +0100 [thread overview]
Message-ID: <230209.86fsbfznot.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqlel7rj9z.fsf_-_@gitster.g>
On Wed, Feb 08 2023, Junio C Hamano wrote:
> Instead of forcing the porcelain commands to always read the
> configuration variables related to the signing and verifying
> signatures, lazily initialize the necessary subsystem on demand upon
> the first use.
>
> This hopefully would make it more future-proof as we do not have to
> think and decide whether we should call git_gpg_config() in the
> git_config() callback for each command.
One thing left un-noted here is that this will defer any errors in the
config now until use (or lazy init), so e.g.:
git -c gpg.mintrustlevel=bad show --show-signature
Used to exit with code 128 and an error, but will now (at least for me)
proceed to run show successfully.
I think that's OK overall, and most of our config reading these days
works like that, but it's probably worth noting.
> Quite a many git_config() callback functions that used to be custom
> callbacks are now just a thin wrapper around git_default_config().
> We could further remove, git_FOO_config and replace calls to
> git_config(git_FOO_config) with git_config(git_default_config), but
> to make it clear which ones are affected and the effect is only the
> removal of git_gpg_config(), it is vastly preferred not to do such a
> change in this step (they can be done on top once the dust settled).
Yeah, we could do that later, but I think it's trivially easy to see
which ones would be affected, i.e. these...
> diff --git c/builtin/am.c w/builtin/am.c
> index 82a41cbfc4..40126b59c5 100644
> --- c/builtin/am.c
> +++ w/builtin/am.c
> @@ -2314,12 +2314,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>
> static int git_am_config(const char *k, const char *v, void *cb UNUSED)
> {
> - int status;
> -
> - status = git_gpg_config(k, v, NULL);
> - if (status)
> - return status;
> -
> return git_default_config(k, v, NULL);
> }
>
> diff --git c/builtin/commit-tree.c w/builtin/commit-tree.c
> index cc8d584be2..f6a099d601 100644
> --- c/builtin/commit-tree.c
> +++ w/builtin/commit-tree.c
> @@ -39,9 +39,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
>
> static int commit_tree_config(const char *var, const char *value, void *cb)
> {
> - int status = git_gpg_config(var, value, NULL);
> - if (status)
> - return status;
> return git_default_config(var, value, cb);
> }
...but not a bunch of elided ones here, and then these...
> diff --git c/builtin/verify-commit.c w/builtin/verify-commit.c
> index 3ebad32b0f..3c5d0b024c 100644
> --- c/builtin/verify-commit.c
> +++ w/builtin/verify-commit.c
> @@ -54,9 +54,6 @@ static int verify_commit(const char *name, unsigned flags)
>
> static int git_verify_commit_config(const char *var, const char *value, void *cb)
> {
> - int status = git_gpg_config(var, value, cb);
> - if (status)
> - return status;
> return git_default_config(var, value, cb);
> }
>
> diff --git c/builtin/verify-tag.c w/builtin/verify-tag.c
> index 217566952d..ecffb069bf 100644
> --- c/builtin/verify-tag.c
> +++ w/builtin/verify-tag.c
> @@ -21,9 +21,6 @@ static const char * const verify_tag_usage[] = {
>
> static int git_verify_tag_config(const char *var, const char *value, void *cb)
> {
> - int status = git_gpg_config(var, value, cb);
> - if (status)
> - return status;
> return git_default_config(var, value, cb);
> }
...we can see are now just git_default_config() by another name. I'd
prefer to just see them gone in this same commit.
> @@ -632,6 +644,8 @@ int check_signature(struct signature_check *sigc,
> struct gpg_format *fmt;
> int status;
>
> + gpg_interface_lazy_init();
> +
> sigc->result = 'N';
> sigc->trust_level = -1;
>
This is needed, because we need "configured_min_trust_level" populated.
> @@ -695,11 +709,13 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct
>
> void set_signing_key(const char *key)
> {
> + gpg_interface_lazy_init();
> +
> free(configured_signing_key);
> configured_signing_key = xstrdup(key);
> }
But this is not, we could say that we're doing it for good measure, but
it's hard to imagine a scenario where we would end up actually needing
lazy init here. we'll just set a variable here, which...
> -int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
> +static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
> {
> struct gpg_format *fmt = NULL;
> char *fmtname = NULL;
> @@ -888,6 +904,8 @@ static const char *get_ssh_key_id(void) {
> /* Returns a textual but unique representation of the signing key */
> const char *get_signing_key_id(void)
> {
> + gpg_interface_lazy_init();
> +
...we'll read back here, and here the lazy init is needed, because...
> if (use_format->get_key_id) {
...this is one of the lazy init'd variables.
> return use_format->get_key_id();
> }
> @@ -898,6 +916,8 @@ const char *get_signing_key_id(void)
>
> const char *get_signing_key(void)
> {
> + gpg_interface_lazy_init();
> +
ditto, this is needed.
> if (configured_signing_key)
> return configured_signing_key;
> if (use_format->get_default_key) {
> @@ -923,6 +943,8 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level)
>
> int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
> {
> + gpg_interface_lazy_init();
> +
and this.
next prev parent reply other threads:[~2023-02-09 0:29 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-08 15:56 git rev-list fails to verify ssh-signed commits (but git log works) Max Gautier
2023-02-08 16:43 ` Jeff King
2023-02-08 17:56 ` Junio C Hamano
2023-02-08 18:20 ` Junio C Hamano
2023-02-08 20:31 ` [PATCH] gpg-interface: lazily initialize and read the configuration Junio C Hamano
2023-02-09 0:17 ` Ævar Arnfjörð Bjarmason [this message]
2023-02-09 2:05 ` Junio C Hamano
2023-02-09 2:24 ` Ævar Arnfjörð Bjarmason
2023-02-09 12:49 ` Jeff King
2023-02-09 16:38 ` Junio C Hamano
2023-02-09 20:24 ` [PATCH v2] " Junio C Hamano
2023-02-26 22:40 ` Jeff King
2023-02-27 16:00 ` Junio C Hamano
2023-03-08 8:34 ` Ævar Arnfjörð Bjarmason
2023-03-09 3:28 ` Jeff King
2023-03-09 17:03 ` Junio C Hamano
2023-03-10 9:01 ` Jeff King
2023-02-09 12:41 ` git rev-list fails to verify ssh-signed commits (but git log works) Jeff King
2023-02-09 16:44 ` Junio C Hamano
2023-02-08 17:00 ` Junio C Hamano
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=230209.86fsbfznot.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=max.gautier@redhat.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).