[PATCH 00/11] git_config callers rewritten with the new config-set API

[PATCH 00/11] git_config callers rewritten with the new config-set API

[Tanay Abhra]

The ta/config-set API is more or less solidified.

This series builds on the top of 4c715ebb in pu (ta/config-set). On top of it,
it also requires series [1] (Rewrite `git_config()` using config-set API) for
proper error checking.

This series is the first batch of patches which rewrites the existing callers
using a non-callback approach.
This series aims to,

* rewrite the existing callers, as you can see from the diff stat the bew API
  provides a much concise and clear control flow.

* stress test the new API, see if any corner cases or deficiencies arise or not.

The series passes all the tests, only thing to watch is that the config variables
that have been rewritten are single valued only. Though I have tried my best to
ascertain it, still mistakes may arise.

p.s: I haven't decided yet about whether to introduce a new helper set, for example
     git_config_get_value_fmt() which would behave like strbuf_addf(). It will probably
     come in a later series.

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

Tanay Abhra (11):

 alias.c        | 25 ++++++------------------
 archive.c      | 12 +++---------
 branch.c       | 27 +++++++-------------------
 builtin/gc.c   | 51 +++++++++++++++++++-----------------------------
 daemon.c       | 27 +++++---------------------
 fetch-pack.c   | 35 ++++++++-------------------------
 http-backend.c | 31 ++++++++++++-----------------
 imap-send.c    | 61 ++++++++++++++++++++++++++--------------------------------
 pager.c        | 40 +++++++++++++-------------------------
 read-cache.c   | 14 +++-----------
 rerere.c       | 43 ++++++++++++-----------------------------
 11 files changed, 116 insertions(+), 250 deletions(-)

-- 
1.9.0.GIT

[PATCH 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family

[Tanay Abhra]

Use `git_config_get_bool()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 daemon.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/daemon.c b/daemon.c
index e6b51ed..fb16664 100644
--- a/daemon.c
+++ b/daemon.c
@@ -230,23 +230,6 @@ struct daemon_service {
 	int overridable;
 };
 
-static struct daemon_service *service_looking_at;
-static int service_enabled;
-
-static int git_daemon_config(const char *var, const char *value, void *cb)
-{
-	const char *service;
-
-	if (skip_prefix(var, "daemon.", &service) &&
-	    !strcmp(service, service_looking_at->config_name)) {
-		service_enabled = git_config_bool(var, value);
-		return 0;
-	}
-
-	/* we are not interested in parsing any other configuration here */
-	return 0;
-}
-
 static int daemon_error(const char *dir, const char *msg)
 {
 	if (!informative_errors)
@@ -324,6 +307,7 @@ static int run_service(const char *dir, struct daemon_service *service)
 {
 	const char *path;
 	int enabled = service->enabled;
+	struct strbuf var = STRBUF_INIT;
 
 	loginfo("Request %s for '%s'", service->name, dir);
 
@@ -354,12 +338,11 @@ static int run_service(const char *dir, struct daemon_service *service)
 	}
 
 	if (service->overridable) {
-		service_looking_at = service;
-		service_enabled = -1;
-		git_config(git_daemon_config, NULL);
-		if (0 <= service_enabled)
-			enabled = service_enabled;
+		strbuf_addf(&var, "daemon.%s", service->config_name);
+		git_config_get_bool(var.buf, &enabled);
+		strbuf_release(&var);
 	}
+
 	if (!enabled) {
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
-- 
1.9.0.GIT

Re: [PATCH 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> @@ -354,12 +338,11 @@ static int run_service(const char *dir, struct daemon_service *service)
[...]
>  	}
> +
>  	if (!enabled) {
>  		logerror("'%s': service not enabled for '%s'",
>  			 service->name, path);

Avoid whitespace-only change like this one.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family

[Tanay Abhra]

Use `git_config_get_bool()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 http-backend.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 80790bb..106ca6b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -219,29 +219,22 @@ static void get_idx_file(char *name)
 	send_local_file("application/x-git-packed-objects-toc", name);
 }
 
-static int http_config(const char *var, const char *value, void *cb)
+static void http_config(void)
 {
-	const char *p;
+	int i, value = 0;
+	struct strbuf var = STRBUF_INIT;
 
-	if (!strcmp(var, "http.getanyfile")) {
-		getanyfile = git_config_bool(var, value);
-		return 0;
-	}
+	git_config_get_bool("http.getanyfile", &getanyfile);
 
-	if (skip_prefix(var, "http.", &p)) {
-		int i;
-
-		for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
-			struct rpc_service *svc = &rpc_service[i];
-			if (!strcmp(p, svc->config_name)) {
-				svc->enabled = git_config_bool(var, value);
-				return 0;
-			}
-		}
+	for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
+		struct rpc_service *svc = &rpc_service[i];
+		strbuf_addf(&var, "http.%s", svc->config_name);
+		if (!git_config_get_bool(var.buf, &value))
+			svc->enabled = value;
+		strbuf_reset(&var);
 	}
 
-	/* we are not interested in parsing any other configuration here */
-	return 0;
+	strbuf_release(&var);
 }
 
 static struct rpc_service *select_service(const char *name)
@@ -627,7 +620,7 @@ int main(int argc, char **argv)
 	    access("git-daemon-export-ok", F_OK) )
 		not_found("Repository not exported: '%s'", dir);
 
-	git_config(http_config, NULL);
+	http_config();
 	cmd->imp(cmd_arg);
 	return 0;
 }
-- 
1.9.0.GIT

Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family

[Eric Sunshine]

On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Use `git_config_get_bool()` family instead of `git_config()` to take advantage of
> the config-set API which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  http-backend.c | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/http-backend.c b/http-backend.c
> index 80790bb..106ca6b 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -219,29 +219,22 @@ static void get_idx_file(char *name)
>         send_local_file("application/x-git-packed-objects-toc", name);
>  }
>
> -static int http_config(const char *var, const char *value, void *cb)
> +static void http_config(void)
>  {
> -       const char *p;
> +       int i, value = 0;
> +       struct strbuf var = STRBUF_INIT;
>
> -       if (!strcmp(var, "http.getanyfile")) {
> -               getanyfile = git_config_bool(var, value);
> -               return 0;
> -       }
> +       git_config_get_bool("http.getanyfile", &getanyfile);
>
> -       if (skip_prefix(var, "http.", &p)) {
> -               int i;
> -
> -               for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
> -                       struct rpc_service *svc = &rpc_service[i];
> -                       if (!strcmp(p, svc->config_name)) {
> -                               svc->enabled = git_config_bool(var, value);
> -                               return 0;
> -                       }
> -               }
> +       for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
> +               struct rpc_service *svc = &rpc_service[i];
> +               strbuf_addf(&var, "http.%s", svc->config_name);
> +               if (!git_config_get_bool(var.buf, &value))
> +                       svc->enabled = value;
> +               strbuf_reset(&var);
>         }

There is a behavior change here. The original code set svc->enabled to
the first matching rpc_service[] entry, whereas the new code sets it
to the last matching entry. Is this change intentional?

> -       /* we are not interested in parsing any other configuration here */
> -       return 0;
> +       strbuf_release(&var);
>  }
>
>  static struct rpc_service *select_service(const char *name)
> @@ -627,7 +620,7 @@ int main(int argc, char **argv)
>             access("git-daemon-export-ok", F_OK) )
>                 not_found("Repository not exported: '%s'", dir);
>
> -       git_config(http_config, NULL);
> +       http_config();
>         cmd->imp(cmd_arg);
>         return 0;
>  }
> --
> 1.9.0.GIT

Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family

[Tanay Abhra]

On 8/5/2014 12:29 AM, Eric Sunshine wrote:
> On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use `git_config_get_bool()` family instead of `git_config()` to take advantage of
>> the config-set API which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  http-backend.c | 31 ++++++++++++-------------------
>>  1 file changed, 12 insertions(+), 19 deletions(-)
>>
>> diff --git a/http-backend.c b/http-backend.c
>> index 80790bb..106ca6b 100644
>> --- a/http-backend.c
>> +++ b/http-backend.c
>> @@ -219,29 +219,22 @@ static void get_idx_file(char *name)
>>         send_local_file("application/x-git-packed-objects-toc", name);
>>  }
>>
>> -static int http_config(const char *var, const char *value, void *cb)
>> +static void http_config(void)
>>  {
>> -       const char *p;
>> +       int i, value = 0;
>> +       struct strbuf var = STRBUF_INIT;
>>
>> -       if (!strcmp(var, "http.getanyfile")) {
>> -               getanyfile = git_config_bool(var, value);
>> -               return 0;
>> -       }
>> +       git_config_get_bool("http.getanyfile", &getanyfile);
>>
>> -       if (skip_prefix(var, "http.", &p)) {
>> -               int i;
>> -
>> -               for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>> -                       struct rpc_service *svc = &rpc_service[i];
>> -                       if (!strcmp(p, svc->config_name)) {
>> -                               svc->enabled = git_config_bool(var, value);
>> -                               return 0;
>> -                       }
>> -               }
>> +       for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>> +               struct rpc_service *svc = &rpc_service[i];
>> +               strbuf_addf(&var, "http.%s", svc->config_name);
>> +               if (!git_config_get_bool(var.buf, &value))
>> +                       svc->enabled = value;
>> +               strbuf_reset(&var);
>>         }
> 
> There is a behavior change here. The original code set svc->enabled to
> the first matching rpc_service[] entry, whereas the new code sets it
> to the last matching entry. Is this change intentional?
>

I was preparing the reroll and I saw that I had missed your mail.
I think that I haven't changed the behaviour, the original one is
written in callback form so it has to go through the array every time for each
new value.
When there are multiple entries for a service say,

http.receivepack = 1
http.receivepack = 0

the old code would have at overwritten the previous entry with the new value.
The new code just populates the whole array of `rpc_service` in one go, choosing
the last matching value for each entry. For reviewing purpose the original array is
this,

	struct rpc_service {
	const char *name;
	const char *config_name;
	signed enabled : 2;
	};

	static struct rpc_service rpc_service[] = {
		{ "upload-pack", "uploadpack", 1 },
		{ "receive-pack", "receivepack", -1 },
	};

What do you think, am I interpreting it wrong? Thanks.

>> -       /* we are not interested in parsing any other configuration here */
>> -       return 0;
>> +       strbuf_release(&var);
>>  }
>>
>>  static struct rpc_service *select_service(const char *name)
>> @@ -627,7 +620,7 @@ int main(int argc, char **argv)
>>             access("git-daemon-export-ok", F_OK) )
>>                 not_found("Repository not exported: '%s'", dir);
>>
>> -       git_config(http_config, NULL);
>> +       http_config();
>>         cmd->imp(cmd_arg);
>>         return 0;
>>  }
>> --
>> 1.9.0.GIT

Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 8/5/2014 12:29 AM, Eric Sunshine wrote:
>> On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>> -       if (skip_prefix(var, "http.", &p)) {
>>> -               int i;
>>> -
>>> -               for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>>> -                       struct rpc_service *svc = &rpc_service[i];
>>> -                       if (!strcmp(p, svc->config_name)) {
>>> -                               svc->enabled = git_config_bool(var, value);
>>> -                               return 0;
>>> -                       }
>>> -               }
>>> +       for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>>> +               struct rpc_service *svc = &rpc_service[i];
>>> +               strbuf_addf(&var, "http.%s", svc->config_name);
>>> +               if (!git_config_get_bool(var.buf, &value))
>>> +                       svc->enabled = value;
>>> +               strbuf_reset(&var);
>>>         }
>> 
>> There is a behavior change here. The original code set svc->enabled to
>> the first matching rpc_service[] entry, whereas the new code sets it
>> to the last matching entry. Is this change intentional?
>>
>
> I was preparing the reroll and I saw that I had missed your mail.
> I think that I haven't changed the behaviour, the original one is
> written in callback form so it has to go through the array every time for each
> new value.
> When there are multiple entries for a service say,
>
> http.receivepack = 1
> http.receivepack = 0

I first got convinced by Eric, but I now think you're right.

Eric's point was (I think) not about multiple entries for the same
variable, but about multiple entries for different services, like

http.receivepack = 1
http.uploadpack = 0

The order of assignments to svn->enabled change, but it doesn't matter
because svc is just a local variable pointing to the right element of
rpc_service[i]. So in both cases, you'll assign

rpc_service[<index of service>].enabled = <last occurence of variable http.<service> >

even though the order of assignments will change.

Eric, am I interpreting right?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family

[Eric Sunshine]

On Wed, Aug 6, 2014 at 11:44 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> On 8/5/2014 12:29 AM, Eric Sunshine wrote:
>>> On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>>> -       if (skip_prefix(var, "http.", &p)) {
>>>> -               int i;
>>>> -
>>>> -               for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>>>> -                       struct rpc_service *svc = &rpc_service[i];
>>>> -                       if (!strcmp(p, svc->config_name)) {
>>>> -                               svc->enabled = git_config_bool(var, value);
>>>> -                               return 0;
>>>> -                       }
>>>> -               }
>>>> +       for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>>>> +               struct rpc_service *svc = &rpc_service[i];
>>>> +               strbuf_addf(&var, "http.%s", svc->config_name);
>>>> +               if (!git_config_get_bool(var.buf, &value))
>>>> +                       svc->enabled = value;
>>>> +               strbuf_reset(&var);
>>>>         }
>>>
>>> There is a behavior change here. The original code set svc->enabled to
>>> the first matching rpc_service[] entry, whereas the new code sets it
>>> to the last matching entry. Is this change intentional?
>>>
>>
>> I was preparing the reroll and I saw that I had missed your mail.
>> I think that I haven't changed the behaviour, the original one is
>> written in callback form so it has to go through the array every time for each
>> new value.
>> When there are multiple entries for a service say,
>>
>> http.receivepack = 1
>> http.receivepack = 0
>
> I first got convinced by Eric, but I now think you're right.
>
> Eric's point was (I think) not about multiple entries for the same
> variable, but about multiple entries for different services, like
>
> http.receivepack = 1
> http.uploadpack = 0
>
> The order of assignments to svn->enabled change, but it doesn't matter
> because svc is just a local variable pointing to the right element of
> rpc_service[i]. So in both cases, you'll assign
>
> rpc_service[<index of service>].enabled = <last occurence of variable http.<service> >
>
> even though the order of assignments will change.
>
> Eric, am I interpreting right?

Yes. During initial review, I either didn't read the old code closely
enough or I misinterpreted it. Upon re-read, Tanay's rewrite looks
fine.

[PATCH 03/11] read-cache.c: replace `git_config()` with `git_config_get_*()` family

[Tanay Abhra]

Use `git_config_get_*()` family instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow.

Use an intermediate value, as `version` can not be used directly in
git_config_get_int() due to incompatible type.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 read-cache.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 5d3c8bd..acb132d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1238,24 +1238,16 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
 
 #define INDEX_FORMAT_DEFAULT 3
 
-static int index_format_config(const char *var, const char *value, void *cb)
-{
-	unsigned int *version = cb;
-	if (!strcmp(var, "index.version")) {
-		*version = git_config_int(var, value);
-		return 0;
-	}
-	return 1;
-}
-
 static unsigned int get_index_format_default(void)
 {
 	char *envversion = getenv("GIT_INDEX_VERSION");
 	char *endp;
+	int value;
 	unsigned int version = INDEX_FORMAT_DEFAULT;
 
 	if (!envversion) {
-		git_config(index_format_config, &version);
+		if (!git_config_get_int("index.version", &value))
+			version = value;
 		if (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
 			warning(_("index.version set, but the value is invalid.\n"
 				  "Using version %i"), INDEX_FORMAT_DEFAULT);
-- 
1.9.0.GIT

[PATCH 04/11] archive.c: replace `git_config()` with `git_config_get_bool()` family

[Tanay Abhra]

Use `git_config_get_bool()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 archive.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/archive.c b/archive.c
index 3fc0fb2..952a659 100644
--- a/archive.c
+++ b/archive.c
@@ -402,14 +402,6 @@ static int parse_archive_args(int argc, const char **argv,
 	return argc;
 }
 
-static int git_default_archive_config(const char *var, const char *value,
-				      void *cb)
-{
-	if (!strcmp(var, "uploadarchive.allowunreachable"))
-		remote_allow_unreachable = git_config_bool(var, value);
-	return git_default_config(var, value, cb);
-}
-
 int write_archive(int argc, const char **argv, const char *prefix,
 		  int setup_prefix, const char *name_hint, int remote)
 {
@@ -420,7 +412,9 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	if (setup_prefix && prefix == NULL)
 		prefix = setup_git_directory_gently(&nongit);
 
-	git_config(git_default_archive_config, NULL);
+	git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
+	git_config(git_default_config, NULL);
+
 	init_tar_archiver();
 	init_zip_archiver();
 
-- 
1.9.0.GIT

[PATCH 05/11] fetchpack.c: replace `git_config()` with `git_config_get_*()` family

[Tanay Abhra]

Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 fetch-pack.c | 35 ++++++++---------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index b8a58fa..a13e9db 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -869,34 +869,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	return ref;
 }
 
-static int fetch_pack_config(const char *var, const char *value, void *cb)
+static void fetch_pack_config(void)
 {
-	if (strcmp(var, "fetch.unpacklimit") == 0) {
-		fetch_unpack_limit = git_config_int(var, value);
-		return 0;
-	}
-
-	if (strcmp(var, "transfer.unpacklimit") == 0) {
-		transfer_unpack_limit = git_config_int(var, value);
-		return 0;
-	}
-
-	if (strcmp(var, "repack.usedeltabaseoffset") == 0) {
-		prefer_ofs_delta = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "fetch.fsckobjects")) {
-		fetch_fsck_objects = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "transfer.fsckobjects")) {
-		transfer_fsck_objects = git_config_bool(var, value);
-		return 0;
-	}
+	git_config_get_int("fetch.unpacklimit", &fetch_unpack_limit);
+	git_config_get_int("transfer.unpacklimit", &transfer_unpack_limit);
+	git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta);
+	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
+	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
 
-	return git_default_config(var, value, cb);
+	git_config(git_default_config, NULL);
 }
 
 static void fetch_pack_setup(void)
@@ -904,7 +885,7 @@ static void fetch_pack_setup(void)
 	static int did_setup;
 	if (did_setup)
 		return;
-	git_config(fetch_pack_config, NULL);
+	fetch_pack_config();
 	if (0 <= transfer_unpack_limit)
 		unpack_limit = transfer_unpack_limit;
 	else if (0 <= fetch_unpack_limit)
-- 
1.9.0.GIT

[PATCH 06/11] rerere.c: replace `git_config()` with `git_config_get_*()` family

[Tanay Abhra]

Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 rerere.c | 43 ++++++++++++-------------------------------
 1 file changed, 12 insertions(+), 31 deletions(-)

diff --git a/rerere.c b/rerere.c
index d84b495..20b18ad 100644
--- a/rerere.c
+++ b/rerere.c
@@ -573,15 +573,11 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 	return write_rr(rr, fd);
 }
 
-static int git_rerere_config(const char *var, const char *value, void *cb)
+static void git_rerere_config(void)
 {
-	if (!strcmp(var, "rerere.enabled"))
-		rerere_enabled = git_config_bool(var, value);
-	else if (!strcmp(var, "rerere.autoupdate"))
-		rerere_autoupdate = git_config_bool(var, value);
-	else
-		return git_default_config(var, value, cb);
-	return 0;
+	git_config_get_bool("rerere.enabled", &rerere_enabled);
+	git_config_get_bool("rerere.autoupdate", &rerere_autoupdate);
+	git_config(git_default_config, NULL);
 }
 
 static int is_rerere_enabled(void)
@@ -606,7 +602,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)
 {
 	int fd;
 
-	git_config(git_rerere_config, NULL);
+	git_rerere_config();
 	if (!is_rerere_enabled())
 		return -1;
 
@@ -699,24 +695,6 @@ static void unlink_rr_item(const char *name)
 	rmdir(git_path("rr-cache/%s", name));
 }
 
-struct rerere_gc_config_cb {
-	int cutoff_noresolve;
-	int cutoff_resolve;
-};
-
-static int git_rerere_gc_config(const char *var, const char *value, void *cb)
-{
-	struct rerere_gc_config_cb *cf = cb;
-
-	if (!strcmp(var, "gc.rerereresolved"))
-		cf->cutoff_resolve = git_config_int(var, value);
-	else if (!strcmp(var, "gc.rerereunresolved"))
-		cf->cutoff_noresolve = git_config_int(var, value);
-	else
-		return git_default_config(var, value, cb);
-	return 0;
-}
-
 void rerere_gc(struct string_list *rr)
 {
 	struct string_list to_remove = STRING_LIST_INIT_DUP;
@@ -724,9 +702,12 @@ void rerere_gc(struct string_list *rr)
 	struct dirent *e;
 	int i, cutoff;
 	time_t now = time(NULL), then;
-	struct rerere_gc_config_cb cf = { 15, 60 };
+	int cutoff_noresolve = 15;
+	int cutoff_resolve = 60;
 
-	git_config(git_rerere_gc_config, &cf);
+	git_config_get_int("gc.rerereresolved", &cutoff_resolve);
+	git_config_get_int("gc.rerereunresolved", &cutoff_noresolve);
+	git_config(git_default_config, NULL);
 	dir = opendir(git_path("rr-cache"));
 	if (!dir)
 		die_errno("unable to open rr-cache directory");
@@ -736,12 +717,12 @@ void rerere_gc(struct string_list *rr)
 
 		then = rerere_last_used_at(e->d_name);
 		if (then) {
-			cutoff = cf.cutoff_resolve;
+			cutoff = cutoff_resolve;
 		} else {
 			then = rerere_created_at(e->d_name);
 			if (!then)
 				continue;
-			cutoff = cf.cutoff_noresolve;
+			cutoff = cutoff_noresolve;
 		}
 		if (then < now - cutoff * 86400)
 			string_list_append(&to_remove, e->d_name);
-- 
1.9.0.GIT

[PATCH 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family

[Tanay Abhra]

Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 builtin/gc.c | 51 ++++++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 8d219d8..4612ef5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -55,44 +55,33 @@ static void remove_pidfile_on_signal(int signo)
 	raise(signo);
 }
 
-static int gc_config(const char *var, const char *value, void *cb)
+static void gc_config(void)
 {
-	if (!strcmp(var, "gc.packrefs")) {
+	const char *value;
+
+	if (!git_config_get_value("gc.packrefs", &value)) {
 		if (value && !strcmp(value, "notbare"))
 			pack_refs = -1;
 		else
-			pack_refs = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "gc.aggressivewindow")) {
-		aggressive_window = git_config_int(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "gc.aggressivedepth")) {
-		aggressive_depth = git_config_int(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "gc.auto")) {
-		gc_auto_threshold = git_config_int(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "gc.autopacklimit")) {
-		gc_auto_pack_limit = git_config_int(var, value);
-		return 0;
+			pack_refs = git_config_bool("gc.packrefs", value);
 	}
-	if (!strcmp(var, "gc.autodetach")) {
-		detach_auto = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "gc.pruneexpire")) {
-		if (value && strcmp(value, "now")) {
+
+	git_config_get_int("gc.aggressivewindow", &aggressive_window);
+	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
+	git_config_get_int("gc.auto", &gc_auto_threshold);
+	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
+	git_config_get_bool("gc.autodetach", &detach_auto);
+
+	if (!git_config_get_string_const("gc.pruneexpire", &prune_expire)) {
+		if (strcmp(prune_expire, "now")) {
 			unsigned long now = approxidate("now");
-			if (approxidate(value) >= now)
-				return error(_("Invalid %s: '%s'"), var, value);
+			if (approxidate(prune_expire) >= now) {
+				error(_("Invalid %s: '%s'"), "gc.pruneexpire", prune_expire);
+				git_die_config("gc.pruneexpire");
+			}
 		}
-		return git_config_string(&prune_expire, var, value);
 	}
-	return git_default_config(var, value, cb);
+	git_config(git_default_config, NULL);
 }
 
 static int too_many_loose_objects(void)
@@ -301,7 +290,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	argv_array_pushl(&prune, "prune", "--expire", NULL );
 	argv_array_pushl(&rerere, "rerere", "gc", NULL);
 
-	git_config(gc_config, NULL);
+	gc_config();
 
 	if (pack_refs < 0)
 		pack_refs = !is_bare_repository();
-- 
1.9.0.GIT

Re: [PATCH 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +			if (approxidate(prune_expire) >= now) {
> +				error(_("Invalid %s: '%s'"), "gc.pruneexpire", prune_expire);
> +				git_die_config("gc.pruneexpire");
> +			}

That is a case where the API looks suboptimal, see Junio's remark in the
other thread.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 08/11] pager.c: replace `git_config()` with `git_config_get_value()`

[Tanay Abhra]

Use `git_config_get_value()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 pager.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
 #define DEFAULT_PAGER "less"
 #endif
 
-struct pager_config {
-	const char *cmd;
-	int want;
-	char *value;
-};
-
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
 	return width;
 }
 
-static int pager_command_config(const char *var, const char *value, void *data)
+/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
+int check_pager_config(const char *cmd)
 {
-	struct pager_config *c = data;
-	if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
-		int b = git_config_maybe_bool(var, value);
+	int want = -1;
+	struct strbuf key = STRBUF_INIT;
+	const char *value = NULL;
+	strbuf_addf(&key, "pager.%s", cmd);
+	if (!git_config_get_value(key.buf, &value)) {
+		int b = git_config_maybe_bool(key.buf, value);
 		if (b >= 0)
-			c->want = b;
+			want = b;
 		else {
-			c->want = 1;
-			c->value = xstrdup(value);
+			want = 1;
+			pager_program = xstrdup(value);
 		}
 	}
-	return 0;
-}
-
-/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
-int check_pager_config(const char *cmd)
-{
-	struct pager_config c;
-	c.cmd = cmd;
-	c.want = -1;
-	c.value = NULL;
-	git_config(pager_command_config, &c);
-	if (c.value)
-		pager_program = c.value;
-	return c.want;
+	strbuf_release(&key);
+	return want;
 }
-- 
1.9.0.GIT

Re: [PATCH 08/11] pager.c: replace `git_config()` with `git_config_get_value()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Use `git_config_get_value()` instead of `git_config()` to take advantage of
> the config-set API which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  pager.c | 40 +++++++++++++---------------------------

I find the patch more readable with --histogram, cut-and-pasted below in
case other reviewers find it better too.

diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
 #define DEFAULT_PAGER "less"
 #endif
 
-struct pager_config {
-	const char *cmd;
-	int want;
-	char *value;
-};
-
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
 	return width;
 }
 
-static int pager_command_config(const char *var, const char *value, void *data)
-{
-	struct pager_config *c = data;
-	if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
-		int b = git_config_maybe_bool(var, value);
-		if (b >= 0)
-			c->want = b;
-		else {
-			c->want = 1;
-			c->value = xstrdup(value);
-		}
-	}
-	return 0;
-}
-
 /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
 int check_pager_config(const char *cmd)
 {
-	struct pager_config c;
-	c.cmd = cmd;
-	c.want = -1;
-	c.value = NULL;
-	git_config(pager_command_config, &c);
-	if (c.value)
-		pager_program = c.value;
-	return c.want;
+	int want = -1;
+	struct strbuf key = STRBUF_INIT;
+	const char *value = NULL;
+	strbuf_addf(&key, "pager.%s", cmd);
+	if (!git_config_get_value(key.buf, &value)) {
+		int b = git_config_maybe_bool(key.buf, value);
+		if (b >= 0)
+			want = b;
+		else {
+			want = 1;
+			pager_program = xstrdup(value);
+		}
+	}
+	strbuf_release(&key);
+	return want;
 }


I first wondered why you were not using git_config_get_maybe_bool(), but
you want to interpret non-boolean values as command-names, so you do
need this two-steps get_value -> maybe_bool.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 09/11] imap-send.c: replace `git_config()` with `git_config_get_*()` family

[Tanay Abhra]

Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 imap-send.c | 61 +++++++++++++++++++++++++++----------------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 524fbab..586bdd8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,43 +1326,36 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
 
 static char *imap_folder;
 
-static int git_imap_config(const char *key, const char *val, void *cb)
+static void git_imap_config(void)
 {
-	if (!skip_prefix(key, "imap.", &key))
-		return 0;
+	const char *val = NULL;
+
+	git_config_get_bool("imap.sslverify", &server.ssl_verify);
+	git_config_get_bool("imap.preformattedhtml", &server.use_html);
+	git_config_get_string("imap.folder", &imap_folder);
 
-	/* check booleans first, and barf on others */
-	if (!strcmp("sslverify", key))
-		server.ssl_verify = git_config_bool(key, val);
-	else if (!strcmp("preformattedhtml", key))
-		server.use_html = git_config_bool(key, val);
-	else if (!val)
-		return config_error_nonbool(key);
-
-	if (!strcmp("folder", key)) {
-		imap_folder = xstrdup(val);
-	} else if (!strcmp("host", key)) {
-		if (starts_with(val, "imap:"))
-			val += 5;
-		else if (starts_with(val, "imaps:")) {
-			val += 6;
-			server.use_ssl = 1;
+	if (!git_config_get_value("imap.host", &val)) {
+		if (!val) {
+			config_error_nonbool("imap.host");
+			git_die_config("imap.host");
+		} else {
+			if (starts_with(val, "imap:"))
+				val += 5;
+			else if (starts_with(val, "imaps:")) {
+				val += 6;
+				server.use_ssl = 1;
+			}
+			if (starts_with(val, "//"))
+				val += 2;
+			server.host = xstrdup(val);
 		}
-		if (starts_with(val, "//"))
-			val += 2;
-		server.host = xstrdup(val);
-	} else if (!strcmp("user", key))
-		server.user = xstrdup(val);
-	else if (!strcmp("pass", key))
-		server.pass = xstrdup(val);
-	else if (!strcmp("port", key))
-		server.port = git_config_int(key, val);
-	else if (!strcmp("tunnel", key))
-		server.tunnel = xstrdup(val);
-	else if (!strcmp("authmethod", key))
-		server.auth_method = xstrdup(val);
+	}
 
-	return 0;
+	git_config_get_string("imap.user", &server.user);
+	git_config_get_string("imap.pass", &server.pass);
+	git_config_get_int("imap.port", &server.port);
+	git_config_get_string("imap.tunnel", &server.tunnel);
+	git_config_get_string("imap.authmethod", &server.auth_method);
 }
 
 int main(int argc, char **argv)
@@ -1383,7 +1376,7 @@ int main(int argc, char **argv)
 		usage(imap_send_usage);
 
 	setup_git_directory_gently(&nongit_ok);
-	git_config(git_imap_config, NULL);
+	git_imap_config();
 
 	if (!server.port)
 		server.port = server.use_ssl ? 993 : 143;
-- 
1.9.0.GIT

[PATCH 10/11] alias.c: replace `git_config()` with `git_config_get_string()`

[Tanay Abhra]

Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 alias.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/alias.c b/alias.c
index 758c867..6aa164a 100644
--- a/alias.c
+++ b/alias.c
@@ -1,26 +1,13 @@
 #include "cache.h"
 
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
-	const char *name;
-	if (skip_prefix(k, "alias.", &name) && !strcmp(name, alias_key)) {
-		if (!v)
-			return config_error_nonbool(k);
-		alias_val = xstrdup(v);
-		return 0;
-	}
-	return 0;
-}
-
 char *alias_lookup(const char *alias)
 {
-	alias_key = alias;
-	alias_val = NULL;
-	git_config(alias_lookup_cb, NULL);
-	return alias_val;
+	char *v = NULL;
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "alias.%s", alias);
+	git_config_get_string(key.buf, &v);
+	strbuf_release(&key);
+	return v;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1
-- 
1.9.0.GIT

[PATCH 11/11] branch.c: replace `git_config()` with `git_config_get_string()

[Tanay Abhra]

Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 branch.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/branch.c b/branch.c
index 735767d..df6b120 100644
--- a/branch.c
+++ b/branch.c
@@ -140,30 +140,17 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	return 0;
 }
 
-struct branch_desc_cb {
-	const char *config_name;
-	const char *value;
-};
-
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
-	struct branch_desc_cb *desc = cb;
-	if (strcmp(desc->config_name, var))
-		return 0;
-	free((char *)desc->value);
-	return git_config_string(&desc->value, var, value);
-}
-
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
-	struct branch_desc_cb cb;
+	char *v = NULL;
 	struct strbuf name = STRBUF_INIT;
 	strbuf_addf(&name, "branch.%s.description", branch_name);
-	cb.config_name = name.buf;
-	cb.value = NULL;
-	git_config(read_branch_desc_cb, &cb);
-	if (cb.value)
-		strbuf_addstr(buf, cb.value);
+	if (git_config_get_string(name.buf, &v)) {
+		strbuf_release(&name);
+		return -1;
+	}
+	strbuf_addstr(buf, v);
+	free(v);
 	strbuf_release(&name);
 	return 0;
 }
-- 
1.9.0.GIT

Re: [PATCH 00/11] git_config callers rewritten with the new config-set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> This series is the first batch of patches which rewrites the existing callers
> using a non-callback approach.
> This series aims to,
>
> * rewrite the existing callers, as you can see from the diff stat the bew API
>   provides a much concise and clear control flow.
>
> * stress test the new API, see if any corner cases or deficiencies arise or not.

I went through the series. I agree with Eric that there's a slight
behavior change in http-backend.c, and I don't think it's a good thing.

Other than that, the series look good to me, although I would probably
need a second look to double check.

Tanay: I suggest you keep this as one 11-patches series. If you rewrite
more callsites, I'd find it easier to review as a second, independant
series rather than an ever-growing single series.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/