cp: Updated prompt logic to handle update with and without interactive mode enabled #6069

Vikrant2691 · 2024-03-14T00:13:37Z

Fixes #6019
The issue was that the condition preemptively returned Ok when the status was UpdateMode::ReplaceIfOlder.
But if the condition is removed, it will prompt for coreutils cp -i -u old new.
This is a very peculiar condition where we don't want to prompt where we know time(old)>time(new). (update only overwrites when a new file is copied to an old file.)
So, the prompt needs to be closer to the condition where the time for the folders is checked.

cakebaker · 2024-03-14T06:14:36Z

Can you please add a test to ensure we don't regress in the future?

github-actions · 2024-03-14T06:39:37Z

GNU testsuite comparison:

GNU test failed: tests/rm/rm2. tests/rm/rm2 is passing on 'main'. Maybe you have to rebase?

Vikrant2691 · 2024-03-14T18:53:06Z

Can you please add a test to ensure we don't regress in the future?

Hi @cakebaker, there was already a test case written for the current scenario. and for coreutils cp -i -u old new we already have a test case. According to me, all tests are covered.

cakebaker · 2024-03-15T13:38:11Z

@Vikrant2691 all the better if there is already a test :) I don't know if you have seen that it fails?

Vikrant2691 · 2024-03-15T13:39:30Z

I will look into it today and let you know :)

…

On Fri, Mar 15, 2024, 13:38 Daniel Hofstetter ***@***.***> wrote: @Vikrant2691 <https://github.com/Vikrant2691> all the better if there is already a test :) I don't know if you have seen that it fails? — Reply to this email directly, view it on GitHub <#6069 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3BNPRDWRUFSEQYMXP2PYLYYL2VVAVCNFSM6AAAAABEVDVBCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJZGY4DKNJXG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions · 2024-03-23T06:45:13Z

GNU testsuite comparison:

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

github-actions · 2024-03-23T13:21:22Z

GNU testsuite comparison:

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

BenWiederhake

test_cp_arg_update_interactive failed. Congratulations, this fix reveals the reason for a flaky test, because now it depends on which file is newer; and because the test fixture copies file in "random" order (typically inode order, which is "random"), this will fail 50% of the time.

Please fix this test; perhaps remove it entirely because this is now covered by the new tests.

Additional comments:

I don't quite understand how cp is structured, and it seems surprising to me that handle_existing_dest() checks the update mode, even though handle_copy_mode() checks it again a bit later. Is there a scenario where it ends up asking for confirmation twice?
If you feel like writing more tests, I think it's a good idea to test test_cp_arg_interactive_update_overwrite_older() but with the input Y.
Although this project seems to pay not much attention to it, I would like to recommend git rebase to you. This is a great opportunity to get started! In this case, maybe a single or perhaps two commits would be easier to understand than 11 fix-fixing-fix-commits and pointless merge-commits.

sylvestre · 2024-03-25T16:11:56Z

tests/by-util/test_cp.rs

    // -u -i *WILL* show the prompt to validate the override.
    // Therefore, the error code depends on the prompt response.
    let (at, mut ucmd) = at_and_ucmd!();
    at.touch("b");
+    std::thread::sleep(Duration::from_secs(1));


could you please add a comment why sleep ?
and can we make it shorter ?
thanks

yes, this issue is what I posted in the chat. The test itself was failing initially. The reason is that the two create calls are so close to each other that the files get at the same time. To mitigate that we have two options. 1) add a delay and 2) set time explicitly.
I tried to set time explicitly initially, but the libraries I used were unstable (FileTimes). Please let me know if you have some option to set time while creating the file. I had no other option than to use 1.
Sorry, I did not get what you meant by shorter...

For 2) you could use something like:

let _ = at.make_file("b").set_modified(std::time::SystemTime::UNIX_EPOCH);

Update: Please ignore it, I just realized this snippet requires Rust 1.75 :|

For 2) you could use something like:

let _ = at.make_file("b").set_modified(std::time::SystemTime::UNIX_EPOCH);

Update: Please ignore it, I just realized this snippet requires Rust 1.75 :|

yes, I tried FileTimes but it did not work as you said...

Vikrant2691 · 2024-03-25T18:52:34Z

test_cp_arg_update_interactive failed. Congratulations, this fix reveals the reason for a flaky test, because now it depends on which file is newer; and because the test fixture copies file in "random" order (typically inode order, which is "random"), this will fail 50% of the time.

Please fix this test; perhaps remove it entirely because this is now covered by the new tests.

Additional comments:

I don't quite understand how cp is structured, and it seems surprising to me that handle_existing_dest() checks the update mode, even though handle_copy_mode() checks it again a bit later. Is there a scenario where it ends up asking for confirmation twice?

If you feel like writing more tests, I think it's a good idea to test test_cp_arg_interactive_update_overwrite_older() but with the input Y.

Although this project seems to pay not much attention to it, I would like to recommend git rebase to you. This is a great opportunity to get started! In this case, maybe a single or perhaps two commits would be easier to understand than 11 fix-fixing-fix-commits and pointless merge-commits.

Actions on me:

I will remove the flaky test
Add a test for the 'Y' option.

Answering additional comments

you are right. The structure and flag management need to be looked at for cp. handle_existing_dest() handles "overwrite" and handle_copy_mode() checks the "update" modes. I tried not to rock the boat too much. I just changed where we are "prompting" the user. -i -u is specific with the times. So I had to route the prompt where the time is checked. we can move the check as well. Please let me know if we can do it better.
I will follow the rebase suggestion .. thank you

Vikrant2691 · 2024-03-28T19:02:58Z

I am not sure why the checks in the pipeline failed.. could we please run it again

cakebaker · 2024-03-29T06:41:56Z

@Vikrant2691 The failure of the windows-gnu checks are unrelated to your PR, they currently fail for all PRs :|

BenWiederhake

Looks good to me!

EDIT: Looks like this needs a slightly more involved rebase. I currently don't have the capacity to do it, sorry. Your options are:

Some other maintainer steps in and rebases it
You rebase it (highly recommended!), and make it trivial to merge
A long time passes, I have enough time, and then I'll rebase it.

Vikrant2691 · 2024-04-08T21:35:33Z

Looks good to me!

EDIT: Looks like this needs a slightly more involved rebase. I currently don't have the capacity to do it, sorry. Your options are:

Some other maintainer steps in and rebases it

You rebase it (highly recommended!), and make it trivial to merge

A long time passes, I have enough time, and then I'll rebase it.

okk let me check

Vikrant2691 · 2024-04-08T21:53:47Z

Looks good to me!

EDIT: Looks like this needs a slightly more involved rebase. I currently don't have the capacity to do it, sorry. Your options are:

Some other maintainer steps in and rebases it

You rebase it (highly recommended!), and make it trivial to merge

A long time passes, I have enough time, and then I'll rebase it.

Could you please check if the rebase is successful?

BenWiederhake · 2024-04-09T13:21:54Z

Sorry, my bad, let me try to fix this.

BenWiederhake · 2024-04-09T13:31:01Z

I messed up, I didn't want to close this PR, and because of reasons that I understand but cannot fix, this PR cannot be re-opened easily. Instead, please see #6207. I promise to learn from my mistake!

Please sanity-check whether this looks correct. git diff says there is no difference in content between c94809b (your original final commit) and 9b9c0cc (this commit).

If you agree that there is no difference, I'll just merge #6207 instead.

Vikrant2691 · 2024-04-10T09:27:07Z

Okk let me check

…

On Tue, Apr 9, 2024, 14:31 Ben Wiederhake ***@***.***> wrote: I messed up, I didn't want to close this PR, and because of reasons that I understand but cannot fix, this PR cannot be re-opened easily. Instead, please see #6207 <#6207>. I promise to learn from my mistake! Please sanity-check whether this looks correct. git diff says there is no difference in content between c94809b <c94809b> (your original final commit) and 9b9c0cc <9b9c0cc> (this commit). If you agree that there is no difference, I'll just merge #6207 <#6207> instead. — Reply to this email directly, view it on GitHub <#6069 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3BNPTKEX6G3EXSTCATUU3Y4PUSXAVCNFSM6AAAAABEVDVBCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBVGE4DONJUHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

cakebaker changed the title ~~Fixes #6019: Updated prompt logic to handle update with and without interactive mode enabled~~ cp: Updated prompt logic to handle update with and without interactive mode enabled Mar 14, 2024

sylvestre force-pushed the main branch from 9f0caf4 to 1a7877b Compare March 23, 2024 11:22

BenWiederhake requested changes Mar 24, 2024

View reviewed changes

sylvestre reviewed Mar 25, 2024

View reviewed changes

Vikrant2691 requested a review from BenWiederhake April 2, 2024 19:30

BenWiederhake approved these changes Apr 4, 2024

View reviewed changes

BenWiederhake closed this Apr 9, 2024

BenWiederhake force-pushed the main branch from c94809b to aeafabd Compare April 9, 2024 13:21

BenWiederhake mentioned this pull request Apr 9, 2024

cp: handle update prompt with and without interactive mode enabled #6207

Merged

BenWiederhake mentioned this pull request Apr 24, 2024

cp: Wrong handling of -i -u #6019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cp: Updated prompt logic to handle update with and without interactive mode enabled #6069

cp: Updated prompt logic to handle update with and without interactive mode enabled #6069

Vikrant2691 commented Mar 14, 2024 •

edited

Loading

cakebaker commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

Vikrant2691 commented Mar 14, 2024

cakebaker commented Mar 15, 2024

Vikrant2691 commented Mar 15, 2024 via email

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

BenWiederhake left a comment

sylvestre Mar 25, 2024

Vikrant2691 Mar 25, 2024

cakebaker Mar 28, 2024 •

edited

Loading

Vikrant2691 Mar 28, 2024

Vikrant2691 commented Mar 25, 2024

Vikrant2691 commented Mar 28, 2024

cakebaker commented Mar 29, 2024

BenWiederhake left a comment •

edited

Loading

Vikrant2691 commented Apr 8, 2024

Vikrant2691 commented Apr 8, 2024

BenWiederhake commented Apr 9, 2024

BenWiederhake commented Apr 9, 2024

Vikrant2691 commented Apr 10, 2024 via email

cp: Updated prompt logic to handle update with and without interactive mode enabled #6069

cp: Updated prompt logic to handle update with and without interactive mode enabled #6069

Conversation

Vikrant2691 commented Mar 14, 2024 • edited Loading

cakebaker commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

Vikrant2691 commented Mar 14, 2024

cakebaker commented Mar 15, 2024

Vikrant2691 commented Mar 15, 2024 via email

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

BenWiederhake left a comment

Choose a reason for hiding this comment

sylvestre Mar 25, 2024

Choose a reason for hiding this comment

Vikrant2691 Mar 25, 2024

Choose a reason for hiding this comment

cakebaker Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Vikrant2691 Mar 28, 2024

Choose a reason for hiding this comment

Vikrant2691 commented Mar 25, 2024

Vikrant2691 commented Mar 28, 2024

cakebaker commented Mar 29, 2024

BenWiederhake left a comment • edited Loading

Choose a reason for hiding this comment

Vikrant2691 commented Apr 8, 2024

Vikrant2691 commented Apr 8, 2024

BenWiederhake commented Apr 9, 2024

BenWiederhake commented Apr 9, 2024

Vikrant2691 commented Apr 10, 2024 via email

Vikrant2691 commented Mar 14, 2024 •

edited

Loading

cakebaker Mar 28, 2024 •

edited

Loading

BenWiederhake left a comment •

edited

Loading