Merge lp:~bzr/bzr/faster-ls into lp:~bzr/bzr/trunk-old

This patch makes ls faster. In particular, ls on a historical revision is faster: ls -r-1 on the OpenOffice trunk drops from 62 seconds to 1.3 seconds. Plain ls on a subdirectory gets faster as well, dropping from 3-4 seconds to around 1 second.

As well as improving performance, I've moved most of the ls logic out of builtins.py to ls.py. New unit tests are needed for this new public API. Before I add those, I'd like a reviewer to confirm:

1. the new API looks right
2. my changes to the blackbox tests are correct.

In the latter case, I believe that 'ls dir --from-root' ought to show just the stuff in dir, not everything (as the current test expects).

I would *like* to see 'ls' work on top of "iter_entries_by_dir()" rather than list_files().

As near as I can tell, the only thing that list_files gives is "kind character" (though perhaps it also does more with unversioned files?)

I'd like to know if you think that is possible, or if it is too different of an api.

I'm not sure about having a file named "ls.py", maybe it's okay. I don't really have a much better name for it.

+# Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Canonical Ltd

I'm pretty sure 'ls.py' is only (C) 2009 :)

+def ls(tree, outf, from_dir=None, recursive=False, kind=None, unknown=False,
+ versioned=False, ignored=False, verbose=False, null=False, show_ids=False,
+ prefix=None, from_root=False):

^- When I see a function with that many parameters, I wonder if it wouldn't be better implemented as some sort of class... Not saying that is true here, but it seems like it might.

I think we should also think about whether it would be possible to add "bzr ls --added" more easily with this change or not. As near as I can tell, it would be ~ the same effort, so at least it isn't worse.

The main difficulty we've had is that "Versioned" is a state of a single tree, but "Added" is a diff between two trees. So you need an api that can give you the difference among 3 trees:
1) basis_tree
2) current working tree (versioned info)
3) filesystem

Since that gives you "Newly Versioned" "Renamed" "Ignored" etc.

As for the blackbox changes... You changed everything to be '--recurse' to '--no-recurse' (since the default changed), but you didn't add back any "--recurse" tests.

Also, is this the correct behavior:
        self.ls_equals('subdir/b\n'
                       , '--from-root')

I'm thinking it probably is, but I'll note that it is a change in behavior. 'bzr ls --from-root' used to be equivalent to 'bzr ls ../../' until you got to the root. We now don't have a way from a subdirectory to say "ls everything" without giving the full path to the root.

While doing "bzr ls --from-root ." would be an easy way to specify the opposite.

Sorry, I hit 'Save' before I was done.

=== modified file 'bzrlib/transform.py'

I really wonder if this should be in "bzrlib/preview_tree.py". Not that you have to do that.

I noticed you didn't add "bzrlib/tests/test_ls.py". Since the code is now refactored, it would be nice if most of the 'blackbox/tests_ls.py' was actually done as whitebox tests. Again, not required, but if you at least added the test file and some very basic tests, it would be a place that could be obviously expanded later.

So overall, I think it is worth landing something like this.

Just a couple of comments.

ls.py doesn't seem to know if its UI layer or logic layer.

I'd personally rather have a clear logic layer - something that takes
the needed parameters and provides an iterator over [semi-]structured
data.

then the command can be
for thing in [dols]:
    self.output.write("%s%s\n" % (thing[0], thing[1]))

This would make this useful for GUI's as well.

I think a good home for this would be tree.py, rather than a new python
module, as it is very tree specific code (in fact, in refactoring terms,
the first parameter 'tree' stands out like a ForeignMethod).

-Rob

> This patch makes ls faster. In particular, ls on a historical revision is
> faster: ls -r-1 on the OpenOffice trunk drops from 62 seconds to 1.3 seconds.
> Plain ls on a subdirectory gets faster as well, dropping from 3-4 seconds to
> around 1 second.

Just a meta-comment: also explaining in the cover letter what you changed to do that will help with reviews. People may have comments only on the approach without reading the patches and also it helps verify the patch does what was intended.

> Just a couple of comments.
>
> ls.py doesn't seem to know if its UI layer or logic layer.
>
> I'd personally rather have a clear logic layer - something that takes
> the needed parameters and provides an iterator over [semi-]structured
> data.
>
> then the command can be
> for thing in [dols]:
> self.output.write("%s%s\n" % (thing[0], thing[1]))
>
> This would make this useful for GUI's as well.
>
> I think a good home for this would be tree.py, rather than a new python
> module, as it is very tree specific code (in fact, in refactoring terms,
> the first parameter 'tree' stands out like a ForeignMethod).

All good points, though the use case for a GUI ls client isn't really there. (The functionality tends to be provided by far more powerful GUIs like qbrowse and Olive). I'll resubmit this in a less ambitious form, just focusing on the logic bug and performance issue for now.