patchconfig: Use parseopt.

> 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, "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_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;

> @@ -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.");

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

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

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

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

> 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, "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_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.

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

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

> 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(-)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> 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

> 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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