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
   #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 <mike@fluffypenguin.org>

1 files changed, 37 insertions, 44 deletions

diff --git a/repository.go b/repository.go
index e5b12b0..a94dc2f 100644
--- 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