[RFC/PATCH V2] alias.c: replace git_config with git_config_get_string

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <redacted>
---
 alias.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/alias.c b/alias.c
index 5efc3d6..0fe32bc 100644
--- a/alias.c
+++ b/alias.c

@@ -1,25 +1,17 @@
 #include "cache.h"
 
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
-	if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
-		if (!v)
-			return config_error_nonbool(k);
-		alias_val = xstrdup(v);
-		return 0;
-	}
-	return 0;
-}
-
 char *alias_lookup(const char *alias)
 {
-	alias_key = alias;
-	alias_val = NULL;
-	git_config(alias_lookup_cb, NULL);
-	return alias_val;
+	const char *v;
+	char *value;
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "alias.%s", alias);
+	git_config_get_string(key.buf, &v);
+	if (!v)
+		config_error_nonbool(key.buf);
+	value = xstrdup(v);
+	strbuf_release(&key);
+	return value;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1

-- 
1.9.0.GIT

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <redacted>
---
 branch.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..c9a2a0d 100644
--- a/branch.c
+++ b/branch.c

@@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	return 0;
 }
 
-struct branch_desc_cb {
+struct branch_desc {
 	const char *config_name;
 	const char *value;
 };
 
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
-	struct branch_desc_cb *desc = cb;
-	if (strcmp(desc->config_name, var))
-		return 0;
-	free((char *)desc->value);
-	return git_config_string(&desc->value, var, value);
-}
-
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
-	struct branch_desc_cb cb;
+	const char *value = NULL;
+	struct branch_desc desc;
 	struct strbuf name = STRBUF_INIT;
 	strbuf_addf(&name, "branch.%s.description", branch_name);
-	cb.config_name = name.buf;
-	cb.value = NULL;
-	if (git_config(read_branch_desc_cb, &cb) < 0) {
+	desc.config_name = name.buf;
+	desc.value = NULL;
+	git_config_get_string(desc.config_name, &value);
+	if (git_config_string(&desc.value, desc.config_name, value) < 0) {
 		strbuf_release(&name);
 		return -1;
 	}
-	if (cb.value)
-		strbuf_addstr(buf, cb.value);
+	strbuf_addstr(buf, desc.value);
 	strbuf_release(&name);
 	return 0;
 }

-- 
1.9.0.GIT

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

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;
+
+	if (!git_config_get_string("imap.sslverify", &value))
+		server.ssl_verify = git_config_bool("sslverify", value);
+	if (!git_config_get_string("imap.preformattedhtml", &value))
+		server.use_html = git_config_bool("preformattedhtml", value);
+	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);
+	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

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <redacted>
---
 notes-utils.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c

@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
 		return NULL;
 }
 
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
 {
-	struct notes_rewrite_cfg *c = cb;
-	if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
-		c->enabled = git_config_bool(k, v);
-		return 0;
-	} else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
+	struct strbuf key = STRBUF_INIT;
+	const char *v;
+	strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
+
+	if (!git_config_get_string(key.buf, &v))
+		c->enabled = git_config_bool(key.buf, v);
+
+	if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
 		if (!v)
-			return config_error_nonbool(k);
+			config_error_nonbool("notes.rewritemode");
 		c->combine = parse_combine_notes_fn(v);
-		if (!c->combine) {
+		if (!c->combine)
 			error(_("Bad notes.rewriteMode value: '%s'"), v);
-			return 1;
-		}
-		return 0;
-	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+	}
+	if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
 		/* note that a refs/ prefix is implied in the
 		 * underlying for_each_glob_ref */
 		if (starts_with(v, "refs/notes/"))

@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
 		else
 			warning(_("Refusing to rewrite notes in %s"
 				" (outside of refs/notes/)"), v);
-		return 0;
 	}
-
-	return 0;
+	strbuf_release(&key);
 }
 
 

@@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 		c->refs_from_env = 1;
 		string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
 	}
-	git_config(notes_rewrite_config, c);
+	notes_rewrite_config(c);
 	if (!c->enabled || !c->refs->nr) {
 		string_list_clear(c->refs, 0);
 		free(c->refs);

-- 
1.9.0.GIT

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <redacted>
---
 notes.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/notes.c b/notes.c
index 5fe691d..fc92eec 100644
--- a/notes.c
+++ b/notes.c

@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 	free(globs_copy);
 }
 
-static int notes_display_config(const char *k, const char *v, void *cb)
-{
-	int *load_refs = cb;
-
-	if (*load_refs && !strcmp(k, "notes.displayref")) {
-		if (!v)
-			config_error_nonbool(k);
-		string_list_add_refs_by_glob(&display_notes_refs, v);
-	}
-
-	return 0;
-}
-
 const char *default_notes_ref(void)
 {
 	const char *notes_ref = NULL;

@@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 void init_display_notes(struct display_notes_opt *opt)
 {
 	char *display_ref_env;
+	const char *value;
 	int load_config_refs = 0;
 	display_notes_refs.strdup_strings = 1;
 

@@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)
 			load_config_refs = 1;
 	}
 
-	git_config(notes_display_config, &load_config_refs);
+	if (load_config_refs && !git_config_get_string("notes.displayref", &value)) {
+		if (!value)
+			config_error_nonbool("notes.displayref");
+		string_list_add_refs_by_glob(&display_notes_refs, value);
+	}
 
 	if (opt) {
 		struct string_list_item *item;

-- 
1.9.0.GIT

Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <redacted>
---
 pager.c | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/pager.c b/pager.c
index 8b5cbc5..96abe6d 100644
--- a/pager.c
+++ b/pager.c

@@ -6,12 +6,6 @@
 #define DEFAULT_PAGER "less"
 #endif
 
-struct pager_config {
-	const char *cmd;
-	int want;
-	char *value;
-};
-
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.

@@ -155,30 +149,22 @@ int decimal_width(int number)
 	return width;
 }
 
-static int pager_command_config(const char *var, const char *value, void *data)
-{
-	struct pager_config *c = data;
-	if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
-		int b = git_config_maybe_bool(var, value);
-		if (b >= 0)
-			c->want = b;
-		else {
-			c->want = 1;
-			c->value = xstrdup(value);
-		}
-	}
-	return 0;
-}
-
 /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
 int check_pager_config(const char *cmd)
 {
-	struct pager_config c;
-	c.cmd = cmd;
-	c.want = -1;
-	c.value = NULL;
-	git_config(pager_command_config, &c);
-	if (c.value)
-		pager_program = c.value;
-	return c.want;
+	struct strbuf key = STRBUF_INIT;
+	int want = -1;
+	const char *value = NULL;
+	strbuf_addf(&key, "pager.%s", cmd);
+	if (!git_config_get_string(key.buf, &value)) {
+		int b = git_config_maybe_bool(key.buf, value);
+		if (b >= 0)
+			want = b;
+		else
+			want = 1;
+	}
+	if (value)
+		pager_program = value;
+	strbuf_release(&key);
+	return want;
 }

-- 
1.9.0.GIT

Tanay Abhra wrote:

 alias.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

What commit are these patches against?  Are they a continuation
of the "git config cache & special querying api" series?

Thanks,
Jonathan

On 6/24/2014 4:08 AM, Jonathan Nieder wrote:

Tanay Abhra wrote:

quoted

  alias.c | 28 ++++++++++------------------
  1 file changed, 10 insertions(+), 18 deletions(-)

What commit are these patches against?  Are they a continuation
of the "git config cache & special querying api" series?

My fault, I should have marked them. You have inferred correctly,
they have been built on top of "git config cache & special" querying
api" series[1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/252329

Thanks,
Tanay.

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

If 'v' is NULL, you correctly report an error, but then fall through
and invoke xstrdup() with NULL, which invites undefined behavior [1].

[1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html

+       value = xstrdup(v);
+       strbuf_release(&key);
+       return value;

You could release the strbuf earlier, which would allow you to 'return
xstrdup(v)' and drop the 'value' variable. Perhaps you want something
like this:

    const char *v;
    struct strbuf key = STRBUF_INIT;
    strbuf_addf(&key, "alias.%s", alias);
    git_config_get_string(key.buf, &v);
    if (v)
        config_error_nonbool(key.buf);
    strbuf_release(&key);
    return v ? xstrdup(v) : NULL;

 }

 #define SPLIT_CMDLINE_BAD_ENDING 1
--
1.9.0.GIT

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

Two issues:

First, why is 'if(value)' standing by itself? Although this works, it
seems to imply that 'value' might be able to become non-NULL by some
mechanism other than the get_config_maybe_bool() call, which means
that people reading this code have to spend extra time trying to
understand the overall logic. If you follow the example of the
original code, where 'value' is only ever set when 'b < 0', then it is
obvious even to the most casual reader that 'pager_program' is
assigned only for that one condition.

Second, don't you want to xstrdup(value) when assigning to
'pager_program'? If you don't, then 'pager_program' will become a
dangling pointer when config_cache_free() is invoked.

+       strbuf_release(&key);
+       return want;
 }
--
1.9.0.GIT

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

What is the purpose of retaining this structure? Following your
changes, it is never used outside of read_branch_desc(), and
'config_name' and 'value' would be more naturally declared as
variables local to that function.

-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
-       struct branch_desc_cb *desc = cb;
-       if (strcmp(desc->config_name, var))
-               return 0;
-       free((char *)desc->value);
-       return git_config_string(&desc->value, var, value);
-}
-
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
-       struct branch_desc_cb cb;
+       const char *value = NULL;
+       struct branch_desc desc;
        struct strbuf name = STRBUF_INIT;
        strbuf_addf(&name, "branch.%s.description", branch_name);
-       cb.config_name = name.buf;
-       cb.value = NULL;
-       if (git_config(read_branch_desc_cb, &cb) < 0) {
+       desc.config_name = name.buf;
+       desc.value = NULL;
+       git_config_get_string(desc.config_name, &value);
+       if (git_config_string(&desc.value, desc.config_name, value) < 0) {

Although it works in this case, it's somewhat ugly that you ignore the
return value of git_config_get_string(), and a person reading the code
has to spend extra time digging into git_config_string() to figure out
why this is safe. If might be clearer for future readers by rephrasing
like this:

    if (git_config_get_string(desc.config_name, &value) < 0 ||
        git_config_string(&desc.value, desc.config_name, value) < 0) {

                strbuf_release(&name);
                return -1;
        }
-       if (cb.value)
-               strbuf_addstr(buf, cb.value);
+       strbuf_addstr(buf, desc.value);
        strbuf_release(&name);
        return 0;
 }
--
1.9.0.GIT

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.

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"

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

There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.

                c->combine = parse_combine_notes_fn(v);

Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.

It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.

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

Although you correctly diagnose a NULL 'value', you then invoke
string_list_add_refs_by_glob() with that NULL, which will result in a
crash.

This is not a new error. It dates back to 894a9d33 (Support showing
notes from more than one notes tree; 2010-03-12), but your rewrite
should not retain the brokenness. Whether you fix it in this patch or
a lead-in fix-up patch, the fix deserves mention in the commit
message.

+       }

        if (opt) {
                struct string_list_item *item;
--
1.9.0.GIT

On 6/25/2014 10:15 AM, Eric Sunshine wrote:

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

quoted

diff --git a/branch.c b/branch.c
index 660097b..c9a2a0d 100644
--- a/branch.c
+++ b/branch.c

@@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
        return 0;
 }

-struct branch_desc_cb {
+struct branch_desc {
        const char *config_name;
        const char *value;
 };

What is the purpose of retaining this structure? Following your
changes, it is never used outside of read_branch_desc(), and
'config_name' and 'value' would be more naturally declared as
variables local to that function.

Done. :)

quoted

-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
-       struct branch_desc_cb *desc = cb;
-       if (strcmp(desc->config_name, var))
-               return 0;
-       free((char *)desc->value);
-       return git_config_string(&desc->value, var, value);
-}
-
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
-       struct branch_desc_cb cb;
+       const char *value = NULL;
+       struct branch_desc desc;
        struct strbuf name = STRBUF_INIT;
        strbuf_addf(&name, "branch.%s.description", branch_name);
-       cb.config_name = name.buf;
-       cb.value = NULL;
-       if (git_config(read_branch_desc_cb, &cb) < 0) {
+       desc.config_name = name.buf;
+       desc.value = NULL;
+       git_config_get_string(desc.config_name, &value);
+       if (git_config_string(&desc.value, desc.config_name, value) < 0) {

Although it works in this case, it's somewhat ugly that you ignore the
return value of git_config_get_string(), and a person reading the code
has to spend extra time digging into git_config_string() to figure out
why this is safe. If might be clearer for future readers by rephrasing
like this:

    if (git_config_get_string(desc.config_name, &value) < 0 ||
        git_config_string(&desc.value, desc.config_name, value) < 0) {

Noted, also didn't the old code leak desc.value as it was xstrduped
by git_config_string()? Thanks for the review.

quoted

                strbuf_release(&name);
                return -1;
        }
-       if (cb.value)
-               strbuf_addstr(buf, cb.value);
+       strbuf_addstr(buf, desc.value);
        strbuf_release(&name);
        return 0;
 }
--
1.9.0.GIT

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

On 6/25/2014 1:24 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.

Signed-off-by: Tanay Abhra <redacted>
---
 notes-utils.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c

@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
                return NULL;
 }

-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
 {
-       struct notes_rewrite_cfg *c = cb;
-       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
-               c->enabled = git_config_bool(k, v);
-               return 0;
-       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
+       struct strbuf key = STRBUF_INIT;
+       const char *v;
+       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
+
+       if (!git_config_get_string(key.buf, &v))
+               c->enabled = git_config_bool(key.buf, v);
+
+       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
                if (!v)
-                       return config_error_nonbool(k);
+                       config_error_nonbool("notes.rewritemode");

There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.

Is this change serious enough? Can I ignore it?

quoted

                c->combine = parse_combine_notes_fn(v);

Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.

Noted.

quoted

-               if (!c->combine) {
+               if (!c->combine)
                        error(_("Bad notes.rewriteMode value: '%s'"), v);
-                       return 1;
-               }
-               return 0;
-       } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+       }
+       if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
                /* note that a refs/ prefix is implied in the
                 * underlying for_each_glob_ref */
                if (starts_with(v, "refs/notes/"))

@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
                else
                        warning(_("Refusing to rewrite notes in %s"
                                " (outside of refs/notes/)"), v);
-               return 0;
        }
-
-       return 0;
+       strbuf_release(&key);

It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.

Noted. Thanks.

quoted

 }

@@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
                c->refs_from_env = 1;
                string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
        }
-       git_config(notes_rewrite_config, c);
+       notes_rewrite_config(c);
        if (!c->enabled || !c->refs->nr) {
                string_list_clear(c->refs, 0);
                free(c->refs);
--

1.9.0.GIT

On 6/25/2014 1:36 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.

Signed-off-by: Tanay Abhra <redacted>
---
 notes.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/notes.c b/notes.c
index 5fe691d..fc92eec 100644
--- a/notes.c
+++ b/notes.c

@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
        free(globs_copy);
 }

-static int notes_display_config(const char *k, const char *v, void *cb)
-{
-       int *load_refs = cb;
-
-       if (*load_refs && !strcmp(k, "notes.displayref")) {
-               if (!v)
-                       config_error_nonbool(k);
-               string_list_add_refs_by_glob(&display_notes_refs, v);
-       }
-
-       return 0;
-}
-
 const char *default_notes_ref(void)
 {
        const char *notes_ref = NULL;

@@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 void init_display_notes(struct display_notes_opt *opt)
 {
        char *display_ref_env;
+       const char *value;
        int load_config_refs = 0;
        display_notes_refs.strdup_strings = 1;

@@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)
                        load_config_refs = 1;
        }

-       git_config(notes_display_config, &load_config_refs);
+       if (load_config_refs && !git_config_get_string("notes.displayref", &value)) {
+               if (!value)
+                       config_error_nonbool("notes.displayref");
+               string_list_add_refs_by_glob(&display_notes_refs, value);

Although you correctly diagnose a NULL 'value', you then invoke
string_list_add_refs_by_glob() with that NULL, which will result in a
crash.

This is not a new error. It dates back to 894a9d33 (Support showing
notes from more than one notes tree; 2010-03-12), but your rewrite
should not retain the brokenness. Whether you fix it in this patch or
a lead-in fix-up patch, the fix deserves mention in the commit
message.

Done. Thanks.

quoted

+       }

        if (opt) {
                struct string_list_item *item;
--
1.9.0.GIT

On 6/25/2014 9:29 AM, 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.

Signed-off-by: Tanay Abhra <redacted>
---
 pager.c | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/pager.c b/pager.c
index 8b5cbc5..96abe6d 100644
--- a/pager.c
+++ b/pager.c

@@ -6,12 +6,6 @@
 #define DEFAULT_PAGER "less"
 #endif

-struct pager_config {
-       const char *cmd;
-       int want;
-       char *value;
-};
-
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.

@@ -155,30 +149,22 @@ int decimal_width(int number)
        return width;
 }

-static int pager_command_config(const char *var, const char *value, void *data)
-{
-       struct pager_config *c = data;
-       if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
-               int b = git_config_maybe_bool(var, value);
-               if (b >= 0)
-                       c->want = b;
-               else {
-                       c->want = 1;
-                       c->value = xstrdup(value);
-               }
-       }
-       return 0;
-}
-
 /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
 int check_pager_config(const char *cmd)
 {
-       struct pager_config c;
-       c.cmd = cmd;
-       c.want = -1;
-       c.value = NULL;
-       git_config(pager_command_config, &c);
-       if (c.value)
-               pager_program = c.value;
-       return c.want;
+       struct strbuf key = STRBUF_INIT;
+       int want = -1;
+       const char *value = NULL;
+       strbuf_addf(&key, "pager.%s", cmd);
+       if (!git_config_get_string(key.buf, &value)) {
+               int b = git_config_maybe_bool(key.buf, value);
+               if (b >= 0)
+                       want = b;
+               else
+                       want = 1;
+       }
+       if (value)
+               pager_program = value;

Two issues:

First, why is 'if(value)' standing by itself? Although this works, it
seems to imply that 'value' might be able to become non-NULL by some
mechanism other than the get_config_maybe_bool() call, which means
that people reading this code have to spend extra time trying to
understand the overall logic. If you follow the example of the
original code, where 'value' is only ever set when 'b < 0', then it is
obvious even to the most casual reader that 'pager_program' is
assigned only for that one condition.

Noted.

Second, don't you want to xstrdup(value) when assigning to
'pager_program'? If you don't, then 'pager_program' will become a
dangling pointer when config_cache_free() is invoked.

Noted. Thanks.

quoted

+       strbuf_release(&key);
+       return want;
 }
--
1.9.0.GIT

On 6/25/2014 7:42 AM, 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.

Signed-off-by: Tanay Abhra <redacted>
---
 alias.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/alias.c b/alias.c
index 5efc3d6..0fe32bc 100644
--- a/alias.c
+++ b/alias.c

@@ -1,25 +1,17 @@
 #include "cache.h"

-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
-       if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
-               if (!v)
-                       return config_error_nonbool(k);
-               alias_val = xstrdup(v);
-               return 0;
-       }
-       return 0;
-}
-
 char *alias_lookup(const char *alias)
 {
-       alias_key = alias;
-       alias_val = NULL;
-       git_config(alias_lookup_cb, NULL);
-       return alias_val;
+       const char *v;
+       char *value;
+       struct strbuf key = STRBUF_INIT;
+       strbuf_addf(&key, "alias.%s", alias);
+       git_config_get_string(key.buf, &v);
+       if (!v)
+               config_error_nonbool(key.buf);

If 'v' is NULL, you correctly report an error, but then fall through
and invoke xstrdup() with NULL, which invites undefined behavior [1].

[1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html

quoted

+       value = xstrdup(v);
+       strbuf_release(&key);
+       return value;

You could release the strbuf earlier, which would allow you to 'return
xstrdup(v)' and drop the 'value' variable. Perhaps you want something
like this:

    const char *v;
    struct strbuf key = STRBUF_INIT;
    strbuf_addf(&key, "alias.%s", alias);
    git_config_get_string(key.buf, &v);
    if (v)
        config_error_nonbool(key.buf);
    strbuf_release(&key);
    return v ? xstrdup(v) : NULL;

Done. Thanks.

quoted

 }

 #define SPLIT_CMDLINE_BAD_ENDING 1
--
1.9.0.GIT

Am 25.06.2014 05:59, schrieb Eric Sunshine:

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

[...]

quoted

 /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
 int check_pager_config(const char *cmd)
 {
-       struct pager_config c;
-       c.cmd = cmd;
-       c.want = -1;
-       c.value = NULL;
-       git_config(pager_command_config, &c);
-       if (c.value)
-               pager_program = c.value;
-       return c.want;
+       struct strbuf key = STRBUF_INIT;
+       int want = -1;
+       const char *value = NULL;
+       strbuf_addf(&key, "pager.%s", cmd);
+       if (!git_config_get_string(key.buf, &value)) {
+               int b = git_config_maybe_bool(key.buf, value);
+               if (b >= 0)
+                       want = b;
+               else
+                       want = 1;
+       }
+       if (value)
+               pager_program = value;

[...]

Second, don't you want to xstrdup(value) when assigning to
'pager_program'? If you don't, then 'pager_program' will become a
dangling pointer when config_cache_free() is invoked.

I don't think that values from the global config cache should be xstrdup()ed.
After all, caching the values during the lifetime of the git process is the
entire point of the config cache, isn't it?

The only reason to call config_cache_free() is to load a _different_
configuration. In this case, however, you would also need to call the relevant
config functions again, leaking all xstrdup()ed strings.

If for some reason a config string is accessed after config_cache_free()
(which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
will continue to run with some invalid configuration). This is IMO much worse
than failing with segfault.

On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra [off-list ref] wrote:

On 6/25/2014 1:24 PM, Eric Sunshine wrote:

quoted

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.

Signed-off-by: Tanay Abhra <redacted>
---
 notes-utils.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c

@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
                return NULL;
 }

-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
 {
-       struct notes_rewrite_cfg *c = cb;
-       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
-               c->enabled = git_config_bool(k, v);
-               return 0;
-       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
+       struct strbuf key = STRBUF_INIT;
+       const char *v;
+       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
+
+       if (!git_config_get_string(key.buf, &v))
+               c->enabled = git_config_bool(key.buf, v);
+
+       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
                if (!v)
-                       return config_error_nonbool(k);
+                       config_error_nonbool("notes.rewritemode");

There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.

Is this change serious enough? Can I ignore it?

I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:

- if "notes.rewritemode" is bool, it die()s.

- if "notes.rewritemode" doesn't specify a recognized mode, it
error()s but continues

- if "notes.rewriteref" doesn't start with "refs/notes/, it warning()s
and continues

It would be nice to hear an opinion from someone more invested in the
config system.

quoted

quoted

                c->combine = parse_combine_notes_fn(v);

Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.

Noted.

quoted

quoted

-               if (!c->combine) {
+               if (!c->combine)
                        error(_("Bad notes.rewriteMode value: '%s'"), v);
-                       return 1;
-               }
-               return 0;
-       } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+       }
+       if (!c->refs_from_env && !git_config_get_string("notes.rewriteref", &v)) {
                /* note that a refs/ prefix is implied in the
                 * underlying for_each_glob_ref */
                if (starts_with(v, "refs/notes/"))

@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
                else
                        warning(_("Refusing to rewrite notes in %s"
                                " (outside of refs/notes/)"), v);
-               return 0;
        }
-
-       return 0;
+       strbuf_release(&key);

It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.

Noted. Thanks.

quoted

quoted

 }

@@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
                c->refs_from_env = 1;
                string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
        }
-       git_config(notes_rewrite_config, c);
+       notes_rewrite_config(c);
        if (!c->enabled || !c->refs->nr) {
                string_list_clear(c->refs, 0);
                free(c->refs);
--

1.9.0.GIT

On Thu, Jun 26, 2014 at 4:09 AM, Tanay Abhra [off-list ref] wrote:

On 6/25/2014 10:15 AM, Eric Sunshine wrote:

quoted

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

quoted

diff --git a/branch.c b/branch.c
index 660097b..c9a2a0d 100644
--- a/branch.c
+++ b/branch.c

@@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
-       struct branch_desc_cb cb;
+       const char *value = NULL;
+       struct branch_desc desc;
        struct strbuf name = STRBUF_INIT;
        strbuf_addf(&name, "branch.%s.description", branch_name);
-       cb.config_name = name.buf;
-       cb.value = NULL;
-       if (git_config(read_branch_desc_cb, &cb) < 0) {
+       desc.config_name = name.buf;
+       desc.value = NULL;
+       git_config_get_string(desc.config_name, &value);
+       if (git_config_string(&desc.value, desc.config_name, value) < 0) {

Although it works in this case, it's somewhat ugly that you ignore the
return value of git_config_get_string(), and a person reading the code
has to spend extra time digging into git_config_string() to figure out
why this is safe. If might be clearer for future readers by rephrasing
like this:

    if (git_config_get_string(desc.config_name, &value) < 0 ||
        git_config_string(&desc.value, desc.config_name, value) < 0) {

Noted, also didn't the old code leak desc.value as it was xstrduped
by git_config_string()? Thanks for the review.

Looks that way.

Am 29.06.2014 13:01, schrieb Eric Sunshine:

On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra [off-list ref] wrote:

quoted

On 6/25/2014 1:24 PM, Eric Sunshine wrote:

quoted

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.

Signed-off-by: Tanay Abhra <redacted>
---
 notes-utils.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c

@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
                return NULL;
 }

-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
 {
-       struct notes_rewrite_cfg *c = cb;
-       if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
-               c->enabled = git_config_bool(k, v);
-               return 0;
-       } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
+       struct strbuf key = STRBUF_INIT;
+       const char *v;
+       strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
+
+       if (!git_config_get_string(key.buf, &v))
+               c->enabled = git_config_bool(key.buf, v);
+
+       if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) {
                if (!v)
-                       return config_error_nonbool(k);
+                       config_error_nonbool("notes.rewritemode");

There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.

Is this change serious enough? Can I ignore it?