Re: [PATCH] builtin/config.c: don't print a newline with --color

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 2, 2019, 12:34 UTC
Message-ID: <CAPig+cQWCSVqXyOmBC9Wn-Ew79H0yGPRJxRiLCMNJjNOUu2yPw@mail.gmail.com>
In-Reply-To: <b5ca6391fd0273fb7d6b92bc5ada96df93bc5cf2.1551487219.git.me@ttaylorr.com>

On Fri, Mar 1, 2019 at 7:41 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 5 quoted lines

> [...]
> In this case, printing a terminating newline is not desirable. Callers
> may want to print out such a configuration variable in a sub-shell in
> order to color something else, in which case they certainly don't want a
> newline.

It's extra confusing considering that this:

    color=$(git config --get --type=color foo.color)
    echo "${color}hello" >output

works as expected since $(...) swallows the newline, whereas, the case you cite:

    (
    git config --get --type=color foo.color &&
    echo hello
    ) >output

does not.

Illustrating the problem in the commit message by using example code like the above might (or might not) help the reader understand the issue more easily. (I had to read the prose description of the problem a couple times to properly understand it.) Not necessarily worth a re-roll.

Show 10 quoted lines

> To do what callers expect, only print a newline when the type is not
> 'color', and print the escape sequence itself for an exact comparison.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -258,7 +258,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
> -       strbuf_addch(buf, term);
> +       if (type != TYPE_COLOR)
> +               strbuf_addch(buf, term);

The reasoning for this conditional is sufficiently subtle that it might deserve an in-code comment (though, perhaps is not worth a re-roll).