{"thread":{"id":"52702","subject":"[PATCH v2 1/1] config: learn the --stdin option for instructions","startedAt":"2020-01-27T10:17:00Z","lastAt":"2020-01-29T02:37:06Z","messageCount":9,"participants":["Zeger-Jan van de Weg","Christian Couder","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"390549","messageId":"20200127100933.10765-2-git@zjvandeweg.nl","threadId":"52702","inReplyTo":"20200127100933.10765-1-git@zjvandeweg.nl","subject":"[PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Zeger-Jan van de Weg","fromEmail":"git@zjvandeweg.nl","sentAt":"2020-01-27T10:09:33Z","receivedAt":"2020-01-27T10:17:00Z","isPatch":true,"sender":{"key":"git@zjvandeweg.nl","avatar":null},"body":"When setting values in the git config, the value is part of the\narguments for execution. This potentially leaks the value through\nlogging, or other programs like `ps`.\n\nAdd the `--stdin` option that reads from stdin for instructions to set\nand unset values to hide them from prying eyes. The instructions are based\non the `update-ref` DSL, and accept the set and unset commands.\n\nSigned-off-by: Zeger-Jan van de Weg <git@zjvandeweg.nl>\n---\n Documentation/git-config.txt |  29 ++++++++++\n builtin/config.c             | 104 +++++++++++++++++++++++++++++++++++\n t/t1300-config.sh            |  60 ++++++++++++++++++++\n 3 files changed, 193 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 899e92a1c9..9f7462284d 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -23,6 +23,7 @@ SYNOPSIS\n 'git config' [<file-option>] [--show-origin] [-z|--null] [--name-only] -l | --list\n 'git config' [<file-option>] --get-color name [default]\n 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]\n+'git config' [<file-option>] [-z|--null] --stdin\n 'git config' [<file-option>] -e | --edit\n \n DESCRIPTION\n@@ -212,6 +213,10 @@ Valid `<type>`'s include:\n \toutput without getting confused e.g. by values that\n \tcontain line breaks.\n \n+--stdin::\n+\tInstructions are read from the standard input, instead of the command\n+\tline interface. Refer to the STDIN section.\n+\n --name-only::\n \tOutput only the names of config variables for `--list` or\n \t`--get-regexp`.\n@@ -259,6 +264,30 @@ Valid `<type>`'s include:\n   When using `--get`, and the requested variable is not found, behave as if\n   <value> were the value assigned to the that variable.\n \n+STDIN\n+-----\n+\n+With `--stdin`, config reads instructions from standard input and performs\n+all modifications in sequence.\n+\n+Specify commands of the form:\n+\n+\tset SP <key> SP <newvalue>\n+\tunset SP <key>\n+\n+Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without\n+quoting:\n+\n+\tset SP <key> NULL <newvalue>\n+\tunset SP <key>\n+\n+set::\n+\tSet or update the value for <key> to <newvalue>.\n+\n+unset:\n+\tRemove the value from the configuration by <key>, when the <key> isn't\n+\tpresent in the configuration no error is returned.\n+\n CONFIGURATION\n -------------\n `pager.config` is only respected when listing configuration, i.e., when\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 98d65bc0ad..a449b00b65 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -50,6 +50,7 @@ static int show_origin;\n #define ACTION_GET_COLOR (1<<13)\n #define ACTION_GET_COLORBOOL (1<<14)\n #define ACTION_GET_URLMATCH (1<<15)\n+#define ACTION_STDIN (1<<16)\n \n /*\n  * The actions \"ACTION_LIST | ACTION_GET_*\" which may produce more than\n@@ -143,6 +144,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT('e', \"edit\", &actions, N_(\"open an editor\"), ACTION_EDIT),\n \tOPT_BIT(0, \"get-color\", &actions, N_(\"find the color configured: slot [default]\"), ACTION_GET_COLOR),\n \tOPT_BIT(0, \"get-colorbool\", &actions, N_(\"find the color setting: slot [stdout-is-tty]\"), ACTION_GET_COLORBOOL),\n+\tOPT_BIT(0, \"stdin\", &actions, N_(\"input changes from stdin\"), ACTION_STDIN),\n \tOPT_GROUP(N_(\"Type\")),\n \tOPT_CALLBACK('t', \"type\", &type, \"\", N_(\"value is given this type\"), option_parse_type),\n \tOPT_CALLBACK_VALUE(0, \"bool\", &type, N_(\"value is \\\"true\\\" or \\\"false\\\"\"), TYPE_BOOL),\n@@ -594,6 +596,102 @@ static char *default_user_config(void)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n+static char *parse_key_or_value(const char **next, const char *max_char) {\n+\tstruct strbuf token = STRBUF_INIT;\n+\n+\tif (term) {\n+\t\tif (**next == '\"') {\n+\t\t\tconst char **orig = next;\n+\n+\t\t\tif (unquote_c_style(&token, *next, next))\n+\t\t\t\tdie(\"badly quoted argument: %s\", *orig);\n+\t\t\tif (*next && !isspace(**next))\n+\t\t\t\tdie(\"unexpected character after quoted argument: %s\", *orig);\n+\t\t} else {\n+\t\t\twhile (*next && !isspace(**next)) {\n+\t\t\t\tif (*next > max_char)\n+\t\t\t\t\tdie(\"unexpected end of buffer\");\n+\n+\t\t\t\tstrbuf_addch(&token, *(*next)++);\n+\t\t\t}\n+\t\t}\n+\n+\t\t(*next)++;\n+\t} else {\n+\t\tstrbuf_addstr(&token, *next);\n+\t\t*next += token.len + 1;\n+\t}\n+\n+\treturn strbuf_detach(&token, NULL);\n+}\n+\n+static const char *parse_cmd_set(const char *next, const char *max_char)\n+{\n+\tchar *key, *value;\n+\tint ret;\n+\n+\tkey = parse_key_or_value(&next, max_char);\n+\tif (!key)\n+\t\tdie(_(\"set: missing key\"));\n+\n+\tvalue = parse_key_or_value(&next, max_char);\n+\tif (!value)\n+\t\tdie(_(\"set: missing value\"));\n+\n+\tret = git_config_set_in_file_gently(given_config_source.file, key, value);\n+\tif (ret)\n+\t\tdie(_(\"cannot set key value pair: %d\"), ret);\n+\n+\tfree(key);\n+\tfree(value);\n+\treturn next;\n+}\n+\n+\n+static const char *parse_cmd_unset(const char *next, const char *max_char)\n+{\n+\tchar *key;\n+\tint ret;\n+\n+\tkey = parse_key_or_value(&next, max_char);\n+\tif (!key)\n+\t\tdie(_(\"no key found to unset\"));\n+\n+\tret = git_config_set_in_file_gently(given_config_source.file, key, NULL);\n+\tif (ret)\n+\t\tdie(_(\"cannot unset key: %d\"), ret);\n+\n+\tfree(key);\n+\treturn next;\n+}\n+\n+static void update_config_stdin()\n+{\n+\tstruct strbuf input = STRBUF_INIT;\n+\tconst char *next;\n+\tconst char *max_char;\n+\n+\tif (strbuf_read(&input, 0, 1000) < 0)\n+\t\tdie_errno(_(\"could not read from stdin\"));\n+\tnext = input.buf;\n+\n+\tmax_char = input.buf + input.len;\n+\twhile(next < max_char) {\n+\t\tif (*next == term)\n+\t\t\tdie(_(\"empty command in input\"));\n+\t\telse if (isspace(*next))\n+\t\t\tdie(_(\"whitespace before command\"));\n+\t\telse if (skip_prefix(next, \"set \", &next))\n+\t\t\tnext = parse_cmd_set(next, max_char);\n+\t\telse if (skip_prefix(next, \"unset \", &next))\n+\t\t\tnext = parse_cmd_unset(next, max_char);\n+\t\telse\n+\t\t\tdie(_(\"unknown command %s\"), next);\n+\t}\n+\n+\tstrbuf_release(&input);\n+}\n+\n int cmd_config(int argc, const char **argv, const char *prefix)\n {\n \tint nongit = !startup_info->have_repository;\n@@ -867,6 +965,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\tcolor_stdout_is_tty = git_config_bool(\"command line\", argv[1]);\n \t\treturn get_colorbool(argv[0], argc == 2);\n \t}\n+\telse if (actions == ACTION_STDIN) {\n+\t\tcheck_write();\n+\t\tcheck_argc(argc, 0, 0);\n+\n+\t\tupdate_config_stdin();\n+\t}\n \n \treturn 0;\n }\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 983a0a1583..d8ad82922d 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -380,6 +380,66 @@ test_expect_success '--add' '\n \ttest_cmp expect output\n '\n \n+test_expect_success '--stdin to set a value' '\n+\techo \"set foo.mepmep true\" | git config --stdin &&\n+\tgit config --get foo.mepmep >output &&\n+\techo true >expect &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--stdin multiline support' '\n+\tcat >stdin <<-EOF &&\n+\tset bar.mepmep true\n+\tset foo.mepmep true\n+\tEOF\n+\tgit config --stdin <stdin &&\n+\tgit config --get bar.mepmep >output &&\n+\tgit config --get foo.mepmep >>output &&\n+\techo -e \"true\\ntrue\" > expect &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--stdin hides input on errors' '\n+\tcat >stdin <<-EOF &&\n+\tset bar.mepmep\n+\tEOF\n+\ttest_must_fail git config --stdin <stdin 2>output &&\n+\techo \"fatal: unexpected end of buffer\" >expect &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--stdin fails on leading whitespace' '\n+\tcat >stdin <<-EOF &&\n+\t set bar.mepmep\n+\tEOF\n+\ttest_must_fail git config --stdin <stdin\n+'\n+\n+test_expect_success '--stdin fails on unknown command' '\n+\tcat >stdin <<-EOF &&\n+\tfoo bar.mepmep\n+\tEOF\n+\ttest_must_fail git config --stdin <stdin\n+'\n+\n+test_expect_success '--stdin works on no input' '\n+\techo -n \"\" | git config --stdin\n+'\n+\n+test_expect_success '--stdin fails on unbalanced quotes' '\n+\tcat >stdin <<-EOF &&\n+\tset \"foo.bar\n+\tEOF\n+\ttest_must_fail git config --stdin <stdin\n+'\n+\n+test_expect_success '--stdin with --null flag' '\n+\techo -ne \"set bar.baz\\0false\" | git config --stdin --null &&\n+\tgit config --get bar.baz >output &&\n+\techo false >expect &&\n+\ttest_cmp expect output\n+'\n+\n cat > .git/config << EOF\n [novalue]\n \tvariable\n-- \n2.24.1\n\n"},{"id":"390550","messageId":"20200127100933.10765-1-git@zjvandeweg.nl","threadId":"52702","inReplyTo":null,"subject":"[PATCH v2 0/1] config: read instructions from stdin","fromName":"Zeger-Jan van de Weg","fromEmail":"git@zjvandeweg.nl","sentAt":"2020-01-27T10:09:32Z","receivedAt":"2020-01-27T10:17:03Z","isPatch":true,"sender":{"key":"git@zjvandeweg.nl","avatar":null},"body":"This patch is a continuation of my previous work on `git config --add\n--stdin`.[1] In the review it was noted that the approach could be more\nextensible by using a DSL or instruction set like `update-ref`.\n\nLike the previous patch, it hides input from the system allowing to set\nsecrets in the config file more securely.\n\n[1]: https://public-inbox.org/git/20190917133135.190145-1-git@zjvandeweg.nl/T/#m287ad04b3dec77269643e35698df7fe4eb2f4f4f\n\nZeger-Jan van de Weg (1):\n  config: learn the --stdin option for instructions\n\n Documentation/git-config.txt |  29 ++++++++++\n builtin/config.c             | 104 +++++++++++++++++++++++++++++++++++\n t/t1300-config.sh            |  60 ++++++++++++++++++++\n 3 files changed, 193 insertions(+)\n\n-- \n2.24.1\n\n"},{"id":"390552","messageId":"CAP8UFD1S9yE_SkxJbLWz=HHasYmJnDsR9MuefBsLkdWaYFdu-Q@mail.gmail.com","threadId":"52702","inReplyTo":"20200127100933.10765-2-git@zjvandeweg.nl","subject":"Re: [PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-27T11:44:46Z","receivedAt":"2020-01-27T11:45:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 27, 2020 at 11:21 AM Zeger-Jan van de Weg <git@zjvandeweg.nl> wrote:\n\n> +static void update_config_stdin()\n> +{\n> +       struct strbuf input = STRBUF_INIT;\n> +       const char *next;\n> +       const char *max_char;\n> +\n> +       if (strbuf_read(&input, 0, 1000) < 0)\n> +               die_errno(_(\"could not read from stdin\"));\n> +       next = input.buf;\n> +\n> +       max_char = input.buf + input.len;\n> +       while(next < max_char) {\n\nNit: space missing between \"while\" and \"(\".\n\n> +               if (*next == term)\n> +                       die(_(\"empty command in input\"));\n> +               else if (isspace(*next))\n> +                       die(_(\"whitespace before command\"));\n> +               else if (skip_prefix(next, \"set \", &next))\n> +                       next = parse_cmd_set(next, max_char);\n> +               else if (skip_prefix(next, \"unset \", &next))\n> +                       next = parse_cmd_unset(next, max_char);\n> +               else\n> +                       die(_(\"unknown command %s\"), next);\n> +       }\n> +\n> +       strbuf_release(&input);\n> +}\n\nThanks,\nChristian.\n"},{"id":"390568","messageId":"CAPig+cS03oS9PhN_cusjpzoCzwNmgc5rXiVAmG3ceUCFw71PVg@mail.gmail.com","threadId":"52702","inReplyTo":"20200127100933.10765-2-git@zjvandeweg.nl","subject":"Re: [PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-27T16:59:03Z","receivedAt":"2020-01-27T16:59:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 27, 2020 at 5:17 AM Zeger-Jan van de Weg <git@zjvandeweg.nl> wrote:\n> When setting values in the git config, the value is part of the\n> arguments for execution. This potentially leaks the value through\n> logging, or other programs like `ps`.\n>\n> Add the `--stdin` option that reads from stdin for instructions to set\n> and unset values to hide them from prying eyes. The instructions are based\n> on the `update-ref` DSL, and accept the set and unset commands.\n>\n> Signed-off-by: Zeger-Jan van de Weg <git@zjvandeweg.nl>\n> ---\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> @@ -259,6 +264,30 @@ Valid `<type>`'s include:\n> +STDIN\n> +-----\n> +\n> +With `--stdin`, config reads instructions from standard input and performs\n> +all modifications in sequence.\n> +\n> +Specify commands of the form:\n> +\n> +    set SP <key> SP <newvalue>\n> +    unset SP <key>\n\nIf you follow the precedent of the git-update-ref documentation, these\nshould be:\n\n    set SP <key> SP <newvalue> LF\n    unset SP <key> LF\n\nI'm not sure we really need to be calling the value \"newvalue\" (I\nguess you picked that up from git-update-ref.txt). \"value\" should be\nfine, thus:\n\n    set SP <key> SP <value> LF\n    unset SP <key> LF\n\n> +Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without\n> +quoting:\n> +\n> +    set SP <key> NULL <newvalue>\n> +    unset SP <key>\n\nA few comments:\n\nFirst, this is talking about the NUL character, not a NULL pointer,\nso: s/NULL/NUL/\n\nSecond, this doesn't give any indication about how the lines should be\nterminated. It should instead be written as:\n\n    set SP <key> NUL <value> NUL\n    unset SP <key> NUL\n\nThird, importantly, unlike git-update-ref from which this DSL takes\ninspiration and in which \"refs\" might have oddball names for which\nNUL-termination makes sense, it's hard to imagine a case in which a\nconfiguration key would be so strange as to warrant NUL-termination.\nThis observation suggests a simpler DSL in which only <value> is\nNUL-terminated:\n\n    set SP <key> SP <value> NUL\n    unset SP <key> LF\n\n(The proposed code changes in config.c would need adjustment, as well,\nto implement this revised DSL.)\n\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> @@ -380,6 +380,66 @@ test_expect_success '--add' '\n> +test_expect_success '--stdin works on no input' '\n> +    echo -n \"\" | git config --stdin\n> +'\n\n`echo -n` is not portable. If you want no input at all, either grab it\nfrom /dev/null:\n\n    git config --stdin </dev/null\n\nor use `printf` to suppress printing of the line-terminator:\n\n    printf \"\" | git config --stdin\n\n> +test_expect_success '--stdin with --null flag' '\n> +    echo -ne \"set bar.baz\\0false\" | git config --stdin --null &&\n> +    Git config --get bar.baz >output &&\n> +    echo false >expect &&\n> +    test_cmp expect output\n> +'\n\nLikewise, non-portable use of `echo -n` and `echo \"...\\0...\". Use\n`printf` instead:\n\n    printf \"set bar.baz\\0false\" | git config --stdin --null &&\n\n(But note that this isn't even testing NUL-termination of <value>.)\n"},{"id":"390616","messageId":"20200128091914.GA574544@coredump.intra.peff.net","threadId":"52702","inReplyTo":"20200127100933.10765-2-git@zjvandeweg.nl","subject":"Re: [PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-28T09:19:14Z","receivedAt":"2020-01-28T09:19:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 27, 2020 at 11:09:33AM +0100, Zeger-Jan van de Weg wrote:\n\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 899e92a1c9..9f7462284d 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -23,6 +23,7 @@ SYNOPSIS\n>  'git config' [<file-option>] [--show-origin] [-z|--null] [--name-only] -l | --list\n>  'git config' [<file-option>] --get-color name [default]\n>  'git config' [<file-option>] --get-colorbool name [stdout-is-tty]\n> +'git config' [<file-option>] [-z|--null] --stdin\n\nInstead of a stdin mode just for setting, you have a general --stdin\nmode. That would later allow \"get\" commands, too. I'm not sure if that's\na good idea, though. Here are two complications which came to mind\nimmediately:\n\n  - the permissions models for getting and setting might be different.\n    Might we ever feed untrusted input to something like \"git config\n    --get --stdin\", but not want it to be able to set any values?\n\n  - what happens when get/set commands are intermixed? When are set\n    commands flushed? E.g., if I send:\n\n     set foo.bar baz\n     get foo.bar\n\n    then do I see \"baz\", or whatever was there before?\n\nI think those are solvable, but unless we have a compelling use case for\na process that will both get and set values, it seems like it might be\nsimpler to keep the major modes separate.\n\n> +STDIN\n> +-----\n> +\n> +With `--stdin`, config reads instructions from standard input and performs\n> +all modifications in sequence.\n> +\n> +Specify commands of the form:\n> +\n> +\tset SP <key> SP <newvalue>\n> +\tunset SP <key>\n\nThis protocol doesn't leave much room for giving more details about each\nset operation. There are several things you can do on the command line\nnow that you might want to specify on a per-key basis:\n\n  - adding keys versus replacing\n\n  - likewise, replace-all versus a single replace\n\n  - likewise, unset versus unset-all\n\n  - type interpretation / normalization\n\n  - value regex matching\n\nEven if we don't implement all of that immediately, it would be nice to\nhave a plan for how they'd be added to stdin mode eventually.\n\n> +Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without\n> +quoting:\n\nThis mentions quoting, but the earlier form doesn't discuss it at all. I\ncan see from the quote that it does unquote_c_style(). It might be worth\nsaying so explicitly; since we're dealing with config, people may assume\nthat the syntax is like the one in config files (which is similar, but\nnot quite the same).\n\n> +static const char *parse_cmd_set(const char *next, const char *max_char)\n> +{\n> +\tchar *key, *value;\n> +\tint ret;\n> +\n> +\tkey = parse_key_or_value(&next, max_char);\n> +\tif (!key)\n> +\t\tdie(_(\"set: missing key\"));\n> +\n> +\tvalue = parse_key_or_value(&next, max_char);\n> +\tif (!value)\n> +\t\tdie(_(\"set: missing value\"));\n> +\n> +\tret = git_config_set_in_file_gently(given_config_source.file, key, value);\n> +\tif (ret)\n> +\t\tdie(_(\"cannot set key value pair: %d\"), ret);\n> +\n> +\tfree(key);\n> +\tfree(value);\n> +\treturn next;\n> +}\n\nI mentioned flushing earlier. It looks like this will flush each\nindividual \"set\" command by rewriting the whole file. But another reason\none might want to use --stdin is to set multiple keys in one go, either\nfor efficiency or because you'd like readers to see the result\natomically.\n\nI don't think we actually have a function to set multiple keys in one\npass yet. And I'd be OK starting with a less-efficient implementation.\nBut we probably should document that the current flushing behavior is\nnot something people should count on, to leave us room to change it in\nthe future.\n\n\nThe code itself looked cleanly done, though I admit I didn't look too\nclosely. If I've convinced you on any of the design considerations\nabove, I think it would need rewritten a bit.\n\n-Peff\n"},{"id":"390617","messageId":"20200128092414.GB574544@coredump.intra.peff.net","threadId":"52702","inReplyTo":"CAPig+cS03oS9PhN_cusjpzoCzwNmgc5rXiVAmG3ceUCFw71PVg@mail.gmail.com","subject":"Re: [PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-28T09:24:14Z","receivedAt":"2020-01-28T09:24:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 27, 2020 at 11:59:03AM -0500, Eric Sunshine wrote:\n\n> Second, this doesn't give any indication about how the lines should be\n> terminated. It should instead be written as:\n> \n>     set SP <key> NUL <value> NUL\n>     unset SP <key> NUL\n> \n> Third, importantly, unlike git-update-ref from which this DSL takes\n> inspiration and in which \"refs\" might have oddball names for which\n> NUL-termination makes sense, it's hard to imagine a case in which a\n> configuration key would be so strange as to warrant NUL-termination.\n> This observation suggests a simpler DSL in which only <value> is\n> NUL-terminated:\n> \n>     set SP <key> SP <value> NUL\n>     unset SP <key> LF\n> \n> (The proposed code changes in config.c would need adjustment, as well,\n> to implement this revised DSL.)\n\nThe section and key parts of a config key are pretty restricted, but the\nsubsection portion can contain anything except newline and NUL. So in\nparticular, it would be valid to have a space, which would make the\ninput ambiguous.\n\nI agree it would probably be rare, but isn't the whole point of \"-z\" to\nbe able to represent anything without worrying about quoting?\n\n-Peff\n"},{"id":"390622","messageId":"CAPig+cQJeT-pU-cUE4YEKcb4P_0X2sB1srGMCiOfyt6DZEcbgg@mail.gmail.com","threadId":"52702","inReplyTo":"20200128092414.GB574544@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-28T13:42:21Z","receivedAt":"2020-01-28T13:42:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 28, 2020 at 4:24 AM Jeff King <peff@peff.net> wrote:\n> On Mon, Jan 27, 2020 at 11:59:03AM -0500, Eric Sunshine wrote:\n> >     set SP <key> SP <value> NUL\n> >     unset SP <key> LF\n>\n> The section and key parts of a config key are pretty restricted, but the\n> subsection portion can contain anything except newline and NUL. So in\n> particular, it would be valid to have a space, which would make the\n> input ambiguous.\n\nI was wondering if that might be the case (particularly, if space was\nallowed). Thanks for the education.\n"},{"id":"390663","messageId":"xmqq36bz8job.fsf@gitster-ct.c.googlers.com","threadId":"52702","inReplyTo":"20200128092414.GB574544@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-28T19:28:36Z","receivedAt":"2020-01-28T19:28:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Jan 27, 2020 at 11:59:03AM -0500, Eric Sunshine wrote:\n>\n>> Second, this doesn't give any indication about how the lines should be\n>> terminated. It should instead be written as:\n>> \n>>     set SP <key> NUL <value> NUL\n>>     unset SP <key> NUL\n>> \n>> Third, importantly, unlike git-update-ref from which this DSL takes\n>> inspiration and in which \"refs\" might have oddball names for which\n>> NUL-termination makes sense, it's hard to imagine a case in which a\n>> configuration key would be so strange as to warrant NUL-termination.\n>> This observation suggests a simpler DSL in which only <value> is\n>> NUL-terminated:\n>> \n>>     set SP <key> SP <value> NUL\n>>     unset SP <key> LF\n>> \n>> (The proposed code changes in config.c would need adjustment, as well,\n>> to implement this revised DSL.)\n>\n> The section and key parts of a config key are pretty restricted, but the\n> subsection portion can contain anything except newline and NUL. So in\n> particular, it would be valid to have a space, which would make the\n> input ambiguous.\n>\n> I agree it would probably be rare, but isn't the whole point of \"-z\" to\n> be able to represent anything without worrying about quoting?\n\nYup.  I was tempted to say, in addition to \"without worrying about\nquoting\", \"without worrying about the syntax\".\n\nBut unfortunately that would not work.  If we were to add a new\n\"frotz\" subcommand in a future version of Git that takes N args,\n\n\tfrotz SP <arg1> NUL <arg2> NUL ... <argN> NUL\n\nmay how you'd express it, and it would be wonderful if the current\nversion of Git that does not know \"frotz\" subcommand can at least\nparse and ignore it.  That cannot however be done, though, because\nthere is no syntactic difference between the argument terminator\n(after each arg) and subcommand terminator (after each subcommand).\n"},{"id":"390683","messageId":"20200129023704.GB596379@coredump.intra.peff.net","threadId":"52702","inReplyTo":"xmqq36bz8job.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] config: learn the --stdin option for instructions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-29T02:37:04Z","receivedAt":"2020-01-29T02:37:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 28, 2020 at 11:28:36AM -0800, Junio C Hamano wrote:\n\n> > I agree it would probably be rare, but isn't the whole point of \"-z\" to\n> > be able to represent anything without worrying about quoting?\n> \n> Yup.  I was tempted to say, in addition to \"without worrying about\n> quoting\", \"without worrying about the syntax\".\n> \n> But unfortunately that would not work.  If we were to add a new\n> \"frotz\" subcommand in a future version of Git that takes N args,\n> \n> \tfrotz SP <arg1> NUL <arg2> NUL ... <argN> NUL\n> \n> may how you'd express it, and it would be wonderful if the current\n> version of Git that does not know \"frotz\" subcommand can at least\n> parse and ignore it.  That cannot however be done, though, because\n> there is no syntactic difference between the argument terminator\n> (after each arg) and subcommand terminator (after each subcommand).\n\nI do agree it would be nice if we could separate syntax from semantics,\njust because it lets us reuse and modify parsers more easily. We have\nthe same problem on output (e.g., anything parsing \"diff --name-status\n-z\" has to know there are two name fields for an R or C entry), and it\nhas tripped people up in the past.\n\nBut:\n\n  - I don't think we'd want a current version of Git to ignore an\n    unknown frotz anyway. It would probably say \"I don't understand\n    frotz, so I don't know if it's safe to ignore it or not\".\n\n  - Given only the tool of NUL bytes, I'm not sure how to easily\n    design a syntax that isn't awful. I.e., clearly something like:\n\n      frotz SP 2 NUL <arg1> NUL <arg2> NUL\n\n    works, but have we really made the world a better place? It would\n    make more sense to me to use a standard serialization format like\n    JSON, but that generally comes with its own inherited baggage (like\n    non-UTF8 strings).\n\n-Peff\n"}]}