improve cleanup implementation, add more tests

Signed-off-by: Santiago M. Mola <[email protected]>

Author: smola

Diff for: 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,65 +343,19 @@ 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
 	}
 
 	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 for: 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) {