Cautionary tale about optimizing path manipulation.

BUG=https://code.google.com/p/dart/issues/detail?id=20628
R=nweiz@google.com

Review URL: https://codereview.chromium.org//485893008

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge@39502 260f80e4-7a28-3924-810f-c04153c831b5

lib/src/barback/asset_environment.dart

@@ -520,7 +520,7 @@ class AssetEnvironment {
   /// For large packages, listing the contents is a performance bottleneck, so
   /// this is optimized for our needs in here instead of using the more general
   /// but slower [listDir].
-  List<AssetId> _listDirectorySources(Package package, String dir) {
+  Iterable<AssetId> _listDirectorySources(Package package, String dir) {
     var subdirectory = path.join(package.dir, dir);
     if (!dirExists(subdirectory)) return [];
 
@@ -529,29 +529,18 @@ class AssetEnvironment {
     // readability than most code in pub. In particular, it avoids using the
     // path package, since re-parsing a path is very expensive relative to
     // string operations.
-    return package.listFiles(beneath: subdirectory).expand((file) {
-      var relative = file.substring(package.dir.length + 1);
-
-      // Skip files that were (most likely) compiled from nearby ".dart"
-      // files. These are created by the Editor's "Run as JavaScript"
-      // command and are written directly into the package's directory.
-      // When pub's dart2js transformer then tries to create the same file
-      // name, we get a build error. To avoid that, just don't consider
-      // that file to be a source.
-      // TODO(rnystrom): Remove these when the Editor no longer generates
-      // .js files and users have had enough time that they no longer have
-      // these files laying around. See #15859.
-      if (relative.endsWith(".dart.js")) return [];
-      if (relative.endsWith(".dart.js.map")) return [];
-      if (relative.endsWith(".dart.precompiled.js")) return [];
+    return package.listFiles(beneath: subdirectory).map((file) {
+      // From profiling, path.relative here is just as fast as a raw substring
+      // and is correct in the case where package.dir has a trailing slash.
+      var relative = path.relative(file, from: package.dir);
 
       if (Platform.operatingSystem == 'windows') {
         relative = relative.replaceAll("\\", "/");
       }
 
       var uri = new Uri(pathSegments: relative.split("/"));
-      return [new AssetId(package.name, uri.toString())];
-    }).toList();
+      return new AssetId(package.name, uri.toString());
+    });
   }
 
   /// Adds a file watcher for [dir] within [package], if the directory exists

lib/src/io.dart

@@ -289,6 +289,16 @@ List<String> listDir(String dir, {bool recursive: false,
     if (entity is Link) return false;
     if (includeHidden) return true;
 
+    // Using substring here is generally problematic in cases where dir has one
+    // or more trailing slashes. If you do listDir("foo"), you'll get back
+    // paths like "foo/bar". If you do listDir("foo/"), you'll get "foo/bar"
+    // (note the trailing slash was dropped. If you do listDir("foo//"), you'll
+    // get "foo//bar".
+    //
+    // This means if you strip off the prefix, the resulting string may have a
+    // leading separator (if the prefix did not have a trailing one) or it may
+    // not. However, since we are only using the results of that to call
+    // contains() on, the leading separator is harmless.
     assert(entity.path.startsWith(dir));
     var pathInDir = entity.path.substring(dir.length);
 

lib/src/package.dart

@@ -172,6 +172,17 @@ class Package {
     }
 
     return files.where((file) {
+      // Using substring here is generally problematic in cases where dir has
+      // one or more trailing slashes. If you do listDir("foo"), you'll get back
+      // paths like "foo/bar". If you do listDir("foo/"), you'll get "foo/bar"
+      // (note the trailing slash was dropped. If you do listDir("foo//"),
+      // you'll get "foo//bar".
+      //
+      // This means if you strip off the prefix, the resulting string may have a
+      // leading separator (if the prefix did not have a trailing one) or it may
+      // not. However, since we are only using the results of that to call
+      // contains() on, the leading separator is harmless.
+      assert(file.startsWith(beneath));
       file = file.substring(beneath.length);
       return !_blacklistedFiles.any(file.endsWith) &&
           !_blacklistedDirs.any(file.contains);

test/build/ignores_existing_compiled_js_files_test.dart

-// Copyright (c) 2013, 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.
-
-import 'package:scheduled_test/scheduled_test.dart';
-
-import '../descriptor.dart' as d;
-import '../test_pub.dart';
-
-main() {
-  initConfig();
-
-  integration("ignores existing JS files that were compiled to Dart", () {
-    // Dart2js can take a long time to compile dart code, so we increase the
-    // timeout to cope with that.
-    currentSchedule.timeout *= 3;
-
-    d.dir(appPath, [
-      d.appPubspec(),
-      d.dir('web', [
-        d.file('file.dart', 'void main() => print("hello");'),
-        d.file('file.dart.js', 'some js code'),
-        d.file('file.dart.js.map', 'source map'),
-        d.file('file.dart.precompiled.js', 'some js code'),
-        d.dir('subdir', [
-          d.file('subfile.dart', 'void main() => print("ping");'),
-          d.file('subfile.dart.js', 'some js code'),
-          d.file('subfile.dart.js.map', 'source map'),
-          d.file('subfile.dart.precompiled.js', 'some js code')
-        ])
-      ])
-    ]).create();
-
-    schedulePub(args: ["build", "--mode", "debug"],
-        output: new RegExp(r'Built \d+ files to "build".'));
-
-    d.dir(appPath, [
-      d.dir('build', [
-        d.dir('web', [
-          d.matcherFile('file.dart.js', isNot(equals('some js code'))),
-          d.matcherFile('file.dart.js.map', isNot(equals('source map'))),
-          d.matcherFile('file.dart.precompiled.js',
-                        isNot(equals('some js code'))),
-          d.dir('subdir', [
-            d.matcherFile('subfile.dart.js', isNot(equals('some js code'))),
-            d.matcherFile('subfile.dart.js.map', isNot(equals('source map'))),
-            d.matcherFile('subfile.dart.precompiled.js',
-                          isNot(equals('some js code')))
-          ])
-        ])
-      ])
-    ]).validate();
-  });
-}