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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2010, 11:39 UTC
Message-ID: <7vmxzcoxla.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1266098475-21929-1-git-send-email-lodatom@gmail.com>

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.