Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:01:44
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra [off-list ref] 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.
quoted hunk ↗ jump to hunk
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. 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"
quoted hunk ↗ jump to hunk
+ 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