backup_control: Fix backup mode determination #2467

Funky185540 · 2021-06-29T13:35:34Z

Fixes determination of the backup mode, as specified in the GNU manual.

Previously the backup mode determination wouldn't allow abbreviations to the backup mode, and didn't allow to differ between the short -b option (which automatically defaults to the "existing" backup style) and the long --backup option (which has more special handling).

Also, handling of the --backup option has been revamped such that it can now be passed without an argument to it (in which case the VERSION_CONTROL env variable is read, and if that doesn't exist it defaults to "existing"). To achieve this the separator between this option and a following argument, if present, must be an equals-sign ("="). This is required in order to supply --backup without arguments (i.e. as a flag).

Due to this, previously it was impossible to use the option as a flag, as it expected arguments to be space-delimited. I.e. a syntax like ... --backup foo bar, where foo and bar are meant as arguments to the utility (i.e. file arguments to mv) would have consumed foo as backup style, which isn't meant by the user nor valid.

Also modifies the backup-specific help-texts and adds error outputs for invalid or ambiguous backup modes.

Funky185540 · 2021-06-29T13:38:23Z

Hello dear maintainers!

This merge isn't complete yet. I have so far only implemented the new interface to the determine_backup_mode function for the mv utlity. Others, such as cp, install and ln are still missing, but I wanted to keep things simple and discuss the changes on this example first.

If you find that the change is ok, I will modify the other utils that implement --backup accordingly and remove the "WIP".

src/uucore/src/lib/mods/backup_control.rs

miDeb

From my perspective this looks good!
I just have some minor suggestions.
It would be great if you could add some tests as well to make sure everything works (and keeps working) correctly :)

src/uucore/src/lib/mods/backup_control.rs

Funky185540 · 2021-06-30T08:07:23Z

From my perspective this looks good!
I just have some minor suggestions.
It would be great if you could add some tests as well to make sure everything works (and keeps working) correctly :)

Thanks for the feedback!

I tried to incorporate the tests into a tests module inside backup_control (Because I thought that it just fits nicely there). I have marked it with #[cfg(test)] and marked all test functions with #[test] but for some reason the test functions don't show up in the output of cargo test. Is there something else I need to consider to add the test, do I need to explicitly "register" them somewhere?

miDeb · 2021-06-30T09:37:13Z

The tests are not run because this repo is structured as a workspace of multiple crates. If you run cargo test only the tests in the top level crate (tests/*) will be run. To run all tests in the workspace you can run cargo test --workspace. I think the easier way would be to cd src/uucore first and run cargo test from there. This will only run the tests in uucore.

EDIT: I just noticed you can also run cargo test -p uucore to only run the tests in uucore.

Funky185540 · 2021-06-30T10:16:38Z

Thanks a lot!

2 Issues remain with the tests:

My doctest doesn't find clap, and I can't tell why
Some of the tests use env-variables, and it seems to me that these are shared between the test runner threads. Is there a better way to declare environment variables in tests? Or is there a directive that tells rust to run the tests in this module in series rather than parallel?

miDeb · 2021-06-30T10:35:57Z

My doctest doesn't find clap, and I can't tell why

I think that's because clap isn't a dependency of uucore. You can add it as a dev-dependency in Cargo.toml (the one in uucore).

Is there a better way to declare environment variables in tests? Or is there a directive that tells rust to run the tests in this module in series rather than parallel?

I don't know of any of that. A way to test this would be to extract the logic of determining the backup mode into a private function that takes the environment variable as an argument. You can then test that function.

To test with an actual environment variable you could add integration tests for the utilities that use backup_control. Integration tests live in tests/by-util/<utility>. The ucmd thing that you can see there frequently creates a new process and you can do ucmd.env() to set an environment variable just for that process.

Funky185540 · 2021-07-03T05:02:37Z

I think I fixed it now. I did some research on this type of issue and found this discussion: rust-lang/rust#47506
One of the solutions proposed in there is the use of the rusty-fork crate, which is what I have gone for now. Alternatively I could try to use lazy_static to setup a shared mutex in the test submodule and lock the mutex before running the tests.

I mostly went for rusty-fork as I wasn't aware when implementing the Mutex that one can import single macros in Rust 2018 -- because rust would refuse to compile my code when I included #[macro_use] extern crate lazy_static;, as I didn't import the macros at the crate root.

If you'd like me to change this I can try that. I'm actually not sure about the cross-platform support that rusty-fork offers...

Funky185540 · 2021-07-03T06:50:44Z

I have now got a solution that works with a global Mutex (scoped for the test submodule only) instead. Both work fine it seems, so i'll leave the final choice up to you. It feels to me like the Mutex-solution is more cross-platform compatible, but that's just a guess.

The previous help string for the backup subroutines didn't comply with the formatting found in the `--help` output of e.g. `mv` or `ln`. Use the exact help string from these utilities instead.

Add a function that takes a user-supplied backup option and checks if it matches any of the valid backup options. This is because GNU allows to abbreviate backup options, as long as they are valid and unambiguous. In case a backup option is either invalid or ambiguous, an error type is returned that contains a formatted error string for output to the user.

Refactor the function that determines which backup mode to select based on user input. It now complies with what the [GNU manual][1] specifies. [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html

Adds a tests submodule that performs tests on the `determine_backup_mode` function to ensure it handles backup options like specified by [GNU][1]. [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html

Funky185540 · 2021-07-05T12:26:28Z

Rebased onto master. I have noticed that the test_du_bytes test fails, but that's unrelated to my changes as it happens on the master branch as well.

I have now implemented the Mutex-based approach as it's more cross-platform compatible. This requires me to lock the mutex in a few tests, so I went ahead and lock it in all tests just to make sure I don't miss one. As a consequence these tests are now run in series instead of parallel, but due to their low cost it's hardly noticable.

I have also adapted the install utility as it got support for --backup & co. with pull #2457 .

Funky185540 · 2021-07-06T06:36:36Z

Build/Makefile failed a test in cp, this isn't related to this PR.

Funky185540 · 2021-07-10T08:36:03Z

@miDeb Do you have some more thoughts on the current implementation?

miDeb · 2021-07-19T12:55:02Z

Sorry, I didn't see your last comment. I will take a look now

miDeb

LGTM, sorry for the delay. Thanks for working on this!

src/uu/cp/src/cp.rs

miDeb · 2021-07-19T13:43:10Z

src/uu/install/src/install.rs

+    let backup_mode = backup_control::determine_backup_mode(
+        matches.is_present(OPT_BACKUP_NO_ARG),
+        matches.is_present(OPT_BACKUP),
+        matches.value_of(OPT_BACKUP),
+    );


What would you think about making determine_backup_mode take the wholematches instead of three arguments? (I think this version is fine though, too)

I already thought about it and that'd be cool, but in that case I'd have to streamline the naming of the various arguments involved across the utilities... I'll try to find the time tomorrow.

Example:

install calls the arguments: "backup", "backup2", "suffix"

cp calls the arguments: "backup", "b", "suffix"

You know I gave this some more thought. I'd go even one step further with this: All utils that implement backups functionality have exactly the same options for that. So I'd propose that instead of defining the options in each of the applications separately, I may just as well define the backup-related options in the backup_control as "finished" clap-Arguments (with all the texts and flags etc.) and then add these to the App in each of the applications. Then I can pass in all matches to determine_backup_mode, throw errors if the flags weren't present and continue as before if all is well and defined properly.

I think that I'd like to that in a separate PR though, as this is already somewhat big and I'm afraid I may lose myself in the changes... Would that be okay for you or should I attempt to do it in this PR?

Agree, doing this in a new PR is fine

miDeb · 2021-07-19T13:46:53Z

src/uu/cp/src/cp.rs

+        let backup_mode = match backup_mode {
+            Err(err) => {
+                return Err(Error::Backup(err));
+                // show_usage_error!("{}", err);


That's not something you have to address now, but it would be interesting to integrate this with our new error handling mechanism at a later point when all utilities support it, instead of having to repeat the error handling in all utilities.

Yes, definitely! I think I'll add it as a UCommonError, as it is consistent across utilities.

Make all tests lock a mutex to ensure that they're run in series rather than parallel. We must take this precaution due to the fact that all tests are run in parallel as threads of one parent process. As all threads in a process share e.g. environment variables, we use the Mutex to ensure they're run one after another. This way we can guarantee that tests that rely on environment variables to have specific values will see these variables, too. An alternative implementation could have used the [rusty fork][1] crate to run all tests that need env variables in separate processes rather than threads. However, rusty fork likely wouldn't run on all platforms that the utilities are supposed to run on.

Funky185540 · 2021-07-20T10:33:25Z

Okay, I'll start working on a new PR to:

Streamline the backup-control related argument handling
Add a UCustomError for Backup-related errors

Unless you have other suggestions to this PR it can be merged now. :)

Funky185540 commented Jun 29, 2021

View reviewed changes

src/uucore/src/lib/mods/backup_control.rs Show resolved Hide resolved

miDeb reviewed Jun 29, 2021

View reviewed changes

Funky185540 mentioned this pull request Jun 30, 2021

install: implement --backup (and -b) #1755

Closed

Funky185540 force-pushed the backup_mode_determination branch from 6dcb0fb to aa96595 Compare June 30, 2021 09:19

Funky185540 force-pushed the backup_mode_determination branch from aa96595 to e3c797e Compare June 30, 2021 10:14

Funky185540 added 6 commits July 5, 2021 12:43

backup_control: Add backup help string from GNU utils

3b8f135

The previous help string for the backup subroutines didn't comply with the formatting found in the `--help` output of e.g. `mv` or `ln`. Use the exact help string from these utilities instead.

backup_control: Make utility functions private

1309757

backup_control: Refactor backup mode determination

89c6d32

Refactor the function that determines which backup mode to select based on user input. It now complies with what the [GNU manual][1] specifies. [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html

backup_control: Add module tests

a783d05

Adds a tests submodule that performs tests on the `determine_backup_mode` function to ensure it handles backup options like specified by [GNU][1]. [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html

mv: Adapt for new backup_control utilities

9d94307

Funky185540 force-pushed the backup_mode_determination branch from c03c0f7 to 89d8028 Compare July 5, 2021 12:21

Funky185540 force-pushed the backup_mode_determination branch from 89d8028 to 25857c0 Compare July 5, 2021 12:51

Funky185540 changed the title ~~WIP: backup_control: Fix backup mode determination~~ backup_control: Fix backup mode determination Jul 5, 2021

Funky185540 mentioned this pull request Jul 19, 2021

uucore: error: Add docs to the code and usage functions #2501

Merged

miDeb approved these changes Jul 19, 2021

View reviewed changes

Funky185540 added 3 commits July 19, 2021 17:01

cp: Adapt to modified backup mode determination

3a01643

ln: Adapt to modified backup mode determination

2db1ec9

install: Adapt to modified backup mode determination

e07b4e9

Funky185540 force-pushed the backup_mode_determination branch from 25857c0 to e07b4e9 Compare July 19, 2021 15:02

tertsdiepraam merged commit eae8c72 into uutils:master Jul 20, 2021

Funky185540 mentioned this pull request Jul 21, 2021

backup_control: Streamline CLI arguments, use UError #2509

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backup_control: Fix backup mode determination #2467

backup_control: Fix backup mode determination #2467

Funky185540 commented Jun 29, 2021 •

edited

Loading

Funky185540 commented Jun 29, 2021

miDeb left a comment

Funky185540 commented Jun 30, 2021

miDeb commented Jun 30, 2021 •

edited

Loading

Funky185540 commented Jun 30, 2021

miDeb commented Jun 30, 2021

Funky185540 commented Jul 3, 2021

Funky185540 commented Jul 3, 2021

Funky185540 commented Jul 5, 2021

Funky185540 commented Jul 6, 2021

Funky185540 commented Jul 10, 2021

miDeb commented Jul 19, 2021

miDeb left a comment

miDeb Jul 19, 2021

Funky185540 Jul 19, 2021

Funky185540 Jul 20, 2021

miDeb Jul 20, 2021

miDeb Jul 19, 2021

Funky185540 Jul 19, 2021

Funky185540 commented Jul 20, 2021

backup_control: Fix backup mode determination #2467

backup_control: Fix backup mode determination #2467

Conversation

Funky185540 commented Jun 29, 2021 • edited Loading

Funky185540 commented Jun 29, 2021

miDeb left a comment

Choose a reason for hiding this comment

Funky185540 commented Jun 30, 2021

miDeb commented Jun 30, 2021 • edited Loading

Funky185540 commented Jun 30, 2021

miDeb commented Jun 30, 2021

Funky185540 commented Jul 3, 2021

Funky185540 commented Jul 3, 2021

Funky185540 commented Jul 5, 2021

Funky185540 commented Jul 6, 2021

Funky185540 commented Jul 10, 2021

miDeb commented Jul 19, 2021

miDeb left a comment

Choose a reason for hiding this comment

miDeb Jul 19, 2021

Choose a reason for hiding this comment

Funky185540 Jul 19, 2021

Choose a reason for hiding this comment

Funky185540 Jul 20, 2021

Choose a reason for hiding this comment

miDeb Jul 20, 2021

Choose a reason for hiding this comment

miDeb Jul 19, 2021

Choose a reason for hiding this comment

Funky185540 Jul 19, 2021

Choose a reason for hiding this comment

Funky185540 commented Jul 20, 2021

Funky185540 commented Jun 29, 2021 •

edited

Loading

miDeb commented Jun 30, 2021 •

edited

Loading