Respect git's ignore rules pervasively in pub.

Respect git's ignore rules pervasively in pub.

In particular, this makes barback and the "pub lish" validators use gitignore
rules.

R=rnystrom@google.com
BUG=12102, 11198
Committed: https://code.google.com/p/dart/source/detail?r=37469

[Bob Nystrom, 2014-06-17 21:48:57]

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart (right):

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:507: /// but slower
[listDir].
You can remove this paragraph now.

Just as a sanity check, did you test and make sure this doesn't regress perf?
Running it on a package with lots of files is a good way to validate.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:109: /// The basenames of files that
are included in [list] despite being hidden
"."

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:110: final _WHITELISTED_FILES = const
['.htaccess'];
Add a test for this.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:121: List<String> list({String
beneath, bool includeDirectories: false}) {
"list" is a bit ambiguous in the context of a package. How about "listFiles"?

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:125: if (git.isInstalled &&
dirExists(path.join(dir, '.git'))) {
Do we care about the case where a parent directory is a git repo?

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:134: relativeBeneath],
Having git stuff in package.dart feels weird. How about making a listFiles()
function in git.dart that encapsulates this?

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:141: files = listDir(beneath,
recursive: true);
Definitely verify this doesn't regress perf. At some point in time, I think
before listDir() itself was changed to use a recursive list from dart:io, this
was *way* slower because it traversed into packages/ symlinks.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:155: return
!path.split(relative).any(_BLACKLISTED_DIRS.contains);
You should do this check before the whitelist check. Otherwise:

foo/packages/bar/.htaccess

will get included, which we don't want.

[nweiz, 2014-06-18 19:18:15]

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart (right):

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:507: /// but slower
[listDir].
On 2014/06/17 21:48:56, Bob Nystrom wrote:
> You can remove this paragraph now.

Done.

> Just as a sanity check, did you test and make sure this doesn't regress perf?
> Running it on a package with lots of files is a good way to validate.

A very good question. It turned out this was a lot slower, even for non-Git
packages. This led me to poke around and figure out why. It turns out that the
big difference between using [listDir] versus a raw [Directory.list] call was
path manipulation. I changed uses of path in [listDir], [Package.list], and
[_listDirectorySources] to use manual string manipulation instead and got a
dramatic speed boost.

On bleeding edge, it currently takes about 3.2s to list 50k files. With my new
changes, it takes 1s without git and 1.3s with git, which I think is pretty
good.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:109: /// The basenames of files that
are included in [list] despite being hidden
On 2014/06/17 21:48:56, Bob Nystrom wrote:
> "."

Done.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:110: final _WHITELISTED_FILES = const
['.htaccess'];
On 2014/06/17 21:48:56, Bob Nystrom wrote:
> Add a test for this.

Done.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:121: List<String> list({String
beneath, bool includeDirectories: false}) {
On 2014/06/17 21:48:57, Bob Nystrom wrote:
> "list" is a bit ambiguous in the context of a package. How about "listFiles"?

Done.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:125: if (git.isInstalled &&
dirExists(path.join(dir, '.git'))) {
On 2014/06/17 21:48:56, Bob Nystrom wrote:
> Do we care about the case where a parent directory is a git repo?

We haven't historically.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:134: relativeBeneath],
On 2014/06/17 21:48:56, Bob Nystrom wrote:
> Having git stuff in package.dart feels weird. How about making a listFiles()
> function in git.dart that encapsulates this?

We don't do that for other git commands. It seems weird to me to do it just for
this one.

In general, I don't like the idea of having to modify git.dart every time a new
git command is invoked.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:155: return
!path.split(relative).any(_BLACKLISTED_DIRS.contains);
On 2014/06/17 21:48:57, Bob Nystrom wrote:
> You should do this check before the whitelist check. Otherwise:
> 
> foo/packages/bar/.htaccess
> 
> will get included, which we don't want.

Done.

[Bob Nystrom, 2014-06-18 20:40:27]

I'm a little iffy about the git stuff in package, but LGTM either way.

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:134: relativeBeneath],
On 2014/06/18 19:18:15, nweiz wrote:
> On 2014/06/17 21:48:56, Bob Nystrom wrote:
> > Having git stuff in package.dart feels weird. How about making a listFiles()
> > function in git.dart that encapsulates this?
> 
> We don't do that for other git commands. It seems weird to me to do it just
for
> this one.

The other git commands are invoked from libraries that are clearly coupled to
git, like the git source. Up until now Package was pretty high-level and
decoupled from stuff like the OS and git. Aside from getting the readme path, it
didn't touch the file system at all. Stuff a full git command in here feels out
of place.

I think it's OK, since it is reasonable for package to knows its files, but I
figured at least encapsulating the git call behind a single method would make it
a little less coupled.

> In general, I don't like the idea of having to modify git.dart every time a
new
> git command is invoked.

Sure, in general we won't. Most other places that use git.dart are already
deeply tied to git.

[nweiz, 2014-06-18 22:45:19]

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/342553002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:134: relativeBeneath],
On 2014/06/18 20:40:27, Bob Nystrom wrote:
> On 2014/06/18 19:18:15, nweiz wrote:
> > On 2014/06/17 21:48:56, Bob Nystrom wrote:
> > > Having git stuff in package.dart feels weird. How about making a
listFiles()
> > > function in git.dart that encapsulates this?
> > 
> > We don't do that for other git commands. It seems weird to me to do it just
> for
> > this one.
> 
> The other git commands are invoked from libraries that are clearly coupled to
> git, like the git source. Up until now Package was pretty high-level and
> decoupled from stuff like the OS and git. Aside from getting the readme path,
it
> didn't touch the file system at all. Stuff a full git command in here feels
out
> of place.
> 
> I think it's OK, since it is reasonable for package to knows its files, but I
> figured at least encapsulating the git call behind a single method would make
it
> a little less coupled.

I don't completely understand what you mean by "encapsulated" and "coupled"
here. It's not like we can provide an interface for listing files with git that
makes sense outside of the context of a package; moving the git part of this
logic elsewhere just means there are more places we need to change if we want to
change the behavior of this function.

> > In general, I don't like the idea of having to modify git.dart every time a
> new
> > git command is invoked.
> 
> Sure, in general we won't. Most other places that use git.dart are already
> deeply tied to git.

[nweiz, 2014-06-18 22:48:15]

Committed patchset #2 manually as r37469 (presubmit successful).