-
Notifications
You must be signed in to change notification settings - Fork 123
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
Normalize git commondir path. #463
Conversation
Specifically, the commondir file contains |
@@ -491,6 +491,8 @@ private static bool IsGitDirectory(string directory, out string commonDirectory) | |||
try | |||
{ | |||
commonDirectory = Path.Combine(directory, File.ReadAllText(commonLinkPath).TrimEnd(CharUtils.AsciiWhitespace)); | |||
// noralize relative paths |
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.
Typo in comment. I don't think the comment is needed since it's obvious what GetFullPath
does. Also, you can simplify by calling Path.GetFullPath()
directly on the previous line Path.GetFullPath(Path.Combine(...))
@AustinWise Thanks for the PR. We are definitely missing
|
Ok, I will update with a test and a fix for the typo. |
The constructor for GitRepositoryLocation asserts that this path is normalized. On my machine, a git worktree created by git 2.22.0 has a commondir file that contains "../..". This results in path that does not satisfy this assert.
82962fe
to
0244a3a
Compare
@tmat I added tests and fixed the comment. I was not sure if you want me to replace the existing test or add a new one, so for now I added a new one. The existing test creates a git layout that does not match the repository specification nor the layout |
Thanks! |
The constructor for GitRepositoryLocation asserts that this path is
normalized. On my machine, a git worktree created by Git 2.22.0 does not
result in path that satisfies this assert.
Fixes #473