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/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)
 		}
 	}
@@ -217,19 +217,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)
+	version, err := lookupVersionForLockedProject(pi, pp.Constraint, revision, 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
+		// Only warn about the problem, it is not enough to warrant failing
+		g.logger.Println(err.Error())
 	}
 
 	lp := gps.NewLockedProject(pi, version, nil)

cmd/dep/glide_importer_test.go

@@ -68,7 +68,7 @@ func TestGlideConfig_Import(t *testing.T) {
 		t.Fatal("Expected the lock to be generated")
 	}
 
-	goldenFile := "glide/expected_import_output.txt"
+	goldenFile := "glide/golden.txt"
 	got := verboseOutput.String()
 	want := h.GetTestFileString(goldenFile)
 	if want != got {
@@ -91,9 +91,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/glide.yaml")
+	projectRoot := h.Path(testGlideProjectRoot)
 
 	g := newGlideImporter(ctx.Err, true, sm)
 	if !g.HasDepMetadata(projectRoot) {

cmd/dep/godep_importer.go

@@ -125,16 +125,16 @@ func (g *godepImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e
 				ProjectRoot: gps.ProjectRoot(pkg.ImportPath),
 			}
 			revision := gps.Revision(pkg.Rev)
-			version, err := lookupVersionForRevision(revision, pi, g.sm)
-			if err != nil {
-				warn := errors.Wrapf(err, "Unable to lookup the version represented by %s in %s. Falling back to locking the revision only.", pkg.Rev, pi.ProjectRoot)
-				g.logger.Printf(warn.Error())
-				version = revision
-			}
 
-			pp := getProjectPropertiesFromVersion(version)
-			if pp.Constraint != nil {
-				pkg.Comment = pp.Constraint.String()
+			version, err := lookupVersionForLockedProject(pi, nil, revision, g.sm)
+			if err != nil {
+				// Only warn about the problem, it is not enough to warrant failing
+				g.logger.Println(err.Error())
+			} else {
+				pp := getProjectPropertiesFromVersion(version)
+				if pp.Constraint != nil {
+					pkg.Comment = pp.Constraint.String()
+				}
 			}
 		}
 
@@ -147,7 +147,7 @@ func (g *godepImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e
 			manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{Constraint: pc.Constraint}
 		}
 
-		lp := g.buildLockedProject(pkg)
+		lp := g.buildLockedProject(pkg, manifest)
 		lock.P = append(lock.P, lp)
 	}
 
@@ -170,16 +170,15 @@ func (g *godepImporter) buildProjectConstraint(pkg godepPackage) (pc gps.Project
 }
 
 // buildLockedProject uses the package Rev and Comment to create lock project
-func (g *godepImporter) buildLockedProject(pkg godepPackage) gps.LockedProject {
+func (g *godepImporter) buildLockedProject(pkg godepPackage, manifest *dep.Manifest) gps.LockedProject {
 	pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.ImportPath)}
+	revision := gps.Revision(pkg.Rev)
+	pp := manifest.Constraints[pi.ProjectRoot]
 
-	var version gps.Version
-
-	if pkg.Comment != "" {
-		ver := gps.NewVersion(pkg.Comment)
-		version = ver.Pair(gps.Revision(pkg.Rev))
-	} else {
-		version = gps.Revision(pkg.Rev)
+	version, err := lookupVersionForLockedProject(pi, pp.Constraint, revision, g.sm)
+	if err != nil {
+		// Only warn about the problem, it is not enough to warrant failing
+		g.logger.Println(err.Error())
 	}
 
 	lp := gps.NewLockedProject(pi, version, nil)

cmd/dep/godep_importer_test.go

@@ -214,8 +214,8 @@ func TestGodepConfig_ConvertProject_EmptyComment(t *testing.T) {
 	}
 
 	ver := lpv.String()
-	if ver != "^1.0.0" {
-		t.Fatalf("Expected locked version to be '^1.0.0', got %s", ver)
+	if ver != "v1.0.0" {
+		t.Fatalf("Expected locked version to be 'v1.0.0', got %s", ver)
 	}
 }
 

cmd/dep/root_analyzer.go

@@ -173,11 +173,15 @@ func (a *rootAnalyzer) Info() gps.ProjectAnalyzerInfo {
 	}
 }
 
-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) (gps.Version, 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)
+		return rev, 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)
 	}
 
 	gps.SortPairedForUpgrade(versions) // Sort versions in asc order
@@ -187,5 +191,14 @@ func lookupVersionForRevision(rev gps.Revision, pi gps.ProjectIdentifier, sm gps
 		}
 	}
 
+	// Use the version from the manifest as long as it wasn't a range
+	switch tv := c.(type) {
+	case gps.PairedVersion:
+		return tv.Unpair().Pair(rev), nil
+	case gps.UnpairedVersion:
+		return tv.Pair(rev), nil
+	}
+
+	// Give up and lock only to a revision
 	return rev, nil
 }

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, err := lookupVersionForLockedProject(pi, c, rev, sm)
+	h.Must(err)
+
+	wantV := "v1.0.0"
+	gotV := v.String()
+	if gotV != wantV {
+		t.Fatalf("Expected the locked version to be the tag paired with the manifest's pinned revision: wanted '%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, err := lookupVersionForLockedProject(pi, c, rev, sm)
+	h.Must(err)
+
+	wantV := c.String()
+	gotV := v.String()
+	if gotV != wantV {
+		t.Fatalf("Expected the locked version to be defaulted from the manifest's branch constraint: wanted '%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, err := lookupVersionForLockedProject(pi, nil, rev, sm)
+	h.Must(err)
+
+	wantV := rev.String()
+	gotV := v.String()
+	if gotV != wantV {
+		t.Fatalf("Expected the locked version to be the manifest's pinned revision: wanted '%s', got '%s'", wantV, gotV)
+	}
+}

cmd/dep/testdata/glide/expected_import_output.txt renamed to cmd/dep/testdata/glide/golden.txt

@@ -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/godep/expected_import_output.txt

@@ -1,6 +1,6 @@
 Detected godep configuration files...
 Converting from Godeps.json ...
   Using ^0.8.1 as initial constraint for imported dep github.com/sdboyer/deptest
-  Trying ^0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest
+  Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest
   Using ^2.0.0 as initial constraint for imported dep github.com/sdboyer/deptestdos
   Trying v2.0.0 (5c60720) as initial lock for imported dep github.com/sdboyer/deptestdos

cmd/dep/testdata/harness_tests/init/godep/case1/final/Gopkg.lock

@@ -462,39 +462,20 @@ func (sm *SourceMgr) DeduceProjectRoot(ip string) (ProjectRoot, error) {
 	return ProjectRoot(pd.root), err
 }
 
-// InferConstraint tries to puzzle out what kind of version is given in a string -
-// semver, a revision, or as a fallback, a plain tag
+// 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.
 func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (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
-		}
-
-		SortPairedForUpgrade(versions)
-		for _, v := range versions {
-			if v.Type() == IsBranch {
-				return v.Unpair(), nil
-			}
-		}
-	}
-
-	// always semver if we can
-	c, err := 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 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
@@ -507,24 +488,44 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint
 		}
 
 		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 Revision(s), nil
 		}
 	}
 
-	// call out to network and get the package's versions
+	// Lookup the string in the repository
+	var version 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
+	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() == IsBranch {
+		return version.Unpair(), nil
+	}
+
+	// Semver Constraint
+	c, err := 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)
 }