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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 ↗(On Diff #19256)

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 ↗(On Diff #19256)

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 ↗(On Diff #19256)

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 ↗(On Diff #19256)

Good idea

kevincox added inline comments.Jan 15 2020, 3:07 PM
rust/hg-core/src/utils/files.rs
109 ↗(On Diff #19256)

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 ↗(On Diff #19256)

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/hg_path.rs (170 lines)

Diff 19464

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

} }
let is_dir = base.as_bytes().ends_with(b"/"); let is_dir = base.as_bytes().ends_with(b"/");
if is_dir && self.starts_with(base) { if is_dir && self.starts_with(base) {
Some(HgPath::new(&self.inner[base.len()..])) Some(HgPath::new(&self.inner[base.len()..]))
} else { } else {
None None
} }
} }
#[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<'a>(&self) -> (&HgPath, &HgPath) {
let bytes = self.as_bytes();
let is_sep = |b| std::path::is_separator(b as char);
if self.len() < 2 {
(HgPath::new(b""), &self)
} else if is_sep(bytes[0])
&& is_sep(bytes[1])
&& (self.len() == 2 || !is_sep(bytes[2]))
{
// Is a UNC path:
// vvvvvvvvvvvvvvvvvvvv drive letter or UNC path
// \\machine\mountpoint\directory\etc\...
// directory ^^^^^^^^^^^^^^^
let machine_end_index = bytes[2..].iter().position(|b| is_sep(*b));
let mountpoint_start_index = if let Some(i) = machine_end_index {
i + 2
} else {
return (HgPath::new(b""), &self);
};
match bytes[mountpoint_start_index + 1..]
.iter()
.position(|b| is_sep(*b))
{
// A UNC path can't have two slashes in a row
// (after the initial two)
Some(0) => (HgPath::new(b""), &self),
Some(i) => {
let (a, b) =
bytes.split_at(mountpoint_start_index + 1 + i);
(HgPath::new(a), HgPath::new(b))
}
None => (&self, HgPath::new(b"")),
}
} else if bytes[1] == b':' {
// Drive path c:\directory
let (a, b) = bytes.split_at(2);
(HgPath::new(a), HgPath::new(b))
} else {
(HgPath::new(b""), &self)
}
}
#[cfg(unix)]
/// Split a pathname into drive and path. On Posix, drive is always empty.
pub fn split_drive(&self) -> (&HgPath, &HgPath) {
(HgPath::new(b""), &self)
}
/// Checks for errors in the path, short-circuiting at the first one. /// Checks for errors in the path, short-circuiting at the first one.
/// This generates fine-grained errors useful for debugging. /// This generates fine-grained errors useful for debugging.
/// To simply check if the path is valid during tests, use `is_valid`. /// To simply check if the path is valid during tests, use `is_valid`.
pub fn check_state(&self) -> Result<(), HgPathError> { pub fn check_state(&self) -> Result<(), HgPathError> {
if self.len() == 0 { if self.len() == 0 {
return Ok(()); return Ok(());
} }
let bytes = self.as_bytes(); let bytes = self.as_bytes();
let path = HgPath::new(b"nested/path/to/b"); let path = HgPath::new(b"nested/path/to/b");
let base = HgPath::new(b"nested/path/"); let base = HgPath::new(b"nested/path/");
assert_eq!(Some(HgPath::new(b"to/b")), path.relative_to(base)); assert_eq!(Some(HgPath::new(b"to/b")), path.relative_to(base));
let path = HgPath::new(b"ends/with/dir/"); let path = HgPath::new(b"ends/with/dir/");
let base = HgPath::new(b"ends/"); let base = HgPath::new(b"ends/");
assert_eq!(Some(HgPath::new(b"with/dir/")), path.relative_to(base)); assert_eq!(Some(HgPath::new(b"with/dir/")), path.relative_to(base));
} }
#[test]
#[cfg(unix)]
fn test_split_drive() {
// Taken from the Python stdlib's tests
assert_eq!(
HgPath::new(br"/foo/bar").split_drive(),
(HgPath::new(b""), HgPath::new(br"/foo/bar"))
);
assert_eq!(
HgPath::new(br"foo:bar").split_drive(),
(HgPath::new(b""), HgPath::new(br"foo:bar"))
);
assert_eq!(
HgPath::new(br":foo:bar").split_drive(),
(HgPath::new(b""), HgPath::new(br":foo:bar"))
);
// Also try NT paths; should not split them
assert_eq!(
HgPath::new(br"c:\foo\bar").split_drive(),
(HgPath::new(b""), HgPath::new(br"c:\foo\bar"))
);
assert_eq!(
HgPath::new(b"c:/foo/bar").split_drive(),
(HgPath::new(b""), HgPath::new(br"c:/foo/bar"))
);
assert_eq!(
HgPath::new(br"\\conky\mountpoint\foo\bar").split_drive(),
(
HgPath::new(b""),
HgPath::new(br"\\conky\mountpoint\foo\bar")
)
);
}
#[test]
#[cfg(windows)]
fn test_split_drive() {
assert_eq!(
HgPath::new(br"c:\foo\bar").split_drive(),
(HgPath::new(br"c:"), HgPath::new(br"\foo\bar"))
);
assert_eq!(
HgPath::new(b"c:/foo/bar").split_drive(),
(HgPath::new(br"c:"), HgPath::new(br"/foo/bar"))
);
assert_eq!(
HgPath::new(br"\\conky\mountpoint\foo\bar").split_drive(),
(
HgPath::new(br"\\conky\mountpoint"),
HgPath::new(br"\foo\bar")
)
);
assert_eq!(
HgPath::new(br"//conky/mountpoint/foo/bar").split_drive(),
(
HgPath::new(br"//conky/mountpoint"),
HgPath::new(br"/foo/bar")
)
);
assert_eq!(
HgPath::new(br"\\\conky\mountpoint\foo\bar").split_drive(),
(
HgPath::new(br""),
HgPath::new(br"\\\conky\mountpoint\foo\bar")
)
);
assert_eq!(
HgPath::new(br"///conky/mountpoint/foo/bar").split_drive(),
(
HgPath::new(br""),
HgPath::new(br"///conky/mountpoint/foo/bar")
)
);
assert_eq!(
HgPath::new(br"\\conky\\mountpoint\foo\bar").split_drive(),
(
HgPath::new(br""),
HgPath::new(br"\\conky\\mountpoint\foo\bar")
)
);
assert_eq!(
HgPath::new(br"//conky//mountpoint/foo/bar").split_drive(),
(
HgPath::new(br""),
HgPath::new(br"//conky//mountpoint/foo/bar")
)
);
// UNC part containing U+0130
assert_eq!(
HgPath::new(b"//conky/MOUNTPO\xc4\xb0NT/foo/bar").split_drive(),
(
HgPath::new(b"//conky/MOUNTPO\xc4\xb0NT"),
HgPath::new(br"/foo/bar")
)
);
}
} }