| Message ID | xmqqlel7rj9z.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpg-interface: lazily initialize and read the configuration | expand |
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.
Æ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.
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.
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
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.
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); }