Add repo root dir to retention list before projection updates#1160
Add repo root dir to retention list before projection updates#1160chrisd8088 merged 1 commit intomicrosoft:masterfrom
Conversation
derrickstolee
left a comment
There was a problem hiding this comment.
I only have a minor style comment, but added @kewillford who is the expert with this projection stuff.
| !isFolder || | ||
| folderPlaceholder.IsPossibleTombstoneFolder) | ||
| folderPlaceholder.IsPossibleTombstoneFolder) && | ||
| folderPlaceholder.Path != string.Empty) |
There was a problem hiding this comment.
How about we add the string.Empty to the folderPlaceholdersToKeep when we initialize it since we will always be keeping that folder?
There was a problem hiding this comment.
I thought about that too, but was concerned it might "match against everything" and short-circuit all the path comparisons. However, a little testing and a more careful read of the code shows this is not at all the case, so I rebased with your suggestion implemented in 46cca89. Thanks!
wilbaker
left a comment
There was a problem hiding this comment.
Waiting to hear thoughts on Kevin's suggestion to add the root to the "folders to keep" list
The GitIndexProjection.UpdatePlaceholders() method, after processing the list of placeholder files, iterates over the list of placeholder folders and tries to remove any which are actually files, or not in the projection, or a possible tombstone. The last placeholder folder in the ordered list appears to always correspond to the root of the repository; this placeholder has an empty path string, and IsPathProjected() returns false for both the isProjected and isFolder flags for this entry. Because both flags are false, RemoveFolderPlaceholderIfEmpty() is called, which then results in a call to DeleteFile() in the lower-level ProjFS layer. This always fails, because the root folder is never empty (it contains, among other things, the .git folder), but since RemoveFolderPlaceholderIfEmpty() handles the FSResult.DirectoryNotEmpty error code, no exception is propagated. We can avoid these unnecessary attempts to remove the repository's root folder on "git checkout" operations by adding the empty path to the folderPlaceholdersToKeep list. Co-authored-by: Kevin Willford <kewillf@microsoft.com>
e79b790 to
46cca89
Compare
|
@wilbaker -- reworked using @kewillford's much tidier suggestion! |
kewillford
left a comment
There was a problem hiding this comment.
Looks great thanks! Please update the title and description before merging.
The
GitIndexProjection.UpdatePlaceholders()method, after processing the list of placeholder files, iterates over the list of placeholder folders and tries to remove any which are actually files, or not in the projection, or a possible tombstone.The last placeholder folder in the ordered list appears to always correspond to the root of the repository; this placeholder has an empty path string, and
IsPathProjected()returnsfalsefor both theisProjectedandisFolderflags for this entry.Because both flags are
false,RemoveFolderPlaceholderIfEmpty()is called, which then results in a call toDeleteFile()in the lower-levelProjFSlayer. This always fails, because the root folder is never empty (it contains, among other things, the.gitfolder), but sinceRemoveFolderPlaceholderIfEmpty()handles theFSResult.DirectoryNotEmptyerror code, no exception is propagated.We can avoid these unnecessary attempts to remove the repository's root folder on
git checkoutoperations by adding the empty path to thefolderPlaceholdersToKeeplist.This behaviour was noted while testing #1128, which implements the underlying
DeleteFile()method on Linux, where the use of a true filesystem mount point for the root of the VFSForGit repository means that an attempt tormdir(2)the repository's root directory does not returnENOTEMPTYbut ratherEBUSY(Device or resource busy). This is not silently handled byRemoveFolderPlaceholderIfEmpty()but instead logs an error./cc @jrbriggs, @kivikakk