Re: [PATCH v2 1/1] config: learn the --stdin option for instructions

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 27, 2020 at 5:17 AM Zeger-Jan van de Weg <git@zjvandeweg.nl> wrote:
> When setting values in the git config, the value is part of the
> arguments for execution. This potentially leaks the value through
> logging, or other programs like `ps`.
>
> Add the `--stdin` option that reads from stdin for instructions to set
> and unset values to hide them from prying eyes. The instructions are based
> on the `update-ref` DSL, and accept the set and unset commands.
>
> Signed-off-by: Zeger-Jan van de Weg <git@zjvandeweg.nl>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -259,6 +264,30 @@ Valid `<type>`'s include:
> +STDIN
> +-----
> +
> +With `--stdin`, config reads instructions from standard input and performs
> +all modifications in sequence.
> +
> +Specify commands of the form:
> +
> +    set SP <key> SP <newvalue>
> +    unset SP <key>

If you follow the precedent of the git-update-ref documentation, these
should be:

    set SP <key> SP <newvalue> LF
    unset SP <key> LF

I'm not sure we really need to be calling the value "newvalue" (I
guess you picked that up from git-update-ref.txt). "value" should be
fine, thus:

    set SP <key> SP <value> LF
    unset SP <key> LF

> +Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without
> +quoting:
> +
> +    set SP <key> NULL <newvalue>
> +    unset SP <key>

A few comments:

First, this is talking about the NUL character, not a NULL pointer,
so: s/NULL/NUL/

Second, this doesn't give any indication about how the lines should be
terminated. It should instead be written as:

    set SP <key> NUL <value> NUL
    unset SP <key> NUL

Third, importantly, unlike git-update-ref from which this DSL takes
inspiration and in which "refs" might have oddball names for which
NUL-termination makes sense, it's hard to imagine a case in which a
configuration key would be so strange as to warrant NUL-termination.
This observation suggests a simpler DSL in which only <value> is
NUL-terminated:

    set SP <key> SP <value> NUL
    unset SP <key> LF

(The proposed code changes in config.c would need adjustment, as well,
to implement this revised DSL.)

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> @@ -380,6 +380,66 @@ test_expect_success '--add' '
> +test_expect_success '--stdin works on no input' '
> +    echo -n "" | git config --stdin
> +'

`echo -n` is not portable. If you want no input at all, either grab it
from /dev/null:

    git config --stdin </dev/null

or use `printf` to suppress printing of the line-terminator:

    printf "" | git config --stdin

> +test_expect_success '--stdin with --null flag' '
> +    echo -ne "set bar.baz\0false" | git config --stdin --null &&
> +    Git config --get bar.baz >output &&
> +    echo false >expect &&
> +    test_cmp expect output
> +'

Likewise, non-portable use of `echo -n` and `echo "...\0...". Use
`printf` instead:

    printf "set bar.baz\0false" | git config --stdin --null &&

(But note that this isn't even testing NUL-termination of <value>.)