sync: return after checking all inputs#10742
Conversation
|
GNU testsuite comparison: |
|
Hi @ChrisDryden , would you mind taking a look at this change? Testing locally, I get the desired output: ❯ cargo run -q -- --data bad1 bad2
sync: error opening 'bad1': No such file or directory
sync: error opening 'bad2': No such file or directoryI added a test case to cover this as well. Some of the tests in the CI are failing, but are unrelated to my change. |
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
patch/sync-check-all
branch
from
abb9966 to
c0fd338
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
src/uu/sync/src/sync.rs
Outdated
| )); | ||
| } | ||
|
|
||
| let mut has_error = false; |
There was a problem hiding this comment.
instead of using this boolean, we have the helper utility set_error_code that we can use to make it more idiomatic. You can just call that function after the show error and then you dont need to have the tracking variable and if statement at the end
There was a problem hiding this comment.
Thanks for the suggestion, getting rid of the tracking variable is great. However, I think we still need the if condition for an early exit. If we just set the flag, the sync will attempt to run then exit on the first error, and we would end up with something like:
sync: error opening 'bad1': No such file or directory
sync: error opening 'bad2': No such file or directory
sync: bad1: No such file or directoryI was trying to follow the GNU output as closely as possible, and the early exit will prevent the last line from appearing.
src/uu/sync/src/sync.rs
Outdated
| )?; | ||
| ); | ||
| if let Err(err) = err { | ||
| show_error!("{}", err.to_string()); |
There was a problem hiding this comment.
You can simplify all of the map_err_context with the error code:
show!(SyncError::OpeningFile(f.clone(), e.into()));
set_exit_code(1);
There was a problem hiding this comment.
I have tried to follow your suggestions, but found myself needing to update the translation templates to also allow passing in of the error. I hope that is ok. Otherwise, the error would be the following instead:
sync: error opening 'bad1'
sync: error opening 'bad2'and would also differ from the GNU stderr.
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
Hi @ChrisDryden , |
|
GNU testsuite comparison: |
|
Hi @ChrisDryden , |
|
I need to investigate this more, this should be passing the updated sync gnu tests but it does not appear to be passing in the ci |
|
GNU testsuite comparison: |
|
It needed to be rebased but if it passes the sync gnu test I think it should be good to go |
|
GNU testsuite comparison: |
|
Thanks! You've fixed one of the gnu tests |
* sync: return after checking all inputs * fix missing error context * use set_exit_code and update translation templates to show errors * remove thiserror dependency * fix typo --------- Co-authored-by: Sylvestre Ledru <sylvestre@debian.org> Co-authored-by: Chris Dryden <christopher.paul.dryden@gmail.com>
3 participants
Closes #10735 .
Instead of returning early, shows the errors as they are encountered and sets the error flag.
If the error flag is present, return a
USimpleErrorwith an empty message to match the GNU stderr.