Interpret strings as branches > semver constraints

When a user supplied string in an imported config file, or specified to
dep ensure, can be interpreted multiple ways, prefer the branch over a
semver constraint.

In #710, glide.yaml specified v2 for https://github.com/go-mgo/mgo.
When we assume that is a semver constraint, solve fails because the
hinted revision in the lock (a commit on the v2 branch) doesn't satisfy
the assumed constraint of ^2.0.0.

The new preferred match order for the user string is:

* revision
* branch
* semver constraint
* tag

I am giving preference of a semver constraint over a tag so that a bare
version, 1.0.0, is interpreted more loosely with an implied caret,
^1.0.0, instead of the stricter exact match.

Author: carolynvs

cmd/dep/ensure.go

@@ -304,39 +304,19 @@ func getProjectConstraint(arg string, sm gps.SourceManager) (gps.ProjectConstrai
 	return gps.ProjectConstraint{Ident: pi, Constraint: c}, nil
 }
 
-// deduceConstraint tries to puzzle out what kind of version is given in a string -
-// semver, a revision, or as a fallback, a plain tag
+// deduceConstraint tries to puzzle out what kind of version is given in a string.
+// Preference is given first for revisions, then branches, then semver constraints, then plain tags.
 func deduceConstraint(s string, pi gps.ProjectIdentifier, sm gps.SourceManager) (gps.Constraint, error) {
-	if s == "" {
-		// Find the default branch
-		versions, err := sm.ListVersions(pi)
-		if err != nil {
-			return nil, errors.Wrapf(err, "list versions for %s(%s)", pi.ProjectRoot, pi.Source) // means repo does not exist
-		}
-
-		gps.SortPairedForUpgrade(versions)
-		for _, v := range versions {
-			if v.Type() == gps.IsBranch {
-				return v.Unpair(), nil
-			}
-		}
-	}
-
-	// always semver if we can
-	c, err := gps.NewSemverConstraintIC(s)
-	if err == nil {
-		return c, nil
-	}
-
 	slen := len(s)
 	if slen == 40 {
-		if _, err = hex.DecodeString(s); err == nil {
+		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 gps.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
@@ -349,24 +329,44 @@ func deduceConstraint(s string, pi gps.ProjectIdentifier, sm gps.SourceManager)
 		}
 
 		i2 := strings.LastIndex(s[:i3], "-")
-		if _, err = strconv.ParseUint(s[i2+1:i3], 10, 64); err == nil {
+		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 gps.Revision(s), nil
 		}
 	}
 
-	// call out to network and get the package's versions
+	// Lookup the string in the repository
+	var version gps.PairedVersion
 	versions, err := sm.ListVersions(pi)
 	if err != nil {
 		return nil, errors.Wrapf(err, "list versions for %s(%s)", pi.ProjectRoot, pi.Source) // means repo does not exist
 	}
-
-	for _, version := range versions {
-		if s == version.String() {
-			return version.Unpair(), nil
+	gps.SortPairedForUpgrade(versions)
+	for _, v := range versions {
+		// Pick the default branch if no constraint is given
+		if s == "" || s == v.String() {
+			version = v
+			break
 		}
 	}
+
+	// Branch
+	if version != nil && version.Type() == gps.IsBranch {
+		return version.Unpair(), nil
+	}
+
+	// Semver Constraint
+	c, err := gps.NewSemverConstraintIC(s)
+	if c != nil && err == nil {
+		return c, nil
+	}
+
+	// Tag
+	if version != nil {
+		return version.Unpair(), nil
+	}
+
 	return nil, errors.Errorf("%s is not a valid version for the package %s(%s)", s, pi.ProjectRoot, pi.Source)
 }
 

cmd/dep/glide_importer.go

@@ -178,11 +178,11 @@ func (g *glideImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e
 		lock = &dep.Lock{}
 
 		for _, pkg := range g.lock.Imports {
-			lp := g.buildLockedProject(pkg)
+			lp := g.buildLockedProject(pkg, manifest)
 			lock.P = append(lock.P, lp)
 		}
 		for _, pkg := range g.lock.TestImports {
-			lp := g.buildLockedProject(pkg)
+			lp := g.buildLockedProject(pkg, manifest)
 			lock.P = append(lock.P, lp)
 		}
 	}
@@ -213,19 +213,18 @@ func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.Project
 	return
 }
 
-func (g *glideImporter) buildLockedProject(pkg glideLockedPackage) gps.LockedProject {
+func (g *glideImporter) buildLockedProject(pkg glideLockedPackage, manifest *dep.Manifest) gps.LockedProject {
 	pi := gps.ProjectIdentifier{
 		ProjectRoot: gps.ProjectRoot(pkg.Name),
 		Source:      pkg.Repository,
 	}
 	revision := gps.Revision(pkg.Reference)
+	pp := manifest.Constraints[pi.ProjectRoot]
 
-	version, err := lookupVersionForRevision(revision, pi, g.sm)
-	if err != nil {
-		// Warn about the problem, it is not enough to warrant failing
-		warn := errors.Wrapf(err, "Unable to lookup the version represented by %s in %s(%s). Falling back to locking the revision only.", revision, pi.ProjectRoot, pi.Source)
-		g.logger.Printf(warn.Error())
-		version = revision
+	version, warning := lookupVersionForLockedProject(pi, pp.Constraint, revision, g.sm)
+	if warning != nil {
+		// Only warn about the problem, it is not enough to warrant failing
+		g.logger.Println(warning.Error())
 	}
 
 	f := fb.NewLockedProjectFeedback(version, pi.ProjectRoot, fb.DepTypeImported)

cmd/dep/glide_importer_test.go

@@ -44,8 +44,8 @@ func TestGlideConfig_Import(t *testing.T) {
 	defer sm.Release()
 
 	h.TempDir(filepath.Join("src", testGlideProjectRoot))
-	h.TempCopy(filepath.Join(testGlideProjectRoot, glideYamlName), "glide/glide.yaml")
-	h.TempCopy(filepath.Join(testGlideProjectRoot, glideLockName), "glide/glide.lock")
+	h.TempCopy(filepath.Join(testGlideProjectRoot, glideYamlName), "glide/glide0.yaml")
+	h.TempCopy(filepath.Join(testGlideProjectRoot, glideLockName), "glide/glide0.lock")
 	projectRoot := h.Path(testGlideProjectRoot)
 
 	// Capture stderr so we can verify output
@@ -68,7 +68,55 @@ func TestGlideConfig_Import(t *testing.T) {
 		t.Fatal("Expected the lock to be generated")
 	}
 
-	goldenFile := "glide/expected_import_output.txt"
+	goldenFile := "glide/golden0.txt"
+	got := verboseOutput.String()
+	want := h.GetTestFileString(goldenFile)
+	if want != got {
+		if *test.UpdateGolden {
+			if err := h.WriteTestFile(goldenFile, got); err != nil {
+				t.Fatalf("%+v", errors.Wrapf(err, "Unable to write updated golden file %s", goldenFile))
+			}
+		} else {
+			t.Fatalf("expected %s, got %s", want, got)
+		}
+	}
+}
+
+func TestGlideConfig_Import_BranchWithSemverName(t *testing.T) {
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+
+	ctx := newTestContext(h)
+	sm, err := ctx.SourceManager()
+	h.Must(err)
+	defer sm.Release()
+
+	h.TempDir(filepath.Join("src", testGlideProjectRoot))
+	h.TempCopy(filepath.Join(testGlideProjectRoot, glideYamlName), "glide/glide1.yaml")
+	h.TempCopy(filepath.Join(testGlideProjectRoot, glideLockName), "glide/glide1.lock")
+	projectRoot := h.Path(testGlideProjectRoot)
+
+	// Capture stderr so we can verify output
+	verboseOutput := &bytes.Buffer{}
+	ctx.Err = log.New(verboseOutput, "", 0)
+
+	g := newGlideImporter(ctx.Err, false, sm) // Disable verbose so that we don't print values that change each test run
+	if !g.HasDepMetadata(projectRoot) {
+		t.Fatal("Expected the importer to detect the glide configuration files")
+	}
+
+	m, l, err := g.Import(projectRoot, testGlideProjectRoot)
+	h.Must(err)
+
+	if m == nil {
+		t.Fatal("Expected the manifest to be generated")
+	}
+
+	if l == nil {
+		t.Fatal("Expected the lock to be generated")
+	}
+
+	goldenFile := "glide/golden1.txt"
 	got := verboseOutput.String()
 	want := h.GetTestFileString(goldenFile)
 	if want != got {
@@ -91,9 +139,9 @@ func TestGlideConfig_Import_MissingLockFile(t *testing.T) {
 	h.Must(err)
 	defer sm.Release()
 
-	h.TempDir(filepath.Join("src", "glidetest"))
-	h.TempCopy(filepath.Join("glidetest", glideYamlName), "glide/glide.yaml")
-	projectRoot := h.Path("glidetest")
+	h.TempDir(filepath.Join("src", testGlideProjectRoot))
+	h.TempCopy(filepath.Join(testGlideProjectRoot, glideYamlName), "glide/glide0.yaml")
+	projectRoot := h.Path(testGlideProjectRoot)
 
 	g := newGlideImporter(ctx.Err, true, sm)
 	if !g.HasDepMetadata(projectRoot) {

cmd/dep/root_analyzer.go

@@ -146,19 +146,37 @@ func (a *rootAnalyzer) Info() (string, int) {
 	return name, version
 }
 
-func lookupVersionForRevision(rev gps.Revision, pi gps.ProjectIdentifier, sm gps.SourceManager) (gps.Version, error) {
+// lookupVersionForLockedProject figures out the appropriate version for a locked
+// project based on the locked revision and the constraint from the manifest.
+// First try matching the revision to a version, then try the constraint from the
+// manifest, then finally the revision.
+func lookupVersionForLockedProject(pi gps.ProjectIdentifier, c gps.Constraint, rev gps.Revision, sm gps.SourceManager) (version gps.Version, warning error) {
 	// Find the version that goes with this revision, if any
 	versions, err := sm.ListVersions(pi)
 	if err != nil {
-		return nil, errors.Wrapf(err, "Unable to list versions for %s(%s)", pi.ProjectRoot, pi.Source)
+		warning = errors.Wrapf(err, "Unable to lookup the version represented by %s in %s(%s). Falling back to locking the revision only.", rev, pi.ProjectRoot, pi.Source)
+		return
 	}
 
 	gps.SortPairedForUpgrade(versions) // Sort versions in asc order
 	for _, v := range versions {
 		if v.Underlying() == rev {
-			return v, nil
+			version = v
+			return
 		}
 	}
 
-	return rev, nil
+	// Use the version from the manifest as long as it wasn't a range
+	switch tv := c.(type) {
+	case gps.PairedVersion:
+		version = tv.Unpair().Is(rev)
+		return
+	case gps.UnpairedVersion:
+		version = tv.Is(rev)
+		return
+	}
+
+	// Give up and lock only to a revision
+	version = rev
+	return
 }

cmd/dep/root_analyzer_test.go

@@ -4,7 +4,12 @@
 
 package main
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/golang/dep/internal/gps"
+	"github.com/golang/dep/internal/test"
+)
 
 func TestRootAnalyzer_Info(t *testing.T) {
 	testCases := map[bool]string{
@@ -19,3 +24,68 @@ func TestRootAnalyzer_Info(t *testing.T) {
 		}
 	}
 }
+
+func TestLookupVersionForLockedProject_MatchRevisionToTag(t *testing.T) {
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+
+	ctx := newTestContext(h)
+	sm, err := ctx.SourceManager()
+	h.Must(err)
+	defer sm.Release()
+
+	pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/sdboyer/deptest")}
+	c, _ := gps.NewSemverConstraint("^0.8.1")
+	rev := gps.Revision("ff2948a2ac8f538c4ecd55962e919d1e13e74baf")
+	v, warning := lookupVersionForLockedProject(pi, c, rev, sm)
+	h.Must(warning)
+
+	wantV := "v1.0.0"
+	gotV := v.String()
+	if gotV != wantV {
+		t.Fatalf("Expected '%s', got '%s'", wantV, gotV)
+	}
+}
+
+func TestLookupVersionForLockedProject_FallbackToConstraint(t *testing.T) {
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+
+	ctx := newTestContext(h)
+	sm, err := ctx.SourceManager()
+	h.Must(err)
+	defer sm.Release()
+
+	pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/sdboyer/deptest")}
+	c := gps.NewBranch("master")
+	rev := gps.Revision("c575196502940c07bf89fd6d95e83b999162e051")
+	v, warning := lookupVersionForLockedProject(pi, c, rev, sm)
+	h.Must(warning)
+
+	wantV := c.String()
+	gotV := v.String()
+	if gotV != wantV {
+		t.Fatalf("Expected '%s', got '%s'", wantV, gotV)
+	}
+}
+
+func TestLookupVersionForLockedProject_FallbackToRevision(t *testing.T) {
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+
+	ctx := newTestContext(h)
+	sm, err := ctx.SourceManager()
+	h.Must(err)
+	defer sm.Release()
+
+	pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/sdboyer/deptest")}
+	rev := gps.Revision("c575196502940c07bf89fd6d95e83b999162e051")
+	v, warning := lookupVersionForLockedProject(pi, nil, rev, sm)
+	h.Must(warning)
+
+	wantV := rev.String()
+	gotV := v.String()
+	if gotV != wantV {
+		t.Fatalf("Expected '%s', got '%s'", wantV, gotV)
+	}
+}

cmd/dep/testdata/glide/glide.lock cmd/dep/testdata/glide/glide0.lock

@@ -0,0 +1,10 @@
+hash: 3b7d5be7d68cd94765ac91ea2f58b554151f67f5d0bf499ea8cadb600538a0e5
+updated: 2017-05-03T15:13:27.629768623+02:00
+imports:
+- name: gopkg.in/mgo.v2
+  version: 29cc868a5ca65f401ff318143f9408d02f4799cc
+  subpackages:
+  - bson
+  - internal/sasl
+  - internal/scram
+devImports: []

cmd/dep/testdata/glide/glide.yaml cmd/dep/testdata/glide/glide0.yaml

@@ -0,0 +1,4 @@
+package: github.com/golang/notexist
+import:
+- package: gopkg.in/mgo.v2
+  version: v2

cmd/dep/testdata/glide/glide1.lock

@@ -5,3 +5,4 @@ Converting from glide.yaml and glide.lock...
   Using master as initial constraint for imported dep github.com/golang/lint
   Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest
   Trying v2.0.0 (5c60720) as initial lock for imported dep github.com/sdboyer/deptestdos
+  Trying master (cb00e56) as initial lock for imported dep github.com/golang/lint

cmd/dep/testdata/glide/glide1.yaml

@@ -0,0 +1,4 @@
+Detected glide configuration files...
+Converting from glide.yaml and glide.lock...
+  Using v2 as initial constraint for imported dep gopkg.in/mgo.v2
+  Trying v2 (29cc868) as initial lock for imported dep gopkg.in/mgo.v2

cmd/dep/testdata/glide/expected_import_output.txt cmd/dep/testdata/glide/golden0.txt

@@ -41,6 +41,10 @@ func TestGetConstraintString(t *testing.T) {
 			feedback: GetLockingFeedback("master", "436f39d", DepTypeTransitive, "github.com/baz/qux"),
 			want:     "Locking in master (436f39d) for transitive dep github.com/baz/qux",
 		},
+		{
+			feedback: GetLockingFeedback("master", "436f39d", DepTypeImported, "github.com/baz/qux"),
+			want:     "Trying master (436f39d) as initial lock for imported dep github.com/baz/qux",
+		},
 	}
 
 	for _, c := range cases {