{"thread":{"id":"54706","subject":"[BUG] git imap-send does not honor 'core.askpass'","startedAt":"2020-11-24T15:20:24Z","lastAt":"2020-11-25T19:18:43Z","messageCount":5,"participants":["Philippe Blain","Nicolas Morey-Chaisemartin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"410685","messageId":"76d2be10-0c42-70f4-101c-ee15e3039821@gmail.com","threadId":"54706","inReplyTo":null,"subject":"[BUG] git imap-send does not honor 'core.askpass'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-11-24T15:20:00Z","receivedAt":"2020-11-24T15:20:24Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi all,\n\nI've just noticed that 'git imap-send' does not look at \nthe core.askpass config variable, because it does not call\ngit_default_config. Setting 'GIT_ASKPASS' in the environment\nworks, though.\n\nI've CC'ed people that seem to have been involved in adding\ncredential support for imap-send (git log --grep credential \n--grep imap-send --all-match).\n\nCheers,\n\nPhilippe.\n"},{"id":"410769","messageId":"8a21b031-fbfd-81c2-1f91-eff8c03bafb7@suse.com","threadId":"54706","inReplyTo":"76d2be10-0c42-70f4-101c-ee15e3039821@gmail.com","subject":"[PATCH] imap-send: parse default git config","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.com","sentAt":"2020-11-25T08:05:52Z","receivedAt":"2020-11-25T08:06:23Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"git imap-send does not parse the default git config settings and thus ignore\ncore.askpass value.\nFix it by calling git_config(git_default_config)\n\nReported-by: Philippe Blain \nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n imap-send.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 5764dd812ca7..790780b76da2 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1367,6 +1367,7 @@ static void git_imap_config(void)\n \tgit_config_get_int(\"imap.port\", &server.port);\n \tgit_config_get_string(\"imap.tunnel\", &server.tunnel);\n \tgit_config_get_string(\"imap.authmethod\", &server.auth_method);\n+\tgit_config(git_default_config, NULL);\n }\n \n static int append_msgs_to_imap(struct imap_server_conf *server,\n-- \n2.29.2.366.gb291b0a62802\n\n"},{"id":"410770","messageId":"xmqqh7pdg7ma.fsf@gitster.c.googlers.com","threadId":"54706","inReplyTo":"8a21b031-fbfd-81c2-1f91-eff8c03bafb7@suse.com","subject":"Re: [PATCH] imap-send: parse default git config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T08:31:41Z","receivedAt":"2020-11-25T08:32:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> git imap-send does not parse the default git config settings and thus ignore\n> core.askpass value.\n> Fix it by calling git_config(git_default_config)\n>\n> Reported-by: Philippe Blain \n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> imap-send.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index 5764dd812ca7..790780b76da2 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1367,6 +1367,7 @@ static void git_imap_config(void)\n> \tgit_config_get_int(\"imap.port\", &server.port);\n> \tgit_config_get_string(\"imap.tunnel\", &server.tunnel);\n> \tgit_config_get_string(\"imap.authmethod\", &server.auth_method);\n> +\tgit_config(git_default_config, NULL);\n\nThere are two styles of parsing configuration variables to get\nvalues. The way imap-send.c works is to grab individual values by\ncalling git_config_get_*() functions. The other is to give a\ncallback function to git_config() to iterate over all configuration\nvariables and pick the relevant ones.\n\nOnce we start doing the latter, the existing git_config_get_*()\ncalls we see above should also be folded into it to avoid mixing two\nstyles for code clarity.\n\nIOW, I'd expect\n\n (1) The call to git_imap_config() near the beginning of cmd_main()\n is changed to a call to git_config(git_imap_config, NULL);\n\n (2) git_imap_config() function is updated to begin like so:\n\n static void git_imap_config(const char *var, const char *value, void *cb)\n {\n if (!strcmp(\"imap.sslverify\", var))\n server.ssl_verify = git_config_bool(var, value);\n else if (!strcmp(\"imap.preformattedhtml\", var))\n server.ssl_verify = git_config_bool(var, value);\n else if (!strcmp(\"imap.preformattedhtml\", var))\n server.use_html = git_config_bool(var, value);\n\t\t...\n\n to parse the \"imap.*\" variables the function currently parses,\n and end like so:\n\n\t\t...\n\t\telse\n\t\t\treturn git_default_config(var, value, cb);\n\t\treturn 0;\n\t}\n\n to delegate the parsing of other configuration variables that\n ought to be read by default.\n\nOf course you could also unify in the other direction and instead of\nrunning git_config(git_defauilt_config, NULL), pick the exact\nvariables you care about (did you say askpass???).\n\n"},{"id":"410771","messageId":"9c410f99-6b9f-3d94-bd4c-8e4197b3cde3@suse.com","threadId":"54706","inReplyTo":"xmqqh7pdg7ma.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] imap-send: parse default git config","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.com","sentAt":"2020-11-25T08:43:01Z","receivedAt":"2020-11-25T08:43:32Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nOn 11/25/20 9:31 AM, Junio C Hamano wrote:\n> Nicolas Morey-Chaisemartin writes:\n> \n>> git imap-send does not parse the default git config settings and thus ignore\n>> core.askpass value.\n>> Fix it by calling git_config(git_default_config)\n>>\n>> Reported-by: Philippe Blain \n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> imap-send.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index 5764dd812ca7..790780b76da2 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -1367,6 +1367,7 @@ static void git_imap_config(void)\n>> \tgit_config_get_int(\"imap.port\", &server.port);\n>> \tgit_config_get_string(\"imap.tunnel\", &server.tunnel);\n>> \tgit_config_get_string(\"imap.authmethod\", &server.auth_method);\n>> +\tgit_config(git_default_config, NULL);\n> \n> There are two styles of parsing configuration variables to get\n> values. The way imap-send.c works is to grab individual values by\n> calling git_config_get_*() functions. The other is to give a\n> callback function to git_config() to iterate over all configuration\n> variables and pick the relevant ones.\n> \n\nOK. I thought it wouldn't be THAT easy :)\n\n\n> Of course you could also unify in the other direction and instead of\n> running git_config(git_defauilt_config, NULL), pick the exact\n> variables you care about (did you say askpass???).\n\nThe only one we care about for this specific case if core.askpass as user gets prompted to authenticate on his IMAP server.\nSo picking just this one would be simpler. However isn't the other way around cleaner if we happen to depend on another \"generic/core\" setting ?\n\nNicolas\n\n"},{"id":"410794","messageId":"xmqqd001fdok.fsf@gitster.c.googlers.com","threadId":"54706","inReplyTo":"9c410f99-6b9f-3d94-bd4c-8e4197b3cde3@suse.com","subject":"Re: [PATCH] imap-send: parse default git config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T19:18:19Z","receivedAt":"2020-11-25T19:18:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n>> Of course you could also unify in the other direction and instead\n>> of running git_config(git_defauilt_config, NULL), pick the exact\n>> variables you care about (did you say askpass???).\n>\n> The only one we care about for this specific case if core.askpass\n> as user gets prompted to authenticate on his IMAP server. So\n> picking just this one would be simpler. However isn't the other\n> way around cleaner if we happen to depend on another\n> \"generic/core\" setting ?\n\nYes, \"the other way around\" is cleaner and more desirable exactly\nfor that reason. \n\nThere is an established way to ask another parser to handle\nvariables you do not handle yourself with the callback-style\nconfiguration parsing. \"git grep 'return git_default_config('\"\nshows places that are taking advantage of the technique. There is\nan in-core collection of all the configuration (variable, value)\ndefinitions, which is populated by reading the on-disk files just\nonce, and a call to git_config() iterates over this collection and\ncalls the callback-style parser for each (variable, value)\ndefinition, resulting in a single pass.\n\nThe parser imap-send currently uses is based on a more recent style,\nwhere each of the individual variables the caller is interested in\nis looked up from the same in-core (variable, value) definitions.\nIt is easier to start writing, but does not have a good established\nway to ask the more basic layer to grab things they care about, \nwithout doing a full git_config() call if the basic layer only has\ncallback-style parser.\n\nIn the longer term (read: I do *not* think it is a good idea to do\nthis as part of this series; I am just pointing at a future course\nto make it easier to use the config API in general, not just by the\nimap-send command), we probably should come up with a way to add\nanother config helper that grabs the same set of variables as the\ncallback-style config helper at the same layer to help config\nparsers like the ones in imap-send. E.g. git_default_config() is a\ncallback helper to grab all the basic configuration variable, and it\nis suited for calling from a callback style configuration parser,\nbut it would be nicer to the current git_imap_config() and its\nfriends if there were a function they can call that is better than\n\"git_config(git_default_config)\", which causes all the variables\nthat the default layer do not care about to be fed to the callback\nonly to be discarded.\n\nIn the far longer term (read: I do *not* think it is a good idea to\nthink about this for too long in the context of this series, and I\nam not even sure what I speculate would be what I'd be convinced in\n6 months myself), we may want to choose one style over the other.\nMy current inclination is to write off the targeted \"what's the\nvalue of this variable?\" style as a failed experiment (because it\nexactly has the \"cannot chain easily\" problem) and standardise on\nthe callback-style parsers, but at the same time I think it is\npossible to establish a good convention and set of config parsing\nhelpers for subcommand specific config parsers that are not\ncallback-style to delegate parsing of configuration variables at the\nmore basic layer with enough engineering effort, and it would\nprobably be a more desirable outcome in the longer term, resulting\nin removal of the callback-style parsers.\n\nBut for now, without such support, \"the other way around\" would\nresult in a cleaner solution that is futureproof until we solve the\n\"in the far longer term\" issue.\n\nThanks.\n\n\n\n"}]}