| Message ID | xmqqpmaimvtd.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fd2d4c135ed974fdddf2af687748d28c58575984 |
| Headers | show |
| Series | [v2] gpg-interface: lazily initialize and read the configuration | expand |
On Thu, Feb 09, 2023 at 12:24:14PM -0800, 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. Sorry, I seem to have missed this when you originally posted it. And I saw it marked as "will merge to next?" in the latest what's cooking. It looks good to me, and I think we can proceed with it (though of course it is not urgent and can probably wait until post-2.40). > A few 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). Yes, I think it is good not to do so in this patch. If we want to do it now on top, here's a patch. Though I could also see the argument for just leaving them. -- >8 -- Subject: [PATCH] drop pure pass-through config callbacks Commit fd2d4c135e (gpg-interface: lazily initialize and read the configuration, 2023-02-09) shrunk a few custom config callbacks so that they are just one-liners of: return git_default_config(...); We can drop them entirely and replace them direct calls of git_default_config() intead. This makes the code a little shorter and easier to understand (with the downside being that if they do grow custom options again later, we'll have to recreate the functions). Signed-off-by: Jeff King <peff@peff.net> --- I looked over the output of: git grep --function-context 'return git_default_config' to see if there were other cases, not caused by fd2d4c135e. But I didn't see any. builtin/am.c | 7 +------ builtin/commit-tree.c | 7 +------ builtin/verify-commit.c | 7 +------ builtin/verify-tag.c | 7 +------ 4 files changed, 4 insertions(+), 24 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 40126b59c5..fccf40f8ee 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2312,11 +2312,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar return 0; } -static int git_am_config(const char *k, const char *v, void *cb UNUSED) -{ - return git_default_config(k, v, NULL); -} - int cmd_am(int argc, const char **argv, const char *prefix) { struct am_state state; @@ -2440,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) if (argc == 2 && !strcmp(argv[1], "-h")) usage_with_options(usage, options); - git_config(git_am_config, NULL); + git_config(git_default_config, NULL); am_state_init(&state); diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c index f6a099d601..c0bbe9373d 100644 --- a/builtin/commit-tree.c +++ b/builtin/commit-tree.c @@ -37,11 +37,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p) commit_list_insert(parent, parents_p); } -static int commit_tree_config(const char *var, const char *value, void *cb) -{ - return git_default_config(var, value, cb); -} - static int parse_parent_arg_callback(const struct option *opt, const char *arg, int unset) { @@ -118,7 +113,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix) OPT_END() }; - git_config(commit_tree_config, NULL); + git_config(git_default_config, NULL); if (argc < 2 || !strcmp(argv[1], "-h")) usage_with_options(commit_tree_usage, options); diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c index 3c5d0b024c..7aedf10e85 100644 --- a/builtin/verify-commit.c +++ b/builtin/verify-commit.c @@ -52,11 +52,6 @@ static int verify_commit(const char *name, unsigned flags) return run_gpg_verify((struct commit *)obj, flags); } -static int git_verify_commit_config(const char *var, const char *value, void *cb) -{ - return git_default_config(var, value, cb); -} - int cmd_verify_commit(int argc, const char **argv, const char *prefix) { int i = 1, verbose = 0, had_error = 0; @@ -67,7 +62,7 @@ int cmd_verify_commit(int argc, const char **argv, const char *prefix) OPT_END() }; - git_config(git_verify_commit_config, NULL); + git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, verify_commit_options, verify_commit_usage, PARSE_OPT_KEEP_ARGV0); diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index ecffb069bf..5c00b0b0f7 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -19,11 +19,6 @@ static const char * const verify_tag_usage[] = { NULL }; -static int git_verify_tag_config(const char *var, const char *value, void *cb) -{ - return git_default_config(var, value, cb); -} - int cmd_verify_tag(int argc, const char **argv, const char *prefix) { int i = 1, verbose = 0, had_error = 0; @@ -36,7 +31,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) OPT_END() }; - git_config(git_verify_tag_config, NULL); + git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, verify_tag_options, verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
Jeff King <peff@peff.net> writes: > Sorry, I seem to have missed this when you originally posted it. And I > saw it marked as "will merge to next?" in the latest what's cooking. It > looks good to me, and I think we can proceed with it (though of course > it is not urgent and can probably wait until post-2.40). > ... > Yes, I think it is good not to do so in this patch. If we want to do it > now on top, here's a patch. Though I could also see the argument for > just leaving them. Ah, of course I completely forgot about the patch and mentioning a possible follow-on work. > I looked over the output of: > > git grep --function-context 'return git_default_config' > > to see if there were other cases, not caused by fd2d4c135e. But I didn't > see any. > > builtin/am.c | 7 +------ > builtin/commit-tree.c | 7 +------ > builtin/verify-commit.c | 7 +------ > builtin/verify-tag.c | 7 +------ > 4 files changed, 4 insertions(+), 24 deletions(-) Nice to see these reductions. Thanks. > diff --git a/builtin/am.c b/builtin/am.c > index 40126b59c5..fccf40f8ee 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -2312,11 +2312,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar > return 0; > } > > -static int git_am_config(const char *k, const char *v, void *cb UNUSED) > -{ > - return git_default_config(k, v, NULL); > -} > - > int cmd_am(int argc, const char **argv, const char *prefix) > { > struct am_state state; > @@ -2440,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) > if (argc == 2 && !strcmp(argv[1], "-h")) > usage_with_options(usage, options); > > - git_config(git_am_config, NULL); > + git_config(git_default_config, NULL); > > am_state_init(&state); > > diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c > index f6a099d601..c0bbe9373d 100644 > --- a/builtin/commit-tree.c > +++ b/builtin/commit-tree.c > @@ -37,11 +37,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p) > commit_list_insert(parent, parents_p); > } > > -static int commit_tree_config(const char *var, const char *value, void *cb) > -{ > - return git_default_config(var, value, cb); > -} > - > static int parse_parent_arg_callback(const struct option *opt, > const char *arg, int unset) > { > @@ -118,7 +113,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix) > OPT_END() > }; > > - git_config(commit_tree_config, NULL); > + git_config(git_default_config, NULL); > > if (argc < 2 || !strcmp(argv[1], "-h")) > usage_with_options(commit_tree_usage, options); > diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c > index 3c5d0b024c..7aedf10e85 100644 > --- a/builtin/verify-commit.c > +++ b/builtin/verify-commit.c > @@ -52,11 +52,6 @@ static int verify_commit(const char *name, unsigned flags) > return run_gpg_verify((struct commit *)obj, flags); > } > > -static int git_verify_commit_config(const char *var, const char *value, void *cb) > -{ > - return git_default_config(var, value, cb); > -} > - > int cmd_verify_commit(int argc, const char **argv, const char *prefix) > { > int i = 1, verbose = 0, had_error = 0; > @@ -67,7 +62,7 @@ int cmd_verify_commit(int argc, const char **argv, const char *prefix) > OPT_END() > }; > > - git_config(git_verify_commit_config, NULL); > + git_config(git_default_config, NULL); > > argc = parse_options(argc, argv, prefix, verify_commit_options, > verify_commit_usage, PARSE_OPT_KEEP_ARGV0); > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c > index ecffb069bf..5c00b0b0f7 100644 > --- a/builtin/verify-tag.c > +++ b/builtin/verify-tag.c > @@ -19,11 +19,6 @@ static const char * const verify_tag_usage[] = { > NULL > }; > > -static int git_verify_tag_config(const char *var, const char *value, void *cb) > -{ > - return git_default_config(var, value, cb); > -} > - > int cmd_verify_tag(int argc, const char **argv, const char *prefix) > { > int i = 1, verbose = 0, had_error = 0; > @@ -36,7 +31,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) > OPT_END() > }; > > - git_config(git_verify_tag_config, NULL); > + git_config(git_default_config, NULL); > > argc = parse_options(argc, argv, prefix, verify_tag_options, > verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
On Sun, Feb 26 2023, Jeff King wrote: > On Thu, Feb 09, 2023 at 12:24:14PM -0800, 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. > > Sorry, I seem to have missed this when you originally posted it. And I > saw it marked as "will merge to next?" in the latest what's cooking. It > looks good to me, and I think we can proceed with it (though of course > it is not urgent and can probably wait until post-2.40). > >> A few 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). > > Yes, I think it is good not to do so in this patch. If we want to do it > now on top, here's a patch. Though I could also see the argument for > just leaving them. > > -- >8 -- > Subject: [PATCH] drop pure pass-through config callbacks > > Commit fd2d4c135e (gpg-interface: lazily initialize and read the > configuration, 2023-02-09) shrunk a few custom config callbacks so that > they are just one-liners of: > > return git_default_config(...); > > We can drop them entirely and replace them direct calls of > git_default_config() intead. This makes the code a little shorter and > easier to understand (with the downside being that if they do grow > custom options again later, we'll have to recreate the functions). > > Signed-off-by: Jeff King <peff@peff.net> > --- > I looked over the output of: > > git grep --function-context 'return git_default_config' > > to see if there were other cases, not caused by fd2d4c135e. But I didn't > see any. As added review: This is the same patch diff as I sent on February 9th: https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/; my local range-diff to my previously submitted topic & next being: 229: cc5d1d32fd4 ! 2: d93c160dcbc drop pure pass-through config callbacks @@ ## Metadata ## -Author: Jeff King <peff@peff.net> +Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Commit message ## - drop pure pass-through config callbacks + {am,commit-tree,verify-{commit,tag}}: refactor away config wrapper - Commit fd2d4c135e (gpg-interface: lazily initialize and read the - configuration, 2023-02-09) shrunk a few custom config callbacks so that - they are just one-liners of: + In the preceding commit these config functions became mere wrappers + for git_default_config(), so let's invoke it directly instead. - return git_default_config(...); - - We can drop them entirely and replace them direct calls of - git_default_config() intead. This makes the code a little shorter and - easier to understand (with the downside being that if they do grow - custom options again later, we'll have to recreate the functions). - - Signed-off-by: Jeff King <peff@peff.net> - Signed-off-by: Junio C Hamano <gitster@pobox.com> + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## builtin/am.c ## @@ builtin/am.c: static int parse_opt_show_current_patch(const struct option *opt, const char *ar -: ----------- > 3: c099d48b4bf gpg-interface.c: lazily get GPG config variables on demand So this LGTM.
On Wed, Mar 08, 2023 at 09:34:15AM +0100, Ævar Arnfjörð Bjarmason wrote: > As added review: This is the same patch diff as I sent on February 9th: > https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/; > my local range-diff to my previously submitted topic & next being: > [...] > So this LGTM. Thanks, and sorry for stealing your patch. I forgot that yours existed in that thread (and obviously I'm happy if either is applied). -Peff
Jeff King <peff@peff.net> writes: > On Wed, Mar 08, 2023 at 09:34:15AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> As added review: This is the same patch diff as I sent on February 9th: >> https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/; >> my local range-diff to my previously submitted topic & next being: >> [...] >> So this LGTM. > > Thanks, and sorry for stealing your patch. I forgot that yours existed > in that thread (and obviously I'm happy if either is applied). I am not Ævar but the last time this happened what he said was that he did so not because he wanted to complain that somebody else stole his thunder but because he wanted to show his agreement to the patch by pointing at an indenendent invention of the same thing. I personally do not appreciate that tactics, exactly because it can easily be misinterpreted as a complaint. Saying "I read the patch and I think it is exactly how I would solve the problem, too. Looking good" would have been much safer in that regard and conveyed the same agreement. Thanks.
On Thu, Mar 09, 2023 at 09:03:35AM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Wed, Mar 08, 2023 at 09:34:15AM +0100, Ævar Arnfjörð Bjarmason wrote: > > > >> As added review: This is the same patch diff as I sent on February 9th: > >> https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/; > >> my local range-diff to my previously submitted topic & next being: > >> [...] > >> So this LGTM. > > > > Thanks, and sorry for stealing your patch. I forgot that yours existed > > in that thread (and obviously I'm happy if either is applied). > > I am not Ævar but the last time this happened what he said was that > he did so not because he wanted to complain that somebody else stole > his thunder but because he wanted to show his agreement to the patch > by pointing at an indenendent invention of the same thing. > > I personally do not appreciate that tactics, exactly because it can > easily be misinterpreted as a complaint. Saying "I read the patch > and I think it is exactly how I would solve the problem, too. > Looking good" would have been much safer in that regard and conveyed > the same agreement. I think you are being too hard on Ævar here. While I agree it can be interpreted as passive aggressive sniping, I'd be a little frustrated, too, if I had written a patch and then somebody submitted the exact same thing later. (Hence my response that it was "oops" and not an intentional slight). If two people are independently doing the same work, we're wasting effort, and it's worth thinking about how we can avoid that. In this particular case, my opinion is that Ævar's original patch was OK by itself, but it was coupled with an unwelcome change. Submitting the cleanup to the now-empty callbacks on their own would have had a greater chance of success. (As a maintainer, you can also split up patches after the fact, but there is cognitive load in doing so. Minimizing that load is something submitters can do to help the project scale). -Peff
diff --git a/builtin/am.c b/builtin/am.c index 82a41cbfc4..40126b59c5 100644 --- a/builtin/am.c +++ b/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 a/builtin/commit-tree.c b/builtin/commit-tree.c index cc8d584be2..f6a099d601 100644 --- a/builtin/commit-tree.c +++ b/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); } diff --git a/builtin/commit.c b/builtin/commit.c index 44b763d7cd..794500dc9e 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1600,7 +1600,6 @@ int cmd_status(int argc, const char **argv, const char *prefix) static int git_commit_config(const char *k, const char *v, void *cb) { struct wt_status *s = cb; - int status; if (!strcmp(k, "commit.template")) return git_config_pathname(&template_file, k, v); @@ -1620,9 +1619,6 @@ static int git_commit_config(const char *k, const char *v, void *cb) return 0; } - status = git_gpg_config(k, v, NULL); - if (status) - return status; return git_status_config(k, v, s); } diff --git a/builtin/log.c b/builtin/log.c index 04412dd9c9..56741c6d37 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -601,8 +601,6 @@ static int git_log_config(const char *var, const char *value, void *cb) return 0; } - if (git_gpg_config(var, value, cb) < 0) - return -1; return git_diff_ui_config(var, value, cb); } diff --git a/builtin/merge.c b/builtin/merge.c index 74de2ebd2b..289c13656c 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -659,9 +659,6 @@ static int git_merge_config(const char *k, const char *v, void *cb) } status = fmt_merge_msg_config(k, v, cb); - if (status) - return status; - status = git_gpg_config(k, v, NULL); if (status) return status; return git_diff_ui_config(k, v, cb); diff --git a/builtin/pull.c b/builtin/pull.c index 1ab4de0005..4367727db6 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -359,8 +359,6 @@ static enum rebase_type config_get_rebase(int *rebase_unspecified) */ static int git_pull_config(const char *var, const char *value, void *cb) { - int status; - if (!strcmp(var, "rebase.autostash")) { config_autostash = git_config_bool(var, value); return 0; @@ -372,10 +370,6 @@ static int git_pull_config(const char *var, const char *value, void *cb) check_trust_level = 0; } - status = git_gpg_config(var, value, cb); - if (status) - return status; - return git_default_config(var, value, cb); } diff --git a/builtin/push.c b/builtin/push.c index 60ac8017e5..8f8904dd08 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -502,11 +502,6 @@ static int git_push_config(const char *k, const char *v, void *cb) { const char *slot_name; int *flags = cb; - int status; - - status = git_gpg_config(k, v, NULL); - if (status) - return status; if (!strcmp(k, "push.followtags")) { if (git_config_bool(k, v)) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a90af30363..9894dbdc79 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -133,10 +133,6 @@ static int receive_pack_config(const char *var, const char *value, void *cb) { int status = parse_hide_refs_config(var, value, "receive", &hidden_refs); - if (status) - return status; - - status = git_gpg_config(var, value, NULL); if (status) return status; diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 4c5d125fa0..c31e27346b 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -130,8 +130,6 @@ static void print_helper_status(struct ref *ref) static int send_pack_config(const char *k, const char *v, void *cb) { - git_gpg_config(k, v, NULL); - if (!strcmp(k, "push.gpgsign")) { const char *value; if (!git_config_get_value("push.gpgsign", &value)) { diff --git a/builtin/tag.c b/builtin/tag.c index d428c45dc8..725cfcd62b 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -180,8 +180,6 @@ static const char tag_template_nocleanup[] = static int git_tag_config(const char *var, const char *value, void *cb) { - int status; - if (!strcmp(var, "tag.gpgsign")) { config_sign_tag = git_config_bool(var, value); return 0; @@ -194,9 +192,6 @@ static int git_tag_config(const char *var, const char *value, void *cb) return 0; } - status = git_gpg_config(var, value, cb); - if (status) - return status; if (!strcmp(var, "tag.forcesignannotated")) { force_sign_annotate = git_config_bool(var, value); return 0; diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c index 3ebad32b0f..3c5d0b024c 100644 --- a/builtin/verify-commit.c +++ b/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 a/builtin/verify-tag.c b/builtin/verify-tag.c index 217566952d..ecffb069bf 100644 --- a/builtin/verify-tag.c +++ b/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); } diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c index f48f44f9cd..9b83c95a08 100644 --- a/fmt-merge-msg.c +++ b/fmt-merge-msg.c @@ -17,8 +17,6 @@ static struct string_list suppress_dest_patterns = STRING_LIST_INIT_DUP; int fmt_merge_msg_config(const char *key, const char *value, void *cb) { - int status = 0; - if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) { int is_bool; merge_log_config = git_config_bool_or_int(key, value, &is_bool); @@ -37,9 +35,6 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb) string_list_append(&suppress_dest_patterns, value); suppress_dest_pattern_seen = 1; } else { - status = git_gpg_config(key, value, NULL); - if (status) - return status; return git_default_config(key, value, cb); } return 0; diff --git a/gpg-interface.c b/gpg-interface.c index 687236430b..404d4cccf3 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -9,6 +9,18 @@ #include "tempfile.h" #include "alias.h" +static int git_gpg_config(const char *, const char *, void *); + +static void gpg_interface_lazy_init(void) +{ + static int done; + + if (done) + return; + done = 1; + git_config(git_gpg_config, NULL); +} + static char *configured_signing_key; static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file; static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED; @@ -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; @@ -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); } -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(); + if (use_format->get_key_id) { 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(); + 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(); + return use_format->sign_buffer(buffer, signature, signing_key); } diff --git a/gpg-interface.h b/gpg-interface.h index 8a9ef41779..143cdc1c02 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -79,7 +79,6 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, */ const char *gpg_trust_level_to_str(enum signature_trust_level level); -int git_gpg_config(const char *, const char *, void *); void set_signing_key(const char *); const char *get_signing_key(void); diff --git a/sequencer.c b/sequencer.c index 3e4a197289..a234b1ff88 100644 --- a/sequencer.c +++ b/sequencer.c @@ -263,10 +263,6 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) if (opts->action == REPLAY_REVERT && !strcmp(k, "revert.reference")) opts->commit_use_reference = git_config_bool(k, v); - status = git_gpg_config(k, v, NULL); - if (status) - return status; - return git_diff_basic_config(k, v, NULL); } diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh index 36eb86a4b1..20913b3713 100755 --- a/t/t7031-verify-tag-signed-ssh.sh +++ b/t/t7031-verify-tag-signed-ssh.sh @@ -200,4 +200,14 @@ test_expect_success GPGSSH 'verifying a forged tag with --format should fail sil test_must_be_empty actual-forged ' +test_expect_success GPGSSH 'rev-list --format=%G' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git rev-list -1 --format="%G? %H" sixth-signed >actual && + cat >expect <<-EOF && + commit $(git rev-parse sixth-signed^0) + G $(git rev-parse sixth-signed^0) + EOF + test_cmp expect actual +' + test_done
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. A few 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). Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * OK, this time it comes with a small test addition ;-) builtin/am.c | 6 ------ builtin/commit-tree.c | 3 --- builtin/commit.c | 4 ---- builtin/log.c | 2 -- builtin/merge.c | 3 --- builtin/pull.c | 6 ------ builtin/push.c | 5 ----- builtin/receive-pack.c | 4 ---- builtin/send-pack.c | 2 -- builtin/tag.c | 5 ----- builtin/verify-commit.c | 3 --- builtin/verify-tag.c | 3 --- fmt-merge-msg.c | 5 ----- gpg-interface.c | 24 +++++++++++++++++++++++- gpg-interface.h | 1 - sequencer.c | 4 ---- t/t7031-verify-tag-signed-ssh.sh | 10 ++++++++++ 17 files changed, 33 insertions(+), 57 deletions(-)