Taylor Blau <me@xxxxxxxxxxxx> 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.
