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

Don't restore files that still exist unchanged #721

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

grote
Copy link
Collaborator

@grote grote commented Aug 16, 2024

Before, we would restore files no matter what. The idea was that restore only happens in SUW, so there would be no pre-existing files anyway. In practice, we created duplicated with (1) suffixes which is not really desirable.

To support #671, we now check if the file is already there (and if its size and lastModified timestamp is still the same) and if so, we don't restore it. This is a quick implementation and could be better in the sense that we could show the user how many existing files were retained instead of just claiming restore.

This MR also includes some other commits fixing some issues I encountered along the way. So best to review individual commits.

@grote grote force-pushed the dont-restore-existing-files branch from dc7f9d4 to b141c1d Compare August 16, 2024 21:14
@t-m-w
Copy link
Collaborator

t-m-w commented Aug 21, 2024

The title says this won't restore "files that still exist unchanged", but the description says it won't restore files that already exist at all. Which behavior should I expect?

If the latter: I know this is a quick implementation, but in the future, it would be useful to either allow some kind of choice or to still restore files that differ, even if with a (1), especially if the user is intentionally trying to recover changed files (e.g. ransomware, if that were more of a thing on Android).

@grote
Copy link
Collaborator Author

grote commented Aug 21, 2024

The title says this won't restore "files that still exist unchanged", but the description says it won't restore files that already exist at all. Which behavior should I expect?

I clarified the description. If the file has changed, we still do the (1) restore.

If the latter: I know this is a quick implementation

Indeed, this is just to fix the biggest annoyance where most of your files would get duplicated if you restored again.

it would be useful to either allow some kind of choice or to still restore files that differ, even if with a (1), especially if the user is intentionally trying to recover changed files (e.g. ransomware, if that were more of a thing on Android).

I think we should always restore files that differ, so maybe no choice needed here?
If the user is only after certain files, they can now use the new file restore chooser.

@grote grote force-pushed the dont-restore-existing-files branch from b141c1d to a2189bc Compare August 22, 2024 12:36
Copy link
Collaborator

@t-m-w t-m-w left a comment

Choose a reason for hiding this comment

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

This looks like it's working! One observation, not a problem really: if you have a different variant of a file locally, then the one from the backup is restored with a (1) or (2), etc on the end, as expected. If you restore the same files backup again, although you already have the restored file as (1), it will restore again as a duplicate of (1) as (2). Again, just thought I'd mention for completeness, but not really unexpected behavior.

Tested deleting a file, and it came back. Tested changing a file, and it was restored with a number on the end. Tested creating a new file, and it was not removed.

@grote grote merged commit 5418a8e into seedvault-app:android14 Aug 22, 2024
1 check passed
@grote grote deleted the dont-restore-existing-files branch August 22, 2024 20:39
@grote
Copy link
Collaborator Author

grote commented Aug 22, 2024

Thanks for testing!

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