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

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.

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(+)

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

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

On 11/25/20 9:31 AM, Junio C Hamano wrote:

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

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

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

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.