Update cached source before listing packages

[carolynvs]

This addresses #484. It does fix the simple case that I was testing against, which I'm able to reliably repeat locally, though wasn't sure how to make this into an integration test because it relies on git shenanigans.

1. Create go package with a release and push it to github.

   package main
   
   import "fmt"
   
   func main() {
       fmt.Println("I'm a dependency!")
   }

2. Create a go package that imports this dependency and then run dep init. This populates the cache with the dependency.

   package main
   
   import _ "github.com/carolynvs/tmp1"
   
   func main () {}

   [[dependencies]]
   name = "github.com/carolynvs/tmp1"
   version = "2.0.2"

   [[projects]]
     name = "github.com/carolynvs/tmp1"
     packages = ["."]
     revision = "b3739bbc9944d9f4c82676e6b90ed161f0981351"
     version = "2.0.2"

3. Create another release of the dependency and push to github.

4. Edit the lock and manifest to update to the newest release. Note that this commit hash and tag are not in the local cache.

   [[dependencies]]
   name = "github.com/carolynvs/tmp1"
   version = "2.0.4"

   [[projects]]
     name = "github.com/carolynvs/tmp1"
     packages = ["."]
     revision = "5a5948420cfa7f3bdc2c54a9b0d31bfd19e0dede"
     version = "2.0.4"

5. Run dep ensure -v -n to reproduce the following error (with some debugging thrown in):

   $ dep ensure -n -v
   Root project is "foo"
    1 transitively valid internal packages
    1 external packages imported from 1 projects
   (0)   ✓ select (root)
   (1)	? attempt github.com/carolynvs/tmp1 with 1 pkgs; at least 1 versions to try
   (1)	    try github.com/carolynvs/[email protected]
   === DEBUG: checkRequiredPackagesExist===
   === DEBUG: src.listpackages.sg.convertToRevision ===
   === DEBUG: sg.cache.toRevision -> 5a5948420cfa7f3bdc2c54a9b0d31bfd19e0dede ===
   (1)	✗   Unable to update checked out version: fatal: reference is not a tree: 5a5948420cfa7f3bdc2c54a9b0d31bfd19e0dede
   

What makes me pause is that I don't understand why this isn't a more common bug considering my repo steps. If you can help me figure out why dep is working fine for most people without this, I'd really appreciate it!

If I understand correctly what dep is doing, getting a list of packages included in a dependency at a particular revision, and then having the latest source code seems like the correct fix. But again, I don't get why this isn't a problem for more people then? 😀

[sdboyer]

So, first...big relief that this fixes the problem. Stale caches have plagued glide ever since a central cache was introduced, and they were a problem for a while in gps until the most recent refactor. They make for a really nasty, trust-shattering experience, because the problems dep mgmt solves are hard enough to understand without the added possibility of a stale cache confusing everything further.

While I don't know the exact paths that would make this occur without some pretty exhaustive testing (the variety in input states to the solver yield a larger state space than I can hold in my head), I'm pretty sure I know the answer here. Basically, it doesn't happen more often because yes, there's a different path that's often followed: when encountering a dependency, IFF the solver thinks that it might need the whole list of versions for it, it'll call SourceManager.SyncSourceFor() (here and here) - which solves the problem here.

The basic motivation for this approach is a performance optimization - if there's already a locked version for a project, and we have it in local cache already, AND the user didn't request an upgrade for it, then then we might be able to make it through the whole solve run without needing to hit the network. If any of these conditions aren't met, though, then we try to gain a little wall time speedup by initiating the network activity in the background (via SyncSourceFor()), prior to when the solver actually needs the data.

Most of the time, this is a good guess. But not always. If it's not, then the solver will make more targeted calls when it needs additional data, without having already called SyncSourceFor(). And that's where the path with the fix you made comes in - it's an adaptive recovery method, utilized by a number of SourceManager methods when the version argument provided to it isn't found.

I don't know why I neglected to put that last flag on there - probably just an oversight at the time. And because it's quite arduous to test (for all the reasons you've described above), I never wrote a test for this path. But if you read the flags being required in that method, this error should make some sense - we've ensured that we have the latest list of versions from upstream, so the SourceManager thinks that version exists. So, it goes ahead and tries it...and, boom, fatal: reference is not a tree: 5a5948420cfa7f3bdc2c54a9b0d31bfd19e0dede.

Note that this is only possible with git, because in hg and bzr, sourceHasLatestVersionList entails that sourceHasLatestLocally. The latter two have to do a clone/update to get the version list, but git does not, as we can use git ls-remote to retrieve the version list without needing to clone or fetch.

[sdboyer]

I'mma follow up with an issue about how we might devise a testing apparatus for this.

[carolynvs]

Thank you for the details and pointers! ♥️ I was spelunking in gps and had some of that figured out but was missing the alternate code path.

[felipejfc]

@cscatolini fyi