git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [BUG] git imap-send does not honor 'core.askpass'
@ 2020-11-24 15:20 Philippe Blain
  2020-11-25  8:05 ` [PATCH] imap-send: parse default git config Nicolas Morey-Chaisemartin
  0 siblings, 1 reply; 5+ messages in thread
From: Philippe Blain @ 2020-11-24 15:20 UTC (permalink / raw)
  To: git; +Cc: Nicolas Morey-Chaisemartin, Dan Albert

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] imap-send: parse default git config
  2020-11-24 15:20 [BUG] git imap-send does not honor 'core.askpass' Philippe Blain
@ 2020-11-25  8:05 ` Nicolas Morey-Chaisemartin
  2020-11-25  8:31   ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2020-11-25  8:05 UTC (permalink / raw)
  To: git; +Cc: levraiphilippeblain

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] imap-send: parse default git config
  2020-11-25  8:05 ` [PATCH] imap-send: parse default git config Nicolas Morey-Chaisemartin
@ 2020-11-25  8:31   ` Junio C Hamano
  2020-11-25  8:43     ` Nicolas Morey-Chaisemartin
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2020-11-25  8:31 UTC (permalink / raw)
  To: Nicolas Morey-Chaisemartin; +Cc: git, levraiphilippeblain

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???).


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] imap-send: parse default git config
  2020-11-25  8:31   ` Junio C Hamano
@ 2020-11-25  8:43     ` Nicolas Morey-Chaisemartin
  2020-11-25 19:18       ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2020-11-25  8:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, levraiphilippeblain



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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] imap-send: parse default git config
  2020-11-25  8:43     ` Nicolas Morey-Chaisemartin
@ 2020-11-25 19:18       ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2020-11-25 19:18 UTC (permalink / raw)
  To: Nicolas Morey-Chaisemartin; +Cc: git, levraiphilippeblain

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.




^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-25 19:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-24 15:20 [BUG] git imap-send does not honor 'core.askpass' Philippe Blain
2020-11-25  8:05 ` [PATCH] imap-send: parse default git config Nicolas Morey-Chaisemartin
2020-11-25  8:31   ` Junio C Hamano
2020-11-25  8:43     ` Nicolas Morey-Chaisemartin
2020-11-25 19:18       ` Junio C Hamano

Code repositories for project(s) associated with this 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).