[PATCH B v4 0/5] git config: reorganization

DORMANTno replies

11 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH B v4 0/5] git config: reorganization

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

This patch series is the first of a two parts reorganization for more
maintainability and user friendliness.

The main change is the switch to use parseopt which provides a nicer help usage
dialog. The rest of the changes take advantage of the initial reorganization to
do more strict option parsing.

Felipe Contreras (5):
  git config: reorganize to use parseopt
  git config: don't allow multiple config file locations
  git config: don't allow multiple variable types
  git config: don't allow extra arguments for -e or -l.
  git config: don't allow --get-color* and variable type

 builtin-config.c |  385 +++++++++++++++++++++++++++++++-----------------------
 1 files changed, 223 insertions(+), 162 deletions(-)

[PATCH B v4 2/5] git config: don't allow multiple config file locations

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

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

Signed-off-by: Felipe Contreras <redacted>
---
 builtin-config.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index 302846e..876eb0e 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -315,6 +315,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
+	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 B v4 4/5] git config: don't allow extra arguments for -e or -l.

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

As suggested by Johannes Schindelin.

Signed-off-by: Felipe Contreras <redacted>
---
 builtin-config.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index c2a30f2..8045926 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -373,6 +373,7 @@ 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) {
 			if (config_exclusive_filename)
 				die("unable to read config file %s: %s",
@@ -382,6 +383,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 		}
 	}
 	else if (actions == ACTION_EDIT) {
+		check_argc(argc, 0, 0);
 		git_config(git_default_config, NULL);
 		launch_editor(config_exclusive_filename ?
 			      config_exclusive_filename : git_path("config"),
-- 
1.6.1.3

[PATCH B v4 5/5] git config: don't allow --get-color* and variable type

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

Doing so would be incoherent since --get-color would pick a color slot
and ignore the variable type option (e.g. --bool), and the type would
require a variable name.

Suggested by Junio C Hamano.

Signed-off-by: Felipe Contreras <redacted>
---
 builtin-config.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index 8045926..9930568 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	if (get_colorbool_slot)
 	    actions |= ACTION_GET_COLORBOOL;
 
+	if ((get_color_slot || get_colorbool_slot) && types) {
+		error("--get-color and variable type are incoherent");
+		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 B v4 3/5] git config: don't allow multiple variable types

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

Only --bool, --int, or --bool-or-int can be used, but not any
combination of them.

Signed-off-by: Felipe Contreras <redacted>
---
 builtin-config.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index 876eb0e..c2a30f2 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;
 
@@ -43,6 +42,10 @@ static int end_null;
 #define ACTION_GET_COLOR (1<<13)
 #define ACTION_GET_COLORBOOL (1<<14)
 
+#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"),
@@ -63,9 +66,9 @@ static struct option builtin_config_options[] = {
 	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("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_GROUP("Other"),
 	OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
 	OPT_END(),
@@ -109,11 +112,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)
@@ -212,18 +215,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)
@@ -346,6 +349,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 (get_color_slot)
 	    actions |= ACTION_GET_COLOR;
 	if (get_colorbool_slot)
-- 
1.6.1.3

Re: [PATCH B v4 0/5] git config: reorganization

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

On Sat, Feb 21, 2009 at 2:49 AM, Felipe Contreras
[off-list ref] wrote:
This patch series is the first of a two parts reorganization for more
maintainability and user friendliness.
Err, the second.

-- 
Felipe Contreras

[PATCH B v4 1/5] git config: reorganize to use parseopt

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

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

Signed-off-by: Felipe Contreras <redacted>
---
 builtin-config.c |  349 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 195 insertions(+), 154 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index 0836880..302846e 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,63 @@ 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)
+#define ACTION_SET (1<<11)
+#define ACTION_SET_ALL (1<<12)
+#define ACTION_GET_COLOR (1<<13)
+#define ACTION_GET_COLORBOOL (1<<14)
+
+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_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),
+	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_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("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_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,162 +312,143 @@ 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) {
-				if (config_exclusive_filename)
-					die("unable to read config file %s: %s",
-					    config_exclusive_filename, strerror(errno));
-				else
-					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")) {
-			if (argc != 2)
-				usage(git_config_set_usage);
-			git_config(git_default_config, NULL);
-			launch_editor(config_exclusive_filename ?
-				      config_exclusive_filename : git_path("config"),
-				      NULL, NULL);
-			return 0;
-		} else
-			break;
-		argc--;
-		argv++;
-	}
-
-	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);
+	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+
+	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);
+	}
+	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 (get_color_slot)
+	    actions |= ACTION_GET_COLOR;
+	if (get_colorbool_slot)
+	    actions |= ACTION_GET_COLORBOOL;
+
+	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_SET; break;
+		case 3: actions = ACTION_SET_ALL; break;
+		default:
+			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
-	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);
+
+	if (actions == ACTION_LIST) {
+		if (git_config(show_all_config, NULL) < 0) {
+			if (config_exclusive_filename)
+				die("unable to read config file %s: %s",
+				    config_exclusive_filename, strerror(errno));
+			else
+				die("error processing config file(s)");
 		}
-	case 1:
-	default:
-		usage(git_config_set_usage);
 	}
+	else if (actions == ACTION_EDIT) {
+		git_config(git_default_config, NULL);
+		launch_editor(config_exclusive_filename ?
+			      config_exclusive_filename : git_path("config"),
+			      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]);
+		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 (actions == ACTION_GET_COLOR) {
+		get_color(argv[0]);
+	}
+	else if (actions == ACTION_GET_COLORBOOL) {
+		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 != 0);
+	}
+
 	return 0;
 }
-- 
1.6.1.3

Re: [PATCH B v4 1/5] git config: reorganize to use parseopt

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:15

Felipe Contreras [off-list ref] writes:
+	else if (actions == ACTION_GET_COLORBOOL) {
+		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 != 0);
+	}
I see you fixed this from the last series...

Re: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:15

Felipe Contreras [off-list ref] writes:
quoted hunk
Doing so would be incoherent since --get-color would pick a color slot
and ignore the variable type option (e.g. --bool), and the type would
require a variable name.

Suggested by Junio C Hamano.

Signed-off-by: Felipe Contreras <redacted>
---
 builtin-config.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index 8045926..9930568 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	if (get_colorbool_slot)
 	    actions |= ACTION_GET_COLORBOOL;
 
+	if ((get_color_slot || get_colorbool_slot) && types) {
+		error("--get-color and variable type are incoherent");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
I do not think I suggested anything like this, so I'd decline to take
credit for this patch.

Strictly speaking, "--bool --get-colorbool diff.color 1" shouldn't error
out, don't you think?  And it certainly shouldn't say "--get-color".

Re: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

On Sun, Feb 22, 2009 at 6:48 PM, Junio C Hamano [off-list ref] wrote:
Felipe Contreras [off-list ref] writes:
quoted
Doing so would be incoherent since --get-color would pick a color slot
and ignore the variable type option (e.g. --bool), and the type would
require a variable name.

Suggested by Junio C Hamano.

Signed-off-by: Felipe Contreras <redacted>
---
 builtin-config.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c
index 8045926..9930568 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
      if (get_colorbool_slot)
          actions |= ACTION_GET_COLORBOOL;

+     if ((get_color_slot || get_colorbool_slot) && types) {
+             error("--get-color and variable type are incoherent");
+             usage_with_options(builtin_config_usage, builtin_config_options);
+     }
+
I do not think I suggested anything like this, so I'd decline to take
credit for this patch.

Strictly speaking, "--bool --get-colorbool diff.color 1" shouldn't error
out, don't you think?  And it certainly shouldn't say "--get-color".
Huh? I misinterpreted:
I see "git config --bool --get-color diff.color.whitespace" is still
allowed, which you might want to tighten further.

I don't really understand the --get-color* options, so please drop the patch.

-- 
Felipe Contreras

Re: [PATCH B v4 1/5] git config: reorganize to use parseopt

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:46:15

On Sun, Feb 22, 2009 at 6:48 PM, Junio C Hamano [off-list ref] wrote:
Felipe Contreras [off-list ref] writes:
quoted
+     else if (actions == ACTION_GET_COLORBOOL) {
+             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 != 0);
+     }
I see you fixed this from the last series...
Yes, argc != 1 was wrong. Now I've made sure all the tests pass.

-- 
Felipe Contreras
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help