Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string

From: Karsten Blees <karsten.blees@gmail.com>
Date: Jun 26, 2014, 23:57 UTC
Message-ID: <53ACB381.80307@gmail.com>
In-Reply-To: <vpqbntfmx81.fsf@anie.imag.fr>

Am 26.06.2014 18:50, schrieb Matthieu Moy:

Show 22 quoted lines

> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +	if (!git_config_get_string("imap.user", &value))
>> +		server.user = xstrdup(value);
>> +	if (!git_config_get_string("imap.pass", &value))
>> +		server.pass = xstrdup(value);
>> +	if (!git_config_get_string("imap.port", &value))
>> +		server.port = git_config_int("port", value);
>> +	if (!git_config_get_string("imap.tunnel", &value))
>> +		server.tunnel = xstrdup(value);
>> +	if (!git_config_get_string("imap.authmethod", &value))
>> +		server.auth_method = xstrdup(value);
> 
> Given this kind of systematic code, I find it very tempting to factor
> this with a new helper function as
> 
> ...
> git_config_get_string_dup("imap.tunnel", &server.tunnel)
> git_config_get_string_dup("imap.authmethod", &server.auth_method)
> 
> Is there any reason not to do so?
>

With a pull-style API, you no longer need global variables for everything, so IMO the helper functions should _return_ the values rather than taking an output parameter.

E.g. with helper functions as suggested here [1] we could have:

  if (git_config_get_bool("imap.preformattedhtml", 0))
    wrap_in_html(&msg);

...rather than needing an extra variable:

  int bool_value;
  git_config_get_bool("imap.preformattedhtml", &bool_value);
  if (bool_value)
    wrap_in_html(&msg);

...and specify default values along with their respective keys:

  server.ssl_verify = git_config_get_bool("imap.sslverify", 1);
  server.port = git_config_get_int("imap.port", server.use_ssl ? 993 : 143);

...rather than ~1300 lines apart (yuck):

  static struct imap_server_conf server = {
    NULL,  /* name */
    NULL,  /* tunnel */
    NULL,  /* host */
    0,     /* port */
    NULL,  /* user */
    NULL,  /* pass */
    0,     /* use_ssl */
    1,     /* ssl_verify */
    0,     /* use_html */
    NULL,  /* auth_method */
  };

Regarding xstrdup(), I think this is a remnant from the callback version, which _requires_ you to xstrdup() (the value parameter is invalidated after returning from the callback).

Side note: with the current callback design, config variables may get passed to the callback multiple times (last value wins), so each xstrdup() in current 'git_*_config' functions actually causes memory leaks (unless prefixed with 'free(my_config_var);').

[1] http://git.661346.n2.nabble.com/PATCH-v3-0-3-git-config-cache-special-querying-api-utilizing-the-cache-tp7613911p7614050.html