threads / patch / 37287

patch, 11 partsgit_config callers rewritten with the new config-set API

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

## tl;dr

20 messages between Aug 4, 2014 and Aug 6, 2014. Diffs are folded; open one to read it.

replies: 19people: 3as markdown or json

Tanay Abhra· Aug 4, 2014, 18:33 UTC · lore
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
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to daemon.c +5 −22
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
Matthieu Moy· Aug 4, 2014, 20:23 UTC · re: Tanay Abhra · lore

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

Tanay Abhra <tanayabh@gmail.com> writes:
> @@ -354,12 +338,11 @@ static int run_service(const char *dir, struct daemon_service *service)
[...]
Show 5 quoted lines
>  	}
> +
>  	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/
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to http-backend.c +12 −19
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
Eric Sunshine· Aug 4, 2014, 18:59 UTC · re: Tanay Abhra · lore

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

On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
Show 46 quoted lines
> 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?

Show 17 quoted lines
> -       /* 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
Tanay Abhra· Aug 6, 2014, 15:18 UTC · re: Eric Sunshine · lore

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

On 8/5/2014 12:29 AM, Eric Sunshine wrote:
Show 52 quoted lines
> 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.
Show 17 quoted lines
>> -       /* 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
Matthieu Moy· Aug 6, 2014, 15:44 UTC · re: Tanay Abhra · lore

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

Tanay Abhra <tanayabh@gmail.com> writes:
Show 33 quoted lines
> 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/
Eric Sunshine· Aug 6, 2014, 20:04 UTC · re: Matthieu Moy · lore

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

On Wed, Aug 6, 2014 at 11:44 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 53 quoted lines
> 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.

Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to read-cache.c +3 −11
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
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to archive.c +3 −9
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
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to fetch-pack.c +8 −27
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
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to rerere.c +12 −31
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
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to builtin/gc.c +20 −31
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
Matthieu Moy· Aug 4, 2014, 20:41 UTC · re: Tanay Abhra · lore

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

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/
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to pager.c +13 −27
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
Matthieu Moy· Aug 4, 2014, 20:36 UTC · re: Tanay Abhra · lore

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

Tanay Abhra <tanayabh@gmail.com> writes:
Show 6 quoted lines
> 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.

Show changes to pager.c +15 −29
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/
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to imap-send.c +27 −34
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
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to alias.c +6 −19
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
Tanay Abhra· Aug 4, 2014, 18:33 UTC · re: Tanay Abhra · lore

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

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(-)
Show changes to branch.c +7 −20
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
Matthieu Moy· Aug 4, 2014, 20:42 UTC · re: Tanay Abhra · lore

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

Tanay Abhra <tanayabh@gmail.com> writes:
Show 8 quoted lines
> 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/

← back to recent threads