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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 27, 2020, 16:59 UTC
Message-ID: <CAPig+cS03oS9PhN_cusjpzoCzwNmgc5rXiVAmG3ceUCFw71PVg@mail.gmail.com>
In-Reply-To: <20200127100933.10765-2-git@zjvandeweg.nl>

On Mon, Jan 27, 2020 at 5:17 AM Zeger-Jan van de Weg <git@zjvandeweg.nl> wrote:

Show 22 quoted lines

> 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

Show 5 quoted lines

> +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.)

Show 5 quoted lines

> 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

Show 6 quoted lines

> +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>.)