This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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?

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"?