{"thread":{"id":"37179","subject":"[PATCH v3 0/6] git_config callers rewritten with the new config cache API","startedAt":"2014-07-21T11:12:19Z","lastAt":"2014-07-31T17:13:43Z","messageCount":27,"participants":["Tanay Abhra","Matthieu Moy","Ramsay Jones","Junio C Hamano","Jeff King","Samuel Bronson"],"isPatch":true,"patchVersion":3,"patchTotal":6},"messages":[{"id":"246435","messageId":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","threadId":"37179","inReplyTo":null,"subject":"[PATCH v3 0/6] git_config callers rewritten with the new config cache API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:12:19Z","receivedAt":"2014-07-21T11:12:19Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"[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\tSee [1] for the documentation of the new functions.\n\nThis series builds on the top of 5def4132 in pu or topic[1] in the mailing list\nwith name \"git config cache & special querying API utilizing the cache\".\n\nAll patches pass every test, but there is a catch, there is slight behaviour\nchange in most of them where originally the callback returns\nconfig_error_nonbool() when it sees a NULL value for a key causing a die\nspecified in git_parse_source in config.c.\n\nThe die also prints the file name and the line number as,\n\n\t\"die(\"bad config file line %d in %s\", cf->linenr, cf->name);\"\n\nWe lose the fine grained error checking when switching to this method.\nStill, I will try to correct this anomaly in my next series.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/253862\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/252334\n\nTanay Abhra (6):\n\n alias.c | 27 +++++++--------------------\n branch.c | 24 ++++--------------------\n imap-send.c | 41 +++++++++++++++--------------------------\n notes-utils.c | 33 ++++++++++++++++-----------------\n notes.c | 21 +++++++--------------\n pager.c | 40 +++++++++++++---------------------------\n 6 files changed, 62 insertions(+), 124 deletions(-)\n\n-- \n1.9.0.GIT\n"},{"id":"246436","messageId":"1405941145-12120-2-git-send-email-tanayabh@gmail.com","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:12:20Z","receivedAt":"2014-07-21T11:12:20Z","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.\nThe function now raises an error instead of dying when a NULL value is found.\n\nSigned-off-by: Tanay Abhra \n---\n alias.c | 27 +++++++--------------------\n 1 file changed, 7 insertions(+), 20 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 758c867..a453bd8 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+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 = 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 (char*)v;\n }\n \n #define SPLIT_CMDLINE_BAD_ENDING 1\n-- \n1.9.0.GIT\n"},{"id":"246439","messageId":"1405941145-12120-3-git-send-email-tanayabh@gmail.com","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 2/6] branch.c: replace `git_config()` with `git_config_get_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:12:21Z","receivedAt":"2014-07-21T11:12:21Z","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 \n---\n branch.c | 24 ++++--------------------\n 1 file changed, 4 insertions(+), 20 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 46e8aa8..827307f 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+\tconst char *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((char*)v);\n \tstrbuf_release(&name);\n \treturn 0;\n }\n-- \n1.9.0.GIT\n"},{"id":"246437","messageId":"1405941145-12120-4-git-send-email-tanayabh@gmail.com","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 3/6] imap-send.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:12:22Z","receivedAt":"2014-07-21T11:12:22Z","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.\nThe function now raises an error instead of dying in cases where a NULL value is\nnot allowed.\n\nSigned-off-by: Tanay Abhra \n---\n imap-send.c | 62 +++++++++++++++++++++++++++----------------------------------\n 1 file changed, 27 insertions(+), 35 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 524fbab..b7ec98a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1326,43 +1326,35 @@ 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-\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+\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\", (const char**)&imap_folder);\n+\n+\tif (!git_config_get_value(\"imap.host\", &val)) {\n+\t\tif(!val)\n+\t\t\tconfig_error_nonbool(\"imap.host\");\n+\t\telse {\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\", (const char**)&server.user);\n+\tgit_config_get_string(\"imap.pass\", (const char**)&server.pass);\n+\tgit_config_get_string(\"imap.port\", (const char**)&server.port);\n+\tgit_config_get_string(\"imap.tunnel\", (const char**)&server.tunnel);\n+\tgit_config_get_string(\"imap.authmethod\", (const char**)&server.auth_method);\n }\n \n int main(int argc, char **argv)\n@@ -1383,7 +1375,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-- \n"},{"id":"246438","messageId":"1405941145-12120-5-git-send-email-tanayabh@gmail.com","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 4/6] notes.c: replace `git_config()` with `git_config_get_value()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:12:23Z","receivedAt":"2014-07-21T11:12:23Z","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, also previously\n'string_list_add_refs_by_glob()' was called even when the retrieved value\nwas NULL, correct it while we are at it.\n\nSigned-off-by: Tanay Abhra \n---\n notes.c | 21 +++++++--------------\n 1 file changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 5fe691d..20c20f5 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 = NULL;\n \tint load_config_refs = 0;\n \tdisplay_notes_refs.strdup_strings = 1;\n \n@@ -1058,7 +1046,12 @@ 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_value(\"notes.displayref\", &value)) {\n+\t\tif (!value)\n+\t\t\tconfig_error_nonbool(\"notes.displayref\");\n+\t\telse\n+\t\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":"246440","messageId":"1405941145-12120-6-git-send-email-tanayabh@gmail.com","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 5/6] pager.c: replace `git_config()` with `git_config_get_value()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:12:24Z","receivedAt":"2014-07-21T11:12:24Z","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 \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":"246441","messageId":"1405941145-12120-7-git-send-email-tanayabh@gmail.com","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 6/6] notes-util.c: replace `git_config()` with `git_config_get_value()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:12:25Z","receivedAt":"2014-07-21T11:12:25Z","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.\nThe function now raises an error instead of dying when a NULL value is found\nfor key \"notes.rewritemode\".\n\nSigned-off-by: Tanay Abhra \n---\n notes-utils.c | 33 ++++++++++++++++-----------------\n 1 file changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/notes-utils.c b/notes-utils.c\nindex b64dc1b..ffa2b70 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -69,22 +69,24 @@ 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+\tconst char *v;\n+\tstruct strbuf key = STRBUF_INIT;\n+\tstrbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n+\tgit_config_get_bool(key.buf, &c->enabled);\n+\tstrbuf_release(&key);\n+\n+\tif (!c->mode_from_env && !git_config_get_value(\"notes.rewritemode\", &v)) {\n \t\tif (!v)\n-\t\t\treturn config_error_nonbool(k);\n-\t\tc->combine = parse_combine_notes_fn(v);\n-\t\tif (!c->combine) {\n-\t\t\terror(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n-\t\t\treturn 1;\n+\t\t\tconfig_error_nonbool(\"notes.rewritemode\");\n+\t\telse {\n+\t\t\tc->combine = parse_combine_notes_fn(v);\n+\t\t\tif (!c->combine)\n+\t\t\t\terror(_(\"Bad notes.rewriteMode value: '%s'\"), v);\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_value(\"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@@ -92,10 +94,7 @@ 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 }\n \n \n@@ -124,7 +123,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":"246442","messageId":"53CCFD02.6010704@gmail.com","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH/RFC] rewrite `git_default_config()` using config-set API functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T11:44:02Z","receivedAt":"2014-07-21T11:44:02Z","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 \n---\nConsider this as a proof of concept as the others callers have to be rewritten\nas well.\nI think that it is not so buggy as it passes all the tests.\nAfter the first six patches in the series which you have already seen there are\nfive or four left which can rewritten without touching git_default_config().\n\nThus, this rewrite will serve as the base for rewriting other git_config()\ncallers which pass control to git_default_config() at the end of the function.\nAlso there are more than thirty direct callers to git_default_config()\n(i.e git_config(git_default_config, NULL)), so this rewrite solves them\nin one sweep.\n\nSlight behaviour change, config_error_nonbool() has been replaced with\ndie(\"Missing value for '%s'\", var);.\nThe original code also alerted the file name and the line number which we lose here.\n\nCheers,\nTanay Abhra.\n\n advice.c | 18 ++--\n advice.h | 2 +-\n cache.h | 2 +-\n config.c | 287 ++++++++++++++++++++-------------------------------------------\n ident.c | 15 ++--\n 5 files changed, 104 insertions(+), 220 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 9b42033..92d89a9 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -59,22 +59,16 @@ void advise(const char *advice, ...)\n \tstrbuf_release(&buf);\n }\n\n-int git_default_advice_config(const char *var, const char *value)\n+void git_default_advice_config(void)\n {\n-\tconst char *k;\n+\tstruct strbuf var = STRBUF_INIT;\n \tint i;\n-\n-\tif (!skip_prefix(var, \"advice.\", &k))\n-\t\treturn 0;\n-\n \tfor (i = 0; i < ARRAY_SIZE(advice_config); i++) {\n-\t\tif (strcmp(k, advice_config[i].name))\n-\t\t\tcontinue;\n-\t\t*advice_config[i].preference = git_config_bool(var, value);\n-\t\treturn 0;\n+\t\tstrbuf_addf(&var, \"advice.%s\", advice_config[i].name);\n+\t\tgit_config_get_bool(var.buf, advice_config[i].preference);\n+\t\tstrbuf_reset(&var);\n \t}\n-\n-\treturn 0;\n+\tstrbuf_release(&var);\n }\n\n int error_resolve_conflict(const char *me)\ndiff --git a/advice.h b/advice.h\nindex 5ecc6c1..5bfe46c 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -19,7 +19,7 @@ extern int advice_set_upstream_failure;\n extern int advice_object_name_warning;\n extern int advice_rm_hints;\n\n-int git_default_advice_config(const char *var, const char *value);\n+void git_default_advice_config(void);\n __attribute__((format (printf, 1, 2)))\n void advise(const char *advice, ...);\n int error_resolve_conflict(const char *me);\ndiff --git a/cache.h b/cache.h\nindex e53651c..e667d92 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1061,7 +1061,7 @@ extern const char *fmt_name(const char *name, const char *email);\n extern const char *ident_default_email(void);\n extern const char *git_editor(void);\n extern const char *git_pager(int stdout_is_tty);\n-extern int git_ident_config(const char *, const char *, void *);\n+extern void git_ident_config(void);\n\n struct ident_split {\n \tconst char *name_begin;\ndiff --git a/config.c b/config.c\nindex fe9f399..72196a9 100644\n--- a/config.c\n+++ b/config.c\n@@ -666,88 +666,47 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n\n-static int git_default_core_config(const char *var, const char *value)\n+static void git_default_core_config(void)\n {\n+\tconst char *value = NULL;\n \t/* This needs a better name */\n-\tif (!strcmp(var, \"core.filemode\")) {\n-\t\ttrust_executable_bit = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"core.trustctime\")) {\n-\t\ttrust_ctime = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"core.checkstat\")) {\n+\tgit_config_get_bool(\"core.filemode\", &trust_executable_bit);\n+\tgit_config_get_bool(\"core.trustctime\", &trust_ctime);\n+\n+\tif (!git_config_get_value(\"core.checkstat\", &value)) {\n \t\tif (!strcasecmp(value, \"default\"))\n \t\t\tcheck_stat = 1;\n \t\telse if (!strcasecmp(value, \"minimal\"))\n \t\t\tcheck_stat = 0;\n \t}\n\n-\tif (!strcmp(var, \"core.quotepath\")) {\n-\t\tquote_path_fully = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.symlinks\")) {\n-\t\thas_symlinks = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.ignorecase\")) {\n-\t\tignore_case = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.attributesfile\"))\n-\t\treturn git_config_pathname(&git_attributes_file, var, value);\n-\n-\tif (!strcmp(var, \"core.bare\")) {\n-\t\tis_bare_repository_cfg = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.ignorestat\")) {\n-\t\tassume_unchanged = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.prefersymlinkrefs\")) {\n-\t\tprefer_symlink_refs = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.logallrefupdates\")) {\n-\t\tlog_all_ref_updates = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n+\tgit_config_get_bool(\"core.quotepath\", "e_path_fully);\n+\tgit_config_get_bool(\"core.symlinks\", &has_symlinks);\n+\tgit_config_get_bool(\"core.ignorecase\", &ignore_case);\n+\tgit_config_get_pathname(\"core.attributesfile\", &git_attributes_file);\n+\tgit_config_get_bool(\"core.bare\", &is_bare_repository_cfg);\n+\tgit_config_get_bool(\"core.ignorestat\", &assume_unchanged);\n+\tgit_config_get_bool(\"core.prefersymlinkrefs\", &prefer_symlink_refs);\n+\tgit_config_get_bool(\"core.logallrefupdates\", &log_all_ref_updates);\n+\tgit_config_get_bool(\"core.warnambiguousrefs\", &warn_ambiguous_refs);\n\n-\tif (!strcmp(var, \"core.warnambiguousrefs\")) {\n-\t\twarn_ambiguous_refs = git_config_bool(var, value);\n-\t\treturn 0;\n+\tint abbrev;\n+\tif (!git_config_get_int(\"core.abbrev\", &abbrev)) {\n+\t\tif (abbrev >= minimum_abbrev && abbrev <= 40)\n+\t\t\tdefault_abbrev = abbrev;\n \t}\n\n-\tif (!strcmp(var, \"core.abbrev\")) {\n-\t\tint abbrev = git_config_int(var, value);\n-\t\tif (abbrev < minimum_abbrev || abbrev > 40)\n-\t\t\treturn -1;\n-\t\tdefault_abbrev = abbrev;\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.loosecompression\")) {\n-\t\tint level = git_config_int(var, value);\n+\tint level;\n+\tif (!git_config_get_int(\"core.loosecompression\", &level)) {\n \t\tif (level == -1)\n \t\t\tlevel = Z_DEFAULT_COMPRESSION;\n \t\telse if (level < 0 || level > Z_BEST_COMPRESSION)\n \t\t\tdie(\"bad zlib compression level %d\", level);\n \t\tzlib_compression_level = level;\n \t\tzlib_compression_seen = 1;\n-\t\treturn 0;\n \t}\n\n-\tif (!strcmp(var, \"core.compression\")) {\n-\t\tint level = git_config_int(var, value);\n+\tif (!git_config_get_int(\"core.compression\", &level)) {\n \t\tif (level == -1)\n \t\t\tlevel = Z_DEFAULT_COMPRESSION;\n \t\telse if (level < 0 || level > Z_BEST_COMPRESSION)\n@@ -756,57 +715,39 @@ static int git_default_core_config(const char *var, const char *value)\n \t\tcore_compression_seen = 1;\n \t\tif (!zlib_compression_seen)\n \t\t\tzlib_compression_level = level;\n-\t\treturn 0;\n \t}\n\n-\tif (!strcmp(var, \"core.packedgitwindowsize\")) {\n+\tif (!git_config_get_ulong(\"core.packedgitwindowsize\", (long unsigned int*)&packed_git_window_size)) {\n \t\tint pgsz_x2 = getpagesize() * 2;\n-\t\tpacked_git_window_size = git_config_ulong(var, value);\n\n \t\t/* This value must be multiple of (pagesize * 2) */\n \t\tpacked_git_window_size /= pgsz_x2;\n \t\tif (packed_git_window_size < 1)\n \t\t\tpacked_git_window_size = 1;\n \t\tpacked_git_window_size *= pgsz_x2;\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.bigfilethreshold\")) {\n-\t\tbig_file_threshold = git_config_ulong(var, value);\n-\t\treturn 0;\n \t}\n\n-\tif (!strcmp(var, \"core.packedgitlimit\")) {\n-\t\tpacked_git_limit = git_config_ulong(var, value);\n-\t\treturn 0;\n-\t}\n+\tgit_config_get_ulong(\"core.bigfilethreshold\", &big_file_threshold);\n+\tgit_config_get_ulong(\"core.packedgitlimit\", (long unsigned int*)&packed_git_limit);\n+\tgit_config_get_ulong(\"core.deltabasecachelimit\", (long unsigned int*)&delta_base_cache_limit);\n\n-\tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n-\t\tdelta_base_cache_limit = git_config_ulong(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.autocrlf\")) {\n+\tif (!git_config_get_value(\"core.autocrlf\", &value)) {\n \t\tif (value && !strcasecmp(value, \"input\")) {\n \t\t\tif (core_eol == EOL_CRLF)\n-\t\t\t\treturn error(\"core.autocrlf=input conflicts with core.eol=crlf\");\n+\t\t\t\tdie(\"core.autocrlf=input conflicts with core.eol=crlf\");\n \t\t\tauto_crlf = AUTO_CRLF_INPUT;\n-\t\t\treturn 0;\n-\t\t}\n-\t\tauto_crlf = git_config_bool(var, value);\n-\t\treturn 0;\n+\t\t} else\n+\t\t\tauto_crlf = git_config_bool(\"core.autocrlf\", value);\n \t}\n\n-\tif (!strcmp(var, \"core.safecrlf\")) {\n-\t\tif (value && !strcasecmp(value, \"warn\")) {\n+\tif (!git_config_get_value(\"core.safecrlf\", &value)) {\n+\t\tif (value && !strcasecmp(value, \"warn\"))\n \t\t\tsafe_crlf = SAFE_CRLF_WARN;\n-\t\t\treturn 0;\n-\t\t}\n-\t\tsafe_crlf = git_config_bool(var, value);\n-\t\treturn 0;\n+\t\telse\n+\t\t\tsafe_crlf = git_config_bool(\"core.safecrlf\", value);\n \t}\n\n-\tif (!strcmp(var, \"core.eol\")) {\n+\tif (!git_config_get_value(\"core.eol\", &value)) {\n \t\tif (value && !strcasecmp(value, \"lf\"))\n \t\t\tcore_eol = EOL_LF;\n \t\telse if (value && !strcasecmp(value, \"crlf\"))\n@@ -816,108 +757,74 @@ static int git_default_core_config(const char *var, const char *value)\n \t\telse\n \t\t\tcore_eol = EOL_UNSET;\n \t\tif (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)\n-\t\t\treturn error(\"core.autocrlf=input conflicts with core.eol=crlf\");\n-\t\treturn 0;\n+\t\t\tdie(\"core.autocrlf=input conflicts with core.eol=crlf\");\n \t}\n\n-\tif (!strcmp(var, \"core.notesref\")) {\n-\t\tnotes_ref_name = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.pager\"))\n-\t\treturn git_config_string(&pager_program, var, value);\n-\n-\tif (!strcmp(var, \"core.editor\"))\n-\t\treturn git_config_string(&editor_program, var, value);\n+\tgit_config_get_string(\"core.notesref\", (const char**)¬es_ref_name);\n+\tgit_config_get_string(\"core.pager\", &pager_program);\n+\tgit_config_get_string(\"core.editor\", &editor_program);\n\n-\tif (!strcmp(var, \"core.commentchar\")) {\n-\t\tconst char *comment;\n-\t\tint ret = git_config_string(&comment, var, value);\n-\t\tif (ret)\n-\t\t\treturn ret;\n-\t\telse if (!strcasecmp(comment, \"auto\"))\n+\tconst char *comment;\n+\tif (!git_config_get_string(\"core.commentchar\", &comment)) {\n+\t\tif (!strcasecmp(comment, \"auto\"))\n \t\t\tauto_comment_line_char = 1;\n \t\telse if (comment[0] && !comment[1]) {\n \t\t\tcomment_line_char = comment[0];\n \t\t\tauto_comment_line_char = 0;\n \t\t} else\n-\t\t\treturn error(\"core.commentChar should only be one character\");\n-\t\treturn 0;\n+\t\t\tdie(\"core.commentChar should only be one character\");\n \t}\n\n-\tif (!strcmp(var, \"core.askpass\"))\n-\t\treturn git_config_string(&askpass_program, var, value);\n+\tgit_config_get_string(\"core.askpass\", &askpass_program);\n\n-\tif (!strcmp(var, \"core.excludesfile\"))\n-\t\treturn git_config_pathname(&excludes_file, var, value);\n+\tgit_config_get_pathname(\"core.excludesfile\", &excludes_file);\n\n-\tif (!strcmp(var, \"core.whitespace\")) {\n+\tif (!git_config_get_value(\"core.whitespace\", &value)) {\n \t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\twhitespace_rule_cfg = parse_whitespace_rule(value);\n-\t\treturn 0;\n+\t\t\tconfig_error_nonbool(\"core.whitespace\");\n+\t\telse\n+\t\t\twhitespace_rule_cfg = parse_whitespace_rule(value);\n \t}\n\n-\tif (!strcmp(var, \"core.fsyncobjectfiles\")) {\n-\t\tfsync_object_files = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n+\tgit_config_get_bool(\"core.fsyncobjectfiles\", &fsync_object_files);\n+\tgit_config_get_bool(\"core.preloadindex\", &core_preload_index);\n\n-\tif (!strcmp(var, \"core.preloadindex\")) {\n-\t\tcore_preload_index = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.createobject\")) {\n+\tif (!git_config_get_value(\"core.createobject\", &value)) {\n \t\tif (!strcmp(value, \"rename\"))\n \t\t\tobject_creation_mode = OBJECT_CREATION_USES_RENAMES;\n \t\telse if (!strcmp(value, \"link\"))\n \t\t\tobject_creation_mode = OBJECT_CREATION_USES_HARDLINKS;\n \t\telse\n \t\t\tdie(\"Invalid mode for object creation: %s\", value);\n-\t\treturn 0;\n \t}\n\n-\tif (!strcmp(var, \"core.sparsecheckout\")) {\n-\t\tcore_apply_sparse_checkout = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"core.precomposeunicode\")) {\n-\t\tprecomposed_unicode = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n+\tgit_config_get_bool(\"core.sparsecheckout\", &core_apply_sparse_checkout);\n+\tgit_config_get_bool(\"core.precomposeunicode\", &precomposed_unicode);\n\n \t/* Add other config variables here and to Documentation/config.txt. */\n-\treturn 0;\n }\n\n-static int git_default_i18n_config(const char *var, const char *value)\n+static void git_default_i18n_config(void)\n {\n-\tif (!strcmp(var, \"i18n.commitencoding\"))\n-\t\treturn git_config_string(&git_commit_encoding, var, value);\n-\n-\tif (!strcmp(var, \"i18n.logoutputencoding\"))\n-\t\treturn git_config_string(&git_log_output_encoding, var, value);\n+\tgit_config_get_string(\"i18n.commitencoding\", &git_commit_encoding);\n+\tgit_config_get_string(\"i18n.logoutputencoding\", &git_log_output_encoding);\n\n \t/* Add other config variables here and to Documentation/config.txt. */\n-\treturn 0;\n }\n\n-static int git_default_branch_config(const char *var, const char *value)\n+static void git_default_branch_config(void)\n {\n-\tif (!strcmp(var, \"branch.autosetupmerge\")) {\n-\t\tif (value && !strcasecmp(value, \"always\")) {\n+\tconst char *value = NULL;\n+\tif (!git_config_get_value(\"branch.autosetupmerge\", &value)) {\n+\t\tif (value && !strcasecmp(value, \"always\"))\n \t\t\tgit_branch_track = BRANCH_TRACK_ALWAYS;\n-\t\t\treturn 0;\n-\t\t}\n-\t\tgit_branch_track = git_config_bool(var, value);\n-\t\treturn 0;\n+\t\telse\n+\t\t\tgit_branch_track = git_config_bool(\"branch.autosetupmerge\", value);\n \t}\n-\tif (!strcmp(var, \"branch.autosetuprebase\")) {\n+\n+\tif (!git_config_get_value(\"branch.autosetuprebase\", &value)) {\n \t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n+\t\t\tdie(\"Missing value for 'branch.autosetuprebase'\");\n \t\telse if (!strcmp(value, \"never\"))\n \t\t\tautorebase = AUTOREBASE_NEVER;\n \t\telse if (!strcmp(value, \"local\"))\n@@ -927,19 +834,18 @@ static int git_default_branch_config(const char *var, const char *value)\n \t\telse if (!strcmp(value, \"always\"))\n \t\t\tautorebase = AUTOREBASE_ALWAYS;\n \t\telse\n-\t\t\treturn error(\"Malformed value for %s\", var);\n-\t\treturn 0;\n+\t\t\tdie(\"Malformed value for branch.autosetuprebase\");\n \t}\n\n \t/* Add other config variables here and to Documentation/config.txt. */\n-\treturn 0;\n }\n\n-static int git_default_push_config(const char *var, const char *value)\n+static void git_default_push_config(void)\n {\n-\tif (!strcmp(var, \"push.default\")) {\n+\tconst char *value = NULL;\n+\tif (!git_config_get_value(\"push.default\", &value)) {\n \t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n+\t\t\tdie(\"Missing value for 'push.default'\");\n \t\telse if (!strcmp(value, \"nothing\"))\n \t\t\tpush_default = PUSH_DEFAULT_NOTHING;\n \t\telse if (!strcmp(value, \"matching\"))\n@@ -953,60 +859,47 @@ static int git_default_push_config(const char *var, const char *value)\n \t\telse if (!strcmp(value, \"current\"))\n \t\t\tpush_default = PUSH_DEFAULT_CURRENT;\n \t\telse {\n-\t\t\terror(\"Malformed value for %s: %s\", var, value);\n-\t\t\treturn error(\"Must be one of nothing, matching, simple, \"\n+\t\t\terror(\"Malformed value for %s: %s\", \"push.default\", value);\n+\t\t\tdie(\"Must be one of nothing, matching, simple, \"\n \t\t\t\t \"upstream or current.\");\n \t\t}\n-\t\treturn 0;\n \t}\n\n \t/* Add other config variables here and to Documentation/config.txt. */\n-\treturn 0;\n }\n\n-static int git_default_mailmap_config(const char *var, const char *value)\n+static void git_default_mailmap_config(void)\n {\n-\tif (!strcmp(var, \"mailmap.file\"))\n-\t\treturn git_config_pathname(&git_mailmap_file, var, value);\n-\tif (!strcmp(var, \"mailmap.blob\"))\n-\t\treturn git_config_string(&git_mailmap_blob, var, value);\n+\tgit_config_get_pathname(\"mailmap.file\", &git_mailmap_file);\n+\tgit_config_get_string(\"mailmap.blob\", &git_mailmap_blob);\n\n \t/* Add other config variables here and to Documentation/config.txt. */\n-\treturn 0;\n }\n\n int git_default_config(const char *var, const char *value, void *dummy)\n {\n-\tif (starts_with(var, \"core.\"))\n-\t\treturn git_default_core_config(var, value);\n+\tconst char *v = NULL;\n\n-\tif (starts_with(var, \"user.\"))\n-\t\treturn git_ident_config(var, value, dummy);\n+\tgit_default_core_config();\n\n-\tif (starts_with(var, \"i18n.\"))\n-\t\treturn git_default_i18n_config(var, value);\n+\tgit_ident_config();\n\n-\tif (starts_with(var, \"branch.\"))\n-\t\treturn git_default_branch_config(var, value);\n+\tgit_default_i18n_config();\n\n-\tif (starts_with(var, \"push.\"))\n-\t\treturn git_default_push_config(var, value);\n+\tgit_default_branch_config();\n\n-\tif (starts_with(var, \"mailmap.\"))\n-\t\treturn git_default_mailmap_config(var, value);\n+\tgit_default_push_config();\n\n-\tif (starts_with(var, \"advice.\"))\n-\t\treturn git_default_advice_config(var, value);\n+\tgit_default_mailmap_config();\n\n-\tif (!strcmp(var, \"pager.color\") || !strcmp(var, \"color.pager\")) {\n-\t\tpager_use_color = git_config_bool(var,value);\n-\t\treturn 0;\n-\t}\n+\tgit_default_advice_config();\n\n-\tif (!strcmp(var, \"pack.packsizelimit\")) {\n-\t\tpack_size_limit_cfg = git_config_ulong(var, value);\n-\t\treturn 0;\n-\t}\n+\tif (!git_config_get_value(\"pager.color\", &v))\n+\t\tpager_use_color = git_config_bool(\"pager.color\",v);\n+\telse if (!git_config_get_value(\"color.pager\", &v))\n+\t\tpager_use_color = git_config_bool(\"color.pager\",v);\n+\n+\tgit_config_get_ulong(\"pack.packsizelimit\", &pack_size_limit_cfg);\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\n }\ndiff --git a/ident.c b/ident.c\nindex 1d9b6e7..da889cf 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -392,29 +392,26 @@ int author_ident_sufficiently_given(void)\n \treturn ident_is_sufficient(author_ident_explicitly_given);\n }\n\n-int git_ident_config(const char *var, const char *value, void *data)\n+void git_ident_config(void)\n {\n-\tif (!strcmp(var, \"user.name\")) {\n+\tconst char *value = NULL;\n+\tif (!git_config_get_value(\"user.name\", &value)) {\n \t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n+\t\t\tdie(\"Missing value for 'user.name'\");\n \t\tstrbuf_reset(&git_default_name);\n \t\tstrbuf_addstr(&git_default_name, value);\n \t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n-\t\treturn 0;\n \t}\n\n-\tif (!strcmp(var, \"user.email\")) {\n+\tif (!git_config_get_value(\"user.email\", &value)) {\n \t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n+\t\t\tdie(\"Missing value for 'user.email'\");\n \t\tstrbuf_reset(&git_default_email);\n \t\tstrbuf_addstr(&git_default_email, value);\n \t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\t\treturn 0;\n \t}\n-\n-\treturn 0;\n }\n\n static int buf_cmp(const char *a_begin, const char *a_end,\n-- \n1.9.0.GIT\n"},{"id":"246444","messageId":"vpqegxeeuyb.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"1405941145-12120-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-21T12:51:24Z","receivedAt":"2014-07-21T12:51:24Z","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> [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> \tSee [1] for the documentation of the new functions.\n>\n> This series builds on the top of 5def4132 in pu or topic[1] in the mailing list\n> with name \"git config cache & special querying API utilizing the cache\".\n\nIt's now called ta/config-set (see last \"What's cooking in git.git\").\n\n> All patches pass every test, but there is a catch, there is slight behaviour\n> change in most of them where originally the callback returns\n> config_error_nonbool() when it sees a NULL value for a key causing a die\n> specified in git_parse_source in config.c.\n>\n> The die also prints the file name and the line number as,\n>\n> \t\"die(\"bad config file line %d in %s\", cf->linenr, cf->name);\"\n>\n> We lose the fine grained error checking when switching to this method.\n\nI think a first step would be something like this:\n\n--- a/config.c\n+++ b/config.c\n@@ -656,6 +656,15 @@ int git_config_string(const char **dest, const char *var, const char *value)\n return 0;\n }\n \n+// TODO: either make it static or export it properly\n+int git_config_string_or_die(const char **dest, const char *var, const char *value)\n+{\n+ if (git_config_string(dest, var, value) < 0)\n+ die(\"bad config file (TODO: file/line info)\");\n+ else\n+ return 0;\n+}\n+\n int git_config_pathname(const char **dest, const char *var, const char *value)\n {\n if (!value)\n@@ -1336,7 +1345,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, const char\n {\n const char *value;\n if (!git_configset_get_value(cs, key, &value))\n- return git_config_string(dest, key, value);\n+ return git_config_string_or_die(dest, key, value);\n else\n return 1;\n }\n\nIn the original API, git_config_string was called at parsing time, hence\nthe file/line information was available through \"cf\". Here, we're\nquerying the cache which doesn't have this information yet.\n\nI initially thought that managing properly file/line information would\nbe just an addition, but this example shows that it is actually needed\nto be feature-complete wrt the old API. And I think we should be\nfeature-complete (i.e. make the code cleaner without harming the user).\n\nSo, I think it now makes sense to resurect your \"file line info\" patch:\n\n http://article.gmane.org/gmane.comp.version-control.git/253123\n\nNow that the series is properly reviewed, avoid modifying existing\npatches as much as possible, and add these file/line info on top of the\nexisting.\n\nI think you need to:\n\n1) Modify the hashmap data structure and the code that fills it in to\n store the file/line info (already done in your previous WIP patch).\n\n2) Add a by-address parameter to git_configset_get_value that allows the\n user to get the file and line information. In your previous patch,\n that would mean returning a pointer to the corresponding struct\n key_source.\n\n3) Pass this information to git_config_string_or_die, and die with the\n right message (with a helper like die_config(struct key_source *ks)\n that takes care of the formatting)\n\n4) apply the same to git_config_get_.\n\nI'd actually add a step 0) before that: add a test that checks your\nbehavior change. The test should pass without your patches, and fail\nwith your current patch. Then, it should pass again once you completed\nthe work.\n\nOn a side note, re-reading your previous patch, I found this which\nsounds suspicious:\n\n+\tstruct config_hash_entry *e;\n+\tstruct string_list_item *si;\n+\tstruct key_source *ks = xmalloc(sizeof(*e));\n\nDidn't you mean xmalloc(sizeof(*ks))?\n ^^\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246445","messageId":"vpq8unmeuwa.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"1405941145-12120-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-21T12:52:37Z","receivedAt":"2014-07-21T12:52:37Z","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> --- 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> +char *alias_lookup(const char* alias)\n\nStyle: keep the * stuck to alias.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246447","messageId":"53CD1280.1080107@gmail.com","threadId":"37179","inReplyTo":"vpqegxeeuyb.fsf@anie.imag.fr","subject":"Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T13:15:44Z","receivedAt":"2014-07-21T13:15:44Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/21/2014 6:21 PM, Matthieu Moy wrote:\n> Tanay Abhra writes:\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>> \tSee [1] for the documentation of the new functions.\n>>\n>> This series builds on the top of 5def4132 in pu or topic[1] in the mailing list\n>> with name \"git config cache & special querying API utilizing the cache\".\n> \n> It's now called ta/config-set (see last \"What's cooking in git.git\").\n>\n\nNoted. More below.\n\n>> All patches pass every test, but there is a catch, there is slight behaviour\n>> change in most of them where originally the callback returns\n>> config_error_nonbool() when it sees a NULL value for a key causing a die\n>> specified in git_parse_source in config.c.\n>>\n>> The die also prints the file name and the line number as,\n>>\n>> \t\"die(\"bad config file line %d in %s\", cf->linenr, cf->name);\"\n>>\n>> We lose the fine grained error checking when switching to this method.\n> \n> I think a first step would be something like this:\n> \n> --- a/config.c\n> +++ b/config.c\n> @@ -656,6 +656,15 @@ int git_config_string(const char **dest, const char *var, const char *value)\n> return 0;\n> }\n> \n> +// TODO: either make it static or export it properly\n> +int git_config_string_or_die(const char **dest, const char *var, const char *value)\n> +{\n> + if (git_config_string(dest, var, value) < 0)\n> + die(\"bad config file (TODO: file/line info)\");\n> + else\n> + return 0;\n> +}\n> +\n> int git_config_pathname(const char **dest, const char *var, const char *value)\n> {\n> if (!value)\n> @@ -1336,7 +1345,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, const char\n> {\n> const char *value;\n> if (!git_configset_get_value(cs, key, &value))\n> - return git_config_string(dest, key, value);\n> + return git_config_string_or_die(dest, key, value);\n> else\n> return 1;\n> }\n> \n> In the original API, git_config_string was called at parsing time, hence\n> the file/line information was available through \"cf\". Here, we're\n> querying the cache which doesn't have this information yet.\n> \n> I initially thought that managing properly file/line information would\n> be just an addition, but this example shows that it is actually needed\n> to be feature-complete wrt the old API. And I think we should be\n> feature-complete (i.e. make the code cleaner without harming the user).\n> \n> So, I think it now makes sense to resurect your \"file line info\" patch:\n> \n> http://article.gmane.org/gmane.comp.version-control.git/253123\n> \n> Now that the series is properly reviewed, avoid modifying existing\n> patches as much as possible, and add these file/line info on top of the\n> existing.\n> \n> I think you need to:\n> \n> 1) Modify the hashmap data structure and the code that fills it in to\n> store the file/line info (already done in your previous WIP patch).\n> \n> 2) Add a by-address parameter to git_configset_get_value that allows the\n> user to get the file and line information. In your previous patch,\n> that would mean returning a pointer to the corresponding struct\n> key_source.\n\nWill this extra complexity be good for \"git_configset_get_value\"?\nInstead can we provide a function like die_config(char *key)\nwhich prints\n\tdie(\"bad config file line %d in %s\", linenr, filename);?\nA variation would be die_config_multi(char *key, char *value)\nfor multi valued keys.\n\n> 3) Pass this information to git_config_string_or_die, and die with the\n> right message (with a helper like die_config(struct key_source *ks)\n> that takes care of the formatting)\n\nNo need for passing if we use the above method. We will just call die_config()\ninside it for NULL values\n\n> 4) apply the same to git_config_get_.\n>\n> I'd actually add a step 0) before that: add a test that checks your\n> behavior change. The test should pass without your patches, and fail\n> with your current patch. Then, it should pass again once you completed\n> the work.\n>\n\nNoted, I will add it.\n\n> On a side note, re-reading your previous patch, I found this which\n> sounds suspicious:\n> \n> +\tstruct config_hash_entry *e;\n> +\tstruct string_list_item *si;\n> +\tstruct key_source *ks = xmalloc(sizeof(*e));\n> \n> Didn't you mean xmalloc(sizeof(*ks))?\n>\n\nYikes, Thanks.\n"},{"id":"246449","messageId":"53CD1900.6040909@ramsay1.demon.co.uk","threadId":"37179","inReplyTo":"1405941145-12120-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()`","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-21T13:43:28Z","receivedAt":"2014-07-21T13:43:28Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 21/07/14 12:12, Tanay Abhra wrote:\n> Use `git_config_get_string()` instead of `git_config()` to take advantage of\n> the config-set API which provides a cleaner control flow.\n> The function now raises an error instead of dying when a NULL value is found.\n> \n> Signed-off-by: Tanay Abhra \n> ---\n> alias.c | 27 +++++++--------------------\n> 1 file changed, 7 insertions(+), 20 deletions(-)\n> \n> diff --git a/alias.c b/alias.c\n> index 758c867..a453bd8 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> +char *alias_lookup(const char* alias)\n\nNo, this is not C++. :-D\n\n> {\n> -\talias_key = alias;\n> -\talias_val = NULL;\n> -\tgit_config(alias_lookup_cb, NULL);\n> -\treturn alias_val;\n> +\tconst char *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 (char*)v;\n> }\n> \n> #define SPLIT_CMDLINE_BAD_ENDING 1\n> \n"},{"id":"246450","messageId":"vpqha2addyn.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"53CCFD02.6010704@gmail.com","subject":"Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-21T13:43:44Z","receivedAt":"2014-07-21T13:43:44Z","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> Consider this as a proof of concept as the others callers have to be rewritten\n> as well.\n> I think that it is not so buggy as it passes all the tests.\n\nBefore and after your patch, git_default_config() is called once per\nconfig key. Before the patch, it made sense, but after the patch, it\ndoesn't anymore, as it loads the whole config file in one call.\n\nI think it's OK to have this as an intermediate patch, but it should be\nclear to reviewers.\n\nAt the end of your series, git_default_config should become an\nargumentless function.\n\nActually, I'm wondering whether it makes sense to keep\ngit_default_config as-is, and introduce a new function like\ngit_load_default_config(void), initially empty. Then, move and change\ncode from git_default_config(...) to git_load_default_config(), and\nfinally remove git_default_config(...) once it's empty.\n\nYou have several decl-after-statements in your patch. You should have\nsomething like this in config.mak to catch them:\n\nCFLAGS += -Wdeclaration-after-statement -Wall -Werror\n\nThere are several (long unsigned int*) casts that seem useless to me.\nUseless casts distract the reader, and prevent usefull warnings from the\ncompiler.\n\nActually, I'm wondering whether these casts are safe (they cast from\nsize_t * to ulong *). On Windows 64 bits for example, sizeof(size_t) ==\n8 and sizeof(unsigned long) == 4 if google informed me correctly. On a\nbig-endian machine, this would be totally broken.\n\nIt is probably safer to add a new git_config_get_size_t analog to\ngit_config_get_ulong.\n\n> +\tif > +\tgit_config_get_string(\"core.notesref\", (const char**)¬es_ref_name);\n\nThis cast is needed only because notes_ref_name is declared as\nnon-const, but a better fix would be to make the variable const, and\nremove the cast.\n\nThe following patch solves these issues (modulo the question above on\ncast safety).\n\ndiff --git a/cache.h b/cache.h\nindex e667d92..1271904 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -674,7 +674,7 @@ enum object_creation_mode {\n \n extern enum object_creation_mode object_creation_mode;\n \n-extern char *notes_ref_name;\n+extern const char *notes_ref_name;\n \n extern int grafts_replace_parents;\n \ndiff --git a/config.c b/config.c\nindex 72196a9..c2664c3 100644\n--- a/config.c\n+++ b/config.c\n@@ -669,6 +669,9 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n static void git_default_core_config(void)\n {\n const char *value = NULL;\n+ int abbrev;\n+ int level;\n+ const char *comment;\n /* This needs a better name */\n git_config_get_bool(\"core.filemode\", &trust_executable_bit);\n git_config_get_bool(\"core.trustctime\", &trust_ctime);\n@@ -690,13 +693,11 @@ static void git_default_core_config(void)\n git_config_get_bool(\"core.logallrefupdates\", &log_all_ref_updates);\n git_config_get_bool(\"core.warnambiguousrefs\", &warn_ambiguous_refs);\n \n- int abbrev;\n if (!git_config_get_int(\"core.abbrev\", &abbrev)) {\n if (abbrev >= minimum_abbrev && abbrev <= 40)\n default_abbrev = abbrev;\n }\n \n- int level;\n if (!git_config_get_int(\"core.loosecompression\", &level)) {\n if (level == -1)\n level = Z_DEFAULT_COMPRESSION;\n@@ -717,7 +718,7 @@ static void git_default_core_config(void)\n zlib_compression_level = level;\n }\n \n- if (!git_config_get_ulong(\"core.packedgitwindowsize\", (long unsigned int*)&packed_git_window_size)) {\n+ if (!git_config_get_ulong(\"core.packedgitwindowsize\", &packed_git_window_size)) {\n int pgsz_x2 = getpagesize() * 2;\n \n /* This value must be multiple of (pagesize * 2) */\n@@ -728,8 +729,8 @@ static void git_default_core_config(void)\n }\n \n git_config_get_ulong(\"core.bigfilethreshold\", &big_file_threshold);\n- git_config_get_ulong(\"core.packedgitlimit\", (long unsigned int*)&packed_git_limit);\n- git_config_get_ulong(\"core.deltabasecachelimit\", (long unsigned int*)&delta_base_cache_limit);\n+ git_config_get_ulong(\"core.packedgitlimit\", &packed_git_limit);\n+ git_config_get_ulong(\"core.deltabasecachelimit\", &delta_base_cache_limit);\n \n if (!git_config_get_value(\"core.autocrlf\", &value)) {\n if (value && !strcasecmp(value, \"input\")) {\n@@ -760,11 +761,10 @@ static void git_default_core_config(void)\n die(\"core.autocrlf=input conflicts with core.eol=crlf\");\n }\n \n- git_config_get_string(\"core.notesref\", (const char**)¬es_ref_name);\n+ git_config_get_string(\"core.notesref\", ¬es_ref_name);\n git_config_get_string(\"core.pager\", &pager_program);\n git_config_get_string(\"core.editor\", &editor_program);\n \n- const char *comment;\n if (!git_config_get_string(\"core.commentchar\", &comment)) {\n if (!strcasecmp(comment, \"auto\"))\n auto_comment_line_char = 1;\ndiff --git a/environment.c b/environment.c\nindex 565f652..21d4dbb 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -56,7 +56,7 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS\n #endif\n enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n-char *notes_ref_name;\n+const char *notes_ref_name;\n int grafts_replace_parents = 1;\n int core_apply_sparse_checkout;\n int merge_log_config = -1;\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246451","messageId":"vpqy4vmakq3.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"53CD1280.1080107@gmail.com","subject":"Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-21T13:45:56Z","receivedAt":"2014-07-21T13:45:56Z","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 7/21/2014 6:21 PM, Matthieu Moy wrote:\n>> 2) Add a by-address parameter to git_configset_get_value that allows the\n>> user to get the file and line information. In your previous patch,\n>> that would mean returning a pointer to the corresponding struct\n>> key_source.\n>\n> Will this extra complexity be good for \"git_configset_get_value\"?\n> Instead can we provide a function like die_config(char *key)\n> which prints\n> \tdie(\"bad config file line %d in %s\", linenr, filename);?\n\nWhere would you call this function, and where would you take linenr and\nfilename?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246453","messageId":"53CD1CC7.1020004@ramsay1.demon.co.uk","threadId":"37179","inReplyTo":"53CCFD02.6010704@gmail.com","subject":"Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-21T13:59:35Z","receivedAt":"2014-07-21T13:59:35Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 21/07/14 12:44, Tanay Abhra wrote:\n> Use `git_config_get_*()` family instead of `git_config()` to take advantage of\n> the config-set API which provides a cleaner control flow.\n> \n> Signed-off-by: Tanay Abhra \n> ---\n> Consider this as a proof of concept as the others callers have to be rewritten\n> as well.\n> I think that it is not so buggy as it passes all the tests.\n> After the first six patches in the series which you have already seen there are\n> five or four left which can rewritten without touching git_default_config().\n> \n> Thus, this rewrite will serve as the base for rewriting other git_config()\n> callers which pass control to git_default_config() at the end of the function.\n> Also there are more than thirty direct callers to git_default_config()\n> (i.e git_config(git_default_config, NULL)), so this rewrite solves them\n> in one sweep.\n> \n> Slight behaviour change, config_error_nonbool() has been replaced with\n> die(\"Missing value for '%s'\", var);.\n> The original code also alerted the file name and the line number which we lose here.\n> \n> Cheers,\n> Tanay Abhra.\n> \n> advice.c | 18 ++--\n> advice.h | 2 +-\n> cache.h | 2 +-\n> config.c | 287 ++++++++++++++++++++-------------------------------------------\n> ident.c | 15 ++--\n> 5 files changed, 104 insertions(+), 220 deletions(-)\n> \n\n[snip]\n\n\n> diff --git a/config.c b/config.c\n> index fe9f399..72196a9 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -666,88 +666,47 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n> \treturn 0;\n> }\n> \n> -static int git_default_core_config(const char *var, const char *value)\n> +static void git_default_core_config(void)\n> {\n> +\tconst char *value = NULL;\n> \t/* This needs a better name */\n> -\tif (!strcmp(var, \"core.filemode\")) {\n> -\t\ttrust_executable_bit = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\tif (!strcmp(var, \"core.trustctime\")) {\n> -\t\ttrust_ctime = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\tif (!strcmp(var, \"core.checkstat\")) {\n> +\tgit_config_get_bool(\"core.filemode\", &trust_executable_bit);\n> +\tgit_config_get_bool(\"core.trustctime\", &trust_ctime);\n> +\n> +\tif (!git_config_get_value(\"core.checkstat\", &value)) {\n> \t\tif (!strcasecmp(value, \"default\"))\n> \t\t\tcheck_stat = 1;\n> \t\telse if (!strcasecmp(value, \"minimal\"))\n> \t\t\tcheck_stat = 0;\n> \t}\n> \n> -\tif (!strcmp(var, \"core.quotepath\")) {\n> -\t\tquote_path_fully = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.symlinks\")) {\n> -\t\thas_symlinks = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.ignorecase\")) {\n> -\t\tignore_case = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.attributesfile\"))\n> -\t\treturn git_config_pathname(&git_attributes_file, var, value);\n> -\n> -\tif (!strcmp(var, \"core.bare\")) {\n> -\t\tis_bare_repository_cfg = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.ignorestat\")) {\n> -\t\tassume_unchanged = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.prefersymlinkrefs\")) {\n> -\t\tprefer_symlink_refs = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.logallrefupdates\")) {\n> -\t\tlog_all_ref_updates = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> +\tgit_config_get_bool(\"core.quotepath\", "e_path_fully);\n> +\tgit_config_get_bool(\"core.symlinks\", &has_symlinks);\n> +\tgit_config_get_bool(\"core.ignorecase\", &ignore_case);\n> +\tgit_config_get_pathname(\"core.attributesfile\", &git_attributes_file);\n> +\tgit_config_get_bool(\"core.bare\", &is_bare_repository_cfg);\n> +\tgit_config_get_bool(\"core.ignorestat\", &assume_unchanged);\n> +\tgit_config_get_bool(\"core.prefersymlinkrefs\", &prefer_symlink_refs);\n> +\tgit_config_get_bool(\"core.logallrefupdates\", &log_all_ref_updates);\n> +\tgit_config_get_bool(\"core.warnambiguousrefs\", &warn_ambiguous_refs);\n> \n> -\tif (!strcmp(var, \"core.warnambiguousrefs\")) {\n> -\t\twarn_ambiguous_refs = git_config_bool(var, value);\n> -\t\treturn 0;\n> +\tint abbrev;\n\ndeclaration after statement.\n\n> +\tif (!git_config_get_int(\"core.abbrev\", &abbrev)) {\n> +\t\tif (abbrev >= minimum_abbrev && abbrev <= 40)\n> +\t\t\tdefault_abbrev = abbrev;\n> \t}\n> \n> -\tif (!strcmp(var, \"core.abbrev\")) {\n> -\t\tint abbrev = git_config_int(var, value);\n> -\t\tif (abbrev < minimum_abbrev || abbrev > 40)\n> -\t\t\treturn -1;\n> -\t\tdefault_abbrev = abbrev;\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.loosecompression\")) {\n> -\t\tint level = git_config_int(var, value);\n> +\tint level;\n\nditto.\n\n> +\tif (!git_config_get_int(\"core.loosecompression\", &level)) {\n> \t\tif (level == -1)\n> \t\t\tlevel = Z_DEFAULT_COMPRESSION;\n> \t\telse if (level < 0 || level > Z_BEST_COMPRESSION)\n> \t\t\tdie(\"bad zlib compression level %d\", level);\n> \t\tzlib_compression_level = level;\n> \t\tzlib_compression_seen = 1;\n> -\t\treturn 0;\n> \t}\n> \n> -\tif (!strcmp(var, \"core.compression\")) {\n> -\t\tint level = git_config_int(var, value);\n> +\tif (!git_config_get_int(\"core.compression\", &level)) {\n> \t\tif (level == -1)\n> \t\t\tlevel = Z_DEFAULT_COMPRESSION;\n> \t\telse if (level < 0 || level > Z_BEST_COMPRESSION)\n> @@ -756,57 +715,39 @@ static int git_default_core_config(const char *var, const char *value)\n> \t\tcore_compression_seen = 1;\n> \t\tif (!zlib_compression_seen)\n> \t\t\tzlib_compression_level = level;\n> -\t\treturn 0;\n> \t}\n> \n> -\tif (!strcmp(var, \"core.packedgitwindowsize\")) {\n> +\tif (!git_config_get_ulong(\"core.packedgitwindowsize\", (long unsigned int*)&packed_git_window_size)) {\n> \t\tint pgsz_x2 = getpagesize() * 2;\n> -\t\tpacked_git_window_size = git_config_ulong(var, value);\n> \n> \t\t/* This value must be multiple of (pagesize * 2) */\n> \t\tpacked_git_window_size /= pgsz_x2;\n> \t\tif (packed_git_window_size < 1)\n> \t\t\tpacked_git_window_size = 1;\n> \t\tpacked_git_window_size *= pgsz_x2;\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.bigfilethreshold\")) {\n> -\t\tbig_file_threshold = git_config_ulong(var, value);\n> -\t\treturn 0;\n> \t}\n> \n> -\tif (!strcmp(var, \"core.packedgitlimit\")) {\n> -\t\tpacked_git_limit = git_config_ulong(var, value);\n> -\t\treturn 0;\n> -\t}\n> +\tgit_config_get_ulong(\"core.bigfilethreshold\", &big_file_threshold);\n> +\tgit_config_get_ulong(\"core.packedgitlimit\", (long unsigned int*)&packed_git_limit);\n> +\tgit_config_get_ulong(\"core.deltabasecachelimit\", (long unsigned int*)&delta_base_cache_limit);\n> \n> -\tif (!strcmp(var, \"core.deltabasecachelimit\")) {\n> -\t\tdelta_base_cache_limit = git_config_ulong(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.autocrlf\")) {\n> +\tif (!git_config_get_value(\"core.autocrlf\", &value)) {\n> \t\tif (value && !strcasecmp(value, \"input\")) {\n> \t\t\tif (core_eol == EOL_CRLF)\n> -\t\t\t\treturn error(\"core.autocrlf=input conflicts with core.eol=crlf\");\n> +\t\t\t\tdie(\"core.autocrlf=input conflicts with core.eol=crlf\");\n> \t\t\tauto_crlf = AUTO_CRLF_INPUT;\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t\tauto_crlf = git_config_bool(var, value);\n> -\t\treturn 0;\n> +\t\t} else\n> +\t\t\tauto_crlf = git_config_bool(\"core.autocrlf\", value);\n> \t}\n> \n> -\tif (!strcmp(var, \"core.safecrlf\")) {\n> -\t\tif (value && !strcasecmp(value, \"warn\")) {\n> +\tif (!git_config_get_value(\"core.safecrlf\", &value)) {\n> +\t\tif (value && !strcasecmp(value, \"warn\"))\n> \t\t\tsafe_crlf = SAFE_CRLF_WARN;\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t\tsafe_crlf = git_config_bool(var, value);\n> -\t\treturn 0;\n> +\t\telse\n> +\t\t\tsafe_crlf = git_config_bool(\"core.safecrlf\", value);\n> \t}\n> \n> -\tif (!strcmp(var, \"core.eol\")) {\n> +\tif (!git_config_get_value(\"core.eol\", &value)) {\n> \t\tif (value && !strcasecmp(value, \"lf\"))\n> \t\t\tcore_eol = EOL_LF;\n> \t\telse if (value && !strcasecmp(value, \"crlf\"))\n> @@ -816,108 +757,74 @@ static int git_default_core_config(const char *var, const char *value)\n> \t\telse\n> \t\t\tcore_eol = EOL_UNSET;\n> \t\tif (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)\n> -\t\t\treturn error(\"core.autocrlf=input conflicts with core.eol=crlf\");\n> -\t\treturn 0;\n> +\t\t\tdie(\"core.autocrlf=input conflicts with core.eol=crlf\");\n> \t}\n> \n> -\tif (!strcmp(var, \"core.notesref\")) {\n> -\t\tnotes_ref_name = xstrdup(value);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!strcmp(var, \"core.pager\"))\n> -\t\treturn git_config_string(&pager_program, var, value);\n> -\n> -\tif (!strcmp(var, \"core.editor\"))\n> -\t\treturn git_config_string(&editor_program, var, value);\n> +\tgit_config_get_string(\"core.notesref\", (const char**)¬es_ref_name);\n> +\tgit_config_get_string(\"core.pager\", &pager_program);\n> +\tgit_config_get_string(\"core.editor\", &editor_program);\n> \n> -\tif (!strcmp(var, \"core.commentchar\")) {\n> -\t\tconst char *comment;\n> -\t\tint ret = git_config_string(&comment, var, value);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> -\t\telse if (!strcasecmp(comment, \"auto\"))\n> +\tconst char *comment;\n\nditto. I gave up here.\n\nATB,\nRamsay Jones\n"},{"id":"246454","messageId":"53CD1D0F.5000605@gmail.com","threadId":"37179","inReplyTo":"vpqy4vmakq3.fsf@anie.imag.fr","subject":"Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T14:00:47Z","receivedAt":"2014-07-21T14:00:47Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/21/2014 7:15 PM, Matthieu Moy wrote:\n> Tanay Abhra writes:\n> \n>> On 7/21/2014 6:21 PM, Matthieu Moy wrote:\n>>> 2) Add a by-address parameter to git_configset_get_value that allows the\n>>> user to get the file and line information. In your previous patch,\n>>> that would mean returning a pointer to the corresponding struct\n>>> key_source.\n>>\n>> Will this extra complexity be good for \"git_configset_get_value\"?\n>> Instead can we provide a function like die_config(char *key)\n>> which prints\n>> \tdie(\"bad config file line %d in %s\", linenr, filename);?\n> \n> Where would you call this function, and where would you take linenr and\n> filename?\n>\n\nUsage can be like this,\n\nif(!git_config_get_value(k, &v)) {\n\tif (!v) {\n\t\tconfig_error_nonbool(k);\n\t\tdie_config(k);\n\t\t/* die_config calls git_config_get_value_multi for 'k',\n\t\t * gets the string list with the util pointer containing\n\t\t * the linenr and the file name, dies printing the message.\n\t\t */\n\t} else\n\t\t/* do work */\n}\n\nAbove example works just like the current code. Currently the callbacks\ndoes not have the access to the linenr and file name anyway.\n"},{"id":"246456","messageId":"53CD2273.3000600@gmail.com","threadId":"37179","inReplyTo":"vpqha2addyn.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T14:23:47Z","receivedAt":"2014-07-21T14:23:47Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n> \n>> +\tif > +\tgit_config_get_string(\"core.notesref\", (const char**)¬es_ref_name);\n> \n> This cast is needed only because notes_ref_name is declared as\n> non-const, but a better fix would be to make the variable const, and\n> remove the cast.\n\nSame casts had to be used in imap-send.c patch, I will have to use an\nintermediate variable there to remove the cast thus destroying the one\nliners or will have to update the variable declarations.\n"},{"id":"246458","messageId":"vpqlhrm4wiu.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"53CD1D0F.5000605@gmail.com","subject":"Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-21T14:27:37Z","receivedAt":"2014-07-21T14:27:37Z","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 7/21/2014 7:15 PM, Matthieu Moy wrote:\n>> Tanay Abhra writes:\n>> \n>>> On 7/21/2014 6:21 PM, Matthieu Moy wrote:\n>>>> 2) Add a by-address parameter to git_configset_get_value that allows the\n>>>> user to get the file and line information. In your previous patch,\n>>>> that would mean returning a pointer to the corresponding struct\n>>>> key_source.\n>>>\n>>> Will this extra complexity be good for \"git_configset_get_value\"?\n>>> Instead can we provide a function like die_config(char *key)\n>>> which prints\n>>> \tdie(\"bad config file line %d in %s\", linenr, filename);?\n>> \n>> Where would you call this function, and where would you take linenr and\n>> filename?\n>>\n>\n> Usage can be like this,\n>\n> if(!git_config_get_value(k, &v)) {\n> \tif (!v) {\n> \t\tconfig_error_nonbool(k);\n> \t\tdie_config(k);\n> \t\t/* die_config calls git_config_get_value_multi for 'k',\n> \t\t * gets the string list with the util pointer containing\n> \t\t * the linenr and the file name, dies printing the message.\n> \t\t */\n> \t} else\n> \t\t/* do work */\n> }\n\nOK, so you query the cache twice (which is OK, it's cheap and happens\njust once before dying). That would work too.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246459","messageId":"vpqzjg2204x.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"53CD2273.3000600@gmail.com","subject":"Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-21T15:37:50Z","receivedAt":"2014-07-21T15:37:50Z","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>> \n>>> +\tif > +\tgit_config_get_string(\"core.notesref\", (const char**)¬es_ref_name);\n>> \n>> This cast is needed only because notes_ref_name is declared as\n>> non-const, but a better fix would be to make the variable const, and\n>> remove the cast.\n>\n> Same casts had to be used in imap-send.c patch, I will have to use an\n> intermediate variable there to remove the cast thus destroying the one\n> liners or will have to update the variable declarations.\n\nUpdating the declaration like this should just work:\n\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1324,7 +1324,7 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)\n return 1;\n }\n \n-static char *imap_folder;\n+static const char *imap_folder;\n \n static void git_imap_config(void)\n {\n@@ -1332,7 +1332,7 @@ static void git_imap_config(void)\n \n git_config_get_bool(\"imap.sslverify\", &server.ssl_verify);\n git_config_get_bool(\"imap.preformattedhtml\", &server.use_html);\n- git_config_get_string(\"imap.folder\", (const char**)&imap_folder);\n+ git_config_get_string(\"imap.folder\", &imap_folder);\n \n if (!git_config_get_value(\"imap.host\", &val)) {\n if(!val)\n\nIn general, most strings one manipulates are \"const char *\", it's\nfrequent to modify a pointer to a string, but rather rare to modify the\nstring itself.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246468","messageId":"xmqqiomqk2yu.fsf@gitster.dls.corp.google.com","threadId":"37179","inReplyTo":"1405941145-12120-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 2/6] branch.c: replace `git_config()` with `git_config_get_string()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-21T17:59:21Z","receivedAt":"2014-07-21T17:59:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> Use `git_config_get_string()` instead of `git_config()` to take advantage of\n> the config-set API which provides a cleaner control flow.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> branch.c | 24 ++++--------------------\n> 1 file changed, 4 insertions(+), 20 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 46e8aa8..827307f 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> +\tconst char *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((char*)v);\n\nIn this cast, I smell an API mistake to insist an extra constness to\nthe output parameter of git_config_get_string() in [3/4] of the\nprevious series. Unlike the underlying git_config_get_value(),\nwhich lets the caller peek into the internal cached copy, the caller\nof git_config_get_string() is given its own copy, and I do not\noffhand see a good reason to forbid the caller from modifying it.\n\n> \tstrbuf_release(&name);\n> \treturn 0;\n> }\n"},{"id":"246469","messageId":"xmqqegxek2w4.fsf@gitster.dls.corp.google.com","threadId":"37179","inReplyTo":"1405941145-12120-4-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 3/6] imap-send.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-21T18:00:59Z","receivedAt":"2014-07-21T18:00:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> +\tgit_config_get_string(\"imap.folder\", (const char**)&imap_folder);\n\nThe same \"why (const char **)--is that an API mistake?\" question\napplies here and other calls to git_config_get_string() in this\npatch.\n"},{"id":"246470","messageId":"vpqppgymvri.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"xmqqiomqk2yu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/6] branch.c: replace `git_config()` with `git_config_get_string()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-21T18:06:41Z","receivedAt":"2014-07-21T18:06:41Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Tanay Abhra writes:\n>\n>> -\tif (cb.value)\n>> -\t\tstrbuf_addstr(buf, cb.value);\n>> +\tstrbuf_addstr(buf, v);\n>> +\tfree((char*)v);\n>\n> In this cast, I smell an API mistake to insist an extra constness to\n> the output parameter of git_config_get_string() in [3/4] of the\n> previous series. Unlike the underlying git_config_get_value(),\n> which lets the caller peek into the internal cached copy, the caller\n> of git_config_get_string() is given its own copy, and I do not\n> offhand see a good reason to forbid the caller from modifying it.\n\nIndeed. My suggestion to change the declaration of char * variables\nactually seem like a workaround for an API mistake after reading this.\n\n(well, actually, the declaration could be modified if we think it brings\nany new safety, but that should be another topic)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246471","messageId":"xmqqa982k2lm.fsf@gitster.dls.corp.google.com","threadId":"37179","inReplyTo":"vpqzjg2204x.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-21T18:07:17Z","receivedAt":"2014-07-21T18:07:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> In general, most strings one manipulates are \"const char *\", it's\n> frequent to modify a pointer to a string, but rather rare to modify the\n> string itself.\n\nWe seem to have a disagreement. Unlike git_config_get_value() that\nlets callers peek the only cached copy, git_config_get_string()\ngives its caller a new copy that the caller needs to free. Such a\nnew string can and should be given as mutable, I would say.\n"},{"id":"246472","messageId":"53CD57C3.4000806@gmail.com","threadId":"37179","inReplyTo":"xmqqiomqk2yu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/6] branch.c: replace `git_config()` with `git_config_get_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-21T18:11:15Z","receivedAt":"2014-07-21T18:11:15Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/21/2014 11:29 PM, Junio C Hamano wrote:\n> Tanay Abhra writes:\n> \n>> Use `git_config_get_string()` instead of `git_config()` to take advantage of\n>> the config-set API which provides a cleaner control flow.\n>>\n>> Signed-off-by: Tanay Abhra \n>> ---\n>> branch.c | 24 ++++--------------------\n>> 1 file changed, 4 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/branch.c b/branch.c\n>> index 46e8aa8..827307f 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>> +\tconst char *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((char*)v);\n> \n> In this cast, I smell an API mistake to insist an extra constness to\n> the output parameter of git_config_get_string() in [3/4] of the\n> previous series. Unlike the underlying git_config_get_value(),\n> which lets the caller peek into the internal cached copy, the caller\n> of git_config_get_string() is given its own copy, and I do not\n> offhand see a good reason to forbid the caller from modifying it.\n>\n\nI modeled git_config_get_string() on the previously existing API function\ngit_config_string() with the signature,\nint git_config_string(const char **dest, const char *var, const char *value).\n\nBut after writing this series I do think there isn't a good reason to\nkeep the constness in the new function also since the dest is given\nits own copy.\n\n\n>> \tstrbuf_release(&name);\n>> \treturn 0;\n>> }\n"},{"id":"246491","messageId":"vpqsiltkeu9.fsf@anie.imag.fr","threadId":"37179","inReplyTo":"xmqqa982k2lm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-22T07:55:10Z","receivedAt":"2014-07-22T07:55:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> In general, most strings one manipulates are \"const char *\", it's\n>> frequent to modify a pointer to a string, but rather rare to modify the\n>> string itself.\n>\n> We seem to have a disagreement. Unlike git_config_get_value() that\n> lets callers peek the only cached copy, git_config_get_string()\n> gives its caller a new copy that the caller needs to free. Such a\n> new string can and should be given as mutable, I would say.\n\nYou're right (I guess you replied to this one before seeing my other\nmessage). imap_folder could be declared const char *, but\ngit_config_get_string() shouldn't be the one to force this.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246497","messageId":"20140722112311.GB386@peff.net","threadId":"37179","inReplyTo":"1405941145-12120-7-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 6/6] notes-util.c: replace `git_config()` with `git_config_get_value()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-22T11:23:11Z","receivedAt":"2014-07-22T11:23:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 21, 2014 at 04:12:25AM -0700, Tanay Abhra wrote:\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> +\tconst char *v;\n> +\tstruct strbuf key = STRBUF_INIT;\n> +\tstrbuf_addf(&key, \"notes.rewrite.%s\", c->cmd);\n> +\tgit_config_get_bool(key.buf, &c->enabled);\n> +\tstrbuf_release(&key);\n\nI wonder if it is worth teaching the accessors to form such strings\nthemselves, like:\n\n void git_config_get_bool(int *out, const char *fmt, ...);\n\nso you could do:\n\n git_config_get_bool(&c->enabled, \"notes.rewrite.%s\", c->cmd);\n\nThe \"normal\" cases where we do not need any run-time modification could\nbe used as-is (I swapped the parameter order above, but you would not\nhave to do so). But I guess that would require us doing extra work in\nthe common \"normal\" case to print the string into a buffer, even though\nit does not have any expansions (or to do a strchr, I guess, to look for\n\"%\"). It's probably not worth it considering how few config keys have\ncomputed values like this.\n\n-Peff\n"},{"id":"247068","messageId":"87bns5xxh4.fsf@naesten.mooo.com","threadId":"37179","inReplyTo":"53CD1900.6040909@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()`","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2014-07-31T17:13:43Z","receivedAt":"2014-07-31T17:13:43Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"Ramsay Jones writes:\n> On 21/07/14 12:12, Tanay Abhra wrote:\n>> -char *alias_lookup(const char *alias)\n>> +char *alias_lookup(const char* alias)\n>\n> No, this is not C++. :-D\n\nWhy would C++ make a difference? Shouldn't you *never* do that?\n\n-- \nHi! I'm a .signature virus! Copy me into your ~/.signature to help me spread!\n"}]}