Skip to content

Fix restore filter#218

Merged
fd0 merged 4 commits intomasterfrom
fix-202
Jul 9, 2015
Merged

Fix restore filter#218
fd0 merged 4 commits intomasterfrom
fix-202

Conversation

@fd0
Copy link
Copy Markdown
Member

@fd0 fd0 commented Jul 6, 2015

Internally rename restorer.Filter -> restorer.SelectForRestore to make
semantic clear.

In addition, swap parameters to filepath.Match() so that the pattern can
really be matched.

Limitation: The filter only works on the filename, not on any path
component, e.g. *.go selects all go files, subdir/foo* doesn't
select anything.

This fixes #202.

restorer.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I kinda feel like the filtering should be up to the caller and not happen in restoreNodeTo. Feels like this is not this method's responsibility.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed.

@fw42
Copy link
Copy Markdown
Member

fw42 commented Jul 7, 2015

Limitation: The filter only works on the filename

Why is that?

@fw42
Copy link
Copy Markdown
Member

fw42 commented Jul 7, 2015

not a fan of all this debug.Log clutter :-( is that really that helpful?

@fd0
Copy link
Copy Markdown
Member Author

fd0 commented Jul 8, 2015

The limitation comes from using filepath.Match, which doesn't handle separator chars / well, see https://godoc.org/path/filepath#Match. I'm open to suggestions with what to replace filepath.Match with, so that the fiter function is improved, regular expressions feels like overkill...

The debug.Log stuff helps a lot, in my opinion. This is how I debugged it, and I think it's good to leave it in. This allows users (who are able to recompile restic with -tags debug) to create a really verbose logfile of all the internal things restic does, so we can remotely figure out what went wrong.

Do you have a better idea?

@fd0
Copy link
Copy Markdown
Member Author

fd0 commented Jul 8, 2015

I've moved the filter selection to restoreTo as you suggested. We need a test for this functionality, afterwards the PR can be merged I think. I can live with the file name filter limitation for now, and I've created #223 for it so we can track the progress.

Any other thoughts?

restorer.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe it would be nicer to initialize res.SelectForRestore to func(...) { return true }, then you wouldn't have to do this kind of check... dunno if that's necessarily better. just an idea.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hm, I like that. I suppose this doesn't introduce any performance problems.

@fw42
Copy link
Copy Markdown
Member

fw42 commented Jul 8, 2015

👍 Looks good. Tests would be A+.

@fd0
Copy link
Copy Markdown
Member Author

fd0 commented Jul 9, 2015

Test has been added, this can be merged. I'll rebase it to master and run travis again.

fd0 added 4 commits July 9, 2015 20:12
Internally rename restorer.Filter -> restorer.SelectForRestore to make
semantic clear.

In addition, swap parameters to filepath.Match() so that the pattern can
really be matched.

Limitation: The filter only works on the filename, not on any path
component, e.g. '*.go' selects all go files, 'subdir/foo*' doesn't
select anything.

Fixes #202.
@fd0 fd0 merged commit 389ec9b into master Jul 9, 2015
fd0 added a commit that referenced this pull request Jul 9, 2015
@fd0 fd0 deleted the fix-202 branch July 9, 2015 20:42
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.

Restore filter is broken

2 participants