improve ResolveRevision's Ref lookup path

1) lookups on an annotated tag oid now work 2) there was a lot of complexity around detection of ambiguity, but unlike git, ambiguous refs are rejected (which causes bugs like src-d#823). The new code matches rev-parse's behavior (prefer the OID), though there is no warning path to report the same warning. Signed-off-by: Mike Lundy <[email protected]>
patrickdevivo · May 14, 2019 · 33f05f3 · 33f05f3
1 parent 52fcf7d
commit 33f05f3
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 49 deletions.
diff --git a/repository.go b/repository.go
@@ -1306,16 +1306,6 @@ func (r *Repository) Worktree() (*Worktree, error) {
 	return &Worktree{r: r, Filesystem: r.wt}, nil
 }
 
-func countTrue(vals ...bool) int {
-	sum := 0
-	for _, v := range vals {
-		if v {
-			sum++
-		}
-	}
-	return sum
-}
-
 // ResolveRevision resolves revision to corresponding hash. It will always
 // resolve to a commit hash, not a tree or annotated tag.
 //
@@ -1336,54 +1326,57 @@ func (r *Repository) ResolveRevision(rev plumbing.Revision) (*plumbing.Hash, err
 		switch item.(type) {
 		case revision.Ref:
 			revisionRef := item.(revision.Ref)
-			var ref *plumbing.Reference
-			var hashCommit, refCommit, tagCommit *object.Commit
-			var rErr, hErr, tErr error
+
+			var tryHashes []plumbing.Hash
+
+			maybeHash := plumbing.NewHash(string(revisionRef))
+
+			if !maybeHash.IsZero() {
+				tryHashes = append(tryHashes, maybeHash)
+			}
 
 			for _, rule := range append([]string{"%s"}, plumbing.RefRevParseRules...) {
-				ref, err = storer.ResolveReference(r.Storer, plumbing.ReferenceName(fmt.Sprintf(rule, revisionRef)))
+				ref, err := storer.ResolveReference(r.Storer, plumbing.ReferenceName(fmt.Sprintf(rule, revisionRef)))
 
 				if err == nil {
+					tryHashes = append(tryHashes, ref.Hash())
 					break
 				}
 			}
 
-			if ref != nil {
-				tag, tObjErr := r.TagObject(ref.Hash())
-				if tObjErr != nil {
-					tErr = tObjErr
-				} else {
-					tagCommit, tErr = tag.Commit()
+			// in ambiguous cases, `git rev-parse` will emit a warning, but
+			// will always return the oid in preference to a ref; we don't have
+			// the ability to emit a warning here, so (for speed purposes)
+			// don't bother to detect the ambiguity either, just return in the
+			// priority that git would.
+			gotOne := false
+			for _, hash := range tryHashes {
+				commitObj, err := r.CommitObject(hash)
+				if err == nil {
+					commit = commitObj
+					gotOne = true
+					break
 				}
-				refCommit, rErr = r.CommitObject(ref.Hash())
-			} else {
-				rErr = plumbing.ErrReferenceNotFound
-				tErr = plumbing.ErrReferenceNotFound
-			}
 
-			maybeHash := plumbing.NewHash(string(revisionRef)).String() == string(revisionRef)
-			if maybeHash {
-				hashCommit, hErr = r.CommitObject(plumbing.NewHash(string(revisionRef)))
-			} else {
-				hErr = plumbing.ErrReferenceNotFound
+				tagObj, err := r.TagObject(hash)
+				if err == nil {
+					// If the tag target lookup fails here, this most likely
+					// represents some sort of repo corruption, so let the
+					// error bubble up.
+					tagCommit, err := tagObj.Commit()
+					if err != nil {
+						return &plumbing.ZeroHash, err
+					}
+					commit = tagCommit
+					gotOne = true
+					break
+				}
 			}
 
-			isTag := tErr == nil
-			isCommit := rErr == nil
-			isHash := hErr == nil
-
-			switch {
-			case countTrue(isTag, isCommit, isHash) > 1:
-				return &plumbing.ZeroHash, fmt.Errorf(`refname "%s" is ambiguous`, revisionRef)
-			case isTag:
-				commit = tagCommit
-			case isCommit:
-				commit = refCommit
-			case isHash:
-				commit = hashCommit
-			default:
+			if !gotOne {
 				return &plumbing.ZeroHash, plumbing.ErrReferenceNotFound
 			}
+
 		case revision.CaretPath:
 			depth := item.(revision.CaretPath).Depth
 

diff --git a/repository_test.go b/repository_test.go
@@ -2415,7 +2415,7 @@ func (s *RepositorySuite) TestResolveRevision(c *C) {
 	for rev, hash := range datas {
 		h, err := r.ResolveRevision(plumbing.Revision(rev))
 
-		c.Assert(err, IsNil)
+		c.Assert(err, IsNil, Commentf("while checking %s", rev))
 		c.Check(h.String(), Equals, hash, Commentf("while checking %s", rev))
 	}
 }
@@ -2427,13 +2427,14 @@ func (s *RepositorySuite) TestResolveRevisionAnnotated(c *C) {
 	c.Assert(err, IsNil)
 
 	datas := map[string]string{
-		"refs/tags/annotated-tag": "f7b877701fbf855b44c0a9e86f3fdce2c298b07f",
+		"refs/tags/annotated-tag":                  "f7b877701fbf855b44c0a9e86f3fdce2c298b07f",
+		"b742a2a9fa0afcfa9a6fad080980fbc26b007c69": "f7b877701fbf855b44c0a9e86f3fdce2c298b07f",
 	}
 
 	for rev, hash := range datas {
 		h, err := r.ResolveRevision(plumbing.Revision(rev))
 
-		c.Assert(err, IsNil)
+		c.Assert(err, IsNil, Commentf("while checking %s", rev))
 		c.Check(h.String(), Equals, hash, Commentf("while checking %s", rev))
 	}
 }
@@ -2459,12 +2460,11 @@ func (s *RepositorySuite) TestResolveRevisionWithErrors(c *C) {
 		"HEAD^3":            `Revision invalid : "3" found must be 0, 1 or 2 after "^"`,
 		"HEAD^{/whatever}":  `No commit message match regexp : "whatever"`,
 		"4e1243bd22c66e76c2ba9eddc1f91394e57f9f83": "reference not found",
-		"918c48b83bd081e863dbe1b80f8998f058cd8294": `refname "918c48b83bd081e863dbe1b80f8998f058cd8294" is ambiguous`,
 	}
 
 	for rev, rerr := range datas {
 		_, err := r.ResolveRevision(plumbing.Revision(rev))
-
+		c.Assert(err, NotNil)
 		c.Assert(err.Error(), Equals, rerr)
 	}
 }