{"thread":{"id":"37247","subject":"[PATCH v4 0/5] git_config callers rewritten with the new config cache API","startedAt":"2014-07-30T13:39:04Z","lastAt":"2014-07-31T12:13:31Z","messageCount":16,"participants":["Tanay Abhra","Matthieu Moy","Ramsay Jones"],"isPatch":true,"patchVersion":4,"patchTotal":5},"messages":[{"id":"246963","messageId":"1406727549-22334-1-git-send-email-tanayabh@gmail.com","threadId":"37247","inReplyTo":null,"subject":"[PATCH v4 0/5] git_config callers rewritten with the new config cache API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T13:39:04Z","receivedAt":"2014-07-30T13:39:04Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"[PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked.\n\tOne thing to check is if the config variables I changed in the series\n\tare single valued or multi valued. Though I have tried to ascertain\n\tif the variable was single valued or not by reading the docs and code,\n\tmistakes may creep in.\n\n[PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion.\n\tAlso, new helpers introduced in v7 of the config-set API series have been used.\n\nThis series builds on the top of 4c715ebb in pu (ta/config-set).\nSee [1] for the documentation of the new API functions and a general description of the\nnew API.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/254286/\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/252334\n\nTanay Abhra (5):\n\n alias.c     | 25 ++++++-------------------\n branch.c    | 24 ++++--------------------\n imap-send.c | 61 +++++++++++++++++++++++++++----------------------------------\n notes.c     | 29 ++++++++++++++---------------\n pager.c     | 40 +++++++++++++---------------------------\n 5 files changed, 64 insertions(+), 115 deletions(-)\n\n-- \n1.9.0.GIT\n"},{"id":"246967","messageId":"1406727549-22334-2-git-send-email-tanayabh@gmail.com","threadId":"37247","inReplyTo":"1406727549-22334-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v4 1/5] pager.c: replace `git_config()` with `git_config_get_value()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T13:39:05Z","receivedAt":"2014-07-30T13:39:05Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use `git_config_get_value()` instead of `git_config()` to take advantage of\nthe config-set API which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n pager.c | 40 +++++++++++++---------------------------\n 1 file changed, 13 insertions(+), 27 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 8b5cbc5..b7eb7e7 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+/* 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 = data;\n-\tif (starts_with(var, \"pager.\") && !strcmp(var + 6, c->cmd)) {\n-\t\tint b = git_config_maybe_bool(var, value);\n+\tint want = -1;\n+\tstruct strbuf key = STRBUF_INIT;\n+\tconst char *value = NULL;\n+\tstrbuf_addf(&key, \"pager.%s\", cmd);\n+\tif (!git_config_get_value(key.buf, &value)) {\n+\t\tint b = git_config_maybe_bool(key.buf, value);\n \t\tif (b >= 0)\n-\t\t\tc->want = b;\n+\t\t\twant = b;\n \t\telse {\n-\t\t\tc->want = 1;\n-\t\t\tc->value = xstrdup(value);\n+\t\t\twant = 1;\n+\t\t\tpager_program = 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+\tstrbuf_release(&key);\n+\treturn want;\n }\n-- \n1.9.0.GIT\n"},{"id":"246964","messageId":"1406727549-22334-3-git-send-email-tanayabh@gmail.com","threadId":"37247","inReplyTo":"1406727549-22334-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T13:39:06Z","receivedAt":"2014-07-30T13:39:06Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use `git_config_get_value_multi()` instead of `git_config()` to take\nadvantage of the config-set API which provides a cleaner control flow,\nalso previously 'string_list_add_refs_by_glob()' was called even when\nthe retrieved value was NULL, correct it while we are at it.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n notes.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 5fe691d..abb0ce0 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,7 +1028,8 @@ 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-\tint load_config_refs = 0;\n+\tconst struct string_list *values;\n+\tint load_config_refs = 0, i;\n \tdisplay_notes_refs.strdup_strings = 1;\n \n \tassert(!display_notes_trees);\n@@ -1058,7 +1046,18 @@ 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) {\n+\t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n+\t\tif (values) {\n+\t\t\tfor (i = 0; i < values->nr; i++) {\n+\t\t\t\tif (!values->items[i].string)\n+\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n+\t\t\t\telse\n+\t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n+\t\t\t\t\t\t\t\t     values->items[i].string);\n+\t\t\t}\n+\t\t}\n+\t}\n \n \tif (opt) {\n \t\tstruct string_list_item *item;\n-- \n1.9.0.GIT\n"},{"id":"246966","messageId":"1406727549-22334-4-git-send-email-tanayabh@gmail.com","threadId":"37247","inReplyTo":"1406727549-22334-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v4 3/5] imap-send.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T13:39:07Z","receivedAt":"2014-07-30T13:39:07Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use `git_config_get_*()` family instead of `git_config()` to take advantage of\nthe config-set API which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n imap-send.c | 61 +++++++++++++++++++++++++++----------------------------------\n 1 file changed, 27 insertions(+), 34 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 524fbab..586bdd8 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1326,43 +1326,36 @@ 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-\tif (!skip_prefix(key, \"imap.\", &key))\n-\t\treturn 0;\n+\tconst char *val = NULL;\n+\n+\tgit_config_get_bool(\"imap.sslverify\", &server.ssl_verify);\n+\tgit_config_get_bool(\"imap.preformattedhtml\", &server.use_html);\n+\tgit_config_get_string(\"imap.folder\", &imap_folder);\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-\t\t\tserver.use_ssl = 1;\n+\tif (!git_config_get_value(\"imap.host\", &val)) {\n+\t\tif (!val) {\n+\t\t\tconfig_error_nonbool(\"imap.host\");\n+\t\t\tgit_die_config(\"imap.host\");\n+\t\t} else {\n+\t\t\tif (starts_with(val, \"imap:\"))\n+\t\t\t\tval += 5;\n+\t\t\telse if (starts_with(val, \"imaps:\")) {\n+\t\t\t\tval += 6;\n+\t\t\t\tserver.use_ssl = 1;\n+\t\t\t}\n+\t\t\tif (starts_with(val, \"//\"))\n+\t\t\t\tval += 2;\n+\t\t\tserver.host = xstrdup(val);\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+\t}\n \n-\treturn 0;\n+\tgit_config_get_string(\"imap.user\", &server.user);\n+\tgit_config_get_string(\"imap.pass\", &server.pass);\n+\tgit_config_get_int(\"imap.port\", &server.port);\n+\tgit_config_get_string(\"imap.tunnel\", &server.tunnel);\n+\tgit_config_get_string(\"imap.authmethod\", &server.auth_method);\n }\n \n int main(int argc, char **argv)\n@@ -1383,7 +1376,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":"246965","messageId":"1406727549-22334-5-git-send-email-tanayabh@gmail.com","threadId":"37247","inReplyTo":"1406727549-22334-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v4 4/5] branch.c: replace `git_config()` with `git_config_get_string()","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T13:39:08Z","receivedAt":"2014-07-30T13:39:08Z","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-set API which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n branch.c | 24 ++++--------------------\n 1 file changed, 4 insertions(+), 20 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 46e8aa8..df6b120 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -140,33 +140,17 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \treturn 0;\n }\n \n-struct branch_desc_cb {\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+\tchar *v = NULL;\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+\tif (git_config_get_string(name.buf, &v)) {\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, v);\n+\tfree(v);\n \tstrbuf_release(&name);\n \treturn 0;\n }\n-- \n1.9.0.GIT\n"},{"id":"246968","messageId":"1406727549-22334-6-git-send-email-tanayabh@gmail.com","threadId":"37247","inReplyTo":"1406727549-22334-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v4 5/5] alias.c: replace `git_config()` with `git_config_get_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T13:39:09Z","receivedAt":"2014-07-30T13:39:09Z","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-set API which provides a cleaner control flow.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n alias.c | 25 ++++++-------------------\n 1 file changed, 6 insertions(+), 19 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 758c867..6aa164a 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -1,26 +1,13 @@\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-\tconst char *name;\n-\tif (skip_prefix(k, \"alias.\", &name) && !strcmp(name, 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+\tchar *v = NULL;\n+\tstruct strbuf key = STRBUF_INIT;\n+\tstrbuf_addf(&key, \"alias.%s\", alias);\n+\tgit_config_get_string(key.buf, &v);\n+\tstrbuf_release(&key);\n+\treturn v;\n }\n \n #define SPLIT_CMDLINE_BAD_ENDING 1\n-- \n1.9.0.GIT\n"},{"id":"246969","messageId":"vpq1tt3rmbt.fsf@anie.imag.fr","threadId":"37247","inReplyTo":"1406727549-22334-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-30T13:46:30Z","receivedAt":"2014-07-30T13:46:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked.\n> \tOne thing to check is if the config variables I changed in the series\n> \tare single valued or multi valued. Though I have tried to ascertain\n> \tif the variable was single valued or not by reading the docs and code,\n> \tmistakes may creep in.\n>\n> [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion.\n> \tAlso, new helpers introduced in v7 of the config-set API series have been used.\n>\n> This series builds on the top of 4c715ebb in pu (ta/config-set).\n\nI think it semantically needs your other WIP series, that makes\ngit_config_get_* die in case of error. Otherwise, there's an unexpected\nbehavior change in case of error.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246970","messageId":"53D8FB39.7040904@gmail.com","threadId":"37247","inReplyTo":"vpq1tt3rmbt.fsf@anie.imag.fr","subject":"Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T14:03:37Z","receivedAt":"2014-07-30T14:03:37Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/30/2014 7:16 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked.\n>> \tOne thing to check is if the config variables I changed in the series\n>> \tare single valued or multi valued. Though I have tried to ascertain\n>> \tif the variable was single valued or not by reading the docs and code,\n>> \tmistakes may creep in.\n>>\n>> [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion.\n>> \tAlso, new helpers introduced in v7 of the config-set API series have been used.\n>>\n>> This series builds on the top of 4c715ebb in pu (ta/config-set).\n> \n> I think it semantically needs your other WIP series, that makes\n> git_config_get_* die in case of error. Otherwise, there's an unexpected\n> behavior change in case of error.\n>\n\nYes you are right, there is a call to git_die_config() also in the series. I will add\nthe info in the next reroll. Also, any thoughts on what to do with git_default_config()?\nWe can,\n\n1> make a new function git_load_default_config(), use it for the rewrites.\n\nor\n\n2> git_default_config() is rewritten to be loaded only once even if it is called\nagain and again during the process run, so that we use the same function in callbacks\nand in the new API using rewrites.\n\nThanks.\n"},{"id":"246971","messageId":"vpq1tt3q6ic.fsf@anie.imag.fr","threadId":"37247","inReplyTo":"1406727549-22334-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-30T14:13:31Z","receivedAt":"2014-07-30T14:13:31Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> -\tgit_config(notes_display_config, &load_config_refs);\n> +\tif (load_config_refs) {\n> +\t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n> +\t\tif (values) {\n> +\t\t\tfor (i = 0; i < values->nr; i++) {\n> +\t\t\t\tif (!values->items[i].string)\n> +\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n> +\t\t\t\telse\n> +\t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n> +\t\t\t\t\t\t\t\t     values->items[i].string);\n> +\t\t\t}\n> +\t\t}\n> +\t}\n\nIt seems to me that you're doing a lot here that should have been done\nonce in the config API:\n\n* if (values) {\n          for (i = 0; i < values->nr\n\n  => We could avoid the \"if\" statement if git_config_get_value_multi was\n  always returning a string_list, possibly empty (values->nr == 0\n  instead of values == NULL).\n\n  Not as obvious as it seems, because you normally return a pointer to\n  the string_list that is already in the hashmap, so you can't just\n  malloc() an empty one if you don't want to leak it.\n\n  Another option would be to provide an iterator that would call a\n  function on each value of the list, and do nothing when there's no\n  list at all (back to the callback-style API, but you would iterate\n  only through the values for the right key).\n\n* if (!values->items[i].string)\n          config_error_nonbool(\n\n  => This check could be done once and for all in a function, say\n  git_config_get_value_multi_nonbool, a trivial wrapper around\n  git_config_get_value_multi like\n\nconst struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)\n{\n\tstruct string_list l = git_configset_get_value_multi(cs, key);\n        // possibly if(l) depending on the point above.\n\tfor (i = 0; i < values->nr; i++) {\n\t\tif (!values->items[i].string)\n\t\t\tgit_config_die(key);\n\t}\n\treturn l;\n}\n\nconst struct string_list *git_config_get_value_multi_nonbool(const char *key)\n{\n\tgit_config_check_init();\n\treturn git_configset_get_value_multi_nonbool(&the_config_set, key);\n}\n\n\n  (totally untested)\n\n  BTW, is it intentional that you call config_error_nonbool() without\n  die-ing?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246972","messageId":"53D903E0.4080209@gmail.com","threadId":"37247","inReplyTo":"vpq1tt3q6ic.fsf@anie.imag.fr","subject":"Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-30T14:40:32Z","receivedAt":"2014-07-30T14:40:32Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/30/2014 7:43 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> -\tgit_config(notes_display_config, &load_config_refs);\n>> +\tif (load_config_refs) {\n>> +\t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n>> +\t\tif (values) {\n>> +\t\t\tfor (i = 0; i < values->nr; i++) {\n>> +\t\t\t\tif (!values->items[i].string)\n>> +\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n>> +\t\t\t\telse\n>> +\t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n>> +\t\t\t\t\t\t\t\t     values->items[i].string);\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n> \n> It seems to me that you're doing a lot here that should have been done\n> once in the config API:\n> \n> * if (values) {\n>           for (i = 0; i < values->nr\n> \n>   => We could avoid the \"if\" statement if git_config_get_value_multi was\n>   always returning a string_list, possibly empty (values->nr == 0\n>   instead of values == NULL).\n>\n\nor we can do something like,\n\n\tif (!git_config_get_value_multi(\"notes.displayref\", &values)) {\n\t\t/* return 0 if there is a value_list for the key */\n\n>   Not as obvious as it seems, because you normally return a pointer to\n>   the string_list that is already in the hashmap, so you can't just\n>   malloc() an empty one if you don't want to leak it.\n> \n>   Another option would be to provide an iterator that would call a\n>   function on each value of the list, and do nothing when there's no\n>   list at all (back to the callback-style API, but you would iterate\n>   only through the values for the right key).\n>\n\nThis is also a good idea, but still we are back to the callback API,\nand what we are gaining is fewer loop iterations than git_config().\n\nWhich way do you prefer, a reroll is easy but Junio might have been sick\nof replacing the patches in pu by now. :)\n\n> * if (!values->items[i].string)\n>           config_error_nonbool(\n> \n>   => This check could be done once and for all in a function, say\n>   git_config_get_value_multi_nonbool, a trivial wrapper around\n>   git_config_get_value_multi like\n> \n> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)\n> {\n> \tstruct string_list l = git_configset_get_value_multi(cs, key);\n>         // possibly if(l) depending on the point above.\n> \tfor (i = 0; i < values->nr; i++) {\n> \t\tif (!values->items[i].string)\n> \t\t\tgit_config_die(key);\n> \t}\n> \treturn l;\n> }\n>\n\nNot worth it, most the multi value calls do not die on a nonbool.\n\n> const struct string_list *git_config_get_value_multi_nonbool(const char *key)\n> {\n> \tgit_config_check_init();\n> \treturn git_configset_get_value_multi_nonbool(&the_config_set, key);\n> }\n> \n> \n>   (totally untested)\n> \n>   BTW, is it intentional that you call config_error_nonbool() without\n>   die-ing?\n>\n\nYup, it's intentional, original code didn't die for empty values, and it seemed\nlogical to me to emulate that over to the rewrite.\n"},{"id":"246973","messageId":"vpqr412q0ht.fsf@anie.imag.fr","threadId":"37247","inReplyTo":"1406727549-22334-5-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v4 4/5] branch.c: replace `git_config()` with `git_config_get_string()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-30T16:23:26Z","receivedAt":"2014-07-30T16:23:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n>  int read_branch_desc(struct strbuf *buf, const char *branch_name)\n>  {\n> -\tstruct branch_desc_cb cb;\n> +\tchar *v = NULL;\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> +\tif (git_config_get_string(name.buf, &v)) {\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, v);\n> +\tfree(v);\n>  \tstrbuf_release(&name);\n>  \treturn 0;\n>  }\n\nI think this is a behavior change. if (git_config(read_branch_desc_cb,\n&cb) < 0) was never true in practice, so the \"return -1\" was essentially\ndead code. You now return -1 when no value is found.\n\nIt probably doesn't matter, since all caller except\nfmt-merge-msg.c:add_branch_desc() ignore the return value, and if I read\ncorrectly, add_branch_desc does not need the test on the return value,\nas the then branch of the if does nothing if no value is found anyway.\n\nBut here again, I have to wonder why the function does not just return\nvoid.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246974","messageId":"vpqsiliol1o.fsf@anie.imag.fr","threadId":"37247","inReplyTo":"53D903E0.4080209@gmail.com","subject":"Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-30T16:42:27Z","receivedAt":"2014-07-30T16:42:27Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> On 7/30/2014 7:43 PM, Matthieu Moy wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>> \n>>> -\tgit_config(notes_display_config, &load_config_refs);\n>>> +\tif (load_config_refs) {\n>>> +\t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n>>> +\t\tif (values) {\n>>> +\t\t\tfor (i = 0; i < values->nr; i++) {\n>>> +\t\t\t\tif (!values->items[i].string)\n>>> +\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n>>> +\t\t\t\telse\n>>> +\t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n>>> +\t\t\t\t\t\t\t\t     values->items[i].string);\n>>> +\t\t\t}\n>>> +\t\t}\n>>> +\t}\n>> \n>> It seems to me that you're doing a lot here that should have been done\n>> once in the config API:\n>> \n>> * if (values) {\n>>           for (i = 0; i < values->nr\n>> \n>>   => We could avoid the \"if\" statement if git_config_get_value_multi was\n>>   always returning a string_list, possibly empty (values->nr == 0\n>>   instead of values == NULL).\n>>\n>\n> or we can do something like,\n>\n> \tif (!git_config_get_value_multi(\"notes.displayref\", &values)) {\n> \t\t/* return 0 if there is a value_list for the key */\n>\n>>   Not as obvious as it seems, because you normally return a pointer to\n>>   the string_list that is already in the hashmap, so you can't just\n>>   malloc() an empty one if you don't want to leak it.\n>> \n>>   Another option would be to provide an iterator that would call a\n>>   function on each value of the list, and do nothing when there's no\n>>   list at all (back to the callback-style API, but you would iterate\n>>   only through the values for the right key).\n>>\n>\n> This is also a good idea, but still we are back to the callback API,\n> and what we are gaining is fewer loop iterations than git_config().\n\nRegardless of performance, the code would also be a bit shorter, since\nthe callback just gets the values for the right key, so it doesn't need\nto re-test that the key is the right one.\n\nHere, the callback would basically be the body of the for loop above.\n\n> Which way do you prefer, a reroll is easy but Junio might have been sick\n> of replacing the patches in pu by now. :)\n\nNo need to replace anything, you can add new helpers on top of the\nexisting.\n\nDo it the way you feel is better, I'm just giving ideas.\n\n>> * if (!values->items[i].string)\n>>           config_error_nonbool(\n>> \n>>   => This check could be done once and for all in a function, say\n>>   git_config_get_value_multi_nonbool, a trivial wrapper around\n>>   git_config_get_value_multi like\n>> \n>> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)\n>> {\n>> \tstruct string_list l = git_configset_get_value_multi(cs, key);\n>>         // possibly if(l) depending on the point above.\n>> \tfor (i = 0; i < values->nr; i++) {\n>> \t\tif (!values->items[i].string)\n>> \t\t\tgit_config_die(key);\n>> \t}\n>> \treturn l;\n>> }\n>>\n>\n> Not worth it, most the multi value calls do not die on a nonbool.\n\nCan you cite some multi-value variables that can be nonbool? I can't\nfind many multi-valued variables, and I can't find any which would allow\nbool and nonbool.\n\n>> const struct string_list *git_config_get_value_multi_nonbool(const char *key)\n>> {\n>> \tgit_config_check_init();\n>> \treturn git_configset_get_value_multi_nonbool(&the_config_set, key);\n>> }\n>> \n>> \n>>   (totally untested)\n>> \n>>   BTW, is it intentional that you call config_error_nonbool() without\n>>   die-ing?\n>>\n>\n> Yup, it's intentional, original code didn't die for empty values, and it seemed\n> logical to me to emulate that over to the rewrite.\n\nThe old code was doing\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\nIt seems that the intent of the programmer was\n\n\tif (*load_refs && !strcmp(k, \"notes.displayref\")) {\n\t\tif (!v)\n\t\t\treturn config_error_nonbool(k); // <---------------\n\t\tstring_list_add_refs_by_glob(&display_notes_refs, v);\n\nAt least, that would explain why the code uses v even after testing that\nit is a NULL pointer.\n\nYou're already fixing a bug in your patch by not using NULL values, but\nthen I don't see any reason to keep the old weird behavior (display an\nerror but do not die).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246975","messageId":"vpqiomeokx7.fsf@anie.imag.fr","threadId":"37247","inReplyTo":"53D8FB39.7040904@gmail.com","subject":"Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-30T16:45:08Z","receivedAt":"2014-07-30T16:45:08Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> Yes you are right, there is a call to git_die_config() also in the series. I will add\n> the info in the next reroll.\n\nIf unsure, rebase your branch locally on the commit on which it is meant\nto apply, and check that it works for you.\n\n> Also, any thoughts on what to do with git_default_config()? We can,\n>\n> 1> make a new function git_load_default_config(), use it for the rewrites.\n\nThat seems the most sensible option. It could be called it git.c before\nthe command-dependant part, so that any call to git loads this.\n\nI didn't check if it was correct (e.g. do some command rely on the fact\nthat the default config is not loaded?)\n\n> 2> git_default_config() is rewritten to be loaded only once even if it is called\n> again and again during the process run, so that we use the same function in callbacks\n> and in the new API using rewrites.\n\nSeems like a workaround, and to me the point of your GSoC is to make the\ncode cleaner, hence avoid workaraounds ...\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247031","messageId":"53DA2A5E.4020101@ramsay1.demon.co.uk","threadId":"37247","inReplyTo":"vpqiomeokx7.fsf@anie.imag.fr","subject":"Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-31T11:37:02Z","receivedAt":"2014-07-31T11:37:02Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 30/07/14 17:45, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n>> Also, any thoughts on what to do with git_default_config()? We can,\n>>\n>> 1> make a new function git_load_default_config(), use it for the rewrites.\n> \n> That seems the most sensible option. It could be called it git.c before\n> the command-dependant part, so that any call to git loads this.\n> \n> I didn't check if it was correct (e.g. do some command rely on the fact\n> that the default config is not loaded?)\n> \n\nHmm, here be dragons ... :-P\n\nI don't know that there has actually been any kind of policy\nregarding the reading of config files/variables in git (or if\nthere is a different policy for plumbing vs porcelain), but it\nhas always seemed to be somewhat ad-hoc; each command decides\nfor itself what it wants to read.\n\nHowever, with increased use of common code which _uses_ certain\nconfig variables for correct operation, the 'choice' is much\nharder to make (and may change after the fact!).\n\nFor example, about a year ago I submitted a couple of patches\nwhich added a call to git_config(git_default_config, NULL) to\nboth 'git pack-refs' and 'git show-refs'. This was as a result\nof the 'mh/ref-races' branch which introduced a 'stat_validity'\napi for checking if the packed-refs file had changed on the\nfilesystem since last you looked. This re-used some of the same\ncode used to handle index updates that used config variables\nlike core.checkstat and core.trustctime. These config variables\ncan affect the correctness and/or the efficiency of the code on\nsome platforms (e.g. cygwin, mingw).\n\n[Note: 'stat_validity' has since been re-used again (why not?)\nin some shallow clone code, so similar comments may apply ...\nI haven't looked.]\n\nHowever, those patches were dropped, because it resulted in an\n(unwanted) change in behaviour. In particular, 'git show-refs'\nchanged behaviour because it now 'listened' to core.abbrev!\n\nI started to look at splitting the 'core config variables' into\ntwo groups; essential variables that _all_ git commands should\nread for correct/efficient/consistent behaviour and everything\nelse (mainly UI related variables).\n\nHowever, something else came up ...\n\nJust an FYI.\n\nATB,\nRamsay Jones\n"},{"id":"247032","messageId":"vpqmwbpdagf.fsf@anie.imag.fr","threadId":"37247","inReplyTo":"vpqsiliol1o.fsf@anie.imag.fr","subject":"Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-31T11:38:56Z","receivedAt":"2014-07-31T11:38:56Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Tanay Abhra <tanayabh@gmail.com> writes:\n>\n>> On 7/30/2014 7:43 PM, Matthieu Moy wrote:\n>>> * if (!values->items[i].string)\n>>>           config_error_nonbool(\n>>> \n>>>   => This check could be done once and for all in a function, say\n>>>   git_config_get_value_multi_nonbool, a trivial wrapper around\n>>>   git_config_get_value_multi like\n>>> \n>>> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)\n>>> {\n>>> \tstruct string_list l = git_configset_get_value_multi(cs, key);\n>>>         // possibly if(l) depending on the point above.\n>>> \tfor (i = 0; i < values->nr; i++) {\n>>> \t\tif (!values->items[i].string)\n>>> \t\t\tgit_config_die(key);\n>>> \t}\n>>> \treturn l;\n>>> }\n>>>\n>>\n>> Not worth it, most the multi value calls do not die on a nonbool.\n>\n> Can you cite some multi-value variables that can be nonbool? I can't\n> find many multi-valued variables, and I can't find any which would allow\n> bool and nonbool.\n\nThinking a bit more about it: we actually need more than your patch and\nmy code example above to give accurate error messages. Your code gives\nno error message, and mine uses git_config_die(key); which gives the\nline of the _last_ entry, but not necessarily the right line.\n\nThe right line number should be extracted from the info field of the\nstring_list. It's not completely trivial, hence I'd rather have a helper\ndoing it well in config.c than letting callers re-do the check and\npossibly give wrong line in their error message as I did in my first\nattempt.\n\nI think you can introduce a helper git_config_die_linenr(key, linenr)\nthat displays the right error message. Then git_config_die becomes a\nwrapper around it that does the lookup to find linenr from the hash.\n\nYou already have a duplicate piece of code in your other series:\n\n\t\t\tif (!kv_info->linenr)\n\t\t\t\tdie(\"unable to parse '%s' from command-line config\", entry->key);\n\t\t\telse\n\t\t\t\tdie(\"bad config variable '%s' at file line %d in %s\",\n\t\t\t\t\tentry->key,\n\t\t\t\t\tkv_info->linenr,\n\t\t\t\t\tkv_info->filename);\n\nThat would be the content of git_config_die_linenr().\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247036","messageId":"53DA32EB.70504@gmail.com","threadId":"37247","inReplyTo":"vpqmwbpdagf.fsf@anie.imag.fr","subject":"Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T12:13:31Z","receivedAt":"2014-07-31T12:13:31Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/31/2014 5:08 PM, Matthieu Moy wrote:\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> \n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>>\n>>> On 7/30/2014 7:43 PM, Matthieu Moy wrote:\n>>>> * if (!values->items[i].string)\n>>>>           config_error_nonbool(\n>>>>\n>>>>   => This check could be done once and for all in a function, say\n>>>>   git_config_get_value_multi_nonbool, a trivial wrapper around\n>>>>   git_config_get_value_multi like\n>>>>\n>>>> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)\n>>>> {\n>>>> \tstruct string_list l = git_configset_get_value_multi(cs, key);\n>>>>         // possibly if(l) depending on the point above.\n>>>> \tfor (i = 0; i < values->nr; i++) {\n>>>> \t\tif (!values->items[i].string)\n>>>> \t\t\tgit_config_die(key);\n>>>> \t}\n>>>> \treturn l;\n>>>> }\n>>>>\n>>>\n>>> Not worth it, most the multi value calls do not die on a nonbool.\n>>\n>> Can you cite some multi-value variables that can be nonbool? I can't\n>> find many multi-valued variables, and I can't find any which would allow\n>> bool and nonbool.\n> \n> Thinking a bit more about it: we actually need more than your patch and\n> my code example above to give accurate error messages. Your code gives\n> no error message, and mine uses git_config_die(key); which gives the\n> line of the _last_ entry, but not necessarily the right line.\n> \n> The right line number should be extracted from the info field of the\n> string_list. It's not completely trivial, hence I'd rather have a helper\n> doing it well in config.c than letting callers re-do the check and\n> possibly give wrong line in their error message as I did in my first\n> attempt.\n> \n> I think you can introduce a helper git_config_die_linenr(key, linenr)\n> that displays the right error message. Then git_config_die becomes a\n> wrapper around it that does the lookup to find linenr from the hash.\n> \n> You already have a duplicate piece of code in your other series:\n> \n> \t\t\tif (!kv_info->linenr)\n> \t\t\t\tdie(\"unable to parse '%s' from command-line config\", entry->key);\n> \t\t\telse\n> \t\t\t\tdie(\"bad config variable '%s' at file line %d in %s\",\n> \t\t\t\t\tentry->key,\n> \t\t\t\t\tkv_info->linenr,\n> \t\t\t\t\tkv_info->filename);\n> \n> That would be the content of git_config_die_linenr().\n> \n\nThanks. I will add it in the next reroll.\n"}]}