-
Notifications
You must be signed in to change notification settings - Fork 451
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
Mac: Handle projection changes where git deletes folders that are still in the projection #368
Mac: Handle projection changes where git deletes folders that are still in the projection #368
Conversation
…ll in the projection
// - Create an empty file if none exists | ||
// - Fail if a file already exists at this path | ||
FILE* file = fopen(fullPath, "wbx"); | ||
// Mode "wx" means: |
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.
Why did you remove the "b" here?
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.
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.
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.
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.
new PlaceholderListDatabase.PlaceholderData(childRelativePath, sha)); | ||
break; | ||
|
||
case FSResult.FileOrPathNotFound: |
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.
Should we just check if the parent directory exists before creating the placeholder, to avoid this happening in the first place?
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.
Should we just check if the parent directory exists before creating the placeholder, to avoid this happening in the first place?
We could do that (I actually started with that approach), but I figured the more common case is that git has not deleted a folder as part of the projection change.
By using the current approach we avoid the cost of checking the disk for every directory that has to add a new file.
At this point I have not done any perf testing to measure the impact of checking if the directory exists, however, historically it's been worth it to avoid unnecessary I/O during projection changes.
I'm not sure that checking for the directory's existence first would make the code much simpler, but let me know if you disagree.
…tory to return a more specific error message
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.
Latest changes look good
Fixes #367
The problem was that VFS for Git was assuming that all folder placeholders that existed prior to a git command running would still exist after the git command completes. The fix was to remove missing folders from
existingFolderPlaceholders
when the folder is not on disk so it will be created again (when the folder's parent is expanded).Changes
CheckoutBranchDirectoryWithOneFileWrite
testPrjFS_WritePlaceholderFile
to return more specific error codes when part of the path does not existReExpandFolder
to handle the scenario wherePrjFS_WritePlaceholderFile
fails due to git deleting the folder that's been expanded