diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b741b3bab..bf48d74694 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ BUG FIXES: IMPROVEMENTS: +* Reduce network access by trusting local source information and only pulling +from upstream when necessary ([#1250](https://github.com/golang/dep/pull/1250)). + # v0.4.1 BUG FIXES: diff --git a/gps/deduce.go b/gps/deduce.go index d29481d46c..796681738f 100644 --- a/gps/deduce.go +++ b/gps/deduce.go @@ -16,7 +16,7 @@ import ( "strings" "sync" - radix "github.com/armon/go-radix" + "github.com/armon/go-radix" "github.com/pkg/errors" ) @@ -104,7 +104,7 @@ type pathDeducer interface { // So the return of the above string would be // "github.com/some-user/some-package" deduceRoot(string) (string, error) - deduceSource(string, *url.URL) (maybeSource, error) + deduceSource(string, *url.URL) (maybeSources, error) } type githubDeducer struct { @@ -120,7 +120,7 @@ func (m githubDeducer) deduceRoot(path string) (string, error) { return "github.com" + v[2], nil } -func (m githubDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) { +func (m githubDeducer) deduceSource(path string, u *url.URL) (maybeSources, error) { v := m.regexp.FindStringSubmatch(path) if v == nil { return nil, fmt.Errorf("%s is not a valid path for a source on github.com", path) @@ -138,7 +138,7 @@ func (m githubDeducer) deduceSource(path string, u *url.URL) (maybeSource, error if u.Scheme == "ssh" { u.User = url.User("git") } - return maybeGitSource{url: u}, nil + return maybeSources{maybeGitSource{url: u}}, nil } mb := make(maybeSources, len(gitSchemes)) @@ -167,7 +167,7 @@ func (m bitbucketDeducer) deduceRoot(path string) (string, error) { return "bitbucket.org" + v[2], nil } -func (m bitbucketDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) { +func (m bitbucketDeducer) deduceSource(path string, u *url.URL) (maybeSources, error) { v := m.regexp.FindStringSubmatch(path) if v == nil { return nil, fmt.Errorf("%s is not a valid path for a source on bitbucket.org", path) @@ -189,12 +189,12 @@ func (m bitbucketDeducer) deduceSource(path string, u *url.URL) (maybeSource, er // superset of the hg schemes return nil, fmt.Errorf("%s is not a valid scheme for accessing a git repository", u.Scheme) } - return maybeGitSource{url: u}, nil + return maybeSources{maybeGitSource{url: u}}, nil } else if ishg { if !validhg { return nil, fmt.Errorf("%s is not a valid scheme for accessing an hg repository", u.Scheme) } - return maybeHgSource{url: u}, nil + return maybeSources{maybeHgSource{url: u}}, nil } else if !validgit && !validhg { return nil, fmt.Errorf("%s is not a valid scheme for accessing either a git or hg repository", u.Scheme) } @@ -265,7 +265,7 @@ func (m gopkginDeducer) parseAndValidatePath(p string) ([]string, error) { return v, nil } -func (m gopkginDeducer) deduceSource(p string, u *url.URL) (maybeSource, error) { +func (m gopkginDeducer) deduceSource(p string, u *url.URL) (maybeSources, error) { // Reuse root detection logic for initial validation v, err := m.parseAndValidatePath(p) if err != nil { @@ -329,7 +329,7 @@ func (m launchpadDeducer) deduceRoot(path string) (string, error) { return "launchpad.net" + v[2], nil } -func (m launchpadDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) { +func (m launchpadDeducer) deduceSource(path string, u *url.URL) (maybeSources, error) { v := m.regexp.FindStringSubmatch(path) if v == nil { return nil, fmt.Errorf("%s is not a valid path for a source on launchpad.net", path) @@ -342,7 +342,7 @@ func (m launchpadDeducer) deduceSource(path string, u *url.URL) (maybeSource, er if !validateVCSScheme(u.Scheme, "bzr") { return nil, fmt.Errorf("%s is not a valid scheme for accessing a bzr repository", u.Scheme) } - return maybeBzrSource{url: u}, nil + return maybeSources{maybeBzrSource{url: u}}, nil } mb := make(maybeSources, len(bzrSchemes)) @@ -369,7 +369,7 @@ func (m launchpadGitDeducer) deduceRoot(path string) (string, error) { return "git.launchpad.net" + v[2], nil } -func (m launchpadGitDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) { +func (m launchpadGitDeducer) deduceSource(path string, u *url.URL) (maybeSources, error) { v := m.regexp.FindStringSubmatch(path) if v == nil { return nil, fmt.Errorf("%s is not a valid path for a source on git.launchpad.net", path) @@ -382,7 +382,7 @@ func (m launchpadGitDeducer) deduceSource(path string, u *url.URL) (maybeSource, if !validateVCSScheme(u.Scheme, "git") { return nil, fmt.Errorf("%s is not a valid scheme for accessing a git repository", u.Scheme) } - return maybeGitSource{url: u}, nil + return maybeSources{maybeGitSource{url: u}}, nil } mb := make(maybeSources, len(gitSchemes)) @@ -408,7 +408,7 @@ func (m jazzDeducer) deduceRoot(path string) (string, error) { return "hub.jazz.net" + v[2], nil } -func (m jazzDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) { +func (m jazzDeducer) deduceSource(path string, u *url.URL) (maybeSources, error) { v := m.regexp.FindStringSubmatch(path) if v == nil { return nil, fmt.Errorf("%s is not a valid path for a source on hub.jazz.net", path) @@ -422,7 +422,7 @@ func (m jazzDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) u.Scheme = "https" fallthrough case "https": - return maybeGitSource{url: u}, nil + return maybeSources{maybeGitSource{url: u}}, nil default: return nil, fmt.Errorf("IBM's jazz hub only supports https, %s is not allowed", u.String()) } @@ -441,7 +441,7 @@ func (m apacheDeducer) deduceRoot(path string) (string, error) { return "git.apache.org" + v[2], nil } -func (m apacheDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) { +func (m apacheDeducer) deduceSource(path string, u *url.URL) (maybeSources, error) { v := m.regexp.FindStringSubmatch(path) if v == nil { return nil, fmt.Errorf("%s is not a valid path for a source on git.apache.org", path) @@ -454,7 +454,7 @@ func (m apacheDeducer) deduceSource(path string, u *url.URL) (maybeSource, error if !validateVCSScheme(u.Scheme, "git") { return nil, fmt.Errorf("%s is not a valid scheme for accessing a git repository", u.Scheme) } - return maybeGitSource{url: u}, nil + return maybeSources{maybeGitSource{url: u}}, nil } mb := make(maybeSources, len(gitSchemes)) @@ -480,7 +480,7 @@ func (m vcsExtensionDeducer) deduceRoot(path string) (string, error) { return v[1], nil } -func (m vcsExtensionDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) { +func (m vcsExtensionDeducer) deduceSource(path string, u *url.URL) (maybeSources, error) { v := m.regexp.FindStringSubmatch(path) if v == nil { return nil, fmt.Errorf("%s contains no vcs extension hints for matching", path) @@ -500,11 +500,11 @@ func (m vcsExtensionDeducer) deduceSource(path string, u *url.URL) (maybeSource, switch v[4] { case "git": - return maybeGitSource{url: u}, nil + return maybeSources{maybeGitSource{url: u}}, nil case "bzr": - return maybeBzrSource{url: u}, nil + return maybeSources{maybeBzrSource{url: u}}, nil case "hg": - return maybeHgSource{url: u}, nil + return maybeSources{maybeHgSource{url: u}}, nil } } @@ -590,7 +590,7 @@ func (dc *deductionCoordinator) deduceRootPath(ctx context.Context, path string) dc.mut.RUnlock() if has && isPathPrefixOrEqual(prefix, path) { switch d := data.(type) { - case maybeSource: + case maybeSources: return pathDeduction{root: prefix, mb: d}, nil case *httpMetadataDeducer: // Multiple calls have come in for a similar path shape during @@ -652,7 +652,7 @@ func (dc *deductionCoordinator) deduceRootPath(ctx context.Context, path string) // the source. type pathDeduction struct { root string - mb maybeSource + mb maybeSources } var errNoKnownPathMatch = errors.New("no known path match") @@ -759,11 +759,11 @@ func (hmd *httpMetadataDeducer) deduce(ctx context.Context, path string) (pathDe switch vcs { case "git": - pd.mb = maybeGitSource{url: repoURL} + pd.mb = maybeSources{maybeGitSource{url: repoURL}} case "bzr": - pd.mb = maybeBzrSource{url: repoURL} + pd.mb = maybeSources{maybeBzrSource{url: repoURL}} case "hg": - pd.mb = maybeHgSource{url: repoURL} + pd.mb = maybeSources{maybeHgSource{url: repoURL}} default: hmd.deduceErr = errors.Errorf("unsupported vcs type %s in go-get metadata from %s", vcs, path) return diff --git a/gps/deduce_test.go b/gps/deduce_test.go index bb44d1f54f..0eb05eb359 100644 --- a/gps/deduce_test.go +++ b/gps/deduce_test.go @@ -18,7 +18,7 @@ type pathDeductionFixture struct { in string root string rerr error - mb maybeSource + mb maybeSources srcerr error } @@ -69,17 +69,23 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ { in: "git@github.com:sdboyer/gps", root: "github.com/sdboyer/gps", - mb: maybeGitSource{url: mkurl("ssh://git@github.com/sdboyer/gps")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("ssh://git@github.com/sdboyer/gps")}, + }, }, { in: "https://github.com/sdboyer/gps", root: "github.com/sdboyer/gps", - mb: maybeGitSource{url: mkurl("https://github.com/sdboyer/gps")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://github.com/sdboyer/gps")}, + }, }, { in: "https://github.com/sdboyer/gps/foo/bar", root: "github.com/sdboyer/gps", - mb: maybeGitSource{url: mkurl("https://github.com/sdboyer/gps")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://github.com/sdboyer/gps")}, + }, }, { in: "github.com/sdboyer-/gps/foo", @@ -163,6 +169,7 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ in: "gopkg.in/yaml.v1/foo/bar", root: "gopkg.in/yaml.v1", mb: maybeSources{ + maybeGopkginSource{opath: "gopkg.in/yaml.v1", url: mkurl("https://github.com/go-yaml/yaml"), major: 1}, maybeGopkginSource{opath: "gopkg.in/yaml.v1", url: mkurl("http://github.com/go-yaml/yaml"), major: 1}, }, @@ -186,12 +193,16 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ { in: "hub.jazz.net/git/user1/pkgname", root: "hub.jazz.net/git/user1/pkgname", - mb: maybeGitSource{url: mkurl("https://hub.jazz.net/git/user1/pkgname")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://hub.jazz.net/git/user1/pkgname")}, + }, }, { in: "hub.jazz.net/git/user1/pkgname/submodule/submodule/submodule", root: "hub.jazz.net/git/user1/pkgname", - mb: maybeGitSource{url: mkurl("https://hub.jazz.net/git/user1/pkgname")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://hub.jazz.net/git/user1/pkgname")}, + }, }, { in: "hub.jazz.net/someotherprefix", @@ -218,7 +229,9 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ { in: "hub.jazz.net/git/user1/pkg.name", root: "hub.jazz.net/git/user1/pkg.name", - mb: maybeGitSource{url: mkurl("https://hub.jazz.net/git/user1/pkg.name")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://hub.jazz.net/git/user1/pkg.name")}, + }, }, // User names cannot have uppercase letters { @@ -275,7 +288,9 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ { in: "git@bitbucket.org:sdboyer/reporoot.git", root: "bitbucket.org/sdboyer/reporoot.git", - mb: maybeGitSource{url: mkurl("ssh://git@bitbucket.org/sdboyer/reporoot.git")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("ssh://git@bitbucket.org/sdboyer/reporoot.git")}, + }, }, { in: "bitbucket.org/sdboyer/reporoot.hg", @@ -289,7 +304,9 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ { in: "hg@bitbucket.org:sdboyer/reporoot", root: "bitbucket.org/sdboyer/reporoot", - mb: maybeHgSource{url: mkurl("ssh://hg@bitbucket.org/sdboyer/reporoot")}, + mb: maybeSources{ + maybeHgSource{url: mkurl("ssh://hg@bitbucket.org/sdboyer/reporoot")}, + }, }, { in: "git://bitbucket.org/sdboyer/reporoot.hg", @@ -417,22 +434,30 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ { in: "git@foobar.com:baz.git", root: "foobar.com/baz.git", - mb: maybeGitSource{url: mkurl("ssh://git@foobar.com/baz.git")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("ssh://git@foobar.com/baz.git")}, + }, }, { in: "bzr+ssh://foobar.com/baz.bzr", root: "foobar.com/baz.bzr", - mb: maybeBzrSource{url: mkurl("bzr+ssh://foobar.com/baz.bzr")}, + mb: maybeSources{ + maybeBzrSource{url: mkurl("bzr+ssh://foobar.com/baz.bzr")}, + }, }, { in: "ssh://foobar.com/baz.bzr", root: "foobar.com/baz.bzr", - mb: maybeBzrSource{url: mkurl("ssh://foobar.com/baz.bzr")}, + mb: maybeSources{ + maybeBzrSource{url: mkurl("ssh://foobar.com/baz.bzr")}, + }, }, { in: "https://foobar.com/baz.hg", root: "foobar.com/baz.hg", - mb: maybeHgSource{url: mkurl("https://foobar.com/baz.hg")}, + mb: maybeSources{ + maybeHgSource{url: mkurl("https://foobar.com/baz.hg")}, + }, }, { in: "git://foobar.com/baz.hg", @@ -457,17 +482,23 @@ var pathDeductionFixtures = map[string][]pathDeductionFixture{ { in: "golang.org/x/exp", root: "golang.org/x/exp", - mb: maybeGitSource{url: mkurl("https://go.googlesource.com/exp")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://go.googlesource.com/exp")}, + }, }, { in: "golang.org/x/exp/inotify", root: "golang.org/x/exp", - mb: maybeGitSource{url: mkurl("https://go.googlesource.com/exp")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://go.googlesource.com/exp")}, + }, }, { in: "golang.org/x/net/html", root: "golang.org/x/net", - mb: maybeGitSource{url: mkurl("https://go.googlesource.com/net")}, + mb: maybeSources{ + maybeGitSource{url: mkurl("https://go.googlesource.com/net")}, + }, }, }, } @@ -501,28 +532,13 @@ func TestDeduceFromPath(t *testing.T) { t.SkipNow() } - var printmb func(mb maybeSource, t *testing.T) string - printmb = func(mb maybeSource, t *testing.T) string { - switch tmb := mb.(type) { - case maybeSources: - var buf bytes.Buffer - fmt.Fprintf(&buf, "%v maybeSources:", len(tmb)) - for _, elem := range tmb { - fmt.Fprintf(&buf, "\n\t\t%s", printmb(elem, t)) - } - return buf.String() - case maybeGitSource: - return fmt.Sprintf("%T: %s", tmb, ufmt(tmb.url)) - case maybeBzrSource: - return fmt.Sprintf("%T: %s", tmb, ufmt(tmb.url)) - case maybeHgSource: - return fmt.Sprintf("%T: %s", tmb, ufmt(tmb.url)) - case maybeGopkginSource: - return fmt.Sprintf("%T: %s (v%v) %s ", tmb, tmb.opath, tmb.major, ufmt(tmb.url)) - default: - t.Errorf("Unknown maybeSource type: %T", mb) + printmb := func(mb maybeSources) string { + var buf bytes.Buffer + fmt.Fprintf(&buf, "%v maybeSources:", len(mb)) + for _, elem := range mb { + fmt.Fprintf(&buf, "\n\t\t%s", elem) } - return "" + return buf.String() } for _, fix := range fixtures { @@ -564,16 +580,11 @@ func TestDeduceFromPath(t *testing.T) { } } else if !reflect.DeepEqual(mb, fix.mb) { if mb == nil { - t.Errorf("Deducer returned source maybes, but none expected:\n\t(GOT) (none)\n\t(WNT) %s", printmb(fix.mb, t)) + t.Errorf("Deducer returned source maybes, but none expected:\n\t(GOT) (none)\n\t(WNT) %s", printmb(fix.mb)) } else if fix.mb == nil { - t.Errorf("Deducer returned source maybes, but none expected:\n\t(GOT) %s\n\t(WNT) (none)", printmb(mb, t)) + t.Errorf("Deducer returned source maybes, but none expected:\n\t(GOT) %s\n\t(WNT) (none)", printmb(mb)) } else { - t.Errorf("Deducer did not return expected source:\n\t(GOT) %s\n\t(WNT) %s", printmb(mb, t), printmb(fix.mb, t)) - } - } else { - gotURLs, wantURLs := mb.possibleURLs(), fix.mb.possibleURLs() - if !reflect.DeepEqual(gotURLs, wantURLs) { - t.Errorf("Deducer did not return expected source:\n\t(GOT) %s\n\t(WNT) %s", gotURLs, wantURLs) + t.Errorf("Deducer did not return expected source:\n\t(GOT) %s\n\t(WNT) %s", printmb(mb), printmb(fix.mb)) } } }) @@ -623,7 +634,12 @@ func TestVanityDeduction(t *testing.T) { return } - goturl, wanturl := pd.mb.(maybeGitSource).url.String(), fix.mb.(maybeGitSource).url.String() + if len(pd.mb) != 1 { + t.Errorf("Expected single maybeSource, but found: %d", len(pd.mb)) + return + } + + goturl, wanturl := pd.mb[0].(maybeGitSource).url.String(), fix.mb[0].(maybeGitSource).url.String() if goturl != wanturl { t.Errorf("Deduced repo ident does not match fixture:\n\t(GOT) %s\n\t(WNT) %s", goturl, wanturl) } @@ -663,17 +679,3 @@ func TestVanityDeductionSchemeMismatch(t *testing.T) { t.Error("should have errored on scheme mismatch between input and go-get metadata") } } - -// borrow from stdlib -// more useful string for debugging than fmt's struct printer -func ufmt(u *url.URL) string { - var user, pass interface{} - if u.User != nil { - user = u.User.Username() - if p, ok := u.User.Password(); ok { - pass = p - } - } - return fmt.Sprintf("host=%q, path=%q, opaque=%q, scheme=%q, user=%#v, pass=%#v, rawpath=%q, rawq=%q, frag=%q", - u.Host, u.Path, u.Opaque, u.Scheme, user, pass, u.RawPath, u.RawQuery, u.Fragment) -} diff --git a/gps/error.go b/gps/error.go new file mode 100644 index 0000000000..ea15957b8a --- /dev/null +++ b/gps/error.go @@ -0,0 +1,34 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gps + +import ( + "bytes" + "fmt" +) + +type errorSlice []error + +func (errs errorSlice) Error() string { + var buf bytes.Buffer + fmt.Fprintln(&buf) + for i, err := range errs { + fmt.Fprintf(&buf, "\t(%d) %s\n", i+1, err) + } + return buf.String() +} + +func (errs errorSlice) Format(f fmt.State, c rune) { + fmt.Fprintln(f) + for i, err := range errs { + if ferr, ok := err.(fmt.Formatter); ok { + fmt.Fprintf(f, "\t(%d) ", i+1) + ferr.Format(f, c) + fmt.Fprint(f, "\n") + } else { + fmt.Fprintf(f, "\t(%d) %s\n", i+1, err) + } + } +} diff --git a/gps/manager_test.go b/gps/manager_test.go index 4551e60816..1ea9108825 100644 --- a/gps/manager_test.go +++ b/gps/manager_test.go @@ -376,24 +376,26 @@ func (f sourceCreationTestFixture) run(t *testing.T) { t.Errorf("want %v gateways in the sources map, but got %v", f.srccount, len(sm.srcCoord.srcs)) } - var keys []string - for k := range sm.srcCoord.nameToURL { - keys = append(keys, k) - } - sort.Strings(keys) + if t.Failed() { + var keys []string + for k := range sm.srcCoord.nameToURL { + keys = append(keys, k) + } + sort.Strings(keys) - var buf bytes.Buffer - w := tabwriter.NewWriter(&buf, 0, 4, 2, ' ', 0) - fmt.Fprint(w, "NAME\tMAPPED URL\n") - for _, r := range keys { - fmt.Fprintf(w, "%s\t%s\n", r, sm.srcCoord.nameToURL[r]) - } - w.Flush() - t.Log("\n", buf.String()) + var buf bytes.Buffer + w := tabwriter.NewWriter(&buf, 0, 4, 2, ' ', 0) + fmt.Fprint(w, "NAME\tMAPPED URL\n") + for _, r := range keys { + fmt.Fprintf(w, "%s\t%s\n", r, sm.srcCoord.nameToURL[r]) + } + w.Flush() + t.Log("\n", buf.String()) - t.Log("SRC KEYS") - for k := range sm.srcCoord.srcs { - t.Log(k) + t.Log("SRC KEYS") + for k := range sm.srcCoord.srcs { + t.Log(k) + } } } @@ -420,9 +422,11 @@ func TestSourceCreationCounts(t *testing.T) { roots: []ProjectIdentifier{ mkPI("gopkg.in/sdboyer/gpkt.v1"), mkPI("github.com/sdboyer/gpkt"), + mkPI("http://github.com/sdboyer/gpkt"), + mkPI("https://github.com/sdboyer/gpkt"), }, - namecount: 4, - srccount: 2, + namecount: 5, + srccount: 3, }, "case variance across path and URL-based access": { roots: []ProjectIdentifier{ @@ -533,16 +537,14 @@ func TestFSCaseSensitivityConvergesSources(t *testing.T) { } path1 := sg1.src.(*gitSource).repo.LocalPath() - t.Log("path1:", path1) stat1, err := os.Stat(path1) if err != nil { - t.Fatal(err) + t.Fatal("path1:", path1, err) } path2 := sg2.src.(*gitSource).repo.LocalPath() - t.Log("path2:", path2) stat2, err := os.Stat(path2) if err != nil { - t.Fatal(err) + t.Fatal("path2:", path2, err) } same, count := os.SameFile(stat1, stat2), len(sm.srcCoord.srcs) diff --git a/gps/maybe_source.go b/gps/maybe_source.go index 3128181dcd..ea4b79d59d 100644 --- a/gps/maybe_source.go +++ b/gps/maybe_source.go @@ -5,14 +5,13 @@ package gps import ( - "bytes" "context" "fmt" "net/url" + "os" "path/filepath" "github.com/Masterminds/vcs" - "github.com/pkg/errors" ) // A maybeSource represents a set of information that, given some @@ -23,46 +22,18 @@ import ( // * Allows control over when deduction logic triggers network activity // * Makes it easy to attempt multiple URLs for a given import path type maybeSource interface { - try(ctx context.Context, cachedir string, c singleSourceCache, superv *supervisor) (source, sourceState, error) - possibleURLs() []*url.URL -} - -type errorSlice []error - -func (errs *errorSlice) Error() string { - var buf bytes.Buffer - for _, err := range *errs { - fmt.Fprintf(&buf, "\n\t%s", err) - } - return buf.String() + // try tries to set up a source. + try(ctx context.Context, cachedir string) (source, error) + URL() *url.URL + fmt.Stringer } type maybeSources []maybeSource -func (mbs maybeSources) try(ctx context.Context, cachedir string, c singleSourceCache, superv *supervisor) (source, sourceState, error) { - var errs errorSlice - for _, mb := range mbs { - src, state, err := mb.try(ctx, cachedir, c, superv) - if err == nil { - return src, state, nil - } - urls := "" - for _, url := range mb.possibleURLs() { - urls += url.String() + "\n" - } - errs = append(errs, errors.Wrapf(err, "failed to set up sources from the following URLs:\n%s", urls)) - } - - return nil, 0, errors.Wrap(&errs, "no valid source could be created") -} - -// This really isn't generally intended to be used - the interface is for -// maybeSources to be able to interrogate its members, not other things to -// interrogate a maybeSources. func (mbs maybeSources) possibleURLs() []*url.URL { - urlslice := make([]*url.URL, 0, len(mbs)) - for _, mb := range mbs { - urlslice = append(urlslice, mb.possibleURLs()...) + urlslice := make([]*url.URL, len(mbs)) + for i, mb := range mbs { + urlslice[i] = mb.URL() } return urlslice } @@ -76,50 +47,32 @@ type maybeGitSource struct { url *url.URL } -func (m maybeGitSource) try(ctx context.Context, cachedir string, c singleSourceCache, superv *supervisor) (source, sourceState, error) { +func (m maybeGitSource) try(ctx context.Context, cachedir string) (source, error) { ustr := m.url.String() + path := sourceCachePath(cachedir, ustr) - r, err := newCtxRepo(vcs.Git, ustr, sourceCachePath(cachedir, ustr)) + r, err := vcs.NewGitRepo(ustr, path) if err != nil { - return nil, 0, unwrapVcsErr(err) + os.RemoveAll(path) + r, err = vcs.NewGitRepo(ustr, path) + if err != nil { + return nil, unwrapVcsErr(err) + } } - src := &gitSource{ + return &gitSource{ baseVCSSource: baseVCSSource{ - repo: r, + repo: &gitRepo{r}, }, - } - - // Pinging invokes the same action as calling listVersions, so just do that. - var vl []PairedVersion - if err := superv.do(ctx, "git:lv:maybe", ctListVersions, func(ctx context.Context) error { - var err error - vl, err = src.listVersions(ctx) - return errors.Wrapf(err, "remote repository at %s does not exist, or is inaccessible", ustr) - }); err != nil { - return nil, 0, err - } - - state := sourceIsSetUp | sourceExistsUpstream | sourceHasLatestVersionList - - if r.CheckLocal() { - state |= sourceExistsLocally - - if err := superv.do(ctx, "git", ctValidateLocal, func(ctx context.Context) error { - // If repository already exists on disk, make a pass to be sure - // everything's clean. - return src.ensureClean(ctx) - }); err != nil { - return nil, 0, err - } - } + }, nil +} - c.setVersionMap(vl) - return src, state, nil +func (m maybeGitSource) URL() *url.URL { + return m.url } -func (m maybeGitSource) possibleURLs() []*url.URL { - return []*url.URL{m.url} +func (m maybeGitSource) String() string { + return fmt.Sprintf("%T: %s", m, ufmt(m.url)) } type maybeGopkginSource struct { @@ -136,7 +89,7 @@ type maybeGopkginSource struct { unstable bool } -func (m maybeGopkginSource) try(ctx context.Context, cachedir string, c singleSourceCache, superv *supervisor) (source, sourceState, error) { +func (m maybeGopkginSource) try(ctx context.Context, cachedir string) (source, error) { // 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. @@ -144,119 +97,112 @@ func (m maybeGopkginSource) try(ctx context.Context, cachedir string, c singleSo path := sourceCachePath(cachedir, aliasURL) ustr := m.url.String() - r, err := newCtxRepo(vcs.Git, ustr, path) + r, err := vcs.NewGitRepo(ustr, path) if err != nil { - return nil, 0, unwrapVcsErr(err) + os.RemoveAll(path) + r, err = vcs.NewGitRepo(ustr, path) + if err != nil { + return nil, unwrapVcsErr(err) + } } - src := &gopkginSource{ + return &gopkginSource{ gitSource: gitSource{ baseVCSSource: baseVCSSource{ - repo: r, + repo: &gitRepo{r}, }, }, major: m.major, unstable: m.unstable, aliasURL: aliasURL, - } - - var vl []PairedVersion - if err := superv.do(ctx, "git:lv:maybe", ctListVersions, func(ctx context.Context) error { - var err error - vl, err = src.listVersions(ctx) - return errors.Wrapf(err, "remote repository at %s does not exist, or is inaccessible", ustr) - }); err != nil { - return nil, 0, err - } - - c.setVersionMap(vl) - state := sourceIsSetUp | sourceExistsUpstream | sourceHasLatestVersionList + }, nil +} - if r.CheckLocal() { - state |= sourceExistsLocally +func (m maybeGopkginSource) URL() *url.URL { + return &url.URL{ + Scheme: m.url.Scheme, + Path: m.opath, } - - return src, state, nil } -func (m maybeGopkginSource) possibleURLs() []*url.URL { - return []*url.URL{m.url} +func (m maybeGopkginSource) String() string { + return fmt.Sprintf("%T: %s (v%v) %s ", m, m.opath, m.major, ufmt(m.url)) } type maybeBzrSource struct { url *url.URL } -func (m maybeBzrSource) try(ctx context.Context, cachedir string, c singleSourceCache, superv *supervisor) (source, sourceState, error) { +func (m maybeBzrSource) try(ctx context.Context, cachedir string) (source, error) { ustr := m.url.String() + path := sourceCachePath(cachedir, ustr) - r, err := newCtxRepo(vcs.Bzr, ustr, sourceCachePath(cachedir, ustr)) + r, err := vcs.NewBzrRepo(ustr, path) if err != nil { - return nil, 0, unwrapVcsErr(err) - } - - if err := superv.do(ctx, "bzr:ping", ctSourcePing, func(ctx context.Context) error { - if !r.Ping() { - return fmt.Errorf("remote repository at %s does not exist, or is inaccessible", ustr) + os.RemoveAll(path) + r, err = vcs.NewBzrRepo(ustr, path) + if err != nil { + return nil, unwrapVcsErr(err) } - return nil - }); err != nil { - return nil, 0, err - } - - state := sourceIsSetUp | sourceExistsUpstream - if r.CheckLocal() { - state |= sourceExistsLocally } - src := &bzrSource{ + return &bzrSource{ baseVCSSource: baseVCSSource{ - repo: r, + repo: &bzrRepo{r}, }, - } + }, nil +} - return src, state, nil +func (m maybeBzrSource) URL() *url.URL { + return m.url } -func (m maybeBzrSource) possibleURLs() []*url.URL { - return []*url.URL{m.url} +func (m maybeBzrSource) String() string { + return fmt.Sprintf("%T: %s", m, ufmt(m.url)) } type maybeHgSource struct { url *url.URL } -func (m maybeHgSource) try(ctx context.Context, cachedir string, c singleSourceCache, superv *supervisor) (source, sourceState, error) { +func (m maybeHgSource) try(ctx context.Context, cachedir string) (source, error) { ustr := m.url.String() + path := sourceCachePath(cachedir, ustr) - r, err := newCtxRepo(vcs.Hg, ustr, sourceCachePath(cachedir, ustr)) + r, err := vcs.NewHgRepo(ustr, path) if err != nil { - return nil, 0, unwrapVcsErr(err) - } - - if err := superv.do(ctx, "hg:ping", ctSourcePing, func(ctx context.Context) error { - if !r.Ping() { - return fmt.Errorf("remote repository at %s does not exist, or is inaccessible", ustr) + os.RemoveAll(path) + r, err = vcs.NewHgRepo(ustr, path) + if err != nil { + return nil, unwrapVcsErr(err) } - return nil - }); err != nil { - return nil, 0, err } - state := sourceIsSetUp | sourceExistsUpstream - if r.CheckLocal() { - state |= sourceExistsLocally - } - - src := &hgSource{ + return &hgSource{ baseVCSSource: baseVCSSource{ - repo: r, + repo: &hgRepo{r}, }, - } + }, nil +} + +func (m maybeHgSource) URL() *url.URL { + return m.url +} - return src, state, nil +func (m maybeHgSource) String() string { + return fmt.Sprintf("%T: %s", m, ufmt(m.url)) } -func (m maybeHgSource) possibleURLs() []*url.URL { - return []*url.URL{m.url} +// borrow from stdlib +// more useful string for debugging than fmt's struct printer +func ufmt(u *url.URL) string { + var user, pass interface{} + if u.User != nil { + user = u.User.Username() + if p, ok := u.User.Password(); ok { + pass = p + } + } + return fmt.Sprintf("host=%q, path=%q, opaque=%q, scheme=%q, user=%#v, pass=%#v, rawpath=%q, rawq=%q, frag=%q", + u.Host, u.Path, u.Opaque, u.Scheme, user, pass, u.RawPath, u.RawQuery, u.Fragment) } diff --git a/gps/maybe_source_test.go b/gps/maybe_source_test.go new file mode 100644 index 0000000000..464996aefe --- /dev/null +++ b/gps/maybe_source_test.go @@ -0,0 +1,139 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gps + +import ( + "archive/tar" + "compress/gzip" + "context" + "io" + "io/ioutil" + "net/url" + "os" + "path/filepath" + "testing" + + "github.com/Masterminds/vcs" +) + +func TestMaybeGitSource_try(t *testing.T) { + t.Parallel() + + tempDir, err := ioutil.TempDir("", "go-try-happy-test") + if err != nil { + t.Fatal(err) + } + defer func() { + err = os.RemoveAll(tempDir) + if err != nil { + t.Error(err) + } + }() + + url, err := url.Parse(gitRemoteTestRepo) + if err != nil { + t.Fatal(err) + } + var ms maybeSource = maybeGitSource{url: url} + _, err = ms.try(context.Background(), tempDir) + if err != nil { + t.Fatal(err) + } +} + +func TestMaybeGitSource_try_recovery(t *testing.T) { + t.Parallel() + + tempDir, err := ioutil.TempDir("", "go-try-recovery-test") + if err != nil { + t.Fatal(err) + } + defer func() { + err = os.RemoveAll(tempDir) + if err != nil { + t.Error(err) + } + }() + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + src := filepath.Join(cwd, "_testdata", "badrepo", "corrupt_dot_git_directory.tar") + f, err := os.Open(src) + if err != nil { + t.Fatal(err) + } + defer f.Close() + + dest := filepath.Join(tempDir, ".git") + err = untar(dest, f) + if err != nil { + t.Fatalf("could not untar corrupt repo into temp folder: %v\n", err) + } + + _, err = vcs.NewGitRepo(gitRemoteTestRepo, tempDir) + if err != nil { + if _, ok := err.(*vcs.LocalError); !ok { + t.Fatalf("expected a local error but got: %v\n", err) + } + } else { + t.Fatal("expected getVCSRepo to fail when pointing to a corrupt local path. It is possible that vcs.GitNewRepo updated to gracefully handle this test scenario. Check the return of vcs.GitNewRepo.") + } + + url, err := url.Parse(gitRemoteTestRepo) + if err != nil { + t.Fatal(err) + } + var ms maybeSource = maybeGitSource{url: url} + _, err = ms.try(context.Background(), tempDir) + if err != nil { + t.Fatal(err) + } +} + +func untar(dst string, r io.Reader) error { + gzr, err := gzip.NewReader(r) + if err != nil { + return err + } + defer gzr.Close() + + tr := tar.NewReader(gzr) + + for { + header, err := tr.Next() + + switch { + case err == io.EOF: + return nil + case err != nil: + return err + case header == nil: + continue + } + + target := filepath.Join(dst, header.Name) + switch header.Typeflag { + case tar.TypeDir: + if _, err := os.Stat(target); err != nil { + if err := os.MkdirAll(target, 0755); err != nil { + return err + } + } + case tar.TypeReg: + f, err := os.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) + if err != nil { + return err + } + defer f.Close() + + if _, err := io.Copy(f, tr); err != nil { + return err + } + } + } +} diff --git a/gps/pkgtree/pkgtree.go b/gps/pkgtree/pkgtree.go index 7938b89083..7d0ab4c3c7 100644 --- a/gps/pkgtree/pkgtree.go +++ b/gps/pkgtree/pkgtree.go @@ -14,7 +14,6 @@ import ( "go/token" "os" "path/filepath" - "reflect" "sort" "strconv" "strings" @@ -607,38 +606,28 @@ func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore *IgnoredRules // This is really only useful as a defensive measure to prevent external state // mutations. func (t PackageTree) Copy() PackageTree { - t2 := PackageTree{ + return PackageTree{ ImportRoot: t.ImportRoot, - Packages: make(map[string]PackageOrErr, len(t.Packages)), + Packages: CopyPackages(t.Packages, nil), } +} +// CopyPackages returns a deep copy of p, optionally modifying the entries with fn. +func CopyPackages(p map[string]PackageOrErr, fn func(string, PackageOrErr) (string, PackageOrErr)) map[string]PackageOrErr { + p2 := make(map[string]PackageOrErr, len(p)) // Walk through and count up the total number of string slice elements we'll // need, then allocate them all at once. strcount := 0 - for _, poe := range t.Packages { + for _, poe := range p { strcount = strcount + len(poe.P.Imports) + len(poe.P.TestImports) } pool := make([]string, strcount) - for path, poe := range t.Packages { + for path, poe := range p { var poe2 PackageOrErr if poe.Err != nil { - refl := reflect.ValueOf(poe.Err) - switch refl.Kind() { - case reflect.Ptr: - poe2.Err = reflect.New(refl.Elem().Type()).Interface().(error) - case reflect.Slice: - err2 := reflect.MakeSlice(refl.Type(), refl.Len(), refl.Len()) - reflect.Copy(err2, refl) - poe2.Err = err2.Interface().(error) - default: - // This shouldn't be too onerous to maintain - the set of errors - // we can get here is restricted by what ListPackages() allows. - // So just panic if one is outside the expected kinds of ptr or - // slice, as that would mean we've missed something notable. - panic(fmt.Sprintf("unrecognized PackgeOrErr error type, %T", poe.Err)) - } + poe2.Err = poe.Err } else { poe2.P = poe.P il, til := len(poe.P.Imports), len(poe.P.TestImports) @@ -651,11 +640,13 @@ func (t PackageTree) Copy() PackageTree { copy(poe2.P.TestImports, poe.P.TestImports) } } - - t2.Packages[path] = poe2 + if fn != nil { + path, poe2 = fn(path, poe2) + } + p2[path] = poe2 } - return t2 + return p2 } // TrimHiddenPackages returns a new PackageTree where packages that are ignored, diff --git a/gps/pkgtree/pkgtree_test.go b/gps/pkgtree/pkgtree_test.go index 16b0ddc712..a2b9a9a2e9 100644 --- a/gps/pkgtree/pkgtree_test.go +++ b/gps/pkgtree/pkgtree_test.go @@ -2271,3 +2271,31 @@ func TestCanaryPackageTreeCopy(t *testing.T) { t.Errorf("PackageTree.Copy is designed to work with an exact set of fields in the Package struct - make sure it (and this test) have been updated!\n\t(GOT):%s\n\t(WNT):%s", packageFields, packageRefl) } } + +func TestPackageTreeCopy(t *testing.T) { + want := PackageTree{ + ImportRoot: "ren", + Packages: map[string]PackageOrErr{ + "ren": { + Err: &build.NoGoError{ + Dir: "some/dir", + }, + }, + "ren/m1p": { + P: Package{ + ImportPath: "ren/m1p", + CommentPath: "", + Name: "m1p", + Imports: []string{ + "github.com/sdboyer/gps", + "sort", + }, + }, + }, + }, + } + got := want.Copy() + if !reflect.DeepEqual(want, got) { + t.Errorf("Did not get expected PackageOrErrs:\n\t(GOT): %+v\n\t(WNT): %+v", got, want) + } +} diff --git a/gps/source.go b/gps/source.go index 54723754d0..2deb699ec6 100644 --- a/gps/source.go +++ b/gps/source.go @@ -5,6 +5,7 @@ package gps import ( + "bytes" "context" "fmt" "log" @@ -21,34 +22,50 @@ import ( type sourceState int32 const ( - sourceIsSetUp sourceState = 1 << iota - sourceExistsUpstream + // sourceExistsUpstream means the chosen source was verified upstream, during this execution. + sourceExistsUpstream sourceState = 1 << iota + // sourceExistsLocally means the repo was retrieved in the past. sourceExistsLocally + // sourceHasLatestVersionList means the version list was refreshed within the cache window. sourceHasLatestVersionList + // sourceHasLatestLocally means the repo was pulled fresh during this execution. sourceHasLatestLocally ) -type srcReturnChans struct { - ret chan *sourceGateway - err chan error +func (state sourceState) String() string { + var b bytes.Buffer + for _, s := range []struct { + sourceState + string + }{ + {sourceExistsUpstream, "sourceExistsUpstream"}, + {sourceExistsLocally, "sourceExistsLocally"}, + {sourceHasLatestVersionList, "sourceHasLatestVersionList"}, + {sourceHasLatestLocally, "sourceHasLatestLocally"}, + } { + if state&s.sourceState > 0 { + if b.Len() > 0 { + b.WriteString("|") + } + b.WriteString(s.string) + } + } + return b.String() } -func (rc srcReturnChans) awaitReturn() (sg *sourceGateway, err error) { - select { - case sg = <-rc.ret: - case err = <-rc.err: - } - return +type srcReturn struct { + *sourceGateway + error } type sourceCoordinator struct { supervisor *supervisor - srcmut sync.RWMutex // guards srcs and nameToURL maps + deducer deducer + srcmut sync.RWMutex // guards srcs and srcIdx srcs map[string]*sourceGateway nameToURL map[string]string psrcmut sync.Mutex // guards protoSrcs map - protoSrcs map[string][]srcReturnChans - deducer deducer + protoSrcs map[string][]chan srcReturn cachedir string logger *log.Logger } @@ -61,7 +78,7 @@ func newSourceCoordinator(superv *supervisor, deducer deducer, cachedir string, logger: logger, srcs: make(map[string]*sourceGateway), nameToURL: make(map[string]string), - protoSrcs: make(map[string][]srcReturnChans), + protoSrcs: make(map[string][]chan srcReturn), } } @@ -107,6 +124,7 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // If the folded name differs from the input name, then there may // already be an entry for it in the nameToURL map, so check again. if url, has := sc.nameToURL[foldedNormalName]; has { + srcGate, has := sc.srcs[url] // There was a match on the canonical folded variant. Upgrade to a // write lock, so that future calls on this name don't need to // burn cycles on folding. @@ -118,8 +136,6 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // words, these operations commute, so we can safely write here // without checking again. sc.nameToURL[normalizedName] = url - - srcGate, has := sc.srcs[url] sc.srcmut.Unlock() if has { return srcGate, nil @@ -135,32 +151,22 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project if chans, has := sc.protoSrcs[foldedNormalName]; has { // Another goroutine is already working on this normalizedName. Fold // in with that work by attaching our return channels to the list. - rc := srcReturnChans{ - ret: make(chan *sourceGateway, 1), - err: make(chan error, 1), - } + rc := make(chan srcReturn, 1) sc.protoSrcs[foldedNormalName] = append(chans, rc) sc.psrcmut.Unlock() - return rc.awaitReturn() + ret := <-rc + return ret.sourceGateway, ret.error } - sc.protoSrcs[foldedNormalName] = []srcReturnChans{} + sc.protoSrcs[foldedNormalName] = []chan srcReturn{} sc.psrcmut.Unlock() doReturn := func(sg *sourceGateway, err error) { + ret := srcReturn{sourceGateway: sg, error: err} sc.psrcmut.Lock() - if sg != nil { - for _, rc := range sc.protoSrcs[foldedNormalName] { - rc.ret <- sg - } - } else if err != nil { - for _, rc := range sc.protoSrcs[foldedNormalName] { - rc.err <- err - } - } else { - panic("sg and err both nil") + for _, rc := range sc.protoSrcs[foldedNormalName] { + rc <- ret } - delete(sc.protoSrcs, foldedNormalName) sc.psrcmut.Unlock() } @@ -178,7 +184,6 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // sources map after the initial unlock, but before this goroutine got // scheduled. Guard against that by checking the main sources map again // and bailing out if we find an entry. - var srcGate *sourceGateway sc.srcmut.RLock() if url, has := sc.nameToURL[foldedNormalName]; has { if srcGate, has := sc.srcs[url]; has { @@ -190,36 +195,40 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project } sc.srcmut.RUnlock() - srcGate = newSourceGateway(pd.mb, sc.supervisor, sc.cachedir) + sc.srcmut.Lock() + defer sc.srcmut.Unlock() - // The normalized name is usually different from the source URL- e.g. - // github.com/sdboyer/gps vs. https://github.com/sdboyer/gps. But it's - // possible to arrive here with a full URL as the normalized name - and both - // paths *must* lead to the same sourceGateway instance in order to ensure - // disk access is correctly managed. - // - // Therefore, we now must query the sourceGateway to get the actual - // sourceURL it's operating on, and ensure it's *also* registered at - // that path in the map. This will cause it to actually initiate the - // maybeSource.try() behavior in order to settle on a URL. - url, err := srcGate.sourceURL(ctx) - if err != nil { - doReturn(nil, err) - return nil, err + // Get or create a sourceGateway. + var srcGate *sourceGateway + var url, unfoldedURL string + var errs errorSlice + for _, m := range pd.mb { + url = m.URL().String() + if notFolded { + // If the normalizedName and foldedNormalName differ, then we're pretty well + // guaranteed that returned URL will also need folding into canonical form. + unfoldedURL = url + url = toFold(url) + } + if sg, has := sc.srcs[url]; has { + srcGate = sg + break + } + src, err := m.try(ctx, sc.cachedir) + if err == nil { + srcGate, err = newSourceGateway(ctx, src, sc.supervisor, sc.cachedir) + if err == nil { + sc.srcs[url] = srcGate + break + } + } + errs = append(errs, err) } - - // If the normalizedName and foldedNormalName differ, then we're pretty well - // guaranteed that returned URL will also need folding into canonical form. - var unfoldedURL string - if notFolded { - unfoldedURL = url - url = toFold(url) + if srcGate == nil { + doReturn(nil, errs) + return nil, errs } - // We know we have a working srcGateway at this point, and need to - // integrate it back into the main map. - sc.srcmut.Lock() - defer sc.srcmut.Unlock() // Record the name -> URL mapping, making sure that we also get the // self-mapping. sc.nameToURL[foldedNormalName] = url @@ -234,13 +243,6 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project sc.nameToURL[unfoldedURL] = url } - if sa, has := sc.srcs[url]; has { - // URL already had an entry in the main map; use that as the result. - doReturn(sa, nil) - return sa, nil - } - - sc.srcs[url] = srcGate doReturn(srcGate, nil) return srcGate, nil } @@ -249,7 +251,6 @@ func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id Project // and caching them as needed. type sourceGateway struct { cachedir string - maybe maybeSource srcState sourceState src source cache singleSourceCache @@ -257,54 +258,63 @@ type sourceGateway struct { suprvsr *supervisor } -func newSourceGateway(maybe maybeSource, superv *supervisor, cachedir string) *sourceGateway { +// newSourceGateway returns a new gateway for src. If the source exists locally, +// the local state may be cleaned, otherwise we ping upstream. +func newSourceGateway(ctx context.Context, src source, superv *supervisor, cachedir string) (*sourceGateway, error) { + var state sourceState + local := src.existsLocally(ctx) + if local { + state |= sourceExistsLocally + if err := superv.do(ctx, src.upstreamURL(), ctValidateLocal, func(ctx context.Context) error { + return src.maybeClean(ctx) + }); err != nil { + return nil, err + } + } + sg := &sourceGateway{ - maybe: maybe, + srcState: state, + src: src, cachedir: cachedir, suprvsr: superv, } sg.cache = sg.createSingleSourceCache() - return sg + if !local { + if err := sg.require(ctx, sourceExistsUpstream); err != nil { + return nil, err + } + } + + return sg, nil } func (sg *sourceGateway) syncLocal(ctx context.Context) error { sg.mu.Lock() - defer sg.mu.Unlock() - - _, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally|sourceHasLatestLocally) + err := sg.require(ctx, sourceExistsLocally|sourceHasLatestLocally) + sg.mu.Unlock() return err } -func (sg *sourceGateway) existsInCache(ctx context.Context) bool { +func (sg *sourceGateway) existsInCache(ctx context.Context) error { sg.mu.Lock() - defer sg.mu.Unlock() - - _, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally) - if err != nil { - return false - } - - return sg.srcState&sourceExistsLocally != 0 + err := sg.require(ctx, sourceExistsLocally) + sg.mu.Unlock() + return err } -func (sg *sourceGateway) existsUpstream(ctx context.Context) bool { +func (sg *sourceGateway) existsUpstream(ctx context.Context) error { sg.mu.Lock() - defer sg.mu.Unlock() - - _, err := sg.require(ctx, sourceIsSetUp|sourceExistsUpstream) - if err != nil { - return false - } - - return sg.srcState&sourceExistsUpstream != 0 + err := sg.require(ctx, sourceExistsUpstream) + sg.mu.Unlock() + return err } func (sg *sourceGateway) exportVersionTo(ctx context.Context, v Version, to string) error { sg.mu.Lock() defer sg.mu.Unlock() - _, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally) + err := sg.require(ctx, sourceExistsLocally) if err != nil { return err } @@ -325,7 +335,7 @@ func (sg *sourceGateway) exportVersionTo(ctx context.Context, v Version, to stri // TODO(sdboyer) It'd be better if we could check the error to see if this // actually was the cause of the problem. if err != nil && sg.srcState&sourceHasLatestLocally == 0 { - if _, err = sg.require(ctx, sourceHasLatestLocally); err == nil { + if err = sg.require(ctx, sourceHasLatestLocally); err == nil { err = sg.suprvsr.do(ctx, sg.src.upstreamURL(), ctExportTree, func(ctx context.Context) error { return sg.src.exportRevisionTo(ctx, r, to) }) @@ -349,7 +359,7 @@ func (sg *sourceGateway) getManifestAndLock(ctx context.Context, pr ProjectRoot, return m, l, nil } - _, err = sg.require(ctx, sourceIsSetUp|sourceExistsLocally) + err = sg.require(ctx, sourceExistsLocally) if err != nil { return nil, nil, err } @@ -369,7 +379,7 @@ func (sg *sourceGateway) getManifestAndLock(ctx context.Context, pr ProjectRoot, if err != nil && sg.srcState&sourceHasLatestLocally == 0 { // TODO(sdboyer) we should warn/log/something in adaptive recovery // situations like this - _, err = sg.require(ctx, sourceHasLatestLocally) + err = sg.require(ctx, sourceHasLatestLocally) if err != nil { return nil, nil, err } @@ -388,8 +398,6 @@ func (sg *sourceGateway) getManifestAndLock(ctx context.Context, pr ProjectRoot, return m, l, nil } -// FIXME ProjectRoot input either needs to parameterize the cache, or be -// incorporated on the fly on egress...? func (sg *sourceGateway) listPackages(ctx context.Context, pr ProjectRoot, v Version) (pkgtree.PackageTree, error) { sg.mu.Lock() defer sg.mu.Unlock() @@ -399,12 +407,12 @@ func (sg *sourceGateway) listPackages(ctx context.Context, pr ProjectRoot, v Ver return pkgtree.PackageTree{}, err } - ptree, has := sg.cache.getPackageTree(r) + ptree, has := sg.cache.getPackageTree(r, pr) if has { return ptree, nil } - _, err = sg.require(ctx, sourceIsSetUp|sourceExistsLocally) + err = sg.require(ctx, sourceExistsLocally) if err != nil { return pkgtree.PackageTree{}, err } @@ -424,7 +432,7 @@ func (sg *sourceGateway) listPackages(ctx context.Context, pr ProjectRoot, v Ver if err != nil && sg.srcState&sourceHasLatestLocally == 0 { // TODO(sdboyer) we should warn/log/something in adaptive recovery // situations like this - _, err = sg.require(ctx, sourceHasLatestLocally) + err = sg.require(ctx, sourceHasLatestLocally) if err != nil { return pkgtree.PackageTree{}, err } @@ -443,6 +451,7 @@ func (sg *sourceGateway) listPackages(ctx context.Context, pr ProjectRoot, v Ver return ptree, nil } +// caller must hold sg.mu. func (sg *sourceGateway) convertToRevision(ctx context.Context, v Version) (Revision, error) { // When looking up by Version, there are four states that may have // differing opinions about version->revision mappings: @@ -470,7 +479,7 @@ func (sg *sourceGateway) convertToRevision(ctx context.Context, v Version) (Revi // The version list is out of date; it's possible this version might // show up after loading it. - _, err := sg.require(ctx, sourceIsSetUp|sourceHasLatestVersionList) + err := sg.require(ctx, sourceHasLatestVersionList) if err != nil { return "", err } @@ -487,10 +496,11 @@ func (sg *sourceGateway) listVersions(ctx context.Context) ([]PairedVersion, err sg.mu.Lock() defer sg.mu.Unlock() - // TODO(sdboyer) The problem here is that sourceExistsUpstream may not be - // sufficient (e.g. bzr, hg), but we don't want to force local b/c git - // doesn't need it - _, err := sg.require(ctx, sourceIsSetUp|sourceExistsUpstream|sourceHasLatestVersionList) + if pvs, ok := sg.cache.getAllVersions(); ok { + return pvs, nil + } + + err := sg.require(ctx, sourceHasLatestVersionList) if err != nil { return nil, err } @@ -504,7 +514,7 @@ func (sg *sourceGateway) revisionPresentIn(ctx context.Context, r Revision) (boo sg.mu.Lock() defer sg.mu.Unlock() - _, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally) + err := sg.require(ctx, sourceExistsLocally) if err != nil { return false, err } @@ -524,7 +534,7 @@ func (sg *sourceGateway) disambiguateRevision(ctx context.Context, r Revision) ( sg.mu.Lock() defer sg.mu.Unlock() - _, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally) + err := sg.require(ctx, sourceExistsLocally) if err != nil { return "", err } @@ -532,18 +542,6 @@ func (sg *sourceGateway) disambiguateRevision(ctx context.Context, r Revision) ( return sg.src.disambiguateRevision(ctx, r) } -func (sg *sourceGateway) sourceURL(ctx context.Context) (string, error) { - sg.mu.Lock() - defer sg.mu.Unlock() - - _, err := sg.require(ctx, sourceIsSetUp) - if err != nil { - return "", err - } - - return sg.src.upstreamURL(), nil -} - // createSingleSourceCache creates a singleSourceCache instance for use by // the encapsulated source. func (sg *sourceGateway) createSingleSourceCache() singleSourceCache { @@ -552,56 +550,84 @@ func (sg *sourceGateway) createSingleSourceCache() singleSourceCache { return newMemoryCache() } -func (sg *sourceGateway) require(ctx context.Context, wanted sourceState) (errState sourceState, err error) { +// sourceExistsUpstream verifies that the source exists upstream and that the +// upstreamURL has not changed and returns any additional sourceState, or an error. +func (sg *sourceGateway) sourceExistsUpstream(ctx context.Context) (sourceState, error) { + if sg.src.existsCallsListVersions() { + return sg.loadLatestVersionList(ctx) + } + err := sg.suprvsr.do(ctx, sg.src.sourceType(), ctSourcePing, func(ctx context.Context) error { + if !sg.src.existsUpstream(ctx) { + return errors.Errorf("source does not exist upstream: %s: %s", sg.src.sourceType(), sg.src.upstreamURL()) + } + return nil + }) + return 0, err +} + +// initLocal initializes the source locally and returns the resulting sourceState. +func (sg *sourceGateway) initLocal(ctx context.Context) (sourceState, error) { + if err := sg.suprvsr.do(ctx, sg.src.sourceType(), ctSourceInit, func(ctx context.Context) error { + err := sg.src.initLocal(ctx) + return errors.Wrapf(err, "failed to fetch source for %s", sg.src.upstreamURL()) + }); err != nil { + return 0, err + } + return sourceExistsUpstream | sourceExistsLocally | sourceHasLatestLocally, nil +} + +// loadLatestVersionList loads the latest version list, possibly ensuring the source +// exists locally first, and returns the resulting sourceState. +func (sg *sourceGateway) loadLatestVersionList(ctx context.Context) (sourceState, error) { + var addlState sourceState + if sg.src.listVersionsRequiresLocal() && !sg.src.existsLocally(ctx) { + as, err := sg.initLocal(ctx) + if err != nil { + return 0, err + } + addlState |= as + } + var pvl []PairedVersion + if err := sg.suprvsr.do(ctx, sg.src.sourceType(), ctListVersions, func(ctx context.Context) error { + var err error + pvl, err = sg.src.listVersions(ctx) + return errors.Wrapf(err, "failed to list versions for %s", sg.src.upstreamURL()) + }); err != nil { + return addlState, err + } + sg.cache.setVersionMap(pvl) + return addlState | sourceHasLatestVersionList, nil +} + +// require ensures the sourceGateway has the wanted sourceState, fetching more +// data if necessary. Returns an error if the state could not be reached. +// caller must hold sg.mu +func (sg *sourceGateway) require(ctx context.Context, wanted sourceState) (err error) { todo := (^sg.srcState) & wanted var flag sourceState = 1 for todo != 0 { if todo&flag != 0 { - // Assign the currently visited bit to errState so that we can - // return easily later. - // - // Also set up addlState so that individual ops can easily attach + // Set up addlState so that individual ops can easily attach // more states that were incidentally satisfied by the op. - errState = flag var addlState sourceState switch flag { - case sourceIsSetUp: - sg.src, addlState, err = sg.maybe.try(ctx, sg.cachedir, sg.cache, sg.suprvsr) case sourceExistsUpstream: - err = sg.suprvsr.do(ctx, sg.src.sourceType(), ctSourcePing, func(ctx context.Context) error { - if !sg.src.existsUpstream(ctx) { - return fmt.Errorf("%s does not exist upstream", sg.src.upstreamURL()) - } - return nil - }) + addlState, err = sg.sourceExistsUpstream(ctx) case sourceExistsLocally: if !sg.src.existsLocally(ctx) { - err = sg.suprvsr.do(ctx, sg.src.sourceType(), ctSourceInit, func(ctx context.Context) error { - return sg.src.initLocal(ctx) - }) - - if err == nil { - addlState |= sourceHasLatestLocally - } else { - err = errors.Wrapf(err, "%s does not exist in the local cache and fetching failed", sg.src.upstreamURL()) - } + addlState, err = sg.initLocal(ctx) } case sourceHasLatestVersionList: - var pvl []PairedVersion - err = sg.suprvsr.do(ctx, sg.src.sourceType(), ctListVersions, func(ctx context.Context) error { - pvl, err = sg.src.listVersions(ctx) - return err - }) - - if err == nil { - sg.cache.setVersionMap(pvl) + if _, ok := sg.cache.getAllVersions(); !ok { + addlState, err = sg.loadLatestVersionList(ctx) } case sourceHasLatestLocally: err = sg.suprvsr.do(ctx, sg.src.sourceType(), ctSourceFetch, func(ctx context.Context) error { return sg.src.updateLocal(ctx) }) + addlState = sourceExistsUpstream | sourceExistsLocally } if err != nil { @@ -616,7 +642,7 @@ func (sg *sourceGateway) require(ctx context.Context, wanted sourceState) (errSt flag <<= 1 } - return 0, nil + return nil } // source is an abstraction around the different underlying types (git, bzr, hg, @@ -628,6 +654,8 @@ type source interface { upstreamURL() string initLocal(context.Context) error updateLocal(context.Context) error + // maybeClean is a no-op when the underlying source does not support cleaning. + maybeClean(context.Context) error listVersions(context.Context) ([]PairedVersion, error) getManifestAndLock(context.Context, ProjectRoot, Revision, ProjectAnalyzer) (Manifest, Lock, error) listPackages(context.Context, ProjectRoot, Revision) (pkgtree.PackageTree, error) @@ -635,4 +663,10 @@ type source interface { disambiguateRevision(context.Context, Revision) (Revision, error) exportRevisionTo(context.Context, Revision, string) error sourceType() string + // existsCallsListVersions returns true if calling existsUpstream actually lists + // versions underneath, meaning listVersions might as well be used instead. + existsCallsListVersions() bool + // listVersionsRequiresLocal returns true if calling listVersions first + // requires the source to exist locally. + listVersionsRequiresLocal() bool } diff --git a/gps/source_cache.go b/gps/source_cache.go index 7b5b566064..11aaefd319 100644 --- a/gps/source_cache.go +++ b/gps/source_cache.go @@ -6,6 +6,8 @@ package gps import ( "fmt" + "path" + "strings" "sync" "github.com/golang/dep/gps/pkgtree" @@ -26,7 +28,7 @@ type singleSourceCache interface { setPackageTree(Revision, pkgtree.PackageTree) // Get the PackageTree for a given revision. - getPackageTree(Revision) (pkgtree.PackageTree, bool) + getPackageTree(Revision, ProjectRoot) (pkgtree.PackageTree, bool) // Indicate to the cache that an individual revision is known to exist. markRevisionExists(r Revision) @@ -61,18 +63,21 @@ type singleSourceCache interface { } type singleSourceCacheMemory struct { - mut sync.RWMutex // protects all fields - infos map[ProjectAnalyzerInfo]map[Revision]projectInfo - ptrees map[Revision]pkgtree.PackageTree - vList []PairedVersion // replaced, never modified - vMap map[UnpairedVersion]Revision - rMap map[Revision][]UnpairedVersion + // Protects all fields. + mut sync.RWMutex + infos map[ProjectAnalyzerInfo]map[Revision]projectInfo + // Replaced, never modified. Imports are *relative* (ImportRoot prefix trimmed). + ptrees map[Revision]map[string]pkgtree.PackageOrErr + // Replaced, never modified. + vList []PairedVersion + vMap map[UnpairedVersion]Revision + rMap map[Revision][]UnpairedVersion } func newMemoryCache() singleSourceCache { return &singleSourceCacheMemory{ infos: make(map[ProjectAnalyzerInfo]map[Revision]projectInfo), - ptrees: make(map[Revision]pkgtree.PackageTree), + ptrees: make(map[Revision]map[string]pkgtree.PackageOrErr), vMap: make(map[UnpairedVersion]Revision), rMap: make(map[Revision][]UnpairedVersion), } @@ -117,8 +122,14 @@ func (c *singleSourceCacheMemory) getManifestAndLock(r Revision, pai ProjectAnal } func (c *singleSourceCacheMemory) setPackageTree(r Revision, ptree pkgtree.PackageTree) { + // Make a copy, with relative import paths. + pkgs := pkgtree.CopyPackages(ptree.Packages, func(ip string, poe pkgtree.PackageOrErr) (string, pkgtree.PackageOrErr) { + poe.P.ImportPath = "" // Don't store this + return strings.TrimPrefix(ip, ptree.ImportRoot), poe + }) + c.mut.Lock() - c.ptrees[r] = ptree + c.ptrees[r] = pkgs // Ensure there's at least an entry in the rMap so that the rMap always has // a complete picture of the revisions we know to exist @@ -128,11 +139,28 @@ func (c *singleSourceCacheMemory) setPackageTree(r Revision, ptree pkgtree.Packa c.mut.Unlock() } -func (c *singleSourceCacheMemory) getPackageTree(r Revision) (pkgtree.PackageTree, bool) { +func (c *singleSourceCacheMemory) getPackageTree(r Revision, pr ProjectRoot) (pkgtree.PackageTree, bool) { c.mut.Lock() - ptree, has := c.ptrees[r] + rptree, has := c.ptrees[r] c.mut.Unlock() - return ptree, has + + if !has { + return pkgtree.PackageTree{}, false + } + + // Return a copy, with full import paths. + pkgs := pkgtree.CopyPackages(rptree, func(rpath string, poe pkgtree.PackageOrErr) (string, pkgtree.PackageOrErr) { + ip := path.Join(string(pr), rpath) + if poe.Err == nil { + poe.P.ImportPath = ip + } + return ip, poe + }) + + return pkgtree.PackageTree{ + ImportRoot: string(pr), + Packages: pkgs, + }, true } func (c *singleSourceCacheMemory) setVersionMap(versionList []PairedVersion) { diff --git a/gps/source_cache_bolt.go b/gps/source_cache_bolt.go index 02d700988f..c9764363e5 100644 --- a/gps/source_cache_bolt.go +++ b/gps/source_cache_bolt.go @@ -8,7 +8,9 @@ import ( "fmt" "log" "os" + "path" "path/filepath" + "strings" "time" "github.com/boltdb/bolt" @@ -54,7 +56,6 @@ func newBoltCache(cd string, epoch int64, logger *log.Logger) (*boltCache, error func (c *boltCache) newSingleSourceCache(pi ProjectIdentifier) singleSourceCache { return &singleSourceCacheBolt{ boltCache: c, - pi: pi, sourceName: []byte(pi.normalizedSource()), } } @@ -101,7 +102,6 @@ func (c *boltCache) close() error { // Values: Unpaired Versions serialized via ConstraintMsg type singleSourceCacheBolt struct { *boltCache - pi ProjectIdentifier sourceName []byte } @@ -199,8 +199,14 @@ func (s *singleSourceCacheBolt) setPackageTree(rev Revision, ptree pkgtree.Packa return err } + root := string(ptree.ImportRoot) for ip, poe := range ptree.Packages { - pb, err := ptrees.CreateBucket([]byte(ip)) + // Stored by relative import path. + rip := strings.TrimPrefix(ip, root) + if rip == "" { + rip = "/" + } + pb, err := ptrees.CreateBucket([]byte(rip)) if err != nil { return err } @@ -216,7 +222,7 @@ func (s *singleSourceCacheBolt) setPackageTree(rev Revision, ptree pkgtree.Packa } } -func (s *singleSourceCacheBolt) getPackageTree(rev Revision) (ptree pkgtree.PackageTree, ok bool) { +func (s *singleSourceCacheBolt) getPackageTree(rev Revision, pr ProjectRoot) (ptree pkgtree.PackageTree, ok bool) { err := s.viewRevBucket(rev, func(b *bolt.Bucket) error { ptrees := b.Bucket(cacheKeyPTree) if ptrees == nil { @@ -224,21 +230,27 @@ func (s *singleSourceCacheBolt) getPackageTree(rev Revision) (ptree pkgtree.Pack } pkgs := make(map[string]pkgtree.PackageOrErr) - err := ptrees.ForEach(func(ip, _ []byte) error { - poe, err := cacheGetPackageOrErr(ptrees.Bucket(ip)) + err := ptrees.ForEach(func(rip, _ []byte) error { + poe, err := cacheGetPackageOrErr(ptrees.Bucket(rip)) if err != nil { return err } + srip := string(rip) + if srip == "/" { + srip = "" + } + // Return full import paths. + ip := path.Join(string(pr), srip) if poe.Err == nil { - poe.P.ImportPath = string(ip) + poe.P.ImportPath = ip } - pkgs[string(ip)] = poe + pkgs[ip] = poe return nil }) if err != nil { return err } - ptree.ImportRoot = string(s.pi.ProjectRoot) + ptree.ImportRoot = string(pr) ptree.Packages = pkgs ok = true return nil diff --git a/gps/source_cache_bolt_encode.go b/gps/source_cache_bolt_encode.go index 2851b24b31..61d062c5cb 100644 --- a/gps/source_cache_bolt_encode.go +++ b/gps/source_cache_bolt_encode.go @@ -334,6 +334,7 @@ func cacheGetLock(b *bolt.Bucket) (*safeLock, error) { } // cachePutPackageOrError stores the pkgtree.PackageOrErr as fields in the bolt.Bucket. +// Package.ImportPath is ignored. func cachePutPackageOrErr(b *bolt.Bucket, poe pkgtree.PackageOrErr) error { if poe.Err != nil { err := b.Put(cacheKeyError, []byte(poe.Err.Error())) diff --git a/gps/source_cache_bolt_test.go b/gps/source_cache_bolt_test.go index 66a5664269..c8ad87e54a 100644 --- a/gps/source_cache_bolt_test.go +++ b/gps/source_cache_bolt_test.go @@ -7,6 +7,7 @@ package gps import ( "io/ioutil" "log" + "path" "testing" "time" @@ -65,9 +66,19 @@ func TestBoltCacheTimeout(t *testing.T) { ptree := pkgtree.PackageTree{ ImportRoot: root, Packages: map[string]pkgtree.PackageOrErr{ - "simple": { + root: { P: pkgtree.Package{ - ImportPath: "simple", + ImportPath: root, + CommentPath: "comment", + Name: "test", + Imports: []string{ + "sort", + }, + }, + }, + path.Join(root, "simple"): { + P: pkgtree.Package{ + ImportPath: path.Join(root, "simple"), CommentPath: "comment", Name: "simple", Imports: []string{ @@ -76,9 +87,9 @@ func TestBoltCacheTimeout(t *testing.T) { }, }, }, - "m1p": { + path.Join(root, "m1p"): { P: pkgtree.Package{ - ImportPath: "m1p", + ImportPath: path.Join(root, "m1p"), CommentPath: "", Name: "m1p", Imports: []string{ @@ -113,7 +124,7 @@ func TestBoltCacheTimeout(t *testing.T) { t.Errorf("lock differences:\n\t %#v", dl) } - got, ok := c.getPackageTree(rev) + got, ok := c.getPackageTree(rev, root) if !ok { t.Errorf("no package tree found:\n\t(WNT): %#v", ptree) } @@ -155,7 +166,7 @@ func TestBoltCacheTimeout(t *testing.T) { t.Errorf("lock differences:\n\t %#v", dl) } - gotPtree, ok := c.getPackageTree(rev) + gotPtree, ok := c.getPackageTree(rev, root) if !ok { t.Errorf("no package tree found:\n\t(WNT): %#v", ptree) } @@ -188,7 +199,7 @@ func TestBoltCacheTimeout(t *testing.T) { t.Errorf("lock differences:\n\t %#v", dl) } - got, ok := c.getPackageTree(rev) + got, ok := c.getPackageTree(rev, root) if !ok { t.Errorf("no package tree found:\n\t(WNT): %#v", ptree) } @@ -231,9 +242,9 @@ func TestBoltCacheTimeout(t *testing.T) { newPtree := pkgtree.PackageTree{ ImportRoot: root, Packages: map[string]pkgtree.PackageOrErr{ - "simple": { + path.Join(root, "simple"): { P: pkgtree.Package{ - ImportPath: "simple", + ImportPath: path.Join(root, "simple"), CommentPath: "newcomment", Name: "simple", Imports: []string{ @@ -242,9 +253,9 @@ func TestBoltCacheTimeout(t *testing.T) { }, }, }, - "m1p": { + path.Join(root, "m1p"): { P: pkgtree.Package{ - ImportPath: "m1p", + ImportPath: path.Join(root, "m1p"), CommentPath: "", Name: "m1p", Imports: []string{ @@ -276,7 +287,7 @@ func TestBoltCacheTimeout(t *testing.T) { t.Errorf("lock differences:\n\t %#v", dl) } - got, ok := c.getPackageTree(rev) + got, ok := c.getPackageTree(rev, root) if !ok { t.Errorf("no package tree found:\n\t(WNT): %#v", newPtree) } diff --git a/gps/source_cache_multi.go b/gps/source_cache_multi.go index e28a2b1cad..dede66e080 100644 --- a/gps/source_cache_multi.go +++ b/gps/source_cache_multi.go @@ -43,13 +43,13 @@ func (c *multiCache) setPackageTree(r Revision, ptree pkgtree.PackageTree) { c.disk.setPackageTree(r, ptree) } -func (c *multiCache) getPackageTree(r Revision) (pkgtree.PackageTree, bool) { - ptree, ok := c.mem.getPackageTree(r) +func (c *multiCache) getPackageTree(r Revision, pr ProjectRoot) (pkgtree.PackageTree, bool) { + ptree, ok := c.mem.getPackageTree(r, pr) if ok { return ptree, true } - ptree, ok = c.disk.getPackageTree(r) + ptree, ok = c.disk.getPackageTree(r, pr) if ok { c.mem.setPackageTree(r, ptree) return ptree, true diff --git a/gps/source_cache_test.go b/gps/source_cache_test.go index 590619da74..4d93c2110c 100644 --- a/gps/source_cache_test.go +++ b/gps/source_cache_test.go @@ -7,6 +7,7 @@ package gps import ( "io/ioutil" "log" + "path" "reflect" "sort" "testing" @@ -190,7 +191,7 @@ func (test singleSourceCacheTest) run(t *testing.T) { const rev Revision = "rev_adsfjkl" - if got, ok := c.getPackageTree(rev); ok { + if got, ok := c.getPackageTree(rev, root); ok { t.Fatalf("unexpected result before setting package tree: %v", got) } @@ -204,9 +205,19 @@ func (test singleSourceCacheTest) run(t *testing.T) { pt := pkgtree.PackageTree{ ImportRoot: root, Packages: map[string]pkgtree.PackageOrErr{ - "simple": { + root: { P: pkgtree.Package{ - ImportPath: "simple", + ImportPath: root, + CommentPath: "comment", + Name: "test", + Imports: []string{ + "sort", + }, + }, + }, + path.Join(root, "simple"): { + P: pkgtree.Package{ + ImportPath: path.Join(root, "simple"), CommentPath: "comment", Name: "simple", Imports: []string{ @@ -215,9 +226,9 @@ func (test singleSourceCacheTest) run(t *testing.T) { }, }, }, - "m1p": { + path.Join(root, "m1p"): { P: pkgtree.Package{ - ImportPath: "m1p", + ImportPath: path.Join(root, "m1p"), CommentPath: "", Name: "m1p", Imports: []string{ @@ -238,7 +249,7 @@ func (test singleSourceCacheTest) run(t *testing.T) { c, close = test.newCache(t, cpath, root) } - got, ok := c.getPackageTree(rev) + got, ok := c.getPackageTree(rev, root) if !ok { t.Errorf("no package tree found:\n\t(WNT): %#v", pt) } @@ -254,7 +265,7 @@ func (test singleSourceCacheTest) run(t *testing.T) { pt = pkgtree.PackageTree{ ImportRoot: root, Packages: map[string]pkgtree.PackageOrErr{ - "test": { + path.Join(root, "test"): { Err: errors.New("error"), }, }, @@ -268,7 +279,7 @@ func (test singleSourceCacheTest) run(t *testing.T) { c, close = test.newCache(t, cpath, root) } - got, ok = c.getPackageTree(rev) + got, ok = c.getPackageTree(rev, root) if !ok { t.Errorf("no package tree found:\n\t(WNT): %#v", pt) } @@ -443,9 +454,9 @@ func comparePackageTree(t *testing.T, want, got pkgtree.PackageTree) { } else { for k, v := range want { if v2, ok := got[k]; !ok { - t.Errorf("key %q: expected %v but got none", k, v) + t.Errorf("key %s: expected %v but got none", k, v) } else if !packageOrErrEqual(v, v2) { - t.Errorf("key %q: expected %v but got %v", k, v, v2) + t.Errorf("key %s: expected %v but got %v", k, v, v2) } } } @@ -549,7 +560,7 @@ func (discardCache) getManifestAndLock(Revision, ProjectAnalyzerInfo) (Manifest, func (discardCache) setPackageTree(Revision, pkgtree.PackageTree) {} -func (discardCache) getPackageTree(Revision) (pkgtree.PackageTree, bool) { +func (discardCache) getPackageTree(Revision, ProjectRoot) (pkgtree.PackageTree, bool) { return pkgtree.PackageTree{}, false } diff --git a/gps/source_manager.go b/gps/source_manager.go index 26683979cc..86a0ec8fc2 100644 --- a/gps/source_manager.go +++ b/gps/source_manager.go @@ -492,7 +492,13 @@ func (sm *SourceMgr) SourceExists(id ProjectIdentifier) (bool, error) { } ctx := context.TODO() - return srcg.existsInCache(ctx) || srcg.existsUpstream(ctx), nil + if err := srcg.existsInCache(ctx); err == nil { + return true, nil + } + if err := srcg.existsUpstream(ctx); err != nil { + return false, err + } + return true, nil } // SyncSourceFor will ensure that all local caches and information about a diff --git a/gps/source_test.go b/gps/source_test.go index f2c86726ab..636cc8cf2a 100644 --- a/gps/source_test.go +++ b/gps/source_test.go @@ -11,7 +11,6 @@ import ( "log" "os" "path/filepath" - "reflect" "testing" "github.com/golang/dep/gps/pkgtree" @@ -42,7 +41,10 @@ func testSourceGateway(t *testing.T) { do := func(wantstate sourceState) func(t *testing.T) { return func(t *testing.T) { superv := newSupervisor(ctx) - sc := newSourceCoordinator(superv, newDeductionCoordinator(superv), cachedir, log.New(test.Writer{TB: t}, "", 0)) + deducer := newDeductionCoordinator(superv) + logger := log.New(test.Writer{TB: t}, "", 0) + sc := newSourceCoordinator(superv, deducer, cachedir, logger) + defer sc.close() id := mkPI("github.com/sdboyer/deptest") sg, err := sc.getSourceGatewayFor(ctx, id) @@ -50,30 +52,33 @@ func testSourceGateway(t *testing.T) { t.Fatal(err) } - if _, ok := sg.src.(*gitSource); !ok { - t.Fatalf("Expected a gitSource, got a %T", sg.src) + if sg.srcState != wantstate { + t.Fatalf("expected state to be %q, got %q", wantstate, sg.srcState) + } + + if err := sg.existsUpstream(ctx); err != nil { + t.Fatalf("failed to verify upstream source: %s", err) } + wantstate |= sourceExistsUpstream + if sg.src.existsCallsListVersions() { + wantstate |= sourceHasLatestVersionList + } if sg.srcState != wantstate { - t.Fatalf("expected state on initial create to be %v, got %v", wantstate, sg.srcState) + t.Fatalf("expected state to be %q, got %q", wantstate, sg.srcState) } if err := sg.syncLocal(ctx); err != nil { t.Fatalf("error on cloning git repo: %s", err) } - cvlist, ok := sg.cache.getAllVersions() - if !ok || len(cvlist) != 4 { - t.Fatalf("repo setup should've cached four versions, got %v: %s", len(cvlist), cvlist) + wantstate |= sourceExistsLocally | sourceHasLatestLocally + if sg.srcState != wantstate { + t.Fatalf("expected state to be %q, got %q", wantstate, sg.srcState) } - wanturl := "https://" + id.normalizedSource() - goturl, err := sg.sourceURL(ctx) - if err != nil { - t.Fatalf("got err from sourceURL: %s", err) - } - if wanturl != goturl { - t.Fatalf("Expected %s as source URL, got %s", wanturl, goturl) + if _, ok := sg.src.(*gitSource); !ok { + t.Fatalf("Expected a gitSource, got a %T", sg.src) } vlist, err := sg.listVersions(ctx) @@ -81,6 +86,11 @@ func testSourceGateway(t *testing.T) { t.Fatalf("Unexpected error getting version pairs from git repo: %s", err) } + wantstate |= sourceHasLatestVersionList + if sg.srcState != wantstate { + t.Fatalf("expected state to be %q, got %q", wantstate, sg.srcState) + } + if len(vlist) != 4 { t.Fatalf("git test repo should've produced four versions, got %v: vlist was %s", len(vlist), vlist) } else { @@ -91,8 +101,14 @@ func testSourceGateway(t *testing.T) { NewVersion("v0.8.0").Pair(Revision("ff2948a2ac8f538c4ecd55962e919d1e13e74baf")), newDefaultBranch("master").Pair(Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f")), } - if !reflect.DeepEqual(vlist, evl) { - t.Fatalf("Version list was not what we expected:\n\t(GOT): %s\n\t(WNT): %s", vlist, evl) + if len(evl) != len(vlist) { + t.Errorf("expected %d versions but got %d", len(evl), len(vlist)) + } else { + for i := range evl { + if !evl[i].identical(vlist[i]) { + t.Errorf("index %d: expected version identical to %#v but got %#v", i, evl[i], vlist[i]) + } + } } } @@ -103,7 +119,7 @@ func testSourceGateway(t *testing.T) { t.Fatal("shouldn't have bare revs in cache without specifically requesting them") } - is, err := sg.revisionPresentIn(ctx, Revision("c575196502940c07bf89fd6d95e83b999162e051")) + is, err := sg.revisionPresentIn(ctx, rev) if err != nil { t.Fatalf("unexpected error while checking revision presence: %s", err) } else if !is { @@ -159,22 +175,17 @@ func testSourceGateway(t *testing.T) { if err != nil { t.Fatalf("unexpected err when getting package tree with known rev: %s", err) } - if !reflect.DeepEqual(wantptree, ptree) { - t.Fatalf("got incorrect PackageTree:\n\t(GOT): %#v\n\t(WNT): %#v", ptree, wantptree) - } + comparePackageTree(t, wantptree, ptree) ptree, err = sg.listPackages(ctx, ProjectRoot("github.com/sdboyer/deptest"), NewVersion("v1.0.0")) if err != nil { t.Fatalf("unexpected err when getting package tree with unpaired good version: %s", err) } - if !reflect.DeepEqual(wantptree, ptree) { - t.Fatalf("got incorrect PackageTree:\n\t(GOT): %#v\n\t(WNT): %#v", ptree, wantptree) - } + comparePackageTree(t, wantptree, ptree) } } - // Run test twice so that we cover both the existing and non-existing case; - // only difference in results is the initial setup state. - t.Run("empty", do(sourceIsSetUp|sourceExistsUpstream|sourceHasLatestVersionList)) - t.Run("exists", do(sourceIsSetUp|sourceExistsLocally|sourceExistsUpstream|sourceHasLatestVersionList)) + // Run test twice so that we cover both the existing and non-existing case. + t.Run("empty", do(sourceExistsUpstream|sourceHasLatestVersionList)) + t.Run("exists", do(sourceExistsLocally)) } diff --git a/gps/vcs_repo.go b/gps/vcs_repo.go index cb500a5a07..0835b35736 100644 --- a/gps/vcs_repo.go +++ b/gps/vcs_repo.go @@ -5,9 +5,9 @@ package gps import ( + "bytes" "context" "encoding/xml" - "fmt" "os" "path/filepath" "runtime" @@ -26,38 +26,11 @@ type ctxRepo interface { //ping(context.Context) (bool, error) } -func newCtxRepo(s vcs.Type, ustr, path string) (ctxRepo, error) { - r, err := getVCSRepo(s, ustr, path) - if err != nil { - // if vcs could not initialize the repo due to a local error - // then the local repo is in an incorrect state. Remove and - // treat it as a new not-yet-cloned repo. - - // TODO(marwan-at-work): warn/give progress of the above comment. - os.RemoveAll(path) - r, err = getVCSRepo(s, ustr, path) - } - - return r, err -} - -func getVCSRepo(s vcs.Type, ustr, path string) (ctxRepo, error) { - switch s { - case vcs.Git: - repo, err := vcs.NewGitRepo(ustr, path) - return &gitRepo{repo}, err - case vcs.Bzr: - repo, err := vcs.NewBzrRepo(ustr, path) - return &bzrRepo{repo}, err - case vcs.Hg: - repo, err := vcs.NewHgRepo(ustr, path) - return &hgRepo{repo}, err - case vcs.Svn: - repo, err := vcs.NewSvnRepo(ustr, path) - return &svnRepo{repo}, err - default: - panic(fmt.Sprintf("Unrecognized format: %v", s)) - } +// ensureCleaner is an optional extension of ctxRepo. +type ensureCleaner interface { + // ensureClean ensures a repository is clean and in working order, + // or returns an error if the adaptive recovery attempts fail. + ensureClean(context.Context) error } // original implementation of these methods come from @@ -186,6 +159,71 @@ func (r *gitRepo) defendAgainstSubmodules(ctx context.Context) error { return nil } +func (r *gitRepo) ensureClean(ctx context.Context) error { + cmd := commandContext( + ctx, + "git", + "status", + "--porcelain", + ) + cmd.SetDir(r.LocalPath()) + + out, err := cmd.CombinedOutput() + if err != nil { + // An error on simple git status indicates some aggressive repository + // corruption, outside of the purview that we can deal with here. + return err + } + + if len(bytes.TrimSpace(out)) == 0 { + // No output from status indicates a clean tree, without any modified or + // untracked files - we're in good shape. + return nil + } + + // We could be more parsimonious about this, but it's probably not worth it + // - it's a rare case to have to do any cleanup anyway, so when we do, we + // might as well just throw the kitchen sink at it. + cmd = commandContext( + ctx, + "git", + "reset", + "--hard", + ) + cmd.SetDir(r.LocalPath()) + _, err = cmd.CombinedOutput() + if err != nil { + return err + } + + // We also need to git clean -df; just reuse defendAgainstSubmodules here, + // even though it's a bit layer-breaky. + err = r.defendAgainstSubmodules(ctx) + if err != nil { + return err + } + + // Check status one last time. If it's still not clean, give up. + cmd = commandContext( + ctx, + "git", + "status", + "--porcelain", + ) + cmd.SetDir(r.LocalPath()) + + out, err = cmd.CombinedOutput() + if err != nil { + return err + } + + if len(bytes.TrimSpace(out)) != 0 { + return errors.Errorf("failed to clean up git repository at %s - dirty? corrupted? status output: \n%s", r.LocalPath(), string(out)) + } + + return nil +} + type bzrRepo struct { *vcs.BzrRepo } diff --git a/gps/vcs_repo_test.go b/gps/vcs_repo_test.go index 2f281f2057..db2a53bebf 100644 --- a/gps/vcs_repo_test.go +++ b/gps/vcs_repo_test.go @@ -5,14 +5,10 @@ package gps import ( - "archive/tar" - "compress/gzip" "context" "errors" - "io" "io/ioutil" "os" - "path/filepath" "testing" "time" @@ -52,73 +48,6 @@ func TestErrs(t *testing.T) { } } -func TestNewCtxRepoHappyPath(t *testing.T) { - t.Parallel() - - tempDir, err := ioutil.TempDir("", "go-ctx-repo-happy-test") - if err != nil { - t.Fatal(err) - } - defer func() { - err = os.RemoveAll(tempDir) - if err != nil { - t.Error(err) - } - }() - - _, err = newCtxRepo(vcs.Git, gitRemoteTestRepo, tempDir) - if err != nil { - t.Fatal(err) - } -} - -func TestNewCtxRepoRecovery(t *testing.T) { - t.Parallel() - - tempDir, err := ioutil.TempDir("", "go-ctx-repo-recovery-test") - if err != nil { - t.Fatal(err) - } - defer func() { - err = os.RemoveAll(tempDir) - if err != nil { - t.Error(err) - } - }() - - cwd, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - - src := filepath.Join(cwd, "_testdata", "badrepo", "corrupt_dot_git_directory.tar") - f, err := os.Open(src) - if err != nil { - t.Fatal(err) - } - defer f.Close() - - dest := filepath.Join(tempDir, ".git") - err = untar(dest, f) - if err != nil { - t.Fatalf("could not untar corrupt repo into temp folder: %v\n", err) - } - - _, err = getVCSRepo(vcs.Git, gitRemoteTestRepo, tempDir) - if err != nil { - if _, ok := err.(*vcs.LocalError); !ok { - t.Fatalf("expected a local error but got: %v\n", err) - } - } else { - t.Fatal("expected getVCSRepo to fail when pointing to a corrupt local path. It is possible that vcs.GitNewRepo updated to gracefully handle this test scenario. Check the return of vcs.GitNewRepo.") - } - - _, err = newCtxRepo(vcs.Git, gitRemoteTestRepo, tempDir) - if err != nil { - t.Fatal(err) - } -} - func testSvnRepo(t *testing.T) { t.Parallel() @@ -417,46 +346,3 @@ func testBzrRepo(t *testing.T) { t.Fatalf("Current failed to detect Bzr on rev 2 of branch. Got version: %s", v) } } - -func untar(dst string, r io.Reader) error { - gzr, err := gzip.NewReader(r) - if err != nil { - return err - } - defer gzr.Close() - - tr := tar.NewReader(gzr) - - for { - header, err := tr.Next() - - switch { - case err == io.EOF: - return nil - case err != nil: - return err - case header == nil: - continue - } - - target := filepath.Join(dst, header.Name) - switch header.Typeflag { - case tar.TypeDir: - if _, err := os.Stat(target); err != nil { - if err := os.MkdirAll(target, 0755); err != nil { - return err - } - } - case tar.TypeReg: - f, err := os.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) - if err != nil { - return err - } - defer f.Close() - - if _, err := io.Copy(f, tr); err != nil { - return err - } - } - } -} diff --git a/gps/vcs_source.go b/gps/vcs_source.go index f6b7aef136..23e5c7c42a 100644 --- a/gps/vcs_source.go +++ b/gps/vcs_source.go @@ -31,9 +31,16 @@ func (bs *baseVCSSource) existsLocally(ctx context.Context) bool { return bs.repo.CheckLocal() } -// TODO reimpl for git func (bs *baseVCSSource) existsUpstream(ctx context.Context) bool { - return !bs.repo.Ping() + return bs.repo.Ping() +} + +func (*baseVCSSource) existsCallsListVersions() bool { + return false +} + +func (*baseVCSSource) listVersionsRequiresLocal() bool { + return false } func (bs *baseVCSSource) upstreamURL() string { @@ -85,8 +92,32 @@ func (bs *baseVCSSource) initLocal(ctx context.Context) error { // source is fully up to date with that of the canonical upstream source. func (bs *baseVCSSource) updateLocal(ctx context.Context) error { err := bs.repo.fetch(ctx) + if err == nil { + return nil + } - if err != nil { + ec, ok := bs.repo.(ensureCleaner) + if !ok { + return err + } + + if err := ec.ensureClean(ctx); err != nil { + return unwrapVcsErr(err) + } + + if err := bs.repo.fetch(ctx); err != nil { + return unwrapVcsErr(err) + } + return nil +} + +func (bs *baseVCSSource) maybeClean(ctx context.Context) error { + ec, ok := bs.repo.(ensureCleaner) + if !ok { + return nil + } + + if err := ec.ensureClean(ctx); err != nil { return unwrapVcsErr(err) } return nil @@ -128,74 +159,6 @@ type gitSource struct { baseVCSSource } -// ensureClean sees to it that a git repository is clean and in working order, -// or returns an error if the adaptive recovery attempts fail. -func (s *gitSource) ensureClean(ctx context.Context) error { - r := s.repo.(*gitRepo) - cmd := commandContext( - ctx, - "git", - "status", - "--porcelain", - ) - cmd.SetDir(r.LocalPath()) - - out, err := cmd.CombinedOutput() - if err != nil { - // An error on simple git status indicates some aggressive repository - // corruption, outside of the purview that we can deal with here. - return err - } - - if len(bytes.TrimSpace(out)) == 0 { - // No output from status indicates a clean tree, without any modified or - // untracked files - we're in good shape. - return nil - } - - // We could be more parsimonious about this, but it's probably not worth it - // - it's a rare case to have to do any cleanup anyway, so when we do, we - // might as well just throw the kitchen sink at it. - cmd = commandContext( - ctx, - "git", - "reset", - "--hard", - ) - cmd.SetDir(r.LocalPath()) - _, err = cmd.CombinedOutput() - if err != nil { - return err - } - - // We also need to git clean -df; just reuse defendAgainstSubmodules here, - // even though it's a bit layer-breaky. - err = r.defendAgainstSubmodules(ctx) - if err != nil { - return err - } - - // Check status one last time. If it's still not clean, give up. - cmd = commandContext( - ctx, - "git", - "status", - "--porcelain", - ) - cmd.SetDir(r.LocalPath()) - - out, err = cmd.CombinedOutput() - if err != nil { - return err - } - - if len(bytes.TrimSpace(out)) != 0 { - return errors.Errorf("failed to clean up git repository at %s - dirty? corrupted? status output: \n%s", r.LocalPath(), string(out)) - } - - return nil -} - func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to string) error { r := s.repo @@ -247,6 +210,10 @@ func (s *gitSource) isValidHash(hash []byte) bool { return gitHashRE.Match(hash) } +func (*gitSource) existsCallsListVersions() bool { + return true +} + func (s *gitSource) listVersions(ctx context.Context) (vlist []PairedVersion, err error) { r := s.repo @@ -495,17 +462,13 @@ func (s *bzrSource) exportRevisionTo(ctx context.Context, rev Revision, to strin return os.RemoveAll(filepath.Join(to, ".bzr")) } +func (s *bzrSource) listVersionsRequiresLocal() bool { + return true +} + func (s *bzrSource) listVersions(ctx context.Context) ([]PairedVersion, error) { r := s.repo - // TODO(sdboyer) this should be handled through the gateway's FSM - if !r.CheckLocal() { - err := s.initLocal(ctx) - if err != nil { - return nil, err - } - } - // Now, list all the tags tagsCmd := commandContext(ctx, "bzr", "tags", "--show-ids", "-v") tagsCmd.SetDir(r.LocalPath()) @@ -572,17 +535,14 @@ func (s *hgSource) exportRevisionTo(ctx context.Context, rev Revision, to string return os.RemoveAll(filepath.Join(to, ".hg")) } +func (s *hgSource) listVersionsRequiresLocal() bool { + return true +} + func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { var vlist []PairedVersion r := s.repo - // TODO(sdboyer) this should be handled through the gateway's FSM - if !r.CheckLocal() { - err := s.initLocal(ctx) - if err != nil { - return nil, err - } - } // Now, list all the tags tagsCmd := commandContext(ctx, "hg", "tags", "--debug", "--verbose") diff --git a/gps/vcs_source_test.go b/gps/vcs_source_test.go index ee39ecba02..c30450f720 100644 --- a/gps/vcs_source_test.go +++ b/gps/vcs_source_test.go @@ -67,17 +67,11 @@ func testGitSourceInteractions(t *testing.T) { } ctx := context.Background() - superv := newSupervisor(ctx) - isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) + isrc, err := mb.try(ctx, cpath) if err != nil { t.Fatalf("Unexpected error while setting up gitSource for test repo: %s", err) } - wantstate := sourceIsSetUp | sourceExistsUpstream | sourceHasLatestVersionList - if state != wantstate { - t.Errorf("Expected return state to be %v, got %v", wantstate, state) - } - err = isrc.initLocal(ctx) if err != nil { t.Fatalf("Error on cloning git repo: %s", err) @@ -119,8 +113,11 @@ func testGitSourceInteractions(t *testing.T) { NewBranch("v1.1").Pair(Revision("f1fbc520489a98306eb28c235204e39fa8a89c84")), NewBranch("v3").Pair(Revision("4a54adf81c75375d26d376459c00d5ff9b703e5e")), } - if !reflect.DeepEqual(vlist, evl) { - t.Errorf("Version list was not what we expected:\n\t(GOT): %s\n\t(WNT): %s", vlist, evl) + for k, e := range evl { + if !vlist[k].identical(e) { + t.Errorf("version list was not what we expected:\n\t(GOT): %#v\n\t(WNT): %#v", vlist, evl) + break + } } } @@ -169,18 +166,12 @@ func testGopkginSourceInteractions(t *testing.T) { } ctx := context.Background() - superv := newSupervisor(ctx) - isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) + isrc, err := mb.try(ctx, cpath) if err != nil { t.Errorf("Unexpected error while setting up gopkginSource for test repo: %s", err) return } - wantstate := sourceIsSetUp | sourceExistsUpstream | sourceHasLatestVersionList - if state != wantstate { - t.Errorf("Expected return state to be %v, got %v", wantstate, state) - } - err = isrc.initLocal(ctx) if err != nil { t.Fatalf("Error on cloning git repo: %s", err) @@ -341,17 +332,11 @@ func testBzrSourceInteractions(t *testing.T) { } ctx := context.Background() - superv := newSupervisor(ctx) - isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) + isrc, err := mb.try(ctx, cpath) if err != nil { t.Fatalf("Unexpected error while setting up bzrSource for test repo: %s", err) } - wantstate := sourceIsSetUp | sourceExistsUpstream - if state != wantstate { - t.Errorf("Expected return state to be %v, got %v", wantstate, state) - } - err = isrc.initLocal(ctx) if err != nil { t.Fatalf("Error on cloning bzr repo: %s", err) @@ -362,9 +347,6 @@ func testBzrSourceInteractions(t *testing.T) { t.Fatalf("Expected a bzrSource, got a %T", isrc) } - if state != wantstate { - t.Errorf("Expected return state to be %v, got %v", wantstate, state) - } if un != src.upstreamURL() { t.Errorf("Expected %s as source URL, got %s", un, src.upstreamURL()) } @@ -452,21 +434,16 @@ func testHgSourceInteractions(t *testing.T) { } ctx := context.Background() - superv := newSupervisor(ctx) - isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) + isrc, err := mb.try(ctx, cpath) if err != nil { t.Errorf("Unexpected error while setting up hgSource for test repo: %s", err) return } - wantstate := sourceIsSetUp | sourceExistsUpstream - if state != wantstate { - t.Errorf("Expected return state to be %v, got %v", wantstate, state) - } - err = isrc.initLocal(ctx) if err != nil { - t.Fatalf("Error on cloning hg repo: %s", err) + t.Errorf("Error on cloning hg repo: %s", err) + return } src, ok := isrc.(*hgSource) @@ -475,9 +452,6 @@ func testHgSourceInteractions(t *testing.T) { return } - if state != wantstate { - t.Errorf("Expected return state to be %v, got %v", wantstate, state) - } if un != src.upstreamURL() { t.Errorf("Expected %s as source URL, got %s", un, src.upstreamURL()) } @@ -500,8 +474,12 @@ func testHgSourceInteractions(t *testing.T) { t.Errorf("hg test repo should've produced %v versions, got %v", len(evl), len(vlist)) } else { SortForUpgrade(vlist) - if !reflect.DeepEqual(vlist, evl) { - t.Errorf("Version list was not what we expected:\n\t(GOT): %s\n\t(WNT): %s", vlist, evl) + + for k, e := range evl { + if !vlist[k].identical(e) { + t.Errorf("version list was not what we expected:\n\t(GOT): %#v\n\t(WNT): %#v", vlist, evl) + break + } } } @@ -516,8 +494,11 @@ func testHgSourceInteractions(t *testing.T) { t.Errorf("hg test repo should've produced %v versions, got %v", len(evl), len(vlist)) } else { SortForUpgrade(vlist) - if !reflect.DeepEqual(vlist, evl) { - t.Errorf("Version list was not what we expected:\n\t(GOT): %s\n\t(WNT): %s", vlist, evl) + for k, e := range evl { + if !vlist[k].identical(e) { + t.Errorf("version list was not what we expected:\n\t(GOT): %#v\n\t(WNT): %#v", vlist, evl) + break + } } } @@ -586,8 +567,7 @@ func TestGitSourceListVersionsNoHEAD(t *testing.T) { mb := maybeGitSource{u} ctx := context.Background() - superv := newSupervisor(ctx) - isrc, _, err := mb.try(ctx, cpath, newMemoryCache(), superv) + isrc, err := mb.try(ctx, cpath) if err != nil { t.Fatalf("Unexpected error while setting up gitSource for test repo: %s", err) } @@ -643,17 +623,11 @@ func TestGitSourceListVersionsNoDupes(t *testing.T) { } ctx := context.Background() - superv := newSupervisor(ctx) - src, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) + src, err := mb.try(ctx, cpath) if err != nil { t.Fatalf("Unexpected error while setting up gitSource for test repo: %s", err) } - wantstate := sourceIsSetUp | sourceExistsUpstream | sourceHasLatestVersionList - if state != wantstate { - t.Errorf("Expected return state to be %v, got %v", wantstate, state) - } - err = src.initLocal(ctx) if err != nil { t.Fatalf("Error on cloning git repo: %s", err) @@ -804,8 +778,7 @@ func Test_bzrSource_exportRevisionTo_removeVcsFiles(t *testing.T) { mb := maybeBzrSource{u} ctx := context.Background() - superv := newSupervisor(ctx) - isrc, _, err := mb.try(ctx, cpath, newMemoryCache(), superv) + isrc, err := mb.try(ctx, cpath) if err != nil { t.Fatalf("unexpected error while setting up hgSource for test repo: %s", err) } @@ -858,8 +831,7 @@ func Test_hgSource_exportRevisionTo_removeVcsFiles(t *testing.T) { mb := maybeHgSource{u} ctx := context.Background() - superv := newSupervisor(ctx) - isrc, _, err := mb.try(ctx, cpath, newMemoryCache(), superv) + isrc, err := mb.try(ctx, cpath) if err != nil { t.Fatalf("unexpected error while setting up hgSource for test repo: %s", err) }