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: gnu test case to-symlink fix #6578

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Conversation

matrixhead
Copy link
Contributor

@matrixhead matrixhead commented Jul 17, 2024

tries to fix #6577

Behaviors changed

  • mv would remove the destination file first, if we are trying to move the file between different partitions and the destination is a symlink

Copy link

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/mv/to-symlink is no longer failing!

Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Code looks fine, and the test would detect most types of bugs. Great!

The remaining CI failures are unrelated issues: GNU test timeout is flaky, #6534, #6570 (both).

Can I ask you to improve the error message though? Right now it's very cryptic. Also, the test could use an additional check, just to make extra sure that we will never accidentally overwrite other_fs_file.txt or something.

tests/by-util/test_mv.rs Outdated Show resolved Hide resolved
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/to-symlink is no longer failing!

@BenWiederhake
Copy link
Collaborator

Looks good, the only remaining CI failure is unrelated: #6534.

I'm a bit unhappy about the many fully-qualified usages of various traits/structs, like std::io::Write::write_all and std::os::unix::fs::symlink etc. However, I couldn't find a different test that has to deal with absolute filenames like this, and I can't come up with a better way to do it, so I just accept it and merge it.

If you can come up with a nicer way in the future, please do create a PR :)

@BenWiederhake BenWiederhake merged commit 84f8b7a into uutils:main Jul 18, 2024
67 of 68 checks passed
@matrixhead
Copy link
Contributor Author

matrixhead commented Jul 18, 2024

okay, I will try 😃

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.

mv: gnu test case to-symlink compatibility
2 participants