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

Merged
merged 1 commit into from Nov 2, 2019

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Oct 25, 2019

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

@AustinWise
Copy link
Contributor Author

Specifically, the commondir file contains ../... In my limited testing this only effects debug builds. Release builds without Debug.Assert seem happy with the relative path.

@@ -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
Copy link
Member

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(...))

@tmat
Copy link
Member

tmat commented Oct 30, 2019

@AustinWise Thanks for the PR. We are definitely missing GetFullPath call. Would you mind adding a test to GitRepositoryTests, similar to

public void TryFindRepository_Worktree()
{
using var temp = new TempRoot();
var mainWorkingDir = temp.CreateDirectory();
var mainWorkingSubDir = mainWorkingDir.CreateDirectory("A");
var mainGitDir = mainWorkingDir.CreateDirectory(".git");
mainGitDir.CreateFile("HEAD");
var worktreeGitDir = temp.CreateDirectory();
var worktreeGitSubDir = worktreeGitDir.CreateDirectory("B");
var worktreeDir = temp.CreateDirectory();
var worktreeSubDir = worktreeDir.CreateDirectory("C");
var worktreeGitFile = worktreeDir.CreateFile(".git").WriteAllText("gitdir: " + worktreeGitDir + " \r\n\t\v");
worktreeGitDir.CreateFile("HEAD");
worktreeGitDir.CreateFile("commondir").WriteAllText(mainGitDir.Path + " \r\n\t\v");
worktreeGitDir.CreateFile("gitdir").WriteAllText(worktreeGitFile.Path + " \r\n\t\v");
// start under main repository directory:
Assert.True(GitRepository.TryFindRepository(mainWorkingSubDir.Path, out var location));
Assert.Equal(mainGitDir.Path, location.GitDirectory);
Assert.Equal(mainGitDir.Path, location.CommonDirectory);
Assert.Equal(mainWorkingDir.Path, location.WorkingDirectory);
?

@AustinWise
Copy link
Contributor Author

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.
@AustinWise
Copy link
Contributor Author

@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 git worktree currently creates. So perhaps the old test should be replaced with the new test?

@tmat
Copy link
Member

tmat commented Nov 2, 2019

Thanks!

@tmat tmat merged commit 16fa3ff into dotnet:master Nov 2, 2019
@AustinWise AustinWise deleted the austin/NormalizeGitCommonPath branch July 24, 2020 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SourceLink fails in git worktree and emits many build warnings
2 participants