Code review comment for lp:~bzr/bzr/faster-ls

Revision history for this message
John A Meinel (jameinel) wrote :

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.

review: Needs Information

« Back to merge proposal