config: RefSpec.Validate returning errors and doc (Fixes #232)

3 files changed, 47 insertions, 21 deletions

diff --git a/config/config.go b/config/config.go
index 15844be..866ae8e 100644
--- a/config/config.go
+++ b/config/config.go
@@ -1,4 +1,4 @@
-// Package config contains the abstraction of config files
+// Package config contains the abstraction of multiple config files
 package config
 
 import (
@@ -89,8 +89,7 @@ func (c *Config) Unmarshal(b []byte) error {
 	}
 
 	c.unmarshalCore()
-	c.unmarshalRemotes()
-	return nil
+	return c.unmarshalRemotes()
 }
 
 func (c *Config) unmarshalCore() {
@@ -100,14 +99,18 @@ func (c *Config) unmarshalCore() {
 	}
 }
 
-func (c *Config) unmarshalRemotes() {
+func (c *Config) unmarshalRemotes() error {
 	s := c.raw.Section(remoteSection)
 	for _, sub := range s.Subsections {
 		r := &RemoteConfig{}
-		r.unmarshal(sub)
+		if err := r.unmarshal(sub); err != nil {
+			return err
+		}
 
 		c.Remotes[r.Name] = r
 	}
+
+	return nil
 }
 
 // Marshal returns Config encoded as a git-config file
@@ -163,6 +166,12 @@ func (c *RemoteConfig) Validate() error {
 		return ErrRemoteConfigEmptyURL
 	}
 
+	for _, r := range c.Fetch {
+		if err := r.Validate(); err != nil {
+			return err
+		}
+	}
+
 	if len(c.Fetch) == 0 {
 		c.Fetch = []RefSpec{RefSpec(fmt.Sprintf(DefaultFetchRefSpec, c.Name))}
 	}
@@ -170,20 +179,24 @@ func (c *RemoteConfig) Validate() error {
 	return nil
 }
 
-func (c *RemoteConfig) unmarshal(s *format.Subsection) {
+func (c *RemoteConfig) unmarshal(s *format.Subsection) error {
 	c.raw = s
 
 	fetch := []RefSpec{}
 	for _, f := range c.raw.Options.GetAll(fetchKey) {
 		rs := RefSpec(f)
-		if rs.IsValid() {
-			fetch = append(fetch, rs)
+		if err := rs.Validate(); err != nil {
+			return err
 		}
+
+		fetch = append(fetch, rs)
 	}
 
 	c.Name = c.raw.Name
 	c.URL = c.raw.Option(urlKey)
 	c.Fetch = fetch
+
+	return nil
 }
 
 func (c *RemoteConfig) marshal() *format.Subsection {
diff --git a/config/refspec.go b/config/refspec.go
index dca0d82..dd68edc 100644
--- a/config/refspec.go
+++ b/config/refspec.go
@@ -1,6 +1,7 @@
 package config
 
 import (
+	"errors"
 	"strings"
 
 	"srcd.works/go-git.v4/plumbing"
@@ -12,6 +13,11 @@ const (
 	refSpecSeparator = ":"
 )
 
+var (
+	ErrRefSpecMalformedSeparator = errors.New("malformed refspec, separators are wrong")
+	ErrRefSpecMalformedWildcard  = errors.New("malformed refspec, missmatched number of wildcards")
+)
+
 // RefSpec is a mapping from local branches to remote references
 // The format of the refspec is an optional +, followed by <src>:<dst>, where
 // <src> is the pattern for references on the remote side and <dst> is where
@@ -22,21 +28,25 @@ const (
 // https://git-scm.com/book/es/v2/Git-Internals-The-Refspec
 type RefSpec string
 
-// IsValid validates the RefSpec
-func (s RefSpec) IsValid() bool {
+// Validate validates the RefSpec
+func (s RefSpec) Validate() error {
 	spec := string(s)
 	if strings.Count(spec, refSpecSeparator) != 1 {
-		return false
+		return ErrRefSpecMalformedSeparator
 	}
 
 	sep := strings.Index(spec, refSpecSeparator)
-	if sep == len(spec) {
-		return false
+	if sep == len(spec)-1 {
+		return ErrRefSpecMalformedSeparator
 	}
 
 	ws := strings.Count(spec[0:sep], refSpecWildcard)
 	wd := strings.Count(spec[sep+1:], refSpecWildcard)
-	return ws == wd && ws < 2 && wd < 2
+	if ws == wd && ws < 2 && wd < 2 {
+		return nil
+	}
+
+	return ErrRefSpecMalformedWildcard
 }
 
 // IsForceUpdate returns if update is allowed in non fast-forward merges
diff --git a/config/refspec_test.go b/config/refspec_test.go
index 6f97422..6672cd2 100644
--- a/config/refspec_test.go
+++ b/config/refspec_test.go
@@ -15,25 +15,28 @@ func Test(t *testing.T) { TestingT(t) }
 
 func (s *RefSpecSuite) TestRefSpecIsValid(c *C) {
 	spec := RefSpec("+refs/heads/*:refs/remotes/origin/*")
-	c.Assert(spec.IsValid(), Equals, true)
+	c.Assert(spec.Validate(), Equals, nil)
 
 	spec = RefSpec("refs/heads/*:refs/remotes/origin/")
-	c.Assert(spec.IsValid(), Equals, false)
+	c.Assert(spec.Validate(), Equals, ErrRefSpecMalformedWildcard)
 
 	spec = RefSpec("refs/heads/master:refs/remotes/origin/master")
-	c.Assert(spec.IsValid(), Equals, true)
+	c.Assert(spec.Validate(), Equals, nil)
 
 	spec = RefSpec(":refs/heads/master")
-	c.Assert(spec.IsValid(), Equals, true)
+	c.Assert(spec.Validate(), Equals, nil)
 
 	spec = RefSpec(":refs/heads/*")
-	c.Assert(spec.IsValid(), Equals, false)
+	c.Assert(spec.Validate(), Equals, ErrRefSpecMalformedWildcard)
 
 	spec = RefSpec(":*")
-	c.Assert(spec.IsValid(), Equals, false)
+	c.Assert(spec.Validate(), Equals, ErrRefSpecMalformedWildcard)
 
 	spec = RefSpec("refs/heads/*")
-	c.Assert(spec.IsValid(), Equals, false)
+	c.Assert(spec.Validate(), Equals, ErrRefSpecMalformedSeparator)
+
+	spec = RefSpec("refs/heads:")
+	c.Assert(spec.Validate(), Equals, ErrRefSpecMalformedSeparator)
 }
 
 func (s *RefSpecSuite) TestRefSpecIsForceUpdate(c *C) {