Make Windows.combine take account of partial-relative paths

[ndmitchell]

Migrated from https://ghc.haskell.org/trac/ghc/ticket/8752:

@joeyh said:

"Combine two paths, if the second path isAbsolute, then it returns the second."

But, the implementation of combine checks if the first character of a path is a path separator, which on Windows is not the same as checking if isAbsolute.

This can have counterintuitive results. For example:

import System.FilePath.Windows

prop_windows_is_sane :: Bool
prop_windows_is_sane = isAbsolute p || ("C:\\STUFF" </> p /= p)
  where p = "\\foo\\bar"

@thomie said:

Are you proposing a change to the behavior of combine, or can we just change its docstring to:

"Combine two paths, if the second path 'hasDrive' or starts with a path separator, then it returns the second."

@joeyh said:

Changing the doc string in only System.FilePath.Windows.combine would be confusing because System.FilePath.Posix.combine has the same doc string. And I assume most users just use System.FilePath and assume it works for any OS, so probably won't notice if the windows version has a caveat added.

Would changing the implementation to really check isAbsolute be likely to break things?

@thomie said:

Can you explain why the docstring I posed is confusing? It is valid for both Posix as Windows, so it's not really a caveat.

Correct me if I'm wrong, but the way I understand it is that on ​Windows, if a filepath starts with a single slash, it is considered to be relative to the root of the current drive. The system keeps track of the current drive along with the current directory of that drive.

Currently, if the second argument starts with a slash, combine returns it as is. The implementation is essentially:

combine :: FilePath -> FilePath -> FilePath 
combine a b | hasDrive b || hasLeadingPathSeparator b = b
            | otherwise = combineAlways a b

I am guessing you are looking for the following behavior:

combine "C:\\" "\\foo\\bar" == "C:\\foo\\bar"
combine "C:\\STUFF" "\\foo\\bar" == "C:\\foo\\bar"
combine "\\\\shared" "\\foo\\bar" == "\\\\shared\\foo\\bar"
combine "\\\\shared\\stuff" "\\foo\\bar" == "\\\\shared\\foo\\bar"

But, since anything else doesn't make any sense:

combine "home" "\\bob" == "\\bob"

So just changing the check to isAbsolute (or hasDrive, since combining with "C:foo", which isRelative, is not implemented at the moment, but that is another issue) doesn't work.

An implementation that has this behavior would be:

combine :: FilePath -> FilePath -> FilePath 
combine a b | hasDrive b || hasLeadingPathSeparator b && not (hasDrive a) = b
            | hasLeadingPathSeparator b = combineAlways (takeDrive a) (dropLeadingPathSeparator b)
            | otherwise = combineAlways a b

Or, an option with the same behavior as above, except:

combine "C:\\STUFF" "\\foo\\bar" == "\\foo\\bar"
combine ""\\\\shared\\stuff" "\\foo\\bar" == "\\foo\\bar"

, would be:

combine :: FilePath -> FilePath -> FilePath 
combine a b | hasDrive b || hasLeadingPathSeparator b && not (isDrive a) = b
            | hasLeadingPathSeparator b = combineAlways a (dropLeadingPathSeparator b)
            | otherwise = combineAlways a b

I'm not sure which of the three is better. We have to change the docstring either way.

[ndmitchell]

This is now documented. I'm not against making "C:" </> "/foo" return "C:/foo", any thoughts @thomie?

[thomie]

Not strictly related to this issue, but looks ok to me.

[ndmitchell]

I've updated the title. I think the changes that are required are if the LHS is partially-relative, then some of the details from the left should be used in conjunction with the right. Probably wants to wait for rewriting to an ADT as per #12.

[ndmitchell]

To be done after #53.