housekeeping: Extract function to search temporary objects

Right now, `Perform()` is quite entangled with the logic to search for
temporary objects and remove them. This makes the function hard to
maintain and extend, so this commit extracts `findTemporaryObjects()`
and deletes objects in a separate step afterwards.

2 files changed, 49 insertions, 33 deletions

diff --git a/internal/git/housekeeping/housekeeping.go b/internal/git/housekeeping/housekeeping.go
index 6d2f4a510..e42d1d819 100644
--- a/internal/git/housekeeping/housekeeping.go
+++ b/internal/git/housekeeping/housekeeping.go
@@ -20,8 +20,48 @@ const (
 func Perform(ctx context.Context, repoPath string) error {
 	logger := myLogger(ctx)
 
-	filesMarkedForRemoval := 0
+	temporaryObjects, err := findTemporaryObjects(ctx, repoPath)
+	if err != nil {
+		return err
+	}
+
 	unremovableFiles := 0
+	for _, path := range temporaryObjects {
+		if err := forceRemove(ctx, path); err != nil {
+			unremovableFiles++
+			logger.WithError(err).WithField("path", path).Warn("unable to remove stray file")
+		}
+	}
+
+	if len(temporaryObjects) > 0 {
+		logger.WithFields(log.Fields{
+			"files":    len(temporaryObjects),
+			"failures": unremovableFiles,
+		}).Info("removed files")
+	}
+
+	return err
+}
+
+// Delete a directory structure while ensuring the current user has permission to delete the directory structure
+func forceRemove(ctx context.Context, path string) error {
+	err := os.RemoveAll(path)
+	if err == nil {
+		return nil
+	}
+
+	// Delete failed. Try again after chmod'ing directories recursively
+	if err := FixDirectoryPermissions(ctx, path); err != nil {
+		return err
+	}
+
+	return os.RemoveAll(path)
+}
+
+func findTemporaryObjects(ctx context.Context, repoPath string) ([]string, error) {
+	var temporaryObjects []string
+
+	logger := myLogger(ctx)
 
 	err := filepath.Walk(repoPath, func(path string, info os.FileInfo, err error) error {
 		if info == nil {
@@ -37,16 +77,11 @@ func Perform(ctx context.Context, repoPath string) error {
 			return nil
 		}
 
-		if !shouldRemove(path, info.ModTime(), info.Mode()) {
+		if !isStaleTemporaryObject(path, info.ModTime(), info.Mode()) {
 			return nil
 		}
 
-		filesMarkedForRemoval++
-
-		if err := forceRemove(ctx, path); err != nil {
-			unremovableFiles++
-			logger.WithError(err).WithField("path", path).Warn("unable to remove stray file")
-		}
+		temporaryObjects = append(temporaryObjects, path)
 
 		if info.IsDir() {
 			// Do not walk removed directories
@@ -55,33 +90,14 @@ func Perform(ctx context.Context, repoPath string) error {
 
 		return nil
 	})
-
-	if filesMarkedForRemoval > 0 {
-		logger.WithFields(log.Fields{
-			"files":    filesMarkedForRemoval,
-			"failures": unremovableFiles,
-		}).Info("removed files")
+	if err != nil {
+		return nil, err
 	}
 
-	return err
-}
-
-// Delete a directory structure while ensuring the current user has permission to delete the directory structure
-func forceRemove(ctx context.Context, path string) error {
-	err := os.RemoveAll(path)
-	if err == nil {
-		return nil
-	}
-
-	// Delete failed. Try again after chmod'ing directories recursively
-	if err := FixDirectoryPermissions(ctx, path); err != nil {
-		return err
-	}
-
-	return os.RemoveAll(path)
+	return temporaryObjects, nil
 }
 
-func shouldRemove(path string, modTime time.Time, mode os.FileMode) bool {
+func isStaleTemporaryObject(path string, modTime time.Time, mode os.FileMode) bool {
 	base := filepath.Base(path)
 
 	// Only delete entries starting with `tmp_` and older than a week
diff --git a/internal/git/housekeeping/housekeeping_test.go b/internal/git/housekeeping/housekeeping_test.go
index b385a8d0a..94141f1e5 100644
--- a/internal/git/housekeeping/housekeeping_test.go
+++ b/internal/git/housekeeping/housekeeping_test.go
@@ -187,7 +187,7 @@ func TestPerform(t *testing.T) {
 	}
 }
 
-func TestShouldUnlink(t *testing.T) {
+func TestShouldRemoveTemporaryObject(t *testing.T) {
 	type args struct {
 		path    string
 		modTime time.Time
@@ -256,7 +256,7 @@ func TestShouldUnlink(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.want, shouldRemove(tc.args.path, tc.args.modTime, tc.args.mode))
+			require.Equal(t, tc.want, isStaleTemporaryObject(tc.args.path, tc.args.modTime, tc.args.mode))
 		})
 	}
 }