This repository was archived by the owner on Jun 21, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Be consistent about how we use relative paths and Git paths #2386
Merged
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToWindowsPath
would be clearer.
ToWindowsPath is clearer because it doesn't convert from an absolute path.
jcansdale
commented
Aug 9, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me.
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
LibGit2Sharp
we were using prior to #2142,LibGit2Sharp
didn'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
LibGit2Sharp
didn'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
relativePath
when a parameter contains a Windows style relative pathgitPath
when a parameter contains a Git style relative pathGuard.ArgumentIsRelativePath
to check we're not passing absolute pathsPaths.ToGitPath
andPaths.ToWindowsPath
AddBufferTag
an absolute pathPullRequestSessionFile
used a Git relative path andPullRequestSessionLiveFile
a Windows relative pathGetOldFileName
was using a Windows style relative pathHow to test
Here is how to test one of the bugs it fixes
View File
Fixes #2380