+
Skip to content

mv: gnu test case to-symlink fix #6578

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 18, 2024
Merged

mv: gnu test case to-symlink fix #6578

merged 2 commits into from
Jul 18, 2024

Conversation

matrixhead
Copy link
Contributor

@matrixhead matrixhead commented Jul 17, 2024 •
edited
Loading

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

@github-actions GitHub Actions
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!

BenWiederhake
BenWiederhake reviewed Jul 17, 2024
View reviewed changes
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.

@matrixhead matrixhead requested a review from BenWiederhake July 18, 2024 10:03
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

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

BenWiederhake
BenWiederhake reviewed Jul 18, 2024
View reviewed changes
@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
@matrixhead
Copy link
Contributor Author

matrixhead commented Jul 18, 2024 •
edited
Loading

okay, I will try 😃

@matrixhead matrixhead mentioned this pull request Jul 18, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request Nov 16, 2024
Merged
@moonfruit moonfruit mentioned this pull request Nov 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenWiederhake BenWiederhake BenWiederhake left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mv: gnu test case to-symlink compatibility

2 participants

@matrixhead @BenWiederhake
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载