Page MenuHomePhabricator

rust-utils: add util for canonical path
ClosedPublic

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

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:34 PM
kevincox accepted this revision.Jan 15 2020, 11:09 AM
Alphare updated this revision to Diff 19321.Jan 15 2020, 4:56 PM
Alphare updated this revision to Diff 19349.Jan 16 2020, 4:34 AM
Alphare updated this revision to Diff 19359.Jan 16 2020, 7:48 AM
Alphare updated this revision to Diff 19937.Thu, Feb 6, 10:14 AM
martinvonz requested changes to this revision.Thu, Feb 6, 12:39 PM
martinvonz added a subscriber: martinvonz.
martinvonz added inline comments.
rust/hg-core/src/utils/files.rs
208–217

Might be worth having a fast path for when name is not absolute (including "") and neither name nor cwd contains "../"so we don't do let name = root.join(&cwd).join(&name) immediately followed by let name = name.strip_prefix(&root).unwrap(). Especially since that seems like the common case. Or can that ever produce different results? Anyway, that can be done later.

221–223

Is this accurate? It looks like we break when name.parent() == None.

228–231

name.components().next() will be None only if name is something like "/", right? But then we shouldn't return "" (a repo-relative path), we should error out.

This revision now requires changes to proceed.Thu, Feb 6, 12:39 PM
Alphare added inline comments.Thu, Feb 6, 4:21 PM
rust/hg-core/src/utils/files.rs
208–217

That might be worth investigating indeed. I will defer to a later time, though. :)

221–223

I could reword this sentence if you feel that it does not convey the same meaning. It fits the Python implementation better, that's for sure.

228–231

For b'/' it would return Some(std::Path::Components::RootDir). It only returns None if there are no components.

martinvonz added inline comments.Thu, Feb 6, 4:28 PM
rust/hg-core/src/utils/files.rs
221–223

To me, the current sentence very much makes it sound like it iterates until name != name.parent(), which doesn't seem correct. So, yes, please reword it. (Unless I'm just misunderstanding something.)

228–231

Okay, but it seems that that doesn't address my concern. If I understand correctly (now that you've corrected me), we get here if name == "". However, name is an absolute path (right?), so with root = "/some/path/" and name = "", why should we return Ok()? If name is indeed an absolute path, I would interpret "" as the root (file system root, not repo root), which is not inside "/some/path". What am I missing?

martinvonz added inline comments.Thu, Feb 6, 5:20 PM
rust/hg-core/src/utils/files.rs
232

Related to the comment above, this seems to (generally) return an absolute parent directory instead of a repo-relative path. Try adding a println!() just inside the if same to make sure you have test coverage.

Alphare updated this revision to Diff 20037.Mon, Feb 10, 10:46 AM
Alphare updated this revision to Diff 20051.Mon, Feb 10, 12:50 PM
Alphare updated this revision to Diff 20057.Mon, Feb 10, 3:46 PM
Alphare marked 2 inline comments as done.Mon, Feb 10, 3:48 PM
martinvonz accepted this revision.Mon, Feb 10, 5:57 PM
martinvonz added inline comments.
rust/hg-core/src/utils/files.rs
366

Can the test method be annotated so it's only run on unix?

This revision is now accepted and ready to land.Mon, Feb 10, 5:57 PM
This revision was automatically updated to reflect the committed changes.
Alphare added inline comments.Mon, Feb 10, 6:40 PM
rust/hg-core/src/utils/files.rs
366

Yes, but I think we need to have a more general chat about this subject. A non-negligible number of hg-core's features do not work under non-UNIX systems. Our module policy system does not have the fine-grained control needed to map for a more complexe set of features (in Cargo terms). I think the more reasonable option right now is to have a compilation failure. Maybe we should have a big gated compile_error! at the top of lib.rs to inform users that there is work to be done to be cross-platform?