{"thread":{"id":"17782","subject":"[PATCH] config: Use parseopt.","startedAt":"2009-02-14T02:05:42Z","lastAt":"2009-02-17T00:50:05Z","messageCount":45,"participants":["Felipe Contreras","Junio C Hamano","Johannes Schindelin","Jakub Narebski","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"104568","messageId":"1234577142-22965-1-git-send-email-felipe.contreras@gmail.com","threadId":"17782","inReplyTo":null,"subject":"[PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T02:05:42Z","receivedAt":"2009-02-14T02:05:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Reorganizing the code to use parseopt as suggested by Johannes\nSchindelin.\n\nSigned-off-by: Felipe Contreras \n---\n builtin-config.c | 422 +++++++++++++++++++++++++++---------------------------\n 1 files changed, 210 insertions(+), 212 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex afc4393..f774902 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -1,9 +1,12 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n #include \"color.h\"\n+#include \"parse-options.h\"\n \n-static const char git_config_set_usage[] =\n-\"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 ]\";\n+static const char *const builtin_config_usage[] = {\n+\t\"git config [options]\",\n+\tNULL\n+};\n \n static char *key;\n static regex_t *key_regexp;\n@@ -18,6 +21,42 @@ static char key_delim = ' ';\n static char term = '\\n';\n static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;\n \n+static int use_global_config, use_system_config;\n+static const char *given_config_file;\n+static int do_list, do_edit, do_add, do_get, do_get_all, do_get_regexp, do_replace_all;\n+static int do_unset, do_unset_all, do_rename_section, do_remove_section;\n+static int type_int, type_bool, type_bool_or_int;\n+static const char *get_color_name, *get_colorbool_name;\n+static int end_null;\n+\n+static struct option builtin_config_options[] = {\n+\tOPT_GROUP(\"Config file location\"),\n+\tOPT_BOOLEAN(0, \"global\", &use_global_config, \"use global config file\"),\n+\tOPT_BOOLEAN(0, \"system\", &use_system_config, \"use system config file\"),\n+\tOPT_STRING('f', \"file\", &given_config_file, \"FILE\", \"use given config file\"),\n+\tOPT_GROUP(\"Action\"),\n+\tOPT_BOOLEAN(0, \"get\", &do_get, \"get value: name [value-regex]\"),\n+\tOPT_BOOLEAN(0, \"get-all\", &do_get_all, \"get all values: key [value-regex]\"),\n+\tOPT_BOOLEAN(0, \"get-regexp\", &do_get_regexp, \"get values for regexp: name-regex [value-regex]\"),\n+\tOPT_BOOLEAN(0, \"replace-all\", &do_replace_all, \"replace all options: name [value [value_regex]\"),\n+\tOPT_BOOLEAN(0, \"add\", &do_add, \"adds a new option: name value\"),\n+\tOPT_BOOLEAN(0, \"unset\", &do_unset, \"removes an option: name [value-regex]\"),\n+\tOPT_BOOLEAN(0, \"unset-all\", &do_unset_all, \"removes all matches: name [value-regex]\"),\n+\tOPT_BOOLEAN(0, \"rename-section\", &do_rename_section, \"rename section: old-name new-name\"),\n+\tOPT_BOOLEAN(0, \"remove-section\", &do_remove_section, \"remove a section: name\"),\n+\tOPT_BOOLEAN('l', \"list\", &do_list, \"list all\"),\n+\tOPT_STRING(0, \"get-color\", &get_color_name, \"name\", \"find the color configured: [default]\"),\n+\tOPT_STRING(0, \"get-colorbool\", &get_colorbool_name, \"name\", \"find the color setting: [stdout-is-tty]\"),\n+\tOPT_BOOLEAN('e', \"edit\", &do_edit, \"opens an editor\"),\n+\tOPT_GROUP(\"Type\"),\n+\tOPT_BOOLEAN(0, \"bool\", &type_bool, \"value is \\\"true\\\" or \\\"false\\\"\"),\n+\tOPT_BOOLEAN(0, \"int\", &type_int, \"value is decimal number\"),\n+\tOPT_BOOLEAN(0, \"bool-or-int\", &type_bool_or_int, NULL),\n+\tOPT_GROUP(\"Other\"),\n+\tOPT_BOOLEAN('z', \"null\", &end_null, \"end values with null character\"),\n+\tOPT_END(),\n+};\n+\n static int show_all_config(const char *key_, const char *value_, void *cb)\n {\n \tif (value_)\n@@ -177,12 +216,11 @@ static char *normalize_value(const char *key, const char *value)\n }\n \n static int get_color_found;\n-static const char *get_color_slot;\n+static const char *get_color_name;\n static char parsed_color[COLOR_MAXLEN];\n-\n static int git_get_color_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, get_color_slot)) {\n+\tif (!strcmp(var, get_color_name)) {\n \t\tif (!value)\n \t\t\tconfig_error_nonbool(var);\n \t\tcolor_parse(value, var, parsed_color);\n@@ -191,47 +229,13 @@ static int git_get_color_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n-static int get_color(int argc, const char **argv)\n-{\n-\t/*\n-\t * grab the color setting for the given slot from the configuration,\n-\t * or parse the default value if missing, and return ANSI color\n-\t * escape sequence.\n-\t *\n-\t * e.g.\n-\t * git config --get-color color.diff.whitespace \"blue reverse\"\n-\t */\n-\tconst char *def_color = NULL;\n-\n-\tswitch (argc) {\n-\tdefault:\n-\t\tusage(git_config_set_usage);\n-\tcase 2:\n-\t\tdef_color = argv[1];\n-\t\t/* fallthru */\n-\tcase 1:\n-\t\tget_color_slot = argv[0];\n-\t\tbreak;\n-\t}\n-\n-\tget_color_found = 0;\n-\tparsed_color[0] = '\\0';\n-\tgit_config(git_get_color_config, NULL);\n-\n-\tif (!get_color_found && def_color)\n-\t\tcolor_parse(def_color, \"command line\", parsed_color);\n-\n-\tfputs(parsed_color, stdout);\n-\treturn 0;\n-}\n-\n static int stdout_is_tty;\n static int get_colorbool_found;\n static int get_diff_color_found;\n static int git_get_colorbool_config(const char *var, const char *value,\n \t\tvoid *cb)\n {\n-\tif (!strcmp(var, get_color_slot)) {\n+\tif (!strcmp(var, get_colorbool_name)) {\n \t\tget_colorbool_found =\n \t\t\tgit_config_colorbool(var, value, stdout_is_tty);\n \t}\n@@ -246,41 +250,6 @@ static int git_get_colorbool_config(const char *var, const char *value,\n \treturn 0;\n }\n \n-static int get_colorbool(int argc, const char **argv)\n-{\n-\t/*\n-\t * git config --get-colorbool []\n-\t *\n-\t * returns \"true\" or \"false\" depending on how \n-\t * is configured.\n-\t */\n-\n-\tif (argc == 2)\n-\t\tstdout_is_tty = git_config_bool(\"command line\", argv[1]);\n-\telse if (argc == 1)\n-\t\tstdout_is_tty = isatty(1);\n-\telse\n-\t\tusage(git_config_set_usage);\n-\tget_colorbool_found = -1;\n-\tget_diff_color_found = -1;\n-\tget_color_slot = argv[0];\n-\tgit_config(git_get_colorbool_config, NULL);\n-\n-\tif (get_colorbool_found < 0) {\n-\t\tif (!strcmp(get_color_slot, \"color.diff\"))\n-\t\t\tget_colorbool_found = get_diff_color_found;\n-\t\tif (get_colorbool_found < 0)\n-\t\t\tget_colorbool_found = git_use_color_default;\n-\t}\n-\n-\tif (argc == 1) {\n-\t\treturn get_colorbool_found ? 0 : 1;\n-\t} else {\n-\t\tprintf(\"%s\\n\", get_colorbool_found ? \"true\" : \"false\");\n-\t\treturn 0;\n-\t}\n-}\n-\n int cmd_config(int argc, const char **argv, const char *prefix)\n {\n \tint nongit;\n@@ -289,151 +258,180 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \n \tconfig_exclusive_filename = getenv(CONFIG_ENVIRONMENT);\n \n-\twhile (1 < argc) {\n-\t\tif (!strcmp(argv[1], \"--int\"))\n-\t\t\ttype = T_INT;\n-\t\telse if (!strcmp(argv[1], \"--bool\"))\n-\t\t\ttype = T_BOOL;\n-\t\telse if (!strcmp(argv[1], \"--bool-or-int\"))\n-\t\t\ttype = T_BOOL_OR_INT;\n-\t\telse if (!strcmp(argv[1], \"--list\") || !strcmp(argv[1], \"-l\")) {\n-\t\t\tif (argc != 2)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tif (git_config(show_all_config, NULL) < 0 &&\n-\t\t\t\t\tfile && errno)\n-\t\t\t\tdie(\"unable to read config file %s: %s\", file,\n-\t\t\t\t strerror(errno));\n-\t\t\treturn 0;\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--global\")) {\n-\t\t\tchar *home = getenv(\"HOME\");\n-\t\t\tif (home) {\n-\t\t\t\tchar *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n-\t\t\t\tconfig_exclusive_filename = user_config;\n-\t\t\t} else {\n-\t\t\t\tdie(\"$HOME not set\");\n-\t\t\t}\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--system\"))\n-\t\t\tconfig_exclusive_filename = git_etc_gitconfig();\n-\t\telse if (!strcmp(argv[1], \"--file\") || !strcmp(argv[1], \"-f\")) {\n-\t\t\tif (argc < 3)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tif (!is_absolute_path(argv[2]) && file)\n-\t\t\t\tfile = prefix_filename(file, strlen(file),\n-\t\t\t\t\t\t argv[2]);\n-\t\t\telse\n-\t\t\t\tfile = argv[2];\n-\t\t\tconfig_exclusive_filename = file;\n-\t\t\targc--;\n-\t\t\targv++;\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--null\") || !strcmp(argv[1], \"-z\")) {\n-\t\t\tterm = '\\0';\n-\t\t\tdelim = '\\n';\n-\t\t\tkey_delim = '\\n';\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--rename-section\")) {\n-\t\t\tint ret;\n-\t\t\tif (argc != 4)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tret = git_config_rename_section(argv[2], argv[3]);\n-\t\t\tif (ret < 0)\n-\t\t\t\treturn ret;\n-\t\t\tif (ret == 0) {\n-\t\t\t\tfprintf(stderr, \"No such section!\\n\");\n-\t\t\t\treturn 1;\n-\t\t\t}\n-\t\t\treturn 0;\n+\targc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);\n+\n+\tif (use_global_config) {\n+\t\tchar *home = getenv(\"HOME\");\n+\t\tif (home) {\n+\t\t\tchar *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n+\t\t\tconfig_exclusive_filename = user_config;\n+\t\t} else {\n+\t\t\tdie(\"$HOME not set\");\n \t\t}\n-\t\telse if (!strcmp(argv[1], \"--remove-section\")) {\n-\t\t\tint ret;\n-\t\t\tif (argc != 3)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tret = git_config_rename_section(argv[2], NULL);\n-\t\t\tif (ret < 0)\n-\t\t\t\treturn ret;\n-\t\t\tif (ret == 0) {\n-\t\t\t\tfprintf(stderr, \"No such section!\\n\");\n-\t\t\t\treturn 1;\n+\t}\n+\telse if (use_system_config)\n+\t\tconfig_exclusive_filename = git_etc_gitconfig();\n+\telse if (given_config_file) {\n+\t\tif (!is_absolute_path(given_config_file) && file)\n+\t\t\tfile = prefix_filename(file, strlen(file),\n+\t\t\t\t\t given_config_file);\n+\t\telse\n+\t\t\tfile = given_config_file;\n+\t\tconfig_exclusive_filename = file;\n+\t}\n+\n+\tif (type_int)\n+\t\ttype = T_INT;\n+\telse if (type_bool)\n+\t\ttype = T_BOOL;\n+\telse if (type_bool_or_int)\n+\t\ttype = T_BOOL_OR_INT;\n+\n+\tif (end_null) {\n+\t\tterm = '\\0';\n+\t\tdelim = '\\n';\n+\t\tkey_delim = '\\n';\n+\t}\n+\n+\t{\n+\t\tint action_sum;\n+\t\taction_sum = do_unset + do_unset_all + do_get + do_get_all + \\\n+\t\t\t do_get_regexp + do_add + do_list + do_edit + \\\n+\t\t\t do_rename_section + do_remove_section + do_replace_all;\n+\t\tif (action_sum > 1)\n+\t\t\tdie (\"Can't execute two actions at the same time.\");\n+\t\telse if (action_sum == 0)\n+\t\t\tswitch (argc)\n+\t\t\t{\n+\t\t\t\tcase 1: do_get = 1; break;\n+\t\t\t\tcase 2: do_add = 1; break;\n+\t\t\t\tcase 3: do_replace_all = 1; break;\n+\t\t\t\tdefault:\n+\t\t\t\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t\t\t}\n-\t\t\treturn 0;\n-\t\t} else if (!strcmp(argv[1], \"--get-color\")) {\n-\t\t\treturn get_color(argc-2, argv+2);\n-\t\t} else if (!strcmp(argv[1], \"--get-colorbool\")) {\n-\t\t\treturn get_colorbool(argc-2, argv+2);\n-\t\t} else if (!strcmp(argv[1], \"--edit\") || !strcmp(argv[1], \"-e\")) {\n-\t\t\tconst char *config_filename;\n-\t\t\tif (argc != 2)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tif (config_exclusive_filename)\n-\t\t\t\tconfig_filename = config_exclusive_filename;\n-\t\t\telse\n-\t\t\t\tconfig_filename = git_path(\"config\");\n-\t\t\tgit_config(git_default_config, NULL);\n-\t\t\tlaunch_editor(config_filename, NULL, NULL);\n-\t\t\treturn 0;\n-\t\t} else\n+\t}\n+\n+\tif (do_list) {\n+\t\tif (git_config(show_all_config, NULL) < 0 &&\n+\t\t file && errno)\n+\t\t\tdie(\"unable to read config file %s: %s\", file,\n+\t\t\t strerror(errno));\n+\t}\n+\telse if (do_edit) {\n+\t\tconst char *config_filename;\n+\t\tif (config_exclusive_filename)\n+\t\t\tconfig_filename = config_exclusive_filename;\n+\t\telse\n+\t\t\tconfig_filename = git_path(\"config\");\n+\t\tgit_config(git_default_config, NULL);\n+\t\tlaunch_editor(config_filename, NULL, NULL);\n+\t}\n+\telse if (do_add) {\n+\t\tif (argc > 2)\n+\t\t\tdie(\"Too many arguments.\");\n+\t\tif (argc != 2)\n+\t\t\tdie(\"Need name value.\");\n+\t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\treturn git_config_set_multivar(argv[0], value, \"^$\", 0);\n+\t}\n+\telse if (do_replace_all) {\n+\t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\treturn git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);\n+\t}\n+\telse if (do_get)\n+\t\treturn get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n+\telse if (do_get_all) {\n+\t\tdo_all = 1;\n+\t\treturn get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n+\t}\n+\telse if (do_get_regexp) {\n+\t\tshow_keys = 1;\n+\t\tuse_key_regexp = 1;\n+\t\tdo_all = 1;\n+\t\treturn get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n+\t}\n+\telse if (do_unset) {\n+\t\tif (argc == 2)\n+\t\t\treturn git_config_set_multivar(argv[0], NULL, argv[1], 0);\n+\t\telse\n+\t\t\treturn git_config_set(argv[0], NULL);\n+\t}\n+\telse if (do_unset_all) {\n+\t\treturn git_config_set_multivar(argv[0], NULL, (argc == 2 ? argv[1] : NULL), 1);\n+\t}\n+\telse if (do_rename_section) {\n+\t\tint ret;\n+\t\tif (argc > 2)\n+\t\t\tdie(\"Too many arguments.\");\n+\t\tif (argc != 2)\n+\t\t\tdie(\"Need old_name new_name.\");\n+\t\tret = git_config_rename_section(argv[0], argv[1]);\n+\t\tif (ret < 0)\n+\t\t\treturn ret;\n+\t\tif (ret == 0)\n+\t\t\tdie(\"No such section!\");\n+\t}\n+\telse if (do_remove_section) {\n+\t\tint ret;\n+\t\tif (argc > 1)\n+\t\t\tdie(\"Too many arguments.\");\n+\t\tif (argc != 1)\n+\t\t\tdie(\"Need section name.\");\n+\t\tret = git_config_rename_section(argv[0], NULL);\n+\t\tif (ret < 0)\n+\t\t\treturn ret;\n+\t\tif (ret == 0)\n+\t\t\tdie(\"No such section!\");\n+\t}\n+\telse if (get_color_name) {\n+\t\tconst char *def_color = NULL;\n+\n+\t\tswitch (argc) {\n+\t\tcase 2:\n+\t\t\tdef_color = argv[1];\n+\t\t\t/* fallthru */\n+\t\tcase 1:\n+\t\t\tget_color_name = argv[0];\n \t\t\tbreak;\n-\t\targc--;\n-\t\targv++;\n+\t\tdefault:\n+\t\t\tdie(\"Too many arguments.\");\n+\t\t}\n+\n+\t\tget_color_found = 0;\n+\t\tparsed_color[0] = '\\0';\n+\t\tgit_config(git_get_color_config, NULL);\n+\n+\t\tif (!get_color_found && def_color)\n+\t\t\tcolor_parse(def_color, \"command line\", parsed_color);\n+\n+\t\tfputs(parsed_color, stdout);\n \t}\n+\telse if (get_colorbool_name) {\n+\t\tif (argc == 1)\n+\t\t\tstdout_is_tty = git_config_bool(\"command line\", argv[0]);\n+\t\telse if (argc == 0)\n+\t\t\tstdout_is_tty = isatty(1);\n+\t\telse\n+\t\t\tdie(\"Too many options.\");\n \n-\tswitch (argc) {\n-\tcase 2:\n-\t\treturn get_value(argv[1], NULL);\n-\tcase 3:\n-\t\tif (!strcmp(argv[1], \"--unset\"))\n-\t\t\treturn git_config_set(argv[2], NULL);\n-\t\telse if (!strcmp(argv[1], \"--unset-all\"))\n-\t\t\treturn git_config_set_multivar(argv[2], NULL, NULL, 1);\n-\t\telse if (!strcmp(argv[1], \"--get\"))\n-\t\t\treturn get_value(argv[2], NULL);\n-\t\telse if (!strcmp(argv[1], \"--get-all\")) {\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], NULL);\n-\t\t} else if (!strcmp(argv[1], \"--get-regexp\")) {\n-\t\t\tshow_keys = 1;\n-\t\t\tuse_key_regexp = 1;\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], NULL);\n-\t\t} else {\n-\t\t\tvalue = normalize_value(argv[1], argv[2]);\n-\t\t\treturn git_config_set(argv[1], value);\n+\t\tget_colorbool_found = -1;\n+\t\tget_diff_color_found = -1;\n+\t\tgit_config(git_get_colorbool_config, NULL);\n+\n+\t\tif (get_colorbool_found < 0) {\n+\t\t\tif (!strcmp(get_colorbool_name, \"color.diff\"))\n+\t\t\t\tget_colorbool_found = get_diff_color_found;\n+\t\t\tif (get_colorbool_found < 0)\n+\t\t\t\tget_colorbool_found = git_use_color_default;\n \t\t}\n-\tcase 4:\n-\t\tif (!strcmp(argv[1], \"--unset\"))\n-\t\t\treturn git_config_set_multivar(argv[2], NULL, argv[3], 0);\n-\t\telse if (!strcmp(argv[1], \"--unset-all\"))\n-\t\t\treturn git_config_set_multivar(argv[2], NULL, argv[3], 1);\n-\t\telse if (!strcmp(argv[1], \"--get\"))\n-\t\t\treturn get_value(argv[2], argv[3]);\n-\t\telse if (!strcmp(argv[1], \"--get-all\")) {\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], argv[3]);\n-\t\t} else if (!strcmp(argv[1], \"--get-regexp\")) {\n-\t\t\tshow_keys = 1;\n-\t\t\tuse_key_regexp = 1;\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], argv[3]);\n-\t\t} else if (!strcmp(argv[1], \"--add\")) {\n-\t\t\tvalue = normalize_value(argv[2], argv[3]);\n-\t\t\treturn git_config_set_multivar(argv[2], value, \"^$\", 0);\n-\t\t} else if (!strcmp(argv[1], \"--replace-all\")) {\n-\t\t\tvalue = normalize_value(argv[2], argv[3]);\n-\t\t\treturn git_config_set_multivar(argv[2], value, NULL, 1);\n+\n+\t\tif (argc == 0) {\n+\t\t\treturn get_colorbool_found ? 0 : 1;\n \t\t} else {\n-\t\t\tvalue = normalize_value(argv[1], argv[2]);\n-\t\t\treturn git_config_set_multivar(argv[1], value, argv[3], 0);\n-\t\t}\n-\tcase 5:\n-\t\tif (!strcmp(argv[1], \"--replace-all\")) {\n-\t\t\tvalue = normalize_value(argv[2], argv[3]);\n-\t\t\treturn git_config_set_multivar(argv[2], value, argv[4], 1);\n+\t\t\tprintf(\"%s\\n\", get_colorbool_found ? \"true\" : \"false\");\n+\t\t\treturn 0;\n \t\t}\n-\tcase 1:\n-\tdefault:\n-\t\tusage(git_config_set_usage);\n \t}\n+\n \treturn 0;\n }\n-- \n1.6.1.3\n"},{"id":"104615","messageId":"7vab8pweod.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"1234577142-22965-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-14T09:28:18Z","receivedAt":"2009-02-14T09:28:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Reorganizing the code to use parseopt as suggested by Johannes\n> Schindelin.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin-config.c | 422 +++++++++++++++++++++++++++---------------------------\n> 1 files changed, 210 insertions(+), 212 deletions(-)\n\nWhew. That's a lot of changes.\n\nIs this just \"I am using parseopt because I can\", or \"I want to do this\nfirst because I am planning to do such and such things to this program,\nand the current mess needs to be cleaned up first before doing so\"?\n\nI am asking this _not_ because I'd want to reject the patch if the answer\nis former.\n\n> diff --git a/builtin-config.c b/builtin-config.c\n> index afc4393..f774902 100644\n> --- a/builtin-config.c\n> +++ b/builtin-config.c\n> @@ -1,9 +1,12 @@\n> #include \"builtin.h\"\n> #include \"cache.h\"\n> #include \"color.h\"\n> +#include \"parse-options.h\"\n> \n> -static const char git_config_set_usage[] =\n> -\"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 ]\";\n> +static const char *const builtin_config_usage[] = {\n> +\t\"git config [options]\",\n> +\tNULL\n> +};\n> \n> static char *key;\n> static regex_t *key_regexp;\n> @@ -18,6 +21,42 @@ static char key_delim = ' ';\n> static char term = '\\n';\n> static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;\n> \n> +static int use_global_config, use_system_config;\n> +static const char *given_config_file;\n> +static int do_list, do_edit, do_add, do_get, do_get_all, do_get_regexp, do_replace_all;\n> +static int do_unset, do_unset_all, do_rename_section, do_remove_section;\n> +static int type_int, type_bool, type_bool_or_int;\n\nYou can have either (no type specified, int, bool, bool-or-int) at the\nend. Using three independent variables does not feel right.\n\nHint: OPTION_SET_INT.\n\n> +static const char *get_color_name, *get_colorbool_name;\n> +static int end_null;\n> +\n> +static struct option builtin_config_options[] = {\n> +\tOPT_GROUP(\"Config file location\"),\n> +\tOPT_BOOLEAN(0, \"global\", &use_global_config, \"use global config file\"),\n> +\tOPT_BOOLEAN(0, \"system\", &use_system_config, \"use system config file\"),\n> +\tOPT_STRING('f', \"file\", &given_config_file, \"FILE\", \"use given config file\"),\n\nWhy CAPS?\n\n> +\tOPT_GROUP(\"Action\"),\n> +\tOPT_BOOLEAN(0, \"get\", &do_get, \"get value: name [value-regex]\"),\n> +\tOPT_BOOLEAN(0, \"get-all\", &do_get_all, \"get all values: key [value-regex]\"),\n> +\tOPT_BOOLEAN(0, \"get-regexp\", &do_get_regexp, \"get values for regexp: name-regex [value-regex]\"),\n> +\tOPT_BOOLEAN(0, \"replace-all\", &do_replace_all, \"replace all options: name [value [value_regex]\"),\n\nall matching variables?\n\n> +\tOPT_BOOLEAN(0, \"add\", &do_add, \"adds a new option: name value\"),\n\nnew variable?\n\n> +\tOPT_BOOLEAN(0, \"unset\", &do_unset, \"removes an option: name [value-regex]\"),\n\nPlease don't introduce a new noun \"option\" that has never been used to\nmean a \"configuration variable\" in git documentation. It unnecessarily\nconfuses everybody.\n\n> +\tOPT_BOOLEAN(0, \"unset-all\", &do_unset_all, \"removes all matches: name [value-regex]\"),\n> +\tOPT_BOOLEAN(0, \"rename-section\", &do_rename_section, \"rename section: old-name new-name\"),\n> +\tOPT_BOOLEAN(0, \"remove-section\", &do_remove_section, \"remove a section: name\"),\n> +\tOPT_BOOLEAN('l', \"list\", &do_list, \"list all\"),\n> +\tOPT_STRING(0, \"get-color\", &get_color_name, \"name\", \"find the color configured: [default]\"),\n\nget-color is used to get the defined color for a given slot. Please do not\nrename it to \"name\" (see the original).\n\n> +\tOPT_STRING(0, \"get-colorbool\", &get_colorbool_name, \"name\", \"find the color setting: [stdout-is-tty]\"),\n\nget-colorbool is used to get the boolean setting for a given configuration\nvariable. Please do not rename it to \"name\" (see the original).\n\n> +\tOPT_BOOLEAN('e', \"edit\", &do_edit, \"opens an editor\"),\n> +\tOPT_GROUP(\"Type\"),\n> +\tOPT_BOOLEAN(0, \"bool\", &type_bool, \"value is \\\"true\\\" or \\\"false\\\"\"),\n> +\tOPT_BOOLEAN(0, \"int\", &type_int, \"value is decimal number\"),\n> +\tOPT_BOOLEAN(0, \"bool-or-int\", &type_bool_or_int, NULL),\n> +\tOPT_GROUP(\"Other\"),\n> +\tOPT_BOOLEAN('z', \"null\", &end_null, \"end values with null character\"),\n\nThe name of the character is NUL (with a single el). I'd prefer this to\nsay either \"use machine readable output format\", or \"terminate values with NUL byte\".\n\n> +\tOPT_END(),\n> +};\n> +\n> static int show_all_config(const char *key_, const char *value_, void *cb)\n> {\n> \tif (value_)\n> @@ -177,12 +216,11 @@ static char *normalize_value(const char *key, const char *value)\n> }\n> \n> static int get_color_found;\n> -static const char *get_color_slot;\n> +static const char *get_color_name;\n\nWhy?\n\n> @@ -289,151 +258,180 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> ...\n> +\t{\n> +\t\tint action_sum;\n> +\t\taction_sum = do_unset + do_unset_all + do_get + do_get_all + \\\n> +\t\t\t do_get_regexp + do_add + do_list + do_edit + \\\n> +\t\t\t do_rename_section + do_remove_section + do_replace_all;\n> +\t\tif (action_sum > 1)\n> +\t\t\tdie (\"Can't execute two actions at the same time.\");\n\nHmph. I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.\n\n> +\telse if (do_add) {\n> +\t\tif (argc > 2)\n> +\t\t\tdie(\"Too many arguments.\");\n> +\t\tif (argc != 2)\n> +\t\t\tdie(\"Need name value.\");\n> +\t\tvalue = normalize_value(argv[0], argv[1]);\n> +\t\treturn git_config_set_multivar(argv[0], value, \"^$\", 0);\n\nThis part did not lose argc error checking, but...\n\n> +\t}\n> +\telse if (do_replace_all) {\n> +\t\tvalue = normalize_value(argv[0], argv[1]);\n> +\t\treturn git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);\n\nYou do not check argc here (nor in many \"else if\" below) to make sure you\nhave sufficient number of arguments. \"git config --unset\" is now allowed\nto segfault, and \"git config --unset a b c d e f\" can silently ignore\nexcess arguments for example?\n\n> +\t}\n> +\telse if (do_get)\n> +\t\treturn get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n> +\telse if (do_get_all) {\n> +\t\tdo_all = 1;\n> +\t\treturn get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n> +\t}\n> +\telse if (do_get_regexp) {\n> +\t\tshow_keys = 1;\n> +\t\tuse_key_regexp = 1;\n> +\t\tdo_all = 1;\n> +\t\treturn get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n> +\t}\n> +\telse if (do_unset) {\n> +\t\tif (argc == 2)\n> +\t\t\treturn git_config_set_multivar(argv[0], NULL, argv[1], 0);\n> +\t\telse\n> +\t\t\treturn git_config_set(argv[0], NULL);\n> +\t}\n> ... similar error-ridden parts omitted ...\n\n> +\telse if (get_color_name) {\n> +\t\tconst char *def_color = NULL;\n> +\n> +\t\tswitch (argc) {\n> +\t\tcase 2:\n> +\t\t\tdef_color = argv[1];\n> +\t\t\t/* fallthru */\n> +\t\tcase 1:\n> +\t\t\tget_color_name = argv[0];\n> \t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tdie(\"Too many arguments.\");\n> +\t\t}\n> +\n> +\t\tget_color_found = 0;\n> +\t\tparsed_color[0] = '\\0';\n> +\t\tgit_config(git_get_color_config, NULL);\n> +\n> +\t\tif (!get_color_found && def_color)\n> +\t\t\tcolor_parse(def_color, \"command line\", parsed_color);\n> +\n> +\t\tfputs(parsed_color, stdout);\n\nThis is actively worse than the original that uses a separate helper\nfunction in readability.\n\n> \t}\n> +\telse if (get_colorbool_name) {\n> +\t\tif (argc == 1)\n> +\t\t\tstdout_is_tty = git_config_bool(\"command line\", argv[0]);\n> +\t\telse if (argc == 0)\n> +\t\t\tstdout_is_tty = isatty(1);\n> +\t\telse\n> +\t\t\tdie(\"Too many options.\");\n> \n> +\t\tget_colorbool_found = -1;\n> +\t\tget_diff_color_found = -1;\n> +\t\tgit_config(git_get_colorbool_config, NULL);\n> +\n> +\t\tif (get_colorbool_found < 0) {\n> +\t\t\tif (!strcmp(get_colorbool_name, \"color.diff\"))\n> +\t\t\t\tget_colorbool_found = get_diff_color_found;\n> +\t\t\tif (get_colorbool_found < 0)\n> +\t\t\t\tget_colorbool_found = git_use_color_default;\n> \t\t}\n> +\n> +\t\tif (argc == 0) {\n> +\t\t\treturn get_colorbool_found ? 0 : 1;\n> \t\t} else {\n> +\t\t\tprintf(\"%s\\n\", get_colorbool_found ? \"true\" : \"false\");\n> +\t\t\treturn 0;\n> \t\t}\n\nLikewise.\n\nOverall, with the same number of lines, we lost a lot of error checking in\nexchange for an ability to say \"git config --remove-sec\" instead of \"git\nconfig --remove-section\", and \"git config --help\" may give an easier to\nread message.\n\nGiven that \"git config\" is primarily meant for script use, this particular\nround does not look like a good tradeoff to me.\n"},{"id":"104617","messageId":"7v1vu1webk.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"7vab8pweod.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-14T09:35:59Z","receivedAt":"2009-02-14T09:35:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\tOPT_STRING(0, \"get-color\", &get_color_name, \"name\", \"find the color configured: [default]\"),\n>\n> get-color is used to get the defined color for a given slot. Please do not\n> rename it to \"name\" (see the original).\n>\n>> +\tOPT_STRING(0, \"get-colorbool\", &get_colorbool_name, \"name\", \"find the color setting: [stdout-is-tty]\"),\n>\n> get-colorbool is used to get the boolean setting for a given configuration\n> variable. Please do not rename it to \"name\" (see the original).\n>\n\nBy the way, I think it might be more appropriate if you categorize the\nabove two (especially the \"colorbool\" one) in the \"Types\" section, as that\nreally is what is happening. Instead of doing the usual \"print the value\nas a string\", it does something magical.\n\n> Overall, with the same number of lines, we lost a lot of error checking in\n> exchange for an ability to say \"git config --remove-sec\" instead of \"git\n> config --remove-section\", and \"git config --help\" may give an easier to\n> read message.\n\nAnd I forgot to mention the \"option categorization\" merit.\n\n> Given that \"git config\" is primarily meant for script use, this particular\n> round does not look like a good tradeoff to me.\n\nDon't take this too negatively. The tradeoff will improve if there aren't\nthese obvious bugs you can spot without even running the code.\n"},{"id":"104624","messageId":"94a0d4530902140237o7d26ff4j1c7350d926d12c1a@mail.gmail.com","threadId":"17782","inReplyTo":"7vab8pweod.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T10:37:23Z","receivedAt":"2009-02-14T10:37:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> Reorganizing the code to use parseopt as suggested by Johannes\n>> Schindelin.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> builtin-config.c | 422 +++++++++++++++++++++++++++---------------------------\n>> 1 files changed, 210 insertions(+), 212 deletions(-)\n>\n> Whew. That's a lot of changes.\n>\n> Is this just \"I am using parseopt because I can\", or \"I want to do this\n> first because I am planning to do such and such things to this program,\n> and the current mess needs to be cleaned up first before doing so\"?\n>\n> I am asking this _not_ because I'd want to reject the patch if the answer\n> is former.\n\nThen why are you asking?\n\nThis is more a \"I would like to increase the chances of my patches\nbeing accepted so I'd do some chores to gain the trust of some\ndevelopers\", and Johannes Schindelin was pushing me to do this.\n\nAlso it's a bit of \"I would like to improve git and learn the API\nwhile doing so\".\n\n>> diff --git a/builtin-config.c b/builtin-config.c\n>> index afc4393..f774902 100644\n>> --- a/builtin-config.c\n>> +++ b/builtin-config.c\n>> @@ -1,9 +1,12 @@\n>> #include \"builtin.h\"\n>> #include \"cache.h\"\n>> #include \"color.h\"\n>> +#include \"parse-options.h\"\n>>\n>> -static const char git_config_set_usage[] =\n>> -\"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 ]\";\n>> +static const char *const builtin_config_usage[] = {\n>> + \"git config [options]\",\n>> + NULL\n>> +};\n>>\n>> static char *key;\n>> static regex_t *key_regexp;\n>> @@ -18,6 +21,42 @@ static char key_delim = ' ';\n>> static char term = '\\n';\n>> static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;\n>>\n>> +static int use_global_config, use_system_config;\n>> +static const char *given_config_file;\n>> +static int do_list, do_edit, do_add, do_get, do_get_all, do_get_regexp, do_replace_all;\n>> +static int do_unset, do_unset_all, do_rename_section, do_remove_section;\n>> +static int type_int, type_bool, type_bool_or_int;\n>\n> You can have either (no type specified, int, bool, bool-or-int) at the\n> end. Using three independent variables does not feel right.\n>\n> Hint: OPTION_SET_INT.\n\nThat definitely makes things easier, it would have been nice to see an\nexample of this; I didn't knew it was there.\n\nThe only problem is that --bool and --int would be possible in the\nsame command and there would be no way to output an error, but I guess\nthat's not a big problem.\n\n>> +static const char *get_color_name, *get_colorbool_name;\n>> +static int end_null;\n>> +\n>> +static struct option builtin_config_options[] = {\n>> + OPT_GROUP(\"Config file location\"),\n>> + OPT_BOOLEAN(0, \"global\", &use_global_config, \"use global config file\"),\n>> + OPT_BOOLEAN(0, \"system\", &use_system_config, \"use system config file\"),\n>> + OPT_STRING('f', \"file\", &given_config_file, \"FILE\", \"use given config file\"),\n>\n> Why CAPS?\n\nI looked at some other code, like builtin-commit.c and that's what is\nused there.\n\n>> + OPT_GROUP(\"Action\"),\n>> + OPT_BOOLEAN(0, \"get\", &do_get, \"get value: name [value-regex]\"),\n>> + OPT_BOOLEAN(0, \"get-all\", &do_get_all, \"get all values: key [value-regex]\"),\n>> + OPT_BOOLEAN(0, \"get-regexp\", &do_get_regexp, \"get values for regexp: name-regex [value-regex]\"),\n>> + OPT_BOOLEAN(0, \"replace-all\", &do_replace_all, \"replace all options: name [value [value_regex]\"),\n>\n> all matching variables?\n\nOk.\n\n>> + OPT_BOOLEAN(0, \"add\", &do_add, \"adds a new option: name value\"),\n>\n> new variable?\n\nOk.\n\n>> + OPT_BOOLEAN(0, \"unset\", &do_unset, \"removes an option: name [value-regex]\"),\n>\n> Please don't introduce a new noun \"option\" that has never been used to\n> mean a \"configuration variable\" in git documentation. It unnecessarily\n> confuses everybody.\n\nOk.\n\nAnd true, \"config\" files don't only contain options.\n\n>> + OPT_BOOLEAN(0, \"unset-all\", &do_unset_all, \"removes all matches: name [value-regex]\"),\n>> + OPT_BOOLEAN(0, \"rename-section\", &do_rename_section, \"rename section: old-name new-name\"),\n>> + OPT_BOOLEAN(0, \"remove-section\", &do_remove_section, \"remove a section: name\"),\n>> + OPT_BOOLEAN('l', \"list\", &do_list, \"list all\"),\n>> + OPT_STRING(0, \"get-color\", &get_color_name, \"name\", \"find the color configured: [default]\"),\n>\n> get-color is used to get the defined color for a given slot. Please do not\n> rename it to \"name\" (see the original).\n\nThe documentation says \"Find the color configured for `name`\" so the\nfirst argument would be \"name\", I'm just storing that argument.\n\nPerhaps the documentation needs to be improved?\n\n>> + OPT_STRING(0, \"get-colorbool\", &get_colorbool_name, \"name\", \"find the color setting: [stdout-is-tty]\"),\n>\n> get-colorbool is used to get the boolean setting for a given configuration\n> variable. Please do not rename it to \"name\" (see the original).\n\nSame as get-color, right?\n\n>> + OPT_BOOLEAN('e', \"edit\", &do_edit, \"opens an editor\"),\n>> + OPT_GROUP(\"Type\"),\n>> + OPT_BOOLEAN(0, \"bool\", &type_bool, \"value is \\\"true\\\" or \\\"false\\\"\"),\n>> + OPT_BOOLEAN(0, \"int\", &type_int, \"value is decimal number\"),\n>> + OPT_BOOLEAN(0, \"bool-or-int\", &type_bool_or_int, NULL),\n>> + OPT_GROUP(\"Other\"),\n>> + OPT_BOOLEAN('z', \"null\", &end_null, \"end values with null character\"),\n>\n> The name of the character is NUL (with a single el). I'd prefer this to\n> say either \"use machine readable output format\", or \"terminate values with NUL byte\".\n\nI'd prefer \"terminate values with NUL byte\".\n\n>> + OPT_END(),\n>> +};\n>> +\n>> static int show_all_config(const char *key_, const char *value_, void *cb)\n>> {\n>> if (value_)\n>> @@ -177,12 +216,11 @@ static char *normalize_value(const char *key, const char *value)\n>> }\n>>\n>> static int get_color_found;\n>> -static const char *get_color_slot;\n>> +static const char *get_color_name;\n>\n> Why?\n\nBecause the documentation says the argument is a 'name'.\n\n>> @@ -289,151 +258,180 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>> ...\n>> + {\n>> + int action_sum;\n>> + action_sum = do_unset + do_unset_all + do_get + do_get_all + \\\n>> + do_get_regexp + do_add + do_list + do_edit + \\\n>> + do_rename_section + do_remove_section + do_replace_all;\n>> + if (action_sum > 1)\n>> + die (\"Can't execute two actions at the same time.\");\n>\n> Hmph. I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.\n\nYeap, definitely, thanks for the suggestion.\n\nAgain, it would have been nice to see any example of this.\n\n>> + else if (do_add) {\n>> + if (argc > 2)\n>> + die(\"Too many arguments.\");\n>> + if (argc != 2)\n>> + die(\"Need name value.\");\n>> + value = normalize_value(argv[0], argv[1]);\n>> + return git_config_set_multivar(argv[0], value, \"^$\", 0);\n>\n> This part did not lose argc error checking, but...\n>\n>> + }\n>> + else if (do_replace_all) {\n>> + value = normalize_value(argv[0], argv[1]);\n>> + return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);\n>\n> You do not check argc here (nor in many \"else if\" below) to make sure you\n> have sufficient number of arguments. \"git config --unset\" is now allowed\n> to segfault, and \"git config --unset a b c d e f\" can silently ignore\n> excess arguments for example?\n\nYes the arguments check need to be revised.\n\nMy hope was somebody would review this and suggest a clever and\ngeneric way of doing this. Perhaps a util function check_min_args, or\nmaybe something in parseopt that receives the number of args?\n\nAlso, since I sent the patch I've learned that if argc is 2, then\nargv[2] will be NULL, so the argc == 3 check is redundant.\n\n>> + }\n>> + else if (do_get)\n>> + return get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n>> + else if (do_get_all) {\n>> + do_all = 1;\n>> + return get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n>> + }\n>> + else if (do_get_regexp) {\n>> + show_keys = 1;\n>> + use_key_regexp = 1;\n>> + do_all = 1;\n>> + return get_value(argv[0], (argc == 2 ? argv[1] : NULL));\n>> + }\n>> + else if (do_unset) {\n>> + if (argc == 2)\n>> + return git_config_set_multivar(argv[0], NULL, argv[1], 0);\n>> + else\n>> + return git_config_set(argv[0], NULL);\n>> + }\n>> ... similar error-ridden parts omitted ...\n>\n>> + else if (get_color_name) {\n>> + const char *def_color = NULL;\n>> +\n>> + switch (argc) {\n>> + case 2:\n>> + def_color = argv[1];\n>> + /* fallthru */\n>> + case 1:\n>> + get_color_name = argv[0];\n>> break;\n>> + default:\n>> + die(\"Too many arguments.\");\n>> + }\n>> +\n>> + get_color_found = 0;\n>> + parsed_color[0] = '\\0';\n>> + git_config(git_get_color_config, NULL);\n>> +\n>> + if (!get_color_found && def_color)\n>> + color_parse(def_color, \"command line\", parsed_color);\n>> +\n>> + fputs(parsed_color, stdout);\n>\n> This is actively worse than the original that uses a separate helper\n> function in readability.\n\nThe original separate helper functions where helping parse the args.\nSince that's not happening any more I saw no reason to pass the argc\nand argv variables, so I thought maybe these options should not be\ntreated any differently than other options.\n\nAnyway, I can create the helper functions again, and maybe have some\nfunction parameters instead of passing argc and argv\n\n>> }\n>> + else if (get_colorbool_name) {\n>> + if (argc == 1)\n>> + stdout_is_tty = git_config_bool(\"command line\", argv[0]);\n>> + else if (argc == 0)\n>> + stdout_is_tty = isatty(1);\n>> + else\n>> + die(\"Too many options.\");\n>>\n>> + get_colorbool_found = -1;\n>> + get_diff_color_found = -1;\n>> + git_config(git_get_colorbool_config, NULL);\n>> +\n>> + if (get_colorbool_found < 0) {\n>> + if (!strcmp(get_colorbool_name, \"color.diff\"))\n>> + get_colorbool_found = get_diff_color_found;\n>> + if (get_colorbool_found < 0)\n>> + get_colorbool_found = git_use_color_default;\n>> }\n>> +\n>> + if (argc == 0) {\n>> + return get_colorbool_found ? 0 : 1;\n>> } else {\n>> + printf(\"%s\\n\", get_colorbool_found ? \"true\" : \"false\");\n>> + return 0;\n>> }\n>\n> Likewise.\n>\n> Overall, with the same number of lines, we lost a lot of error checking in\n> exchange for an ability to say \"git config --remove-sec\" instead of \"git\n> config --remove-section\", and \"git config --help\" may give an easier to\n> read message.\n>\n> Given that \"git config\" is primarily meant for script use, this particular\n> round does not look like a good tradeoff to me.\n\nThat doesn't mean it shouldn't have a nice UI.\n\nAlso, I think the code would be easier to maintain with parseopt.\n\n-- \nFelipe Contreras\n"},{"id":"104625","messageId":"94a0d4530902140241j6547a1d2kd51f784916770943@mail.gmail.com","threadId":"17782","inReplyTo":"7v1vu1webk.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T10:41:37Z","receivedAt":"2009-02-14T10:41:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 11:35 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>>> + OPT_STRING(0, \"get-color\", &get_color_name, \"name\", \"find the color configured: [default]\"),\n>>\n>> get-color is used to get the defined color for a given slot. Please do not\n>> rename it to \"name\" (see the original).\n>>\n>>> + OPT_STRING(0, \"get-colorbool\", &get_colorbool_name, \"name\", \"find the color setting: [stdout-is-tty]\"),\n>>\n>> get-colorbool is used to get the boolean setting for a given configuration\n>> variable. Please do not rename it to \"name\" (see the original).\n>>\n>\n> By the way, I think it might be more appropriate if you categorize the\n> above two (especially the \"colorbool\" one) in the \"Types\" section, as that\n> really is what is happening. Instead of doing the usual \"print the value\n> as a string\", it does something magical.\n\nAll right.\n\nI haven't used most of the config options, so I had to learn and try\nthem; color/colorbool are the ones I still don't get.\n\n>> Overall, with the same number of lines, we lost a lot of error checking in\n>> exchange for an ability to say \"git config --remove-sec\" instead of \"git\n>> config --remove-section\", and \"git config --help\" may give an easier to\n>> read message.\n>\n> And I forgot to mention the \"option categorization\" merit.\n>\n>> Given that \"git config\" is primarily meant for script use, this particular\n>> round does not look like a good tradeoff to me.\n>\n> Don't take this too negatively. The tradeoff will improve if there aren't\n> these obvious bugs you can spot without even running the code.\n\nNo problem, I probably should have explained that this was RFC, I knew\nthere were issues in the code.\n\n-- \nFelipe Contreras\n"},{"id":"104627","messageId":"alpine.DEB.1.00.0902141230250.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"94a0d4530902140237o7d26ff4j1c7350d926d12c1a@mail.gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T11:40:18Z","receivedAt":"2009-02-14T11:40:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Felipe Contreras wrote:\n\n> Then why are you asking?\n\nOut of curiosity, I guess, as it would happen to answer my curiosity as \nwell.\n\n> This is more a \"I would like to increase the chances of my patches\n> being accepted so I'd do some chores to gain the trust of some\n> developers\", and Johannes Schindelin was pushing me to do this.\n\nHeh, I'll gladly take the blame for that!\n\nNote that in contrast to Junio, I think \"git config\" is a chimera between \nplumbing and porcelain, and would benefit tremendously from a nice help.\n\n> >> +static int type_int, type_bool, type_bool_or_int;\n> >\n> > You can have either (no type specified, int, bool, bool-or-int) at the\n> > end. Using three independent variables does not feel right.\n> >\n> > Hint: OPTION_SET_INT.\n> \n> That definitely makes things easier, it would have been nice to see an\n> example of this; I didn't knew it was there.\n> \n> The only problem is that --bool and --int would be possible in the\n> same command and there would be no way to output an error, but I guess\n> that's not a big problem.\n\nI think that is okay.\n\n> >> + else if (do_add) {\n> >> + if (argc > 2)\n> >> + die(\"Too many arguments.\");\n> >> + if (argc != 2)\n> >> + die(\"Need name value.\");\n> >> + value = normalize_value(argv[0], argv[1]);\n> >> + return git_config_set_multivar(argv[0], value, \"^$\", 0);\n> >\n> > This part did not lose argc error checking, but...\n> >\n> >> + }\n> >> + else if (do_replace_all) {\n> >> + value = normalize_value(argv[0], argv[1]);\n> >> + return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);\n> >\n> > You do not check argc here (nor in many \"else if\" below) to make sure you\n> > have sufficient number of arguments. \"git config --unset\" is now allowed\n> > to segfault, and \"git config --unset a b c d e f\" can silently ignore\n> > excess arguments for example?\n> \n> Yes the arguments check need to be revised.\n> \n> My hope was somebody would review this and suggest a clever and\n> generic way of doing this. Perhaps a util function check_min_args, or\n> maybe something in parseopt that receives the number of args?\n\nMaybe a helper, yes. Something like:\n\n\tstatic void check_argc(int argc, int min, int max) {\n\t\tif (argc >= min && argc <= max)\n\t\t\treturn;\n\t\tfprintf(stderr, \"Wrong number of arguments: %d\\n\", argc);\n\t\tusage_with_options(config_usage, config_options);\n\t}\n\nOf course, this assumes that config_usage and config_options are global...\n\n> Also, I think the code would be easier to maintain with parseopt.\n\nI agree.\n\nThanks,\nDscho\n"},{"id":"104631","messageId":"m3skmhteuk.fsf@localhost.localdomain","threadId":"17782","inReplyTo":"1234577142-22965-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-02-14T11:52:56Z","receivedAt":"2009-02-14T11:52:56Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Felipe Contreras writes:\n\n> Reorganizing the code to use parseopt as suggested by Johannes\n> Schindelin.\n\n[...]\n> -static const char git_config_set_usage[] =\n> -\"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 ]\";\n> +static const char *const builtin_config_usage[] = {\n> +\t\"git config [options]\",\n> +\tNULL\n> +};\n\nJust asking: why this change?\n\n> +\tOPT_BOOLEAN('z', \"null\", &end_null, \"end values with null character\"),\n\nTerminate and NUL.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"104632","messageId":"1234612989-32297-1-git-send-email-felipe.contreras@gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902141230250.10279@pacific.mpi-cbg.de","subject":"[PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T12:03:09Z","receivedAt":"2009-02-14T12:03:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Reorganizing the code to use parseopt as suggested by Johannes\nSchindelin.\n\nSigned-off-by: Felipe Contreras \n---\n builtin-config.c | 370 ++++++++++++++++++++++++++---------------------------\n 1 files changed, 182 insertions(+), 188 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex 6937eaf..aefec0d 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -1,9 +1,12 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n #include \"color.h\"\n+#include \"parse-options.h\"\n \n-static const char git_config_set_usage[] =\n-\"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 ]\";\n+static const char *const builtin_config_usage[] = {\n+\t\"git config [options]\",\n+\tNULL\n+};\n \n static char *key;\n static regex_t *key_regexp;\n@@ -18,6 +21,59 @@ static char key_delim = ' ';\n static char term = '\\n';\n static enum { T_RAW, T_INT, T_BOOL, T_BOOL_OR_INT } type = T_RAW;\n \n+static int use_global_config, use_system_config;\n+static const char *given_config_file;\n+static int actions;\n+static const char *get_color_slot, *get_colorbool_slot;\n+static int end_null;\n+\n+#define ACTION_GET (1<<0)\n+#define ACTION_GET_ALL (1<<1)\n+#define ACTION_GET_REGEXP (1<<2)\n+#define ACTION_REPLACE_ALL (1<<3)\n+#define ACTION_ADD (1<<4)\n+#define ACTION_UNSET (1<<5)\n+#define ACTION_UNSET_ALL (1<<6)\n+#define ACTION_RENAME_SECTION (1<<7)\n+#define ACTION_REMOVE_SECTION (1<<8)\n+#define ACTION_LIST (1<<9)\n+#define ACTION_EDIT (1<<10)\n+\n+static struct option builtin_config_options[] = {\n+\tOPT_GROUP(\"Config file location\"),\n+\tOPT_BOOLEAN(0, \"global\", &use_global_config, \"use global config file\"),\n+\tOPT_BOOLEAN(0, \"system\", &use_system_config, \"use system config file\"),\n+\tOPT_STRING('f', \"file\", &given_config_file, \"FILE\", \"use given config file\"),\n+\tOPT_GROUP(\"Action\"),\n+\tOPT_BIT(0, \"get\", &actions, \"get value: name [value-regex]\", ACTION_GET),\n+\tOPT_BIT(0, \"get-all\", &actions, \"get all values: key [value-regex]\", ACTION_GET),\n+\tOPT_BIT(0, \"get-regexp\", &actions, \"get values for regexp: name-regex [value-regex]\", ACTION_GET_REGEXP),\n+\tOPT_BIT(0, \"replace-all\", &actions, \"replace all matching variables: name [value [value_regex]\", ACTION_REPLACE_ALL),\n+\tOPT_BIT(0, \"add\", &actions, \"adds a new variable: name value\", ACTION_ADD),\n+\tOPT_BIT(0, \"unset\", &actions, \"removes a variable: name [value-regex]\", ACTION_UNSET),\n+\tOPT_BIT(0, \"unset-all\", &actions, \"removes all matches: name [value-regex]\", ACTION_UNSET_ALL),\n+\tOPT_BIT(0, \"rename-section\", &actions, \"rename section: old-name new-name\", ACTION_RENAME_SECTION),\n+\tOPT_BIT(0, \"remove-section\", &actions, \"remove a section: name\", ACTION_REMOVE_SECTION),\n+\tOPT_BIT('l', \"list\", &actions, \"list all\", ACTION_LIST),\n+\tOPT_BIT('e', \"edit\", &actions, \"opens an editor\", ACTION_EDIT),\n+\tOPT_GROUP(\"Type\"),\n+\tOPT_SET_INT(0, \"bool\", &type, \"value is \\\"true\\\" or \\\"false\\\"\", T_BOOL),\n+\tOPT_SET_INT(0, \"int\", &type, \"value is decimal number\", T_INT),\n+\tOPT_SET_INT(0, \"bool-or-int\", &type, NULL, T_BOOL_OR_INT),\n+\tOPT_STRING(0, \"get-color\", &get_color_slot, \"slot\", \"find the color configured: [default]\"),\n+\tOPT_STRING(0, \"get-colorbool\", &get_colorbool_slot, \"slot\", \"find the color setting: [stdout-is-tty]\"),\n+\tOPT_GROUP(\"Other\"),\n+\tOPT_BOOLEAN('z', \"null\", &end_null, \"terminate values with NUL byte\"),\n+\tOPT_END(),\n+};\n+\n+static void check_argc(int argc, int min, int max) {\n+\tif (argc >= min && argc <= max)\n+\t\treturn;\n+\terror(\"wrong number of arguments\");\n+\tusage_with_options(builtin_config_usage, builtin_config_options);\n+}\n+\n static int show_all_config(const char *key_, const char *value_, void *cb)\n {\n \tif (value_)\n@@ -179,7 +235,6 @@ static char *normalize_value(const char *key, const char *value)\n static int get_color_found;\n static const char *get_color_slot;\n static char parsed_color[COLOR_MAXLEN];\n-\n static int git_get_color_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, get_color_slot)) {\n@@ -191,29 +246,8 @@ static int git_get_color_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n-static int get_color(int argc, const char **argv)\n+static void get_color(const char *def_color)\n {\n-\t/*\n-\t * grab the color setting for the given slot from the configuration,\n-\t * or parse the default value if missing, and return ANSI color\n-\t * escape sequence.\n-\t *\n-\t * e.g.\n-\t * git config --get-color color.diff.whitespace \"blue reverse\"\n-\t */\n-\tconst char *def_color = NULL;\n-\n-\tswitch (argc) {\n-\tdefault:\n-\t\tusage(git_config_set_usage);\n-\tcase 2:\n-\t\tdef_color = argv[1];\n-\t\t/* fallthru */\n-\tcase 1:\n-\t\tget_color_slot = argv[0];\n-\t\tbreak;\n-\t}\n-\n \tget_color_found = 0;\n \tparsed_color[0] = '\\0';\n \tgit_config(git_get_color_config, NULL);\n@@ -222,7 +256,6 @@ static int get_color(int argc, const char **argv)\n \t\tcolor_parse(def_color, \"command line\", parsed_color);\n \n \tfputs(parsed_color, stdout);\n-\treturn 0;\n }\n \n static int stdout_is_tty;\n@@ -231,7 +264,7 @@ static int get_diff_color_found;\n static int git_get_colorbool_config(const char *var, const char *value,\n \t\tvoid *cb)\n {\n-\tif (!strcmp(var, get_color_slot)) {\n+\tif (!strcmp(var, get_colorbool_slot)) {\n \t\tget_colorbool_found =\n \t\t\tgit_config_colorbool(var, value, stdout_is_tty);\n \t}\n@@ -246,39 +279,24 @@ static int git_get_colorbool_config(const char *var, const char *value,\n \treturn 0;\n }\n \n-static int get_colorbool(int argc, const char **argv)\n+static int get_colorbool(int print)\n {\n-\t/*\n-\t * git config --get-colorbool []\n-\t *\n-\t * returns \"true\" or \"false\" depending on how \n-\t * is configured.\n-\t */\n-\n-\tif (argc == 2)\n-\t\tstdout_is_tty = git_config_bool(\"command line\", argv[1]);\n-\telse if (argc == 1)\n-\t\tstdout_is_tty = isatty(1);\n-\telse\n-\t\tusage(git_config_set_usage);\n \tget_colorbool_found = -1;\n \tget_diff_color_found = -1;\n-\tget_color_slot = argv[0];\n \tgit_config(git_get_colorbool_config, NULL);\n \n \tif (get_colorbool_found < 0) {\n-\t\tif (!strcmp(get_color_slot, \"color.diff\"))\n+\t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\n \t\t\tget_colorbool_found = get_diff_color_found;\n \t\tif (get_colorbool_found < 0)\n \t\t\tget_colorbool_found = git_use_color_default;\n \t}\n \n-\tif (argc == 1) {\n-\t\treturn get_colorbool_found ? 0 : 1;\n-\t} else {\n+\tif (print) {\n \t\tprintf(\"%s\\n\", get_colorbool_found ? \"true\" : \"false\");\n \t\treturn 0;\n-\t}\n+\t} else\n+\t\treturn get_colorbool_found ? 0 : 1;\n }\n \n int cmd_config(int argc, const char **argv, const char *prefix)\n@@ -289,151 +307,127 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \n \tconfig_exclusive_filename = getenv(CONFIG_ENVIRONMENT);\n \n-\twhile (1 < argc) {\n-\t\tif (!strcmp(argv[1], \"--int\"))\n-\t\t\ttype = T_INT;\n-\t\telse if (!strcmp(argv[1], \"--bool\"))\n-\t\t\ttype = T_BOOL;\n-\t\telse if (!strcmp(argv[1], \"--bool-or-int\"))\n-\t\t\ttype = T_BOOL_OR_INT;\n-\t\telse if (!strcmp(argv[1], \"--list\") || !strcmp(argv[1], \"-l\")) {\n-\t\t\tif (argc != 2)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tif (git_config(show_all_config, NULL) < 0 &&\n-\t\t\t\t\tfile && errno)\n-\t\t\t\tdie(\"unable to read config file %s: %s\", file,\n-\t\t\t\t strerror(errno));\n-\t\t\treturn 0;\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--global\")) {\n-\t\t\tchar *home = getenv(\"HOME\");\n-\t\t\tif (home) {\n-\t\t\t\tchar *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n-\t\t\t\tconfig_exclusive_filename = user_config;\n-\t\t\t} else {\n-\t\t\t\tdie(\"$HOME not set\");\n-\t\t\t}\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--system\"))\n-\t\t\tconfig_exclusive_filename = git_etc_gitconfig();\n-\t\telse if (!strcmp(argv[1], \"--file\") || !strcmp(argv[1], \"-f\")) {\n-\t\t\tif (argc < 3)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tif (!is_absolute_path(argv[2]) && file)\n-\t\t\t\tfile = prefix_filename(file, strlen(file),\n-\t\t\t\t\t\t argv[2]);\n-\t\t\telse\n-\t\t\t\tfile = argv[2];\n-\t\t\tconfig_exclusive_filename = file;\n-\t\t\targc--;\n-\t\t\targv++;\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--null\") || !strcmp(argv[1], \"-z\")) {\n-\t\t\tterm = '\\0';\n-\t\t\tdelim = '\\n';\n-\t\t\tkey_delim = '\\n';\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--rename-section\")) {\n-\t\t\tint ret;\n-\t\t\tif (argc != 4)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tret = git_config_rename_section(argv[2], argv[3]);\n-\t\t\tif (ret < 0)\n-\t\t\t\treturn ret;\n-\t\t\tif (ret == 0) {\n-\t\t\t\tfprintf(stderr, \"No such section!\\n\");\n-\t\t\t\treturn 1;\n-\t\t\t}\n-\t\t\treturn 0;\n-\t\t}\n-\t\telse if (!strcmp(argv[1], \"--remove-section\")) {\n-\t\t\tint ret;\n-\t\t\tif (argc != 3)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tret = git_config_rename_section(argv[2], NULL);\n-\t\t\tif (ret < 0)\n-\t\t\t\treturn ret;\n-\t\t\tif (ret == 0) {\n-\t\t\t\tfprintf(stderr, \"No such section!\\n\");\n-\t\t\t\treturn 1;\n-\t\t\t}\n-\t\t\treturn 0;\n-\t\t} else if (!strcmp(argv[1], \"--get-color\")) {\n-\t\t\treturn get_color(argc-2, argv+2);\n-\t\t} else if (!strcmp(argv[1], \"--get-colorbool\")) {\n-\t\t\treturn get_colorbool(argc-2, argv+2);\n-\t\t} else if (!strcmp(argv[1], \"--edit\") || !strcmp(argv[1], \"-e\")) {\n-\t\t\tconst char *config_filename;\n-\t\t\tif (argc != 2)\n-\t\t\t\tusage(git_config_set_usage);\n-\t\t\tif (config_exclusive_filename)\n-\t\t\t\tconfig_filename = config_exclusive_filename;\n-\t\t\telse\n-\t\t\t\tconfig_filename = git_path(\"config\");\n-\t\t\tgit_config(git_default_config, NULL);\n-\t\t\tlaunch_editor(config_filename, NULL, NULL);\n-\t\t\treturn 0;\n-\t\t} else\n-\t\t\tbreak;\n-\t\targc--;\n-\t\targv++;\n-\t}\n+\targc = parse_options(argc, argv, builtin_config_options, builtin_config_usage, 0);\n \n-\tswitch (argc) {\n-\tcase 2:\n-\t\treturn get_value(argv[1], NULL);\n-\tcase 3:\n-\t\tif (!strcmp(argv[1], \"--unset\"))\n-\t\t\treturn git_config_set(argv[2], NULL);\n-\t\telse if (!strcmp(argv[1], \"--unset-all\"))\n-\t\t\treturn git_config_set_multivar(argv[2], NULL, NULL, 1);\n-\t\telse if (!strcmp(argv[1], \"--get\"))\n-\t\t\treturn get_value(argv[2], NULL);\n-\t\telse if (!strcmp(argv[1], \"--get-all\")) {\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], NULL);\n-\t\t} else if (!strcmp(argv[1], \"--get-regexp\")) {\n-\t\t\tshow_keys = 1;\n-\t\t\tuse_key_regexp = 1;\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], NULL);\n+\tif (use_global_config) {\n+\t\tchar *home = getenv(\"HOME\");\n+\t\tif (home) {\n+\t\t\tchar *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n+\t\t\tconfig_exclusive_filename = user_config;\n \t\t} else {\n-\t\t\tvalue = normalize_value(argv[1], argv[2]);\n-\t\t\treturn git_config_set(argv[1], value);\n+\t\t\tdie(\"$HOME not set\");\n \t\t}\n-\tcase 4:\n-\t\tif (!strcmp(argv[1], \"--unset\"))\n-\t\t\treturn git_config_set_multivar(argv[2], NULL, argv[3], 0);\n-\t\telse if (!strcmp(argv[1], \"--unset-all\"))\n-\t\t\treturn git_config_set_multivar(argv[2], NULL, argv[3], 1);\n-\t\telse if (!strcmp(argv[1], \"--get\"))\n-\t\t\treturn get_value(argv[2], argv[3]);\n-\t\telse if (!strcmp(argv[1], \"--get-all\")) {\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], argv[3]);\n-\t\t} else if (!strcmp(argv[1], \"--get-regexp\")) {\n-\t\t\tshow_keys = 1;\n-\t\t\tuse_key_regexp = 1;\n-\t\t\tdo_all = 1;\n-\t\t\treturn get_value(argv[2], argv[3]);\n-\t\t} else if (!strcmp(argv[1], \"--add\")) {\n-\t\t\tvalue = normalize_value(argv[2], argv[3]);\n-\t\t\treturn git_config_set_multivar(argv[2], value, \"^$\", 0);\n-\t\t} else if (!strcmp(argv[1], \"--replace-all\")) {\n-\t\t\tvalue = normalize_value(argv[2], argv[3]);\n-\t\t\treturn git_config_set_multivar(argv[2], value, NULL, 1);\n-\t\t} else {\n-\t\t\tvalue = normalize_value(argv[1], argv[2]);\n-\t\t\treturn git_config_set_multivar(argv[1], value, argv[3], 0);\n-\t\t}\n-\tcase 5:\n-\t\tif (!strcmp(argv[1], \"--replace-all\")) {\n-\t\t\tvalue = normalize_value(argv[2], argv[3]);\n-\t\t\treturn git_config_set_multivar(argv[2], value, argv[4], 1);\n+\t}\n+\telse if (use_system_config)\n+\t\tconfig_exclusive_filename = git_etc_gitconfig();\n+\telse if (given_config_file) {\n+\t\tif (!is_absolute_path(given_config_file) && file)\n+\t\t\tfile = prefix_filename(file, strlen(file),\n+\t\t\t\t\t given_config_file);\n+\t\telse\n+\t\t\tfile = given_config_file;\n+\t\tconfig_exclusive_filename = file;\n+\t}\n+\n+\tif (end_null) {\n+\t\tterm = '\\0';\n+\t\tdelim = '\\n';\n+\t\tkey_delim = '\\n';\n+\t}\n+\n+\tif (HAS_MULTI_BITS(actions)) {\n+\t\terror(\"only one action at a time.\");\n+\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t}\n+\tif (actions == 0)\n+\t\tswitch (argc) {\n+\t\tcase 1: actions |= ACTION_GET; break;\n+\t\tcase 2: actions |= ACTION_ADD; break;\n+\t\tcase 3: actions |= ACTION_REPLACE_ALL; break;\n+\t\tdefault:\n+\t\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t\t}\n-\tcase 1:\n-\tdefault:\n-\t\tusage(git_config_set_usage);\n+\n+\tif (actions & ACTION_LIST) {\n+\t\tif (git_config(show_all_config, NULL) < 0 &&\n+\t\t file && errno)\n+\t\t\tdie(\"unable to read config file %s: %s\", file,\n+\t\t\t strerror(errno));\n+\t}\n+\telse if (actions & ACTION_EDIT) {\n+\t\tconst char *config_filename;\n+\t\tif (config_exclusive_filename)\n+\t\t\tconfig_filename = config_exclusive_filename;\n+\t\telse\n+\t\t\tconfig_filename = git_path(\"config\");\n+\t\tgit_config(git_default_config, NULL);\n+\t\tlaunch_editor(config_filename, NULL, NULL);\n+\t}\n+\telse if (actions & ACTION_ADD) {\n+\t\tcheck_argc(argc, 2, 2);\n+\t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\treturn git_config_set_multivar(argv[0], value, \"^$\", 0);\n+\t}\n+\telse if (actions & ACTION_REPLACE_ALL) {\n+\t\tcheck_argc(argc, 2, 3);\n+\t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\treturn git_config_set_multivar(argv[0], value, argv[2], 1);\n \t}\n+\telse if (actions & ACTION_GET) {\n+\t\tcheck_argc(argc, 1, 2);\n+\t\treturn get_value(argv[0], argv[1]);\n+\t}\n+\telse if (actions & ACTION_GET_ALL) {\n+\t\tdo_all = 1;\n+\t\tcheck_argc(argc, 1, 2);\n+\t\treturn get_value(argv[0], argv[1]);\n+\t}\n+\telse if (actions & ACTION_GET_REGEXP) {\n+\t\tshow_keys = 1;\n+\t\tuse_key_regexp = 1;\n+\t\tdo_all = 1;\n+\t\tcheck_argc(argc, 1, 2);\n+\t\treturn get_value(argv[0], argv[1]);\n+\t}\n+\telse if (actions & ACTION_UNSET) {\n+\t\tcheck_argc(argc, 1, 2);\n+\t\tif (argc == 2)\n+\t\t\treturn git_config_set_multivar(argv[0], NULL, argv[1], 0);\n+\t\telse\n+\t\t\treturn git_config_set(argv[0], NULL);\n+\t}\n+\telse if (actions & ACTION_UNSET_ALL) {\n+\t\tcheck_argc(argc, 1, 2);\n+\t\treturn git_config_set_multivar(argv[0], NULL, argv[1], 1);\n+\t}\n+\telse if (actions & ACTION_RENAME_SECTION) {\n+\t\tint ret;\n+\t\tcheck_argc(argc, 2, 2);\n+\t\tret = git_config_rename_section(argv[0], argv[1]);\n+\t\tif (ret < 0)\n+\t\t\treturn ret;\n+\t\tif (ret == 0)\n+\t\t\tdie(\"No such section!\");\n+\t}\n+\telse if (actions & ACTION_REMOVE_SECTION) {\n+\t\tint ret;\n+\t\tcheck_argc(argc, 1, 1);\n+\t\tret = git_config_rename_section(argv[0], NULL);\n+\t\tif (ret < 0)\n+\t\t\treturn ret;\n+\t\tif (ret == 0)\n+\t\t\tdie(\"No such section!\");\n+\t}\n+\telse if (get_color_slot) {\n+\t\tget_color(argv[0]);\n+\t}\n+\telse if (get_colorbool_slot) {\n+\t\tif (argc == 1)\n+\t\t\tstdout_is_tty = git_config_bool(\"command line\", argv[0]);\n+\t\telse if (argc == 0)\n+\t\t\tstdout_is_tty = isatty(1);\n+\t\treturn get_colorbool(argc != 1);\n+\t}\n+\n \treturn 0;\n }\n-- \n1.6.1.3\n"},{"id":"104633","messageId":"94a0d4530902140406l5ccd79b6x964ad53b83079799@mail.gmail.com","threadId":"17782","inReplyTo":"m3skmhteuk.fsf@localhost.localdomain","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T12:06:41Z","receivedAt":"2009-02-14T12:06:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 1:52 PM, Jakub Narebski wrote:\n> Felipe Contreras writes:\n>\n>> Reorganizing the code to use parseopt as suggested by Johannes\n>> Schindelin.\n>\n> [...]\n>> -static const char git_config_set_usage[] =\n>> -\"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 ]\";\n>> +static const char *const builtin_config_usage[] = {\n>> + \"git config [options]\",\n>> + NULL\n>> +};\n>\n> Just asking: why this change?\n\nSo it's easier to understand and maintain?\n\n>> + OPT_BOOLEAN('z', \"null\", &end_null, \"end values with null character\"),\n>\n> Terminate and NUL.\n\nOk. Junio already suggested that.\n\n-- \nFelipe Contreras\n"},{"id":"104635","messageId":"94a0d4530902140415j4168d09dh8abac0d6eba0b8cf@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902141230250.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T12:15:37Z","receivedAt":"2009-02-14T12:15:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 1:40 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>\n>> Then why are you asking?\n>\n> Out of curiosity, I guess, as it would happen to answer my curiosity as\n> well.\n>\n>> This is more a \"I would like to increase the chances of my patches\n>> being accepted so I'd do some chores to gain the trust of some\n>> developers\", and Johannes Schindelin was pushing me to do this.\n>\n> Heh, I'll gladly take the blame for that!\n>\n> Note that in contrast to Junio, I think \"git config\" is a chimera between\n> plumbing and porcelain, and would benefit tremendously from a nice help.\n\nI agree on that.\n\n>> >> +static int type_int, type_bool, type_bool_or_int;\n>> >\n>> > You can have either (no type specified, int, bool, bool-or-int) at the\n>> > end. Using three independent variables does not feel right.\n>> >\n>> > Hint: OPTION_SET_INT.\n>>\n>> That definitely makes things easier, it would have been nice to see an\n>> example of this; I didn't knew it was there.\n>>\n>> The only problem is that --bool and --int would be possible in the\n>> same command and there would be no way to output an error, but I guess\n>> that's not a big problem.\n>\n> I think that is okay.\n>\n>> >> + else if (do_add) {\n>> >> + if (argc > 2)\n>> >> + die(\"Too many arguments.\");\n>> >> + if (argc != 2)\n>> >> + die(\"Need name value.\");\n>> >> + value = normalize_value(argv[0], argv[1]);\n>> >> + return git_config_set_multivar(argv[0], value, \"^$\", 0);\n>> >\n>> > This part did not lose argc error checking, but...\n>> >\n>> >> + }\n>> >> + else if (do_replace_all) {\n>> >> + value = normalize_value(argv[0], argv[1]);\n>> >> + return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);\n>> >\n>> > You do not check argc here (nor in many \"else if\" below) to make sure you\n>> > have sufficient number of arguments. \"git config --unset\" is now allowed\n>> > to segfault, and \"git config --unset a b c d e f\" can silently ignore\n>> > excess arguments for example?\n>>\n>> Yes the arguments check need to be revised.\n>>\n>> My hope was somebody would review this and suggest a clever and\n>> generic way of doing this. Perhaps a util function check_min_args, or\n>> maybe something in parseopt that receives the number of args?\n>\n> Maybe a helper, yes. Something like:\n>\n> static void check_argc(int argc, int min, int max) {\n> if (argc >= min && argc <= max)\n> return;\n> fprintf(stderr, \"Wrong number of arguments: %d\\n\", argc);\n> usage_with_options(config_usage, config_options);\n> }\n>\n> Of course, this assumes that config_usage and config_options are global...\n\nCool.\n\nI've sent a new patch with this helper (a bit modified), and all the\nchanges Junio suggested.\n\nI still have a few doubts:\n\n1) --list when no config file is given uses all the config files,\nwouldn't it make sense to have a --repo option?\n\n2) --get-colorbool prints \"true\" or \"false\" only when there are two\narguments, is that correct or should stdout_is_tty be used instead?\n\n3) should the documentation be updated for the --get-color* options to\nuse 'slot' instead of 'name'?\n\n-- \nFelipe Contreras\n"},{"id":"104639","messageId":"20090214151725.GB3887@sigill.intra.peff.net","threadId":"17782","inReplyTo":"m3skmhteuk.fsf@localhost.localdomain","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-14T15:17:25Z","receivedAt":"2009-02-14T15:17:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 14, 2009 at 03:52:56AM -0800, Jakub Narebski wrote:\n\n> > -static const char git_config_set_usage[] =\n> > -\"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 ]\";\n> > +static const char *const builtin_config_usage[] = {\n> > +\t\"git config [options]\",\n> > +\tNULL\n> > +};\n> \n> Just asking: why this change?\n\nIf you are asking about this specific hunk, it is because parse-options\nwill generate the list of options.\n\n-Peff\n"},{"id":"104641","messageId":"20090214152155.GD3887@sigill.intra.peff.net","threadId":"17782","inReplyTo":"1234612989-32297-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-14T15:21:55Z","receivedAt":"2009-02-14T15:21:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 14, 2009 at 02:03:09PM +0200, Felipe Contreras wrote:\n\n> Reorganizing the code to use parseopt as suggested by Johannes\n> Schindelin.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> builtin-config.c | 370 ++++++++++++++++++++++++++---------------------------\n> 1 files changed, 182 insertions(+), 188 deletions(-)\n\nWhat is this built on top of? I get very large conflicts applying it on\ntop of Junio's \"master\".\n\n-Peff\n"},{"id":"104642","messageId":"94a0d4530902140724m72fdb47bt7fead5dc22733299@mail.gmail.com","threadId":"17782","inReplyTo":"20090214152155.GD3887@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T15:24:47Z","receivedAt":"2009-02-14T15:24:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 5:21 PM, Jeff King wrote:\n> On Sat, Feb 14, 2009 at 02:03:09PM +0200, Felipe Contreras wrote:\n>\n>> Reorganizing the code to use parseopt as suggested by Johannes\n>> Schindelin.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> builtin-config.c | 370 ++++++++++++++++++++++++++---------------------------\n>> 1 files changed, 182 insertions(+), 188 deletions(-)\n>\n> What is this built on top of? I get very large conflicts applying it on\n> top of Junio's \"master\".\n\nBy mistake I used a dirty master branch, but it should apply on top of 'pu'.\n\n-- \nFelipe Contreras\n"},{"id":"104660","messageId":"alpine.DEB.1.00.0902142003300.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"94a0d4530902140415j4168d09dh8abac0d6eba0b8cf@mail.gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T19:11:02Z","receivedAt":"2009-02-14T19:11:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Felipe Contreras wrote:\n\n> 1) --list when no config file is given uses all the config files,\n> wouldn't it make sense to have a --repo option?\n\nThe idea of --list is not \"cat .git/config\". The idea is to help users or \nscripts to list the current settings (_including_ the global settings).\n\nYou can force showing the repo-specific config with \"git --file \n.git/config\", though.\n\nCiao,\nDscho\n"},{"id":"104662","messageId":"94a0d4530902141114s7352841cmf5c4259211a793e3@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142003300.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T19:14:10Z","receivedAt":"2009-02-14T19:14:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>\n>> 1) --list when no config file is given uses all the config files,\n>> wouldn't it make sense to have a --repo option?\n>\n> The idea of --list is not \"cat .git/config\". The idea is to help users or\n> scripts to list the current settings (_including_ the global settings).\n>\n> You can force showing the repo-specific config with \"git --file\n> .git/config\", though.\n\nWhen you are on the root directory of the repo, and you don't have\nGIT_DIR, or --git-dir.\n\n-- \nFelipe Contreras\n"},{"id":"104667","messageId":"alpine.DEB.1.00.0902142022340.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"94a0d4530902141114s7352841cmf5c4259211a793e3@mail.gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T19:24:19Z","receivedAt":"2009-02-14T19:24:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Felipe Contreras wrote:\n\n> On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin\n> wrote:\n> > Hi,\n> >\n> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> >\n> >> 1) --list when no config file is given uses all the config files,\n> >> wouldn't it make sense to have a --repo option?\n> >\n> > The idea of --list is not \"cat .git/config\". The idea is to help users or\n> > scripts to list the current settings (_including_ the global settings).\n> >\n> > You can force showing the repo-specific config with \"git --file\n> > .git/config\", though.\n> \n> When you are on the root directory of the repo, and you don't have\n> GIT_DIR, or --git-dir.\n\nWhen I wrote my response, I briefly considered if I had to be verbose, and \ndecided against it.\n\nBut this is what I should have written:\n\n\tgit --file $(git rev-parse --git-dir)/config --list\n\nCiao,\nDscho\n"},{"id":"104668","messageId":"alpine.DEB.1.00.0902142025420.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142022340.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T19:26:42Z","receivedAt":"2009-02-14T19:26:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Johannes Schindelin wrote:\n\n> On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> \n> > On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin\n> > wrote:\n> > > Hi,\n> > >\n> > > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> > >\n> > >> 1) --list when no config file is given uses all the config files,\n> > >> wouldn't it make sense to have a --repo option?\n> > >\n> > > The idea of --list is not \"cat .git/config\". The idea is to help users or\n> > > scripts to list the current settings (_including_ the global settings).\n> > >\n> > > You can force showing the repo-specific config with \"git --file\n> > > .git/config\", though.\n> > \n> > When you are on the root directory of the repo, and you don't have\n> > GIT_DIR, or --git-dir.\n> \n> When I wrote my response, I briefly considered if I had to be verbose, and \n> decided against it.\n> \n> But this is what I should have written:\n> \n> \tgit --file $(git rev-parse --git-dir)/config --list\n\nOkay, before anybody points out that I was not verbose enough -- again -- \nthis is what I really should have written:\n\n\tgit --file \"$(git rev-parse --git-dir)\"/config --list\n\nCiao,\nDscho\n"},{"id":"104671","messageId":"7vhc2wu8a0.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"94a0d4530902140237o7d26ff4j1c7350d926d12c1a@mail.gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-14T19:29:27Z","receivedAt":"2009-02-14T19:29:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\n>>> Reorganizing the code to use parseopt as suggested by Johannes\n>>> Schindelin.\n>>>\n>>> Signed-off-by: Felipe Contreras \n>>> ---\n>>> builtin-config.c | 422 +++++++++++++++++++++++++++---------------------------\n>>> 1 files changed, 210 insertions(+), 212 deletions(-)\n>>\n>> Whew. That's a lot of changes.\n>>\n>> Is this just \"I am using parseopt because I can\", or \"I want to do this\n>> first because I am planning to do such and such things to this program,\n>> and the current mess needs to be cleaned up first before doing so\"?\n>>\n>> I am asking this _not_ because I'd want to reject the patch if the answer\n>> is former.\n>\n> Then why are you asking?\n\nSo that I can prioritize topics and patches that are more important in the\nshort, medium and longer time according to the urgency. For example, it\nis a very bad tradeoff for me to take \"This does not change anything\nexternally visible, but cleans up the implementation\" near or during the\nrelease freeze. \"This cleans up, and then the next one will fix a\nlongstanding bug building on top of the cleaner code\" is different.\n\nAnd preferably the latter kind should be queued for 'maint', not for\n'master', if the fix that will come later is important enough.\n\n> This is more a \"I would like to increase the chances of my patches\n> being accepted so I'd do some chores to gain the trust of some\n> developers\", and Johannes Schindelin was pushing me to do this.\n>\n> Also it's a bit of \"I would like to improve git and learn the API\n> while doing so\".\n\nI personally do not think \"I rewrote this command's option parser using\nparseopt\" earns any \"trust point\". I think the latter is a *great* thing\nto do, though.\n\n> The only problem is that --bool and --int would be possible in the\n> same command and there would be no way to output an error, but I guess\n> that's not a big problem.\n\nThe existing code does not have a check, and you are right, it would be\nnice to have such an error check, perhaps as a separate follow-up patch.\n\n>>> +static const char *get_color_name, *get_colorbool_name;\n>>> +static int end_null;\n>>> +\n>>> +static struct option builtin_config_options[] = {\n>>> + OPT_GROUP(\"Config file location\"),\n>>> + OPT_BOOLEAN(0, \"global\", &use_global_config, \"use global config file\"),\n>>> + OPT_BOOLEAN(0, \"system\", &use_system_config, \"use system config file\"),\n>>> + OPT_STRING('f', \"file\", &given_config_file, \"FILE\", \"use given config file\"),\n>>\n>> Why CAPS?\n>\n> I looked at some other code, like builtin-commit.c and that's what is\n> used there.\n\nI see, there are mixtures; apply, blame, fmt-merge-msg and tag say \"file\"\nand commit and fast-export say \"FILE\". commit's one could be almost\njustifyable, because it uses caps in all (except for -u=), but\noutput from fast-export is a different story. Compare \"git commit -h\" and\n\"git fast-export -h\" to see what I mean.\n\nOne possible patch could make \"git cmd -h\" messages for all \"cmd\" spell\ntheir \"fill your value here\" markings consistently, and the explanation of\nthe change in the proposed commit log message would have\n\n (1) an analysis similar to the one I did in the above paragraph (but\n doing it more thouroughly; I didn't look beyond commands that grep\n for \"file\" or \"FILE\" hits in the sources) of the current situation;\n and\n\n (2) the reason why you picked the case (be it lower or upper, I do not\n deeply care either way).\n\nSuch a patch could earn a \"trust point\", not because such it is extremely\nimportant for the system's usability (it is *not*), but because it is a\ngood way to demonstrate that you can do a thorough job and think things\nthrough.\n\n>> Hmph. I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.\n>\n> Yeap, definitely, thanks for the suggestion.\n>\n> Again, it would have been nice to see any example of this.\n\nI actually had to look for HAS_MULTI_BITS because I *knew* such a facility\nexisted, I did not recall what it was called, and api-parse-options.txt in\nDocumentation/technical did not talk about it. We can definitely improve\nthings in many places.\n\nMistakes made in the past and resulting flaws that remain in the current\nsource do not justify a new patch adding more mistakes to the codebase.\nReviewers help the author from adding more.\n\nOne bad thing about the current process (and I am certainly one of the\nguilty parties because I do a lot of reviews) around this area is that a\nreview comment that points out a mistake similar to the ones made in the\npast sound to the author of the patch as if the reviewer is telling that\nthe patch will not be accepted unless the existing mistakes are also fixed\nby the patch author. Such a review certainly does not mean that --- it is\nmore likely that the patch author simply mimicked existing code that is\ndoing a wrong thing without realizing it being wrong, and because the\nreviewer *did not know* that the mistake was copied from elsewhere, he\njust pointed it out the one in the patch.\n\nIn such a situation, I've seen two kinds of responses from the patch\nauthor (this assumes that the nit pointed out by the reviewer was indeed a\ngood thing to fix):\n\n * This and that code already do things in a similar way. I won't be\n changing my patch.\n\n * Ok, I'll fix the issue in my patch, but this and that code have the\n same mistake. Should I fix them in the same patch as well?\n\nNeither is an exactly optimal solution. It is very likely that the patch\nauthor knows more of the existing instances of the same mistake than the\nreviewer, but there probably are more instances than he tried to defend\nhis patch with. The places he knows of were found merely because he was\nlooking for an example to do something similar to what he wanted to do,\nnot because he was looking for all instances of a specific class of\nmistake to fix in the existing code.\n\nAn ideal response would be a follow up patch that fixes the issue that the\noriginal patch had, and a separate message that summarizes such issues in\nthe existing codebase (saying that the instances listed are not\nexhaustive). The patch author can attack them in a separate series if he\nwants to, and that would certainly be appreciated, but he does not have\nto. It is a separate issue that others (not necessarily the reviewer who\nfirst pointed out the mistake in the patch) can attack.\n\nUnfortunately, not many patch authors write such a summary. Sometimes we\nsee summaries on things that were discussed but nobody has followed\nthrough posted by third parties (including myself), but we do not seem to\nhave enough helpers to do that either. This does not take much technical\nskills but is a good \"trust point\" earner.\n\n> Yes the arguments check need to be revised.\n>\n> My hope was somebody would review this and suggest a clever and\n> generic way of doing this. Perhaps a util function check_min_args, or\n> maybe something in parseopt that receives the number of args?\n\nI thought what the original code before your patch had was quite\nreasonable, switching on argc.\n\n> Anyway, I can create the helper functions again, and maybe have some\n> function parameters instead of passing argc and argv\n\nIt might actually be a good idea to turn the cascade of if..elseif..fi\ninto a form of look-up in a table whose elements have a pointer to a\nfunction. A loooong function like this cmd_config() is very hard to\nfollow its logic.\n\n>> Overall, with the same number of lines, we lost a lot of error checking in\n>> exchange for an ability to say \"git config --remove-sec\" instead of \"git\n>> config --remove-section\", and \"git config --help\" may give an easier to\n>> read message.\n>>\n>> Given that \"git config\" is primarily meant for script use, this particular\n>> round does not look like a good tradeoff to me.\n>\n> That doesn't mean it shouldn't have a nice UI.\n\nThe need for \"UI\" is much less than that of Porcelain for this particular\ncommand, so it certainly affects the tradeoff points.\n\n> Also, I think the code would be easier to maintain with parseopt.\n\nNot with the patch you posted, but yes, the current mess could be\nimproved, and parseopt could be one of the tools for such code\nrestructuring.\n"},{"id":"104679","messageId":"alpine.DEB.1.00.0902142041370.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"1234612989-32297-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T19:59:36Z","receivedAt":"2009-02-14T19:59:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Felipe Contreras wrote:\n\n> @@ -231,7 +264,7 @@ static int get_diff_color_found;\n> static int git_get_colorbool_config(const char *var, const char *value,\n> \t\tvoid *cb)\n> {\n> -\tif (!strcmp(var, get_color_slot)) {\n> +\tif (!strcmp(var, get_colorbool_slot)) {\n> \t\tget_colorbool_found =\n> \t\t\tgit_config_colorbool(var, value, stdout_is_tty);\n> \t}\n\nName changes like this make it harder to read the patch; can you separate \nthat change out into its own patch?\n\n> +\tif (use_global_config) {\n> +\t\tchar *home = getenv(\"HOME\");\n> +\t\tif (home) {\n> +\t\t\tchar *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n> +\t\t\tconfig_exclusive_filename = user_config;\n\nIn a subsequent patch, you might add a check only one of --global, \n--system or --file was given.\n\n> +\telse if (given_config_file) {\n> +\t\tif (!is_absolute_path(given_config_file) && file)\n> +\t\t\tfile = prefix_filename(file, strlen(file),\n> +\t\t\t\t\t given_config_file);\n> +\t\telse\n> +\t\t\tfile = given_config_file;\n> +\t\tconfig_exclusive_filename = file;\n\nIt took me a considerable amount of time to figure out that \"file\" is \nactually the \"prefix\"! That cleanup would be nice to have before the \nparseopt patch, methinks, especially since the code is reindented, and \nthus hard to follow in the diff.\n\n> +\tif (actions & ACTION_LIST) {\n> +\t\tif (git_config(show_all_config, NULL) < 0 &&\n> +\t\t file && errno)\n\nShould this not be config_exclusive_filename?\n\n> +\t\t\tdie(\"unable to read config file %s: %s\", file,\n> +\t\t\t strerror(errno));\n\nDo we really only want to die() in case we know the file name? AFAICT at \nthis point we have no idea in which of the possibly three files the error \noccurred. And there need not be any errno set, for example when there was \na parse error.\n\n> +\telse if (actions & ACTION_EDIT) {\n> +\t\tconst char *config_filename;\n> +\t\tif (config_exclusive_filename)\n> +\t\t\tconfig_filename = config_exclusive_filename;\n> +\t\telse\n> +\t\t\tconfig_filename = git_path(\"config\");\n\nWhy not reuse config_exclusive_filename here?\n\n> +\telse if (actions & ACTION_ADD) {\n> +\t\tcheck_argc(argc, 2, 2);\n\nBTW what about check_argc() in the previous two cases?\n\n> +\t\treturn git_config_set_multivar(argv[0], value, \"^$\", 0);\n\nNow that I see this, there is another idea for a possible cleanup after \nthe parseoptification: cmd_config() should not return -1, as that will be \nturned into the exit status. So maybe prefix the return value with \"!!\"?\n\nOr maybe even better: set a variable \"ret\" and at the end of cmd_config(), \n\"return !!ret;\"?\n\nThe rest looks good to me.\n \nThanks,\nDscho\n"},{"id":"104681","messageId":"94a0d4530902141209j7a3a9976l80355bee526852ed@mail.gmail.com","threadId":"17782","inReplyTo":"7vhc2wu8a0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T20:09:59Z","receivedAt":"2009-02-14T20:09:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 9:29 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano wrote:\n>>> Felipe Contreras writes:\n>>>\n>>>> Reorganizing the code to use parseopt as suggested by Johannes\n>>>> Schindelin.\n>>>>\n>>>> Signed-off-by: Felipe Contreras \n>>>> ---\n>>>> builtin-config.c | 422 +++++++++++++++++++++++++++---------------------------\n>>>> 1 files changed, 210 insertions(+), 212 deletions(-)\n>>>\n>>> Whew. That's a lot of changes.\n>>>\n>>> Is this just \"I am using parseopt because I can\", or \"I want to do this\n>>> first because I am planning to do such and such things to this program,\n>>> and the current mess needs to be cleaned up first before doing so\"?\n>>>\n>>> I am asking this _not_ because I'd want to reject the patch if the answer\n>>> is former.\n>>\n>> Then why are you asking?\n>\n> So that I can prioritize topics and patches that are more important in the\n> short, medium and longer time according to the urgency. For example, it\n> is a very bad tradeoff for me to take \"This does not change anything\n> externally visible, but cleans up the implementation\" near or during the\n> release freeze. \"This cleans up, and then the next one will fix a\n> longstanding bug building on top of the cleaner code\" is different.\n>\n> And preferably the latter kind should be queued for 'maint', not for\n> 'master', if the fix that will come later is important enough.\n\nOh, in that case, no, I don't have any further patches on top of this.\n\n>> This is more a \"I would like to increase the chances of my patches\n>> being accepted so I'd do some chores to gain the trust of some\n>> developers\", and Johannes Schindelin was pushing me to do this.\n>>\n>> Also it's a bit of \"I would like to improve git and learn the API\n>> while doing so\".\n>\n> I personally do not think \"I rewrote this command's option parser using\n> parseopt\" earns any \"trust point\". I think the latter is a *great* thing\n> to do, though.\n\nI disagree. Making a patch pass through all the filters must mean\nsomething, and the more patches the more trust.\n\n>> The only problem is that --bool and --int would be possible in the\n>> same command and there would be no way to output an error, but I guess\n>> that's not a big problem.\n>\n> The existing code does not have a check, and you are right, it would be\n> nice to have such an error check, perhaps as a separate follow-up patch.\n\nYeap.\n\n>>>> +static const char *get_color_name, *get_colorbool_name;\n>>>> +static int end_null;\n>>>> +\n>>>> +static struct option builtin_config_options[] = {\n>>>> + OPT_GROUP(\"Config file location\"),\n>>>> + OPT_BOOLEAN(0, \"global\", &use_global_config, \"use global config file\"),\n>>>> + OPT_BOOLEAN(0, \"system\", &use_system_config, \"use system config file\"),\n>>>> + OPT_STRING('f', \"file\", &given_config_file, \"FILE\", \"use given config file\"),\n>>>\n>>> Why CAPS?\n>>\n>> I looked at some other code, like builtin-commit.c and that's what is\n>> used there.\n>\n> I see, there are mixtures; apply, blame, fmt-merge-msg and tag say \"file\"\n> and commit and fast-export say \"FILE\". commit's one could be almost\n> justifyable, because it uses caps in all (except for -u=), but\n> output from fast-export is a different story. Compare \"git commit -h\" and\n> \"git fast-export -h\" to see what I mean.\n>\n> One possible patch could make \"git cmd -h\" messages for all \"cmd\" spell\n> their \"fill your value here\" markings consistently, and the explanation of\n> the change in the proposed commit log message would have\n>\n> (1) an analysis similar to the one I did in the above paragraph (but\n> doing it more thouroughly; I didn't look beyond commands that grep\n> for \"file\" or \"FILE\" hits in the sources) of the current situation;\n> and\n>\n> (2) the reason why you picked the case (be it lower or upper, I do not\n> deeply care either way).\n>\n> Such a patch could earn a \"trust point\", not because such it is extremely\n> important for the system's usability (it is *not*), but because it is a\n> good way to demonstrate that you can do a thorough job and think things\n> through.\n\nI might do that.\n\n>>> Hmph. I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.\n>>\n>> Yeap, definitely, thanks for the suggestion.\n>>\n>> Again, it would have been nice to see any example of this.\n>\n> I actually had to look for HAS_MULTI_BITS because I *knew* such a facility\n> existed, I did not recall what it was called, and api-parse-options.txt in\n> Documentation/technical did not talk about it. We can definitely improve\n> things in many places.\n>\n> Mistakes made in the past and resulting flaws that remain in the current\n> source do not justify a new patch adding more mistakes to the codebase.\n> Reviewers help the author from adding more.\n>\n> One bad thing about the current process (and I am certainly one of the\n> guilty parties because I do a lot of reviews) around this area is that a\n> review comment that points out a mistake similar to the ones made in the\n> past sound to the author of the patch as if the reviewer is telling that\n> the patch will not be accepted unless the existing mistakes are also fixed\n> by the patch author. Such a review certainly does not mean that --- it is\n> more likely that the patch author simply mimicked existing code that is\n> doing a wrong thing without realizing it being wrong, and because the\n> reviewer *did not know* that the mistake was copied from elsewhere, he\n> just pointed it out the one in the patch.\n>\n> In such a situation, I've seen two kinds of responses from the patch\n> author (this assumes that the nit pointed out by the reviewer was indeed a\n> good thing to fix):\n>\n> * This and that code already do things in a similar way. I won't be\n> changing my patch.\n>\n> * Ok, I'll fix the issue in my patch, but this and that code have the\n> same mistake. Should I fix them in the same patch as well?\n>\n> Neither is an exactly optimal solution. It is very likely that the patch\n> author knows more of the existing instances of the same mistake than the\n> reviewer, but there probably are more instances than he tried to defend\n> his patch with. The places he knows of were found merely because he was\n> looking for an example to do something similar to what he wanted to do,\n> not because he was looking for all instances of a specific class of\n> mistake to fix in the existing code.\n>\n> An ideal response would be a follow up patch that fixes the issue that the\n> original patch had, and a separate message that summarizes such issues in\n> the existing codebase (saying that the instances listed are not\n> exhaustive). The patch author can attack them in a separate series if he\n> wants to, and that would certainly be appreciated, but he does not have\n> to. It is a separate issue that others (not necessarily the reviewer who\n> first pointed out the mistake in the patch) can attack.\n>\n> Unfortunately, not many patch authors write such a summary. Sometimes we\n> see summaries on things that were discussed but nobody has followed\n> through posted by third parties (including myself), but we do not seem to\n> have enough helpers to do that either. This does not take much technical\n> skills but is a good \"trust point\" earner.\n\nFor me it's easier, and more fun to write a separate patch that fixes\nthe issues than writing a summary, but in the mean time it makes sense\nto do one patch at a time and make sure it really gets merged,\nspecially since I don't have many, and it's motivating to know it's\ngoing to be on the next release.\n\nBut, if there's code that already has the same issues the patch has,\nit doesn't look like a good argument for patch rejection. Perhaps the\nquality standards increased, but on the other hand things wouldn't get\nworst by applying the patch.\n\n>> Yes the arguments check need to be revised.\n>>\n>> My hope was somebody would review this and suggest a clever and\n>> generic way of doing this. Perhaps a util function check_min_args, or\n>> maybe something in parseopt that receives the number of args?\n>\n> I thought what the original code before your patch had was quite\n> reasonable, switching on argc.\n\nThe code for --replace-all was repeated on 3 places, one for each case\nof args provided, and similarly for other options. Don't you think\nit's better to have the code in one single place like in my patch?\n\nAlso the git_config_set_usage variable was getting quite bug, which\nwas the reason Johannes suggested this change. Which probably lead to\nsome easy mistakes like the fact that -l is not mentioned in the\nusage.\n\n>> Anyway, I can create the helper functions again, and maybe have some\n>> function parameters instead of passing argc and argv\n>\n> It might actually be a good idea to turn the cascade of if..elseif..fi\n> into a form of look-up in a table whose elements have a pointer to a\n> function. A loooong function like this cmd_config() is very hard to\n> follow its logic.\n\nPerhaps, but at least it's not bigger than the previous one.\n\n\n\n-- \nFelipe Contreras\n"},{"id":"104682","messageId":"7viqncsreo.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142041370.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-14T20:19:11Z","receivedAt":"2009-02-14T20:19:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> +\telse if (given_config_file) {\n>> +\t\tif (!is_absolute_path(given_config_file) && file)\n>> +\t\t\tfile = prefix_filename(file, strlen(file),\n>> +\t\t\t\t\t given_config_file);\n>> +\t\telse\n>> +\t\t\tfile = given_config_file;\n>> +\t\tconfig_exclusive_filename = file;\n>\n> It took me a considerable amount of time to figure out that \"file\" is \n> actually the \"prefix\"! That cleanup would be nice to have before the \n> parseopt patch, methinks, especially since the code is reindented, and \n> thus hard to follow in the diff.\n\nI noticed that \"file\" thing during my review of the first round. It\nprobably was a cute attempt to avoid using two variables \"prefix\" and\n\"file\", but made the result a lot harder to read. I agree that a clean-up\n*before* code restructuring would be good for this particular wart.\n\n A note that is off-topic to this patch.\n\n I also noticed that the diff was impossible to read because it matched\n the lines with only an indented close brace or whitespace between the\n preimage and the postimage too aggressively. Your --patience did seem\n to help a little bit, at least it produced a different result, but not\n much (not that patience was meant to make this kind of change easier\n to read). It may have helped if we had that \"do not match trivial\n lines too aggressively just to reduce the patch size\" option.\n"},{"id":"104689","messageId":"94a0d4530902141231t143067e5n872558a4e515be4a@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142041370.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T20:31:09Z","receivedAt":"2009-02-14T20:31:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>\n>> @@ -231,7 +264,7 @@ static int get_diff_color_found;\n>> static int git_get_colorbool_config(const char *var, const char *value,\n>> void *cb)\n>> {\n>> - if (!strcmp(var, get_color_slot)) {\n>> + if (!strcmp(var, get_colorbool_slot)) {\n>> get_colorbool_found =\n>> git_config_colorbool(var, value, stdout_is_tty);\n>> }\n>\n> Name changes like this make it harder to read the patch; can you separate\n> that change out into its own patch?\n\nIn that case I couldn't use OPT_STRING to store that value; I would\nhave to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and\nstore check the argc (since OPT_STRING isn't doing that anymore) and\nsave argv[1] to get_color_slot,\n\n>> + if (use_global_config) {\n>> + char *home = getenv(\"HOME\");\n>> + if (home) {\n>> + char *user_config = xstrdup(mkpath(\"%s/.gitconfig\", home));\n>> + config_exclusive_filename = user_config;\n>\n> In a subsequent patch, you might add a check only one of --global,\n> --system or --file was given.\n\nYeap.\n\n>> + else if (given_config_file) {\n>> + if (!is_absolute_path(given_config_file) && file)\n>> + file = prefix_filename(file, strlen(file),\n>> + given_config_file);\n>> + else\n>> + file = given_config_file;\n>> + config_exclusive_filename = file;\n>\n> It took me a considerable amount of time to figure out that \"file\" is\n> actually the \"prefix\"! That cleanup would be nice to have before the\n> parseopt patch, methinks, especially since the code is reindented, and\n> thus hard to follow in the diff.\n\nTrue.\n\nI tried to minimize changes, so that code is exactly the same as\nbefore (except vi didn't preserve the indent).\n\n>> + if (actions & ACTION_LIST) {\n>> + if (git_config(show_all_config, NULL) < 0 &&\n>> + file && errno)\n>\n> Should this not be config_exclusive_filename?\n\nLooks like that.\n\n>> + die(\"unable to read config file %s: %s\", file,\n>> + strerror(errno));\n>\n> Do we really only want to die() in case we know the file name? AFAICT at\n> this point we have no idea in which of the possibly three files the error\n> occurred. And there need not be any errno set, for example when there was\n> a parse error.\n\nYes, there should be an error even if 'file' is not set. But if the\nfile is set what's wrong with that die command?\n\n>> + else if (actions & ACTION_EDIT) {\n>> + const char *config_filename;\n>> + if (config_exclusive_filename)\n>> + config_filename = config_exclusive_filename;\n>> + else\n>> + config_filename = git_path(\"config\");\n>\n> Why not reuse config_exclusive_filename here?\n\nYou mean:\nif (!config_exclusive_filename)\n config_exclusive_filename = git_path(\"config\");\n\n>> + else if (actions & ACTION_ADD) {\n>> + check_argc(argc, 2, 2);\n>\n> BTW what about check_argc() in the previous two cases?\n\nYou mean fail if -e or -l have extra arguments?\n\n>> + return git_config_set_multivar(argv[0], value, \"^$\", 0);\n>\n> Now that I see this, there is another idea for a possible cleanup after\n> the parseoptification: cmd_config() should not return -1, as that will be\n> turned into the exit status. So maybe prefix the return value with \"!!\"?\n>\n> Or maybe even better: set a variable \"ret\" and at the end of cmd_config(),\n> \"return !!ret;\"?\n\nHuh? So git commands don't return negative error values?\n\n-- \nFelipe Contreras\n"},{"id":"104690","messageId":"7v8wo8sqnc.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"94a0d4530902141209j7a3a9976l80355bee526852ed@mail.gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-14T20:35:35Z","receivedAt":"2009-02-14T20:35:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> I personally do not think \"I rewrote this command's option parser using\n>> parseopt\" earns any \"trust point\". I think the latter is a *great* thing\n>> to do, though.\n>\n> I disagree. Making a patch pass through all the filters must mean\n> something, and the more patches the more trust.\n\nWhy are you arguing?\n\nI am saying I do not feel more trust in people _merely because_ they\nrewrote command parser to use parseopt. Telling me that I am wrong and I\nshould trust you more for such a patch would not help.\n\n>> Mistakes made in the past and resulting flaws that remain in the current\n>> source do not justify a new patch adding more mistakes to the codebase.\n>> Reviewers help the author from adding more.\n>>\n>> One bad thing about the current process (and I am certainly one of the\n>> guilty parties because I do a lot of reviews) around this area is that a\n>> review comment that points out a mistake similar to the ones made in the\n>> past sound to the author of the patch as if the reviewer is telling that\n>> the patch will not be accepted unless the existing mistakes are also fixed\n>> by the patch author. Such a review certainly does not mean that ...\n>> ...\n> But, if there's code that already has the same issues the patch has,\n> it doesn't look like a good argument for patch rejection. Perhaps the\n> quality standards increased, but on the other hand things wouldn't get\n> worst by applying the patch.\n\nIf you read the above quoted block again, you will notice that we are\nalmost in full agreement, *if* you change \"by applying the patch\" in your\nlast sentence to \"by applying a patch that is revised to fix the problem\npointed out during the review in it, even if it does not address the\nsimilar ones in the existing code\".\n\nAdding more breakages of the same kind may not increase the number of\nclasses of breakages, but surely it increases the number of places that\nneed to be fixed later, and it is actively wrong to discard time and\nenergy somebody already spent to prevent one more instance of known\nbreakage to get into the codebase.\n"},{"id":"104693","messageId":"94a0d4530902141301x2cbf5ed1yb2a23299f84e4319@mail.gmail.com","threadId":"17782","inReplyTo":"7v8wo8sqnc.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T21:01:18Z","receivedAt":"2009-02-14T21:01:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 10:35 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> I personally do not think \"I rewrote this command's option parser using\n>>> parseopt\" earns any \"trust point\". I think the latter is a *great* thing\n>>> to do, though.\n>>\n>> I disagree. Making a patch pass through all the filters must mean\n>> something, and the more patches the more trust.\n>\n> Why are you arguing?\n>\n> I am saying I do not feel more trust in people _merely because_ they\n> rewrote command parser to use parseopt. Telling me that I am wrong and I\n> should trust you more for such a patch would not help.\n\nI'm not arguing, that's just my opinion.\n\nI'm not saying writing a patch should give anyone penguin points, it's\ngoing through the review process up to patch acceptance. Maybe not to\nyou, but maybe for other developers.\n\n>>> Mistakes made in the past and resulting flaws that remain in the current\n>>> source do not justify a new patch adding more mistakes to the codebase.\n>>> Reviewers help the author from adding more.\n>>>\n>>> One bad thing about the current process (and I am certainly one of the\n>>> guilty parties because I do a lot of reviews) around this area is that a\n>>> review comment that points out a mistake similar to the ones made in the\n>>> past sound to the author of the patch as if the reviewer is telling that\n>>> the patch will not be accepted unless the existing mistakes are also fixed\n>>> by the patch author. Such a review certainly does not mean that ...\n>>> ...\n>> But, if there's code that already has the same issues the patch has,\n>> it doesn't look like a good argument for patch rejection. Perhaps the\n>> quality standards increased, but on the other hand things wouldn't get\n>> worst by applying the patch.\n>\n> If you read the above quoted block again, you will notice that we are\n> almost in full agreement, *if* you change \"by applying the patch\" in your\n> last sentence to \"by applying a patch that is revised to fix the problem\n> pointed out during the review in it, even if it does not address the\n> similar ones in the existing code\".\n>\n> Adding more breakages of the same kind may not increase the number of\n> classes of breakages, but surely it increases the number of places that\n> need to be fixed later, and it is actively wrong to discard time and\n> energy somebody already spent to prevent one more instance of known\n> breakage to get into the codebase.\n\nI think are in full agreement, it's just that I wasn't clear enough;\nthose are not the kind of issues I was talking about. If there's a\nknown way how to do something then it's obvious the patch should be\nre-submitted with the 'right way'. I was thinking more on \"FILE\" vs\n\"file\", or any kind of issue that would require a separate patch to\nreach consistency in the existing code; those can wait until after the\noriginal patch is accepted.\n\n-- \nFelipe Contreras\n"},{"id":"104694","messageId":"alpine.DEB.1.00.0902142157000.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"7viqncsreo.fsf@gitster.siamese.dyndns.org","subject":"human readable diffs, was Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T21:08:38Z","receivedAt":"2009-02-14T21:08:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Junio C Hamano wrote:\n\n> I also noticed that the diff was impossible to read because it \n> matched the lines with only an indented close brace or whitespace \n> between the preimage and the postimage too aggressively. Your \n> --patience did seem to help a little bit, at least it produced a \n> different result, but not much (not that patience was meant to make \n> this kind of change easier to read). It may have helped if we had \n> that \"do not match trivial lines too aggressively just to reduce the \n> patch size\" option.\n\nYeah, I think that your objections to my '--collapse-non-alnums' patch \nshow to be relevant especially with this patch: an '} else {' matches, but \nactually disturbs readability.\n\nSo an option would be nice, indeed, which can merge a single matching line \n(or a given maximal number of matching lines) between runs of a certain \nminimal number of _non-matching_ lines.\n\nBut how to call it?\n\nAnd how determine the ideal ratio between matching/non-matching lines \nuntil which to consider the matching lines to be non-matches? Maybe a \nsimple fraction would be enough...\n\nOr maybe an absolute number of matching lines? But it is obvious how to \nconstruct a case where this breaks down.\n\nCiao,\nDscho\n"},{"id":"104695","messageId":"7vtz6wrahg.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"94a0d4530902141209j7a3a9976l80355bee526852ed@mail.gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-14T21:10:03Z","receivedAt":"2009-02-14T21:10:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> Unfortunately, not many patch authors write such a summary. Sometimes we\n>> see summaries on things that were discussed but nobody has followed\n>> through posted by third parties (including myself), but we do not seem to\n>> have enough helpers to do that either. This does not take much technical\n>> skills but is a good \"trust point\" earner.\n>\n> For me it's easier, and more fun to write a separate patch that fixes\n> the issues than writing a summary,...\n\nThat certainly is something we should take into consideration.\n\nI however think an unwritten assumption around here so far has been that\nthe patch author who gets review comments is expected to keep track of the\nissues raised, both about the patch itself and about the similar breakages\nin the existing code pointed out during the review process, if only\nbecause the patch author is the focal point of the discussion.\n\nWe probably need to break that.\n\nBecause it is very likely that the reviewer does not even realize that\nsuch similar breakages in the existing code when a review is made, we\ncannot ask reviewers to always start a separate discussion. Some reviews\ndo say \"Admittedly, we already have the same pattern in here and there,\nbut this in your patch is wrong,\" but the way how we collectively realize\nan existing breakage is often by hearing the patch author respond with\n\"but there already are this and that breakages in the existing code.\"\n\nWe do not want such knowledge of existing breakages go to waste in either\ncase. Perhaps it would be a good start to make it the responsibility of\nthe first person who mentions an existing breakage (either the reviewer's\n\"Admittedly\", or the patch author's \"but there already are\") to begin a\nseparate thread, so that mail archive would remember it. It shouldn't\ntake more than 3 minutes.\n"},{"id":"104696","messageId":"94a0d4530902141313i24ddc571m6a041f3879960ab2@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142025420.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T21:13:23Z","receivedAt":"2009-02-14T21:13:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 9:26 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sat, 14 Feb 2009, Johannes Schindelin wrote:\n>\n>> On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>>\n>> > On Sat, Feb 14, 2009 at 9:11 PM, Johannes Schindelin\n>> > wrote:\n>> > > Hi,\n>> > >\n>> > > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>> > >\n>> > >> 1) --list when no config file is given uses all the config files,\n>> > >> wouldn't it make sense to have a --repo option?\n>> > >\n>> > > The idea of --list is not \"cat .git/config\". The idea is to help users or\n>> > > scripts to list the current settings (_including_ the global settings).\n>> > >\n>> > > You can force showing the repo-specific config with \"git --file\n>> > > .git/config\", though.\n>> >\n>> > When you are on the root directory of the repo, and you don't have\n>> > GIT_DIR, or --git-dir.\n>>\n>> When I wrote my response, I briefly considered if I had to be verbose, and\n>> decided against it.\n>>\n>> But this is what I should have written:\n>>\n>> git --file $(git rev-parse --git-dir)/config --list\n>\n> Okay, before anybody points out that I was not verbose enough -- again --\n> this is what I really should have written:\n>\n> git --file \"$(git rev-parse --git-dir)\"/config --list\n\nYeap, it's possible, but I think people would not complain about \"git\n--repo --list\". I think it would be much clearer to have --global\n--system --repo and --all. Then we can say --list by default uses\n--all, and --edit uses --repo, and it doesn't make sense to do --edit\n--all.\n\nAnyway, I don't care that much.\n\n-- \nFelipe Contreras\n"},{"id":"104697","messageId":"alpine.DEB.1.00.0902142213090.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"94a0d4530902141209j7a3a9976l80355bee526852ed@mail.gmail.com","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T21:15:59Z","receivedAt":"2009-02-14T21:15:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Felipe Contreras wrote:\n\n> On Sat, Feb 14, 2009 at 9:29 PM, Junio C Hamano wrote:\n> > Felipe Contreras writes:\n> >\n> >> This is more a \"I would like to increase the chances of my patches \n> >> being accepted so I'd do some chores to gain the trust of some \n> >> developers\", and Johannes Schindelin was pushing me to do this.\n> >>\n> >> Also it's a bit of \"I would like to improve git and learn the API \n> >> while doing so\".\n> >\n> > I personally do not think \"I rewrote this command's option parser \n> > using parseopt\" earns any \"trust point\". I think the latter is a \n> > *great* thing to do, though.\n> \n> I disagree. Making a patch pass through all the filters must mean \n> something, and the more patches the more trust.\n\nMaybe I should point out something that is obvious to somebody who \nfollowed the Git list for a long time: there are two areas of the code \nthat had such a track record of regressions that Junio grew a distaste for \nthem: git-config and parse options.\n\nHowever, I imagine if you manage to provide a patch that touches both \nareas _and_ that are without flaws, then you get some brownie points :-)\n\nCiao,\nDscho\n"},{"id":"104699","messageId":"94a0d4530902141324w1cea462ex99a698e5a702d85a@mail.gmail.com","threadId":"17782","inReplyTo":"7vtz6wrahg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-14T21:24:03Z","receivedAt":"2009-02-14T21:24:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 14, 2009 at 11:10 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> Unfortunately, not many patch authors write such a summary. Sometimes we\n>>> see summaries on things that were discussed but nobody has followed\n>>> through posted by third parties (including myself), but we do not seem to\n>>> have enough helpers to do that either. This does not take much technical\n>>> skills but is a good \"trust point\" earner.\n>>\n>> For me it's easier, and more fun to write a separate patch that fixes\n>> the issues than writing a summary,...\n>\n> That certainly is something we should take into consideration.\n>\n> I however think an unwritten assumption around here so far has been that\n> the patch author who gets review comments is expected to keep track of the\n> issues raised, both about the patch itself and about the similar breakages\n> in the existing code pointed out during the review process, if only\n> because the patch author is the focal point of the discussion.\n>\n> We probably need to break that.\n>\n> Because it is very likely that the reviewer does not even realize that\n> such similar breakages in the existing code when a review is made, we\n> cannot ask reviewers to always start a separate discussion. Some reviews\n> do say \"Admittedly, we already have the same pattern in here and there,\n> but this in your patch is wrong,\" but the way how we collectively realize\n> an existing breakage is often by hearing the patch author respond with\n> \"but there already are this and that breakages in the existing code.\"\n>\n> We do not want such knowledge of existing breakages go to waste in either\n> case. Perhaps it would be a good start to make it the responsibility of\n> the first person who mentions an existing breakage (either the reviewer's\n> \"Admittedly\", or the patch author's \"but there already are\") to begin a\n> separate thread, so that mail archive would remember it. It shouldn't\n> take more than 3 minutes.\n\nThat is true, however I propose something a bit different. At least in\nthis review there has been a number of issues brought up, it would be\noverkill to create a separate thread for each one of these issues as\nthey where found, and if the patch submitter is new, he probably\nwouldn't know about this rule.\n\nSo, I propose that at the end of the patch review process the ack\nperson (or somebody else) asks the patch submitter (possibly cc'ing\nthe reviewers) to start a new thread mentioning the pending issues\nbrought up in the review.\n\n-- \nFelipe Contreras\n"},{"id":"104710","messageId":"alpine.DEB.1.00.0902142328530.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"94a0d4530902141231t143067e5n872558a4e515be4a@mail.gmail.com","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T22:32:12Z","receivedAt":"2009-02-14T22:32:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Feb 2009, Felipe Contreras wrote:\n\n> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n> wrote:\n>\n> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> >\n> >> @@ -231,7 +264,7 @@ static int get_diff_color_found;\n> >> static int git_get_colorbool_config(const char *var, const char *value,\n> >> void *cb)\n> >> {\n> >> - if (!strcmp(var, get_color_slot)) {\n> >> + if (!strcmp(var, get_colorbool_slot)) {\n> >> get_colorbool_found =\n> >> git_config_colorbool(var, value, stdout_is_tty);\n> >> }\n> >\n> > Name changes like this make it harder to read the patch; can you separate\n> > that change out into its own patch?\n> \n> In that case I couldn't use OPT_STRING to store that value; I would\n> have to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and\n> store check the argc (since OPT_STRING isn't doing that anymore) and\n> save argv[1] to get_color_slot,\n\nWhat I meant was: have a patch renaming get_color_slot to \nget_colorbool_slot _before_ the (already large) parseoptification.\n\n> >> + die(\"unable to read config file %s: %s\", file,\n> >> + strerror(errno));\n> >\n> > Do we really only want to die() in case we know the file name? AFAICT at\n> > this point we have no idea in which of the possibly three files the error\n> > occurred. And there need not be any errno set, for example when there was\n> > a parse error.\n> \n> Yes, there should be an error even if 'file' is not set. But if the\n> file is set what's wrong with that die command?\n\nI think we should die() in all cases, not just one, and we might want to \nskip the \"file\" parameter (and the \"errno\") parameter, as the file \ncontaining the error could be different from \"file\".\n\n> >> + else if (actions & ACTION_EDIT) {\n> >> + const char *config_filename;\n> >> + if (config_exclusive_filename)\n> >> + config_filename = config_exclusive_filename;\n> >> + else\n> >> + config_filename = git_path(\"config\");\n> >\n> > Why not reuse config_exclusive_filename here?\n> \n> You mean:\n> if (!config_exclusive_filename)\n> config_exclusive_filename = git_path(\"config\");\n\nYes.\n\n> >> + else if (actions & ACTION_ADD) {\n> >> + check_argc(argc, 2, 2);\n> >\n> > BTW what about check_argc() in the previous two cases?\n> \n> You mean fail if -e or -l have extra arguments?\n\nYes.\n\n> >> + return git_config_set_multivar(argv[0], value, \"^$\", 0);\n> >\n> > Now that I see this, there is another idea for a possible cleanup \n> > after the parseoptification: cmd_config() should not return -1, as \n> > that will be turned into the exit status. So maybe prefix the return \n> > value with \"!!\"?\n> >\n> > Or maybe even better: set a variable \"ret\" and at the end of \n> > cmd_config(), \"return !!ret;\"?\n> \n> Huh? So git commands don't return negative error values?\n\nAFAICT an exit status is supposed to be between 0 and 127.\n\nCiao,\nDscho\n"},{"id":"104711","messageId":"gn7h17$o6t$1@ger.gmane.org","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142328530.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-02-14T22:36:26Z","receivedAt":"2009-02-14T22:36:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Johannes Schindelin wrote:\n\n>> You mean fail if -e or -l have extra arguments?\n> \n> Yes.\n\n'-l' can have extra arguments: '-z'\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"104715","messageId":"alpine.DEB.1.00.0902142353530.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"gn7h17$o6t$1@ger.gmane.org","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-14T22:54:35Z","receivedAt":"2009-02-14T22:54:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nplease do not cull the Cc: list.\n\nOn Sat, 14 Feb 2009, Jakub Narebski wrote:\n\n> Johannes Schindelin wrote:\n> \n> >> You mean fail if -e or -l have extra arguments?\n> > \n> > Yes.\n> \n> '-l' can have extra arguments: '-z'\n\nWe meant \"non-option argument\".\n\nCiao,\nDscho\n"},{"id":"104721","messageId":"7vab8oqw09.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142213090.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-15T02:22:46Z","receivedAt":"2009-02-15T02:22:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Maybe I should point out something that is obvious to somebody who \n> followed the Git list for a long time: there are two areas of the code \n> that had such a track record of regressions that Junio grew a distaste for \n> them: git-config and parse options.\n\nThere is a difference between them; please don't confuse new readers.\n\ngit-config does have a rather unfortunate track record and the code may\nstill be a mess. But parse-options itself is a good code overall. Only\nthat some of the parse-opt-ification attempts in the past might have been\nquite bad.\n\nParse-opt-ification is an obvious, trivial change with a limited scope,\nwell-defined end results and clear gain to the end users. The API exists\nso the patch author does not have to invent a new framework, the change a\npatch needs will typically be limited to a single command, the set of\noptions the command needs to accept/reject are already defined, and at the\nend you can give unique prefix of flags from the command line.\n\nThere may be a correlation between parse-opt-ification attempts and bad\nreview cycles, but there is no such a causal relationship \"parse-opt\npatches are bad because parse-opt is a bad idea\". \n\nIf there is such a correlation, it is more likely that it is merely\nbecause parse-opt-ification attracted more inexperienced people than\ntricker areas like revision traversal or extended SHA-1 syntax.\n\nBut people can send bad patches to any area ;-).\n"},{"id":"104749","messageId":"94a0d4530902150104n555b845bofc5897230c64a5f2@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142328530.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-15T09:04:11Z","receivedAt":"2009-02-15T09:04:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>\n>> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n>> wrote:\n>>\n>> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>> >\n>> >> @@ -231,7 +264,7 @@ static int get_diff_color_found;\n>> >> static int git_get_colorbool_config(const char *var, const char *value,\n>> >> void *cb)\n>> >> {\n>> >> - if (!strcmp(var, get_color_slot)) {\n>> >> + if (!strcmp(var, get_colorbool_slot)) {\n>> >> get_colorbool_found =\n>> >> git_config_colorbool(var, value, stdout_is_tty);\n>> >> }\n>> >\n>> > Name changes like this make it harder to read the patch; can you separate\n>> > that change out into its own patch?\n>>\n>> In that case I couldn't use OPT_STRING to store that value; I would\n>> have to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and\n>> store check the argc (since OPT_STRING isn't doing that anymore) and\n>> save argv[1] to get_color_slot,\n>\n> What I meant was: have a patch renaming get_color_slot to\n> get_colorbool_slot _before_ the (already large) parseoptification.\n\nDone.\n\n>> >> + die(\"unable to read config file %s: %s\", file,\n>> >> + strerror(errno));\n>> >\n>> > Do we really only want to die() in case we know the file name? AFAICT at\n>> > this point we have no idea in which of the possibly three files the error\n>> > occurred. And there need not be any errno set, for example when there was\n>> > a parse error.\n>>\n>> Yes, there should be an error even if 'file' is not set. But if the\n>> file is set what's wrong with that die command?\n>\n> I think we should die() in all cases, not just one, and we might want to\n> skip the \"file\" parameter (and the \"errno\") parameter, as the file\n> containing the error could be different from \"file\".\n\nDone.\n\n>> >> + else if (actions & ACTION_EDIT) {\n>> >> + const char *config_filename;\n>> >> + if (config_exclusive_filename)\n>> >> + config_filename = config_exclusive_filename;\n>> >> + else\n>> >> + config_filename = git_path(\"config\");\n>> >\n>> > Why not reuse config_exclusive_filename here?\n>>\n>> You mean:\n>> if (!config_exclusive_filename)\n>> config_exclusive_filename = git_path(\"config\");\n>\n> Yes.\n\nI'm not sure about this one. At least git_config should be moved\nbefore that code, otherwise it will only try to read core.editor from\nthe exclusive_filename and that's not what we want.\n\nIf somebody adds some extra code that uses config_exclusive_filename\nthere would be issues. I would prefer to leave it like that since\nthat's how the code in config.c is handling config_exclusive_filename.\n\n>> >> + else if (actions & ACTION_ADD) {\n>> >> + check_argc(argc, 2, 2);\n>> >\n>> > BTW what about check_argc() in the previous two cases?\n>>\n>> You mean fail if -e or -l have extra arguments?\n>\n> Yes.\n\nDone.\n\n>> >> + return git_config_set_multivar(argv[0], value, \"^$\", 0);\n>> >\n>> > Now that I see this, there is another idea for a possible cleanup\n>> > after the parseoptification: cmd_config() should not return -1, as\n>> > that will be turned into the exit status. So maybe prefix the return\n>> > value with \"!!\"?\n>> >\n>> > Or maybe even better: set a variable \"ret\" and at the end of\n>> > cmd_config(), \"return !!ret;\"?\n>>\n>> Huh? So git commands don't return negative error values?\n>\n> AFAICT an exit status is supposed to be between 0 and 127.\n\nOk. Done.\n\n-- \nFelipe Contreras\n"},{"id":"104761","messageId":"alpine.DEB.1.00.0902151224450.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"94a0d4530902150104n555b845bofc5897230c64a5f2@mail.gmail.com","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-15T11:26:28Z","receivedAt":"2009-02-15T11:26:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 15 Feb 2009, Felipe Contreras wrote:\n\n> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin\n> wrote:\n> > Hi,\n> >\n> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> >\n> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n> >> wrote:\n> >>\n> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> >> >\n> >> >> + else if (actions & ACTION_EDIT) {\n> >> >> + const char *config_filename;\n> >> >> + if (config_exclusive_filename)\n> >> >> + config_filename = config_exclusive_filename;\n> >> >> + else\n> >> >> + config_filename = git_path(\"config\");\n> >> >\n> >> > Why not reuse config_exclusive_filename here?\n> >>\n> >> You mean:\n> >> if (!config_exclusive_filename)\n> >> config_exclusive_filename = git_path(\"config\");\n> >\n> > Yes.\n> \n> I'm not sure about this one. At least git_config should be moved before \n> that code, otherwise it will only try to read core.editor from the \n> exclusive_filename and that's not what we want.\n\nAh! I did not think about core.editor, of course, but that is what got \nyou started with git-config, after all.\n\nHowever, the next line sets config_exclusive_filename, does it not?\n\nCiao,\nDscho\n"},{"id":"104764","messageId":"94a0d4530902150407u1bc6669cr2584ff8f1a8b47af@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902151224450.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-15T12:07:44Z","receivedAt":"2009-02-15T12:07:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sun, 15 Feb 2009, Felipe Contreras wrote:\n>\n>> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin\n>> wrote:\n>> > Hi,\n>> >\n>> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>> >\n>> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n>> >> wrote:\n>> >>\n>> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>> >> >\n>> >> >> + else if (actions & ACTION_EDIT) {\n>> >> >> + const char *config_filename;\n>> >> >> + if (config_exclusive_filename)\n>> >> >> + config_filename = config_exclusive_filename;\n>> >> >> + else\n>> >> >> + config_filename = git_path(\"config\");\n>> >> >\n>> >> > Why not reuse config_exclusive_filename here?\n>> >>\n>> >> You mean:\n>> >> if (!config_exclusive_filename)\n>> >> config_exclusive_filename = git_path(\"config\");\n>> >\n>> > Yes.\n>>\n>> I'm not sure about this one. At least git_config should be moved before\n>> that code, otherwise it will only try to read core.editor from the\n>> exclusive_filename and that's not what we want.\n>\n> Ah! I did not think about core.editor, of course, but that is what got\n> you started with git-config, after all.\n>\n> However, the next line sets config_exclusive_filename, does it not?\n\nHuh? Which line?\n\n-- \nFelipe Contreras\n"},{"id":"104767","messageId":"alpine.DEB.1.00.0902151329370.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"94a0d4530902150407u1bc6669cr2584ff8f1a8b47af@mail.gmail.com","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-15T12:33:03Z","receivedAt":"2009-02-15T12:33:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 15 Feb 2009, Felipe Contreras wrote:\n\n> On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin\n> wrote:\n> > Hi,\n> >\n> > On Sun, 15 Feb 2009, Felipe Contreras wrote:\n> >\n> >> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin\n> >> wrote:\n> >> > Hi,\n> >> >\n> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> >> >\n> >> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n> >> >> wrote:\n> >> >>\n> >> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n> >> >> >\n> >> >> >> + else if (actions & ACTION_EDIT) {\n> >> >> >> + const char *config_filename;\n> >> >> >> + if (config_exclusive_filename)\n> >> >> >> + config_filename = config_exclusive_filename;\n> >> >> >> + else\n> >> >> >> + config_filename = git_path(\"config\");\n> >> >> >\n> >> >> > Why not reuse config_exclusive_filename here?\n> >> >>\n> >> >> You mean:\n> >> >> if (!config_exclusive_filename)\n> >> >> config_exclusive_filename = git_path(\"config\");\n> >> >\n> >> > Yes.\n> >>\n> >> I'm not sure about this one. At least git_config should be moved before\n> >> that code, otherwise it will only try to read core.editor from the\n> >> exclusive_filename and that's not what we want.\n> >\n> > Ah! I did not think about core.editor, of course, but that is what got\n> > you started with git-config, after all.\n> >\n> > However, the next line sets config_exclusive_filename, does it not?\n> \n> Huh? Which line?\n\nAh, I misremembered. Your current patch shows this:\n\n+ else if (actions & ACTION_EDIT) {\n+ const char *config_filename;\n+ if (config_exclusive_filename)\n+ config_filename = config_exclusive_filename;\n+ else\n+ config_filename = git_path(\"config\");\n+ git_config(git_default_config, NULL);\n+ launch_editor(config_filename, NULL, NULL);\n+ }\n\n... which makes me believe that the point about moving git_config() before \nsetting config_exclusive_filename is rather weak :-)\n\n+ else if (actions & ACTION_EDIT) {\n+ git_config(git_default_config, NULL);\n+ launch_editor(config_exclusive_filename ?\n+\t\t\tconfig_exclusive_filename : git_path(\"config\"), \n+\t\t\tNULL, NULL);\n+ }\n\n(This has whitespace issues, as I copy-pasted it using kterm's clipboard \nfunctions.)\n\nCiao,\nDscho\n"},{"id":"104771","messageId":"94a0d4530902150451w2467bee2q225643f15ffffac1@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902151329370.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-15T12:51:45Z","receivedAt":"2009-02-15T12:51:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Feb 15, 2009 at 2:33 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sun, 15 Feb 2009, Felipe Contreras wrote:\n>\n>> On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin\n>> wrote:\n>> > Hi,\n>> >\n>> > On Sun, 15 Feb 2009, Felipe Contreras wrote:\n>> >\n>> >> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin\n>> >> wrote:\n>> >> > Hi,\n>> >> >\n>> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>> >> >\n>> >> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n>> >> >> wrote:\n>> >> >>\n>> >> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>> >> >> >\n>> >> >> >> + else if (actions & ACTION_EDIT) {\n>> >> >> >> + const char *config_filename;\n>> >> >> >> + if (config_exclusive_filename)\n>> >> >> >> + config_filename = config_exclusive_filename;\n>> >> >> >> + else\n>> >> >> >> + config_filename = git_path(\"config\");\n>> >> >> >\n>> >> >> > Why not reuse config_exclusive_filename here?\n>> >> >>\n>> >> >> You mean:\n>> >> >> if (!config_exclusive_filename)\n>> >> >> config_exclusive_filename = git_path(\"config\");\n>> >> >\n>> >> > Yes.\n>> >>\n>> >> I'm not sure about this one. At least git_config should be moved before\n>> >> that code, otherwise it will only try to read core.editor from the\n>> >> exclusive_filename and that's not what we want.\n>> >\n>> > Ah! I did not think about core.editor, of course, but that is what got\n>> > you started with git-config, after all.\n>> >\n>> > However, the next line sets config_exclusive_filename, does it not?\n>>\n>> Huh? Which line?\n>\n> Ah, I misremembered. Your current patch shows this:\n>\n> + else if (actions & ACTION_EDIT) {\n> + const char *config_filename;\n> + if (config_exclusive_filename)\n> + config_filename = config_exclusive_filename;\n> + else\n> + config_filename = git_path(\"config\");\n> + git_config(git_default_config, NULL);\n> + launch_editor(config_filename, NULL, NULL);\n> + }\n>\n> ... which makes me believe that the point about moving git_config() before\n> setting config_exclusive_filename is rather weak :-)\n>\n> + else if (actions & ACTION_EDIT) {\n> + git_config(git_default_config, NULL);\n> + launch_editor(config_exclusive_filename ?\n> + config_exclusive_filename : git_path(\"config\"),\n> + NULL, NULL);\n> + }\n>\n> (This has whitespace issues, as I copy-pasted it using kterm's clipboard\n> functions.)\n\nI thought on that before but now it makes more sense to me. Attaching the patch.\n\n-- \nFelipe Contreras\n\n\nFrom 4b3db3d13261849cd7d561dc20bddb375f1430c8 Mon Sep 17 00:00:00 2001\nFrom: Felipe Contreras \nDate: Sun, 15 Feb 2009 14:46:58 +0200\nSubject: [PATCH] config: Cleanup editor action.\n\nCopy-paste from Johannes Schindelin.\n\nSigned-off-by: Felipe Contreras \n---\n builtin-config.c | 9 +++------\n 1 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex 2c1ad71..bd7bac4 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -370,13 +370,10 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)\n \t}\n \telse if (actions & ACTION_EDIT) {\n \t\tcheck_argc(argc, 0, 0);\n-\t\tconst char *config_filename;\n-\t\tif (config_exclusive_filename)\n-\t\t\tconfig_filename = config_exclusive_filename;\n-\t\telse\n-\t\t\tconfig_filename = git_path(\"config\");\n \t\tgit_config(git_default_config, NULL);\n-\t\tlaunch_editor(config_filename, NULL, NULL);\n+\t\tlaunch_editor(config_exclusive_filename ?\n+\t\t\t config_exclusive_filename : git_path(\"config\"),\n+\t\t\t NULL, NULL);\n \t}\n \telse if (actions & ACTION_ADD) {\n \t\tcheck_argc(argc, 2, 2);\n-- \n1.6.1.3\n\n"},{"id":"104776","messageId":"20090215133806.GC13810@annwn","threadId":"17782","inReplyTo":"94a0d4530902150451w2467bee2q225643f15ffffac1@mail.gmail.com","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-15T13:38:06Z","receivedAt":"2009-02-15T13:38:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Feb 15, 2009 at 02:51:45PM +0200, Felipe Contreras wrote:\n> On Sun, Feb 15, 2009 at 2:33 PM, Johannes Schindelin\n> wrote:\n> > Ah, I misremembered. Your current patch shows this:\n> >\n> > + else if (actions & ACTION_EDIT) {\n> > + const char *config_filename;\n> > + if (config_exclusive_filename)\n> > + config_filename = config_exclusive_filename;\n> > + else\n> > + config_filename = git_path(\"config\");\n> > + git_config(git_default_config, NULL);\n> > + launch_editor(config_filename, NULL, NULL);\n> > + }\n> >\n> > ... which makes me believe that the point about moving git_config() before\n> > setting config_exclusive_filename is rather weak :-)\n> >\n> > + else if (actions & ACTION_EDIT) {\n> > + git_config(git_default_config, NULL);\n> > + launch_editor(config_exclusive_filename ?\n> > + config_exclusive_filename : git_path(\"config\"),\n> > + NULL, NULL);\n> > + }\n> >\n> > (This has whitespace issues, as I copy-pasted it using kterm's clipboard\n> > functions.)\n> \n> I thought on that before but now it makes more sense to me. Attaching the patch.\n\nNow inlined (sorry):\n\n---\n builtin-config.c | 9 +++------\n 1 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex 2c1ad71..bd7bac4 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -370,13 +370,10 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)\n \t}\n \telse if (actions & ACTION_EDIT) {\n \t\tcheck_argc(argc, 0, 0);\n-\t\tconst char *config_filename;\n-\t\tif (config_exclusive_filename)\n-\t\t\tconfig_filename = config_exclusive_filename;\n-\t\telse\n-\t\t\tconfig_filename = git_path(\"config\");\n \t\tgit_config(git_default_config, NULL);\n-\t\tlaunch_editor(config_filename, NULL, NULL);\n+\t\tlaunch_editor(config_exclusive_filename ?\n+\t\t\t config_exclusive_filename : git_path(\"config\"),\n+\t\t\t NULL, NULL);\n \t}\n \telse if (actions & ACTION_ADD) {\n \t\tcheck_argc(argc, 2, 2);\n-- \n1.6.1.3\n\n-- \nFelipe Contreras\n"},{"id":"104801","messageId":"7v1vtzmr9j.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142328530.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-15T19:31:04Z","receivedAt":"2009-02-15T19:31:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > Or maybe even better: set a variable \"ret\" and at the end of \n>> > cmd_config(), \"return !!ret;\"?\n>> \n>> Huh? So git commands don't return negative error values?\n>\n> AFAICT an exit status is supposed to be between 0 and 127.\n\nAre you two talking about the return value from cmd_config()?\n\ngit.c::run_builtin() already knows what to do with status codes from the\nbuiltins to protect you from (rare) shells that do not cope with a\nnegative return that come from the common pattern of doing:\n\n\treturn error(\"it is wrong in this way\")\n\nSo \"negative\" is not really a problem.\n\nIndeed, if the old code was doing:\n\n\tret = git_config_set_multivar(...);\n if (ret)\n\t\treturn ret;\n\nand if you are changing it to:\n\n\tret = git_config_set_multivar(...);\n if (ret)\n\t\treturn !!ret;\n\nyou are changing an externally observable behaviour that _could_ break the\ncalling end user scripts.\n\n.\n"},{"id":"104808","messageId":"7v1vtzlcfi.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902142328530.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-15T19:36:49Z","receivedAt":"2009-02-15T19:36:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sat, 14 Feb 2009, Felipe Contreras wrote:\n>> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin\n>> wrote:\n>> > Or maybe even better: set a variable \"ret\" and at the end of \n>> > cmd_config(), \"return !!ret;\"?\n>> \n>> Huh? So git commands don't return negative error values?\n>\n> AFAICT an exit status is supposed to be between 0 and 127.\n\nAre you two talking about the return value from cmd_config()?\n\ngit.c::run_builtin() already knows what to do with status codes from the\nbuiltins to protect you from (rare) shells that do not cope with a\nnegative return that come from the common pattern of doing:\n\n\treturn error(\"it is wrong in this way\")\n\nSo \"negative\" is not really a problem. See 2488df8 (builtin run_command:\ndo not exit with -1., 2007-11-13).\n\nBut if the old code was doing:\n\n\tret = git_config_set_multivar(...);\n if (ret)\n\t\treturn ret;\n\nand if you are changing it to:\n\n\tret = git_config_set_multivar(...);\n if (ret)\n\t\treturn !!ret;\n\nyou are changing an externally observable behaviour.\n"},{"id":"104809","messageId":"alpine.DEB.1.00.0902152040350.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"7v1vtzmr9j.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-15T19:41:02Z","receivedAt":"2009-02-15T19:41:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 15 Feb 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> > Or maybe even better: set a variable \"ret\" and at the end of \n> >> > cmd_config(), \"return !!ret;\"?\n> >> \n> >> Huh? So git commands don't return negative error values?\n> >\n> > AFAICT an exit status is supposed to be between 0 and 127.\n> \n> Are you two talking about the return value from cmd_config()?\n> \n> git.c::run_builtin() already knows what to do with status codes from the\n> builtins to protect you from (rare) shells that do not cope with a\n> negative return that come from the common pattern of doing:\n> \n> \treturn error(\"it is wrong in this way\")\n> \n> So \"negative\" is not really a problem.\n\nOoops. I missed that.\n\nCiao,\nDscho\n"},{"id":"104822","messageId":"7vzlgnieei.fsf@gitster.siamese.dyndns.org","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902152040350.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-15T21:22:29Z","receivedAt":"2009-02-15T21:22:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sun, 15 Feb 2009, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> >> > Or maybe even better: set a variable \"ret\" and at the end of \n>> >> > cmd_config(), \"return !!ret;\"?\n>> >> \n>> >> Huh? So git commands don't return negative error values?\n>> >\n>> > AFAICT an exit status is supposed to be between 0 and 127.\n>> \n>> Are you two talking about the return value from cmd_config()?\n>> \n>> git.c::run_builtin() already knows what to do with status codes from the\n>> builtins to protect you from (rare) shells that do not cope with a\n>> negative return that come from the common pattern of doing:\n>> \n>> \treturn error(\"it is wrong in this way\")\n>> \n>> So \"negative\" is not really a problem.\n>\n> Ooops. I missed that.\n\nNote that I refrained from using stronger words like \"regression\" on\npurpose, because I do not think any caller tells various error codes that\ncome out of git_config_set_multivar() apart and act differently in\npractice.\n\nBut it does appear that the said function wants to say why the call failed\nwith its return code, and using !!ret to lose information does not feel\nright.\n"},{"id":"104824","messageId":"alpine.DEB.1.00.0902152228530.10279@pacific.mpi-cbg.de","threadId":"17782","inReplyTo":"7vzlgnieei.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-15T21:29:02Z","receivedAt":"2009-02-15T21:29:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 15 Feb 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Sun, 15 Feb 2009, Junio C Hamano wrote:\n> >\n> >> Johannes Schindelin writes:\n> >> \n> >> >> > Or maybe even better: set a variable \"ret\" and at the end of \n> >> >> > cmd_config(), \"return !!ret;\"?\n> >> >> \n> >> >> Huh? So git commands don't return negative error values?\n> >> >\n> >> > AFAICT an exit status is supposed to be between 0 and 127.\n> >> \n> >> Are you two talking about the return value from cmd_config()?\n> >> \n> >> git.c::run_builtin() already knows what to do with status codes from the\n> >> builtins to protect you from (rare) shells that do not cope with a\n> >> negative return that come from the common pattern of doing:\n> >> \n> >> \treturn error(\"it is wrong in this way\")\n> >> \n> >> So \"negative\" is not really a problem.\n> >\n> > Ooops. I missed that.\n> \n> Note that I refrained from using stronger words like \"regression\" on\n> purpose, because I do not think any caller tells various error codes that\n> come out of git_config_set_multivar() apart and act differently in\n> practice.\n> \n> But it does appear that the said function wants to say why the call failed\n> with its return code, and using !!ret to lose information does not feel\n> right.\n\nI fully agree.\n\nCiao,\nDscho\n"},{"id":"105038","messageId":"94a0d4530902161650ld10602dxdb73b78d5d9a6223@mail.gmail.com","threadId":"17782","inReplyTo":"alpine.DEB.1.00.0902152228530.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] config: Use parseopt.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-02-17T00:50:05Z","receivedAt":"2009-02-17T00:50:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Feb 15, 2009 at 11:29 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sun, 15 Feb 2009, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> > On Sun, 15 Feb 2009, Junio C Hamano wrote:\n>> >\n>> >> Johannes Schindelin writes:\n>> >>\n>> >> >> > Or maybe even better: set a variable \"ret\" and at the end of\n>> >> >> > cmd_config(), \"return !!ret;\"?\n>> >> >>\n>> >> >> Huh? So git commands don't return negative error values?\n>> >> >\n>> >> > AFAICT an exit status is supposed to be between 0 and 127.\n>> >>\n>> >> Are you two talking about the return value from cmd_config()?\n>> >>\n>> >> git.c::run_builtin() already knows what to do with status codes from the\n>> >> builtins to protect you from (rare) shells that do not cope with a\n>> >> negative return that come from the common pattern of doing:\n>> >>\n>> >> return error(\"it is wrong in this way\")\n>> >>\n>> >> So \"negative\" is not really a problem.\n>> >\n>> > Ooops. I missed that.\n>>\n>> Note that I refrained from using stronger words like \"regression\" on\n>> purpose, because I do not think any caller tells various error codes that\n>> come out of git_config_set_multivar() apart and act differently in\n>> practice.\n>>\n>> But it does appear that the said function wants to say why the call failed\n>> with its return code, and using !!ret to lose information does not feel\n>> right.\n>\n> I fully agree.\n\nOk, dropping the patch.\n\n-- \nFelipe Contreras\n"}]}