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

rm: rm3 now passes #4013

Merged
merged 35 commits into from
Oct 31, 2022
Merged

rm: rm3 now passes #4013

merged 35 commits into from
Oct 31, 2022

Conversation

palaster
Copy link
Contributor

@palaster palaster commented Oct 8, 2022

  • Changed the use of hard-coded prompts messages to use uucore::util_name instead
  • Rewrote prompt_write_protected and prompt_file into a single function prompt_file because I a lot of the code was being reused for both

@palaster palaster mentioned this pull request Oct 8, 2022
Closed
@sylvestre
Copy link
Contributor

it seems that your change broke tests/cp/perm in GNU
could you please have a look ?
thanks

Error: GNU test failed: tests/cp/perm. tests/cp/perm is passing on 'main'. Maybe you have to rebase?

@palaster
Copy link
Contributor Author

palaster commented Oct 9, 2022

I found out why it was causing those not to pass it is because the --force (-f) argument implementation wasn't complete. When using -f it forces it to never prompt and that wasn't implemented

@palaster
Copy link
Contributor Author

palaster commented Oct 10, 2022 •
edited
Loading

I am not sure why this is breaking the tail test, test_tail::test_follow_name_remove, for macos. And I run a linux machine and don't have a good way to go about testing the code

tertsdiepraam
tertsdiepraam reviewed Oct 10, 2022
View reviewed changes
src/uu/rm/src/rm.rs Outdated Show resolved Hide resolved
@uutils uutils deleted a comment from github-actions bot Oct 10, 2022
@uutils uutils deleted a comment from github-actions bot Oct 10, 2022
@tertsdiepraam
Copy link
Member

I am not sure why this is breaking the tail test, test_tail::test_follow_name_remove, for macos.

It's probably an intermittently failing test. I'm rerunning the job.

tertsdiepraam
tertsdiepraam reviewed Oct 11, 2022
View reviewed changes
src/uu/rm/src/rm.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Oct 11, 2022 •
edited by sylvestre
Loading

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

sylvestre
sylvestre reviewed Oct 12, 2022
View reviewed changes
src/uu/rm/src/rm.rs Outdated Show resolved Hide resolved
sylvestre
sylvestre reviewed Oct 12, 2022
View reviewed changes
src/uu/rm/src/rm.rs Outdated Show resolved Hide resolved
sylvestre
sylvestre reviewed Oct 12, 2022
View reviewed changes
src/uu/rm/src/rm.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@uutils uutils deleted a comment from github-actions bot Oct 13, 2022
sylvestre
sylvestre approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

Looks great to me

(@tertsdiepraam if you merge it, please squash it ;)

tertsdiepraam
tertsdiepraam requested changes Oct 13, 2022
View reviewed changes
src/uu/rm/src/rm.rs Show resolved Hide resolved
src/uu/rm/src/rm.rs Outdated Show resolved Hide resolved
src/uu/rm/src/rm.rs Show resolved Hide resolved
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!
Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

1 similar comment
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

1 similar comment
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@palaster
Copy link
Contributor Author

Is anything else needed?

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!
GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

1 similar comment
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm3 is no longer failing!

@sylvestre
Copy link
Contributor

Oh, rm is complex!
Happy with the change. @tertsdiepraam your call now :)

tertsdiepraam
tertsdiepraam approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Yep looks all good now! @palaster thank you for sticking with this!

@tertsdiepraam tertsdiepraam merged commit ba3bb56 into uutils:main Oct 31, 2022
@sylvestre
Copy link
Contributor

Indeed, bravo!

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Dec 19, 2022 •
edited
Loading

So, I took another look at this and it seems to me that we've overcomplicated this, unless I'm missing something. As far as I can see, -f should simply override -i and -I (because they change the same variables in GNU) and that should give us most of the behaviour we want. But we need to keep this custom index check between -f and --interactive, because --interactive=never does not interact with -f, while --interactive=once and --interactive=always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre approved these changes

@tertsdiepraam tertsdiepraam tertsdiepraam approved these changes

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
@palaster @sylvestre @tertsdiepraam