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

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

[Felipe Contreras]

Essensially this replaces 'file' with 'prefix' in the cases where the
variable is used as a prefix, which is consistent with other git
commands.

When using the --list option general errors where not properly reported,
only errors related with the 'file'. Now they are reported, and 'file'
is irrelevant.

That reduces the rest of 'file' usage to nothing, therefore now only
'prefix' remains.

Suggested by Johannes Schindelin.

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

diff --git a/builtin-config.c b/builtin-config.c
index 6937eaf..da754e0 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;
@@ -281,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);
 
@@ -299,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")) {
@@ -319,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 v2 2/8] config: Reorganize get_color*.

[Felipe Contreras]

In preparation for parseopt.

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

diff --git a/builtin-config.c b/builtin-config.c
index da754e0..8fd6171 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -180,7 +180,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 +191,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 +201,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 +224,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 +237,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)
@@ -358,9 +320,20 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 			}
 			return 0;
 		} else if (!strcmp(argv[1], "--get-color")) {
-			return get_color(argc-2, argv+2);
+			if (argc > 4 || argc < 3)
+				usage(git_config_set_usage);
+			get_color_slot = argv[2];
+			get_color(argv[3]);
+			return 0;
 		} else if (!strcmp(argv[1], "--get-colorbool")) {
-			return get_colorbool(argc-2, argv+2);
+			if (argc == 4)
+				stdout_is_tty = git_config_bool("command line", argv[3]);
+			else if (argc == 3)
+				stdout_is_tty = isatty(1);
+			else
+				usage(git_config_set_usage);
+			get_colorbool_slot = argv[2];
+			return get_colorbool(argc == 3);
 		} else if (!strcmp(argv[1], "--edit") || !strcmp(argv[1], "-e")) {
 			const char *config_filename;
 			if (argc != 2)
-- 
1.6.1.3

[PATCH v2 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 |  325 +++++++++++++++++++++++++++++-------------------------
 1 files changed, 173 insertions(+), 152 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 8fd6171..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_)
@@ -252,160 +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")) {
-			if (argc > 4 || argc < 3)
-				usage(git_config_set_usage);
-			get_color_slot = argv[2];
-			get_color(argv[3]);
-			return 0;
-		} else if (!strcmp(argv[1], "--get-colorbool")) {
-			if (argc == 4)
-				stdout_is_tty = git_config_bool("command line", argv[3]);
-			else if (argc == 3)
-				stdout_is_tty = isatty(1);
-			else
-				usage(git_config_set_usage);
-			get_colorbool_slot = argv[2];
-			return get_colorbool(argc == 3);
-		} 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);
+	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(argv[1], value);
+			die("$HOME not set");
 		}
-	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);
-		} else {
-			value = normalize_value(argv[1], argv[2]);
-			return git_config_set_multivar(argv[1], value, argv[3], 0);
-		}
-	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 v2 4/8] config: Disallow multiple variable types.

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

[PATCH v2 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 |    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

[PATCH v2 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 cdd8a12..5891a4e 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -363,10 +363,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 v2 7/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 5891a4e..ff9e029 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -86,7 +86,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;
@@ -133,7 +133,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;
@@ -306,7 +306,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;
 	const char *prefix = setup_git_directory_gently(&nongit);
 
 	config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
-- 
1.6.1.3

[PATCH v2 8/8] config: Cleanup editor action.

[Felipe Contreras]

Copy-paste from Johannes Schindelin.

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

diff --git a/builtin-config.c b/builtin-config.c
index ff9e029..bc97854 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -369,13 +369,10 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	}
 	else if (actions & ACTION_EDIT) {
 		check_argc(argc, 0, 0);
-		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);
+		launch_editor(config_exclusive_filename ?
+			      config_exclusive_filename : git_path("config"),
+			      NULL, NULL);
 	}
 	else if (actions & ACTION_ADD) {
 		check_argc(argc, 2, 2);
-- 
1.6.1.3

Re: [PATCH v2 8/8] config: Cleanup editor action.

[Junio C Hamano]

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

> Copy-paste from Johannes Schindelin.

Sorry, I do not understand.  Are you saying:

 (1) This duplicated code was introduced by copy-and-paste by Dscho and I
     am cleaning up his mess?

or

 (2) This change came from Dscho and I am just sending it by copy-pasting
     what he did?

If the latter, then this is not a good way to describe what happened.  You
would use in-body From: header to give attribution, and more importantly,
you need to say what you would usually say in the commit log message (what
it does, why the original is bad, etc.)

Re: [PATCH v2 4/8] config: Disallow multiple variable types.

[Junio C Hamano]

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

> +	if (HAS_MULTI_BITS(types)) {
> +		error("only one type at a time.");
> +		usage_with_options(builtin_config_usage, builtin_config_options);
> +	}

You would be able to catch "config --bool --get-color color.diff.whitespace"
as an error by doing something similar to the "fake actions" for get-color*
as I suggested in my review on [3/8].

I did not find anything objectionable in the remainder of the series.

Thanks.

Re: [PATCH v2 3/8] config: Use parseopt.

[Junio C Hamano]

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

> +	if (HAS_MULTI_BITS(actions)) {
> +		error("only one action at a time.");
> +		usage_with_options(builtin_config_usage, builtin_config_options);

My initial reaction was:

	Can we easily say "--get and --getall are mutually incompatible"?

and knowing that it would take much more code, the second reaction was:

	Does the user know what we mean by "action"?

Since the answer to this is "Yes, the usage message comes from parseopt
and there is a clear categorization", I think the message is good enough.

What happens when the user says "config --get --get-colorbool user.name"?
Is it an error?  Is it diagnosed as an error?

It probably is easy to fix it by defining two bits of fake actions and do:

	if (get_color_slot)
        	actions |= ACTION_GET_COLOR;
	if (get_colorbool_slot)
        	actions |= ACTION_GET_COLORBOOL;

immediately before this HAS_MULTI_BITS check.

I know I suggested these to are type-like, but I realize that these two
are better categorized as actions tied to a specific type (color), as you
had in your earlier round.

> +	if (actions == 0)
> +		switch (argc) {
> +		case 1: actions |= ACTION_GET; break;
> +		case 2: actions |= ACTION_ADD; break;
> +		case 3: actions |= ACTION_REPLACE_ALL; break;

Straight assignment, not ORing-in please.  It wastes a few seconds from
the reader wondering what other bits in the variable "actions" are used
for things other than ACTION_* (the answer is none).

Similarly, later conditions:

> +	if (actions & ACTION_LIST) {

would read better if they used equality == checks.

Re: [PATCH v2 3/8] config: Use parseopt.

[Felipe Contreras]

On Tue, Feb 17, 2009 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +     if (HAS_MULTI_BITS(actions)) {
>> +             error("only one action at a time.");
>> +             usage_with_options(builtin_config_usage, builtin_config_options);
>
> My initial reaction was:
>
>        Can we easily say "--get and --getall are mutually incompatible"?
>
> and knowing that it would take much more code, the second reaction was:
>
>        Does the user know what we mean by "action"?
>
> Since the answer to this is "Yes, the usage message comes from parseopt
> and there is a clear categorization", I think the message is good enough.
>
> What happens when the user says "config --get --get-colorbool user.name"?
> Is it an error?  Is it diagnosed as an error?
>
> It probably is easy to fix it by defining two bits of fake actions and do:
>
>        if (get_color_slot)
>                actions |= ACTION_GET_COLOR;
>        if (get_colorbool_slot)
>                actions |= ACTION_GET_COLORBOOL;
>
> immediately before this HAS_MULTI_BITS check.
>
> I know I suggested these to are type-like, but I realize that these two
> are better categorized as actions tied to a specific type (color), as you
> had in your earlier round.

All right, done.

>> +     if (actions == 0)
>> +             switch (argc) {
>> +             case 1: actions |= ACTION_GET; break;
>> +             case 2: actions |= ACTION_ADD; break;
>> +             case 3: actions |= ACTION_REPLACE_ALL; break;
>
> Straight assignment, not ORing-in please.  It wastes a few seconds from
> the reader wondering what other bits in the variable "actions" are used
> for things other than ACTION_* (the answer is none).
>
> Similarly, later conditions:
>
>> +     if (actions & ACTION_LIST) {
>
> would read better if they used equality == checks.

Cool, I was worried those where not logical to the reader but I
couldn't think of a better solution... this one looks much better!

I've also made the same change for the 'types' variable in a later patch.

-- 
Felipe Contreras

Re: [PATCH v2 3/8] config: Use parseopt.

[Junio C Hamano]

I've queued the entire series on top of fc/config-editor topic and even
merged the result in 'pu' once, but I had to reintegrate 'pu' without the
series.

Before this commit, t/t1300-repo-config.sh passes, but this one breaks
the test.

Re: [PATCH v2 3/8] config: Use parseopt.

[Felipe Contreras]

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

On Tue, Feb 17, 2009 at 7:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I've queued the entire series on top of fc/config-editor topic and even
> merged the result in 'pu' once, but I had to reintegrate 'pu' without the
> series.
>
> Before this commit, t/t1300-repo-config.sh passes, but this one breaks
> the test.

Ah, I didn't know there was a test for that.

I've fixed most the issues but unfortunately parseopt barfs when -1 is
used as an argument. That should be fixed somehow, otherwise this
patch will never pass the test.

I'm attaching a patch that makes the test pass (for review), but
shouldn't be merged.

-- 
Felipe Contreras

[-- Attachment #2: 0001-config-Fixes-parseopt-for-t1300-repo-config.patch --]
[-- Type: application/octet-stream, Size: 3982 bytes --]

From 22e4d1a472027d2b7f650a99f487fef78ef3b8ad Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Tue, 17 Feb 2009 12:30:26 +0200
Subject: [NOMERGE/PATCH] config: Fixes parseopt for t1300-repo-config.

This is patch isn't meant for merging, just for review.

Ideally parseopt shouldn't fail with -1 as an argument.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c       |   18 +++++++++++++++---
 t/t1300-repo-config.sh |    8 ++++----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 084222a..19f274a 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -38,6 +38,8 @@ static int end_null;
 #define ACTION_REMOVE_SECTION (1<<8)
 #define ACTION_LIST (1<<9)
 #define ACTION_EDIT (1<<10)
+#define ACTION_SET (1<<11)
+#define ACTION_SET_ALL (1<<12)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP("Config file location"),
@@ -46,7 +48,7 @@ static struct option builtin_config_options[] = {
 	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-all", &actions, "get all values: key [value-regex]", ACTION_GET_ALL),
 	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),
@@ -343,8 +345,8 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	if (actions == 0)
 		switch (argc) {
 		case 1: actions |= ACTION_GET; break;
-		case 2: actions |= ACTION_ADD; break;
-		case 3: actions |= ACTION_REPLACE_ALL; break;
+		case 2: actions |= ACTION_SET; break;
+		case 3: actions |= ACTION_SET_ALL; break;
 		default:
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
@@ -362,6 +364,16 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 		git_config(git_default_config, NULL);
 		launch_editor(config_filename, NULL, NULL);
 	}
+	else if (actions & ACTION_SET) {
+		check_argc(argc, 2, 2);
+		value = normalize_value(argv[0], argv[1]);
+		return git_config_set(argv[0], value);
+	}
+	else if (actions & ACTION_SET_ALL) {
+		check_argc(argc, 2, 3);
+		value = normalize_value(argv[0], argv[1]);
+		return git_config_set_multivar(argv[0], value, argv[2], 0);
+	}
 	else if (actions & ACTION_ADD) {
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 11b82f4..be7104d 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -528,7 +528,7 @@ EOF
 test_expect_success bool '
 
 	git config bool.true1 01 &&
-	git config bool.true2 -1 &&
+	git config bool.true2 -- -1 &&
 	git config bool.true3 YeS &&
 	git config bool.true4 true &&
 	git config bool.false1 000 &&
@@ -569,7 +569,7 @@ EOF
 test_expect_success 'set --bool' '
 
 	git config --bool bool.true1 01 &&
-	git config --bool bool.true2 -1 &&
+	git config --bool bool.true2 -- -1 &&
 	git config --bool bool.true3 YeS &&
 	git config --bool bool.true4 true &&
 	git config --bool bool.false1 000 &&
@@ -590,7 +590,7 @@ EOF
 test_expect_success 'set --int' '
 
 	git config --int int.val1 01 &&
-	git config --int int.val2 -1 &&
+	git config --int int.val2 -- -1 &&
 	git config --int int.val3 5m &&
 	cmp expect .git/config'
 
@@ -648,7 +648,7 @@ test_expect_success 'set --bool-or-int' '
 	git config --bool-or-int bool.false2 no &&
 	git config --bool-or-int int.int1 0 &&
 	git config --bool-or-int int.int2 1 &&
-	git config --bool-or-int int.int3 -1 &&
+	git config --bool-or-int int.int3 -- -1 &&
 	test_cmp expect .git/config
 '
 
-- 
1.6.1.3

Re: [PATCH v2 3/8] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Tue, 17 Feb 2009, Felipe Contreras wrote:

> On Tue, Feb 17, 2009 at 7:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > I've queued the entire series on top of fc/config-editor topic and even
> > merged the result in 'pu' once, but I had to reintegrate 'pu' without the
> > series.
> >
> > Before this commit, t/t1300-repo-config.sh passes, but this one breaks
> > the test.
> 
> Ah, I didn't know there was a test for that.
> 
> I've fixed most the issues but unfortunately parseopt barfs when -1 is
> used as an argument. That should be fixed somehow, otherwise this
> patch will never pass the test.

Have you seen PARSE_OPT_STOP_AT_NON_OPTION?

Ciao,
Dscho

Re: [PATCH v2 3/8] config: Use parseopt.

[Felipe Contreras]

On Tue, Feb 17, 2009 at 1:55 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 17 Feb 2009, Felipe Contreras wrote:
>
>> On Tue, Feb 17, 2009 at 7:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> > I've queued the entire series on top of fc/config-editor topic and even
>> > merged the result in 'pu' once, but I had to reintegrate 'pu' without the
>> > series.
>> >
>> > Before this commit, t/t1300-repo-config.sh passes, but this one breaks
>> > the test.
>>
>> Ah, I didn't know there was a test for that.
>>
>> I've fixed most the issues but unfortunately parseopt barfs when -1 is
>> used as an argument. That should be fixed somehow, otherwise this
>> patch will never pass the test.
>
> Have you seen PARSE_OPT_STOP_AT_NON_OPTION?

Thanks, that works... cooking up a new series.

-- 
Felipe Contreras

Re: [PATCH v2 2/8] config: Reorganize get_color*.

[Felipe Contreras]

On Tue, Feb 17, 2009 at 2:54 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> In preparation for parseopt.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---

<snip/>

> @@ -274,12 +237,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;
>  }

<snip/>

>                } else if (!strcmp(argv[1], "--get-colorbool")) {
> -                       return get_colorbool(argc-2, argv+2);
> +                       if (argc == 4)
> +                               stdout_is_tty = git_config_bool("command line", argv[3]);
> +                       else if (argc == 3)
> +                               stdout_is_tty = isatty(1);
> +                       else
> +                               usage(git_config_set_usage);
> +                       get_colorbool_slot = argv[2];
> +                       return get_colorbool(argc == 3);

Agh, crap, that should be argc != 3. Anyway, the next patch does it properly.

-- 
Felipe Contreras

Re: [PATCH v2 2/8] config: Reorganize get_color*.

[Junio C Hamano]

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

> In preparation for parseopt.

I like this patch, because it clarifies what get_colorbool() and
get_color() functions are meant to do by moving the boundary of
responsibility between the two callers and these two functions.

The above log message does not do justice to the patch text.

>  builtin-config.c |   63 +++++++++++++++--------------------------------------
>  1 files changed, 18 insertions(+), 45 deletions(-)

I like a patch that results in code reduction, so I got quite interested
in seeing what you did.  But there was no magic --- you lost a lot of
comments on what each function is supposed to do.

They are all described in the documentation, and removal of these comments
that can go stale is probably a good thing, but you could have avoided
dissapointing me who expected a magic by mentioning the removal of the
comments (and why it is a good idea) upfront ;-)

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

[Junio C Hamano]

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

> When using the --list option general errors where not properly reported,
> only errors related with the 'file'. Now they are reported, and 'file'
> is irrelevant.
> ...
> @@ -299,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)");

Does the author of 93a56c2 (git-config: print error message if the config
file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?

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

[Felipe Contreras]

On Tue, Feb 17, 2009 at 3:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> When using the --list option general errors where not properly reported,
>> only errors related with the 'file'. Now they are reported, and 'file'
>> is irrelevant.
>> ...
>> @@ -299,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)");
>
> Does the author of 93a56c2 (git-config: print error message if the config
> file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?

I looked at the debian bug report[1], the guy complains about 2 things:

1) git-config --file fails silently if the filename isn't absolute

This is still fixed.

2) git-config -l --file doesn't do what you might expect and list the
contents of the specified file. Instead it ignores the --file option since it
came after the -l. Nice way to shoot oneself in the foot.

This is now fixed after the parseopt patch.

Also, before this patch 'git config --global -l' would fail silently
if there isn't any ~/.gitconfig. Now at least git reports "error
processing config file(s)".

A more ideal solution would be:

if (config_exclusive_filename)
	die("unable to read config file %s: %s",
	    config_exclusive_filename, strerror(errno));
else
	die("error processing config file(s)");

So, if a file is specified with --file, --global, or --system, then
the correct error would be reported.

I digged a bit more and it turns out if there's parse error
git_config() will die immediately, and the only time it will return a
negative value is when the config file(s) are not present, at which
point there will be an errno set, and when config_exclusive_filename
is specified that means the errno will be the one of fopen trying to
open that file.

Still, "error processing config file(s)" will be reported when no file
is specified 'git config -l', there isn't any repo config file (cwd is
not in a git repo).

Even better I think would be to allow 'git config -l' to work even if
we are not in a git repo, and error only when there isn't any config
file (repo, system or global).

This is how it would look like:

 int git_config(config_fn_t fn, void *data)
 {
-       int ret = 0;
+       int ret = 0, found = 0;
        char *repo_config = NULL;
        const char *home = NULL;

        /* Setting $GIT_CONFIG makes git read _only_ the given config file. */
        if (config_exclusive_filename)
                return git_config_from_file(fn,
config_exclusive_filename, data);
-       if (git_config_system() && !access(git_etc_gitconfig(), R_OK))
+       if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
                ret += git_config_from_file(fn, git_etc_gitconfig(),
                                            data);
+               found += 1;
+       }

        home = getenv("HOME");
        if (git_config_global() && home) {
                char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
-               if (!access(user_config, R_OK))
+               if (!access(user_config, R_OK)) {
                        ret += git_config_from_file(fn, user_config, data);
+                       found += 1;
+               }
                free(user_config);
        }

        repo_config = git_pathdup("config");
-       ret += git_config_from_file(fn, repo_config, data);
+       if (!access(repo_config, R_OK)) {
+               ret += git_config_from_file(fn, repo_config, data);
+               found += 1;
+       }
        free(repo_config);
+       if (found == 0)
+               error("no config file found");
        return ret;
 }

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=445208

-- 
Felipe Contreras

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

[Johannes Schindelin]

Hi,

On Tue, 17 Feb 2009, Felipe Contreras wrote:

>         free(repo_config);
> +       if (found == 0)
> +               error("no config file found");
>         return ret;

Err, you mean "return error(..)"?

But it might be not an error at all: think of "cd / && git ls-remote $URL" 
without /etc/gitconfig nor $HOME/.gitconfig.

Ciao,
Dscho

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

[Felipe Contreras]

On Tue, Feb 17, 2009 at 2:01 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 17 Feb 2009, Felipe Contreras wrote:
>
>>         free(repo_config);
>> +       if (found == 0)
>> +               error("no config file found");
>>         return ret;
>
> Err, you mean "return error(..)"?
>
> But it might be not an error at all: think of "cd / && git ls-remote $URL"
> without /etc/gitconfig nor $HOME/.gitconfig.

Yeah, I thought about that when doing the patch. I've removed the
error(), now it will only return -1. Almost nobody is checking the
return code anyways.

-- 
Felipe Contreras

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

[Gerrit Pape]

On Mon, Feb 16, 2009 at 05:45:00PM -0800, Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > When using the --list option general errors where not properly reported,
> > only errors related with the 'file'. Now they are reported, and 'file'
> > is irrelevant.
> > ...
> > @@ -299,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)");
> 
> Does the author of 93a56c2 (git-config: print error message if the config
> file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?

Hm, we lose some information from the error message when called with
--file, but it seems to improve on other cases.  The filename doesn't
matter that much, but it would be nice to know the reason.  I wouldn't
object against this hunk though if that isn't possible.

Regards, Gerrit.

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

[Johannes Schindelin]

Hi,

On Tue, 17 Feb 2009, Gerrit Pape wrote:

> On Mon, Feb 16, 2009 at 05:45:00PM -0800, Junio C Hamano wrote:
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> > > When using the --list option general errors where not properly reported,
> > > only errors related with the 'file'. Now they are reported, and 'file'
> > > is irrelevant.
> > > ...
> > > @@ -299,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)");
> > 
> > Does the author of 93a56c2 (git-config: print error message if the config
> > file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?
> 
> Hm, we lose some information from the error message when called with
> --file, but it seems to improve on other cases.  The filename doesn't
> matter that much, but it would be nice to know the reason.  I wouldn't
> object against this hunk though if that isn't possible.

The point is: when _not_ using --file, the output could be wrong 
(mentioning another config file than the one having an issue), or not be 
shown at all -- I haven't checked, but both options to not look good to 
me.

Ciao,
Dscho