Skip to content

Cleanup snapshot filter options#3912

Merged
rawtaz merged 3 commits intorestic:masterfrom
MichaelEischer:cleanup-snapshot-filter-options
Sep 10, 2022
Merged

Cleanup snapshot filter options#3912
rawtaz merged 3 commits intorestic:masterfrom
MichaelEischer:cleanup-snapshot-filter-options

Conversation

@MichaelEischer
Copy link
Copy Markdown
Member

@MichaelEischer MichaelEischer commented Sep 3, 2022

What does this PR change? What problem does it solve?

This PR cleans up the help text for the --host, --tag and --path options used to filter snapshots. It explicitly does not cleanup up the snapshot filter code itself, that has to wait until some later PR (I've started some experiments in that direction in https://github.com/MichaelEischer/restic/tree/layered-repo2).

The commands backup, cat and diff are not affected by this PR as these use a special way to resolve the snapshot (backup) or just process a single specific snapshot ID (i.e. latest is not allowed).

The remaining commands be grouped into 3 categories:

  • Using FindFilteredSnapshots: copy, find, forget, snapshots, stats, tag. The filtering options apply either when no snapshots are specified or when latest is given.
  • Using restic.FindFilteredSnapshots: mount. The filtering options are used to limit the shown snapshots.
  • Others: ls (FindFilteredSnapshots but limited to exactly a single snapshot), dump, restore (FindSnapshot/FindLatestSnapshot). The filtering options apply when latest is given.

The previous state of the help texts is as shown below:

copy     "host", "H", nil, "only consider snapshots for this `host`, when no snapshot ID is given (can be specified multiple times)")
find     "host", "H", nil, "only consider snapshots for this `host`, when no snapshot ID is given (can be specified multiple times)")
forget   "host",      nil, "only consider snapshots for this `host` (can be specified multiple times)")
snapshot "host", "H", nil, "only consider snapshots for this `host` (can be specified multiple times)")
stats    "host", "H", nil, "only consider snapshots for this `host` (can be specified multiple times)")
tag      "host", "H", nil, "only consider snapshots for this `host`, when no snapshot ID is given (can be specified multiple times)")

mount    "host", "H", nil, "only consider snapshots for this `host` (can be specified multiple times)")

ls       "host", "H", nil, "only consider snapshots for this `host`, when snapshot ID \"latest\" is given (can be specified multiple times)")
dump     "host", "H", nil, `only consider snapshots for this host when the snapshot ID is "latest" (can be specified multiple times)`)
restore  "host", "H", nil, "only consider snapshots for this `host`, when snapshot ID \"latest\" is given (can be specified multiple times)")

I've normalized this to only consider snapshots for this 'host' (can be specified multiple times) (for the first two categories) and only consider snapshots for this 'host', when snapshot ID \"latest\" is given (can be specified multiple times) (for the last category). The claim that the host filter only applies when no snapshot ID is given to copy, find and tag is wrong. We could try to explain the "no snapshots"/"latest" cases though, but that would result in a rather long description.

copy     "tag", "only consider snapshots which include this `taglist`, when no snapshot ID is given")
find     "tag", "only consider snapshots which include this `taglist`, when no snapshot-ID is given")
forget   "tag", "only consider snapshots which include this `taglist` in the format `tag[,tag,...]` (can be specified multiple times)")
snapshot "tag", "only consider snapshots which include this `taglist` in the format `tag[,tag,...]` (can be specified multiple times)")
stats    "tag", "only consider snapshots which include this `taglist` in the format `tag[,tag,...]` (can be specified multiple times)")
tag      "tag", "only consider snapshots which include this `taglist`, when no snapshot-ID is given")

mount    "tag", "only consider snapshots which include this `taglist`")

ls       "tag", "only consider snapshots which include this `taglist`, when snapshot ID \"latest\" is given (can be specified multiple times)")
dump     "tag", "only consider snapshots which include this `taglist` for snapshot ID \"latest\"")
restore  "tag", "only consider snapshots which include this `taglist` for snapshot ID \"latest\"")

Is now only consider snapshots which include this 'taglist' in the format 'tag[,tag,...]' (can be specified multiple times) and only consider snapshots which include this 'taglist', when snapshot ID \"latest\" is given (can be specified multiple times). The reasoning is similar as above. For dump and restore the explanation was missing that the option can be specified multiple times.

copy     "path", nil, "only consider snapshots which include this (absolute) `path`, when no snapshot ID is given")
find     "path", nil, "only consider snapshots which include this (absolute) `path`, when no snapshot-ID is given")
forget   "path", nil, "only consider snapshots which include this (absolute) `path` (can be specified multiple times)")
snapshot "path", nil, "only consider snapshots for this `path` (can be specified multiple times)")
stats    "path", nil, "only consider snapshots which include this (absolute) `path` (can be specified multiple times)")
tag      "path", nil, "only consider snapshots which include this (absolute) `path`, when no snapshot-ID is given")

mount    "path", nil, "only consider snapshots which include this (absolute) `path`")

ls       "path", nil, "only consider snapshots which include this (absolute) `path`, when snapshot ID \"latest\" is given (can be specified multiple times)")
dump     "path", nil, "only consider snapshots which include this (absolute) `path` for snapshot ID \"latest\"")
restore  "path", nil, "only consider snapshots which include this (absolute) `path` for snapshot ID \"latest\"")

And finally only consider snapshots which include this (absolute) 'path' (can be specified multiple times) and only consider snapshots which include this (absolute) 'path', when snapshot ID \"latest\" is given (can be specified multiple times). Now all commands ask for an absolute path, although this is (currently) not strictly accurate, it can avoid problems. Depending on the snapshot filtering method relative paths are processed different leading to unexpected behavior. For example filtering snapshots by paths won't work between Windows and Unix-like systems. Thus, I plan to drop the handling of relative paths completely.

Was the change previously discussed in an issue or on the forum?

Fixes https://forum.restic.net/t/restic-0-14-flags-syntax/5387

Checklist

  • I have read the contribution guidelines.
  • I have enabled maintainer edits.
  • I have added tests for all code changes.
  • I have added documentation for relevant changes (in the manual).
  • [ ] There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • 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.

@MichaelEischer MichaelEischer force-pushed the cleanup-snapshot-filter-options branch from 97317c2 to d630996 Compare September 4, 2022 08:27
@MichaelEischer MichaelEischer force-pushed the cleanup-snapshot-filter-options branch from a7ad368 to 381da04 Compare September 10, 2022 21:55
@MichaelEischer
Copy link
Copy Markdown
Member Author

MichaelEischer commented Sep 10, 2022

Credit for the latest description tweaks belongs to @rawtaz .

rawtaz
rawtaz approved these changes Sep 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rawtaz rawtaz left a comment

Choose a reason for hiding this comment

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

LGTM!

@rawtaz rawtaz merged commit 14d09a6 into restic:master Sep 10, 2022
@rawtaz rawtaz mentioned this pull request Sep 10, 2022
Merged
8 tasks
@MichaelEischer MichaelEischer deleted the cleanup-snapshot-filter-options branch September 11, 2022 13:29
@MichaelEischer MichaelEischer mentioned this pull request Oct 3, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rawtaz rawtaz rawtaz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaelEischer @rawtaz