Merge pull request #5559 from MichaelEischer/cleanup-repack

repository: remove unused return value from Repack

cmd/restic/cmd_copy.go

@@ -243,9 +243,7 @@ func copyTree(ctx context.Context, srcRepo restic.Repository, dstRepo restic.Rep
 	}
 
 	bar := printer.NewCounter("packs copied")
-	bar.SetMax(uint64(len(packList)))
-	_, err = repository.Repack(ctx, srcRepo, dstRepo, packList, copyBlobs, bar, printer.P)
-	bar.Done()
+	err = repository.Repack(ctx, srcRepo, dstRepo, packList, copyBlobs, bar, printer.P)
 	if err != nil {
 		return errors.Fatalf("%s", err)
 	}

internal/repository/prune.go

@@ -563,9 +563,7 @@ func (plan *PrunePlan) Execute(ctx context.Context, printer progress.Printer) er
 	if len(plan.repackPacks) != 0 {
 		printer.P("repacking packs\n")
 		bar := printer.NewCounter("packs repacked")
-		bar.SetMax(uint64(len(plan.repackPacks)))
-		_, err := Repack(ctx, repo, repo, plan.repackPacks, plan.keepBlobs, bar, printer.P)
-		bar.Done()
+		err := Repack(ctx, repo, repo, plan.repackPacks, plan.keepBlobs, bar, printer.P)
 		if err != nil {
 			return errors.Fatalf("%s", err)
 		}

internal/repository/repack.go

@@ -36,26 +36,22 @@ func Repack(
 	keepBlobs repackBlobSet,
 	p *progress.Counter,
 	logf LogFunc,
-) (obsoletePacks restic.IDSet, err error) {
+) error {
 	debug.Log("repacking %d packs while keeping %d blobs", len(packs), keepBlobs.Len())
 
 	if logf == nil {
 		logf = func(_ string, _ ...interface{}) {}
 	}
+	p.SetMax(uint64(len(packs)))
+	defer p.Done()
 
 	if repo == dstRepo && dstRepo.Connections() < 2 {
-		return nil, errors.New("repack step requires a backend connection limit of at least two")
+		return errors.New("repack step requires a backend connection limit of at least two")
 	}
 
-	err = dstRepo.WithBlobUploader(ctx, func(ctx context.Context, uploader restic.BlobSaver) error {
-		var err error
-		obsoletePacks, err = repack(ctx, repo, dstRepo, uploader, packs, keepBlobs, p, logf)
-		return err
+	return dstRepo.WithBlobUploader(ctx, func(ctx context.Context, uploader restic.BlobSaver) error {
+		return repack(ctx, repo, dstRepo, uploader, packs, keepBlobs, p, logf)
 	})
-	if err != nil {
-		return nil, err
-	}
-	return obsoletePacks, nil
 }
 
 func repack(
@@ -67,18 +63,19 @@ func repack(
 	keepBlobs repackBlobSet,
 	p *progress.Counter,
 	logf LogFunc,
-) (obsoletePacks restic.IDSet, err error) {
+) error {
+
 	wg, wgCtx := errgroup.WithContext(ctx)
 
 	if feature.Flag.Enabled(feature.S3Restore) {
 		job, err := repo.StartWarmup(ctx, packs)
 		if err != nil {
-			return nil, err
+			return err
 		}
 		if job.HandleCount() != 0 {
 			logf("warming up %d packs from cold storage, this may take a while...", job.HandleCount())
 			if err := job.Wait(ctx); err != nil {
-				return nil, err
+				return err
 			}
 		}
 	}
@@ -156,9 +153,5 @@ func repack(
 		wg.Go(worker)
 	}
 
-	if err := wg.Wait(); err != nil {
-		return nil, err
-	}
-
-	return packs, nil
+	return wg.Wait()
 }

internal/repository/repack_test.go

@@ -150,16 +150,10 @@ func findPacksForBlobs(t *testing.T, repo restic.Repository, blobs restic.BlobSe
 }
 
 func repack(t *testing.T, repo restic.Repository, be backend.Backend, packs restic.IDSet, blobs restic.BlobSet) {
-	repackedBlobs, err := repository.Repack(context.TODO(), repo, repo, packs, blobs, nil, nil)
-	if err != nil {
-		t.Fatal(err)
-	}
+	rtest.OK(t, repository.Repack(context.TODO(), repo, repo, packs, blobs, nil, nil))
 
-	for id := range repackedBlobs {
-		err = be.Remove(context.TODO(), backend.Handle{Type: restic.PackFile, Name: id.String()})
-		if err != nil {
-			t.Fatal(err)
-		}
+	for id := range packs {
+		rtest.OK(t, be.Remove(context.TODO(), backend.Handle{Type: restic.PackFile, Name: id.String()}))
 	}
 }
 
@@ -269,10 +263,7 @@ func testRepackCopy(t *testing.T, version uint) {
 	_, keepBlobs := selectBlobs(t, random, repo, 0.2)
 	copyPacks := findPacksForBlobs(t, repo, keepBlobs)
 
-	_, err := repository.Repack(context.TODO(), repoWrapped, dstRepoWrapped, copyPacks, keepBlobs, nil, nil)
-	if err != nil {
-		t.Fatal(err)
-	}
+	rtest.OK(t, repository.Repack(context.TODO(), repoWrapped, dstRepoWrapped, copyPacks, keepBlobs, nil, nil))
 	rebuildAndReloadIndex(t, dstRepo)
 
 	for h := range keepBlobs {
@@ -308,7 +299,7 @@ func testRepackWrongBlob(t *testing.T, version uint) {
 	_, keepBlobs := selectBlobs(t, random, repo, 0)
 	rewritePacks := findPacksForBlobs(t, repo, keepBlobs)
 
-	_, err := repository.Repack(context.TODO(), repo, repo, rewritePacks, keepBlobs, nil, nil)
+	err := repository.Repack(context.TODO(), repo, repo, rewritePacks, keepBlobs, nil, nil)
 	if err == nil {
 		t.Fatal("expected repack to fail but got no error")
 	}
@@ -355,8 +346,7 @@ func testRepackBlobFallback(t *testing.T, version uint) {
 	}))
 
 	// repack must fallback to valid copy
-	_, err := repository.Repack(context.TODO(), repo, repo, rewritePacks, keepBlobs, nil, nil)
-	rtest.OK(t, err)
+	rtest.OK(t, repository.Repack(context.TODO(), repo, repo, rewritePacks, keepBlobs, nil, nil))
 
 	keepBlobs = restic.NewBlobSet(restic.BlobHandle{Type: restic.DataBlob, ID: id})
 	packs := findPacksForBlobs(t, repo, keepBlobs)

internal/restic/repository.go

@@ -53,8 +53,9 @@ type Repository interface {
 	LoadRaw(ctx context.Context, t FileType, id ID) (data []byte, err error)
 	// LoadUnpacked loads and decrypts the file with the given type and ID.
 	LoadUnpacked(ctx context.Context, t FileType, id ID) (data []byte, err error)
+	// SaveUnpacked stores a file in the repository. This is restricted to snapshots.
 	SaveUnpacked(ctx context.Context, t WriteableFileType, buf []byte) (ID, error)
-	// RemoveUnpacked removes a file from the repository. This will eventually be restricted to deleting only snapshots.
+	// RemoveUnpacked removes a file from the repository. This is restricted to snapshots.
 	RemoveUnpacked(ctx context.Context, t WriteableFileType, id ID) error
 
 	// StartWarmup creates a new warmup job, requesting the backend to warmup the specified packs.