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

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

On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra [off-list ref] wrote:

quoted

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,

quoted

Signed-off-by: Tanay Abhra <redacted>
---
 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.

quoted

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

quoted

+       if (!git_config_get_string("imap.preformattedhtml", &value))
+               server.use_html = git_config_bool("preformattedhtml", value);

Ditto regarding error message: "imap.preformattedhtml"

quoted

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

quoted

+       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