* git rev-list fails to verify ssh-signed commits (but git log works) @ 2023-02-08 15:56 Max Gautier 2023-02-08 16:43 ` Jeff King 2023-02-08 17:00 ` Junio C Hamano 0 siblings, 2 replies; 20+ messages in thread From: Max Gautier @ 2023-02-08 15:56 UTC (permalink / raw) To: git Hi. I was trying to implement a pre-push hook to verify my commits are properly signed before pushing them, and stumbled upon the following output (which looks like a bug to me): $ git rev-list @{u}..HEAD --format='%G? %H' commit 9497d347b048dbea7f527624f815f7926594c4bc error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification N 9497d347b048dbea7f527624f815f7926594c4bc commit 2466c5b3c0f2053b3cdadf4af299aab35e74aa0c error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification N 2466c5b3c0f2053b3cdadf4af299aab35e74aa0c commit ded83bc7f31df14b2e9a8d7bdfa1e95eee2bf5c1 error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification N ded83bc7f31df14b2e9a8d7bdfa1e95eee2bf5c1 commit 16d17277c608d995ad4d0b495d029c753509930c error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification N 16d17277c608d995ad4d0b495d029c753509930c While git log works and is able to retrieve the signatures $ git log @{u}..HEAD --format='%G? %H' G 9497d347b048dbea7f527624f815f7926594c4bc G 2466c5b3c0f2053b3cdadf4af299aab35e74aa0c G ded83bc7f31df14b2e9a8d7bdfa1e95eee2bf5c1 G 16d17277c608d995ad4d0b495d029c753509930c I get the error even though I have the following config : $ git config --list | grep 'allowed' gpg.ssh.allowedsignersfile=~/.config/git/MY_SIGNER_KEYS # by the way the actual config entry in ~/.config/git/config is # #[gpg "ssh"] # allowedSignersFile = ~/.config/git/MY_SIGNER_KEYS $ cat ~/.config/git/MY_SIGNER_KEYS mg@max.gautier.name,max.gautier@redhat.com sk-ssh-ed25519@openssh.com AAAAGnNrLXNzaC1lZDI1NTE5QG9wZW5zc2guY29tAAAAIL3W2Y4eAF92ySEW6ZE7d8Q+GXvP2G5quvN0zM+f1jGUAAAAB3NzaDphbGw= mg@max.gautier.name,max.gautier@redhat.com sk-ssh-ed25519@openssh.com AAAAGnNrLXNzaC1lZDI1NTE5QG9wZW5zc2guY29tAAAAIGBP0XfpNXRoFBIW9uEgfnCrrjgvzxr0taOYy0A03DtKAAAABHNzaDo= Am I missing something obvious ? Or is it git rev-list running in such a context than it can't find the allowedSignersFile ? Thanks -- Max Gautier Software Engineer, Open Services Group, Emerging Technologies Red Hat max.gautier@redhat.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: git rev-list fails to verify ssh-signed commits (but git log works) 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 17:00 ` Junio C Hamano 1 sibling, 1 reply; 20+ messages in thread From: Jeff King @ 2023-02-08 16:43 UTC (permalink / raw) To: Max Gautier; +Cc: git On Wed, Feb 08, 2023 at 04:56:53PM +0100, Max Gautier wrote: > I was trying to implement a pre-push hook to verify my commits are > properly signed before pushing them, and stumbled upon the following > output (which looks like a bug to me): > > $ git rev-list @{u}..HEAD --format='%G? %H' > commit 9497d347b048dbea7f527624f815f7926594c4bc > error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification > > [...] > > While git log works and is able to retrieve the signatures Yeah, I think this is a bug. The issue is that not every command loads the config callback for every config option. This is how we traditionally implemented the split between porcelain and plumbing (e.g., user-facing "git diff" will parse and respect "color.diff", but the scriptable "git diff-files" would not). In this case, the gpg config has been pushed to its own handler, and a few specific commands (like git-log) call it. I don't know if there is a good reason to avoid loading the config in plumbing, or if it was simply cargo-culted. I didn't test, but I suspect the patch below would fix your problem: diff --git a/config.c b/config.c index 00090a32fc..7ac9f1f5bc 100644 --- a/config.c +++ b/config.c @@ -1881,6 +1881,14 @@ int git_default_config(const char *var, const char *value, void *cb) if (starts_with(var, "core.")) return git_default_core_config(var, value, cb); + /* + * yikes, this needs to come early in the function because it + * also handles user.signingkey, which would otherwise get + * shunted to git_ident_config() below + */ + if (git_gpg_config(var, value, cb) < 0) + return -1; + if (starts_with(var, "user.") || starts_with(var, "author.") || starts_with(var, "committer.")) but it would need a bit more work: 1. Somebody would need to dig into the reasons, if any, for not calling git_gpg_config() everywhere. It might be fine, but there may be a good reason which we're now violating. Digging in the history and looking at the code might yield some hints. 2. The individual calls to git_gpg_config() in other programs should go away. 3. It's possible some refactoring may let us avoid the "yikes" comment above (e.g., should user.signingkey just go into the normal ident config handler?). -Peff ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: git rev-list fails to verify ssh-signed commits (but git log works) 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-09 12:41 ` git rev-list fails to verify ssh-signed commits (but git log works) Jeff King 0 siblings, 2 replies; 20+ messages in thread From: Junio C Hamano @ 2023-02-08 17:56 UTC (permalink / raw) To: Jeff King; +Cc: Max Gautier, git Jeff King <peff@peff.net> writes: > + /* > + * yikes, this needs to come early in the function because it > + * also handles user.signingkey, which would otherwise get > + * shunted to git_ident_config() below > + */ > + if (git_gpg_config(var, value, cb) < 0) > + return -1; Indeed. > if (starts_with(var, "user.") || > starts_with(var, "author.") || > starts_with(var, "committer.")) > > but it would need a bit more work: > > 1. Somebody would need to dig into the reasons, if any, for not > calling git_gpg_config() everywhere. It might be fine, but there > may be a good reason which we're now violating. Digging in the > history and looking at the code might yield some hints. Hmph, I didn't consider calling gpg_config unconditionally. It may do a bit more than what a typical config callback does (i.e. as opposed to just store the string values it gets, it tries table look-ups and stuff) but it is not too bad. > 2. The individual calls to git_gpg_config() in other programs should > go away. Naturally. > 3. It's possible some refactoring may let us avoid the "yikes" comment > above (e.g., should user.signingkey just go into the normal ident > config handler?). Hindsight is golden---if this were called gpg.signingkey we wouldn't be having this discussion X-<. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: git rev-list fails to verify ssh-signed commits (but git log works) 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 12:41 ` git rev-list fails to verify ssh-signed commits (but git log works) Jeff King 1 sibling, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2023-02-08 18:20 UTC (permalink / raw) To: Jeff King; +Cc: Max Gautier, git Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> + /* >> + * yikes, this needs to come early in the function because it >> + * also handles user.signingkey, which would otherwise get >> + * shunted to git_ident_config() below >> + */ >> + if (git_gpg_config(var, value, cb) < 0) >> + return -1; >> ... >> if (starts_with(var, "user.") || >> ... >> 3. It's possible some refactoring may let us avoid the "yikes" comment >> above (e.g., should user.signingkey just go into the normal ident >> config handler?). > > Hindsight is golden---if this were called gpg.signingkey we wouldn't > be having this discussion X-<. I wonder if gpg-interface functions can and should be taught to initialize themselves lazily without relying on the usual git_config(git_gpg_config) sequence. I.e. the first call to sign_buffer(), check_signature(), get_signing_key_id(), etc. would internally make a git_config(git_gpg_config) call, with the current callers of git_config(git_gpg_config) removed. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] gpg-interface: lazily initialize and read the configuration 2023-02-08 18:20 ` Junio C Hamano @ 2023-02-08 20:31 ` Junio C Hamano 2023-02-09 0:17 ` Ævar Arnfjörð Bjarmason 2023-02-09 12:49 ` Jeff King 0 siblings, 2 replies; 20+ messages in thread From: Junio C Hamano @ 2023-02-08 20:31 UTC (permalink / raw) To: git; +Cc: Max Gautier, Jeff King 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. 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). Signed-off-by: Junio C Hamano <gitster@pobox.com> --- > I wonder if gpg-interface functions can and should be taught to > initialize themselves lazily without relying on the usual > git_config(git_gpg_config) sequence. I.e. the first call to > sign_buffer(), check_signature(), get_signing_key_id(), etc. > would internally make a git_config(git_gpg_config) call, with the > current callers of git_config(git_gpg_config) removed. So here is such a change. I only checked that it passed t/ tests locally (and I do not run some tests like svn and p4). 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 ---- 16 files changed, 23 insertions(+), 57 deletions(-) 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); } diff --git c/builtin/commit.c w/builtin/commit.c index 44b763d7cd..794500dc9e 100644 --- c/builtin/commit.c +++ w/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 c/builtin/log.c w/builtin/log.c index 04412dd9c9..56741c6d37 100644 --- c/builtin/log.c +++ w/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 c/builtin/merge.c w/builtin/merge.c index 74de2ebd2b..289c13656c 100644 --- c/builtin/merge.c +++ w/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 c/builtin/pull.c w/builtin/pull.c index 1ab4de0005..4367727db6 100644 --- c/builtin/pull.c +++ w/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 c/builtin/push.c w/builtin/push.c index 60ac8017e5..8f8904dd08 100644 --- c/builtin/push.c +++ w/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 c/builtin/receive-pack.c w/builtin/receive-pack.c index a90af30363..9894dbdc79 100644 --- c/builtin/receive-pack.c +++ w/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 c/builtin/send-pack.c w/builtin/send-pack.c index 4c5d125fa0..c31e27346b 100644 --- c/builtin/send-pack.c +++ w/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 c/builtin/tag.c w/builtin/tag.c index d428c45dc8..725cfcd62b 100644 --- c/builtin/tag.c +++ w/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 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); } diff --git c/fmt-merge-msg.c w/fmt-merge-msg.c index f48f44f9cd..9b83c95a08 100644 --- c/fmt-merge-msg.c +++ w/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 c/gpg-interface.c w/gpg-interface.c index 687236430b..3b9ea3ac4c 100644 --- c/gpg-interface.c +++ w/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, 0); +} + 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 c/gpg-interface.h w/gpg-interface.h index 8a9ef41779..143cdc1c02 100644 --- c/gpg-interface.h +++ w/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 c/sequencer.c w/sequencer.c index 3e4a197289..a234b1ff88 100644 --- c/sequencer.c +++ w/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); } ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] gpg-interface: lazily initialize and read the configuration 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 2023-02-09 2:05 ` Junio C Hamano 2023-02-09 12:49 ` Jeff King 1 sibling, 1 reply; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-02-09 0:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Max Gautier, Jeff King 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gpg-interface: lazily initialize and read the configuration 2023-02-09 0:17 ` Ævar Arnfjörð Bjarmason @ 2023-02-09 2:05 ` Junio C Hamano 2023-02-09 2:24 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2023-02-09 2:05 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Max Gautier, Jeff King Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. That one is probably a good thing. We shouldn't interrupt users for a misspelt configuration value that the user is not using. >> @@ -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. I specifically did not want anybody to start doing this line of analysis, because it will add unnecessary bugs, and also introduce maintenance problems. You may be able to grab the current state of the code, but that will get stale and won't be a good guide to keep our code robust. >> @@ -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... And especially this one, we must have init or we'll be incorrect, I think. There is a direct set_signing_key() caller (I think in "git tag") that does not come from the git_config() callback route. Without the lazy initialization, we'd get configured_signing_key from the config because early in the start-up sequence of the command we would do git_gpg_config() via git_config(), and then try to process "-u keyid" by calling this one again. If you forget to lazily initialize here, configured_signing_key gets the keyid obtained via "-u keyid", and then when control reaches the real signing function, we'd realize that we still haven't initialized ourselves. And we call lazy init, which finds configured keyID, which is very likely different from "-u keyid" (the user would not be passing "-u keyid" from the command line to override, if that is the same as the configured one), and clobber it. Thanks. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gpg-interface: lazily initialize and read the configuration 2023-02-09 2:05 ` Junio C Hamano @ 2023-02-09 2:24 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-02-09 2:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Max Gautier, Jeff King On Wed, Feb 08 2023, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> 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. > > That one is probably a good thing. We shouldn't interrupt users for > a misspelt configuration value that the user is not using. *nod*, just noting it. >>> @@ -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. > > I specifically did not want anybody to start doing this line of > analysis, because it will add unnecessary bugs, and also introduce > maintenance problems. You may be able to grab the current state of > the code, but that will get stale and won't be a good guide to keep > our code robust. > >>> @@ -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... > > And especially this one, we must have init or we'll be incorrect, I > think. There is a direct set_signing_key() caller (I think in "git > tag") that does not come from the git_config() callback route. > Without the lazy initialization, we'd get configured_signing_key > from the config because early in the start-up sequence of the > command we would do git_gpg_config() via git_config(), and then try > to process "-u keyid" by calling this one again. > > If you forget to lazily initialize here, configured_signing_key gets > the keyid obtained via "-u keyid", and then when control reaches the > real signing function, we'd realize that we still haven't > initialized ourselves. And we call lazy init, which finds > configured keyID, which is very likely different from "-u keyid" > (the user would not be passing "-u keyid" from the command line to > override, if that is the same as the configured one), and clobber > it. Yeah, I take your general point that it's good to sprinkle the gpg_interface_lazy_init(). In this case I think we'll just barely do the right thing, the only external caller is tag.c, which first does: set_signing_key(keyid); And then: sign_buffer(buffer, buffer, get_signing_key()); So we'll (I think): 1. Get the non-config'd key from before 2. Call sign_buffer() 3. Promptly clobber that key 4. It won't matter because at that point we'll be passing a key as a parma, which overrides the "config" But yeah, dropping it would mean we end up with the wrong key in the variable afterwards, which even if it's not used is nasty. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gpg-interface: lazily initialize and read the configuration 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 @ 2023-02-09 12:49 ` Jeff King 2023-02-09 16:38 ` Junio C Hamano 1 sibling, 1 reply; 20+ messages in thread From: Jeff King @ 2023-02-09 12:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Max Gautier On Wed, Feb 08, 2023 at 12:31:36PM -0800, Junio C Hamano wrote: > > I wonder if gpg-interface functions can and should be taught to > > initialize themselves lazily without relying on the usual > > git_config(git_gpg_config) sequence. I.e. the first call to > > sign_buffer(), check_signature(), get_signing_key_id(), etc. > > would internally make a git_config(git_gpg_config) call, with the > > current callers of git_config(git_gpg_config) removed. > > So here is such a change. I only checked that it passed t/ tests > locally (and I do not run some tests like svn and p4). I think the tests tell us two things: - you didn't miss a spot where config needed to be initialized lazily here. The risk here is that many tests will be using defaults, not configured values, so coverage is not as good as you might hope. - there isn't a case where initializing the config all the time is a problem (i.e., the plumbing/porcelain thing discussed earlier). My "yikes" patch from upthread likewise passed, so that gives us a little confidence (though again, it's not clear that the plumbing cases which didn't _expect_ to read config would have test coverage). That said, having manually reviewed what the function is doing, I think it's probably OK (see my other response). > 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 ---- This all looks fairly sensible to me. I think we'd really want to see a "rev-list --format" test, too. One, because that's the immediate goal of this change. But two, because I think we are only guessing that loading the config is sufficient here. We've had bug with other subsystems where they expected to be initialized but plumbing callers didn't (e.g., the lazy init of notes-refs, etc). I _think_ we're probably good here. Just looking at "git log" (where we know --format, etc, works), it doesn't seem to do anything beyond initializing the config. -Peff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] gpg-interface: lazily initialize and read the configuration 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 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2023-02-09 16:38 UTC (permalink / raw) To: Jeff King; +Cc: git, Max Gautier Jeff King <peff@peff.net> writes: > This all looks fairly sensible to me. I think we'd really want to see a > "rev-list --format" test, too. One, because that's the immediate goal of > this change. Heh. I primarily wanted to see how much damage to the code does it take to implement such a lazy-loading scheme ;-) Which turned out to be "not much". Maybe when I have time next time, but no promises. > But two, because I think we are only guessing that loading > the config is sufficient here. We've had bug with other subsystems where > they expected to be initialized but plumbing callers didn't (e.g., the > lazy init of notes-refs, etc). Yup. > I _think_ we're probably good here. Just looking at "git log" (where we > know --format, etc, works), it doesn't seem to do anything beyond > initializing the config. That was my recollection from back when gpg-interface was split out of "git tag". Thanks for sanity checking. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] gpg-interface: lazily initialize and read the configuration 2023-02-09 16:38 ` Junio C Hamano @ 2023-02-09 20:24 ` Junio C Hamano 2023-02-26 22:40 ` Jeff King 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2023-02-09 20:24 UTC (permalink / raw) To: git 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(-) 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 -- 2.39.1-418-g7876265d61 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] gpg-interface: lazily initialize and read the configuration 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 0 siblings, 2 replies; 20+ messages in thread From: Jeff King @ 2023-02-26 22:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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); -- 2.40.0.rc0.479.g8b3a13b6b0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] gpg-interface: lazily initialize and read the configuration 2023-02-26 22:40 ` Jeff King @ 2023-02-27 16:00 ` Junio C Hamano 2023-03-08 8:34 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2023-02-27 16:00 UTC (permalink / raw) To: Jeff King; +Cc: git 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); ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] gpg-interface: lazily initialize and read the configuration 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 1 sibling, 1 reply; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-03-08 8:34 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] gpg-interface: lazily initialize and read the configuration 2023-03-08 8:34 ` Ævar Arnfjörð Bjarmason @ 2023-03-09 3:28 ` Jeff King 2023-03-09 17:03 ` Junio C Hamano 0 siblings, 1 reply; 20+ messages in thread From: Jeff King @ 2023-03-09 3:28 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] gpg-interface: lazily initialize and read the configuration 2023-03-09 3:28 ` Jeff King @ 2023-03-09 17:03 ` Junio C Hamano 2023-03-10 9:01 ` Jeff King 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2023-03-09 17:03 UTC (permalink / raw) To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] gpg-interface: lazily initialize and read the configuration 2023-03-09 17:03 ` Junio C Hamano @ 2023-03-10 9:01 ` Jeff King 0 siblings, 0 replies; 20+ messages in thread From: Jeff King @ 2023-03-10 9:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: git rev-list fails to verify ssh-signed commits (but git log works) 2023-02-08 17:56 ` Junio C Hamano 2023-02-08 18:20 ` Junio C Hamano @ 2023-02-09 12:41 ` Jeff King 2023-02-09 16:44 ` Junio C Hamano 1 sibling, 1 reply; 20+ messages in thread From: Jeff King @ 2023-02-09 12:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Max Gautier, git On Wed, Feb 08, 2023 at 09:56:16AM -0800, Junio C Hamano wrote: > > 1. Somebody would need to dig into the reasons, if any, for not > > calling git_gpg_config() everywhere. It might be fine, but there > > may be a good reason which we're now violating. Digging in the > > history and looking at the code might yield some hints. > > Hmph, I didn't consider calling gpg_config unconditionally. It may > do a bit more than what a typical config callback does (i.e. as > opposed to just store the string values it gets, it tries table > look-ups and stuff) but it is not too bad. It all looks pretty typical for config parsing to me. Matching in a constant-sized list of strings happens in lots of places (e.g., push.default). Performance-wise it's probably fine. I'd be more worried about correctness (command "foo" must not parse option "bar" because it is plumbing and must use the default). But looking over the options, I really don't see anything like that. The one I'd expect (or worry most about) is "we do/do not bother to enable signatures at all based on config" but I don't think that is the case. We default use_format to &gpg_format[0], so there is always a signature format defined, even if the config is skipped. The original split comes from your 2f47eae2a1 (Split GPG interface into its own helper library, 2011-09-07), where it was just moving bits out of the git-tag config. So I think we just grew into this situation organically. > I wonder if gpg-interface functions can and should be taught to > initialize themselves lazily without relying on the usual > git_config(git_gpg_config) sequence. I.e. the first call to > sign_buffer(), check_signature(), get_signing_key_id(), etc. > would internally make a git_config(git_gpg_config) call, with the > current callers of git_config(git_gpg_config) removed. If the gpg code used git_config_get_string(), etc, then they could just access each key on demand (efficiently, from an internal hash table), which reduces the risk of "oops, we forgot to initialize the config here". It does probably mean restructuring the code a little, though (since you'd often have an accessor function to get "foo.bar" rather than assuming "foo.bar" was parsed into an enum already, etc). That may not be worth the effort (and risk of regression) to convert. -Peff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: git rev-list fails to verify ssh-signed commits (but git log works) 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 0 siblings, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2023-02-09 16:44 UTC (permalink / raw) To: Jeff King; +Cc: Max Gautier, git Jeff King <peff@peff.net> writes: > I'd be more worried about correctness (command "foo" must not parse > option "bar" because it is plumbing and must use the default). But > looking over the options, I really don't see anything like that. The one > I'd expect (or worry most about) is "we do/do not bother to enable > signatures at all based on config" but I don't think that is the case. > We default use_format to &gpg_format[0], so there is always a signature > format defined, even if the config is skipped. I arrived at the same conclusion after seeing what these configuration affected. If some plumbing codepaths should avoid certain gpg-interface features, I didn't see a good reason for them to be calling into gpg-interface.c functions (e.g. that would have meant that "rev-list '--format=%G'" should have errored out with a "%G is only available in log but not in rev-list which is a plumbing" error message) and at that point reading configuration is much lessor problem. > If the gpg code used git_config_get_string(), etc, then they could just > access each key on demand (efficiently, from an internal hash table), > which reduces the risk of "oops, we forgot to initialize the config > here". It does probably mean restructuring the code a little, though > (since you'd often have an accessor function to get "foo.bar" rather > than assuming "foo.bar" was parsed into an enum already, etc). That may > not be worth the effort (and risk of regression) to convert. Yup. Care must be taken not to break "-u <keyid>" that is set via calling set_signing_key() directly, which assumes that the default key is already set from the config earlier, but the resulting code may make it clearer that our rule is "command line first, and then peek into configuration as a fallback value", if we did such a conversion. Thanks. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: git rev-list fails to verify ssh-signed commits (but git log works) 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:00 ` Junio C Hamano 1 sibling, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2023-02-08 17:00 UTC (permalink / raw) To: Max Gautier; +Cc: git Max Gautier <max.gautier@redhat.com> writes: > $ git rev-list @{u}..HEAD --format='%G? %H' > commit 9497d347b048dbea7f527624f815f7926594c4bc > error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification "git rev-list" as a plumbing does not want to read in or be affected by unnecessary configuration variables, so it uses git_default_config() to read the barebones, unlike "git log" that uses git_log_config() that calls, among other things, git_gpg_config(). Untill "git rev-list" learns to inspect the --format string and checks necessary configuration on-demand (e.g. presence of "%G" signals the need to call git_gpg_config()), this is sort of expected, unfortunately. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-03-10 9:05 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).