{"thread":{"id":"56570","subject":"ANSI sequences produced on non-ANSI terminal","startedAt":"2021-09-23T05:27:50Z","lastAt":"2021-10-01T23:17:08Z","messageCount":23,"participants":["The Grey Wolf","Jeff King","Junio C Hamano","Randall S. Becker","Ævar Arnfjörð Bjarmason","Greywolf","Kevin Daudt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"436829","messageId":"20210923052122.2F655CE@eddie.starwolf.com","threadId":"56570","inReplyTo":null,"subject":"ANSI sequences produced on non-ANSI terminal","fromName":"The Grey Wolf","fromEmail":"greywolf@starwolf.com","sentAt":"2021-09-23T05:21:22Z","receivedAt":"2021-09-23T05:27:50Z","isPatch":false,"sender":{"key":"greywolf@starwolf.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\tLogged on to a hard terminal and ran 'git status .' and\n\t'git pull'.\n\nWhat did you expect to happen? (Expected behavior)\n\tI expected that colour sequences would not be output, or at least\n\tnot hardcoded to ANSI standard. In the case of 'pull', I expected\n\tthat the stats would be just printed using \\r.\n\nWhat happened instead? (Actual behavior)\n\tI got escape sequences that made the output unreadable.\n\nWhat's different between what you expected and what actually happened?\n\tOne produces mangled ouptut and the other doesn't.\n\nAnything else you want to add:\n\tI searched google and the documentation as best I was able for\n\tthis, but I am unable to find anywhere that will let me disable\n\t(or enable) colour for a particular term type. Sometimes I'm on\n\tan xterm, for which this is GREAT. Sometimes I'm on a Wyse WY60,\n\tfor which this is sub-optimal. My workaround is to disable colour\n\tcompletely, which is reluctantly acceptable, but it would be nice\n\tto say \"If I'm on an xterm/aterm/urxvt/ansi terminal, enable\n\tcolour or cursor-positioning, otherwise shut it off.\" If this\n\tseems too much of a one-off to handle, fine, but most things that\n\ttalk fancy to screens are kind enough to allow an opt-out based on\n\tterminal type. :)\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n[System Info]\ngit version:\ngit version 2.32.0\ncpu: amd64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: 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\ncompiler info: gnuc: 10.3\nlibc info: no libc information available (actually there's a LOT of it,\n\tbut I'm not sure you really want it -- please let me know if you do).\n$SHELL (typically, interactive shell): /bin/bash\n$TERM: wy60\n\n[Enabled Hooks]\n\tI don't know enough about git yet to use these.\n\n"},{"id":"436896","messageId":"YUzvhLUmvsdF5w+r@coredump.intra.peff.net","threadId":"56570","inReplyTo":"20210923052122.2F655CE@eddie.starwolf.com","subject":"Re: ANSI sequences produced on non-ANSI terminal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-23T21:20:04Z","receivedAt":"2021-09-23T21:20:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:\n\n> Anything else you want to add:\n> \tI searched google and the documentation as best I was able for\n> \tthis, but I am unable to find anywhere that will let me disable\n> \t(or enable) colour for a particular term type. Sometimes I'm on\n> \tan xterm, for which this is GREAT. Sometimes I'm on a Wyse WY60,\n> \tfor which this is sub-optimal. My workaround is to disable colour\n> \tcompletely, which is reluctantly acceptable, but it would be nice\n> \tto say \"If I'm on an xterm/aterm/urxvt/ansi terminal, enable\n> \tcolour or cursor-positioning, otherwise shut it off.\" If this\n> \tseems too much of a one-off to handle, fine, but most things that\n> \ttalk fancy to screens are kind enough to allow an opt-out based on\n> \tterminal type. :)\n\nGit doesn't have any kind of list of terminals, beyond knowing that\n\"dumb\" should disable auto-color. It's possible we could expand that if\nthere are known terminals that don't understand ANSI colors. I'm a bit\nwary of having a laundry list of obscure terminals, though.\n\nIf we built against ncurses or some other terminfo-aware library we\ncould outsource that, but that would be a new dependency. I'm hesitant\nto do that even as an optional dependency given the bang-for-the-buck\n(and certainly making it require would be right out).\n\nObviously you can wrap Git with a script to tweak the config based on\nthe current setting of the $TERM variable. It would be nice if you could\nhave conditional config for that. E.g., something like:\n\n [includeIf \"env:TERM==xterm\"]\n path = gitconfig-color\n\nThat doesn't exist, but would fit in reasonably well with our other\nconditional config options.\n\nAs far as generating non-ANSI codes, that's all Git knows how to do. I'm\nnot sure what kind of color codes your terminal might support. It\n_might_ be possible to support multiple, but from my knowledge of Git's\ncolor code I suspect it would be quite ugly. You'd probably be better\noff post-processing the ANSI codes. You can do so automatically-ish with\nsomething like:\n\n git config pager.log 'convert-ansi-to-whatever | less'\n\nI don't know offhand of anything that would do such conversion out of\nthe box, but you could probably built it around tput or a terminfo\nlibrary.\n\nNote that we do similar post-processing on Windows, albeit internally\nby intercepting fprintf, etc (yuck). See compat/winansi.c, which might\ngive you some logic which would be reused.\n\n-Peff\n"},{"id":"436902","messageId":"xmqqmto3x8ik.fsf@gitster.g","threadId":"56570","inReplyTo":"YUzvhLUmvsdF5w+r@coredump.intra.peff.net","subject":"Re: ANSI sequences produced on non-ANSI terminal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T21:54:43Z","receivedAt":"2021-09-23T21:54:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:\n>\n>> Anything else you want to add:\n>> \tI searched google and the documentation as best I was able for\n>> \tthis, but I am unable to find anywhere that will let me disable\n>> \t(or enable) colour for a particular term type. Sometimes I'm on\n>> \tan xterm, for which this is GREAT. Sometimes I'm on a Wyse WY60,\n>> \tfor which this is sub-optimal. My workaround is to disable colour\n>> \tcompletely, which is reluctantly acceptable, but it would be nice\n>> \tto say \"If I'm on an xterm/aterm/urxvt/ansi terminal, enable\n>> \tcolour or cursor-positioning, otherwise shut it off.\" If this\n>> \tseems too much of a one-off to handle, fine, but most things that\n>> \ttalk fancy to screens are kind enough to allow an opt-out based on\n>> \tterminal type. :)\n>\n> Git doesn't have any kind of list of terminals, beyond knowing that\n> \"dumb\" should disable auto-color. It's possible we could expand that if\n> there are known terminals that don't understand ANSI colors. I'm a bit\n> wary of having a laundry list of obscure terminals, though.\n>\n> If we built against ncurses or some other terminfo-aware library we\n> could outsource that, but that would be a new dependency. I'm hesitant\n> to do that even as an optional dependency given the bang-for-the-buck\n> (and certainly making it require would be right out).\n\nI was wondering if Gray Wolf can run screen on the Wyse, and then\nwouldn't git see TERM=screen which is pretty much ANSI if I am not\nmistaken ;-)?\n"},{"id":"436905","messageId":"038801d7b0c6$f9345a90$eb9d0fb0$@nexbridge.com","threadId":"56570","inReplyTo":"xmqqmto3x8ik.fsf@gitster.g","subject":"RE: ANSI sequences produced on non-ANSI terminal","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-09-23T22:04:20Z","receivedAt":"2021-09-23T22:04:44Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On September 23, 2021 5:55 PM, Junio C Hamano wrote:\n>Jeff King writes:\n>\n>> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:\n>>\n>>> Anything else you want to add:\n>>> \tI searched google and the documentation as best I was able for\n>>> \tthis, but I am unable to find anywhere that will let me disable\n>>> \t(or enable) colour for a particular term type. Sometimes I'm on\n>>> \tan xterm, for which this is GREAT. Sometimes I'm on a Wyse WY60,\n>>> \tfor which this is sub-optimal. My workaround is to disable colour\n>>> \tcompletely, which is reluctantly acceptable, but it would be nice\n>>> \tto say \"If I'm on an xterm/aterm/urxvt/ansi terminal, enable\n>>> \tcolour or cursor-positioning, otherwise shut it off.\" If this\n>>> \tseems too much of a one-off to handle, fine, but most things that\n>>> \ttalk fancy to screens are kind enough to allow an opt-out based on\n>>> \tterminal type. :)\n>>\n>> Git doesn't have any kind of list of terminals, beyond knowing that\n>> \"dumb\" should disable auto-color. It's possible we could expand that\n>> if there are known terminals that don't understand ANSI colors. I'm a\n>> bit wary of having a laundry list of obscure terminals, though.\n>>\n>> If we built against ncurses or some other terminfo-aware library we\n>> could outsource that, but that would be a new dependency. I'm hesitant\n>> to do that even as an optional dependency given the bang-for-the-buck\n>> (and certainly making it require would be right out).\n>\n>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\nnot\n>mistaken ;-)?\n\nWould something like switch in .gitconfig make a difference? Like core.colourize=false. There are situations where SSH sessions come\nin from automation, like Jenkins and Travis, which sets term to something other than dumb by default. Coloring makes a mess of the\noutput. The ability to turn off colouring off by user might be helpful.\n\n-Randall\n\n"},{"id":"436910","messageId":"patch-1.1-1fe6f60d2bf-20210924T005553Z-avarab@gmail.com","threadId":"56570","inReplyTo":"YUzvhLUmvsdF5w+r@coredump.intra.peff.net","subject":"[PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-24T00:58:04Z","receivedAt":"2021-09-24T00:58:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add an \"includeIf\" directive that's conditional on the value of an\nenvironment variable. This has been suggested at least a couple of\ntimes[1][2] as a proposed mechanism to drive dynamic includes, and to\ne.g. match based on TERM=xterm in the user's environment.\n\nI initially tried to implement just an \"env\" keyword, but found that\nsplitting them up was both easier to implement and to explain to\nusers. I.e. an \"env\" will need to handle an optional \"=\" for matching\nthe value, and should the semantics be if the variable exists? If it's\nboolean?\n\nBy splitting them up we present a less ambiguous interface to users,\nand make it easy to extend this in the future. I didn't see any point\nin implementing a \"/i\" variant, that only makes sense for \"gitdir\"'s\nmatching of FS paths.\n\nI would like syntax that used a \"=\" better, i.e. \"envIs:TERM=xterm\"\ninstead of the \"envIs:TERM:xterm\" implemented here, but the problem\nwith that is that it isn't possible to specify those sorts of config\nkeys on the command-line:\n\n $ git -c includeIf.someVerb:FOO:BAR.path=bar status --short\n $ git -c includeIf.someVerb:FOO=BAR.path=bar status --short\n error: invalid key: includeIf.someVerb:FOO\n fatal: unable to parse command-line config\n $\n\nI.e. not only isn't the \"someVerb\" in that scenario not understood by\nan older git, but it'll hard error on seeing such a key. See\n1ff21c05ba9 (config: store \"git -c\" variables using more robust\nformat, 2021-01-12) for a discussion about \"=\" in config keys. By\npicking any other character (e.g. \":\") we avoid that whole issue.\n\n1. https://lore.kernel.org/git/YUzvhLUmvsdF5w+r@coredump.intra.peff.net/\n2. https://lore.kernel.org/git/X9OB7ek8fVRXUBdK@coredump.intra.peff.net/\n3. https://lore.kernel.org/git/87in9ucsbb.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\n> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:\n>\n>> Anything else you want to add:\n>> I searched google and the documentation as best I was able for\n>> this, but I am unable to find anywhere that will let me disable\n>> (or enable) colour for a particular term type. Sometimes I'm on\n>> an xterm, for which this is GREAT. Sometimes I'm on a Wyse WY60,\n>> for which this is sub-optimal. My workaround is to disable colour\n>> completely, which is reluctantly acceptable, but it would be nice\n>> to say \"If I'm on an xterm/aterm/urxvt/ansi terminal, enable\n>> colour or cursor-positioning, otherwise shut it off.\" If this\n>> seems too much of a one-off to handle, fine, but most things that\n>> talk fancy to screens are kind enough to allow an opt-out based on\n>> terminal type. :)\n>\n> Git doesn't have any kind of list of terminals, beyond knowing that\n> \"dumb\" should disable auto-color. It's possible we could expand that if\n> there are known terminals that don't understand ANSI colors. I'm a bit\n> wary of having a laundry list of obscure terminals, though.\n>\n> If we built against ncurses or some other terminfo-aware library we\n> could outsource that, but that would be a new dependency. I'm hesitant\n> to do that even as an optional dependency given the bang-for-the-buck\n> (and certainly making it require would be right out).\n>\n> Obviously you can wrap Git with a script to tweak the config based on\n> the current setting of the $TERM variable. It would be nice if you could\n> have conditional config for that. E.g., something like:\n>\n> [includeIf \"env:TERM==xterm\"]\n> path = gitconfig-color\n>\n> That doesn't exist, but would fit in reasonably well with our other\n> conditional config options.\n\nPerhaps something like this?\n\n Documentation/config.txt | 35 ++++++++++++++++++++\n config.c | 61 +++++++++++++++++++++++++++++++++--\n t/t1305-config-include.sh | 67 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 161 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0c0e6b859f1..58f6d49216d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -159,6 +159,41 @@ all branches that begin with `foo/`. This is useful if your branches are\n organized hierarchically and you would like to apply a configuration to\n all the branches in that hierarchy.\n \n+`envExists`::\n+\tThe data that follows the keyword `envExists:` is taken to be the\n+\tname of an environment variable, e.g. `envExists:NAME`. If the\n+\tvariable (`NAME` in this case) exists the include condition is\n+\tmet.\n+\n+`envBool`::\n+\tThe data that follows the keyword `envBool:` is taken to be the\n+\tname of an environment variable, e.g. `envBool:NAME`. If the\n+\tvariable (`NAME` in this case) exists, its value will be\n+\tchecked for boolean truth.\n++\n+The accepted boolean values are the same as those that `--type bool`\n+would accept. A value that's normalized to \"true\" will satisfy the\n+include condition (a nonexisting environment variable is \"false\").\n+\n+`envIs`::\n+\tThe data that follows the keyword `envIs:` is taken to be the\n+\tname of an environment variable, followed by a mandatory \":\"\n+\tcharacter, followed by the expected value of the environment\n+\tvariable. If the value matches the include condition is\n+\tsatisfied.\n++\n+Values may contain any characters otherwise accepted by the config\n+mechanism (including \":\").\n+\n+`envMatch`::\n+\tLike `envIs`, except that the value is matched using standard\n+\tglobbing wildcards.\n+\n+There is no way to match an environment variable name with any of the\n+`env*` directives if that variable name contains a \":\" character. Such\n+names are allowed by POSIX, but it is assumed that nobody will need to\n+match such a variable name in practice.\n+\n A few more notes on matching via `gitdir` and `gitdir/i`:\n \n * Symlinks in `$GIT_DIR` are not resolved before matching.\ndiff --git a/config.c b/config.c\nindex 2edf835262f..064059b0d6d 100644\n--- a/config.c\n+++ b/config.c\n@@ -271,6 +271,54 @@ static int include_by_gitdir(const struct config_options *opts,\n \treturn ret;\n }\n \n+static int include_by_env_exists(const char *cond, size_t cond_len)\n+{\n+\tchar *cfg = xstrndup(cond, cond_len);\n+\tint ret = !!getenv(cfg);\n+\tfree(cfg);\n+\treturn ret;\n+}\n+\n+static int include_by_env_bool(const char *cond, size_t cond_len)\n+{\n+\tchar *cfg = xstrndup(cond, cond_len);\n+\tint ret = git_env_bool(cfg, 0);\n+\tfree(cfg);\n+\treturn ret;\n+}\n+\n+static int include_by_env_match(const char *cond, size_t cond_len, int glob,\n+\t\t\t\tint *err)\n+{\n+\tconst char *eq;\n+\tconst char *value;\n+\tconst char *env;\n+\tchar *cfg = xstrndup(cond, cond_len);\n+\tchar *key = NULL;\n+\tint ret = 0;\n+\n+\teq = strchr(cfg, ':');\n+\tif (!eq) {\n+\t\t*err = error(_(\"'%s:%.*s' missing a ':' to match the value\"),\n+\t\t\t glob ? \"envMatch\" : \"envIs\", (int)(cond_len),\n+\t\t\t cond);\n+\t\tgoto cleanup;\n+\t}\n+\tvalue = eq + 1;\n+\n+\tkey = xmemdupz(cfg, eq - cfg);\n+\tenv = getenv(key);\n+\tif (!env)\n+\t\tgoto cleanup;\n+\n+\tret = glob ? !wildmatch(value, env, 0) : !strcmp(value, env);\n+\n+cleanup:\n+\tfree(key);\n+\tfree(cfg);\n+\treturn ret;\n+}\n+\n static int include_by_branch(const char *cond, size_t cond_len)\n {\n \tint flags;\n@@ -292,7 +340,8 @@ static int include_by_branch(const char *cond, size_t cond_len)\n }\n \n static int include_condition_is_true(const struct config_options *opts,\n-\t\t\t\t const char *cond, size_t cond_len)\n+\t\t\t\t const char *cond, size_t cond_len,\n+\t\t\t\t int *err)\n {\n \n \tif (skip_prefix_mem(cond, cond_len, \"gitdir:\", &cond, &cond_len))\n@@ -301,6 +350,14 @@ static int include_condition_is_true(const struct config_options *opts,\n \t\treturn include_by_gitdir(opts, cond, cond_len, 1);\n \telse if (skip_prefix_mem(cond, cond_len, \"onbranch:\", &cond, &cond_len))\n \t\treturn include_by_branch(cond, cond_len);\n+\telse if (skip_prefix_mem(cond, cond_len, \"envExists:\", &cond, &cond_len))\n+\t\treturn include_by_env_exists(cond, cond_len);\n+\telse if (skip_prefix_mem(cond, cond_len, \"envBool:\", &cond, &cond_len))\n+\t\treturn include_by_env_bool(cond, cond_len);\n+\telse if (skip_prefix_mem(cond, cond_len, \"envIs:\", &cond, &cond_len))\n+\t\treturn include_by_env_match(cond, cond_len, 0, err);\n+\telse if (skip_prefix_mem(cond, cond_len, \"envMatch:\", &cond, &cond_len))\n+\t\treturn include_by_env_match(cond, cond_len, 1, err);\n \n \t/* unknown conditionals are always false */\n \treturn 0;\n@@ -325,7 +382,7 @@ int git_config_include(const char *var, const char *value, void *data)\n \t\tret = handle_path_include(value, inc);\n \n \tif (!parse_config_key(var, \"includeif\", &cond, &cond_len, &key) &&\n-\t (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&\n+\t (cond && include_condition_is_true(inc->opts, cond, cond_len, &ret)) &&\n \t !strcmp(key, \"path\"))\n \t\tret = handle_path_include(value, inc);\n \ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex ccbb116c016..cebe2bb75f1 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -348,6 +348,73 @@ test_expect_success 'conditional include, onbranch, implicit /** for /' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'conditional include, envExists:*' '\n+\techo value >expect &&\n+\tgit config -f envExists.cfg some.key $(cat expect) &&\n+\n+\ttest_must_fail git -c includeIf.envExists:VAR.path=\"$PWD/envExists.cfg\" config some.key 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\tVAR= git -c includeIf.envExists:VAR.path=\"$PWD/envExists.cfg\" config some.key >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect actual &&\n+\n+\tVAR=0 git -c includeIf.envExists:VAR.path=\"$PWD/envExists.cfg\" config some.key >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+'\n+\n+test_expect_success 'conditional include, envBool:*' '\n+\techo value >expect &&\n+\tgit config -f envBool.cfg some.key $(cat expect) &&\n+\n+\ttest_must_fail env VAR= git -c includeIf.envBool:VAR.path=\"$PWD/envBool.cfg\" config some.key 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\ttest_must_fail env VAR=0 git -c includeIf.envBool:VAR.path=\"$PWD/envBool.cfg\" config some.key 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\ttest_must_fail env VAR=false git -c includeIf.envBool:VAR.path=\"$PWD/envBool.cfg\" config some.key 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\t# envBool:* bad value\n+\tcat >expect.err <<-\\EOF &&\n+\tfatal: bad boolean config value '\\''gibberish'\\'' for '\\''VAR'\\''\n+\tEOF\n+\ttest_must_fail env VAR=gibberish git -c includeIf.envBool:VAR.path=\"$PWD/envBool.cfg\" config some.key 2>err.actual &&\n+\ttest_cmp expect.err err.actual\n+'\n+\n+test_expect_success 'conditional include, envIs:*' '\n+\techo value >expect &&\n+\tgit config -f envIs.cfg some.key $(cat expect) &&\n+\n+\tVAR=foo git -c includeIf.envIs:VAR:foo.path=\"$PWD/envExists.cfg\" config some.key &&\n+\ttest_must_fail env VAR=foo git -c includeIf.envIs:VAR:*f*.path=\"$PWD/envExists.cfg\" config some.key &&\n+\n+\tcat >expect.err <<-\\EOF &&\n+\terror: '\\''envIs:VAR'\\'' missing a '\\'':'\\'' to match the value\n+\tfatal: unable to parse command-line config\n+\tEOF\n+\ttest_must_fail env VAR=x git -c includeIf.envIs:VAR.path=\"$PWD/envBool.cfg\" config some.key 2>err.actual &&\n+\ttest_cmp expect.err err.actual\n+'\n+\n+test_expect_success 'conditional include, envMatch:*' '\n+\techo value >expect &&\n+\tgit config -f envMatch.cfg some.key $(cat expect) &&\n+\n+\tVAR=foo git -c includeIf.envMatch:VAR:foo.path=\"$PWD/envExists.cfg\" config some.key &&\n+\tVAR=foo git -c includeIf.envMatch:VAR:*f*.path=\"$PWD/envExists.cfg\" config some.key &&\n+\n+\tcat >expect.err <<-\\EOF &&\n+\terror: '\\''envMatch:VAR'\\'' missing a '\\'':'\\'' to match the value\n+\tfatal: unable to parse command-line config\n+\tEOF\n+\ttest_must_fail env VAR=x git -c includeIf.envMatch:VAR.path=\"$PWD/envBool.cfg\" config some.key 2>err.actual &&\n+\ttest_cmp expect.err err.actual\n+'\n+\n test_expect_success 'include cycles are detected' '\n \tgit init --bare cycle &&\n \tgit -C cycle config include.path cycle &&\n-- \n2.33.0.1231.g24d802460a8\n\n"},{"id":"437045","messageId":"YU49+Y+nRhl1mgof@coredump.intra.peff.net","threadId":"56570","inReplyTo":"patch-1.1-1fe6f60d2bf-20210924T005553Z-avarab@gmail.com","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T21:07:05Z","receivedAt":"2021-09-24T21:07:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 24, 2021 at 02:58:04AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Add an \"includeIf\" directive that's conditional on the value of an\n> environment variable. This has been suggested at least a couple of\n> times[1][2] as a proposed mechanism to drive dynamic includes, and to\n> e.g. match based on TERM=xterm in the user's environment.\n\nThanks. I think this is a reasonable thing to have (not surprising,\nsince both of those suggestion references point to me!), and it's not\nmuch of a burden to carry, even if it isn't all that commonly used.\n\nI probably would have started with a smaller set of variants (just\nequality, with a missing variable presented as an empty string). But I\ndon't think the bool/glob/exists variants are a lot of extra code or\ncomplexity.\n\n> I initially tried to implement just an \"env\" keyword, but found that\n> splitting them up was both easier to implement and to explain to\n> users. I.e. an \"env\" will need to handle an optional \"=\" for matching\n> the value, and should the semantics be if the variable exists? If it's\n> boolean?\n> \n> By splitting them up we present a less ambiguous interface to users,\n> and make it easy to extend this in the future. I didn't see any point\n> in implementing a \"/i\" variant, that only makes sense for \"gitdir\"'s\n> matching of FS paths.\n\nI had thought to extend with the operator, like:\n\n # equality\n [includeIf \"env:FOO==value\"]\n # regex\n [includeIf \"env:FOO=~v[a]l\"]\n\nBut as you note, \"=\" is somewhat problematic, and without that we can't\nuse the \"usual\" operators. Plus there's no usual operator for globbing. ;)\nSo embedding it in the name is fine by me (and mostly a bikeshed thing\nanyway).\n\nI agree we don't really need a \"/i\" variant here.\n\n> I would like syntax that used a \"=\" better, i.e. \"envIs:TERM=xterm\"\n> instead of the \"envIs:TERM:xterm\" implemented here, but the problem\n> with that is that it isn't possible to specify those sorts of config\n> keys on the command-line:\n> \n> $ git -c includeIf.someVerb:FOO:BAR.path=bar status --short\n> $ git -c includeIf.someVerb:FOO=BAR.path=bar status --short\n> error: invalid key: includeIf.someVerb:FOO\n> fatal: unable to parse command-line config\n> $\n\nYeah, it's annoying that it doesn't work, and it's nice to think about\nthat when designing the syntax. OTOH, I kind of wonder how often folks\nwould write such a thing anyway. For one-offs like this, you'd just do\nan unconditional include (or set the actual variables you care about)\nanyway. This kind of conditional stuff is much more likely to appear in\nan actual file.\n\nPlus we will be stuck with whatever syntax we design here forever.\nWhereas we may eventually provide a split version of \"-c\" that can\nhandle names with equals, at which point our syntax decision here will\njust be a historical wart. In fact, we already have \"--config-env\" that\ncan do this.\n\n> Documentation/config.txt | 35 ++++++++++++++++++++\n> config.c | 61 +++++++++++++++++++++++++++++++++--\n> t/t1305-config-include.sh | 67 +++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 161 insertions(+), 2 deletions(-)\n\nThe patch itself looks correct to me. Everything below is bikeshedding\n(as if the parts above were not!), but you may or may not find some of\nit compelling.\n\n> +`envExists`::\n> [...]\n> +`envMatch`::\n\nAs I said, these four variants seem OK. Two other variants we might\nconsider:\n\n - regex vs glob; I think globbing is probably fine for now and regex\n would be overkill. We might want to reconsider the use of the word\n \"match\" though, since I assumed it to be a regex at first.\n\n - negation; for the TERM example discussed recently, I wonder if TERM\n != xterm would be a more natural fit. I had imagined \"!=\" as the\n operator, but in your scheme, it would probably be \"!envIs\", etc.\n\n> +There is no way to match an environment variable name with any of the\n> +`env*` directives if that variable name contains a \":\" character. Such\n> +names are allowed by POSIX, but it is assumed that nobody will need to\n> +match such a variable name in practice.\n\nThat seems like a perfectly reasonable restriction.\n\nShould we allow whitespace around key names and values? E.g.:\n\n [includeIf \"env: FOO: bar\"]\n\nis IMHO more readable (even more so if we had infix operators like\n\"==\").\n\n> +static int include_by_env_exists(const char *cond, size_t cond_len)\n> +{\n> +\tchar *cfg = xstrndup(cond, cond_len);\n> +\tint ret = !!getenv(cfg);\n> +\tfree(cfg);\n> +\treturn ret;\n> +}\n\nHaving to xstrndup() in each one of these is ugly. But doing it in the\ncaller would be even uglier, as we don't discover cond/cond_len until we\nmatch via skip_prefix() in the big if/else chain. And certainly it's not\nnew to your patch anyway, just something I noticed.\n\nBTW, I notice you used xmemdupz() in one case later on. I generally\nprefer it to xstrndup() because it's more straight-forward: the string\nis this long, and it needs to be NUL-terminated. Whereas xstrndup() is\n_mostly_ equivalent, but will produce a smaller string if there are\nembedded NULs. We would not expect them in this case, so I don't think\nit matters functionally. It just seemed funny to me to see them mixed.\n\n(I actually suspect 99% or more of xstrndup() calls should just be\nxmemdupz(), and I'd be happy to consolidate and drop one of them, but it\nwould be finicky looking at each one to see if that's really true).\n\n> +static int include_by_env_match(const char *cond, size_t cond_len, int glob,\n> +\t\t\t\tint *err)\n> +{\n> +\tconst char *eq;\n> +\tconst char *value;\n> +\tconst char *env;\n> +\tchar *cfg = xstrndup(cond, cond_len);\n> +\tchar *key = NULL;\n> +\tint ret = 0;\n> +\n> +\teq = strchr(cfg, ':');\n> +\tif (!eq) {\n> +\t\t*err = error(_(\"'%s:%.*s' missing a ':' to match the value\"),\n> +\t\t\t glob ? \"envMatch\" : \"envIs\", (int)(cond_len),\n> +\t\t\t cond);\n\nYou made a string out of (cond, cond_len) already, so you could just use\n\"cfg\" here in the error (what you have isn't wrong, but I always find\n%.*s hard to read).\n\n> +\tkey = xmemdupz(cfg, eq - cfg);\n\nAnd this is the mixed xstrndup()/xmemdupz() case I mentioned. :)\n\n> static int include_condition_is_true(const struct config_options *opts,\n> -\t\t\t\t const char *cond, size_t cond_len)\n> +\t\t\t\t const char *cond, size_t cond_len,\n> +\t\t\t\t int *err)\n\nOK, we need to return not just \"true\" or \"not true\" from the return now,\nso we stuff it into an out-parameter. We could use a tri-state instead.\nOur if-else would already propagate it:\n\n> @@ -301,6 +350,14 @@ static int include_condition_is_true(const struct config_options *opts,\n> \t\treturn include_by_gitdir(opts, cond, cond_len, 1);\n> \telse if (skip_prefix_mem(cond, cond_len, \"onbranch:\", &cond, &cond_len))\n> \t\treturn include_by_branch(cond, cond_len);\n> +\telse if (skip_prefix_mem(cond, cond_len, \"envExists:\", &cond, &cond_len))\n> +\t\treturn include_by_env_exists(cond, cond_len);\n> +\telse if (skip_prefix_mem(cond, cond_len, \"envBool:\", &cond, &cond_len))\n> +\t\treturn include_by_env_bool(cond, cond_len);\n> +\telse if (skip_prefix_mem(cond, cond_len, \"envIs:\", &cond, &cond_len))\n> +\t\treturn include_by_env_match(cond, cond_len, 0, err);\n> +\telse if (skip_prefix_mem(cond, cond_len, \"envMatch:\", &cond, &cond_len))\n> +\t\treturn include_by_env_match(cond, cond_len, 1, err);\n\nBut it would mess up this:\n\n> \tif (!parse_config_key(var, \"includeif\", &cond, &cond_len, &key) &&\n> -\t (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&\n> +\t (cond && include_condition_is_true(inc->opts, cond, cond_len, &ret)) &&\n> \t !strcmp(key, \"path\"))\n> \t\tret = handle_path_include(value, inc);\n\nSo the out-parameter seems like a reasonable path.\n\nI did find it interesting that there are no other error-cases in the\nexisting conditions. They mostly just evaluate to false (including if we\ndon't recognize the condition at all). But in the cases you're catching\nhere, it really is syntactic nonsense.\n\nI guess you could define \"there is no colon\" as \"the value we'd parse\nafter it is assumed to be the empty string\" which makes the error case\ngo away. I'm not sure it really matters all that much either way in\npractice.\n\n> +test_expect_success 'conditional include, envExists:*' '\n> +\techo value >expect &&\n> +\tgit config -f envExists.cfg some.key $(cat expect) &&\n> +\n> +\ttest_must_fail git -c includeIf.envExists:VAR.path=\"$PWD/envExists.cfg\" config some.key 2>err &&\n> +\ttest_must_be_empty err &&\n\nThe tests all look sane to me. We do define some exit codes for\ngit-config here, so:\n\n test_expect_code 1 git -c ...\n\nwould be tighter (and you could probably just ditch the empty-err check\nthen).\n\nObviously avoiding the \"=\" question is beneficial here for testing via\n\"-c\". As I said earlier, I think it's more realistic to expect these in\nactual files, but I think testing them either way is fine. If you did\ntest them in a file, though, you could use a relative path in the\ninclude. Plus all three invocations could use the same file, so you\ndon't have to repeat it over and over. I.e.:\n\n test_config includeIf.envExists:VAR.path envExists.cfg &&\n\n test_expect_code 1 git config some.key &&\n\n VAR= git config some.key >actual &&\n test_cmp expect actual &&\n\n VAR=0 git config some.key >actual &&\n test_cmp expect actual\n\nshould work.\n\n-Peff\n"},{"id":"437048","messageId":"xmqqa6k1slxe.fsf@gitster.g","threadId":"56570","inReplyTo":"YU49+Y+nRhl1mgof@coredump.intra.peff.net","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-24T21:28:29Z","receivedAt":"2021-09-24T21:28:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I had thought to extend with the operator, like:\n>\n> # equality\n> [includeIf \"env:FOO==value\"]\n> # regex\n> [includeIf \"env:FOO=~v[a]l\"]\n\nYup, that matched my aesthetics better ;-)\n\n> But as you note, \"=\" is somewhat problematic, and without that we can't\n> use the \"usual\" operators. Plus there's no usual operator for globbing. ;)\n> So embedding it in the name is fine by me (and mostly a bikeshed thing\n> anyway).\n\nPerhaps. I am not sure if we deeply care about \"git -c var=val\" in\nthis case, especially since this is part of includeif, though. It\nmay be more important to keep the syntax useful and extensible for\neveryday use than for one-off \"git -c\" testing.\n\n> I agree we don't really need a \"/i\" variant here.\n\nCase insensitive environment variable names, no, but case\ninsensitive matching of values, maybe? But I'd be happy to see us\nstart very minimally (even just envEQ alone without any other\nfrills, or optionally envNE to negate it, would be fine by me).\n\n> Should we allow whitespace around key names and values? E.g.:\n>\n> [includeIf \"env: FOO: bar\"]\n>\n> is IMHO more readable (even more so if we had infix operators like\n> \"==\").\n\nThis asserts what? FOO=\" bar\"?\n"},{"id":"437052","messageId":"YU5KOpGkS5sH4iFJ@coredump.intra.peff.net","threadId":"56570","inReplyTo":"xmqqa6k1slxe.fsf@gitster.g","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T21:59:22Z","receivedAt":"2021-09-24T21:59:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 24, 2021 at 02:28:29PM -0700, Junio C Hamano wrote:\n\n> > But as you note, \"=\" is somewhat problematic, and without that we can't\n> > use the \"usual\" operators. Plus there's no usual operator for globbing. ;)\n> > So embedding it in the name is fine by me (and mostly a bikeshed thing\n> > anyway).\n> \n> Perhaps. I am not sure if we deeply care about \"git -c var=val\" in\n> this case, especially since this is part of includeif, though. It\n> may be more important to keep the syntax useful and extensible for\n> everyday use than for one-off \"git -c\" testing.\n\nYeah, see my comments later in that mail. :)\n\n> > I agree we don't really need a \"/i\" variant here.\n> \n> Case insensitive environment variable names, no, but case\n> insensitive matching of values, maybe? But I'd be happy to see us\n> start very minimally (even just envEQ alone without any other\n> frills, or optionally envNE to negate it, would be fine by me).\n\nYeah, as long as we leave the door open syntactically, I think it is OK.\n\n> > Should we allow whitespace around key names and values? E.g.:\n> >\n> > [includeIf \"env: FOO: bar\"]\n> >\n> > is IMHO more readable (even more so if we had infix operators like\n> > \"==\").\n> \n> This asserts what? FOO=\" bar\"?\n\nWhoops, that should have been \"envIs\", asserting that $FOO contains\n\"bar\".\n\nAs I said, I think it matters more with the infix operators, as:\n\n [includeIf \"env:FOO == bar\"]\n\nis more readable than:\n\n [includeIf \"env:FOO==bar\"]\n\nBut I do think:\n\n [includeIf \"envIs:FOO:bar\"]\n\nis harder to read than even:\n\n [includeIf \"envIs:FOO: bar\"]\n\n-Peff\n"},{"id":"437063","messageId":"592a799b-0d16-1615-4737-3c634d029d7f@starwolf.com","threadId":"56570","inReplyTo":"YUzvhLUmvsdF5w+r@coredump.intra.peff.net","subject":"Re: ANSI sequences produced on non-ANSI terminal","fromName":"Greywolf","fromEmail":"greywolf@starwolf.com","sentAt":"2021-09-24T23:57:11Z","receivedAt":"2021-09-24T23:57:15Z","isPatch":false,"sender":{"key":"greywolf@starwolf.com","avatar":null},"body":"Greetings and thank you ALL for your responses!\n\nOn 9/23/2021 14:20, Jeff King wrote:\n\n> Git doesn't have any kind of list of terminals, beyond knowing that \"dumb\"\n> should disable auto-color. It's possible we could expand that if there are\n> known terminals that don't understand ANSI colors. I'm a bit wary of having\n> a laundry list of obscure terminals, though.\n\nOh, gods, I wouldn't have that at all! No, I just want it NOT to spit out\nnot only the colour codes, but the cursor positioning codes as it seems\nwont to do when I do a fetch. I'm more than happy to turn coloring off\n(conditional on TERM would be a bonus, however it's done) on my own;\nin fact, I have done so, but the fetch/pull still seem to be messing up\nmy screen, with color turned off (unless I'm not turning it off\n*enough*, which is entirely possible).\n\n> If we built against ncurses or some other terminfo-aware library we could\n> outsource that, but that would be a new dependency. I'm hesitant to do that\n> even as an optional dependency given the bang-for-the-buck (and certainly\n> making it require would be right out).\n\nWell understood. Also, not asking for people to jump thru flaming hoops.\nJust trying to figure out how to get git to stop assuming things.\n(as stated, I am aware it could be my fault for not setting variables\nproperly all the way).\n\n> Obviously you can wrap Git with a script to tweak the config based on the\n> current setting of the $TERM variable. It would be nice if you could have\n> conditional config for that. E.g., something like:\n> \n> [includeIf \"env:TERM==xterm\"] path = gitconfig-color\n> \n> That doesn't exist, but would fit in reasonably well with our other \n> conditional config options.\n\nThat is a consideration; and one I had not thought of.\n\n> As far as generating non-ANSI codes, that's all Git knows how to do.\n\nJust need to have it NOT generate ANSI codes, if requested. I'm certainly\nnot requesting the world of terminals to be incorporated -- just some\nuniversal readability.\n\nAs far as the suggestion to use \"screen\", I'm not going to be starting up\na screen session every time I log in. :)\n\n\t\t\t\tThank you all very much!\n\n\t\t\t\t--*greywolf;\n"},{"id":"437067","messageId":"YU64WQOg/zY7P+Gh@coredump.intra.peff.net","threadId":"56570","inReplyTo":"592a799b-0d16-1615-4737-3c634d029d7f@starwolf.com","subject":"Re: ANSI sequences produced on non-ANSI terminal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-25T05:49:13Z","receivedAt":"2021-09-25T05:49:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 24, 2021 at 04:57:11PM -0700, Greywolf wrote:\n\n> On 9/23/2021 14:20, Jeff King wrote:\n> \n> > Git doesn't have any kind of list of terminals, beyond knowing that \"dumb\"\n> > should disable auto-color. It's possible we could expand that if there are\n> > known terminals that don't understand ANSI colors. I'm a bit wary of having\n> > a laundry list of obscure terminals, though.\n> \n> Oh, gods, I wouldn't have that at all! No, I just want it NOT to spit out\n> not only the colour codes, but the cursor positioning codes as it seems\n> wont to do when I do a fetch. I'm more than happy to turn coloring off\n> (conditional on TERM would be a bonus, however it's done) on my own;\n> in fact, I have done so, but the fetch/pull still seem to be messing up\n> my screen, with color turned off (unless I'm not turning it off\n> *enough*, which is entirely possible).\n\nOK, that makes things a bit easier. The colors, as you noticed, can be\ndisabled by config. The other thing you're seeing is ANSI ESC[K, which\nis used to clear to the end of line. We use this in a couple places,\nnotably when relaying progress lines from the server (with the \"remote:\"\nprefix) which may use carriage-returns to overwrite lines.\n\nSee ebe8fa738d (fix display overlap between remote and local progress,\n2007-11-04) if you're really interested.\n\nAnyway, there's no config option to disable that. However, we do disable\nit if TERM is empty or set to \"dumb\" (and instead just write some extra\nspaces to clear out the line). So that may be an option, though of\ncourse setting TERM=dumb may affect other programs you use.\n\nI don't think it would be unreasonable to have a config option to\nselect whether we use the ANSI or dumb-term version.\n\n> > If we built against ncurses or some other terminfo-aware library we could\n> > outsource that, but that would be a new dependency. I'm hesitant to do that\n> > even as an optional dependency given the bang-for-the-buck (and certainly\n> > making it require would be right out).\n> \n> Well understood. Also, not asking for people to jump thru flaming hoops.\n> Just trying to figure out how to get git to stop assuming things.\n> (as stated, I am aware it could be my fault for not setting variables\n> properly all the way).\n\nNah, it sounds like you actually set the variables correctly. We've just\nassumed that we can get by with ANSI codes as a lowest common\ndenominator in the modern world, without having to resort to all the\ncomplexities of using a terminfo library. It's worked pretty well so\nfar. ;)\n\n-Peff\n"},{"id":"437069","messageId":"YU7FiMwLYdrhSc0P@alpha","threadId":"56570","inReplyTo":"038801d7b0c6$f9345a90$eb9d0fb0$@nexbridge.com","subject":"Re: ANSI sequences produced on non-ANSI terminal","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2021-09-25T06:45:28Z","receivedAt":"2021-09-25T06:46:04Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Thu, Sep 23, 2021 at 06:04:20PM -0400, Randall S. Becker wrote:\n> On September 23, 2021 5:55 PM, Junio C Hamano wrote:\n> >Jeff King writes:\n> >\n> >> On Wed, Sep 22, 2021 at 10:21:22PM -0700, The Grey Wolf wrote:\n> >>\n> >>> Anything else you want to add:\n> >>> \tI searched google and the documentation as best I was able for\n> >>> \tthis, but I am unable to find anywhere that will let me disable\n> >>> \t(or enable) colour for a particular term type. Sometimes I'm on\n> >>> \tan xterm, for which this is GREAT. Sometimes I'm on a Wyse WY60,\n> >>> \tfor which this is sub-optimal. My workaround is to disable colour\n> >>> \tcompletely, which is reluctantly acceptable, but it would be nice\n> >>> \tto say \"If I'm on an xterm/aterm/urxvt/ansi terminal, enable\n> >>> \tcolour or cursor-positioning, otherwise shut it off.\" If this\n> >>> \tseems too much of a one-off to handle, fine, but most things that\n> >>> \ttalk fancy to screens are kind enough to allow an opt-out based on\n> >>> \tterminal type. :)\n> >>\n> >> Git doesn't have any kind of list of terminals, beyond knowing that\n> >> \"dumb\" should disable auto-color. It's possible we could expand that\n> >> if there are known terminals that don't understand ANSI colors. I'm a\n> >> bit wary of having a laundry list of obscure terminals, though.\n> >>\n> >> If we built against ncurses or some other terminfo-aware library we\n> >> could outsource that, but that would be a new dependency. I'm hesitant\n> >> to do that even as an optional dependency given the bang-for-the-buck\n> >> (and certainly making it require would be right out).\n> >\n> >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\n> not\n> >mistaken ;-)?\n> \n> Would something like switch in .gitconfig make a difference? Like core.colourize=false. There are situations where SSH sessions come\n> in from automation, like Jenkins and Travis, which sets term to something other than dumb by default. Coloring makes a mess of the\n> output. The ability to turn off colouring off by user might be helpful.\n> \n> -Randall\n> \n\nThat already exists: `color.ui`:\n\n> This variable determines the default value for variables such as\n> color.diff and color.grep that control the use of color per command\n> family.\n> [..]\n> Set it to false or never if you prefer Git commands not to use color\n> unless enabled explicitly with some other configuration or the --color\n> option.\n\nKevin\n"},{"id":"437150","messageId":"xmqqo88eq8um.fsf@gitster.g","threadId":"56570","inReplyTo":"YU5KOpGkS5sH4iFJ@coredump.intra.peff.net","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-27T16:30:41Z","receivedAt":"2021-09-27T16:30:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > Should we allow whitespace around key names and values? E.g.:\n>> >\n>> > [includeIf \"env: FOO: bar\"]\n>> >\n>> > is IMHO more readable (even more so if we had infix operators like\n>> > \"==\").\n>> \n>> This asserts what? FOO=\" bar\"?\n>\n> Whoops, that should have been \"envIs\", asserting that $FOO contains\n> \"bar\".\n\nOh, \"can we check with a literal with leading whitespace?\" was what\nmy question was about ;-)\n\n> As I said, I think it matters more with the infix operators, as:\n>\n> [includeIf \"env:FOO == bar\"]\n>\n> is more readable than:\n>\n> [includeIf \"env:FOO==bar\"]\n\nSure, but at that point, we'd probably want some quoting mechanism\nfor the literal to be compared, e.g.\n\n\t[includeIf \"env:PATH ~= \\\"(:|^)/usr/bin(:|$)\\\"\"]\n\n> But I do think:\n>\n> [includeIf \"envIs:FOO:bar\"]\n>\n> is harder to read than even:\n>\n> [includeIf \"envIs:FOO: bar\"]\n\nHmph, that's quite subjective, I am afraid. When I see the latter\nin the configuration file, \"do I have to have a single space before\n'bar' in the value of $FOO\" would be the first question that would\ncome to my mind.\n\nWith an understanding that our syntax is so limited that we cannot\neven write '=' and need to resort to Is: instead, I'd actually find\nthat the former less confusing than the latter.\n\nThanks.\n"},{"id":"437183","messageId":"YVImeFHxY7hmb3wY@coredump.intra.peff.net","threadId":"56570","inReplyTo":"xmqqo88eq8um.fsf@gitster.g","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-27T20:15:52Z","receivedAt":"2021-09-27T20:15:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 27, 2021 at 09:30:41AM -0700, Junio C Hamano wrote:\n\n> >> This asserts what? FOO=\" bar\"?\n> >\n> > Whoops, that should have been \"envIs\", asserting that $FOO contains\n> > \"bar\".\n> \n> Oh, \"can we check with a literal with leading whitespace?\" was what\n> my question was about ;-)\n\nMy assumption was that nobody would really care about doing so. It is\ntrue that it's less flexible, though (and is a decision we can't easily\ntake back later).\n\n> > As I said, I think it matters more with the infix operators, as:\n> >\n> > [includeIf \"env:FOO == bar\"]\n> >\n> > is more readable than:\n> >\n> > [includeIf \"env:FOO==bar\"]\n> \n> Sure, but at that point, we'd probably want some quoting mechanism\n> for the literal to be compared, e.g.\n> \n> \t[includeIf \"env:PATH ~= \\\"(:|^)/usr/bin(:|$)\\\"\"]\n\nIck. The extra quoting of the internal double-quotes is pretty horrid to\nlook at. Also, how does one match a double-quote in the value? \\\\\\\"?\n\nIf it were optional, that would make the common cases easy (no dq, no\nwhitespace), and the hard ones possible.\n\nI think this is getting into a bit of a digression, though. I'm willing\nto defer to Ævar, who is doing the actual work, and I don't know if he\nhas found any of this compelling. ;)\n\n> > But I do think:\n> >\n> > [includeIf \"envIs:FOO:bar\"]\n> >\n> > is harder to read than even:\n> >\n> > [includeIf \"envIs:FOO: bar\"]\n> \n> Hmph, that's quite subjective, I am afraid. When I see the latter\n> in the configuration file, \"do I have to have a single space before\n> 'bar' in the value of $FOO\" would be the first question that would\n> come to my mind.\n\nI think it's just the mashed-up colons that I find ugly in the first\none. But I agree the latter isn't that nice either, and introduces the\nambiguity you describe.\n\n> With an understanding that our syntax is so limited that we cannot\n> even write '=' and need to resort to Is: instead, I'd actually find\n> that the former less confusing than the latter.\n\nThat I think is the most interesting question: is the \"=\" actually\nout-of-bounds? I tend to think not, based on our responses earlier in\nthe thread.\n\n-Peff\n"},{"id":"437190","messageId":"00ee01d7b3e1$ceb06840$6c1138c0$@nexbridge.com","threadId":"56570","inReplyTo":"YVImeFHxY7hmb3wY@coredump.intra.peff.net","subject":"RE: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-09-27T20:53:59Z","receivedAt":"2021-09-27T20:54:35Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On September 27, 2021 4:16 PM, Jeff King wrote:\n>Subject: Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}\n>\n>On Mon, Sep 27, 2021 at 09:30:41AM -0700, Junio C Hamano wrote:\n>\n>> >> This asserts what? FOO=\" bar\"?\n>> >\n>> > Whoops, that should have been \"envIs\", asserting that $FOO contains\n>> > \"bar\".\n>>\n>> Oh, \"can we check with a literal with leading whitespace?\" was what my\n>> question was about ;-)\n>\n>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\n>take back later).\n>\n>> > As I said, I think it matters more with the infix operators, as:\n>> >\n>> > [includeIf \"env:FOO == bar\"]\n>> >\n>> > is more readable than:\n>> >\n>> > [includeIf \"env:FOO==bar\"]\n>>\n>> Sure, but at that point, we'd probably want some quoting mechanism for\n>> the literal to be compared, e.g.\n>>\n>> \t[includeIf \"env:PATH ~= \\\"(:|^)/usr/bin(:|$)\\\"\"]\n>\n>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? \\\\\\\"?\n>\n>If it were optional, that would make the common cases easy (no dq, no whitespace), and the hard ones possible.\n>\n>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\n>found any of this compelling. ;)\n\nWhat about something like:\n\n\t[includeIf \"env:PATH ~= '^(.*😊)/usr/bin(:.*)*$' \"]\n\nUsing 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.\n-Randall\n\n"},{"id":"437197","messageId":"YVI5rYamHBkGQ/jy@coredump.intra.peff.net","threadId":"56570","inReplyTo":"00ee01d7b3e1$ceb06840$6c1138c0$@nexbridge.com","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-27T21:37:49Z","receivedAt":"2021-09-27T21:37:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 27, 2021 at 04:53:59PM -0400, Randall S. Becker wrote:\n\n> What about something like:\n> \n> \t[includeIf \"env:PATH ~= '^(.*😊)/usr/bin(:.*)*$' \"]\n> \n> Using single quotes and a full regex pattern instead of trying to\n> provide a syntax to extract a pattern and then match. One call to\n> regexec() would be easier. Then escaping is regcomp's problem\n> (mostly). Potentially, you could even remove the outer \", but that\n> would be wonky. You could omit the ^ and $ by default assuming a full\n> match.\n\nI almost suggested that, but then...how do you put single-quotes in your\npattern? You can backslash-escape them, but:\n\n - do you need to escape the backslash to get it through the config\n parser intact?\n\n - it seems extra funny to me because single quotes usually imply a\n lack of interpolation\n\n-Peff\n"},{"id":"437200","messageId":"00f701d7b3ea$7eb59eb0$7c20dc10$@nexbridge.com","threadId":"56570","inReplyTo":"YVI5rYamHBkGQ/jy@coredump.intra.peff.net","subject":"RE: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-09-27T21:56:10Z","receivedAt":"2021-09-27T21:56:43Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On September 27, 2021 5:38 PM, Jeff King wrote:\n>Subject: Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}\n>\n>On Mon, Sep 27, 2021 at 04:53:59PM -0400, Randall S. Becker wrote:\n>\n>> What about something like:\n>>\n>> \t[includeIf \"env:PATH ~= '^(.*😊)/usr/bin(:.*)*$' \"]\n>>\n>> Using single quotes and a full regex pattern instead of trying to\n>> provide a syntax to extract a pattern and then match. One call to\n>> regexec() would be easier. Then escaping is regcomp's problem\n>> (mostly). Potentially, you could even remove the outer \", but that\n>> would be wonky. You could omit the ^ and $ by default assuming a full\n>> match.\n>\n>I almost suggested that, but then...how do you put single-quotes in your pattern? You can backslash-escape them, but:\n>\n> - do you need to escape the backslash to get it through the config\n> parser intact?\n>\n> - it seems extra funny to me because single quotes usually imply a\n> lack of interpolation\n\nExactly 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.\n\n"},{"id":"437212","messageId":"87lf3hzhkr.fsf@evledraar.gmail.com","threadId":"56570","inReplyTo":"YVImeFHxY7hmb3wY@coredump.intra.peff.net","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-27T23:52:26Z","receivedAt":"2021-09-28T00:09:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 27 2021, Jeff King wrote:\n\n> On Mon, Sep 27, 2021 at 09:30:41AM -0700, Junio C Hamano wrote:\n>\n>> >> This asserts what? FOO=\" bar\"?\n>> >\n>> > Whoops, that should have been \"envIs\", asserting that $FOO contains\n>> > \"bar\".\n>> \n>> Oh, \"can we check with a literal with leading whitespace?\" was what\n>> my question was about ;-)\n>\n> My assumption was that nobody would really care about doing so. It is\n> true that it's less flexible, though (and is a decision we can't easily\n> take back later).\n\nYeah, I think nobody really cares about stripspace() or not for this\nsort of feature.\n\nI do think having implicit and explicit complexity like that makes it\nharder to document, implement and understand for users though. I.e. is\n\"env:FOO == bar\" the same as \"env:FOO ==bar\" etc., what whitespace\nexactly is accepted etc.\n\n>> > As I said, I think it matters more with the infix operators, as:\n>> >\n>> > [includeIf \"env:FOO == bar\"]\n>> >\n>> > is more readable than:\n>> >\n>> > [includeIf \"env:FOO==bar\"]\n>> \n>> Sure, but at that point, we'd probably want some quoting mechanism\n>> for the literal to be compared, e.g.\n>> \n>> \t[includeIf \"env:PATH ~= \\\"(:|^)/usr/bin(:|$)\\\"\"]\n>\n> Ick. The extra quoting of the internal double-quotes is pretty horrid to\n> look at. Also, how does one match a double-quote in the value? \\\\\\\"?\n>\n> If it were optional, that would make the common cases easy (no dq, no\n> whitespace), and the hard ones possible.\n\nAn implicit assumption of mine in the simpler positive-match-only\nversion (which I should have made clear) is that anyone who needs this\nsort of complexity can just arrange to wrap their \"git\" in a function,\nor do this sort of thing in their ~/.bashrc, i.e. just:\n\n if code_of_arbitrary_complexity\n then\n export GIT_DO_XYZ_INCLUDES=1\n fi\n\nThen in your config:\n\n includeIf.envBool:GIT_DO_XYZ_INCLUDES.path=~/.gitconfig.d/xyz.cfg\n\nAnd having written that out I think the best thing to do is probably to\nhave a version that only does the envExists and envBool version (or just\nenvBool), and skip envIs and envMatch entirely.\n\nIn the case of env:PATH we're just setting users up for some buggy or\nunexpected interaction with something that would be better done either\nvia a gitdir include, or if they really need $PATH they can just wrap\n\"git\" in a function that sets a boolean inclusion variable.\n\nThat would get us out of having to support emergent behavior where some\ngit tool invoked via run_command() or something chdir's somewhere as an\nimplementation detail, and such an env:PATH match means we'd need to\nsupport that, or potentially break existing user config.\n\nOr, since we might not be invoked via a shell, the same issue with a\n$PATH being \"stale\" from the POV of a user who's wondering why say a\ncommand like:\n\n # status in the \"t\" subdirectory\n git -C t \n\nDoesn't have the \"right\" $PWD, which we might not have as some future\nshortcut in decided not to bother chdir()-ing to answer the user's\n.\n\n> I think this is getting into a bit of a digression, though. I'm willing\n> to defer to Ævar, who is doing the actual work, and I don't know if he\n> has found any of this compelling. ;)\n>\n>> > But I do think:\n>> >\n>> > [includeIf \"envIs:FOO:bar\"]\n>> >\n>> > is harder to read than even:\n>> >\n>> > [includeIf \"envIs:FOO: bar\"]\n>> \n>> Hmph, that's quite subjective, I am afraid. When I see the latter\n>> in the configuration file, \"do I have to have a single space before\n>> 'bar' in the value of $FOO\" would be the first question that would\n>> come to my mind.\n>\n> I think it's just the mashed-up colons that I find ugly in the first\n> one. But I agree the latter isn't that nice either, and introduces the\n> ambiguity you describe.\n\nFWIW I hacked up a --config-key --config-value pairing so you could set\nkeys with \"=\" in them on the command-line, I'm not sure I like the\ninterface, but it gets rid of that \":\" v.s. \"=\" edge case:\nhttps://github.com/avar/git/commit/a86053df48b\n\n>> With an understanding that our syntax is so limited that we cannot\n>> even write '=' and need to resort to Is: instead, I'd actually find\n>> that the former less confusing than the latter.\n>\n> That I think is the most interesting question: is the \"=\" actually\n> out-of-bounds? I tend to think not, based on our responses earlier in\n> the thread.\n"},{"id":"437213","messageId":"xmqqee99mtsk.fsf@gitster.g","threadId":"56570","inReplyTo":"YVImeFHxY7hmb3wY@coredump.intra.peff.net","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-28T00:24:11Z","receivedAt":"2021-09-28T00:24:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Sure, but at that point, we'd probably want some quoting mechanism\n>> for the literal to be compared, e.g.\n>> \n>> \t[includeIf \"env:PATH ~= \\\"(:|^)/usr/bin(:|$)\\\"\"]\n>\n> Ick. The extra quoting of the internal double-quotes is pretty horrid to\n> look at. Also, how does one match a double-quote in the value? \\\\\\\"?\n\nIck indeed. I didn't mean to say you must always dq quote. It\nstarted more like \n\n\tincludeIf \"env:VAR == ' value with leading whitespace'\"\n\n(or use \\\" inside \"\"-pair to mean a double-quote) as a demonstration\nof an escape hatch needed if we took your \"let's by default strip\nthe whitespace around the value\" example in the message I was\nresponding to.\n\nJust like we in most cases do not have to quote the value in the\nconfiguration files, unless you have strange needs like wanting to\nexpress a value with leading whitespace that should not be stripped,\nif we were to go this route, \n\n> If it were optional, that would make the common cases easy (no dq, no\n> whitespace), and the hard ones possible.\n\nYup.\n"},{"id":"437217","messageId":"YVJkx2HMf9WlPx6G@coredump.intra.peff.net","threadId":"56570","inReplyTo":"87lf3hzhkr.fsf@evledraar.gmail.com","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T00:41:43Z","receivedAt":"2021-09-28T00:41:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 01:52:26AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> An implicit assumption of mine in the simpler positive-match-only\n> version (which I should have made clear) is that anyone who needs this\n> sort of complexity can just arrange to wrap their \"git\" in a function,\n> or do this sort of thing in their ~/.bashrc, i.e. just:\n> \n> if code_of_arbitrary_complexity\n> then\n> export GIT_DO_XYZ_INCLUDES=1\n> fi\n> \n> Then in your config:\n> \n> includeIf.envBool:GIT_DO_XYZ_INCLUDES.path=~/.gitconfig.d/xyz.cfg\n> \n> And having written that out I think the best thing to do is probably to\n> have a version that only does the envExists and envBool version (or just\n> envBool), and skip envIs and envMatch entirely.\n\nI'm not sure I agree. If you are willing to wrap git, then you can just\nadd:\n\n git -c include.path=~/.gitconfig.d/xyz.cfg\n\nto the command-line in the first place. Or if you're willing to use our\nundocumented interface, you can even do it in your .bashrc:\n\n if code_of_arbitrary_complexity\n then\n GIT_CONFIG_PARAMETERS=\"'include.path'='~/.gitconfig.d/xyz.cfg'\"\n fi\n\nThe value of this env matching is that it is done at run-time without\nwrapping, and can meaningfully inspect the state of the world. E.g., the\n$TERM thing that started this thread.\n\n> In the case of env:PATH we're just setting users up for some buggy or\n> unexpected interaction with something that would be better done either\n> via a gitdir include, or if they really need $PATH they can just wrap\n> \"git\" in a function that sets a boolean inclusion variable.\n\nYes, I have trouble imagining why any matching on env:PATH would be\nuseful (or $PWD, since we have the much less confusing gitdir\nconditional). Which isn't to say I want to forbid it, but just because\npeople can shoot themselves in the foot with complexity doesn't mean\nthat \"envIs\" is a bad thing when it's not misused.\n\n> > I think it's just the mashed-up colons that I find ugly in the first\n> > one. But I agree the latter isn't that nice either, and introduces the\n> > ambiguity you describe.\n> \n> FWIW I hacked up a --config-key --config-value pairing so you could set\n> keys with \"=\" in them on the command-line, I'm not sure I like the\n> interface, but it gets rid of that \":\" v.s. \"=\" edge case:\n> https://github.com/avar/git/commit/a86053df48b\n\nYeah, we talked about that a while ago, but nobody liked the interface\nenough to actually code it (and as far as I know, it's really\ntheoretical; nobody has actually wanted to set such an option from the\ncommand-line yet, and we have the --config-env stuff for people who want\nto robustly pass along arbitrary keys).\n\nA perhaps more subtle but less awkward to type version is to just\nrequire two arguments, like:\n\n git --config ...\n\nbut I'd just as soon continue to leave it un-implemented if nobody has\nactually needed it in practice.\n\n-Peff\n"},{"id":"437243","messageId":"878rzhz9yw.fsf@evledraar.gmail.com","threadId":"56570","inReplyTo":"YVJkx2HMf9WlPx6G@coredump.intra.peff.net","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T02:42:51Z","receivedAt":"2021-09-28T02:54:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 27 2021, Jeff King wrote:\n\n> On Tue, Sep 28, 2021 at 01:52:26AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> An implicit assumption of mine in the simpler positive-match-only\n>> version (which I should have made clear) is that anyone who needs this\n>> sort of complexity can just arrange to wrap their \"git\" in a function,\n>> or do this sort of thing in their ~/.bashrc, i.e. just:\n>> \n>> if code_of_arbitrary_complexity\n>> then\n>> export GIT_DO_XYZ_INCLUDES=1\n>> fi\n>> \n>> Then in your config:\n>> \n>> includeIf.envBool:GIT_DO_XYZ_INCLUDES.path=~/.gitconfig.d/xyz.cfg\n>> \n>> And having written that out I think the best thing to do is probably to\n>> have a version that only does the envExists and envBool version (or just\n>> envBool), and skip envIs and envMatch entirely.\n>\n> I'm not sure I agree. If you are willing to wrap git, then you can just\n> add:\n>\n> git -c include.path=~/.gitconfig.d/xyz.cfg\n>\n> to the command-line in the first place. Or if you're willing to use our\n> undocumented interface, you can even do it in your .bashrc:\n>\n> if code_of_arbitrary_complexity\n> then\n> GIT_CONFIG_PARAMETERS=\"'include.path'='~/.gitconfig.d/xyz.cfg'\"\n> fi\n\nSort of, that'll give you unconditional inclusion, but won't e.g. handle\na case where the env include only runs in some .git/config, or depending\non other inclusion (e.g. in ~/dev/git, but only if XYZ env var).\n\nBut yeah, it won't handle all potential cases. I figured for this sort\nof thing it was better to start small and see if the provided interface\nwas enough..\n\n> The value of this env matching is that it is done at run-time without\n> wrapping, and can meaningfully inspect the state of the world. E.g., the\n> $TERM thing that started this thread.\n\nYeah, maybe we should have at least an ifStrEQ, whatever we call it...\n\n>> In the case of env:PATH we're just setting users up for some buggy or\n>> unexpected interaction with something that would be better done either\n>> via a gitdir include, or if they really need $PATH they can just wrap\n>> \"git\" in a function that sets a boolean inclusion variable.\n>\n> Yes, I have trouble imagining why any matching on env:PATH would be\n> useful (or $PWD, since we have the much less confusing gitdir\n> conditional). Which isn't to say I want to forbid it, but just because\n> people can shoot themselves in the foot with complexity doesn't mean\n> that \"envIs\" is a bad thing when it's not misused.\n\nI'm biased by past on-list discussions where existing behavior, no\nmatter if unintentional or emergent can be really hard to fix once\nestablished.\n\n>> > I think it's just the mashed-up colons that I find ugly in the first\n>> > one. But I agree the latter isn't that nice either, and introduces the\n>> > ambiguity you describe.\n>> \n>> FWIW I hacked up a --config-key --config-value pairing so you could set\n>> keys with \"=\" in them on the command-line, I'm not sure I like the\n>> interface, but it gets rid of that \":\" v.s. \"=\" edge case:\n>> https://github.com/avar/git/commit/a86053df48b\n>\n> Yeah, we talked about that a while ago, but nobody liked the interface\n> enough to actually code it (and as far as I know, it's really\n> theoretical; nobody has actually wanted to set such an option from the\n> command-line yet, and we have the --config-env stuff for people who want\n> to robustly pass along arbitrary keys).\n>\n> A perhaps more subtle but less awkward to type version is to just\n> require two arguments, like:\n>\n> git --config ...\n\nI suppose --config would work like that, you can'd to it with \"-c\". I\nthink it's more confusing to have a \"-c\" and \"--config\" which unlike\nmost other things don't follow the obvious long and short option names\nworking the same way.\n\n> but I'd just as soon continue to leave it un-implemented if nobody has\n> actually needed it in practice.\n\n*nod*. I do think it's bad design to introduce an \"env\" inclusion\nfeature that relies on \"=\" though while we don't have something like\nthat, i.e.\n\nI think we should probably not add that --config-{key,value}, but\navoiding the arbitrary limitation of not being able to specify certain\nconfig keys seems prudent in that case, and since the \"=\" v.s. \":\" is\nonly an aesthetic preference I think being able to compose things\nwithout limitations wins out.\n\nWe do have the \"=\" key limitation now, but I don't think it's there for\nany key we currently define, except things like \"url..insteadOf\"\nif the \" has a \"=\" in it (and maybe just that one).\n"},{"id":"437250","messageId":"YVKrRooSIN7OeLy9@coredump.intra.peff.net","threadId":"56570","inReplyTo":"878rzhz9yw.fsf@evledraar.gmail.com","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T05:42:30Z","receivedAt":"2021-09-28T05:42:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 04:42:51AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > A perhaps more subtle but less awkward to type version is to just\n> > require two arguments, like:\n> >\n> > git --config ...\n> \n> I suppose --config would work like that, you can'd to it with \"-c\". I\n> think it's more confusing to have a \"-c\" and \"--config\" which unlike\n> most other things don't follow the obvious long and short option names\n> working the same way.\n\nYeah, probably \"--config-pair\" or something might be less confusing.\nAnyway...\n\n> > but I'd just as soon continue to leave it un-implemented if nobody has\n> > actually needed it in practice.\n> \n> *nod*. I do think it's bad design to introduce an \"env\" inclusion\n> feature that relies on \"=\" though while we don't have something like\n> that, i.e.\n> \n> I think we should probably not add that --config-{key,value}, but\n> avoiding the arbitrary limitation of not being able to specify certain\n> config keys seems prudent in that case, and since the \"=\" v.s. \":\" is\n> only an aesthetic preference I think being able to compose things\n> without limitations wins out.\n\nI don't really agree with that. Whatever syntax we use now, we'll be\nstuck with forever. It seems a shame to predicate that choice only on\nthe \"-c doesn't support =\" thing that nobody has actually run across in\npractice (and I don't think is something people will run into with\nthis).\n\n> We do have the \"=\" key limitation now, but I don't think it's there for\n> any key we currently define, except things like \"url..insteadOf\"\n> if the \" has a \"=\" in it (and maybe just that one).\n\nIt's really a potential problem for any 3-level config key. So urls,\nbranch names, remote names, various tool names, filter/diff drivers,\nexisting includeIf conditions. This might be the first one where we\nreally _encourage_ the use of \"=\" signs, but it still strikes me as\nweird that you'd want to do so on the command-line in practice.\n\n-Peff\n"},{"id":"437345","messageId":"87bl4cwkr8.fsf@evledraar.gmail.com","threadId":"56570","inReplyTo":"YVKrRooSIN7OeLy9@coredump.intra.peff.net","subject":"Re: [PATCH] config: add an includeIf.env{Exists,Bool,Is,Match}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T19:28:25Z","receivedAt":"2021-09-28T19:41:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Jeff King wrote:\n\n> On Tue, Sep 28, 2021 at 04:42:51AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > A perhaps more subtle but less awkward to type version is to just\n>> > require two arguments, like:\n>> >\n>> > git --config ...\n>> \n>> I suppose --config would work like that, you can'd to it with \"-c\". I\n>> think it's more confusing to have a \"-c\" and \"--config\" which unlike\n>> most other things don't follow the obvious long and short option names\n>> working the same way.\n>\n> Yeah, probably \"--config-pair\" or something might be less confusing.\n> Anyway...\n\n*nod*\n\n>> > but I'd just as soon continue to leave it un-implemented if nobody has\n>> > actually needed it in practice.\n>> \n>> *nod*. I do think it's bad design to introduce an \"env\" inclusion\n>> feature that relies on \"=\" though while we don't have something like\n>> that, i.e.\n>> \n>> I think we should probably not add that --config-{key,value}, but\n>> avoiding the arbitrary limitation of not being able to specify certain\n>> config keys seems prudent in that case, and since the \"=\" v.s. \":\" is\n>> only an aesthetic preference I think being able to compose things\n>> without limitations wins out.\n>\n> I don't really agree with that. Whatever syntax we use now, we'll be\n> stuck with forever. It seems a shame to predicate that choice only on\n> the \"-c doesn't support =\" thing that nobody has actually run across in\n> practice (and I don't think is something people will run into with\n> this).\n\nYeah, anyway. I don't care much either way, and not enough to drive this\nforward. I.e. it seemed like an easy thing to hack up, but if anyone\nelse is interested in driving it forward...\n\n>> We do have the \"=\" key limitation now, but I don't think it's there for\n>> any key we currently define, except things like \"url..insteadOf\"\n>> if the \" has a \"=\" in it (and maybe just that one).\n>\n> It's really a potential problem for any 3-level config key. So urls,\n> branch names, remote names, various tool names, filter/diff drivers,\n> existing includeIf conditions. This might be the first one where we\n> really _encourage_ the use of \"=\" signs, but it still strikes me as\n> weird that you'd want to do so on the command-line in practice.\n\nJust for future reference:\n\nI think given the discussion in the thread, and particularly if we're\ngoing to have some regex syntax for these keys that the artificial\nstraitjacket of putting this all in one config key is something we\nshould just do away with.\n\nI.e. I'm not proposing a *specific* schema other than noting that\nthere's no law that forces us to take say Junio's (in\nhttps://lore.kernel.org/git/xmqqo88eq8um.fsf@gitster.g/):\n\n [includeIf \"env:PATH ~= \\\"(:|^)/usr/bin(:|$)\\\"\"]\n\nOver say:\n\n [includeCondition]\n type = envRegex\n envVariable = PATH\n envRegex = \"(:|^)/usr/bin(:|$)\"\n path = ~/.gitconfig.d/env-stuff.cfg\n\nOr whatever, i.e. the state machine of seeing an \"includeCondition\" in\nthe config's event parser, and then erroring unless the next N config\nkeys satisfy some mandatory minimum set of config keys is rather simple.\n\nWe could then make any such syntax optional for existing constructs,\ni.e. you could write:\n\n [includeIf \"gitdir:/path/to/group/\"]\n path = /path/to/foo.inc\n\nAs:\n\n [includeCondition]\n type = gitdir\n path = /path/to/foo.inc\n gitdirPath = /path/to/group/\n\nOr something. And say add \"includeCondition.negated = true\" to that for\na \"!=\" match.\n\nThe shorthand syntax could then be omitted for anything new if it's\ndeemed too gnarly to represent it all in one key.\n\nThe key names & schema is something I came up with offhand, please don't\nread too much into it. The point is that we don't need to make it all\none key.\n\nThat approach also fits nicely in with the rest of the config framework,\ni.e. you can incrementally edit things using \"git config\" options, and\nwe could add a \"--type regex\" or whatever which we could then\nset/validate say the \"includeCondition.envRegex\" key with.\n"},{"id":"437747","messageId":"c6c853d8-990a-2085-5e91-ced12536c125@starwolf.com","threadId":"56570","inReplyTo":"YU64WQOg/zY7P+Gh@coredump.intra.peff.net","subject":"Re: ANSI sequences produced on non-ANSI terminal","fromName":"Greywolf","fromEmail":"greywolf@starwolf.com","sentAt":"2021-10-01T23:17:04Z","receivedAt":"2021-10-01T23:17:08Z","isPatch":false,"sender":{"key":"greywolf@starwolf.com","avatar":null},"body":"On 9/24/2021 22:49, Jeff King wrote:\n\n> OK, that makes things a bit easier. The colors, as you noticed, can be \n> disabled by config. The other thing you're seeing is ANSI ESC[K, which is \n> used to clear to the end of line. We use this in a couple places, notably \n> when relaying progress lines from the server (with the \"remote:\" prefix) \n> which may use carriage-returns to overwrite lines.\n\nThose would be some of the culprits. I'll have to do a 'script' and see what\nit is spitting out.\n\n> Anyway, there's no config option to disable that. However, we do disable\n> it if TERM is empty or set to \"dumb\" (and instead just write some extra\n> spaces to clear out the line). So that may be an option, though of course\n> setting TERM=dumb may affect other programs you use.\n\nEditors, in particular, tend not to like interacting with TERM=dumb as they \nhave no idea how to behave around one (\"I need CE and UP\" comes to mind).\n\n> I don't think it would be unreasonable to have a config option to select \n> whether we use the ANSI or dumb-term version.\n\nGIT_TERM might be an option to override TERM, but I am loath to actually\nsuggest YAEV.\n\n> Nah, it sounds like you actually set the variables correctly. We've just \n> assumed that we can get by with ANSI codes as a lowest common denominator \n> in the modern world, without having to resort to all the complexities of \n> using a terminfo library. It's worked pretty well so far. ;)\n\nLaughing out loud at that. Part of me is apologetic to be The Weird Kid.\nThe other part of me is looking for more ways to be weird.\n\nThank you for taking a look at this.\n\n> \n> -Peff\n> \n\n\t\t\t\tCheers,\n\n\t\t\t\t--*greywolf;\n"}]}