Change $OutputEncoding to use iso-8859-1 encoding rather than ASCII #5361
Conversation
This enables a number of useful scenarios. With this change it is now possible to do the following: ```powershell get-content -raw -encoding iso8859 /tmp/archive.tgz | gunzip | tar xvf - ``` and expand a compressed tar archive. It also ensures that the bytes/characters passed to a native executable are not changed by the pipeline. What is passed to the pipeline is passed to the next native application.
JamesWTruher
requested review from
daxian-dbw,
SteveL-MSFT,
adityapatwardhan and
PaulHigin
JamesWTruher
requested review from
BrucePay,
dantraMSFT,
lzybkr,
mirichmo and
TravisEz13
as code owners
There was a problem hiding this comment.
@JamesWTruher Per our discussion, you are going to add newlines when reading the redirected output from a native command. I don't see that change in this PR. Will that be a separate PR?
| @@ -1797,8 +1797,7 @@ internal void Start(Process process, NativeCommandIOFormat inputFormat) | |||
| //Get the encoding for writing to native command. Note we get the Encoding | |||
| //from the current scope so a script or function can use a different encoding | |||
| //than global value. | |||
| Encoding pipeEncoding = _command.Context.GetVariableValue(SpecialVariables.OutputEncodingVarPath) as System.Text.Encoding ?? | |||
| Encoding.ASCII; | |||
| Encoding pipeEncoding = _command.Context.GetVariableValue(SpecialVariables.OutputEncodingVarPath) as System.Text.Encoding ?? Utils.iso8859; | |||
There was a problem hiding this comment.
Minor comment: there is an extra space before Utils.iso8859.
| @@ -4410,7 +4410,7 @@ .ForwardHelpCategory Cmdlet | |||
| // Variable which controls the encoding for piping data to a NativeCommand | |||
There was a problem hiding this comment.
It would be great if you can add more comments about the characteristics of this encoding and mention that's why we use it as the default output encoding.
| encodingToUse = Encoding.GetEncoding(28591); | ||
| } | ||
| Console.InputEncoding = encodingToUse; | ||
| string pipedText = Console.In.ReadToEnd(); |
There was a problem hiding this comment.
Maybe we should just spit out the characters read in without having the encoding involved? For example, like this:
int ch;
while ((ch = Console.In.Read()) != -1)
{
Console.WriteLine(ch);
}
There was a problem hiding this comment.
I like that much better, thanks
updated
daxian-dbw
added
the
Documentation Needed in this repo
| new UTF8Encoding(encoderShouldEmitUTF8Identifier: false); | ||
| internal static readonly UTF8Encoding utf8NoBom = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false); | ||
| // encoding which is essentially a passthru | ||
| internal static readonly Encoding iso8859 = Encoding.GetEncoding(28591); |
There was a problem hiding this comment.
You should use the overload that takes a string instead:
GetEncoding("iso-8859-1");| { | ||
| // write a string of characters (bytes), this closest resembles a binary | ||
| // write the string "test" with an accent over the e | ||
| byte[] testbytes = new byte[] { 116, 233, 115, 116 }; |
There was a problem hiding this comment.
Why not just write all the bytes from 0 to 255?
There was a problem hiding this comment.
no need
[char]233 is all that is needed to show the problem, and is readable from the command line if you run it.
| } | ||
| catch | ||
| { | ||
| ; |
There was a problem hiding this comment.
Seems like you should fail this and return non-zero exit code if invalid encoding is provided. Also suggest using the string overload version instead of the int making the code easier to read.
There was a problem hiding this comment.
i've replaced this with something far simpler (see dongbo's suggestion above)
| } | ||
| if ( encodingToUse == null ) | ||
| { | ||
| encodingToUse = Encoding.GetEncoding(28591); |
There was a problem hiding this comment.
Use GetEncoding("iso-8859-1")
|
This PR is fundamentally misguided, as discussed here. |
|
Based on the discussion @mklement0 linked, it seems that for console text output, we should default to UTF-8. Binary pipeline is something we'll have to defer to later. |
|
@SteveL-MSFT: That's a great summary - thank you. |
|
closing in favor of a new PR which simply changes the output encoding to utf8nobom. |
joeyaiello
removed
Documentation Needed in this repo
This enables a number of useful scenarios.
With this change it is now possible to do the following:
and expand a compressed tar archive.
It also ensures that the bytes/characters passed to a native executable are not changed by the pipeline.
What is passed into the pipeline is passed to the next native application
This does not address #1908, as that requires a larger re-architecture of the pipeline. This PR continues to have a
[environment]::newlinewhich is added to the native output which should be addressed when 1908 is fixed.This is marked as a breaking change because the output of the pipeline is not altered as it was previously, anyone relying on ascii encoding (especially for extended ascii sequences) will no longer have that behavior (broken though it was).