Details
- Reviewers
kevincox martinvonz - Group Reviewers
hg-reviewers - Commits
- rHG4caac36c66bc: rust-utils: add util for canonical path
rHGecf816b17825: rust-utils: add util for canonical path
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
| 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. | |
| 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. | |
| 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? | |
| 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. | |
| rust/hg-core/src/utils/files.rs | ||
|---|---|---|
| 366 | Can the test method be annotated so it's only run on unix? | |
| 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? | |
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.