Re: [PATCH] config: Use parseopt.

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2009, 09:28 UTC
Message-ID: <7vab8pweod.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1234577142-22965-1-git-send-email-felipe.contreras@gmail.com>

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

Show 7 quoted lines

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

Show 28 quoted lines

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

Show 8 quoted lines

> +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?

Show 5 quoted lines

> +	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.

Show 5 quoted lines

> +	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).

Show 7 quoted lines

> +	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".

Show 12 quoted lines

> +	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?

Show 9 quoted lines

> @@ -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.

Show 7 quoted lines

> +	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?

Show 20 quoted lines

> +	}
> +	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 ...

Show 22 quoted lines

> +	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.

Show 26 quoted lines

>  	}
> +	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.