{"thread":{"id":"37287","subject":"[PATCH 00/11] git_config callers rewritten with the new config-set API","startedAt":"2014-08-04T18:33:38Z","lastAt":"2014-08-06T20:04:14Z","messageCount":20,"participants":["Tanay Abhra","Eric Sunshine","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"247222","messageId":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":null,"subject":"[PATCH 00/11] git_config callers rewritten with the new config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:38Z","receivedAt":"2014-08-04T18:33:38Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"The ta/config-set API is more or less solidified.\n\nThis series builds on the top of 4c715ebb in pu (ta/config-set). On top of it,\nit also requires series [1] (Rewrite `git_config()` using config-set API) for\nproper error checking.\n\nThis series is the first batch of patches which rewrites the existing callers\nusing a non-callback approach.\nThis series aims to,\n\n* rewrite the existing callers, as you can see from the diff stat the bew API\n provides a much concise and clear control flow.\n\n* stress test the new API, see if any corner cases or deficiencies arise or not.\n\nThe series passes all the tests, only thing to watch is that the config variables\nthat have been rewritten are single valued only. Though I have tried my best to\nascertain it, still mistakes may arise.\n\np.s: I haven't decided yet about whether to introduce a new helper set, for example\n git_config_get_value_fmt() which would behave like strbuf_addf(). It will probably\n come in a later series.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/254633/\n\nTanay Abhra (11):\n\n alias.c | 25 ++++++------------------\n archive.c | 12 +++---------\n branch.c | 27 +++++++-------------------\n builtin/gc.c | 51 +++++++++++++++++++-----------------------------\n daemon.c | 27 +++++---------------------\n fetch-pack.c | 35 ++++++++-------------------------\n http-backend.c | 31 ++++++++++++-----------------\n imap-send.c | 61 ++++++++++++++++++++++++++--------------------------------\n pager.c | 40 +++++++++++++-------------------------\n read-cache.c | 14 +++-----------\n rerere.c | 43 ++++++++++++-----------------------------\n 11 files changed, 116 insertions(+), 250 deletions(-)\n\n-- \n1.9.0.GIT\n"},{"id":"247223","messageId":"1407177229-30081-2-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:39Z","receivedAt":"2014-08-04T18:33:39Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use `git_config_get_bool()` 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---\n daemon.c | 27 +++++----------------------\n 1 file changed, 5 insertions(+), 22 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex e6b51ed..fb16664 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -230,23 +230,6 @@ struct daemon_service {\n \tint overridable;\n };\n \n-static struct daemon_service *service_looking_at;\n-static int service_enabled;\n-\n-static int git_daemon_config(const char *var, const char *value, void *cb)\n-{\n-\tconst char *service;\n-\n-\tif (skip_prefix(var, \"daemon.\", &service) &&\n-\t !strcmp(service, service_looking_at->config_name)) {\n-\t\tservice_enabled = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\t/* we are not interested in parsing any other configuration here */\n-\treturn 0;\n-}\n-\n static int daemon_error(const char *dir, const char *msg)\n {\n \tif (!informative_errors)\n@@ -324,6 +307,7 @@ static int run_service(const char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n \tint enabled = service->enabled;\n+\tstruct strbuf var = STRBUF_INIT;\n \n \tloginfo(\"Request %s for '%s'\", service->name, dir);\n \n@@ -354,12 +338,11 @@ static int run_service(const char *dir, struct daemon_service *service)\n \t}\n \n \tif (service->overridable) {\n-\t\tservice_looking_at = service;\n-\t\tservice_enabled = -1;\n-\t\tgit_config(git_daemon_config, NULL);\n-\t\tif (0 <= service_enabled)\n-\t\t\tenabled = service_enabled;\n+\t\tstrbuf_addf(&var, \"daemon.%s\", service->config_name);\n+\t\tgit_config_get_bool(var.buf, &enabled);\n+\t\tstrbuf_release(&var);\n \t}\n+\n \tif (!enabled) {\n \t\tlogerror(\"'%s': service not enabled for '%s'\",\n \t\t\t service->name, path);\n-- \n1.9.0.GIT\n"},{"id":"247224","messageId":"1407177229-30081-3-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:40Z","receivedAt":"2014-08-04T18:33:40Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use `git_config_get_bool()` 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---\n http-backend.c | 31 ++++++++++++-------------------\n 1 file changed, 12 insertions(+), 19 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 80790bb..106ca6b 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -219,29 +219,22 @@ static void get_idx_file(char *name)\n \tsend_local_file(\"application/x-git-packed-objects-toc\", name);\n }\n \n-static int http_config(const char *var, const char *value, void *cb)\n+static void http_config(void)\n {\n-\tconst char *p;\n+\tint i, value = 0;\n+\tstruct strbuf var = STRBUF_INIT;\n \n-\tif (!strcmp(var, \"http.getanyfile\")) {\n-\t\tgetanyfile = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n+\tgit_config_get_bool(\"http.getanyfile\", &getanyfile);\n \n-\tif (skip_prefix(var, \"http.\", &p)) {\n-\t\tint i;\n-\n-\t\tfor (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n-\t\t\tstruct rpc_service *svc = &rpc_service[i];\n-\t\t\tif (!strcmp(p, svc->config_name)) {\n-\t\t\t\tsvc->enabled = git_config_bool(var, value);\n-\t\t\t\treturn 0;\n-\t\t\t}\n-\t\t}\n+\tfor (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n+\t\tstruct rpc_service *svc = &rpc_service[i];\n+\t\tstrbuf_addf(&var, \"http.%s\", svc->config_name);\n+\t\tif (!git_config_get_bool(var.buf, &value))\n+\t\t\tsvc->enabled = value;\n+\t\tstrbuf_reset(&var);\n \t}\n \n-\t/* we are not interested in parsing any other configuration here */\n-\treturn 0;\n+\tstrbuf_release(&var);\n }\n \n static struct rpc_service *select_service(const char *name)\n@@ -627,7 +620,7 @@ int main(int argc, char **argv)\n \t access(\"git-daemon-export-ok\", F_OK) )\n \t\tnot_found(\"Repository not exported: '%s'\", dir);\n \n-\tgit_config(http_config, NULL);\n+\thttp_config();\n \tcmd->imp(cmd_arg);\n \treturn 0;\n }\n-- \n1.9.0.GIT\n"},{"id":"247228","messageId":"1407177229-30081-4-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 03/11] read-cache.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:41Z","receivedAt":"2014-08-04T18:33:41Z","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\nadvantage of the config-set API which provides a cleaner control flow.\n\nUse an intermediate value, as `version` can not be used directly in\ngit_config_get_int() due to incompatible type.\n\nSigned-off-by: Tanay Abhra \n---\n read-cache.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5d3c8bd..acb132d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1238,24 +1238,16 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,\n \n #define INDEX_FORMAT_DEFAULT 3\n \n-static int index_format_config(const char *var, const char *value, void *cb)\n-{\n-\tunsigned int *version = cb;\n-\tif (!strcmp(var, \"index.version\")) {\n-\t\t*version = git_config_int(var, value);\n-\t\treturn 0;\n-\t}\n-\treturn 1;\n-}\n-\n static unsigned int get_index_format_default(void)\n {\n \tchar *envversion = getenv(\"GIT_INDEX_VERSION\");\n \tchar *endp;\n+\tint value;\n \tunsigned int version = INDEX_FORMAT_DEFAULT;\n \n \tif (!envversion) {\n-\t\tgit_config(index_format_config, &version);\n+\t\tif (!git_config_get_int(\"index.version\", &value))\n+\t\t\tversion = value;\n \t\tif (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n \t\t\twarning(_(\"index.version set, but the value is invalid.\\n\"\n \t\t\t\t \"Using version %i\"), INDEX_FORMAT_DEFAULT);\n-- \n1.9.0.GIT\n"},{"id":"247225","messageId":"1407177229-30081-5-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 04/11] archive.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:42Z","receivedAt":"2014-08-04T18:33:42Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Use `git_config_get_bool()` 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---\n archive.c | 12 +++---------\n 1 file changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 3fc0fb2..952a659 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -402,14 +402,6 @@ static int parse_archive_args(int argc, const char **argv,\n \treturn argc;\n }\n \n-static int git_default_archive_config(const char *var, const char *value,\n-\t\t\t\t void *cb)\n-{\n-\tif (!strcmp(var, \"uploadarchive.allowunreachable\"))\n-\t\tremote_allow_unreachable = git_config_bool(var, value);\n-\treturn git_default_config(var, value, cb);\n-}\n-\n int write_archive(int argc, const char **argv, const char *prefix,\n \t\t int setup_prefix, const char *name_hint, int remote)\n {\n@@ -420,7 +412,9 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \tif (setup_prefix && prefix == NULL)\n \t\tprefix = setup_git_directory_gently(&nongit);\n \n-\tgit_config(git_default_archive_config, NULL);\n+\tgit_config_get_bool(\"uploadarchive.allowunreachable\", &remote_allow_unreachable);\n+\tgit_config(git_default_config, NULL);\n+\n \tinit_tar_archiver();\n \tinit_zip_archiver();\n \n-- \n1.9.0.GIT\n"},{"id":"247227","messageId":"1407177229-30081-6-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 05/11] fetchpack.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:43Z","receivedAt":"2014-08-04T18:33:43Z","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---\n fetch-pack.c | 35 ++++++++---------------------------\n 1 file changed, 8 insertions(+), 27 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b8a58fa..a13e9db 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -869,34 +869,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n-static int fetch_pack_config(const char *var, const char *value, void *cb)\n+static void fetch_pack_config(void)\n {\n-\tif (strcmp(var, \"fetch.unpacklimit\") == 0) {\n-\t\tfetch_unpack_limit = git_config_int(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (strcmp(var, \"transfer.unpacklimit\") == 0) {\n-\t\ttransfer_unpack_limit = git_config_int(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (strcmp(var, \"repack.usedeltabaseoffset\") == 0) {\n-\t\tprefer_ofs_delta = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"fetch.fsckobjects\")) {\n-\t\tfetch_fsck_objects = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"transfer.fsckobjects\")) {\n-\t\ttransfer_fsck_objects = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n+\tgit_config_get_int(\"fetch.unpacklimit\", &fetch_unpack_limit);\n+\tgit_config_get_int(\"transfer.unpacklimit\", &transfer_unpack_limit);\n+\tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n+\tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n+\tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n \n-\treturn git_default_config(var, value, cb);\n+\tgit_config(git_default_config, NULL);\n }\n \n static void fetch_pack_setup(void)\n@@ -904,7 +885,7 @@ static void fetch_pack_setup(void)\n \tstatic int did_setup;\n \tif (did_setup)\n \t\treturn;\n-\tgit_config(fetch_pack_config, NULL);\n+\tfetch_pack_config();\n \tif (0 <= transfer_unpack_limit)\n \t\tunpack_limit = transfer_unpack_limit;\n \telse if (0 <= fetch_unpack_limit)\n-- \n1.9.0.GIT\n"},{"id":"247226","messageId":"1407177229-30081-7-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 06/11] rerere.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:44Z","receivedAt":"2014-08-04T18:33:44Z","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---\n rerere.c | 43 ++++++++++++-------------------------------\n 1 file changed, 12 insertions(+), 31 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex d84b495..20b18ad 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -573,15 +573,11 @@ static int do_plain_rerere(struct string_list *rr, int fd)\n \treturn write_rr(rr, fd);\n }\n \n-static int git_rerere_config(const char *var, const char *value, void *cb)\n+static void git_rerere_config(void)\n {\n-\tif (!strcmp(var, \"rerere.enabled\"))\n-\t\trerere_enabled = git_config_bool(var, value);\n-\telse if (!strcmp(var, \"rerere.autoupdate\"))\n-\t\trerere_autoupdate = git_config_bool(var, value);\n-\telse\n-\t\treturn git_default_config(var, value, cb);\n-\treturn 0;\n+\tgit_config_get_bool(\"rerere.enabled\", &rerere_enabled);\n+\tgit_config_get_bool(\"rerere.autoupdate\", &rerere_autoupdate);\n+\tgit_config(git_default_config, NULL);\n }\n \n static int is_rerere_enabled(void)\n@@ -606,7 +602,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)\n {\n \tint fd;\n \n-\tgit_config(git_rerere_config, NULL);\n+\tgit_rerere_config();\n \tif (!is_rerere_enabled())\n \t\treturn -1;\n \n@@ -699,24 +695,6 @@ static void unlink_rr_item(const char *name)\n \trmdir(git_path(\"rr-cache/%s\", name));\n }\n \n-struct rerere_gc_config_cb {\n-\tint cutoff_noresolve;\n-\tint cutoff_resolve;\n-};\n-\n-static int git_rerere_gc_config(const char *var, const char *value, void *cb)\n-{\n-\tstruct rerere_gc_config_cb *cf = cb;\n-\n-\tif (!strcmp(var, \"gc.rerereresolved\"))\n-\t\tcf->cutoff_resolve = git_config_int(var, value);\n-\telse if (!strcmp(var, \"gc.rerereunresolved\"))\n-\t\tcf->cutoff_noresolve = git_config_int(var, value);\n-\telse\n-\t\treturn git_default_config(var, value, cb);\n-\treturn 0;\n-}\n-\n void rerere_gc(struct string_list *rr)\n {\n \tstruct string_list to_remove = STRING_LIST_INIT_DUP;\n@@ -724,9 +702,12 @@ void rerere_gc(struct string_list *rr)\n \tstruct dirent *e;\n \tint i, cutoff;\n \ttime_t now = time(NULL), then;\n-\tstruct rerere_gc_config_cb cf = { 15, 60 };\n+\tint cutoff_noresolve = 15;\n+\tint cutoff_resolve = 60;\n \n-\tgit_config(git_rerere_gc_config, &cf);\n+\tgit_config_get_int(\"gc.rerereresolved\", &cutoff_resolve);\n+\tgit_config_get_int(\"gc.rerereunresolved\", &cutoff_noresolve);\n+\tgit_config(git_default_config, NULL);\n \tdir = opendir(git_path(\"rr-cache\"));\n \tif (!dir)\n \t\tdie_errno(\"unable to open rr-cache directory\");\n@@ -736,12 +717,12 @@ void rerere_gc(struct string_list *rr)\n \n \t\tthen = rerere_last_used_at(e->d_name);\n \t\tif (then) {\n-\t\t\tcutoff = cf.cutoff_resolve;\n+\t\t\tcutoff = cutoff_resolve;\n \t\t} else {\n \t\t\tthen = rerere_created_at(e->d_name);\n \t\t\tif (!then)\n \t\t\t\tcontinue;\n-\t\t\tcutoff = cf.cutoff_noresolve;\n+\t\t\tcutoff = cutoff_noresolve;\n \t\t}\n \t\tif (then < now - cutoff * 86400)\n \t\t\tstring_list_append(&to_remove, e->d_name);\n-- \n1.9.0.GIT\n"},{"id":"247229","messageId":"1407177229-30081-8-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:45Z","receivedAt":"2014-08-04T18:33:45Z","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---\n builtin/gc.c | 51 ++++++++++++++++++++-------------------------------\n 1 file changed, 20 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 8d219d8..4612ef5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -55,44 +55,33 @@ static void remove_pidfile_on_signal(int signo)\n \traise(signo);\n }\n \n-static int gc_config(const char *var, const char *value, void *cb)\n+static void gc_config(void)\n {\n-\tif (!strcmp(var, \"gc.packrefs\")) {\n+\tconst char *value;\n+\n+\tif (!git_config_get_value(\"gc.packrefs\", &value)) {\n \t\tif (value && !strcmp(value, \"notbare\"))\n \t\t\tpack_refs = -1;\n \t\telse\n-\t\t\tpack_refs = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"gc.aggressivewindow\")) {\n-\t\taggressive_window = git_config_int(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"gc.aggressivedepth\")) {\n-\t\taggressive_depth = git_config_int(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"gc.auto\")) {\n-\t\tgc_auto_threshold = git_config_int(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"gc.autopacklimit\")) {\n-\t\tgc_auto_pack_limit = git_config_int(var, value);\n-\t\treturn 0;\n+\t\t\tpack_refs = git_config_bool(\"gc.packrefs\", value);\n \t}\n-\tif (!strcmp(var, \"gc.autodetach\")) {\n-\t\tdetach_auto = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"gc.pruneexpire\")) {\n-\t\tif (value && strcmp(value, \"now\")) {\n+\n+\tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n+\tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n+\tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\n+\tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n+\tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n+\n+\tif (!git_config_get_string_const(\"gc.pruneexpire\", &prune_expire)) {\n+\t\tif (strcmp(prune_expire, \"now\")) {\n \t\t\tunsigned long now = approxidate(\"now\");\n-\t\t\tif (approxidate(value) >= now)\n-\t\t\t\treturn error(_(\"Invalid %s: '%s'\"), var, value);\n+\t\t\tif (approxidate(prune_expire) >= now) {\n+\t\t\t\terror(_(\"Invalid %s: '%s'\"), \"gc.pruneexpire\", prune_expire);\n+\t\t\t\tgit_die_config(\"gc.pruneexpire\");\n+\t\t\t}\n \t\t}\n-\t\treturn git_config_string(&prune_expire, var, value);\n \t}\n-\treturn git_default_config(var, value, cb);\n+\tgit_config(git_default_config, NULL);\n }\n \n static int too_many_loose_objects(void)\n@@ -301,7 +290,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \targv_array_pushl(&prune, \"prune\", \"--expire\", NULL );\n \targv_array_pushl(&rerere, \"rerere\", \"gc\", NULL);\n \n-\tgit_config(gc_config, NULL);\n+\tgc_config();\n \n \tif (pack_refs < 0)\n \t\tpack_refs = !is_bare_repository();\n-- \n1.9.0.GIT\n"},{"id":"247231","messageId":"1407177229-30081-9-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 08/11] pager.c: replace `git_config()` with `git_config_get_value()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:46Z","receivedAt":"2014-08-04T18:33:46Z","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":"247230","messageId":"1407177229-30081-10-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 09/11] imap-send.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:47Z","receivedAt":"2014-08-04T18:33:47Z","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---\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":"247232","messageId":"1407177229-30081-11-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 10/11] alias.c: replace `git_config()` with `git_config_get_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:48Z","receivedAt":"2014-08-04T18:33:48Z","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 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":"247233","messageId":"1407177229-30081-12-git-send-email-tanayabh@gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 11/11] branch.c: replace `git_config()` with `git_config_get_string()","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:33:49Z","receivedAt":"2014-08-04T18:33:49Z","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 | 27 +++++++--------------------\n 1 file changed, 7 insertions(+), 20 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 735767d..df6b120 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -140,30 +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-\tgit_config(read_branch_desc_cb, &cb);\n-\tif (cb.value)\n-\t\tstrbuf_addstr(buf, cb.value);\n+\tif (git_config_get_string(name.buf, &v)) {\n+\t\tstrbuf_release(&name);\n+\t\treturn -1;\n+\t}\n+\tstrbuf_addstr(buf, v);\n+\tfree(v);\n \tstrbuf_release(&name);\n \treturn 0;\n }\n-- \n1.9.0.GIT\n"},{"id":"247237","messageId":"CAPig+cRpf9fc4HhNWpJ9Su+hSVEDc2+m4QmhY38zLD3RO8pGnA@mail.gmail.com","threadId":"37287","inReplyTo":"1407177229-30081-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-08-04T18:59:10Z","receivedAt":"2014-08-04T18:59:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra wrote:\n> Use `git_config_get_bool()` 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> http-backend.c | 31 ++++++++++++-------------------\n> 1 file changed, 12 insertions(+), 19 deletions(-)\n>\n> diff --git a/http-backend.c b/http-backend.c\n> index 80790bb..106ca6b 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -219,29 +219,22 @@ static void get_idx_file(char *name)\n> send_local_file(\"application/x-git-packed-objects-toc\", name);\n> }\n>\n> -static int http_config(const char *var, const char *value, void *cb)\n> +static void http_config(void)\n> {\n> - const char *p;\n> + int i, value = 0;\n> + struct strbuf var = STRBUF_INIT;\n>\n> - if (!strcmp(var, \"http.getanyfile\")) {\n> - getanyfile = git_config_bool(var, value);\n> - return 0;\n> - }\n> + git_config_get_bool(\"http.getanyfile\", &getanyfile);\n>\n> - if (skip_prefix(var, \"http.\", &p)) {\n> - int i;\n> -\n> - for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n> - struct rpc_service *svc = &rpc_service[i];\n> - if (!strcmp(p, svc->config_name)) {\n> - svc->enabled = git_config_bool(var, value);\n> - return 0;\n> - }\n> - }\n> + for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n> + struct rpc_service *svc = &rpc_service[i];\n> + strbuf_addf(&var, \"http.%s\", svc->config_name);\n> + if (!git_config_get_bool(var.buf, &value))\n> + svc->enabled = value;\n> + strbuf_reset(&var);\n> }\n\nThere is a behavior change here. The original code set svc->enabled to\nthe first matching rpc_service[] entry, whereas the new code sets it\nto the last matching entry. Is this change intentional?\n\n> - /* we are not interested in parsing any other configuration here */\n> - return 0;\n> + strbuf_release(&var);\n> }\n>\n> static struct rpc_service *select_service(const char *name)\n> @@ -627,7 +620,7 @@ int main(int argc, char **argv)\n> access(\"git-daemon-export-ok\", F_OK) )\n> not_found(\"Repository not exported: '%s'\", dir);\n>\n> - git_config(http_config, NULL);\n> + http_config();\n> cmd->imp(cmd_arg);\n> return 0;\n> }\n> --\n> 1.9.0.GIT\n"},{"id":"247245","messageId":"vpq38dcatrq.fsf@anie.imag.fr","threadId":"37287","inReplyTo":"1407177229-30081-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T20:23:37Z","receivedAt":"2014-08-04T20:23: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> @@ -354,12 +338,11 @@ static int run_service(const char *dir, struct daemon_service *service)\n[...]\n> \t}\n> +\n> \tif (!enabled) {\n> \t\tlogerror(\"'%s': service not enabled for '%s'\",\n> \t\t\t service->name, path);\n\nAvoid whitespace-only change like this one.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247248","messageId":"vpqsilc9elm.fsf@anie.imag.fr","threadId":"37287","inReplyTo":"1407177229-30081-9-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 08/11] pager.c: replace `git_config()` with `git_config_get_value()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T20:36:37Z","receivedAt":"2014-08-04T20:36: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> Use `git_config_get_value()` 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> pager.c | 40 +++++++++++++---------------------------\n\nI find the patch more readable with --histogram, cut-and-pasted below in\ncase other reviewers find it better too.\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-{\n-\tstruct pager_config *c = data;\n-\tif (starts_with(var, \"pager.\") && !strcmp(var + 6, c->cmd)) {\n-\t\tint b = git_config_maybe_bool(var, value);\n-\t\tif (b >= 0)\n-\t\t\tc->want = b;\n-\t\telse {\n-\t\t\tc->want = 1;\n-\t\t\tc->value = xstrdup(value);\n-\t\t}\n-\t}\n-\treturn 0;\n-}\n-\n /* returns 0 for \"no pager\", 1 for \"use pager\", and -1 for \"not specified\" */\n int check_pager_config(const char *cmd)\n {\n-\tstruct pager_config c;\n-\tc.cmd = cmd;\n-\tc.want = -1;\n-\tc.value = NULL;\n-\tgit_config(pager_command_config, &c);\n-\tif (c.value)\n-\t\tpager_program = c.value;\n-\treturn c.want;\n+\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\twant = b;\n+\t\telse {\n+\t\t\twant = 1;\n+\t\t\tpager_program = xstrdup(value);\n+\t\t}\n+\t}\n+\tstrbuf_release(&key);\n+\treturn want;\n }\n\n\nI first wondered why you were not using git_config_get_maybe_bool(), but\nyou want to interpret non-boolean values as command-names, so you do\nneed this two-steps get_value -> maybe_bool.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247249","messageId":"vpq61i89eda.fsf@anie.imag.fr","threadId":"37287","inReplyTo":"1407177229-30081-8-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T20:41:37Z","receivedAt":"2014-08-04T20:41: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> +\t\t\tif (approxidate(prune_expire) >= now) {\n> +\t\t\t\terror(_(\"Invalid %s: '%s'\"), \"gc.pruneexpire\", prune_expire);\n> +\t\t\t\tgit_die_config(\"gc.pruneexpire\");\n> +\t\t\t}\n\nThat is a case where the API looks suboptimal, see Junio's remark in the\nother thread.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247250","messageId":"vpq38dc9ebv.fsf@anie.imag.fr","threadId":"37287","inReplyTo":"1407177229-30081-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 00/11] git_config callers rewritten with the new config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T20:42:28Z","receivedAt":"2014-08-04T20:42:28Z","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> This series is the first batch of patches which rewrites the existing callers\n> using a non-callback approach.\n> This series aims to,\n>\n> * rewrite the existing callers, as you can see from the diff stat the bew API\n> provides a much concise and clear control flow.\n>\n> * stress test the new API, see if any corner cases or deficiencies arise or not.\n\nI went through the series. I agree with Eric that there's a slight\nbehavior change in http-backend.c, and I don't think it's a good thing.\n\nOther than that, the series look good to me, although I would probably\nneed a second look to double check.\n\nTanay: I suggest you keep this as one 11-patches series. If you rewrite\nmore callsites, I'd find it easier to review as a second, independant\nseries rather than an ever-growing single series.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247328","messageId":"53E24750.4000602@gmail.com","threadId":"37287","inReplyTo":"CAPig+cRpf9fc4HhNWpJ9Su+hSVEDc2+m4QmhY38zLD3RO8pGnA@mail.gmail.com","subject":"Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T15:18:40Z","receivedAt":"2014-08-06T15:18:40Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 8/5/2014 12:29 AM, Eric Sunshine wrote:\n> On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra wrote:\n>> Use `git_config_get_bool()` 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>> http-backend.c | 31 ++++++++++++-------------------\n>> 1 file changed, 12 insertions(+), 19 deletions(-)\n>>\n>> diff --git a/http-backend.c b/http-backend.c\n>> index 80790bb..106ca6b 100644\n>> --- a/http-backend.c\n>> +++ b/http-backend.c\n>> @@ -219,29 +219,22 @@ static void get_idx_file(char *name)\n>> send_local_file(\"application/x-git-packed-objects-toc\", name);\n>> }\n>>\n>> -static int http_config(const char *var, const char *value, void *cb)\n>> +static void http_config(void)\n>> {\n>> - const char *p;\n>> + int i, value = 0;\n>> + struct strbuf var = STRBUF_INIT;\n>>\n>> - if (!strcmp(var, \"http.getanyfile\")) {\n>> - getanyfile = git_config_bool(var, value);\n>> - return 0;\n>> - }\n>> + git_config_get_bool(\"http.getanyfile\", &getanyfile);\n>>\n>> - if (skip_prefix(var, \"http.\", &p)) {\n>> - int i;\n>> -\n>> - for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n>> - struct rpc_service *svc = &rpc_service[i];\n>> - if (!strcmp(p, svc->config_name)) {\n>> - svc->enabled = git_config_bool(var, value);\n>> - return 0;\n>> - }\n>> - }\n>> + for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n>> + struct rpc_service *svc = &rpc_service[i];\n>> + strbuf_addf(&var, \"http.%s\", svc->config_name);\n>> + if (!git_config_get_bool(var.buf, &value))\n>> + svc->enabled = value;\n>> + strbuf_reset(&var);\n>> }\n> \n> There is a behavior change here. The original code set svc->enabled to\n> the first matching rpc_service[] entry, whereas the new code sets it\n> to the last matching entry. Is this change intentional?\n>\n\nI was preparing the reroll and I saw that I had missed your mail.\nI think that I haven't changed the behaviour, the original one is\nwritten in callback form so it has to go through the array every time for each\nnew value.\nWhen there are multiple entries for a service say,\n\nhttp.receivepack = 1\nhttp.receivepack = 0\n\nthe old code would have at overwritten the previous entry with the new value.\nThe new code just populates the whole array of `rpc_service` in one go, choosing\nthe last matching value for each entry. For reviewing purpose the original array is\nthis,\n\n\tstruct rpc_service {\n\tconst char *name;\n\tconst char *config_name;\n\tsigned enabled : 2;\n\t};\n\n\tstatic struct rpc_service rpc_service[] = {\n\t\t{ \"upload-pack\", \"uploadpack\", 1 },\n\t\t{ \"receive-pack\", \"receivepack\", -1 },\n\t};\n\nWhat do you think, am I interpreting it wrong? Thanks.\n\n>> - /* we are not interested in parsing any other configuration here */\n>> - return 0;\n>> + strbuf_release(&var);\n>> }\n>>\n>> static struct rpc_service *select_service(const char *name)\n>> @@ -627,7 +620,7 @@ int main(int argc, char **argv)\n>> access(\"git-daemon-export-ok\", F_OK) )\n>> not_found(\"Repository not exported: '%s'\", dir);\n>>\n>> - git_config(http_config, NULL);\n>> + http_config();\n>> cmd->imp(cmd_arg);\n>> return 0;\n>> }\n>> --\n>> 1.9.0.GIT\n"},{"id":"247334","messageId":"vpqfvh9tygl.fsf@anie.imag.fr","threadId":"37287","inReplyTo":"53E24750.4000602@gmail.com","subject":"Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-06T15:44:10Z","receivedAt":"2014-08-06T15:44:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> On 8/5/2014 12:29 AM, Eric Sunshine wrote:\n>> On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra wrote:\n>>> - if (skip_prefix(var, \"http.\", &p)) {\n>>> - int i;\n>>> -\n>>> - for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n>>> - struct rpc_service *svc = &rpc_service[i];\n>>> - if (!strcmp(p, svc->config_name)) {\n>>> - svc->enabled = git_config_bool(var, value);\n>>> - return 0;\n>>> - }\n>>> - }\n>>> + for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n>>> + struct rpc_service *svc = &rpc_service[i];\n>>> + strbuf_addf(&var, \"http.%s\", svc->config_name);\n>>> + if (!git_config_get_bool(var.buf, &value))\n>>> + svc->enabled = value;\n>>> + strbuf_reset(&var);\n>>> }\n>> \n>> There is a behavior change here. The original code set svc->enabled to\n>> the first matching rpc_service[] entry, whereas the new code sets it\n>> to the last matching entry. Is this change intentional?\n>>\n>\n> I was preparing the reroll and I saw that I had missed your mail.\n> I think that I haven't changed the behaviour, the original one is\n> written in callback form so it has to go through the array every time for each\n> new value.\n> When there are multiple entries for a service say,\n>\n> http.receivepack = 1\n> http.receivepack = 0\n\nI first got convinced by Eric, but I now think you're right.\n\nEric's point was (I think) not about multiple entries for the same\nvariable, but about multiple entries for different services, like\n\nhttp.receivepack = 1\nhttp.uploadpack = 0\n\nThe order of assignments to svn->enabled change, but it doesn't matter\nbecause svc is just a local variable pointing to the right element of\nrpc_service[i]. So in both cases, you'll assign\n\nrpc_service[].enabled = >\n\neven though the order of assignments will change.\n\nEric, am I interpreting right?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247359","messageId":"CAPig+cT1jaRjBEU6iS2i7-Wk4ocZc=HJ5EWZdVrN01virO4mLA@mail.gmail.com","threadId":"37287","inReplyTo":"vpqfvh9tygl.fsf@anie.imag.fr","subject":"Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-08-06T20:04:14Z","receivedAt":"2014-08-06T20:04:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 6, 2014 at 11:44 AM, Matthieu Moy\n wrote:\n> Tanay Abhra writes:\n>\n>> On 8/5/2014 12:29 AM, Eric Sunshine wrote:\n>>> On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra wrote:\n>>>> - if (skip_prefix(var, \"http.\", &p)) {\n>>>> - int i;\n>>>> -\n>>>> - for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n>>>> - struct rpc_service *svc = &rpc_service[i];\n>>>> - if (!strcmp(p, svc->config_name)) {\n>>>> - svc->enabled = git_config_bool(var, value);\n>>>> - return 0;\n>>>> - }\n>>>> - }\n>>>> + for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n>>>> + struct rpc_service *svc = &rpc_service[i];\n>>>> + strbuf_addf(&var, \"http.%s\", svc->config_name);\n>>>> + if (!git_config_get_bool(var.buf, &value))\n>>>> + svc->enabled = value;\n>>>> + strbuf_reset(&var);\n>>>> }\n>>>\n>>> There is a behavior change here. The original code set svc->enabled to\n>>> the first matching rpc_service[] entry, whereas the new code sets it\n>>> to the last matching entry. Is this change intentional?\n>>>\n>>\n>> I was preparing the reroll and I saw that I had missed your mail.\n>> I think that I haven't changed the behaviour, the original one is\n>> written in callback form so it has to go through the array every time for each\n>> new value.\n>> When there are multiple entries for a service say,\n>>\n>> http.receivepack = 1\n>> http.receivepack = 0\n>\n> I first got convinced by Eric, but I now think you're right.\n>\n> Eric's point was (I think) not about multiple entries for the same\n> variable, but about multiple entries for different services, like\n>\n> http.receivepack = 1\n> http.uploadpack = 0\n>\n> The order of assignments to svn->enabled change, but it doesn't matter\n> because svc is just a local variable pointing to the right element of\n> rpc_service[i]. So in both cases, you'll assign\n>\n> rpc_service[].enabled = >\n>\n> even though the order of assignments will change.\n>\n> Eric, am I interpreting right?\n\nYes. During initial review, I either didn't read the old code closely\nenough or I misinterpreted it. Upon re-read, Tanay's rewrite looks\nfine.\n"}]}