Am 26.06.2014 18:50, schrieb Matthieu Moy:
> Tanay Abhra <tanayabh@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html