[BUG] git imap-send does not honor 'core.askpass'

[BUG] git imap-send does not honor 'core.askpass'

[Philippe Blain]

Hi all,

I've just noticed that 'git imap-send' does not look at 
the core.askpass config variable, because it does not call
git_default_config. Setting 'GIT_ASKPASS' in the environment
works, though.

I've CC'ed people that seem to have been involved in adding
credential support for imap-send (git log  --grep credential 
--grep imap-send --all-match).

Cheers,

Philippe.

[PATCH] imap-send: parse default git config

[Nicolas Morey-Chaisemartin]

git imap-send does not parse the default git config settings and thus ignore
core.askpass value.
Fix it by calling git_config(git_default_config)

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
---
  imap-send.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/imap-send.c b/imap-send.c
index 5764dd812ca7..790780b76da2 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1367,6 +1367,7 @@ static void git_imap_config(void)
  	git_config_get_int("imap.port", &server.port);
  	git_config_get_string("imap.tunnel", &server.tunnel);
  	git_config_get_string("imap.authmethod", &server.auth_method);
+	git_config(git_default_config, NULL);
  }
  
  static int append_msgs_to_imap(struct imap_server_conf *server,
-- 
2.29.2.366.gb291b0a62802

Re: [PATCH] imap-send: parse default git config

[Junio C Hamano]

Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com> writes:

> git imap-send does not parse the default git config settings and thus ignore
> core.askpass value.
> Fix it by calling git_config(git_default_config)
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
> ---
>  imap-send.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/imap-send.c b/imap-send.c
> index 5764dd812ca7..790780b76da2 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1367,6 +1367,7 @@ static void git_imap_config(void)
>  	git_config_get_int("imap.port", &server.port);
>  	git_config_get_string("imap.tunnel", &server.tunnel);
>  	git_config_get_string("imap.authmethod", &server.auth_method);
> +	git_config(git_default_config, NULL);

There are two styles of parsing configuration variables to get
values.  The way imap-send.c works is to grab individual values by
calling git_config_get_*() functions.  The other is to give a
callback function to git_config() to iterate over all configuration
variables and pick the relevant ones.

Once we start doing the latter, the existing git_config_get_*()
calls we see above should also be folded into it to avoid mixing two
styles for code clarity.

IOW, I'd expect

 (1) The call to git_imap_config() near the beginning of cmd_main()
     is changed to a call to git_config(git_imap_config, NULL);

 (2) git_imap_config() function is updated to begin like so:

        static void git_imap_config(const char *var, const char *value, void *cb)
        {
                if (!strcmp("imap.sslverify", var))
                        server.ssl_verify = git_config_bool(var, value);
                else if (!strcmp("imap.preformattedhtml", var))
                        server.ssl_verify = git_config_bool(var, value);
                else if (!strcmp("imap.preformattedhtml", var))
                        server.use_html = git_config_bool(var, value);
		...

     to parse the "imap.*" variables the function currently parses,
     and end like so:

		...
		else
			return git_default_config(var, value, cb);
		return 0;
	}

     to delegate the parsing of other configuration variables that
     ought to be read by default.

Of course you could also unify in the other direction and instead of
running git_config(git_defauilt_config, NULL), pick the exact
variables you care about (did you say askpass???).

Re: [PATCH] imap-send: parse default git config

[Nicolas Morey-Chaisemartin]

On 11/25/20 9:31 AM, Junio C Hamano wrote:
> Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com> writes:
> 
>> git imap-send does not parse the default git config settings and thus ignore
>> core.askpass value.
>> Fix it by calling git_config(git_default_config)
>>
>> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
>> ---
>>   imap-send.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 5764dd812ca7..790780b76da2 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1367,6 +1367,7 @@ static void git_imap_config(void)
>>   	git_config_get_int("imap.port", &server.port);
>>   	git_config_get_string("imap.tunnel", &server.tunnel);
>>   	git_config_get_string("imap.authmethod", &server.auth_method);
>> +	git_config(git_default_config, NULL);
> 
> There are two styles of parsing configuration variables to get
> values.  The way imap-send.c works is to grab individual values by
> calling git_config_get_*() functions.  The other is to give a
> callback function to git_config() to iterate over all configuration
> variables and pick the relevant ones.
> 

OK. I thought it wouldn't be THAT easy :)


> Of course you could also unify in the other direction and instead of
> running git_config(git_defauilt_config, NULL), pick the exact
> variables you care about (did you say askpass???).

The only one we care about for this specific case if core.askpass as user gets prompted to authenticate on his IMAP server.
So 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 ?

Nicolas

Re: [PATCH] imap-send: parse default git config

[Junio C Hamano]

Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com> writes:

>> Of course you could also unify in the other direction and instead
>> of running git_config(git_defauilt_config, NULL), pick the exact
>> variables you care about (did you say askpass???).
>
> The only one we care about for this specific case if core.askpass
> as user gets prompted to authenticate on his IMAP server.  So
> 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 ?

Yes, "the other way around" is cleaner and more desirable exactly
for that reason.  

There is an established way to ask another parser to handle
variables you do not handle yourself with the callback-style
configuration parsing.  "git grep 'return git_default_config('"
shows places that are taking advantage of the technique.  There is
an in-core collection of all the configuration (variable, value)
definitions, which is populated by reading the on-disk files just
once, and a call to git_config() iterates over this collection and
calls the callback-style parser for each (variable, value)
definition, resulting in a single pass.

The parser imap-send currently uses is based on a more recent style,
where each of the individual variables the caller is interested in
is looked up from the same in-core (variable, value) definitions.
It is easier to start writing, but does not have a good established
way to ask the more basic layer to grab things they care about, 
without doing a full git_config() call if the basic layer only has
callback-style parser.

In the longer term (read: I do *not* think it is a good idea to do
this as part of this series; I am just pointing at a future course
to make it easier to use the config API in general, not just by the
imap-send command), we probably should come up with a way to add
another config helper that grabs the same set of variables as the
callback-style config helper at the same layer to help config
parsers like the ones in imap-send.  E.g. git_default_config() is a
callback helper to grab all the basic configuration variable, and it
is suited for calling from a callback style configuration parser,
but it would be nicer to the current git_imap_config() and its
friends if there were a function they can call that is better than
"git_config(git_default_config)", which causes all the variables
that the default layer do not care about to be fed to the callback
only to be discarded.

In the far longer term (read: I do *not* think it is a good idea to
think about this for too long in the context of this series, and I
am not even sure what I speculate would be what I'd be convinced in
6 months myself), we may want to choose one style over the other.
My current inclination is to write off the targeted "what's the
value of this variable?"  style as a failed experiment (because it
exactly has the "cannot chain easily" problem) and standardise on
the callback-style parsers, but at the same time I think it is
possible to establish a good convention and set of config parsing
helpers for subcommand specific config parsers that are not
callback-style to delegate parsing of configuration variables at the
more basic layer with enough engineering effort, and it would
probably be a more desirable outcome in the longer term, resulting
in removal of the callback-style parsers.

But for now, without such support, "the other way around" would
result in a cleaner solution that is futureproof until we solve the
"in the far longer term" issue.

Thanks.