Skip to content

Conversation

@aawsome
Copy link
Member

@aawsome aawsome commented Dec 24, 2024

Use typed_path to use a common unix path representation on both windows/unix. This will increase windows compatibility and makes testing easier.

PR in rustic to integrate this change: rustic-rs/rustic#1466

@aawsome aawsome force-pushed the typed_path branch 2 times, most recently from dbc9dab to 32ae757 Compare December 29, 2024 14:35
@aawsome aawsome changed the title WIP: use same paths representation for unix/windows WIP: refactor!: use same paths representation for unix/windows Dec 29, 2024
@aawsome aawsome force-pushed the typed_path branch 2 times, most recently from a9d526f to af6ae1c Compare January 12, 2025 08:20
@codecov
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

❌ Patch coverage is 52.78689% with 288 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.7%. Comparing base (64d052d) to head (5db575d).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/chunker/rabin.rs 59.6% 42 Missing ⚠️
crates/core/src/commands/check.rs 46.8% 42 Missing ⚠️
crates/core/src/repofile/snapshotfile.rs 67.3% 33 Missing ⚠️
crates/core/src/backend/local_destination.rs 0.0% 26 Missing ⚠️
crates/core/src/commands/backup.rs 27.7% 26 Missing ⚠️
crates/core/src/util.rs 36.8% 24 Missing ⚠️
crates/core/src/blob/tree.rs 44.4% 15 Missing ⚠️
crates/core/src/commands/restore.rs 0.0% 14 Missing ⚠️
crates/core/src/repofile/configfile.rs 25.0% 12 Missing ⚠️
crates/core/src/vfs.rs 42.8% 12 Missing ⚠️
... and 13 more
Additional details and impacted files
Files with missing lines Coverage Δ
crates/core/src/archiver.rs 64.0% <100.0%> (+4.2%) ⬆️
crates/core/src/archiver/parent.rs 64.7% <100.0%> (-1.5%) ⬇️
crates/core/src/archiver/tree_archiver.rs 71.0% <100.0%> (+1.3%) ⬆️
crates/core/src/backend.rs 62.9% <100.0%> (+9.6%) ⬆️
crates/core/src/backend/childstdout.rs 66.6% <100.0%> (-6.7%) ⬇️
crates/core/src/commands/init.rs 93.7% <ø> (ø)
crates/core/src/repository/command_input.rs 64.8% <100.0%> (+2.8%) ⬆️
crates/core/tests/integration.rs 82.7% <100.0%> (-1.9%) ⬇️
crates/core/src/archiver/tree.rs 67.7% <93.3%> (+10.1%) ⬆️
crates/core/src/backend/ignore.rs 40.9% <88.8%> (-0.8%) ⬇️
... and 21 more

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aawsome aawsome force-pushed the typed_path branch 3 times, most recently from 6731779 to 626d588 Compare January 13, 2025 19:34
@aawsome aawsome changed the title WIP: refactor!: use same paths representation for unix/windows refactor!: use same paths representation for unix/windows Jan 26, 2025
@aawsome aawsome requested a review from simonsan January 26, 2025 09:40
@aawsome aawsome added the S-waiting-for-review Status: PRs waiting for review label Apr 24, 2025
@aawsome
Copy link
Member Author

aawsome commented May 23, 2025

I manually rebased to newest main.
@simonsan Can you please have a look at this PR? I think that a test on Windows is very much necessary here, but I don't own a windows system...

Copy link
Contributor

@simonsan simonsan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 2/42 files in but have a question

};
let node = Node::new_node(&p, NodeType::Dir, meta);
return Some(TreeType::NewTree((self.path.clone(), node, p)));
if let Some(p) = missing_dirs.components().next() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the for loop?

Both typed_path::UnixPathBuf::components and std::path::PathBuf::components seem "normalize" the same way their output (methods have identical documentation) which makes me wonder why it's ok to remove the loop.

PS: after seeing the now deleted comp_to_osstr, I am starting to think, that the typed_path::UnixPathBuf::components output might be "much" simpler than std.., is it related to that?

#[case(r#"d:\"#, "/d")]
#[case(r#"a\b\"#, "a/b")]
#[case(r#"a\b\c"#, "a/b/c")]
fn test_windows_path_to_unix_path(#[case] windows_path: &str, #[case] unix_path: &str) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe more testing could be good for these methods, for instance: are conversion methods reversible? foo == windows_to_unix(unix_to_windows(foo))?

Copy link
Contributor

@nardoor nardoor Jul 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you thing about renaming this file, could be something like path_util.
I propose that because there are only path related utils methods.

Copy link
Contributor

@nardoor nardoor Jul 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, all these conversions could be error prone. I don't feel confident in my review for this part.

@aawsome
Copy link
Member Author

aawsome commented Sep 20, 2025

waiting for BurntSushi/ripgrep@79f5a5a to be released in globset.

@aawsome aawsome force-pushed the typed_path branch 2 times, most recently from b28ace2 to 847378f Compare October 22, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-for-review Status: PRs waiting for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants