Home directory for non-existent user should not fall back to /var/empty or %ALLUSERSPROFILE%

[jmschonfeld]

When porting FileManager to swift-foundation, I made the mistake of changing the underlying home directory lookup behavior for non-existent users to fallback to /var/empty. I also synced this behavior to Windows where it now falls back to %ALLUSERSPROFILE% and updated unit tests to match this behavior. However, this behavior is incorrect. Falling back to /var/empty or %ALLUSERSPROFILE% is an alright choice for the current user, but for an arbitrary other user, we should instead return nil as these APIs return optional values. This PR splits the user home directory implementation into two: one for the current user that falls back to /var/empty/%ALLUSERSPROFILE% and one for a specific user that returns nil on failure.

[jmschonfeld]

@swift-ci please test

[kperryua]

Thanks!

[compnerd]

There seems to be a bit of bleeding whitespace that this change is introducing. It seems mostly like an extraction of the windows logic into a helper function which is adding a fair amount of contextual change.

[jrflat]

Should we return nil for empty user on non-Windows platforms, too?

[jrflat]

Oh, I see that Platform.homeDirectory(forUserName: "") returns nil.

[jmschonfeld]

Yeah we have some unit tests that check for this behavior that aren't platform specific so in the end I think the behavior matches, but happy to restructure if you think this should be moved out of the #if block

[kperryua]

Just noticed this. The behavior appears consistent with CFCopyHomeDirectoryURLForUser, but I'm not sure it's correct. 😓 An issue for a different day, perhaps.

[jmschonfeld]

Yeah it did seem a bit weird that CFFIXED_USER_HOME affects all users and not just the current user, but I agree probably an issue to address separately to minimize the behavioral changes of this patch (this was sort of easily overlooked before but is now very clear since the implementations for current vs. specific user are split)

[jmschonfeld]

There seems to be a bit of bleeding whitespace that this change is introducing. It seems mostly like an extraction of the windows logic into a helper function which is adding a fair amount of contextual change.

Somewhat yes. The windows logic was previously

if let user {
    // Other user specific logic
}

// Current user logic

I split this into two separate functions (one where user is non-optional and one where there is no user variable) so you're likely seeing the indentation change of the user-specific logic moving from an if let user block to just the top level of a function which more closely matches the structure of the non-Windows implementation. I can see if there's a way to break it up into two commits in case that's helpful but not sure how successful that'd be - it might be best to view this diff in the Split View rather than the unified view.