New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix garbled characters in history view with non-ascii characters (fixes#124) #136

Closed
wants to merge 29 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@shiena
Contributor

shiena commented Jul 13, 2017

fixes #124
fixes #35

Depends on:

I replaced the reading of standard output from asynchronous to synchronous to eliminate garbled characters.

image

Synchronous reading of both standard output and standard error may cause deadlock. So I replaced standard output only.
Reference: https://stackoverflow.com/questions/7160187/standardoutput-readtoend-hangs

Also, I'm worried about whether performance will be bad.

@shiena

This comment has been minimized.

Show comment
Hide comment
@shiena

shiena Jul 18, 2017

Contributor

@StanleyGoldman
The appveyor build failed with an error occurred in nuget restore GitHub.Unity.sln.
Can you build it again?

Contributor

shiena commented Jul 18, 2017

@StanleyGoldman
The appveyor build failed with an error occurred in nuget restore GitHub.Unity.sln.
Can you build it again?

@shiena

This comment has been minimized.

Show comment
Hide comment
@shiena

shiena Jul 22, 2017

Contributor

@StanleyGoldman
A pull request has been completed. I will summarize the changes.

  • Using Process::StandardOutput::BaseStream::BeginRead instead of Process::BeginOutputReadLine
  • Also change StandardError
  • However, since .NET 4.6 has no problem in character encoding, use the original code
  • Add options to git command according to Git for Windows Unicode support
Contributor

shiena commented Jul 22, 2017

@StanleyGoldman
A pull request has been completed. I will summarize the changes.

  • Using Process::StandardOutput::BaseStream::BeginRead instead of Process::BeginOutputReadLine
  • Also change StandardError
  • However, since .NET 4.6 has no problem in character encoding, use the original code
  • Add options to git command according to Git for Windows Unicode support
@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Jul 31, 2017

Contributor

Hey @shiena sorry I've been MIA, @shana and I have been on vacation.
We will take a look at this soon.

Contributor

StanleyGoldman commented Jul 31, 2017

Hey @shiena sorry I've been MIA, @shana and I have been on vacation.
We will take a look at this soon.

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

Hey @shiena, sorry it's taken so long to get to this.

Processes are executed in a separate thread, so because of the bug in Unity we can definitely go with synchronous calls here. Also until Unity removes support for their .net 3.5 profile we will not compile this library in .net 4.6.

So when you get the chance remove all the #if/#endif and keep the .net 3.5 side. I think there are a few more changes to make so after that I will review again.

Contributor

StanleyGoldman commented Aug 2, 2017

Hey @shiena, sorry it's taken so long to get to this.

Processes are executed in a separate thread, so because of the bug in Unity we can definitely go with synchronous calls here. Also until Unity removes support for their .net 3.5 profile we will not compile this library in .net 4.6.

So when you get the chance remove all the #if/#endif and keep the .net 3.5 side. I think there are a few more changes to make so after that I will review again.

@StanleyGoldman StanleyGoldman self-requested a review Aug 2, 2017

@StanleyGoldman

Remove all the #if/#endif with NET_4_6

@shiena

This comment has been minimized.

Show comment
Hide comment
@shiena

shiena Aug 2, 2017

Contributor

@StanleyGoldman Thank you for review.
I kept the .net 3.5 side according to the compilation policy.

Contributor

shiena commented Aug 2, 2017

@StanleyGoldman Thank you for review.
I kept the .net 3.5 side according to the compilation policy.

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

💨... Thanks for the quick changes ...
I'm testing your changes against the https://github.com/unity3d-jp/piranhan repo now.

Contributor

StanleyGoldman commented Aug 2, 2017

💨... Thanks for the quick changes ...
I'm testing your changes against the https://github.com/unity3d-jp/piranhan repo now.

Show outdated Hide outdated src/GitHub.Api/NewTaskSystem/ProcessTask.cs
{
var encoded = outputEncoding.GetString(outputBuffer, 0, bytesRead);
outputContents.Append(encoded);
Process.StandardOutput.BaseStream.BeginRead(outputBuffer, 0, outputBuffer.Length, outputCallback, Process.StandardOutput);

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

I believe this line of code should not be here.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

I believe this line of code should not be here.

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

It seems I am mistaken

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

It seems I am mistaken

Show outdated Hide outdated src/GitHub.Api/NewTaskSystem/ProcessTask.cs
{
var encoded = errorEncoding.GetString(errorBuffer, 0, bytesRead);
errorContents.Append(encoded);
Process.StandardError.BaseStream.BeginRead(errorBuffer, 0, errorBuffer.Length, errorCallback, Process.StandardError);

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

I think the same goes for this line.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

I think the same goes for this line.

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

Mistaken here too

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

Mistaken here too

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

I understand why you've done what you've done... 🤔... I'm thinking about it

Contributor

StanleyGoldman commented Aug 2, 2017

I understand why you've done what you've done... 🤔... I'm thinking about it

@StanleyGoldman

I understand what happened now, you worked real hard to keep both async and sync methods possible. Since we are going to go sync, you can rewrite the way the output is being handled to be simpler & easier to follow.

Show outdated Hide outdated src/GitHub.Api/NewTaskSystem/ProcessTask.cs
Process.BeginOutputReadLine();
{
outputReset.Reset();
Process.StandardOutput.BaseStream.BeginRead(

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

Use a loop here to read the bytes and encode the string with the correct encoding. After that you can read the lines and call the appropriate output method. This way we won't need the complicated AsyncCallback that has a reference to itself.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

Use a loop here to read the bytes and encode the string with the correct encoding. After that you can read the lines and call the appropriate output method. This way we won't need the complicated AsyncCallback that has a reference to itself.

This comment has been minimized.

@shiena

shiena Aug 3, 2017

Contributor

If you read the standard output using a loop there, it blocks processing.
Because RunCredentialHelper writes to the standard input of git-credential-wincred command when signing in to GitHub.

Logger.Trace("RunCredentialHelper helper:\"{0}\" action:\"{1}\"", credHelper, action);
SimpleProcessTask task;
if (credHelper.StartsWith('!'))
{
// it's a separate app, run it as such
task = new SimpleProcessTask(taskManager.Token, credHelper.Substring(1).ToNPath(), action);
}
else
{
var args = $"credential-{credHelper} {action}";
task = new SimpleProcessTask(taskManager.Token, args);
}
task.Configure(processManager, true);
task.OnStartProcess += proc =>
{
foreach (var line in lines)
{
proc.StandardInput.WriteLine(line);
}
proc.StandardInput.Close();
};
return task;

For that reason, I think that we need an asynchronous BeginRead.
Please tell me if there is a better way.

@shiena

shiena Aug 3, 2017

Contributor

If you read the standard output using a loop there, it blocks processing.
Because RunCredentialHelper writes to the standard input of git-credential-wincred command when signing in to GitHub.

Logger.Trace("RunCredentialHelper helper:\"{0}\" action:\"{1}\"", credHelper, action);
SimpleProcessTask task;
if (credHelper.StartsWith('!'))
{
// it's a separate app, run it as such
task = new SimpleProcessTask(taskManager.Token, credHelper.Substring(1).ToNPath(), action);
}
else
{
var args = $"credential-{credHelper} {action}";
task = new SimpleProcessTask(taskManager.Token, args);
}
task.Configure(processManager, true);
task.OnStartProcess += proc =>
{
foreach (var line in lines)
{
proc.StandardInput.WriteLine(line);
}
proc.StandardInput.Close();
};
return task;

For that reason, I think that we need an asynchronous BeginRead.
Please tell me if there is a better way.

Show outdated Hide outdated src/GitHub.Api/NewTaskSystem/ProcessTask.cs
Process.BeginErrorReadLine();
{
errorReset.Reset();
Process.StandardError.BaseStream.BeginRead(

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

Do the same thing here.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

Do the same thing here.

Show outdated Hide outdated src/GitHub.Api/NewTaskSystem/ProcessTask.cs
if (Process.StartInfo.RedirectStandardOutput)
outputReset.WaitOne();
if (Process.StartInfo.RedirectStandardError)
errorReset.WaitOne();

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

That way we no longer need these ManualResetEvent objects.

@StanleyGoldman

StanleyGoldman Aug 2, 2017

Contributor

That way we no longer need these ManualResetEvent objects.

StanleyGoldman added some commits Aug 10, 2017

Merge branch 'fixes/process-wrapper' into pr/136-fix-garbled-characte…
…rs-in-history-view-with-non-ascii-characters-fixes-124

# Conflicts:
#	src/GitHub.Api/NewTaskSystem/ProcessTask.cs
@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

Hey @shiena even though I'm supposed to be able to push to the branch, I'm not able to.

Could you merge this branch into your PR and test it out.

Contributor

StanleyGoldman commented Aug 11, 2017

Hey @shiena even though I'm supposed to be able to push to the branch, I'm not able to.

Could you merge this branch into your PR and test it out.

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

I get the impression I need to set Japanese as my default language in Windows to properly debug this error.

Contributor

StanleyGoldman commented Aug 11, 2017

I get the impression I need to set Japanese as my default language in Windows to properly debug this error.

@shiena

This comment has been minimized.

Show comment
Hide comment
@shiena

shiena Aug 11, 2017

Contributor

@StanleyGoldman I merge and tested your branch.
Japanese log was correctly displayed.

Contributor

shiena commented Aug 11, 2017

@StanleyGoldman I merge and tested your branch.
Japanese log was correctly displayed.

StanleyGoldman added some commits Aug 11, 2017

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

@shiena there is another branch based off this, could you test it out for me.

https://github.com/github-for-unity/Unity/tree/fixes/returning-lines-sooner

Contributor

StanleyGoldman commented Aug 11, 2017

@shiena there is another branch based off this, could you test it out for me.

https://github.com/github-for-unity/Unity/tree/fixes/returning-lines-sooner

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

LFS Verification checks are preventing me from pushing to your repo

Contributor

StanleyGoldman commented Aug 11, 2017

LFS Verification checks are preventing me from pushing to your repo

@shiena

This comment has been minimized.

Show comment
Hide comment
@shiena

shiena Aug 11, 2017

Contributor

@StanleyGoldman Japanese also displayed correctly for that branch.

Contributor

shiena commented Aug 11, 2017

@StanleyGoldman Japanese also displayed correctly for that branch.

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

Edited the pull request description to show what this PR depends on

Contributor

StanleyGoldman commented Aug 11, 2017

Edited the pull request description to show what this PR depends on

@shiena

This comment has been minimized.

Show comment
Hide comment
@shiena

shiena Aug 11, 2017

Contributor

@StanleyGoldman I think that #35 has also been fixed.

Contributor

shiena commented Aug 11, 2017

@StanleyGoldman I think that #35 has also been fixed.

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

You may be right, let's see

Contributor

StanleyGoldman commented Aug 11, 2017

You may be right, let's see

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

You are correct.

Contributor

StanleyGoldman commented Aug 11, 2017

You are correct.

Show outdated Hide outdated src/GitHub.Api/NewTaskSystem/ProcessTask.cs
if (token.IsCancellationRequested)
var outputStream = Process.StandardOutput.BaseStream;
var outputBuffer = new byte[bufferSize];
var outputEncoding = Process.StartInfo.StandardOutputEncoding ?? Console.Out.Encoding;

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

Can Process.StartInfo.StandardOutputEncoding ever be null?

@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

Can Process.StartInfo.StandardOutputEncoding ever be null?

This comment has been minimized.

@shiena

shiena Aug 13, 2017

Contributor

It could not be null by tracing the call hierarchy. So delete the null check.

@shiena

shiena Aug 13, 2017

Contributor

It could not be null by tracing the call hierarchy. So delete the null check.

Show outdated Hide outdated src/GitHub.Api/NewTaskSystem/ProcessTask.cs
}
var errorStream = Process.StandardError.BaseStream;
var errorBuffer = new byte[bufferSize];
var errorEncoding = Process.StartInfo.StandardErrorEncoding ?? Console.Error.Encoding;

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

Same question about Process.StartInfo.StandardErrorEncoding ...

@StanleyGoldman

StanleyGoldman Aug 11, 2017

Contributor

Same question about Process.StartInfo.StandardErrorEncoding ...

This comment has been minimized.

@shiena

shiena Aug 13, 2017

Contributor

Also delete null check.

@shiena

shiena Aug 13, 2017

Contributor

Also delete null check.

@shiena

This comment has been minimized.

Show comment
Hide comment
@shiena

shiena Aug 22, 2017

Contributor

@StanleyGoldman @shana
A process encoding bug has been avoided as #181 has been merged. (Process.StandardOutput.ReadLine correctly encodes)
However, if the user sets encoding options other than utf8 in git config, the history view garbled.
Therefore, I recommend setting -C logoutputencoding=utf8 etc for git command line option.

Even if you need the git command line option, this pull request is redundant, so I would like to create another pull request.

Contributor

shiena commented Aug 22, 2017

@StanleyGoldman @shana
A process encoding bug has been avoided as #181 has been merged. (Process.StandardOutput.ReadLine correctly encodes)
However, if the user sets encoding options other than utf8 in git config, the history view garbled.
Therefore, I recommend setting -C logoutputencoding=utf8 etc for git command line option.

Even if you need the git command line option, this pull request is redundant, so I would like to create another pull request.

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Aug 22, 2017

Contributor

@shiena Yup, we were literally just discussing that 10 minutes ago and came to the same conclusion 😄 👍

Contributor

shana commented Aug 22, 2017

@shiena Yup, we were literally just discussing that 10 minutes ago and came to the same conclusion 😄 👍

@StanleyGoldman

This comment has been minimized.

Show comment
Hide comment
@StanleyGoldman

StanleyGoldman Aug 22, 2017

Contributor

Hey @shiena we were about to say the same thing to you...

Contributor

StanleyGoldman commented Aug 22, 2017

Hey @shiena we were about to say the same thing to you...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment