Add --tag filtering to every command, where applicable #866
Conversation
src/cmds/restic/find.go
Outdated
| } | ||
| } | ||
| return | ||
| } else { |
There was a problem hiding this comment.
if block ends with a return statement, so drop this else and outdent its block
|
Hm. I appreciate the switch to |
src/cmds/restic/cmd_find.go
Outdated
| err := findInSnapshot(repo, pat, snapshotID) | ||
|
|
||
| if err != nil { | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Use context.TODO() instead of context.Background(), the latter should only be used in the main() (or equivalent). The former is made specifically to allow refactoring a code base piece by piece and easily find the places where a new context has been introduced that'd need to be derived from a parent context.
There was a problem hiding this comment.
Uhm. This is main() or equivalent. Unless cobra is going to supply us with a context, this is as high in the tree as we can get. So context.Background() felt like the right choice there.
For me, context.TODO() would appear in say refactoring the backends, when the restic library itself does not plumb the context through, so you would have to make one up.
There was a problem hiding this comment.
Hm, I'd rather pass down a context from main(), so we can have the cleanup handlers cancel that context. And I think in main() is the right place for this to do (as we only need to do it once and not in each function). So I still think context.TODO() is the better choice here. But that should not be a merge blocker for now ;)
There was a problem hiding this comment.
Done. I had to add it to the GlobalOptions, but at least that makes it accessable to all cmd's.
...
Oh, I see what you did there. You want to pass a context.WithCancel context down. Nice. Not super useful right now, but nice. Done.
src/cmds/restic/cmd_find.go
Outdated
| if err != nil { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| for sn := range FindFilteredSnapshots(ctx, repo, opts.Host, opts.Tags, opts.Paths, opts.Snapshots) { |
There was a problem hiding this comment.
Maybe bundle the filter criteria for snapshots into a new struct, e.g.:
type SnapshotFilter struct {
Host string
Tags []string
Paths []string
IDs []restic.ID
}
src/cmds/restic/cmd_forget.go
Outdated
| @@ -55,8 +55,9 @@ func init() { | |||
| f.IntVarP(&forgetOptions.Yearly, "keep-yearly", "y", 0, "keep the last `n` yearly snapshots") | |||
|
|
|||
| f.StringSliceVar(&forgetOptions.KeepTags, "keep-tag", []string{}, "always keep snapshots with this `tag` (can be specified multiple times)") | |||
| f.StringVar(&forgetOptions.Hostname, "hostname", "", "only forget snapshots for the given hostname") | |||
| f.StringSliceVar(&forgetOptions.Tags, "tag", []string{}, "only forget snapshots with the `tag` (can be specified multiple times)") | |||
| f.StringVar(&forgetOptions.Host, "host", "", "only consider snapshots for this `host`") | |||
There was a problem hiding this comment.
Same comment as in the other PR: we're changing from --hostname to --host, we need to find a workaround for people using --hostname in their scripts.
There was a problem hiding this comment.
Ack, but... I made the change in the other PR, so if that gets submitted, I can resolve the conflicts (it's going to be more than just this one.)
There was a problem hiding this comment.
I actually rebased this tree on fix-841. So it should be fine now.
src/cmds/restic/cmd_forget.go
Outdated
| } else { | ||
| Verbosef("would remove snapshot %v\n", id.Str()) | ||
| // Not cool, we lose the benefit of yielding, e.g. only one snapshot in memory. |
There was a problem hiding this comment.
What is this comment about? What should it tell future readers of this source code?
There was a problem hiding this comment.
That using a channel to hand over a Snapshot one by one is completely defeated by putting them all in a list. But if you don't like the remainder, I can certainly make it go the way of the dodo's ^^
Codecov Report
@@ Coverage Diff @@
## master #866 +/- ##
==========================================
+ Coverage 53.36% 53.48% +0.12%
==========================================
Files 92 92
Lines 7603 7565 -38
==========================================
- Hits 4057 4046 -11
+ Misses 2931 2909 -22
+ Partials 615 610 -5
Continue to review full report at Codecov.
|
|
Regarding the use of the old context in fuse. I don't think it matters much, they call us with a context and none of the restics fuse code uses it. Then I found this: So while not having a definite answer, it seems not a super big issue out there. |
|
Ah, thanks for looking it up, I forgot that |
There was a problem hiding this comment.
I like what you did here, good work!
Some things still need to be done, from my point of view these are:
- Add the new requirement for Go 1.7 to the readme and the manual
- Add
--tagto thesnapshotscommand, that seems to be missing - Allow passing in a snapshot ID prefix (add a call to
restic.FindSnapshot()for each ID
src/cmds/restic/find.go
Outdated
| ) | ||
|
|
||
| // FindFilteredSnapshots yields Snapshots, either given explicitly by `args` or filtered from the list of all snapshots. | ||
| func FindFilteredSnapshots(ctx context.Context, repo *repository.Repository, host string, tags []string, paths []string, args []string) <-chan *restic.Snapshot { |
There was a problem hiding this comment.
I'd suggest renaming args to snapshotIDs so it becomes clear what should be in there from the function signature.
There was a problem hiding this comment.
I named then ids as it is not sure yet they are actual snapshotIDs
src/cmds/restic/find.go
Outdated
| continue | ||
| } | ||
| } else { | ||
| id, err = restic.ParseID(s) |
There was a problem hiding this comment.
The code here now requires passing a complete (non abbreviated) snapshot ID, that's not a good user experience:
$ bin/restic snapshots
ID Date Host Tags Directory
----------------------------------------------------------------------
4b122706 2017-03-07 11:11:52 mopped /home/fd0/shared/work/restic/restic
$ bin/restic tag --add foo 4b122706
Create exclusive lock for repository
Ignoring "4b122706", it is not a snapshot id
No snapshots were modified
$ bin/restic list snapshots
4b12270680254642a8fe140dba8e7ede140e40af855899768133f4e01c0c875e
$ bin/restic tag --add foo 4b12270680254642a8fe140dba8e7ede140e40af855899768133f4e01c0c875e
Create exclusive lock for repository
Modified tags on 1 snapshots
There's restic.FindSnapshot() which finds the snapshot ID for from a prefix, it was removed in 63db3045c2bb8661a41ebd357984df6794e62d0d.
There was a problem hiding this comment.
Args.... You are so right.
I think some rebase mucked up my code. Both here and cmd_snapshot are weird. In fact, cmd_snapshot looks rather pristine. :(
Fixed.
|
Addressed your latest comments (i think; it getting hard to see...) |
There was a problem hiding this comment.
Ok, looks great!
Still open things:
- Add proper error handling for
json.Marshal() - What about my idea of bundling together filter criteria in a separate struct? Future work?
- The option
--group-by-tagsis on by default (-> bug in the code, see comment there) - The
findcommand does not print the snapshot the results are found in any more, was that intentional? Sample usage for find on a repo with several snapshots:
$ bin/restic find Dockerfile
/restic/Dockerfile
/restic/Dockerfile
- The
lscommand used to return an error when no snapshot IDs are given, now it lists ALL snapshots. Bug or feature?
src/cmds/restic/cmd_forget.go
Outdated
| } | ||
| sort.StringSlice(sn.Paths).Sort() | ||
| k, _ := json.Marshal(key{Hostname: sn.Hostname, Tags: sn.Tags, Paths: sn.Paths}) |
There was a problem hiding this comment.
Uh, you're throwing away an error here, that's not a good idea. I agree that there shouldn't be an error most of the time, but if there is one, json.Marshal() may return an empty buffer, so string(buf) will be the empty string, which means that all snapshots are sorted into the same group. That's not a good idea, especially not for the forget command...
In addition, there's a bug in this line: use tags instead of sn.Tags.
There was a problem hiding this comment.
Yeah, both fixed. Thanks!
| @@ -49,6 +51,13 @@ func init() { | |||
| globalOptions.password = pw | |||
| } | |||
|
|
|||
| var cancel context.CancelFunc | |||
| globalOptions.ctx, cancel = context.WithCancel(context.Background()) | |||
|
So, I went through the code and listed all open issues in the comment above. I know that it's hard to do, but it would be great if you could split such huge changes into sensible smaller commits. Reviewing one huge commit is really hard if so much is changed at once... Maybe have a look at how to rework and merge commits with |
$ restic -r rest:http://qnap.**.********.**:8000 find -l bin
Found matching entries in snapshot 0a4ce6e3
Lrwxrwxrwx 0 0 0 2016-12-14 12:54:30 /bin -> usr/bin
dr-xr-xr-x 0 0 0 2016-12-14 12:59:29 /usr/bin
Lrwxrwxrwx 0 0 0 2016-07-03 14:41:12 /usr/lib/debug/bin -> usr/bin
drwxr-xr-x 0 0 0 2016-07-03 14:41:12 /usr/lib/debug/usr/bin
drwxr-xr-x 0 0 0 2016-07-03 14:44:42 /usr/lib64/pm-utils/bin
drwxr-xr-x 0 0 0 2016-11-05 16:38:36 /usr/local/bin
drwxr-xr-x 0 0 0 2016-11-05 16:38:37 /usr/share/locale/bin
Found matching entries in snapshot 0f65f72a
Lrwxrwxrwx 0 0 0 2016-12-14 10:23:13 /bin -> usr/bin
drwxrwxr-x 1000 1000 0 2016-10-13 20:53:56 /home/middelin/.atom/packages/build/node_modules/cross-spawn-async/node_modules/which/bin
drwxrwxr-x 1000 1000 0 2016-10-13 20:53:56 /home/middelin/.atom/packages/build/node_modules/cson-parser/node_modules/coffee-script/bin
drwxrwxr-x 1000 1000 0 2016-10-13 20:53:56 /home/middelin/.atom/packages/build/node_modules/getmac/bin
drwxrwxr-x 1000 1000 0 2016-10-13 20:53:56 /home/middelin/.atom/packages/build/node_modules/js-yaml/bin
drwxrwxr-x 1000 1000 0 2016-10-13 20:53:56 /home/middelin/.atom/packages/build/node_modules/js-yaml/node_modules/esprima/bin
...
Not sure. I can make a case for either ^^
Alright, lets not rock the boat too much. I put in a check for it.
Uhm. I don't like it very much. First it will make the call to the function not at all easier to understand:
Ouch. So one PR with a commit for say find.go and then all the cmd_xxx's separate? Aside some ordering issues where I reuse some functions from other commands, I think it should be do-able. Or did you mean a PR per commit? If you want smaller reviews, I think you meant that one... |
Add `tags` argument to `FindLatestSnapshot()`
Set up a cancelble context in global options, hook it into the ctrl-C handler for proper cancel propegation. Bump up minimal requirement for Go to version 1.7 in documentation and test-build files.
Hm, I think it's way easier to read: FindFilteredSnapshots(ctx, repo, opts.Host, opts.Tags, opts.Paths, args)versus: f:= restic.Filter{Host:opts.Host, Tags:opts.Tags, Paths:opts.Paths, SnapshotIDs: args}
FindFilteredSnapshots(ctx, repo, f)
And for this reason I've for example used this pattern for the snapshot filter for I don't like functions with long argument lists, I'd rather prefer to group them into logical chunks via custom types.
That is indeed true, but:
That's not necessary, there's |
|
But we can also leave the filter thing as an optimization for the future. |
This helper function takes a set of filters and/or a list of snapshots from the commandline. It returns a channel of *Snapshot. When snapshot ids are given, they are checked for validity and their corresponding Snapshots returned. The snapshot id "latest" is handled special to return either the last snapshot (no filters) or the last snapshot matching the filters. When no arguments are given, the filters are applied over all available snapshots and these are returned.
Implement filtering by using `FindFilteredSnapshots()` to iterate over the snapshots Refactor cmd_snapshots' `PrintSnapshots()` so its pretty printing can be used from both `forget` and `snapshots`. Use contexts.
Implement filtering by using `FindFilteredSnapshots()` to iterate over the snapshots Refactor cmd_ls' `PrintNode()` into format.go, reuse its pretty printing in both `find` and `ls` commands. Use contexts.
Factor out and reuse `rebuildIndex()` in cmd_rebuild_index and cmd_prune. Use contexts.
Add --tag filtering to every command, where applicable
|
Thanks, merged! |
Add helper function
FindFilteredSnapshots()to handle the bulk of iteratingover the snapshots and perform filtering.
Started using contexts (which means minimal requirement is now Go 1.7)
Fixed long silent runs of
findby outputting results as we find them.(We lost the ability to print how many matches we found however.)
Made
mountmore interesting, it now applies filters when filling thesnapshotsdirectory.Closes #863