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.

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

mv: gnu test case part-hardlink fix #6632

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

mv: gnu test case part-hardlink fix #6632

wants to merge 16 commits into from

Conversation

matrixhead
Copy link
Contributor

fix for #6631

behaviours changed

  • mv will now preserve links during an inter-device move.
  • Previously, mv would silently fail to create a backup when no-target-directory was specified and the destination ended with a / This patch resolves that issue.
  • The progress bar will now work for simple inter-device file moves as well.

sylvestre
sylvestre reviewed Aug 11, 2024
View reviewed changes
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

sylvestre
sylvestre reviewed Aug 11, 2024
View reviewed changes
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
@sylvestre
Copy link
Sponsor Contributor

Bravo

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

1 similar comment
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

sylvestre
sylvestre reviewed Aug 13, 2024
View reviewed changes
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/mv/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

@BenWiederhake BenWiederhake mentioned this pull request Sep 7, 2024
Open
BenWiederhake
BenWiederhake reviewed Sep 7, 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.

Thank you for tackling these issues! However, combining these three behavioral changes seems like a bad idea, given that each of them have delicate consequences.

It seems that get_dir_content is the entirely wrong tool to approach mv, and it causes incorrect behavior.

Finally, it seems you fell into a common trap / bad pattern: Guessing a potential error condition (and not detecting others), then doing an action (and badly handling most error conditions.) This causes unhelpful error messages and unnecessary syscalls.

src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
tests/by-util/test_mv.rs Outdated Show resolved Hide resolved
tests/by-util/test_mv.rs Outdated Show resolved Hide resolved
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
BenWiederhake
BenWiederhake requested changes Sep 7, 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.

Whoops, I meant to set the "Request changes" flag. See above for my reasoning. In particular, get_dir_content must go.

May I suggest to split the tree changes into two or three PRs?

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 8, 2024

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/hard-link-1 is no longer failing!
Congrats! The gnu test tests/mv/part-hardlink is no longer failing!

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?

@matrixhead matrixhead marked this pull request as draft September 11, 2024 17:47
@matrixhead
Copy link
Contributor Author

@BenWiederhake I cleaned up this PR. Now, it focuses solely on the directory copying part. I'll work on verbose output and error reporting next. For now, could you let me know if this approach looks okay?

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

sylvestre
sylvestre reviewed Sep 12, 2024
View reviewed changes
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@matrixhead matrixhead force-pushed the part-hardlink branch 3 times, most recently from 9a6a187 to 660eb13 Compare October 9, 2024 09:19
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/rm/rm1 (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/rm/rm2 is no longer failing!

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/rm/rm1 (fails in this run but passes in the 'main' branch)
GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/rm/rm2 is no longer failing!

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/rm/rm1 (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/rm/rm2 is no longer failing!

@BenWiederhake
Copy link
Collaborator

@matrixhead I'm unclear on how you wish to proceed. CI is green and ready to merge (assuming I would find no new issues in another review), but the PR is marked as "draft". You also keep force-pushing the branch, and I'm not sure whether you're introducing new changes each time, or just updating to latest master.

@matrixhead
Copy link
Contributor Author

I'm sorry for all the force pushes. I was just trying to fix a bug in one of the tests I wrote, which I couldn't reproduce locally. I think everything is good now, and the PR is ready to merge, except for that one review I couldn't resolve.

@matrixhead matrixhead marked this pull request as ready for review October 9, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenWiederhake BenWiederhake Awaiting requested review from BenWiederhake

@sylvestre sylvestre Awaiting requested review from sylvestre

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matrixhead @sylvestre @BenWiederhake