From 898c1dfe183550dd445f8c1279ce69a3364fef03 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 5 Sep 2017 22:23:00 -0400 Subject: [PATCH 1/4] Separate gopkg.in sources via an aliasURL --- internal/gps/maybe_source.go | 4 +++- internal/gps/vcs_source.go | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/gps/maybe_source.go b/internal/gps/maybe_source.go index 31774887ba..38e068849c 100644 --- a/internal/gps/maybe_source.go +++ b/internal/gps/maybe_source.go @@ -144,7 +144,8 @@ func (m maybeGopkginSource) try(ctx context.Context, cachedir string, c singleSo // We don't actually need a fully consistent transform into the on-disk path // - just something that's unique to the particular gopkg.in domain context. // So, it's OK to just dumb-join the scheme with the path. - path := sourceCachePath(cachedir, m.url.Scheme+"/"+m.opath) + aliasURL := m.url.Scheme + "://" + m.opath + path := sourceCachePath(cachedir, aliasURL) ustr := m.url.String() r, err := newCtxRepo(vcs.Git, ustr, path) @@ -160,6 +161,7 @@ func (m maybeGopkginSource) try(ctx context.Context, cachedir string, c singleSo }, major: m.major, unstable: m.unstable, + aliasURL: aliasURL, } var vl []PairedVersion diff --git a/internal/gps/vcs_source.go b/internal/gps/vcs_source.go index 4890da2191..cbae3602a2 100644 --- a/internal/gps/vcs_source.go +++ b/internal/gps/vcs_source.go @@ -286,6 +286,13 @@ type gopkginSource struct { gitSource major uint64 unstable bool + // The aliased URL we report as being the one we talk to, even though we're + // actually talking directly to GitHub. + aliasURL string +} + +func (s *gopkginSource) upstreamURL() string { + return s.aliasURL } func (s *gopkginSource) listVersions(ctx context.Context) ([]PairedVersion, error) { From 7ae2d01e303e09ce73d3387ceea7afa44fd209b8 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 5 Sep 2017 22:55:17 -0400 Subject: [PATCH 2/4] Add test for gopkg.in src uniqueness --- internal/gps/manager_test.go | 60 ++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index 2dca368c0d..36dc700547 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -348,6 +348,66 @@ func TestMgrMethodsFailWithBadPath(t *testing.T) { } } +type sourceCreationTestFixture struct { + roots []ProjectIdentifier + urlcount, srccount int +} + +func (f sourceCreationTestFixture) run(t *testing.T) { + t.Parallel() + sm, clean := mkNaiveSM(t) + defer clean() + + for _, pi := range f.roots { + _, err := sm.SourceExists(pi) + if err != nil { + t.Fatal(err) + } + } + + if len(sm.srcCoord.nameToURL) != f.urlcount { + t.Errorf("want %v names in the name->url map, but got %v. contents: \n%v", f.urlcount, len(sm.srcCoord.nameToURL), sm.srcCoord.nameToURL) + } + + if len(sm.srcCoord.srcs) != f.srccount { + t.Errorf("want %v gateways in the sources map, but got %v", f.srccount, len(sm.srcCoord.srcs)) + } +} + +// This test is primarily about making sure that the logic around folding +// together different ways of referencing the same underlying resource - whether +// that be intentionally folding them, or intentionally keeping them separate - +// work as intended. +func TestSourceCreationCounts(t *testing.T) { + if testing.Short() { + t.Skip("Skipping slow test in short mode") + } + + fixtures := map[string]sourceCreationTestFixture{ + "gopkgin uniqueness": { + roots: []ProjectIdentifier{ + mkPI("gopkg.in/sdboyer/gpkt.v1"), + mkPI("gopkg.in/sdboyer/gpkt.v2"), + mkPI("gopkg.in/sdboyer/gpkt.v3"), + }, + urlcount: 6, + srccount: 3, + }, + "gopkgin separation from github": { + roots: []ProjectIdentifier{ + mkPI("gopkg.in/sdboyer/gpkt.v1"), + mkPI("github.com/sdboyer/gpkt"), + }, + urlcount: 4, + srccount: 2, + }, + } + + for name, fix := range fixtures { + t.Run(name, fix.run) + } +} + func TestGetSources(t *testing.T) { // This test is a tad slow, skip it on -short if testing.Short() { From 5ecd38e07d1c9d7440ebddeb80b1bdcf83d679b9 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 5 Sep 2017 23:12:32 -0400 Subject: [PATCH 3/4] Map URL to itself to prevent more work later --- internal/gps/source.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/gps/source.go b/internal/gps/source.go index a2d5d076e8..5f8b34219b 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -168,8 +168,12 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // integrate it back into the main map. sc.srcmut.Lock() defer sc.srcmut.Unlock() - // Record the name -> URL mapping, even if it's a self-mapping. + // Record the name -> URL mapping, making sure that we also get the + // self-mapping. sc.nameToURL[normalizedName] = url + if url != normalizedName { + sc.nameToURL[url] = url + } if sa, has := sc.srcs[url]; has { // URL already had an entry in the main map; use that as the result. From b41ae7945240d9dd63135dacb2d10ff5ac7db33e Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 5 Sep 2017 23:25:12 -0400 Subject: [PATCH 4/4] Expect the alias URL in gopkgin source tests --- internal/gps/vcs_source_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/gps/vcs_source_test.go b/internal/gps/vcs_source_test.go index a13a1c6b6d..fd9c372dfc 100644 --- a/internal/gps/vcs_source_test.go +++ b/internal/gps/vcs_source_test.go @@ -151,8 +151,8 @@ func testGopkginSourceInteractions(t *testing.T) { }() tfunc := func(opath, n string, major uint64, evl []Version) { - un := "https://" + n - u, err := url.Parse(un) + un := "https://" + opath + u, err := url.Parse("https://" + n) if err != nil { t.Errorf("URL was bad, lolwut? errtext: %s", err) return