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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 25, 2014, 07:09 UTC
Message-ID: <CAPig+cTStpB09Fh3bAF2NcdWR9W_UMaL5qutvUuQNfEL4=GwNw@mail.gmail.com>
In-Reply-To: <1403520105-23250-3-git-send-email-tanayabh@gmail.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.

Show 39 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. 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"

Show 34 quoted lines

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

Show 18 quoted lines

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