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

Add restore function #12

Merged
merged 7 commits into from
Jan 17, 2022
Merged

Add restore function #12

merged 7 commits into from
Jan 17, 2022

Conversation

Jammyjamjamman
Copy link
Collaborator

#5

Jammyjamjamman and others added 4 commits May 2, 2021 04:14
Files can be restored using -z option.
TODO: I think some refactoring is needed.
* Remove "renamed_list" usage when restoring.
* Percent_decode restore path of file.
* Add test for info_path() function.
test for restoring using relative and absolute path

test for url encoding and decoding

commented test for appended time string when duplicate exists at
destination
src/main.rs Show resolved Hide resolved
src/trashinfo.rs Outdated
pub fn to_contents(&self) -> String {
let pct_string = percent_encode(&self.1);
format!("{}\nPath={}\nDeletionDate={}\n", self.0, pct_string, self.2)
}

pub fn from_file(path: &str) -> io::Result<Trashinfo> {
let info = configster::parse_file(path, '\n')?;
Copy link
Member

Choose a reason for hiding this comment

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

Would using the replace() string method would be a little less overhead? e.g. replace "Path=" with "" if the first 5 characters of the string are == "Path="

src/trashinfo.rs Outdated

pub fn from_file(path: &str) -> io::Result<Trashinfo> {
let info = configster::parse_file(path, '\n')?;
Ok(Trashinfo::new(&percent_decode(&info[1].value.primary).unwrap(), &info[2].value.primary))
Copy link
Member

Choose a reason for hiding this comment

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

You'll want to check to see if it's an absolute path or not. If it doesn't start with a '/'. If it doesn't start with a '/', it needs to get restored to the media root.

Copy link
Member

Choose a reason for hiding this comment

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

You'll want to check to see if it's an absolute path or not. If it doesn't start with a '/'. If it doesn't start with a '/', it needs to get restored to the media root.

Actually let's leave this for a different patch. I think it would be too difficult to test right now because rmwrs only writes absolute paths to the Path key.

* Read trashinfo line-by-line.
* Cut out "trash=" and "DeletionDate=" from lines. (I.e. replace 0..5
and 0..13 of lines with "".)
* Add noclobber suffix to file to restore, when restore path already
 exists.
src/main.rs Outdated
let mut destination = trash_info.1.clone();
let dest_path = Path::new(&destination);
if dest_path.exists() {
let noclobber_suffix = &chrono::NaiveDateTime::parse_from_str(&trash_info.2, "%Y-%m-%dT%H:%M:%S").expect("Could not parse datetime from trashinfo.").format("_%H%M%S-%y%m%d").to_string();
Copy link
Member

Choose a reason for hiding this comment

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

@Jammyjamjamman This variable already exists (see L27)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this shouldn't cause a problem, due to Rust's shadowing rules. But if it's too ugly I'd be happy to change it.

Copy link
Member

Choose a reason for hiding this comment

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

But why wouldn't you just use the variable that's already been created on L27, instead of recreating one with the same name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The other noclobber suffix gives now's datetime. I was trying to give it the same date as in its .trashinfo. Or do we want to use the current datetime as the suffix?

Copy link
Member

Choose a reason for hiding this comment

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

They should both be now but different format.

The trashinfo spec requires "%Y-%m-%dT%H:%M:%S", which is written to the .trashinfo file.

The no_clobber suffix format implemented on L27 ("_%H%M%S-%y%m%d") is one I've been using for rmw for several years. It just assures that destination files don't clobber existing files with the same name when either being restored or rmw'ed.

It's not in the .trashinfo spec. no_clobber uses a slightly shorter format, which is sufficient. unless someone manages to run rmw (or rmwrs) more than once within the same second. It doesn't need to match the same format as the DeletionDate.

It's explained a bit more in the rmw README

* Fix and simplify code for the suffix.
* Activate test for noclobber suffix.
* Add `-e` to `echo` commands in test.sh, so newlines are recognised.
pub fn from_file(path: &str) -> io::Result<Trashinfo> {
let mut lines = read_lines(path)?.skip(1);
let mut path_and_filename = lines.next().expect("Failed to read the path in trashinfo.")?;
path_and_filename.replace_range(..5, "");
Copy link
Member

Choose a reason for hiding this comment

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

To make things more robust, I think there should be a condition here to make sure that the first 5 chars of the string match "Path=".

Copy link
Member

@andy5995 andy5995 left a comment

Choose a reason for hiding this comment

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

Just the one change and I think we'll be all good

@andy5995 andy5995 merged commit 6b1be41 into trunk Jan 17, 2022
@andy5995 andy5995 deleted the add-restore-function branch January 17, 2022 20:51
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.

2 participants