Skip to content

Mac: Handle projection changes where git deletes folders that are still in the projection #368

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.

Sign up for GitHub

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

Merged
wilbaker merged 4 commits into from
Oct 10, 2018
Merged

Mac: Handle projection changes where git deletes folders that are still in the projection #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1af2617
Mac: Handle projection changes where git deletes folders that are sti…
wilbaker Oct 9, 2018
5d62ef3
Fix StyleCop issue
wilbaker Oct 10, 2018
d57e791
Add additional functional test and update PrjFS_WritePlaceholderDirec…
wilbaker Oct 10, 2018
74743c0
Code cleanup
wilbaker Oct 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 47 additions & 1 deletion GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,12 +892,58 @@ public void CheckoutBranchDirectoryWithOneFileRead()
}

[TestCase]
[Category(Categories.MacTODO.M3)]
public void CheckoutBranchDirectoryWithOneFileWrite()
Comment thread
wilbaker marked this conversation as resolved.
{
this.RunFileDirectoryWriteTest("checkout", commandBranch: GitRepoTests.DirectoryWithDifferentFileAfterBranch);
}

[TestCase]
public void CheckoutBranchDirectoryWithOneDeepFileWrite()
{
this.ControlGitRepo.Fetch(GitRepoTests.DeepDirectoryWithOneFile);
this.ControlGitRepo.Fetch(GitRepoTests.DeepDirectoryWithOneDifferentFile);
this.ValidateGitCommand($"checkout {GitRepoTests.DeepDirectoryWithOneFile}");
this.FileShouldHaveContents(
"TestFile1\n",
"GitCommandsTests",
"CheckoutBranchDirectoryWithOneDeepFile",
"FolderDepth1",
"FolderDepth2",
"FolderDepth3",
"File1.txt");

// Edit the file and commit the change so that git will
// delete the file (and its parent directories) when
// changing branches
this.EditFile(
"Change file",
"GitCommandsTests",
"CheckoutBranchDirectoryWithOneDeepFile",
"FolderDepth1",
"FolderDepth2",
"FolderDepth3",
"File1.txt");
this.ValidateGitCommand("add --all");
this.RunGitCommand("commit -m \"Some change\"");

this.ValidateGitCommand($"checkout {GitRepoTests.DeepDirectoryWithOneDifferentFile}");
this.FileShouldHaveContents(
"TestFile2\n",
"GitCommandsTests",
"CheckoutBranchDirectoryWithOneDeepFile",
"FolderDepth1",
"FolderDepth2",
"FolderDepth3",
"File2.txt");
this.ShouldNotExistOnDisk(
"GitCommandsTests",
"CheckoutBranchDirectoryWithOneDeepFile",
"FolderDepth1",
"FolderDepth2",
"FolderDepth3",
"File1.txt");
}

private static void CopyIndexAndRename(string indexPath)
{
string tempIndexPath = indexPath + ".lock";
Expand Down
2 changes: 2 additions & 0 deletions GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ public abstract class GitRepoTests
protected const string DirectoryWithFileBeforeBranch = "FunctionalTests/20171025_DirectoryWithFileBefore";
protected const string DirectoryWithFileAfterBranch = "FunctionalTests/20171025_DirectoryWithFileAfter";
protected const string DirectoryWithDifferentFileAfterBranch = "FunctionalTests/20171025_DirectoryWithDifferentFile";
protected const string DeepDirectoryWithOneFile = "FunctionalTests/20181010_DeepFolderOneFile";
protected const string DeepDirectoryWithOneDifferentFile = "FunctionalTests/20181010_DeepFolderOneDifferentFile";

private bool enlistmentPerTest;

Expand Down
53 changes: 35 additions & 18 deletions GVFS/GVFS.Virtualization/Projection/GitIndexProjection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,8 +1158,11 @@ private void UpdatePlaceholders()
new HashSet<string>(placeholderFoldersListCopy.Select(x => x.Path), StringComparer.OrdinalIgnoreCase) :
null;

// Order the folders in decscending order so that we walk the tree from bottom up (ensuring child folders are deleted before
// their parents)
// Order the folders in decscending order so that we walk the tree from bottom up.
// Traversing the folders in this order:
// 1. Ensures child folders are deleted before their parents
// 2. Ensures that folders that have been deleted by git (but are still in the projection) are found before their
// parent folder is re-expanded (only applies on platforms where EnumerationExpandsDirectories is true)
foreach (PlaceholderListDatabase.PlaceholderData folderPlaceholder in placeholderFoldersListCopy.OrderByDescending(x => x.Path))
{
// Remove folder placeholders before re-expansion to ensure that projection changes that convert a folder to a file work
Expand Down Expand Up @@ -1388,28 +1391,42 @@ private void ReExpandFolder(
childRelativePath = relativeFolderPath + Path.DirectorySeparatorChar + childEntry.Name.GetString();
}

// TODO(Mac): Issue #245, handle failures of WritePlaceholderDirectory and WritePlaceholderFile
if (childEntry.IsFolder)
bool newChild = childEntry.IsFolder ? !existingFolderPlaceholders.Contains(childRelativePath) : !updatedPlaceholderList.ContainsKey(childRelativePath);

if (newChild)
{
if (!existingFolderPlaceholders.Contains(childRelativePath))
FileSystemResult result;
string fileShaOrFolderValue;
if (childEntry.IsFolder)
{
this.fileSystemVirtualizer.WritePlaceholderDirectory(childRelativePath);
updatedPlaceholderList.TryAdd(
childRelativePath,
new PlaceholderListDatabase.PlaceholderData(childRelativePath, PlaceholderListDatabase.PartialFolderValue));
fileShaOrFolderValue = PlaceholderListDatabase.PartialFolderValue;
result = this.fileSystemVirtualizer.WritePlaceholderDirectory(childRelativePath);
}
}
else
{
if (!updatedPlaceholderList.ContainsKey(childRelativePath))
else
{
FileData childFileData = childEntry as FileData;
string sha = childFileData.Sha.ToString();
fileShaOrFolderValue = childFileData.Sha.ToString();
result = this.fileSystemVirtualizer.WritePlaceholderFile(childRelativePath, childFileData.Size, fileShaOrFolderValue);
}

switch (result.Result)
{
case FSResult.Ok:
updatedPlaceholderList.TryAdd(
childRelativePath,
new PlaceholderListDatabase.PlaceholderData(childRelativePath, fileShaOrFolderValue));
break;

this.fileSystemVirtualizer.WritePlaceholderFile(childRelativePath, childFileData.Size, sha);
updatedPlaceholderList.TryAdd(
childRelativePath,
new PlaceholderListDatabase.PlaceholderData(childRelativePath, sha));
case FSResult.FileOrPathNotFound:
// Git command must have removed the folder being re-expanded (relativeFolderPath)
// Remove the folder from existingFolderPlaceholders so that its parent will create
// it again (when it's re-expanded)
existingFolderPlaceholders.Remove(relativeFolderPath);
return;

default:
// TODO(Mac): Issue #245, handle failures of WritePlaceholderDirectory and WritePlaceholderFile
break;
}
}
}
Expand Down
41 changes: 35 additions & 6 deletions ProjFS.Mac/PrjFSLib/PrjFSLib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,24 +281,37 @@ PrjFS_Result PrjFS_WritePlaceholderDirectory(
return PrjFS_Result_EInvalidArgs;
}

PrjFS_Result result = PrjFS_Result_Invalid;
char fullPath[PrjFSMaxPath];
CombinePaths(s_virtualizationRootFullPath.c_str(), relativePath, fullPath);

if (mkdir(fullPath, 0777))
{
switch(errno)
{
// TODO(Mac): Return more specific error codes for other failure scenarios
case ENOENT: // A component of the path prefix does not exist or path is an empty string
result = PrjFS_Result_EPathNotFound;
break;
default:
result = PrjFS_Result_EIOError;
break;
}

goto CleanupAndFail;
}

if (!InitializeEmptyPlaceholder(fullPath))
{
result = PrjFS_Result_EIOError;
goto CleanupAndFail;
}

return PrjFS_Result_Success;

CleanupAndFail:
// TODO: cleanup the directory on disk if needed
return PrjFS_Result_EIOError;
return result;
}

PrjFS_Result PrjFS_WritePlaceholderFile(
Expand All @@ -323,23 +336,37 @@ PrjFS_Result PrjFS_WritePlaceholderFile(
return PrjFS_Result_EInvalidArgs;
}

PrjFS_Result result = PrjFS_Result_Invalid;
PrjFSFileXAttrData fileXattrData = {};

char fullPath[PrjFSMaxPath];
CombinePaths(s_virtualizationRootFullPath.c_str(), relativePath, fullPath);

// Mode "wbx" means
// - Create an empty file if none exists
// - Fail if a file already exists at this path
FILE* file = fopen(fullPath, "wbx");
// Mode "wx" means:
Copy link
Copy Markdown
Contributor

@sanoursa sanoursa Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the "b" here?

Copy link
Copy Markdown
Member Author

@wilbaker wilbaker Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the "b" here?

When I was digging into the documentation for fopen I found the "b" wasn't doing anything.

From the Mac manpage for fopen:

The mode string can also include the letter "b" after either the "+'' or the first letter. This is strictly for compatibility with ISO/IEC 9899:1990 ('ISO C90') and has effect only for fmemopen() ; otherwise "b'' is ignored.

The linux documentation had a few more details:

The mode string can also include the letter 'b' either as a last character or as a character between the characters in any of the two-character strings described above. This is strictly for compatibility with C89 and has no effect; the 'b' is ignored on all POSIX conforming systems, including Linux. (Other systems may treat text files and binary files differently, and adding the 'b' may be a good idea if you do I/O to a binary file and expect that your program may be ported to non-UNIX environments.)

From this it sounds like C's fopen has "b" to support non-POSIX systems, but that on Mac it's not doing anything.

Let me know if you'd rather I leave it in as it doesn't appear to be hurting anything.

Copy link
Copy Markdown
Contributor

@sanoursa sanoursa Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I was looking at more generic docs that said that if you don't specify "b", it can translate some of the characters as you read/write, which we don't want. If it's a noop on Mac, then this is fine.

// - "w": Open for writing. The stream is positioned at the beginning of the file. Create the file if it does not exist.
// - "x": If the file already exists, fopen() fails, and sets errno to EEXIST.
FILE* file = fopen(fullPath, "wx");
if (nullptr == file)
{
switch(errno)
{
// TODO(Mac): Return more specific error codes for other failure scenarios
case ENOENT: // A directory component in fullPath does not exist or is a dangling symbolic link.
result = PrjFS_Result_EPathNotFound;
break;
case EEXIST: // The file already exists
default:
result = PrjFS_Result_EIOError;
break;
}

goto CleanupAndFail;
}

// Expand the file to the desired size
if (ftruncate(fileno(file), fileSize))
{
result = PrjFS_Result_EIOError;
goto CleanupAndFail;
}

Expand All @@ -354,12 +381,14 @@ PrjFS_Result PrjFS_WritePlaceholderFile(
&fileXattrData,
PrjFSFileXAttrName))
{
result = PrjFS_Result_EIOError;
goto CleanupAndFail;
}

// TODO(Mac): Only call chmod if fileMode is different than the default file mode
if (chmod(fullPath, fileMode))
{
result = PrjFS_Result_EIOError;
goto CleanupAndFail;
}

Expand All @@ -375,7 +404,7 @@ PrjFS_Result PrjFS_WritePlaceholderFile(
file = nullptr;
}

return PrjFS_Result_EIOError;
return result;
}

PrjFS_Result PrjFS_WriteSymLink(
Expand Down