Merge pull request #21811 from MichaelEischer/misc-fixes

Address various code smells, outdated comments and nits

cmd/restic/cmd_find.go

@@ -22,6 +22,9 @@ import (
 	"github.com/restic/restic/internal/walker"
 )
 
+// errFindDone is returned from the tree walk when all requested tree IDs were found.
+var errFindDone = errors.New("find: all tree IDs found")
+
 func newFindCommand(globalOptions *global.Options) *cobra.Command {
 	var opts FindOptions
 
@@ -160,7 +163,7 @@ func (s *statefulOutput) PrintPatternJSON(path string, node *data.Node) {
 		findNode:    (*findNode)(node),
 	})
 	if err != nil {
-		s.printer.E("Marshall failed: %v", err)
+		s.printer.E("Marshal failed: %v", err)
 		return
 	}
 	if !s.inuse {
@@ -219,7 +222,7 @@ func (s *statefulOutput) PrintObjectJSON(kind, id, nodepath, treeID string, sn *
 		Time:       sn.Time,
 	})
 	if err != nil {
-		s.printer.E("Marshall failed: %v", err)
+		s.printer.E("Marshal failed: %v", err)
 		return
 	}
 	if !s.inuse {
@@ -375,7 +378,7 @@ func (f *Finder) findTree(treeID restic.ID, nodepath string) error {
 		// looking for blobs)
 		if f.itemsFound >= len(f.treeIDs) && f.blobIDs == nil {
 			// Return an error to terminate the Walk
-			return errors.New("OK")
+			return errFindDone
 		}
 	}
 	return nil
@@ -688,7 +691,7 @@ func runFind(ctx context.Context, opts FindOptions, gopts global.Options, args [
 
 	for _, sn := range filteredSnapshots {
 		if f.blobIDs != nil || f.treeIDs != nil {
-			if err = f.findIDs(ctx, sn); err != nil && err.Error() != "OK" {
+			if err = f.findIDs(ctx, sn); err != nil && !errors.Is(err, errFindDone) {
 				return err
 			}
 			continue

cmd/restic/find.go

@@ -11,7 +11,7 @@ import (
 )
 
 // initMultiSnapshotFilter is used for commands that work on multiple snapshots
-// MUST be combined with restic.FindFilteredSnapshots or FindFilteredSnapshots
+// MUST be combined with FindFilteredSnapshots
 // MUST be followed by finalizeSnapshotFilter after flag parsing
 func initMultiSnapshotFilter(flags *pflag.FlagSet, filt *data.SnapshotFilter, addHostShorthand bool) {
 	hostShorthand := "H"
@@ -24,7 +24,7 @@ func initMultiSnapshotFilter(flags *pflag.FlagSet, filt *data.SnapshotFilter, ad
 }
 
 // initSingleSnapshotFilter is used for commands that work on a single snapshot
-// MUST be combined with restic.FindFilteredSnapshot
+// MUST be combined with (*data.SnapshotFilter).FindLatest
 // MUST be followed by finalizeSnapshotFilter after flag parsing
 func initSingleSnapshotFilter(flags *pflag.FlagSet, filt *data.SnapshotFilter) {
 	flags.StringArrayVarP(&filt.Hosts, "host", "H", nil, "only consider snapshots for this `host`, when snapshot ID \"latest\" is given (can be specified multiple times, use empty string to unset default value) (default: $RESTIC_HOST)")

cmd/restic/lock.go

@@ -37,6 +37,8 @@ func internalOpenWithLocked(ctx context.Context, gopts global.Options, dryRun bo
 
 func openWithReadLock(ctx context.Context, gopts global.Options, noLock bool, printer progress.Printer) (context.Context, *repository.Repository, func(), error) {
 	// TODO enforce read-only operations once the locking code has moved to the repository
+	// As in-depth hardening, put the repository into read-only mode if noLock is true
+	// Not possible if the repository has to be locked.
 	return internalOpenWithLocked(ctx, gopts, noLock, false, printer)
 }
 

internal/archiver/archiver.go

@@ -162,7 +162,7 @@ func (o Options) ApplyDefaults() Options {
 	if o.SaveTreeConcurrency == 0 {
 		// can either wait for a file, wait for a tree, serialize a tree or wait for saveblob
 		// the last two are cpu-bound and thus mutually exclusive.
-		// Also allow waiting for FileReadConcurrency files, this is the maximum of files
+		// Also allow waiting for ReadConcurrency files, this is the maximum of files
 		// which currently can be in progress. The main backup loop blocks when trying to queue
 		// more files to read.
 		o.SaveTreeConcurrency = uint(runtime.GOMAXPROCS(0)) + o.ReadConcurrency

internal/archiver/exclude.go

@@ -214,7 +214,7 @@ func newDeviceMap(allowedSourcePaths []string, fs fs.FS) (deviceMap, error) {
 	return deviceMap, nil
 }
 
-// IsAllowed returns true if the path is located on an allowed device.
+// IsAllowed reports whether the path is located on an allowed device.
 func (m deviceMap) IsAllowed(item string, deviceID uint64, fs fs.FS) (bool, error) {
 	for dir := item; ; dir = fs.Dir(dir) {
 		debug.Log("item %v, test dir %v", item, dir)

internal/archiver/file_saver.go

@@ -64,7 +64,7 @@ type fileCompleteFunc func(*data.Node, ItemStats)
 // Save stores the file f and returns the data once it has been completed. The
 // file is closed by Save. completeReading is only called if the file was read
 // successfully. complete is always called. If completeReading is called, then
-// this will always happen before calling complete.
+// this will always happen before calling complete. The callbacks must not block.
 func (s *fileSaver) Save(ctx context.Context, snPath string, target string, file fs.File, start func(), completeReading func(), complete fileCompleteFunc) futureNode {
 	fn, ch := newFutureNode()
 	job := saveFileJob{
@@ -174,17 +174,19 @@ func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat
 			buf.Release()
 			break
 		}
-
-		buf.Data = chunk.Data
-		node.Size += uint64(chunk.Length)
-
 		if err != nil {
+			buf.Release()
 			_ = f.Close()
 			completeError(err)
 			return
 		}
+
+		buf.Data = chunk.Data
+		node.Size += uint64(chunk.Length)
+
 		// test if the context has been cancelled, return the error
 		if ctx.Err() != nil {
+			buf.Release()
 			_ = f.Close()
 			completeError(ctx.Err())
 			return

internal/backend/retry/backend_retry.go

@@ -182,7 +182,7 @@ func (be *Backend) Save(ctx context.Context, h backend.Handle, rd backend.Rewind
 			debug.Log("Save(%v) failed with error, removing file: %v", h, err)
 			rerr := be.Backend.Remove(ctx, h)
 			if rerr != nil {
-				debug.Log("Remove(%v) returned error: %v", h, err)
+				debug.Log("Remove(%v) returned error: %v", h, rerr)
 			}
 		}
 

internal/checker/checker.go

@@ -302,6 +302,9 @@ func (c *Checker) ReadPacks(ctx context.Context, filter func(packs map[restic.ID
 
 	packfileFilter := func(allPacks map[restic.ID]int64) map[restic.ID]int64 {
 		filteredPacks := make(map[restic.ID]int64)
+		c.blobRefs.Lock()
+		defer c.blobRefs.Unlock()
+
 		// convert used blobs into their encompassing packfiles
 		for bh := range c.blobRefs.M.Keys() {
 			for _, pb := range c.repo.LookupBlob(bh.Type, bh.ID) {

internal/data/snapshot.go

@@ -138,7 +138,7 @@ func (sn *Snapshot) fillUserInfo() error {
 	return err
 }
 
-// AddTags adds the given tags to the snapshots tags, preventing duplicates.
+// AddTags adds the given tags to the snapshot's tags, preventing duplicates.
 // It returns true if any changes were made.
 func (sn *Snapshot) AddTags(addTags []string) (changed bool) {
 nextTag:
@@ -154,7 +154,7 @@ nextTag:
 	return
 }
 
-// RemoveTags removes the given tags from the snapshots tags and
+// RemoveTags removes the given tags from the snapshot's tags and
 // returns true if any changes were made.
 func (sn *Snapshot) RemoveTags(removeTags []string) (changed bool) {
 	for _, remove := range removeTags {

internal/filter/doc.go

@@ -1,5 +1,5 @@
-// Package filter implements filters for files similar to filepath.Glob, but
-// in contrast to filepath.Glob a pattern may specify directories.
+// Package filter implements path filters similar to filepath.Match, but
+// patterns may span multiple path components and use a recursive "**" wildcard.
 //
-// For a list of valid patterns please see the documentation on filepath.Glob.
+// Single-component wildcards follow filepath.Match rules; see Match for details.
 package filter

internal/filter/exclude.go

@@ -39,11 +39,12 @@ func RejectByPattern(patterns []string, warnf func(msg string, args ...interface
 
 // RejectByInsensitivePattern is like RejectByPattern but case insensitive.
 func RejectByInsensitivePattern(patterns []string, warnf func(msg string, args ...interface{})) RejectByNameFunc {
+	lowerPatterns := make([]string, len(patterns))
 	for index, path := range patterns {
-		patterns[index] = strings.ToLower(path)
+		lowerPatterns[index] = strings.ToLower(path)
 	}
 
-	rejFunc := RejectByPattern(patterns, warnf)
+	rejFunc := RejectByPattern(lowerPatterns, warnf)
 	return func(item string) bool {
 		return rejFunc(strings.ToLower(item))
 	}

internal/filter/filter.go

@@ -7,8 +7,8 @@ import (
 	"github.com/restic/restic/internal/errors"
 )
 
-// ErrBadString is returned when Match is called with the empty string as the
-// second argument.
+// ErrBadString is returned when an empty path string is passed to prepareStr
+// (used by Match, ChildMatch, List, and ListWithChild).
 var ErrBadString = errors.New("filter.Match: string is empty")
 
 type patternPart struct {
@@ -65,9 +65,10 @@ func splitPath(p string) []string {
 	return parts
 }
 
-// Match returns true if str matches the pattern. When the pattern is
+// Match reports whether str matches the pattern. When the pattern is
 // malformed, filepath.ErrBadPattern is returned. The empty pattern matches
 // everything, when str is the empty string ErrBadString is returned.
+// Patterns prefixed with "!" are not supported; use List or ListWithChild.
 //
 // Pattern can be a combination of patterns suitable for filepath.Match, joined
 // by filepath.Separator.
@@ -90,9 +91,10 @@ func Match(patternStr, str string) (matched bool, err error) {
 	return match(pattern, strs)
 }
 
-// ChildMatch returns true if children of str can match the pattern. When the pattern is
+// ChildMatch reports whether children of str can match the pattern. When the pattern is
 // malformed, filepath.ErrBadPattern is returned. The empty pattern matches
 // everything, when str is the empty string ErrBadString is returned.
+// Patterns prefixed with "!" are not supported; use List or ListWithChild.
 //
 // Pattern can be a combination of patterns suitable for filepath.Match, joined
 // by filepath.Separator.
@@ -228,8 +230,8 @@ func (e *InvalidPatternError) Error() string {
 	return "invalid pattern(s) provided:\n" + strings.Join(e.InvalidPatterns, "\n")
 }
 
-// ValidatePatterns validates a slice of patterns.
-// Returns true if all patterns are valid - false otherwise, along with the invalid patterns.
+// ValidatePatterns validates a slice of patterns. Returns an error if any pattern is invalid.
+// The invalid patterns are returned in the error.
 func ValidatePatterns(patterns []string) error {
 	invalidPatterns := make([]string, 0)
 
@@ -266,18 +268,20 @@ func ParsePatterns(pattern []string) []Pattern {
 	return patpat
 }
 
-// List returns true if str matches one of the patterns. Empty patterns are ignored.
+// List reports whether str matches one of the patterns. Empty patterns are ignored.
 func List(patterns []Pattern, str string) (matched bool, err error) {
 	matched, _, err = list(patterns, false, str)
 	return matched, err
 }
 
-// ListWithChild returns true if str matches one of the patterns. Empty patterns are ignored.
+// ListWithChild reports whether str matches one of the patterns and whether child
+// paths might still match. Empty patterns are ignored.
 func ListWithChild(patterns []Pattern, str string) (matched bool, childMayMatch bool, err error) {
 	return list(patterns, true, str)
 }
 
-// list returns true if str matches one of the patterns. Empty patterns are ignored.
+// list reports whether str matches one of the patterns and, when checkChildMatches is
+// true, whether child paths might still match. Empty patterns are ignored.
 // Patterns prefixed by "!" are negated: any matching file excluded by a previous pattern
 // will become included again.
 func list(patterns []Pattern, checkChildMatches bool, str string) (matched bool, childMayMatch bool, err error) {

internal/filter/include.go

@@ -75,7 +75,7 @@ func (opts IncludePatternOptions) CollectPatterns(warnf func(msg string, args ..
 	return fs, nil
 }
 
-// IncludeByPattern returns a IncludeByNameFunc which includes files that match
+// IncludeByPattern returns an IncludeByNameFunc which includes files that match
 // one of the patterns.
 func IncludeByPattern(patterns []string, warnf func(msg string, args ...interface{})) IncludeByNameFunc {
 	parsedPatterns := ParsePatterns(patterns)
@@ -89,14 +89,15 @@ func IncludeByPattern(patterns []string, warnf func(msg string, args ...interfac
 	}
 }
 
-// IncludeByInsensitivePattern returns a IncludeByNameFunc which includes files that match
+// IncludeByInsensitivePattern returns an IncludeByNameFunc which includes files that match
 // one of the patterns, ignoring the casing of the filenames.
 func IncludeByInsensitivePattern(patterns []string, warnf func(msg string, args ...interface{})) IncludeByNameFunc {
+	lowerPatterns := make([]string, len(patterns))
 	for index, path := range patterns {
-		patterns[index] = strings.ToLower(path)
+		lowerPatterns[index] = strings.ToLower(path)
 	}
 
-	includeFunc := IncludeByPattern(patterns, warnf)
+	includeFunc := IncludeByPattern(lowerPatterns, warnf)
 	return func(item string) (matched bool, childMayMatch bool) {
 		return includeFunc(strings.ToLower(item))
 	}

internal/fs/ea_windows.go

@@ -160,7 +160,7 @@ func setFileEA(handle windows.Handle, iosb *ioStatusBlock, buf *uint8, bufLen ui
 	return
 }
 
-// pathSupportsExtendedAttributes returns true if the path supports extended attributes.
+// pathSupportsExtendedAttributes reports whether the path's volume supports extended attributes.
 func pathSupportsExtendedAttributes(path string) (supported bool, err error) {
 	var fileSystemFlags uint32
 	utf16Path, err := windows.UTF16PtrFromString(path)

internal/fs/fs_local_vss.go

@@ -49,7 +49,7 @@ type ErrorHandler func(item string, err error)
 // MessageHandler is used to report errors/messages via callbacks.
 type MessageHandler func(msg string, args ...interface{})
 
-// VolumeFilter is used to filter volumes by it's mount point or GUID path.
+// VolumeFilter is used to filter volumes by their mount point or GUID path.
 type VolumeFilter func(volume string) bool
 
 // LocalVss is a wrapper around the local file system which uses windows volume
@@ -160,8 +160,6 @@ func (fs *LocalVss) snapshotPath(path string) string {
 	if strings.HasPrefix(fixPath, `\\?\UNC\`) {
 		// UNC network shares are currently not supported so we access the regular file
 		// without snapshotting
-		// TODO: right now there is a problem in fixpath(): "\\host\share" is not returned as a UNC path
-		//       "\\host\share\" is returned as a valid UNC path
 		return path
 	}
 

internal/fs/node_windows.go

@@ -292,7 +292,7 @@ func fixEncryptionAttribute(path string, attrs *uint32, pathPointer *uint16) (er
 			return fmt.Errorf("failed to get file attributes for existing file: %s : %v", path, err)
 		}
 		if existingAttrs&windows.FILE_ATTRIBUTE_ENCRYPTED != 0 {
-			// File should not be encrypted, but its already encrypted. Decrypt it.
+			// File should not be encrypted, but it's already encrypted. Decrypt it.
 			err = decryptFile(pathPointer)
 			if err != nil {
 				if IsAccessDenied(err) || errors.Is(err, windows.ERROR_FILE_READ_ONLY) {

internal/fs/vss.go

@@ -27,7 +27,7 @@ type VssSnapshot struct {
 	mountPointInfo map[string]MountPoint
 }
 
-// HasSufficientPrivilegesForVSS returns true if the user is allowed to use VSS.
+// HasSufficientPrivilegesForVSS returns nil if the user is allowed to use VSS.
 func HasSufficientPrivilegesForVSS() error {
 	return errors.New("VSS snapshots are only supported on windows")
 }

internal/fs/vss_windows.go

@@ -1244,7 +1244,7 @@ func queryInterface(oleIUnknown *ole.IUnknown, guid *ole.GUID) (*interface{}, er
 	return ivss, nil
 }
 
-// isRunningOn64BitWindows returns true if running on 64-bit windows.
+// isRunningOn64BitWindows reports whether the process is running on 64-bit Windows.
 func isRunningOn64BitWindows() (bool, error) {
 	if runtime.GOARCH == "amd64" {
 		return true, nil

internal/global/secondary_repo.go

@@ -101,7 +101,7 @@ func (opts *SecondaryRepoOptions) FillGlobalOpts(ctx context.Context, gopts Opti
 		dstGopts.PasswordFile = opts.LegacyPasswordFile
 		dstGopts.PasswordCommand = opts.LegacyPasswordCommand
 		dstGopts.KeyHint = opts.LegacyKeyHint
-		// keep existing bevhaior for legacy options
+		// keep existing behavior for legacy options
 		dstGopts.InsecureNoPassword = false
 
 		pwdEnv = "RESTIC_PASSWORD2"

internal/repository/pack/pack.go

@@ -15,6 +15,10 @@ import (
 	"github.com/restic/restic/internal/crypto"
 )
 
+// ErrBroken is returned by Add and Finalize after a write error. The packer
+// must not be used again.
+var ErrBroken = errors.New("packer cannot be used after a write error")
+
 // Packer is used to create a new Pack.
 type Packer struct {
 	blobs []restic.Blob
@@ -22,6 +26,7 @@ type Packer struct {
 	bytes uint
 	k     *crypto.Key
 	wr    io.Writer
+	err   error // packer is unusable after the first error
 
 	m sync.Mutex
 }
@@ -37,17 +42,30 @@ func (p *Packer) Add(t restic.BlobType, id restic.ID, data []byte, uncompressedL
 	p.m.Lock()
 	defer p.m.Unlock()
 
-	c := restic.Blob{BlobHandle: restic.BlobHandle{Type: t, ID: id}}
+	if p.err != nil {
+		return 0, errors.Join(ErrBroken, p.err)
+	}
 
 	n, err := p.wr.Write(data)
-	c.Length = uint(n)
-	c.Offset = p.bytes
-	c.UncompressedLength = uint(uncompressedLength)
+	if err != nil {
+		p.err = errors.Wrap(err, "Write")
+		return n, p.err
+	}
+	if n != len(data) {
+		p.err = errors.New("short write")
+		return n, p.err
+	}
+
+	c := restic.Blob{
+		BlobHandle:         restic.BlobHandle{Type: t, ID: id},
+		Length:             uint(n),
+		Offset:             p.bytes,
+		UncompressedLength: uint(uncompressedLength),
+	}
 	p.bytes += uint(n)
 	p.blobs = append(p.blobs, c)
-	n += CalculateEntrySize(c)
 
-	return n, errors.Wrap(err, "Write")
+	return n + CalculateEntrySize(c), nil
 }
 
 var entrySize = uint(binary.Size(restic.BlobType(0)) + 2*headerLengthSize + len(restic.ID{}))
@@ -75,6 +93,10 @@ func (p *Packer) Finalize() error {
 	p.m.Lock()
 	defer p.m.Unlock()
 
+	if p.err != nil {
+		return errors.Join(ErrBroken, p.err)
+	}
+
 	header, err := makeHeader(p.blobs)
 	if err != nil {
 		return err
@@ -94,11 +116,13 @@ func (p *Packer) Finalize() error {
 	// append the header
 	n, err := p.wr.Write(encryptedHeader)
 	if err != nil {
-		return errors.Wrap(err, "Write")
+		p.err = errors.Wrap(err, "Write")
+		return p.err
 	}
 
 	if n != len(encryptedHeader) {
-		return errors.New("wrong number of bytes written")
+		p.err = errors.New("wrong number of bytes written")
+		return p.err
 	}
 	p.bytes += uint(len(encryptedHeader))
 

internal/repository/pack/pack_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/backend/mem"
 	"github.com/restic/restic/internal/crypto"
+	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/repository/pack"
 	"github.com/restic/restic/internal/restic"
 	rtest "github.com/restic/restic/internal/test"
@@ -149,6 +150,30 @@ func TestShortPack(t *testing.T) {
 	verifyBlobs(t, bufs, k, backend.ReaderAt(context.TODO(), b, handle), packSize)
 }
 
+type failWriter struct{}
+
+func (failWriter) Write([]byte) (int, error) {
+	return 0, errors.New("test error")
+}
+
+func TestPackerBroken(t *testing.T) {
+	k := crypto.NewRandomKey()
+	p := pack.NewPacker(k, failWriter{})
+
+	bufs := createBuffers(t, []int{100})
+
+	_, err := p.Add(restic.DataBlob, bufs[0].id, bufs[0].data, 0)
+	rtest.Assert(t, err != nil, "expected error on first Add")
+	rtest.Assert(t, !errors.Is(err, pack.ErrBroken), "first error must not be ErrBroken: %v", err)
+	rtest.Equals(t, 0, p.Count())
+
+	_, err = p.Add(restic.DataBlob, bufs[0].id, bufs[0].data, 0)
+	rtest.Assert(t, errors.Is(err, pack.ErrBroken), "expected ErrBroken on reuse, got %v", err)
+
+	err = p.Finalize()
+	rtest.Assert(t, errors.Is(err, pack.ErrBroken), "expected ErrBroken from Finalize, got %v", err)
+}
+
 func TestPackMerge(t *testing.T) {
 	k := crypto.NewRandomKey()
 

internal/restorer/hardlinks_index.go

@@ -9,7 +9,7 @@ type HardlinkKey struct {
 	Inode, Device uint64
 }
 
-// HardlinkIndex contains a list of inodes, devices these inodes are one, and associated file names.
+// HardlinkIndex maps inodes on devices to associated values.
 type HardlinkIndex[T any] struct {
 	m     sync.Mutex
 	Index map[HardlinkKey]T

internal/restorer/restorer.go

@@ -632,10 +632,8 @@ func (res *Restorer) VerifyFiles(ctx context.Context, dst string, countRestoredF
 		work     = make(chan mustCheck, 2*nVerifyWorkers)
 	)
 
-	if p != nil {
-		p.SetMax(countRestoredFiles)
-		defer p.Done()
-	}
+	p.SetMax(countRestoredFiles)
+	defer p.Done()
 
 	g, ctx := errgroup.WithContext(ctx)
 

internal/walker/rewriter.go

@@ -66,7 +66,7 @@ func NewTreeRewriter(opts RewriteOpts) *TreeRewriter {
 	return rw
 }
 
-func NewSnapshotSizeRewriter(rewriteNode NodeRewriteFunc, keepEmptyDirecoryFilter NodeKeepEmptyDirectoryFunc) (*TreeRewriter, QueryRewrittenSizeFunc) {
+func NewSnapshotSizeRewriter(rewriteNode NodeRewriteFunc, keepEmptyDirectoryFilter NodeKeepEmptyDirectoryFunc) (*TreeRewriter, QueryRewrittenSizeFunc) {
 	var count uint
 	var size uint64
 
@@ -80,7 +80,7 @@ func NewSnapshotSizeRewriter(rewriteNode NodeRewriteFunc, keepEmptyDirecoryFilte
 			return node
 		},
 		DisableNodeCache:   true,
-		KeepEmptyDirectory: keepEmptyDirecoryFilter,
+		KeepEmptyDirectory: keepEmptyDirectoryFilter,
 	})
 
 	ss := func() SnapshotSize {

internal/walker/walker.go

@@ -15,7 +15,7 @@ var ErrSkipNode = errors.New("skip this node")
 
 // WalkFunc is the type of the function called for each node visited by Walk.
 // Path is the slash-separated path from the root node. If there was a problem
-// loading a node, err is set to a non-nil error. WalkFunc can chose to ignore
+// loading a node, err is set to a non-nil error. WalkFunc can choose to ignore
 // it by returning nil.
 //
 // When the special value ErrSkipNode is returned and node is a dir node, it is