aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorSantiago M. Mola <santi@mola.io>2018-11-27 11:42:07 +0100
committerSantiago M. Mola <santi@mola.io>2018-11-27 13:18:52 +0100
commit7441885e61650066f1b3ffa948caa86f9410bc86 (patch)
treebccbba050baaac17657165b679ebf2197f59b629
parentf62cd8e3495579a8323455fa0c4e6c44bb0d5e09 (diff)
downloadgo-git-7441885e61650066f1b3ffa948caa86f9410bc86.tar.gz
repository: fix plain clone error handling regression
PR #1008 introduced a regression by changing the errors returned by PlainClone when a repository did not exist. This change goes back to returned errors as they were in v4.7.0. Fixes #1027 Signed-off-by: Santiago M. Mola <santi@mola.io>
-rw-r--r--repository.go25
-rw-r--r--repository_test.go51
2 files changed, 57 insertions, 19 deletions
diff --git a/repository.go b/repository.go
index f548e9a..97134ec 100644
--- a/repository.go
+++ b/repository.go
@@ -342,8 +342,9 @@ func PlainClone(path string, isBare bool, o *CloneOptions) (*Repository, error)
// 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 @@ func PlainCloneContext(ctx context.Context, path string, isBare bool, o *CloneOp
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 newRepository(s storage.Storer, worktree billy.Filesystem) *Repository {
}
}
-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 @@ func cleanUpDir(path string, all bool) error {
}
}
- 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 @@ import (
"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 @@ func (s *RepositorySuite) TestCloneContext(c *C) {
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 @@ func (s *RepositorySuite) TestPlainCloneWithRemoteName(c *C) {
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 @@ func (s *RepositorySuite) TestPlainCloneContextWithProperParameters(c *C) {
})
c.Assert(r, NotNil)
- c.Assert(err, NotNil)
+ c.Assert(err, ErrorMatches, ".* context canceled")
}
func (s *RepositorySuite) TestPlainCloneContextNonExistentWithExistentDir(c *C) {
@@ -604,7 +619,7 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithExistentDir(c *C)
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 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNonExistentDir(c *
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 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotDir(c *C) {
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 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotEmptyDir(c *C)
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) {