Re: [PATCH B v4 5/5] git config: don't allow --get-color* and variable type

On Sun, Feb 22, 2009 at 7:35 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

quoted

quoted

diff --git a/builtin-config.c b/builtin-config.c
index 8045926..9930568 100644
--- a/builtin-config.c
+++ b/builtin-config.c

@@ -359,6 +359,11 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)

      if (get_colorbool_slot)
          actions |= ACTION_GET_COLORBOOL;

+     if ((get_color_slot || get_colorbool_slot) && types) {
+             error("--get-color and variable type are incoherent");
+             usage_with_options(builtin_config_usage, builtin_config_options);
+     }
+

I do not think I suggested anything like this, so I'd decline to take
credit for this patch.

Strictly speaking, "--bool --get-colorbool diff.color 1" shouldn't error
out, don't you think?  And it certainly shouldn't say "--get-color".

Huh? I misinterpreted:
I see "git config --bool --get-color diff.color.whitespace" is still
allowed, which you might want to tighten further.

"--get-color" gets, escape sequence to throw at the terminal to get the
color configured.  It does not make sense to ask for bool (or int) for
that action.

"--get-colorbool" asks if it is appropriate to use such escape sequence
(e.g. when the output is tty and config says "auto", you would get "please
use color").  In other words, its type is always bool, so using it as
--int does not make sense but we cannot really say using it with --bool is
nonsense.

Now I get it, but why would somebody want to do '--bool
--get-colorbool'? That is redundant.

I think --bool should be used only for boolean variables, not color ones.

Please let me know if the original patch is ok and I'll resend it with
'--get-color*'.

-- 
Felipe Contreras