threads / patch / 22648

patchAdd an optional argument for --color options

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

## tl;dr

11 messages between Feb 13, 2010 and Feb 15, 2010. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Mark Lodato· Feb 13, 2010, 22:01 UTC · lore

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(-)
Show changes to 12 files +67 −7

Documentation/diff-options.txt, Documentation/git-branch.txt, Documentation/git-grep.txt, Documentation/git-show-branch.txt, Documentation/technical/api-parse-options.txt, builtin-branch.c, builtin-grep.c, builtin-show-branch.c, color.c, diff.c, parse-options.c, parse-options.h

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
Jeff King· Feb 14, 2010, 06:44 UTC · re: Mark Lodato · lore

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

On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:
Show 7 quoted lines
> 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
Jonathan Nieder· Feb 14, 2010, 12:21 UTC · re: Jeff King · lore

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

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
Mark Lodato· Feb 14, 2010, 14:58 UTC · re: Jeff King · lore

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

On Sun, Feb 14, 2010 at 1:44 AM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
> 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

Jonathan Nieder· Feb 15, 2010, 01:18 UTC · re: Mark Lodato · lore

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

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
Jeff King· Feb 15, 2010, 05:23 UTC · re: Jonathan Nieder · lore

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

On Sun, Feb 14, 2010 at 07:18:04PM -0600, Jonathan Nieder wrote:
Show 7 quoted lines
> > 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
Jonathan Nieder· Feb 15, 2010, 01:23 UTC · re: Mark Lodato · lore

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

Mark Lodato wrote:
Show 5 quoted lines
> 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
Jeff King· Feb 15, 2010, 05:21 UTC · re: Mark Lodato · lore

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

On Sun, Feb 14, 2010 at 09:58:58AM -0500, Mark Lodato wrote:
Show 11 quoted lines
> > 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
Junio C Hamano· Feb 15, 2010, 06:02 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> 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.

Junio C Hamano· Feb 14, 2010, 11:39 UTC · re: Mark Lodato · lore

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

Mark Lodato <lodatom@gmail.com> writes:
Show 17 quoted lines
> 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?

Show 13 quoted lines
> 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?

Show 11 quoted lines
> 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.
Show 16 quoted lines
> 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".

Show 24 quoted lines
> 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.                        
Mark Lodato· Feb 14, 2010, 14:46 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 14, 2010 at 6:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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.)

Show 16 quoted lines
>> 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?

Show 20 quoted lines
>> 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.
Show 11 quoted lines
>> +     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

← back to recent threads