diff options
24 files changed, 89 insertions, 76 deletions
diff --git a/internal/git/localrepo/bundle.go b/internal/git/localrepo/bundle.go index 53a02a9b9..c1ca87b8c 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -76,7 +76,7 @@ func (repo *Repo) CreateBundle(ctx context.Context, out io.Writer, opts *CreateB func (repo *Repo) CloneBundle(ctx context.Context, reader io.Reader) error { // When cloning from a file, `git-clone(1)` requires the path to the file. Create a temporary // file with the Git bundle contents that is used for cloning. - bundlePath, err := repo.createTempBundle(ctx, reader) + bundlePath, _, err := repo.createTempBundle(ctx, reader) if err != nil { return err } @@ -143,7 +143,7 @@ func (repo *Repo) FetchBundle(ctx context.Context, txManager transaction.Manager opts = &FetchBundleOpts{} } - bundlePath, err := repo.createTempBundle(ctx, reader) + bundlePath, _, err := repo.createTempBundle(ctx, reader) if err != nil { return fmt.Errorf("fetch bundle: %w", err) } @@ -170,17 +170,17 @@ func (repo *Repo) FetchBundle(ctx context.Context, txManager transaction.Manager // createTempBundle copies reader onto the filesystem so that a path can be // passed to git. git-fetch does not support streaming a bundle over a pipe. -func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundlPath string, returnErr error) { - tmpDir, err := tempdir.New(ctx, repo.GetStorageName(), repo.logger, repo.locator) +func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundlPath string, cleanup func() error, returnErr error) { + tmpDir, cleanup, err := tempdir.New(ctx, repo.GetStorageName(), repo.locator) if err != nil { - return "", fmt.Errorf("create temp bundle: %w", err) + return "", nil, fmt.Errorf("create temp bundle: %w", err) } bundlePath := filepath.Join(tmpDir.Path(), "repo.bundle") file, err := os.Create(bundlePath) if err != nil { - return "", fmt.Errorf("create temp bundle: %w", err) + return "", nil, fmt.Errorf("create temp bundle: %w", err) } defer func() { if err := file.Close(); err != nil && returnErr == nil { @@ -189,10 +189,10 @@ func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundl }() if _, err = io.Copy(file, reader); err != nil { - return "", fmt.Errorf("create temp bundle: %w", err) + return "", nil, fmt.Errorf("create temp bundle: %w", err) } - return bundlePath, nil + return bundlePath, cleanup, nil } // updateHeadFromBundle updates HEAD from a bundle file diff --git a/internal/git/quarantine/quarantine.go b/internal/git/quarantine/quarantine.go index d726822e3..40058d50f 100644 --- a/internal/git/quarantine/quarantine.go +++ b/internal/git/quarantine/quarantine.go @@ -34,21 +34,21 @@ type Dir struct { // New creates a new quarantine directory and returns the directory. The repository is cleaned // up when the user invokes the Migrate() functionality on the Dir. -func New(ctx context.Context, repo *gitalypb.Repository, logger log.Logger, locator storage.Locator) (*Dir, error) { +func New(ctx context.Context, repo *gitalypb.Repository, locator storage.Locator) (*Dir, func() error, error) { repoPath, err := locator.GetRepoPath(repo, storage.WithRepositoryVerificationSkipped()) if err != nil { - return nil, structerr.NewInternal("getting repo path: %w", err) + return nil, nil, structerr.NewInternal("getting repo path: %w", err) } - quarantineDir, err := tempdir.NewWithPrefix(ctx, repo.GetStorageName(), - storage.QuarantineDirectoryPrefix(repo), logger, locator) + quarantineDir, cleanup, err := tempdir.NewWithPrefix(ctx, repo.GetStorageName(), + storage.QuarantineDirectoryPrefix(repo), locator) if err != nil { - return nil, fmt.Errorf("creating quarantine: %w", err) + return nil, nil, fmt.Errorf("creating quarantine: %w", err) } quarantinedRepo, err := Apply(repoPath, repo, quarantineDir.Path()) if err != nil { - return nil, err + return nil, nil, err } return &Dir{ @@ -56,7 +56,7 @@ func New(ctx context.Context, repo *gitalypb.Repository, logger log.Logger, loca quarantinedRepo: quarantinedRepo, locator: locator, dir: quarantineDir, - }, nil + }, cleanup, nil } // Apply applies the quarantine on the repository. This is done by setting the quarantineDirectory diff --git a/internal/gitaly/repoutil/create.go b/internal/gitaly/repoutil/create.go index 83eb2d847..a9df87da1 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -85,16 +85,11 @@ func Create( return structerr.NewAlreadyExists("repository exists already") } - newRepo, newRepoDir, err := tempdir.NewRepository(ctx, repository.GetStorageName(), logger, locator) + newRepo, newRepoDir, cleanup, err := tempdir.NewRepository(ctx, repository.GetStorageName(), locator) if err != nil { return fmt.Errorf("creating temporary repository: %w", err) } - defer func() { - // We don't really care about whether this succeeds or not. It will either get - // cleaned up after the context is done, or eventually by the tempdir walker when - // it's old enough. - _ = os.RemoveAll(newRepoDir.Path()) - }() + defer cleanup() // Note that we do not create the repository directly in its target location, but // instead create it in a temporary directory, first. This is done such that we can diff --git a/internal/gitaly/repoutil/custom_hooks.go b/internal/gitaly/repoutil/custom_hooks.go index f81d827e8..faa2e6842 100644 --- a/internal/gitaly/repoutil/custom_hooks.go +++ b/internal/gitaly/repoutil/custom_hooks.go @@ -133,16 +133,12 @@ func SetCustomHooks( // temporarily store the current repository hooks. This enables "atomic" // directory swapping by acting as an intermediary storage location between // moves. - tmpDir, err := tempdir.NewWithoutContext(repo.GetStorageName(), logger, locator) + tmpDir, cleanup, err := tempdir.NewWithoutContext(repo.GetStorageName(), locator) if err != nil { return fmt.Errorf("creating temp directory: %w", err) } - defer func() { - if err := os.RemoveAll(tmpDir.Path()); err != nil { - logger.WithError(err).WarnContext(ctx, "failed to remove temporary directory") - } - }() + defer cleanup() if err := ExtractHooks(ctx, logger, reader, tmpDir.Path(), false); err != nil { return fmt.Errorf("extracting hooks: %w", err) diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go index d5f62a810..49190de68 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -23,10 +23,11 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s return structerr.NewInvalidArgument("%w", err) } - _, quarantineRepo, err := s.quarantinedRepo(ctx, request.GetRepository()) + _, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, request.GetRepository()) if err != nil { return err } + defer cleanup() ours, err := quarantineRepo.ResolveRevision(ctx, git.Revision(request.OurCommitOid+"^{commit}")) if err != nil { diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index 43a60ba7b..b855185b7 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -143,10 +143,11 @@ func (s *server) resolveConflicts(header *gitalypb.ResolveConflictsRequestHeader return err } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, header.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, header.GetRepository()) if err != nil { return err } + defer cleanup() if err := s.repoWithBranchCommit(ctx, quarantineRepo, diff --git a/internal/gitaly/service/conflicts/server.go b/internal/gitaly/service/conflicts/server.go index 745380d78..6169dd62c 100644 --- a/internal/gitaly/service/conflicts/server.go +++ b/internal/gitaly/service/conflicts/server.go @@ -47,13 +47,13 @@ func (s *server) localrepo(repo storage.Repository) *localrepo.Repo { func (s *server) quarantinedRepo( ctx context.Context, repo *gitalypb.Repository, -) (*quarantine.Dir, *localrepo.Repo, error) { - quarantineDir, err := quarantine.New(ctx, repo, s.logger, s.locator) +) (*quarantine.Dir, *localrepo.Repo, func() error, error) { + quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.locator) if err != nil { - return nil, nil, structerr.NewInternal("creating object quarantine: %w", err) + return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) } quarantineRepo := s.localrepo(quarantineDir.QuarantinedRepo()) - return quarantineDir, quarantineRepo, nil + return quarantineDir, quarantineRepo, cleanup, nil } diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go index 27de0544d..b0459d19a 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -21,10 +21,11 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer cleanup() startRevision, err := s.fetchStartRevision(ctx, quarantineRepo, req) if err != nil { diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 6ef7fcecf..e894ce32a 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -516,10 +516,11 @@ func (s *Server) userCommitFiles( stream gitalypb.OperationService_UserCommitFilesServer, objectHash git.ObjectHash, ) error { - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, header.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, header.GetRepository()) if err != nil { return err } + defer cleanup() repoPath, err := quarantineRepo.Path() if err != nil { diff --git a/internal/gitaly/service/operations/ff_branch.go b/internal/gitaly/service/operations/ff_branch.go index 4b378e82e..25a19e515 100644 --- a/internal/gitaly/service/operations/ff_branch.go +++ b/internal/gitaly/service/operations/ff_branch.go @@ -23,10 +23,11 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ // While we're creating a quarantine directory, we know that it won't ever have any new // objects given that we're doing a fast-forward merge. We still want to create one such // that Rails can efficiently compute new objects. - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, in.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, in.GetRepository()) if err != nil { return nil, err } + defer cleanup() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/merge_branch.go b/internal/gitaly/service/operations/merge_branch.go index 677c96e6f..1bf064c41 100644 --- a/internal/gitaly/service/operations/merge_branch.go +++ b/internal/gitaly/service/operations/merge_branch.go @@ -27,10 +27,11 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc return structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, firstRequest.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, firstRequest.GetRepository()) if err != nil { return err } + defer cleanup() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/rebase_confirmable.go b/internal/gitaly/service/operations/rebase_confirmable.go index 884eb0368..2b4d14eec 100644 --- a/internal/gitaly/service/operations/rebase_confirmable.go +++ b/internal/gitaly/service/operations/rebase_confirmable.go @@ -31,10 +31,11 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba ctx := stream.Context() - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, header.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, header.GetRepository()) if err != nil { return structerr.NewInternal("creating repo quarantine: %w", err) } + defer cleanup() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/rebase_to_ref.go b/internal/gitaly/service/operations/rebase_to_ref.go index 2a1c7b817..a32eaee3a 100644 --- a/internal/gitaly/service/operations/rebase_to_ref.go +++ b/internal/gitaly/service/operations/rebase_to_ref.go @@ -19,10 +19,11 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, request.Repository) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, request.Repository) if err != nil { return nil, structerr.NewInternal("creating repo quarantine: %w", err) } + defer cleanup() oid, err := quarantineRepo.ResolveRevision(ctx, git.Revision(request.FirstParentRef)) if err != nil { diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index fcd1813fd..a2e14afc0 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -20,10 +20,11 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer cleanup() startRevision, err := s.fetchStartRevision(ctx, quarantineRepo, req) if err != nil { diff --git a/internal/gitaly/service/operations/server.go b/internal/gitaly/service/operations/server.go index 9673bb2da..cd3543562 100644 --- a/internal/gitaly/service/operations/server.go +++ b/internal/gitaly/service/operations/server.go @@ -53,13 +53,13 @@ func (s *Server) localrepo(repo storage.Repository) *localrepo.Repo { func (s *Server) quarantinedRepo( ctx context.Context, repo *gitalypb.Repository, -) (*quarantine.Dir, *localrepo.Repo, error) { - quarantineDir, err := quarantine.New(ctx, repo, s.logger, s.locator) +) (*quarantine.Dir, *localrepo.Repo, func() error, error) { + quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.locator) if err != nil { - return nil, nil, structerr.NewInternal("creating object quarantine: %w", err) + return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) } quarantineRepo := s.localrepo(quarantineDir.QuarantinedRepo()) - return quarantineDir, quarantineRepo, nil + return quarantineDir, quarantineRepo, cleanup, nil } diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go index 82b26d5ac..4da67f595 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -77,10 +77,11 @@ func validateUserSquashRequest(locator storage.Locator, req *gitalypb.UserSquash func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest) (string, error) { // All new objects are staged into a quarantine directory first so that we can do // transactional voting before we commit data to disk. - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return "", structerr.NewInternal("creating quarantine: %w", err) } + defer cleanup() // We need to retrieve the start commit such that we can create the new commit with // all parents of the start commit. diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go index bef9e5d62..d68079696 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -25,10 +25,11 @@ func (s *Server) UserUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer cleanup() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 83b2d7b1f..87a442eca 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -129,10 +129,11 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer cleanup() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/user_create_branch.go b/internal/gitaly/service/operations/user_create_branch.go index 8546182f5..52d724117 100644 --- a/internal/gitaly/service/operations/user_create_branch.go +++ b/internal/gitaly/service/operations/user_create_branch.go @@ -34,10 +34,11 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB if err := validateUserCreateBranchRequest(s.locator, req); err != nil { return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer cleanup() // BEGIN TODO: Uncomment if StartPoint started behaving sensibly // like BranchName. See diff --git a/internal/gitaly/service/operations/user_update_branch.go b/internal/gitaly/service/operations/user_update_branch.go index 053939b73..7e6ece60e 100644 --- a/internal/gitaly/service/operations/user_update_branch.go +++ b/internal/gitaly/service/operations/user_update_branch.go @@ -61,10 +61,11 @@ func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateB referenceName := git.NewReferenceNameFromBranchName(string(req.BranchName)) - quarantineDir, _, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, _, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer cleanup() if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newOID, oldOID); err != nil { var customHookErr updateref.CustomHookError diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 5e1efe40f..239407488 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -30,10 +30,11 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, targetRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, targetRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer cleanup() sourceRepo, err := remoterepo.New(ctx, req.GetSourceRepository(), s.conns) if err != nil { diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go index 74c7f7b61..d20689b70 100644 --- a/internal/gitaly/service/repository/replicate.go +++ b/internal/gitaly/service/repository/replicate.go @@ -141,10 +141,11 @@ func validateReplicateRepository(locator storage.Locator, in *gitalypb.Replicate func (s *server) create(ctx context.Context, in *gitalypb.ReplicateRepositoryRequest, repoPath string) error { // if the directory exists, remove it if _, err := os.Stat(repoPath); err == nil { - tempDir, err := tempdir.NewWithoutContext(in.GetRepository().GetStorageName(), s.logger, s.locator) + tempDir, cleanup, err := tempdir.NewWithoutContext(in.GetRepository().GetStorageName(), s.locator) if err != nil { return err } + defer cleanup() if err = os.Rename(repoPath, filepath.Join(tempDir.Path(), filepath.Base(repoPath))); err != nil { return fmt.Errorf("error deleting invalid repo: %w", err) diff --git a/internal/gitaly/service/repository/server.go b/internal/gitaly/service/repository/server.go index 230fb4160..e00edf777 100644 --- a/internal/gitaly/service/repository/server.go +++ b/internal/gitaly/service/repository/server.go @@ -65,13 +65,13 @@ func (s *server) localrepo(repo storage.Repository) *localrepo.Repo { func (s *server) quarantinedRepo( ctx context.Context, repo *gitalypb.Repository, -) (*quarantine.Dir, *localrepo.Repo, error) { - quarantineDir, err := quarantine.New(ctx, repo, s.logger, s.locator) +) (*quarantine.Dir, *localrepo.Repo, func() error, error) { + quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.locator) if err != nil { - return nil, nil, structerr.NewInternal("creating object quarantine: %w", err) + return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) } quarantineRepo := s.localrepo(quarantineDir.QuarantinedRepo()) - return quarantineDir, quarantineRepo, nil + return quarantineDir, quarantineRepo, cleanup, nil } diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go index 7ad88c469..e1d81ec7d 100644 --- a/internal/tempdir/tempdir.go +++ b/internal/tempdir/tempdir.go @@ -17,7 +17,6 @@ import ( type Dir struct { logger log.Logger path string - doneCh chan struct{} } // Path returns the absolute path of the temporary directory. @@ -27,67 +26,73 @@ func (d Dir) Path() string { // New returns the path of a new temporary directory for the given storage. The directory is removed // asynchronously with os.RemoveAll when the context expires. -func New(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (Dir, error) { - return NewWithPrefix(ctx, storageName, "repo", logger, locator) +func New(ctx context.Context, storageName string, locator storage.Locator) (Dir, func() error, error) { + return NewWithPrefix(ctx, storageName, "repo", locator) } // NewWithPrefix returns the path of a new temporary directory for the given storage with a specific // prefix used to create the temporary directory's name. The directory is removed asynchronously // with os.RemoveAll when the context expires. -func NewWithPrefix(ctx context.Context, storageName, prefix string, logger log.Logger, locator storage.Locator) (Dir, error) { - dir, err := newDirectory(ctx, storageName, prefix, logger, locator) +func NewWithPrefix(ctx context.Context, storageName, prefix string, locator storage.Locator) (Dir, func() error, error) { + dir, cleanup, err := newDirectory(ctx, storageName, prefix, locator) if err != nil { - return Dir{}, err + return Dir{}, nil, err } - go dir.cleanupOnDone(ctx) - - return dir, nil + return dir, cleanup, nil } // NewWithoutContext returns a temporary directory for the given storage suitable which is not // storage scoped. The temporary directory will thus not get cleaned up when the context expires, // but instead when the temporary directory is older than MaxAge. -func NewWithoutContext(storageName string, logger log.Logger, locator storage.Locator) (Dir, error) { +func NewWithoutContext(storageName string, locator storage.Locator) (Dir, func() error, error) { prefix := fmt.Sprintf("%s-repositories.old.%d.", storageName, time.Now().Unix()) return newDirectory(context.Background(), storageName, prefix, logger, locator) } // NewRepository is the same as New, but it returns a *gitalypb.Repository for the created directory // as well as the bare path as a string. -func NewRepository(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (*gitalypb.Repository, Dir, error) { +func NewRepository(ctx context.Context, storageName string, locator storage.Locator) (*gitalypb.Repository, Dir, func() error, error) { storagePath, err := locator.GetStorageByName(storageName) if err != nil { - return nil, Dir{}, err + return nil, Dir{}, nil, err } - dir, err := New(ctx, storageName, logger, locator) + dir, cleanup, err := New(ctx, storageName, locator) if err != nil { - return nil, Dir{}, err + return nil, Dir{}, nil, err } newRepo := &gitalypb.Repository{StorageName: storageName} newRepo.RelativePath, err = filepath.Rel(storagePath, dir.Path()) if err != nil { - return nil, Dir{}, err + return nil, Dir{}, nil, err } - return newRepo, dir, nil + return newRepo, dir, cleanup, nil } -func newDirectory(ctx context.Context, storageName string, prefix string, logger log.Logger, loc storage.Locator) (Dir, error) { +func newDirectory(ctx context.Context, storageName string, prefix string, loc storage.Locator) (Dir, func() error, error) { root, err := loc.TempDir(storageName) if err != nil { - return Dir{}, fmt.Errorf("temp directory: %w", err) + return Dir{}, nil, fmt.Errorf("temp directory: %w", err) } if err := os.MkdirAll(root, perm.PrivateDir); err != nil { - return Dir{}, err + return Dir{}, nil, err } tempDir, err := os.MkdirTemp(root, prefix) if err != nil { - return Dir{}, err + return Dir{}, nil, err + } + + cleanup := func() error { + if err := os.RemoveAll(tempDir); err != nil { + log.FromContext(ctx).WithError(err).Warn("failed to remove temporary directory") + return err + } + return nil } return Dir{ @@ -109,4 +114,5 @@ func (d Dir) cleanupOnDone(ctx context.Context) { // call. This is mainly intended for use in tests. func (d Dir) WaitForCleanup() { <-d.doneCh + }, cleanup, err } |