[PATCH 1/4] fast-import.c: replace `git_config()` with `git_config_get_*()` family

[PATCH 1/4] fast-import.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>
---
 fast-import.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index d73f58c..eca5ed4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3274,36 +3274,32 @@ static void parse_option(const char *option)
 	die("This version of fast-import does not support option: %s", option);
 }
 
-static int git_pack_config(const char *k, const char *v, void *cb)
+static void git_pack_config(void)
 {
-	if (!strcmp(k, "pack.depth")) {
-		max_depth = git_config_int(k, v);
+	int indexversion_value;
+	unsigned long packsizelimit_value;
+
+	if (!git_config_get_ulong("pack.depth", &max_depth)) {
 		if (max_depth > MAX_DEPTH)
 			max_depth = MAX_DEPTH;
-		return 0;
 	}
-	if (!strcmp(k, "pack.compression")) {
-		int level = git_config_int(k, v);
-		if (level == -1)
-			level = Z_DEFAULT_COMPRESSION;
-		else if (level < 0 || level > Z_BEST_COMPRESSION)
-			die("bad pack compression level %d", level);
-		pack_compression_level = level;
+	if (!git_config_get_int("pack.compression", &pack_compression_level)) {
+		if (pack_compression_level == -1)
+			pack_compression_level = Z_DEFAULT_COMPRESSION;
+		else if (pack_compression_level < 0 ||
+			 pack_compression_level > Z_BEST_COMPRESSION)
+			die("bad pack compression level %d", pack_compression_level);
 		pack_compression_seen = 1;
-		return 0;
 	}
-	if (!strcmp(k, "pack.indexversion")) {
-		pack_idx_opts.version = git_config_int(k, v);
+	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
+		pack_idx_opts.version = indexversion_value;
 		if (pack_idx_opts.version > 2)
-			die("bad pack.indexversion=%"PRIu32,
-			    pack_idx_opts.version);
-		return 0;
+			die("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
 	}
-	if (!strcmp(k, "pack.packsizelimit")) {
-		max_packsize = git_config_ulong(k, v);
-		return 0;
-	}
-	return git_default_config(k, v, cb);
+	if (!git_config_get_ulong("pack.packsizelimit", &packsizelimit_value))
+		max_packsize = packsizelimit_value;
+
+	git_config(git_default_config, NULL);
 }
 
 static const char fast_import_usage[] =
@@ -3356,7 +3352,7 @@ int main(int argc, char **argv)
 
 	setup_git_directory();
 	reset_pack_idx_option(&pack_idx_opts);
-	git_config(git_pack_config, NULL);
+	git_pack_config();
 	if (!pack_compression_seen && core_compression_seen)
 		pack_compression_level = core_compression_level;
 
-- 
1.9.0.GIT

[PATCH 2/4] ll-merge.c: refactor `read_merge_config()` to use `git_config_string()`

[Tanay Abhra]

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 ll-merge.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..8ea03e5 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -225,11 +225,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
 	const char *key, *name;
 	int namelen;
 
-	if (!strcmp(var, "merge.default")) {
-		if (value)
-			default_ll_merge = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "merge.default"))
+		return git_config_string(&default_ll_merge, var, value);
 
 	/*
 	 * We are not interested in anything but "merge.<name>.variable";
@@ -254,12 +251,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
 		ll_user_merge_tail = &(fn->next);
 	}
 
-	if (!strcmp("name", key)) {
-		if (!value)
-			return error("%s: lacks value", var);
-		fn->description = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp("name", key))
+		return git_config_string(&fn->description, var, value);
 
 	if (!strcmp("driver", key)) {
 		if (!value)
@@ -285,12 +278,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (!strcmp("recursive", key)) {
-		if (!value)
-			return error("%s: lacks value", var);
-		fn->recursive = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp("recursive", key))
+		return git_config_string(&fn->recursive, var, value);
 
 	return 0;
 }
-- 
1.9.0.GIT

Re: [PATCH 2/4] ll-merge.c: refactor `read_merge_config()` to use `git_config_string()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  ll-merge.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/ll-merge.c b/ll-merge.c
> index fb61ea6..8ea03e5 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> @@ -225,11 +225,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
>  	const char *key, *name;
>  	int namelen;
>  
> -	if (!strcmp(var, "merge.default")) {
> -		if (value)
> -			default_ll_merge = xstrdup(value);
> -		return 0;
> -	}
> +	if (!strcmp(var, "merge.default"))
> +		return git_config_string(&default_ll_merge, var, value);

Previously, merge.default without value was a no-op. It's now an error.

I think it makes perfect sense, but should be documented in the log
message.

Also, I think you should explain briefly the reason for not using your
non-callback API here.

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

[PATCH v2 2/4] ll-merge.c: refactor `read_merge_config()` to use `git_config_string()`

[Tanay Abhra]

There is one slight behavior change, previously "merge.default"
silently ignored a NULL value and didn't raise any error. But,
in the same function, all other values raise an error on a NULL
value. So to conform with other call sites in Git, a NULL value
for "merge.default" raises an error.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
We cannot easily use the new config-set API here, because
much of the function is dedicated to processing
"merge.<name>.variable" which does not easily translate to
the new API. If it were for variables like,
"merge.summary", "merge.tool", and "merge.verbosity", we
could use the new API.

 ll-merge.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..8ea03e5 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -225,11 +225,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
 	const char *key, *name;
 	int namelen;

-	if (!strcmp(var, "merge.default")) {
-		if (value)
-			default_ll_merge = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "merge.default"))
+		return git_config_string(&default_ll_merge, var, value);

 	/*
 	 * We are not interested in anything but "merge.<name>.variable";
@@ -254,12 +251,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
 		ll_user_merge_tail = &(fn->next);
 	}

-	if (!strcmp("name", key)) {
-		if (!value)
-			return error("%s: lacks value", var);
-		fn->description = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp("name", key))
+		return git_config_string(&fn->description, var, value);

 	if (!strcmp("driver", key)) {
 		if (!value)
@@ -285,12 +278,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
 		return 0;
 	}

-	if (!strcmp("recursive", key)) {
-		if (!value)
-			return error("%s: lacks value", var);
-		fn->recursive = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp("recursive", key))
+		return git_config_string(&fn->recursive, var, value);

 	return 0;
 }
-- 
1.9.0.GIT

Re: [PATCH v2 2/4] ll-merge.c: refactor `read_merge_config()` to use `git_config_string()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> There is one slight behavior change, previously "merge.default"
> silently ignored a NULL value and didn't raise any error. But,
> in the same function, all other values raise an error on a NULL
> value. So to conform with other call sites in Git, a NULL value
> for "merge.default" raises an error.

Good, thanks.

> We cannot easily use the new config-set API here, because
> much of the function is dedicated to processing
> "merge.<name>.variable" which does not easily translate to
> the new API. If it were for variables like,
> "merge.summary", "merge.tool", and "merge.verbosity", we
> could use the new API.

I think this would deserve to be in the commit message, but I'm fine
with keeping it here too.

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

Re: [PATCH v2 2/4] ll-merge.c: refactor `read_merge_config()` to use `git_config_string()`

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> There is one slight behavior change, previously "merge.default"
> silently ignored a NULL value and didn't raise any error. But,
> in the same function, all other values raise an error on a NULL
> value. So to conform with other call sites in Git, a NULL value
> for "merge.default" raises an error.

Better explained than v1 ;-)

> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
> We cannot easily use the new config-set API here, because
> much of the function is dedicated to processing
> "merge.<name>.variable" which does not easily translate to
> the new API. If it were for variables like,
> "merge.summary", "merge.tool", and "merge.verbosity", we
> could use the new API.

I think this comment belongs to the log message, if only to serve as
a reminder for us that the API needs to be made more usable when the
caller wants to use these three-level names, which are quite common.
This code path knows the name of a low-level merge driver and wants
to learn everything about that driver.  Another code path may know
the name of the branch and may want to scan "branch.<name>.*".

>  ll-merge.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/ll-merge.c b/ll-merge.c
> index fb61ea6..8ea03e5 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> @@ -225,11 +225,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
>  	const char *key, *name;
>  	int namelen;
>
> -	if (!strcmp(var, "merge.default")) {
> -		if (value)
> -			default_ll_merge = xstrdup(value);
> -		return 0;
> -	}
> +	if (!strcmp(var, "merge.default"))
> +		return git_config_string(&default_ll_merge, var, value);
>
>  	/*
>  	 * We are not interested in anything but "merge.<name>.variable";
> @@ -254,12 +251,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
>  		ll_user_merge_tail = &(fn->next);
>  	}
>
> -	if (!strcmp("name", key)) {
> -		if (!value)
> -			return error("%s: lacks value", var);
> -		fn->description = xstrdup(value);
> -		return 0;
> -	}
> +	if (!strcmp("name", key))
> +		return git_config_string(&fn->description, var, value);
>
>  	if (!strcmp("driver", key)) {
>  		if (!value)
> @@ -285,12 +278,8 @@ static int read_merge_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>
> -	if (!strcmp("recursive", key)) {
> -		if (!value)
> -			return error("%s: lacks value", var);
> -		fn->recursive = xstrdup(value);
> -		return 0;
> -	}
> +	if (!strcmp("recursive", key))
> +		return git_config_string(&fn->recursive, var, value);
>
>  	return 0;
>  }

[PATCH 3/4] merge-recursive.c: replace `git_config()` with `git_config_get_int()`

[Tanay Abhra]

Use `git_config_get_int()` 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>
---
 merge-recursive.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1d332b8..8ab944c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2026,22 +2026,12 @@ int merge_recursive_generic(struct merge_options *o,
 	return clean ? 0 : 1;
 }
 
-static int merge_recursive_config(const char *var, const char *value, void *cb)
+static void merge_recursive_config(struct merge_options *o)
 {
-	struct merge_options *o = cb;
-	if (!strcmp(var, "merge.verbosity")) {
-		o->verbosity = git_config_int(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "diff.renamelimit")) {
-		o->diff_rename_limit = git_config_int(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "merge.renamelimit")) {
-		o->merge_rename_limit = git_config_int(var, value);
-		return 0;
-	}
-	return git_xmerge_config(var, value, cb);
+	git_config_get_int("merge.verbosity", &o->verbosity);
+	git_config_get_int("diff.renamelimit", &o->diff_rename_limit);
+	git_config_get_int("merge.renamelimit", &o->merge_rename_limit);
+	git_config(git_xmerge_config, NULL);
 }
 
 void init_merge_options(struct merge_options *o)
@@ -2052,7 +2042,7 @@ void init_merge_options(struct merge_options *o)
 	o->diff_rename_limit = -1;
 	o->merge_rename_limit = -1;
 	o->renormalize = 0;
-	git_config(merge_recursive_config, o);
+	merge_recursive_config(o);
 	if (getenv("GIT_MERGE_VERBOSITY"))
 		o->verbosity =
 			strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
-- 
1.9.0.GIT

Re: [PATCH 3/4] merge-recursive.c: replace `git_config()` with `git_config_get_int()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

>  merge-recursive.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)

>  builtin/apply.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

These two look straightforward and good.

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

[PATCH 4/4] builtin/apply.c: replace `git_config()` with `git_config_get_string_const()`

[Tanay Abhra]

Use `git_config_get_string_const()` 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/apply.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index be2b4ce..66acf32 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4269,13 +4269,11 @@ static int apply_patch(int fd, const char *filename, int options)
 	return 0;
 }
 
-static int git_apply_config(const char *var, const char *value, void *cb)
+static void git_apply_config(void)
 {
-	if (!strcmp(var, "apply.whitespace"))
-		return git_config_string(&apply_default_whitespace, var, value);
-	else if (!strcmp(var, "apply.ignorewhitespace"))
-		return git_config_string(&apply_default_ignorewhitespace, var, value);
-	return git_default_config(var, value, cb);
+	git_config_get_string_const("apply.whitespace", &apply_default_whitespace);
+	git_config_get_string_const("apply.ignorewhitespace", &apply_default_ignorewhitespace);
+	git_config(git_default_config, NULL);
 }
 
 static int option_parse_exclude(const struct option *opt,
@@ -4423,7 +4421,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 
 	prefix = prefix_;
 	prefix_length = prefix ? strlen(prefix) : 0;
-	git_config(git_apply_config, NULL);
+	git_apply_config();
 	if (apply_default_whitespace)
 		parse_whitespace_option(apply_default_whitespace);
 	if (apply_default_ignorewhitespace)
-- 
1.9.0.GIT

Re: [PATCH 1/4] fast-import.c: replace `git_config()` with `git_config_get_*()` family

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

>  fast-import.c | 42 +++++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)

Only 4 lines less, how disappointing ;-).

More seriously, the old code was essentially dealing with special
values, which your new code needs to do too, so you can hardly do any
less.

> +	if (!git_config_get_int("pack.compression", &pack_compression_level)) {
> +		if (pack_compression_level == -1)
> +			pack_compression_level = Z_DEFAULT_COMPRESSION;
> +		else if (pack_compression_level < 0 ||
> +			 pack_compression_level > Z_BEST_COMPRESSION)
> +			die("bad pack compression level %d", pack_compression_level);

That would be a good use for git_die_config(), to give a better error
message, right?

> -	if (!strcmp(k, "pack.indexversion")) {
> -		pack_idx_opts.version = git_config_int(k, v);
> +	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
> +		pack_idx_opts.version = indexversion_value;

I wondered why you were not assigning to pack_idx_opts.version directly,
but pack_idx_opts.version is uint32 and you don't have
config_get_uint32, so it's OK.

>  		if (pack_idx_opts.version > 2)
> -			die("bad pack.indexversion=%"PRIu32,
> -			    pack_idx_opts.version);
> -		return 0;
> +			die("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);

One more opportunity for git_die_config()?

Not that it's terribly important, but I think it's good that your
refactoring also brings a few end-users benefits. It will help you show
off when you tell your friends what you did this summer (not "I did
useless code churn" ;-) ), and helps everybody see the benefits of your
GSoC ;-).

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

Re: [PATCH 1/4] fast-import.c: replace `git_config()` with `git_config_get_*()` family

[Tanay Abhra]

>>  		if (pack_idx_opts.version > 2)
>> -			die("bad pack.indexversion=%"PRIu32,
>> -			    pack_idx_opts.version);
>> -		return 0;
>> +			die("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
> 
> One more opportunity for git_die_config()?
>

Yup, I had thought of changing that and above to git_die_config(), but didn't do
it, will send a revised patch.


> Not that it's terribly important, but I think it's good that your
> refactoring also brings a few end-users benefits. It will help you show

I have been rewriting callers and the law of diminishing returns kicked in. I had
rewritten some other call sites but I didn't see them bringing any benefits (cleaner
control flow, less lines, bugs eliminated), so I left them out.

> off when you tell your friends what you did this summer (not "I did
> useless code churn" ;-) ), and helps everybody see the benefits of your
> GSoC ;-).
>

[PATCH v2 1/5] fast-import.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>
---
 fast-import.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index d73f58c..34e780d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3274,36 +3274,34 @@ static void parse_option(const char *option)
 	die("This version of fast-import does not support option: %s", option);
 }

-static int git_pack_config(const char *k, const char *v, void *cb)
+static void git_pack_config(void)
 {
-	if (!strcmp(k, "pack.depth")) {
-		max_depth = git_config_int(k, v);
+	int indexversion_value;
+	unsigned long packsizelimit_value;
+
+	if (!git_config_get_ulong("pack.depth", &max_depth)) {
 		if (max_depth > MAX_DEPTH)
 			max_depth = MAX_DEPTH;
-		return 0;
 	}
-	if (!strcmp(k, "pack.compression")) {
-		int level = git_config_int(k, v);
-		if (level == -1)
-			level = Z_DEFAULT_COMPRESSION;
-		else if (level < 0 || level > Z_BEST_COMPRESSION)
-			die("bad pack compression level %d", level);
-		pack_compression_level = level;
+	if (!git_config_get_int("pack.compression", &pack_compression_level)) {
+		if (pack_compression_level == -1)
+			pack_compression_level = Z_DEFAULT_COMPRESSION;
+		else if (pack_compression_level < 0 ||
+			 pack_compression_level > Z_BEST_COMPRESSION)
+			git_die_config("pack.compression",
+					"bad pack compression level %d", pack_compression_level);
 		pack_compression_seen = 1;
-		return 0;
 	}
-	if (!strcmp(k, "pack.indexversion")) {
-		pack_idx_opts.version = git_config_int(k, v);
+	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
+		pack_idx_opts.version = indexversion_value;
 		if (pack_idx_opts.version > 2)
-			die("bad pack.indexversion=%"PRIu32,
-			    pack_idx_opts.version);
-		return 0;
+			git_die_config("pack.indexversion",
+					"bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
 	}
-	if (!strcmp(k, "pack.packsizelimit")) {
-		max_packsize = git_config_ulong(k, v);
-		return 0;
-	}
-	return git_default_config(k, v, cb);
+	if (!git_config_get_ulong("pack.packsizelimit", &packsizelimit_value))
+		max_packsize = packsizelimit_value;
+
+	git_config(git_default_config, NULL);
 }

 static const char fast_import_usage[] =
@@ -3356,7 +3354,7 @@ int main(int argc, char **argv)

 	setup_git_directory();
 	reset_pack_idx_option(&pack_idx_opts);
-	git_config(git_pack_config, NULL);
+	git_pack_config();
 	if (!pack_compression_seen && core_compression_seen)
 		pack_compression_level = core_compression_level;

-- 
1.9.0.GIT

Re: [PATCH v2 1/5] fast-import.c: replace `git_config()` with `git_config_get_*()` family

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +	if (!git_config_get_int("pack.compression", &pack_compression_level)) {
> +		if (pack_compression_level == -1)
> +			pack_compression_level = Z_DEFAULT_COMPRESSION;
> +		else if (pack_compression_level < 0 ||
> +			 pack_compression_level > Z_BEST_COMPRESSION)
> +			git_die_config("pack.compression",
> +					"bad pack compression level %d", pack_compression_level);

Perfect. With v2 for PATCH 2 and PATCH 5, the series looks good to me.

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

[PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API

[Tanay Abhra]

git_default_config() now uses config-set API functions to query for
values.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
Sorry, for the short log message, I will explain why.
The git_default_config() rewrite is 100% complete, the only
problem remains is the call sites; there are too many of them.
Some are called from callback functions which pass the remaining
variables to git_default_config() which they do not have any use for.
Those call sites can remain as they are, because git_default_config()
is a single call function now, and is guarded by a sentinel value.
So after one call, it would just return immediately instead of going on
checking.

For callers like git_config(git_default_config, NULL) (there are 38 of them),
we can leave them as they are or rewrite them as I have illustrated in the
next attached patch.

I will take this series out of RFC as soon as we have decided what to do with
the call sites.

Cheers,
Tanay Abhra.

 advice.c |  17 ++--
 advice.h |   2 +-
 cache.h  |   2 +-
 config.c | 346 ++++++++++++++++++++++-----------------------------------------
 ident.c  |  17 ++--
 5 files changed, 136 insertions(+), 248 deletions(-)

diff --git a/advice.c b/advice.c
index 9b42033..34e1ccc 100644
--- a/advice.c
+++ b/advice.c
@@ -59,22 +59,17 @@ void advise(const char *advice, ...)
 	strbuf_release(&buf);
 }

-int git_default_advice_config(const char *var, const char *value)
+void git_default_advice_config(void)
 {
-	const char *k;
+	struct strbuf var = STRBUF_INIT;
 	int i;

-	if (!skip_prefix(var, "advice.", &k))
-		return 0;
-
 	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
-		if (strcmp(k, advice_config[i].name))
-			continue;
-		*advice_config[i].preference = git_config_bool(var, value);
-		return 0;
+		strbuf_addf(&var, "advice.%s", advice_config[i].name);
+		git_config_get_bool(var.buf, advice_config[i].preference);
+		strbuf_reset(&var);
 	}
-
-	return 0;
+	strbuf_release(&var);
 }

 int error_resolve_conflict(const char *me)
diff --git a/advice.h b/advice.h
index 5ecc6c1..5bfe46c 100644
--- a/advice.h
+++ b/advice.h
@@ -19,7 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;

-int git_default_advice_config(const char *var, const char *value);
+void git_default_advice_config(void);
 __attribute__((format (printf, 1, 2)))
 void advise(const char *advice, ...);
 int error_resolve_conflict(const char *me);
diff --git a/cache.h b/cache.h
index 2693a37..fa28b40 100644
--- a/cache.h
+++ b/cache.h
@@ -1065,7 +1065,7 @@ extern const char *fmt_name(const char *name, const char *email);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
-extern int git_ident_config(const char *, const char *, void *);
+extern void git_ident_config(void);

 struct ident_split {
 	const char *name_begin;
diff --git a/config.c b/config.c
index 427850a..36b9124 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,8 @@ static int zlib_compression_seen;
  */
 static struct config_set the_config_set;

+static int default_config_loaded;
+
 static int config_file_fgetc(struct config_source *conf)
 {
 	return fgetc(conf->u.file);
@@ -670,147 +672,91 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
 	return 0;
 }

-static int git_default_core_config(const char *var, const char *value)
+static void git_default_core_config(void)
 {
+	const char *value = NULL;
+	unsigned long v_l = 0;
+	int abbrev;
+	const char *comment;
+
 	/* This needs a better name */
-	if (!strcmp(var, "core.filemode")) {
-		trust_executable_bit = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "core.trustctime")) {
-		trust_ctime = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "core.checkstat")) {
+	git_config_get_bool("core.filemode", &trust_executable_bit);
+	git_config_get_bool("core.trustctime", &trust_ctime);
+
+	if (!git_config_get_value("core.checkstat", &value)) {
 		if (!strcasecmp(value, "default"))
 			check_stat = 1;
 		else if (!strcasecmp(value, "minimal"))
 			check_stat = 0;
 	}

-	if (!strcmp(var, "core.quotepath")) {
-		quote_path_fully = git_config_bool(var, value);
-		return 0;
-	}
+	git_config_get_bool("core.quotepath", &quote_path_fully);
+	git_config_get_bool("core.symlinks", &has_symlinks);
+	git_config_get_bool("core.ignorecase", &ignore_case);
+	git_config_get_pathname("core.attributesfile", &git_attributes_file);
+	git_config_get_bool("core.bare", &is_bare_repository_cfg);
+	git_config_get_bool("core.ignorestat", &assume_unchanged);
+	git_config_get_bool("core.prefersymlinkrefs", &prefer_symlink_refs);
+	git_config_get_bool("core.logallrefupdates", &log_all_ref_updates);
+	git_config_get_bool("core.warnambiguousrefs", &warn_ambiguous_refs);

-	if (!strcmp(var, "core.symlinks")) {
-		has_symlinks = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.ignorecase")) {
-		ignore_case = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.attributesfile"))
-		return git_config_pathname(&git_attributes_file, var, value);
-
-	if (!strcmp(var, "core.bare")) {
-		is_bare_repository_cfg = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.ignorestat")) {
-		assume_unchanged = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.prefersymlinkrefs")) {
-		prefer_symlink_refs = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.logallrefupdates")) {
-		log_all_ref_updates = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.warnambiguousrefs")) {
-		warn_ambiguous_refs = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.abbrev")) {
-		int abbrev = git_config_int(var, value);
+	if (!git_config_get_int("core.abbrev", &abbrev)) {
 		if (abbrev < minimum_abbrev || abbrev > 40)
-			return -1;
+			git_die_config("core.abbrev", NULL);
 		default_abbrev = abbrev;
-		return 0;
 	}
-
-	if (!strcmp(var, "core.loosecompression")) {
-		int level = git_config_int(var, value);
-		if (level == -1)
-			level = Z_DEFAULT_COMPRESSION;
-		else if (level < 0 || level > Z_BEST_COMPRESSION)
-			die(_("bad zlib compression level %d"), level);
-		zlib_compression_level = level;
+	if (!git_config_get_int("core.loosecompression", &zlib_compression_level)) {
+		if (zlib_compression_level == -1)
+			zlib_compression_level = Z_DEFAULT_COMPRESSION;
+		else if (zlib_compression_level < 0 ||
+			 zlib_compression_level > Z_BEST_COMPRESSION)
+			git_die_config("core.loosecompression", _("bad zlib compression level %d"),
+					zlib_compression_level);
 		zlib_compression_seen = 1;
-		return 0;
 	}
-
-	if (!strcmp(var, "core.compression")) {
-		int level = git_config_int(var, value);
-		if (level == -1)
-			level = Z_DEFAULT_COMPRESSION;
-		else if (level < 0 || level > Z_BEST_COMPRESSION)
-			die(_("bad zlib compression level %d"), level);
-		core_compression_level = level;
+	if (!git_config_get_int("core.compression", &core_compression_level)) {
+		if (core_compression_level == -1)
+			core_compression_level = Z_DEFAULT_COMPRESSION;
+		else if (core_compression_level < 0 ||
+			 core_compression_level > Z_BEST_COMPRESSION)
+			git_die_config("core.compression", _("bad zlib compression level %d"),
+					core_compression_level);
 		core_compression_seen = 1;
 		if (!zlib_compression_seen)
-			zlib_compression_level = level;
-		return 0;
+			zlib_compression_level = core_compression_level;
 	}
-
-	if (!strcmp(var, "core.packedgitwindowsize")) {
+	if (!git_config_get_ulong("core.packedgitwindowsize", &v_l)) {
 		int pgsz_x2 = getpagesize() * 2;
-		packed_git_window_size = git_config_ulong(var, value);
+		packed_git_window_size = v_l;

 		/* This value must be multiple of (pagesize * 2) */
 		packed_git_window_size /= pgsz_x2;
 		if (packed_git_window_size < 1)
 			packed_git_window_size = 1;
 		packed_git_window_size *= pgsz_x2;
-		return 0;
-	}
-
-	if (!strcmp(var, "core.bigfilethreshold")) {
-		big_file_threshold = git_config_ulong(var, value);
-		return 0;
 	}
+	git_config_get_ulong("core.bigfilethreshold", &big_file_threshold);
+	if (!git_config_get_ulong("core.packedgitlimit", &v_l))
+		packed_git_limit = v_l;
+	if (!git_config_get_ulong("core.deltabasecachelimit", &v_l))
+		delta_base_cache_limit = v_l;

-	if (!strcmp(var, "core.packedgitlimit")) {
-		packed_git_limit = git_config_ulong(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.deltabasecachelimit")) {
-		delta_base_cache_limit = git_config_ulong(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.autocrlf")) {
+	if (!git_config_get_value("core.autocrlf", &value)) {
 		if (value && !strcasecmp(value, "input")) {
 			if (core_eol == EOL_CRLF)
-				return error("core.autocrlf=input conflicts with core.eol=crlf");
+				git_die_config("core.autocrlf",
+						"core.autocrlf=input conflicts with core.eol=crlf");
 			auto_crlf = AUTO_CRLF_INPUT;
-			return 0;
-		}
-		auto_crlf = git_config_bool(var, value);
-		return 0;
+		} else
+			auto_crlf = git_config_bool("core.autocrlf", value);
 	}
-
-	if (!strcmp(var, "core.safecrlf")) {
-		if (value && !strcasecmp(value, "warn")) {
+	if (!git_config_get_value("core.safecrlf", &value)) {
+		if (value && !strcasecmp(value, "warn"))
 			safe_crlf = SAFE_CRLF_WARN;
-			return 0;
-		}
-		safe_crlf = git_config_bool(var, value);
-		return 0;
+		else
+			safe_crlf = git_config_bool("core.safecrlf", value);
 	}
-
-	if (!strcmp(var, "core.eol")) {
+	if (!git_config_get_value("core.eol", &value)) {
 		if (value && !strcasecmp(value, "lf"))
 			core_eol = EOL_LF;
 		else if (value && !strcasecmp(value, "crlf"))
@@ -820,106 +766,71 @@ static int git_default_core_config(const char *var, const char *value)
 		else
 			core_eol = EOL_UNSET;
 		if (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)
-			return error("core.autocrlf=input conflicts with core.eol=crlf");
-		return 0;
+			git_die_config("core.autocrlf",
+					"core.autocrlf=input conflicts with core.eol=crlf");
 	}
+	git_config_get_string("core.notesref", &notes_ref_name);
+	git_config_get_string_const("core.pager", &pager_program);
+	git_config_get_string_const("core.editor", &editor_program);

-	if (!strcmp(var, "core.notesref")) {
-		notes_ref_name = xstrdup(value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.pager"))
-		return git_config_string(&pager_program, var, value);
-
-	if (!strcmp(var, "core.editor"))
-		return git_config_string(&editor_program, var, value);
-
-	if (!strcmp(var, "core.commentchar")) {
-		if (!value)
-			return config_error_nonbool(var);
-		else if (!strcasecmp(value, "auto"))
+	if (!git_config_get_string_const("core.commentchar", &comment)) {
+		if (!strcasecmp(comment, "auto"))
 			auto_comment_line_char = 1;
-		else if (value[0] && !value[1]) {
-			comment_line_char = value[0];
+		else if (comment[0] && !comment[1]) {
+			comment_line_char = comment[0];
 			auto_comment_line_char = 0;
 		} else
-			return error("core.commentChar should only be one character");
-		return 0;
+			git_die_config("core.commentchar",
+					"core.commentchar should only be one character");
 	}
+	git_config_get_string_const("core.askpass", &askpass_program);
+	git_config_get_pathname("core.excludesfile", &excludes_file);

-	if (!strcmp(var, "core.askpass"))
-		return git_config_string(&askpass_program, var, value);
-
-	if (!strcmp(var, "core.excludesfile"))
-		return git_config_pathname(&excludes_file, var, value);
-
-	if (!strcmp(var, "core.whitespace")) {
+	if (!git_config_get_value("core.whitespace", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
-		whitespace_rule_cfg = parse_whitespace_rule(value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.fsyncobjectfiles")) {
-		fsync_object_files = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.preloadindex")) {
-		core_preload_index = git_config_bool(var, value);
-		return 0;
+			git_die_config("core.whitespace", "Missing value for 'core.whitespace'");
+		else
+			whitespace_rule_cfg = parse_whitespace_rule(value);
 	}
+	git_config_get_bool("core.fsyncobjectfiles", &fsync_object_files);
+	git_config_get_bool("core.preloadindex", &core_preload_index);

-	if (!strcmp(var, "core.createobject")) {
+	if (!git_config_get_value("core.createobject", &value)) {
 		if (!strcmp(value, "rename"))
 			object_creation_mode = OBJECT_CREATION_USES_RENAMES;
 		else if (!strcmp(value, "link"))
 			object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
 		else
-			die(_("invalid mode for object creation: %s"), value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.sparsecheckout")) {
-		core_apply_sparse_checkout = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.precomposeunicode")) {
-		precomposed_unicode = git_config_bool(var, value);
-		return 0;
+			git_die_config("core.createobject",
+					_("Invalid mode for object creation: %s"), value);
 	}
+	git_config_get_bool("core.sparsecheckout", &core_apply_sparse_checkout);
+	git_config_get_bool("core.precomposeunicode", &precomposed_unicode);

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_i18n_config(const char *var, const char *value)
+static void git_default_i18n_config(void)
 {
-	if (!strcmp(var, "i18n.commitencoding"))
-		return git_config_string(&git_commit_encoding, var, value);
-
-	if (!strcmp(var, "i18n.logoutputencoding"))
-		return git_config_string(&git_log_output_encoding, var, value);
+	git_config_get_string_const("i18n.commitencoding", &git_commit_encoding);
+	git_config_get_string_const("i18n.logoutputencoding", &git_log_output_encoding);

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_branch_config(const char *var, const char *value)
+static void git_default_branch_config(void)
 {
-	if (!strcmp(var, "branch.autosetupmerge")) {
-		if (value && !strcasecmp(value, "always")) {
+	const char *value = NULL;
+	if (!git_config_get_value("branch.autosetupmerge", &value)) {
+		if (value && !strcasecmp(value, "always"))
 			git_branch_track = BRANCH_TRACK_ALWAYS;
-			return 0;
-		}
-		git_branch_track = git_config_bool(var, value);
-		return 0;
+		else
+			git_branch_track = git_config_bool("branch.autosetupmerge", value);
 	}
-	if (!strcmp(var, "branch.autosetuprebase")) {
+	if (!git_config_get_value("branch.autosetuprebase", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			git_die_config("branch.autosetuprebase",
+					"Missing value for 'branch.autosetuprebase'");
 		else if (!strcmp(value, "never"))
 			autorebase = AUTOREBASE_NEVER;
 		else if (!strcmp(value, "local"))
@@ -929,19 +840,19 @@ static int git_default_branch_config(const char *var, const char *value)
 		else if (!strcmp(value, "always"))
 			autorebase = AUTOREBASE_ALWAYS;
 		else
-			return error("Malformed value for %s", var);
-		return 0;
+			git_die_config("branch.autosetuprebase",
+					"Malformed value for branch.autosetuprebase");
 	}

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_push_config(const char *var, const char *value)
+static void git_default_push_config(void)
 {
-	if (!strcmp(var, "push.default")) {
+	const char *value  = NULL;
+	if (!git_config_get_value("push.default", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			git_die_config("push.default", "Missing value for 'push.default'");
 		else if (!strcmp(value, "nothing"))
 			push_default = PUSH_DEFAULT_NOTHING;
 		else if (!strcmp(value, "matching"))
@@ -955,60 +866,44 @@ static int git_default_push_config(const char *var, const char *value)
 		else if (!strcmp(value, "current"))
 			push_default = PUSH_DEFAULT_CURRENT;
 		else {
-			error("Malformed value for %s: %s", var, value);
-			return error("Must be one of nothing, matching, simple, "
+			error("Malformed value for push.default: %s", value);
+			git_die_config("push.default", "Must be one of nothing, matching, simple, "
 				     "upstream or current.");
 		}
-		return 0;
 	}

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_mailmap_config(const char *var, const char *value)
+static void git_default_mailmap_config(void)
 {
-	if (!strcmp(var, "mailmap.file"))
-		return git_config_pathname(&git_mailmap_file, var, value);
-	if (!strcmp(var, "mailmap.blob"))
-		return git_config_string(&git_mailmap_blob, var, value);
+	git_config_get_pathname("mailmap.file", &git_mailmap_file);
+	git_config_get_string_const("mailmap.blob", &git_mailmap_blob);

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-int git_default_config(const char *var, const char *value, void *dummy)
+int git_default_config(const char *unused, const char *unused2, void *dummy)
 {
-	if (starts_with(var, "core."))
-		return git_default_core_config(var, value);
-
-	if (starts_with(var, "user."))
-		return git_ident_config(var, value, dummy);
-
-	if (starts_with(var, "i18n."))
-		return git_default_i18n_config(var, value);
-
-	if (starts_with(var, "branch."))
-		return git_default_branch_config(var, value);
-
-	if (starts_with(var, "push."))
-		return git_default_push_config(var, value);
+	const char *v = NULL;

-	if (starts_with(var, "mailmap."))
-		return git_default_mailmap_config(var, value);
-
-	if (starts_with(var, "advice."))
-		return git_default_advice_config(var, value);
-
-	if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) {
-		pager_use_color = git_config_bool(var,value);
-		return 0;
-	}
-
-	if (!strcmp(var, "pack.packsizelimit")) {
-		pack_size_limit_cfg = git_config_ulong(var, value);
+	if (default_config_loaded)
 		return 0;
-	}
+	git_default_core_config();
+	git_ident_config();
+	git_default_i18n_config();
+	git_default_branch_config();
+	git_default_push_config();
+	git_default_mailmap_config();
+	git_default_advice_config();
+
+	if (!git_config_get_value("pager.color", &v))
+		pager_use_color = git_config_bool("pager.color",v);
+	else if (!git_config_get_value("color.pager", &v))
+		pager_use_color = git_config_bool("color.pager",v);
+
+	git_config_get_ulong("pack.packsizelimit", &pack_size_limit_cfg);
+	default_config_loaded = 1;
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
@@ -2082,6 +1977,7 @@ int git_config_set_multivar_in_file(const char *config_filename,

 	/* Invalidate the config cache */
 	git_config_clear();
+	default_config_loaded = 0;

 out_free:
 	if (lock)
diff --git a/ident.c b/ident.c
index 1d9b6e7..0db595f 100644
--- a/ident.c
+++ b/ident.c
@@ -392,29 +392,26 @@ int author_ident_sufficiently_given(void)
 	return ident_is_sufficient(author_ident_explicitly_given);
 }

-int git_ident_config(const char *var, const char *value, void *data)
+void git_ident_config(void)
 {
-	if (!strcmp(var, "user.name")) {
+	const char *value = NULL;
+
+	if (!git_config_get_value("user.name", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			git_die_config("user.name", "Missing value for 'user.name'");
 		strbuf_reset(&git_default_name);
 		strbuf_addstr(&git_default_name, value);
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		return 0;
 	}
-
-	if (!strcmp(var, "user.email")) {
+	if (!git_config_get_value("user.email", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			git_die_config("user.email", "Missing value for 'user.email'");
 		strbuf_reset(&git_default_email);
 		strbuf_addstr(&git_default_email, value);
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		return 0;
 	}
-
-	return 0;
 }

 static int buf_cmp(const char *a_begin, const char *a_end,
-- 
1.9.0.GIT

[PATCH/RFC v2 2/2] use the new git_default_config()

[Tanay Abhra]

If we change the signature to void git_default_config(void),
we would have to use a patch like this to change the call sites
of the function. This patch is just for illustrative purpose,
I couldn't finalize if this was unnecessary code cruft or
a valid approach.

---
 builtin/check-attr.c         | 2 +-
 builtin/check-ignore.c       | 2 +-
 builtin/check-mailmap.c      | 2 +-
 builtin/checkout-index.c     | 2 +-
 builtin/clone.c              | 2 +-
 builtin/config.c             | 2 +-
 builtin/describe.c           | 2 +-
 builtin/fast-export.c        | 2 +-
 builtin/for-each-ref.c       | 2 +-
 builtin/hash-object.c        | 2 +-
 builtin/init-db.c            | 2 +-
 builtin/ls-files.c           | 2 +-
 builtin/ls-tree.c            | 2 +-
 builtin/merge-base.c         | 2 +-
 builtin/mv.c                 | 2 +-
 builtin/name-rev.c           | 2 +-
 builtin/notes.c              | 2 +-
 builtin/push.c               | 2 +-
 builtin/read-tree.c          | 2 +-
 builtin/reset.c              | 2 +-
 builtin/rev-list.c           | 2 +-
 builtin/rev-parse.c          | 2 +-
 builtin/revert.c             | 4 ++--
 builtin/rm.c                 | 2 +-
 builtin/shortlog.c           | 2 +-
 builtin/stripspace.c         | 2 +-
 builtin/symbolic-ref.c       | 2 +-
 builtin/unpack-file.c        | 2 +-
 builtin/unpack-objects.c     | 2 +-
 builtin/update-index.c       | 2 +-
 builtin/update-ref.c         | 2 +-
 builtin/update-server-info.c | 2 +-
 builtin/var.c                | 2 +-
 builtin/verify-pack.c        | 2 +-
 builtin/write-tree.c         | 2 +-
 http-fetch.c                 | 2 +-
 pager.c                      | 2 +-
 37 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 5600ec3..e2d7826 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -105,7 +105,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	if (!is_bare_repository())
 		setup_work_tree();

-	git_config(git_default_config, NULL);
+	git_default_config();

 	argc = parse_options(argc, argv, prefix, check_attr_options,
 			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 594463a..c14c977 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -144,7 +144,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
 	int num_ignored;
 	struct dir_struct dir;

-	git_config(git_default_config, NULL);
+	git_default_config();

 	argc = parse_options(argc, argv, prefix, check_ignore_options,
 			     check_ignore_usage, 0);
diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index 8f4d809..f9d0de6 100644
--- a/builtin/check-mailmap.c
+++ b/builtin/check-mailmap.c
@@ -40,7 +40,7 @@ int cmd_check_mailmap(int argc, const char **argv, const char *prefix)
 	int i;
 	struct string_list mailmap = STRING_LIST_INIT_NODUP;

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, check_mailmap_options,
 			     check_mailmap_usage, 0);
 	if (argc == 0 && !use_stdin)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 05edd9e..197a987 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -213,7 +213,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_checkout_index_usage,
 				   builtin_checkout_index_options);
-	git_config(git_default_config, NULL);
+	git_default_config();
 	state.base_dir = "";
 	prefix_length = prefix ? strlen(prefix) : 0;

diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..bcfd322 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -864,7 +864,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	init_db(option_template, INIT_DB_QUIET);
 	write_config(&option_config);

-	git_config(git_default_config, NULL);
+	git_default_config();

 	if (option_bare) {
 		if (option_mirror)
diff --git a/builtin/config.c b/builtin/config.c
index fcd8474..eed430d 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -558,7 +558,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			die("editing stdin is not supported");
 		if (given_config_source.blob)
 			die("editing blobs is not supported");
-		git_config(git_default_config, NULL);
+		git_default_config();
 		launch_editor(given_config_source.file ?
 			      given_config_source.file : git_path("config"),
 			      NULL, NULL);
diff --git a/builtin/describe.c b/builtin/describe.c
index ee6a3b9..a4969d8 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -413,7 +413,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, options, describe_usage, 0);
 	if (abbrev < 0)
 		abbrev = DEFAULT_ABBREV;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 92b4624..344a8a4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -726,7 +726,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 		usage_with_options (fast_export_usage, options);

 	/* we handle encodings */
-	git_config(git_default_config, NULL);
+	git_default_config();

 	init_revisions(&revs, prefix);
 	revs.topo_order = 1;
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 47bd624..3991679 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -1095,7 +1095,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		sort = default_sort();

 	/* for warn_ambiguous_refs */
-	git_config(git_default_config, NULL);
+	git_default_config();

 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.grab_pattern = argv;
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index d7fcf4c..0a9fe1b 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -96,7 +96,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 			vpath = prefix_filename(prefix, prefix_length, vpath);
 	}

-	git_config(git_default_config, NULL);
+	git_default_config();

 	if (stdin_paths) {
 		if (hashstdin)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..337cd0a 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -211,7 +211,7 @@ static int create_default_files(const char *template_path)
 	 */
 	copy_templates(template_path);

-	git_config(git_default_config, NULL);
+	git_default_config();
 	is_bare_repository_cfg = init_is_bare_repository;

 	/* reading existing config may have overwrote it */
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 47c3880..18d4241 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -513,7 +513,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
-	git_config(git_default_config, NULL);
+	git_default_config();

 	if (read_cache() < 0)
 		die("index file corrupt");
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 51184df..814b358 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -146,7 +146,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	ls_tree_prefix = prefix;
 	if (prefix && *prefix)
 		chomp_prefix = strlen(prefix);
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 0ecde8d..c442e49 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -223,7 +223,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, options, merge_base_usage, 0);

 	if (cmdmode == 'a') {
diff --git a/builtin/mv.c b/builtin/mv.c
index 6ffe540..ea5da87 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -78,7 +78,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;

 	gitmodules_config();
-	git_config(git_default_config, NULL);
+	git_default_config();

 	argc = parse_options(argc, argv, prefix, builtin_mv_options,
 			     builtin_mv_usage, 0);
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 3c8f319..fdf9771 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -325,7 +325,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
 	if (all + transform_stdin + !!argc > 1) {
 		error("Specify either a list, or --all, not both!");
diff --git a/builtin/notes.c b/builtin/notes.c
index 820c341..47cec8a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -944,7 +944,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, options, git_notes_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);

diff --git a/builtin/push.c b/builtin/push.c
index f50e3d5..a25fc00 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -510,7 +510,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};

 	packet_trace_identity("push");
-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);

 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index e7e1c33..40d41fa 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -144,7 +144,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;

-	git_config(git_default_config, NULL);
+	git_default_config();

 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
diff --git a/builtin/reset.c b/builtin/reset.c
index 855d478..3f5c0c0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -288,7 +288,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();

 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..3243fcd 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -280,7 +280,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	int bisect_find_all = 0;
 	int use_bitmap_index = 0;

-	git_config(git_default_config, NULL);
+	git_default_config();
 	init_revisions(&revs, prefix);
 	revs.abbrev = DEFAULT_ABBREV;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index d85e08c..0465540 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -527,7 +527,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	}

 	prefix = setup_git_directory();
-	git_config(git_default_config, NULL);
+	git_default_config();
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];

diff --git a/builtin/revert.c b/builtin/revert.c
index f9ed5bd..1975f46 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -190,7 +190,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	if (isatty(0))
 		opts.edit = 1;
 	opts.action = REPLAY_REVERT;
-	git_config(git_default_config, NULL);
+	git_default_config();
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
@@ -205,7 +205,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)

 	memset(&opts, 0, sizeof(opts));
 	opts.action = REPLAY_PICK;
-	git_config(git_default_config, NULL);
+	git_default_config();
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
diff --git a/builtin/rm.c b/builtin/rm.c
index bc6490b..7e4c2c6 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -283,7 +283,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	char *seen;

 	gitmodules_config();
-	git_config(git_default_config, NULL);
+	git_default_config();

 	argc = parse_options(argc, argv, prefix, builtin_rm_options,
 			     builtin_rm_usage, 0);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 4b7e536..9f4627b 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -239,7 +239,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)

 	struct parse_opt_ctx_t ctx;

-	git_config(git_default_config, NULL);
+	git_default_config();
 	shortlog_init(&log);
 	init_revisions(&rev, prefix);
 	parse_options_start(&ctx, argc, argv, prefix, options,
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 1259ed7..27cd0bb 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -105,7 +105,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 		usage(usage_msg);

 	if (strip_comments || mode == COMMENT_LINES)
-		git_config(git_default_config, NULL);
+		git_default_config();

 	if (strbuf_read(&buf, 0, 1024) < 0)
 		die_errno("could not read the input");
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index b6a711d..c6abff3 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -44,7 +44,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, options,
 			     git_symbolic_ref_usage, 0);
 	if (msg && !*msg)
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 1920029..47eba5f 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -29,7 +29,7 @@ int cmd_unpack_file(int argc, const char **argv, const char *prefix)
 	if (get_sha1(argv[1], sha1))
 		die("Not a valid object name %s", argv[1]);

-	git_config(git_default_config, NULL);
+	git_default_config();

 	puts(create_temp_file(sha1));
 	return 0;
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 99cde45..970056b 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -499,7 +499,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)

 	check_replace_refs = 0;

-	git_config(git_default_config, NULL);
+	git_default_config();

 	quiet = !isatty(2);

diff --git a/builtin/update-index.c b/builtin/update-index.c
index e8c7fd4..0a5fff9 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -837,7 +837,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(update_index_usage, options);

-	git_config(git_default_config, NULL);
+	git_default_config();

 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
 	lock_file = xcalloc(1, sizeof(struct lock_file));
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3067b11..e12edfe 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -355,7 +355,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, options, git_update_ref_usage,
 			     0);
 	if (msg && !*msg)
diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c
index 6c8cc3e..ecf791c 100644
--- a/builtin/update-server-info.c
+++ b/builtin/update-server-info.c
@@ -15,7 +15,7 @@ int cmd_update_server_info(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, options,
 			     update_server_info_usage, 0);
 	if (argc > 0)
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..d4b7dcd 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -82,7 +82,7 @@ int cmd_var(int argc, const char **argv, const char *prefix)
 		list_vars();
 		return 0;
 	}
-	git_config(git_default_config, NULL);
+	git_default_config();
 	val = read_var(argv[1]);
 	if (!val)
 		usage(var_usage);
diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c
index 972579f..c017efd 100644
--- a/builtin/verify-pack.c
+++ b/builtin/verify-pack.c
@@ -69,7 +69,7 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, prefix, verify_pack_options,
 			     verify_pack_usage, 0);
 	if (argc < 1)
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 084c0df..95a0ca4 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -33,7 +33,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 		OPT_END()
 	};

-	git_config(git_default_config, NULL);
+	git_default_config();
 	argc = parse_options(argc, argv, unused_prefix, write_tree_options,
 			     write_tree_usage, 0);

diff --git a/http-fetch.c b/http-fetch.c
index ba3ea10..afa9f40 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -67,7 +67,7 @@ int main(int argc, const char **argv)

 	setup_git_directory();

-	git_config(git_default_config, NULL);
+	git_default_config();

 	http_init(NULL, url, 0);
 	walker = get_http_walker(url);
diff --git a/pager.c b/pager.c
index 8b5cbc5..c22fc04 100644
--- a/pager.c
+++ b/pager.c
@@ -47,7 +47,7 @@ const char *git_pager(int stdout_is_tty)
 	pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
-			git_config(git_default_config, NULL);
+			git_default_config();
 		pager = pager_program;
 	}
 	if (!pager)
-- 
1.9.0.GIT

Re: [PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> git_default_config() now uses config-set API functions to query for
> values.

I believe you missed a few spots:

$ git grep -n 'git_default_config[^(]'
Documentation/user-manual.txt:4287:        git_config(git_default_config);
archive.c:416:  git_config(git_default_config, NULL);
builtin/config.c:577:           git_config(git_default_config, NULL);
color.h:73: * if you are just going to change to git_default_config, too.
fetch-pack.c:880:       git_config(git_default_config, NULL);
http.c:393:     config.cascade_fn = git_default_config;
rerere.c:580:   git_config(git_default_config, NULL);
rerere.c:710:   git_config(git_default_config, NULL);

The following ones should probably be rewritten too:

archive.c:416:  git_config(git_default_config, NULL);
builtin/config.c:577:           git_config(git_default_config, NULL);
fetch-pack.c:880:       git_config(git_default_config, NULL);
rerere.c:580:   git_config(git_default_config, NULL);
rerere.c:710:   git_config(git_default_config, NULL);

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

Re: [PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> git_default_config() now uses config-set API functions to query for
>> values.
>
> I believe you missed a few spots:
>
> $ git grep -n 'git_default_config[^(]'
> Documentation/user-manual.txt:4287:        git_config(git_default_config);
> archive.c:416:  git_config(git_default_config, NULL);
> builtin/config.c:577:           git_config(git_default_config, NULL);
> color.h:73: * if you are just going to change to git_default_config, too.
> fetch-pack.c:880:       git_config(git_default_config, NULL);
> http.c:393:     config.cascade_fn = git_default_config;
> rerere.c:580:   git_config(git_default_config, NULL);
> rerere.c:710:   git_config(git_default_config, NULL);
>
> The following ones should probably be rewritten too:
>
> archive.c:416:  git_config(git_default_config, NULL);
> builtin/config.c:577:           git_config(git_default_config, NULL);
> fetch-pack.c:880:       git_config(git_default_config, NULL);
> rerere.c:580:   git_config(git_default_config, NULL);
> rerere.c:710:   git_config(git_default_config, NULL);

For a one-person toy project it is OK to repurpose the existing
git_default_config() to do completely different thing and make it a
flag day to switch the entire codebase, but in a collaborative
environment where there may be multiple topics in flight, some of
which may be happening where you are not even aware of, it is better
to remove the existing git_default_config() and use a different name
for the different function you are introducing, to force new places
that expect the old git_default_config() to work as before to be
noticed with a linkage error.

Re: [PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> git_default_config() now uses config-set API functions to query for
> values.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
> Sorry, for the short log message, I will explain why.
> The git_default_config() rewrite is 100% complete, the only
> problem remains is the call sites; there are too many of them.
> Some are called from callback functions which pass the remaining
> variables to git_default_config() which they do not have any use for.
> Those call sites can remain as they are, because git_default_config()
> is a single call function now, and is guarded by a sentinel value.

They can remain as they are, but it would also be relatively easy to
turn them into non-callback style by doing something like this on each
call:

--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -37,7 +37,7 @@ static int commit_tree_config(const char *var, const char *value, void *cb)
                sign_commit = git_config_bool(var, value) ? "" : NULL;
                return 0;
        }
-       return git_default_config(var, value, cb);
+       return 0
 }
 
 int cmd_commit_tree(int argc, const char **argv, const char *prefix)
@@ -49,6 +49,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
        struct strbuf buffer = STRBUF_INIT;
 
        git_config(commit_tree_config, NULL);
+       git_default_config();
 
        if (argc < 2 || !strcmp(argv[1], "-h"))
                usage(commit_tree_usage);

> -int git_default_config(const char *var, const char *value, void *dummy)
> +int git_default_config(const char *unused, const char *unused2, void *dummy)

By having these dummy arguments, you force callers to pass dummy actual
parameters.

Actually, you don't pass anything in PATCH 2, hence the result is not
compilable:

http-fetch.c:70:2: error: too few arguments to function ‘git_default_config’
  git_default_config();
  ^
In file included from http-fetch.c:1:0:
cache.h:1299:12: note: declared here
 extern int git_default_config(const char *, const char *, void *);

After your patch, there are two things git_default_config do:

1) normal callers want to call git_default_config();

2) callback-style callers want to write
   return git_default_config(var, value, cb);

I think this deserves two functions, calling each others:

/* For 1) */
void git_load_default_config(void)
{
	do the actual stuff
}

/* For 2) */
int git_default_config(const char *unused, const char *unused2, void *dummy)
{
	if (default_config_loaded)
		return;
	git_load_default_config(NULL, NULL, NULL);
	default_config_loaded = 1;
	return 0;
}

In an ideal world, git_default_config would disappear after the rewrite
is completed. In practice, it may stay if needed, it doesn't harm
anyone.

PATCH 2 would turn git_config(git_default_config, NULL); into
git_load_default_config().

> @@ -2082,6 +1977,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
>
>  	/* Invalidate the config cache */
>  	git_config_clear();
> +	default_config_loaded = 0;

What about the other callsite in setup.c? We may have left the
configuration half-loaded, and if anyone calls git_load_default_config()
again after that, we do want to reload it, don't we?

Which leads to another question: why not put this default_config_loaded
= 0; inside git_config_clear(), to avoid forgetting?

> index 1d9b6e7..0db595f 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -392,29 +392,26 @@ int author_ident_sufficiently_given(void)
>  	return ident_is_sufficient(author_ident_explicitly_given);
>  }
>
> -int git_ident_config(const char *var, const char *value, void *data)
> +void git_ident_config(void)
>  {
> -	if (!strcmp(var, "user.name")) {
> +	const char *value = NULL;
> +
> +	if (!git_config_get_value("user.name", &value)) {
>  		if (!value)
> -			return config_error_nonbool(var);
> +			git_die_config("user.name", "Missing value for 'user.name'");

I'd rather have git_config_get_string() and a free() afterwards to avoid
duplicating this "Missing value for 'user.name'" (which should be _()-ed
if it stays).

> -
> -	if (!strcmp(var, "user.email")) {
> +	if (!git_config_get_value("user.email", &value)) {
>  		if (!value)
> -			return config_error_nonbool(var);
> +			git_die_config("user.email", "Missing value for 'user.email'");

Likewise.

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

Re: [PATCH 1/4] fast-import.c: replace `git_config()` with `git_config_get_*()` family

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Not that it's terribly important, but I think it's good that your
> refactoring also brings a few end-users benefits. It will help you show
> off when you tell your friends what you did this summer (not "I did
> useless code churn" ;-) ), and helps everybody see the benefits of your
> GSoC ;-).

;-)