Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

find: Implement -samefile #389

Merged
merged 17 commits into from
Jun 27, 2024
Merged

find: Implement -samefile #389

merged 17 commits into from
Jun 27, 2024

Conversation

hanbings
Copy link
Collaborator

@hanbings hanbings commented May 24, 2024

implement: #373

BFS test +1 PASS and GNU test PASS +8 / FAIL -20 in commit bc8ae98

@hanbings hanbings marked this pull request as ready for review May 24, 2024 16:43
Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.43%. Comparing base (42075f5) to head (a523add).
Report is 4 commits behind head on main.

Files Patch % Lines
src/find/matchers/mod.rs 60.00% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   64.96%   65.43%   +0.47%     
==========================================
  Files          32       33       +1     
  Lines        3810     3888      +78     
  Branches      874      892      +18     
==========================================
+ Hits         2475     2544      +69     
  Misses        993      993              
- Partials      342      351       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


#[test]
#[serial(working_dir)]
#[cfg(target_os = "linux")]
Copy link
Contributor

Choose a reason for hiding this comment

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

why linux only ? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The -samefile option is primarily used in Linux/Unix systems to determine whether two files are the same file based on their inode (hard link) information.

The ino() method of the MetadataExt can be utilized to obtain the inode.
However, the equivalent file_index() method in std::os::windows::fs::MetadataExt (Rust Docs) on the Windows platform is still considered experimental, so I would recommend exercising caution when using it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So actually I haven't implemented it on Windows platform yet (has left FIXME mark).

@cakebaker cakebaker linked an issue May 25, 2024 that may be closed by this pull request

#[test]
#[serial(working_dir)]
#[cfg(target_os = "linux")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(target_os = "linux")]
#[cfg(unix)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Modified in latest commit. This target is more suitable than linux. : )

#[cfg(unix)]
fn matches(&self, file_info: &walkdir::DirEntry, _matcher_io: &mut super::MatcherIO) -> bool {
let meta = file_info.metadata().unwrap();
meta.ino() == self.metadata.ino()
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to check meta.dev(). Otherwise you'll return true for two unrelated files on different filesystems that happen to get the same inode number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I've added a check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we have https://github.com/uutils/coreutils/blob/8cac375dddb4d4e87519c137c45fa831ac7f2619/src/uucore/src/lib/features/fs.rs#L559 it would not be a good candidate ?

Thanks! This has now been changed to uucore::fs::paths_refer_to_same_file in f612c0d and it works great!

@sylvestre
Copy link
Contributor

And I waiting for #400 to review this first :)

Copy link

GNU testsuite comparison:

Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O2 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O3 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O2 is now passing!
Run GNU findutils tests: GNU tests summary = TOTAL: 706 / PASS: 441 / FAIL: 262 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +8 / FAIL -20 / ERROR +0 / SKIP +0 
Run BFS tests: Changes from main: PASS +0 / SKIP +0 / FAIL +0
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 182 / SKIP: 1 / FAIL: 105

@sylvestre
Copy link
Contributor

@hanbings is that expected ? :)
PASS +8 / FAIL -20 / ERROR +0 / SKIP +0

@hanbings
Copy link
Collaborator Author

If we mean the GNU/BFS tests for the code in this PR itself, yes.

Copy link

GNU testsuite comparison:

Run BFS tests: Changes from main: PASS +0 / SKIP +0 / FAIL +0
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 182 / SKIP: 1 / FAIL: 105
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O2 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O3 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O2 is now passing!
Run GNU findutils tests: GNU tests summary = TOTAL: 706 / PASS: 441 / FAIL: 262 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +8 / FAIL -20 / ERROR +0 / SKIP +0 


#[cfg(not(unix))]
fn matches(&self, _file_info: &walkdir::DirEntry, _matcher_io: &mut super::MatcherIO) -> bool {
// FIXME
Copy link
Contributor

Choose a reason for hiding this comment

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

please create an issue for this once it landed

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also, it supports windows? https://github.com/uutils/coreutils/blob/92665144c9ddf100d5044dc0c52af122a94587d0/src/uu/split/src/platform/windows.rs#L47

Oh! That's right! Changed to support multiple platforms. Thanks!

Copy link

GNU testsuite comparison:

Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O2 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O3 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O2 is now passing!
Run GNU findutils tests: GNU tests summary = TOTAL: 706 / PASS: 441 / FAIL: 262 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +8 / FAIL -20 / ERROR +0 / SKIP +0 
Run BFS tests: Changes from main: PASS +0 / SKIP +0 / FAIL +0
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 184 / SKIP: 1 / FAIL: 103

Copy link

GNU testsuite comparison:

Run BFS tests: Changes from main: PASS +0 / SKIP +0 / FAIL +0
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 184 / SKIP: 1 / FAIL: 103
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O2 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-copy.new-O3 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O0 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O1 is now passing!
Run GNU findutils tests: Congrats! The GNU test samefile-link.new-O2 is now passing!
Run GNU findutils tests: GNU tests summary = TOTAL: 706 / PASS: 441 / FAIL: 262 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +8 / FAIL -20 / ERROR +0 / SKIP +0 

@sylvestre sylvestre merged commit fa4cce8 into uutils:main Jun 27, 2024
19 checks passed
@hanbings hanbings deleted the implement-373 branch July 4, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement -samefile
3 participants