[PATCH] Add an optional argument for --color options

[PATCH] Add an optional argument for --color options

[Mark Lodato]

Make git-branch, git-show-branch, git-grep, and all the diff-based
programs accept an optional argument <when> for --color.  The argument
is a colorbool: "always", "never", or "auto".  If no argument is given,
"always" is used;  --no-color is an alias for --color=never.  This makes
the command-line interface consistent with other GNU tools, such as `ls'
and `grep', and with the git-config color options.  Note that, without
an argument, --color and --no-color work exactly as before.

To implement this, two internal changes were made:

1. Allow the first argument of git_config_colorbool() to be NULL,
   in which case it returns -1 if the argument isn't "always", "never",
   or "auto".

2. Add OPT_COLOR_FLAG(), OPT__COLOR(), and parse_opt_color_flag_cb()
   to the option parsing library.  The callback uses
   git_config_colorbool(), so color.h is now a dependency
   of parse-options.c.

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---

If the argument is not valid for a diff-family program, a completely
unhelpful usage message is shown.  It seems that all the other diff
options silently ignore invalid inputs, so this is consistent.  Perhaps
this aspect should be tweaked.

Also, I was not sure whether to put the option parsing stuff in
parse-options.[ch] or color.[ch].  It seemed to go better in the former,
which is where I stuck it, but I can move it if the latter is
preferable.

 Documentation/diff-options.txt                |    4 +++-
 Documentation/git-branch.txt                  |    6 ++++--
 Documentation/git-grep.txt                    |    6 ++++--
 Documentation/git-show-branch.txt             |    6 ++++--
 Documentation/technical/api-parse-options.txt |   12 ++++++++++++
 builtin-branch.c                              |    2 +-
 builtin-grep.c                                |    2 +-
 builtin-show-branch.c                         |    4 ++--
 color.c                                       |    3 +++
 diff.c                                        |    9 +++++++++
 parse-options.c                               |   17 +++++++++++++++++
 parse-options.h                               |    7 +++++++
 12 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 8707d0e..60e922e 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -117,12 +117,14 @@ any of those replacements occurred.
 	option and lists the commits in that commit range like the 'summary'
 	option of linkgit:git-submodule[1] does.
 
---color::
+--color[=<when>]::
 	Show colored diff.
+	The value must be always (the default), never, or auto.
 
 --no-color::
 	Turn off colored diff, even when the configuration file
 	gives the default to color output.
+	Same as `--color=never`.
 
 --color-words[=<regex>]::
 	Show colored word diff, i.e., color words which have changed.
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 6b6c3da..903a690 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -8,7 +8,7 @@ git-branch - List, create, or delete branches
 SYNOPSIS
 --------
 [verse]
-'git branch' [--color | --no-color] [-r | -a]
+'git branch' [--color[=<when>] | --no-color] [-r | -a]
 	[-v [--abbrev=<length> | --no-abbrev]]
 	[(--merged | --no-merged | --contains) [<commit>]]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
@@ -84,12 +84,14 @@ OPTIONS
 -M::
 	Move/rename a branch even if the new branch name already exists.
 
---color::
+--color[=<when>]::
 	Color branches to highlight current, local, and remote branches.
+	The value must be always (the default), never, or auto.
 
 --no-color::
 	Turn off branch colors, even when the configuration file gives the
 	default to color output.
+	Same as `--color=never`.
 
 -r::
 	List or delete (if used with -d) the remote-tracking branches.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index e019e76..70c7ef9 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -18,7 +18,7 @@ SYNOPSIS
 	   [-z | --null]
 	   [-c | --count] [--all-match] [-q | --quiet]
 	   [--max-depth <depth>]
-	   [--color | --no-color]
+	   [--color[=<when>] | --no-color]
 	   [-A <post-context>] [-B <pre-context>] [-C <context>]
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...] [<tree>...]
@@ -111,12 +111,14 @@ OPTIONS
 	Instead of showing every matched line, show the number of
 	lines that match.
 
---color::
+--color[=<when>]::
 	Show colored matches.
+	The value must be always (the default), never, or auto.
 
 --no-color::
 	Turn off match highlighting, even when the configuration file
 	gives the default to color output.
+	Same as `--color=never`.
 
 -[ABC] <context>::
 	Show `context` trailing (`A` -- after), or leading (`B`
diff --git a/Documentation/git-show-branch.txt b/Documentation/git-show-branch.txt
index 7343361..519f9e1 100644
--- a/Documentation/git-show-branch.txt
+++ b/Documentation/git-show-branch.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git show-branch' [-a|--all] [-r|--remotes] [--topo-order | --date-order]
-		[--current] [--color | --no-color] [--sparse]
+		[--current] [--color[=<when>] | --no-color] [--sparse]
 		[--more=<n> | --list | --independent | --merge-base]
 		[--no-name | --sha1-name] [--topics]
 		[<rev> | <glob>]...
@@ -117,13 +117,15 @@ OPTIONS
 	When no explicit <ref> parameter is given, it defaults to the
 	current branch (or `HEAD` if it is detached).
 
---color::
+--color[=<when>]::
 	Color the status sign (one of these: `*` `!` `+` `-`) of each commit
 	corresponding to the branch it's in.
+	The value must be always (the default), never, or auto.
 
 --no-color::
 	Turn off colored output, even when the configuration file gives the
 	default to color output.
+	Same as `--color=never`.
 
 Note that --more, --list, --independent and --merge-base options
 are mutually exclusive.
diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 50f9e9a..19d8436 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -115,6 +115,9 @@ There are some macros to easily define options:
 `OPT__ABBREV(&int_var)`::
 	Add `\--abbrev[=<n>]`.
 
+`OPT__COLOR(&int_var, description)`::
+	Add `\--color[=<when>]` and `--no-color`.
+
 `OPT__DRY_RUN(&int_var)`::
 	Add `-n, \--dry-run`.
 
@@ -183,6 +186,15 @@ There are some macros to easily define options:
 	arguments.  Short options that happen to be digits take
 	precedence over it.
 
+`OPT_COLOR_FLAG(short, long, &int_var, description)`::
+	Introduce an option that takes an optional argument that can
+	have one of three values: "always", "never", or "auto".  If the
+	argument is not given, it defaults to "always".  The +--no-+ form
+	works like +--long=never+; it cannot take an argument.  If
+	"always", set +int_var+ to 1; if "never", set +int_var+ to 0; if
+	"auto", set +int_var+ to 1 if stdout is a tty or a pager,
+	0 otherwise.
+
 
 The last element of the array must be `OPT_END()`.
 
diff --git a/builtin-branch.c b/builtin-branch.c
index a28a139..6cf7e72 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -610,7 +610,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			BRANCH_TRACK_EXPLICIT),
 		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
 			BRANCH_TRACK_OVERRIDE),
-		OPT_BOOLEAN( 0 , "color",  &branch_use_color, "use colored output"),
+		OPT__COLOR(&branch_use_color, "use colored output"),
 		OPT_SET_INT('r', NULL,     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
 		{
diff --git a/builtin-grep.c b/builtin-grep.c
index 26d4deb..00cbd90 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -782,7 +782,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			"print NUL after filenames"),
 		OPT_BOOLEAN('c', "count", &opt.count,
 			"show the number of matches instead of matching lines"),
-		OPT_SET_INT(0, "color", &opt.color, "highlight matches", 1),
+		OPT__COLOR(&opt.color, "highlight matches"),
 		OPT_GROUP(""),
 		OPT_CALLBACK('C', NULL, &opt, "n",
 			"show <n> context lines before and after matches",
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 9f13caa..32d862a 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -6,7 +6,7 @@
 #include "parse-options.h"
 
 static const char* show_branch_usage[] = {
-    "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>]...",
+    "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>]...",
     "git show-branch (-g|--reflog)[=<n>[,<base>]] [--list] [<ref>]",
     NULL
 };
@@ -661,7 +661,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			    "show remote-tracking and local branches"),
 		OPT_BOOLEAN('r', "remotes", &all_remotes,
 			    "show remote-tracking branches"),
-		OPT_BOOLEAN(0, "color", &showbranch_use_color,
+		OPT__COLOR(&showbranch_use_color,
 			    "color '*!+-' corresponding to the branch"),
 		{ OPTION_INTEGER, 0, "more", &extra, "n",
 			    "show <n> more commits after the common ancestor",
diff --git a/color.c b/color.c
index 62977f4..790ac91 100644
--- a/color.c
+++ b/color.c
@@ -138,6 +138,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 			goto auto_color;
 	}
 
+	/* If var is not given, return an error */
+	if (!var)
+		return -1;
 	/* Missing or explicit false to turn off colorization */
 	if (!git_config_bool(var, value))
 		return 0;
diff --git a/diff.c b/diff.c
index 381cc8d..110e63b 100644
--- a/diff.c
+++ b/diff.c
@@ -2826,6 +2826,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_SET(options, FOLLOW_RENAMES);
 	else if (!strcmp(arg, "--color"))
 		DIFF_OPT_SET(options, COLOR_DIFF);
+	else if (!prefixcmp(arg, "--color=")) {
+		int value = git_config_colorbool(NULL, arg+8, -1);
+		if (value == 0)
+			DIFF_OPT_CLR(options, COLOR_DIFF);
+		else if (value > 0)
+			DIFF_OPT_SET(options, COLOR_DIFF);
+		else
+			return 0;
+	}
 	else if (!strcmp(arg, "--no-color"))
 		DIFF_OPT_CLR(options, COLOR_DIFF);
 	else if (!strcmp(arg, "--color-words")) {
diff --git a/parse-options.c b/parse-options.c
index d218122..20ce6e3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -2,6 +2,7 @@
 #include "parse-options.h"
 #include "cache.h"
 #include "commit.h"
+#include "color.h"
 
 static int parse_options_usage(const char * const *usagestr,
 			       const struct option *opts);
@@ -599,6 +600,22 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
 	return 0;
 }
 
+int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
+			    int unset)
+{
+	int value;
+	if (unset && arg)
+		return opterror(opt, "takes no value", OPT_UNSET);
+	if (!arg)
+		arg = unset ? "never" :(const char *)opt->defval;
+	value = git_config_colorbool(NULL, arg, -1);
+	if (value < 0)
+		return opterror(opt, "expects \"always\", \"auto\", "
+				"or \"never\"", 0);
+	*(int *)opt->value = value;
+	return 0;
+}
+
 int parse_opt_verbosity_cb(const struct option *opt, const char *arg,
 			   int unset)
 {
diff --git a/parse-options.h b/parse-options.h
index 0c99691..9429f7e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -135,6 +135,10 @@ struct option {
 	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, (f) }
 #define OPT_FILENAME(s, l, v, h)    { OPTION_FILENAME, (s), (l), (v), \
 				       "FILE", (h) }
+#define OPT_COLOR_FLAG(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), "when", (h), PARSE_OPT_OPTARG, \
+		parse_opt_color_flag_cb, (intptr_t)"always" }
+
 
 /* parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[].
@@ -187,6 +191,7 @@ extern int parse_options_end(struct parse_opt_ctx_t *ctx);
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
 extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
+extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
 extern int parse_opt_with_commit(const struct option *, const char *, int);
 extern int parse_opt_tertiary(const struct option *, const char *, int);
@@ -203,5 +208,7 @@ extern int parse_opt_tertiary(const struct option *, const char *, int);
 	{ OPTION_CALLBACK, 0, "abbrev", (var), "n", \
 	  "use <n> digits to display SHA-1s", \
 	  PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }
+#define OPT__COLOR(var, h) \
+	OPT_COLOR_FLAG(0, "color", (var), (h))
 
 #endif
-- 
1.7.0

Re: [PATCH] Add an optional argument for --color options

[Jeff King]

On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:

> Make git-branch, git-show-branch, git-grep, and all the diff-based
> programs accept an optional argument <when> for --color.  The argument
> is a colorbool: "always", "never", or "auto".  If no argument is given,
> "always" is used;  --no-color is an alias for --color=never.  This makes
> the command-line interface consistent with other GNU tools, such as `ls'
> and `grep', and with the git-config color options.  Note that, without
> an argument, --color and --no-color work exactly as before.

I think this is a sensible change, and reading over the patch it looks
fine to me.

> If the argument is not valid for a diff-family program, a completely
> unhelpful usage message is shown.  It seems that all the other diff
> options silently ignore invalid inputs, so this is consistent.  Perhaps
> this aspect should be tweaked.

Hmm...the only one I see that silently ignores is "--submodule=bogus".
But it seems that "git log -Bfoobar" fails but does not print a useful
message.  Probably both should be fixed, and your option should follow
the same convention as those.

>  Documentation/technical/api-parse-options.txt |   12 ++++++++++++

Ooh, api documentation. It is nice to review a patch that is thorough.
:)

My only complaint in that respect is that there are no tests.  However,
I'm not sure we can get a very satisfying test, since the test scripts
may or may not have stdout going to a tty.

-Peff

Re: [PATCH] Add an optional argument for --color options

[Jonathan Nieder]

Jeff King wrote:
> On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:

>> Make git-branch, git-show-branch, git-grep, and all the diff-based
>> programs accept an optional argument <when> for --color. 
[...]
> My only complaint in that respect is that there are no tests.  However,
> I'm not sure we can get a very satisfying test, since the test scripts
> may or may not have stdout going to a tty.

See [1]. ;-)

Hope that helps,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/139831/focus=139906

Re: [PATCH] Add an optional argument for --color options

[Mark Lodato]

On Sun, Feb 14, 2010 at 1:44 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:
>
>> If the argument is not valid for a diff-family program, a completely
>> unhelpful usage message is shown.  It seems that all the other diff
>> options silently ignore invalid inputs, so this is consistent.  Perhaps
>> this aspect should be tweaked.
>
> Hmm...the only one I see that silently ignores is "--submodule=bogus".
> But it seems that "git log -Bfoobar" fails but does not print a useful
> message.  Probably both should be fixed, and your option should follow
> the same convention as those.

Just wondering, why does diff use a separate option parsing mechanism
than the rest of the code?  Would it be worthwhile to switch to
parse_opt?  This may make the code cleaner, and it would definitely
make the command-line interface more consistent with the rest of the
suite.  From a user's point of view, the biggest win would be "-h"
printing all of the options, like all the non-diff commands do.

> My only complaint in that respect is that there are no tests.  However,
> I'm not sure we can get a very satisfying test, since the test scripts
> may or may not have stdout going to a tty.

Perhaps I can throw the tests in Jonathan's "tests for automatic use
of pager", t7006-pager?  Or, create a new test that mimics his?

Thanks for the feedback,
Mark

Re: [PATCH] Add an optional argument for --color options

[Jonathan Nieder]

Mark Lodato wrote:

> Just wondering, why does diff use a separate option parsing mechanism
> than the rest of the code?  Would it be worthwhile to switch to
> parse_opt?

Historical reasons, I think.  And yes. ;-)

> Perhaps I can throw the tests in Jonathan's "tests for automatic use
> of pager", t7006-pager?  Or, create a new test that mimics his?

I would suggest copying whatever functions you need to a new
lib-terminal.sh and sourcing that with . from a new test.  Then I
could adapt t7006-pager to use your library and avoid duplication of
code.

I am also interested in feedback on the techniques used in that test.
Should it just rely on redirects to /dev/tty instead, and work to
avoid sending any actual output there?  Is there an easier way to
detect use of color?

Jonathan

Re: [PATCH] Add an optional argument for --color options

[Jeff King]

On Sun, Feb 14, 2010 at 07:18:04PM -0600, Jonathan Nieder wrote:

> > Perhaps I can throw the tests in Jonathan's "tests for automatic use
> > of pager", t7006-pager?  Or, create a new test that mimics his?
> 
> I would suggest copying whatever functions you need to a new
> lib-terminal.sh and sourcing that with . from a new test.  Then I
> could adapt t7006-pager to use your library and avoid duplication of
> code.

Yes, I think that is a reasonable way to go.

> I am also interested in feedback on the techniques used in that test.
> Should it just rely on redirects to /dev/tty instead, and work to
> avoid sending any actual output there?  Is there an easier way to
> detect use of color?

Keep in mind that you might not even have a terminal at all (e.g., tests
run from a cron job), so redirecting /dev/tty won't help there. It is
easy enough to fake, as I posted in the other thread, so I think that is
probably simplest (unless we go with the "it only works under
--verbose" scheme).

-Peff

Usage messages produced by parseopt (Re: [PATCH] Add an optional argument for --color options)

[Jonathan Nieder]

Mark Lodato wrote:

> Would it be worthwhile to switch to
> parse_opt?  This may make the code cleaner, and it would definitely
> make the command-line interface more consistent with the rest of the
> suite.  From a user's point of view, the biggest win would be "-h"
> printing all of the options, like all the non-diff commands do.

Side note: I actually prefer the shorter usage messages, since when I
use the "-h" option, I tend to be just looking for a reminder.  Am I
alone in this?  Would there be interest in a "git <whatever>
--help=short" option or similar?

Jonathan

Re: [PATCH] Add an optional argument for --color options

[Jeff King]

On Sun, Feb 14, 2010 at 09:58:58AM -0500, Mark Lodato wrote:

> > Hmm...the only one I see that silently ignores is "--submodule=bogus".
> > But it seems that "git log -Bfoobar" fails but does not print a useful
> > message.  Probably both should be fixed, and your option should follow
> > the same convention as those.
> 
> Just wondering, why does diff use a separate option parsing mechanism
> than the rest of the code?  Would it be worthwhile to switch to
> parse_opt?  This may make the code cleaner, and it would definitely
> make the command-line interface more consistent with the rest of the
> suite.  From a user's point of view, the biggest win would be "-h"
> printing all of the options, like all the non-diff commands do.

It's historical. The diff option parser predates parse-options by quite
a bit, and was never converted. Pierre made some attempts at converting
it and the revision parser some time back, and we ended up with the more
iterative approach (you can step through each argument with
parse-options, and then alternatively feed it to the revision and diff
options parser).

I don't remember if there were any technical limitations, though (e.g.,
places where the revision parser does not conform to parse-options
standards or needs some special treatment). You'd have to search the
list archives to see what actually happened.

-Peff

Re: [PATCH] Add an optional argument for --color options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It's historical. The diff option parser predates parse-options by quite
> a bit, and was never converted. Pierre made some attempts at converting
> it and the revision parser some time back, and we ended up with the more
> iterative approach (you can step through each argument with
> parse-options, and then alternatively feed it to the revision and diff
> options parser).
>
> I don't remember if there were any technical limitations,...

I think one of the biggie we didn't solve was what to do with the
cascading options table (e.g. log family use both diff and revision in
addition to their own).  The design needs to cover both parsing and also
the help text.

Re: [PATCH] Add an optional argument for --color options

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> index 50f9e9a..19d8436 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -183,6 +186,15 @@ There are some macros to easily define options:
>  	arguments.  Short options that happen to be digits take
>  	precedence over it.
>  
> +`OPT_COLOR_FLAG(short, long, &int_var, description)`::
> +	Introduce an option that takes an optional argument that can
> +	have one of three values: "always", "never", or "auto".  If the
> +	argument is not given, it defaults to "always".  The +--no-+ form
> +	works like +--long=never+; it cannot take an argument.  If
> +	"always", set +int_var+ to 1; if "never", set +int_var+ to 0; if
> +	"auto", set +int_var+ to 1 if stdout is a tty or a pager,
> +	0 otherwise.
> +

Everybody else in the vicinity seems to write these like `--something` and
this new paragraph uses '+--something+'.  Why be original only to be
different?  Is the mark-up known to be understood by various versions of
AsciiDoc people use?

> diff --git a/builtin-show-branch.c b/builtin-show-branch.c
> index 9f13caa..32d862a 100644
> --- a/builtin-show-branch.c
> +++ b/builtin-show-branch.c
> @@ -6,7 +6,7 @@
>  #include "parse-options.h"
>  
>  static const char* show_branch_usage[] = {
> -    "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>]...",
> +    "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>]...",
>      "git show-branch (-g|--reflog)[=<n>[,<base>]] [--list] [<ref>]",
>      NULL
>  };

An unrelated topic, but we should clean this up.  I thought parseopt users
should say [options] in the short description?

> diff --git a/color.c b/color.c
> index 62977f4..790ac91 100644
> --- a/color.c
> +++ b/color.c
> @@ -138,6 +138,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
>  			goto auto_color;
>  	}
>  
> +	/* If var is not given, return an error */
> +	if (!var)
> +		return -1;

This is not a good comment for more than one reasons:

 * The natural callers of this function (i.e. git_config() callback
   functions) will _never_ give a NULL in var, hence it is obvious that
   !var is an error.  And you return negative which is the conventional
   way to signal error from git_config() callback functions.  The comment
   states the obvious without adding any useful information.

 * Worse yet, the callers that deliberately give NULL to this function are
   your new callers, and for them, !var is not an error condition at all.
   You expect that the earlier code in the function to switch on the given
   value and want the control reach this point from your new callers when
   the user gave you a wrong input.  This is to check if the caller is
   using a non-standard calling convention, and return -1 upon unknown
   input only for such callers.

 * Even worse, you do not even treat this -1 as an error consistently; one
   of your new callers takes it as 'dunno--ignore', and the other one
   issues an error message.

> diff --git a/diff.c b/diff.c
> index 381cc8d..110e63b 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2826,6 +2826,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>  		DIFF_OPT_SET(options, FOLLOW_RENAMES);
>  	else if (!strcmp(arg, "--color"))
>  		DIFF_OPT_SET(options, COLOR_DIFF);
> +	else if (!prefixcmp(arg, "--color=")) {
> +		int value = git_config_colorbool(NULL, arg+8, -1);
> +		if (value == 0)
> +			DIFF_OPT_CLR(options, COLOR_DIFF);
> +		else if (value > 0)
> +			DIFF_OPT_SET(options, COLOR_DIFF);
> +		else
> +			return 0;

Earlier you said "git diff --blorb" says "error: invalid option: --blorb"
and that is unhelpful, but I do not understand why that justifies to
silently ignore "git diff --color=bogo".

> diff --git a/parse-options.c b/parse-options.c
> index d218122..20ce6e3 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -2,6 +2,7 @@
>  #include "parse-options.h"
>  #include "cache.h"
>  #include "commit.h"
> +#include "color.h"
>  
>  static int parse_options_usage(const char * const *usagestr,
>  			       const struct option *opts);
> @@ -599,6 +600,22 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
> +			    int unset)
> +{
> +	int value;
> +	if (unset && arg)
> +		return opterror(opt, "takes no value", OPT_UNSET);
> +	if (!arg)
> +		arg = unset ? "never" :(const char *)opt->defval;

missing SP after colon.

> +	value = git_config_colorbool(NULL, arg, -1);
> +	if (value < 0)
> +		return opterror(opt, "expects \"always\", \"auto\", "
> +				"or \"never\"", 0);

Instead of breaking a string into two lines, please write it like this:

		return opterror(opt,
			"expects \"always\", \"auto\", or \"never\"", 0);

so that we can grep.                        

Re: [PATCH] Add an optional argument for --color options

[Mark Lodato]

On Sun, Feb 14, 2010 at 6:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Mark Lodato <lodatom@gmail.com> writes:
>>
>> +`OPT_COLOR_FLAG(short, long, &int_var, description)`::
>> +     Introduce an option that takes an optional argument that can
>> +     have one of three values: "always", "never", or "auto".  If the
>> +     argument is not given, it defaults to "always".  The +--no-+ form
>> +     works like +--long=never+; it cannot take an argument.  If
>> +     "always", set +int_var+ to 1; if "never", set +int_var+ to 0; if
>> +     "auto", set +int_var+ to 1 if stdout is a tty or a pager,
>> +     0 otherwise.
>> +
>
> Everybody else in the vicinity seems to write these like `--something` and
> this new paragraph uses '+--something+'.  Why be original only to be
> different?  Is the mark-up known to be understood by various versions of
> AsciiDoc people use?

In my version of AsciiDoc (8.4.4), backticks were not rendering
correctly.  Instead they were showing up starting with an opening
quote and ending with a grave accent.  The same problem was occurring
in the other paragraphs, too.  I believe the problem occurred whenever
there was a single quote on the same line as a backtick: AsciiDoc was
reading the first backtick to the first single quote as a quoted
string, rather than the first backtick to the second backtick as a
literal string.  So, I used +'s here to fix it in this paragraph, and
then I was going to post a patch to fix the others.    But, after I
emailed this patch, I realized that the 'html' branch does *not* have
this problem.  So, I'll change this to backticks in the next version
of the patch.

(In case you're wondering: no, there are no single quotes in the above
paragraph, but there were in earlier versions.  I didn't realize it
was a single quote issue until now.)

>> diff --git a/color.c b/color.c
>> index 62977f4..790ac91 100644
>> --- a/color.c
>> +++ b/color.c
>> @@ -138,6 +138,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
>>                       goto auto_color;
>>       }
>>
>> +     /* If var is not given, return an error */
>> +     if (!var)
>> +             return -1;
>
> This is not a good comment for more than one reasons:
>
> [...]
>

You're right, it's a crappy comment.  Should I try to reword it, or
just leave it out?

>> diff --git a/diff.c b/diff.c
>> index 381cc8d..110e63b 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2826,6 +2826,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>>               DIFF_OPT_SET(options, FOLLOW_RENAMES);
>>       else if (!strcmp(arg, "--color"))
>>               DIFF_OPT_SET(options, COLOR_DIFF);
>> +     else if (!prefixcmp(arg, "--color=")) {
>> +             int value = git_config_colorbool(NULL, arg+8, -1);
>> +             if (value == 0)
>> +                     DIFF_OPT_CLR(options, COLOR_DIFF);
>> +             else if (value > 0)
>> +                     DIFF_OPT_SET(options, COLOR_DIFF);
>> +             else
>> +                     return 0;
>
> Earlier you said "git diff --blorb" says "error: invalid option: --blorb"
> and that is unhelpful, but I do not understand why that justifies to
> silently ignore "git diff --color=bogo".

"git diff --color=bogo" is not silently ignored.  A useless message is
printed instead.

$ ./git-diff --color=asdf
error: invalid option: --color=asdf

I did this because that's what the surrounding code did.  Instead, I
would have preferred

    return error("option `color' expects \"always\", \"auto\", or \"never\"");

which seems to work.  Is this the right way to go?

> [...]
> missing SP after colon.

Oops.  Fixed.

>> +     value = git_config_colorbool(NULL, arg, -1);
>> +     if (value < 0)
>> +             return opterror(opt, "expects \"always\", \"auto\", "
>> +                             "or \"never\"", 0);
>
> Instead of breaking a string into two lines, please write it like this:
>
>                return opterror(opt,
>                        "expects \"always\", \"auto\", or \"never\"", 0);
>
> so that we can grep.

How do you prefer to handle really long lines?  Just let them extend
past 80 columns?  This is what appears to be the convention, but I
just want to double check.


I have some other color-related patches that I plan to email today.
They are all essentially independent, meaning you can accept some but
not others, but I will roll them all into a single PATCHv2 thread so
that they stay together. That is, unless you'd prefer them to be
separate.

Thanks for the feedback,
Mark