threads / patch / 17798

patch, 8 partsconfig: Use parseopt.

Subject: [PATCH 3/8] config: Use parseopt.

## tl;dr

19 messages between Feb 15, 2009 and Feb 16, 2009. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Felipe Contreras· Feb 15, 2009, 09:00 UTC · lore

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

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(-)
Show changes to builtin-config.c +4 −3
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
Felipe Contreras· Feb 15, 2009, 09:00 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to builtin-config.c +9 −11
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
Felipe Contreras· Feb 15, 2009, 09:00 UTC · re: Felipe Contreras · lore

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(-)
Show changes to builtin-config.c +178 −184
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
Felipe Contreras· Feb 15, 2009, 09:00 UTC · re: Felipe Contreras · lore

[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 |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)
Show changes to builtin-config.c +19 −4
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
Felipe Contreras· Feb 15, 2009, 09:00 UTC · re: Felipe Contreras · lore

[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 |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
Show changes to builtin-config.c +10 −0
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
Felipe Contreras· Feb 15, 2009, 09:00 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to builtin-config.c +2 −0
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
Felipe Contreras· Feb 15, 2009, 09:00 UTC · re: Felipe Contreras · lore

[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 |   27 +++++++++++----------------
 1 files changed, 11 insertions(+), 16 deletions(-)
Show changes to builtin-config.c +11 −16
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
Felipe Contreras· Feb 15, 2009, 09:01 UTC · re: Felipe Contreras · lore

[PATCH 8/8] config: Codestyle cleanups.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to builtin-config.c +3 −3
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
Johannes Schindelin· Feb 15, 2009, 12:22 UTC · re: Felipe Contreras · lore

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

Hi,
On Sun, 15 Feb 2009, Felipe Contreras wrote:
Show 9 quoted lines
>  	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.
Show 9 quoted lines
>  	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

Felipe Contreras· Feb 15, 2009, 12:39 UTC · re: Johannes Schindelin · lore

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

On Sun, Feb 15, 2009 at 2:22 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 28 quoted lines
> 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


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
Felipe Contreras· Feb 15, 2009, 13:30 UTC · re: Felipe Contreras · lore

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

On Sun, Feb 15, 2009 at 02:39:47PM +0200, Felipe Contreras wrote:
Show 32 quoted lines
> 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(-)
Show changes to builtin-config.c +15 −16
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
Johannes Schindelin· Feb 15, 2009, 12:26 UTC · re: Felipe Contreras · lore

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

Hi,
On Sun, 15 Feb 2009, Felipe Contreras wrote:
Show 25 quoted lines
> 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

Felipe Contreras· Feb 15, 2009, 12:44 UTC · re: Johannes Schindelin · lore

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

On Sun, Feb 15, 2009 at 2:26 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 35 quoted lines
> 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


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
Felipe Contreras· Feb 15, 2009, 13:35 UTC · re: Felipe Contreras · lore

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

On Sun, Feb 15, 2009 at 02:44:29PM +0200, Felipe Contreras wrote:
Show 39 quoted lines
> 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(-)
Show changes to builtin-config.c +5 −0
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
Johannes Schindelin· Feb 15, 2009, 12:24 UTC · re: Felipe Contreras · lore

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

Hi,
On Sun, 15 Feb 2009, Felipe Contreras wrote:
Show 6 quoted lines
> +	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

Felipe Contreras· Feb 15, 2009, 12:43 UTC · re: Johannes Schindelin · lore

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

On Sun, Feb 15, 2009 at 2:24 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 13 quoted lines
> 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


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
Felipe Contreras· Feb 15, 2009, 13:34 UTC · re: Felipe Contreras · lore

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

On Sun, Feb 15, 2009 at 02:43:15PM +0200, Felipe Contreras wrote:
Show 21 quoted lines
> 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(-)
Show changes to builtin-config.c +20 −12
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
Jeff King· Feb 15, 2009, 20:15 UTC · re: Felipe Contreras · lore

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

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
Junio C Hamano· Feb 16, 2009, 01:15 UTC · re: Felipe Contreras · lore

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

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?

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

← back to recent threads