8000 Be consistent about how we use relative paths and Git paths by jcansdale · Pull Request #2386 · github/VisualStudio · GitHub
[go: up one dir, main page]

Skip to content
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

Merged
merged 11 commits into from
Aug 9, 2019

Conversation

jcansdale
Copy link
Collaborator
@jcansdale jcansdale commented Jun 13, 2019

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 of LibGit2Sharp, 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

  • Use relativePath when a parameter contains a Windows style relative path
  • Use gitPath when a parameter contains a Git style relative path
  • Use Guard.ArgumentIsRelativePath to check we're not passing absolute paths
  • Automatically convert Windows paths to Git style relative paths before using them
  • Factor out conversions into Paths.ToGitPath and Paths.ToWindowsPath
  • Fix Inline comment margin doesn't appear when viewing file #2380 which was caused by passing AddBufferTag an absolute path
  • Fix issue where PullRequestSessionFile used a Git relative path and PullRequestSessionLiveFile a Windows relative path
  • Fix issue where GetOldFileName was using a Windows style relative path

How to test

Here is how to test one of the bugs it fixes

  1. Open a solution that contains pull requests
  2. Open the pull request detail view without checking out the PR
  3. Right-click on a PR file that has comments and View File
  4. Check that comments appear on comment margin

Fixes #2380

jcansdale and others added 8 commits June 10, 2019 16:11
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.
@@ -119,7 +120,7 @@ public class PullRequestEditorService : IPullRequestEditorService

if (!workingDirectory)
{
AddBufferTag(wpfTextView.TextBuffer, session, fullPath, commitSha, null);
AddBufferTag(wpfTextView.TextBuffer, session, relativePath, commitSha, null);
Copy link
Collaborator Author

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.

/// </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)
Copy link
Collaborator Author

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.
Copy link
Collaborator Author
@jcansdale jcansdale left a 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.

@jcansdale jcansdale merged commit ce9e5c4 into master Aug 9, 2019
@jcansdale jcansdale deleted the refactor/relative-and-git-paths branch August 9, 2019 16:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline comment margin doesn't appear when viewing file
2 participants
0