Fixes to parentDirectory() and abspath()

[ajbeamon]

parentDirectory() and abspath() can now optionally resolve symbolic links and/or require the given path to exist. All ".." and "." references will now be fully resolved by both functions even if the path doesn't fully exist (if the mustExist flag allows success in this case). The following inputs are now handled correctly:

• paths that are direct children of root
• paths where not all components exist but which contain .. references after the prefix that does exist
• absolute paths where no path components exist

[ajbeamon]

As stated above, I currently have this open against 5.2. I marked it with the 6.0 milestone so it will hopefully get attention while I'm out, but if we don't feel comfortable enough with it by the time we want to make the release, it could be considered for a future patch release.

[ajbeamon]

I've merged in master and switched the base branch from release-6.0 to master.

[satherton]

It's a bit unclear, and undocumented, exactly what behavior we want from parentDirectory() in context (fdbmonitor and flow/platform). Points to consider,

• Should the input path be required to exist on the filesystem already?
• Should relative input paths be supported?
• Should the empty string input (arguably a form of relative input meaning the current directory) be supported?
• Should the output always contain a trailing slash? (A useful property bit differs from realpath() behavior..)
• Is the parent of root an error or an empty result?
• Should the result have duplicate path separators removed?
• Should the result have symbolic links resolved to their absolute paths a la realpath()?
• Should the result have ../ and ./ references resolved?

For inputs that actually exist, using realpath() is a good way to get a canonical path from which a parent can be found easily as it resolves ./ and ../ references and removes trailing and duplicate separators. However, the caller may wish to preserve symbolic links in the input so it can monitor the parent directory symlink for changes (fdbmonitor does this). While parentDirectory exists in fdbmonitor independently from flow/platform, it would be nice if one function definition could satisfy all use cases, perhaps with parameters. And of course there is reason for parentDirectory to work on input strings that do not actually exist on the filesystem.

We should probably revisit abspath() behavior as well. Currently, abspath("/blah") returns "/blah" if /blah exists on the filesystem, and otherwise returns "<absolute_path_of_current_working_directory>/blah" which doesn't seem right for any use case. Also, abspath() will not remove trailing separators for paths that do not exist.

[satherton]

I've committed a significant refactoring which I think makes absPath() and parentDirectory() behave in sensible ways. They each use a new platform function called cleanPath() which canonicalizes a path without consulting the filesystem. Among the more serious bad behaviors fixed, it is no longer the case that you get a different result for abspath("/path/to") or parentDirectory("/path/to") based on whether or not "/path/to" actually exists.

The main thing to review is the unit test at the end of Platform.cpp and its output. I'm certain there are still edge cases to add. For convenience, here is the current output:

PASS: cleanPath('') -> ''
PASS: cleanPath('') -> ''
PASS: cleanPath('/') -> '/'
PASS: cleanPath('///.///') -> '/'
PASS: cleanPath('/a/b/.././../c/./././////./d/..//') -> '/c'
PASS: cleanPath('a/b/.././../c/./././////./d/..//') -> 'c'
PASS: cleanPath('..') -> '..'
PASS: cleanPath('../.././') -> '../..'
PASS: cleanPath('../a/b/..//') -> '../a'
PASS: cleanPath('a/b/.././../c/./././////./d/..//..') -> ''
PASS: cleanPath('/..') -> Platform error
PASS: cleanPath('/a/b/../.././../') -> Platform error
PASS: abspath('') -> Platform error
PASS: abspath('/') -> '/'
PASS: abspath('/dev') -> '/dev'
PASS: abspath('/dev/') -> '/dev'
PASS: abspath('/dev/..') -> '/'
PASS: abspath('/dev/../') -> '/'
PASS: abspath('/dev/../') -> '/'
PASS: abspath('/dev/..///') -> '/'
PASS: abspath('blahfoo') -> '/Users/Steve/foundation/foundationdb.clean/blahfoo'
PASS: abspath('blahfoo/bar') -> '/Users/Steve/foundation/foundationdb.clean/blahfoo/bar'
PASS: abspath('blahfoo/bar/') -> '/Users/Steve/foundation/foundationdb.clean/blahfoo/bar'
PASS: abspath('blahfoo/bar/..//') -> '/Users/Steve/foundation/foundationdb.clean/blahfoo'
PASS: abspath('blahfoo/bar/..//../') -> '/Users/Steve/foundation/foundationdb.clean'
PASS: abspath('/blahfoo') -> '/blahfoo'
PASS: abspath('/blahfoo/bar') -> '/blahfoo/bar'
PASS: abspath('/blahfoo/bar/') -> '/blahfoo/bar'
PASS: parentDirectory('') -> Platform error
PASS: parentDirectory('/') -> Platform error
PASS: parentDirectory('//') -> Platform error
PASS: parentDirectory('/a') -> '/'
PASS: parentDirectory('/a/') -> '/'
PASS: parentDirectory('/a/b') -> '/a/'
PASS: parentDirectory('/a/b/c/.././') -> '/a/'
PASS: parentDirectory('a/b/c/.././') -> '/Users/Steve/foundation/foundationdb.clean/a/'

[satherton]

cleanPath() should probably handle '/' as well regardless of what the canonical separator is. On windows, cleanPath() would not work for fully qualified network paths which begin with '\'.

[satherton]

It should probably be the case that
cleanPath(x) == cleanPath(cleanPath(x))
since it is supposed to return a canonical form, so cleanPath(".") should be "." and not "" as it is currently.

[ajbeamon]

The GitHub UI won't let me approve because I originated this PR, but it looks good to me. Feel free to merge when the build checks are finished.