Re: [PATCH] t5702: be more resilient to env quoting

[Collin Funk <collin.funk1@gmail.com>]

Todd Zullinger <tmz@pobox.com> writes:

> Jeff King wrote:
>> A few tests in t5702 check whether we specify the v2 protocol via the
>> GIT_PROTOCOL environment variable. They do this by redirecting the
>> output of "env" into a file and grepping it. But our exact pattern
>> depends on how env quotes its output. GNU coreutils 9.10 will output
>> this:
>> 
>>   GIT_PROTOCOL=version=2
>> 
>> but version 9.12 will output this:
>> 
>>   GIT_PROTOCOL='version=2'
>> 
>> It doesn't quote every variable, but the presence of "=" now triggers
>> it. We could loosen our regex to accommodate both versions, but there's
>> a simpler fix. We only care about the value of one variable, so we can
>> just print it using the shell invoked for the server-side command.
>
> Per the coreutils-9.12 NEWS¹, it now honors "the
> QUOTING_STYLE environment variable, defaulting to
> shell-escape style."
>
> Not that I think we should keep the env parsing; as you note
> there are some pros to replacing it anyway.  But if we want
> to use env somewhere else, we can set QUOTING_STYLE=literal
> to get the unquoted values too.
>
> ¹ https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121
>
>   I'd link to the main Savannah cgit or gitweb, but those
>   are currently down, thanks to AI scraper bots. :(

Nice catch on the NEWS entry. Sorry to you both for the extra work. I am
at least partly to blame for those changes.

In coreutils-9.12, we made 'env' and 'printenv' quote their output. This
was to make the output more robust in the case of whitespace, special
characters, and in particular, newlines in environment variables.
Specifically, any program that parses the output of 'env', without -0,
will certainly mistake environment variables with newlines in them as
multiple environment variables. See the following example of the output
before coreutils-9.12:

    $ env -i env --version | head -n 1
    env (GNU coreutils) 9.10
    $ env -i env VAR1=real$'\n'VAR2=fake
    VAR1=real
    VAR2=fake

Compared to the output of 9.12:

    $ env -i ~/.local/bin/env --version | head -n1
    env (GNU coreutils) 9.12.163-9e804
    $ env -i ~/.local/bin/env VAR1=real$'\n'VAR2=fake
    VAR1='real'$'\n''VAR2=fake'

In previous instances, we only quoted when emitting text to a terminal,
e.g., with the quoting performed by 'ls':

    $ touch a$'\n'b
    $ /bin/ls .
    'a'$'\n''b'
    $ /bin/ls . | cat
    a
    b

However, we saw benefit in not doing that in this case. One could use
'env' in a shell script, redirect it to a file, and eval it later, for
example. We thought this would be okay, since we did not find any
instances in Debian code search where this would have caused problems.

Evidently, that search was not thorough enough, and various places ran
into issues [1] [2]. That code, in some cases, does have the problem I
mentioned previously, where it could mistake parts of the values in
environment variables as separate variables. However, we obviously do
not want to break things that have worked forever, and the quoting was
causing more problems than the uncommon issue that it was trying to
prevent.

As a result, we changed the behavior to quote only when standard output
is a terminal. E.g., here is the output from my current build from the
master branch:

    $ env -i src/env VAR1=real$'\n'VAR2=fake
    VAR1='real'$'\n''VAR2=fake'
    $ env -i src/env VAR1=real$'\n'VAR2=fake | cat
    VAR1=real
    VAR2=fake

In all of the cases that I have seen where the 9.12 behavior caused
problems, quoting only when standard output is a terminal would have
avoided all of them.

Pádraig Brady and I reached out to various distributions that tend to
update quickly about that patch [3], in hopes of preventing their users
from needing to debug applications/libraries which they may or may not
even be aware use 'env' in a way that this change breaks. I had
forgotten to submit a Debian bug report, but reported it today to the
Debian maintainers, who have applied it now [4]. I'm too familiar with
Debian packaging, but it appears that 9.12-0.3 should be available with
the patch [5].

Note that some distributions may have also picked up the patch adding
the --quoting-style option [6] (I think Slackware did, not sure if
anyone else did). But that hasn't been in a release yet, so you probably
don't want to use it.

Sorry again for the extra work of having to investigate that behavior
change.

Collin

[1] https://github.com/coreutils/coreutils/issues/355
[2] https://bugs.kde.org/525805
[3] https://github.com/coreutils/coreutils/commit/782a1e5bc2090212273bb731dceee2cc2a071e54
[4] https://bugs.debian.org/1152651
[5] https://packages.debian.org/sid/coreutils