Skip to content
This repository was archived by the owner on Aug 31, 2021. It is now read-only.

Conversation

livecodesebastien
Copy link
Contributor

No description provided.

@livecodesebastien livecodesebastien added this to the 6.7.8-rc-1 milestone Sep 3, 2015
@runrevmark
Copy link
Contributor

I wonder if we should be providing some facility for using '' on Windows too:

If the leading char is '' then treat that the same as if it were '/'.
If the leading two chars are '' then treat that the same as if it were '//'.

Since we already will happily process C:\foobar correctly (at least I think we do, from the code); this would be useful - particularly for older stacks which do use \ on Windows rather than / (since it used to work to some degree).

If we put this in the Windows specific code, there isn't a problem. Windows can't have '' in its filenames, so if an app from another platform is using '' in a filename, it won't work on Windows anyway. i.e. Supporting '' in terms of absolute path detection on Windows can have no negative impact on any other platform; nor on Windows itself.

@livecodesebastien
Copy link
Contributor Author

That sounds like a good idea to me, I'll add this.

Also, currently MCS_resolvepath does not return an absolute path on 7.0 (but does it on 6.7 after all the changes for the bug 15432).
Should this behaviour be unified between 6.7 and 7.0??

@runrevmark
Copy link
Contributor

Well, I presume the problems present due to 15432 are also present in 7.0, in which case that would be a good thing to do :)

@livecodesebastien
Copy link
Contributor Author

Pull request updated.


if (path[0] == '/' && path[1] != '/')
if (t_use_backslashes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to map all / -> , then check for absoluteness and make absolute if necessary, then map \ to /? (I'm not clear what the use_backslashes is for)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that is simply the opposite being made here: \ first mapped to / if backslashes are in use in path, the path is made absolute if necessary, and then if a mapping occurred (that is, if t_use_backslashes is true) then / are mapped again to \.
(MCU_path2native and MCU_path2std are the same, and simply change every '' to '/' and every '/' to '')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite work though. If there is a backslash in the path you are mapping / -> \ and \ -> / -- then checking for absoluteness. However consider the following path:

//foo/bar/baz\foobar.txt

This would fail to be recognized as a absolute path as you would be testing:

\\foo\bar\baz/foobar.txt

Whilst it is possible for NTFS filenames to contain , I suspect bad things happen in the UI on Windows and elsewhere if such a thing is encountered. Given this is the case, I wonder if the best thing to do is to not do any \ <-> mapping here, and instead check for either / or \ in the appropriate places.

This will preserve the user's file path, whilst still prepending the CWD if it is a relative path.

It is then up to the FileSystem implementation functions to map appropriately - which I believe they already do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, and will make things simpler. Thanks

@runrevmark
Copy link
Contributor

@livecode-vulcan review ok 1e50685

@livecode-vulcan
Copy link
Contributor

💙 review by @runrevmark ok 1e50685

livecode-vulcan added a commit that referenced this pull request Sep 9, 2015
[[ Bug 15814 ]] UNC paths should be seen as canonical paths on Windows
// SN-2015-09-03: [[ Bug 15814 ]] UNC paths (such as //servername/path)
// should not be changed, as they are already valid.
else if ((path[0] != '/' && path[1] != '/')
|| (path[0] != '\\' && path[1] != '\\'))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always true.
@livecode-vulcan review not ok 1e50685

Check appropriately whether the path starts with // or \\
@runrevmark
Copy link
Contributor

@livecodesebastien: Good catch :)

Can I suggest we preface that if with a descriptive comment:

// The following rules are used to process paths on Windows:
// - an absolute UNIX path is mapped to an absolute windows path using the drive of the CWD:
// /foo/bar -> CWD-DRIVE:/foo/bar
// - an absolute windows path is left as is:
// //foo/bar -> //foo/bar
// C:/foo/bar -> C:/foo/bar
// - a relative path is prefixed by the CWD:
// foo/bar -> CWD/foo/bar
// Note: / and \ are treated the same, but not changed. When adding a path separator / is used.

@livecodesebastien
Copy link
Contributor Author

@runrevmark I'll add the comment, more description never hurts

@runrevmark
Copy link
Contributor

@livecode-vulcan review ok d2fcdd0

@livecode-vulcan
Copy link
Contributor

💙 review by @runrevmark ok d2fcdd0

livecode-vulcan added a commit that referenced this pull request Sep 9, 2015
[[ Bug 15814 ]] UNC paths should be seen as canonical paths on Windows
@livecode-vulcan
Copy link
Contributor

😎 test success d2fcdd0

livecodesebastien added a commit that referenced this pull request Sep 9, 2015
[[ Bug 15814 ]] UNC paths should be seen as canonical paths on Windows
@livecodesebastien livecodesebastien merged commit 8c5c90f into livecode:develop-6.7 Sep 9, 2015
@livecodesebastien livecodesebastien deleted the bugfix-15814 branch September 9, 2015 13:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants