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

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

[Taylor Blau]

Invocations of 'git config <name>' print a trailing newline after
writing the value assigned to the given configuration variable.

This has an unexpected interaction with 63e2a0f8e9 (builtin/config:
introduce `color` type specifier, 2018-04-09), which causes a newline to
be printed after a color's ANSI escape sequence.

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.

This bug has survived because there was never a test that would have
caught it. The old test used 'test_decode_color', which checks that its
input begins with a color, but stops parsing once it has parsed a single
color successfully. In this case, it was ignoring the trailing '\n'.

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>
---
 builtin/config.c  | 3 ++-
 t/t1300-config.sh | 5 ++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 98d65bc0ad..c8f088af38 100644
--- 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_setlen(buf, buf->len - 1);
 		}
 	}
-	strbuf_addch(buf, term);
+	if (type != TYPE_COLOR)
+		strbuf_addch(buf, term);
 	return 0;
 }
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 428177c390..ec1b3a852d 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -907,9 +907,8 @@ test_expect_success 'get --expiry-date' '
 test_expect_success 'get --type=color' '
 	rm .git/config &&
 	git config foo.color "red" &&
-	git config --get --type=color foo.color >actual.raw &&
-	test_decode_color <actual.raw >actual &&
-	echo "<RED>" >expect &&
+	printf "\\033[31m" >expect &&
+	git config --get --type=color foo.color >actual &&
 	test_cmp expect actual
 '
 
-- 
2.20.1

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

[Eric Sunshine]

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

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

[Johannes Schindelin]

Hi Taylor,

On Fri, 1 Mar 2019, Taylor Blau wrote:

> Invocations of 'git config <name>' print a trailing newline after
> writing the value assigned to the given configuration variable.
> 
> This has an unexpected interaction with 63e2a0f8e9 (builtin/config:
> introduce `color` type specifier, 2018-04-09), which causes a newline to
> be printed after a color's ANSI escape sequence.
> 
> 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.
> 
> This bug has survived because there was never a test that would have
> caught it. The old test used 'test_decode_color', which checks that its
> input begins with a color, but stops parsing once it has parsed a single
> color successfully. In this case, it was ignoring the trailing '\n'.

Isn't the reason rather that our test compared the output to an expected
text *with a newline*? IOW:

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 428177c390..ec1b3a852d 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -907,9 +907,8 @@ test_expect_success 'get --expiry-date' '
>  test_expect_success 'get --type=color' '
>  	rm .git/config &&
>  	git config foo.color "red" &&
> -	git config --get --type=color foo.color >actual.raw &&
> -	test_decode_color <actual.raw >actual &&
> -	echo "<RED>" >expect &&

This should do the right thing if you write

	printf "<RED>" >expect

instead?

Ciao,
Dscho

> +	printf "\\033[31m" >expect &&
> +	git config --get --type=color foo.color >actual &&
>  	test_cmp expect actual
>  '
>  
> -- 
> 2.20.1
> 

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Invocations of 'git config <name>' print a trailing newline after
> writing the value assigned to the given configuration variable.
>
> This has an unexpected interaction with 63e2a0f8e9 (builtin/config:
> introduce `color` type specifier, 2018-04-09), which causes a newline to
> be printed after a color's ANSI escape sequence.
>
> 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.
>
> This bug has survived because there was never a test that would have
> caught it. The old test used 'test_decode_color', which checks that its
> input begins with a color, but stops parsing once it has parsed a single
> color successfully. In this case, it was ignoring the trailing '\n'.

The output from "git config" plumbing command were designed to help
people writing shell scripts Porcelain around it, so the expected
use for them has always been

	ERR=$(git config --type=color --default=red ui.color.error)
	... some time later ..
	echo "${ERR}this is an error message"

where the first assignment will strip the final LF (i.e. the value
of the $ERR variable does not have it).

An interesting aspect of the above is that this is *NOT* limited to
colors.  Regardless of the type you are reading, be it an int or a
bool, VAR=$(git config ...) will strip the trailing LF, and existing
scripts people have do rely on that, i.e. when people write

	VAR=$(git config ...)
	echo "var setting is $VAR"

they rely on VAR=$(...) assignment to strip trailing LF and echo to
add a final LF to the string.

So if we are going to change anything, the change MUST NOT single
out "color".  IOW, the title of the patch already tells us that it
is giving a wrong solution.

Whether you limit it to color or not, to Porcelain writers who are
writing in shell, I suspect that the code after the proposed change
will not be a huge regression.  VAR=$(git config ...) assignment,
when the output from the command ends without the final LF (i.e. an
incomplete line), will keep the string intact, so the behaviour of
these shell scripts would not change.

If an existing Porcelain script were written in Perl and uses chop
to strip the last LF coming out of "git config", however, the
proposed change WILL BREAK such a script.

Needless to say, "using chop in Perl is wrong to begin with" misses
the point from two directions---(1) 'chop in Perl' is a mere
example---scripts not written in Perl using chop may still rely on
the existing behaviour that the output always has the final LF, and
(2) even if we agree that using chop in Perl is a bad idea, such a
script has happily been working, and suddenly breaking it is a
regression no matter what.

So, I am not hugely enthused by this change, even though I am
somewhat sympathetic to it, as it would help one narrow use case,
i.e. "interpolation".

	cat <<EOF
	$(git config ...)Foo Bar$(git config ...)
	EOF

or

	(
		git config ...
		echo Foo Bar
                git config ...
	)

would lack LF before Foo automatically, and forcing those who want
to have LF to add it manually would sound easier than forcing those
who want to strip LF when they do not want it.

But when you are making a list, getting the final LF for free is a
feature, so it cuts both ways.

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

[Jeff King]

On Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote:

> > This bug has survived because there was never a test that would have
> > caught it. The old test used 'test_decode_color', which checks that its
> > input begins with a color, but stops parsing once it has parsed a single
> > color successfully. In this case, it was ignoring the trailing '\n'.
> 
> Isn't the reason rather that our test compared the output to an expected
> text *with a newline*? IOW:
> [...]
> This should do the right thing if you write
> 
> 	printf "<RED>" >expect

No, there are actually two problems that cancel each other out. Because
test_decode_color() is implemented in awk, it doesn't see if there's
actually a newline or not in each record. So it _always_ adds a newline
after printing out the line (and I don't think Taylor's explanation
above is quite right; it does have a loop to handle multiple colors).

So regardless of whether git-config is sending the newline or not, the
"actual" file will always have a newline in it.

And then to match that, we used "echo" which of course has a newline,
and matches the test_decode_color output.

So you're right that we need to switch to printf. But doing so without
dropping test_decode_color() would mean the test would always fail. We
have to printf the raw bytes, which is what the new test does.

-Peff

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

[Jeff King]

On Sun, Mar 03, 2019 at 10:18:11AM +0900, Junio C Hamano wrote:

> An interesting aspect of the above is that this is *NOT* limited to
> colors.  Regardless of the type you are reading, be it an int or a
> bool, VAR=$(git config ...) will strip the trailing LF, and existing
> scripts people have do rely on that, i.e. when people write
> 
> 	VAR=$(git config ...)
> 	echo "var setting is $VAR"
> 
> they rely on VAR=$(...) assignment to strip trailing LF and echo to
> add a final LF to the string.
> 
> So if we are going to change anything, the change MUST NOT single
> out "color".  IOW, the title of the patch already tells us that it
> is giving a wrong solution.

I'm not sure I agree. Colors have always been special, and
"--type=color" was advertised as a replacement for "--get-color". And
"--get-color" never output the newline.

Philosophically speaking, I'd make the argument that the "color" type is
a bit special because unlike other output, it is unreadable binary gunk.
But I dunno. It does suck for the output to be dependent on "--type",
because it means that anything consuming it has to understand the
various types. It also creates potential complications if we ever
implement a "--stdin" mode to grab the (type-interpreted) values of
several keys, where presumably we'd have to have newlines as part of the
protocol.

> Needless to say, "using chop in Perl is wrong to begin with" misses
> the point from two directions---(1) 'chop in Perl' is a mere
> example---scripts not written in Perl using chop may still rely on
> the existing behaviour that the output always has the final LF, and
> (2) even if we agree that using chop in Perl is a bad idea, such a
> script has happily been working, and suddenly breaking it is a
> regression no matter what.

With respect to backwards compatibility, my thinking on the matter was
basically:

  1. Since --type=color was supposed to be a drop-in replacement for
     --get-color, it's a bug that they don't behave the same.

  2. It's a fairly recent feature, so nobody really noticed the bug
     until recently (and it was in fact me who noticed and got annoyed
     by it). If it were an ancient behavior, we might think about
     retaining even bug compatibility, but that's not the case here.

But I do admit your argument about consistency is giving me second
thoughts.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure I agree. Colors have always been special, and
> "--type=color" was advertised as a replacement for "--get-color". And
> "--get-color" never output the newline.

OK, that part of the motivation I completely missed.  If end-users
and scripters are happy with the behaviour of --get-color that does
not terminate its output with LF (which I think is a reasonable
thing to do, as "color" is special in that context, i.e. having a
dedicated --get option suitable for that type), as we advertise
"--type=color" is the same as "--get-color" (only better), we need
to special case it, and omitting LF at the end similarly does make
sense.

> With respect to backwards compatibility, my thinking on the matter was
> basically:
>
>   1. Since --type=color was supposed to be a drop-in replacement for
>      --get-color, it's a bug that they don't behave the same.
>
>   2. It's a fairly recent feature, so nobody really noticed the bug
>      until recently (and it was in fact me who noticed and got annoyed
>      by it). If it were an ancient behavior, we might think about
>      retaining even bug compatibility, but that's not the case here.

Now I think "we weren't consistent to begin with with --get-color,
and treating --type=color as a special case is justifiable"; and I
agree with the above two points.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> I'm not sure I agree. Colors have always been special, and
>> "--type=color" was advertised as a replacement for "--get-color". And
>> "--get-color" never output the newline.
>
> OK, that part of the motivation I completely missed.  If end-users
> and scripters are happy with the behaviour of --get-color that does
> not terminate its output with LF (which I think is a reasonable
> thing to do, as "color" is special in that context, i.e. having a
> dedicated --get option suitable for that type), as we advertise
> "--type=color" is the same as "--get-color" (only better), we need
> to special case it, and omitting LF at the end similarly does make
> sense.
>
>> With respect to backwards compatibility, my thinking on the matter was
>> basically:
>>
>>   1. Since --type=color was supposed to be a drop-in replacement for
>>      --get-color, it's a bug that they don't behave the same.
>>
>>   2. It's a fairly recent feature, so nobody really noticed the bug
>>      until recently (and it was in fact me who noticed and got annoyed
>>      by it). If it were an ancient behavior, we might think about
>>      retaining even bug compatibility, but that's not the case here.
>
> Now I think "we weren't consistent to begin with with --get-color,
> and treating --type=color as a special case is justifiable"; and I
> agree with the above two points.

Just to avoid an awkward situation where the ball gets dropped and
left on the floor forgotten, the above does not mean I am 100% happy
with the patch as posted.  There is no mention of --get-color
anywhere, let alone it shows the ANSI sequence without traililng LF,
which I would consider to be the most important part of the
justification.  It is much stronger than "I expected there won't be
any trailing LF from 'git config'", which was the only thing I
managed to read in the original and led to my response.

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

[Jeff King]

On Mon, Mar 04, 2019 at 02:05:40PM +0900, Junio C Hamano wrote:

> >> With respect to backwards compatibility, my thinking on the matter was
> >> basically:
> >>
> >>   1. Since --type=color was supposed to be a drop-in replacement for
> >>      --get-color, it's a bug that they don't behave the same.
> >>
> >>   2. It's a fairly recent feature, so nobody really noticed the bug
> >>      until recently (and it was in fact me who noticed and got annoyed
> >>      by it). If it were an ancient behavior, we might think about
> >>      retaining even bug compatibility, but that's not the case here.
> >
> > Now I think "we weren't consistent to begin with with --get-color,
> > and treating --type=color as a special case is justifiable"; and I
> > agree with the above two points.
> 
> Just to avoid an awkward situation where the ball gets dropped and
> left on the floor forgotten, the above does not mean I am 100% happy
> with the patch as posted.  There is no mention of --get-color
> anywhere, let alone it shows the ANSI sequence without traililng LF,
> which I would consider to be the most important part of the
> justification.  It is much stronger than "I expected there won't be
> any trailing LF from 'git config'", which was the only thing I
> managed to read in the original and led to my response.

Yeah, I agree it needs to be the main justification in the commit
message.

I do wonder, though, if we're digging ourselves a hole with the
inconsistency between different --types that will bite us later. Given
that it's not that hard to chomp the output (and as you noted, the shell
does it fairly transparently), and given that the caller has to switch
between "--get-color" and "--type=color", it's not that hard to handle
the output differently if you know to do so.

Mostly I was just surprised by the new behavior. Perhaps the right
solution is not a patch to the code, but to the documentation. Something
like:

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 495bb57416..61f3a9cdd7 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -252,7 +252,9 @@ Valid `<type>`'s include:
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 +
-`--type=color [--default=<default>]` is preferred over `--get-color`.
+`--type=color [--default=<default>]` is preferred over `--get-color`
+(but note that `--get-color` will omit the trailing newline printed by
+--type=color).
 
 -e::
 --edit::

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do wonder, though, if we're digging ourselves a hole with the
> inconsistency between different --types that will bite us later. Given
> that it's not that hard to chomp the output (and as you noted, the shell
> does it fairly transparently), and given that the caller has to switch
> between "--get-color" and "--type=color", it's not that hard to handle
> the output differently if you know to do so.
>
> Mostly I was just surprised by the new behavior. Perhaps the right
> solution is not a patch to the code, but to the documentation. Something
> like:
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 495bb57416..61f3a9cdd7 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -252,7 +252,9 @@ Valid `<type>`'s include:
>  	output.  The optional `default` parameter is used instead, if
>  	there is no color configured for `name`.
>  +
> -`--type=color [--default=<default>]` is preferred over `--get-color`.
> +`--type=color [--default=<default>]` is preferred over `--get-color`
> +(but note that `--get-color` will omit the trailing newline printed by
> +--type=color).
>  
>  -e::
>  --edit::

Yup, that would be a very sensible first step, regardless of what
the next step is.

After that, choices are

 (1) we'd introduce new inconsistency among --type=<type> by
     matching what --type=color does to what --get-color does, to
     allow us to revert that documentation update, or

 (2) we'd drop LF from all --type=<type>, that makes everything
     consistent and risk breaking a few existing scripts while doing
     so, and get yelled at by end users, or

 (3) we stop at this documentation update and do nothing else.

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

[Johannes Schindelin]

Hi Peff,

On Sun, 3 Mar 2019, Jeff King wrote:

> On Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote:
> 
> > > This bug has survived because there was never a test that would have
> > > caught it. The old test used 'test_decode_color', which checks that its
> > > input begins with a color, but stops parsing once it has parsed a single
> > > color successfully. In this case, it was ignoring the trailing '\n'.
> > 
> > Isn't the reason rather that our test compared the output to an expected
> > text *with a newline*? IOW:
> > [...]
> > This should do the right thing if you write
> > 
> > 	printf "<RED>" >expect
> 
> No, there are actually two problems that cancel each other out. Because
> test_decode_color() is implemented in awk, it doesn't see if there's
> actually a newline or not in each record.

Ah, so it is actually another instance of "we really need a precise
test-tool command for this rather than a somewhat incomplete and fragile
script" ;-)

> So it _always_ adds a newline after printing out the line (and I don't
> think Taylor's explanation above is quite right; it does have a loop to
> handle multiple colors).
> 
> So regardless of whether git-config is sending the newline or not, the
> "actual" file will always have a newline in it.
> 
> And then to match that, we used "echo" which of course has a newline,
> and matches the test_decode_color output.
> 
> So you're right that we need to switch to printf. But doing so without
> dropping test_decode_color() would mean the test would always fail. We
> have to printf the raw bytes, which is what the new test does.

Fair enough.

Ciao,
Dscho

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

[Taylor Blau]

On Tue, Mar 05, 2019 at 02:57:32PM +0900, Junio C Hamano wrote:
> Yup, that would be a very sensible first step, regardless of what
> the next step is.
>
> After that, choices are
>
>  (1) we'd introduce new inconsistency among --type=<type> by
>      matching what --type=color does to what --get-color does, to
>      allow us to revert that documentation update, or

I suppose... though I think that if others agree, I'd rather update the
documentation instead of introduce some inconsistency.

Yes, there's an argument to be made that if we're encouraging users to
go from '--get-color' -> '--type=color', that the two should behave the
same, but I don't think the cost we pay for behavioral equivalence
between the two is worth inconsistency among '--type=color' and all the
rest.

>  (2) we'd drop LF from all --type=<type>, that makes everything
>      consistent and risk breaking a few existing scripts while doing
>      so, and get yelled at by end users, or

As you indicate, I think that this option is one we should _not_ do. In
the interpolation example you shared earlier in the thread, script
writers most likely want and expect a trailing LF after each invocation
of 'git config'.

I'd argue that this case is more common than not wanting a LF when
interpolating with `--type=color`, so I agree it seems the tradeoff here
is not a good one.

>  (3) we stop at this documentation update and do nothing else.

To restate my response to (1), I think that the documentation update in
isolation makes the most sense here. I, too, was surprised in the same
way that Peff was when we stumbled upon this, but I think that
ultimately the consistency is most favorable.

Thanks all for your discussion and feedback.


Thanks,
Taylor

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

[Taylor Blau]

Hi Johannes,

On Sat, Mar 02, 2019 at 09:25:28PM +0100, Johannes Schindelin wrote:
> Hi Taylor,
>
> On Fri, 1 Mar 2019, Taylor Blau wrote:
>
> > [ ... ]
>
> This should do the right thing if you write
>
> 	printf "<RED>" >expect
>
> instead?
>
> Ciao,
> Dscho
>
> > +	printf "\\033[31m" >expect &&
> > +	git config --get --type=color foo.color >actual &&
> >  	test_cmp expect actual
> >  '
> >
> > --
> > 2.20.1

Thank you for your suggestion. It occurred to me that this might be a
preferable way to do things[1] after I sent the patch, but I am glad
that you suggested it here explicitly.

That said, I think that this patch has moved on from the code change,
after some discussion between Junio and Peff, so I think that I will
leave this (and the rest of the code changes) behind.

Thanks,
Taylor

[1] ... preferable in the way that we don't have to write "\\033[31m" in
    the test.

[PATCH v2] Documentation/config: note trailing newline with --type=color

[Taylor Blau]

In 63e2a0f8e9 (builtin/config: introduce `color` type specifier,
2018-04-09), `--type=color` was introduced and preferred to the
old-style `--get-color`.

The two behave the same in almost every way, save for the fact that
`--type=color` prints a trailing newline where `--get-color` does not.
Instead of introducing ambiguity between `--type=color` and the other
`--type` variants, document the difference between `--type=color` and
`--get-color` instead.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 1bfe9f56a7..d0b9c50d20 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -240,7 +240,9 @@ Valid `<type>`'s include:
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 +
-`--type=color [--default=<default>]` is preferred over `--get-color`.
+`--type=color [--default=<default>]` is preferred over `--get-color`
+(but note that `--get-color` will omit the trailing newline printed by
+`--type=color`).
 
 -e::
 --edit::
-- 
2.20.1

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Mostly I was just surprised by the new behavior. Perhaps the right
> solution is not a patch to the code, but to the documentation. Something
> like:

Let me forge your sign-off and commit this to prevent us from
forgetting.

Thanks, all.

-- >8 --
From: Jeff King <peff@peff.net>
Date: Mon, 4 Mar 2019 23:20:51 -0500
Subject: [PATCH] config: document --type=color output is a complete line

Even though the newer "--type=color" option to "git config" is meant
to be upward compatible with the traditional "--get-color" option,
unlike the latter, its output is not an incomplete line that lack
the LF at the end.  That makes it consistent with output of other
types like "git config --type=bool".

Document it, as it sometimes surprises unsuspecting users.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-config.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 1bfe9f56a7..611a32445c 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -240,7 +240,9 @@ Valid `<type>`'s include:
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 +
-`--type=color [--default=<default>]` is preferred over `--get-color`.
+`--type=color [--default=<default>]` is preferred over `--get-color`
+(but note that `--get-color` will omit the trailing newline printed by
+--type=color).
 
 -e::
 --edit::
-- 
2.21.0-4-g36eb1cb9cf

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

[Jeff King]

On Thu, Mar 07, 2019 at 09:50:57AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Mostly I was just surprised by the new behavior. Perhaps the right
> > solution is not a patch to the code, but to the documentation. Something
> > like:
> 
> Let me forge your sign-off and commit this to prevent us from
> forgetting.
> 
> Thanks, all.

Thanks for tying this up. One minor nit:

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 1bfe9f56a7..611a32445c 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -240,7 +240,9 @@ Valid `<type>`'s include:
>  	output.  The optional `default` parameter is used instead, if
>  	there is no color configured for `name`.
>  +
> -`--type=color [--default=<default>]` is preferred over `--get-color`.
> +`--type=color [--default=<default>]` is preferred over `--get-color`
> +(but note that `--get-color` will omit the trailing newline printed by
> +--type=color).

That final line probably should have literal quotes, like:

  `--type=color`).

-Peff

Re: [PATCH v2] Documentation/config: note trailing newline with --type=color

[Jeff King]

On Wed, Mar 06, 2019 at 03:52:38PM -0800, Taylor Blau wrote:

> In 63e2a0f8e9 (builtin/config: introduce `color` type specifier,
> 2018-04-09), `--type=color` was introduced and preferred to the
> old-style `--get-color`.
> 
> The two behave the same in almost every way, save for the fact that
> `--type=color` prints a trailing newline where `--get-color` does not.
> Instead of introducing ambiguity between `--type=color` and the other
> `--type` variants, document the difference between `--type=color` and
> `--get-color` instead.

I just responded to the one Junio posted elsewhere in the thread, but
for the record this one is fine to me, too (and it gets the literal
quoting right ;) ).

-Peff

Re: [PATCH v2] Documentation/config: note trailing newline with --type=color

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 06, 2019 at 03:52:38PM -0800, Taylor Blau wrote:
>
>> In 63e2a0f8e9 (builtin/config: introduce `color` type specifier,
>> 2018-04-09), `--type=color` was introduced and preferred to the
>> old-style `--get-color`.
>> 
>> The two behave the same in almost every way, save for the fact that
>> `--type=color` prints a trailing newline where `--get-color` does not.
>> Instead of introducing ambiguity between `--type=color` and the other
>> `--type` variants, document the difference between `--type=color` and
>> `--get-color` instead.
>
> I just responded to the one Junio posted elsewhere in the thread, but
> for the record this one is fine to me, too (and it gets the literal
> quoting right ;) ).

Yeah, I haven't reached that message when I was doing mine.  Other
than "co-authored-by", which probably is not quite correct (i.e. I
would have added "[tb: wrapped it up with a commit log message]" and
kept you as the author, if I were doing it), I do not have a problem
with Taylor's version, either.