[PATCH 1/8] config: Trivial rename in preparation for parseopt.

[PATCH 1/8] config: Trivial rename in preparation for parseopt.

[Felipe Contreras]

As suggested by Johannes Schindelin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 6937eaf..bf8df58 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -178,6 +178,7 @@ static char *normalize_value(const char *key, const char *value)
 
 static int get_color_found;
 static const char *get_color_slot;
+static const char *get_colorbool_slot;
 static char parsed_color[COLOR_MAXLEN];
 
 static int git_get_color_config(const char *var, const char *value, void *cb)
@@ -231,7 +232,7 @@ static int get_diff_color_found;
 static int git_get_colorbool_config(const char *var, const char *value,
 		void *cb)
 {
-	if (!strcmp(var, get_color_slot)) {
+	if (!strcmp(var, get_colorbool_slot)) {
 		get_colorbool_found =
 			git_config_colorbool(var, value, stdout_is_tty);
 	}
@@ -263,11 +264,11 @@ static int get_colorbool(int argc, const char **argv)
 		usage(git_config_set_usage);
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
-	get_color_slot = argv[0];
+	get_colorbool_slot = argv[0];
 	git_config(git_get_colorbool_config, NULL);
 
 	if (get_colorbool_found < 0) {
-		if (!strcmp(get_color_slot, "color.diff"))
+		if (!strcmp(get_colorbool_slot, "color.diff"))
 			get_colorbool_found = get_diff_color_found;
 		if (get_colorbool_found < 0)
 			get_colorbool_found = git_use_color_default;
-- 
1.6.1.3

[PATCH 2/8] config: Cleanup config file handling.

[Felipe Contreras]

As suggested by Johannes Schindelin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index bf8df58..da754e0 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -282,11 +282,11 @@ static int get_colorbool(int argc, const char **argv)
 	}
 }
 
-int cmd_config(int argc, const char **argv, const char *prefix)
+int cmd_config(int argc, const char **argv, const char *unused_prefix)
 {
 	int nongit;
 	char* value;
-	const char *file = setup_git_directory_gently(&nongit);
+	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
 
@@ -300,10 +300,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) {
 			if (argc != 2)
 				usage(git_config_set_usage);
-			if (git_config(show_all_config, NULL) < 0 &&
-					file && errno)
-				die("unable to read config file %s: %s", file,
-				    strerror(errno));
+			if (git_config(show_all_config, NULL) < 0)
+				die("error processing config file(s)");
 			return 0;
 		}
 		else if (!strcmp(argv[1], "--global")) {
@@ -320,12 +318,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "--file") || !strcmp(argv[1], "-f")) {
 			if (argc < 3)
 				usage(git_config_set_usage);
-			if (!is_absolute_path(argv[2]) && file)
-				file = prefix_filename(file, strlen(file),
-						       argv[2]);
+			if (!is_absolute_path(argv[2]) && prefix)
+				config_exclusive_filename = prefix_filename(prefix,
+									    strlen(prefix),
+									    argv[2]);
 			else
-				file = argv[2];
-			config_exclusive_filename = file;
+				config_exclusive_filename = argv[2];
 			argc--;
 			argv++;
 		}
-- 
1.6.1.3

[PATCH 3/8] config: Use parseopt.

[Felipe Contreras]

Reorganizing the code to use parseopt as suggested by Johannes
Schindelin.

This patch has benefited from comments by Johannes
Schindelin and Junio C Hamano.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |  362 ++++++++++++++++++++++++++---------------------------
 1 files changed, 178 insertions(+), 184 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index da754e0..084222a 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -1,9 +1,12 @@
 #include "builtin.h"
 #include "cache.h"
 #include "color.h"
+#include "parse-options.h"
 
-static const char git_config_set_usage[] =
-"git config [ --global | --system | [ -f | --file ] config-file ] [ --bool | --int | --bool-or-int ] [ -z | --null ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list | --get-color var [default] | --get-colorbool name [stdout-is-tty] | --edit | -e ]";
+static const char *const builtin_config_usage[] = {
+	"git config [options]",
+	NULL
+};
 
 static char *key;
 static regex_t *key_regexp;
@@ -18,6 +21,59 @@ static char key_delim = ' ';
 static char term = '\n';
 static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;
 
+static int use_global_config, use_system_config;
+static const char *given_config_file;
+static int actions;
+static const char *get_color_slot, *get_colorbool_slot;
+static int end_null;
+
+#define ACTION_GET (1<<0)
+#define ACTION_GET_ALL (1<<1)
+#define ACTION_GET_REGEXP (1<<2)
+#define ACTION_REPLACE_ALL (1<<3)
+#define ACTION_ADD (1<<4)
+#define ACTION_UNSET (1<<5)
+#define ACTION_UNSET_ALL (1<<6)
+#define ACTION_RENAME_SECTION (1<<7)
+#define ACTION_REMOVE_SECTION (1<<8)
+#define ACTION_LIST (1<<9)
+#define ACTION_EDIT (1<<10)
+
+static struct option builtin_config_options[] = {
+	OPT_GROUP("Config file location"),
+	OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"),
+	OPT_BOOLEAN(0, "system", &use_system_config, "use system config file"),
+	OPT_STRING('f', "file", &given_config_file, "FILE", "use given config file"),
+	OPT_GROUP("Action"),
+	OPT_BIT(0, "get", &actions, "get value: name [value-regex]", ACTION_GET),
+	OPT_BIT(0, "get-all", &actions, "get all values: key [value-regex]", ACTION_GET),
+	OPT_BIT(0, "get-regexp", &actions, "get values for regexp: name-regex [value-regex]", ACTION_GET_REGEXP),
+	OPT_BIT(0, "replace-all", &actions, "replace all matching variables: name [value [value_regex]", ACTION_REPLACE_ALL),
+	OPT_BIT(0, "add", &actions, "adds a new variable: name value", ACTION_ADD),
+	OPT_BIT(0, "unset", &actions, "removes a variable: name [value-regex]", ACTION_UNSET),
+	OPT_BIT(0, "unset-all", &actions, "removes all matches: name [value-regex]", ACTION_UNSET_ALL),
+	OPT_BIT(0, "rename-section", &actions, "rename section: old-name new-name", ACTION_RENAME_SECTION),
+	OPT_BIT(0, "remove-section", &actions, "remove a section: name", ACTION_REMOVE_SECTION),
+	OPT_BIT('l', "list", &actions, "list all", ACTION_LIST),
+	OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT),
+	OPT_GROUP("Type"),
+	OPT_SET_INT(0, "bool", &type, "value is \"true\" or \"false\"", T_BOOL),
+	OPT_SET_INT(0, "int", &type, "value is decimal number", T_INT),
+	OPT_SET_INT(0, "bool-or-int", &type, NULL, T_BOOL_OR_INT),
+	OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"),
+	OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"),
+	OPT_GROUP("Other"),
+	OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
+	OPT_END(),
+};
+
+static void check_argc(int argc, int min, int max) {
+	if (argc >= min && argc <= max)
+		return;
+	error("wrong number of arguments");
+	usage_with_options(builtin_config_usage, builtin_config_options);
+}
+
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
 	if (value_)
@@ -180,7 +236,6 @@ static int get_color_found;
 static const char *get_color_slot;
 static const char *get_colorbool_slot;
 static char parsed_color[COLOR_MAXLEN];
-
 static int git_get_color_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, get_color_slot)) {
@@ -192,29 +247,8 @@ static int git_get_color_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
-static int get_color(int argc, const char **argv)
+static void get_color(const char *def_color)
 {
-	/*
-	 * grab the color setting for the given slot from the configuration,
-	 * or parse the default value if missing, and return ANSI color
-	 * escape sequence.
-	 *
-	 * e.g.
-	 * git config --get-color color.diff.whitespace "blue reverse"
-	 */
-	const char *def_color = NULL;
-
-	switch (argc) {
-	default:
-		usage(git_config_set_usage);
-	case 2:
-		def_color = argv[1];
-		/* fallthru */
-	case 1:
-		get_color_slot = argv[0];
-		break;
-	}
-
 	get_color_found = 0;
 	parsed_color[0] = '\0';
 	git_config(git_get_color_config, NULL);
@@ -223,7 +257,6 @@ static int get_color(int argc, const char **argv)
 		color_parse(def_color, "command line", parsed_color);
 
 	fputs(parsed_color, stdout);
-	return 0;
 }
 
 static int stdout_is_tty;
@@ -247,24 +280,10 @@ static int git_get_colorbool_config(const char *var, const char *value,
 	return 0;
 }
 
-static int get_colorbool(int argc, const char **argv)
+static int get_colorbool(int print)
 {
-	/*
-	 * git config --get-colorbool <slot> [<stdout-is-tty>]
-	 *
-	 * returns "true" or "false" depending on how <slot>
-	 * is configured.
-	 */
-
-	if (argc == 2)
-		stdout_is_tty = git_config_bool("command line", argv[1]);
-	else if (argc == 1)
-		stdout_is_tty = isatty(1);
-	else
-		usage(git_config_set_usage);
 	get_colorbool_found = -1;
 	get_diff_color_found = -1;
-	get_colorbool_slot = argv[0];
 	git_config(git_get_colorbool_config, NULL);
 
 	if (get_colorbool_found < 0) {
@@ -274,12 +293,11 @@ static int get_colorbool(int argc, const char **argv)
 			get_colorbool_found = git_use_color_default;
 	}
 
-	if (argc == 1) {
-		return get_colorbool_found ? 0 : 1;
-	} else {
+	if (print) {
 		printf("%s\n", get_colorbool_found ? "true" : "false");
 		return 0;
-	}
+	} else
+		return get_colorbool_found ? 0 : 1;
 }
 
 int cmd_config(int argc, const char **argv, const char *unused_prefix)
@@ -290,149 +308,125 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
 
-	while (1 < argc) {
-		if (!strcmp(argv[1], "--int"))
-			type = T_INT;
-		else if (!strcmp(argv[1], "--bool"))
-			type = T_BOOL;
-		else if (!strcmp(argv[1], "--bool-or-int"))
-			type = T_BOOL_OR_INT;
-		else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) {
-			if (argc != 2)
-				usage(git_config_set_usage);
-			if (git_config(show_all_config, NULL) < 0)
-				die("error processing config file(s)");
-			return 0;
-		}
-		else if (!strcmp(argv[1], "--global")) {
-			char *home = getenv("HOME");
-			if (home) {
-				char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
-				config_exclusive_filename = user_config;
-			} else {
-				die("$HOME not set");
-			}
-		}
-		else if (!strcmp(argv[1], "--system"))
-			config_exclusive_filename = git_etc_gitconfig();
-		else if (!strcmp(argv[1], "--file") || !strcmp(argv[1], "-f")) {
-			if (argc < 3)
-				usage(git_config_set_usage);
-			if (!is_absolute_path(argv[2]) && prefix)
-				config_exclusive_filename = prefix_filename(prefix,
-									    strlen(prefix),
-									    argv[2]);
-			else
-				config_exclusive_filename = argv[2];
-			argc--;
-			argv++;
-		}
-		else if (!strcmp(argv[1], "--null") || !strcmp(argv[1], "-z")) {
-			term = '\0';
-			delim = '\n';
-			key_delim = '\n';
-		}
-		else if (!strcmp(argv[1], "--rename-section")) {
-			int ret;
-			if (argc != 4)
-				usage(git_config_set_usage);
-			ret = git_config_rename_section(argv[2], argv[3]);
-			if (ret < 0)
-				return ret;
-			if (ret == 0) {
-				fprintf(stderr, "No such section!\n");
-				return 1;
-			}
-			return 0;
-		}
-		else if (!strcmp(argv[1], "--remove-section")) {
-			int ret;
-			if (argc != 3)
-				usage(git_config_set_usage);
-			ret = git_config_rename_section(argv[2], NULL);
-			if (ret < 0)
-				return ret;
-			if (ret == 0) {
-				fprintf(stderr, "No such section!\n");
-				return 1;
-			}
-			return 0;
-		} else if (!strcmp(argv[1], "--get-color")) {
-			return get_color(argc-2, argv+2);
-		} else if (!strcmp(argv[1], "--get-colorbool")) {
-			return get_colorbool(argc-2, argv+2);
-		} else if (!strcmp(argv[1], "--edit") || !strcmp(argv[1], "-e")) {
-			const char *config_filename;
-			if (argc != 2)
-				usage(git_config_set_usage);
-			if (config_exclusive_filename)
-				config_filename = config_exclusive_filename;
-			else
-				config_filename = git_path("config");
-			git_config(git_default_config, NULL);
-			launch_editor(config_filename, NULL, NULL);
-			return 0;
-		} else
-			break;
-		argc--;
-		argv++;
-	}
+	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
 
-	switch (argc) {
-	case 2:
-		return get_value(argv[1], NULL);
-	case 3:
-		if (!strcmp(argv[1], "--unset"))
-			return git_config_set(argv[2], NULL);
-		else if (!strcmp(argv[1], "--unset-all"))
-			return git_config_set_multivar(argv[2], NULL, NULL, 1);
-		else if (!strcmp(argv[1], "--get"))
-			return get_value(argv[2], NULL);
-		else if (!strcmp(argv[1], "--get-all")) {
-			do_all = 1;
-			return get_value(argv[2], NULL);
-		} else if (!strcmp(argv[1], "--get-regexp")) {
-			show_keys = 1;
-			use_key_regexp = 1;
-			do_all = 1;
-			return get_value(argv[2], NULL);
-		} else {
-			value = normalize_value(argv[1], argv[2]);
-			return git_config_set(argv[1], value);
-		}
-	case 4:
-		if (!strcmp(argv[1], "--unset"))
-			return git_config_set_multivar(argv[2], NULL, argv[3], 0);
-		else if (!strcmp(argv[1], "--unset-all"))
-			return git_config_set_multivar(argv[2], NULL, argv[3], 1);
-		else if (!strcmp(argv[1], "--get"))
-			return get_value(argv[2], argv[3]);
-		else if (!strcmp(argv[1], "--get-all")) {
-			do_all = 1;
-			return get_value(argv[2], argv[3]);
-		} else if (!strcmp(argv[1], "--get-regexp")) {
-			show_keys = 1;
-			use_key_regexp = 1;
-			do_all = 1;
-			return get_value(argv[2], argv[3]);
-		} else if (!strcmp(argv[1], "--add")) {
-			value = normalize_value(argv[2], argv[3]);
-			return git_config_set_multivar(argv[2], value, "^$", 0);
-		} else if (!strcmp(argv[1], "--replace-all")) {
-			value = normalize_value(argv[2], argv[3]);
-			return git_config_set_multivar(argv[2], value, NULL, 1);
+	if (use_global_config) {
+		char *home = getenv("HOME");
+		if (home) {
+			char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
+			config_exclusive_filename = user_config;
 		} else {
-			value = normalize_value(argv[1], argv[2]);
-			return git_config_set_multivar(argv[1], value, argv[3], 0);
+			die("$HOME not set");
 		}
-	case 5:
-		if (!strcmp(argv[1], "--replace-all")) {
-			value = normalize_value(argv[2], argv[3]);
-			return git_config_set_multivar(argv[2], value, argv[4], 1);
+	}
+	else if (use_system_config)
+		config_exclusive_filename = git_etc_gitconfig();
+	else if (given_config_file) {
+		if (!is_absolute_path(given_config_file) && prefix)
+			config_exclusive_filename = prefix_filename(prefix,
+								    strlen(prefix),
+								    argv[2]);
+		else
+			config_exclusive_filename = given_config_file;
+	}
+
+	if (end_null) {
+		term = '\0';
+		delim = '\n';
+		key_delim = '\n';
+	}
+
+	if (HAS_MULTI_BITS(actions)) {
+		error("only one action at a time.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+	if (actions == 0)
+		switch (argc) {
+		case 1: actions |= ACTION_GET; break;
+		case 2: actions |= ACTION_ADD; break;
+		case 3: actions |= ACTION_REPLACE_ALL; break;
+		default:
+			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
-	case 1:
-	default:
-		usage(git_config_set_usage);
+
+	if (actions & ACTION_LIST) {
+		if (git_config(show_all_config, NULL) < 0)
+			die("error processing config file(s)");
+	}
+	else if (actions & ACTION_EDIT) {
+		const char *config_filename;
+		if (config_exclusive_filename)
+			config_filename = config_exclusive_filename;
+		else
+			config_filename = git_path("config");
+		git_config(git_default_config, NULL);
+		launch_editor(config_filename, NULL, NULL);
+	}
+	else if (actions & ACTION_ADD) {
+		check_argc(argc, 2, 2);
+		value = normalize_value(argv[0], argv[1]);
+		return git_config_set_multivar(argv[0], value, "^$", 0);
+	}
+	else if (actions & ACTION_REPLACE_ALL) {
+		check_argc(argc, 2, 3);
+		value = normalize_value(argv[0], argv[1]);
+		return git_config_set_multivar(argv[0], value, argv[2], 1);
+	}
+	else if (actions & ACTION_GET) {
+		check_argc(argc, 1, 2);
+		return get_value(argv[0], argv[1]);
 	}
+	else if (actions & ACTION_GET_ALL) {
+		do_all = 1;
+		check_argc(argc, 1, 2);
+		return get_value(argv[0], argv[1]);
+	}
+	else if (actions & ACTION_GET_REGEXP) {
+		show_keys = 1;
+		use_key_regexp = 1;
+		do_all = 1;
+		check_argc(argc, 1, 2);
+		return get_value(argv[0], argv[1]);
+	}
+	else if (actions & ACTION_UNSET) {
+		check_argc(argc, 1, 2);
+		if (argc == 2)
+			return git_config_set_multivar(argv[0], NULL, argv[1], 0);
+		else
+			return git_config_set(argv[0], NULL);
+	}
+	else if (actions & ACTION_UNSET_ALL) {
+		check_argc(argc, 1, 2);
+		return git_config_set_multivar(argv[0], NULL, argv[1], 1);
+	}
+	else if (actions & ACTION_RENAME_SECTION) {
+		int ret;
+		check_argc(argc, 2, 2);
+		ret = git_config_rename_section(argv[0], argv[1]);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)
+			die("No such section!");
+	}
+	else if (actions & ACTION_REMOVE_SECTION) {
+		int ret;
+		check_argc(argc, 1, 1);
+		ret = git_config_rename_section(argv[0], NULL);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)
+			die("No such section!");
+	}
+	else if (get_color_slot) {
+		get_color(argv[0]);
+	}
+	else if (get_colorbool_slot) {
+		if (argc == 1)
+			stdout_is_tty = git_config_bool("command line", argv[0]);
+		else if (argc == 0)
+			stdout_is_tty = isatty(1);
+		return get_colorbool(argc != 1);
+	}
+
 	return 0;
 }
-- 
1.6.1.3

[PATCH 4/8] config: Improve variable 'type' handling.

[Felipe Contreras]

So now only either --bool, --int, or --bool-or-int can be used, but not
any combination of them.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 084222a..83f8b74 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -23,7 +23,7 @@ static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;
 
 static int use_global_config, use_system_config;
 static const char *given_config_file;
-static int actions;
+static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
 
@@ -39,6 +39,10 @@ static int end_null;
 #define ACTION_LIST (1<<9)
 #define ACTION_EDIT (1<<10)
 
+#define TYPE_BOOL (1<<0)
+#define TYPE_INT (1<<1)
+#define TYPE_BOOL_OR_INT (1<<2)
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP("Config file location"),
 	OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"),
@@ -57,9 +61,9 @@ static struct option builtin_config_options[] = {
 	OPT_BIT('l', "list", &actions, "list all", ACTION_LIST),
 	OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT),
 	OPT_GROUP("Type"),
-	OPT_SET_INT(0, "bool", &type, "value is \"true\" or \"false\"", T_BOOL),
-	OPT_SET_INT(0, "int", &type, "value is decimal number", T_INT),
-	OPT_SET_INT(0, "bool-or-int", &type, NULL, T_BOOL_OR_INT),
+	OPT_BIT(0, "bool", &types, "value is \"true\" or \"false\"", TYPE_BOOL),
+	OPT_BIT(0, "int", &types, "value is decimal number", TYPE_INT),
+	OPT_BIT(0, "bool-or-int", &types, NULL, TYPE_BOOL_OR_INT),
 	OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"),
 	OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"),
 	OPT_GROUP("Other"),
@@ -336,6 +340,17 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 		key_delim = '\n';
 	}
 
+	if (HAS_MULTI_BITS(types)) {
+		error("only one type at a time.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+	switch (types) {
+	case TYPE_BOOL: type = T_BOOL; break;
+	case TYPE_INT: type = T_INT; break;
+	case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break;
+	default: break;
+	}
+
 	if (HAS_MULTI_BITS(actions)) {
 		error("only one action at a time.");
 		usage_with_options(builtin_config_usage, builtin_config_options);
-- 
1.6.1.3

[PATCH 5/8] config: Disallow multiple config file locations.

[Felipe Contreras]

Either --global, --system, or --file should be used, but not any
combination.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 83f8b74..e744ad8 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 
 	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
 
+	{
+		int config_file_count = use_global_config + use_system_config;
+		if (given_config_file)
+			config_file_count++;
+		if (config_file_count > 1) {
+			error("only one config file at a time.");
+			usage_with_options(builtin_config_usage, builtin_config_options);
+		}
+	}
+
 	if (use_global_config) {
 		char *home = getenv("HOME");
 		if (home) {
-- 
1.6.1.3

[PATCH 6/8] config: Don't allow extra arguments for -e or -l.

[Felipe Contreras]

As suggested by Johannes Schindelin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index e744ad8..3463b1c 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -375,10 +375,12 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 		}
 
 	if (actions & ACTION_LIST) {
+		check_argc(argc, 0, 0);
 		if (git_config(show_all_config, NULL) < 0)
 			die("error processing config file(s)");
 	}
 	else if (actions & ACTION_EDIT) {
+		check_argc(argc, 0, 0);
 		const char *config_filename;
 		if (config_exclusive_filename)
 			config_filename = config_exclusive_filename;
-- 
1.6.1.3

[PATCH 7/8] config: Don't return negative exit codes.

[Felipe Contreras]

As suggested by Johannes Schindelin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |   27 +++++++++++----------------
 1 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 3463b1c..677ae3f 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -308,6 +308,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 {
 	int nongit;
 	char* value;
+	int ret = 0;
 	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
@@ -392,55 +393,49 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	else if (actions & ACTION_ADD) {
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, "^$", 0);
+		ret = git_config_set_multivar(argv[0], value, "^$", 0);
 	}
 	else if (actions & ACTION_REPLACE_ALL) {
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, argv[2], 1);
+		ret = git_config_set_multivar(argv[0], value, argv[2], 1);
 	}
 	else if (actions & ACTION_GET) {
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_GET_ALL) {
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_GET_REGEXP) {
 		show_keys = 1;
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_UNSET) {
 		check_argc(argc, 1, 2);
 		if (argc == 2)
-			return git_config_set_multivar(argv[0], NULL, argv[1], 0);
+			ret = git_config_set_multivar(argv[0], NULL, argv[1], 0);
 		else
-			return git_config_set(argv[0], NULL);
+			ret = git_config_set(argv[0], NULL);
 	}
 	else if (actions & ACTION_UNSET_ALL) {
 		check_argc(argc, 1, 2);
-		return git_config_set_multivar(argv[0], NULL, argv[1], 1);
+		ret = git_config_set_multivar(argv[0], NULL, argv[1], 1);
 	}
 	else if (actions & ACTION_RENAME_SECTION) {
-		int ret;
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section(argv[0], argv[1]);
-		if (ret < 0)
-			return ret;
 		if (ret == 0)
 			die("No such section!");
 	}
 	else if (actions & ACTION_REMOVE_SECTION) {
-		int ret;
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section(argv[0], NULL);
-		if (ret < 0)
-			return ret;
 		if (ret == 0)
 			die("No such section!");
 	}
@@ -452,8 +447,8 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 			stdout_is_tty = git_config_bool("command line", argv[0]);
 		else if (argc == 0)
 			stdout_is_tty = isatty(1);
-		return get_colorbool(argc != 1);
+		ret = get_colorbool(argc != 1);
 	}
 
-	return 0;
+	return !!ret;
 }
-- 
1.6.1.3

[PATCH 8/8] config: Codestyle cleanups.

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 677ae3f..4d1805b 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -87,7 +87,7 @@ static int show_all_config(const char *key_, const char *value_, void *cb)
 	return 0;
 }
 
-static int show_config(const char* key_, const char* value_, void *cb)
+static int show_config(const char *key_, const char *value_, void *cb)
 {
 	char value[256];
 	const char *vptr = value;
@@ -134,7 +134,7 @@ static int show_config(const char* key_, const char* value_, void *cb)
 	return 0;
 }
 
-static int get_value(const char* key_, const char* regex_)
+static int get_value(const char *key_, const char *regex_)
 {
 	int ret = -1;
 	char *tl;
@@ -307,7 +307,7 @@ static int get_colorbool(int print)
 int cmd_config(int argc, const char **argv, const char *unused_prefix)
 {
 	int nongit;
-	char* value;
+	char *value;
 	int ret = 0;
 	const char *prefix = setup_git_directory_gently(&nongit);
 
-- 
1.6.1.3

Re: [PATCH 7/8] config: Don't return negative exit codes.

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Felipe Contreras wrote:


>  	else if (actions & ACTION_RENAME_SECTION) {
> -		int ret;
>  		check_argc(argc, 2, 2);
>  		ret = git_config_rename_section(argv[0], argv[1]);
> -		if (ret < 0)
> -			return ret;
>  		if (ret == 0)
>  			die("No such section!");
>  	}

You need an "if (ret > 0) ret = 0;" to avoid regressions.

>  	else if (actions & ACTION_REMOVE_SECTION) {
> -		int ret;
>  		check_argc(argc, 1, 1);
>  		ret = git_config_rename_section(argv[0], NULL);
> -		if (ret < 0)
> -			return ret;
>  		if (ret == 0)
>  			die("No such section!");
>  	}

Likewise.

Thanks,
Dscho

Re: [PATCH 4/8] config: Improve variable 'type' handling.

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Felipe Contreras wrote:

> +	switch (types) {
> +	case TYPE_BOOL: type = T_BOOL; break;
> +	case TYPE_INT: type = T_INT; break;
> +	case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break;
> +	default: break;
> +	}

Would it not be nicer to get rid of the variable named "type" and use 
"types == TYPE_BOOL" and similar statements instead?

Ciao,
Dscho

Re: [PATCH 5/8] config: Disallow multiple config file locations.

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Felipe Contreras wrote:

> Either --global, --system, or --file should be used, but not any
> combination.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin-config.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-config.c b/builtin-config.c
> index 83f8b74..e744ad8 100644
> --- a/builtin-config.c
> +++ b/builtin-config.c
> @@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
>  
>  	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
>  
> +	{
> +		int config_file_count = use_global_config + use_system_config;
> +		if (given_config_file)
> +			config_file_count++;
> +		if (config_file_count > 1) {
> +			error("only one config file at a time.");
> +			usage_with_options(builtin_config_usage, builtin_config_options);
> +		}
> +	}

Hmm.  Is this a convoluted way to write

	if (use_global_config + use_system_config + !!given_config_file > 1)

or am I misunderstanding anything?

Ciao,
Dscho

Re: [PATCH 7/8] config: Don't return negative exit codes.

[Felipe Contreras]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Sun, Feb 15, 2009 at 2:22 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Felipe Contreras wrote:
>
>
>>       else if (actions & ACTION_RENAME_SECTION) {
>> -             int ret;
>>               check_argc(argc, 2, 2);
>>               ret = git_config_rename_section(argv[0], argv[1]);
>> -             if (ret < 0)
>> -                     return ret;
>>               if (ret == 0)
>>                       die("No such section!");
>>       }
>
> You need an "if (ret > 0) ret = 0;" to avoid regressions.
>
>>       else if (actions & ACTION_REMOVE_SECTION) {
>> -             int ret;
>>               check_argc(argc, 1, 1);
>>               ret = git_config_rename_section(argv[0], NULL);
>> -             if (ret < 0)
>> -                     return ret;
>>               if (ret == 0)
>>                       die("No such section!");
>>       }
>
> Likewise.

True. Fixed in the attached patch.

-- 
Felipe Contreras

[-- Attachment #2: 0007-config-Don-t-return-negative-exit-codes.patch --]
[-- Type: application/octet-stream, Size: 3275 bytes --]

From b682131b61d01cac2946e7c7ab6666fef86441a4 Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sun, 15 Feb 2009 10:54:53 +0200
Subject: [PATCH 7/8] config: Don't return negative exit codes.

As suggested by Johannes Schindelin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 5891a4e..0606ada 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -307,6 +307,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 {
 	int nongit;
 	char* value;
+	int ret = 0;
 	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
@@ -380,57 +381,55 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	else if (actions & ACTION_ADD) {
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, "^$", 0);
+		ret = git_config_set_multivar(argv[0], value, "^$", 0);
 	}
 	else if (actions & ACTION_REPLACE_ALL) {
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, argv[2], 1);
+		ret = git_config_set_multivar(argv[0], value, argv[2], 1);
 	}
 	else if (actions & ACTION_GET) {
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_GET_ALL) {
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_GET_REGEXP) {
 		show_keys = 1;
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_UNSET) {
 		check_argc(argc, 1, 2);
 		if (argc == 2)
-			return git_config_set_multivar(argv[0], NULL, argv[1], 0);
+			ret = git_config_set_multivar(argv[0], NULL, argv[1], 0);
 		else
-			return git_config_set(argv[0], NULL);
+			ret = git_config_set(argv[0], NULL);
 	}
 	else if (actions & ACTION_UNSET_ALL) {
 		check_argc(argc, 1, 2);
-		return git_config_set_multivar(argv[0], NULL, argv[1], 1);
+		ret = git_config_set_multivar(argv[0], NULL, argv[1], 1);
 	}
 	else if (actions & ACTION_RENAME_SECTION) {
-		int ret;
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section(argv[0], argv[1]);
-		if (ret < 0)
-			return ret;
 		if (ret == 0)
 			die("No such section!");
+		if (ret > 0)
+			ret = 0;
 	}
 	else if (actions & ACTION_REMOVE_SECTION) {
-		int ret;
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section(argv[0], NULL);
-		if (ret < 0)
-			return ret;
 		if (ret == 0)
 			die("No such section!");
+		if (ret > 0)
+			ret = 0;
 	}
 	else if (get_color_slot) {
 		get_color(argv[0]);
@@ -440,8 +439,8 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 			stdout_is_tty = git_config_bool("command line", argv[0]);
 		else if (argc == 0)
 			stdout_is_tty = isatty(1);
-		return get_colorbool(argc != 1);
+		ret = get_colorbool(argc != 1);
 	}
 
-	return 0;
+	return !!ret;
 }
-- 
1.6.1.3

Re: [PATCH 4/8] config: Improve variable 'type' handling.

[Felipe Contreras]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On Sun, Feb 15, 2009 at 2:24 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Felipe Contreras wrote:
>
>> +     switch (types) {
>> +     case TYPE_BOOL: type = T_BOOL; break;
>> +     case TYPE_INT: type = T_INT; break;
>> +     case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break;
>> +     default: break;
>> +     }
>
> Would it not be nicer to get rid of the variable named "type" and use
> "types == TYPE_BOOL" and similar statements instead?

I'm not too sure about that. If you read the code you might think you
could actually have multiple types, but the same can be said about
actions.

Anyway, attached is a patch with that change.

-- 
Felipe Contreras

[-- Attachment #2: 0004-config-Improve-variable-type-handling.patch --]
[-- Type: application/octet-stream, Size: 3837 bytes --]

From a2ca08ace97e070d10496eee3e84c8ba2f052b50 Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sun, 15 Feb 2009 10:38:01 +0200
Subject: [PATCH 4/8] config: Improve variable 'type' handling.

So now only either --bool, --int, or --bool-or-int can be used, but not
any combination of them.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 084222a..30de93e 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -19,11 +19,10 @@ static int seen;
 static char delim = '=';
 static char key_delim = ' ';
 static char term = '\n';
-static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;
 
 static int use_global_config, use_system_config;
 static const char *given_config_file;
-static int actions;
+static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
 
@@ -39,6 +38,10 @@ static int end_null;
 #define ACTION_LIST (1<<9)
 #define ACTION_EDIT (1<<10)
 
+#define TYPE_BOOL (1<<0)
+#define TYPE_INT (1<<1)
+#define TYPE_BOOL_OR_INT (1<<2)
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP("Config file location"),
 	OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"),
@@ -57,9 +60,9 @@ static struct option builtin_config_options[] = {
 	OPT_BIT('l', "list", &actions, "list all", ACTION_LIST),
 	OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT),
 	OPT_GROUP("Type"),
-	OPT_SET_INT(0, "bool", &type, "value is \"true\" or \"false\"", T_BOOL),
-	OPT_SET_INT(0, "int", &type, "value is decimal number", T_INT),
-	OPT_SET_INT(0, "bool-or-int", &type, NULL, T_BOOL_OR_INT),
+	OPT_BIT(0, "bool", &types, "value is \"true\" or \"false\"", TYPE_BOOL),
+	OPT_BIT(0, "int", &types, "value is decimal number", TYPE_INT),
+	OPT_BIT(0, "bool-or-int", &types, NULL, TYPE_BOOL_OR_INT),
 	OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"),
 	OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"),
 	OPT_GROUP("Other"),
@@ -105,11 +108,11 @@ static int show_config(const char* key_, const char* value_, void *cb)
 	}
 	if (seen && !do_all)
 		dup_error = 1;
-	if (type == T_INT)
+	if (types & TYPE_INT)
 		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
-	else if (type == T_BOOL)
+	else if (types & TYPE_BOOL)
 		vptr = git_config_bool(key_, value_) ? "true" : "false";
-	else if (type == T_BOOL_OR_INT) {
+	else if (types & TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key_, value_, &is_bool);
 		if (is_bool)
@@ -208,18 +211,18 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (type == T_RAW)
+	if (types == 0)
 		normalized = xstrdup(value);
 	else {
 		normalized = xmalloc(64);
-		if (type == T_INT) {
+		if (types & TYPE_INT) {
 			int v = git_config_int(key, value);
 			sprintf(normalized, "%d", v);
 		}
-		else if (type == T_BOOL)
+		else if (types & TYPE_BOOL)
 			sprintf(normalized, "%s",
 				git_config_bool(key, value) ? "true" : "false");
-		else if (type == T_BOOL_OR_INT) {
+		else if (types & TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key, value, &is_bool);
 			if (!is_bool)
@@ -336,6 +339,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 		key_delim = '\n';
 	}
 
+	if (HAS_MULTI_BITS(types)) {
+		error("only one type at a time.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (HAS_MULTI_BITS(actions)) {
 		error("only one action at a time.");
 		usage_with_options(builtin_config_usage, builtin_config_options);
-- 
1.6.1.3

Re: [PATCH 5/8] config: Disallow multiple config file locations.

[Felipe Contreras]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On Sun, Feb 15, 2009 at 2:26 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Felipe Contreras wrote:
>
>> Either --global, --system, or --file should be used, but not any
>> combination.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  builtin-config.c |   10 ++++++++++
>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/builtin-config.c b/builtin-config.c
>> index 83f8b74..e744ad8 100644
>> --- a/builtin-config.c
>> +++ b/builtin-config.c
>> @@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
>>
>>       argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
>>
>> +     {
>> +             int config_file_count = use_global_config + use_system_config;
>> +             if (given_config_file)
>> +                     config_file_count++;
>> +             if (config_file_count > 1) {
>> +                     error("only one config file at a time.");
>> +                     usage_with_options(builtin_config_usage, builtin_config_options);
>> +             }
>> +     }
>
> Hmm.  Is this a convoluted way to write
>
>        if (use_global_config + use_system_config + !!given_config_file > 1)
>
> or am I misunderstanding anything?

Ah, much better. (the !!foo trick is new to me)

-- 
Felipe Contreras

[-- Attachment #2: 0005-config-Disallow-multiple-config-file-locations.patch --]
[-- Type: application/octet-stream, Size: 1044 bytes --]

From 1ef7e251a2ab24f213af3022dd0a98785c8127b7 Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sun, 15 Feb 2009 10:46:03 +0200
Subject: [PATCH 5/8] config: Disallow multiple config file locations.

Either --global, --system, or --file should be used, but not any
combination.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 30de93e..cdd8a12 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -313,6 +313,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 
 	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
 
+	if (use_global_config + use_system_config + !!given_config_file > 1) {
+		error("only one config file at a time.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (use_global_config) {
 		char *home = getenv("HOME");
 		if (home) {
-- 
1.6.1.3

Re: [PATCH 7/8] config: Don't return negative exit codes.

[Felipe Contreras]

On Sun, Feb 15, 2009 at 02:39:47PM +0200, Felipe Contreras wrote:
> On Sun, Feb 15, 2009 at 2:22 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Sun, 15 Feb 2009, Felipe Contreras wrote:
> >
> >
> >>       else if (actions & ACTION_RENAME_SECTION) {
> >> -             int ret;
> >>               check_argc(argc, 2, 2);
> >>               ret = git_config_rename_section(argv[0], argv[1]);
> >> -             if (ret < 0)
> >> -                     return ret;
> >>               if (ret == 0)
> >>                       die("No such section!");
> >>       }
> >
> > You need an "if (ret > 0) ret = 0;" to avoid regressions.
> >
> >>       else if (actions & ACTION_REMOVE_SECTION) {
> >> -             int ret;
> >>               check_argc(argc, 1, 1);
> >>               ret = git_config_rename_section(argv[0], NULL);
> >> -             if (ret < 0)
> >> -                     return ret;
> >>               if (ret == 0)
> >>                       die("No such section!");
> >>       }
> >
> > Likewise.
> 
> True. Fixed in the attached patch.

I'm inlining the patch as Johannes requested:

---
 builtin-config.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 5891a4e..0606ada 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -307,6 +307,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 {
 	int nongit;
 	char* value;
+	int ret = 0;
 	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
@@ -380,57 +381,55 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	else if (actions & ACTION_ADD) {
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, "^$", 0);
+		ret = git_config_set_multivar(argv[0], value, "^$", 0);
 	}
 	else if (actions & ACTION_REPLACE_ALL) {
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar(argv[0], value, argv[2], 1);
+		ret = git_config_set_multivar(argv[0], value, argv[2], 1);
 	}
 	else if (actions & ACTION_GET) {
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_GET_ALL) {
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_GET_REGEXP) {
 		show_keys = 1;
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		ret = get_value(argv[0], argv[1]);
 	}
 	else if (actions & ACTION_UNSET) {
 		check_argc(argc, 1, 2);
 		if (argc == 2)
-			return git_config_set_multivar(argv[0], NULL, argv[1], 0);
+			ret = git_config_set_multivar(argv[0], NULL, argv[1], 0);
 		else
-			return git_config_set(argv[0], NULL);
+			ret = git_config_set(argv[0], NULL);
 	}
 	else if (actions & ACTION_UNSET_ALL) {
 		check_argc(argc, 1, 2);
-		return git_config_set_multivar(argv[0], NULL, argv[1], 1);
+		ret = git_config_set_multivar(argv[0], NULL, argv[1], 1);
 	}
 	else if (actions & ACTION_RENAME_SECTION) {
-		int ret;
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section(argv[0], argv[1]);
-		if (ret < 0)
-			return ret;
 		if (ret == 0)
 			die("No such section!");
+		if (ret > 0)
+			ret = 0;
 	}
 	else if (actions & ACTION_REMOVE_SECTION) {
-		int ret;
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section(argv[0], NULL);
-		if (ret < 0)
-			return ret;
 		if (ret == 0)
 			die("No such section!");
+		if (ret > 0)
+			ret = 0;
 	}
 	else if (get_color_slot) {
 		get_color(argv[0]);
@@ -440,8 +439,8 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 			stdout_is_tty = git_config_bool("command line", argv[0]);
 		else if (argc == 0)
 			stdout_is_tty = isatty(1);
-		return get_colorbool(argc != 1);
+		ret = get_colorbool(argc != 1);
 	}
 
-	return 0;
+	return !!ret;
 }
-- 
1.6.1.3

-- 
Felipe Contreras

Re: [PATCH 4/8] config: Improve variable 'type' handling.

[Felipe Contreras]

On Sun, Feb 15, 2009 at 02:43:15PM +0200, Felipe Contreras wrote:
> On Sun, Feb 15, 2009 at 2:24 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Sun, 15 Feb 2009, Felipe Contreras wrote:
> >
> >> +     switch (types) {
> >> +     case TYPE_BOOL: type = T_BOOL; break;
> >> +     case TYPE_INT: type = T_INT; break;
> >> +     case TYPE_BOOL_OR_INT: type = T_BOOL_OR_INT; break;
> >> +     default: break;
> >> +     }
> >
> > Would it not be nicer to get rid of the variable named "type" and use
> > "types == TYPE_BOOL" and similar statements instead?
> 
> I'm not too sure about that. If you read the code you might think you
> could actually have multiple types, but the same can be said about
> actions.
> 
> Anyway, attached is a patch with that change.

Sorry, again, but now inlined:

---
 builtin-config.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 084222a..30de93e 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -19,11 +19,10 @@ static int seen;
 static char delim = '=';
 static char key_delim = ' ';
 static char term = '\n';
-static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;
 
 static int use_global_config, use_system_config;
 static const char *given_config_file;
-static int actions;
+static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
 
@@ -39,6 +38,10 @@ static int end_null;
 #define ACTION_LIST (1<<9)
 #define ACTION_EDIT (1<<10)
 
+#define TYPE_BOOL (1<<0)
+#define TYPE_INT (1<<1)
+#define TYPE_BOOL_OR_INT (1<<2)
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP("Config file location"),
 	OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"),
@@ -57,9 +60,9 @@ static struct option builtin_config_options[] = {
 	OPT_BIT('l', "list", &actions, "list all", ACTION_LIST),
 	OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT),
 	OPT_GROUP("Type"),
-	OPT_SET_INT(0, "bool", &type, "value is \"true\" or \"false\"", T_BOOL),
-	OPT_SET_INT(0, "int", &type, "value is decimal number", T_INT),
-	OPT_SET_INT(0, "bool-or-int", &type, NULL, T_BOOL_OR_INT),
+	OPT_BIT(0, "bool", &types, "value is \"true\" or \"false\"", TYPE_BOOL),
+	OPT_BIT(0, "int", &types, "value is decimal number", TYPE_INT),
+	OPT_BIT(0, "bool-or-int", &types, NULL, TYPE_BOOL_OR_INT),
 	OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"),
 	OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"),
 	OPT_GROUP("Other"),
@@ -105,11 +108,11 @@ static int show_config(const char* key_, const char* value_, void *cb)
 	}
 	if (seen && !do_all)
 		dup_error = 1;
-	if (type == T_INT)
+	if (types & TYPE_INT)
 		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
-	else if (type == T_BOOL)
+	else if (types & TYPE_BOOL)
 		vptr = git_config_bool(key_, value_) ? "true" : "false";
-	else if (type == T_BOOL_OR_INT) {
+	else if (types & TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key_, value_, &is_bool);
 		if (is_bool)
@@ -208,18 +211,18 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (type == T_RAW)
+	if (types == 0)
 		normalized = xstrdup(value);
 	else {
 		normalized = xmalloc(64);
-		if (type == T_INT) {
+		if (types & TYPE_INT) {
 			int v = git_config_int(key, value);
 			sprintf(normalized, "%d", v);
 		}
-		else if (type == T_BOOL)
+		else if (types & TYPE_BOOL)
 			sprintf(normalized, "%s",
 				git_config_bool(key, value) ? "true" : "false");
-		else if (type == T_BOOL_OR_INT) {
+		else if (types & TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key, value, &is_bool);
 			if (!is_bool)
@@ -336,6 +339,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 		key_delim = '\n';
 	}
 
+	if (HAS_MULTI_BITS(types)) {
+		error("only one type at a time.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (HAS_MULTI_BITS(actions)) {
 		error("only one action at a time.");
 		usage_with_options(builtin_config_usage, builtin_config_options);
-- 
1.6.1.3

-- 
Felipe Contreras

Re: [PATCH 5/8] config: Disallow multiple config file locations.

[Felipe Contreras]

On Sun, Feb 15, 2009 at 02:44:29PM +0200, Felipe Contreras wrote:
> On Sun, Feb 15, 2009 at 2:26 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Sun, 15 Feb 2009, Felipe Contreras wrote:
> >
> >> Either --global, --system, or --file should be used, but not any
> >> combination.
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  builtin-config.c |   10 ++++++++++
> >>  1 files changed, 10 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/builtin-config.c b/builtin-config.c
> >> index 83f8b74..e744ad8 100644
> >> --- a/builtin-config.c
> >> +++ b/builtin-config.c
> >> @@ -314,6 +314,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
> >>
> >>       argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
> >>
> >> +     {
> >> +             int config_file_count = use_global_config + use_system_config;
> >> +             if (given_config_file)
> >> +                     config_file_count++;
> >> +             if (config_file_count > 1) {
> >> +                     error("only one config file at a time.");
> >> +                     usage_with_options(builtin_config_usage, builtin_config_options);
> >> +             }
> >> +     }
> >
> > Hmm.  Is this a convoluted way to write
> >
> >        if (use_global_config + use_system_config + !!given_config_file > 1)
> >
> > or am I misunderstanding anything?
> 
> Ah, much better. (the !!foo trick is new to me)

Now inlined:

---
 builtin-config.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 30de93e..cdd8a12 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -313,6 +313,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 
 	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
 
+	if (use_global_config + use_system_config + !!given_config_file > 1) {
+		error("only one config file at a time.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (use_global_config) {
 		char *home = getenv("HOME");
 		if (home) {
-- 
1.6.1.3


-- 
Felipe Contreras

Re: [PATCH 2/8] config: Cleanup config file handling.

[Jeff King]

On Sun, Feb 15, 2009 at 11:00:54AM +0200, Felipe Contreras wrote:

> As suggested by Johannes Schindelin.

Thank you for splitting this patch up. I have to admit I was too scared
to read the original after seeing the diffstat.

But part of the nice thing about splitting up is that you can write
meaningful commit messages for each individual change. Reading this
commit message, I don't know what to expect.

Your 1/8, though short, tells me what to expect: a trivial rename. But
"clean up config file handling" is quite ambiguous. And after reading
the patch, I am left wondering why the filename and errno are not useful
as part of the die() message. I think even a single sentence would
probably make it clear.

And yes, I know I can look elsewhere in the thread to find the
discussion between you and Dscho. But think of the poor "git log" user
six months from now.

-Peff

Re: [PATCH 2/8] config: Cleanup config file handling.

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> As suggested by Johannes Schindelin.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Dscho won't be the only person who will read "git log" output.  What was
suggested, what convinced you it is a good idea, what goodness would this
patch add if I take it?

> -			if (git_config(show_all_config, NULL) < 0 &&
> -					file && errno)
> -				die("unable to read config file %s: %s", file,
> -				    strerror(errno));
> +			if (git_config(show_all_config, NULL) < 0)
> +				die("error processing config file(s)");
>  			return 0;

This is the only change with substance in this patch.  What difference
does it make?  Earlier we failed to say "die" with message in some
situations, and now we die when git_config() signals any error.

What are the situations in whoich the command misbehaved?  The commit log
message is where you are expected to explain that.