{"thread":{"id":"59218","subject":"[PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API","startedAt":"2023-02-09T14:35:15Z","lastAt":"2023-02-10T19:02:15Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"471857","messageId":"cover-0.2-00000000000-20230209T142225Z-avarab@gmail.com","threadId":"59218","inReplyTo":"+TqEM21o+3TGx6D@coredump.intra.peff.net","subject":"[PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-09T14:35:04Z","receivedAt":"2023-02-09T14:35:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Feb 09 2023, Jeff King wrote:\n\n> If the gpg code used git_config_get_string(), etc, then they could just\n> access each key on demand (efficiently, from an internal hash table),\n> which reduces the risk of \"oops, we forgot to initialize the config\n> here\". It does probably mean restructuring the code a little, though\n> (since you'd often have an accessor function to get \"foo.bar\" rather\n> than assuming \"foo.bar\" was parsed into an enum already, etc). That may\n> not be worth the effort (and risk of regression) to convert.\n\nI'd already played around with that a bit as part of reviewing Junio's\nchange, this goes on top of that.\n\nI found that continuing this conversion was getting harder, but these\n3 cases really were trivial cases where we're just reading a variable\nglobally, and then proceeding to use it in one specific place.\n\nOut of the remaining ones gpg.program et all looked easiest, but I\ndidn't continue with it.\n\nFor anyone interested think it would be best to continue by converting\nthe remaining bits by having commit, tag etc. set up some \"struct\ngpg\", so that when they could directly instruct it ot do its config\nreading before parse_options(). The remaining complexity is mainly\nwith the file-global & having to juggle in what order we read & set\nwhat.\n\nFWIW when poking at this I found that we have fairly robust testing\nsupport for this area, but it could be better, but it's good enough to\nspot that if we stop reading these we'll fail tests.\n\nBut e.g. for the \"gpg.program\" we've got tests that'll fail if the\n\"gpg\" program variable isn't read, but not for the \"ssh\" variable, but\nas they'll both share the same/similar reader code any future\nmigration should spot any glaring bugs, just possibly not subtle ones.\n\nBranch & passing[1] CI at:\nhttps://github.com/avar/git/tree/avar/gpg-lazy-init-configset\n\n1. Well, passing except for the general current Windows CI dumpster\n   fire on topics based off current \"master\".\n\nÆvar Arnfjörð Bjarmason (2):\n  {am,commit-tree,verify-{commit,tag}}: refactor away config wrapper\n  gpg-interface.c: lazily get GPG config variables on demand\n\n builtin/am.c            |  7 +----\n builtin/commit-tree.c   |  7 +----\n builtin/verify-commit.c |  7 +----\n builtin/verify-tag.c    |  7 +----\n gpg-interface.c         | 66 ++++++++++++++++-------------------------\n 5 files changed, 29 insertions(+), 65 deletions(-)\n\n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471858","messageId":"patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com","threadId":"59218","inReplyTo":"cover-0.2-00000000000-20230209T142225Z-avarab@gmail.com","subject":"[PATCH 1/2] {am,commit-tree,verify-{commit,tag}}: refactor away config wrapper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-09T14:35:05Z","receivedAt":"2023-02-09T14:35:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding commit these config functions became mere wrappers\nfor git_default_config(), so let's invoke it directly instead.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/am.c            | 7 +------\n builtin/commit-tree.c   | 7 +------\n builtin/verify-commit.c | 7 +------\n builtin/verify-tag.c    | 7 +------\n 4 files changed, 4 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 40126b59c54..fccf40f8ee7 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2312,11 +2312,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n \treturn 0;\n }\n \n-static int git_am_config(const char *k, const char *v, void *cb UNUSED)\n-{\n-\treturn git_default_config(k, v, NULL);\n-}\n-\n int cmd_am(int argc, const char **argv, const char *prefix)\n {\n \tstruct am_state state;\n@@ -2440,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(usage, options);\n \n-\tgit_config(git_am_config, NULL);\n+\tgit_config(git_default_config, NULL);\n \n \tam_state_init(&state);\n \ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex f6a099d601c..c0bbe9373d0 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -37,11 +37,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)\n \tcommit_list_insert(parent, parents_p);\n }\n \n-static int commit_tree_config(const char *var, const char *value, void *cb)\n-{\n-\treturn git_default_config(var, value, cb);\n-}\n-\n static int parse_parent_arg_callback(const struct option *opt,\n \t\tconst char *arg, int unset)\n {\n@@ -118,7 +113,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(commit_tree_config, NULL);\n+\tgit_config(git_default_config, NULL);\n \n \tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(commit_tree_usage, options);\ndiff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\nindex 3c5d0b024c9..7aedf10e856 100644\n--- a/builtin/verify-commit.c\n+++ b/builtin/verify-commit.c\n@@ -52,11 +52,6 @@ static int verify_commit(const char *name, unsigned flags)\n \treturn run_gpg_verify((struct commit *)obj, flags);\n }\n \n-static int git_verify_commit_config(const char *var, const char *value, void *cb)\n-{\n-\treturn git_default_config(var, value, cb);\n-}\n-\n int cmd_verify_commit(int argc, const char **argv, const char *prefix)\n {\n \tint i = 1, verbose = 0, had_error = 0;\n@@ -67,7 +62,7 @@ int cmd_verify_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_verify_commit_config, NULL);\n+\tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, verify_commit_options,\n \t\t\t     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex ecffb069bf1..5c00b0b0f77 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -19,11 +19,6 @@ static const char * const verify_tag_usage[] = {\n \t\tNULL\n };\n \n-static int git_verify_tag_config(const char *var, const char *value, void *cb)\n-{\n-\treturn git_default_config(var, value, cb);\n-}\n-\n int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n {\n \tint i = 1, verbose = 0, had_error = 0;\n@@ -36,7 +31,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_verify_tag_config, NULL);\n+\tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, verify_tag_options,\n \t\t\t     verify_tag_usage, PARSE_OPT_KEEP_ARGV0);\n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471859","messageId":"patch-2.2-c099d48b4bf-20230209T142225Z-avarab@gmail.com","threadId":"59218","inReplyTo":"cover-0.2-00000000000-20230209T142225Z-avarab@gmail.com","subject":"[PATCH 2/2] gpg-interface.c: lazily get GPG config variables on demand","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-09T14:35:06Z","receivedAt":"2023-02-09T14:35:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding commit we started calling gpg_interface_lazy_init()\nwhen the interface is used, in order to lazily init config.\n\nParts of the git_gpg_config() we were left with then are going to be\nharder to convert to the configset API. E.g. in the case of\n\"configured_signing_key\" the set_signing_key() and get_signing_key()\nwill modify our global variable, assigning either the config to it, a\nuser-supplied key (see \"keyid\" in builtin/tag.c). To avoid the global\nwe'd need to pass that \"keyid\" all the way down to the callbacks in\n\"struct gpg_format\".\n\nBut in the cases being changed here we can move the reading of the\nconfig variable to be adjacent to its use.\n\nAs with the preceding change this isn't without its downsides, just as\nin the preceding commit this stopped being an immediate error, and\ninstead depends on whether we'll reach something that lazily inits the\nGPG config:\n\n\tgit -c gpg.mintrustlevel=bad show --show-signature\n\nThis change likewise defers our initialization of these variables even\nfurther. But this should be OK, it's the common pattern for most other\nconfig we read.\n\nAt this point we could remove gpg_interface_lazy_init() from\ncheck_signature(), as it only uses gpg.minTrustLevel, and calls\nfunctions that don't need the lazy config, but let's keep to\nfuture-proof changes to the API that may need the initialization at a\ndistance.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n gpg-interface.c | 66 +++++++++++++++++++------------------------------\n 1 file changed, 25 insertions(+), 41 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 404d4cccf34..ab24ed3c57b 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -22,8 +22,6 @@ static void gpg_interface_lazy_init(void)\n }\n \n static char *configured_signing_key;\n-static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file;\n-static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;\n \n struct gpg_format {\n \tconst char *name;\n@@ -453,6 +451,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \tstruct strbuf ssh_keygen_out = STRBUF_INIT;\n \tstruct strbuf ssh_keygen_err = STRBUF_INIT;\n \tstruct strbuf verify_time = STRBUF_INIT;\n+\tchar *ssh_allowed_signers;\n \tconst struct date_mode verify_date_mode = {\n \t\t.type = DATE_STRFTIME,\n \t\t.strftime_fmt = \"%Y%m%d%H%M%S\",\n@@ -460,7 +459,8 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t.local = 1,\n \t};\n \n-\tif (!ssh_allowed_signers) {\n+\tif (git_config_get_string(\"gpg.ssh.allowedsignersfile\",\n+\t\t\t\t  &ssh_allowed_signers)) {\n \t\terror(_(\"gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification\"));\n \t\treturn -1;\n \t}\n@@ -520,6 +520,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t     *line;\n \t\t     line = next) {\n \t\t\tconst char *end_of_text;\n+\t\t\tchar *ssh_revocation_file;\n \n \t\t\tnext = end_of_text = strchrnul(line, '\\n');\n \n@@ -556,7 +557,8 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\t\t     verify_time.buf,\n \t\t\t\t     NULL);\n \n-\t\t\tif (ssh_revocation_file) {\n+\t\t\tif (!git_config_get_pathname(\"gpg.ssh.revocationfile\",\n+\t\t\t\t\t\t     (const char **)&ssh_revocation_file)) {\n \t\t\t\tif (file_exists(ssh_revocation_file)) {\n \t\t\t\t\tstrvec_pushl(&ssh_keygen.args, \"-r\",\n \t\t\t\t\t\t     ssh_revocation_file, NULL);\n@@ -564,6 +566,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\t\t\twarning(_(\"ssh signing revocation file configured but not found: %s\"),\n \t\t\t\t\t\tssh_revocation_file);\n \t\t\t\t}\n+\t\t\t\tfree(ssh_revocation_file);\n \t\t\t}\n \n \t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n@@ -599,6 +602,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \tstrbuf_release(&ssh_keygen_out);\n \tstrbuf_release(&ssh_keygen_err);\n \tstrbuf_release(&verify_time);\n+\tfree(ssh_allowed_signers);\n \n \treturn ret;\n }\n@@ -643,6 +647,8 @@ int check_signature(struct signature_check *sigc,\n {\n \tstruct gpg_format *fmt;\n \tint status;\n+\tstatic enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;\n+\tconst char *value;\n \n \tgpg_interface_lazy_init();\n \n@@ -661,6 +667,19 @@ int check_signature(struct signature_check *sigc,\n \tif (status && !sigc->output)\n \t\treturn !!status;\n \n+\tif (!git_config_get_string_tmp(\"gpg.mintrustlevel\", &value)) {\n+\t\tchar *trust;\n+\t\tint ret;\n+\n+\t\ttrust = xstrdup_toupper(value);\n+\t\tret = parse_gpg_trust_level(trust, &configured_min_trust_level);\n+\t\tfree(trust);\n+\n+\t\tif (ret)\n+\t\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n+\t\t\t\t     \"gpg.mintrustlevel\", value);\n+\t}\n+\n \tstatus |= sigc->result != 'G';\n \tstatus |= sigc->trust_level < configured_min_trust_level;\n \n@@ -719,8 +738,6 @@ static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)\n {\n \tstruct gpg_format *fmt = NULL;\n \tchar *fmtname = NULL;\n-\tchar *trust;\n-\tint ret;\n \n \tif (!strcmp(var, \"user.signingkey\")) {\n \t\tif (!value)\n@@ -740,38 +757,6 @@ static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"gpg.mintrustlevel\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\n-\t\ttrust = xstrdup_toupper(value);\n-\t\tret = parse_gpg_trust_level(trust, &configured_min_trust_level);\n-\t\tfree(trust);\n-\n-\t\tif (ret)\n-\t\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n-\t\t\t\t     var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"gpg.ssh.defaultkeycommand\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\treturn git_config_string(&ssh_default_key_command, var, value);\n-\t}\n-\n-\tif (!strcmp(var, \"gpg.ssh.allowedsignersfile\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\treturn git_config_pathname(&ssh_allowed_signers, var, value);\n-\t}\n-\n-\tif (!strcmp(var, \"gpg.ssh.revocationfile\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\treturn git_config_pathname(&ssh_revocation_file, var, value);\n-\t}\n-\n \tif (!strcmp(var, \"gpg.program\") || !strcmp(var, \"gpg.openpgp.program\"))\n \t\tfmtname = \"openpgp\";\n \n@@ -851,16 +836,15 @@ static const char *get_default_ssh_signing_key(void)\n \tint ret = -1;\n \tstruct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;\n \tstruct strbuf **keys;\n-\tchar *key_command = NULL;\n+\tchar *key_command;\n \tconst char **argv;\n \tint n;\n \tchar *default_key = NULL;\n \tconst char *literal_key = NULL;\n \n-\tif (!ssh_default_key_command)\n+\tif (git_config_get_string(\"gpg.ssh.defaultkeycommand\", &key_command))\n \t\tdie(_(\"either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured\"));\n \n-\tkey_command = xstrdup(ssh_default_key_command);\n \tn = split_cmdline(key_command, &argv);\n \n \tif (n < 0)\n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471878","messageId":"xmqqlel6mswo.fsf@gitster.g","threadId":"59218","inReplyTo":"cover-0.2-00000000000-20230209T142225Z-avarab@gmail.com","subject":"Re: [PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-09T21:27:03Z","receivedAt":"2023-02-09T21:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> On Thu, Feb 09 2023, Jeff King wrote:\n>\n>> If the gpg code used git_config_get_string(), etc, then they could just\n>> access each key on demand (efficiently, from an internal hash table),\n>> which reduces the risk of \"oops, we forgot to initialize the config\n>> here\". It does probably mean restructuring the code a little, though\n>> (since you'd often have an accessor function to get \"foo.bar\" rather\n>> than assuming \"foo.bar\" was parsed into an enum already, etc). That may\n>> not be worth the effort (and risk of regression) to convert.\n>\n> I'd already played around with that a bit as part of reviewing Junio's\n> change, this goes on top of that.\n\nWhat's your intention of sending these?  I think we are already in\nagreement that the churn may not be worth the risk, so if these are\n\"and here is the churn would look like, not for application\", I\nwould understand it and appreciate it.  But did you mean that these\npatches are for application?  I am not sure...\n\nThanks.\n"},{"id":"471910","messageId":"230210.86mt5lx0bq.gmgdl@evledraar.gmail.com","threadId":"59218","inReplyTo":"xmqqlel6mswo.fsf@gitster.g","subject":"Re: [PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-10T10:29:31Z","receivedAt":"2023-02-10T10:49:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 09 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> On Thu, Feb 09 2023, Jeff King wrote:\n>>\n>>> If the gpg code used git_config_get_string(), etc, then they could just\n>>> access each key on demand (efficiently, from an internal hash table),\n>>> which reduces the risk of \"oops, we forgot to initialize the config\n>>> here\". It does probably mean restructuring the code a little, though\n>>> (since you'd often have an accessor function to get \"foo.bar\" rather\n>>> than assuming \"foo.bar\" was parsed into an enum already, etc). That may\n>>> not be worth the effort (and risk of regression) to convert.\n>>\n>> I'd already played around with that a bit as part of reviewing Junio's\n>> change, this goes on top of that.\n>\n> What's your intention of sending these?\n\nFor them to be picked up on top of your jc/gpg-lazy-init.\n\n> I think we are already in\n> agreement that the churn may not be worth the risk, so if these are\n> \"and here is the churn would look like, not for application\", I\n> would understand it and appreciate it.  But did you mean that these\n> patches are for application?  I am not sure...\n\nI understood your \"I specifically did not want anybody to start doing\nthis line of analysis\" in [1] to mean that you didn't want to have the\nsort of change that the last paragraph of 2/2 notes that we're\ndeliberately not doing.\n\nI.e. that we'd like to keep the gpg_interface_lazy_init() boilerplate,\neven though we might carefully reason that a specific API entry point\nwon't need to initialize the file-scoped config variables right now.\n\nI then took your \"it is vastly preferred not to do such a change in this\nstep\" in [2] as a note that it was deliberate that the change in 1/2\nhere wasn't part of your jc/gpg-lazy-init, but not that we shouldn't\nfollow-up with such a clean-up.\n\nThe \"on top once the dust settled\" in [2] can then be addressed by\ngraduating your jc/gpg-lazy-init soon, and keeping this in \"seen\" for a\nbit, although I think the changes here (and in particular 1/2) are\ntrivial enough to graduate soon thereafter.\n\nGiven that I had mixed feelings about submitting this now, but Jeff's\n[3] convinced me. I.e. the change in 2/2 'reduces the risk of \"oops, we\nforgot to initialize the config here\"' in the future.\n\nBut obviously it's up to you whether you pick this up, and you don't\nseem especially keen on doing so, so if not I guess we'll just drop\nthis, but I'd be happy if you did.\n\nI do think that the 2/2 here has the added benefit of making your change\neasier to review, and that's why I wrote it initially. I was poking at\nyour patch to see what behavior changes, logic errors or bugs I could\nfind in it.\n\nI.e. your end state is that we're reading 7 config variables (I'm\ncounting the *.program ones as one variable). The 2/2 here brings that\ndown to just 3. Thus the surface area of potential issues where we don't\ncall gpg_interface_lazy_init() before accessing the values is reduced.\n\nWhich is also I why I opted to send this sooner than later, having that\nas a review aid helps others now, and not in a few months.\n\n1. https://lore.kernel.org/git/xmqq5ycbpp8a.fsf@gitster.g/\n2. https://lore.kernel.org/git/xmqqpmaimvtd.fsf_-_@gitster.g/\n3. https://lore.kernel.org/git/Y+TqEM21o+3TGx6D@coredump.intra.peff.net/\n"},{"id":"471933","messageId":"xmqqv8k9ibtd.fsf@gitster.g","threadId":"59218","inReplyTo":"230210.86mt5lx0bq.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-10T19:02:06Z","receivedAt":"2023-02-10T19:02:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> What's your intention of sending these?\n>\n> For them to be picked up on top of your jc/gpg-lazy-init.\n>\n>> I think we are already in\n>> agreement that the churn may not be worth the risk, so if these are\n>> \"and here is the churn would look like, not for application\", I\n>> would understand it and appreciate it.  But did you mean that these\n>> patches are for application?  I am not sure...\n>\n> I understood your \"I specifically did not want anybody to start doing\n> this line of analysis\" in [1] to mean that you didn't want to have the\n> sort of change that the last paragraph of 2/2 notes that we're\n> deliberately not doing.\n\nI didn't want to see \"oh you are calling lazy_init here but you can\ndelay it even further\" kind of comments that is wrong and wastes our\ntime.  \n\n> I.e. that we'd like to keep the gpg_interface_lazy_init() boilerplate,\n> even though we might carefully reason that a specific API entry point\n> won't need to initialize the file-scoped config variables right now.\n\nIt is the complete opposite of what I meant.\n\nChanging\n\n\tgit_am_config(...) {\n\t\treturn git_default_config(...);\n\t}\n\t... \n\t\tgit_config(git_am_config);\n\nto\n\n\t/* no git_am_config() */\n\t...\n\t\tgit_config(git_default_config);\n\nis perfectly fine as a clean-up post series.\n\nIf we are moving away from git_config() callback style, and move to\ngit_config_get_*() style, the upthread already said it does not have\na good risk/benefit ratio, but if we were to do so, then we should\nnot leave some still using the callback style while others using\ngit_config_get_*(), which will lead to configuration read in a wrong\norder and easily breaking precedence rules.\n\nAnd if we were to move away completely from the callback style, then\nI do not see a point to build such a series on top of the lazy init\npatch, which is about staying with the callback style.\n\nSo, that is exactly why I asked the question after seeing it was\nmarked to apply on top of the lazy init thing, which did not make\nsense to me.\n"}]}