Merge pull request #1028 from smola/clone-regression
repository: fix plain clone error handling regression
diff --git a/repository.go b/repository.go
index f548e9a..97134ec 100644
--- a/repository.go
+++ b/repository.go
@@ -342,8 +342,9 @@
// transport operations.
//
// TODO(mcuadros): move isBare to CloneOptions in v5
+// TODO(smola): refuse upfront to clone on a non-empty directory in v5, see #1027
func PlainCloneContext(ctx context.Context, path string, isBare bool, o *CloneOptions) (*Repository, error) {
- dirExists, err := checkExistsAndIsEmptyDir(path)
+ cleanup, cleanupParent, err := checkIfCleanupIsNeeded(path)
if err != nil {
return nil, err
}
@@ -355,7 +356,9 @@
err = r.clone(ctx, o)
if err != nil && err != ErrRepositoryAlreadyExists {
- cleanUpDir(path, !dirExists)
+ if cleanup {
+ cleanUpDir(path, cleanupParent)
+ }
}
return r, err
@@ -369,37 +372,37 @@
}
}
-func checkExistsAndIsEmptyDir(path string) (exists bool, err error) {
+func checkIfCleanupIsNeeded(path string) (cleanup bool, cleanParent bool, err error) {
fi, err := os.Stat(path)
if err != nil {
if os.IsNotExist(err) {
- return false, nil
+ return true, true, nil
}
- return false, err
+ return false, false, err
}
if !fi.IsDir() {
- return false, fmt.Errorf("path is not a directory: %s", path)
+ return false, false, fmt.Errorf("path is not a directory: %s", path)
}
f, err := os.Open(path)
if err != nil {
- return false, err
+ return false, false, err
}
defer ioutil.CheckClose(f, &err)
_, err = f.Readdirnames(1)
if err == io.EOF {
- return true, nil
+ return true, false, nil
}
if err != nil {
- return true, err
+ return false, false, err
}
- return true, fmt.Errorf("directory is not empty: %s", path)
+ return false, false, nil
}
func cleanUpDir(path string, all bool) error {
@@ -425,7 +428,7 @@
}
}
- return nil
+ return err
}
// Config return the repository config
diff --git a/repository_test.go b/repository_test.go
index 59ab89e..70e344e 100644
--- a/repository_test.go
+++ b/repository_test.go
@@ -21,6 +21,7 @@
"gopkg.in/src-d/go-git.v4/plumbing/cache"
"gopkg.in/src-d/go-git.v4/plumbing/object"
"gopkg.in/src-d/go-git.v4/plumbing/storer"
+ "gopkg.in/src-d/go-git.v4/plumbing/transport"
"gopkg.in/src-d/go-git.v4/storage"
"gopkg.in/src-d/go-git.v4/storage/filesystem"
"gopkg.in/src-d/go-git.v4/storage/memory"
@@ -177,11 +178,12 @@
ctx, cancel := context.WithCancel(context.Background())
cancel()
- _, err := CloneContext(ctx, memory.NewStorage(), nil, &CloneOptions{
+ r, err := CloneContext(ctx, memory.NewStorage(), nil, &CloneOptions{
URL: s.GetBasicLocalRepositoryURL(),
})
- c.Assert(err, NotNil)
+ c.Assert(r, NotNil)
+ c.Assert(err, ErrorMatches, ".* context canceled")
}
func (s *RepositorySuite) TestCloneWithTags(c *C) {
@@ -581,7 +583,20 @@
c.Assert(remote, NotNil)
}
-func (s *RepositorySuite) TestPlainCloneContextWithProperParameters(c *C) {
+func (s *RepositorySuite) TestPlainCloneOverExistingGitDirectory(c *C) {
+ tmpDir := c.MkDir()
+ r, err := PlainInit(tmpDir, false)
+ c.Assert(r, NotNil)
+ c.Assert(err, IsNil)
+
+ r, err = PlainClone(tmpDir, false, &CloneOptions{
+ URL: s.GetBasicLocalRepositoryURL(),
+ })
+ c.Assert(r, IsNil)
+ c.Assert(err, Equals, ErrRepositoryAlreadyExists)
+}
+
+func (s *RepositorySuite) TestPlainCloneContextCancel(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
@@ -590,7 +605,7 @@
})
c.Assert(r, NotNil)
- c.Assert(err, NotNil)
+ c.Assert(err, ErrorMatches, ".* context canceled")
}
func (s *RepositorySuite) TestPlainCloneContextNonExistentWithExistentDir(c *C) {
@@ -604,7 +619,7 @@
URL: "incorrectOnPurpose",
})
c.Assert(r, NotNil)
- c.Assert(err, NotNil)
+ c.Assert(err, Equals, transport.ErrRepositoryNotFound)
_, err = os.Stat(repoDir)
c.Assert(os.IsNotExist(err), Equals, false)
@@ -625,7 +640,7 @@
URL: "incorrectOnPurpose",
})
c.Assert(r, NotNil)
- c.Assert(err, NotNil)
+ c.Assert(err, Equals, transport.ErrRepositoryNotFound)
_, err = os.Stat(repoDir)
c.Assert(os.IsNotExist(err), Equals, true)
@@ -645,7 +660,7 @@
URL: "incorrectOnPurpose",
})
c.Assert(r, IsNil)
- c.Assert(err, NotNil)
+ c.Assert(err, ErrorMatches, ".*not a directory.*")
fi, err := os.Stat(repoDir)
c.Assert(err, IsNil)
@@ -668,8 +683,28 @@
r, err := PlainCloneContext(ctx, repoDirPath, false, &CloneOptions{
URL: "incorrectOnPurpose",
})
+ c.Assert(r, NotNil)
+ c.Assert(err, Equals, transport.ErrRepositoryNotFound)
+
+ _, err = os.Stat(dummyFile)
+ c.Assert(err, IsNil)
+
+}
+
+func (s *RepositorySuite) TestPlainCloneContextNonExistingOverExistingGitDirectory(c *C) {
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+
+ tmpDir := c.MkDir()
+ r, err := PlainInit(tmpDir, false)
+ c.Assert(r, NotNil)
+ c.Assert(err, IsNil)
+
+ r, err = PlainCloneContext(ctx, tmpDir, false, &CloneOptions{
+ URL: "incorrectOnPurpose",
+ })
c.Assert(r, IsNil)
- c.Assert(err, NotNil)
+ c.Assert(err, Equals, ErrRepositoryAlreadyExists)
}
func (s *RepositorySuite) TestPlainCloneWithRecurseSubmodules(c *C) {
