internal/gps: Parse abbreviated git revisions

internal/gps/source.go

@@ -450,6 +450,18 @@ func (sg *sourceGateway) revisionPresentIn(ctx context.Context, r Revision) (boo
 	return present, err
 }
 
+func (sg *sourceGateway) disambiguateRevision(ctx context.Context, r Revision) (Revision, error) {
+	sg.mu.Lock()
+	defer sg.mu.Unlock()
+
+	_, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally)
+	if err != nil {
+		return "", err
+	}
+
+	return sg.src.disambiguateRevision(ctx, r)
+}
+
 func (sg *sourceGateway) sourceURL(ctx context.Context) (string, error) {
 	sg.mu.Lock()
 	defer sg.mu.Unlock()
@@ -550,6 +562,7 @@ type source interface {
 	getManifestAndLock(context.Context, ProjectRoot, Revision, ProjectAnalyzer) (Manifest, Lock, error)
 	listPackages(context.Context, ProjectRoot, Revision) (pkgtree.PackageTree, error)
 	revisionPresentIn(Revision) (bool, error)
+	disambiguateRevision(context.Context, Revision) (Revision, error)
 	exportRevisionTo(context.Context, Revision, string) error
 	sourceType() string
 }

internal/gps/source_manager.go

@@ -6,13 +6,11 @@ package gps
 
 import (
 	"context"
-	"encoding/hex"
 	"fmt"
 	"os"
 	"os/signal"
 	"path/filepath"
 	"runtime"
-	"strconv"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -512,42 +510,13 @@ func (sm *SourceMgr) DeduceProjectRoot(ip string) (ProjectRoot, error) {
 }
 
 // InferConstraint tries to puzzle out what kind of version is given in a
-// string. Preference is given first for revisions, then branches, then semver
-// constraints, and then plain tags.
+// string. Preference is given first for branches, then semver constraints, then
+// plain tags, and then revisions.
 func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint, error) {
 	if s == "" {
 		return Any(), nil
 	}
 
-	slen := len(s)
-	if slen == 40 {
-		if _, err := hex.DecodeString(s); err == nil {
-			// Whether or not it's intended to be a SHA1 digest, this is a
-			// valid byte sequence for that, so go with Revision. This
-			// covers git and hg
-			return Revision(s), nil
-		}
-	}
-
-	// Next, try for bzr, which has a three-component GUID separated by
-	// dashes. There should be two, but the email part could contain
-	// internal dashes
-	if strings.Contains(s, "@") && strings.Count(s, "-") >= 2 {
-		// Work from the back to avoid potential confusion from the email
-		i3 := strings.LastIndex(s, "-")
-		// Skip if - is last char, otherwise this would panic on bounds err
-		if slen == i3+1 {
-			return NewVersion(s), nil
-		}
-
-		i2 := strings.LastIndex(s[:i3], "-")
-		if _, err := strconv.ParseUint(s[i2+1:i3], 10, 64); err == nil {
-			// Getting this far means it'd pretty much be nuts if it's not a
-			// bzr rev, so don't bother parsing the email.
-			return Revision(s), nil
-		}
-	}
-
 	// Lookup the string in the repository
 	var version PairedVersion
 	versions, err := sm.ListVersions(pi)
@@ -578,9 +547,29 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint
 		return version.Unpair(), nil
 	}
 
+	// Revision, possibly abbreviated
+	r, err := sm.disambiguateRevision(context.TODO(), pi, Revision(s))
+	if err == nil {
+		return r, nil
+	}
+
 	return nil, errors.Errorf("%s is not a valid version for the package %s(%s)", s, pi.ProjectRoot, pi.Source)
 }
 
+// disambiguateRevision looks up a revision in the underlying source, spitting
+// it back out in an unabbreviated, disambiguated form.
+//
+// For example, if pi refers to a git-based project, then rev could be an
+// abbreviated git commit hash. disambiguateRevision would return the complete
+// hash.
+func (sm *SourceMgr) disambiguateRevision(ctx context.Context, pi ProjectIdentifier, rev Revision) (Revision, error) {
+	srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), pi)
+	if err != nil {
+		return "", err
+	}
+	return srcg.disambiguateRevision(ctx, rev)
+}
+
 type timeCount struct {
 	count int
 	start time.Time

internal/gps/source_manager_test.go

@@ -13,71 +13,82 @@ import (
 
 func TestSourceManager_InferConstraint(t *testing.T) {
 	t.Parallel()
-	h := test.NewHelper(t)
-	cacheDir := "gps-repocache"
-	h.TempDir(cacheDir)
-	sm, err := NewSourceManager(h.Path(cacheDir))
-	h.Must(err)
-
-	sv, err := NewSemverConstraintIC("v0.8.1")
-	if err != nil {
-		t.Fatal(err)
-	}
 
-	svs, err := NewSemverConstraintIC("v0.12.0-12-de4dcafe0")
-	if err != nil {
-		t.Fatal(err)
-	}
+	testcase := func(str string, pi ProjectIdentifier, want Constraint) func(*testing.T) {
+		return func(t *testing.T) {
+			t.Parallel()
+			h := test.NewHelper(t)
+			defer h.Cleanup()
 
-	constraints := map[string]Constraint{
-		"":       Any(),
-		"v0.8.1": sv,
-		"v2":     NewBranch("v2"),
-		"v0.12.0-12-de4dcafe0": svs,
-		"master":               NewBranch("master"),
-		"5b3352dc16517996fb951394bcbbe913a2a616e3": Revision("5b3352dc16517996fb951394bcbbe913a2a616e3"),
-
-		// valid bzr rev
-		"jess@linux.com-20161116211307-wiuilyamo9ian0m7": Revision("jess@linux.com-20161116211307-wiuilyamo9ian0m7"),
-		// invalid bzr rev
-		"go4@golang.org-sadfasdf-": NewVersion("go4@golang.org-sadfasdf-"),
-	}
+			cacheDir := "gps-repocache"
+			h.TempDir(cacheDir)
 
-	pi := ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"}
-	for str, want := range constraints {
-		got, err := sm.InferConstraint(str, pi)
-		h.Must(err)
+			sm, err := NewSourceManager(h.Path(cacheDir))
+			h.Must(err)
 
-		wantT := reflect.TypeOf(want)
-		gotT := reflect.TypeOf(got)
-		if wantT != gotT {
-			t.Errorf("expected type: %s, got %s, for input %s", wantT, gotT, str)
-		}
-		if got.String() != want.String() {
-			t.Errorf("expected value: %s, got %s for input %s", want, got, str)
+			got, err := sm.InferConstraint(str, pi)
+			h.Must(err)
+
+			wantT := reflect.TypeOf(want)
+			gotT := reflect.TypeOf(got)
+			if wantT != gotT {
+				t.Errorf("expected type: %s, got %s, for input %s", wantT, gotT, str)
+			}
+			if got.String() != want.String() {
+				t.Errorf("expected value: %s, got %s for input %s", want, got, str)
+			}
 		}
 	}
-}
 
-func TestSourceManager_InferConstraint_InvalidInput(t *testing.T) {
-	h := test.NewHelper(t)
+	var (
+		gitProj = ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"}
+		bzrProj = ProjectIdentifier{ProjectRoot: "launchpad.net/govcstestbzrrepo"}
+		hgProj  = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"}
+	)
 
-	cacheDir := "gps-repocache"
-	h.TempDir(cacheDir)
-	sm, err := NewSourceManager(h.Path(cacheDir))
-	h.Must(err)
+	t.Run("git", func(t *testing.T) {
+		t.Parallel()
+		t.Run("empty", testcase("", gitProj, Any()))
 
-	constraints := []string{
-		// invalid bzr revs
-		"go4@golang.org-lskjdfnkjsdnf-ksjdfnskjdfn",
-		"20120425195858-psty8c35ve2oej8t",
-	}
+		v081, err := NewSemverConstraintIC("v0.8.1")
+		if err != nil {
+			t.Fatal(err)
+		}
 
-	pi := ProjectIdentifier{ProjectRoot: "github.com/sdboyer/deptest"}
-	for _, str := range constraints {
-		_, err := sm.InferConstraint(str, pi)
-		if err == nil {
-			t.Errorf("expected %s to produce an error", str)
+		v012, err := NewSemverConstraintIC("v0.12.0-12-de4dcafe0")
+		if err != nil {
+			t.Fatal(err)
 		}
-	}
+
+		t.Run("semver constraint", testcase("v0.8.1", gitProj, v081))
+		t.Run("long semver constraint", testcase("v0.12.0-12-de4dcafe0", gitProj, v012))
+		t.Run("branch v2", testcase("v2", gitProj, NewBranch("v2")))
+		t.Run("branch master", testcase("master", gitProj, NewBranch("master")))
+		t.Run("long revision", testcase("3f4c3bea144e112a69bbe5d8d01c1b09a544253f", gitProj, Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f")))
+		t.Run("short revision", testcase("3f4c3bea", gitProj, Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f")))
+	})
+
+	t.Run("bzr", func(t *testing.T) {
+		t.Parallel()
+		v1, err := NewSemverConstraintIC("v1.0.0")
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Run("empty", testcase("", bzrProj, Any()))
+		t.Run("semver", testcase("v1.0.0", bzrProj, v1))
+		t.Run("revision", testcase("matt@mattfarina.com-20150731135137-pbphasfppmygpl68", bzrProj, Revision("matt@mattfarina.com-20150731135137-pbphasfppmygpl68")))
+	})
+
+	t.Run("hg", func(t *testing.T) {
+		t.Parallel()
+		v1, err := NewSemverConstraintIC("v1.0.0")
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Run("empty", testcase("", hgProj, Any()))
+		t.Run("semver", testcase("v1.0.0", hgProj, v1))
+		t.Run("default branch", testcase("default", hgProj, NewBranch("default")))
+		t.Run("revision", testcase("6f55e1f03d91f8a7cce35d1968eb60a2352e4d59", hgProj, Revision("6f55e1f03d91f8a7cce35d1968eb60a2352e4d59")))
+		t.Run("short revision", testcase("6f55e1f03d91", hgProj, Revision("6f55e1f03d91f8a7cce35d1968eb60a2352e4d59")))
+	})
 }

internal/gps/vcs_source.go

@@ -40,6 +40,14 @@ func (bs *baseVCSSource) upstreamURL() string {
 	return bs.repo.Remote()
 }
 
+func (bs *baseVCSSource) disambiguateRevision(ctx context.Context, r Revision) (Revision, error) {
+	ci, err := bs.repo.CommitInfo(string(r))
+	if err != nil {
+		return "", err
+	}
+	return Revision(ci.Commit), nil
+}
+
 func (bs *baseVCSSource) getManifestAndLock(ctx context.Context, pr ProjectRoot, r Revision, an ProjectAnalyzer) (Manifest, Lock, error) {
 	err := bs.repo.updateVersion(ctx, r.String())
 	if err != nil {
@@ -406,6 +414,21 @@ func (s *bzrSource) listVersions(ctx context.Context) ([]PairedVersion, error) {
 	return vlist, nil
 }
 
+func (s *bzrSource) disambiguateRevision(ctx context.Context, r Revision) (Revision, error) {
+	// If we used the default baseVCSSource behavior here, we would return the
+	// bazaar revision number, which is not a globally unique identifier - it is
+	// only unique within a branch. This is just the way that
+	// github.com/Masterminds/vcs chooses to handle bazaar. We want a
+	// disambiguated unique ID, though, so we need slightly different behavior:
+	// check whether r doesn't error when we try to look it up. If so, trust that
+	// it's a revision.
+	_, err := s.repo.CommitInfo(string(r))
+	if err != nil {
+		return "", err
+	}
+	return r, nil
+}
+
 // hgSource is a generic hg repository implementation that should work with
 // all standard mercurial servers.
 type hgSource struct {