From 507681b9a317a91c176460b6aaff1915ba4b9533 Mon Sep 17 00:00:00 2001 From: Bartek Jaroszewski Date: Wed, 18 Jul 2018 11:43:00 +0200 Subject: repository: added cleanup for the PlainCloneContext() Signed-off-by: Bartek Jaroszewski --- repository.go | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++- repository_test.go | 48 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 102 insertions(+), 6 deletions(-) diff --git a/repository.go b/repository.go index 507ff44..bb89b28 100644 --- a/repository.go +++ b/repository.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "io" stdioutil "io/ioutil" "os" "path" @@ -50,6 +51,7 @@ var ( ErrIsBareRepository = errors.New("worktree not available in a bare repository") ErrUnableToResolveCommit = errors.New("unable to resolve commit") ErrPackedObjectsNotSupported = errors.New("Packed objects not supported") + ErrDirNotEmpty = errors.New("directory is not empty") ) // Repository represents a git repository @@ -342,12 +344,68 @@ func PlainClone(path string, isBare bool, o *CloneOptions) (*Repository, error) // // TODO(mcuadros): move isBare to CloneOptions in v5 func PlainCloneContext(ctx context.Context, path string, isBare bool, o *CloneOptions) (*Repository, error) { + dirEmpty := false + dirExist := false + + file, err := os.Stat(path) + if err != nil { + return nil, err + } + + if !os.IsNotExist(err) { + dirExist = file.IsDir() + } + + if dirExist { + fh, err := os.Open(path) + if err != nil { + return nil, err + } + defer fh.Close() + + names, err := fh.Readdirnames(1) + if err != io.EOF && err != nil { + return nil, err + } + if len(names) == 0 { + dirEmpty = true + } else { + return nil, ErrDirNotEmpty + } + } + r, err := PlainInit(path, isBare) if err != nil { return nil, err } - return r, r.clone(ctx, o) + err = r.clone(ctx, o) + if err != nil && err != ErrRepositoryAlreadyExists { + if dirEmpty { + fh, err := os.Open(path) + if err != nil { + return nil, err + } + defer fh.Close() + + names, err := fh.Readdirnames(-1) + if err != nil && err != io.EOF { + return nil, err + } + + for _, name := range names { + err = os.RemoveAll(filepath.Join(path, name)) + if err != nil { + return nil, err + } + } + } else if !dirExist { + os.RemoveAll(path) + return nil, err + } + } + + return r, err } func newRepository(s storage.Storer, worktree billy.Filesystem) *Repository { diff --git a/repository_test.go b/repository_test.go index 07c3570..887901f 100644 --- a/repository_test.go +++ b/repository_test.go @@ -581,17 +581,55 @@ func (s *RepositorySuite) TestPlainCloneWithRemoteName(c *C) { c.Assert(remote, NotNil) } -func (s *RepositorySuite) TestPlainCloneContext(c *C) { +func (s *RepositorySuite) TestPlainCloneContextWithProperParameters(c *C) { ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := PlainCloneContext(ctx, c.MkDir(), false, &CloneOptions{ + r, err := PlainCloneContext(ctx, c.MkDir(), false, &CloneOptions{ URL: s.GetBasicLocalRepositoryURL(), }) + c.Assert(r, NotNil) c.Assert(err, NotNil) } +func (s *RepositorySuite) TestPlainCloneContextWithIncorrectRepo(c *C) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + tmpDir := c.MkDir() + repoDir := filepath.Join(tmpDir, "repoDir") + r, err := PlainCloneContext(ctx, repoDir, false, &CloneOptions{ + URL: "incorrectOnPurpose", + }) + c.Assert(r, IsNil) + c.Assert(err, NotNil) + + _, err = os.Stat(repoDir) + dirNotExist := os.IsNotExist(err) + c.Assert(dirNotExist, Equals, true) +} + +func (s *RepositorySuite) TestPlainCloneContextWithNotEmptyDir(c *C) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + tmpDir := c.MkDir() + repoDirPath := filepath.Join(tmpDir, "repoDir") + err := os.Mkdir(repoDirPath, 0777) + c.Assert(err, IsNil) + + dummyFile := filepath.Join(repoDirPath, "dummyFile") + err = ioutil.WriteFile(dummyFile, []byte(fmt.Sprint("dummyContent")), 0644) + c.Assert(err, IsNil) + + r, err := PlainCloneContext(ctx, repoDirPath, false, &CloneOptions{ + URL: "incorrectOnPurpose", + }) + c.Assert(r, IsNil) + c.Assert(err, Equals, ErrDirNotEmpty) +} + func (s *RepositorySuite) TestPlainCloneWithRecurseSubmodules(c *C) { if testing.Short() { c.Skip("skipping test in short mode.") @@ -2104,9 +2142,9 @@ func (s *RepositorySuite) TestResolveRevisionWithErrors(c *C) { c.Assert(err, IsNil) datas := map[string]string{ - "efs/heads/master~": "reference not found", - "HEAD^3": `Revision invalid : "3" found must be 0, 1 or 2 after "^"`, - "HEAD^{/whatever}": `No commit message match regexp : "whatever"`, + "efs/heads/master~": "reference not found", + "HEAD^3": `Revision invalid : "3" found must be 0, 1 or 2 after "^"`, + "HEAD^{/whatever}": `No commit message match regexp : "whatever"`, "4e1243bd22c66e76c2ba9eddc1f91394e57f9f83": "reference not found", "918c48b83bd081e863dbe1b80f8998f058cd8294": `refname "918c48b83bd081e863dbe1b80f8998f058cd8294" is ambiguous`, } -- cgit From 3332e8d67dbe810d1607285d43fcd79470cf9961 Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Tue, 30 Oct 2018 13:10:00 +0100 Subject: improve cleanup implementation, add more tests Signed-off-by: Santiago M. Mola --- repository.go | 110 +++++++++++++++++++++++++++++------------------------ repository_test.go | 54 +++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 55 deletions(-) diff --git a/repository.go b/repository.go index bb89b28..9420af9 100644 --- a/repository.go +++ b/repository.go @@ -51,7 +51,6 @@ var ( ErrIsBareRepository = errors.New("worktree not available in a bare repository") ErrUnableToResolveCommit = errors.New("unable to resolve commit") ErrPackedObjectsNotSupported = errors.New("Packed objects not supported") - ErrDirNotEmpty = errors.New("directory is not empty") ) // Repository represents a git repository @@ -344,36 +343,11 @@ func PlainClone(path string, isBare bool, o *CloneOptions) (*Repository, error) // // TODO(mcuadros): move isBare to CloneOptions in v5 func PlainCloneContext(ctx context.Context, path string, isBare bool, o *CloneOptions) (*Repository, error) { - dirEmpty := false - dirExist := false - - file, err := os.Stat(path) + dirExists, err := checkExistsAndIsEmptyDir(path) if err != nil { return nil, err } - if !os.IsNotExist(err) { - dirExist = file.IsDir() - } - - if dirExist { - fh, err := os.Open(path) - if err != nil { - return nil, err - } - defer fh.Close() - - names, err := fh.Readdirnames(1) - if err != io.EOF && err != nil { - return nil, err - } - if len(names) == 0 { - dirEmpty = true - } else { - return nil, ErrDirNotEmpty - } - } - r, err := PlainInit(path, isBare) if err != nil { return nil, err @@ -381,28 +355,7 @@ func PlainCloneContext(ctx context.Context, path string, isBare bool, o *CloneOp err = r.clone(ctx, o) if err != nil && err != ErrRepositoryAlreadyExists { - if dirEmpty { - fh, err := os.Open(path) - if err != nil { - return nil, err - } - defer fh.Close() - - names, err := fh.Readdirnames(-1) - if err != nil && err != io.EOF { - return nil, err - } - - for _, name := range names { - err = os.RemoveAll(filepath.Join(path, name)) - if err != nil { - return nil, err - } - } - } else if !dirExist { - os.RemoveAll(path) - return nil, err - } + cleanUpDir(path, !dirExists) } return r, err @@ -416,6 +369,65 @@ func newRepository(s storage.Storer, worktree billy.Filesystem) *Repository { } } +func checkExistsAndIsEmptyDir(path string) (exists bool, err error) { + fi, err := os.Stat(path) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + + return false, err + } + + if !fi.IsDir() { + return false, fmt.Errorf("path is not a directory: %s", path) + } + + f, err := os.Open(path) + if err != nil { + return false, err + } + + defer ioutil.CheckClose(f, &err) + + _, err = f.Readdirnames(1) + if err == io.EOF { + return true, nil + } + + if err != nil { + return true, err + } + + return true, fmt.Errorf("directory is not empty: %s", path) +} + +func cleanUpDir(path string, all bool) error { + if all { + return os.RemoveAll(path) + } + + f, err := os.Open(path) + if err != nil { + return err + } + + defer ioutil.CheckClose(f, &err) + + names, err := f.Readdirnames(-1) + if err != nil { + return err + } + + for _, name := range names { + if err := os.RemoveAll(filepath.Join(path, name)); err != nil { + return err + } + } + + return nil +} + // Config return the repository config func (r *Repository) Config() (*config.Config, error) { return r.Storer.Config() diff --git a/repository_test.go b/repository_test.go index 887901f..ba2cf1a 100644 --- a/repository_test.go +++ b/repository_test.go @@ -593,24 +593,66 @@ func (s *RepositorySuite) TestPlainCloneContextWithProperParameters(c *C) { c.Assert(err, NotNil) } -func (s *RepositorySuite) TestPlainCloneContextWithIncorrectRepo(c *C) { +func (s *RepositorySuite) TestPlainCloneContextNonExistentWithExistentDir(c *C) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + tmpDir := c.MkDir() + repoDir := tmpDir + + r, err := PlainCloneContext(ctx, repoDir, false, &CloneOptions{ + URL: "incorrectOnPurpose", + }) + c.Assert(r, NotNil) + c.Assert(err, NotNil) + + _, err = os.Stat(repoDir) + c.Assert(os.IsNotExist(err), Equals, false) + + names, err := ioutil.ReadDir(repoDir) + c.Assert(err, IsNil) + c.Assert(names, HasLen, 0) +} + +func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNonExistentDir(c *C) { ctx, cancel := context.WithCancel(context.Background()) cancel() tmpDir := c.MkDir() repoDir := filepath.Join(tmpDir, "repoDir") + r, err := PlainCloneContext(ctx, repoDir, false, &CloneOptions{ URL: "incorrectOnPurpose", }) - c.Assert(r, IsNil) + c.Assert(r, NotNil) c.Assert(err, NotNil) _, err = os.Stat(repoDir) - dirNotExist := os.IsNotExist(err) - c.Assert(dirNotExist, Equals, true) + c.Assert(os.IsNotExist(err), Equals, true) +} + +func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotDir(c *C) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + tmpDir := c.MkDir() + repoDir := filepath.Join(tmpDir, "repoDir") + f, err := os.Create(repoDir) + c.Assert(err, IsNil) + c.Assert(f.Close(), IsNil) + + r, err := PlainCloneContext(ctx, repoDir, false, &CloneOptions{ + URL: "incorrectOnPurpose", + }) + c.Assert(r, IsNil) + c.Assert(err, NotNil) + + fi, err := os.Stat(repoDir) + c.Assert(err, IsNil) + c.Assert(fi.IsDir(), Equals, false) } -func (s *RepositorySuite) TestPlainCloneContextWithNotEmptyDir(c *C) { +func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotEmptyDir(c *C) { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -627,7 +669,7 @@ func (s *RepositorySuite) TestPlainCloneContextWithNotEmptyDir(c *C) { URL: "incorrectOnPurpose", }) c.Assert(r, IsNil) - c.Assert(err, Equals, ErrDirNotEmpty) + c.Assert(err, NotNil) } func (s *RepositorySuite) TestPlainCloneWithRecurseSubmodules(c *C) { -- cgit