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

Implement support for following symlinks #131

Closed
wants to merge 1 commit into from

Conversation

refi64
Copy link
Contributor

@refi64 refi64 commented Jan 22, 2022

Signed-off-by: Ryan Gonzalez [email protected]


Unfortunately, it wasn't possible to make work as needed without these two PRs to walkdir: BurntSushi/walkdir#159 BurntSushi/walkdir#160

However, it seems that PRs to walkdir have been stagnant since late 2020 / early 2021, so for the purpose of testing, I created a single branch on my fork with both PRs merged in, and Cargo.toml in here is pointing to that branch. I'm not quite sure how to handle this as a result, so this is in draft status. Any thoughts on how to handle this?

@codecov
Copy link

codecov bot commented Jan 22, 2022

Codecov Report

Merging #131 (9492b36) into main (1517cc0) will decrease coverage by 1.96%.
The diff coverage is 93.75%.

❗ Current head 9492b36 differs from pull request most recent head 29e5f52. Consider uploading reports for the commit 29e5f52 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   52.52%   50.56%   -1.97%     
==========================================
  Files          23       19       -4     
  Lines        4655     2589    -2066     
  Branches     1531      688     -843     
==========================================
- Hits         2445     1309    -1136     
+ Misses       1664     1039     -625     
+ Partials      546      241     -305     
Impacted Files Coverage Δ
src/find/mod.rs 62.76% <85.71%> (+1.40%) ⬆️
src/find/matchers/printf.rs 67.49% <88.88%> (-0.22%) ⬇️
tests/find_cmd_tests.rs 93.67% <100.00%> (+15.60%) ⬆️
src/lib.rs 25.70% <0.00%> (-10.17%) ⬇️
src/testing/commandline/main.rs 70.27% <0.00%> (-1.16%) ⬇️
tests/xargs_tests.rs
src/xargs/main.rs
src/xargs/mod.rs
src/find/matchers/regex.rs
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1517cc0...29e5f52. Read the comment docs.

@sylvestre
Copy link
Contributor

Clap clap:


Warning: Congrats! The bfs test test_L is now passing!
Warning: Congrats! The bfs test test_L_depth is now passing!
Warning: Congrats! The bfs test test_L_type_l is now passing!
Warning: Congrats! The bfs test test_flag_comma is now passing!
Warning: Congrats! The bfs test test_flag_weird_names is now passing!
Warning: Congrats! The bfs test test_type_l is now passing!

@refi64
Copy link
Contributor Author

refi64 commented Jan 24, 2022

Ah nice! This seems to affect quite a few tests. Any ideas for how to handle the walkdir issue, though?

@mcharsley
Copy link
Contributor

mcharsley commented Jan 24, 2022 via email

@tavianator
Copy link
Contributor

It is possible to work around BurntSushi/walkdir#160 the same way fd does it: https://github.com/sharkdp/fd/blob/9f5ed8534e824b3e2a0934ff3aa0b781cb370688/src/walk.rs#L490-L503. But it would be nice if walkdir would do it itself.

@refi64
Copy link
Contributor Author

refi64 commented Jan 31, 2022

@tavianator I was originally going to try using a similar system here, but there were 2 things that made me go with the walkdir patches instead:

  • walkdir patches are needed anyway so that find a-symlink will not traverse into it without -L. The alternative would be to manually check if it's a symlink ourselves first but...
  • ...findutils does seem to use more platform-specific DirEntry functionality that fd does, so re-implementing a custom DirEntry wrapper would be a decent bit messier, at least from my initial investigations

That being said, if the walkdir PR stays stagnant for too long, it may be worth switching over.

@sylvestre
Copy link
Contributor

no activity for a while, closing

@sylvestre sylvestre closed this Jun 24, 2024
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.

4 participants