Hardlink notifications on Mac & Windows and rename notifications on Mac by wilbaker · Pull Request #203 · microsoft/VFSForGit

wilbaker · 2018-08-22T16:57:31Z

Part of the work for #152 and fixes #200.

Summary of changes

Added hardlink notifications to VFSForGit for Mac and Windows
Added new hardlink functional tests
Added rename notifications on Mac
Enabled additional functional tests on Mac

Future work

Unlike VFSForGit on Windows, on Mac users can rename\move folders have not been expanded. To ensure that all placeholders are on disk prior to the rename the folder must be expanded in the PreDelete notification (#202)

Functional test run (on Mac) with these changes:

Test Run Summary
  Overall result: Warning
  Test Count: 206, Passed: 204, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 2
    Skipped Tests - Ignored: 2, Explicit: 0, Other: 0
  Start time: 2018-08-22 22:09:59Z
    End time: 2018-08-22 22:14:22Z
    Duration: 263.482 seconds

sanoursa

Just a quick first pass to make sure I understand the overall logic. I still need to spend a little more time with it, and test it out as well.

sanoursa · 2018-08-22T17:05:20Z

GVFS/GVFS.FunctionalTests/FileSystemRunners/PowerShellRunner.cs


+        public override void CreateHardLink(string targetPath, string newLinkPath)
+        {
+            Assert.Fail($"{nameof(PowerShellRunner)} does not support {nameof(this.CreateHardLink)}");


Nit: move this to the base class?

Sure, I'll move a default SupportsHardlinkCreation to the base as well

sanoursa · 2018-08-22T17:10:15Z

GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs

+            string relativePath,
+            string destinationPath)
+        {
+            this.OnHardLinkCreated(relativePath, destinationPath);


Are these parameters ordered properly? The first parameter is ambiguous: relativePath could map to either relativeTargetPath or relativeNewLinkPath. But the second parameter sure looks like it maps better to relativeTargetPath, which makes me think these are swapped?

These are ordered correctly.

I agree the naming could be better (and I could change these names), but as a start I went with the approach of matching the parameter names declared in the ProjFS Managed API:

/// <param name="relativePath">The path, relative to the virtualization root, of the file for /// which the hard link was created. /// <para>This parameter will be "" to indicate that the hard link name was created inside /// the virtualization instance, i.e. a new hard link was created inside the virtualization /// instance to a file that exists outside the virtualization instance.</para> /// </param> /// <param name="destinationPath">The path, relative to the virtualization root, of the new hard /// link name. /// <para>This parameter will be "" to indicate that the hard link name was created /// outside the virtualization instance, i.e. a new hard link was created outside the /// virtualization instance for a file that exists inside the virtualization instance.</para> /// </param>

Let me know if you think it's better to match the API names or use our own names.

sanoursa · 2018-08-22T17:10:47Z

GVFS/GVFS.Virtualization/FileSystem/FileSystemVirtualizer.cs

+            }
+        }
+
+        protected void OnHardLinkCreated(string relativeTargetPath, string relativeNewLinkPath)


Why do we even need the relativeTargetPath parameter? It doesn't seem to be used at all.

And why would it always be relative? Couldn't you create a hardlink inside your repo to some path outside of it? It seems like the target would need to be treated as an absolute path.

Why do we even need the relativeTargetPath parameter? It doesn't seem to be used at all.

It's not strictly needed, but we do log it if there's an exception:

metadata.Add(nameof(relativeTargetPath), relativeTargetPath);

My thought was that if something goes wrong it's helpful to have as much information as we can.

And why would it always be relative? Couldn't you create a hardlink inside your repo to some path outside of it? It seems like the target would need to be treated as an absolute path.

See related comment as well (for detailed descriptions of these paths), if either of the paths are outside of the repo they will be empty strings.

sanoursa · 2018-08-22T17:12:53Z

MirrorProvider/MirrorProvider.Mac/MacFileSystemVirtualizer.cs

            Console.WriteLine($"OnNewFileCreated (isDirectory: {isDirectory}): {relativePath}");
        }
+
+        private void OnFileRenamed(string relativeDestinationPath, bool isDirectory)


It seems really odd to get a rename event with only a single path in it. I understand the reasons why, but it's confusing from an API perspective. Should we just treat this as a new file event instead, and just not have a rename event?

For historical purposes, it should be noted that we could deliver both paths on Mac, but because VFSForGit does not strictly need the source path on Mac we opted to keep the kext simpler and only deliver the destination path for now.

but it's confusing from an API perspective. Should we just treat this as a new file event instead, and just not have a rename event?

I've found it helpful when debugging\developing to keep these as separate events (and Keeping them as separate events also makes us more consistent with ProjFS on Windows).

I lean toward keeping them separate and having VFSForGit decide to handle this notification in the same way as a new file notification.

I've added some additional comments in the notification callback

I've added some additional comments in the notification callback

Actually, comments were added in VFSForGit, not in the MirrorProvider

sanoursa · 2018-08-22T17:13:09Z

MirrorProvider/MirrorProvider.Windows/WindowsFileSystemVirtualizer.cs

        }

+        private void OnFileRenamed(
+            string relativePath,


relativeSourcePath?

See other comment as well. Currently these names match the names used in the ProjFS API, let me know if you think it's better to keep them consistent or change them.

sanoursa · 2018-08-22T17:13:26Z

MirrorProvider/MirrorProvider.Windows/WindowsFileSystemVirtualizer.cs

+        }
+
+        private void OnHardlinkCreated(
+            string relativePath,


Same question as elsewhere - there's no guarantee this is relative, right?

Er, for one of these two parameters. I actually can't tell which one is the path of the link and which one is the path of the target :-)

Same question as elsewhere - there's no guarantee this is relative, right?

As discussed elsewhere, any paths outside of the root will be an empty string (by design)

I actually can't tell which one is the path of the link and which one is the path of the target :-)

Same comment here too, do you think we should use our own names or stay consistent with ProjFS?

sanoursa · 2018-08-22T17:14:16Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

 static uint32_t ReadVNodeFileFlags(vnode_t vn, vfs_context_t context);
-static inline bool FileFlagsBitIsSet(uint32_t fileFlags, uint32_t bit);
-static inline bool FileIsFlaggedAsInRoot(vnode_t vnode, vfs_context_t context);
-static inline bool ActionBitIsSet(kauth_action_t action, kauth_action_t mask);


Just curious, why did you take the inlines back out?

We can add them back.

I remembered while working on the code that the inline is only required on the definition and not the declaration (see: https://isocpp.org/wiki/faq/inline-functions#inline-nonmember-fns).

But it doesn't hurt anything to leave them (and makes it clearer that they are expected to be inline) so I can add them back in.

sanoursa · 2018-08-22T17:15:22Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp


-        vtype vnodeType = vnode_vtype(currentVnode);
-        if (ShouldIgnoreVnodeType(vnodeType, currentVnode))
+        // TODO(Mac): Improve error handling


And more importantly, we just lost a notification here. We will need some pattern for dealing with that.

Good point, I will add some more details to this comment

sanoursa · 2018-08-22T17:16:06Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

+        int kauthResult;
+        int kauthError;
+        if (!TrySendRequestAndWaitForResponse(
+            root,


For consistency with your other code, indent one more level here

wilbaker · 2018-08-22T18:57:57Z

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitFilesTests.cs

        [TestCase, Order(2)]
+        public void CreateHardLinkTest()
+        {
+            if (!this.fileSystem.SupportsHardlinkCreation)


This test is not getting any coverage on Mac as it's not using BashRunner

Test fixture updated to use all runners (whatever used to cause failures with BashRunner is no longer an issue)

wilbaker · 2018-08-22T18:59:10Z

GVFS/GVFS.FunctionalTests/Tests/GitCommands/AddStageTests.cs

+        [Category(Categories.Mac.M2)]
+        public void AddAndStageHardLinksTest()
+        {
+            if (!this.FileSystem.SupportsHardlinkCreation)


This is not adding any coverage is AddStageTests are only run with SystemIORunner

Updated CreateHardLink to always use BashRunner to create the hard link.

sanoursa

The updated code looks great, and I confirmed that all the functional tests pass on my machine. I didn't get a chance to do any manual testing yet, but no need to block on that.

sanoursa · 2018-08-22T23:39:09Z

GVFS/GVFS.FunctionalTests/Tests/GitCommands/AddStageTests.cs

+                return;
+            }
+
+            this.CreateHardLink("Readme.md", "ReadmeLink.md");


Nit: in the IPlatformFileSystem interface up above, I saw that the link name was first and the target name was second. I think it would help us keep these straight if we keep a consistent ordering through all of our own code.

👍 I'll clean this up

sanoursa · 2018-08-22T23:40:51Z

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs

+            // creation) so use a BashRunner instead.
+            this.FileSystem.ShouldBeOfType<SystemIORunner>();
+            this.FileSystem.SupportsHardlinkCreation.ShouldBeFalse(
+                "FileSystem *does* support hard link creation, CreateHardLink no longer needs to create a BashRunner");


This message is confusing. You're saying that if in the future it ends up supporting hardlinks, we can remove this code?

Yep that's right, let me try re-working this message.

wilbaker added 7 commits August 21, 2018 15:42

Mac: Add file renamed notification

9088c04

Update MirrorProvider for rename notification

d3e24b3

Mac: Hardlink created notification

1918f53

Update MirrorProvider for hardlink notification

690c1d4

Update Windows code to handle hardlink creation

c144821

Add hardlink creation functional tests

67eebcd

Cleanup after rebasing

635f4d8

wilbaker added domain: kext domain: user-mode type: bug labels Aug 22, 2018

wilbaker requested review from nickgra and sanoursa August 22, 2018 17:00

wilbaker changed the title ~~Users/wilbaker/hardlink and mac rename notifications~~ Hardlink notifications on Mac\Windows and rename notifications on Mac Aug 22, 2018

wilbaker changed the title ~~Hardlink notifications on Mac\Windows and rename notifications on Mac~~ Hardlink notifications on Mac & Windows and rename notifications on Mac Aug 22, 2018

sanoursa reviewed Aug 22, 2018

View reviewed changes

Register for HardLinkCreated on Windows

a5bea1e

wilbaker commented Aug 22, 2018

View reviewed changes

PR Feedback: Code cleanup and test fixes

b4fb449

sanoursa approved these changes Aug 22, 2018

View reviewed changes

More cleanup for PR comments

4e90178

wilbaker merged commit 950d6e6 into microsoft:master Aug 23, 2018

jrbriggs mentioned this pull request Oct 12, 2018

Merge milestones/M142 to releases/shipped #381

Merged

Conversation

wilbaker commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanoursa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilbaker Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanoursa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wilbaker commented Aug 22, 2018 •

edited

Loading

wilbaker Aug 22, 2018 •

edited

Loading