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: fix invalid numbered backup path #6119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hamflx
Copy link

@hamflx hamflx commented Mar 24, 2024

see #6040 (comment)

the gnu mv:

hamflx@hamflx-workstation:~/coreutils/test$ find . -delete && mkdir C D E && touch C/c D/d E/e
hamflx@hamflx-workstation:~/coreutils/test$ mv -T --backup=numbered C E/
hamflx@hamflx-workstation:~/coreutils/test$ tree -a
.
├── D
│   └── d
├── E
│   └── c
└── E.~1~
    └── e

3 directories, 3 files

the uu_mv:

hamflx@hamflx-workstation:~/coreutils/test$ find . -delete && mkdir C D E && touch C/c D/d E/e
hamflx@hamflx-workstation:~/coreutils/test$ ../target/debug/mv -T --backup=numbered C E/
hamflx@hamflx-workstation:~/coreutils/test$ tree -a
.
├── D
│   └── d
└── E
    └── c

2 directories, 2 files

@sylvestre sylvestre force-pushed the fix/invalid-backup-numbered-path branch from d4bc4cd to 8d5334a Compare March 30, 2024 21:51
@sylvestre
Copy link
Contributor

Could you please add an highlevel integration test in https://github.com/uutils/coreutils/blob/main/tests/by-util/test_mv.rs ? thanks

@hamflx hamflx force-pushed the fix/invalid-backup-numbered-path branch from 8d5334a to 5f2f7dc Compare April 3, 2024 01:52
@hamflx
Copy link
Author

hamflx commented Apr 3, 2024

@sylvestre I have added integration test.

Copy link

github-actions bot commented Apr 3, 2024

GNU testsuite comparison:

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

@hamflx
Copy link
Author

hamflx commented Apr 11, 2024

@sylvestre @tertsdiepraam Is there anything else I need to do if I want this PR to be merged?

let test_path = Path::new(test_path_str);
let file_name = path.file_name().unwrap_or_default();
let mut numbered_file_name = file_name.to_os_string();
numbered_file_name.push(".~1~");
Copy link
Contributor

Choose a reason for hiding this comment

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

can it be incremental ? like .2, .3, etc

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to keep the logic the same as before. I think this just checks if the numbered backup exists. Only checking for '.1' is enough. If it exists, then we're still using the numbered backup, else we're using simple backup.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping ?

Copy link
Author

Choose a reason for hiding this comment

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

ping ?

sorry, English isn't my native language. What does "ping" mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, if you are still working on it :)

Copy link
Author

Choose a reason for hiding this comment

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

Is there anything else I need to do next?

Copy link
Author

Choose a reason for hiding this comment

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

I've noticed that there are some compilation errors being reported currently. If you intend to merge this PR, I can go and handle them.

@sylvestre sylvestre force-pushed the fix/invalid-backup-numbered-path branch from 5f2f7dc to 9e4566b Compare November 25, 2024 09:26
@hamflx hamflx force-pushed the fix/invalid-backup-numbered-path branch from 9e4566b to 4b7b26f Compare November 26, 2024 07:45
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

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