diff --git a/internal/gps/source.go b/internal/gps/source.go index c8d64f9cd7..1ef3e42a6c 100644 --- a/internal/gps/source.go +++ b/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 } diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index d9283162ce..eb619dc158 100644 --- a/internal/gps/source_manager.go +++ b/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 diff --git a/internal/gps/source_manager_test.go b/internal/gps/source_manager_test.go index 631c5f4450..83df379446 100644 --- a/internal/gps/source_manager_test.go +++ b/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 - "jess@linux.com-20161116211307-wiuilyamo9ian0m7": Revision("jess@linux.com-20161116211307-wiuilyamo9ian0m7"), - // invalid bzr rev - "go4@golang.org-sadfasdf-": NewVersion("go4@golang.org-sadfasdf-"), + // 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", "matt@mattfarina.com-20150731135137-pbphasfppmygpl68", + Revision("matt@mattfarina.com-20150731135137-pbphasfppmygpl68")}, + + {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 - "go4@golang.org-lskjdfnkjsdnf-ksjdfnskjdfn", - "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) + } + }) } } diff --git a/internal/gps/vcs_source.go b/internal/gps/vcs_source.go index c4458c3697..bff5e6b1d0 100644 --- a/internal/gps/vcs_source.go +++ b/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 {