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

mv: check if --target is a directory #4794

Merged
merged 4 commits into from
Apr 29, 2023
Merged

Conversation

shinhs0506
Copy link
Contributor

should fix mv/diag.sh

@sylvestre
Copy link
Contributor

Still fails it seems:

2023-04-26T19:10:55.8539986Z FAIL: tests/mv/diag
2023-04-26T19:10:55.8540067Z ===================
2023-04-26T19:10:55.8540074Z 
2023-04-26T19:10:55.8540249Z --- exp	2023-04-26 19:00:50.588473849 +0000
2023-04-26T19:10:55.8540423Z +++ out	2023-04-26 19:00:50.588473849 +0000
2023-04-26T19:10:55.8540548Z @@ -1,6 +1,17 @@
2023-04-26T19:10:55.8540690Z -mv: missing file operand
2023-04-26T19:10:55.8541018Z -Try 'mv --help' for more information.
2023-04-26T19:10:55.8541226Z -mv: missing destination file operand after 'no-file'
2023-04-26T19:10:55.8541392Z -Try 'mv --help' for more information.
2023-04-26T19:10:55.8541705Z +error: the following required arguments were not provided:
2023-04-26T19:10:55.8541797Z +  <files>...
2023-04-26T19:10:55.8541872Z +
2023-04-26T19:10:55.8542049Z +Usage: mv [OPTION]... [-T] SOURCE DEST
2023-04-26T19:10:55.8542165Z +       mv [OPTION]... SOURCE... DIRECTORY
2023-04-26T19:10:55.8542345Z +       mv [OPTION]... -t DIRECTORY SOURCE...
2023-04-26T19:10:55.8542418Z +
2023-04-26T19:10:55.8542585Z +For more information, try '--help'.
2023-04-26T19:10:55.8543197Z +error: The argument '<files>...' requires at least 2 values, but only 1 was provided
2023-04-26T19:10:55.8543255Z +
2023-04-26T19:10:55.8543427Z +Usage: mv [OPTION]... [-T] SOURCE DEST
2023-04-26T19:10:55.8543538Z +       mv [OPTION]... SOURCE... DIRECTORY
2023-04-26T19:10:55.8543719Z +       mv [OPTION]... -t DIRECTORY SOURCE...
2023-04-26T19:10:55.8543792Z +
2023-04-26T19:10:55.8543956Z +For more information, try '--help'.
2023-04-26T19:10:55.8544108Z  mv: target 'f1': Not a directory
2023-04-26T19:10:55.8544288Z  mv: target directory 'f2': Not a directory

@shinhs0506
Copy link
Contributor Author

yep still working on it

@shinhs0506
Copy link
Contributor Author

@sylvestre our program is outputting more error messages than it should (although still helpful) do we try to fix those as well?
this pr still fixes the last 2 lines of the error messages in the screenshot.
mv-diag

@cakebaker
Copy link
Contributor

Can you please add a test to tests/by-util/test-mv.rs so we don't regress in the future?

Comment on lines +323 to +325
match b.target_dir {
Some(_) => {
return Err(MvError::TargetNotADirectory(target_dir.quote().to_string()).into())
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a hack ;-) When reading this snippet, I translate it to "If it's a target directory, return a 'Target is not a directory' error".

I think the TargetNotADirectory error should be returned from where the cli args are processed.

@uutils uutils deleted a comment from github-actions bot Apr 28, 2023
@uutils uutils deleted a comment from github-actions bot Apr 28, 2023
@cakebaker
Copy link
Contributor

Thanks for adding the tests, great work!

tests/by-util/test_mv.rs Outdated Show resolved Hide resolved
tests/by-util/test_mv.rs Outdated Show resolved Hide resolved
@sylvestre
Copy link
Contributor

@shinhs0506 please don't use screenshot. they are terrible for accessibility and search

To answer to your question, as clap is usually giving excellent error, we adjust the GNU test to match our.
Example:
https:/uutils/coreutils/blob/main/util/build-gnu.sh#L223

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/diag is no longer failing!

@sylvestre
Copy link
Contributor

Congrats! The gnu test tests/mv/diag is no longer failing!
bravo :)

util/build-gnu.sh Outdated Show resolved Hide resolved
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/diag is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@sylvestre sylvestre merged commit c5fb6ea into uutils:main Apr 29, 2023
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.

3 participants