*: don't check in repository pool methods if is a siva/git valid repositoriy when is added

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

Author: mcarmonaa

cmd/gitbase/command/server.go

@@ -15,7 +15,6 @@ import (
 	gopilosa "github.com/pilosa/go-pilosa"
 	"github.com/sirupsen/logrus"
 	"github.com/uber/jaeger-client-go/config"
-	git "gopkg.in/src-d/go-git.v4"
 	sqle "gopkg.in/src-d/go-mysql-server.v0"
 	"gopkg.in/src-d/go-mysql-server.v0/server"
 	"gopkg.in/src-d/go-mysql-server.v0/sql"
@@ -258,30 +257,8 @@ func (c *Server) addMatch(match string) error {
 		}
 
 		if info.IsDir() {
-			ok, err := isGitRepo(path)
-			if err != nil {
-				logrus.WithFields(logrus.Fields{
-					"path":  path,
-					"error": err,
-				}).Error("path couldn't be inspected")
-
-				return filepath.SkipDir
-			}
-
-			if ok {
-				if !c.DisableGit {
-					if err := c.pool.AddGitWithID(info.Name(), path); err != nil {
-						logrus.WithFields(logrus.Fields{
-							"id":    info.Name(),
-							"path":  path,
-							"error": err,
-						}).Error("repository could not be added")
-					}
-
-					logrus.WithField("path", path).Debug("repository added")
-				}
-
-				return filepath.SkipDir
+			if err := c.addIfGitRepo(path); err != nil {
+				return err
 			}
 
 			depth := strings.Count(path, string(os.PathSeparator)) - initDepth
@@ -293,7 +270,7 @@ func (c *Server) addMatch(match string) error {
 		}
 
 		if !c.DisableSiva &&
-			info.Mode().IsRegular() && strings.HasSuffix(info.Name(), ".siva") {
+			info.Mode().IsRegular() && gitbase.IsSivaFile(info.Name()) {
 			if err := c.pool.AddSivaFile(path); err != nil {
 				logrus.WithFields(logrus.Fields{
 					"path":  path,
@@ -310,14 +287,34 @@ func (c *Server) addMatch(match string) error {
 	})
 }
 
-func isGitRepo(path string) (bool, error) {
-	if _, err := git.PlainOpen(path); err != nil {
-		if git.ErrRepositoryNotExists == err {
-			return false, nil
+func (c *Server) addIfGitRepo(path string) error {
+	ok, err := gitbase.IsGitRepo(path)
+	if err != nil {
+		logrus.WithFields(logrus.Fields{
+			"path":  path,
+			"error": err,
+		}).Error("path couldn't be inspected")
+
+		return filepath.SkipDir
+	}
+
+	if ok {
+		if !c.DisableGit {
+			base := filepath.Base(path)
+			if err := c.pool.AddGitWithID(base, path); err != nil {
+				logrus.WithFields(logrus.Fields{
+					"id":    base,
+					"path":  path,
+					"error": err,
+				}).Error("repository could not be added")
+			}
+
+			logrus.WithField("path", path).Debug("repository added")
 		}
 
-		return false, err
+		// either the repository is added or not, the path must be skipped
+		return filepath.SkipDir
 	}
 
-	return true, nil
+	return nil
 }

common_test.go

@@ -38,10 +38,14 @@ func buildSession(t *testing.T, repos fixtures.Fixtures,
 	pool := NewRepositoryPool()
 	for _, fixture := range repos {
 		path := fixture.Worktree().Root()
-		if err := pool.AddGit(path); err == nil {
-			_, err := pool.GetRepo(path)
-			require.NoError(err)
-			paths = append(paths, path)
+		ok, err := IsGitRepo(path)
+		require.NoError(err)
+		if ok {
+			if err := pool.AddGit(path); err == nil {
+				_, err := pool.GetRepo(path)
+				require.NoError(err)
+				paths = append(paths, path)
+			}
 		}
 	}
 

integration_test.go

@@ -419,9 +419,8 @@ func TestMissingHeadRefs(t *testing.T) {
 				return err
 			}
 
-			err = pool.AddSivaFile(path)
-			if err != nil {
-				require.EqualError(err, "the repository is not: siva")
+			if gitbase.IsSivaFile(path) {
+				require.NoError(pool.AddSivaFile(path))
 			}
 
 			return nil

path_utils.go

@@ -3,6 +3,9 @@ package gitbase
 import (
 	"path/filepath"
 	"regexp"
+	"strings"
+
+	git "gopkg.in/src-d/go-git.v4"
 )
 
 // RegMatchChars matches a string with a glob expression inside.
@@ -32,3 +35,21 @@ func removeDsStore(matches []string) []string {
 	}
 	return result
 }
+
+// IsGitRepo checks that the given path is a git repository.
+func IsGitRepo(path string) (bool, error) {
+	if _, err := git.PlainOpen(path); err != nil {
+		if git.ErrRepositoryNotExists == err {
+			return false, nil
+		}
+
+		return false, err
+	}
+
+	return true, nil
+}
+
+//IsSivaFile checks that the given file is a siva file.
+func IsSivaFile(file string) bool {
+	return strings.HasSuffix(file, ".siva")
+}

path_utils_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	fixtures "gopkg.in/src-d/go-git-fixtures.v3"
 )
 
 func TestPatternMatches(t *testing.T) {
@@ -40,3 +41,23 @@ func TestPatternMatches(t *testing.T) {
 		})
 	}
 }
+
+func TestIsGitRepo(t *testing.T) {
+	var require = require.New(t)
+
+	ok, err := IsGitRepo("/do/not/exist")
+	require.NoError(err)
+	require.False(ok)
+
+	path := fixtures.Basic().ByTag("worktree").One().Worktree().Root()
+	ok, err = IsGitRepo(path)
+	require.NoError(err)
+	require.True(ok)
+}
+
+func TestIsSivaFile(t *testing.T) {
+	var require = require.New(t)
+
+	require.True(IsSivaFile("is.siva"))
+	require.False(IsSivaFile("not-siva"))
+}

repository_pool.go

@@ -5,7 +5,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"strings"
 	"sync"
 	"sync/atomic"
 
@@ -181,24 +180,13 @@ func (p *RepositoryPool) AddGit(path string) error {
 // AddGitWithID checks if a git repository can be opened and adds it to the
 // pool. ID should be specified.
 func (p *RepositoryPool) AddGitWithID(id, path string) error {
-	_, err := git.PlainOpen(path)
-	if err != nil {
-		return errRepoCannotOpen.Wrap(err, path)
-	}
-
 	return p.Add(gitRepo(id, path))
 }
 
 // AddSivaFile adds to the pool the given file if it's a siva repository,
 // that is, has the .siva extension
 func (p *RepositoryPool) AddSivaFile(path string) error {
-	file := filepath.Base(path)
-	if !strings.HasSuffix(file, ".siva") {
-		return errInvalidRepoKind.New("siva")
-	}
-
-	p.Add(sivaRepo(path, path))
-	return nil
+	return p.Add(sivaRepo(path, path))
 }
 
 // GetPos retrieves a repository at a given position. If the position is

repository_pool_test.go

@@ -87,10 +87,6 @@ func TestRepositoryPoolGit(t *testing.T) {
 
 	pool := NewRepositoryPool()
 
-	err := pool.AddGit("/do/not/exist")
-	require.Error(err)
-	require.True(errRepoCannotOpen.Is(err))
-
 	require.NoError(pool.AddGit(path))
 
 	repo, err := pool.GetPos(0)
@@ -252,9 +248,8 @@ func TestRepositoryPoolSiva(t *testing.T) {
 				return err
 			}
 
-			err = pool.AddSivaFile(path)
-			if err != nil {
-				require.True(errInvalidRepoKind.Is(err))
+			if IsSivaFile(path) {
+				require.NoError(pool.AddSivaFile(path))
 			}
 
 			return nil

squash_iterator_test.go

@@ -726,7 +726,12 @@ func setupIterWithErrors(t *testing.T, badRepo bool, skipErrors bool) (*sql.Cont
 	}
 
 	for _, f := range fixtures.ByTag("worktree") {
-		pool.AddGit(f.Worktree().Root())
+		path := f.Worktree().Root()
+		ok, err := IsGitRepo(path)
+		require.NoError(t, err)
+		if ok {
+			pool.AddGit(f.Worktree().Root())
+		}
 	}
 
 	session := NewSession(pool, WithSkipGitErrors(skipErrors))