Rename `std::path` to `std::old_path`; introduce new `std::path` #21759

aturon · 2015-01-29T22:39:27Z

This PR implements path reform, and motivation and details for the change can be found there.

For convenience, the old path API is being kept as old_path for the time being. Updating after this PR is just a matter of changing imports to old_path (which is likely not needed, since the prelude entries still export the old path API).

This initial PR does not include additional normalization or platform-specific path extensions. These will be done in follow up commits or PRs.

[breaking-change]

Closes #20034
Closes #12056
Closes #11594
Closes #14028
Closes #14049
Closes #10035

aturon · 2015-01-29T22:39:35Z

r? @alexcrichton

rust-highfive · 2015-01-29T22:39:42Z

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

retep998 · 2015-01-29T23:00:49Z

src/libstd/path.rs

+    ///
+    /// * if `path` has a root but no prefix (e.g. `\windows`), it
+    ///   replaces everything except for the prefix (if any) of `self`.
+    /// * if `path` has a prefix but no root, it replaces `self.


If we have a path C:foo.txt this is a relative path, not an absolute path, so it doesn't make sense to replace self outright.

Consider pushing C:foo.txt onto D:\temp. What would you want to happen?

The existing path API appends the path only for drive prefixes where the volumes match. I wrote some code to do the same initially, but ultimately felt that it was inconsistent behavior, rather than always treating pushing C:foo.txt as a drive-CWD-relative path. But I could be convinced otherwise! The tradeoffs aren't completely clear to me.

Update: based on an IRC chat with @retep998, it's unclear that drive-CWD is available in winapi (though it is within cmd). Also, PathCchCombineEx (from https://msdn.microsoft.com/en-us/library/windows/desktop/hh707086%28v=vs.85%29.aspx) behaves identically to the API proposed here.

I'm somewhat uncomfortable with pushing C:foo.txt onto D:\temp resulting in C:\temp\foo.txt, but I really don't know in what contexts this situation is likely to come up (and what you'd want it to do for those).

As it turns out, Windows does have drive specific current directories. Unfortunately I cannot find a way to directly get the CWD of a specific drive. Also the CWD is a very racy thing that is shared across threads and may change values on you between calls.
Since methods in winapi treat C:foo.txt as an absolute path when joining, we should replicate that behavior.
Any method that actually interacts with the filesystem and is given a C:foo.txt will treat it as a path relative to the CWD on that drive. Therefore when normalizing the path based on the filesystem we should probably use GetFullPathNameW to get the absolute path. The only other method I can think of involves changing the CWD to the drive of that path, and then getting the CWD, which is undesirable because it changes the CWD for other threads.

alexcrichton · 2015-01-30T06:42:03Z

src/libstd/path.rs

+        }
+    }
+
+    #[derive(Copy, Clone, PartialEq)]


Not sure if I'll see something down below, but are paths like C:\foo and c:\foo equivalent? If so, does this derived implementation handle that?

Ah it looks like PartialEq is on Component below, but also not case-insensitive, just confirming that is desired.

Unfortunately case sensitivity is very hard for us to do right, because the case sensitivity tables are stored in the partition itself. This means that we cannot determine whether two paths are equal if they differ in case unless we ask the filesystem to resolve the two paths for us.
However, we can at least assume that the drive letter is always case insensitive so C:\foo and c:\foo should always be considered equal.

Ah yeah I would only consider the drive letter to be considered for case insensitivity, not the actual components of the path itself. Thanks for clarifying!

alexcrichton · 2015-01-30T07:13:08Z

src/libstd/path.rs

+    ///
+    /// Does nothing, returnning `None` if the path consists of just a prefix
+    /// and/or root directory reference.
+    pub fn parent(&self) -> Option<&Path> {


Does this mean that "a/b".parent() == "a"?

(may be nice to have some quick examples)

alexcrichton · 2015-01-30T07:36:43Z

Ok, I left a bunch of comments all over the place. I think it may be good to get some internal usage of this new module as well just to run it through the ringer and be sure we get stuff like basic traits, etc. Perhaps something like compiletest could be moved over to Path with an internal helper to convert to old_path? It may not be worth it to do at this stage though and might be best to just land quickly.

alexcrichton · 2015-02-03T03:55:54Z

src/libstd/path.rs

+
+    /// Allocate a `PathBuf` with initial contents given by the
+    /// argument.
+    pub fn new<S: ?Sized + AsOsStr>(s: &S) -> PathBuf {


From our discussion the other day, this would be an interesting point to take IntoOsString vs AsOsString (a possible future modification).

alexcrichton · 2015-02-03T04:01:56Z

I think I got through all the implementation meat last time, so this time I just skimmed looking at the API, and it all looks great to me!

r=me when tests are passing and tidy is satisfied.

As part of [RFC 474](rust-lang/rfcs#474), this commit renames `std::path` to `std::old_path`, leaving the existing path API in place to ease migration to the new one. Updating should be as simple as adjusting imports, and the prelude still maps to the old path APIs for now. [breaking-change]

Implements [RFC 474](rust-lang/rfcs#474); see that RFC for details/motivation for this change. This initial commit does not include additional normalization or platform-specific path extensions. These will be done in follow up commits or PRs.

aturon · 2015-02-03T22:53:05Z

@bors: r=alexcrichton 45ddf50 p=1

bors · 2015-02-03T23:32:44Z

⌛ Testing commit 45ddf50 with merge 449cb73...

bors · 2015-02-04T00:38:19Z

💔 Test failed - auto-win-32-nopt-t

This PR implements [path reform](rust-lang/rfcs#474), and motivation and details for the change can be found there. For convenience, the old path API is being kept as `old_path` for the time being. Updating after this PR is just a matter of changing imports to `old_path` (which is likely not needed, since the prelude entries still export the old path API). This initial PR does not include additional normalization or platform-specific path extensions. These will be done in follow up commits or PRs. [breaking-change] Closes rust-lang#20034 Closes rust-lang#12056 Closes rust-lang#11594 Closes rust-lang#14028 Closes rust-lang#14049 Closes rust-lang#10035

blaenk · 2015-02-04T17:20:42Z

👏

rust-highfive assigned huonw Jan 29, 2015

rust-highfive assigned alexcrichton and unassigned huonw Jan 29, 2015

retep998 reviewed Jan 29, 2015
View reviewed changes

aturon force-pushed the new-path branch from 43681cc to 2a865c1 Compare January 30, 2015 06:41

alexcrichton reviewed Jan 30, 2015
View reviewed changes

aturon mentioned this pull request Jan 30, 2015

Stabilization for 1.0-alpha2 #20761

Closed

38 tasks

aturon force-pushed the new-path branch from 2a865c1 to 0fbe8ba Compare February 3, 2015 02:01

alexcrichton reviewed Feb 3, 2015
View reviewed changes

aturon force-pushed the new-path branch from 0fbe8ba to cf8e13f Compare February 3, 2015 22:28

aturon force-pushed the new-path branch 2 times, most recently from e4aed2b to 1e2e30a Compare February 3, 2015 22:44

Add new path module

45ddf50

Implements [RFC 474](rust-lang/rfcs#474); see that RFC for details/motivation for this change. This initial commit does not include additional normalization or platform-specific path extensions. These will be done in follow up commits or PRs.

aturon force-pushed the new-path branch from 1e2e30a to 45ddf50 Compare February 3, 2015 22:52

bors merged commit 45ddf50 into rust-lang:master Feb 4, 2015

daboross mentioned this pull request Feb 5, 2015

Update for std::path reform in rustc version ba2f13ef0 hannobraun/inotify-rs#13

Merged

aturon mentioned this pull request Feb 18, 2015

Stabilization for 1.0-beta #22500

Closed

91 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `std::path` to `std::old_path`; introduce new `std::path` #21759

Rename `std::path` to `std::old_path`; introduce new `std::path` #21759

aturon commented Jan 29, 2015

aturon commented Jan 29, 2015

rust-highfive commented Jan 29, 2015

retep998 Jan 29, 2015

aturon Jan 29, 2015

aturon Jan 29, 2015

retep998 Jan 30, 2015

alexcrichton Jan 30, 2015

alexcrichton Jan 30, 2015

retep998 Jan 30, 2015

alexcrichton Jan 30, 2015

alexcrichton Jan 30, 2015

alexcrichton commented Jan 30, 2015

alexcrichton Feb 3, 2015

alexcrichton commented Feb 3, 2015

aturon commented Feb 3, 2015

bors commented Feb 3, 2015

bors commented Feb 4, 2015

blaenk commented Feb 4, 2015

Rename std::path to std::old_path; introduce new std::path #21759

Rename std::path to std::old_path; introduce new std::path #21759

Conversation

aturon commented Jan 29, 2015

aturon commented Jan 29, 2015

rust-highfive commented Jan 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jan 30, 2015

Choose a reason for hiding this comment

alexcrichton commented Feb 3, 2015

aturon commented Feb 3, 2015

bors commented Feb 3, 2015

bors commented Feb 4, 2015

blaenk commented Feb 4, 2015

Rename `std::path` to `std::old_path`; introduce new `std::path` #21759

Rename `std::path` to `std::old_path`; introduce new `std::path` #21759