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

Felipe Contreras [off-list ref] writes:

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.

The two use formats with and without is_tty are meant to be used in
scripts this way:

	# without $is_tty
	if git config --get-colorbool color.diff
        then
        	use_color=true
	else
        	use_color=false
	fi

        # with $is_tty
        if test -t 1
        then
        	is_tty=1
	else
        	is_tty=0
	fi
        use_color=$(git config --get-colorbool color.diff $is_tty)

In the latter form, output from the command is captured by $() and does
not see if the stdout of the calling script is a tty, hence the script
feeds that information to the command as an extra parameter.

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