{"thread":{"id":"36982","subject":"[RFC/PATCH V2] alias.c: replace git_config with git_config_get_string","startedAt":"2014-06-23T10:41:40Z","lastAt":"2014-07-01T08:36:10Z","messageCount":41,"participants":["Tanay Abhra","Jonathan Nieder","Eric Sunshine","Matthieu Moy","Karsten Blees","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"244859","messageId":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","threadId":"36982","inReplyTo":null,"subject":"[RFC/PATCH V2] alias.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:41:40Z","receivedAt":"2014-06-23T10:41:40Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use git_config_get_string instead of git_config to take advantage of\nthe config hash-table api which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra \n---\n alias.c | 28 ++++++++++------------------\n 1 file changed, 10 insertions(+), 18 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 5efc3d6..0fe32bc 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -1,25 +1,17 @@\n #include \"cache.h\"\n \n-static const char *alias_key;\n-static char *alias_val;\n-\n-static int alias_lookup_cb(const char *k, const char *v, void *cb)\n-{\n-\tif (starts_with(k, \"alias.\") && !strcmp(k + 6, alias_key)) {\n-\t\tif (!v)\n-\t\t\treturn config_error_nonbool(k);\n-\t\talias_val = xstrdup(v);\n-\t\treturn 0;\n-\t}\n-\treturn 0;\n-}\n-\n char *alias_lookup(const char *alias)\n {\n-\talias_key = alias;\n-\talias_val = NULL;\n-\tgit_config(alias_lookup_cb, NULL);\n-\treturn alias_val;\n+\tconst char *v;\n+\tchar *value;\n+\tstruct strbuf key = STRBUF_INIT;\n+\tstrbuf_addf(&key, \"alias.%s\", alias);\n+\tgit_config_get_string(key.buf, &v);\n+\tif (!v)\n+\t\tconfig_error_nonbool(key.buf);\n+\tvalue = xstrdup(v);\n+\tstrbuf_release(&key);\n+\treturn value;\n }\n \n #define SPLIT_CMDLINE_BAD_ENDING 1\n-- \n1.9.0.GIT\n"},{"id":"244860","messageId":"1403520105-23250-2-git-send-email-tanayabh@gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH V2] branch.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:41:41Z","receivedAt":"2014-06-23T10:41:41Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use git_config_get_string instead of git_config to take advantage of\nthe config hash-table api which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra \n---\n branch.c | 24 ++++++++----------------\n 1 file changed, 8 insertions(+), 16 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 660097b..c9a2a0d 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \treturn 0;\n }\n \n-struct branch_desc_cb {\n+struct branch_desc {\n \tconst char *config_name;\n \tconst char *value;\n };\n \n-static int read_branch_desc_cb(const char *var, const char *value, void *cb)\n-{\n-\tstruct branch_desc_cb *desc = cb;\n-\tif (strcmp(desc->config_name, var))\n-\t\treturn 0;\n-\tfree((char *)desc->value);\n-\treturn git_config_string(&desc->value, var, value);\n-}\n-\n int read_branch_desc(struct strbuf *buf, const char *branch_name)\n {\n-\tstruct branch_desc_cb cb;\n+\tconst char *value = NULL;\n+\tstruct branch_desc desc;\n \tstruct strbuf name = STRBUF_INIT;\n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n-\tcb.config_name = name.buf;\n-\tcb.value = NULL;\n-\tif (git_config(read_branch_desc_cb, &cb) < 0) {\n+\tdesc.config_name = name.buf;\n+\tdesc.value = NULL;\n+\tgit_config_get_string(desc.config_name, &value);\n+\tif (git_config_string(&desc.value, desc.config_name, value) < 0) {\n \t\tstrbuf_release(&name);\n \t\treturn -1;\n \t}\n-\tif (cb.value)\n-\t\tstrbuf_addstr(buf, cb.value);\n+\tstrbuf_addstr(buf, desc.value);\n \tstrbuf_release(&name);\n \treturn 0;\n }\n-- \n1.9.0.GIT\n"},{"id":"244861","messageId":"1403520105-23250-3-git-send-email-tanayabh@gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH] imap-send.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:41:42Z","receivedAt":"2014-06-23T10:41:42Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use git_config_get_string instead of git_config to take advantage of\nthe config hash-table api which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra \n---\n imap-send.c | 68 ++++++++++++++++++++++++++-----------------------------------\n 1 file changed, 29 insertions(+), 39 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 83a6ed2..87bd418 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)\n \n static char *imap_folder;\n \n-static int git_imap_config(const char *key, const char *val, void *cb)\n+static void git_imap_config(void)\n {\n-\tchar imap_key[] = \"imap.\";\n-\n-\tif (strncmp(key, imap_key, sizeof imap_key - 1))\n-\t\treturn 0;\n-\n-\tkey += sizeof imap_key - 1;\n-\n-\t/* check booleans first, and barf on others */\n-\tif (!strcmp(\"sslverify\", key))\n-\t\tserver.ssl_verify = git_config_bool(key, val);\n-\telse if (!strcmp(\"preformattedhtml\", key))\n-\t\tserver.use_html = git_config_bool(key, val);\n-\telse if (!val)\n-\t\treturn config_error_nonbool(key);\n-\n-\tif (!strcmp(\"folder\", key)) {\n-\t\timap_folder = xstrdup(val);\n-\t} else if (!strcmp(\"host\", key)) {\n-\t\tif (starts_with(val, \"imap:\"))\n-\t\t\tval += 5;\n-\t\telse if (starts_with(val, \"imaps:\")) {\n-\t\t\tval += 6;\n+\tconst char *value;\n+\n+\tif (!git_config_get_string(\"imap.sslverify\", &value))\n+\t\tserver.ssl_verify = git_config_bool(\"sslverify\", value);\n+\tif (!git_config_get_string(\"imap.preformattedhtml\", &value))\n+\t\tserver.use_html = git_config_bool(\"preformattedhtml\", value);\n+\tif (!git_config_get_string(\"imap.folder\", &value))\n+\t\timap_folder = xstrdup(value);\n+\tif (!git_config_get_string(\"imap.host\", &value)) {\n+\t\tif (starts_with(value, \"imap:\"))\n+\t\t\tvalue += 5;\n+\t\telse if (starts_with(value, \"imaps:\")) {\n+\t\t\tvalue += 6;\n \t\t\tserver.use_ssl = 1;\n \t\t}\n-\t\tif (starts_with(val, \"//\"))\n-\t\t\tval += 2;\n-\t\tserver.host = xstrdup(val);\n-\t} else if (!strcmp(\"user\", key))\n-\t\tserver.user = xstrdup(val);\n-\telse if (!strcmp(\"pass\", key))\n-\t\tserver.pass = xstrdup(val);\n-\telse if (!strcmp(\"port\", key))\n-\t\tserver.port = git_config_int(key, val);\n-\telse if (!strcmp(\"tunnel\", key))\n-\t\tserver.tunnel = xstrdup(val);\n-\telse if (!strcmp(\"authmethod\", key))\n-\t\tserver.auth_method = xstrdup(val);\n-\n-\treturn 0;\n+\t\tif (starts_with(value, \"//\"))\n+\t\t\tvalue += 2;\n+\t\tserver.host = xstrdup(value);\n+\t}\n+\tif (!git_config_get_string(\"imap.user\", &value))\n+\t\tserver.user = xstrdup(value);\n+\tif (!git_config_get_string(\"imap.pass\", &value))\n+\t\tserver.pass = xstrdup(value);\n+\tif (!git_config_get_string(\"imap.port\", &value))\n+\t\tserver.port = git_config_int(\"port\", value);\n+\tif (!git_config_get_string(\"imap.tunnel\", &value))\n+\t\tserver.tunnel = xstrdup(value);\n+\tif (!git_config_get_string(\"imap.authmethod\", &value))\n+\t\tserver.auth_method = xstrdup(value);\n }\n \n int main(int argc, char **argv)\n@@ -1387,7 +1377,7 @@ int main(int argc, char **argv)\n \t\tusage(imap_send_usage);\n \n \tsetup_git_directory_gently(&nongit_ok);\n-\tgit_config(git_imap_config, NULL);\n+\tgit_imap_config();\n \n \tif (!server.port)\n \t\tserver.port = server.use_ssl ? 993 : 143;\n-- \n1.9.0.GIT\n"},{"id":"244862","messageId":"1403520105-23250-4-git-send-email-tanayabh@gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:41:43Z","receivedAt":"2014-06-23T10:41:43Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use git_config_get_string instead of git_config to take advantage of\nthe config hash-table api which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra \n---\n notes-utils.c | 31 +++++++++++++++----------------\n 1 file changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/notes-utils.c b/notes-utils.c\nindex a0b1d7b..fdc9912 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n \t\treturn NULL;\n }\n \n-static int notes_rewrite_config(const char *k, const char *v, void *cb)\n+static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n {\n-\tstruct notes_rewrite_cfg *c = cb;\n-\tif (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n-\t\tc->enabled = git_config_bool(k, v);\n-\t\treturn 0;\n-\t} else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n+\tstruct strbuf key = STRBUF_INIT;\n+\tconst char *v;\n+\tstrbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n+\n+\tif (!git_config_get_string(key.buf, &v))\n+\t\tc->enabled = git_config_bool(key.buf, v);\n+\n+\tif (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n \t\tif (!v)\n-\t\t\treturn config_error_nonbool(k);\n+\t\t\tconfig_error_nonbool(\"notes.rewritemode\");\n \t\tc->combine = parse_combine_notes_fn(v);\n-\t\tif (!c->combine) {\n+\t\tif (!c->combine)\n \t\t\terror(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n-\t\t\treturn 1;\n-\t\t}\n-\t\treturn 0;\n-\t} else if (!c->refs_from_env && !strcmp(k, \"notes.rewriteref\")) {\n+\t}\n+\tif (!c->refs_from_env && !git_config_get_string(\"notes.rewriteref\", &v)) {\n \t\t/* note that a refs/ prefix is implied in the\n \t\t * underlying for_each_glob_ref */\n \t\tif (starts_with(v, \"refs/notes/\"))\n@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)\n \t\telse\n \t\t\twarning(_(\"Refusing to rewrite notes in %s\"\n \t\t\t\t\" (outside of refs/notes/)\"), v);\n-\t\treturn 0;\n \t}\n-\n-\treturn 0;\n+\tstrbuf_release(&key);\n }\n \n \n@@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n \t\tc->refs_from_env = 1;\n \t\tstring_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);\n \t}\n-\tgit_config(notes_rewrite_config, c);\n+\tnotes_rewrite_config(c);\n \tif (!c->enabled || !c->refs->nr) {\n \t\tstring_list_clear(c->refs, 0);\n \t\tfree(c->refs);\n-- \n1.9.0.GIT\n"},{"id":"244863","messageId":"1403520105-23250-5-git-send-email-tanayabh@gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH] notes.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:41:44Z","receivedAt":"2014-06-23T10:41:44Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use git_config_get_string instead of git_config to take advantage of\nthe config hash-table api which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra \n---\n notes.c | 20 ++++++--------------\n 1 file changed, 6 insertions(+), 14 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 5fe691d..fc92eec 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n \tfree(globs_copy);\n }\n \n-static int notes_display_config(const char *k, const char *v, void *cb)\n-{\n-\tint *load_refs = cb;\n-\n-\tif (*load_refs && !strcmp(k, \"notes.displayref\")) {\n-\t\tif (!v)\n-\t\t\tconfig_error_nonbool(k);\n-\t\tstring_list_add_refs_by_glob(&display_notes_refs, v);\n-\t}\n-\n-\treturn 0;\n-}\n-\n const char *default_notes_ref(void)\n {\n \tconst char *notes_ref = NULL;\n@@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n void init_display_notes(struct display_notes_opt *opt)\n {\n \tchar *display_ref_env;\n+\tconst char *value;\n \tint load_config_refs = 0;\n \tdisplay_notes_refs.strdup_strings = 1;\n \n@@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)\n \t\t\tload_config_refs = 1;\n \t}\n \n-\tgit_config(notes_display_config, &load_config_refs);\n+\tif (load_config_refs && !git_config_get_string(\"notes.displayref\", &value)) {\n+\t\tif (!value)\n+\t\t\tconfig_error_nonbool(\"notes.displayref\");\n+\t\tstring_list_add_refs_by_glob(&display_notes_refs, value);\n+\t}\n \n \tif (opt) {\n \t\tstruct string_list_item *item;\n-- \n1.9.0.GIT\n"},{"id":"244864","messageId":"1403520105-23250-6-git-send-email-tanayabh@gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:41:45Z","receivedAt":"2014-06-23T10:41:45Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use git_config_get_string instead of git_config to take advantage of\nthe config hash-table api which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra \n---\n pager.c | 44 +++++++++++++++-----------------------------\n 1 file changed, 15 insertions(+), 29 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 8b5cbc5..96abe6d 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -6,12 +6,6 @@\n #define DEFAULT_PAGER \"less\"\n #endif\n \n-struct pager_config {\n-\tconst char *cmd;\n-\tint want;\n-\tchar *value;\n-};\n-\n /*\n * This is split up from the rest of git so that we can do\n * something different on Windows.\n@@ -155,30 +149,22 @@ int decimal_width(int number)\n \treturn width;\n }\n \n-static int pager_command_config(const char *var, const char *value, void *data)\n-{\n-\tstruct pager_config *c = data;\n-\tif (starts_with(var, \"pager.\") && !strcmp(var + 6, c->cmd)) {\n-\t\tint b = git_config_maybe_bool(var, value);\n-\t\tif (b >= 0)\n-\t\t\tc->want = b;\n-\t\telse {\n-\t\t\tc->want = 1;\n-\t\t\tc->value = xstrdup(value);\n-\t\t}\n-\t}\n-\treturn 0;\n-}\n-\n /* returns 0 for \"no pager\", 1 for \"use pager\", and -1 for \"not specified\" */\n int check_pager_config(const char *cmd)\n {\n-\tstruct pager_config c;\n-\tc.cmd = cmd;\n-\tc.want = -1;\n-\tc.value = NULL;\n-\tgit_config(pager_command_config, &c);\n-\tif (c.value)\n-\t\tpager_program = c.value;\n-\treturn c.want;\n+\tstruct strbuf key = STRBUF_INIT;\n+\tint want = -1;\n+\tconst char *value = NULL;\n+\tstrbuf_addf(&key, \"pager.%s\", cmd);\n+\tif (!git_config_get_string(key.buf, &value)) {\n+\t\tint b = git_config_maybe_bool(key.buf, value);\n+\t\tif (b >= 0)\n+\t\t\twant = b;\n+\t\telse\n+\t\t\twant = 1;\n+\t}\n+\tif (value)\n+\t\tpager_program = value;\n+\tstrbuf_release(&key);\n+\treturn want;\n }\n-- \n1.9.0.GIT\n"},{"id":"244898","messageId":"20140623223825.GC4445@google.com","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-23T22:38:25Z","receivedAt":"2014-06-23T22:38:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Tanay Abhra wrote:\n\n> alias.c | 28 ++++++++++------------------\n> 1 file changed, 10 insertions(+), 18 deletions(-)\n\nWhat commit are these patches against? Are they a continuation\nof the \"git config cache & special querying api\" series?\n\nThanks,\nJonathan\n"},{"id":"244907","messageId":"53A8D955.9050200@gmail.com","threadId":"36982","inReplyTo":"20140623223825.GC4445@google.com","subject":"Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-24T01:50:13Z","receivedAt":"2014-06-24T01:50:13Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 6/24/2014 4:08 AM, Jonathan Nieder wrote:\n> Tanay Abhra wrote:\n>\n>> alias.c | 28 ++++++++++------------------\n>> 1 file changed, 10 insertions(+), 18 deletions(-)\n>\n> What commit are these patches against? Are they a continuation\n> of the \"git config cache & special querying api\" series?\n>\n\nMy fault, I should have marked them. You have inferred correctly,\nthey have been built on top of \"git config cache & special\" querying\napi\" series[1].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/252329\n\nThanks,\nTanay.\n"},{"id":"244955","messageId":"CAPig+cQ3hzFFRRhbDTddArBC0nB9gZ7yMXuh35GHe73x-1JDng@mail.gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T02:12:09Z","receivedAt":"2014-06-25T02:12:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n> Use git_config_get_string instead of git_config to take advantage of\n> the config hash-table api which provides a cleaner control flow.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> alias.c | 28 ++++++++++------------------\n> 1 file changed, 10 insertions(+), 18 deletions(-)\n>\n> diff --git a/alias.c b/alias.c\n> index 5efc3d6..0fe32bc 100644\n> --- a/alias.c\n> +++ b/alias.c\n> @@ -1,25 +1,17 @@\n> #include \"cache.h\"\n>\n> -static const char *alias_key;\n> -static char *alias_val;\n> -\n> -static int alias_lookup_cb(const char *k, const char *v, void *cb)\n> -{\n> - if (starts_with(k, \"alias.\") && !strcmp(k + 6, alias_key)) {\n> - if (!v)\n> - return config_error_nonbool(k);\n> - alias_val = xstrdup(v);\n> - return 0;\n> - }\n> - return 0;\n> -}\n> -\n> char *alias_lookup(const char *alias)\n> {\n> - alias_key = alias;\n> - alias_val = NULL;\n> - git_config(alias_lookup_cb, NULL);\n> - return alias_val;\n> + const char *v;\n> + char *value;\n> + struct strbuf key = STRBUF_INIT;\n> + strbuf_addf(&key, \"alias.%s\", alias);\n> + git_config_get_string(key.buf, &v);\n> + if (!v)\n> + config_error_nonbool(key.buf);\n\nIf 'v' is NULL, you correctly report an error, but then fall through\nand invoke xstrdup() with NULL, which invites undefined behavior [1].\n\n[1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html\n\n> + value = xstrdup(v);\n> + strbuf_release(&key);\n> + return value;\n\nYou could release the strbuf earlier, which would allow you to 'return\nxstrdup(v)' and drop the 'value' variable. Perhaps you want something\nlike this:\n\n const char *v;\n struct strbuf key = STRBUF_INIT;\n strbuf_addf(&key, \"alias.%s\", alias);\n git_config_get_string(key.buf, &v);\n if (v)\n config_error_nonbool(key.buf);\n strbuf_release(&key);\n return v ? xstrdup(v) : NULL;\n\n> }\n>\n> #define SPLIT_CMDLINE_BAD_ENDING 1\n> --\n> 1.9.0.GIT\n"},{"id":"244956","messageId":"CAPig+cTwBB8bmKDtdv8zPojR+6Kyu6fYB7Sw0-UJoSHFLQX+fQ@mail.gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-6-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T03:59:19Z","receivedAt":"2014-06-25T03:59:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n> Use git_config_get_string instead of git_config to take advantage of\n> the config hash-table api which provides a cleaner control flow.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> pager.c | 44 +++++++++++++++-----------------------------\n> 1 file changed, 15 insertions(+), 29 deletions(-)\n>\n> diff --git a/pager.c b/pager.c\n> index 8b5cbc5..96abe6d 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -6,12 +6,6 @@\n> #define DEFAULT_PAGER \"less\"\n> #endif\n>\n> -struct pager_config {\n> - const char *cmd;\n> - int want;\n> - char *value;\n> -};\n> -\n> /*\n> * This is split up from the rest of git so that we can do\n> * something different on Windows.\n> @@ -155,30 +149,22 @@ int decimal_width(int number)\n> return width;\n> }\n>\n> -static int pager_command_config(const char *var, const char *value, void *data)\n> -{\n> - struct pager_config *c = data;\n> - if (starts_with(var, \"pager.\") && !strcmp(var + 6, c->cmd)) {\n> - int b = git_config_maybe_bool(var, value);\n> - if (b >= 0)\n> - c->want = b;\n> - else {\n> - c->want = 1;\n> - c->value = xstrdup(value);\n> - }\n> - }\n> - return 0;\n> -}\n> -\n> /* returns 0 for \"no pager\", 1 for \"use pager\", and -1 for \"not specified\" */\n> int check_pager_config(const char *cmd)\n> {\n> - struct pager_config c;\n> - c.cmd = cmd;\n> - c.want = -1;\n> - c.value = NULL;\n> - git_config(pager_command_config, &c);\n> - if (c.value)\n> - pager_program = c.value;\n> - return c.want;\n> + struct strbuf key = STRBUF_INIT;\n> + int want = -1;\n> + const char *value = NULL;\n> + strbuf_addf(&key, \"pager.%s\", cmd);\n> + if (!git_config_get_string(key.buf, &value)) {\n> + int b = git_config_maybe_bool(key.buf, value);\n> + if (b >= 0)\n> + want = b;\n> + else\n> + want = 1;\n> + }\n> + if (value)\n> + pager_program = value;\n\nTwo issues:\n\nFirst, why is 'if(value)' standing by itself? Although this works, it\nseems to imply that 'value' might be able to become non-NULL by some\nmechanism other than the get_config_maybe_bool() call, which means\nthat people reading this code have to spend extra time trying to\nunderstand the overall logic. If you follow the example of the\noriginal code, where 'value' is only ever set when 'b < 0', then it is\nobvious even to the most casual reader that 'pager_program' is\nassigned only for that one condition.\n\nSecond, don't you want to xstrdup(value) when assigning to\n'pager_program'? If you don't, then 'pager_program' will become a\ndangling pointer when config_cache_free() is invoked.\n\n> + strbuf_release(&key);\n> + return want;\n> }\n> --\n> 1.9.0.GIT\n"},{"id":"244957","messageId":"CAPig+cTMWKtAjN931voVs7aY7JdEyaRVZj+-qDUiDk0C_Tq6-A@mail.gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-2-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T04:45:14Z","receivedAt":"2014-06-25T04:45:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n> Use git_config_get_string instead of git_config to take advantage of\n> the config hash-table api which provides a cleaner control flow.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> branch.c | 24 ++++++++----------------\n> 1 file changed, 8 insertions(+), 16 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 660097b..c9a2a0d 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n> return 0;\n> }\n>\n> -struct branch_desc_cb {\n> +struct branch_desc {\n> const char *config_name;\n> const char *value;\n> };\n\nWhat is the purpose of retaining this structure? Following your\nchanges, it is never used outside of read_branch_desc(), and\n'config_name' and 'value' would be more naturally declared as\nvariables local to that function.\n\n> -static int read_branch_desc_cb(const char *var, const char *value, void *cb)\n> -{\n> - struct branch_desc_cb *desc = cb;\n> - if (strcmp(desc->config_name, var))\n> - return 0;\n> - free((char *)desc->value);\n> - return git_config_string(&desc->value, var, value);\n> -}\n> -\n> int read_branch_desc(struct strbuf *buf, const char *branch_name)\n> {\n> - struct branch_desc_cb cb;\n> + const char *value = NULL;\n> + struct branch_desc desc;\n> struct strbuf name = STRBUF_INIT;\n> strbuf_addf(&name, \"branch.%s.description\", branch_name);\n> - cb.config_name = name.buf;\n> - cb.value = NULL;\n> - if (git_config(read_branch_desc_cb, &cb) < 0) {\n> + desc.config_name = name.buf;\n> + desc.value = NULL;\n> + git_config_get_string(desc.config_name, &value);\n> + if (git_config_string(&desc.value, desc.config_name, value) < 0) {\n\nAlthough it works in this case, it's somewhat ugly that you ignore the\nreturn value of git_config_get_string(), and a person reading the code\nhas to spend extra time digging into git_config_string() to figure out\nwhy this is safe. If might be clearer for future readers by rephrasing\nlike this:\n\n if (git_config_get_string(desc.config_name, &value) < 0 ||\n git_config_string(&desc.value, desc.config_name, value) < 0) {\n\n> strbuf_release(&name);\n> return -1;\n> }\n> - if (cb.value)\n> - strbuf_addstr(buf, cb.value);\n> + strbuf_addstr(buf, desc.value);\n> strbuf_release(&name);\n> return 0;\n> }\n> --\n> 1.9.0.GIT\n"},{"id":"244959","messageId":"CAPig+cTStpB09Fh3bAF2NcdWR9W_UMaL5qutvUuQNfEL4=GwNw@mail.gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-3-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T07:09:18Z","receivedAt":"2014-06-25T07:09:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n> Use git_config_get_string instead of git_config to take advantage of\n> the config hash-table api which provides a cleaner control flow.\n\nYou may want to mention as a side-note the slight behavior change\nintroduced by this patch. The original code complained about any\nunknown boolean \"imap.*\" key, whereas the new code does not.\n\nMore below.\n\n> Signed-off-by: Tanay Abhra \n> ---\n> imap-send.c | 68 ++++++++++++++++++++++++++-----------------------------------\n> 1 file changed, 29 insertions(+), 39 deletions(-)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index 83a6ed2..87bd418 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)\n>\n> static char *imap_folder;\n>\n> -static int git_imap_config(const char *key, const char *val, void *cb)\n> +static void git_imap_config(void)\n> {\n> - char imap_key[] = \"imap.\";\n> -\n> - if (strncmp(key, imap_key, sizeof imap_key - 1))\n> - return 0;\n> -\n> - key += sizeof imap_key - 1;\n> -\n> - /* check booleans first, and barf on others */\n> - if (!strcmp(\"sslverify\", key))\n> - server.ssl_verify = git_config_bool(key, val);\n> - else if (!strcmp(\"preformattedhtml\", key))\n> - server.use_html = git_config_bool(key, val);\n> - else if (!val)\n> - return config_error_nonbool(key);\n> -\n> - if (!strcmp(\"folder\", key)) {\n> - imap_folder = xstrdup(val);\n> - } else if (!strcmp(\"host\", key)) {\n> - if (starts_with(val, \"imap:\"))\n> - val += 5;\n> - else if (starts_with(val, \"imaps:\")) {\n> - val += 6;\n> + const char *value;\n\nObservation: If you name this variable 'val', which is the name of the\nargument to the function in the original code, you will get a slightly\nsmaller and more readable diff. In this case, the improvement in the\ndiff is so slight that it might not be worth re-using the old variable\nname, but in general, it's helpful to keep in mind that the smaller\nand simpler the diff, the easier the patch is to review.\n\n> + if (!git_config_get_string(\"imap.sslverify\", &value))\n> + server.ssl_verify = git_config_bool(\"sslverify\", value);\n\nI realize that you are just replicating the behavior of the original\ncode, but the error message emitted here for a non-bool value is less\nthan desirable since it throws away context (namely, the \"imap.\"\nprefix). You can improve the message, and help the user resolve the\nerror more quickly, by presenting the full configuration key (namely,\n\"imap.sslverify\"). Such a change would deserve mention in the commit\nmessage. Alternately, it could be fixed in a follow-up patch.\n\n> + if (!git_config_get_string(\"imap.preformattedhtml\", &value))\n> + server.use_html = git_config_bool(\"preformattedhtml\", value);\n\nDitto regarding error message: \"imap.preformattedhtml\"\n\n> + if (!git_config_get_string(\"imap.folder\", &value))\n> + imap_folder = xstrdup(value);\n> + if (!git_config_get_string(\"imap.host\", &value)) {\n> + if (starts_with(value, \"imap:\"))\n> + value += 5;\n> + else if (starts_with(value, \"imaps:\")) {\n> + value += 6;\n> server.use_ssl = 1;\n> }\n> - if (starts_with(val, \"//\"))\n> - val += 2;\n> - server.host = xstrdup(val);\n> - } else if (!strcmp(\"user\", key))\n> - server.user = xstrdup(val);\n> - else if (!strcmp(\"pass\", key))\n> - server.pass = xstrdup(val);\n> - else if (!strcmp(\"port\", key))\n> - server.port = git_config_int(key, val);\n> - else if (!strcmp(\"tunnel\", key))\n> - server.tunnel = xstrdup(val);\n> - else if (!strcmp(\"authmethod\", key))\n> - server.auth_method = xstrdup(val);\n> -\n> - return 0;\n> + if (starts_with(value, \"//\"))\n> + value += 2;\n> + server.host = xstrdup(value);\n> + }\n> + if (!git_config_get_string(\"imap.user\", &value))\n> + server.user = xstrdup(value);\n> + if (!git_config_get_string(\"imap.pass\", &value))\n> + server.pass = xstrdup(value);\n> + if (!git_config_get_string(\"imap.port\", &value))\n> + server.port = git_config_int(\"port\", value);\n\nSame regarding diagnostic: \"imap.port\"\n\n> + if (!git_config_get_string(\"imap.tunnel\", &value))\n> + server.tunnel = xstrdup(value);\n> + if (!git_config_get_string(\"imap.authmethod\", &value))\n> + server.auth_method = xstrdup(value);\n> }\n>\n> int main(int argc, char **argv)\n> @@ -1387,7 +1377,7 @@ int main(int argc, char **argv)\n> usage(imap_send_usage);\n>\n> setup_git_directory_gently(&nongit_ok);\n> - git_config(git_imap_config, NULL);\n> + git_imap_config();\n>\n> if (!server.port)\n> server.port = server.use_ssl ? 993 : 143;\n> --\n> 1.9.0.GIT\n"},{"id":"244960","messageId":"CAPig+cRcbamkpeaoCMQ9yXtYeMEEL-ve--VWKg4MAWeS08Lqyg@mail.gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-4-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T07:54:57Z","receivedAt":"2014-06-25T07:54:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n> Use git_config_get_string instead of git_config to take advantage of\n> the config hash-table api which provides a cleaner control flow.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> notes-utils.c | 31 +++++++++++++++----------------\n> 1 file changed, 15 insertions(+), 16 deletions(-)\n>\n> diff --git a/notes-utils.c b/notes-utils.c\n> index a0b1d7b..fdc9912 100644\n> --- a/notes-utils.c\n> +++ b/notes-utils.c\n> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n> return NULL;\n> }\n>\n> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n> {\n> - struct notes_rewrite_cfg *c = cb;\n> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n> - c->enabled = git_config_bool(k, v);\n> - return 0;\n> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n> + struct strbuf key = STRBUF_INIT;\n> + const char *v;\n> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n> +\n> + if (!git_config_get_string(key.buf, &v))\n> + c->enabled = git_config_bool(key.buf, v);\n> +\n> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n> if (!v)\n> - return config_error_nonbool(k);\n> + config_error_nonbool(\"notes.rewritemode\");\n\nThere's a behavior change here. In the original code, the callback\nfunction would return -1, which would cause the program to die() if\nthe config.c:die_on_error flag was set. The new code merely emits an\nerror.\n\n> c->combine = parse_combine_notes_fn(v);\n\nWorse: Though you correctly emit an error when 'v' is NULL, you then\n(incorrectly) invoke parse_combine_notes_fn() with that NULL value,\nwhich will result in a crash.\n\n> - if (!c->combine) {\n> + if (!c->combine)\n> error(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n> - return 1;\n> - }\n> - return 0;\n> - } else if (!c->refs_from_env && !strcmp(k, \"notes.rewriteref\")) {\n> + }\n> + if (!c->refs_from_env && !git_config_get_string(\"notes.rewriteref\", &v)) {\n> /* note that a refs/ prefix is implied in the\n> * underlying for_each_glob_ref */\n> if (starts_with(v, \"refs/notes/\"))\n> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)\n> else\n> warning(_(\"Refusing to rewrite notes in %s\"\n> \" (outside of refs/notes/)\"), v);\n> - return 0;\n> }\n> -\n> - return 0;\n> + strbuf_release(&key);\n\nIt would be better to release the strbuf immediately after its final\nuse rather than waiting until the end of function. Not only does that\nreduce cognitive load on people reading the code, but it also reduces\nlikelihood of 'key' being leaked if some future programmer inserts an\nearly 'return' into the function for some reason.\n\n> }\n>\n>\n> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n> c->refs_from_env = 1;\n> string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);\n> }\n> - git_config(notes_rewrite_config, c);\n> + notes_rewrite_config(c);\n> if (!c->enabled || !c->refs->nr) {\n> string_list_clear(c->refs, 0);\n> free(c->refs);\n> --\n> 1.9.0.GIT\n"},{"id":"244961","messageId":"CAPig+cR-fdXCe+9e89++KrLPqfPkbSi4FeU_gD=fwfrMnH-a2Q@mail.gmail.com","threadId":"36982","inReplyTo":"1403520105-23250-5-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH] notes.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T08:06:08Z","receivedAt":"2014-06-25T08:06:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n> Use git_config_get_string instead of git_config to take advantage of\n> the config hash-table api which provides a cleaner control flow.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> notes.c | 20 ++++++--------------\n> 1 file changed, 6 insertions(+), 14 deletions(-)\n>\n> diff --git a/notes.c b/notes.c\n> index 5fe691d..fc92eec 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n> free(globs_copy);\n> }\n>\n> -static int notes_display_config(const char *k, const char *v, void *cb)\n> -{\n> - int *load_refs = cb;\n> -\n> - if (*load_refs && !strcmp(k, \"notes.displayref\")) {\n> - if (!v)\n> - config_error_nonbool(k);\n> - string_list_add_refs_by_glob(&display_notes_refs, v);\n> - }\n> -\n> - return 0;\n> -}\n> -\n> const char *default_notes_ref(void)\n> {\n> const char *notes_ref = NULL;\n> @@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n> void init_display_notes(struct display_notes_opt *opt)\n> {\n> char *display_ref_env;\n> + const char *value;\n> int load_config_refs = 0;\n> display_notes_refs.strdup_strings = 1;\n>\n> @@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)\n> load_config_refs = 1;\n> }\n>\n> - git_config(notes_display_config, &load_config_refs);\n> + if (load_config_refs && !git_config_get_string(\"notes.displayref\", &value)) {\n> + if (!value)\n> + config_error_nonbool(\"notes.displayref\");\n> + string_list_add_refs_by_glob(&display_notes_refs, value);\n\nAlthough you correctly diagnose a NULL 'value', you then invoke\nstring_list_add_refs_by_glob() with that NULL, which will result in a\ncrash.\n\nThis is not a new error. It dates back to 894a9d33 (Support showing\nnotes from more than one notes tree; 2010-03-12), but your rewrite\nshould not retain the brokenness. Whether you fix it in this patch or\na lead-in fix-up patch, the fix deserves mention in the commit\nmessage.\n\n> + }\n>\n> if (opt) {\n> struct string_list_item *item;\n> --\n> 1.9.0.GIT\n"},{"id":"245007","messageId":"53ABD52B.3030502@gmail.com","threadId":"36982","inReplyTo":"CAPig+cTMWKtAjN931voVs7aY7JdEyaRVZj+-qDUiDk0C_Tq6-A@mail.gmail.com","subject":"Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-26T08:09:15Z","receivedAt":"2014-06-26T08:09:15Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\nOn 6/25/2014 10:15 AM, Eric Sunshine wrote:\n> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n\n>> diff --git a/branch.c b/branch.c\n>> index 660097b..c9a2a0d 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n>> return 0;\n>> }\n>>\n>> -struct branch_desc_cb {\n>> +struct branch_desc {\n>> const char *config_name;\n>> const char *value;\n>> };\n> \n> What is the purpose of retaining this structure? Following your\n> changes, it is never used outside of read_branch_desc(), and\n> 'config_name' and 'value' would be more naturally declared as\n> variables local to that function.\n\nDone. :)\n\n> \n>> -static int read_branch_desc_cb(const char *var, const char *value, void *cb)\n>> -{\n>> - struct branch_desc_cb *desc = cb;\n>> - if (strcmp(desc->config_name, var))\n>> - return 0;\n>> - free((char *)desc->value);\n>> - return git_config_string(&desc->value, var, value);\n>> -}\n>> -\n>> int read_branch_desc(struct strbuf *buf, const char *branch_name)\n>> {\n>> - struct branch_desc_cb cb;\n>> + const char *value = NULL;\n>> + struct branch_desc desc;\n>> struct strbuf name = STRBUF_INIT;\n>> strbuf_addf(&name, \"branch.%s.description\", branch_name);\n>> - cb.config_name = name.buf;\n>> - cb.value = NULL;\n>> - if (git_config(read_branch_desc_cb, &cb) < 0) {\n>> + desc.config_name = name.buf;\n>> + desc.value = NULL;\n>> + git_config_get_string(desc.config_name, &value);\n>> + if (git_config_string(&desc.value, desc.config_name, value) < 0) {\n> \n> Although it works in this case, it's somewhat ugly that you ignore the\n> return value of git_config_get_string(), and a person reading the code\n> has to spend extra time digging into git_config_string() to figure out\n> why this is safe. If might be clearer for future readers by rephrasing\n> like this:\n> \n> if (git_config_get_string(desc.config_name, &value) < 0 ||\n> git_config_string(&desc.value, desc.config_name, value) < 0) {\n>\n\nNoted, also didn't the old code leak desc.value as it was xstrduped\nby git_config_string()? Thanks for the review.\n\n>> strbuf_release(&name);\n>> return -1;\n>> }\n>> - if (cb.value)\n>> - strbuf_addstr(buf, cb.value);\n>> + strbuf_addstr(buf, desc.value);\n>> strbuf_release(&name);\n>> return 0;\n>> }\n>> --\n>> 1.9.0.GIT\n> \n"},{"id":"245008","messageId":"53ABD677.4040808@gmail.com","threadId":"36982","inReplyTo":"CAPig+cTStpB09Fh3bAF2NcdWR9W_UMaL5qutvUuQNfEL4=GwNw@mail.gmail.com","subject":"Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-26T08:14:47Z","receivedAt":"2014-06-26T08:14:47Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 6/25/2014 12:39 PM, Eric Sunshine wrote:\n> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>> Use git_config_get_string instead of git_config to take advantage of\n>> the config hash-table api which provides a cleaner control flow.\n> \n> You may want to mention as a side-note the slight behavior change\n> introduced by this patch. The original code complained about any\n> unknown boolean \"imap.*\" key, whereas the new code does not.\n> \n\nAlso, my code is error prone. Previous one had all NULL values returned\nas config_non_boolean. But, now I have to add a NULL check to every strdup\nin the code.\n\nMore below,\n\n> \n>> Signed-off-by: Tanay Abhra \n>> ---\n>> imap-send.c | 68 ++++++++++++++++++++++++++-----------------------------------\n>> 1 file changed, 29 insertions(+), 39 deletions(-)\n>>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index 83a6ed2..87bd418 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)\n>>\n>> static char *imap_folder;\n>>\n>> -static int git_imap_config(const char *key, const char *val, void *cb)\n>> +static void git_imap_config(void)\n>> {\n>> - char imap_key[] = \"imap.\";\n>> -\n>> - if (strncmp(key, imap_key, sizeof imap_key - 1))\n>> - return 0;\n>> -\n>> - key += sizeof imap_key - 1;\n>> -\n>> - /* check booleans first, and barf on others */\n>> - if (!strcmp(\"sslverify\", key))\n>> - server.ssl_verify = git_config_bool(key, val);\n>> - else if (!strcmp(\"preformattedhtml\", key))\n>> - server.use_html = git_config_bool(key, val);\n>> - else if (!val)\n>> - return config_error_nonbool(key);\n>> -\n>> - if (!strcmp(\"folder\", key)) {\n>> - imap_folder = xstrdup(val);\n>> - } else if (!strcmp(\"host\", key)) {\n>> - if (starts_with(val, \"imap:\"))\n>> - val += 5;\n>> - else if (starts_with(val, \"imaps:\")) {\n>> - val += 6;\n>> + const char *value;\n> \n> Observation: If you name this variable 'val', which is the name of the\n> argument to the function in the original code, you will get a slightly\n> smaller and more readable diff. \n\nNoted.\n\n> \n>> + if (!git_config_get_string(\"imap.sslverify\", &value))\n>> + server.ssl_verify = git_config_bool(\"sslverify\", value);\n> \n> I realize that you are just replicating the behavior of the original\n> code, but the error message emitted here for a non-bool value is less\n> than desirable since it throws away context (namely, the \"imap.\"\n> prefix). You can improve the message, and help the user resolve the\n> error more quickly, by presenting the full configuration key (namely,\n> \"imap.sslverify\"). Such a change would deserve mention in the commit\n> message. Alternately, it could be fixed in a follow-up patch.\n> \n\nYes, I thought so also when writing the patch. Will change it in the next\niteration.\n\nThanks.\nTanay Abhra.\n\n>> + if (!git_config_get_string(\"imap.preformattedhtml\", &value))\n>> + server.use_html = git_config_bool(\"preformattedhtml\", value);\n> \n> Ditto regarding error message: \"imap.preformattedhtml\"\n> \n>> + if (!git_config_get_string(\"imap.folder\", &value))\n>> + imap_folder = xstrdup(value);\n>> + if (!git_config_get_string(\"imap.host\", &value)) {\n>> + if (starts_with(value, \"imap:\"))\n>> + value += 5;\n>> + else if (starts_with(value, \"imaps:\")) {\n>> + value += 6;\n>> server.use_ssl = 1;\n>> }\n>> - if (starts_with(val, \"//\"))\n>> - val += 2;\n>> - server.host = xstrdup(val);\n>> - } else if (!strcmp(\"user\", key))\n>> - server.user = xstrdup(val);\n>> - else if (!strcmp(\"pass\", key))\n>> - server.pass = xstrdup(val);\n>> - else if (!strcmp(\"port\", key))\n>> - server.port = git_config_int(key, val);\n>> - else if (!strcmp(\"tunnel\", key))\n>> - server.tunnel = xstrdup(val);\n>> - else if (!strcmp(\"authmethod\", key))\n>> - server.auth_method = xstrdup(val);\n>> -\n>> - return 0;\n>> + if (starts_with(value, \"//\"))\n>> + value += 2;\n>> + server.host = xstrdup(value);\n>> + }\n>> + if (!git_config_get_string(\"imap.user\", &value))\n>> + server.user = xstrdup(value);\n>> + if (!git_config_get_string(\"imap.pass\", &value))\n>> + server.pass = xstrdup(value);\n>> + if (!git_config_get_string(\"imap.port\", &value))\n>> + server.port = git_config_int(\"port\", value);\n> \n> Same regarding diagnostic: \"imap.port\"\n> \n>> + if (!git_config_get_string(\"imap.tunnel\", &value))\n>> + server.tunnel = xstrdup(value);\n>> + if (!git_config_get_string(\"imap.authmethod\", &value))\n>> + server.auth_method = xstrdup(value);\n>> }\n>>\n>> int main(int argc, char **argv)\n>> @@ -1387,7 +1377,7 @@ int main(int argc, char **argv)\n>> usage(imap_send_usage);\n>>\n>> setup_git_directory_gently(&nongit_ok);\n>> - git_config(git_imap_config, NULL);\n>> + git_imap_config();\n>>\n>> if (!server.port)\n>> server.port = server.use_ssl ? 993 : 143;\n>> --\n>> 1.9.0.GIT\n> \n"},{"id":"245009","messageId":"53ABD78E.3050800@gmail.com","threadId":"36982","inReplyTo":"CAPig+cRcbamkpeaoCMQ9yXtYeMEEL-ve--VWKg4MAWeS08Lqyg@mail.gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-26T08:19:26Z","receivedAt":"2014-06-26T08:19:26Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 6/25/2014 1:24 PM, Eric Sunshine wrote:\n> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>> Use git_config_get_string instead of git_config to take advantage of\n>> the config hash-table api which provides a cleaner control flow.\n>>\n>> Signed-off-by: Tanay Abhra \n>> ---\n>> notes-utils.c | 31 +++++++++++++++----------------\n>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/notes-utils.c b/notes-utils.c\n>> index a0b1d7b..fdc9912 100644\n>> --- a/notes-utils.c\n>> +++ b/notes-utils.c\n>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>> return NULL;\n>> }\n>>\n>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>> {\n>> - struct notes_rewrite_cfg *c = cb;\n>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>> - c->enabled = git_config_bool(k, v);\n>> - return 0;\n>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>> + struct strbuf key = STRBUF_INIT;\n>> + const char *v;\n>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>> +\n>> + if (!git_config_get_string(key.buf, &v))\n>> + c->enabled = git_config_bool(key.buf, v);\n>> +\n>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>> if (!v)\n>> - return config_error_nonbool(k);\n>> + config_error_nonbool(\"notes.rewritemode\");\n> \n> There's a behavior change here. In the original code, the callback\n> function would return -1, which would cause the program to die() if\n> the config.c:die_on_error flag was set. The new code merely emits an\n> error.\n> \n\nIs this change serious enough? Can I ignore it?\n\n>> c->combine = parse_combine_notes_fn(v);\n> \n> Worse: Though you correctly emit an error when 'v' is NULL, you then\n> (incorrectly) invoke parse_combine_notes_fn() with that NULL value,\n> which will result in a crash.\n> \n\nNoted.\n\n>> - if (!c->combine) {\n>> + if (!c->combine)\n>> error(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n>> - return 1;\n>> - }\n>> - return 0;\n>> - } else if (!c->refs_from_env && !strcmp(k, \"notes.rewriteref\")) {\n>> + }\n>> + if (!c->refs_from_env && !git_config_get_string(\"notes.rewriteref\", &v)) {\n>> /* note that a refs/ prefix is implied in the\n>> * underlying for_each_glob_ref */\n>> if (starts_with(v, \"refs/notes/\"))\n>> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>> else\n>> warning(_(\"Refusing to rewrite notes in %s\"\n>> \" (outside of refs/notes/)\"), v);\n>> - return 0;\n>> }\n>> -\n>> - return 0;\n>> + strbuf_release(&key);\n> \n> It would be better to release the strbuf immediately after its final\n> use rather than waiting until the end of function. Not only does that\n> reduce cognitive load on people reading the code, but it also reduces\n> likelihood of 'key' being leaked if some future programmer inserts an\n> early 'return' into the function for some reason.\n> \n\nNoted. Thanks.\n\n>> }\n>>\n>>\n>> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n>> c->refs_from_env = 1;\n>> string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);\n>> }\n>> - git_config(notes_rewrite_config, c);\n>> + notes_rewrite_config(c);\n>> if (!c->enabled || !c->refs->nr) {\n>> string_list_clear(c->refs, 0);\n>> free(c->refs);\n>> --\n>> 1.9.0.GIT\n> \n"},{"id":"245010","messageId":"53ABD7CC.105@gmail.com","threadId":"36982","inReplyTo":"CAPig+cR-fdXCe+9e89++KrLPqfPkbSi4FeU_gD=fwfrMnH-a2Q@mail.gmail.com","subject":"Re: [RFC/PATCH] notes.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-26T08:20:28Z","receivedAt":"2014-06-26T08:20:28Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 6/25/2014 1:36 PM, Eric Sunshine wrote:\n> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>> Use git_config_get_string instead of git_config to take advantage of\n>> the config hash-table api which provides a cleaner control flow.\n>>\n>> Signed-off-by: Tanay Abhra \n>> ---\n>> notes.c | 20 ++++++--------------\n>> 1 file changed, 6 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/notes.c b/notes.c\n>> index 5fe691d..fc92eec 100644\n>> --- a/notes.c\n>> +++ b/notes.c\n>> @@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n>> free(globs_copy);\n>> }\n>>\n>> -static int notes_display_config(const char *k, const char *v, void *cb)\n>> -{\n>> - int *load_refs = cb;\n>> -\n>> - if (*load_refs && !strcmp(k, \"notes.displayref\")) {\n>> - if (!v)\n>> - config_error_nonbool(k);\n>> - string_list_add_refs_by_glob(&display_notes_refs, v);\n>> - }\n>> -\n>> - return 0;\n>> -}\n>> -\n>> const char *default_notes_ref(void)\n>> {\n>> const char *notes_ref = NULL;\n>> @@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n>> void init_display_notes(struct display_notes_opt *opt)\n>> {\n>> char *display_ref_env;\n>> + const char *value;\n>> int load_config_refs = 0;\n>> display_notes_refs.strdup_strings = 1;\n>>\n>> @@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)\n>> load_config_refs = 1;\n>> }\n>>\n>> - git_config(notes_display_config, &load_config_refs);\n>> + if (load_config_refs && !git_config_get_string(\"notes.displayref\", &value)) {\n>> + if (!value)\n>> + config_error_nonbool(\"notes.displayref\");\n>> + string_list_add_refs_by_glob(&display_notes_refs, value);\n> \n> Although you correctly diagnose a NULL 'value', you then invoke\n> string_list_add_refs_by_glob() with that NULL, which will result in a\n> crash.\n> \n> This is not a new error. It dates back to 894a9d33 (Support showing\n> notes from more than one notes tree; 2010-03-12), but your rewrite\n> should not retain the brokenness. Whether you fix it in this patch or\n> a lead-in fix-up patch, the fix deserves mention in the commit\n> message.\n\nDone. Thanks.\n\n>> + }\n>>\n>> if (opt) {\n>> struct string_list_item *item;\n>> --\n>> 1.9.0.GIT\n> \n"},{"id":"245011","messageId":"53ABD8AA.10909@gmail.com","threadId":"36982","inReplyTo":"CAPig+cTwBB8bmKDtdv8zPojR+6Kyu6fYB7Sw0-UJoSHFLQX+fQ@mail.gmail.com","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-26T08:24:10Z","receivedAt":"2014-06-26T08:24:10Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 6/25/2014 9:29 AM, Eric Sunshine wrote:\n> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>> Use git_config_get_string instead of git_config to take advantage of\n>> the config hash-table api which provides a cleaner control flow.\n>>\n>> Signed-off-by: Tanay Abhra \n>> ---\n>> pager.c | 44 +++++++++++++++-----------------------------\n>> 1 file changed, 15 insertions(+), 29 deletions(-)\n>>\n>> diff --git a/pager.c b/pager.c\n>> index 8b5cbc5..96abe6d 100644\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -6,12 +6,6 @@\n>> #define DEFAULT_PAGER \"less\"\n>> #endif\n>>\n>> -struct pager_config {\n>> - const char *cmd;\n>> - int want;\n>> - char *value;\n>> -};\n>> -\n>> /*\n>> * This is split up from the rest of git so that we can do\n>> * something different on Windows.\n>> @@ -155,30 +149,22 @@ int decimal_width(int number)\n>> return width;\n>> }\n>>\n>> -static int pager_command_config(const char *var, const char *value, void *data)\n>> -{\n>> - struct pager_config *c = data;\n>> - if (starts_with(var, \"pager.\") && !strcmp(var + 6, c->cmd)) {\n>> - int b = git_config_maybe_bool(var, value);\n>> - if (b >= 0)\n>> - c->want = b;\n>> - else {\n>> - c->want = 1;\n>> - c->value = xstrdup(value);\n>> - }\n>> - }\n>> - return 0;\n>> -}\n>> -\n>> /* returns 0 for \"no pager\", 1 for \"use pager\", and -1 for \"not specified\" */\n>> int check_pager_config(const char *cmd)\n>> {\n>> - struct pager_config c;\n>> - c.cmd = cmd;\n>> - c.want = -1;\n>> - c.value = NULL;\n>> - git_config(pager_command_config, &c);\n>> - if (c.value)\n>> - pager_program = c.value;\n>> - return c.want;\n>> + struct strbuf key = STRBUF_INIT;\n>> + int want = -1;\n>> + const char *value = NULL;\n>> + strbuf_addf(&key, \"pager.%s\", cmd);\n>> + if (!git_config_get_string(key.buf, &value)) {\n>> + int b = git_config_maybe_bool(key.buf, value);\n>> + if (b >= 0)\n>> + want = b;\n>> + else\n>> + want = 1;\n>> + }\n>> + if (value)\n>> + pager_program = value;\n> \n> Two issues:\n> \n> First, why is 'if(value)' standing by itself? Although this works, it\n> seems to imply that 'value' might be able to become non-NULL by some\n> mechanism other than the get_config_maybe_bool() call, which means\n> that people reading this code have to spend extra time trying to\n> understand the overall logic. If you follow the example of the\n> original code, where 'value' is only ever set when 'b < 0', then it is\n> obvious even to the most casual reader that 'pager_program' is\n> assigned only for that one condition.\n> \n\nNoted.\n\n> Second, don't you want to xstrdup(value) when assigning to\n> 'pager_program'? If you don't, then 'pager_program' will become a\n> dangling pointer when config_cache_free() is invoked.\n> \n\nNoted. Thanks.\n\n>> + strbuf_release(&key);\n>> + return want;\n>> }\n>> --\n>> 1.9.0.GIT\n> \n"},{"id":"245012","messageId":"53ABD8C0.1000308@gmail.com","threadId":"36982","inReplyTo":"CAPig+cQ3hzFFRRhbDTddArBC0nB9gZ7yMXuh35GHe73x-1JDng@mail.gmail.com","subject":"Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-26T08:24:32Z","receivedAt":"2014-06-26T08:24:32Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 6/25/2014 7:42 AM, Eric Sunshine wrote:\n> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>> Use git_config_get_string instead of git_config to take advantage of\n>> the config hash-table api which provides a cleaner control flow.\n>>\n>> Signed-off-by: Tanay Abhra \n>> ---\n>> alias.c | 28 ++++++++++------------------\n>> 1 file changed, 10 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/alias.c b/alias.c\n>> index 5efc3d6..0fe32bc 100644\n>> --- a/alias.c\n>> +++ b/alias.c\n>> @@ -1,25 +1,17 @@\n>> #include \"cache.h\"\n>>\n>> -static const char *alias_key;\n>> -static char *alias_val;\n>> -\n>> -static int alias_lookup_cb(const char *k, const char *v, void *cb)\n>> -{\n>> - if (starts_with(k, \"alias.\") && !strcmp(k + 6, alias_key)) {\n>> - if (!v)\n>> - return config_error_nonbool(k);\n>> - alias_val = xstrdup(v);\n>> - return 0;\n>> - }\n>> - return 0;\n>> -}\n>> -\n>> char *alias_lookup(const char *alias)\n>> {\n>> - alias_key = alias;\n>> - alias_val = NULL;\n>> - git_config(alias_lookup_cb, NULL);\n>> - return alias_val;\n>> + const char *v;\n>> + char *value;\n>> + struct strbuf key = STRBUF_INIT;\n>> + strbuf_addf(&key, \"alias.%s\", alias);\n>> + git_config_get_string(key.buf, &v);\n>> + if (!v)\n>> + config_error_nonbool(key.buf);\n> \n> If 'v' is NULL, you correctly report an error, but then fall through\n> and invoke xstrdup() with NULL, which invites undefined behavior [1].\n> \n> [1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html\n> \n>> + value = xstrdup(v);\n>> + strbuf_release(&key);\n>> + return value;\n> \n> You could release the strbuf earlier, which would allow you to 'return\n> xstrdup(v)' and drop the 'value' variable. Perhaps you want something\n> like this:\n> \n> const char *v;\n> struct strbuf key = STRBUF_INIT;\n> strbuf_addf(&key, \"alias.%s\", alias);\n> git_config_get_string(key.buf, &v);\n> if (v)\n> config_error_nonbool(key.buf);\n> strbuf_release(&key);\n> return v ? xstrdup(v) : NULL;\n> \n\nDone. Thanks.\n\n>> }\n>>\n>> #define SPLIT_CMDLINE_BAD_ENDING 1\n>> --\n>> 1.9.0.GIT\n> \n"},{"id":"245024","messageId":"vpqoaxfmxqn.fsf@anie.imag.fr","threadId":"36982","inReplyTo":"1403520105-23250-1-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-26T16:39:12Z","receivedAt":"2014-06-26T16:39:12Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> the config hash-table api which provides a cleaner control flow.\n\napi -> API\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245027","messageId":"vpqbntfmx81.fsf@anie.imag.fr","threadId":"36982","inReplyTo":"1403520105-23250-3-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-26T16:50:22Z","receivedAt":"2014-06-26T16:50:22Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> +\tif (!git_config_get_string(\"imap.user\", &value))\n> +\t\tserver.user = xstrdup(value);\n> +\tif (!git_config_get_string(\"imap.pass\", &value))\n> +\t\tserver.pass = xstrdup(value);\n> +\tif (!git_config_get_string(\"imap.port\", &value))\n> +\t\tserver.port = git_config_int(\"port\", value);\n> +\tif (!git_config_get_string(\"imap.tunnel\", &value))\n> +\t\tserver.tunnel = xstrdup(value);\n> +\tif (!git_config_get_string(\"imap.authmethod\", &value))\n> +\t\tserver.auth_method = xstrdup(value);\n\nGiven this kind of systematic code, I find it very tempting to factor\nthis with a new helper function as\n\n...\ngit_config_get_string_dup(\"imap.tunnel\", &server.tunnel)\ngit_config_get_string_dup(\"imap.authmethod\", &server.auth_method)\n\nIs there any reason not to do so?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245032","messageId":"53AC6A7B.3040602@gmail.com","threadId":"36982","inReplyTo":"CAPig+cTwBB8bmKDtdv8zPojR+6Kyu6fYB7Sw0-UJoSHFLQX+fQ@mail.gmail.com","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-26T18:46:19Z","receivedAt":"2014-06-26T18:46:19Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 25.06.2014 05:59, schrieb Eric Sunshine:\n> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n\n[...]\n\n>> /* returns 0 for \"no pager\", 1 for \"use pager\", and -1 for \"not specified\" */\n>> int check_pager_config(const char *cmd)\n>> {\n>> - struct pager_config c;\n>> - c.cmd = cmd;\n>> - c.want = -1;\n>> - c.value = NULL;\n>> - git_config(pager_command_config, &c);\n>> - if (c.value)\n>> - pager_program = c.value;\n>> - return c.want;\n>> + struct strbuf key = STRBUF_INIT;\n>> + int want = -1;\n>> + const char *value = NULL;\n>> + strbuf_addf(&key, \"pager.%s\", cmd);\n>> + if (!git_config_get_string(key.buf, &value)) {\n>> + int b = git_config_maybe_bool(key.buf, value);\n>> + if (b >= 0)\n>> + want = b;\n>> + else\n>> + want = 1;\n>> + }\n>> + if (value)\n>> + pager_program = value;\n\n[...]\n> \n> Second, don't you want to xstrdup(value) when assigning to\n> 'pager_program'? If you don't, then 'pager_program' will become a\n> dangling pointer when config_cache_free() is invoked.\n> \n\nI don't think that values from the global config cache should be xstrdup()ed.\nAfter all, caching the values during the lifetime of the git process is the\nentire point of the config cache, isn't it?\n\nThe only reason to call config_cache_free() is to load a _different_\nconfiguration. In this case, however, you would also need to call the relevant\nconfig functions again, leaking all xstrdup()ed strings.\n\nIf for some reason a config string is accessed after config_cache_free()\n(which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git\nwill continue to run with some invalid configuration). This is IMO much worse\nthan failing with segfault.\n"},{"id":"245048","messageId":"53ACB381.80307@gmail.com","threadId":"36982","inReplyTo":"vpqbntfmx81.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-26T23:57:53Z","receivedAt":"2014-06-26T23:57:53Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 26.06.2014 18:50, schrieb Matthieu Moy:\n> Tanay Abhra writes:\n> \n>> +\tif (!git_config_get_string(\"imap.user\", &value))\n>> +\t\tserver.user = xstrdup(value);\n>> +\tif (!git_config_get_string(\"imap.pass\", &value))\n>> +\t\tserver.pass = xstrdup(value);\n>> +\tif (!git_config_get_string(\"imap.port\", &value))\n>> +\t\tserver.port = git_config_int(\"port\", value);\n>> +\tif (!git_config_get_string(\"imap.tunnel\", &value))\n>> +\t\tserver.tunnel = xstrdup(value);\n>> +\tif (!git_config_get_string(\"imap.authmethod\", &value))\n>> +\t\tserver.auth_method = xstrdup(value);\n> \n> Given this kind of systematic code, I find it very tempting to factor\n> this with a new helper function as\n> \n> ...\n> git_config_get_string_dup(\"imap.tunnel\", &server.tunnel)\n> git_config_get_string_dup(\"imap.authmethod\", &server.auth_method)\n> \n> Is there any reason not to do so?\n> \n\n\nWith a pull-style API, you no longer need global variables for\neverything, so IMO the helper functions should _return_ the values\nrather than taking an output parameter.\n\nE.g. with helper functions as suggested here [1] we could have:\n\n if (git_config_get_bool(\"imap.preformattedhtml\", 0))\n wrap_in_html(&msg);\n\n...rather than needing an extra variable:\n\n int bool_value;\n git_config_get_bool(\"imap.preformattedhtml\", &bool_value);\n if (bool_value)\n wrap_in_html(&msg);\n\n...and specify default values along with their respective keys:\n\n server.ssl_verify = git_config_get_bool(\"imap.sslverify\", 1);\n server.port = git_config_get_int(\"imap.port\", server.use_ssl ? 993 : 143);\n\n...rather than ~1300 lines apart (yuck):\n\n static struct imap_server_conf server = {\n NULL, /* name */\n NULL, /* tunnel */\n NULL, /* host */\n 0, /* port */\n NULL, /* user */\n NULL, /* pass */\n 0, /* use_ssl */\n 1, /* ssl_verify */\n 0, /* use_html */\n NULL, /* auth_method */\n };\n\n\nRegarding xstrdup(), I think this is a remnant from the callback\nversion, which _requires_ you to xstrdup() (the value parameter is\ninvalidated after returning from the callback).\n\nSide note: with the current callback design, config variables may\nget passed to the callback multiple times (last value wins), so\neach xstrdup() in current 'git_*_config' functions actually\ncauses memory leaks (unless prefixed with 'free(my_config_var);').\n\n[1] http://git.661346.n2.nabble.com/PATCH-v3-0-3-git-config-cache-special-querying-api-utilizing-the-cache-tp7613911p7614050.html\n"},{"id":"245063","messageId":"vpqr42afty5.fsf@anie.imag.fr","threadId":"36982","inReplyTo":"53AC6A7B.3040602@gmail.com","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-27T11:55:14Z","receivedAt":"2014-06-27T11:55:14Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karsten Blees writes:\n\n> If for some reason a config string is accessed after config_cache_free()\n> (which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git\n> will continue to run with some invalid configuration). This is IMO much worse\n> than failing with segfault.\n\nI disagree. In most case, continuing to use the old config value is the\nright thing.\n\nFirst, config_cache_free() is called whenever _any_ configuration\nvariable is set. So, setting \"core.foo\" also invalidates \"core.bar\" in\nthe cache, while a user of \"core.bar\" could continue working with the\nold, unchanged value.\n\nThen, allowing the invalidation of a config variable at any time raises\na lot of tricky cases (if a command uses a configuration variable twice,\ndo we really want to implement and test the behavior for all combination\nof old/new values for both usages?). More tricky cases usually means\nmore bugs on the user-side ...\n\nWhen the code really want the freshest value, it's cheap to re-query the\nconfig cache anyway.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245078","messageId":"53ADA26E.8030801@gmail.com","threadId":"36982","inReplyTo":"vpqr42afty5.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-27T16:57:18Z","receivedAt":"2014-06-27T16:57:18Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 27.06.2014 13:55, schrieb Matthieu Moy:\n> Karsten Blees writes:\n> \n>> If for some reason a config string is accessed after config_cache_free()\n>> (which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git\n>> will continue to run with some invalid configuration). This is IMO much worse\n>> than failing with segfault.\n> \n> I disagree. In most case, continuing to use the old config value is the\n> right thing.\n> \n> First, config_cache_free() is called whenever _any_ configuration\n> variable is set.\n\nThis is illogical. An API should do the right thing, and if the\nright thing is to keep the old values, as you pointed out, then\nsetting a config value shouldn't invalidate the cache (at least\nnot automatically).\n\nI can think of only git-clone and git-init that may want to refresh\nthe global config cache, otherwise, config_cache_free() should\nprobably _never_ be called. Least of all as a side effect of\ngit_config_set_multivar_in_file(), which is used to write all kinds\nof unrelated files that just happen to have config-file format (e.g.\n'.gitmodules', '.git/sequencer/opts').\n\n> So, setting \"core.foo\" also invalidates \"core.bar\" in\n> the cache, while a user of \"core.bar\" could continue working with the\n> old, unchanged value.\n\nBut a user of an xstrdup()ed copy of \"core.foo\" would also continue\nto work with the old value. So if everyone keeps copies of config\nstrings, invalidating the cache when setting a value has no effect.\nWe could just as well not invalidate the cache and not xstrdup()\nstrings.\n\nPerhaps we should see this as an opportunity to get rid of all the\nmemory leaks in current config code (each string value defined in\nsystem / global config and overridden locally will currently be\nleaked with the 'standard' xstrdup() pattern).\n"},{"id":"245091","messageId":"vpqmwcyw47q.fsf@anie.imag.fr","threadId":"36982","inReplyTo":"53ADA26E.8030801@gmail.com","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-27T19:19:05Z","receivedAt":"2014-06-27T19:19:05Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karsten Blees writes:\n\n> Am 27.06.2014 13:55, schrieb Matthieu Moy:\n>> Karsten Blees writes:\n>> \n>>> If for some reason a config string is accessed after config_cache_free()\n>>> (which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git\n>>> will continue to run with some invalid configuration). This is IMO much worse\n>>> than failing with segfault.\n>> \n>> I disagree. In most case, continuing to use the old config value is the\n>> right thing.\n>> \n>> First, config_cache_free() is called whenever _any_ configuration\n>> variable is set.\n>\n> This is illogical. An API should do the right thing, and if the\n> right thing is to keep the old values, as you pointed out, then\n> setting a config value shouldn't invalidate the cache (at least\n> not automatically).\n\nThe reason for which setting any config value invalidates the cache is\nthat it is _much_ simpler to implement. Less complexity, less\ncorner-cases, less bugs.\n\n>> So, setting \"core.foo\" also invalidates \"core.bar\" in\n>> the cache, while a user of \"core.bar\" could continue working with the\n>> old, unchanged value.\n>\n> But a user of an xstrdup()ed copy of \"core.foo\" would also continue\n> to work with the old value.\n\nExactly like the current code does. Has it ever been even close to being\na problem?\n\nWhat you're suggesting brings a lot more complexity in the code, and I\ncan't imagine a case where it would have a real benefit. So, why bother?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245113","messageId":"53AE50A9.6010707@gmail.com","threadId":"36982","inReplyTo":"vpqmwcyw47q.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-28T05:20:41Z","receivedAt":"2014-06-28T05:20:41Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 27.06.2014 21:19, schrieb Matthieu Moy:\n> \n> The reason for which setting any config value invalidates the cache is\n> that it is _much_ simpler to implement. Less complexity, less\n> corner-cases, less bugs.\n> \n\nI think I see what you mean. E.g. in cmd_clone we have:\n\n write_config(&option_config);\n git_config(git_default_config, NULL);\n ...\n git_config_set(\"remote.Foo.url\", repo);\n ...\n remote = remote_get(option_origin); /* which does 'git_config(handle_config)' */\n\nSo if we load the config cache at 'git_config(git_default_config)', then\n'remote_get' won't see \"remote.Foo.url\" unless we invalidate the cache first.\n\n\nI still don't like that the invalidation is done in git_config_set, though, as\nthis is also used to write completely unrelated files. Wouldn't it be better to\nhave a 'git_config_refresh()' that could be used in place of (or before) current\n'git_config(callback)' calls? The initial implementation could just invalidate\nthe config cache. If there's time and energy to spare, a more advanced version\ncould first check if any of the involved config files has changed.\n\n\nThe xstrdup() problem could be solved by interning strings (see the\nattached patch for a trivial implementation). I.e. allocate each distinct\nstring only once (and keep it allocated).\n\nSo if there are 100 instances of \"true\" in the config file, the interned string\npool would contain only one instance (i.e. 5 bytes + hashmap_entry + malloc\noverhead, vs. 100 * (5 bytes + malloc overhead) in case of xstrdup()). If the\nconfig cache is cleared, the interned string in the pool would still remain\nvalid. If the config cache is reloaded, unmodified values would reuse the\nexisting strings from the pool.\n\nSide note: interning getenv() results would fix the non-POSIX-compliant\ngetenv()-usage in git [1].\n\n[1] https://groups.google.com/d/msg/msysgit/4cVWWJkN4to/DR8FGTQ09Q0J\n\n---- 8< ----\nSubject: [PATCH] hashmap: add string interning API\n\nInterning short strings that are likely to have multiple copies may reduce\nmemory footprint and speed up string comparisons.\n\nAdd a strintern() API that uses a hashmap to manage the pool of interned\nstrings.\n\nSigned-off-by: Karsten Blees \n---\n Documentation/technical/api-hashmap.txt | 11 +++++++++++\n hashmap.c | 34 +++++++++++++++++++++++++++++++++\n hashmap.h | 4 ++++\n t/t0011-hashmap.sh | 13 +++++++++++++\n test-hashmap.c | 14 ++++++++++++++\n 5 files changed, 76 insertions(+)\n\ndiff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt\nindex b977ae8..db7c955 100644\n--- a/Documentation/technical/api-hashmap.txt\n+++ b/Documentation/technical/api-hashmap.txt\n@@ -162,6 +162,17 @@ more entries.\n `hashmap_iter_first` is a combination of both (i.e. initializes the iterator\n and returns the first entry, if any).\n \n+`const char *strintern(const char *string)`::\n+\n+\tReturns the unique, interned version of the specified string, similar\n+\tto the `String.intern` API in Java and .NET. Interned strings remain\n+\tvalid for the entire lifetime of the process.\n++\n+Can be used as `[x]strdup()` replacement, except that the interned string must\n+not be modified or freed.\n++\n+Uses a hashmap to store the pool of interned strings.\n+\n Usage example\n -------------\n \ndiff --git a/hashmap.c b/hashmap.c\nindex d1b8056..03cd8f3 100644\n--- a/hashmap.c\n+++ b/hashmap.c\n@@ -226,3 +226,37 @@ void *hashmap_iter_next(struct hashmap_iter *iter)\n \t\tcurrent = iter->map->table[iter->tablepos++];\n \t}\n }\n+\n+struct string_pool_entry {\n+\tstruct hashmap_entry ent;\n+\tchar key[FLEX_ARRAY];\n+};\n+\n+static int string_pool_cmp(const struct string_pool_entry *e1,\n+\t\t\t const struct string_pool_entry *e2,\n+\t\t\t const char *key)\n+{\n+\treturn strcmp(e1->key, key ? key : e2->key);\n+}\n+\n+const char *strintern(const char *string)\n+{\n+\tstatic struct hashmap map;\n+\tstruct string_pool_entry key, *e;\n+\t/* initialize string pool hashmap */\n+\tif (!map.tablesize)\n+\t\thashmap_init(&map, (hashmap_cmp_fn) string_pool_cmp, 0);\n+\n+\t/* lookup interned string in pool */\n+\thashmap_entry_init(&key, strhash(string));\n+\te = hashmap_get(&map, &key, string);\n+\tif (!e) {\n+\t\t/* not found: create it */\n+\t\tint len = strlen(string);\n+\t\te = xmalloc(sizeof(struct string_pool_entry) + len + 1);\n+\t\thashmap_entry_init(e, key.ent.hash);\n+\t\tmemcpy(e->key, string, len + 1);\n+\t\thashmap_add(&map, e);\n+\t}\n+\treturn e->key;\n+}\ndiff --git a/hashmap.h b/hashmap.h\nindex a816ad4..b428677 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -68,4 +68,8 @@ static inline void *hashmap_iter_first(struct hashmap *map,\n \treturn hashmap_iter_next(iter);\n }\n \n+/* string interning */\n+\n+extern const char *strintern(const char *string);\n+\n #endif\ndiff --git a/t/t0011-hashmap.sh b/t/t0011-hashmap.sh\nindex 391e2b6..f97c805 100755\n--- a/t/t0011-hashmap.sh\n+++ b/t/t0011-hashmap.sh\n@@ -237,4 +237,17 @@ test_expect_success 'grow / shrink' '\n \n '\n \n+test_expect_success 'string interning' '\n+\n+test_hashmap \"intern value1\n+intern Value1\n+intern value2\n+intern value2\n+\" \"value1\n+Value1\n+value2\n+value2\"\n+\n+'\n+\n test_done\ndiff --git a/test-hashmap.c b/test-hashmap.c\nindex f5183fb..116cbb5 100644\n--- a/test-hashmap.c\n+++ b/test-hashmap.c\n@@ -239,6 +239,20 @@ int main(int argc, char *argv[])\n \t\t\t/* print table sizes */\n \t\t\tprintf(\"%u %u\\n\", map.tablesize, map.size);\n \n+\t\t} else if (!strcmp(\"intern\", cmd) && l1) {\n+\n+\t\t\t/* test that strintern works */\n+\t\t\tconst char *i1 = strintern(p1);\n+\t\t\tconst char *i2 = strintern(p1);\n+\t\t\tif (strcmp(i1, p1))\n+\t\t\t\tprintf(\"strintern(%s) returns %s\\n\", p1, i1);\n+\t\t\telse if (i1 == p1)\n+\t\t\t\tprintf(\"strintern(%s) returns input pointer\\n\", p1);\n+\t\t\telse if (i1 != i2)\n+\t\t\t\tprintf(\"strintern(%s) != strintern(%s)\", i1, i2);\n+\t\t\telse\n+\t\t\t\tprintf(\"%s\\n\", i1);\n+\n \t\t} else if (!strcmp(\"perfhashmap\", cmd) && l1 && l2) {\n \n \t\t\tperf_hashmap(atoi(p1), atoi(p2));\n-- \n2.0.0.9649.g1eafa1f.dirty\n"},{"id":"245116","messageId":"vpqy4whshbj.fsf@anie.imag.fr","threadId":"36982","inReplyTo":"53AE50A9.6010707@gmail.com","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-28T06:01:52Z","receivedAt":"2014-06-28T06:01:52Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karsten Blees writes:\n\n> I still don't like that the invalidation is done in git_config_set, though, as\n> this is also used to write completely unrelated files.\n\nI don't get it. It is used to write the config files. Yes, we trigger a\ncomplete reload instead of just changing this particular value in the\nhashmap, but I do not see \"unrelated files\" in the picture.\n\n> Wouldn't it be better to have a 'git_config_refresh()' that could be\n> used in place of (or before) current 'git_config(callback)' calls? The\n> initial implementation could just invalidate the config cache. If\n> there's time and energy to spare, a more advanced version could first\n> check if any of the involved config files has changed.\n\nThat would not change the \"xstrdup\" vs \"no xstrdup\" issue, right?\n\n> The xstrdup() problem could be solved by interning strings (see the\n> attached patch for a trivial implementation). I.e. allocate each distinct\n> string only once (and keep it allocated).\n\nThat's an option. We need to be carefull not to modify any string\nin-place, but I guess that would considerably simplify memory management\nfor strings.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245124","messageId":"53AED13D.60705@gmail.com","threadId":"36982","inReplyTo":"vpqy4whshbj.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-28T14:29:17Z","receivedAt":"2014-06-28T14:29:17Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 28.06.2014 08:01, schrieb Matthieu Moy:\n> Karsten Blees writes:\n> \n>> I still don't like that the invalidation is done in git_config_set, though, as\n>> this is also used to write completely unrelated files.\n> \n> I don't get it. It is used to write the config files. Yes, we trigger a\n> complete reload instead of just changing this particular value in the\n> hashmap, but I do not see \"unrelated files\" in the picture.\n> \n\ngit-cherry-pick, revert: writes '.git/sequencer/opts' (sequencer.c:save_opts)\ngit-mv : writes '.gitmodules' (submodule.c:update_path_in_gitmodules)\n ...and the submodule's '.git/config' (submodule.c:connect_work_tree_and_git_dir)\n\n\nNote that the typical configuration commands git-config/remote/branch seem\nto call git_config_set* immediately before exit, so no need to invalidate\nthe config cache here either.\n\nWhich leaves clone, init and push (transport.c:set_upstreams, seems to be\njust before exit as well).\n\nI didn't look further after finding the example in cmd_clone, so with the\nstatistics above, it may well be the only instance of {git_config; \ngit_config_set; git_config}, I don't know.\n\nI've attached the reverse call hierarchy as generated by Eclipse - quite\nuseful for things like this.\n\n>> Wouldn't it be better to have a 'git_config_refresh()' that could be\n>> used in place of (or before) current 'git_config(callback)' calls? The\n>> initial implementation could just invalidate the config cache. If\n>> there's time and energy to spare, a more advanced version could first\n>> check if any of the involved config files has changed.\n> \n> That would not change the \"xstrdup\" vs \"no xstrdup\" issue, right?\n> \n\nRight. (Unless it turns out invalidation isn't needed after all. But even\nthen using interned strings for the config would be a Good Thing IMO.)\n\n>> The xstrdup() problem could be solved by interning strings (see the\n>> attached patch for a trivial implementation). I.e. allocate each distinct\n>> string only once (and keep it allocated).\n> \n> That's an option. We need to be carefull not to modify any string\n> in-place,\n\nHopefully an illegal non-const cast will be caught in the review process.\n\n\n\n\ngit_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int) : int - /git/config.c\n\tcmd_config(int, const char * *, const char *) : int - /git/builtin/config.c (5 matches)\n\tgit_config_set_in_file(const char *, const char *, const char *) : int - /git/config.c\n\t\tcmd_config(int, const char * *, const char *) : int - /git/builtin/config.c (2 matches)\n\t\tconnect_work_tree_and_git_dir(const char *, const char *) : void - /git/submodule.c\n\t\t\tcmd_mv(int, const char * *, const char *) : int - /git/builtin/mv.c\n\t\tsave_opts(replay_opts *) : void - /git/sequencer.c (8 matches)\n\t\t\tsequencer_pick_revisions(replay_opts *) : int - /git/sequencer.c\n\t\t\t\tcmd_cherry_pick(int, const char * *, const char *) : int - /git/builtin/revert.c\n\t\t\t\tcmd_revert(int, const char * *, const char *) : int - /git/builtin/revert.c\n\t\tupdate_path_in_gitmodules(const char *, const char *) : int - /git/submodule.c\n\t\t\tcmd_mv(int, const char * *, const char *) : int - /git/builtin/mv.c\n\tgit_config_set_multivar(const char *, const char *, const char *, int) : int - /git/config.c\n\t\tadd_branch(const char *, const char *, const char *, int, strbuf *) : int - /git/builtin/remote.c\n\t\t\tadd_branches(remote *, const char * *, const char *) : int - /git/builtin/remote.c\n\t\t\t\tset_remote_branches(const char *, const char * *, int) : int - /git/builtin/remote.c\n\t\t\t\t\tset_branches(int, const char * *) : int - /git/builtin/remote.c\n\t\t\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\t\tadd(int, const char * *) : int - /git/builtin/remote.c\n\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\tcmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c (2 matches)\n\t\tgit_config_set(const char *, const char *) : int - /git/config.c\n\t\t\tadd(int, const char * *) : int - /git/builtin/remote.c (3 matches)\n\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c (2 matches)\n\t\t\tcreate_default_files(const char *) : int - /git/builtin/init-db.c (8 matches)\n\t\t\t\tinit_db(const char *, unsigned int) : int - /git/builtin/init-db.c\n\t\t\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\t\t\t\t\tcmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c\n\t\t\tedit_branch_description(const char *) : int - /git/builtin/branch.c\n\t\t\t\tcmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c\n\t\t\tinit_db(const char *, unsigned int) : int - /git/builtin/init-db.c (2 matches)\n\t\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\t\t\t\tcmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c\n\t\t\tinstall_branch_config(int, const char *, const char *, const char *) : void - /git/branch.c (3 matches)\n\t\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\t\t\t\tset_upstreams(transport *, ref *, int) : void - /git/transport.c\n\t\t\t\t\ttransport_push(transport *, int, const char * *, int, unsigned int *) : int - /git/transport.c\n\t\t\t\t\t\tpush_with_options(transport *, int) : int - /git/builtin/push.c\n\t\t\t\t\t\t\tdo_push(const char *, int) : int - /git/builtin/push.c (2 matches)\n\t\t\t\t\t\t\t\tcmd_push(int, const char * *, const char *) : int - /git/builtin/push.c\n\t\t\t\tsetup_tracking(const char *, const char *, enum branch_track, int) : int - /git/branch.c\n\t\t\t\t\tcreate_branch(const char *, const char *, const char *, int, int, int, int, enum branch_track) : void - /git/branch.c\n\t\t\t\t\t\tcmd_branch(int, const char * *, const char *) : int - /git/builtin/branch.c (2 matches)\n\t\t\t\t\t\tupdate_refs_for_switch(const checkout_opts *, branch_info *, branch_info *) : void - /git/builtin/checkout.c\n\t\t\t\t\t\t\tswitch_branches(const checkout_opts *, branch_info *) : int - /git/builtin/checkout.c\n\t\t\t\t\t\t\t\tcheckout_branch(checkout_opts *, branch_info *) : int - /git/builtin/checkout.c\n\t\t\t\t\t\t\t\t\tcmd_checkout(int, const char * *, const char *) : int - /git/builtin/checkout.c\n\t\t\t\tupdate_head(const ref *, const ref *, const char *) : void - /git/builtin/clone.c\n\t\t\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\t\t\tmingw_mark_as_git_dir(const char *) : void - /git/compat/mingw.c\n\t\t\t\tinit_db(const char *, unsigned int) : int - /git/builtin/init-db.c\n\t\t\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\t\t\t\t\tcmd_init_db(int, const char * *, const char *) : int - /git/builtin/init-db.c\n\t\t\tmv(int, const char * *) : int - /git/builtin/remote.c\n\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\t\trm(int, const char * *) : int - /git/builtin/remote.c\n\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\t\tset_url(int, const char * *) : int - /git/builtin/remote.c\n\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\t\twrite_refspec_config(const char *, const ref *, const ref *, strbuf *) : void - /git/builtin/clone.c\n\t\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\t\tmigrate_file(remote *) : int - /git/builtin/remote.c (3 matches)\n\t\t\tmv(int, const char * *) : int - /git/builtin/remote.c\n\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\tmv(int, const char * *) : int - /git/builtin/remote.c (2 matches)\n\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\tremove_all_fetch_refspecs(const char *, const char *) : int - /git/builtin/remote.c\n\t\t\tset_remote_branches(const char *, const char * *, int) : int - /git/builtin/remote.c\n\t\t\t\tset_branches(int, const char * *) : int - /git/builtin/remote.c\n\t\t\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\tset_url(int, const char * *) : int - /git/builtin/remote.c (3 matches)\n\t\t\tcmd_remote(int, const char * *, const char *) : int - /git/builtin/remote.c\n\t\twrite_one_config(const char *, const char *, void *) : int - /git/builtin/clone.c\n\t\t\twrite_config(string_list *) : void - /git/builtin/clone.c\n\t\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\t\twrite_refspec_config(const char *, const ref *, const ref *, strbuf *) : void - /git/builtin/clone.c\n\t\t\tcmd_clone(int, const char * *, const char *) : int - /git/builtin/clone.c\n\tsave_opts(replay_opts *) : void - /git/sequencer.c\n\t\tsequencer_pick_revisions(replay_opts *) : int - /git/sequencer.c\n\t\t\tcmd_cherry_pick(int, const char * *, const char *) : int - /git/builtin/revert.c\n\t\t\tcmd_revert(int, const char * *, const char *) : int - /git/builtin/revert.c"},{"id":"245154","messageId":"CAPig+cSRBk5Q2amvFXyjH4dpoP83qbYUEUizE7v38Wh8Ofnfpw@mail.gmail.com","threadId":"36982","inReplyTo":"53ABD78E.3050800@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-29T11:01:31Z","receivedAt":"2014-06-29T11:01:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>> Use git_config_get_string instead of git_config to take advantage of\n>>> the config hash-table api which provides a cleaner control flow.\n>>>\n>>> Signed-off-by: Tanay Abhra \n>>> ---\n>>> notes-utils.c | 31 +++++++++++++++----------------\n>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>\n>>> diff --git a/notes-utils.c b/notes-utils.c\n>>> index a0b1d7b..fdc9912 100644\n>>> --- a/notes-utils.c\n>>> +++ b/notes-utils.c\n>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>> return NULL;\n>>> }\n>>>\n>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>> {\n>>> - struct notes_rewrite_cfg *c = cb;\n>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>> - c->enabled = git_config_bool(k, v);\n>>> - return 0;\n>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>> + struct strbuf key = STRBUF_INIT;\n>>> + const char *v;\n>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>> +\n>>> + if (!git_config_get_string(key.buf, &v))\n>>> + c->enabled = git_config_bool(key.buf, v);\n>>> +\n>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>> if (!v)\n>>> - return config_error_nonbool(k);\n>>> + config_error_nonbool(\"notes.rewritemode\");\n>>\n>> There's a behavior change here. In the original code, the callback\n>> function would return -1, which would cause the program to die() if\n>> the config.c:die_on_error flag was set. The new code merely emits an\n>> error.\n>\n> Is this change serious enough? Can I ignore it?\n\nI don't know. Even within this single function there is no consistency\nabout whether such problems should die() or just emit a message and\ncontinue. For instance:\n\n- if \"notes.rewritemode\" is bool, it die()s.\n\n- if \"notes.rewritemode\" doesn't specify a recognized mode, it\nerror()s but continues\n\n- if \"notes.rewriteref\" doesn't start with \"refs/notes/, it warning()s\nand continues\n\nIt would be nice to hear an opinion from someone more invested in the\nconfig system.\n\n>>> c->combine = parse_combine_notes_fn(v);\n>>\n>> Worse: Though you correctly emit an error when 'v' is NULL, you then\n>> (incorrectly) invoke parse_combine_notes_fn() with that NULL value,\n>> which will result in a crash.\n>>\n>\n> Noted.\n>\n>>> - if (!c->combine) {\n>>> + if (!c->combine)\n>>> error(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n>>> - return 1;\n>>> - }\n>>> - return 0;\n>>> - } else if (!c->refs_from_env && !strcmp(k, \"notes.rewriteref\")) {\n>>> + }\n>>> + if (!c->refs_from_env && !git_config_get_string(\"notes.rewriteref\", &v)) {\n>>> /* note that a refs/ prefix is implied in the\n>>> * underlying for_each_glob_ref */\n>>> if (starts_with(v, \"refs/notes/\"))\n>>> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>> else\n>>> warning(_(\"Refusing to rewrite notes in %s\"\n>>> \" (outside of refs/notes/)\"), v);\n>>> - return 0;\n>>> }\n>>> -\n>>> - return 0;\n>>> + strbuf_release(&key);\n>>\n>> It would be better to release the strbuf immediately after its final\n>> use rather than waiting until the end of function. Not only does that\n>> reduce cognitive load on people reading the code, but it also reduces\n>> likelihood of 'key' being leaked if some future programmer inserts an\n>> early 'return' into the function for some reason.\n>>\n>\n> Noted. Thanks.\n>\n>>> }\n>>>\n>>>\n>>> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)\n>>> c->refs_from_env = 1;\n>>> string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);\n>>> }\n>>> - git_config(notes_rewrite_config, c);\n>>> + notes_rewrite_config(c);\n>>> if (!c->enabled || !c->refs->nr) {\n>>> string_list_clear(c->refs, 0);\n>>> free(c->refs);\n>>> --\n>>> 1.9.0.GIT\n>>\n"},{"id":"245155","messageId":"CAPig+cRgszpFzvt6JiD=+4DGDyPxdwc=1GBm1bH_DHM1UObD8Q@mail.gmail.com","threadId":"36982","inReplyTo":"53ABD52B.3030502@gmail.com","subject":"Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-29T11:06:04Z","receivedAt":"2014-06-29T11:06:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 26, 2014 at 4:09 AM, Tanay Abhra wrote:\n>\n> On 6/25/2014 10:15 AM, Eric Sunshine wrote:\n>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>> diff --git a/branch.c b/branch.c\n>>> index 660097b..c9a2a0d 100644\n>>> --- a/branch.c\n>>> +++ b/branch.c\n>>> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n>>> int read_branch_desc(struct strbuf *buf, const char *branch_name)\n>>> {\n>>> - struct branch_desc_cb cb;\n>>> + const char *value = NULL;\n>>> + struct branch_desc desc;\n>>> struct strbuf name = STRBUF_INIT;\n>>> strbuf_addf(&name, \"branch.%s.description\", branch_name);\n>>> - cb.config_name = name.buf;\n>>> - cb.value = NULL;\n>>> - if (git_config(read_branch_desc_cb, &cb) < 0) {\n>>> + desc.config_name = name.buf;\n>>> + desc.value = NULL;\n>>> + git_config_get_string(desc.config_name, &value);\n>>> + if (git_config_string(&desc.value, desc.config_name, value) < 0) {\n>>\n>> Although it works in this case, it's somewhat ugly that you ignore the\n>> return value of git_config_get_string(), and a person reading the code\n>> has to spend extra time digging into git_config_string() to figure out\n>> why this is safe. If might be clearer for future readers by rephrasing\n>> like this:\n>>\n>> if (git_config_get_string(desc.config_name, &value) < 0 ||\n>> git_config_string(&desc.value, desc.config_name, value) < 0) {\n>>\n>\n> Noted, also didn't the old code leak desc.value as it was xstrduped\n> by git_config_string()? Thanks for the review.\n\nLooks that way.\n"},{"id":"245156","messageId":"vpq7g3zsz05.fsf@anie.imag.fr","threadId":"36982","inReplyTo":"53AED13D.60705@gmail.com","subject":"Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-29T12:04:26Z","receivedAt":"2014-06-29T12:04:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karsten Blees writes:\n\n> Am 28.06.2014 08:01, schrieb Matthieu Moy:\n>> Karsten Blees writes:\n>> \n>>> I still don't like that the invalidation is done in git_config_set, though, as\n>>> this is also used to write completely unrelated files.\n>> \n>> I don't get it. It is used to write the config files. Yes, we trigger a\n>> complete reload instead of just changing this particular value in the\n>> hashmap, but I do not see \"unrelated files\" in the picture.\n>> \n>\n> git-cherry-pick, revert: writes '.git/sequencer/opts' (sequencer.c:save_opts)\n> git-mv : writes '.gitmodules' (submodule.c:update_path_in_gitmodules)\n> ...and the submodule's '.git/config' (submodule.c:connect_work_tree_and_git_dir)\n\nIn the previous patch series, the cache was only about the system, user\nand repo config files. So, .git/sequencer/opts, .gitmodules are totally\nindependant. In the latest series, there's a new notion of config_set.\n.gitmodules, .git/sequencer/opts and the usual config files are in\ndifferent config sets, and their cache is invalidated independently.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245183","messageId":"53B16748.8080703@gmail.com","threadId":"36982","inReplyTo":"CAPig+cSRBk5Q2amvFXyjH4dpoP83qbYUEUizE7v38Wh8Ofnfpw@mail.gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-30T13:34:00Z","receivedAt":"2014-06-30T13:34:00Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 29.06.2014 13:01, schrieb Eric Sunshine:\n> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>>> Use git_config_get_string instead of git_config to take advantage of\n>>>> the config hash-table api which provides a cleaner control flow.\n>>>>\n>>>> Signed-off-by: Tanay Abhra \n>>>> ---\n>>>> notes-utils.c | 31 +++++++++++++++----------------\n>>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>>\n>>>> diff --git a/notes-utils.c b/notes-utils.c\n>>>> index a0b1d7b..fdc9912 100644\n>>>> --- a/notes-utils.c\n>>>> +++ b/notes-utils.c\n>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>>> return NULL;\n>>>> }\n>>>>\n>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>>> {\n>>>> - struct notes_rewrite_cfg *c = cb;\n>>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>>> - c->enabled = git_config_bool(k, v);\n>>>> - return 0;\n>>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>>> + struct strbuf key = STRBUF_INIT;\n>>>> + const char *v;\n>>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>>> +\n>>>> + if (!git_config_get_string(key.buf, &v))\n>>>> + c->enabled = git_config_bool(key.buf, v);\n>>>> +\n>>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>>> if (!v)\n>>>> - return config_error_nonbool(k);\n>>>> + config_error_nonbool(\"notes.rewritemode\");\n>>>\n>>> There's a behavior change here. In the original code, the callback\n>>> function would return -1, which would cause the program to die() if\n>>> the config.c:die_on_error flag was set. The new code merely emits an\n>>> error.\n>>\n>> Is this change serious enough? Can I ignore it?\n\nIMO its better to Fail Fast than continue with some invalid config (which\nmay lead to more severe errors such as data corruption / data loss).\n\n> \n> I don't know. Even within this single function there is no consistency\n> about whether such problems should die() or just emit a message and\n> continue. For instance:\n> \n> - if \"notes.rewritemode\" is bool, it die()s.\n> \n> - if \"notes.rewritemode\" doesn't specify a recognized mode, it\n> error()s but continues\n> \n\nI think this would also die in git_parse_source():\n...\n if (get_value(fn, data, var) < 0)\n break;\n }\n if (cf->die_on_error)\n die(\"bad config file line %d in %s\", cf->linenr, cf->name);\n...\n\n(AFAICT, die_on_error is always true, except if invoked via 'git-config\n--blob', which isn't used anywhere...)\n\n\nThis, however, raises another issue: switching to the config cache looses\nfile/line-precise error reporting for semantic errors. I don't know if\nthis feature is important enough to do something about it, though. A\nmessage of the form \"Key 'xyz' is bad\" should usually enable a user to\nlocate the problematic file and line.\n"},{"id":"245187","messageId":"CAPig+cSpQ2NS9R-jsUvGFB0Uk5kqwhsFTm3RKhzcx+8FLyQgsw@mail.gmail.com","threadId":"36982","inReplyTo":"53B16748.8080703@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-30T14:32:16Z","receivedAt":"2014-06-30T14:32:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees wrote:\n> Am 29.06.2014 13:01, schrieb Eric Sunshine:\n>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>>>> Use git_config_get_string instead of git_config to take advantage of\n>>>>> the config hash-table api which provides a cleaner control flow.\n>>>>>\n>>>>> Signed-off-by: Tanay Abhra \n>>>>> ---\n>>>>> notes-utils.c | 31 +++++++++++++++----------------\n>>>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>>>\n>>>>> diff --git a/notes-utils.c b/notes-utils.c\n>>>>> index a0b1d7b..fdc9912 100644\n>>>>> --- a/notes-utils.c\n>>>>> +++ b/notes-utils.c\n>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>>>> return NULL;\n>>>>> }\n>>>>>\n>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>>>> {\n>>>>> - struct notes_rewrite_cfg *c = cb;\n>>>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>>>> - c->enabled = git_config_bool(k, v);\n>>>>> - return 0;\n>>>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>>>> + struct strbuf key = STRBUF_INIT;\n>>>>> + const char *v;\n>>>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>>>> +\n>>>>> + if (!git_config_get_string(key.buf, &v))\n>>>>> + c->enabled = git_config_bool(key.buf, v);\n>>>>> +\n>>>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>>>> if (!v)\n>>>>> - return config_error_nonbool(k);\n>>>>> + config_error_nonbool(\"notes.rewritemode\");\n>>>>\n>>>> There's a behavior change here. In the original code, the callback\n>>>> function would return -1, which would cause the program to die() if\n>>>> the config.c:die_on_error flag was set. The new code merely emits an\n>>>> error.\n>>>\n>>> Is this change serious enough? Can I ignore it?\n>\n> IMO its better to Fail Fast than continue with some invalid config (which\n> may lead to more severe errors such as data corruption / data loss).\n>\n>>\n>> I don't know. Even within this single function there is no consistency\n>> about whether such problems should die() or just emit a message and\n>> continue. For instance:\n>>\n>> - if \"notes.rewritemode\" is bool, it die()s.\n>>\n>> - if \"notes.rewritemode\" doesn't specify a recognized mode, it\n>> error()s but continues\n>>\n>\n> I think this would also die in git_parse_source():\n> ...\n> if (get_value(fn, data, var) < 0)\n> break;\n> }\n> if (cf->die_on_error)\n> die(\"bad config file line %d in %s\", cf->linenr, cf->name);\n> ...\n\nOne would expect so, but notes-utils.c:notes_rewrite_config() is\nactually doing this:\n\n if (!c->combine) {\n error(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n return 1;\n }\n\nRather than returning the -1 result of error(), which would make\ngit_parse_source() die(), it's explicitly returning 1, which\nget_parse_source() ignores.\n\n> (AFAICT, die_on_error is always true, except if invoked via 'git-config\n> --blob', which isn't used anywhere...)\n>\n> This, however, raises another issue: switching to the config cache looses\n> file/line-precise error reporting for semantic errors. I don't know if\n> this feature is important enough to do something about it, though. A\n> message of the form \"Key 'xyz' is bad\" should usually enable a user to\n> locate the problematic file and line.\n"},{"id":"245188","messageId":"53B17696.3060808@gmail.com","threadId":"36982","inReplyTo":"53B16748.8080703@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-30T14:39:18Z","receivedAt":"2014-06-30T14:39:18Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\nOn 6/30/2014 7:04 PM, Karsten Blees wrote:\n> Am 29.06.2014 13:01, schrieb Eric Sunshine:\n>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>>>> Use git_config_get_string instead of git_config to take advantage of\n>>>>> the config hash-table api which provides a cleaner control flow.\n>>>>>\n>>>>> Signed-off-by: Tanay Abhra \n>>>>> ---\n>>>>> notes-utils.c | 31 +++++++++++++++----------------\n>>>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>>>\n>>>>> diff --git a/notes-utils.c b/notes-utils.c\n>>>>> index a0b1d7b..fdc9912 100644\n>>>>> --- a/notes-utils.c\n>>>>> +++ b/notes-utils.c\n>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>>>> return NULL;\n>>>>> }\n>>>>>\n>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>>>> {\n>>>>> - struct notes_rewrite_cfg *c = cb;\n>>>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>>>> - c->enabled = git_config_bool(k, v);\n>>>>> - return 0;\n>>>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>>>> + struct strbuf key = STRBUF_INIT;\n>>>>> + const char *v;\n>>>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>>>> +\n>>>>> + if (!git_config_get_string(key.buf, &v))\n>>>>> + c->enabled = git_config_bool(key.buf, v);\n>>>>> +\n>>>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>>>> if (!v)\n>>>>> - return config_error_nonbool(k);\n>>>>> + config_error_nonbool(\"notes.rewritemode\");\n>>>>\n>>>> There's a behavior change here. In the original code, the callback\n>>>> function would return -1, which would cause the program to die() if\n>>>> the config.c:die_on_error flag was set. The new code merely emits an\n>>>> error.\n>>>\n>>> Is this change serious enough? Can I ignore it?\n> \n> IMO its better to Fail Fast than continue with some invalid config (which\n> may lead to more severe errors such as data corruption / data loss).\n\nNoted but, what I am trying to do with the rewrite is emit an error and\nnot set the value if the value found is a NULL. The only change is that\nprogram will not crash in this case and warn the user not set a NULL value for\na non boolean key.\nThis won't lead to severe errors as the value will not be set if found value\nis a NULL.\n\n>>\n>> I don't know. Even within this single function there is no consistency\n>> about whether such problems should die() or just emit a message and\n>> continue. For instance:\n>>\n>> - if \"notes.rewritemode\" is bool, it die()s.\n>>\n>> - if \"notes.rewritemode\" doesn't specify a recognized mode, it\n>> error()s but continues\n>>\n> \n> I think this would also die in git_parse_source():\n> ...\n> if (get_value(fn, data, var) < 0)\n> break;\n> }\n> if (cf->die_on_error)\n> die(\"bad config file line %d in %s\", cf->linenr, cf->name);\n> ...\n> \n> (AFAICT, die_on_error is always true, except if invoked via 'git-config\n> --blob', which isn't used anywhere...)\n> \n\nNoted.\n\n> This, however, raises another issue: switching to the config cache looses\n> file/line-precise error reporting for semantic errors. I don't know if\n> this feature is important enough to do something about it, though. A\n> message of the form \"Key 'xyz' is bad\" should usually enable a user to\n> locate the problematic file and line.\n> \n\nHmn, but during the config cache construction we parse key-value pairs through\ngit_config() which still warns users about semantic errors. This happened\nyesterday only when I was writing tests for the new API.\n\nThanks for the review.\n\nCheers,\nTanay Abhra.\n"},{"id":"245189","messageId":"53B17A1F.2030508@gmail.com","threadId":"36982","inReplyTo":"CAPig+cSpQ2NS9R-jsUvGFB0Uk5kqwhsFTm3RKhzcx+8FLyQgsw@mail.gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-30T14:54:23Z","receivedAt":"2014-06-30T14:54:23Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 30.06.2014 16:32, schrieb Eric Sunshine:\n> On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees wrote:\n>> Am 29.06.2014 13:01, schrieb Eric Sunshine:\n>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>>>>> Use git_config_get_string instead of git_config to take advantage of\n>>>>>> the config hash-table api which provides a cleaner control flow.\n>>>>>>\n>>>>>> Signed-off-by: Tanay Abhra \n>>>>>> ---\n>>>>>> notes-utils.c | 31 +++++++++++++++----------------\n>>>>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>>>>\n>>>>>> diff --git a/notes-utils.c b/notes-utils.c\n>>>>>> index a0b1d7b..fdc9912 100644\n>>>>>> --- a/notes-utils.c\n>>>>>> +++ b/notes-utils.c\n>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>>>>> return NULL;\n>>>>>> }\n>>>>>>\n>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>>>>> {\n>>>>>> - struct notes_rewrite_cfg *c = cb;\n>>>>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>>>>> - c->enabled = git_config_bool(k, v);\n>>>>>> - return 0;\n>>>>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>>>>> + struct strbuf key = STRBUF_INIT;\n>>>>>> + const char *v;\n>>>>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>>>>> +\n>>>>>> + if (!git_config_get_string(key.buf, &v))\n>>>>>> + c->enabled = git_config_bool(key.buf, v);\n>>>>>> +\n>>>>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>>>>> if (!v)\n>>>>>> - return config_error_nonbool(k);\n>>>>>> + config_error_nonbool(\"notes.rewritemode\");\n>>>>>\n>>>>> There's a behavior change here. In the original code, the callback\n>>>>> function would return -1, which would cause the program to die() if\n>>>>> the config.c:die_on_error flag was set. The new code merely emits an\n>>>>> error.\n>>>>\n>>>> Is this change serious enough? Can I ignore it?\n>>\n>> IMO its better to Fail Fast than continue with some invalid config (which\n>> may lead to more severe errors such as data corruption / data loss).\n>>\n>>>\n>>> I don't know. Even within this single function there is no consistency\n>>> about whether such problems should die() or just emit a message and\n>>> continue. For instance:\n>>>\n>>> - if \"notes.rewritemode\" is bool, it die()s.\n>>>\n>>> - if \"notes.rewritemode\" doesn't specify a recognized mode, it\n>>> error()s but continues\n>>>\n>>\n>> I think this would also die in git_parse_source():\n>> ...\n>> if (get_value(fn, data, var) < 0)\n>> break;\n>> }\n>> if (cf->die_on_error)\n>> die(\"bad config file line %d in %s\", cf->linenr, cf->name);\n>> ...\n> \n> One would expect so, but notes-utils.c:notes_rewrite_config() is\n> actually doing this:\n> \n> if (!c->combine) {\n> error(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n> return 1;\n> }\n> \n> Rather than returning the -1 result of error(), which would make\n> git_parse_source() die(), it's explicitly returning 1, which\n> get_parse_source() ignores.\n> \n\nAhh...I missed the '< 0', sorry.\n"},{"id":"245191","messageId":"53B1889D.3010506@gmail.com","threadId":"36982","inReplyTo":"53B17696.3060808@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-30T15:56:13Z","receivedAt":"2014-06-30T15:56:13Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 30.06.2014 16:39, schrieb Tanay Abhra:\n> \n> On 6/30/2014 7:04 PM, Karsten Blees wrote:\n>> Am 29.06.2014 13:01, schrieb Eric Sunshine:\n>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>>>>> Use git_config_get_string instead of git_config to take advantage of\n>>>>>> the config hash-table api which provides a cleaner control flow.\n>>>>>>\n>>>>>> Signed-off-by: Tanay Abhra \n>>>>>> ---\n>>>>>> notes-utils.c | 31 +++++++++++++++----------------\n>>>>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>>>>\n>>>>>> diff --git a/notes-utils.c b/notes-utils.c\n>>>>>> index a0b1d7b..fdc9912 100644\n>>>>>> --- a/notes-utils.c\n>>>>>> +++ b/notes-utils.c\n>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>>>>> return NULL;\n>>>>>> }\n>>>>>>\n>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>>>>> {\n>>>>>> - struct notes_rewrite_cfg *c = cb;\n>>>>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>>>>> - c->enabled = git_config_bool(k, v);\n>>>>>> - return 0;\n>>>>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>>>>> + struct strbuf key = STRBUF_INIT;\n>>>>>> + const char *v;\n>>>>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>>>>> +\n>>>>>> + if (!git_config_get_string(key.buf, &v))\n>>>>>> + c->enabled = git_config_bool(key.buf, v);\n>>>>>> +\n>>>>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>>>>> if (!v)\n>>>>>> - return config_error_nonbool(k);\n>>>>>> + config_error_nonbool(\"notes.rewritemode\");\n>>>>>\n>>>>> There's a behavior change here. In the original code, the callback\n>>>>> function would return -1, which would cause the program to die() if\n>>>>> the config.c:die_on_error flag was set. The new code merely emits an\n>>>>> error.\n>>>>\n>>>> Is this change serious enough? Can I ignore it?\n>>\n>> IMO its better to Fail Fast than continue with some invalid config (which\n>> may lead to more severe errors such as data corruption / data loss).\n> \n> Noted but, what I am trying to do with the rewrite is emit an error and\n> not set the value if the value found is a NULL.\n\nIf you don't set the value and continue, git will proceed with the variable's\ndefault setting.\n\nWhich may not be too harmful in some cases, but if a user changes:\n\n gc.pruneexpire=4.weeks.ago\n\nto\n\n gc.pruneexpire=4.monhts.ago\n\n(note the typo), the next git-gc will warn the user and then happily throw\naway data that the user intended to keep (default is 2.weeks.ago).\n\nThus I think git should die() if it encounters an invalid config setting.\n\n> \n>> This, however, raises another issue: switching to the config cache looses\n>> file/line-precise error reporting for semantic errors. I don't know if\n>> this feature is important enough to do something about it, though. A\n>> message of the form \"Key 'xyz' is bad\" should usually enable a user to\n>> locate the problematic file and line.\n>>\n> \n> Hmn, but during the config cache construction we parse key-value pairs through\n> git_config() which still warns users about semantic errors.\n\nIf I'm not mistaken you only detect _syntax_ errors when loading the file (i.e.\nwhether the config file is structurally correct).\n\nThe semantic value and correctness of a key (e.g. whether its a boolean or an\nint or a string that denotes a known merge algorithm) is only checked when it is\naccessed via git_config_get_. And at this point, : information\nis already lost.\n\nWith the callback approach, both syntactic (structure) and semantic (meaning)\nerrors were checked at load time, resulting in\n\n die(\"bad config file line %d in %s\", cf->linenr, cf->name);\n\nif the callback returned -1.\n"},{"id":"245192","messageId":"53B18E79.8030500@gmail.com","threadId":"36982","inReplyTo":"53B1889D.3010506@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-30T16:21:13Z","receivedAt":"2014-06-30T16:21:13Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 6/30/2014 9:26 PM, Karsten Blees wrote:\n> Am 30.06.2014 16:39, schrieb Tanay Abhra:\n>>\n>> On 6/30/2014 7:04 PM, Karsten Blees wrote:\n>>> Am 29.06.2014 13:01, schrieb Eric Sunshine:\n>>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n>>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>>>>>> Use git_config_get_string instead of git_config to take advantage of\n>>>>>>> the config hash-table api which provides a cleaner control flow.\n>>>>>>>\n>>>>>>> Signed-off-by: Tanay Abhra \n>>>>>>> ---\n>>>>>>> notes-utils.c | 31 +++++++++++++++----------------\n>>>>>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>>>>>\n>>>>>>> diff --git a/notes-utils.c b/notes-utils.c\n>>>>>>> index a0b1d7b..fdc9912 100644\n>>>>>>> --- a/notes-utils.c\n>>>>>>> +++ b/notes-utils.c\n>>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>>>>>> return NULL;\n>>>>>>> }\n>>>>>>>\n>>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>>>>>> {\n>>>>>>> - struct notes_rewrite_cfg *c = cb;\n>>>>>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>>>>>> - c->enabled = git_config_bool(k, v);\n>>>>>>> - return 0;\n>>>>>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>>>>>> + struct strbuf key = STRBUF_INIT;\n>>>>>>> + const char *v;\n>>>>>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>>>>>> +\n>>>>>>> + if (!git_config_get_string(key.buf, &v))\n>>>>>>> + c->enabled = git_config_bool(key.buf, v);\n>>>>>>> +\n>>>>>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>>>>>> if (!v)\n>>>>>>> - return config_error_nonbool(k);\n>>>>>>> + config_error_nonbool(\"notes.rewritemode\");\n>>>>>>\n>>>>>> There's a behavior change here. In the original code, the callback\n>>>>>> function would return -1, which would cause the program to die() if\n>>>>>> the config.c:die_on_error flag was set. The new code merely emits an\n>>>>>> error.\n>>>>>\n>>>>> Is this change serious enough? Can I ignore it?\n>>>\n>>> IMO its better to Fail Fast than continue with some invalid config (which\n>>> may lead to more severe errors such as data corruption / data loss).\n>>\n>> Noted but, what I am trying to do with the rewrite is emit an error and\n>> not set the value if the value found is a NULL.\n> \n> If you don't set the value and continue, git will proceed with the variable's\n> default setting.\n> \n> Which may not be too harmful in some cases, but if a user changes:\n> \n> gc.pruneexpire=4.weeks.ago\n> \n> to\n> \n> gc.pruneexpire=4.monhts.ago\n> \n> (note the typo), the next git-gc will warn the user and then happily throw\n> away data that the user intended to keep (default is 2.weeks.ago).\n> \n> Thus I think git should die() if it encounters an invalid config setting.\n\nOkay, point noted.\n\n>>\n>>> This, however, raises another issue: switching to the config cache looses\n>>> file/line-precise error reporting for semantic errors. I don't know if\n>>> this feature is important enough to do something about it, though. A\n>>> message of the form \"Key 'xyz' is bad\" should usually enable a user to\n>>> locate the problematic file and line.\n>>>\n>>\n>> Hmn, but during the config cache construction we parse key-value pairs through\n>> git_config() which still warns users about semantic errors.\n> \n> If I'm not mistaken you only detect _syntax_ errors when loading the file (i.e.\n> whether the config file is structurally correct).\n> \n> The semantic value and correctness of a key (e.g. whether its a boolean or an\n> int or a string that denotes a known merge algorithm) is only checked when it is\n> accessed via git_config_get_. And at this point, : information\n> is already lost.\n> \n> With the callback approach, both syntactic (structure) and semantic (meaning)\n> errors were checked at load time, resulting in\n> \n> die(\"bad config file line %d in %s\", cf->linenr, cf->name);\n> \n> if the callback returned -1.\n> \n\nYup, you are right, we check only syntax error when loading the file for the cache.\nI could save the : when the loading the file for future error\nreporting. Thanks.\n"},{"id":"245209","messageId":"xmqqioniz3m3.fsf@gitster.dls.corp.google.com","threadId":"36982","inReplyTo":"53B1889D.3010506@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-30T17:52:52Z","receivedAt":"2014-06-30T17:52:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karsten Blees writes:\n\n> Which may not be too harmful in some cases, but if a user changes:\n>\n> gc.pruneexpire=4.weeks.ago\n>\n> to\n>\n> gc.pruneexpire=4.monhts.ago\n>\n> (note the typo), the next git-gc will warn the user and then happily throw\n> away data that the user intended to keep (default is 2.weeks.ago).\n>\n> Thus I think git should die() if it encounters an invalid config setting.\n\nYes but not at the parsing time. Only when we are about to _use_\nthe value for pruneexpire as a time duration we should die of the\nerror. Diagnosing an error early is a separate matter, but if the\noperation does not care about the typo we shouldn't die.\n"},{"id":"245226","messageId":"vpqha31fpc5.fsf@anie.imag.fr","threadId":"36982","inReplyTo":"53B17696.3060808@gmail.com","subject":"Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-01T08:36:10Z","receivedAt":"2014-07-01T08:36:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> On 6/30/2014 7:04 PM, Karsten Blees wrote:\n>> Am 29.06.2014 13:01, schrieb Eric Sunshine:\n>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote:\n>>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:\n>>>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:\n>>>>>> Use git_config_get_string instead of git_config to take advantage of\n>>>>>> the config hash-table api which provides a cleaner control flow.\n>>>>>>\n>>>>>> Signed-off-by: Tanay Abhra \n>>>>>> ---\n>>>>>> notes-utils.c | 31 +++++++++++++++----------------\n>>>>>> 1 file changed, 15 insertions(+), 16 deletions(-)\n>>>>>>\n>>>>>> diff --git a/notes-utils.c b/notes-utils.c\n>>>>>> index a0b1d7b..fdc9912 100644\n>>>>>> --- a/notes-utils.c\n>>>>>> +++ b/notes-utils.c\n>>>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)\n>>>>>> return NULL;\n>>>>>> }\n>>>>>>\n>>>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)\n>>>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)\n>>>>>> {\n>>>>>> - struct notes_rewrite_cfg *c = cb;\n>>>>>> - if (starts_with(k, \"notes.rewrite.\") && !strcmp(k+14, c->cmd)) {\n>>>>>> - c->enabled = git_config_bool(k, v);\n>>>>>> - return 0;\n>>>>>> - } else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n>>>>>> + struct strbuf key = STRBUF_INIT;\n>>>>>> + const char *v;\n>>>>>> + strbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n>>>>>> +\n>>>>>> + if (!git_config_get_string(key.buf, &v))\n>>>>>> + c->enabled = git_config_bool(key.buf, v);\n>>>>>> +\n>>>>>> + if (!c->mode_from_env && !git_config_get_string(\"notes.rewritemode\", &v)) {\n>>>>>> if (!v)\n>>>>>> - return config_error_nonbool(k);\n>>>>>> + config_error_nonbool(\"notes.rewritemode\");\n>>>>>\n>>>>> There's a behavior change here. In the original code, the callback\n>>>>> function would return -1, which would cause the program to die() if\n>>>>> the config.c:die_on_error flag was set. The new code merely emits an\n>>>>> error.\n>>>>\n>>>> Is this change serious enough? Can I ignore it?\n>> \n>> IMO its better to Fail Fast than continue with some invalid config (which\n>> may lead to more severe errors such as data corruption / data loss).\n>\n> Noted but, what I am trying to do with the rewrite is emit an error and\n> not set the value if the value found is a NULL. The only change is that\n> program will not crash in this case and warn the user not set a NULL value for\n> a non boolean key.\n> This won't lead to severe errors as the value will not be set if found value\n> is a NULL.\n\nThe change probably makes sense, but as much as possible, keep\nrefactoring patches and patches introducing a semantic change separate.\nIt's much easier to review, and helps user digging history and finding\none of the commits.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}