Show detailed report on upgrade.

sdk/lib/_internal/pub/lib/src/entrypoint.dart

Index: sdk/lib/_internal/pub/lib/src/entrypoint.dart
diff --git a/sdk/lib/_internal/pub/lib/src/entrypoint.dart b/sdk/lib/_internal/pub/lib/src/entrypoint.dart
index cf7e5d08caef977e0c17e1494d6878d7a37f7f85..e2e60dc7bed4ba6fdf3a707eca97a08deecc520c 100644
--- a/sdk/lib/_internal/pub/lib/src/entrypoint.dart
+++ b/sdk/lib/_internal/pub/lib/src/entrypoint.dart
@@ -109,60 +109,33 @@ class Entrypoint {
     return future;
   }
 
-  /// Gets all dependencies of the [root] package, respecting the [LockFile]
-  /// if present.
+  /// Gets all dependencies of the [root] package.
   ///
-  /// Returns a [Future] that completes when all dependencies are available.
-  Future getDependencies() {
-    return new Future.sync(() {
-      return resolveVersions(cache.sources, root, lockFile: loadLockFile());
-    }).then(_getDependencies);
-  }
-
-  /// Gets the latest available versions of all dependencies of the [root]
-  /// package, writing a new [LockFile].
+  /// [useLatest], if provided, defines a list of packages that will be
+  /// unlocked and upgraded to their latest versions. If [upgradeAll] is
+  /// true, the previous lockfile is ignored and all packages are re-resolved
+  /// from scratch. Otherwise, it will attempt to preserve the versions of all
+  /// previously locked packages.

[nweiz, 2013/12/11 06:48:32]

It sounds like [useLatest] won't do anything if [upgradeAll] is true. Document
that, and maybe throw an ArgumentError if [useLatest] is non-empty and
[upgradeAll] is true.

Alternately, encode this requirement into the methods themselves by leaving this
as two separate methods.

[Bob Nystrom, 2013/12/11 22:36:59]

They are actually orthogonal. useLatest *forces* the latest version and *only*
the latest version. upgradeAll just unlocks. That was the semantics you
originally implemented for pub upgrade, but I'm not sure I ever understood the
full reasoning there.
That's pretty much what was there before, but since 99% of the body is the same,
they just forwarded to a single _getDependencies() method. That would end up
with the same set of overlapping parameters, so wouldn't solve much.

   ///
-  /// Returns a [Future] that completes when all dependencies are available.
-  Future upgradeAllDependencies() {
-    return resolveVersions(cache.sources, root).then(_getDependencies);
-  }
-
-  /// Gets the latest available versions of [dependencies], while leaving
-  /// other dependencies as specified by the [LockFile] if possible.
+  /// If [useLatest] is non-empty or [upgradeAll] is true, displays a detailed
+  /// report of the changes made relative to the previous lockfile.

[nweiz, 2013/12/11 06:48:32]

Explain that this is a temporary situation. Ideally, reference a bug for getting
detailed reports for get/serve/build as well.

[Bob Nystrom, 2013/12/11 22:36:59]

Added a TODO below by the report code.

   ///
-  /// Returns a [Future] that completes when all dependencies are available.
-  Future upgradeDependencies(List<String> dependencies) {
-    return new Future.sync(() {
-      return resolveVersions(cache.sources, root,
-          lockFile: loadLockFile(), useLatest: dependencies);
-    }).then(_getDependencies);
-  }
+  /// Returns a [Future] that completes to the number of changed dependencies.
+  /// It completes when all dependencies are available.
+  Future<int> getDependencies({List<String> useLatest, bool upgradeAll}) {

[nweiz, 2013/12/11 06:48:32]

I don't think I like [getDependencies] as the name here anymore, since most of
the work it's doing is now upgrading packages and it's no longer specifically
"the method that maps to 'pub get'." It's awkward that "get" means both "install
packages so they're accessible to the app" and "non-destructively get a value
from an object", but this is running into that ambiguity and I think renaming it
would work better.

[Bob Nystrom, 2013/12/11 22:36:59]

Yeah, I'm not crazy about it either. Changed to "acquireDependencies".

I have some half-formed thoughts to refactor this further but I didn't want to
go overboard on this patch.

+    if (upgradeAll == null) upgradeAll = false;

[nweiz, 2013/12/11 06:48:32]

I like using explicit defaults for boolean parameters.

[Bob Nystrom, 2013/12/11 22:36:59]

That breaks forwarding. :(

upgradeAll is passed to resolveVersions which in turn passes it to
BacktrackingSolver. If we used an explicit default along the way, then passing
null along would cancel out the default.

Default params are horribly broken.

[nweiz, 2013/12/11 23:31:27]

I don't think we should expect or support null for named boolean parameters. If
we're forwarding the parameter, I think we should just have the default listed
in each declaration that gets forwarded. It's very risky to change the default
for a boolean parameter anyway, since anyone not passing that parameter may just
be doing so as a shorthand for "param: default", so it's unlikely that the two
parameters will get out-of-sync.

Using an explicit default here, though, avoids weird constructions like "param
== true" and makes it easy for a reader of the code or documentation to see what
the default is.

If you don't add explicit defaults, please be sure to document the default
values everywhere they appear.

[Bob Nystrom, 2013/12/12 00:14:58]

OK, done.

+
+    var numChanged = 0;
 
-  /// Removes the old packages directory, gets all dependencies listed in
-  /// [result], and writes a [LockFile].
-  Future _getDependencies(SolveResult result) {
     return new Future.sync(() {
+      return resolveVersions(cache.sources, root, lockFile: loadLockFile(),
+          useLatest: useLatest, upgradeAll: upgradeAll);
+    }).then((result) {
       if (!result.succeeded) throw result.error;
 
-      // Warn the user if any overrides were in effect.
-      if (result.overrides.isNotEmpty) {
-        var buffer = new StringBuffer();
-        buffer.write("Warning: You are using these overridden dependencies:");
-        for (var override in result.overrides) {
-          var source = cache.sources[override.source];
-          buffer.write("\n- ${override.name}");
-          if (override.constraint != VersionConstraint.any) {
-            buffer.write(" version ${override.constraint}");
-          }
-          if (source != cache.sources.defaultSource) {
-            var description = source.formatDescription(root.dir,
-                override.description);
-            buffer.write(" (from ${override.source} $description)");
-          }
-        }
-        log.warning(buffer);
-      }
+      numChanged = result.showReport(showAll: useLatest != null || upgradeAll);
 
+      // Install the packages.
       cleanDir(packagesDir);
       return Future.wait(result.packages.map((id) {
         if (id.isRoot) return new Future.value(id);
@@ -172,6 +145,8 @@ class Entrypoint {
       _saveLockFile(ids);
       _linkSelf();
       _linkSecondaryPackageDirs();
+
+      return numChanged;
     });
   }