From 36d8916354f7e0bf10aeb4639246cbb8e392a89b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 3 Feb 2025 21:52:57 +0100 Subject: [PATCH 1/5] ls: use numeric based enum for SortMode --- cmd/restic/cmd_ls.go | 61 +++++++++++++++++---------- cmd/restic/cmd_ls_integration_test.go | 12 +++--- 2 files changed, 46 insertions(+), 27 deletions(-) diff --git a/cmd/restic/cmd_ls.go b/cmd/restic/cmd_ls.go index 6e0d230b1..d12dd3a31 100644 --- a/cmd/restic/cmd_ls.go +++ b/cmd/restic/cmd_ls.go @@ -66,7 +66,7 @@ type LsOptions struct { Recursive bool HumanReadable bool Ncdu bool - Sort string + Sort SortMode Reverse bool } @@ -81,7 +81,7 @@ func init() { flags.BoolVar(&lsOptions.Recursive, "recursive", false, "include files in subfolders of the listed directories") flags.BoolVar(&lsOptions.HumanReadable, "human-readable", false, "print sizes in human readable format") flags.BoolVar(&lsOptions.Ncdu, "ncdu", false, "output NCDU export format (pipe into 'ncdu -f -')") - flags.StringVarP(&lsOptions.Sort, "sort", "s", "name", "sort output by (name|size|time=mtime|atime|ctime|extension)") + flags.VarP(&lsOptions.Sort, "sort", "s", "sort output by (name|size|time=mtime|atime|ctime|extension)") flags.BoolVar(&lsOptions.Reverse, "reverse", false, "reverse sorted output") } @@ -301,19 +301,13 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri if opts.Ncdu && gopts.JSON { return errors.Fatal("only either '--json' or '--ncdu' can be specified") } - if opts.Sort != "name" && opts.Ncdu { + if opts.Sort != SortModeName && opts.Ncdu { return errors.Fatal("--sort and --ncdu are mutually exclusive") } if opts.Reverse && opts.Ncdu { return errors.Fatal("--reverse and --ncdu are mutually exclusive") } - sortMode := SortModeName - err := sortMode.Set(opts.Sort) - if err != nil { - return err - } - // extract any specific directories to walk var dirs []string if len(args) > 1 { @@ -377,7 +371,7 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri var printer lsPrinter collector := []toSortOutput{} - outputSort := sortMode != SortModeName || opts.Reverse + outputSort := opts.Sort != SortModeName || opts.Reverse if gopts.JSON { printer = &jsonLsPrinter{ @@ -481,13 +475,13 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri } if outputSort { - printSortedOutput(printer, opts, sortMode, collector) + printSortedOutput(printer, opts.Sort, opts.Reverse, collector) } return printer.Close() } -func printSortedOutput(printer lsPrinter, opts LsOptions, sortMode SortMode, collector []toSortOutput) { +func printSortedOutput(printer lsPrinter, sortMode SortMode, reverse bool, collector []toSortOutput) { switch sortMode { case SortModeName: case SortModeSize: @@ -534,7 +528,7 @@ func printSortedOutput(printer lsPrinter, opts LsOptions, sortMode SortMode, col }) } - if opts.Reverse { + if reverse { slices.Reverse(collector) } for _, elem := range collector { @@ -543,17 +537,17 @@ func printSortedOutput(printer lsPrinter, opts LsOptions, sortMode SortMode, col } // SortMode defines the allowed sorting modes -type SortMode string +type SortMode uint // Allowed sort modes const ( - SortModeName SortMode = "name" - SortModeSize SortMode = "size" - SortModeAtime SortMode = "atime" - SortModeCtime SortMode = "ctime" - SortModeMtime SortMode = "mtime" - SortModeExt SortMode = "extension" - SortModeInvalid SortMode = "--invalid--" + SortModeName SortMode = iota + SortModeSize + SortModeAtime + SortModeCtime + SortModeMtime + SortModeExt + SortModeInvalid ) // Set implements the method needed for pflag command flag parsing. @@ -573,8 +567,31 @@ func (c *SortMode) Set(s string) error { *c = SortModeExt default: *c = SortModeInvalid - return fmt.Errorf("invalid sort mode %q, must be one of (name|size|atime|ctime|mtime=time|extension)", s) + return fmt.Errorf("invalid sort mode %q, must be one of (name|size|time=mtime|atime|ctime|extension)", s) } return nil } + +func (c *SortMode) String() string { + switch *c { + case SortModeName: + return "name" + case SortModeSize: + return "size" + case SortModeAtime: + return "atime" + case SortModeCtime: + return "ctime" + case SortModeMtime: + return "mtime" + case SortModeExt: + return "extension" + default: + return "invalid" + } +} + +func (c *SortMode) Type() string { + return "mode" +} diff --git a/cmd/restic/cmd_ls_integration_test.go b/cmd/restic/cmd_ls_integration_test.go index 29e153419..4675814b3 100644 --- a/cmd/restic/cmd_ls_integration_test.go +++ b/cmd/restic/cmd_ls_integration_test.go @@ -19,7 +19,7 @@ func testRunLsWithOpts(t testing.TB, gopts GlobalOptions, opts LsOptions, args [ } func testRunLs(t testing.TB, gopts GlobalOptions, snapshotID string) []string { - out := testRunLsWithOpts(t, gopts, LsOptions{Sort: "name"}, []string{snapshotID}) + out := testRunLsWithOpts(t, gopts, LsOptions{}, []string{snapshotID}) return strings.Split(string(out), "\n") } @@ -45,7 +45,7 @@ func TestRunLsNcdu(t *testing.T) { {"latest", "/0"}, {"latest", "/0", "/0/9"}, } { - ncdu := testRunLsWithOpts(t, env.gopts, LsOptions{Ncdu: true, Sort: "name"}, paths) + ncdu := testRunLsWithOpts(t, env.gopts, LsOptions{Ncdu: true}, paths) assertIsValidJSON(t, ncdu) } } @@ -83,7 +83,7 @@ func TestRunLsSort(t *testing.T) { testRunBackup(t, env.testdata+"/0", []string{"for_cmd_ls"}, opts, env.gopts) // sort by size - out := testRunLsWithOpts(t, env.gopts, LsOptions{Sort: "size"}, []string{"latest"}) + out := testRunLsWithOpts(t, env.gopts, LsOptions{Sort: SortModeSize}, []string{"latest"}) fileList := strings.Split(string(out), "\n") rtest.Assert(t, len(fileList) == 5, "invalid ls --sort size, expected 5 array elements, got %v", len(fileList)) for i, item := range compareSize { @@ -91,7 +91,7 @@ func TestRunLsSort(t *testing.T) { } // sort by file extension - out = testRunLsWithOpts(t, env.gopts, LsOptions{Sort: "extension"}, []string{"latest"}) + out = testRunLsWithOpts(t, env.gopts, LsOptions{Sort: SortModeExt}, []string{"latest"}) fileList = strings.Split(string(out), "\n") rtest.Assert(t, len(fileList) == 5, "invalid ls --sort extension, expected 5 array elements, got %v", len(fileList)) for i, item := range compareExt { @@ -99,10 +99,12 @@ func TestRunLsSort(t *testing.T) { } // explicit name sort - out = testRunLsWithOpts(t, env.gopts, LsOptions{Sort: "name"}, []string{"latest"}) + out = testRunLsWithOpts(t, env.gopts, LsOptions{Sort: SortModeName}, []string{"latest"}) fileList = strings.Split(string(out), "\n") rtest.Assert(t, len(fileList) == 5, "invalid ls --sort name, expected 5 array elements, got %v", len(fileList)) for i, item := range compareName { rtest.Assert(t, item == fileList[i], "invalid ls --sort name, expected element '%s', got '%s'", item, fileList[i]) } + + rtest.Equals(t, SortMode(0), SortModeName, "unexpected default sort mode") } From 993eb112cd344d216ec0178ab37b9da182e8f59e Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 3 Feb 2025 21:58:38 +0100 Subject: [PATCH 2/5] ls: deduplicate sorting test --- cmd/restic/cmd_ls_integration_test.go | 87 ++++++++++++--------------- 1 file changed, 40 insertions(+), 47 deletions(-) diff --git a/cmd/restic/cmd_ls_integration_test.go b/cmd/restic/cmd_ls_integration_test.go index 4675814b3..b9d565364 100644 --- a/cmd/restic/cmd_ls_integration_test.go +++ b/cmd/restic/cmd_ls_integration_test.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "fmt" "strings" "testing" @@ -51,29 +52,7 @@ func TestRunLsNcdu(t *testing.T) { } func TestRunLsSort(t *testing.T) { - compareName := []string{ - "/for_cmd_ls", - "/for_cmd_ls/file1.txt", - "/for_cmd_ls/file2.txt", - "/for_cmd_ls/python.py", - "", // last empty line - } - - compareSize := []string{ - "/for_cmd_ls", - "/for_cmd_ls/file2.txt", - "/for_cmd_ls/file1.txt", - "/for_cmd_ls/python.py", - "", - } - - compareExt := []string{ - "/for_cmd_ls", - "/for_cmd_ls/python.py", - "/for_cmd_ls/file1.txt", - "/for_cmd_ls/file2.txt", - "", - } + rtest.Equals(t, SortMode(0), SortModeName, "unexpected default sort mode") env, cleanup := withTestEnvironment(t) defer cleanup() @@ -82,29 +61,43 @@ func TestRunLsSort(t *testing.T) { opts := BackupOptions{} testRunBackup(t, env.testdata+"/0", []string{"for_cmd_ls"}, opts, env.gopts) - // sort by size - out := testRunLsWithOpts(t, env.gopts, LsOptions{Sort: SortModeSize}, []string{"latest"}) - fileList := strings.Split(string(out), "\n") - rtest.Assert(t, len(fileList) == 5, "invalid ls --sort size, expected 5 array elements, got %v", len(fileList)) - for i, item := range compareSize { - rtest.Assert(t, item == fileList[i], "invalid ls --sort size, expected element '%s', got '%s'", item, fileList[i]) + for _, test := range []struct { + mode SortMode + expected []string + }{ + { + SortModeSize, + []string{ + "/for_cmd_ls", + "/for_cmd_ls/file2.txt", + "/for_cmd_ls/file1.txt", + "/for_cmd_ls/python.py", + "", + }, + }, + { + SortModeExt, + []string{ + "/for_cmd_ls", + "/for_cmd_ls/python.py", + "/for_cmd_ls/file1.txt", + "/for_cmd_ls/file2.txt", + "", + }, + }, + { + SortModeName, + []string{ + "/for_cmd_ls", + "/for_cmd_ls/file1.txt", + "/for_cmd_ls/file2.txt", + "/for_cmd_ls/python.py", + "", // last empty line + }, + }, + } { + out := testRunLsWithOpts(t, env.gopts, LsOptions{Sort: test.mode}, []string{"latest"}) + fileList := strings.Split(string(out), "\n") + rtest.Equals(t, test.expected, fileList, fmt.Sprintf("mismatch for mode %v", test.mode)) } - - // sort by file extension - out = testRunLsWithOpts(t, env.gopts, LsOptions{Sort: SortModeExt}, []string{"latest"}) - fileList = strings.Split(string(out), "\n") - rtest.Assert(t, len(fileList) == 5, "invalid ls --sort extension, expected 5 array elements, got %v", len(fileList)) - for i, item := range compareExt { - rtest.Assert(t, item == fileList[i], "invalid ls --sort extension, expected element '%s', got '%s'", item, fileList[i]) - } - - // explicit name sort - out = testRunLsWithOpts(t, env.gopts, LsOptions{Sort: SortModeName}, []string{"latest"}) - fileList = strings.Split(string(out), "\n") - rtest.Assert(t, len(fileList) == 5, "invalid ls --sort name, expected 5 array elements, got %v", len(fileList)) - for i, item := range compareName { - rtest.Assert(t, item == fileList[i], "invalid ls --sort name, expected element '%s', got '%s'", item, fileList[i]) - } - - rtest.Equals(t, SortMode(0), SortModeName, "unexpected default sort mode") } From 1807627dda45ccf147dab9f79baab82708be46e6 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 3 Feb 2025 22:05:40 +0100 Subject: [PATCH 3/5] ls: refactor sorting into sortedPrinter struct --- cmd/restic/cmd_ls.go | 71 +++++++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/cmd/restic/cmd_ls.go b/cmd/restic/cmd_ls.go index d12dd3a31..f617e21d5 100644 --- a/cmd/restic/cmd_ls.go +++ b/cmd/restic/cmd_ls.go @@ -370,8 +370,6 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri } var printer lsPrinter - collector := []toSortOutput{} - outputSort := opts.Sort != SortModeName || opts.Reverse if gopts.JSON { printer = &jsonLsPrinter{ @@ -381,7 +379,6 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri printer = &ncduLsPrinter{ out: globalOptions.stdout, } - outputSort = false } else { printer = &textLsPrinter{ dirs: dirs, @@ -389,6 +386,13 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri HumanReadable: opts.HumanReadable, } } + if opts.Sort != SortModeName || opts.Reverse { + printer = &sortedPrinter{ + printer: printer, + sortMode: opts.Sort, + reverse: opts.Reverse, + } + } sn, subfolder, err := (&restic.SnapshotFilter{ Hosts: opts.Hosts, @@ -419,12 +423,8 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri printedDir := false if withinDir(nodepath) { // if we're within a target path, print the node - if outputSort { - collector = append(collector, toSortOutput{nodepath, node}) - } else { - if err := printer.Node(nodepath, node, false); err != nil { - return err - } + if err := printer.Node(nodepath, node, false); err != nil { + return err } printedDir = true @@ -439,7 +439,7 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri // there yet), signal the walker to descend into any subdirs if approachingMatchingTree(nodepath) { // print node leading up to the target paths - if !printedDir && !outputSort { + if !printedDir { return printer.Node(nodepath, node, true) } return nil @@ -474,39 +474,55 @@ func runLs(ctx context.Context, opts LsOptions, gopts GlobalOptions, args []stri return err } - if outputSort { - printSortedOutput(printer, opts.Sort, opts.Reverse, collector) - } - return printer.Close() } -func printSortedOutput(printer lsPrinter, sortMode SortMode, reverse bool, collector []toSortOutput) { - switch sortMode { +type sortedPrinter struct { + printer lsPrinter + collector []toSortOutput + sortMode SortMode + reverse bool +} + +func (p *sortedPrinter) Snapshot(sn *restic.Snapshot) error { + return p.printer.Snapshot(sn) +} +func (p *sortedPrinter) Node(path string, node *restic.Node, isPrefixDirectory bool) error { + if !isPrefixDirectory { + p.collector = append(p.collector, toSortOutput{path, node}) + } + return nil +} + +func (p *sortedPrinter) LeaveDir(_ string) error { + return nil +} +func (p *sortedPrinter) Close() error { + switch p.sortMode { case SortModeName: case SortModeSize: - slices.SortStableFunc(collector, func(a, b toSortOutput) int { + slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { return cmp.Or( cmp.Compare(a.node.Size, b.node.Size), cmp.Compare(a.nodepath, b.nodepath), ) }) case SortModeMtime: - slices.SortStableFunc(collector, func(a, b toSortOutput) int { + slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { return cmp.Or( a.node.ModTime.Compare(b.node.ModTime), cmp.Compare(a.nodepath, b.nodepath), ) }) case SortModeAtime: - slices.SortStableFunc(collector, func(a, b toSortOutput) int { + slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { return cmp.Or( a.node.AccessTime.Compare(b.node.AccessTime), cmp.Compare(a.nodepath, b.nodepath), ) }) case SortModeCtime: - slices.SortStableFunc(collector, func(a, b toSortOutput) int { + slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { return cmp.Or( a.node.ChangeTime.Compare(b.node.ChangeTime), cmp.Compare(a.nodepath, b.nodepath), @@ -514,13 +530,13 @@ func printSortedOutput(printer lsPrinter, sortMode SortMode, reverse bool, colle }) case SortModeExt: // map name to extension - mapExt := make(map[string]string, len(collector)) - for _, item := range collector { + mapExt := make(map[string]string, len(p.collector)) + for _, item := range p.collector { ext := filepath.Ext(item.nodepath) mapExt[item.nodepath] = ext } - slices.SortStableFunc(collector, func(a, b toSortOutput) int { + slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { return cmp.Or( cmp.Compare(mapExt[a.nodepath], mapExt[b.nodepath]), cmp.Compare(a.nodepath, b.nodepath), @@ -528,12 +544,13 @@ func printSortedOutput(printer lsPrinter, sortMode SortMode, reverse bool, colle }) } - if reverse { - slices.Reverse(collector) + if p.reverse { + slices.Reverse(p.collector) } - for _, elem := range collector { - _ = printer.Node(elem.nodepath, elem.node, false) + for _, elem := range p.collector { + _ = p.printer.Node(elem.nodepath, elem.node, false) } + return nil } // SortMode defines the allowed sorting modes From c32613a624eb089b75aa5b6c60f329a4d27efe59 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 3 Feb 2025 22:09:55 +0100 Subject: [PATCH 4/5] ls: extract comparator --- cmd/restic/cmd_ls.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/cmd/restic/cmd_ls.go b/cmd/restic/cmd_ls.go index f617e21d5..513540b17 100644 --- a/cmd/restic/cmd_ls.go +++ b/cmd/restic/cmd_ls.go @@ -498,36 +498,37 @@ func (p *sortedPrinter) LeaveDir(_ string) error { return nil } func (p *sortedPrinter) Close() error { + var comparator func(a, b toSortOutput) int switch p.sortMode { case SortModeName: case SortModeSize: - slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { + comparator = func(a, b toSortOutput) int { return cmp.Or( cmp.Compare(a.node.Size, b.node.Size), cmp.Compare(a.nodepath, b.nodepath), ) - }) + } case SortModeMtime: - slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { + comparator = func(a, b toSortOutput) int { return cmp.Or( a.node.ModTime.Compare(b.node.ModTime), cmp.Compare(a.nodepath, b.nodepath), ) - }) + } case SortModeAtime: - slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { + comparator = func(a, b toSortOutput) int { return cmp.Or( a.node.AccessTime.Compare(b.node.AccessTime), cmp.Compare(a.nodepath, b.nodepath), ) - }) + } case SortModeCtime: - slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { + comparator = func(a, b toSortOutput) int { return cmp.Or( a.node.ChangeTime.Compare(b.node.ChangeTime), cmp.Compare(a.nodepath, b.nodepath), ) - }) + } case SortModeExt: // map name to extension mapExt := make(map[string]string, len(p.collector)) @@ -536,14 +537,17 @@ func (p *sortedPrinter) Close() error { mapExt[item.nodepath] = ext } - slices.SortStableFunc(p.collector, func(a, b toSortOutput) int { + comparator = func(a, b toSortOutput) int { return cmp.Or( cmp.Compare(mapExt[a.nodepath], mapExt[b.nodepath]), cmp.Compare(a.nodepath, b.nodepath), ) - }) + } } + if comparator != nil { + slices.SortStableFunc(p.collector, comparator) + } if p.reverse { slices.Reverse(p.collector) } From 6cc06e0812719c3a09e394e2f575d3d21dc70522 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 3 Feb 2025 22:15:32 +0100 Subject: [PATCH 5/5] ls: add missing error handling --- cmd/restic/cmd_ls.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/restic/cmd_ls.go b/cmd/restic/cmd_ls.go index 513540b17..373a31a40 100644 --- a/cmd/restic/cmd_ls.go +++ b/cmd/restic/cmd_ls.go @@ -552,7 +552,9 @@ func (p *sortedPrinter) Close() error { slices.Reverse(p.collector) } for _, elem := range p.collector { - _ = p.printer.Node(elem.nodepath, elem.node, false) + if err := p.printer.Node(elem.nodepath, elem.node, false); err != nil { + return err + } } return nil }