{"thread":{"id":"22687","subject":"[PATCHv2] Add an optional argument for --color options","startedAt":"2010-02-17T04:55:58Z","lastAt":"2010-02-18T09:56:44Z","messageCount":2,"participants":["Mark Lodato","Stephen Boyd"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"134836","messageId":"1266382558-26325-1-git-send-email-lodatom@gmail.com","threadId":"22687","inReplyTo":null,"subject":"[PATCHv2] Add an optional argument for --color options","fromName":"Mark Lodato","fromEmail":"lodatom@gmail.com","sentAt":"2010-02-17T04:55:58Z","receivedAt":"2010-02-17T04:55:58Z","isPatch":false,"sender":{"key":"lodatom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58860?v=4"},"body":"Make git-branch, git-show-branch, git-grep, and all the diff-based\nprograms accept an optional argument <when> for --color.  The argument\nis a colorbool: \"always\", \"never\", or \"auto\".  If no argument is given,\n\"always\" is used;  --no-color is an alias for --color=never.  This makes\nthe command-line interface consistent with other GNU tools, such as `ls'\nand `grep', and with the git-config color options.  Note that, without\nan argument, --color and --no-color work exactly as before.\n\nTo implement this, two internal changes were made:\n\n1. Allow the first argument of git_config_colorbool() to be NULL,\n   in which case it returns -1 if the argument isn't \"always\", \"never\",\n   or \"auto\".\n\n2. Add OPT_COLOR_FLAG(), OPT__COLOR(), and parse_opt_color_flag_cb()\n   to the option parsing library.  The callback uses\n   git_config_colorbool(), so color.h is now a dependency\n   of parse-options.c.\n\nSigned-off-by: Mark Lodato <lodatom@gmail.com>\n---\n\nThe first version is in pu, so here's an update that addresses the issues.  It\nstill needs a test.  I'll try to use Jonathan Nieder's pager tests as a start.\n\n Documentation/diff-options.txt                |    4 +++-\n Documentation/git-branch.txt                  |    6 ++++--\n Documentation/git-grep.txt                    |    6 ++++--\n Documentation/git-show-branch.txt             |    6 ++++--\n Documentation/technical/api-parse-options.txt |   12 ++++++++++++\n builtin-branch.c                              |    2 +-\n builtin-grep.c                                |    2 +-\n builtin-show-branch.c                         |    4 ++--\n color.c                                       |    3 +++\n diff.c                                        |    9 +++++++++\n parse-options.c                               |   17 +++++++++++++++++\n parse-options.h                               |    7 +++++++\n 12 files changed, 67 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 8707d0e..60e922e 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -117,12 +117,14 @@ any of those replacements occurred.\n \toption and lists the commits in that commit range like the 'summary'\n \toption of linkgit:git-submodule[1] does.\n \n---color::\n+--color[=<when>]::\n \tShow colored diff.\n+\tThe value must be always (the default), never, or auto.\n \n --no-color::\n \tTurn off colored diff, even when the configuration file\n \tgives the default to color output.\n+\tSame as `--color=never`.\n \n --color-words[=<regex>]::\n \tShow colored word diff, i.e., color words which have changed.\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 6b6c3da..903a690 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -8,7 +8,7 @@ git-branch - List, create, or delete branches\n SYNOPSIS\n --------\n [verse]\n-'git branch' [--color | --no-color] [-r | -a]\n+'git branch' [--color[=<when>] | --no-color] [-r | -a]\n \t[-v [--abbrev=<length> | --no-abbrev]]\n \t[(--merged | --no-merged | --contains) [<commit>]]\n 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]\n@@ -84,12 +84,14 @@ OPTIONS\n -M::\n \tMove/rename a branch even if the new branch name already exists.\n \n---color::\n+--color[=<when>]::\n \tColor branches to highlight current, local, and remote branches.\n+\tThe value must be always (the default), never, or auto.\n \n --no-color::\n \tTurn off branch colors, even when the configuration file gives the\n \tdefault to color output.\n+\tSame as `--color=never`.\n \n -r::\n \tList or delete (if used with -d) the remote-tracking branches.\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex e019e76..70c7ef9 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -18,7 +18,7 @@ SYNOPSIS\n \t   [-z | --null]\n \t   [-c | --count] [--all-match] [-q | --quiet]\n \t   [--max-depth <depth>]\n-\t   [--color | --no-color]\n+\t   [--color[=<when>] | --no-color]\n \t   [-A <post-context>] [-B <pre-context>] [-C <context>]\n \t   [-f <file>] [-e] <pattern>\n \t   [--and|--or|--not|(|)|-e <pattern>...] [<tree>...]\n@@ -111,12 +111,14 @@ OPTIONS\n \tInstead of showing every matched line, show the number of\n \tlines that match.\n \n---color::\n+--color[=<when>]::\n \tShow colored matches.\n+\tThe value must be always (the default), never, or auto.\n \n --no-color::\n \tTurn off match highlighting, even when the configuration file\n \tgives the default to color output.\n+\tSame as `--color=never`.\n \n -[ABC] <context>::\n \tShow `context` trailing (`A` -- after), or leading (`B`\ndiff --git a/Documentation/git-show-branch.txt b/Documentation/git-show-branch.txt\nindex 7343361..519f9e1 100644\n--- a/Documentation/git-show-branch.txt\n+++ b/Documentation/git-show-branch.txt\n@@ -9,7 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git show-branch' [-a|--all] [-r|--remotes] [--topo-order | --date-order]\n-\t\t[--current] [--color | --no-color] [--sparse]\n+\t\t[--current] [--color[=<when>] | --no-color] [--sparse]\n \t\t[--more=<n> | --list | --independent | --merge-base]\n \t\t[--no-name | --sha1-name] [--topics]\n \t\t[<rev> | <glob>]...\n@@ -117,13 +117,15 @@ OPTIONS\n \tWhen no explicit <ref> parameter is given, it defaults to the\n \tcurrent branch (or `HEAD` if it is detached).\n \n---color::\n+--color[=<when>]::\n \tColor the status sign (one of these: `*` `!` `+` `-`) of each commit\n \tcorresponding to the branch it's in.\n+\tThe value must be always (the default), never, or auto.\n \n --no-color::\n \tTurn off colored output, even when the configuration file gives the\n \tdefault to color output.\n+\tSame as `--color=never`.\n \n Note that --more, --list, --independent and --merge-base options\n are mutually exclusive.\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 50f9e9a..312e3b2 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -115,6 +115,9 @@ There are some macros to easily define options:\n `OPT__ABBREV(&int_var)`::\n \tAdd `\\--abbrev[=<n>]`.\n \n+`OPT__COLOR(&int_var, description)`::\n+\tAdd `\\--color[=<when>]` and `--no-color`.\n+\n `OPT__DRY_RUN(&int_var)`::\n \tAdd `-n, \\--dry-run`.\n \n@@ -183,6 +186,15 @@ There are some macros to easily define options:\n \targuments.  Short options that happen to be digits take\n \tprecedence over it.\n \n+`OPT_COLOR_FLAG(short, long, &int_var, description)`::\n+\tIntroduce an option that takes an optional argument that can\n+\thave one of three values: \"always\", \"never\", or \"auto\".  If the\n+\targument is not given, it defaults to \"always\".  The `--no-` form\n+\tworks like `--long=never`; it cannot take an argument.  If\n+\t\"always\", set `int_var` to 1; if \"never\", set `int_var` to 0; if\n+\t\"auto\", set `int_var` to 1 if stdout is a tty or a pager,\n+\t0 otherwise.\n+\n \n The last element of the array must be `OPT_END()`.\n \ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex a28a139..6cf7e72 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -610,7 +610,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tBRANCH_TRACK_EXPLICIT),\n \t\tOPT_SET_INT( 0, \"set-upstream\",  &track, \"change upstream info\",\n \t\t\tBRANCH_TRACK_OVERRIDE),\n-\t\tOPT_BOOLEAN( 0 , \"color\",  &branch_use_color, \"use colored output\"),\n+\t\tOPT__COLOR(&branch_use_color, \"use colored output\"),\n \t\tOPT_SET_INT('r', NULL,     &kinds, \"act on remote-tracking branches\",\n \t\t\tREF_REMOTE_BRANCH),\n \t\t{\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 26d4deb..00cbd90 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -782,7 +782,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t\"print NUL after filenames\"),\n \t\tOPT_BOOLEAN('c', \"count\", &opt.count,\n \t\t\t\"show the number of matches instead of matching lines\"),\n-\t\tOPT_SET_INT(0, \"color\", &opt.color, \"highlight matches\", 1),\n+\t\tOPT__COLOR(&opt.color, \"highlight matches\"),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_CALLBACK('C', NULL, &opt, \"n\",\n \t\t\t\"show <n> context lines before and after matches\",\ndiff --git a/builtin-show-branch.c b/builtin-show-branch.c\nindex 9f13caa..32d862a 100644\n--- a/builtin-show-branch.c\n+++ b/builtin-show-branch.c\n@@ -6,7 +6,7 @@\n #include \"parse-options.h\"\n \n static const char* show_branch_usage[] = {\n-    \"git show-branch [-a|--all] [-r|--remotes] [--topo-order | --date-order] [--current] [--color | --no-color] [--sparse] [--more=<n> | --list | --independent | --merge-base] [--no-name | --sha1-name] [--topics] [<rev> | <glob>]...\",\n+    \"git show-branch [-a|--all] [-r|--remotes] [--topo-order | --date-order] [--current] [--color[=<when>] | --no-color] [--sparse] [--more=<n> | --list | --independent | --merge-base] [--no-name | --sha1-name] [--topics] [<rev> | <glob>]...\",\n     \"git show-branch (-g|--reflog)[=<n>[,<base>]] [--list] [<ref>]\",\n     NULL\n };\n@@ -661,7 +661,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t    \"show remote-tracking and local branches\"),\n \t\tOPT_BOOLEAN('r', \"remotes\", &all_remotes,\n \t\t\t    \"show remote-tracking branches\"),\n-\t\tOPT_BOOLEAN(0, \"color\", &showbranch_use_color,\n+\t\tOPT__COLOR(&showbranch_use_color,\n \t\t\t    \"color '*!+-' corresponding to the branch\"),\n \t\t{ OPTION_INTEGER, 0, \"more\", &extra, \"n\",\n \t\t\t    \"show <n> more commits after the common ancestor\",\ndiff --git a/color.c b/color.c\nindex 62977f4..8f07fc9 100644\n--- a/color.c\n+++ b/color.c\n@@ -138,6 +138,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)\n \t\t\tgoto auto_color;\n \t}\n \n+\tif (!var)\n+\t\treturn -1;\n+\n \t/* Missing or explicit false to turn off colorization */\n \tif (!git_config_bool(var, value))\n \t\treturn 0;\ndiff --git a/diff.c b/diff.c\nindex 381cc8d..8f645f6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2826,6 +2826,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_SET(options, FOLLOW_RENAMES);\n \telse if (!strcmp(arg, \"--color\"))\n \t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\telse if (!prefixcmp(arg, \"--color=\")) {\n+\t\tint value = git_config_colorbool(NULL, arg+8, -1);\n+\t\tif (value == 0)\n+\t\t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n+\t\telse if (value > 0)\n+\t\t\tDIFF_OPT_SET(options, COLOR_DIFF);\n+\t\telse\n+\t\t\treturn error(\"option `color' expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\");\n+\t}\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\tDIFF_OPT_CLR(options, COLOR_DIFF);\n \telse if (!strcmp(arg, \"--color-words\")) {\ndiff --git a/parse-options.c b/parse-options.c\nindex d218122..22da9be 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -2,6 +2,7 @@\n #include \"parse-options.h\"\n #include \"cache.h\"\n #include \"commit.h\"\n+#include \"color.h\"\n \n static int parse_options_usage(const char * const *usagestr,\n \t\t\t       const struct option *opts);\n@@ -599,6 +600,22 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\n+\t\t\t    int unset)\n+{\n+\tint value;\n+\tif (unset && arg)\n+\t\treturn opterror(opt, \"takes no value\", OPT_UNSET);\n+\tif (!arg)\n+\t\targ = unset ? \"never\" : (const char *)opt->defval;\n+\tvalue = git_config_colorbool(NULL, arg, -1);\n+\tif (value < 0)\n+\t\treturn opterror(opt,\n+\t\t\t\"expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\", 0);\n+\t*(int *)opt->value = value;\n+\treturn 0;\n+}\n+\n int parse_opt_verbosity_cb(const struct option *opt, const char *arg,\n \t\t\t   int unset)\n {\ndiff --git a/parse-options.h b/parse-options.h\nindex 0c99691..9429f7e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -135,6 +135,10 @@ struct option {\n \t  PARSE_OPT_NOARG | PARSE_OPT_NONEG, (f) }\n #define OPT_FILENAME(s, l, v, h)    { OPTION_FILENAME, (s), (l), (v), \\\n \t\t\t\t       \"FILE\", (h) }\n+#define OPT_COLOR_FLAG(s, l, v, h) \\\n+\t{ OPTION_CALLBACK, (s), (l), (v), \"when\", (h), PARSE_OPT_OPTARG, \\\n+\t\tparse_opt_color_flag_cb, (intptr_t)\"always\" }\n+\n \n /* parse_options() will filter out the processed options and leave the\n  * non-option arguments in argv[].\n@@ -187,6 +191,7 @@ extern int parse_options_end(struct parse_opt_ctx_t *ctx);\n /*----- some often used options -----*/\n extern int parse_opt_abbrev_cb(const struct option *, const char *, int);\n extern int parse_opt_approxidate_cb(const struct option *, const char *, int);\n+extern int parse_opt_color_flag_cb(const struct option *, const char *, int);\n extern int parse_opt_verbosity_cb(const struct option *, const char *, int);\n extern int parse_opt_with_commit(const struct option *, const char *, int);\n extern int parse_opt_tertiary(const struct option *, const char *, int);\n@@ -203,5 +208,7 @@ extern int parse_opt_tertiary(const struct option *, const char *, int);\n \t{ OPTION_CALLBACK, 0, \"abbrev\", (var), \"n\", \\\n \t  \"use <n> digits to display SHA-1s\", \\\n \t  PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }\n+#define OPT__COLOR(var, h) \\\n+\tOPT_COLOR_FLAG(0, \"color\", (var), (h))\n \n #endif\n-- \n1.7.0\n"},{"id":"134939","messageId":"4B7D0EDC.8070309@gmail.com","threadId":"22687","inReplyTo":"1266382558-26325-1-git-send-email-lodatom@gmail.com","subject":"Re: [PATCHv2] Add an optional argument for --color options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-02-18T09:56:44Z","receivedAt":"2010-02-18T09:56:44Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 02/16/2010 08:55 PM, Mark Lodato wrote:\n> @@ -599,6 +600,22 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n>  \treturn 0;\n>  }\n>  \n> +int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\n> +\t\t\t    int unset)\n> +{\n> +\tint value;\n> +\tif (unset && arg)\n> +\t\treturn opterror(opt, \"takes no value\", OPT_UNSET);\n>   \n\nWe can't have unset and arg both set at the same time. This exact case\nis handled by get_value() in parse-options.c (did you copy that?)\n\n> +\tif (!arg)\n> +\t\targ = unset ? \"never\" : (const char *)opt->defval;\n>   \n\nI've always been annoyed that the OPTION_CALLBACK doesn't set the\noption's value to the defval before calling the callback when it's\nPARSE_OPT_OPTARG. If it did we could drop half of this if condition too.\n\nHere's something to squash\n--->8----\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 22da9be..0986314 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -604,8 +604,6 @@ int parse_opt_color_flag_cb(const struct option *opt, const \n                            int unset)\n {\n        int value;\n-       if (unset && arg)\n-               return opterror(opt, \"takes no value\", OPT_UNSET);\n        if (!arg)\n                arg = unset ? \"never\" : (const char *)opt->defval;\n        value = git_config_colorbool(NULL, arg, -1);\n"}]}