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

restorer: Add test for restore with include filter #1900

Merged
merged 5 commits into from
Jul 23, 2018
Merged

restorer: Add test for restore with include filter #1900

merged 5 commits into from
Jul 23, 2018

Conversation

fd0
Copy link
Member

@fd0 fd0 commented Jul 21, 2018

What is the purpose of this change? What does it change?

Resolve an error with the restore command when an include filter is used.

Background:

traverseTree() was meant to call enterDir() whenever a directory is selected for restore, either explicitly or implicitly (=contains a file which is to be restored). After restoring a file, leaveDir() is called in reverse order for all intermediate directories so that the metadata can be restored.

When a directory is selected implicitly, the metadata for it is restored. This is different from the previous restorer behavior, which created implicitly selected intermediate directories with permissions 0700 (only user can read/write it).

This PR changes the behavior back to the old one. Only a directory is explicitly selected for restore, enterDir()/leaveDir() are called for it. Otherwise, only visitNode() is called, so visitNode() needs to make sure the parent directory exists. If the directory is explicitly included, leaveDir() will then restore the metadata correctly.

When we decide to change the behavior (restore metadata for all intermediate directories, even if selected implicitly), we should do that in the selection functions, not here.

This finally resolves #1870

Was the change discussed in an issue or in the forum before?

Closes #1870

Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@fd0 fd0 force-pushed the fix-1870 branch 3 times, most recently from e66aee9 to b102959 Compare July 21, 2018 20:45
@fd0 fd0 requested a review from ifedorenko July 21, 2018 20:47
fd0 added 5 commits July 21, 2018 23:24
traverseTree() was meant to call enterDir() whenever a directory is
selected for restore, either explicitly or implicitly (=contains a file
which is to be restored). After restoring a file, leaveDir() is called
in reverse order for all intermediate directories so that the metadata
can be restored.

When a directory is selected implicitly, the metadata for it is
restored. This is different from the previous restorer behavior, which
created implicitly selected intermediate directories with permissions
0700 (only user can read/write it).

This commit changes the behavior back to the old one. Only a directory
is explicitly selected for restore, enterDir()/leaveDir() are called for
it. Otherwise, only visitNode() is called, so visitNode() needs to make
sure the parent directory exists. If the directory is explicitly
included, leaveDir() will then restore the metadata correctly.

When we decide to change the behavior (restore metadata for all
intermediate directories, even if selected implicitly), we should do
that in the selection functions, not here.

This finally resolves #1870
@codecov-io
Copy link

Codecov Report

Merging #1900 into master will increase coverage by 0.09%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1900      +/-   ##
==========================================
+ Coverage   51.06%   51.15%   +0.09%     
==========================================
  Files         169      169              
  Lines       13097    13083      -14     
==========================================
+ Hits         6688     6693       +5     
+ Misses       5401     5383      -18     
+ Partials     1008     1007       -1
Impacted Files Coverage Δ
internal/restorer/restorer.go 45.38% <66.66%> (+3.71%) ⬆️
internal/backend/rclone/backend.go 64.32% <0%> (-1.76%) ⬇️
internal/archiver/file_saver.go 88.59% <0%> (+2.63%) ⬆️
internal/archiver/blob_saver.go 100% <0%> (+7.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67535e0...44924ba. Read the comment docs.

@ifedorenko
Copy link
Contributor

ifedorenko commented Jul 22, 2018

Minor. I think there is no need to precreate dst directory in RestoreTo any more. By removing that code all created directories will default to the same 0700 permissions, which is both safer and more consistent. /~https://github.com/restic/restic/pull/1900/files#diff-7e901d641519cb52e213a5d339ee6d65L200

I also wonder if it'll be more "symmetrical" if directories and other node types were created in visitNode. This way it should be possible to remove enterDir, which would simplify the code. I can open follow-up PR to illustrate this, if you are interested.

Update: I tried removing enterDir and don't like the result.

@fd0
Copy link
Member Author

fd0 commented Jul 22, 2018

I think we should either isolate the (fairly complex) treeVisitor from the restorer, or replace it with the recently introduced walker.Walk(). I'll give this a try so you can have a look.

@fd0 fd0 merged commit 44924ba into master Jul 23, 2018
fd0 added a commit that referenced this pull request Jul 23, 2018
restorer: Add test for restore with include filter
@fd0 fd0 deleted the fix-1870 branch July 23, 2018 18:16
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.

Error in restoring a specific file
3 participants