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

From: Tanay Abhra <tanayabh@gmail.com>
Date: Jun 26, 2014, 08:14 UTC
Message-ID: <53ABD677.4040808@gmail.com>
In-Reply-To: <CAPig+cTStpB09Fh3bAF2NcdWR9W_UMaL5qutvUuQNfEL4=GwNw@mail.gmail.com>

On 6/25/2014 12:39 PM, Eric Sunshine wrote:

Show 8 quoted lines

> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> 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,

Show 44 quoted lines

> 
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  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.

Show 12 quoted lines

> 
>> +       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.

Show 61 quoted lines

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