On 6/25/2014 12:39 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@xxxxxxxxx> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>
> You may want to mention as a side-note the slight behavior change
> introduced by this patch. The original code complained about any
> unknown boolean "imap.*" key, whereas the new code does not.
>
Also, my code is error prone. Previous one had all NULL values returned
as config_non_boolean. But, now I have to add a NULL check to every strdup
in the code.
More below,
>
>> Signed-off-by: Tanay Abhra <tanayabh@xxxxxxxxx>
>> ---
>> imap-send.c | 68 ++++++++++++++++++++++++++-----------------------------------
>> 1 file changed, 29 insertions(+), 39 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 83a6ed2..87bd418 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
>>
>> static char *imap_folder;
>>
>> -static int git_imap_config(const char *key, const char *val, void *cb)
>> +static void git_imap_config(void)
>> {
>> - char imap_key[] = "imap.";
>> -
>> - if (strncmp(key, imap_key, sizeof imap_key - 1))
>> - return 0;
>> -
>> - key += sizeof imap_key - 1;
>> -
>> - /* check booleans first, and barf on others */
>> - if (!strcmp("sslverify", key))
>> - server.ssl_verify = git_config_bool(key, val);
>> - else if (!strcmp("preformattedhtml", key))
>> - server.use_html = git_config_bool(key, val);
>> - else if (!val)
>> - return config_error_nonbool(key);
>> -
>> - if (!strcmp("folder", key)) {
>> - imap_folder = xstrdup(val);
>> - } else if (!strcmp("host", key)) {
>> - if (starts_with(val, "imap:"))
>> - val += 5;
>> - else if (starts_with(val, "imaps:")) {
>> - val += 6;
>> + const char *value;
>
> Observation: If you name this variable 'val', which is the name of the
> argument to the function in the original code, you will get a slightly
> smaller and more readable diff.
Noted.
>
>> + if (!git_config_get_string("imap.sslverify", &value))
>> + server.ssl_verify = git_config_bool("sslverify", value);
>
> I realize that you are just replicating the behavior of the original
> code, but the error message emitted here for a non-bool value is less
> than desirable since it throws away context (namely, the "imap."
> prefix). You can improve the message, and help the user resolve the
> error more quickly, by presenting the full configuration key (namely,
> "imap.sslverify"). Such a change would deserve mention in the commit
> message. Alternately, it could be fixed in a follow-up patch.
>
Yes, I thought so also when writing the patch. Will change it in the next
iteration.
Thanks.
Tanay Abhra.
>> + if (!git_config_get_string("imap.preformattedhtml", &value))
>> + server.use_html = git_config_bool("preformattedhtml", value);
>
> Ditto regarding error message: "imap.preformattedhtml"
>
>> + if (!git_config_get_string("imap.folder", &value))
>> + imap_folder = xstrdup(value);
>> + if (!git_config_get_string("imap.host", &value)) {
>> + if (starts_with(value, "imap:"))
>> + value += 5;
>> + else if (starts_with(value, "imaps:")) {
>> + value += 6;
>> server.use_ssl = 1;
>> }
>> - if (starts_with(val, "//"))
>> - val += 2;
>> - server.host = xstrdup(val);
>> - } else if (!strcmp("user", key))
>> - server.user = xstrdup(val);
>> - else if (!strcmp("pass", key))
>> - server.pass = xstrdup(val);
>> - else if (!strcmp("port", key))
>> - server.port = git_config_int(key, val);
>> - else if (!strcmp("tunnel", key))
>> - server.tunnel = xstrdup(val);
>> - else if (!strcmp("authmethod", key))
>> - server.auth_method = xstrdup(val);
>> -
>> - return 0;
>> + if (starts_with(value, "//"))
>> + value += 2;
>> + server.host = xstrdup(value);
>> + }
>> + 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);
>
> Same regarding diagnostic: "imap.port"
>
>> + 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);
>> }
>>
>> int main(int argc, char **argv)
>> @@ -1387,7 +1377,7 @@ int main(int argc, char **argv)
>> usage(imap_send_usage);
>>
>> setup_git_directory_gently(&nongit_ok);
>> - git_config(git_imap_config, NULL);
>> + git_imap_config();
>>
>> if (!server.port)
>> server.port = server.use_ssl ? 993 : 143;
>> --
>> 1.9.0.GIT
>
--
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