Fix case-insensitive absolute listing.

Fix case-insensitive absolute listing.

This goes to show that we shouldn't avoid using path functions in the
name of efficiency. We were using a raw substring to get a relative
path, and it failed for absolute roots because they didn't have an extra
"/" added to the end.

Closes #4

R=rnystrom@google.com

Committed: https://github.com/dart-lang/glob/commit/ec85232125313a6a8d86ff408e9bcce5cb7d68c2

[Bob Nystrom, 2016-03-08 21:40:32]

Can you explain why this is specific to case-insensitive listing if the issue is
around a trailing slash?

https://codereview.chromium.org/1773293002/diff/1/lib/src/list_tree.dart
File lib/src/list_tree.dart (right):

https://codereview.chromium.org/1773293002/diff/1/lib/src/list_tree.dart#newc...
lib/src/list_tree.dart:317: .where((entity) => _matches(p.relative(entity.path,
from: dir)));
What's the performance impact of this?

It might be worth adding a benchmark and seeing if this noticeably regresses it.

https://codereview.chromium.org/1773293002/diff/1/test/glob_test.dart
File test/glob_test.dart (right):

https://codereview.chromium.org/1773293002/diff/1/test/glob_test.dart#newcode6
test/glob_test.dart:6: import 'package:path/path.dart' as p;
Not needed?

[nweiz, 2016-03-08 22:01:16]

> Can you explain why this is specific to case-insensitive listing if the issue
is
> around a trailing slash?

In case-insensitive mode, the lister needs to list every directory in the
hierarchy. So if you go to list "/tmp/*", it tries to list "/" to find "tmp" or
"TMP" or whatever. In the old code, it would do "/tmp".substring("/".length +
1), which breaks.

https://codereview.chromium.org/1773293002/diff/1/lib/src/list_tree.dart
File lib/src/list_tree.dart (right):

https://codereview.chromium.org/1773293002/diff/1/lib/src/list_tree.dart#newc...
lib/src/list_tree.dart:317: .where((entity) => _matches(p.relative(entity.path,
from: dir)));
On 2016/03/08 21:40:31, Bob Nystrom wrote:
> What's the performance impact of this?
> 
> It might be worth adding a benchmark and seeing if this noticeably regresses
it.

I doubt the performance impact is that high—it's hard to imagine that path
munging could overshadow IO, and p.relative has been pretty heavily
optimized—but also, this is just more correct than the previous version. Even if
it turns out to be substantially slower, there's not much we can do with that
information.

I'm also generally not a fan of adding benchmarking infrastructure to stuff that
we don't know to be performance-critical anywhere. It's extra work, and it can
lead to premature optimization (like this substring thing!).

https://codereview.chromium.org/1773293002/diff/1/test/glob_test.dart
File test/glob_test.dart (right):

https://codereview.chromium.org/1773293002/diff/1/test/glob_test.dart#newcode6
test/glob_test.dart:6: import 'package:path/path.dart' as p;
On 2016/03/08 21:40:31, Bob Nystrom wrote:
> Not needed?

It is; this test was crashing before.

[Bob Nystrom, 2016-03-08 22:11:23]

lgtm

[nweiz, 2016-03-08 23:05:54]

Description was changed from

==========
Fix case-insensitive absolute listing.

This goes to show that we shouldn't avoid using path functions in the
name of efficiency. We were using a raw substring to get a relative
path, and it failed for absolute roots because they didn't have an extra
"/" added to the end.

Closes #4

R=rnystrom@google.com
==========

to

==========
Fix case-insensitive absolute listing.

This goes to show that we shouldn't avoid using path functions in the
name of efficiency. We were using a raw substring to get a relative
path, and it failed for absolute roots because they didn't have an extra
"/" added to the end.

Closes #4

R=rnystrom@google.com

Committed:
https://github.com/dart-lang/glob/commit/ec85232125313a6a8d86ff408e9bcce5cb7d...
==========

[nweiz, 2016-03-08 23:05:56]

Committed patchset #1 (id:1) manually as
ec85232125313a6a8d86ff408e9bcce5cb7d68c2 (presubmit successful).