Merge branch 'pks-refactor-ioutil-readdir-usages' into 'master'

Refactor remaining users of the ioutil package

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4899

Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

8 files changed, 113 insertions, 41 deletions

diff --git a/internal/cache/keyer.go b/internal/cache/keyer.go
index 519edbd38..c3cdf5738 100644
--- a/internal/cache/keyer.go
+++ b/internal/cache/keyer.go
@@ -1,13 +1,12 @@
 package cache
 
-//nolint:depguard
 import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
 	"errors"
 	"fmt"
-	"io/ioutil"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"sort"
@@ -120,7 +119,18 @@ func (keyer leaseKeyer) keyPath(ctx context.Context, repo *gitalypb.Repository,
 
 	anyValidPending := false
 	for _, p := range pending {
-		if time.Since(p.ModTime()) > staleAge {
+		info, err := p.Info()
+		if err != nil {
+			// The lease may have been cleaned up already, so we just ignore it as we
+			// wanted to remove it anyway.
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+
+			return "", fmt.Errorf("statting lease: %w", err)
+		}
+
+		if time.Since(info.ModTime()) > staleAge {
 			pPath := filepath.Join(pDir, p.Name())
 			if err := os.Remove(pPath); err != nil && !os.IsNotExist(err) {
 				return "", err
@@ -221,13 +231,13 @@ func (keyer leaseKeyer) getRepoStatePath(repo *gitalypb.Repository) (string, err
 	return filepath.Join(stateDir, relativePath), nil
 }
 
-func (keyer leaseKeyer) currentLeases(repo *gitalypb.Repository) ([]os.FileInfo, error) {
+func (keyer leaseKeyer) currentLeases(repo *gitalypb.Repository) ([]fs.DirEntry, error) {
 	repoStatePath, err := keyer.getRepoStatePath(repo)
 	if err != nil {
 		return nil, err
 	}
 
-	pendings, err := ioutil.ReadDir(pendingDir(repoStatePath))
+	pendings, err := os.ReadDir(pendingDir(repoStatePath))
 	switch {
 	case os.IsNotExist(err):
 		// pending files subdir don't exist yet, that's okay
diff --git a/internal/cache/walker.go b/internal/cache/walker.go
index 6a0b065df..b14c8adf6 100644
--- a/internal/cache/walker.go
+++ b/internal/cache/walker.go
@@ -5,10 +5,10 @@
 // worker will walk the cache directory every ten minutes.
 package cache
 
-//nolint:depguard
 import (
+	"errors"
 	"fmt"
-	"io/ioutil"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"time"
@@ -31,7 +31,7 @@ func (c *DiskCache) cleanWalk(path string) error {
 	defer time.Sleep(100 * time.Microsecond) // relieve pressure
 
 	c.walkerCheckTotal.Inc()
-	entries, err := ioutil.ReadDir(path)
+	entries, err := os.ReadDir(path)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil
@@ -50,8 +50,19 @@ func (c *DiskCache) cleanWalk(path string) error {
 			continue
 		}
 
+		info, err := e.Info()
+		if err != nil {
+			// The file may have been cleaned up already, so we just ignore it as we
+			// wanted to remove it anyway.
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+
+			return fmt.Errorf("statting cached file: %w", err)
+		}
+
 		c.walkerCheckTotal.Inc()
-		if time.Since(e.ModTime()) < staleAge {
+		if time.Since(info.ModTime()) < staleAge {
 			continue // still fresh
 		}
 
diff --git a/internal/git/housekeeping/worktrees.go b/internal/git/housekeeping/worktrees.go
index 22aae27ef..188ef160b 100644
--- a/internal/git/housekeeping/worktrees.go
+++ b/internal/git/housekeeping/worktrees.go
@@ -1,12 +1,11 @@
 package housekeeping
 
-//nolint:depguard
 import (
 	"bytes"
 	"context"
 	"errors"
 	"fmt"
-	"io/ioutil"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -57,16 +56,27 @@ func cleanStaleWorktrees(ctx context.Context, repo *localrepo.Repo, threshold ti
 		return err
 	}
 
-	worktreeEntries, err := ioutil.ReadDir(worktreePath)
+	worktreeEntries, err := os.ReadDir(worktreePath)
 	if err != nil {
 		return err
 	}
 
-	for _, info := range worktreeEntries {
-		if !info.IsDir() || (info.Mode()&os.ModeSymlink != 0) {
+	for _, entry := range worktreeEntries {
+		if !entry.IsDir() || (entry.Type()&fs.ModeSymlink != 0) {
 			continue
 		}
 
+		info, err := entry.Info()
+		if err != nil {
+			// It's fine if the entry has disappeared meanwhile, we wanted to remove it
+			// anyway.
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+
+			return fmt.Errorf("statting worktree: %w", err)
+		}
+
 		if info.ModTime().Before(threshold) {
 			err := removeWorktree(ctx, repo, info.Name())
 			switch {
diff --git a/internal/git/stats/profile.go b/internal/git/stats/profile.go
index 68d4e0991..85c4015f8 100644
--- a/internal/git/stats/profile.go
+++ b/internal/git/stats/profile.go
@@ -1,10 +1,9 @@
 package stats
 
-//nolint:depguard
 import (
 	"context"
 	"errors"
-	"io/ioutil"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -34,13 +33,13 @@ func PackfilesCount(repoPath string) (int, error) {
 }
 
 // GetPackfiles returns the FileInfo of packfiles inside a repository.
-func GetPackfiles(repoPath string) ([]os.FileInfo, error) {
-	files, err := ioutil.ReadDir(filepath.Join(repoPath, "objects/pack/"))
+func GetPackfiles(repoPath string) ([]fs.DirEntry, error) {
+	files, err := os.ReadDir(filepath.Join(repoPath, "objects/pack/"))
 	if err != nil {
 		return nil, err
 	}
 
-	var packFiles []os.FileInfo
+	var packFiles []fs.DirEntry
 	for _, f := range files {
 		if filepath.Ext(f.Name()) == ".pack" {
 			packFiles = append(packFiles, f)
diff --git a/internal/gitaly/service/repository/gc.go b/internal/gitaly/service/repository/gc.go
index 3acdaa6a0..a06a411eb 100644
--- a/internal/gitaly/service/repository/gc.go
+++ b/internal/gitaly/service/repository/gc.go
@@ -1,11 +1,10 @@
 package repository
 
-//nolint:depguard
 import (
 	"context"
 	"errors"
 	"fmt"
-	"io/ioutil"
+	"io/fs"
 	"os"
 	"path/filepath"
 
@@ -107,7 +106,7 @@ func (s *server) cleanupKeepArounds(ctx context.Context, repo *localrepo.Repo) e
 	keepAroundsPrefix := "refs/keep-around"
 	keepAroundsPath := filepath.Join(repoPath, keepAroundsPrefix)
 
-	refInfos, err := ioutil.ReadDir(keepAroundsPath)
+	refEntries, err := os.ReadDir(keepAroundsPath)
 	if os.IsNotExist(err) {
 		return nil
 	}
@@ -115,11 +114,24 @@ func (s *server) cleanupKeepArounds(ctx context.Context, repo *localrepo.Repo) e
 		return err
 	}
 
-	for _, info := range refInfos {
-		if info.IsDir() {
+	for _, entry := range refEntries {
+		if entry.IsDir() {
 			continue
 		}
 
+		info, err := entry.Info()
+		if err != nil {
+			// Even though the reference has disappeared we know its name
+			// and thus the object hash it is supposed to point to. So
+			// while we technically could try to fix it, we don't as
+			// we seem to be racing with a concurrent process.
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+
+			return fmt.Errorf("statting keep-around ref: %w", err)
+		}
+
 		refName := fmt.Sprintf("%s/%s", keepAroundsPrefix, info.Name())
 		path := filepath.Join(repoPath, keepAroundsPrefix, info.Name())
 
diff --git a/internal/gitaly/service/repository/midx.go b/internal/gitaly/service/repository/midx.go
index 7691c0680..85c0f8809 100644
--- a/internal/gitaly/service/repository/midx.go
+++ b/internal/gitaly/service/repository/midx.go
@@ -2,7 +2,9 @@ package repository
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -209,28 +211,39 @@ func (s *server) midxRepack(ctx context.Context, repo repository.GitRepo) error
 // Reference:
 // - https://public-inbox.org/git/f3b25a9927fe560b764850ea880a71932ec2af32.1598380599.git.gitgitgadget@gmail.com/
 func calculateBatchSize(repoPath string) (int64, error) {
-	files, err := stats.GetPackfiles(repoPath)
+	packfiles, err := stats.GetPackfiles(repoPath)
 	if err != nil {
 		return 0, err
 	}
 
 	// In case of 2 or less packs,
 	// batch size should be 0 for a full repack
-	if len(files) <= 2 {
+	if len(packfiles) <= 2 {
 		return 0, nil
 	}
 
 	var biggestSize int64
 	var secondBiggestSize int64
-	for _, f := range files {
-		if f.Size() > biggestSize {
+	for _, packfile := range packfiles {
+		info, err := packfile.Info()
+		if err != nil {
+			// It's fine if the entry has disappeared meanwhile, we just don't account
+			// for its size.
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+
+			return 0, fmt.Errorf("statting packfile: %w", err)
+		}
+
+		if info.Size() > biggestSize {
 			secondBiggestSize = biggestSize
-			biggestSize = f.Size()
+			biggestSize = info.Size()
 			continue
 		}
 
-		if f.Size() > secondBiggestSize {
-			secondBiggestSize = f.Size()
+		if info.Size() > secondBiggestSize {
+			secondBiggestSize = info.Size()
 		}
 	}
 
diff --git a/internal/gitaly/service/repository/midx_test.go b/internal/gitaly/service/repository/midx_test.go
index ea358bf36..5057fbd37 100644
--- a/internal/gitaly/service/repository/midx_test.go
+++ b/internal/gitaly/service/repository/midx_test.go
@@ -5,6 +5,7 @@ package repository
 import (
 	"context"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"testing"
@@ -111,8 +112,8 @@ func TestMidxRepack(t *testing.T) {
 		"At least 1 pack file should have been created",
 	)
 
-	newPackFile := findNewestPackFile(t, repoPath)
-	assert.True(t, newPackFile.ModTime().After(time.Time{}))
+	_, newPackFileInfo := findNewestPackFile(t, repoPath)
+	assert.True(t, newPackFileInfo.ModTime().After(time.Time{}))
 }
 
 func TestMidxRepack_transactional(t *testing.T) {
@@ -214,21 +215,26 @@ func TestMidxRepackExpire(t *testing.T) {
 }
 
 // findNewestPackFile returns the latest created pack file in repo's odb
-func findNewestPackFile(t *testing.T, repoPath string) os.FileInfo {
+func findNewestPackFile(t *testing.T, repoPath string) (fs.DirEntry, fs.FileInfo) {
 	t.Helper()
 
 	files, err := stats.GetPackfiles(repoPath)
 	require.NoError(t, err)
 
-	var newestPack os.FileInfo
+	var newestPack fs.DirEntry
+	var newestPackInfo fs.FileInfo
 	for _, f := range files {
-		if newestPack == nil || f.ModTime().After(newestPack.ModTime()) {
+		info, err := f.Info()
+		require.NoError(t, err)
+
+		if newestPack == nil || info.ModTime().After(newestPackInfo.ModTime()) {
 			newestPack = f
+			newestPackInfo = info
 		}
 	}
 	require.NotNil(t, newestPack)
 
-	return newestPack
+	return newestPack, newestPackInfo
 }
 
 // addPackFiles creates some packfiles by
diff --git a/internal/tempdir/clean.go b/internal/tempdir/clean.go
index 2edfcc929..7009a443a 100644
--- a/internal/tempdir/clean.go
+++ b/internal/tempdir/clean.go
@@ -1,10 +1,10 @@
 package tempdir
 
-//nolint:depguard
 import (
 	"context"
+	"errors"
 	"fmt"
-	"io/ioutil"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -71,7 +71,7 @@ func clean(locator storage.Locator, storage config.Storage) error {
 		panic(invalidCleanRoot("invalid tempdir clean root: panicking to prevent data loss"))
 	}
 
-	entries, err := ioutil.ReadDir(dir)
+	entries, err := os.ReadDir(dir)
 	if os.IsNotExist(err) {
 		return nil
 	}
@@ -79,7 +79,18 @@ func clean(locator storage.Locator, storage config.Storage) error {
 		return err
 	}
 
-	for _, info := range entries {
+	for _, entry := range entries {
+		info, err := entry.Info()
+		if err != nil {
+			// It's fine if the entry has disappeared meanwhile, we wanted to remove it
+			// anyway.
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+
+			return fmt.Errorf("statting tempdir entry: %w", err)
+		}
+
 		if time.Since(info.ModTime()) < maxAge {
 			continue
 		}