Mac: File modified notifications#156
Conversation
| throw new InvalidOperationException(nameof(this.WriteToDisk) + " requires that collectionAppendsDirectlyToFile be true"); | ||
| } | ||
|
|
||
| // TODO(Mac): Decide if we want to stick with \r\n on all platforms, move them all to \n, or use a mix |
There was a problem hiding this comment.
Do we need to keep this TODO? As far as I know, both Windows and Mac are happy with this format as is.
There was a problem hiding this comment.
@sanoursa Good point, both platforms appear to be happy with it so we can take it out
| fileToOverwriteVirtualPath.ShouldBeAFile(this.fileSystem).WithContents(testContents); | ||
|
|
||
| GVFSHelpers.ModifiedPathsShouldContain(this.fileSystem, this.Enlistment.DotGVFSRoot, fileToOverwriteEntry + Environment.NewLine); | ||
| // TODO(Mac): Switch from "\r\n" to Environment.NewLine or "\n" depending on how FileBasedCollection is updated |
| switch (Path.DirectorySeparatorChar) | ||
| { | ||
| case '/': | ||
| cleanedPath = path.Replace('\\', Path.DirectorySeparatorChar); |
There was a problem hiding this comment.
This line is a bit worrisome - it seems like it could hide tests that are still assuming Windows slashes, rather than force us to clean up the tests.
I would expect this method to convert from Git paths to file system paths, but not to cover over platform-related differences. So to me, that means that it should do a conversion on Windows, and noop on Mac. Thoughts?
There was a problem hiding this comment.
@sanoursa I went back and forth on this as well. I agree with your assessment that we might miss cleaning up tests with this change. I'll switch this to just convert Git path to system path.
|
|
||
| ulong fileSize, | ||
| UInt16 fileMode); | ||
| ushort fileMode); |
There was a problem hiding this comment.
I'm curious, why did you change this type?
There was a problem hiding this comment.
@sanoursa I had to update these types after enabling StyleCop for the project
| OnGetFileStream = this.OnGetFileStream, | ||
|
|
||
| // Note: We must use this.notifyOperationCallback and not this.OnNotifyOperation | ||
| // to ensure we don't create a temporary delegate that will be garbage collected |
There was a problem hiding this comment.
It would be nice to make this more explicit in the code itself. Perhaps something like this?
OnNotifyOperation = (this.preventGCOnNotifyOperationDelegate = new NotifyOperationCallback(this.OnNotifyOperation)),
The root cause of the issue is that there is an implicit delegate allocation, so let's make at least that much of it explicit in the code. I think it would also be clearer to store that instance here, rather than in the constructor.
There was a problem hiding this comment.
I think it would also be clearer to store that instance here, rather than in the constructor.
That sounds good to me (note that preventGCOnNotifyOperationDelegate will no longer be able to be readonly with this change).
|
I had tested your last round of changes and it all worked great. I was able to write changes into a newly hydrated file, and |
3 participants
Part of the work required for #152
These changes add file modified notification handling to kernel and user mode code.
Additional functional tests have also been enabled: