Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Don't change file encoding unnecessarily. #2009

Merged
merged 2 commits into from
Oct 29, 2018
Merged

Don't change file encoding unnecessarily. #2009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84c37c9
Don't change file encoding unnecessarily.
grokys Oct 23, 2018
f1bab19
Merge branch 'master' into fixes/1908-unicode-error
jcansdale Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/GitHub.App/Services/PullRequestService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ public Encoding GetEncoding(ILocalRepositoryModel repository, string relativePat
}
}

return Encoding.Default;
return null;
Copy link
Collaborator

@jcansdale jcansdale Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe can we can simply replace this with the following and avoid having to spacial case null.

Suggested change
return null;
return new UTF8Encoding(false);

This makes the GetEncoding responsible for returning Encoding.UTF8 if the target file has a preamble or new UTF8Encoding(false) if the target file doesn't exist or have a preamble. UTF8 without preamble is Git's default text encoding.

Here is what I see if we return new UTF8Encoding(false):

image

It's possible we could always use new UTF8Encoding(false) and avoid this issue completely. The question is what a diff looks like between UTF8 files with and without preamble. I didn't know new UTF8Encoding(false) was a thing until today.

Copy link
Collaborator

@jcansdale jcansdale Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: wondering if we could always use new UTF8Encoding(false). Alas, this doesn't work. I tried the following:

File.WriteAllText("foo.txt", "foobar", new UTF8Encoding(true));
File.WriteAllText("bar.txt", "foobar", new UTF8Encoding(false));

When I diff them together, I see:
image

I think returning return new UTF8Encoding(false) when there is no preamble or file is probably the cleanest solution.

}

static bool HasPreamble(string file, Encoding encoding)
Expand Down Expand Up @@ -877,7 +877,15 @@ async Task ExtractToTempFile(
}

Directory.CreateDirectory(Path.GetDirectoryName(tempFilePath));
File.WriteAllText(tempFilePath, contents, encoding);

if (encoding != null)
{
File.WriteAllText(tempFilePath, contents, encoding);
}
else
{
File.WriteAllText(tempFilePath, contents);
}
}

IEnumerable<string> GetLocalBranchesInternal(
Expand Down Expand Up @@ -963,7 +971,7 @@ static string CalculateTempFileName(string relativePath, string commitSha, Encod
{
// The combination of relative path, commit SHA and encoding should be sufficient to uniquely identify a file.
var relativeDir = Path.GetDirectoryName(relativePath) ?? string.Empty;
var key = relativeDir + '|' + encoding.WebName;
var key = relativeDir + '|' + (encoding?.WebName ?? "unknown");
var relativePathHash = key.GetSha256Hash();
var tempDir = Path.Combine(Path.GetTempPath(), "GitHubVisualStudio", "FileContents", relativePathHash);
var tempFileName = Invariant($"{Path.GetFileNameWithoutExtension(relativePath)}@{commitSha}{Path.GetExtension(relativePath)}");
Expand Down
8 changes: 5 additions & 3 deletions src/GitHub.Exports.Reactive/Services/IPullRequestService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,12 @@ IObservable<IPullRequestModel> CreatePullRequest(IModelService modelService,
IObservable<Tuple<string, int>> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository);

/// <summary>
/// Gets the encoding for the specified file.
/// Gets the encoding for the specified local file.
/// </summary>
/// <param name="repository">The repository.</param>
/// <param name="relativePath">The relative path to the file in the repository.</param>
/// <returns>
/// The file's encoding or <see cref="Encoding.Default"/> if the file doesn't exist.
/// The file's encoding or null if the file doesn't exist locally.
/// </returns>
Encoding GetEncoding(ILocalRepositoryModel repository, string relativePath);

Expand All @@ -192,7 +192,9 @@ IObservable<IPullRequestModel> CreatePullRequest(IModelService modelService,
/// <param name="pullRequest">The pull request details.</param>
/// <param name="relativePath">The path to the file, relative to the repository root.</param>
/// <param name="commitSha">The SHA of the commit.</param>
/// <param name="encoding">The encoding to use.</param>
/// <param name="encoding">
/// The encoding to save the file with. If null, will use the file's original encoding.
/// </param>
/// <returns>The path to the temporary file.</returns>
Task<string> ExtractToTempFile(
ILocalRepositoryModel repository,
Expand Down