uutils / findutils

Rust implementation of findutils
MIT License
309 stars 38 forks source link

Add support for embedded "{}" #210

Closed int3 closed 1 year ago

int3 commented 1 year ago

Given

find . -exec foo{}bar \;

GNU find will replace {} by the filename. This commit matches that behavior.

Note that this is not required by the POSIX spec.

The GNU testsuite doesn't cover this case, so I've added a test in this repo.

I'm new to Rust, so please be gentle (:

codecov[bot] commented 1 year ago

Codecov Report

Merging #210 (30a2245) into main (a3287a9) will increase coverage by 0.16%. The diff coverage is 92.00%.

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   58.24%   58.41%   +0.16%     
==========================================
  Files          30       30              
  Lines        3504     3523      +19     
  Branches      784      786       +2     
==========================================
+ Hits         2041     2058      +17     
  Misses       1175     1175              
- Partials      288      290       +2     
Impacted Files Coverage Δ
tests/exec_unit_tests.rs 88.99% <88.23%> (-0.14%) :arrow_down:
src/find/matchers/exec.rs 87.17% <100.00%> (+0.69%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

int3 commented 1 year ago

The failing jobs don't seem to be related to my changes

int3 commented 1 year ago

Did I bork the test runner somehow? Perhaps I shouldn't have force-pushed?

sylvestre commented 1 year ago

I wonder if this isn't an issue with out CI.

sylvestre commented 1 year ago

i am surprised that it didn't fix any GNU or BFS tests. Could you please have a look? thanks

tavianator commented 1 year ago

So I can confirm that locally it fixes the common/exec_substring test. Why doesn't that show up on CI? I think it's because the action downloads the most recent bfs-result.json from a run on the main branch: https://github.com/uutils/findutils/actions/runs/4257476104/workflow#L119-L127

But if you look at the runs on the main branch: https://github.com/uutils/findutils/actions?query=branch%3Amain

you'll see it includes this PR itself since the branch is int3:main. So somehow we need to tell action-download-artifact to only look at our main branch, not other branches that might be called main.

int3 commented 1 year ago

Would you like me to make a new PR with a different branch name?

int3 commented 1 year ago

Reopened with a different branch here: https://github.com/uutils/findutils/pull/213