This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7864

rust-utils: add Rust implementation of Python's "os.path.splitdrive"
ClosedPublic

Authored by Alphare on Jan 14 2020, 12:33 PM.

Details

Reviewers
kevincox
pulkit
Group Reviewers
hg-reviewers
Commits
rHGbaa4e7fdfd47: rust-utils: add Rust implementation of Python's "os.path.splitdrive"
rHG03aaa328b6b8: rust-utils: add Rust implementation of Python's "os.path.splitdrive"
Summary

I also wrote the NT version although I didn't mean to at first, so I thought
I would keep it, so that any further effort to get the Rust code working on
Windows is a little easier.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 14 2020, 12:33 PM
kevincox requested changes to this revision.Jan 15 2020, 9:56 AM
kevincox added inline comments.
rust/hg-core/src/utils/files.rs
109

I think this can be simplified. See https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=78136ead96596afe6305e7542a881ca4. I had to use &[u8] to avoid having to copy all of the HgPath stuff into the playground, but the code should be easy to integrate.

Notable changes:

  • Avoid creating norm_bytes vector (and allocation) by creating an is_sep function.
  • Return references to the input since we guarantee that the output will be parts of the input. The caller can always clone.
  • Use slice::split_at to make it obvious that we are returning the entire path just split.
  • Use pattern matching rather than unwrapping.
  • Use fall-through returns to make it obvious we handle every path.
230

We should add an example of a UNC and Drive path here to ensure that they don't get split.

This revision now requires changes to proceed.Jan 15 2020, 9:56 AM
Alphare added inline comments.Jan 15 2020, 2:42 PM
rust/hg-core/src/utils/files.rs
109

Indeed, this is much better. While trying to adapt this code to fit with HgPath, I find myself needing to translate to and from bytes whenever indexing or when using split_at. Should we give a HgPath a split_at method or also all the Index<> ones? I remember that we decided against that earlier.

230

Good idea

kevincox added inline comments.Jan 15 2020, 3:07 PM
rust/hg-core/src/utils/files.rs
109

I would recommend just converting to bytes at the top of the function then converting the return value to a path at the exit. I feel when you are doing manipulation like this it makes the most sense to treat it as plain bytes within the function.

Alternatively I wouldn't mind putting an index operator but have a slight preference for path.as_bytes()[n] to keep it explicit as most of the code shouldn't be reaching into paths.

Alphare added inline comments.Jan 15 2020, 3:12 PM
rust/hg-core/src/utils/files.rs
109

I agree. I'll send a follow-up for all your remarks in an hour.

Alphare updated this revision to Diff 19316.Jan 15 2020, 4:55 PM
kevincox accepted this revision.Jan 16 2020, 4:59 AM

One last note is why not just put this in HgPath?

Alphare added a comment.Jan 16 2020, 5:02 AM
In D7864#116100, @kevincox wrote:

One last note is why not just put this in HgPath?

I was about to start making this a method on HgPath, I guess we agree.

Alphare updated this revision to Diff 19350.Jan 16 2020, 5:21 AM
pulkit accepted this revision.Jan 16 2020, 10:31 AM
This revision is now accepted and ready to land.Jan 16 2020, 10:31 AM
yuja added a subscriber: yuja.Jan 21 2020, 9:41 AM

So HgPath type is no longer intended for "a repository-relative path"?

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/utils/files.rs (174 lines)

Diff 19256

rust/hg-core/src/utils/files.rs

/// TODO more than ASCII? /// TODO more than ASCII?
pub fn normalize_case(path: &HgPath) -> HgPathBuf { pub fn normalize_case(path: &HgPath) -> HgPathBuf {
#[cfg(windows)] // NTFS compares via upper() #[cfg(windows)] // NTFS compares via upper()
return path.to_ascii_uppercase(); return path.to_ascii_uppercase();
#[cfg(unix)] #[cfg(unix)]
path.to_ascii_lowercase() path.to_ascii_lowercase()
} }
#[cfg(windows)]
/// Copied from the Python stdlib's `os.path.splitdrive` implementation.
///
/// Split a pathname into drive/UNC sharepoint and relative path specifiers.
/// Returns a 2-tuple (drive_or_unc, path); either part may be empty.
///
/// If you assign
/// result = split_drive(p)
/// It is always true that:
/// result[0] + result[1] == p
///
/// If the path contained a drive letter, drive_or_unc will contain everything
/// up to and including the colon.
/// e.g. split_drive("c:/dir") returns ("c:", "/dir")
///
/// If the path contained a UNC path, the drive_or_unc will contain the host
/// name and share up to but not including the fourth directory separator
/// character.
/// e.g. split_drive("//host/computer/dir") returns ("//host/computer", "/dir")
///
/// Paths cannot contain both a drive letter and a UNC path.
pub fn split_drive(path: impl AsRef<HgPath>) -> (HgPathBuf, HgPathBuf) {
kevincoxUnsubmitted
Not Done

I think this can be simplified. See https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=78136ead96596afe6305e7542a881ca4. I had to use &[u8] to avoid having to copy all of the HgPath stuff into the playground, but the code should be easy to integrate.

Notable changes:

  • Avoid creating norm_bytes vector (and allocation) by creating an is_sep function.
  • Return references to the input since we guarantee that the output will be parts of the input. The caller can always clone.
  • Use slice::split_at to make it obvious that we are returning the entire path just split.
  • Use pattern matching rather than unwrapping.
  • Use fall-through returns to make it obvious we handle every path.
kevincox: I think this can be simplified. See https://play.rust-lang.org/?
AlphareAuthorUnsubmitted
Done

Indeed, this is much better. While trying to adapt this code to fit with HgPath, I find myself needing to translate to and from bytes whenever indexing or when using split_at. Should we give a HgPath a split_at method or also all the Index<> ones? I remember that we decided against that earlier.

Alphare: Indeed, this is much better. While trying to adapt this code to fit with `HgPath`, I find…
kevincoxUnsubmitted
Not Done

I would recommend just converting to bytes at the top of the function then converting the return value to a path at the exit. I feel when you are doing manipulation like this it makes the most sense to treat it as plain bytes within the function.

Alternatively I wouldn't mind putting an index operator but have a slight preference for path.as_bytes()[n] to keep it explicit as most of the code shouldn't be reaching into paths.

kevincox: I would recommend just converting to bytes at the top of the function then converting the…
AlphareAuthorUnsubmitted
Done

I agree. I'll send a follow-up for all your remarks in an hour.

Alphare: I agree. I'll send a follow-up for all your remarks in an hour.
let path = path.as_ref();
let sep = std::path::MAIN_SEPARATOR as u8;
let bytes = path.as_bytes();
let norm_bytes: Vec<_> = path
.as_bytes()
.iter()
.map(|c| if *c == b'\\' { sep } else { *c })
.collect();
if norm_bytes.len() > 1 {
if norm_bytes[0] == sep
&& norm_bytes[1] == sep
&& (norm_bytes.len() < 3 || norm_bytes[2] != sep)
{
// Is a UNC path:
// vvvvvvvvvvvvvvvvvvvv drive letter or UNC path
// \\machine\mountpoint\directory\etc\...
// directory ^^^^^^^^^^^^^^^
let index = norm_bytes[2..].iter().position(|b| *b == sep);
if index.is_none() {
return (HgPathBuf::new(), path.to_owned());
}
let index = index.unwrap() + 2;
let index2 =
norm_bytes[index + 1..].iter().position(|b| *b == sep);
// A UNC path can't have two slashes in a row
// (after the initial two)
if index2 == Some(0) {
return (HgPathBuf::new(), path.to_owned());
}
let index2 = match index2 {
Some(i) => i + index + 1,
None => norm_bytes.len(),
};
return (
HgPathBuf::from_bytes(&bytes[..index2]),
HgPathBuf::from_bytes(&bytes[index2..]),
);
}
if norm_bytes[1] == b':' {
return (
HgPathBuf::from_bytes(&bytes[..2]),
HgPathBuf::from_bytes(&bytes[2..]),
);
}
}
(HgPathBuf::new(), path.to_owned())
}
#[cfg(unix)]
/// Split a pathname into drive and path. On Posix, drive is always empty.
pub fn split_drive(path: impl AsRef<HgPath>) -> (HgPathBuf, HgPathBuf) {
(HgPathBuf::new(), path.as_ref().into())
}
#[derive(Eq, PartialEq, Ord, PartialOrd, Copy, Clone)] #[derive(Eq, PartialEq, Ord, PartialOrd, Copy, Clone)]
pub struct HgMetadata { pub struct HgMetadata {
pub st_dev: u64, pub st_dev: u64,
pub st_mode: u32, pub st_mode: u32,
pub st_nlink: u64, pub st_nlink: u64,
pub st_size: u64, pub st_size: u64,
pub st_mtime: i64, pub st_mtime: i64,
pub st_ctime: i64, pub st_ctime: i64,
#[test] #[test]
fn find_dirs_empty() { fn find_dirs_empty() {
// looks weird, but mercurial.pathutil.finddirs(b"") yields b"" // looks weird, but mercurial.pathutil.finddirs(b"") yields b""
let mut dirs = super::find_dirs(HgPath::new(b"")); let mut dirs = super::find_dirs(HgPath::new(b""));
assert_eq!(dirs.next(), Some(HgPath::new(b""))); assert_eq!(dirs.next(), Some(HgPath::new(b"")));
assert_eq!(dirs.next(), None); assert_eq!(dirs.next(), None);
assert_eq!(dirs.next(), None); assert_eq!(dirs.next(), None);
} }
#[test]
#[cfg(unix)]
fn test_split_drive() {
// Taken from the Python stdlib's tests
assert_eq!(
split_drive(HgPath::new(br"/foo/bar")),
(HgPathBuf::new(), HgPathBuf::from_bytes(br"/foo/bar"))
);
assert_eq!(
split_drive(HgPath::new(br"foo:bar")),
(HgPathBuf::new(), HgPathBuf::from_bytes(br"foo:bar"))
);
assert_eq!(
split_drive(HgPath::new(br":foo:bar")),
(HgPathBuf::new(), HgPathBuf::from_bytes(br":foo:bar"))
);
}
kevincoxUnsubmitted
Not Done

We should add an example of a UNC and Drive path here to ensure that they don't get split.

kevincox: We should add an example of a UNC and Drive path here to ensure that they don't get split.
AlphareAuthorUnsubmitted
Done

Good idea

Alphare: Good idea
#[test]
#[cfg(windows)]
fn test_split_drive() {
assert_eq!(
split_drive(HgPathBuf::from_bytes(br"c:\foo\bar")),
(
HgPathBuf::from_bytes(br"c:"),
HgPathBuf::from_bytes(br"\foo\bar")
)
);
assert_eq!(
split_drive(HgPathBuf::from_bytes(b"c:/foo/bar")),
(
HgPathBuf::from_bytes(br"c:"),
HgPathBuf::from_bytes(br"/foo/bar")
)
);
assert_eq!(
split_drive(HgPathBuf::from_bytes(br"\\conky\mountpoint\foo\bar")),
(
HgPathBuf::from_bytes(br"\\conky\mountpoint"),
HgPathBuf::from_bytes(br"\foo\bar")
)
);
assert_eq!(
split_drive(HgPathBuf::from_bytes(br"//conky/mountpoint/foo/bar")),
(
HgPathBuf::from_bytes(br"//conky/mountpoint"),
HgPathBuf::from_bytes(br"/foo/bar")
)
);
assert_eq!(
split_drive(HgPathBuf::from_bytes(
br"\\\conky\mountpoint\foo\bar"
)),
(
HgPathBuf::from_bytes(br""),
HgPathBuf::from_bytes(br"\\\conky\mountpoint\foo\bar")
)
);
assert_eq!(
split_drive(HgPathBuf::from_bytes(
br"///conky/mountpoint/foo/bar"
)),
(
HgPathBuf::from_bytes(br""),
HgPathBuf::from_bytes(br"///conky/mountpoint/foo/bar")
)
);
assert_eq!(
split_drive(HgPathBuf::from_bytes(
br"\\conky\\mountpoint\foo\bar"
)),
(
HgPathBuf::from_bytes(br""),
HgPathBuf::from_bytes(br"\\conky\\mountpoint\foo\bar")
)
);
assert_eq!(
split_drive(HgPathBuf::from_bytes(
br"//conky//mountpoint/foo/bar"
)),
(
HgPathBuf::from_bytes(br""),
HgPathBuf::from_bytes(br"//conky//mountpoint/foo/bar")
)
);
// UNC part containing U+0130
assert_eq!(
split_drive(HgPathBuf::from_bytes(
b"//conky/MOUNTPO\xc4\xb0NT/foo/bar"
)),
(
HgPathBuf::from_bytes(b"//conky/MOUNTPO\xc4\xb0NT"),
HgPathBuf::from_bytes(br"/foo/bar")
)
);
}
} }