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.
Why did you remove the "b" here?
There was a problem hiding this comment.
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.
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.
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.
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
sanoursa
left a comment
sanoursa
left a comment
There was a problem hiding this comment.
Latest changes look good
2 participants
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
existingFolderPlaceholderswhen the folder is not on disk so it will be created again (when the folder's parent is expanded).Changes
CheckoutBranchDirectoryWithOneFileWritetestPrjFS_WritePlaceholderFileto return more specific error codes when part of the path does not existReExpandFolderto handle the scenario wherePrjFS_WritePlaceholderFilefails due to git deleting the folder that's been expanded