Revise based on feedback from previous patch.

utils/pub/solver/backtracking_solver.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /// A back-tracking depth-first solver. Attempts to find the best solution for
 /// a root package's transitive dependency graph, where a "solution" is a set
 /// of concrete package versions. A valid solution will select concrete
 /// versions for every package reached from the root package's dependency graph,
 /// and each of those packages will fit the version constraints placed on it.
 ///
@@ skipping 157 matching lines @@
         }
       }
 
       // Advance past the current version of the leaf-most package.
       var previous = _selected.last.removeFirst();
       if (!_selected.last.isEmpty) {
         logSolve();
         return true;
       }
 
-      logSolve('${previous} is last version, backtracking');
+      logSolve('$previous is last version, backtracking');
 
       // That package has no more versions, so pop it and try the next one.
       _selected.removeLast();
     }
 
     return false;
   }
 
   /// Determines if [depender] has a direct or indirect dependency on
   /// [dependent] based on the currently selected versions of all packages.
   /// Returns a string describing the dependency chain if it does, or `null` if
   /// there is no dependency.
   String _getDependencyPath(PackageId depender, String dependent) {
+    // TODO(rnystrom): This is O(n^2) where n is the number of selected
+    // packages. Could store the reverse dependency graph to address that. If
+    // we do that, we need to make sure it gets correctly rolled back when
+    // backtracking occurs.
     var visited = new Set<String>();
-    var resultPath;
-    var currentPath = '${depender.name}';
+    var currentPath = depender.name;

[nweiz, 2013/04/17 00:59:55]

I think my comment on this may have been swallowed last time, but it's confusing
that you're modifying this variable in [walkDeps]. It would be a lot cleaner to
just pass in a `String path` parameter.

[Bob Nystrom, 2013/04/17 17:11:03]

Oh, now I see. I read that earlier comment as just being about the other
resultPath variable. You're exactly right that your suggestion is cleaner.
Fixed.

 
     walkDeps(PackageId package) {
-      if (visited.contains(package.name)) return false;
+      if (visited.contains(package.name)) return null;
       visited.add(package.name);
 
       var pubspec = cache.getCachedPubspec(package);
-      if (pubspec == null) return false;
+      if (pubspec == null) return null;
 
       for (var dep in pubspec.dependencies) {
         var previousPath = currentPath;
         currentPath = '$currentPath -> ${dep.name}';
 
-        if (dep.name == dependent) {
-          resultPath = currentPath;
-          return true;
-        }
+        if (dep.name == dependent) return currentPath;
 
         var selected = getSelected(dep.name);
         // Ignore unselected dependencies. We haven't traversed into them yet,
         // so they can't affect backjumping.
         if (selected == null) continue;
 
-        if (walkDeps(selected)) return true;
+        var depPath = walkDeps(selected);
+        if (depPath != null) return depPath;
 
         currentPath = previousPath;
       }
 
-      return false;
+      return null;
     }
 
-    return walkDeps(depender) ? resultPath : null;
+    return walkDeps(depender);
   }
 
   /// Logs [message] in the context of the current selected packages. If
   /// [message] is omitted, just logs a description of leaf-most selection.
   void logSolve([String message]) {
     if (message == null) {
       if (_selected.isEmpty) {
         message = "* start at root";
       } else {
-        var versions = _selected.last.map((id) => id.version).toList();
-        if (versions.length > 5) {
-          versions = versions.take(5).join(', ') + '...';
-        } else {
-          versions = versions.join(', ');
-        }
-        message = "* select ${_selected.last.first} (from $versions)";
+        var count = _selected.last.length;
+        message = "* select ${_selected.last.first} ($count versions)";
       }
     } else {
       // Otherwise, indent it under the current selected package.
       message = "| $message";
     }
 
     // Indent for the previous selections.
     var buffer = new StringBuffer();
     buffer.writeAll(_selected.skip(1).map((_) => '| '));
     buffer.write(message);
@@ skipping 68 matching lines @@
           return new Future.value(new Pair<PackageRef, int>(ref, 1));
         }
 
         return _solver.cache.getVersions(ref.name, ref.source, ref.description)
             .then((versions) {
           return new Pair<PackageRef, int>(ref, versions.length);
         }).catchError((error) {
           // If it fails for any reason, just treat that as no versions. This
           // will sort this reference higher so that we can traverse into it
           // and report the error more properly.
+          log.solver("Could not get versions for $ref:\n$error\n\n"
+              "${getAttachedStackTrace(error)}");
           return new Pair<PackageRef, int>(ref, 0);
         });
       }
 
       return Future.wait(refs.map(getNumVersions)).then((pairs) {
         // Future.wait() returns an immutable list, so make a copy.
         pairs = pairs.toList();
 
         // Sort in best-first order to minimize backtracking.
         pairs.sort((a, b) {
@@ skipping 193 matching lines @@
 
     var required = _getRequired(name);
     if (required != null) {
       if (package.source.name != required.ref.source.name) return null;
       if (!package.descriptionEquals(required.ref)) return null;
     }
 
     return package;
   }
 }