[PATCH] config: Use parseopt.

[PATCH] config: Use parseopt.

[Felipe Contreras]

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

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

diff --git a/builtin-config.c b/builtin-config.c
index afc4393..f774902 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,42 @@ 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 do_list, do_edit, do_add, do_get, do_get_all, do_get_regexp, do_replace_all;
+static int do_unset, do_unset_all, do_rename_section, do_remove_section;
+static int type_int, type_bool, type_bool_or_int;
+static const char *get_color_name, *get_colorbool_name;
+static int end_null;
+
+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_BOOLEAN(0, "get", &do_get, "get value: name [value-regex]"),
+	OPT_BOOLEAN(0, "get-all", &do_get_all, "get all values: key [value-regex]"),
+	OPT_BOOLEAN(0, "get-regexp", &do_get_regexp, "get values for regexp: name-regex [value-regex]"),
+	OPT_BOOLEAN(0, "replace-all", &do_replace_all, "replace all options: name [value [value_regex]"),
+	OPT_BOOLEAN(0, "add", &do_add, "adds a new option: name value"),
+	OPT_BOOLEAN(0, "unset", &do_unset, "removes an option: name [value-regex]"),
+	OPT_BOOLEAN(0, "unset-all", &do_unset_all, "removes all matches: name [value-regex]"),
+	OPT_BOOLEAN(0, "rename-section", &do_rename_section, "rename section: old-name new-name"),
+	OPT_BOOLEAN(0, "remove-section", &do_remove_section, "remove a section: name"),
+	OPT_BOOLEAN('l', "list", &do_list, "list all"),
+	OPT_STRING(0, "get-color", &get_color_name, "name", "find the color configured: [default]"),
+	OPT_STRING(0, "get-colorbool", &get_colorbool_name, "name", "find the color setting: [stdout-is-tty]"),
+	OPT_BOOLEAN('e', "edit", &do_edit, "opens an editor"),
+	OPT_GROUP("Type"),
+	OPT_BOOLEAN(0, "bool", &type_bool, "value is \"true\" or \"false\""),
+	OPT_BOOLEAN(0, "int", &type_int, "value is decimal number"),
+	OPT_BOOLEAN(0, "bool-or-int", &type_bool_or_int, NULL),
+	OPT_GROUP("Other"),
+	OPT_BOOLEAN('z', "null", &end_null, "end values with null character"),
+	OPT_END(),
+};
+
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
 	if (value_)
@@ -177,12 +216,11 @@ 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_color_name;
 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)) {
+	if (!strcmp(var, get_color_name)) {
 		if (!value)
 			config_error_nonbool(var);
 		color_parse(value, var, parsed_color);
@@ -191,47 +229,13 @@ 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)
-{
-	/*
-	 * 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);
-
-	if (!get_color_found && def_color)
-		color_parse(def_color, "command line", parsed_color);
-
-	fputs(parsed_color, stdout);
-	return 0;
-}
-
 static int stdout_is_tty;
 static int get_colorbool_found;
 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_name)) {
 		get_colorbool_found =
 			git_config_colorbool(var, value, stdout_is_tty);
 	}
@@ -246,41 +250,6 @@ static int git_get_colorbool_config(const char *var, const char *value,
 	return 0;
 }
 
-static int get_colorbool(int argc, const char **argv)
-{
-	/*
-	 * 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_color_slot = argv[0];
-	git_config(git_get_colorbool_config, NULL);
-
-	if (get_colorbool_found < 0) {
-		if (!strcmp(get_color_slot, "color.diff"))
-			get_colorbool_found = get_diff_color_found;
-		if (get_colorbool_found < 0)
-			get_colorbool_found = git_use_color_default;
-	}
-
-	if (argc == 1) {
-		return get_colorbool_found ? 0 : 1;
-	} else {
-		printf("%s\n", get_colorbool_found ? "true" : "false");
-		return 0;
-	}
-}
-
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
@@ -289,151 +258,180 @@ int cmd_config(int argc, const char **argv, const char *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 &&
-					file && errno)
-				die("unable to read config file %s: %s", file,
-				    strerror(errno));
-			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]) && file)
-				file = prefix_filename(file, strlen(file),
-						       argv[2]);
-			else
-				file = argv[2];
-			config_exclusive_filename = file;
-			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;
+	argc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);
+
+	if (use_global_config) {
+		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], "--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;
+	}
+	else if (use_system_config)
+		config_exclusive_filename = git_etc_gitconfig();
+	else if (given_config_file) {
+		if (!is_absolute_path(given_config_file) && file)
+			file = prefix_filename(file, strlen(file),
+					       given_config_file);
+		else
+			file = given_config_file;
+		config_exclusive_filename = file;
+	}
+
+	if (type_int)
+		type = T_INT;
+	else if (type_bool)
+		type = T_BOOL;
+	else if (type_bool_or_int)
+		type = T_BOOL_OR_INT;
+
+	if (end_null) {
+		term = '\0';
+		delim = '\n';
+		key_delim = '\n';
+	}
+
+	{
+		int action_sum;
+		action_sum = do_unset + do_unset_all + do_get + do_get_all + \
+			     do_get_regexp + do_add + do_list + do_edit + \
+			     do_rename_section + do_remove_section + do_replace_all;
+		if (action_sum > 1)
+			die ("Can't execute two actions at the same time.");
+		else if (action_sum == 0)
+			switch (argc)
+			{
+				case 1: do_get = 1; break;
+				case 2: do_add = 1; break;
+				case 3: do_replace_all = 1; break;
+				default:
+					usage_with_options(builtin_config_usage, builtin_config_options);
 			}
-			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
+	}
+
+	if (do_list) {
+		if (git_config(show_all_config, NULL) < 0 &&
+		    file && errno)
+			die("unable to read config file %s: %s", file,
+			    strerror(errno));
+	}
+	else if (do_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 (do_add) {
+		if (argc > 2)
+			die("Too many arguments.");
+		if (argc != 2)
+			die("Need name value.");
+		value = normalize_value(argv[0], argv[1]);
+		return git_config_set_multivar(argv[0], value, "^$", 0);
+	}
+	else if (do_replace_all) {
+		value = normalize_value(argv[0], argv[1]);
+		return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);
+	}
+	else if (do_get)
+		return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
+	else if (do_get_all) {
+		do_all = 1;
+		return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
+	}
+	else if (do_get_regexp) {
+		show_keys = 1;
+		use_key_regexp = 1;
+		do_all = 1;
+		return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
+	}
+	else if (do_unset) {
+		if (argc == 2)
+			return git_config_set_multivar(argv[0], NULL, argv[1], 0);
+		else
+			return git_config_set(argv[0], NULL);
+	}
+	else if (do_unset_all) {
+		return git_config_set_multivar(argv[0], NULL, (argc == 2 ? argv[1] : NULL), 1);
+	}
+	else if (do_rename_section) {
+		int ret;
+		if (argc > 2)
+			die("Too many arguments.");
+		if (argc != 2)
+			die("Need old_name new_name.");
+		ret = git_config_rename_section(argv[0], argv[1]);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)
+			die("No such section!");
+	}
+	else if (do_remove_section) {
+		int ret;
+		if (argc > 1)
+			die("Too many arguments.");
+		if (argc != 1)
+			die("Need section name.");
+		ret = git_config_rename_section(argv[0], NULL);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)
+			die("No such section!");
+	}
+	else if (get_color_name) {
+		const char *def_color = NULL;
+
+		switch (argc) {
+		case 2:
+			def_color = argv[1];
+			/* fallthru */
+		case 1:
+			get_color_name = argv[0];
 			break;
-		argc--;
-		argv++;
+		default:
+			die("Too many arguments.");
+		}
+
+		get_color_found = 0;
+		parsed_color[0] = '\0';
+		git_config(git_get_color_config, NULL);
+
+		if (!get_color_found && def_color)
+			color_parse(def_color, "command line", parsed_color);
+
+		fputs(parsed_color, stdout);
 	}
+	else if (get_colorbool_name) {
+		if (argc == 1)
+			stdout_is_tty = git_config_bool("command line", argv[0]);
+		else if (argc == 0)
+			stdout_is_tty = isatty(1);
+		else
+			die("Too many options.");
 
-	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);
+		get_colorbool_found = -1;
+		get_diff_color_found = -1;
+		git_config(git_get_colorbool_config, NULL);
+
+		if (get_colorbool_found < 0) {
+			if (!strcmp(get_colorbool_name, "color.diff"))
+				get_colorbool_found = get_diff_color_found;
+			if (get_colorbool_found < 0)
+				get_colorbool_found = git_use_color_default;
 		}
-	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 (argc == 0) {
+			return get_colorbool_found ? 0 : 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);
+			printf("%s\n", get_colorbool_found ? "true" : "false");
+			return 0;
 		}
-	case 1:
-	default:
-		usage(git_config_set_usage);
 	}
+
 	return 0;
 }
-- 
1.6.1.3

Re: [PATCH] config: Use parseopt.

[Junio C Hamano]

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

> Reorganizing the code to use parseopt as suggested by Johannes
> Schindelin.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin-config.c |  422 +++++++++++++++++++++++++++---------------------------
>  1 files changed, 210 insertions(+), 212 deletions(-)

Whew.  That's a lot of changes.

Is this just "I am using parseopt because I can", or "I want to do this
first because I am planning to do such and such things to this program,
and the current mess needs to be cleaned up first before doing so"?

I am asking this _not_ because I'd want to reject the patch if the answer
is former.

> diff --git a/builtin-config.c b/builtin-config.c
> index afc4393..f774902 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,42 @@ 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 do_list, do_edit, do_add, do_get, do_get_all, do_get_regexp, do_replace_all;
> +static int do_unset, do_unset_all, do_rename_section, do_remove_section;
> +static int type_int, type_bool, type_bool_or_int;

You can have either (no type specified, int, bool, bool-or-int) at the
end.  Using three independent variables does not feel right.

Hint: OPTION_SET_INT.

> +static const char *get_color_name, *get_colorbool_name;
> +static int end_null;
> +
> +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"),

Why CAPS?

> +	OPT_GROUP("Action"),
> +	OPT_BOOLEAN(0, "get", &do_get, "get value: name [value-regex]"),
> +	OPT_BOOLEAN(0, "get-all", &do_get_all, "get all values: key [value-regex]"),
> +	OPT_BOOLEAN(0, "get-regexp", &do_get_regexp, "get values for regexp: name-regex [value-regex]"),
> +	OPT_BOOLEAN(0, "replace-all", &do_replace_all, "replace all options: name [value [value_regex]"),

all matching variables?

> +	OPT_BOOLEAN(0, "add", &do_add, "adds a new option: name value"),

new variable?

> +	OPT_BOOLEAN(0, "unset", &do_unset, "removes an option: name [value-regex]"),

Please don't introduce a new noun "option" that has never been used to
mean a "configuration variable" in git documentation.  It unnecessarily
confuses everybody.

> +	OPT_BOOLEAN(0, "unset-all", &do_unset_all, "removes all matches: name [value-regex]"),
> +	OPT_BOOLEAN(0, "rename-section", &do_rename_section, "rename section: old-name new-name"),
> +	OPT_BOOLEAN(0, "remove-section", &do_remove_section, "remove a section: name"),
> +	OPT_BOOLEAN('l', "list", &do_list, "list all"),
> +	OPT_STRING(0, "get-color", &get_color_name, "name", "find the color configured: [default]"),

get-color is used to get the defined color for a given slot.  Please do not
rename it to "name" (see the original).

> +	OPT_STRING(0, "get-colorbool", &get_colorbool_name, "name", "find the color setting: [stdout-is-tty]"),

get-colorbool is used to get the boolean setting for a given configuration
variable.  Please do not rename it to "name" (see the original).

> +	OPT_BOOLEAN('e', "edit", &do_edit, "opens an editor"),
> +	OPT_GROUP("Type"),
> +	OPT_BOOLEAN(0, "bool", &type_bool, "value is \"true\" or \"false\""),
> +	OPT_BOOLEAN(0, "int", &type_int, "value is decimal number"),
> +	OPT_BOOLEAN(0, "bool-or-int", &type_bool_or_int, NULL),
> +	OPT_GROUP("Other"),
> +	OPT_BOOLEAN('z', "null", &end_null, "end values with null character"),

The name of the character is NUL (with a single el).  I'd prefer this to
say either "use machine readable output format", or "terminate values with NUL byte".

> +	OPT_END(),
> +};
> +
>  static int show_all_config(const char *key_, const char *value_, void *cb)
>  {
>  	if (value_)
> @@ -177,12 +216,11 @@ 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_color_name;

Why?

> @@ -289,151 +258,180 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> ...
> +	{
> +		int action_sum;
> +		action_sum = do_unset + do_unset_all + do_get + do_get_all + \
> +			     do_get_regexp + do_add + do_list + do_edit + \
> +			     do_rename_section + do_remove_section + do_replace_all;
> +		if (action_sum > 1)
> +			die ("Can't execute two actions at the same time.");

Hmph.  I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.

> +	else if (do_add) {
> +		if (argc > 2)
> +			die("Too many arguments.");
> +		if (argc != 2)
> +			die("Need name value.");
> +		value = normalize_value(argv[0], argv[1]);
> +		return git_config_set_multivar(argv[0], value, "^$", 0);

This part did not lose argc error checking, but...

> +	}
> +	else if (do_replace_all) {
> +		value = normalize_value(argv[0], argv[1]);
> +		return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);

You do not check argc here (nor in many "else if" below) to make sure you
have sufficient number of arguments.  "git config --unset" is now allowed
to segfault, and "git config --unset a b c d e f" can silently ignore
excess arguments for example?

> +	}
> +	else if (do_get)
> +		return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
> +	else if (do_get_all) {
> +		do_all = 1;
> +		return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
> +	}
> +	else if (do_get_regexp) {
> +		show_keys = 1;
> +		use_key_regexp = 1;
> +		do_all = 1;
> +		return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
> +	}
> +	else if (do_unset) {
> +		if (argc == 2)
> +			return git_config_set_multivar(argv[0], NULL, argv[1], 0);
> +		else
> +			return git_config_set(argv[0], NULL);
> +	}
> ... similar error-ridden parts omitted ...

> +	else if (get_color_name) {
> +		const char *def_color = NULL;
> +
> +		switch (argc) {
> +		case 2:
> +			def_color = argv[1];
> +			/* fallthru */
> +		case 1:
> +			get_color_name = argv[0];
>  			break;
> +		default:
> +			die("Too many arguments.");
> +		}
> +
> +		get_color_found = 0;
> +		parsed_color[0] = '\0';
> +		git_config(git_get_color_config, NULL);
> +
> +		if (!get_color_found && def_color)
> +			color_parse(def_color, "command line", parsed_color);
> +
> +		fputs(parsed_color, stdout);

This is actively worse than the original that uses a separate helper
function in readability.

>  	}
> +	else if (get_colorbool_name) {
> +		if (argc == 1)
> +			stdout_is_tty = git_config_bool("command line", argv[0]);
> +		else if (argc == 0)
> +			stdout_is_tty = isatty(1);
> +		else
> +			die("Too many options.");
>  
> +		get_colorbool_found = -1;
> +		get_diff_color_found = -1;
> +		git_config(git_get_colorbool_config, NULL);
> +
> +		if (get_colorbool_found < 0) {
> +			if (!strcmp(get_colorbool_name, "color.diff"))
> +				get_colorbool_found = get_diff_color_found;
> +			if (get_colorbool_found < 0)
> +				get_colorbool_found = git_use_color_default;
>  		}
> +
> +		if (argc == 0) {
> +			return get_colorbool_found ? 0 : 1;
>  		} else {
> +			printf("%s\n", get_colorbool_found ? "true" : "false");
> +			return 0;
>  		}

Likewise.

Overall, with the same number of lines, we lost a lot of error checking in
exchange for an ability to say "git config --remove-sec" instead of "git
config --remove-section", and "git config --help" may give an easier to
read message.

Given that "git config" is primarily meant for script use, this particular
round does not look like a good tradeoff to me.

Re: [PATCH] config: Use parseopt.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	OPT_STRING(0, "get-color", &get_color_name, "name", "find the color configured: [default]"),
>
> get-color is used to get the defined color for a given slot.  Please do not
> rename it to "name" (see the original).
>
>> +	OPT_STRING(0, "get-colorbool", &get_colorbool_name, "name", "find the color setting: [stdout-is-tty]"),
>
> get-colorbool is used to get the boolean setting for a given configuration
> variable.  Please do not rename it to "name" (see the original).
>

By the way, I think it might be more appropriate if you categorize the
above two (especially the "colorbool" one) in the "Types" section, as that
really is what is happening.  Instead of doing the usual "print the value
as a string", it does something magical.

> Overall, with the same number of lines, we lost a lot of error checking in
> exchange for an ability to say "git config --remove-sec" instead of "git
> config --remove-section", and "git config --help" may give an easier to
> read message.

And I forgot to mention the "option categorization" merit.

> Given that "git config" is primarily meant for script use, this particular
> round does not look like a good tradeoff to me.

Don't take this too negatively.  The tradeoff will improve if there aren't
these obvious bugs you can spot without even running the code.

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Reorganizing the code to use parseopt as suggested by Johannes
>> Schindelin.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  builtin-config.c |  422 +++++++++++++++++++++++++++---------------------------
>>  1 files changed, 210 insertions(+), 212 deletions(-)
>
> Whew.  That's a lot of changes.
>
> Is this just "I am using parseopt because I can", or "I want to do this
> first because I am planning to do such and such things to this program,
> and the current mess needs to be cleaned up first before doing so"?
>
> I am asking this _not_ because I'd want to reject the patch if the answer
> is former.

Then why are you asking?

This is more a "I would like to increase the chances of my patches
being accepted so I'd do some chores to gain the trust of some
developers", and Johannes Schindelin was pushing me to do this.

Also it's a bit of "I would like to improve git and learn the API
while doing so".

>> diff --git a/builtin-config.c b/builtin-config.c
>> index afc4393..f774902 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,42 @@ 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 do_list, do_edit, do_add, do_get, do_get_all, do_get_regexp, do_replace_all;
>> +static int do_unset, do_unset_all, do_rename_section, do_remove_section;
>> +static int type_int, type_bool, type_bool_or_int;
>
> You can have either (no type specified, int, bool, bool-or-int) at the
> end.  Using three independent variables does not feel right.
>
> Hint: OPTION_SET_INT.

That definitely makes things easier, it would have been nice to see an
example of this; I didn't knew it was there.

The only problem is that --bool and --int would be possible in the
same command and there would be no way to output an error, but I guess
that's not a big problem.

>> +static const char *get_color_name, *get_colorbool_name;
>> +static int end_null;
>> +
>> +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"),
>
> Why CAPS?

I looked at some other code, like builtin-commit.c and that's what is
used there.

>> +     OPT_GROUP("Action"),
>> +     OPT_BOOLEAN(0, "get", &do_get, "get value: name [value-regex]"),
>> +     OPT_BOOLEAN(0, "get-all", &do_get_all, "get all values: key [value-regex]"),
>> +     OPT_BOOLEAN(0, "get-regexp", &do_get_regexp, "get values for regexp: name-regex [value-regex]"),
>> +     OPT_BOOLEAN(0, "replace-all", &do_replace_all, "replace all options: name [value [value_regex]"),
>
> all matching variables?

Ok.

>> +     OPT_BOOLEAN(0, "add", &do_add, "adds a new option: name value"),
>
> new variable?

Ok.

>> +     OPT_BOOLEAN(0, "unset", &do_unset, "removes an option: name [value-regex]"),
>
> Please don't introduce a new noun "option" that has never been used to
> mean a "configuration variable" in git documentation.  It unnecessarily
> confuses everybody.

Ok.

And true, "config" files don't only contain options.

>> +     OPT_BOOLEAN(0, "unset-all", &do_unset_all, "removes all matches: name [value-regex]"),
>> +     OPT_BOOLEAN(0, "rename-section", &do_rename_section, "rename section: old-name new-name"),
>> +     OPT_BOOLEAN(0, "remove-section", &do_remove_section, "remove a section: name"),
>> +     OPT_BOOLEAN('l', "list", &do_list, "list all"),
>> +     OPT_STRING(0, "get-color", &get_color_name, "name", "find the color configured: [default]"),
>
> get-color is used to get the defined color for a given slot.  Please do not
> rename it to "name" (see the original).

The documentation says "Find the color configured for `name`" so the
first argument would be "name", I'm just storing that argument.

Perhaps the documentation needs to be improved?

>> +     OPT_STRING(0, "get-colorbool", &get_colorbool_name, "name", "find the color setting: [stdout-is-tty]"),
>
> get-colorbool is used to get the boolean setting for a given configuration
> variable.  Please do not rename it to "name" (see the original).

Same as get-color, right?

>> +     OPT_BOOLEAN('e', "edit", &do_edit, "opens an editor"),
>> +     OPT_GROUP("Type"),
>> +     OPT_BOOLEAN(0, "bool", &type_bool, "value is \"true\" or \"false\""),
>> +     OPT_BOOLEAN(0, "int", &type_int, "value is decimal number"),
>> +     OPT_BOOLEAN(0, "bool-or-int", &type_bool_or_int, NULL),
>> +     OPT_GROUP("Other"),
>> +     OPT_BOOLEAN('z', "null", &end_null, "end values with null character"),
>
> The name of the character is NUL (with a single el).  I'd prefer this to
> say either "use machine readable output format", or "terminate values with NUL byte".

I'd prefer "terminate values with NUL byte".

>> +     OPT_END(),
>> +};
>> +
>>  static int show_all_config(const char *key_, const char *value_, void *cb)
>>  {
>>       if (value_)
>> @@ -177,12 +216,11 @@ 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_color_name;
>
> Why?

Because the documentation says the argument is a 'name'.

>> @@ -289,151 +258,180 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>> ...
>> +     {
>> +             int action_sum;
>> +             action_sum = do_unset + do_unset_all + do_get + do_get_all + \
>> +                          do_get_regexp + do_add + do_list + do_edit + \
>> +                          do_rename_section + do_remove_section + do_replace_all;
>> +             if (action_sum > 1)
>> +                     die ("Can't execute two actions at the same time.");
>
> Hmph.  I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.

Yeap, definitely, thanks for the suggestion.

Again, it would have been nice to see any example of this.

>> +     else if (do_add) {
>> +             if (argc > 2)
>> +                     die("Too many arguments.");
>> +             if (argc != 2)
>> +                     die("Need name value.");
>> +             value = normalize_value(argv[0], argv[1]);
>> +             return git_config_set_multivar(argv[0], value, "^$", 0);
>
> This part did not lose argc error checking, but...
>
>> +     }
>> +     else if (do_replace_all) {
>> +             value = normalize_value(argv[0], argv[1]);
>> +             return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);
>
> You do not check argc here (nor in many "else if" below) to make sure you
> have sufficient number of arguments.  "git config --unset" is now allowed
> to segfault, and "git config --unset a b c d e f" can silently ignore
> excess arguments for example?

Yes the arguments check need to be revised.

My hope was somebody would review this and suggest a clever and
generic way of doing this. Perhaps a util function check_min_args, or
maybe something in parseopt that receives the number of args?

Also, since I sent the patch I've learned that if argc is 2, then
argv[2] will be NULL, so the argc == 3 check is redundant.

>> +     }
>> +     else if (do_get)
>> +             return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
>> +     else if (do_get_all) {
>> +             do_all = 1;
>> +             return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
>> +     }
>> +     else if (do_get_regexp) {
>> +             show_keys = 1;
>> +             use_key_regexp = 1;
>> +             do_all = 1;
>> +             return get_value(argv[0], (argc == 2 ? argv[1] : NULL));
>> +     }
>> +     else if (do_unset) {
>> +             if (argc == 2)
>> +                     return git_config_set_multivar(argv[0], NULL, argv[1], 0);
>> +             else
>> +                     return git_config_set(argv[0], NULL);
>> +     }
>> ... similar error-ridden parts omitted ...
>
>> +     else if (get_color_name) {
>> +             const char *def_color = NULL;
>> +
>> +             switch (argc) {
>> +             case 2:
>> +                     def_color = argv[1];
>> +                     /* fallthru */
>> +             case 1:
>> +                     get_color_name = argv[0];
>>                       break;
>> +             default:
>> +                     die("Too many arguments.");
>> +             }
>> +
>> +             get_color_found = 0;
>> +             parsed_color[0] = '\0';
>> +             git_config(git_get_color_config, NULL);
>> +
>> +             if (!get_color_found && def_color)
>> +                     color_parse(def_color, "command line", parsed_color);
>> +
>> +             fputs(parsed_color, stdout);
>
> This is actively worse than the original that uses a separate helper
> function in readability.

The original separate helper functions where helping parse the args.
Since that's not happening any more I saw no reason to pass the argc
and argv variables, so I thought maybe these options should not be
treated any differently than other options.

Anyway, I can create the helper functions again, and maybe have some
function parameters instead of passing argc and argv

>>       }
>> +     else if (get_colorbool_name) {
>> +             if (argc == 1)
>> +                     stdout_is_tty = git_config_bool("command line", argv[0]);
>> +             else if (argc == 0)
>> +                     stdout_is_tty = isatty(1);
>> +             else
>> +                     die("Too many options.");
>>
>> +             get_colorbool_found = -1;
>> +             get_diff_color_found = -1;
>> +             git_config(git_get_colorbool_config, NULL);
>> +
>> +             if (get_colorbool_found < 0) {
>> +                     if (!strcmp(get_colorbool_name, "color.diff"))
>> +                             get_colorbool_found = get_diff_color_found;
>> +                     if (get_colorbool_found < 0)
>> +                             get_colorbool_found = git_use_color_default;
>>               }
>> +
>> +             if (argc == 0) {
>> +                     return get_colorbool_found ? 0 : 1;
>>               } else {
>> +                     printf("%s\n", get_colorbool_found ? "true" : "false");
>> +                     return 0;
>>               }
>
> Likewise.
>
> Overall, with the same number of lines, we lost a lot of error checking in
> exchange for an ability to say "git config --remove-sec" instead of "git
> config --remove-section", and "git config --help" may give an easier to
> read message.
>
> Given that "git config" is primarily meant for script use, this particular
> round does not look like a good tradeoff to me.

That doesn't mean it shouldn't have a nice UI.

Also, I think the code would be easier to maintain with parseopt.

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 11:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +    OPT_STRING(0, "get-color", &get_color_name, "name", "find the color configured: [default]"),
>>
>> get-color is used to get the defined color for a given slot.  Please do not
>> rename it to "name" (see the original).
>>
>>> +    OPT_STRING(0, "get-colorbool", &get_colorbool_name, "name", "find the color setting: [stdout-is-tty]"),
>>
>> get-colorbool is used to get the boolean setting for a given configuration
>> variable.  Please do not rename it to "name" (see the original).
>>
>
> By the way, I think it might be more appropriate if you categorize the
> above two (especially the "colorbool" one) in the "Types" section, as that
> really is what is happening.  Instead of doing the usual "print the value
> as a string", it does something magical.

All right.

I haven't used most of the config options, so I had to learn and try
them; color/colorbool are the ones I still don't get.

>> Overall, with the same number of lines, we lost a lot of error checking in
>> exchange for an ability to say "git config --remove-sec" instead of "git
>> config --remove-section", and "git config --help" may give an easier to
>> read message.
>
> And I forgot to mention the "option categorization" merit.
>
>> Given that "git config" is primarily meant for script use, this particular
>> round does not look like a good tradeoff to me.
>
> Don't take this too negatively.  The tradeoff will improve if there aren't
> these obvious bugs you can spot without even running the code.

No problem, I probably should have explained that this was RFC, I knew
there were issues in the code.

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Felipe Contreras wrote:

> Then why are you asking?

Out of curiosity, I guess, as it would happen to answer my curiosity as 
well.

> This is more a "I would like to increase the chances of my patches
> being accepted so I'd do some chores to gain the trust of some
> developers", and Johannes Schindelin was pushing me to do this.

Heh, I'll gladly take the blame for that!

Note that in contrast to Junio, I think "git config" is a chimera between 
plumbing and porcelain, and would benefit tremendously from a nice help.

> >> +static int type_int, type_bool, type_bool_or_int;
> >
> > You can have either (no type specified, int, bool, bool-or-int) at the
> > end.  Using three independent variables does not feel right.
> >
> > Hint: OPTION_SET_INT.
> 
> That definitely makes things easier, it would have been nice to see an
> example of this; I didn't knew it was there.
> 
> The only problem is that --bool and --int would be possible in the
> same command and there would be no way to output an error, but I guess
> that's not a big problem.

I think that is okay.

> >> +     else if (do_add) {
> >> +             if (argc > 2)
> >> +                     die("Too many arguments.");
> >> +             if (argc != 2)
> >> +                     die("Need name value.");
> >> +             value = normalize_value(argv[0], argv[1]);
> >> +             return git_config_set_multivar(argv[0], value, "^$", 0);
> >
> > This part did not lose argc error checking, but...
> >
> >> +     }
> >> +     else if (do_replace_all) {
> >> +             value = normalize_value(argv[0], argv[1]);
> >> +             return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);
> >
> > You do not check argc here (nor in many "else if" below) to make sure you
> > have sufficient number of arguments.  "git config --unset" is now allowed
> > to segfault, and "git config --unset a b c d e f" can silently ignore
> > excess arguments for example?
> 
> Yes the arguments check need to be revised.
> 
> My hope was somebody would review this and suggest a clever and
> generic way of doing this. Perhaps a util function check_min_args, or
> maybe something in parseopt that receives the number of args?

Maybe a helper, yes.  Something like:

	static void check_argc(int argc, int min, int max) {
		if (argc >= min && argc <= max)
			return;
		fprintf(stderr, "Wrong number of arguments: %d\n", argc);
		usage_with_options(config_usage, config_options);
	}

Of course, this assumes that config_usage and config_options are global...

> Also, I think the code would be easier to maintain with parseopt.

I agree.

Thanks,
Dscho

Re: [PATCH] config: Use parseopt.

[Jakub Narebski]

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

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

[...]
> -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
> +};

Just asking: why this change?

> +	OPT_BOOLEAN('z', "null", &end_null, "end values with null character"),

Terminate and NUL.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

[PATCH v2] config: Use parseopt.

[Felipe Contreras]

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

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

diff --git a/builtin-config.c b/builtin-config.c
index 6937eaf..aefec0d 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_)
@@ -179,7 +235,6 @@ static char *normalize_value(const char *key, const char *value)
 static int get_color_found;
 static const char *get_color_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)) {
@@ -191,29 +246,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);
@@ -222,7 +256,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;
@@ -231,7 +264,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);
 	}
@@ -246,39 +279,24 @@ 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_color_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;
 	}
 
-	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 *prefix)
@@ -289,151 +307,127 @@ int cmd_config(int argc, const char **argv, const char *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 &&
-					file && errno)
-				die("unable to read config file %s: %s", file,
-				    strerror(errno));
-			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]) && file)
-				file = prefix_filename(file, strlen(file),
-						       argv[2]);
-			else
-				file = argv[2];
-			config_exclusive_filename = file;
-			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);
+	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) && file)
+			file = prefix_filename(file, strlen(file),
+					       given_config_file);
+		else
+			file = given_config_file;
+		config_exclusive_filename = 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 &&
+		    file && errno)
+			die("unable to read config file %s: %s", file,
+			    strerror(errno));
+	}
+	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

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 1:52 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Reorganizing the code to use parseopt as suggested by Johannes
>> Schindelin.
>
> [...]
>> -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
>> +};
>
> Just asking: why this change?

So it's easier to understand and maintain?

>> +     OPT_BOOLEAN('z', "null", &end_null, "end values with null character"),
>
> Terminate and NUL.

Ok. Junio already suggested that.

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 1:40 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>
>> Then why are you asking?
>
> Out of curiosity, I guess, as it would happen to answer my curiosity as
> well.
>
>> This is more a "I would like to increase the chances of my patches
>> being accepted so I'd do some chores to gain the trust of some
>> developers", and Johannes Schindelin was pushing me to do this.
>
> Heh, I'll gladly take the blame for that!
>
> Note that in contrast to Junio, I think "git config" is a chimera between
> plumbing and porcelain, and would benefit tremendously from a nice help.

I agree on that.

>> >> +static int type_int, type_bool, type_bool_or_int;
>> >
>> > You can have either (no type specified, int, bool, bool-or-int) at the
>> > end.  Using three independent variables does not feel right.
>> >
>> > Hint: OPTION_SET_INT.
>>
>> That definitely makes things easier, it would have been nice to see an
>> example of this; I didn't knew it was there.
>>
>> The only problem is that --bool and --int would be possible in the
>> same command and there would be no way to output an error, but I guess
>> that's not a big problem.
>
> I think that is okay.
>
>> >> +     else if (do_add) {
>> >> +             if (argc > 2)
>> >> +                     die("Too many arguments.");
>> >> +             if (argc != 2)
>> >> +                     die("Need name value.");
>> >> +             value = normalize_value(argv[0], argv[1]);
>> >> +             return git_config_set_multivar(argv[0], value, "^$", 0);
>> >
>> > This part did not lose argc error checking, but...
>> >
>> >> +     }
>> >> +     else if (do_replace_all) {
>> >> +             value = normalize_value(argv[0], argv[1]);
>> >> +             return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);
>> >
>> > You do not check argc here (nor in many "else if" below) to make sure you
>> > have sufficient number of arguments.  "git config --unset" is now allowed
>> > to segfault, and "git config --unset a b c d e f" can silently ignore
>> > excess arguments for example?
>>
>> Yes the arguments check need to be revised.
>>
>> My hope was somebody would review this and suggest a clever and
>> generic way of doing this. Perhaps a util function check_min_args, or
>> maybe something in parseopt that receives the number of args?
>
> Maybe a helper, yes.  Something like:
>
>        static void check_argc(int argc, int min, int max) {
>                if (argc >= min && argc <= max)
>                        return;
>                fprintf(stderr, "Wrong number of arguments: %d\n", argc);
>                usage_with_options(config_usage, config_options);
>        }
>
> Of course, this assumes that config_usage and config_options are global...

Cool.

I've sent a new patch with this helper (a bit modified), and all the
changes Junio suggested.

I still have a few doubts:

1) --list when no config file is given uses all the config files,
wouldn't it make sense to have a --repo option?

2) --get-colorbool prints "true" or "false" only when there are two
arguments, is that correct or should stdout_is_tty be used instead?

3) should the documentation be updated for the --get-color* options to
use 'slot' instead of 'name'?

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Jeff King]

On Sat, Feb 14, 2009 at 03:52:56AM -0800, Jakub Narebski wrote:

> > -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
> > +};
> 
> Just asking: why this change?

If you are asking about this specific hunk, it is because parse-options
will generate the list of options.

-Peff

Re: [PATCH v2] config: Use parseopt.

[Jeff King]

On Sat, Feb 14, 2009 at 02:03:09PM +0200, Felipe Contreras wrote:

> Reorganizing the code to use parseopt as suggested by Johannes
> Schindelin.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin-config.c |  370 ++++++++++++++++++++++++++---------------------------
>  1 files changed, 182 insertions(+), 188 deletions(-)

What is this built on top of? I get very large conflicts applying it on
top of Junio's "master".

-Peff

Re: [PATCH v2] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 5:21 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 14, 2009 at 02:03:09PM +0200, Felipe Contreras wrote:
>
>> Reorganizing the code to use parseopt as suggested by Johannes
>> Schindelin.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  builtin-config.c |  370 ++++++++++++++++++++++++++---------------------------
>>  1 files changed, 182 insertions(+), 188 deletions(-)
>
> What is this built on top of? I get very large conflicts applying it on
> top of Junio's "master".

By mistake I used a dirty master branch, but it should apply on top of 'pu'.

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Felipe Contreras wrote:

> 1) --list when no config file is given uses all the config files,
> wouldn't it make sense to have a --repo option?

The idea of --list is not "cat .git/config".  The idea is to help users or 
scripts to list the current settings (_including_ the global settings).

You can force showing the repo-specific config with "git --file 
.git/config", though.

Ciao,
Dscho

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>
>> 1) --list when no config file is given uses all the config files,
>> wouldn't it make sense to have a --repo option?
>
> The idea of --list is not "cat .git/config".  The idea is to help users or
> scripts to list the current settings (_including_ the global settings).
>
> You can force showing the repo-specific config with "git --file
> .git/config", though.

When you are on the root directory of the repo, and you don't have
GIT_DIR, or --git-dir.

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Felipe Contreras wrote:

> On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
> >
> >> 1) --list when no config file is given uses all the config files,
> >> wouldn't it make sense to have a --repo option?
> >
> > The idea of --list is not "cat .git/config".  The idea is to help users or
> > scripts to list the current settings (_including_ the global settings).
> >
> > You can force showing the repo-specific config with "git --file
> > .git/config", though.
> 
> When you are on the root directory of the repo, and you don't have
> GIT_DIR, or --git-dir.

When I wrote my response, I briefly considered if I had to be verbose, and 
decided against it.

But this is what I should have written:

	git --file $(git rev-parse --git-dir)/config --list

Ciao,
Dscho

Re: [PATCH] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Johannes Schindelin wrote:

> On Sat, 14 Feb 2009, Felipe Contreras wrote:
> 
> > On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > > Hi,
> > >
> > > On Sat, 14 Feb 2009, Felipe Contreras wrote:
> > >
> > >> 1) --list when no config file is given uses all the config files,
> > >> wouldn't it make sense to have a --repo option?
> > >
> > > The idea of --list is not "cat .git/config".  The idea is to help users or
> > > scripts to list the current settings (_including_ the global settings).
> > >
> > > You can force showing the repo-specific config with "git --file
> > > .git/config", though.
> > 
> > When you are on the root directory of the repo, and you don't have
> > GIT_DIR, or --git-dir.
> 
> When I wrote my response, I briefly considered if I had to be verbose, and 
> decided against it.
> 
> But this is what I should have written:
> 
> 	git --file $(git rev-parse --git-dir)/config --list

Okay, before anybody points out that I was not verbose enough -- again -- 
this is what I really should have written:

	git --file "$(git rev-parse --git-dir)"/config --list

Ciao,
Dscho

Re: [PATCH] config: Use parseopt.

[Junio C Hamano]

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

> On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Reorganizing the code to use parseopt as suggested by Johannes
>>> Schindelin.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>>  builtin-config.c |  422 +++++++++++++++++++++++++++---------------------------
>>>  1 files changed, 210 insertions(+), 212 deletions(-)
>>
>> Whew.  That's a lot of changes.
>>
>> Is this just "I am using parseopt because I can", or "I want to do this
>> first because I am planning to do such and such things to this program,
>> and the current mess needs to be cleaned up first before doing so"?
>>
>> I am asking this _not_ because I'd want to reject the patch if the answer
>> is former.
>
> Then why are you asking?

So that I can prioritize topics and patches that are more important in the
short, medium and longer time according to the urgency.  For example, it
is a very bad tradeoff for me to take "This does not change anything
externally visible, but cleans up the implementation" near or during the
release freeze.  "This cleans up, and then the next one will fix a
longstanding bug building on top of the cleaner code" is different.

And preferably the latter kind should be queued for 'maint', not for
'master', if the fix that will come later is important enough.

> This is more a "I would like to increase the chances of my patches
> being accepted so I'd do some chores to gain the trust of some
> developers", and Johannes Schindelin was pushing me to do this.
>
> Also it's a bit of "I would like to improve git and learn the API
> while doing so".

I personally do not think "I rewrote this command's option parser using
parseopt" earns any "trust point".  I think the latter is a *great* thing
to do, though.

> The only problem is that --bool and --int would be possible in the
> same command and there would be no way to output an error, but I guess
> that's not a big problem.

The existing code does not have a check, and you are right, it would be
nice to have such an error check, perhaps as a separate follow-up patch.

>>> +static const char *get_color_name, *get_colorbool_name;
>>> +static int end_null;
>>> +
>>> +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"),
>>
>> Why CAPS?
>
> I looked at some other code, like builtin-commit.c and that's what is
> used there.

I see, there are mixtures; apply, blame, fmt-merge-msg and tag say "file"
and commit and fast-export say "FILE".  commit's one could be almost
justifyable, because it uses caps in all (except for -u=<mode>), but
output from fast-export is a different story.  Compare "git commit -h" and
"git fast-export -h" to see what I mean.

One possible patch could make "git cmd -h" messages for all "cmd" spell
their "fill your value here" markings consistently, and the explanation of
the change in the proposed commit log message would have

 (1) an analysis similar to the one I did in the above paragraph (but
     doing it more thouroughly; I didn't look beyond commands that grep
     for "file" or "FILE" hits in the sources) of the current situation;
     and

 (2) the reason why you picked the case (be it lower or upper, I do not
     deeply care either way).

Such a patch could earn a "trust point", not because such it is extremely
important for the system's usability (it is *not*), but because it is a
good way to demonstrate that you can do a thorough job and think things
through.

>> Hmph.  I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.
>
> Yeap, definitely, thanks for the suggestion.
>
> Again, it would have been nice to see any example of this.

I actually had to look for HAS_MULTI_BITS because I *knew* such a facility
existed, I did not recall what it was called, and api-parse-options.txt in
Documentation/technical did not talk about it.  We can definitely improve
things in many places.

Mistakes made in the past and resulting flaws that remain in the current
source do not justify a new patch adding more mistakes to the codebase.
Reviewers help the author from adding more.

One bad thing about the current process (and I am certainly one of the
guilty parties because I do a lot of reviews) around this area is that a
review comment that points out a mistake similar to the ones made in the
past sound to the author of the patch as if the reviewer is telling that
the patch will not be accepted unless the existing mistakes are also fixed
by the patch author.  Such a review certainly does not mean that --- it is
more likely that the patch author simply mimicked existing code that is
doing a wrong thing without realizing it being wrong, and because the
reviewer *did not know* that the mistake was copied from elsewhere, he
just pointed it out the one in the patch.

In such a situation, I've seen two kinds of responses from the patch
author (this assumes that the nit pointed out by the reviewer was indeed a
good thing to fix):

 * This and that code already do things in a similar way.  I won't be
   changing my patch.

 * Ok, I'll fix the issue in my patch, but this and that code have the
   same mistake.  Should I fix them in the same patch as well?

Neither is an exactly optimal solution.  It is very likely that the patch
author knows more of the existing instances of the same mistake than the
reviewer, but there probably are more instances than he tried to defend
his patch with.  The places he knows of were found merely because he was
looking for an example to do something similar to what he wanted to do,
not because he was looking for all instances of a specific class of
mistake to fix in the existing code.

An ideal response would be a follow up patch that fixes the issue that the
original patch had, and a separate message that summarizes such issues in
the existing codebase (saying that the instances listed are not
exhaustive).  The patch author can attack them in a separate series if he
wants to, and that would certainly be appreciated, but he does not have
to.  It is a separate issue that others (not necessarily the reviewer who
first pointed out the mistake in the patch) can attack.

Unfortunately, not many patch authors write such a summary.  Sometimes we
see summaries on things that were discussed but nobody has followed
through posted by third parties (including myself), but we do not seem to
have enough helpers to do that either.  This does not take much technical
skills but is a good "trust point" earner.

> Yes the arguments check need to be revised.
>
> My hope was somebody would review this and suggest a clever and
> generic way of doing this. Perhaps a util function check_min_args, or
> maybe something in parseopt that receives the number of args?

I thought what the original code before your patch had was quite
reasonable, switching on argc.

> Anyway, I can create the helper functions again, and maybe have some
> function parameters instead of passing argc and argv

It might actually be a good idea to turn the cascade of if..elseif..fi
into a form of look-up in a table whose elements have a pointer to a
function.  A loooong function like this cmd_config() is very hard to
follow its logic.

>> Overall, with the same number of lines, we lost a lot of error checking in
>> exchange for an ability to say "git config --remove-sec" instead of "git
>> config --remove-section", and "git config --help" may give an easier to
>> read message.
>>
>> Given that "git config" is primarily meant for script use, this particular
>> round does not look like a good tradeoff to me.
>
> That doesn't mean it shouldn't have a nice UI.

The need for "UI" is much less than that of Porcelain for this particular
command, so it certainly affects the tradeoff points.

> Also, I think the code would be easier to maintain with parseopt.

Not with the patch you posted, but yes, the current mess could be
improved, and parseopt could be one of the tools for such code
restructuring.

Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Felipe Contreras wrote:

> @@ -231,7 +264,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);
>  	}

Name changes like this make it harder to read the patch; can you separate 
that change out into its own patch?

> +	if (use_global_config) {
> +		char *home = getenv("HOME");
> +		if (home) {
> +			char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
> +			config_exclusive_filename = user_config;

In a subsequent patch, you might add a check only one of --global, 
--system or --file was given.

> +	else if (given_config_file) {
> +		if (!is_absolute_path(given_config_file) && file)
> +			file = prefix_filename(file, strlen(file),
> +					       given_config_file);
> +		else
> +			file = given_config_file;
> +		config_exclusive_filename = file;

It took me a considerable amount of time to figure out that "file" is 
actually the "prefix"!  That cleanup would be nice to have before the 
parseopt patch, methinks, especially since the code is reindented, and 
thus hard to follow in the diff.

> +	if (actions & ACTION_LIST) {
> +		if (git_config(show_all_config, NULL) < 0 &&
> +		    file && errno)

Should this not be config_exclusive_filename?

> +			die("unable to read config file %s: %s", file,
> +			    strerror(errno));

Do we really only want to die() in case we know the file name?  AFAICT at 
this point we have no idea in which of the possibly three files the error 
occurred.  And there need not be any errno set, for example when there was 
a parse error.

> +	else if (actions & ACTION_EDIT) {
> +		const char *config_filename;
> +		if (config_exclusive_filename)
> +			config_filename = config_exclusive_filename;
> +		else
> +			config_filename = git_path("config");

Why not reuse config_exclusive_filename here?

> +	else if (actions & ACTION_ADD) {
> +		check_argc(argc, 2, 2);

BTW what about check_argc() in the previous two cases?

> +		return git_config_set_multivar(argv[0], value, "^$", 0);

Now that I see this, there is another idea for a possible cleanup after 
the parseoptification: cmd_config() should not return -1, as that will be 
turned into the exit status.  So maybe prefix the return value with "!!"?

Or maybe even better: set a variable "ret" and at the end of cmd_config(), 
"return !!ret;"?

The rest looks good to me.
 
Thanks,
Dscho

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 9:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Reorganizing the code to use parseopt as suggested by Johannes
>>>> Schindelin.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>> ---
>>>>  builtin-config.c |  422 +++++++++++++++++++++++++++---------------------------
>>>>  1 files changed, 210 insertions(+), 212 deletions(-)
>>>
>>> Whew.  That's a lot of changes.
>>>
>>> Is this just "I am using parseopt because I can", or "I want to do this
>>> first because I am planning to do such and such things to this program,
>>> and the current mess needs to be cleaned up first before doing so"?
>>>
>>> I am asking this _not_ because I'd want to reject the patch if the answer
>>> is former.
>>
>> Then why are you asking?
>
> So that I can prioritize topics and patches that are more important in the
> short, medium and longer time according to the urgency.  For example, it
> is a very bad tradeoff for me to take "This does not change anything
> externally visible, but cleans up the implementation" near or during the
> release freeze.  "This cleans up, and then the next one will fix a
> longstanding bug building on top of the cleaner code" is different.
>
> And preferably the latter kind should be queued for 'maint', not for
> 'master', if the fix that will come later is important enough.

Oh, in that case, no, I don't have any further patches on top of this.

>> This is more a "I would like to increase the chances of my patches
>> being accepted so I'd do some chores to gain the trust of some
>> developers", and Johannes Schindelin was pushing me to do this.
>>
>> Also it's a bit of "I would like to improve git and learn the API
>> while doing so".
>
> I personally do not think "I rewrote this command's option parser using
> parseopt" earns any "trust point".  I think the latter is a *great* thing
> to do, though.

I disagree. Making a patch pass through all the filters must mean
something, and the more patches the more trust.

>> The only problem is that --bool and --int would be possible in the
>> same command and there would be no way to output an error, but I guess
>> that's not a big problem.
>
> The existing code does not have a check, and you are right, it would be
> nice to have such an error check, perhaps as a separate follow-up patch.

Yeap.

>>>> +static const char *get_color_name, *get_colorbool_name;
>>>> +static int end_null;
>>>> +
>>>> +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"),
>>>
>>> Why CAPS?
>>
>> I looked at some other code, like builtin-commit.c and that's what is
>> used there.
>
> I see, there are mixtures; apply, blame, fmt-merge-msg and tag say "file"
> and commit and fast-export say "FILE".  commit's one could be almost
> justifyable, because it uses caps in all (except for -u=<mode>), but
> output from fast-export is a different story.  Compare "git commit -h" and
> "git fast-export -h" to see what I mean.
>
> One possible patch could make "git cmd -h" messages for all "cmd" spell
> their "fill your value here" markings consistently, and the explanation of
> the change in the proposed commit log message would have
>
>  (1) an analysis similar to the one I did in the above paragraph (but
>     doing it more thouroughly; I didn't look beyond commands that grep
>     for "file" or "FILE" hits in the sources) of the current situation;
>     and
>
>  (2) the reason why you picked the case (be it lower or upper, I do not
>     deeply care either way).
>
> Such a patch could earn a "trust point", not because such it is extremely
> important for the system's usability (it is *not*), but because it is a
> good way to demonstrate that you can do a thorough job and think things
> through.

I might do that.

>>> Hmph.  I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.
>>
>> Yeap, definitely, thanks for the suggestion.
>>
>> Again, it would have been nice to see any example of this.
>
> I actually had to look for HAS_MULTI_BITS because I *knew* such a facility
> existed, I did not recall what it was called, and api-parse-options.txt in
> Documentation/technical did not talk about it.  We can definitely improve
> things in many places.
>
> Mistakes made in the past and resulting flaws that remain in the current
> source do not justify a new patch adding more mistakes to the codebase.
> Reviewers help the author from adding more.
>
> One bad thing about the current process (and I am certainly one of the
> guilty parties because I do a lot of reviews) around this area is that a
> review comment that points out a mistake similar to the ones made in the
> past sound to the author of the patch as if the reviewer is telling that
> the patch will not be accepted unless the existing mistakes are also fixed
> by the patch author.  Such a review certainly does not mean that --- it is
> more likely that the patch author simply mimicked existing code that is
> doing a wrong thing without realizing it being wrong, and because the
> reviewer *did not know* that the mistake was copied from elsewhere, he
> just pointed it out the one in the patch.
>
> In such a situation, I've seen two kinds of responses from the patch
> author (this assumes that the nit pointed out by the reviewer was indeed a
> good thing to fix):
>
>  * This and that code already do things in a similar way.  I won't be
>   changing my patch.
>
>  * Ok, I'll fix the issue in my patch, but this and that code have the
>   same mistake.  Should I fix them in the same patch as well?
>
> Neither is an exactly optimal solution.  It is very likely that the patch
> author knows more of the existing instances of the same mistake than the
> reviewer, but there probably are more instances than he tried to defend
> his patch with.  The places he knows of were found merely because he was
> looking for an example to do something similar to what he wanted to do,
> not because he was looking for all instances of a specific class of
> mistake to fix in the existing code.
>
> An ideal response would be a follow up patch that fixes the issue that the
> original patch had, and a separate message that summarizes such issues in
> the existing codebase (saying that the instances listed are not
> exhaustive).  The patch author can attack them in a separate series if he
> wants to, and that would certainly be appreciated, but he does not have
> to.  It is a separate issue that others (not necessarily the reviewer who
> first pointed out the mistake in the patch) can attack.
>
> Unfortunately, not many patch authors write such a summary.  Sometimes we
> see summaries on things that were discussed but nobody has followed
> through posted by third parties (including myself), but we do not seem to
> have enough helpers to do that either.  This does not take much technical
> skills but is a good "trust point" earner.

For me it's easier, and more fun to write a separate patch that fixes
the issues than writing a summary, but in the mean time it makes sense
to do one patch at a time and make sure it really gets merged,
specially since I don't have many, and it's motivating to know it's
going to be on the next release.

But, if there's code that already has the same issues the patch has,
it doesn't look like a good argument for patch rejection. Perhaps the
quality standards increased, but on the other hand things wouldn't get
worst by applying the patch.

>> Yes the arguments check need to be revised.
>>
>> My hope was somebody would review this and suggest a clever and
>> generic way of doing this. Perhaps a util function check_min_args, or
>> maybe something in parseopt that receives the number of args?
>
> I thought what the original code before your patch had was quite
> reasonable, switching on argc.

The code for --replace-all was repeated on 3 places, one for each case
of args provided, and similarly for other options. Don't you think
it's better to have the code in one single place like in my patch?

Also the git_config_set_usage variable was getting quite bug, which
was the reason Johannes suggested this change. Which probably lead to
some easy mistakes like the fact that -l is not mentioned in the
usage.

>> Anyway, I can create the helper functions again, and maybe have some
>> function parameters instead of passing argc and argv
>
> It might actually be a good idea to turn the cascade of if..elseif..fi
> into a form of look-up in a table whose elements have a pointer to a
> function.  A loooong function like this cmd_config() is very hard to
> follow its logic.

Perhaps, but at least it's not bigger than the previous one.

<snip/>

-- 
Felipe Contreras

Re: [PATCH v2] config: Use parseopt.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +	else if (given_config_file) {
>> +		if (!is_absolute_path(given_config_file) && file)
>> +			file = prefix_filename(file, strlen(file),
>> +					       given_config_file);
>> +		else
>> +			file = given_config_file;
>> +		config_exclusive_filename = file;
>
> It took me a considerable amount of time to figure out that "file" is 
> actually the "prefix"!  That cleanup would be nice to have before the 
> parseopt patch, methinks, especially since the code is reindented, and 
> thus hard to follow in the diff.

I noticed that "file" thing during my review of the first round.  It
probably was a cute attempt to avoid using two variables "prefix" and
"file", but made the result a lot harder to read.  I agree that a clean-up
*before* code restructuring would be good for this particular wart.

    A note that is off-topic to this patch.

    I also noticed that the diff was impossible to read because it matched
    the lines with only an indented close brace or whitespace between the
    preimage and the postimage too aggressively.  Your --patience did seem
    to help a little bit, at least it produced a different result, but not
    much (not that patience was meant to make this kind of change easier
    to read).  It may have helped if we had that "do not match trivial
    lines too aggressively just to reduce the patch size" option.

Re: [PATCH v2] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>
>> @@ -231,7 +264,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);
>>       }
>
> Name changes like this make it harder to read the patch; can you separate
> that change out into its own patch?

In that case I couldn't use OPT_STRING to store that value; I would
have to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and
store check the argc (since OPT_STRING isn't doing that anymore) and
save argv[1] to get_color_slot,

>> +     if (use_global_config) {
>> +             char *home = getenv("HOME");
>> +             if (home) {
>> +                     char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
>> +                     config_exclusive_filename = user_config;
>
> In a subsequent patch, you might add a check only one of --global,
> --system or --file was given.

Yeap.

>> +     else if (given_config_file) {
>> +             if (!is_absolute_path(given_config_file) && file)
>> +                     file = prefix_filename(file, strlen(file),
>> +                                            given_config_file);
>> +             else
>> +                     file = given_config_file;
>> +             config_exclusive_filename = file;
>
> It took me a considerable amount of time to figure out that "file" is
> actually the "prefix"!  That cleanup would be nice to have before the
> parseopt patch, methinks, especially since the code is reindented, and
> thus hard to follow in the diff.

True.

I tried to minimize changes, so that code is exactly the same as
before (except vi didn't preserve the indent).

>> +     if (actions & ACTION_LIST) {
>> +             if (git_config(show_all_config, NULL) < 0 &&
>> +                 file && errno)
>
> Should this not be config_exclusive_filename?

Looks like that.

>> +                     die("unable to read config file %s: %s", file,
>> +                         strerror(errno));
>
> Do we really only want to die() in case we know the file name?  AFAICT at
> this point we have no idea in which of the possibly three files the error
> occurred.  And there need not be any errno set, for example when there was
> a parse error.

Yes, there should be an error even if 'file' is not set. But if the
file is set what's wrong with that die command?

>> +     else if (actions & ACTION_EDIT) {
>> +             const char *config_filename;
>> +             if (config_exclusive_filename)
>> +                     config_filename = config_exclusive_filename;
>> +             else
>> +                     config_filename = git_path("config");
>
> Why not reuse config_exclusive_filename here?

You mean:
if (!config_exclusive_filename)
  config_exclusive_filename = git_path("config");

>> +     else if (actions & ACTION_ADD) {
>> +             check_argc(argc, 2, 2);
>
> BTW what about check_argc() in the previous two cases?

You mean fail if -e or -l have extra arguments?

>> +             return git_config_set_multivar(argv[0], value, "^$", 0);
>
> Now that I see this, there is another idea for a possible cleanup after
> the parseoptification: cmd_config() should not return -1, as that will be
> turned into the exit status.  So maybe prefix the return value with "!!"?
>
> Or maybe even better: set a variable "ret" and at the end of cmd_config(),
> "return !!ret;"?

Huh? So git commands don't return negative error values?

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Junio C Hamano]

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

>> I personally do not think "I rewrote this command's option parser using
>> parseopt" earns any "trust point".  I think the latter is a *great* thing
>> to do, though.
>
> I disagree. Making a patch pass through all the filters must mean
> something, and the more patches the more trust.

Why are you arguing?

I am saying I do not feel more trust in people _merely because_ they
rewrote command parser to use parseopt.  Telling me that I am wrong and I
should trust you more for such a patch would not help.

>> Mistakes made in the past and resulting flaws that remain in the current
>> source do not justify a new patch adding more mistakes to the codebase.
>> Reviewers help the author from adding more.
>>
>> One bad thing about the current process (and I am certainly one of the
>> guilty parties because I do a lot of reviews) around this area is that a
>> review comment that points out a mistake similar to the ones made in the
>> past sound to the author of the patch as if the reviewer is telling that
>> the patch will not be accepted unless the existing mistakes are also fixed
>> by the patch author.  Such a review certainly does not mean that ...
>> ...
> But, if there's code that already has the same issues the patch has,
> it doesn't look like a good argument for patch rejection. Perhaps the
> quality standards increased, but on the other hand things wouldn't get
> worst by applying the patch.

If you read the above quoted block again, you will notice that we are
almost in full agreement, *if* you change "by applying the patch" in your
last sentence to "by applying a patch that is revised to fix the problem
pointed out during the review in it, even if it does not address the
similar ones in the existing code".

Adding more breakages of the same kind may not increase the number of
classes of breakages, but surely it increases the number of places that
need to be fixed later, and it is actively wrong to discard time and
energy somebody already spent to prevent one more instance of known
breakage to get into the codebase.

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 10:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> I personally do not think "I rewrote this command's option parser using
>>> parseopt" earns any "trust point".  I think the latter is a *great* thing
>>> to do, though.
>>
>> I disagree. Making a patch pass through all the filters must mean
>> something, and the more patches the more trust.
>
> Why are you arguing?
>
> I am saying I do not feel more trust in people _merely because_ they
> rewrote command parser to use parseopt.  Telling me that I am wrong and I
> should trust you more for such a patch would not help.

I'm not arguing, that's just my opinion.

I'm not saying writing a patch should give anyone penguin points, it's
going through the review process up to patch acceptance. Maybe not to
you, but maybe for other developers.

>>> Mistakes made in the past and resulting flaws that remain in the current
>>> source do not justify a new patch adding more mistakes to the codebase.
>>> Reviewers help the author from adding more.
>>>
>>> One bad thing about the current process (and I am certainly one of the
>>> guilty parties because I do a lot of reviews) around this area is that a
>>> review comment that points out a mistake similar to the ones made in the
>>> past sound to the author of the patch as if the reviewer is telling that
>>> the patch will not be accepted unless the existing mistakes are also fixed
>>> by the patch author.  Such a review certainly does not mean that ...
>>> ...
>> But, if there's code that already has the same issues the patch has,
>> it doesn't look like a good argument for patch rejection. Perhaps the
>> quality standards increased, but on the other hand things wouldn't get
>> worst by applying the patch.
>
> If you read the above quoted block again, you will notice that we are
> almost in full agreement, *if* you change "by applying the patch" in your
> last sentence to "by applying a patch that is revised to fix the problem
> pointed out during the review in it, even if it does not address the
> similar ones in the existing code".
>
> Adding more breakages of the same kind may not increase the number of
> classes of breakages, but surely it increases the number of places that
> need to be fixed later, and it is actively wrong to discard time and
> energy somebody already spent to prevent one more instance of known
> breakage to get into the codebase.

I think are in full agreement, it's just that I wasn't clear enough;
those are not the kind of issues I was talking about. If there's a
known way how to do something then it's obvious the patch should be
re-submitted with the 'right way'. I was thinking more on "FILE" vs
"file", or any kind of issue that would require a separate patch to
reach consistency in the existing code; those can wait until after the
original patch is accepted.

-- 
Felipe Contreras

human readable diffs, was Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Junio C Hamano wrote:

>     I also noticed that the diff was impossible to read because it 
>     matched the lines with only an indented close brace or whitespace 
>     between the preimage and the postimage too aggressively.  Your 
>     --patience did seem to help a little bit, at least it produced a 
>     different result, but not much (not that patience was meant to make 
>     this kind of change easier to read).  It may have helped if we had 
>     that "do not match trivial lines too aggressively just to reduce the 
>     patch size" option.

Yeah, I think that your objections to my '--collapse-non-alnums' patch 
show to be relevant especially with this patch: an '} else {' matches, but 
actually disturbs readability.

So an option would be nice, indeed, which can merge a single matching line 
(or a given maximal number of matching lines) between runs of a certain 
minimal number of _non-matching_ lines.

But how to call it?

And how determine the ideal ratio between matching/non-matching lines 
until which to consider the matching lines to be non-matches?  Maybe a 
simple fraction would be enough...

Or maybe an absolute number of matching lines?  But it is obvious how to 
construct a case where this breaks down.

Ciao,
Dscho

Re: [PATCH] config: Use parseopt.

[Junio C Hamano]

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

>> Unfortunately, not many patch authors write such a summary.  Sometimes we
>> see summaries on things that were discussed but nobody has followed
>> through posted by third parties (including myself), but we do not seem to
>> have enough helpers to do that either.  This does not take much technical
>> skills but is a good "trust point" earner.
>
> For me it's easier, and more fun to write a separate patch that fixes
> the issues than writing a summary,...

That certainly is something we should take into consideration.

I however think an unwritten assumption around here so far has been that
the patch author who gets review comments is expected to keep track of the
issues raised, both about the patch itself and about the similar breakages
in the existing code pointed out during the review process, if only
because the patch author is the focal point of the discussion.

We probably need to break that.

Because it is very likely that the reviewer does not even realize that
such similar breakages in the existing code when a review is made, we
cannot ask reviewers to always start a separate discussion.  Some reviews
do say "Admittedly, we already have the same pattern in here and there,
but this in your patch is wrong," but the way how we collectively realize
an existing breakage is often by hearing the patch author respond with
"but there already are this and that breakages in the existing code."

We do not want such knowledge of existing breakages go to waste in either
case.  Perhaps it would be a good start to make it the responsibility of
the first person who mentions an existing breakage (either the reviewer's
"Admittedly", or the patch author's "but there already are") to begin a
separate thread, so that mail archive would remember it.  It shouldn't
take more than 3 minutes.

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 9:26 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Johannes Schindelin wrote:
>
>> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>>
>> > On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin
>> > <Johannes.Schindelin@gmx.de> wrote:
>> > > Hi,
>> > >
>> > > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> > >
>> > >> 1) --list when no config file is given uses all the config files,
>> > >> wouldn't it make sense to have a --repo option?
>> > >
>> > > The idea of --list is not "cat .git/config".  The idea is to help users or
>> > > scripts to list the current settings (_including_ the global settings).
>> > >
>> > > You can force showing the repo-specific config with "git --file
>> > > .git/config", though.
>> >
>> > When you are on the root directory of the repo, and you don't have
>> > GIT_DIR, or --git-dir.
>>
>> When I wrote my response, I briefly considered if I had to be verbose, and
>> decided against it.
>>
>> But this is what I should have written:
>>
>>       git --file $(git rev-parse --git-dir)/config --list
>
> Okay, before anybody points out that I was not verbose enough -- again --
> this is what I really should have written:
>
>        git --file "$(git rev-parse --git-dir)"/config --list

Yeap, it's possible, but I think people would not complain about "git
--repo --list". I think it would be much clearer to have --global
--system --repo and --all. Then we can say --list by default uses
--all, and --edit uses --repo, and it doesn't make sense to do --edit
--all.

Anyway, I don't care that much.

-- 
Felipe Contreras

Re: [PATCH] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Felipe Contreras wrote:

> On Sat, Feb 14, 2009 at 9:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> This is more a "I would like to increase the chances of my patches 
> >> being accepted so I'd do some chores to gain the trust of some 
> >> developers", and Johannes Schindelin was pushing me to do this.
> >>
> >> Also it's a bit of "I would like to improve git and learn the API 
> >> while doing so".
> >
> > I personally do not think "I rewrote this command's option parser 
> > using parseopt" earns any "trust point".  I think the latter is a 
> > *great* thing to do, though.
> 
> I disagree. Making a patch pass through all the filters must mean 
> something, and the more patches the more trust.

Maybe I should point out something that is obvious to somebody who 
followed the Git list for a long time: there are two areas of the code 
that had such a track record of regressions that Junio grew a distaste for 
them: git-config and parse options.

However, I imagine if you manage to provide a patch that touches both 
areas _and_ that are without flaws, then you get some brownie points :-)

Ciao,
Dscho

Re: [PATCH] config: Use parseopt.

[Felipe Contreras]

On Sat, Feb 14, 2009 at 11:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> Unfortunately, not many patch authors write such a summary.  Sometimes we
>>> see summaries on things that were discussed but nobody has followed
>>> through posted by third parties (including myself), but we do not seem to
>>> have enough helpers to do that either.  This does not take much technical
>>> skills but is a good "trust point" earner.
>>
>> For me it's easier, and more fun to write a separate patch that fixes
>> the issues than writing a summary,...
>
> That certainly is something we should take into consideration.
>
> I however think an unwritten assumption around here so far has been that
> the patch author who gets review comments is expected to keep track of the
> issues raised, both about the patch itself and about the similar breakages
> in the existing code pointed out during the review process, if only
> because the patch author is the focal point of the discussion.
>
> We probably need to break that.
>
> Because it is very likely that the reviewer does not even realize that
> such similar breakages in the existing code when a review is made, we
> cannot ask reviewers to always start a separate discussion.  Some reviews
> do say "Admittedly, we already have the same pattern in here and there,
> but this in your patch is wrong," but the way how we collectively realize
> an existing breakage is often by hearing the patch author respond with
> "but there already are this and that breakages in the existing code."
>
> We do not want such knowledge of existing breakages go to waste in either
> case.  Perhaps it would be a good start to make it the responsibility of
> the first person who mentions an existing breakage (either the reviewer's
> "Admittedly", or the patch author's "but there already are") to begin a
> separate thread, so that mail archive would remember it.  It shouldn't
> take more than 3 minutes.

That is true, however I propose something a bit different. At least in
this review there has been a number of issues brought up, it would be
overkill to create a separate thread for each one of these issues as
they where found, and if the patch submitter is new, he probably
wouldn't know about this rule.

So, I propose that at the end of the patch review process the ack
person (or somebody else) asks the patch submitter (possibly cc'ing
the reviewers) to start a new thread mentioning the pending issues
brought up in the review.

-- 
Felipe Contreras

Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sat, 14 Feb 2009, Felipe Contreras wrote:

> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
> >
> >> @@ -231,7 +264,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);
> >>       }
> >
> > Name changes like this make it harder to read the patch; can you separate
> > that change out into its own patch?
> 
> In that case I couldn't use OPT_STRING to store that value; I would
> have to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and
> store check the argc (since OPT_STRING isn't doing that anymore) and
> save argv[1] to get_color_slot,

What I meant was: have a patch renaming get_color_slot to 
get_colorbool_slot _before_ the (already large) parseoptification.

> >> +                     die("unable to read config file %s: %s", file,
> >> +                         strerror(errno));
> >
> > Do we really only want to die() in case we know the file name?  AFAICT at
> > this point we have no idea in which of the possibly three files the error
> > occurred.  And there need not be any errno set, for example when there was
> > a parse error.
> 
> Yes, there should be an error even if 'file' is not set. But if the
> file is set what's wrong with that die command?

I think we should die() in all cases, not just one, and we might want to 
skip the "file" parameter (and the "errno") parameter, as the file 
containing the error could be different from "file".

> >> +     else if (actions & ACTION_EDIT) {
> >> +             const char *config_filename;
> >> +             if (config_exclusive_filename)
> >> +                     config_filename = config_exclusive_filename;
> >> +             else
> >> +                     config_filename = git_path("config");
> >
> > Why not reuse config_exclusive_filename here?
> 
> You mean:
> if (!config_exclusive_filename)
>   config_exclusive_filename = git_path("config");

Yes.

> >> +     else if (actions & ACTION_ADD) {
> >> +             check_argc(argc, 2, 2);
> >
> > BTW what about check_argc() in the previous two cases?
> 
> You mean fail if -e or -l have extra arguments?

Yes.

> >> +             return git_config_set_multivar(argv[0], value, "^$", 0);
> >
> > Now that I see this, there is another idea for a possible cleanup 
> > after the parseoptification: cmd_config() should not return -1, as 
> > that will be turned into the exit status.  So maybe prefix the return 
> > value with "!!"?
> >
> > Or maybe even better: set a variable "ret" and at the end of 
> > cmd_config(), "return !!ret;"?
> 
> Huh? So git commands don't return negative error values?

AFAICT an exit status is supposed to be between 0 and 127.

Ciao,
Dscho

Re: [PATCH v2] config: Use parseopt.

[Jakub Narebski]

Johannes Schindelin wrote:

>> You mean fail if -e or -l have extra arguments?
> 
> Yes.

'-l' can have extra arguments: '-z'

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

please do not cull the Cc: list.

On Sat, 14 Feb 2009, Jakub Narebski wrote:

> Johannes Schindelin wrote:
> 
> >> You mean fail if -e or -l have extra arguments?
> > 
> > Yes.
> 
> '-l' can have extra arguments: '-z'

We meant "non-option argument".

Ciao,
Dscho

Re: [PATCH] config: Use parseopt.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Maybe I should point out something that is obvious to somebody who 
> followed the Git list for a long time: there are two areas of the code 
> that had such a track record of regressions that Junio grew a distaste for 
> them: git-config and parse options.

There is a difference between them; please don't confuse new readers.

git-config does have a rather unfortunate track record and the code may
still be a mess.  But parse-options itself is a good code overall.  Only
that some of the parse-opt-ification attempts in the past might have been
quite bad.

Parse-opt-ification is an obvious, trivial change with a limited scope,
well-defined end results and clear gain to the end users.  The API exists
so the patch author does not have to invent a new framework, the change a
patch needs will typically be limited to a single command, the set of
options the command needs to accept/reject are already defined, and at the
end you can give unique prefix of flags from the command line.

There may be a correlation between parse-opt-ification attempts and bad
review cycles, but there is no such a causal relationship "parse-opt
patches are bad because parse-opt is a bad idea".  

If there is such a correlation, it is more likely that it is merely
because parse-opt-ification attracted more inexperienced people than
tricker areas like revision traversal or extended SHA-1 syntax.

But people can send bad patches to any area ;-).

Re: [PATCH v2] config: Use parseopt.

[Felipe Contreras]

On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>
>> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >
>> >> @@ -231,7 +264,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);
>> >>       }
>> >
>> > Name changes like this make it harder to read the patch; can you separate
>> > that change out into its own patch?
>>
>> In that case I couldn't use OPT_STRING to store that value; I would
>> have to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and
>> store check the argc (since OPT_STRING isn't doing that anymore) and
>> save argv[1] to get_color_slot,
>
> What I meant was: have a patch renaming get_color_slot to
> get_colorbool_slot _before_ the (already large) parseoptification.

Done.

>> >> +                     die("unable to read config file %s: %s", file,
>> >> +                         strerror(errno));
>> >
>> > Do we really only want to die() in case we know the file name?  AFAICT at
>> > this point we have no idea in which of the possibly three files the error
>> > occurred.  And there need not be any errno set, for example when there was
>> > a parse error.
>>
>> Yes, there should be an error even if 'file' is not set. But if the
>> file is set what's wrong with that die command?
>
> I think we should die() in all cases, not just one, and we might want to
> skip the "file" parameter (and the "errno") parameter, as the file
> containing the error could be different from "file".

Done.

>> >> +     else if (actions & ACTION_EDIT) {
>> >> +             const char *config_filename;
>> >> +             if (config_exclusive_filename)
>> >> +                     config_filename = config_exclusive_filename;
>> >> +             else
>> >> +                     config_filename = git_path("config");
>> >
>> > Why not reuse config_exclusive_filename here?
>>
>> You mean:
>> if (!config_exclusive_filename)
>>   config_exclusive_filename = git_path("config");
>
> Yes.

I'm not sure about this one. At least git_config should be moved
before that code, otherwise it will only try to read core.editor from
the exclusive_filename and that's not what we want.

If somebody adds some extra code that uses config_exclusive_filename
there would be issues. I would prefer to leave it like that since
that's how the code in config.c is handling config_exclusive_filename.

>> >> +     else if (actions & ACTION_ADD) {
>> >> +             check_argc(argc, 2, 2);
>> >
>> > BTW what about check_argc() in the previous two cases?
>>
>> You mean fail if -e or -l have extra arguments?
>
> Yes.

Done.

>> >> +             return git_config_set_multivar(argv[0], value, "^$", 0);
>> >
>> > Now that I see this, there is another idea for a possible cleanup
>> > after the parseoptification: cmd_config() should not return -1, as
>> > that will be turned into the exit status.  So maybe prefix the return
>> > value with "!!"?
>> >
>> > Or maybe even better: set a variable "ret" and at the end of
>> > cmd_config(), "return !!ret;"?
>>
>> Huh? So git commands don't return negative error values?
>
> AFAICT an exit status is supposed to be between 0 and 127.

Ok. Done.

-- 
Felipe Contreras

Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Felipe Contreras wrote:

> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
> >
> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >>
> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
> >> >
> >> >> +     else if (actions & ACTION_EDIT) {
> >> >> +             const char *config_filename;
> >> >> +             if (config_exclusive_filename)
> >> >> +                     config_filename = config_exclusive_filename;
> >> >> +             else
> >> >> +                     config_filename = git_path("config");
> >> >
> >> > Why not reuse config_exclusive_filename here?
> >>
> >> You mean:
> >> if (!config_exclusive_filename)
> >>   config_exclusive_filename = git_path("config");
> >
> > Yes.
> 
> I'm not sure about this one. At least git_config should be moved before 
> that code, otherwise it will only try to read core.editor from the 
> exclusive_filename and that's not what we want.

Ah!  I did not think about core.editor, of course, but that is what got 
you started with git-config, after all.

However, the next line sets config_exclusive_filename, does it not?

Ciao,
Dscho

Re: [PATCH v2] config: Use parseopt.

[Felipe Contreras]

On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Felipe Contreras wrote:
>
>> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Hi,
>> >
>> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >
>> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
>> >> <Johannes.Schindelin@gmx.de> wrote:
>> >>
>> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >> >
>> >> >> +     else if (actions & ACTION_EDIT) {
>> >> >> +             const char *config_filename;
>> >> >> +             if (config_exclusive_filename)
>> >> >> +                     config_filename = config_exclusive_filename;
>> >> >> +             else
>> >> >> +                     config_filename = git_path("config");
>> >> >
>> >> > Why not reuse config_exclusive_filename here?
>> >>
>> >> You mean:
>> >> if (!config_exclusive_filename)
>> >>   config_exclusive_filename = git_path("config");
>> >
>> > Yes.
>>
>> I'm not sure about this one. At least git_config should be moved before
>> that code, otherwise it will only try to read core.editor from the
>> exclusive_filename and that's not what we want.
>
> Ah!  I did not think about core.editor, of course, but that is what got
> you started with git-config, after all.
>
> However, the next line sets config_exclusive_filename, does it not?

Huh? Which line?

-- 
Felipe Contreras

Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Felipe Contreras wrote:

> On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Sun, 15 Feb 2009, Felipe Contreras wrote:
> >
> >> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >> > Hi,
> >> >
> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
> >> >
> >> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
> >> >> <Johannes.Schindelin@gmx.de> wrote:
> >> >>
> >> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
> >> >> >
> >> >> >> +     else if (actions & ACTION_EDIT) {
> >> >> >> +             const char *config_filename;
> >> >> >> +             if (config_exclusive_filename)
> >> >> >> +                     config_filename = config_exclusive_filename;
> >> >> >> +             else
> >> >> >> +                     config_filename = git_path("config");
> >> >> >
> >> >> > Why not reuse config_exclusive_filename here?
> >> >>
> >> >> You mean:
> >> >> if (!config_exclusive_filename)
> >> >>   config_exclusive_filename = git_path("config");
> >> >
> >> > Yes.
> >>
> >> I'm not sure about this one. At least git_config should be moved before
> >> that code, otherwise it will only try to read core.editor from the
> >> exclusive_filename and that's not what we want.
> >
> > Ah!  I did not think about core.editor, of course, but that is what got
> > you started with git-config, after all.
> >
> > However, the next line sets config_exclusive_filename, does it not?
> 
> Huh? Which line?

Ah, I misremembered.  Your current patch shows this:

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

... which makes me believe that the point about moving git_config() before 
setting config_exclusive_filename is rather weak :-)

+       else if (actions & ACTION_EDIT) {
+               git_config(git_default_config, NULL);
+               launch_editor(config_exclusive_filename ?
+			config_exclusive_filename : git_path("config"), 
+			NULL, NULL);
+       }

(This has whitespace issues, as I copy-pasted it using kterm's clipboard 
functions.)

Ciao,
Dscho

Re: [PATCH v2] config: Use parseopt.

[Felipe Contreras]

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

On Sun, Feb 15, 2009 at 2:33 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Felipe Contreras wrote:
>
>> On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Hi,
>> >
>> > On Sun, 15 Feb 2009, Felipe Contreras wrote:
>> >
>> >> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
>> >> <Johannes.Schindelin@gmx.de> wrote:
>> >> > Hi,
>> >> >
>> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >> >
>> >> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
>> >> >> <Johannes.Schindelin@gmx.de> wrote:
>> >> >>
>> >> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >> >> >
>> >> >> >> +     else if (actions & ACTION_EDIT) {
>> >> >> >> +             const char *config_filename;
>> >> >> >> +             if (config_exclusive_filename)
>> >> >> >> +                     config_filename = config_exclusive_filename;
>> >> >> >> +             else
>> >> >> >> +                     config_filename = git_path("config");
>> >> >> >
>> >> >> > Why not reuse config_exclusive_filename here?
>> >> >>
>> >> >> You mean:
>> >> >> if (!config_exclusive_filename)
>> >> >>   config_exclusive_filename = git_path("config");
>> >> >
>> >> > Yes.
>> >>
>> >> I'm not sure about this one. At least git_config should be moved before
>> >> that code, otherwise it will only try to read core.editor from the
>> >> exclusive_filename and that's not what we want.
>> >
>> > Ah!  I did not think about core.editor, of course, but that is what got
>> > you started with git-config, after all.
>> >
>> > However, the next line sets config_exclusive_filename, does it not?
>>
>> Huh? Which line?
>
> Ah, I misremembered.  Your current patch shows this:
>
> +       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);
> +       }
>
> ... which makes me believe that the point about moving git_config() before
> setting config_exclusive_filename is rather weak :-)
>
> +       else if (actions & ACTION_EDIT) {
> +               git_config(git_default_config, NULL);
> +               launch_editor(config_exclusive_filename ?
> +                       config_exclusive_filename : git_path("config"),
> +                       NULL, NULL);
> +       }
>
> (This has whitespace issues, as I copy-pasted it using kterm's clipboard
> functions.)

I thought on that before but now it makes more sense to me. Attaching the patch.

-- 
Felipe Contreras

[-- Attachment #2: 0001-config-Cleanup-editor-action.patch --]
[-- Type: application/octet-stream, Size: 1142 bytes --]

From 4b3db3d13261849cd7d561dc20bddb375f1430c8 Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sun, 15 Feb 2009 14:46:58 +0200
Subject: [PATCH] config: Cleanup editor action.

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 2c1ad71..bd7bac4 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -370,13 +370,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] config: Use parseopt.

[Felipe Contreras]

On Sun, Feb 15, 2009 at 02:51:45PM +0200, Felipe Contreras wrote:
> On Sun, Feb 15, 2009 at 2:33 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Ah, I misremembered.  Your current patch shows this:
> >
> > +       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);
> > +       }
> >
> > ... which makes me believe that the point about moving git_config() before
> > setting config_exclusive_filename is rather weak :-)
> >
> > +       else if (actions & ACTION_EDIT) {
> > +               git_config(git_default_config, NULL);
> > +               launch_editor(config_exclusive_filename ?
> > +                       config_exclusive_filename : git_path("config"),
> > +                       NULL, NULL);
> > +       }
> >
> > (This has whitespace issues, as I copy-pasted it using kterm's clipboard
> > functions.)
> 
> I thought on that before but now it makes more sense to me. Attaching the patch.

Now inlined (sorry):

---
 builtin-config.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 2c1ad71..bd7bac4 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -370,13 +370,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

-- 
Felipe Contreras

Re: [PATCH v2] config: Use parseopt.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > Or maybe even better: set a variable "ret" and at the end of 
>> > cmd_config(), "return !!ret;"?
>> 
>> Huh? So git commands don't return negative error values?
>
> AFAICT an exit status is supposed to be between 0 and 127.

Are you two talking about the return value from cmd_config()?

git.c::run_builtin() already knows what to do with status codes from the
builtins to protect you from (rare) shells that do not cope with a
negative return that come from the common pattern of doing:

	return error("it is wrong in this way")

So "negative" is not really a problem.

Indeed, if the old code was doing:

	ret = git_config_set_multivar(...);
        if (ret)
		return ret;

and if you are changing it to:

	ret = git_config_set_multivar(...);
        if (ret)
		return !!ret;

you are changing an externally observable behaviour that _could_ break the
calling end user scripts.

.

Re: [PATCH v2] config: Use parseopt.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Or maybe even better: set a variable "ret" and at the end of 
>> > cmd_config(), "return !!ret;"?
>> 
>> Huh? So git commands don't return negative error values?
>
> AFAICT an exit status is supposed to be between 0 and 127.

Are you two talking about the return value from cmd_config()?

git.c::run_builtin() already knows what to do with status codes from the
builtins to protect you from (rare) shells that do not cope with a
negative return that come from the common pattern of doing:

	return error("it is wrong in this way")

So "negative" is not really a problem.  See 2488df8 (builtin run_command:
do not exit with -1., 2007-11-13).

But if the old code was doing:

	ret = git_config_set_multivar(...);
        if (ret)
		return ret;

and if you are changing it to:

	ret = git_config_set_multivar(...);
        if (ret)
		return !!ret;

you are changing an externally observable behaviour.

Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > Or maybe even better: set a variable "ret" and at the end of 
> >> > cmd_config(), "return !!ret;"?
> >> 
> >> Huh? So git commands don't return negative error values?
> >
> > AFAICT an exit status is supposed to be between 0 and 127.
> 
> Are you two talking about the return value from cmd_config()?
> 
> git.c::run_builtin() already knows what to do with status codes from the
> builtins to protect you from (rare) shells that do not cope with a
> negative return that come from the common pattern of doing:
> 
> 	return error("it is wrong in this way")
> 
> So "negative" is not really a problem.

Ooops.  I missed that.

Ciao,
Dscho

Re: [PATCH v2] config: Use parseopt.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 15 Feb 2009, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> >> > Or maybe even better: set a variable "ret" and at the end of 
>> >> > cmd_config(), "return !!ret;"?
>> >> 
>> >> Huh? So git commands don't return negative error values?
>> >
>> > AFAICT an exit status is supposed to be between 0 and 127.
>> 
>> Are you two talking about the return value from cmd_config()?
>> 
>> git.c::run_builtin() already knows what to do with status codes from the
>> builtins to protect you from (rare) shells that do not cope with a
>> negative return that come from the common pattern of doing:
>> 
>> 	return error("it is wrong in this way")
>> 
>> So "negative" is not really a problem.
>
> Ooops.  I missed that.

Note that I refrained from using stronger words like "regression" on
purpose, because I do not think any caller tells various error codes that
come out of git_config_set_multivar() apart and act differently in
practice.

But it does appear that the said function wants to say why the call failed
with its return code, and using !!ret to lose information does not feel
right.

Re: [PATCH v2] config: Use parseopt.

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 15 Feb 2009, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> >> > Or maybe even better: set a variable "ret" and at the end of 
> >> >> > cmd_config(), "return !!ret;"?
> >> >> 
> >> >> Huh? So git commands don't return negative error values?
> >> >
> >> > AFAICT an exit status is supposed to be between 0 and 127.
> >> 
> >> Are you two talking about the return value from cmd_config()?
> >> 
> >> git.c::run_builtin() already knows what to do with status codes from the
> >> builtins to protect you from (rare) shells that do not cope with a
> >> negative return that come from the common pattern of doing:
> >> 
> >> 	return error("it is wrong in this way")
> >> 
> >> So "negative" is not really a problem.
> >
> > Ooops.  I missed that.
> 
> Note that I refrained from using stronger words like "regression" on
> purpose, because I do not think any caller tells various error codes that
> come out of git_config_set_multivar() apart and act differently in
> practice.
> 
> But it does appear that the said function wants to say why the call failed
> with its return code, and using !!ret to lose information does not feel
> right.

I fully agree.

Ciao,
Dscho

Re: [PATCH v2] config: Use parseopt.

[Felipe Contreras]

On Sun, Feb 15, 2009 at 11:29 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > On Sun, 15 Feb 2009, Junio C Hamano wrote:
>> >
>> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> >>
>> >> >> > Or maybe even better: set a variable "ret" and at the end of
>> >> >> > cmd_config(), "return !!ret;"?
>> >> >>
>> >> >> Huh? So git commands don't return negative error values?
>> >> >
>> >> > AFAICT an exit status is supposed to be between 0 and 127.
>> >>
>> >> Are you two talking about the return value from cmd_config()?
>> >>
>> >> git.c::run_builtin() already knows what to do with status codes from the
>> >> builtins to protect you from (rare) shells that do not cope with a
>> >> negative return that come from the common pattern of doing:
>> >>
>> >>    return error("it is wrong in this way")
>> >>
>> >> So "negative" is not really a problem.
>> >
>> > Ooops.  I missed that.
>>
>> Note that I refrained from using stronger words like "regression" on
>> purpose, because I do not think any caller tells various error codes that
>> come out of git_config_set_multivar() apart and act differently in
>> practice.
>>
>> But it does appear that the said function wants to say why the call failed
>> with its return code, and using !!ret to lose information does not feel
>> right.
>
> I fully agree.

Ok, dropping the patch.

-- 
Felipe Contreras