Refactor group-by to parse options into a struct

cmd/restic/cmd_forget.go

@@ -56,7 +56,7 @@ type ForgetOptions struct {
 	Compact bool
 
 	// Grouping
-	GroupBy string
+	GroupBy restic.SnapshotGroupByOptions
 	DryRun  bool
 	Prune   bool
 }
@@ -90,8 +90,8 @@ func init() {
 	}
 
 	f.BoolVarP(&forgetOptions.Compact, "compact", "c", false, "use compact output format")
-
+	forgetOptions.GroupBy = restic.SnapshotGroupByOptions{Host: true, Path: true}
-	f.StringVarP(&forgetOptions.GroupBy, "group-by", "g", "host,paths", "`group` snapshots by host, paths and/or tags, separated by comma (disable grouping with '')")
+	f.VarP(&forgetOptions.GroupBy, "group-by", "g", "`group` snapshots by host, paths and/or tags, separated by comma (disable grouping with '')")
 	f.BoolVarP(&forgetOptions.DryRun, "dry-run", "n", false, "do not delete anything, just print what would be done")
 	f.BoolVar(&forgetOptions.Prune, "prune", false, "automatically run the 'prune' command if snapshots have been removed")
 

cmd/restic/cmd_snapshots.go

@@ -36,7 +36,7 @@ type SnapshotOptions struct {
 	Compact bool
 	Last    bool // This option should be removed in favour of Latest.
 	Latest  int
-	GroupBy string
+	GroupBy restic.SnapshotGroupByOptions
 }
 
 var snapshotOptions SnapshotOptions
@@ -54,7 +54,7 @@ func init() {
 		panic(err)
 	}
 	f.IntVar(&snapshotOptions.Latest, "latest", 0, "only show the last `n` snapshots for each host and path")
-	f.StringVarP(&snapshotOptions.GroupBy, "group-by", "g", "", "`group` snapshots by host, paths and/or tags, separated by comma")
+	f.VarP(&snapshotOptions.GroupBy, "group-by", "g", "`group` snapshots by host, paths and/or tags, separated by comma")
 }
 
 func runSnapshots(ctx context.Context, opts SnapshotOptions, gopts GlobalOptions, args []string) error {

internal/restic/snapshot_group.go

@@ -8,6 +8,57 @@ import (
 	"github.com/restic/restic/internal/errors"
 )
 
+type SnapshotGroupByOptions struct {
+	Tag  bool
+	Host bool
+	Path bool
+}
+
+func splitSnapshotGroupBy(s string) (SnapshotGroupByOptions, error) {
+	var l SnapshotGroupByOptions
+	for _, option := range strings.Split(s, ",") {
+		switch option {
+		case "host", "hosts":
+			l.Host = true
+		case "path", "paths":
+			l.Path = true
+		case "tag", "tags":
+			l.Tag = true
+		case "":
+		default:
+			return SnapshotGroupByOptions{}, errors.Fatal("unknown grouping option: '" + option + "'")
+		}
+	}
+	return l, nil
+}
+
+func (l SnapshotGroupByOptions) String() string {
+	var parts []string
+	if l.Host {
+		parts = append(parts, "host")
+	}
+	if l.Path {
+		parts = append(parts, "paths")
+	}
+	if l.Tag {
+		parts = append(parts, "tags")
+	}
+	return strings.Join(parts, ",")
+}
+
+func (l *SnapshotGroupByOptions) Set(s string) error {
+	parts, err := splitSnapshotGroupBy(s)
+	if err != nil {
+		return err
+	}
+	*l = parts
+	return nil
+}
+
+func (l *SnapshotGroupByOptions) Type() string {
+	return "group"
+}
+
 // SnapshotGroupKey is the structure for identifying groups in a grouped
 // snapshot list. This is used by GroupSnapshots()
 type SnapshotGroupKey struct {
@@ -18,43 +69,24 @@ type SnapshotGroupKey struct {
 
 // GroupSnapshots takes a list of snapshots and a grouping criteria and creates
 // a group list of snapshots.
-func GroupSnapshots(snapshots Snapshots, options string) (map[string]Snapshots, bool, error) {
+func GroupSnapshots(snapshots Snapshots, groupBy SnapshotGroupByOptions) (map[string]Snapshots, bool, error) {
 	// group by hostname and dirs
 	snapshotGroups := make(map[string]Snapshots)
 
-	var GroupByTag bool
-	var GroupByHost bool
-	var GroupByPath bool
-	GroupOptionList := strings.Split(options, ",")
-
-	for _, option := range GroupOptionList {
-		switch option {
-		case "host", "hosts":
-			GroupByHost = true
-		case "path", "paths":
-			GroupByPath = true
-		case "tag", "tags":
-			GroupByTag = true
-		case "":
-		default:
-			return nil, false, errors.Fatal("unknown grouping option: '" + option + "'")
-		}
-	}
-
 	for _, sn := range snapshots {
 		// Determining grouping-keys
 		var tags []string
 		var hostname string
 		var paths []string
 
-		if GroupByTag {
+		if groupBy.Tag {
 			tags = sn.Tags
 			sort.Strings(tags)
 		}
-		if GroupByHost {
+		if groupBy.Host {
 			hostname = sn.Hostname
 		}
-		if GroupByPath {
+		if groupBy.Path {
 			paths = sn.Paths
 		}
 
@@ -70,5 +102,5 @@ func GroupSnapshots(snapshots Snapshots, options string) (map[string]Snapshots,
 		snapshotGroups[string(k)] = append(snapshotGroups[string(k)], sn)
 	}
 
-	return snapshotGroups, GroupByTag || GroupByHost || GroupByPath, nil
+	return snapshotGroups, groupBy.Tag || groupBy.Host || groupBy.Path, nil
 }

internal/restic/snapshot_group_test.go

@@ -0,0 +1,50 @@
+package restic_test
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/restic/restic/internal/restic"
+	"github.com/restic/restic/internal/test"
+)
+
+func TestGroupByOptions(t *testing.T) {
+	for _, exp := range []struct {
+		from       string
+		opts       restic.SnapshotGroupByOptions
+		normalized string
+	}{
+		{
+			from:       "",
+			opts:       restic.SnapshotGroupByOptions{},
+			normalized: "",
+		},
+		{
+			from:       "host,paths",
+			opts:       restic.SnapshotGroupByOptions{Host: true, Path: true},
+			normalized: "host,paths",
+		},
+		{
+			from:       "host,path,tag",
+			opts:       restic.SnapshotGroupByOptions{Host: true, Path: true, Tag: true},
+			normalized: "host,paths,tags",
+		},
+		{
+			from:       "hosts,paths,tags",
+			opts:       restic.SnapshotGroupByOptions{Host: true, Path: true, Tag: true},
+			normalized: "host,paths,tags",
+		},
+	} {
+		var opts restic.SnapshotGroupByOptions
+		test.OK(t, opts.Set(exp.from))
+		if !cmp.Equal(opts, exp.opts) {
+			t.Errorf("unexpeted opts %s", cmp.Diff(opts, exp.opts))
+		}
+		test.Equals(t, opts.String(), exp.normalized)
+	}
+
+	var opts restic.SnapshotGroupByOptions
+	err := opts.Set("tags,invalid")
+	test.Assert(t, err != nil, "missing error on invalid tags")
+	test.Assert(t, !opts.Host && !opts.Path && !opts.Tag, "unexpected opts %s %s %s", opts.Host, opts.Path, opts.Tag)
+}