fast-import.c: replace `git_config()` with `git_config_get_*()` family

PATCH In review Tanay Abhra 3 participants 19 messages Jun 15, 2016

Share & tools

Tanay Abhra

author

2016-06-15 23:02 UTC #0 lore raw

PATCH 1/4 fast-import.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 <[email protected]>

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

fast-import.c +19 ~~-23~~

index d73f58c..eca5ed4 100644
@@ -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;

Signature · 1 line

1.9.0.GIT

Tanay Abhra

author

2016-06-15 23:02 UTC #1 lore raw

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

Signed-off-by Tanay Abhra <[email protected]>

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

ll-merge.c +6 ~~-17~~

index fb61ea6..8ea03e5 100644
@@ -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;
 }

Signature · 1 line

1.9.0.GIT

Matthieu Moy

2016-06-15 23:02 UTC #2 lore raw

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

Quoted 10 lines · Tanay Abhra

Signed-off-by Tanay Abhra <[email protected]>
---
 ll-merge.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)
ll-merge.c +0 -0
index fb61ea6..8ea03e5 100644

ll-merge.c static int read_merge_config(const char *var, const char *value, void *cb) · line 229

Quoted 10 lines

ll-merge.c +2 -5

@@ -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.

Signature · 2 lines

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

Tanay Abhra

author

2016-06-15 23:02 UTC #3 lore raw

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

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 <[email protected]>

---
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(-)

ll-merge.c +6 ~~-17~~

index fb61ea6..8ea03e5 100644
@@ -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;
 }

Signature · 1 line

1.9.0.GIT

Matthieu Moy

2016-06-15 23:02 UTC #4 lore raw

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

Quoted 5 lines · 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.

Good, thanks.

Quoted 6 lines

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.

Signature · 2 lines

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

Junio C Hamano

2016-06-15 23:02 UTC #5 lore raw

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

In reply to Tanay Abhra

Quoted 5 lines · 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.

Better explained than v1 ;-)

Quoted 8 lines

Signed-off-by Tanay Abhra <[email protected]>
---
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>.*".

Quoted 51 lines

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

ll-merge.c +6 ~~-17~~

index fb61ea6..8ea03e5 100644
@@ -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;
 }

Tanay Abhra

author

2016-06-15 23:02 UTC #6 lore raw

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

In reply to 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 <[email protected]>

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

merge-recursive.c +6 ~~-16~~

index 1d332b8..8ab944c 100644
@@ -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);

Signature · 1 line

1.9.0.GIT

Matthieu Moy

2016-06-15 23:02 UTC #7 lore raw

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

Quoted 5 lines · Tanay Abhra

 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.

Signature · 2 lines

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

Tanay Abhra

author

2016-06-15 23:02 UTC #8 lore raw

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

In reply to 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 <[email protected]>

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

builtin/apply.c +5 -7

index be2b4ce..66acf32 100644
@@ -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)

Signature · 1 line

1.9.0.GIT

Matthieu Moy

2016-06-15 23:02 UTC #9 lore raw

In reply to Tanay Abhra

Quoted 2 lines · Tanay Abhra

 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.

fast-import.c static void parse_option(const char *option) · line 3291

Quoted 6 lines

fast-import.c +6 -0

+	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?

fast-import.c static void parse_option(const char *option) · line 3295

Quoted 4 lines

fast-import.c +2 -2

-	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.

fast-import.c static void parse_option(const char *option) · line 3297

Quoted 5 lines

fast-import.c +1 -3

 		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 ;-).

Signature · 2 lines

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

Tanay Abhra

author

2016-06-15 23:02 UTC #10 lore raw

Quoted 8 lines

fast-import.c static void parse_option(const char *option) · line 3297
Quoted 5 lines
fast-import.c +1 -3
 		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.

Quoted 2 lines

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.

Quoted 4 lines

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 ;-).

Tanay Abhra

author

2016-06-15 23:02 UTC #11 lore raw

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

In reply to Matthieu Moy

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 <[email protected]>

---
 fast-import.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

fast-import.c +21 ~~-23~~

index d73f58c..34e780d 100644
@@ -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;

Signature · 1 line

1.9.0.GIT

Matthieu Moy

2016-06-15 23:02 UTC #12 lore raw

fast-import.c static void parse_option(const char *option) · line 3292

Quoted 7 lines · Tanay Abhra

fast-import.c +7 -0

+	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.

Signature · 2 lines

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

Tanay Abhra

author

2016-06-15 23:02 UTC #13 lore raw

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

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

Signed-off-by Tanay Abhra <[email protected]>

---
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(-)

advice.c +6 ~~-11~~

index 9b42033..34e1ccc 100644
@@ -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)

advice.h +1 -1

index 5ecc6c1..5bfe46c 100644
@@ -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);

cache.h +1 -1

index 2693a37..fa28b40 100644
@@ -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;

config.c +121 ~~-225~~

465 lines not on this page · raw message

ident.c +7 ~~-10~~

index 1d9b6e7..0db595f 100644
@@ -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,

Signature · 1 line

1.9.0.GIT

Tanay Abhra

author

2016-06-15 23:02 UTC #14 lore raw

PATCH v2 2/2 use the new git_default_config()

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(-)

builtin/check-attr.c +1 -1

index 5600ec3..e2d7826 100644
@@ -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);

builtin/check-ignore.c +1 -1

index 594463a..c14c977 100644
@@ -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);

builtin/check-mailmap.c +1 -1

index 8f4d809..f9d0de6 100644
@@ -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)

builtin/checkout-index.c +1 -1

index 05edd9e..197a987 100644
@@ -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;

builtin/clone.c +1 -1

index bbd169c..bcfd322 100644
@@ -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)

builtin/config.c +1 -1

index fcd8474..eed430d 100644
@@ -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);

builtin/describe.c +1 -1

index ee6a3b9..a4969d8 100644
@@ -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;

builtin/fast-export.c +1 -1

index 92b4624..344a8a4 100644
@@ -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;

builtin/for-each-ref.c +1 -1

index 47bd624..3991679 100644
@@ -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;

builtin/hash-object.c +1 -1

index d7fcf4c..0a9fe1b 100644
@@ -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)

builtin/init-db.c +1 -1

index 56f85e2..337cd0a 100644
@@ -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 */

builtin/ls-files.c +1 -1

index 47c3880..18d4241 100644
@@ -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");

builtin/ls-tree.c +1 -1

index 51184df..814b358 100644
@@ -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);

builtin/merge-base.c +1 -1

index 0ecde8d..c442e49 100644
@@ -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') {

builtin/mv.c +1 -1

index 6ffe540..ea5da87 100644
@@ -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);

builtin/name-rev.c +1 -1

index 3c8f319..fdf9771 100644
@@ -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!");

builtin/notes.c +1 -1

index 820c341..47cec8a 100644
@@ -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);

builtin/push.c +1 -1

index f50e3d5..a25fc00 100644
@@ -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))))

builtin/read-tree.c +1 -1

index e7e1c33..40d41fa 100644
@@ -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);

builtin/reset.c +1 -1

index 855d478..3f5c0c0 100644
@@ -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);

builtin/rev-list.c +1 -1

index ff84a82..3243fcd 100644
@@ -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;

builtin/rev-parse.c +1 -1

index d85e08c..0465540 100644
@@ -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];

builtin/revert.c +2 -2

index f9ed5bd..1975f46 100644
@@ -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)

builtin/rm.c +1 -1

index bc6490b..7e4c2c6 100644
@@ -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);

builtin/shortlog.c +1 -1

index 4b7e536..9f4627b 100644
@@ -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,

builtin/stripspace.c +1 -1

index 1259ed7..27cd0bb 100644
@@ -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");

builtin/symbolic-ref.c +1 -1

index b6a711d..c6abff3 100644
@@ -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)

builtin/unpack-file.c +1 -1

index 1920029..47eba5f 100644
@@ -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;

builtin/unpack-objects.c +1 -1

index 99cde45..970056b 100644
@@ -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);

builtin/update-index.c +1 -1

index e8c7fd4..0a5fff9 100644
@@ -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));

builtin/update-ref.c +1 -1

index 3067b11..e12edfe 100644
@@ -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)

builtin/update-server-info.c +1 -1

index 6c8cc3e..ecf791c 100644
@@ -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)

builtin/var.c +1 -1

index aedbb53..d4b7dcd 100644
@@ -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);

builtin/verify-pack.c +1 -1

index 972579f..c017efd 100644
@@ -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)

builtin/write-tree.c +1 -1

index 084c0df..95a0ca4 100644
@@ -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);

http-fetch.c +1 -1

index ba3ea10..afa9f40 100644
@@ -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);

pager.c +1 -1

index 8b5cbc5..c22fc04 100644
@@ -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)

Signature · 1 line

1.9.0.GIT

Matthieu Moy

2016-06-15 23:02 UTC #15 lore raw

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

In reply to Tanay Abhra

Quoted 2 lines · Tanay Abhra

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);

Signature · 2 lines

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

Junio C Hamano

2016-06-15 23:02 UTC #16 lore raw

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

Quoted 17 lines · Matthieu Moy

Quoted 2 lines · Tanay Abhra

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);

Quoted 7 lines

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.

Matthieu Moy

2016-06-15 23:02 UTC #17 lore raw

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

In reply to Tanay Abhra

Quoted 4 lines · Tanay Abhra

git_default_config() now uses config-set API functions to query for values.
Signed-off-by Tanay Abhra <[email protected]>

Quoted 8 lines

---
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:

builtin/commit-tree.c +2 -1

@@ -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);

Review

Quoted 2 lines

(quoted diff) +1 -1

-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().

Review int git_config_set_multivar_in_file(const char *config_filename, · line 1980

Quoted 5 lines

(quoted diff) +1 -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;

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?

Quoted 7 lines

index 1d9b6e7..0db595f 100644

ident.c +0 -0

@@ -392,29 +392,26 @@ int author_ident_sufficiently_given(void)
 	return ident_is_sufficient(author_ident_explicitly_given);
 }

ident.c int author_ident_sufficiently_given(void) · line 401

Quoted 10 lines

ident.c +5 -3

-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).

ident.c int author_ident_sufficiently_given(void) · line 404

Quoted 6 lines

ident.c +2 -3

-
-	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.

Signature · 2 lines

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

Junio C Hamano

2016-06-15 23:02 UTC #18 lore raw

In reply to Matthieu Moy

Quoted 5 lines · Matthieu Moy

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 ;-).

;-)