@ariporad does this work for ls('-d')? I noticed you cut out the mention of options.directory, which makes me suspicious this could work.

Also, this breaks some additional tests, so those should be resolved first.

@nfischer: So, we appear to have some inconsistent ls behavior. If your pass in a file/pattern with no matches, should it have an error, or just return an array with nothing in it?

@ariporad I accounted for this in common.expand(). In bash:

$ ls this*file*doesnt*exist.txt
ls: cannot access this*file*doesnt*exist.txt: No such file or directory
# bash passes a literal 'this*file*doesnt*exist.txt' to the ls executable

@nfischer: So if it can't find the file it should error?

That's what ls() should do, yes. That's why I use common.expand() to inside common.wrap(). It will pass non-existing file names to commands (like ls()). Then those commands are expected to handle the non-existing file themselves (in ls()'s case, it should output an error message).

glob.sync() returns an empty array, I think, which probably isn't what you want

@ariporad pinging this to see if there's any work being done to re-add support for -l and -d.

@nfischer: Sorry, I've been sick. I'll work on that ASAP.

@nfischer: Both -l and -d should be back now.

@ariporad I'm going to make some adjustments to this branch and see if I can fix some of the errors we're seeing.

Ok, thanks! If not, I'll try to look into it tomorrow or when I get back.

@ariporad Take a look now. I fixed the bug you had for Windows (you forgot /g on a regex, that's all). Nice PR!

I tested performance and saw that this was very very slow... So I did my best to squeeze out some more performance. This is about 2x faster than your initial revision (and about 2x uglier...). This is still slower (also about 2x...) than the master branch, but it should be a lot cleaner.

So it's a tradeoff. More maintainable and a bit slower, or we can stick with the tried and true method and get faster.

As for the tests, only one of them needed to change (the one you changed for ls('...*.j')). I just changed some others because I realized I could format them a bit nicer, make them more robust (the tests using idx were actually written poorly, my fault), and let them catch more.

I also fixed a but where ls -l resources/ls would return values with names including resources/ls (they should be relative to resources/ls, not the current directory), so that's what's going on there.

@ariporad This should be rebased off master. If you can, see if you find ways to improve performance (or refactor things into a function), since we're still taking a performance hit for the normal use case, which isn't ideal.

@nfischer: I'm going to try something, give me a second.

@nfischer: Actually, let me talk this through with you first.

Personally I vote for slower and more maintainable, it's not like ls is a big bottleneck.

It also might be better to try to hijack fs.stat, so common.wrap looks something like this:

function wrap(cmd, fn, options) {
  return function() {
    var oldStat = fs.stat;
    var statResults = {};
    fs.stat = function (file) {
        if (!statResults[file]) statResults[file] = oldStat.apply(this, arguments);
        return statResults[file];
    }

    // Whatever common.wrap used to do

    fs.stat = oldStat;
  };
}

I bet this would speed things up a bit.

Nope, no it wouldn't.

But I did learn how to benchmark node things.

@ariporad I just pushed another unit test to ls() to make sure everything is working nicely. Assuming everything still passes (it should), does this look good to merge?

LGTM!

Oh, wait, I have to rebase this off master.

LGTM