This repository was archived by the owner on Jun 21, 2023. It is now read-only.
Be consistent about how we use relative paths and Git paths #2386
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Consistently new up PullRequestTextBufferInfo with Git relative path.
co-authoried-by: Jamie Cansdale <jcansdale@gmail.com>
Use Guard.ArgumentIsRelativePath instead of Guard.ArgumentIsGitPath Automatically convert to Git path where required.
No need to convert from full path back to gitPath.
jcansdale
commented
Jun 13, 2019
| @@ -119,7 +120,7 @@ public class PullRequestEditorService : IPullRequestEditorService | |||
|
|
|||
| if (!workingDirectory) | |||
| { | |||
| AddBufferTag(wpfTextView.TextBuffer, session, fullPath, commitSha, null); | |||
| AddBufferTag(wpfTextView.TextBuffer, session, relativePath, commitSha, null); | |||
There was a problem hiding this comment.
This is the fix for #2380. AddBufferTag expects a relative path.
jcansdale
commented
Aug 9, 2019
| /// </summary> | ||
| /// <param name="gitPath">A working directory relative path which uses the '/' directory separator.</param> | ||
| /// <returns>A relative paht that uses the <see cref="Path.DirectorySeparatorChar"/> directory separator.</returns> | ||
| public static string ToRelativePath(string gitPath) |
There was a problem hiding this comment.
ToWindowsPath would be clearer.
ToWindowsPath is clearer because it doesn't convert from an absolute path.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In versions of
LibGit2Sharpwe were using prior to #2142,LibGit2Sharpdidn't care whether paths passed contained/or\and could cope with absolute as well as working directory relative paths. In recent versions ofLibGit2Sharp, Git paths must be working directory relative and use/as their directory separator. If they're not in this format the operation will silently fail (as if the file simply doesn't exist).Because GitHub for Visual Studio was developed when
LibGit2Sharpdidn't care about directory separators, we often mixed up relative paths and Git paths (which use '/'). This has created the potential for a whole category of bugs and has made the code difficult to reason about. This PR is attempt to bring a bit of sanity to the situation.What this PR does
relativePathwhen a parameter contains a Windows style relative pathgitPathwhen a parameter contains a Git style relative pathGuard.ArgumentIsRelativePathto check we're not passing absolute pathsPaths.ToGitPathandPaths.ToWindowsPathAddBufferTagan absolute pathPullRequestSessionFileused a Git relative path andPullRequestSessionLiveFilea Windows relative pathGetOldFileNamewas using a Windows style relative pathHow to test
Here is how to test one of the bugs it fixes
View FileFixes #2380