Merge pull request #1027 from spenczar/infer_git_abbreviated_shas

internal/gps: Parse abbreviated git revisions

Author: carolynvs

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,94 @@ 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")
+	// Used in git subtests:
+	v081, err := NewSemverConstraintIC("v0.8.1")
 	if err != nil {
 		t.Fatal(err)
 	}
-
-	svs, err := NewSemverConstraintIC("v0.12.0-12-de4dcafe0")
+	v012, err := NewSemverConstraintIC("v0.12.0-12-de4dcafe0")
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	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
-		"[email protected]": Revision("[email protected]"),
-		// invalid bzr rev
-		"[email protected]": NewVersion("[email protected]"),
+	// Used in hg and bzr subtests:
+	v1, err := NewSemverConstraintIC("v1.0.0")
+	if err != nil {
+		t.Fatal(err)
 	}
 
-	pi := ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"}
-	for str, want := range constraints {
-		got, err := sm.InferConstraint(str, pi)
-		h.Must(err)
+	var (
+		gitProj = ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"}
+		bzrProj = ProjectIdentifier{ProjectRoot: "launchpad.net/govcstestbzrrepo"}
+		hgProj  = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"}
 
-		wantT := reflect.TypeOf(want)
-		gotT := reflect.TypeOf(got)
-		if wantT != gotT {
-			t.Errorf("expected type: %s, got %s, for input %s", wantT, gotT, str)
+		testcases = []struct {
+			project ProjectIdentifier
+			name    string
+			str     string
+			want    Constraint
+		}{
+			{gitProj, "empty", "", Any()},
+			{gitProj, "semver-short", "v0.8.1", v081},
+			{gitProj, "long semver constraint", "v0.12.0-12-de4dcafe0", v012},
+			{gitProj, "branch v2", "v2", NewBranch("v2")},
+			{gitProj, "branch master", "master", NewBranch("master")},
+			{gitProj, "long revision", "3f4c3bea144e112a69bbe5d8d01c1b09a544253f",
+				Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f")},
+			{gitProj, "short revision", "3f4c3bea",
+				Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f")},
+
+			{bzrProj, "empty", "", Any()},
+			{bzrProj, "semver", "v1.0.0", v1},
+			{bzrProj, "revision", "[email protected]",
+				Revision("[email protected]")},
+
+			{hgProj, "empty", "", Any()},
+			{hgProj, "semver", "v1.0.0", v1},
+			{hgProj, "default branch", "default", NewBranch("default")},
+			{hgProj, "revision", "6f55e1f03d91f8a7cce35d1968eb60a2352e4d59",
+				Revision("6f55e1f03d91f8a7cce35d1968eb60a2352e4d59")},
+			{hgProj, "short revision", "6f55e1f03d91",
+				Revision("6f55e1f03d91f8a7cce35d1968eb60a2352e4d59")},
 		}
-		if got.String() != want.String() {
-			t.Errorf("expected value: %s, got %s for input %s", want, got, str)
+	)
+
+	for _, tc := range testcases {
+		var subtestName string
+		switch tc.project {
+		case gitProj:
+			subtestName = "git-" + tc.name
+		case bzrProj:
+			subtestName = "bzr-" + tc.name
+		case hgProj:
+			subtestName = "hg-" + tc.name
+		default:
+			subtestName = tc.name
 		}
-	}
-}
 
-func TestSourceManager_InferConstraint_InvalidInput(t *testing.T) {
-	h := test.NewHelper(t)
+		t.Run(subtestName, func(t *testing.T) {
+			t.Parallel()
+			h := test.NewHelper(t)
+			defer h.Cleanup()
 
-	cacheDir := "gps-repocache"
-	h.TempDir(cacheDir)
-	sm, err := NewSourceManager(h.Path(cacheDir))
-	h.Must(err)
+			cacheDir := "gps-repocache"
+			h.TempDir(cacheDir)
 
-	constraints := []string{
-		// invalid bzr revs
-		"[email protected]",
-		"20120425195858-psty8c35ve2oej8t",
-	}
+			sm, err := NewSourceManager(h.Path(cacheDir))
+			h.Must(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)
-		}
+			got, err := sm.InferConstraint(tc.str, tc.project)
+			h.Must(err)
+
+			wantT := reflect.TypeOf(tc.want)
+			gotT := reflect.TypeOf(got)
+			if wantT != gotT {
+				t.Errorf("expected type: %s, got %s, for input %s", wantT, gotT, tc.str)
+			}
+			if got.String() != tc.want.String() {
+				t.Errorf("expected value: %s, got %s for input %s", tc.want, got, tc.str)
+			}
+		})
 	}
 }

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 {