NotificationMapping constructor should accept "." when asking for notifications over the whole repo

[SteveBenz]

The comment for NotificationMapping.NotificationMapping.notificationRoot says "The path to the notification root, relative to the virtualization root." But actually it's not always a relative path; if you want notifications over the whole tree, "" is actually what's asked for. That's described in the remarks section of NotificationMapping. For good or bad, nobody's going to read the . Intellisense is how documentation is served in almost all cases, so the comment in the is only going to be read by a dedicated few.

I think the best way to fix this is to allow "." as the value; preserver "" as a valid value for backwards compatibility (and maybe it makes sense to some people, I don't know). If you're really committed to having "" be the way, then add something to the tag on NotificationMapping.notificationRoot to that effect.

This is quite a serious land-mine, because the net result of using "." instead of "" is you silently get no notifications and no error either.

[cgallred]

For various reasons (mainly to do with us having originally approached ProjFS from the mindset that a provider is an extension of the file system) the underlying native API expects "" to mean the virtualization root. I can add code to the managed wrapper that converts . to "" for consumption by the native API.

I'll also file a work item to add support for . to the native API to make things simpler for native developers in the future.

[wilbaker]

For various reasons (mainly to do with us having originally approached ProjFS from the mindset that a provider is an extension of the file system) the underlying native API expects "" to mean the virtualization root. I can add code to the managed wrapper that converts . to "" for consumption by the native API.

FWIW I think it would be OK to say that the managed API will match the native API here not convert . to "".

Historically the goal has been to keep the managed layer thin and consistent with the managed API, and I'm not sure it's good to have them start diverging when it comes to core behavior/functionality.

[cgallred]

@wilbaker I'm willing to make this change since I plan to also allow the native API to accept .. It will mean that for a while the native and managed APIs behave slightly differently, but I don't think it is enough of a difference to be a big deal.

Besides, if people coming from a non file-system background assume . is valid and that causes them to, without warning, not get any notifications, that is problematic.

[cgallred]

@SteveBenz, @wilbaker I've had a while to think about this and I just don't agree. Specifying . is an error. The only case where . makes sense to specify a relative path is when you're using a command shell to specify a path relative to the current directory. "Current directory" isn't a file system concept, so it doesn't belong in ProjFS.

I'm going to change this fix to an exception. The VirtualizationInstance constructor already throws exceptions, so adding a new one isn't a big deal.

[SteveBenz]

I'm happy with throwing an exception if the user passes "." or even ".\path". Much better than silently getting no notifications, and I see that you've updated the parameter's documentation to clearly call it out.

But the fix for #26 doesn't look right to me. I could have a folder called ".git" and want to specifically see activity within that folder but not other folders. Looks to me like if I pass ".git" as a folder name, it'll fail and there'll be no way to specifically monitor that.

[cgallred]

26 now throws an exception only for paths that are exactly . or that start with .\.

[cgallred]

@SteveBenz @wilbaker The nuget feed has been updated with these changes.