From 531fb9ce9bf2a3091cd3f7ffd7fe3b69d094e2ab Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Thu, 17 Aug 2017 11:16:07 -0400 Subject: [PATCH 1/8] Parse abbreviated git revisions While dep doesn't want to encourage the use of abbreviated git commit identifiers, they come up frequently when we parse vendoring specifiers (like glide.yaml) of existing projects. We should give a shot at parsing these and then expanding them to their unabbreviated form. --- internal/gps/source.go | 14 ++++++++++++++ internal/gps/source_manager.go | 12 ++++++++++++ internal/gps/source_manager_test.go | 2 ++ internal/gps/vcs_source.go | 5 +++++ 4 files changed, 33 insertions(+) diff --git a/internal/gps/source.go b/internal/gps/source.go index d687a1b687..75f8180e88 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -10,6 +10,7 @@ import ( "fmt" "sync" + "github.com/Masterminds/vcs" "github.com/golang/dep/internal/gps/pkgtree" ) @@ -450,6 +451,18 @@ func (sg *sourceGateway) revisionPresentIn(ctx context.Context, r Revision) (boo return present, err } +func (sg *sourceGateway) commitInfo(ctx context.Context, r Revision) (*vcs.CommitInfo, error) { + sg.mu.Lock() + defer sg.mu.Unlock() + + _, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally) + if err != nil { + return nil, err + } + + return sg.src.commitInfo(ctx, r) +} + func (sg *sourceGateway) sourceURL(ctx context.Context) (string, error) { sg.mu.Lock() defer sg.mu.Unlock() @@ -550,6 +563,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) + commitInfo(context.Context, Revision) (*vcs.CommitInfo, 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..8770d94995 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -578,6 +578,18 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint return version.Unpair(), nil } + // Abbreviated git revision? If the string is present, there's a good shot of + // this. + if present, _ := sm.RevisionPresentIn(pi, Revision(s)); present { + srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), pi) + if err == nil { + ci, err := srcg.commitInfo(context.TODO(), Revision(s)) + if err == nil { + return Revision(ci.Commit), nil + } + } + } + return nil, errors.Errorf("%s is not a valid version for the package %s(%s)", s, pi.ProjectRoot, pi.Source) } diff --git a/internal/gps/source_manager_test.go b/internal/gps/source_manager_test.go index 631c5f4450..afa042802a 100644 --- a/internal/gps/source_manager_test.go +++ b/internal/gps/source_manager_test.go @@ -36,6 +36,8 @@ func TestSourceManager_InferConstraint(t *testing.T) { "v0.12.0-12-de4dcafe0": svs, "master": NewBranch("master"), "5b3352dc16517996fb951394bcbbe913a2a616e3": Revision("5b3352dc16517996fb951394bcbbe913a2a616e3"), + // abbreviated git ref + "3f4c3bea": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"), // valid bzr rev "jess@linux.com-20161116211307-wiuilyamo9ian0m7": Revision("jess@linux.com-20161116211307-wiuilyamo9ian0m7"), diff --git a/internal/gps/vcs_source.go b/internal/gps/vcs_source.go index c4458c3697..855bfc9c48 100644 --- a/internal/gps/vcs_source.go +++ b/internal/gps/vcs_source.go @@ -15,6 +15,7 @@ import ( "time" "github.com/Masterminds/semver" + "github.com/Masterminds/vcs" "github.com/golang/dep/internal/fs" "github.com/golang/dep/internal/gps/pkgtree" ) @@ -40,6 +41,10 @@ func (bs *baseVCSSource) upstreamURL() string { return bs.repo.Remote() } +func (bs *baseVCSSource) commitInfo(ctx context.Context, r Revision) (*vcs.CommitInfo, error) { + return bs.repo.CommitInfo(string(r)) +} + 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 { From dfb871980f80dca4e8fa66269e739f2805715d4a Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Thu, 17 Aug 2017 14:32:55 -0400 Subject: [PATCH 2/8] Change commitInfo to disambiguateRevision commitInfo returned a struct from an external package. This coupling isn't ideal, and we wouldn't use the extra info for anything. It's better to just have a method for exactly what we want, which is disambiguation of short revision specifiers. --- internal/gps/source.go | 9 ++++----- internal/gps/source_manager.go | 4 ++-- internal/gps/vcs_source.go | 9 ++++++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/internal/gps/source.go b/internal/gps/source.go index 75f8180e88..b577608e39 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -10,7 +10,6 @@ import ( "fmt" "sync" - "github.com/Masterminds/vcs" "github.com/golang/dep/internal/gps/pkgtree" ) @@ -451,16 +450,16 @@ func (sg *sourceGateway) revisionPresentIn(ctx context.Context, r Revision) (boo return present, err } -func (sg *sourceGateway) commitInfo(ctx context.Context, r Revision) (*vcs.CommitInfo, error) { +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 nil, err + return "", err } - return sg.src.commitInfo(ctx, r) + return sg.src.disambiguateRevision(ctx, r) } func (sg *sourceGateway) sourceURL(ctx context.Context) (string, error) { @@ -563,7 +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) - commitInfo(context.Context, Revision) (*vcs.CommitInfo, 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 8770d94995..5ac7f4971b 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -583,9 +583,9 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint if present, _ := sm.RevisionPresentIn(pi, Revision(s)); present { srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), pi) if err == nil { - ci, err := srcg.commitInfo(context.TODO(), Revision(s)) + r, err := srcg.disambiguateRevision(context.TODO(), Revision(s)) if err == nil { - return Revision(ci.Commit), nil + return r, nil } } } diff --git a/internal/gps/vcs_source.go b/internal/gps/vcs_source.go index 855bfc9c48..784eb6b8c4 100644 --- a/internal/gps/vcs_source.go +++ b/internal/gps/vcs_source.go @@ -15,7 +15,6 @@ import ( "time" "github.com/Masterminds/semver" - "github.com/Masterminds/vcs" "github.com/golang/dep/internal/fs" "github.com/golang/dep/internal/gps/pkgtree" ) @@ -41,8 +40,12 @@ func (bs *baseVCSSource) upstreamURL() string { return bs.repo.Remote() } -func (bs *baseVCSSource) commitInfo(ctx context.Context, r Revision) (*vcs.CommitInfo, error) { - return bs.repo.CommitInfo(string(r)) +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) { From e0f7e1f53439e8a47debc00f2dda774ba8dd1680 Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Thu, 17 Aug 2017 16:28:48 -0400 Subject: [PATCH 3/8] Remove heuristic git hash detection Now that we are disambiguating revisions by going to the source, we don't need the heuristic check of string length and hex parseability anymore. --- internal/gps/source_manager.go | 18 +++--------------- internal/gps/source_manager_test.go | 3 +-- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index 5ac7f4971b..848b7e9d38 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -6,7 +6,6 @@ package gps import ( "context" - "encoding/hex" "fmt" "os" "os/signal" @@ -519,24 +518,13 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint 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 + // Bazaar 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 { + if len(s) == i3+1 { return NewVersion(s), nil } diff --git a/internal/gps/source_manager_test.go b/internal/gps/source_manager_test.go index afa042802a..81d3df2d76 100644 --- a/internal/gps/source_manager_test.go +++ b/internal/gps/source_manager_test.go @@ -35,8 +35,7 @@ func TestSourceManager_InferConstraint(t *testing.T) { "v2": NewBranch("v2"), "v0.12.0-12-de4dcafe0": svs, "master": NewBranch("master"), - "5b3352dc16517996fb951394bcbbe913a2a616e3": Revision("5b3352dc16517996fb951394bcbbe913a2a616e3"), - // abbreviated git ref + "3f4c3bea144e112a69bbe5d8d01c1b09a544253f": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"), "3f4c3bea": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"), // valid bzr rev From 6977b26cecb2415926050660c2521403c93cf5bb Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Thu, 17 Aug 2017 16:31:17 -0400 Subject: [PATCH 4/8] Move revision disambiguation into a private method This just cleans up the SourceMgr implementation a bit. --- internal/gps/source_manager.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index 848b7e9d38..72aebde51f 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -566,21 +566,29 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint return version.Unpair(), nil } - // Abbreviated git revision? If the string is present, there's a good shot of - // this. - if present, _ := sm.RevisionPresentIn(pi, Revision(s)); present { - srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), pi) - if err == nil { - r, err := srcg.disambiguateRevision(context.TODO(), Revision(s)) - if err == nil { - return r, 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 From 2d4b3c25d445575eb1695de0d4c6dc9be4f42659 Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Mon, 28 Aug 2017 09:48:59 -0400 Subject: [PATCH 5/8] Correctly document (*SourceMgr).InferConstraint The previous commit changed the order of prefernce when inferring constraints. Also, moved the bazaar GUID parsing stuff to make sure that it receives the same preference level as revisions for other repository types. --- internal/gps/source_manager.go | 42 ++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index 72aebde51f..6382736ad3 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -511,31 +511,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 } - // Bazaar 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 len(s) == 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) @@ -572,6 +554,26 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint return r, nil } + // Bazaar revision + // + // Bazaar 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 len(s) == 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 + } + } + return nil, errors.Errorf("%s is not a valid version for the package %s(%s)", s, pi.ProjectRoot, pi.Source) } From 960d21a5fa8bfdf6068afd563bdfb45e4196cc68 Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Mon, 28 Aug 2017 11:03:51 -0400 Subject: [PATCH 6/8] Handle bazaar revisions by talking to the actual repo String parsing was a little error prone, so we'd prefer to ask the bazaar repository whether a particular string is a valid revision ID. To do this, we need a very slightly custom version of disambiguateRevision for *bzrSource. This revision also refactors TestSourceManager_InferConstraint pretty dramatically to let it work on repositories coming from different VCSes. --- internal/gps/source_manager.go | 21 ----- internal/gps/source_manager_test.go | 124 +++++++++++++++------------- internal/gps/vcs_source.go | 15 ++++ 3 files changed, 82 insertions(+), 78 deletions(-) diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index 6382736ad3..eb619dc158 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -11,7 +11,6 @@ import ( "os/signal" "path/filepath" "runtime" - "strconv" "strings" "sync" "sync/atomic" @@ -554,26 +553,6 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint return r, nil } - // Bazaar revision - // - // Bazaar 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 len(s) == 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 - } - } - return nil, errors.Errorf("%s is not a valid version for the package %s(%s)", s, pi.ProjectRoot, pi.Source) } diff --git a/internal/gps/source_manager_test.go b/internal/gps/source_manager_test.go index 81d3df2d76..7127b4a6b0 100644 --- a/internal/gps/source_manager_test.go +++ b/internal/gps/source_manager_test.go @@ -13,72 +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"), - "3f4c3bea144e112a69bbe5d8d01c1b09a544253f": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"), - "3f4c3bea": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"), - - // 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"))) + }) } diff --git a/internal/gps/vcs_source.go b/internal/gps/vcs_source.go index 784eb6b8c4..bff5e6b1d0 100644 --- a/internal/gps/vcs_source.go +++ b/internal/gps/vcs_source.go @@ -414,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 { From 7f19f8ba9b92c45459515d5d91a2034764c83849 Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Mon, 28 Aug 2017 13:54:06 -0400 Subject: [PATCH 7/8] Use structs for subtests instead of a function literal --- internal/gps/source_manager_test.go | 135 +++++++++++++++------------- 1 file changed, 75 insertions(+), 60 deletions(-) diff --git a/internal/gps/source_manager_test.go b/internal/gps/source_manager_test.go index 7127b4a6b0..77512cbcaf 100644 --- a/internal/gps/source_manager_test.go +++ b/internal/gps/source_manager_test.go @@ -14,8 +14,75 @@ import ( func TestSourceManager_InferConstraint(t *testing.T) { t.Parallel() - testcase := func(str string, pi ProjectIdentifier, want Constraint) func(*testing.T) { - return func(t *testing.T) { + // Used in git subtests: + v081, err := NewSemverConstraintIC("v0.8.1") + if err != nil { + t.Fatal(err) + } + v012, err := NewSemverConstraintIC("v0.12.0-12-de4dcafe0") + if err != nil { + t.Fatal(err) + } + + // Used in hg and bzr subtests: + v1, err := NewSemverConstraintIC("v1.0.0") + if err != nil { + t.Fatal(err) + } + + var ( + gitProj = ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"} + bzrProj = ProjectIdentifier{ProjectRoot: "launchpad.net/govcstestbzrrepo"} + hgProj = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"} + svnProj = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"} + + 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")}, + } + ) + + 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 + case svnProj: + subtestName = "svn-" + tc.name + default: + subtestName = tc.name + } + + t.Run(subtestName, func(t *testing.T) { t.Parallel() h := test.NewHelper(t) defer h.Cleanup() @@ -26,69 +93,17 @@ func TestSourceManager_InferConstraint(t *testing.T) { sm, err := NewSourceManager(h.Path(cacheDir)) h.Must(err) - got, err := sm.InferConstraint(str, pi) + got, err := sm.InferConstraint(tc.str, tc.project) h.Must(err) - wantT := reflect.TypeOf(want) + wantT := reflect.TypeOf(tc.want) gotT := reflect.TypeOf(got) if wantT != gotT { - t.Errorf("expected type: %s, got %s, for input %s", wantT, gotT, str) + t.Errorf("expected type: %s, got %s, for input %s", wantT, gotT, tc.str) } - if got.String() != want.String() { - t.Errorf("expected value: %s, got %s for input %s", want, got, str) + if got.String() != tc.want.String() { + t.Errorf("expected value: %s, got %s for input %s", tc.want, got, tc.str) } - } + }) } - - var ( - gitProj = ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"} - bzrProj = ProjectIdentifier{ProjectRoot: "launchpad.net/govcstestbzrrepo"} - hgProj = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"} - ) - - t.Run("git", func(t *testing.T) { - t.Parallel() - t.Run("empty", testcase("", gitProj, Any())) - - v081, err := NewSemverConstraintIC("v0.8.1") - if err != nil { - t.Fatal(err) - } - - 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"))) - }) } From 49bf34074c5e0211bba57461ac59c4aa9c095f12 Mon Sep 17 00:00:00 2001 From: Spencer Nelson Date: Mon, 28 Aug 2017 14:26:02 -0400 Subject: [PATCH 8/8] Don't try to test constraint inference for svn --- internal/gps/source_manager_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/gps/source_manager_test.go b/internal/gps/source_manager_test.go index 77512cbcaf..83df379446 100644 --- a/internal/gps/source_manager_test.go +++ b/internal/gps/source_manager_test.go @@ -34,7 +34,6 @@ func TestSourceManager_InferConstraint(t *testing.T) { gitProj = ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"} bzrProj = ProjectIdentifier{ProjectRoot: "launchpad.net/govcstestbzrrepo"} hgProj = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"} - svnProj = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"} testcases = []struct { project ProjectIdentifier @@ -76,8 +75,6 @@ func TestSourceManager_InferConstraint(t *testing.T) { subtestName = "bzr-" + tc.name case hgProj: subtestName = "hg-" + tc.name - case svnProj: - subtestName = "svn-" + tc.name default: subtestName = tc.name }