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

cp: fix verbose output order after prompt #7287

Merged
merged 5 commits into from
Feb 11, 2025

Conversation

aimerlief
Copy link
Contributor

Fixes: #7285

This pull request addresses an ordering issue in the cp command where the verbose message is printed before the interactive prompt for overwrite confirmation.

In GNU cp, the verbose output appears only after the user has responded to the prompt, and this PR adjusts our behavior to match that order.

Copy link

github-actions bot commented Feb 8, 2025

GNU testsuite comparison:

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

@sylvestre
Copy link
Contributor

Could you please add a test to make sure we don't regress? Thanks

@aimerlief
Copy link
Contributor Author

Thank you for your feedback. I'll add a test.

@aimerlief
Copy link
Contributor Author

In the issue #7285, when doing cp -v -i, the file is diff'd by timestamp and --update=older is used. The same test would use filetime crate, but with the following target_os limitation:

#[cfg(any(target_os = "linux", target_os = "android"))]
use filetime::FileTime;

Therefore, in the added test, cp -v -i is performed without --update=older and its output is checked.

@cakebaker
Copy link
Contributor

Therefore, in the added test, cp -v -i is performed without --update=older and its output is checked.

This test verifies the existing behavior, which is fine, but it doesn't account for the changes you made in cp.rs, as it passes without those changes.

Additionally, I think you don't need filetime::FileTime and can use File::set_modified instead (it's used in some other cp tests).

@aimerlief
Copy link
Contributor Author

Thank you for your feedback. My understanding was that the tests were lacking checks for output order in my changes. I've updated the tests in a new commit accordingly. If my understanding is incorrect, please let me know.

@cakebaker
Copy link
Contributor

I think there is some confusion. Let me clarify: your change is about fixing the behavior of cp -v -i --update=older new old from

$ cp -v -i --update=older new old
'new' -> 'old'
cp: overwrite 'old'?

to

$ cp -v -i --update=older new old
cp: overwrite 'old'? y
'new' -> 'old'

and

$ cp -v -i --update=older new old
cp: overwrite 'old'? n

That works fine. However, your test doesn't ensure this new behavior works. It ensures cp -v -i new old works. And that's not the same as cp -v -i --update=older new old.

@aimerlief
Copy link
Contributor Author

Thank you for your feedback and clarifications. I have implemented the changes accordingly; could you please review them?
I now recognize that my understanding of the --update=older option was lacking.

@cakebaker cakebaker merged commit dd7b454 into uutils:main Feb 11, 2025
65 checks passed
@cakebaker
Copy link
Contributor

Thanks!

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.

cp: should print verbose message after interactive prompt
3 participants