[PATCH] New "changed:" RevisionSpec

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gioele Barabucci wrote:
> Please find attached a patch that adds a new "changed:" RevisionSpec. It
> selects the last revision where the specified file has been changed.

The concept is good.  The implementation needs a bit of work.

bb:resubmit

The biggest thing is it lacks tests.  All the code we add to the project
needs to be tested, with very, very few exceptions.  You can find some
examples in bzrlib/tests/test_revisionspec.py

This code is really, really slow.  On bzrtools, which is a small project
with a mere 600 revisions, it takes 22 seconds to find out that the most
recent revision modified the NEWS file.

The big problem is find_touching_revisions.  For one thing, it scales
linearly with the number of revisions, because it calls
branch.revision_history.  And the way you're using it, it generates a
delta for every revision in the revision_history.

It really needs to be rewritten so that it scales with the number of
revisions it's going back in history.  I suggest using
branch.repository.iter_reverse_revision_history(branch.last_revision())

The result should be an iterator, so that you can stop once you've found
the nth revision that changed the file.

Also, we've recently added support for as_revision_id, which is all that
most callers need, and is commonly faster to generate.
RevisionSpec_changed should support that directly.

> +    def _match_on(self, branch, revs):
> +        from bzrlib.workingtree import WorkingTree
> +        from bzrlib.log import find_touching_revisions
> +        
> +        loc = self.spec.find(':')
> +        if loc == -1:
> +            offset = 1
> +            filename = self.spec
> +        else:
> +            offset = int(self.spec[:loc])
> +            filename = self.spec[loc+1:]

Personally, I'd do:

split_spec = spec.split(':', 1)
filename = split_spec[-1]
if len(split_spc) == 2:
    offset = int(split_spec[0])
else:
    offset = 1

> +        tree, relpath = WorkingTree.open_containing(filename)

Using the filename to determine the which tree to open is a bit odd.
Revision specs are typically relative to the branch supplied.

I suggest:
    tree = branch.bzrdir.open_workingtree()

Actually, since the branch may not have a tree, I suggest:

    try:
        tree = branch.bzrdir.open_workingtree()
    except (errors.NoWorkingTree, errors.NotLocalUrl):
        tree = branch.basis_tree()

> +        
> +        #if self.spec != '':
> +        #    offset =  int(self.spec)

We never comment-out code.  We have a VCS, so we can always restore it
if we want to.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFH9wWg0F+nu1YWqI0RAt0xAJ9sLOHDyGaYCqwI0b8hBDyAAunogQCeN8xF
+OYyncqBwSnOS/PhhG2bLVg=
=Duow
-----END PGP SIGNATURE-----