ANSI sequences produced on non-ANSI terminal

ANSI sequences produced on non-ANSI terminal

[The Grey Wolf]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
	Logged on to a hard terminal and ran 'git status .' and
	'git pull'.

What did you expect to happen? (Expected behavior)
	I expected that colour sequences would not be output, or at least
	not hardcoded to ANSI standard.  In the case of 'pull', I expected
	that the stats would be just printed using <string>\r.

What happened instead? (Actual behavior)
	I got escape sequences that made the output unreadable.

What's different between what you expected and what actually happened?
	One produces mangled ouptut and the other doesn't.

Anything else you want to add:
	I searched google and the documentation as best I was able for
	this, but I am unable to find anywhere that will let me disable
	(or enable) colour for a particular term type.  Sometimes I'm on
	an xterm, for which this is GREAT.  Sometimes I'm on a Wyse WY60,
	for which this is sub-optimal.  My workaround is to disable colour
	completely, which is reluctantly acceptable, but it would be nice
	to say "If I'm on an xterm/aterm/urxvt/ansi terminal, enable
	colour or cursor-positioning, otherwise shut it off."  If this
	seems too much of a one-off to handle, fine, but most things that
	talk fancy to screens are kind enough to allow an opt-out based on
	terminal type. :)

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.32.0
cpu: amd64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: NetBSD 9.99.88 NetBSD 9.99.88 (EDDIE) #16: Tue Aug 31 19:14:47 PDT 2021  greywolf@eddie.starwolf.com:/sys/arch/amd64/compile/EDDIE amd64
compiler info: gnuc: 10.3
libc info: no libc information available (actually there's a LOT of it,
	but I'm not sure you really want it -- please let me know if you do).
$SHELL (typically, interactive shell): /bin/bash
$TERM: wy60

[Enabled Hooks]
	I don't know enough about git yet to use these.

Re: ANSI sequences produced on non-ANSI terminal

[Jeff King]

On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:

> Anything else you want to add:
> 	I searched google and the documentation as best I was able for
> 	this, but I am unable to find anywhere that will let me disable
> 	(or enable) colour for a particular term type.  Sometimes I'm on
> 	an xterm, for which this is GREAT.  Sometimes I'm on a Wyse WY60,
> 	for which this is sub-optimal.  My workaround is to disable colour
> 	completely, which is reluctantly acceptable, but it would be nice
> 	to say "If I'm on an xterm/aterm/urxvt/ansi terminal, enable
> 	colour or cursor-positioning, otherwise shut it off."  If this
> 	seems too much of a one-off to handle, fine, but most things that
> 	talk fancy to screens are kind enough to allow an opt-out based on
> 	terminal type. :)

Git doesn't have any kind of list of terminals, beyond knowing that
"dumb" should disable auto-color. It's possible we could expand that if
there are known terminals that don't understand ANSI colors. I'm a bit
wary of having a laundry list of obscure terminals, though.

If we built against ncurses or some other terminfo-aware library we
could outsource that, but that would be a new dependency. I'm hesitant
to do that even as an optional dependency given the bang-for-the-buck
(and certainly making it require would be right out).

Obviously you can wrap Git with a script to tweak the config based on
the current setting of the $TERM variable. It would be nice if you could
have conditional config for that. E.g., something like:

  [includeIf "env:TERM==xterm"]
  path = gitconfig-color

That doesn't exist, but would fit in reasonably well with our other
conditional config options.

As far as generating non-ANSI codes, that's all Git knows how to do. I'm
not sure what kind of color codes your terminal might support. It
_might_ be possible to support multiple, but from my knowledge of Git's
color code I suspect it would be quite ugly. You'd probably be better
off post-processing the ANSI codes. You can do so automatically-ish with
something like:

  git config pager.log 'convert-ansi-to-whatever | less'

I don't know offhand of anything that would do such conversion out of
the box, but you could probably built it around tput or a terminfo
library.

Note that we do similar post-processing on Windows, albeit internally
by intercepting fprintf, etc (yuck). See compat/winansi.c, which might
give you some logic which would be reused.

-Peff

Re: ANSI sequences produced on non-ANSI terminal

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:
>
>> Anything else you want to add:
>> 	I searched google and the documentation as best I was able for
>> 	this, but I am unable to find anywhere that will let me disable
>> 	(or enable) colour for a particular term type.  Sometimes I'm on
>> 	an xterm, for which this is GREAT.  Sometimes I'm on a Wyse WY60,
>> 	for which this is sub-optimal.  My workaround is to disable colour
>> 	completely, which is reluctantly acceptable, but it would be nice
>> 	to say "If I'm on an xterm/aterm/urxvt/ansi terminal, enable
>> 	colour or cursor-positioning, otherwise shut it off."  If this
>> 	seems too much of a one-off to handle, fine, but most things that
>> 	talk fancy to screens are kind enough to allow an opt-out based on
>> 	terminal type. :)
>
> Git doesn't have any kind of list of terminals, beyond knowing that
> "dumb" should disable auto-color. It's possible we could expand that if
> there are known terminals that don't understand ANSI colors. I'm a bit
> wary of having a laundry list of obscure terminals, though.
>
> If we built against ncurses or some other terminfo-aware library we
> could outsource that, but that would be a new dependency. I'm hesitant
> to do that even as an optional dependency given the bang-for-the-buck
> (and certainly making it require would be right out).

I was wondering if Gray Wolf can run screen on the Wyse, and then
wouldn't git see TERM=screen which is pretty much ANSI if I am not
mistaken ;-)?

RE: ANSI sequences produced on non-ANSI terminal

[Randall S. Becker]

On September 23, 2021 5:55 PM, Junio C Hamano wrote:
>Jeff King <peff@peff.net> writes:
>
>> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:
>>
>>> Anything else you want to add:
>>> 	I searched google and the documentation as best I was able for
>>> 	this, but I am unable to find anywhere that will let me disable
>>> 	(or enable) colour for a particular term type.  Sometimes I'm on
>>> 	an xterm, for which this is GREAT.  Sometimes I'm on a Wyse WY60,
>>> 	for which this is sub-optimal.  My workaround is to disable colour
>>> 	completely, which is reluctantly acceptable, but it would be nice
>>> 	to say "If I'm on an xterm/aterm/urxvt/ansi terminal, enable
>>> 	colour or cursor-positioning, otherwise shut it off."  If this
>>> 	seems too much of a one-off to handle, fine, but most things that
>>> 	talk fancy to screens are kind enough to allow an opt-out based on
>>> 	terminal type. :)
>>
>> Git doesn't have any kind of list of terminals, beyond knowing that
>> "dumb" should disable auto-color. It's possible we could expand that
>> if there are known terminals that don't understand ANSI colors. I'm a
>> bit wary of having a laundry list of obscure terminals, though.
>>
>> If we built against ncurses or some other terminfo-aware library we
>> could outsource that, but that would be a new dependency. I'm hesitant
>> to do that even as an optional dependency given the bang-for-the-buck
>> (and certainly making it require would be right out).
>
>I was wondering if Gray Wolf can run screen on the Wyse, and then wouldn't git see TERM=screen which is pretty much ANSI if I am
not
>mistaken ;-)?

Would something like switch in .gitconfig make a difference? Like core.colourize=false. There are situations where SSH sessions come
in from automation, like Jenkins and Travis, which sets term to something other than dumb by default. Coloring makes a mess of the
output. The ability to turn off colouring off by user might be helpful.

-Randall

Re: ANSI sequences produced on non-ANSI terminal

[Kevin Daudt]

On Thu, Sep 23, 2021 at 06:04:20PM -0400, Randall S. Becker wrote:
> On September 23, 2021 5:55 PM, Junio C Hamano wrote:
> >Jeff King <peff@peff.net> writes:
> >
> >> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:
> >>
> >>> Anything else you want to add:
> >>> 	I searched google and the documentation as best I was able for
> >>> 	this, but I am unable to find anywhere that will let me disable
> >>> 	(or enable) colour for a particular term type.  Sometimes I'm on
> >>> 	an xterm, for which this is GREAT.  Sometimes I'm on a Wyse WY60,
> >>> 	for which this is sub-optimal.  My workaround is to disable colour
> >>> 	completely, which is reluctantly acceptable, but it would be nice
> >>> 	to say "If I'm on an xterm/aterm/urxvt/ansi terminal, enable
> >>> 	colour or cursor-positioning, otherwise shut it off."  If this
> >>> 	seems too much of a one-off to handle, fine, but most things that
> >>> 	talk fancy to screens are kind enough to allow an opt-out based on
> >>> 	terminal type. :)
> >>
> >> Git doesn't have any kind of list of terminals, beyond knowing that
> >> "dumb" should disable auto-color. It's possible we could expand that
> >> if there are known terminals that don't understand ANSI colors. I'm a
> >> bit wary of having a laundry list of obscure terminals, though.
> >>
> >> If we built against ncurses or some other terminfo-aware library we
> >> could outsource that, but that would be a new dependency. I'm hesitant
> >> to do that even as an optional dependency given the bang-for-the-buck
> >> (and certainly making it require would be right out).
> >
> >I was wondering if Gray Wolf can run screen on the Wyse, and then wouldn't git see TERM=screen which is pretty much ANSI if I am
> not
> >mistaken ;-)?
> 
> Would something like switch in .gitconfig make a difference? Like core.colourize=false. There are situations where SSH sessions come
> in from automation, like Jenkins and Travis, which sets term to something other than dumb by default. Coloring makes a mess of the
> output. The ability to turn off colouring off by user might be helpful.
> 
> -Randall
> 

That already exists: `color.ui`:

> This variable determines the default value for variables such as
> color.diff and color.grep that control the use of color per command
> family.
> [..]
> Set it to false or never if you prefer Git commands not to use color
> unless enabled explicitly with some other configuration or the --color
> option.

Kevin

[PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Ævar Arnfjörð Bjarmason]

Add an "includeIf" directive that's conditional on the value of an
environment variable. This has been suggested at least a couple of
times[1][2] as a proposed mechanism to drive dynamic includes, and to
e.g. match based on TERM=xterm in the user's environment.

I initially tried to implement just an "env" keyword, but found that
splitting them up was both easier to implement and to explain to
users. I.e. an "env" will need to handle an optional "=" for matching
the value, and should the semantics be if the variable exists? If it's
boolean?

By splitting them up we present a less ambiguous interface to users,
and make it easy to extend this in the future. I didn't see any point
in implementing a "/i" variant, that only makes sense for "gitdir"'s
matching of FS paths.

I would like syntax that used a "=" better, i.e. "envIs:TERM=xterm"
instead of the "envIs:TERM:xterm" implemented here, but the problem
with that is that it isn't possible to specify those sorts of config
keys on the command-line:

    $ git -c includeIf.someVerb:FOO:BAR.path=bar status --short
    $ git -c includeIf.someVerb:FOO=BAR.path=bar status --short
    error: invalid key: includeIf.someVerb:FOO
    fatal: unable to parse command-line config
    $

I.e. not only isn't the "someVerb" in that scenario not understood by
an older git, but it'll hard error on seeing such a key. See
1ff21c05ba9 (config: store "git -c" variables using more robust
format, 2021-01-12) for a discussion about "=" in config keys. By
picking any other character (e.g. ":") we avoid that whole issue.

1. https://lore.kernel.org/git/YUzvhLUmvsdF5w+r@coredump.intra.peff.net/
2. https://lore.kernel.org/git/X9OB7ek8fVRXUBdK@coredump.intra.peff.net/
3. https://lore.kernel.org/git/87in9ucsbb.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:
>
>> Anything else you want to add:
>>      I searched google and the documentation as best I was able for
>>      this, but I am unable to find anywhere that will let me disable
>>      (or enable) colour for a particular term type.  Sometimes I'm on
>>      an xterm, for which this is GREAT.  Sometimes I'm on a Wyse WY60,
>>      for which this is sub-optimal.  My workaround is to disable colour
>>      completely, which is reluctantly acceptable, but it would be nice
>>      to say "If I'm on an xterm/aterm/urxvt/ansi terminal, enable
>>      colour or cursor-positioning, otherwise shut it off."  If this
>>      seems too much of a one-off to handle, fine, but most things that
>>      talk fancy to screens are kind enough to allow an opt-out based on
>>      terminal type. :)
>
> Git doesn't have any kind of list of terminals, beyond knowing that
> "dumb" should disable auto-color. It's possible we could expand that if
> there are known terminals that don't understand ANSI colors. I'm a bit
> wary of having a laundry list of obscure terminals, though.
>
> If we built against ncurses or some other terminfo-aware library we
> could outsource that, but that would be a new dependency. I'm hesitant
> to do that even as an optional dependency given the bang-for-the-buck
> (and certainly making it require would be right out).
>
> Obviously you can wrap Git with a script to tweak the config based on
> the current setting of the $TERM variable. It would be nice if you could
> have conditional config for that. E.g., something like:
>
>   [includeIf "env:TERM==xterm"]
>   path = gitconfig-color
>
> That doesn't exist, but would fit in reasonably well with our other
> conditional config options.

Perhaps something like this?

 Documentation/config.txt  | 35 ++++++++++++++++++++
 config.c                  | 61 +++++++++++++++++++++++++++++++++--
 t/t1305-config-include.sh | 67 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0c0e6b859f1..58f6d49216d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -159,6 +159,41 @@ all branches that begin with `foo/`. This is useful if your branches are
 organized hierarchically and you would like to apply a configuration to
 all the branches in that hierarchy.
 
+`envExists`::
+	The data that follows the keyword `envExists:` is taken to be the
+	name of an environment variable, e.g. `envExists:NAME`. If the
+	variable (`NAME` in this case) exists the include condition is
+	met.
+
+`envBool`::
+	The data that follows the keyword `envBool:` is taken to be the
+	name of an environment variable, e.g. `envBool:NAME`. If the
+	variable (`NAME` in this case) exists, its value will be
+	checked for boolean truth.
++
+The accepted boolean values are the same as those that `--type bool`
+would accept. A value that's normalized to "true" will satisfy the
+include condition (a nonexisting environment variable is "false").
+
+`envIs`::
+	The data that follows the keyword `envIs:` is taken to be the
+	name of an environment variable, followed by a mandatory ":"
+	character, followed by the expected value of the environment
+	variable. If the value matches the include condition is
+	satisfied.
++
+Values may contain any characters otherwise accepted by the config
+mechanism (including ":").
+
+`envMatch`::
+	Like `envIs`, except that the value is matched using standard
+	globbing wildcards.
+
+There is no way to match an environment variable name with any of the
+`env*` directives if that variable name contains a ":" character. Such
+names are allowed by POSIX, but it is assumed that nobody will need to
+match such a variable name in practice.
+
 A few more notes on matching via `gitdir` and `gitdir/i`:
 
  * Symlinks in `$GIT_DIR` are not resolved before matching.
diff --git a/config.c b/config.c
index 2edf835262f..064059b0d6d 100644
--- a/config.c
+++ b/config.c
@@ -271,6 +271,54 @@ static int include_by_gitdir(const struct config_options *opts,
 	return ret;
 }
 
+static int include_by_env_exists(const char *cond, size_t cond_len)
+{
+	char *cfg = xstrndup(cond, cond_len);
+	int ret = !!getenv(cfg);
+	free(cfg);
+	return ret;
+}
+
+static int include_by_env_bool(const char *cond, size_t cond_len)
+{
+	char *cfg = xstrndup(cond, cond_len);
+	int ret = git_env_bool(cfg, 0);
+	free(cfg);
+	return ret;
+}
+
+static int include_by_env_match(const char *cond, size_t cond_len, int glob,
+				int *err)
+{
+	const char *eq;
+	const char *value;
+	const char *env;
+	char *cfg = xstrndup(cond, cond_len);
+	char *key = NULL;
+	int ret = 0;
+
+	eq = strchr(cfg, ':');
+	if (!eq) {
+		*err = error(_("'%s:%.*s' missing a ':' to match the value"),
+			     glob ? "envMatch" : "envIs", (int)(cond_len),
+			     cond);
+		goto cleanup;
+	}
+	value = eq + 1;
+
+	key = xmemdupz(cfg, eq - cfg);
+	env = getenv(key);
+	if (!env)
+		goto cleanup;
+
+	ret = glob ? !wildmatch(value, env, 0) : !strcmp(value, env);
+
+cleanup:
+	free(key);
+	free(cfg);
+	return ret;
+}
+
 static int include_by_branch(const char *cond, size_t cond_len)
 {
 	int flags;
@@ -292,7 +340,8 @@ static int include_by_branch(const char *cond, size_t cond_len)
 }
 
 static int include_condition_is_true(const struct config_options *opts,
-				     const char *cond, size_t cond_len)
+				     const char *cond, size_t cond_len,
+				     int *err)
 {
 
 	if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
@@ -301,6 +350,14 @@ static int include_condition_is_true(const struct config_options *opts,
 		return include_by_gitdir(opts, cond, cond_len, 1);
 	else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
 		return include_by_branch(cond, cond_len);
+	else if (skip_prefix_mem(cond, cond_len, "envExists:", &cond, &cond_len))
+		return include_by_env_exists(cond, cond_len);
+	else if (skip_prefix_mem(cond, cond_len, "envBool:", &cond, &cond_len))
+		return include_by_env_bool(cond, cond_len);
+	else if (skip_prefix_mem(cond, cond_len, "envIs:", &cond, &cond_len))
+		return include_by_env_match(cond, cond_len, 0, err);
+	else if (skip_prefix_mem(cond, cond_len, "envMatch:", &cond, &cond_len))
+		return include_by_env_match(cond, cond_len, 1, err);
 
 	/* unknown conditionals are always false */
 	return 0;
@@ -325,7 +382,7 @@ int git_config_include(const char *var, const char *value, void *data)
 		ret = handle_path_include(value, inc);
 
 	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
-	    (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&
+	    (cond && include_condition_is_true(inc->opts, cond, cond_len, &ret)) &&
 	    !strcmp(key, "path"))
 		ret = handle_path_include(value, inc);
 
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index ccbb116c016..cebe2bb75f1 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -348,6 +348,73 @@ test_expect_success 'conditional include, onbranch, implicit /** for /' '
 	test_cmp expect actual
 '
 
+test_expect_success 'conditional include, envExists:*' '
+	echo value >expect &&
+	git config -f envExists.cfg some.key $(cat expect) &&
+
+	test_must_fail git -c includeIf.envExists:VAR.path="$PWD/envExists.cfg" config some.key 2>err &&
+	test_must_be_empty err &&
+
+	VAR= git -c includeIf.envExists:VAR.path="$PWD/envExists.cfg" config some.key >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect actual &&
+
+	VAR=0 git -c includeIf.envExists:VAR.path="$PWD/envExists.cfg" config some.key >actual 2>err &&
+	test_cmp expect actual &&
+	test_must_be_empty err
+'
+
+test_expect_success 'conditional include, envBool:*' '
+	echo value >expect &&
+	git config -f envBool.cfg some.key $(cat expect) &&
+
+	test_must_fail env VAR= git -c includeIf.envBool:VAR.path="$PWD/envBool.cfg" config some.key 2>err &&
+	test_must_be_empty err &&
+
+	test_must_fail env VAR=0 git -c includeIf.envBool:VAR.path="$PWD/envBool.cfg" config some.key 2>err &&
+	test_must_be_empty err &&
+
+	test_must_fail env VAR=false git -c includeIf.envBool:VAR.path="$PWD/envBool.cfg" config some.key 2>err &&
+	test_must_be_empty err &&
+
+	# envBool:* bad value
+	cat >expect.err <<-\EOF &&
+	fatal: bad boolean config value '\''gibberish'\'' for '\''VAR'\''
+	EOF
+	test_must_fail env VAR=gibberish git -c includeIf.envBool:VAR.path="$PWD/envBool.cfg" config some.key 2>err.actual &&
+	test_cmp expect.err err.actual
+'
+
+test_expect_success 'conditional include, envIs:*' '
+	echo value >expect &&
+	git config -f envIs.cfg some.key $(cat expect) &&
+
+	VAR=foo git -c includeIf.envIs:VAR:foo.path="$PWD/envExists.cfg" config some.key &&
+	test_must_fail env VAR=foo git -c includeIf.envIs:VAR:*f*.path="$PWD/envExists.cfg" config some.key &&
+
+	cat >expect.err <<-\EOF &&
+	error: '\''envIs:VAR'\'' missing a '\'':'\'' to match the value
+	fatal: unable to parse command-line config
+	EOF
+	test_must_fail env VAR=x git -c includeIf.envIs:VAR.path="$PWD/envBool.cfg" config some.key 2>err.actual &&
+	test_cmp expect.err err.actual
+'
+
+test_expect_success 'conditional include, envMatch:*' '
+	echo value >expect &&
+	git config -f envMatch.cfg some.key $(cat expect) &&
+
+	VAR=foo git -c includeIf.envMatch:VAR:foo.path="$PWD/envExists.cfg" config some.key &&
+	VAR=foo git -c includeIf.envMatch:VAR:*f*.path="$PWD/envExists.cfg" config some.key &&
+
+	cat >expect.err <<-\EOF &&
+	error: '\''envMatch:VAR'\'' missing a '\'':'\'' to match the value
+	fatal: unable to parse command-line config
+	EOF
+	test_must_fail env VAR=x git -c includeIf.envMatch:VAR.path="$PWD/envBool.cfg" config some.key 2>err.actual &&
+	test_cmp expect.err err.actual
+'
+
 test_expect_success 'include cycles are detected' '
 	git init --bare cycle &&
 	git -C cycle config include.path cycle &&
-- 
2.33.0.1231.g24d802460a8

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Jeff King]

On Fri, Sep 24, 2021 at 02:58:04AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Add an "includeIf" directive that's conditional on the value of an
> environment variable. This has been suggested at least a couple of
> times[1][2] as a proposed mechanism to drive dynamic includes, and to
> e.g. match based on TERM=xterm in the user's environment.

Thanks. I think this is a reasonable thing to have (not surprising,
since both of those suggestion references point to me!), and it's not
much of a burden to carry, even if it isn't all that commonly used.

I probably would have started with a smaller set of variants (just
equality, with a missing variable presented as an empty string). But I
don't think the bool/glob/exists variants are a lot of extra code or
complexity.

> I initially tried to implement just an "env" keyword, but found that
> splitting them up was both easier to implement and to explain to
> users. I.e. an "env" will need to handle an optional "=" for matching
> the value, and should the semantics be if the variable exists? If it's
> boolean?
> 
> By splitting them up we present a less ambiguous interface to users,
> and make it easy to extend this in the future. I didn't see any point
> in implementing a "/i" variant, that only makes sense for "gitdir"'s
> matching of FS paths.

I had thought to extend with the operator, like:

  # equality
  [includeIf "env:FOO==value"]
  # regex
  [includeIf "env:FOO=~v[a]l"]

But as you note, "=" is somewhat problematic, and without that we can't
use the "usual" operators. Plus there's no usual operator for globbing. ;)
So embedding it in the name is fine by me (and mostly a bikeshed thing
anyway).

I agree we don't really need a "/i" variant here.

> I would like syntax that used a "=" better, i.e. "envIs:TERM=xterm"
> instead of the "envIs:TERM:xterm" implemented here, but the problem
> with that is that it isn't possible to specify those sorts of config
> keys on the command-line:
> 
>     $ git -c includeIf.someVerb:FOO:BAR.path=bar status --short
>     $ git -c includeIf.someVerb:FOO=BAR.path=bar status --short
>     error: invalid key: includeIf.someVerb:FOO
>     fatal: unable to parse command-line config
>     $

Yeah, it's annoying that it doesn't work, and it's nice to think about
that when designing the syntax. OTOH, I kind of wonder how often folks
would write such a thing anyway. For one-offs like this, you'd just do
an unconditional include (or set the actual variables you care about)
anyway. This kind of conditional stuff is much more likely to appear in
an actual file.

Plus we will be stuck with whatever syntax we design here forever.
Whereas we may eventually provide a split version of "-c" that can
handle names with equals, at which point our syntax decision here will
just be a historical wart. In fact, we already have "--config-env" that
can do this.

>  Documentation/config.txt  | 35 ++++++++++++++++++++
>  config.c                  | 61 +++++++++++++++++++++++++++++++++--
>  t/t1305-config-include.sh | 67 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 161 insertions(+), 2 deletions(-)

The patch itself looks correct to me. Everything below is bikeshedding
(as if the parts above were not!), but you may or may not find some of
it compelling.

> +`envExists`::
> [...]
> +`envMatch`::

As I said, these four variants seem OK. Two other variants we might
consider:

  - regex vs glob; I think globbing is probably fine for now and regex
    would be overkill. We might want to reconsider the use of the word
    "match" though, since I assumed it to be a regex at first.

  - negation; for the TERM example discussed recently, I wonder if TERM
    != xterm would be a more natural fit. I had imagined "!=" as the
    operator, but in your scheme, it would probably be "!envIs", etc.

> +There is no way to match an environment variable name with any of the
> +`env*` directives if that variable name contains a ":" character. Such
> +names are allowed by POSIX, but it is assumed that nobody will need to
> +match such a variable name in practice.

That seems like a perfectly reasonable restriction.

Should we allow whitespace around key names and values? E.g.:

  [includeIf "env: FOO: bar"]

is IMHO more readable (even more so if we had infix operators like
"==").

> +static int include_by_env_exists(const char *cond, size_t cond_len)
> +{
> +	char *cfg = xstrndup(cond, cond_len);
> +	int ret = !!getenv(cfg);
> +	free(cfg);
> +	return ret;
> +}

Having to xstrndup() in each one of these is ugly. But doing it in the
caller would be even uglier, as we don't discover cond/cond_len until we
match via skip_prefix() in the big if/else chain. And certainly it's not
new to your patch anyway, just something I noticed.

BTW, I notice you used xmemdupz() in one case later on. I generally
prefer it to xstrndup() because it's more straight-forward: the string
is this long, and it needs to be NUL-terminated. Whereas xstrndup() is
_mostly_ equivalent, but will produce a smaller string if there are
embedded NULs. We would not expect them in this case, so I don't think
it matters functionally. It just seemed funny to me to see them mixed.

(I actually suspect 99% or more of xstrndup() calls should just be
xmemdupz(), and I'd be happy to consolidate and drop one of them, but it
would be finicky looking at each one to see if that's really true).

> +static int include_by_env_match(const char *cond, size_t cond_len, int glob,
> +				int *err)
> +{
> +	const char *eq;
> +	const char *value;
> +	const char *env;
> +	char *cfg = xstrndup(cond, cond_len);
> +	char *key = NULL;
> +	int ret = 0;
> +
> +	eq = strchr(cfg, ':');
> +	if (!eq) {
> +		*err = error(_("'%s:%.*s' missing a ':' to match the value"),
> +			     glob ? "envMatch" : "envIs", (int)(cond_len),
> +			     cond);

You made a string out of (cond, cond_len) already, so you could just use
"cfg" here in the error (what you have isn't wrong, but I always find
%.*s hard to read).

> +	key = xmemdupz(cfg, eq - cfg);

And this is the mixed xstrndup()/xmemdupz() case I mentioned. :)

>  static int include_condition_is_true(const struct config_options *opts,
> -				     const char *cond, size_t cond_len)
> +				     const char *cond, size_t cond_len,
> +				     int *err)

OK, we need to return not just "true" or "not true" from the return now,
so we stuff it into an out-parameter. We could use a tri-state instead.
Our if-else would already propagate it:

> @@ -301,6 +350,14 @@ static int include_condition_is_true(const struct config_options *opts,
>  		return include_by_gitdir(opts, cond, cond_len, 1);
>  	else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
>  		return include_by_branch(cond, cond_len);
> +	else if (skip_prefix_mem(cond, cond_len, "envExists:", &cond, &cond_len))
> +		return include_by_env_exists(cond, cond_len);
> +	else if (skip_prefix_mem(cond, cond_len, "envBool:", &cond, &cond_len))
> +		return include_by_env_bool(cond, cond_len);
> +	else if (skip_prefix_mem(cond, cond_len, "envIs:", &cond, &cond_len))
> +		return include_by_env_match(cond, cond_len, 0, err);
> +	else if (skip_prefix_mem(cond, cond_len, "envMatch:", &cond, &cond_len))
> +		return include_by_env_match(cond, cond_len, 1, err);

But it would mess up this:

>  	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
> -	    (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&
> +	    (cond && include_condition_is_true(inc->opts, cond, cond_len, &ret)) &&
>  	    !strcmp(key, "path"))
>  		ret = handle_path_include(value, inc);

So the out-parameter seems like a reasonable path.

I did find it interesting that there are no other error-cases in the
existing conditions. They mostly just evaluate to false (including if we
don't recognize the condition at all). But in the cases you're catching
here, it really is syntactic nonsense.

I guess you could define "there is no colon" as "the value we'd parse
after it is assumed to be the empty string" which makes the error case
go away. I'm not sure it really matters all that much either way in
practice.

> +test_expect_success 'conditional include, envExists:*' '
> +	echo value >expect &&
> +	git config -f envExists.cfg some.key $(cat expect) &&
> +
> +	test_must_fail git -c includeIf.envExists:VAR.path="$PWD/envExists.cfg" config some.key 2>err &&
> +	test_must_be_empty err &&

The tests all look sane to me. We do define some exit codes for
git-config here, so:

  test_expect_code 1 git -c ...

would be tighter (and you could probably just ditch the empty-err check
then).

Obviously avoiding the "=" question is beneficial here for testing via
"-c".  As I said earlier, I think it's more realistic to expect these in
actual files, but I think testing them either way is fine. If you did
test them in a file, though, you could use a relative path in the
include. Plus all three invocations could use the same file, so you
don't have to repeat it over and over. I.e.:

  test_config includeIf.envExists:VAR.path envExists.cfg &&

  test_expect_code 1 git config some.key &&

  VAR= git config some.key >actual &&
  test_cmp expect actual &&

  VAR=0 git config some.key >actual &&
  test_cmp expect actual

should work.

-Peff

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I had thought to extend with the operator, like:
>
>   # equality
>   [includeIf "env:FOO==value"]
>   # regex
>   [includeIf "env:FOO=~v[a]l"]

Yup, that matched my aesthetics better ;-)

> But as you note, "=" is somewhat problematic, and without that we can't
> use the "usual" operators. Plus there's no usual operator for globbing. ;)
> So embedding it in the name is fine by me (and mostly a bikeshed thing
> anyway).

Perhaps.  I am not sure if we deeply care about "git -c var=val" in
this case, especially since this is part of includeif, though.  It
may be more important to keep the syntax useful and extensible for
everyday use than for one-off "git -c" testing.

> I agree we don't really need a "/i" variant here.

Case insensitive environment variable names, no, but case
insensitive matching of values, maybe?  But I'd be happy to see us
start very minimally (even just envEQ alone without any other
frills, or optionally envNE to negate it, would be fine by me).

> Should we allow whitespace around key names and values? E.g.:
>
>   [includeIf "env: FOO: bar"]
>
> is IMHO more readable (even more so if we had infix operators like
> "==").

This asserts what? FOO=" bar"?

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Jeff King]

On Fri, Sep 24, 2021 at 02:28:29PM -0700, Junio C Hamano wrote:

> > But as you note, "=" is somewhat problematic, and without that we can't
> > use the "usual" operators. Plus there's no usual operator for globbing. ;)
> > So embedding it in the name is fine by me (and mostly a bikeshed thing
> > anyway).
> 
> Perhaps.  I am not sure if we deeply care about "git -c var=val" in
> this case, especially since this is part of includeif, though.  It
> may be more important to keep the syntax useful and extensible for
> everyday use than for one-off "git -c" testing.

Yeah, see my comments later in that mail. :)

> > I agree we don't really need a "/i" variant here.
> 
> Case insensitive environment variable names, no, but case
> insensitive matching of values, maybe?  But I'd be happy to see us
> start very minimally (even just envEQ alone without any other
> frills, or optionally envNE to negate it, would be fine by me).

Yeah, as long as we leave the door open syntactically, I think it is OK.

> > Should we allow whitespace around key names and values? E.g.:
> >
> >   [includeIf "env: FOO: bar"]
> >
> > is IMHO more readable (even more so if we had infix operators like
> > "==").
> 
> This asserts what? FOO=" bar"?

Whoops, that should have been "envIs", asserting that $FOO contains
"bar".

As I said, I think it matters more with the infix operators, as:

  [includeIf "env:FOO == bar"]

is more readable than:

  [includeIf "env:FOO==bar"]

But I do think:

  [includeIf "envIs:FOO:bar"]

is harder to read than even:

  [includeIf "envIs:FOO: bar"]

-Peff

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > Should we allow whitespace around key names and values? E.g.:
>> >
>> >   [includeIf "env: FOO: bar"]
>> >
>> > is IMHO more readable (even more so if we had infix operators like
>> > "==").
>> 
>> This asserts what? FOO=" bar"?
>
> Whoops, that should have been "envIs", asserting that $FOO contains
> "bar".

Oh, "can we check with a literal with leading whitespace?" was what
my question was about ;-)

> As I said, I think it matters more with the infix operators, as:
>
>   [includeIf "env:FOO == bar"]
>
> is more readable than:
>
>   [includeIf "env:FOO==bar"]

Sure, but at that point, we'd probably want some quoting mechanism
for the literal to be compared, e.g.

	[includeIf "env:PATH ~= \"(:|^)/usr/bin(:|$)\""]

> But I do think:
>
>   [includeIf "envIs:FOO:bar"]
>
> is harder to read than even:
>
>   [includeIf "envIs:FOO: bar"]

Hmph, that's quite subjective, I am afraid.  When I see the latter
in the configuration file, "do I have to have a single space before
'bar' in the value of $FOO" would be the first question that would
come to my mind.

With an understanding that our syntax is so limited that we cannot
even write '=' and need to resort to Is: instead, I'd actually find
that the former less confusing than the latter.

Thanks.

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Jeff King]

On Mon, Sep 27, 2021 at 09:30:41AM -0700, Junio C Hamano wrote:

> >> This asserts what? FOO=" bar"?
> >
> > Whoops, that should have been "envIs", asserting that $FOO contains
> > "bar".
> 
> Oh, "can we check with a literal with leading whitespace?" was what
> my question was about ;-)

My assumption was that nobody would really care about doing so. It is
true that it's less flexible, though (and is a decision we can't easily
take back later).

> > As I said, I think it matters more with the infix operators, as:
> >
> >   [includeIf "env:FOO == bar"]
> >
> > is more readable than:
> >
> >   [includeIf "env:FOO==bar"]
> 
> Sure, but at that point, we'd probably want some quoting mechanism
> for the literal to be compared, e.g.
> 
> 	[includeIf "env:PATH ~= \"(:|^)/usr/bin(:|$)\""]

Ick. The extra quoting of the internal double-quotes is pretty horrid to
look at. Also, how does one match a double-quote in the value? \\\"?

If it were optional, that would make the common cases easy (no dq, no
whitespace), and the hard ones possible.

I think this is getting into a bit of a digression, though. I'm willing
to defer to Ævar, who is doing the actual work, and I don't know if he
has found any of this compelling. ;)

> > But I do think:
> >
> >   [includeIf "envIs:FOO:bar"]
> >
> > is harder to read than even:
> >
> >   [includeIf "envIs:FOO: bar"]
> 
> Hmph, that's quite subjective, I am afraid.  When I see the latter
> in the configuration file, "do I have to have a single space before
> 'bar' in the value of $FOO" would be the first question that would
> come to my mind.

I think it's just the mashed-up colons that I find ugly in the first
one. But I agree the latter isn't that nice either, and introduces the
ambiguity you describe.

> With an understanding that our syntax is so limited that we cannot
> even write '=' and need to resort to Is: instead, I'd actually find
> that the former less confusing than the latter.

That I think is the most interesting question: is the "=" actually
out-of-bounds? I tend to think not, based on our responses earlier in
the thread.

-Peff

RE: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Randall S. Becker]

On September 27, 2021 4:16 PM, Jeff King wrote:
>Subject: Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}
>
>On Mon, Sep 27, 2021 at 09:30:41AM -0700, Junio C Hamano wrote:
>
>> >> This asserts what? FOO=" bar"?
>> >
>> > Whoops, that should have been "envIs", asserting that $FOO contains
>> > "bar".
>>
>> Oh, "can we check with a literal with leading whitespace?" was what my
>> question was about ;-)
>
>My assumption was that nobody would really care about doing so. It is true that it's less flexible, though (and is a decision we can't easily
>take back later).
>
>> > As I said, I think it matters more with the infix operators, as:
>> >
>> >   [includeIf "env:FOO == bar"]
>> >
>> > is more readable than:
>> >
>> >   [includeIf "env:FOO==bar"]
>>
>> Sure, but at that point, we'd probably want some quoting mechanism for
>> the literal to be compared, e.g.
>>
>> 	[includeIf "env:PATH ~= \"(:|^)/usr/bin(:|$)\""]
>
>Ick. The extra quoting of the internal double-quotes is pretty horrid to look at. Also, how does one match a double-quote in the value? \\\"?
>
>If it were optional, that would make the common cases easy (no dq, no whitespace), and the hard ones possible.
>
>I think this is getting into a bit of a digression, though. I'm willing to defer to Ævar, who is doing the actual work, and I don't know if he has
>found any of this compelling. ;)

What about something like:

	[includeIf "env:PATH ~= '^(.*😊)/usr/bin(:.*)*$' "]

Using single quotes and a full regex pattern instead of trying to provide a syntax to extract a pattern and then match. One call to regexec() would be easier. Then escaping is regcomp's problem (mostly). Potentially, you could even remove the outer ", but that would be wonky. You could omit the ^ and $ by default assuming a full match.
-Randall

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Jeff King]

On Mon, Sep 27, 2021 at 04:53:59PM -0400, Randall S. Becker wrote:

> What about something like:
> 
> 	[includeIf "env:PATH ~= '^(.*😊)/usr/bin(:.*)*$' "]
> 
> Using single quotes and a full regex pattern instead of trying to
> provide a syntax to extract a pattern and then match. One call to
> regexec() would be easier. Then escaping is regcomp's problem
> (mostly). Potentially, you could even remove the outer ", but that
> would be wonky. You could omit the ^ and $ by default assuming a full
> match.

I almost suggested that, but then...how do you put single-quotes in your
pattern? You can backslash-escape them, but:

  - do you need to escape the backslash to get it through the config
    parser intact?

  - it seems extra funny to me because single quotes usually imply a
    lack of interpolation

-Peff

RE: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Randall S. Becker]

On September 27, 2021 5:38 PM, Jeff King wrote:
>Subject: Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}
>
>On Mon, Sep 27, 2021 at 04:53:59PM -0400, Randall S. Becker wrote:
>
>> What about something like:
>>
>> 	[includeIf "env:PATH ~= '^(.*😊)/usr/bin(:.*)*$' "]
>>
>> Using single quotes and a full regex pattern instead of trying to
>> provide a syntax to extract a pattern and then match. One call to
>> regexec() would be easier. Then escaping is regcomp's problem
>> (mostly). Potentially, you could even remove the outer ", but that
>> would be wonky. You could omit the ^ and $ by default assuming a full
>> match.
>
>I almost suggested that, but then...how do you put single-quotes in your pattern? You can backslash-escape them, but:
>
>  - do you need to escape the backslash to get it through the config
>    parser intact?
>
>  - it seems extra funny to me because single quotes usually imply a
>    lack of interpolation

Exactly so. I think it would be more clear to have a regular expression be provided literally without interpretation other than by regcomp - other than the emergency ' escape, of course.

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 27 2021, Jeff King wrote:

> On Mon, Sep 27, 2021 at 09:30:41AM -0700, Junio C Hamano wrote:
>
>> >> This asserts what? FOO=" bar"?
>> >
>> > Whoops, that should have been "envIs", asserting that $FOO contains
>> > "bar".
>> 
>> Oh, "can we check with a literal with leading whitespace?" was what
>> my question was about ;-)
>
> My assumption was that nobody would really care about doing so. It is
> true that it's less flexible, though (and is a decision we can't easily
> take back later).

Yeah, I think nobody really cares about stripspace() or not for this
sort of feature.

I do think having implicit and explicit complexity like that makes it
harder to document, implement and understand for users though. I.e. is
"env:FOO == bar" the same as "env:FOO ==bar" etc., what whitespace
exactly is accepted etc.

>> > As I said, I think it matters more with the infix operators, as:
>> >
>> >   [includeIf "env:FOO == bar"]
>> >
>> > is more readable than:
>> >
>> >   [includeIf "env:FOO==bar"]
>> 
>> Sure, but at that point, we'd probably want some quoting mechanism
>> for the literal to be compared, e.g.
>> 
>> 	[includeIf "env:PATH ~= \"(:|^)/usr/bin(:|$)\""]
>
> Ick. The extra quoting of the internal double-quotes is pretty horrid to
> look at. Also, how does one match a double-quote in the value? \\\"?
>
> If it were optional, that would make the common cases easy (no dq, no
> whitespace), and the hard ones possible.

An implicit assumption of mine in the simpler positive-match-only
version (which I should have made clear) is that anyone who needs this
sort of complexity can just arrange to wrap their "git" in a function,
or do this sort of thing in their ~/.bashrc, i.e. just:

    if code_of_arbitrary_complexity
    then
        export GIT_DO_XYZ_INCLUDES=1
    fi

Then in your config:

    includeIf.envBool:GIT_DO_XYZ_INCLUDES.path=~/.gitconfig.d/xyz.cfg

And having written that out I think the best thing to do is probably to
have a version that only does the envExists and envBool version (or just
envBool), and skip envIs and envMatch entirely.

In the case of env:PATH we're just setting users up for some buggy or
unexpected interaction with something that would be better done either
via a gitdir include, or if they really need $PATH they can just wrap
"git" in a function that sets a boolean inclusion variable.

That would get us out of having to support emergent behavior where some
git tool invoked via run_command() or something chdir's somewhere as an
implementation detail, and such an env:PATH match means we'd need to
support that, or potentially break existing user config.

Or, since we might not be invoked via a shell, the same issue with a
$PATH being "stale" from the POV of a user who's wondering why say a
command like:

    # status in the "t" subdirectory
    git -C t <cmd> <question>

Doesn't have the "right" $PWD, which we might not have as some future
shortcut in <cmd> decided not to bother chdir()-ing to answer the user's
<question>.

> I think this is getting into a bit of a digression, though. I'm willing
> to defer to Ævar, who is doing the actual work, and I don't know if he
> has found any of this compelling. ;)
>
>> > But I do think:
>> >
>> >   [includeIf "envIs:FOO:bar"]
>> >
>> > is harder to read than even:
>> >
>> >   [includeIf "envIs:FOO: bar"]
>> 
>> Hmph, that's quite subjective, I am afraid.  When I see the latter
>> in the configuration file, "do I have to have a single space before
>> 'bar' in the value of $FOO" would be the first question that would
>> come to my mind.
>
> I think it's just the mashed-up colons that I find ugly in the first
> one. But I agree the latter isn't that nice either, and introduces the
> ambiguity you describe.

FWIW I hacked up a --config-key --config-value pairing so you could set
keys with "=" in them on the command-line, I'm not sure I like the
interface, but it gets rid of that ":" v.s. "=" edge case:
https://github.com/avar/git/commit/a86053df48b

>> With an understanding that our syntax is so limited that we cannot
>> even write '=' and need to resort to Is: instead, I'd actually find
>> that the former less confusing than the latter.
>
> That I think is the most interesting question: is the "=" actually
> out-of-bounds? I tend to think not, based on our responses earlier in
> the thread.

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Jeff King]

On Tue, Sep 28, 2021 at 01:52:26AM +0200, Ævar Arnfjörð Bjarmason wrote:

> An implicit assumption of mine in the simpler positive-match-only
> version (which I should have made clear) is that anyone who needs this
> sort of complexity can just arrange to wrap their "git" in a function,
> or do this sort of thing in their ~/.bashrc, i.e. just:
> 
>     if code_of_arbitrary_complexity
>     then
>         export GIT_DO_XYZ_INCLUDES=1
>     fi
> 
> Then in your config:
> 
>     includeIf.envBool:GIT_DO_XYZ_INCLUDES.path=~/.gitconfig.d/xyz.cfg
> 
> And having written that out I think the best thing to do is probably to
> have a version that only does the envExists and envBool version (or just
> envBool), and skip envIs and envMatch entirely.

I'm not sure I agree. If you are willing to wrap git, then you can just
add:

  git -c include.path=~/.gitconfig.d/xyz.cfg

to the command-line in the first place. Or if you're willing to use our
undocumented interface, you can even do it in your .bashrc:

  if code_of_arbitrary_complexity
  then
          GIT_CONFIG_PARAMETERS="'include.path'='~/.gitconfig.d/xyz.cfg'"
  fi

The value of this env matching is that it is done at run-time without
wrapping, and can meaningfully inspect the state of the world. E.g., the
$TERM thing that started this thread.

> In the case of env:PATH we're just setting users up for some buggy or
> unexpected interaction with something that would be better done either
> via a gitdir include, or if they really need $PATH they can just wrap
> "git" in a function that sets a boolean inclusion variable.

Yes, I have trouble imagining why any matching on env:PATH would be
useful (or $PWD, since we have the much less confusing gitdir
conditional). Which isn't to say I want to forbid it, but just because
people can shoot themselves in the foot with complexity doesn't mean
that "envIs" is a bad thing when it's not misused.

> > I think it's just the mashed-up colons that I find ugly in the first
> > one. But I agree the latter isn't that nice either, and introduces the
> > ambiguity you describe.
> 
> FWIW I hacked up a --config-key --config-value pairing so you could set
> keys with "=" in them on the command-line, I'm not sure I like the
> interface, but it gets rid of that ":" v.s. "=" edge case:
> https://github.com/avar/git/commit/a86053df48b

Yeah, we talked about that a while ago, but nobody liked the interface
enough to actually code it (and as far as I know, it's really
theoretical; nobody has actually wanted to set such an option from the
command-line yet, and we have the --config-env stuff for people who want
to robustly pass along arbitrary keys).

A perhaps more subtle but less awkward to type version is to just
require two arguments, like:

  git --config <key> <value> ...

but I'd just as soon continue to leave it un-implemented if nobody has
actually needed it in practice.

-Peff

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 27 2021, Jeff King wrote:

> On Tue, Sep 28, 2021 at 01:52:26AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> An implicit assumption of mine in the simpler positive-match-only
>> version (which I should have made clear) is that anyone who needs this
>> sort of complexity can just arrange to wrap their "git" in a function,
>> or do this sort of thing in their ~/.bashrc, i.e. just:
>> 
>>     if code_of_arbitrary_complexity
>>     then
>>         export GIT_DO_XYZ_INCLUDES=1
>>     fi
>> 
>> Then in your config:
>> 
>>     includeIf.envBool:GIT_DO_XYZ_INCLUDES.path=~/.gitconfig.d/xyz.cfg
>> 
>> And having written that out I think the best thing to do is probably to
>> have a version that only does the envExists and envBool version (or just
>> envBool), and skip envIs and envMatch entirely.
>
> I'm not sure I agree. If you are willing to wrap git, then you can just
> add:
>
>   git -c include.path=~/.gitconfig.d/xyz.cfg
>
> to the command-line in the first place. Or if you're willing to use our
> undocumented interface, you can even do it in your .bashrc:
>
>   if code_of_arbitrary_complexity
>   then
>           GIT_CONFIG_PARAMETERS="'include.path'='~/.gitconfig.d/xyz.cfg'"
>   fi

Sort of, that'll give you unconditional inclusion, but won't e.g. handle
a case where the env include only runs in some .git/config, or depending
on other inclusion (e.g. in ~/dev/git, but only if XYZ env var).

But yeah, it won't handle all potential cases. I figured for this sort
of thing it was better to start small and see if the provided interface
was enough..

> The value of this env matching is that it is done at run-time without
> wrapping, and can meaningfully inspect the state of the world. E.g., the
> $TERM thing that started this thread.

Yeah, maybe we should have at least an ifStrEQ, whatever we call it...

>> In the case of env:PATH we're just setting users up for some buggy or
>> unexpected interaction with something that would be better done either
>> via a gitdir include, or if they really need $PATH they can just wrap
>> "git" in a function that sets a boolean inclusion variable.
>
> Yes, I have trouble imagining why any matching on env:PATH would be
> useful (or $PWD, since we have the much less confusing gitdir
> conditional). Which isn't to say I want to forbid it, but just because
> people can shoot themselves in the foot with complexity doesn't mean
> that "envIs" is a bad thing when it's not misused.

I'm biased by past on-list discussions where existing behavior, no
matter if unintentional or emergent can be really hard to fix once
established.

>> > I think it's just the mashed-up colons that I find ugly in the first
>> > one. But I agree the latter isn't that nice either, and introduces the
>> > ambiguity you describe.
>> 
>> FWIW I hacked up a --config-key --config-value pairing so you could set
>> keys with "=" in them on the command-line, I'm not sure I like the
>> interface, but it gets rid of that ":" v.s. "=" edge case:
>> https://github.com/avar/git/commit/a86053df48b
>
> Yeah, we talked about that a while ago, but nobody liked the interface
> enough to actually code it (and as far as I know, it's really
> theoretical; nobody has actually wanted to set such an option from the
> command-line yet, and we have the --config-env stuff for people who want
> to robustly pass along arbitrary keys).
>
> A perhaps more subtle but less awkward to type version is to just
> require two arguments, like:
>
>   git --config <key> <value> ...

I suppose --config would work like that, you can'd to it with "-c". I
think it's more confusing to have a "-c" and "--config" which unlike
most other things don't follow the obvious long and short option names
working the same way.

> but I'd just as soon continue to leave it un-implemented if nobody has
> actually needed it in practice.

*nod*. I do think it's bad design to introduce an "env" inclusion
feature that relies on "=" though while we don't have something like
that, i.e.

I think we should probably not add that --config-{key,value}, but
avoiding the arbitrary limitation of not being able to specify certain
config keys seems prudent in that case, and since the "=" v.s. ":" is
only an aesthetic preference I think being able to compose things
without limitations wins out.

We do have the "=" key limitation now, but I don't think it's there for
any key we currently define, except things like "url.<base>.insteadOf"
if the "<base> has a "=" in it (and maybe just that one).

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Jeff King]

On Tue, Sep 28, 2021 at 04:42:51AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > A perhaps more subtle but less awkward to type version is to just
> > require two arguments, like:
> >
> >   git --config <key> <value> ...
> 
> I suppose --config would work like that, you can'd to it with "-c". I
> think it's more confusing to have a "-c" and "--config" which unlike
> most other things don't follow the obvious long and short option names
> working the same way.

Yeah, probably "--config-pair" or something might be less confusing.
Anyway...

> > but I'd just as soon continue to leave it un-implemented if nobody has
> > actually needed it in practice.
> 
> *nod*. I do think it's bad design to introduce an "env" inclusion
> feature that relies on "=" though while we don't have something like
> that, i.e.
> 
> I think we should probably not add that --config-{key,value}, but
> avoiding the arbitrary limitation of not being able to specify certain
> config keys seems prudent in that case, and since the "=" v.s. ":" is
> only an aesthetic preference I think being able to compose things
> without limitations wins out.

I don't really agree with that. Whatever syntax we use now, we'll be
stuck with forever. It seems a shame to predicate that choice only on
the "-c doesn't support =" thing that nobody has actually run across in
practice (and I don't think is something people will run into with
this).

> We do have the "=" key limitation now, but I don't think it's there for
> any key we currently define, except things like "url.<base>.insteadOf"
> if the "<base> has a "=" in it (and maybe just that one).

It's really a potential problem for any 3-level config key. So urls,
branch names, remote names, various tool names, filter/diff drivers,
existing includeIf conditions. This might be the first one where we
really _encourage_ the use of "=" signs, but it still strikes me as
weird that you'd want to do so on the command-line in practice.

-Peff

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Ævar Arnfjörð Bjarmason]

On Tue, Sep 28 2021, Jeff King wrote:

> On Tue, Sep 28, 2021 at 04:42:51AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > A perhaps more subtle but less awkward to type version is to just
>> > require two arguments, like:
>> >
>> >   git --config <key> <value> ...
>> 
>> I suppose --config would work like that, you can'd to it with "-c". I
>> think it's more confusing to have a "-c" and "--config" which unlike
>> most other things don't follow the obvious long and short option names
>> working the same way.
>
> Yeah, probably "--config-pair" or something might be less confusing.
> Anyway...

*nod*

>> > but I'd just as soon continue to leave it un-implemented if nobody has
>> > actually needed it in practice.
>> 
>> *nod*. I do think it's bad design to introduce an "env" inclusion
>> feature that relies on "=" though while we don't have something like
>> that, i.e.
>> 
>> I think we should probably not add that --config-{key,value}, but
>> avoiding the arbitrary limitation of not being able to specify certain
>> config keys seems prudent in that case, and since the "=" v.s. ":" is
>> only an aesthetic preference I think being able to compose things
>> without limitations wins out.
>
> I don't really agree with that. Whatever syntax we use now, we'll be
> stuck with forever. It seems a shame to predicate that choice only on
> the "-c doesn't support =" thing that nobody has actually run across in
> practice (and I don't think is something people will run into with
> this).

Yeah, anyway. I don't care much either way, and not enough to drive this
forward. I.e. it seemed like an easy thing to hack up, but if anyone
else is interested in driving it forward...

>> We do have the "=" key limitation now, but I don't think it's there for
>> any key we currently define, except things like "url.<base>.insteadOf"
>> if the "<base> has a "=" in it (and maybe just that one).
>
> It's really a potential problem for any 3-level config key. So urls,
> branch names, remote names, various tool names, filter/diff drivers,
> existing includeIf conditions. This might be the first one where we
> really _encourage_ the use of "=" signs, but it still strikes me as
> weird that you'd want to do so on the command-line in practice.

Just for future reference:

I think given the discussion in the thread, and particularly if we're
going to have some regex syntax for these keys that the artificial
straitjacket of putting this all in one config key is something we
should just do away with.

I.e. I'm not proposing a *specific* schema other than noting that
there's no law that forces us to take say Junio's (in
https://lore.kernel.org/git/xmqqo88eq8um.fsf@gitster.g/):

        [includeIf "env:PATH ~= \"(:|^)/usr/bin(:|$)\""]

Over say:

    [includeCondition]
        type = envRegex
        envVariable = PATH
        envRegex = "(:|^)/usr/bin(:|$)"
        path = ~/.gitconfig.d/env-stuff.cfg

Or whatever, i.e. the state machine of seeing an "includeCondition" in
the config's event parser, and then erroring unless the next N config
keys satisfy some mandatory minimum set of config keys is rather simple.

We could then make any such syntax optional for existing constructs,
i.e. you could write:

    [includeIf "gitdir:/path/to/group/"]
    path = /path/to/foo.inc

As:

    [includeCondition]
        type = gitdir
        path = /path/to/foo.inc
        gitdirPath = /path/to/group/

Or something. And say add "includeCondition.negated = true" to that for
a "!=" match.

The shorthand syntax could then be omitted for anything new if it's
deemed too gnarly to represent it all in one key.

The key names & schema is something I came up with offhand, please don't
read too much into it. The point is that we don't need to make it all
one key.

That approach also fits nicely in with the rest of the config framework,
i.e. you can incrementally edit things using "git config" options, and
we could add a "--type regex" or whatever which we could then
set/validate say the "includeCondition.envRegex" key with.

Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Sure, but at that point, we'd probably want some quoting mechanism
>> for the literal to be compared, e.g.
>> 
>> 	[includeIf "env:PATH ~= \"(:|^)/usr/bin(:|$)\""]
>
> Ick. The extra quoting of the internal double-quotes is pretty horrid to
> look at. Also, how does one match a double-quote in the value? \\\"?

Ick indeed.  I didn't mean to say you must always dq quote.  It
started more like 

	includeIf "env:VAR == ' value with leading whitespace'"

(or use \" inside ""-pair to mean a double-quote) as a demonstration
of an escape hatch needed if we took your "let's by default strip
the whitespace around the value" example in the message I was
responding to.

Just like we in most cases do not have to quote the value in the
configuration files, unless you have strange needs like wanting to
express a value with leading whitespace that should not be stripped,
if we were to go this route, 

> If it were optional, that would make the common cases easy (no dq, no
> whitespace), and the hard ones possible.

Yup.

Re: ANSI sequences produced on non-ANSI terminal

[Greywolf]

Greetings and thank you ALL for your responses!

On 9/23/2021 14:20, Jeff King wrote:

> Git doesn't have any kind of list of terminals, beyond knowing that "dumb"
> should disable auto-color. It's possible we could expand that if there are
> known terminals that don't understand ANSI colors. I'm a bit wary of having
> a laundry list of obscure terminals, though.

Oh, gods, I wouldn't have that at all!  No, I just want it NOT to spit out
not only the colour codes, but the cursor positioning codes as it seems
wont to do when I do a fetch.  I'm more than happy to turn coloring off
(conditional on TERM would be a bonus, however it's done) on my own;
in fact, I have done so, but the fetch/pull still seem to be messing up
my screen, with color turned off (unless I'm not turning it off
*enough*, which is entirely possible).

> If we built against ncurses or some other terminfo-aware library we could
> outsource that, but that would be a new dependency. I'm hesitant to do that
> even as an optional dependency given the bang-for-the-buck (and certainly
> making it require would be right out).

Well understood.  Also, not asking for people to jump thru flaming hoops.
Just trying to figure out how to get git to stop assuming things.
(as stated, I am aware it could be my fault for not setting variables
properly all the way).

> Obviously you can wrap Git with a script to tweak the config based on the
> current setting of the $TERM variable. It would be nice if you could have
> conditional config for that. E.g., something like:
> 
> [includeIf "env:TERM==xterm"] path = gitconfig-color
> 
> That doesn't exist, but would fit in reasonably well with our other 
> conditional config options.

That is a consideration; and one I had not thought of.

> As far as generating non-ANSI codes, that's all Git knows how to do.

Just need to have it NOT generate ANSI codes, if requested.  I'm certainly
not requesting the world of terminals to be incorporated -- just some
universal readability.

As far as the suggestion to use "screen", I'm not going to be starting up
a screen session every time I log in. :)

				Thank you all very much!

				--*greywolf;

Re: ANSI sequences produced on non-ANSI terminal

[Jeff King]

On Fri, Sep 24, 2021 at 04:57:11PM -0700, Greywolf wrote:

> On 9/23/2021 14:20, Jeff King wrote:
> 
> > Git doesn't have any kind of list of terminals, beyond knowing that "dumb"
> > should disable auto-color. It's possible we could expand that if there are
> > known terminals that don't understand ANSI colors. I'm a bit wary of having
> > a laundry list of obscure terminals, though.
> 
> Oh, gods, I wouldn't have that at all!  No, I just want it NOT to spit out
> not only the colour codes, but the cursor positioning codes as it seems
> wont to do when I do a fetch.  I'm more than happy to turn coloring off
> (conditional on TERM would be a bonus, however it's done) on my own;
> in fact, I have done so, but the fetch/pull still seem to be messing up
> my screen, with color turned off (unless I'm not turning it off
> *enough*, which is entirely possible).

OK, that makes things a bit easier. The colors, as you noticed, can be
disabled by config. The other thing you're seeing is ANSI ESC[K, which
is used to clear to the end of line. We use this in a couple places,
notably when relaying progress lines from the server (with the "remote:"
prefix) which may use carriage-returns to overwrite lines.

See ebe8fa738d (fix display overlap between remote and local progress,
2007-11-04) if you're really interested.

Anyway, there's no config option to disable that. However, we do disable
it if TERM is empty or set to "dumb" (and instead just write some extra
spaces to clear out the line). So that may be an option, though of
course setting TERM=dumb may affect other programs you use.

I don't think it would be unreasonable to have a config option to
select whether we use the ANSI or dumb-term version.

> > If we built against ncurses or some other terminfo-aware library we could
> > outsource that, but that would be a new dependency. I'm hesitant to do that
> > even as an optional dependency given the bang-for-the-buck (and certainly
> > making it require would be right out).
> 
> Well understood.  Also, not asking for people to jump thru flaming hoops.
> Just trying to figure out how to get git to stop assuming things.
> (as stated, I am aware it could be my fault for not setting variables
> properly all the way).

Nah, it sounds like you actually set the variables correctly. We've just
assumed that we can get by with ANSI codes as a lowest common
denominator in the modern world, without having to resort to all the
complexities of using a terminfo library. It's worked pretty well so
far. ;)

-Peff

Re: ANSI sequences produced on non-ANSI terminal

[Greywolf]

On 9/24/2021 22:49, Jeff King wrote:

> OK, that makes things a bit easier. The colors, as you noticed, can be 
> disabled by config. The other thing you're seeing is ANSI ESC[K, which is 
> used to clear to the end of line. We use this in a couple places, notably 
> when relaying progress lines from the server (with the "remote:" prefix) 
> which may use carriage-returns to overwrite lines.

Those would be some of the culprits.  I'll have to do a 'script' and see what
it is spitting out.

> Anyway, there's no config option to disable that. However, we do disable
> it if TERM is empty or set to "dumb" (and instead just write some extra
> spaces to clear out the line). So that may be an option, though of course
> setting TERM=dumb may affect other programs you use.

Editors, in particular, tend not to like interacting with TERM=dumb as they 
have no idea how to behave around one ("I need CE and UP" comes to mind).

> I don't think it would be unreasonable to have a config option to select 
> whether we use the ANSI or dumb-term version.

GIT_TERM might be an option to override TERM, but I am loath to actually
suggest YAEV.

> Nah, it sounds like you actually set the variables correctly. We've just 
> assumed that we can get by with ANSI codes as a lowest common denominator 
> in the modern world, without having to resort to all the complexities of 
> using a terminfo library. It's worked pretty well so far. ;)

Laughing out loud at that.  Part of me is apologetic to be The Weird Kid.
The other part of me is looking for more ways to be weird.

Thank you for taking a look at this.

> 
> -Peff
> 

				Cheers,

				--*greywolf;