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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Mar 1, 2019 at 7:41 PM Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> 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.

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