{"thread":{"id":"22648","subject":"[PATCH] Add an optional argument for --color options","startedAt":"2010-02-13T22:01:15Z","lastAt":"2010-02-15T06:02:06Z","messageCount":11,"participants":["Mark Lodato","Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"134497","messageId":"1266098475-21929-1-git-send-email-lodatom@gmail.com","threadId":"22648","inReplyTo":null,"subject":"[PATCH] Add an optional argument for --color options","fromName":"Mark Lodato","fromEmail":"lodatom@gmail.com","sentAt":"2010-02-13T22:01:15Z","receivedAt":"2010-02-13T22:01:15Z","isPatch":true,"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\nIf the argument is not valid for a diff-family program, a completely\nunhelpful usage message is shown.  It seems that all the other diff\noptions silently ignore invalid inputs, so this is consistent.  Perhaps\nthis aspect should be tweaked.\n\nAlso, I was not sure whether to put the option parsing stuff in\nparse-options.[ch] or color.[ch].  It seemed to go better in the former,\nwhich is where I stuck it, but I can move it if the latter is\npreferable.\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..19d8436 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..790ac91 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+\t/* If var is not given, return an error */\n+\tif (!var)\n+\t\treturn -1;\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..110e63b 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 0;\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..20ce6e3 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, \"expects \\\"always\\\", \\\"auto\\\", \"\n+\t\t\t\t\"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":"134520","messageId":"20100214064408.GB20630@coredump.intra.peff.net","threadId":"22648","inReplyTo":"1266098475-21929-1-git-send-email-lodatom@gmail.com","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-14T06:44:08Z","receivedAt":"2010-02-14T06:44:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:\n\n> Make git-branch, git-show-branch, git-grep, and all the diff-based\n> programs accept an optional argument <when> for --color.  The argument\n> is 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\n> the command-line interface consistent with other GNU tools, such as `ls'\n> and `grep', and with the git-config color options.  Note that, without\n> an argument, --color and --no-color work exactly as before.\n\nI think this is a sensible change, and reading over the patch it looks\nfine to me.\n\n> If the argument is not valid for a diff-family program, a completely\n> unhelpful usage message is shown.  It seems that all the other diff\n> options silently ignore invalid inputs, so this is consistent.  Perhaps\n> this aspect should be tweaked.\n\nHmm...the only one I see that silently ignores is \"--submodule=bogus\".\nBut it seems that \"git log -Bfoobar\" fails but does not print a useful\nmessage.  Probably both should be fixed, and your option should follow\nthe same convention as those.\n\n>  Documentation/technical/api-parse-options.txt |   12 ++++++++++++\n\nOoh, api documentation. It is nice to review a patch that is thorough.\n:)\n\nMy only complaint in that respect is that there are no tests.  However,\nI'm not sure we can get a very satisfying test, since the test scripts\nmay or may not have stdout going to a tty.\n\n-Peff\n"},{"id":"134529","messageId":"7vmxzcoxla.fsf@alter.siamese.dyndns.org","threadId":"22648","inReplyTo":"1266098475-21929-1-git-send-email-lodatom@gmail.com","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-14T11:39:29Z","receivedAt":"2010-02-14T11:39:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Lodato <lodatom@gmail.com> writes:\n\n> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n> index 50f9e9a..19d8436 100644\n> --- a/Documentation/technical/api-parse-options.txt\n> +++ b/Documentation/technical/api-parse-options.txt\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\nEverybody else in the vicinity seems to write these like `--something` and\nthis new paragraph uses '+--something+'.  Why be original only to be\ndifferent?  Is the mark-up known to be understood by various versions of\nAsciiDoc people use?\n\n> diff --git a/builtin-show-branch.c b/builtin-show-branch.c\n> index 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\nAn unrelated topic, but we should clean this up.  I thought parseopt users\nshould say [options] in the short description?\n\n> diff --git a/color.c b/color.c\n> index 62977f4..790ac91 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> +\t/* If var is not given, return an error */\n> +\tif (!var)\n> +\t\treturn -1;\n\nThis is not a good comment for more than one reasons:\n\n * The natural callers of this function (i.e. git_config() callback\n   functions) will _never_ give a NULL in var, hence it is obvious that\n   !var is an error.  And you return negative which is the conventional\n   way to signal error from git_config() callback functions.  The comment\n   states the obvious without adding any useful information.\n\n * Worse yet, the callers that deliberately give NULL to this function are\n   your new callers, and for them, !var is not an error condition at all.\n   You expect that the earlier code in the function to switch on the given\n   value and want the control reach this point from your new callers when\n   the user gave you a wrong input.  This is to check if the caller is\n   using a non-standard calling convention, and return -1 upon unknown\n   input only for such callers.\n\n * Even worse, you do not even treat this -1 as an error consistently; one\n   of your new callers takes it as 'dunno--ignore', and the other one\n   issues an error message.\n\n> diff --git a/diff.c b/diff.c\n> index 381cc8d..110e63b 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 0;\n\nEarlier you said \"git diff --blorb\" says \"error: invalid option: --blorb\"\nand that is unhelpful, but I do not understand why that justifies to\nsilently ignore \"git diff --color=bogo\".\n\n> diff --git a/parse-options.c b/parse-options.c\n> index d218122..20ce6e3 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\nmissing SP after colon.\n\n> +\tvalue = git_config_colorbool(NULL, arg, -1);\n> +\tif (value < 0)\n> +\t\treturn opterror(opt, \"expects \\\"always\\\", \\\"auto\\\", \"\n> +\t\t\t\t\"or \\\"never\\\"\", 0);\n\nInstead of breaking a string into two lines, please write it like this:\n\n\t\treturn opterror(opt,\n\t\t\t\"expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\", 0);\n\nso that we can grep.                        \n"},{"id":"134539","messageId":"20100214122118.GA3630@progeny.tock","threadId":"22648","inReplyTo":"20100214064408.GB20630@coredump.intra.peff.net","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-14T12:21:18Z","receivedAt":"2010-02-14T12:21:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:\n\n>> Make git-branch, git-show-branch, git-grep, and all the diff-based\n>> programs accept an optional argument <when> for --color. \n[...]\n> My only complaint in that respect is that there are no tests.  However,\n> I'm not sure we can get a very satisfying test, since the test scripts\n> may or may not have stdout going to a tty.\n\nSee [1]. ;-)\n\nHope that helps,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/139831/focus=139906\n"},{"id":"134541","messageId":"ca433831002140646v57ce6b1dm4d372d07b0220a16@mail.gmail.com","threadId":"22648","inReplyTo":"7vmxzcoxla.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Mark Lodato","fromEmail":"lodatom@gmail.com","sentAt":"2010-02-14T14:46:35Z","receivedAt":"2010-02-14T14:46:35Z","isPatch":true,"sender":{"key":"lodatom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58860?v=4"},"body":"On Sun, Feb 14, 2010 at 6:39 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Mark Lodato <lodatom@gmail.com> writes:\n>>\n>> +`OPT_COLOR_FLAG(short, long, &int_var, description)`::\n>> +     Introduce an option that takes an optional argument that can\n>> +     have one of three values: \"always\", \"never\", or \"auto\".  If the\n>> +     argument is not given, it defaults to \"always\".  The +--no-+ form\n>> +     works like +--long=never+; it cannot take an argument.  If\n>> +     \"always\", set +int_var+ to 1; if \"never\", set +int_var+ to 0; if\n>> +     \"auto\", set +int_var+ to 1 if stdout is a tty or a pager,\n>> +     0 otherwise.\n>> +\n>\n> Everybody else in the vicinity seems to write these like `--something` and\n> this new paragraph uses '+--something+'.  Why be original only to be\n> different?  Is the mark-up known to be understood by various versions of\n> AsciiDoc people use?\n\nIn my version of AsciiDoc (8.4.4), backticks were not rendering\ncorrectly.  Instead they were showing up starting with an opening\nquote and ending with a grave accent.  The same problem was occurring\nin the other paragraphs, too.  I believe the problem occurred whenever\nthere was a single quote on the same line as a backtick: AsciiDoc was\nreading the first backtick to the first single quote as a quoted\nstring, rather than the first backtick to the second backtick as a\nliteral string.  So, I used +'s here to fix it in this paragraph, and\nthen I was going to post a patch to fix the others.    But, after I\nemailed this patch, I realized that the 'html' branch does *not* have\nthis problem.  So, I'll change this to backticks in the next version\nof the patch.\n\n(In case you're wondering: no, there are no single quotes in the above\nparagraph, but there were in earlier versions.  I didn't realize it\nwas a single quote issue until now.)\n\n>> diff --git a/color.c b/color.c\n>> index 62977f4..790ac91 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>>                       goto auto_color;\n>>       }\n>>\n>> +     /* If var is not given, return an error */\n>> +     if (!var)\n>> +             return -1;\n>\n> This is not a good comment for more than one reasons:\n>\n> [...]\n>\n\nYou're right, it's a crappy comment.  Should I try to reword it, or\njust leave it out?\n\n>> diff --git a/diff.c b/diff.c\n>> index 381cc8d..110e63b 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>>               DIFF_OPT_SET(options, FOLLOW_RENAMES);\n>>       else if (!strcmp(arg, \"--color\"))\n>>               DIFF_OPT_SET(options, COLOR_DIFF);\n>> +     else if (!prefixcmp(arg, \"--color=\")) {\n>> +             int value = git_config_colorbool(NULL, arg+8, -1);\n>> +             if (value == 0)\n>> +                     DIFF_OPT_CLR(options, COLOR_DIFF);\n>> +             else if (value > 0)\n>> +                     DIFF_OPT_SET(options, COLOR_DIFF);\n>> +             else\n>> +                     return 0;\n>\n> Earlier you said \"git diff --blorb\" says \"error: invalid option: --blorb\"\n> and that is unhelpful, but I do not understand why that justifies to\n> silently ignore \"git diff --color=bogo\".\n\n\"git diff --color=bogo\" is not silently ignored.  A useless message is\nprinted instead.\n\n$ ./git-diff --color=asdf\nerror: invalid option: --color=asdf\n\nI did this because that's what the surrounding code did.  Instead, I\nwould have preferred\n\n    return error(\"option `color' expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\");\n\nwhich seems to work.  Is this the right way to go?\n\n> [...]\n> missing SP after colon.\n\nOops.  Fixed.\n\n>> +     value = git_config_colorbool(NULL, arg, -1);\n>> +     if (value < 0)\n>> +             return opterror(opt, \"expects \\\"always\\\", \\\"auto\\\", \"\n>> +                             \"or \\\"never\\\"\", 0);\n>\n> Instead of breaking a string into two lines, please write it like this:\n>\n>                return opterror(opt,\n>                        \"expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\", 0);\n>\n> so that we can grep.\n\nHow do you prefer to handle really long lines?  Just let them extend\npast 80 columns?  This is what appears to be the convention, but I\njust want to double check.\n\n\nI have some other color-related patches that I plan to email today.\nThey are all essentially independent, meaning you can accept some but\nnot others, but I will roll them all into a single PATCHv2 thread so\nthat they stay together. That is, unless you'd prefer them to be\nseparate.\n\nThanks for the feedback,\nMark\n"},{"id":"134542","messageId":"ca433831002140658r30aa539fy5480cae8298d6d6c@mail.gmail.com","threadId":"22648","inReplyTo":"20100214064408.GB20630@coredump.intra.peff.net","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Mark Lodato","fromEmail":"lodatom@gmail.com","sentAt":"2010-02-14T14:58:58Z","receivedAt":"2010-02-14T14:58:58Z","isPatch":true,"sender":{"key":"lodatom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58860?v=4"},"body":"On Sun, Feb 14, 2010 at 1:44 AM, Jeff King <peff@peff.net> wrote:\n> On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:\n>\n>> If the argument is not valid for a diff-family program, a completely\n>> unhelpful usage message is shown.  It seems that all the other diff\n>> options silently ignore invalid inputs, so this is consistent.  Perhaps\n>> this aspect should be tweaked.\n>\n> Hmm...the only one I see that silently ignores is \"--submodule=bogus\".\n> But it seems that \"git log -Bfoobar\" fails but does not print a useful\n> message.  Probably both should be fixed, and your option should follow\n> the same convention as those.\n\nJust wondering, why does diff use a separate option parsing mechanism\nthan the rest of the code?  Would it be worthwhile to switch to\nparse_opt?  This may make the code cleaner, and it would definitely\nmake the command-line interface more consistent with the rest of the\nsuite.  From a user's point of view, the biggest win would be \"-h\"\nprinting all of the options, like all the non-diff commands do.\n\n> My only complaint in that respect is that there are no tests.  However,\n> I'm not sure we can get a very satisfying test, since the test scripts\n> may or may not have stdout going to a tty.\n\nPerhaps I can throw the tests in Jonathan's \"tests for automatic use\nof pager\", t7006-pager?  Or, create a new test that mimics his?\n\nThanks for the feedback,\nMark\n"},{"id":"134585","messageId":"20100215011803.GA15966@progeny.tock","threadId":"22648","inReplyTo":"ca433831002140658r30aa539fy5480cae8298d6d6c@mail.gmail.com","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T01:18:04Z","receivedAt":"2010-02-15T01:18:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Mark Lodato wrote:\n\n> Just wondering, why does diff use a separate option parsing mechanism\n> than the rest of the code?  Would it be worthwhile to switch to\n> parse_opt?\n\nHistorical reasons, I think.  And yes. ;-)\n\n> Perhaps I can throw the tests in Jonathan's \"tests for automatic use\n> of pager\", t7006-pager?  Or, create a new test that mimics his?\n\nI would suggest copying whatever functions you need to a new\nlib-terminal.sh and sourcing that with . from a new test.  Then I\ncould adapt t7006-pager to use your library and avoid duplication of\ncode.\n\nI am also interested in feedback on the techniques used in that test.\nShould it just rely on redirects to /dev/tty instead, and work to\navoid sending any actual output there?  Is there an easier way to\ndetect use of color?\n\nJonathan\n"},{"id":"134586","messageId":"20100215012316.GA16643@progeny.tock","threadId":"22648","inReplyTo":"ca433831002140658r30aa539fy5480cae8298d6d6c@mail.gmail.com","subject":"Usage messages produced by parseopt (Re: [PATCH] Add an optional argument for --color options)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-15T01:23:16Z","receivedAt":"2010-02-15T01:23:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Mark Lodato wrote:\n\n> Would it be worthwhile to switch to\n> parse_opt?  This may make the code cleaner, and it would definitely\n> make the command-line interface more consistent with the rest of the\n> suite.  From a user's point of view, the biggest win would be \"-h\"\n> printing all of the options, like all the non-diff commands do.\n\nSide note: I actually prefer the shorter usage messages, since when I\nuse the \"-h\" option, I tend to be just looking for a reminder.  Am I\nalone in this?  Would there be interest in a \"git <whatever>\n--help=short\" option or similar?\n\nJonathan\n"},{"id":"134610","messageId":"20100215052139.GH3336@coredump.intra.peff.net","threadId":"22648","inReplyTo":"ca433831002140658r30aa539fy5480cae8298d6d6c@mail.gmail.com","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T05:21:39Z","receivedAt":"2010-02-15T05:21:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 09:58:58AM -0500, Mark Lodato wrote:\n\n> > Hmm...the only one I see that silently ignores is \"--submodule=bogus\".\n> > But it seems that \"git log -Bfoobar\" fails but does not print a useful\n> > message.  Probably both should be fixed, and your option should follow\n> > the same convention as those.\n> \n> Just wondering, why does diff use a separate option parsing mechanism\n> than the rest of the code?  Would it be worthwhile to switch to\n> parse_opt?  This may make the code cleaner, and it would definitely\n> make the command-line interface more consistent with the rest of the\n> suite.  From a user's point of view, the biggest win would be \"-h\"\n> printing all of the options, like all the non-diff commands do.\n\nIt's historical. The diff option parser predates parse-options by quite\na bit, and was never converted. Pierre made some attempts at converting\nit and the revision parser some time back, and we ended up with the more\niterative approach (you can step through each argument with\nparse-options, and then alternatively feed it to the revision and diff\noptions parser).\n\nI don't remember if there were any technical limitations, though (e.g.,\nplaces where the revision parser does not conform to parse-options\nstandards or needs some special treatment). You'd have to search the\nlist archives to see what actually happened.\n\n-Peff\n"},{"id":"134611","messageId":"20100215052356.GI3336@coredump.intra.peff.net","threadId":"22648","inReplyTo":"20100215011803.GA15966@progeny.tock","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-15T05:23:56Z","receivedAt":"2010-02-15T05:23:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 14, 2010 at 07:18:04PM -0600, Jonathan Nieder wrote:\n\n> > Perhaps I can throw the tests in Jonathan's \"tests for automatic use\n> > of pager\", t7006-pager?  Or, create a new test that mimics his?\n> \n> I would suggest copying whatever functions you need to a new\n> lib-terminal.sh and sourcing that with . from a new test.  Then I\n> could adapt t7006-pager to use your library and avoid duplication of\n> code.\n\nYes, I think that is a reasonable way to go.\n\n> I am also interested in feedback on the techniques used in that test.\n> Should it just rely on redirects to /dev/tty instead, and work to\n> avoid sending any actual output there?  Is there an easier way to\n> detect use of color?\n\nKeep in mind that you might not even have a terminal at all (e.g., tests\nrun from a cron job), so redirecting /dev/tty won't help there. It is\neasy enough to fake, as I posted in the other thread, so I think that is\nprobably simplest (unless we go with the \"it only works under\n--verbose\" scheme).\n\n-Peff\n"},{"id":"134618","messageId":"7vsk93nijl.fsf@alter.siamese.dyndns.org","threadId":"22648","inReplyTo":"20100215052139.GH3336@coredump.intra.peff.net","subject":"Re: [PATCH] Add an optional argument for --color options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-15T06:02:06Z","receivedAt":"2010-02-15T06:02:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It's historical. The diff option parser predates parse-options by quite\n> a bit, and was never converted. Pierre made some attempts at converting\n> it and the revision parser some time back, and we ended up with the more\n> iterative approach (you can step through each argument with\n> parse-options, and then alternatively feed it to the revision and diff\n> options parser).\n>\n> I don't remember if there were any technical limitations,...\n\nI think one of the biggie we didn't solve was what to do with the\ncascading options table (e.g. log family use both diff and revision in\naddition to their own).  The design needs to cover both parsing and also\nthe help text.\n"}]}