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

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

Closed
sylvestre opened this issue Mar 6, 2021 · 15 comments
Closed

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

sylvestre opened this issue Mar 6, 2021 · 15 comments
Labels
good first issue For newcomers!

Comments

@sylvestre
Copy link
Sponsor Contributor

sylvestre commented Mar 6, 2021

GNU man pages says:

       --backup[=CONTROL]
              make a backup of each existing destination file

As ride along, it is possible to implement -b
-b like --backup but does not accept an argument

Note that src/uu/mv/src/mv.rs has backup support and
we should not duplicate the code.
Instead, we should use src/uucore/src/lib/features

@sylvestre sylvestre added the good first issue For newcomers! label Mar 6, 2021
@Voultapher
Copy link

While tests/by-util/test_install.rs exists I'm unable to run them I've tried for example cargo test install. Also what do you mean with 'Instead, we should use src/uucore/src/lib/features'. I understand factoring out the backup logic in src/uu/mv/src/mv.rs but how does that overlap with using features?

@sylvestre
Copy link
Sponsor Contributor Author

Try to run

cargo test --features feat_os_unix

@sylvestre
Copy link
Sponsor Contributor Author

for the feature, it is just a simple place to have the function used by the two programs (but if it doesn't make much sense, feel free to ignore my comment)

@justizin
Copy link

justizin commented Apr 7, 2021

is someone working on this? i’m looking for a first bug to tackle and ran into this myself, seems like it should be reasonably straightforward and i agree it feels worthwhile to try and reuse this functionality if possible.

@Voultapher
Copy link

Voultapher commented Apr 7, 2021 via email

@sivachandran
Copy link
Contributor

@bitmonk Are you working on it?

@justizin
Copy link

I took a look some weeks back and I'm getting back to it, now. IIRC spent most of my time looking at what might be reusable from mv, though I think the existing code may need to be refactored to serve both purposes.

I'll get back to some of those details, today, but my general thinking is that it may be worth writing backup functionality for install, then trying to combine them. A common maxim I've heard is not to worry too much about reusability if something may only be duplicated once, but I'm fairly sure there are several coreutils commands which could benefit from general-purpose backup functionality.

@Funky185540
Copy link
Contributor

Hello,

I don't mean to be rude but I have attempted to implement the --backup option here in case anyone is interested. Unfortunately I started coding before I actually read the discussion to this issue...

Are you still working on this, @bitmonk?

@justizin
Copy link

I have a stale attempt at this, I got stuck trying to genericize this behavior, then as I mentioned above, decided it was probably not worth it with only two commands having a --backup functionality.

Yours looks great at a glance, would love to chat about what it might take to address this deviation from the manual: master...Funky185540:install-implement-backup?expand=1#diff-f64dac8ba1

@Funky185540
Copy link
Contributor

I have a stale attempt at this, I got stuck trying to genericize this behavior, then as I mentioned above, decided it was probably not worth it with only two commands having a --backup functionality.

Well in fact there are at least 4 commands in the coreutils that use the --backup option: mv, cp ln and install. And they all use pretty much exactly the same code and options for it, too.

Yours looks great at a glance, would love to chat about what it might take to address this deviation from the manual:

Thanks, appreciate it! I already have an idea for a "fix" to this deviation. I wanted to open another PR and refactor the "backup_control" a little. If you feel like it you can have a go, too, then we can compare and maybe fuse our codes together... ?
See how it fares against the critics here... :)

IMO the interface to determine_backup_mode needs to be rewritten such that it accepts the "long" and "short" backup options (the bool returned from 'is_present' or what it's called) separately, as these have a different behavior according to the GNU docs. Then, if '--backup' is supplied, we:

  1. Check if it has an argument supplied. If so:
    • Match (this calls for a regex) against the values that the GNU manual allows for the backup type
    • If there's a match: Take that. If the match is ambiguous: Reject it, print error. If there's no match: Error as well
  2. If there's no argument, check if the env var mentioned in the manual exists
  3. If neither exists, use the default

If there's no long option but only the short one, default to "existing" type. If both the long and short option are supplied (!), the long option always takes precedence.

Then I'd also like to add a new function (maybe call it backup_file) that then performs the actual backup. This is (as far as I could determine today) needed for all utilities that offer the '--backup' option. That way we would have the backup process streamlined, because if you look at what "mv" does, you'll see it has it's own wrapper around fs::rename that contains failsafes and falls back to "copying and deleting" the files to make backups in case fs::rename fails. Now I don't know why fs::rename may fail but "copying and deleting" wouldn't, but it's interesting to see. It also seems to consider and react to some other corner cases...
The backup-implementation for ln on the other hand (that also has '--backup') really only calls fs::rename, without failsafes or other strings attached.

This could be streamlined I think, as the process of "backing up a file" is always the same.

Well those are just some thoughts. Like I said, I'll make a PR for this in the next few days and see how these changes integrate with the CLI programs I mentioned above. Of course, you're more than welcome to make a different implementation, add to this and throw in some arguments yourself. I'd be curious to hear what you think about it, as you have already worked with the code, too!

I'm still very new to rust, and I find this so very exciting... :-)

@rivy
Copy link
Member

rivy commented Jun 24, 2021

Happy to see the interest!

See how it fares against the critics here... :)

We critics look forward to your submission(s). 😄

Then I'd also like to add a new function (maybe call it backup_file) that then performs the actual backup. This is (as far as I could determine today) needed for all utilities that offer the '--backup' option. That way we would have the backup process streamlined, because if you look at what "mv" does, you'll see it has it's own wrapper around fs::rename that contains failsafes and falls back to "copying and deleting" the files to make backups in case fs::rename fails. Now I don't know why fs::rename may fail but "copying and deleting" wouldn't, but it's interesting to see. It also seems to consider and react to some other corner cases...

On various platforms, file renames across file system boundaries will usually fail. Most times, it's not easy (or possible) to determine if the source and destinations are on the same file system. Hence, copy and delete is used as a fallback strategy.

This could be streamlined I think, as the process of "backing up a file" is always the same.

If it's common across utilities, we could add backup_file(...) as feature-gated functionality to uucore.

@Funky185540
Copy link
Contributor

@bitmonk have you had a shot at the --backup behavior yet?

@justizin
Copy link

@Funky185540 - just catching up, here, it sounds like you are way deeper into this than I've gotten and I'd love to contribute to your WIP. Don't forget you can open PRs against branches, so you can leave your current PR intact if it's working, and create a refactor PR against it. I do this sometimes based on my experience of completely destroying WIP. :)

Well in fact there are at least 4 commands in the coreutils that use the --backup option: mv, cp ln and install. And they all use pretty much exactly the same code and options for it, too.

I stand corrected. :) When I looked at this, I only found one coincidence and I recalled it being different, but it's been a few months so I remember the conclusion I came to, but not the details.

Yours looks great at a glance, would love to chat about what it might take to address this deviation from the manual:

Thanks, appreciate it! I already have an idea for a "fix" to this deviation. I wanted to open another PR and refactor the "backup_control" a little. If you feel like it you can have a go, too, then we can compare and maybe fuse our codes together... ?
See how it fares against the critics here... :)

I think @rivy's feedback here is really useful, and I'd love to peek at your next PR. It seems like you're moving at a faster pace than I am, so I don't want to gum up the works.

I'm still very new to rust, and I find this so very exciting... :-)

Same! Part of why my WIP here got stale is I decided to work through some of, "Practical Systems Programming in Rust", so that I'd have better reflexes working through this larger, real-world codebase.

Looks like you're doing great! I'm happy to help however I can!

@Funky185540
Copy link
Contributor

Funky185540 commented Jun 30, 2021

@Funky185540 - just catching up, here, it sounds like you are way deeper into this than I've gotten and I'd love to contribute to your WIP. Don't forget you can open PRs against branches, so you can leave your current PR intact if it's working, and create a refactor PR against it. I do this sometimes based on my experience of completely destroying WIP. :)

Cool trick, thanks! ;)

I think @rivy's feedback here is really useful, and I'd love to peek at your next PR. It seems like you're moving at a faster pace than I am, so I don't want to gum up the works.

I've opened the PR for the improved 'backup option handling' just yesterday: #2467

I'm currently trying to add proper tests to it, but that doesnät work for reasons I don't understand yet...

Same! Part of why my WIP here got stale is I decided to work through some of, "Practical Systems Programming in Rust", so that I'd have better reflexes working through this larger, real-world codebase.

Looks like you're doing great! I'm happy to help however I can!

I totally understand that. I've been wanting to contribute to some FOSS project for a long time, so I decided I'd do it to learn how to code rust. But we all learn differently, right? And that's fine.

So as I see this there are 2 more points to get the backups 'right'.

Improve the backup suffix determination

This is slightly off-topic for this particular issue, but I'll leave it here anyways

This should be a simple one as it only parses either a CLI argument or and env variable. I noticed that there is something odd in the current implementation, because what happens e.g. if you add a path separator into the suffix? So you could give it a suffix of "~/~", which would imply to move the file to backup to a whole different subfolder.

I haven't checked the C implementation, but I did some quick tests with e.g. cp or mv and their -S option. Here's an MWE to give you and idea of what I mean (using the default C utilities):

$ mkdir /tmp/playground && cd /tmp/playground
$ mkdir dest
# So backup works immediately
$ touch a dest/a
# Sanity check, creates "dest/a~"
$ cp -b a dest/a
$ ls dest
a  a~
$ rm dest/a~
# Invalid!
$ cp -b -S "foo/bar~" a dest/a
$ ls dest
a  a~
# Valid
$ cp -b -S "suffix~" a dest/a
$ ls dest
a  a~  asuffix~

Interestingly the erroneous suffix doesn't cause an error, it just silently falls back to the default suffix '~'. It might be worth checking the GNU implementation to see if they handle any other special cases that I haven't thought of yet.

Perform backups

Like @rivy said, it would be cool to have a backup_file function that does the backing up for us. I have started working on this here: https://github.com/Funky185540/coreutils/tree/backup_control_perform_backups

I thought that as it handles files, directories and symlinks, I'd call it perform_backup instead, but that's more of a detail than a feature. The code is mostly taken from what mv has already implemented. It still needs tests, and it'd be cool to have some file I/O expert look over it to tell us whether we forgot some OS-specific corner cases.

Apart from that I'd say that the backup functionality is good to go. Once these changes are in we can refactor the utilities using it to use the same code, instead of their own implementations.

If you want to pick either of these, or both, feel free to do it! I think that once this is done I'll have a stab at the SELinux context options (-Z and --context), because as far as I can tell these aren't implemented in most (all?) of the utlities.

@Funky185540
Copy link
Contributor

I think the issue can be closed as it has been implemented with #2457.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue For newcomers!
Projects
None yet
Development

No branches or pull requests

7 participants