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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

More below.

> 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. In this case, the improvement in the
diff is so slight that it might not be worth re-using the old variable
name, but in general, it's helpful to keep in mind that the smaller
and simpler the diff, the easier the patch is to review.

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

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