On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
>
>> Reorganizing the code to use parseopt as suggested by Johannes
>> Schindelin.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
