[MERGE][Bug #64783] Fixing check to understand the split-up .bzr format

[Aaron Bentley]

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

bb:tweak

Daniel Mark Watkins wrote:
> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py	2008-04-14 08:03:35 +0000
> +++ bzrlib/bzrdir.py	2008-04-20 05:08:23 +0000
> +    @classmethod
> +    def open_containing_tree_branch_or_repository(klass, location):
> +        """Return the branch, working tree and repo contained by a location.
> +
> +        Returns (tree, branch, repository).
> +        If there is no tree containing the location, tree will be None.
> +        If there is no branch containing the location, branch will be None.
> +        If there is no repository containing the location, repository will be
> +        None.
> +
> +        If no branch, tree or repository is found, a NotVersionedError is
> +        raised.
> +        """
> +        try:
> +            bzrdir = klass.open_containing(location)[0]
> +            tree, branch = bzrdir._get_tree_branch()
> +        except errors.NotBranchError:
> +            try:
> +                repo =
klass.open_containing(location)[0].find_repository()
> +                return None, None, repo
> +            except (errors.NoRepositoryPresent, errors.NotBranchError):
> +                raise errors.NotVersionedError(location)
> +        return tree, branch, branch.repository
> +

You are unnecessarily invoking open_containing twice.

I was suggesting something like this:

bzrdir = klass.open_containing(location)[0]
try:
    tree, branch = bzrdir._get_tree_branch()
except errors.NotBranchError:
    try:
        repo = bzrdir.find_repository()
    except errors.NoRepositoryPresent:
        raise errors.NotBranchError

Also, NotVersionedError is not an appropriate error to raise-- it means
that a particular file is not versioned.

Also, instead of discarding the path from open_containing, you should
return it.  This matches the pattern of other open_containing methods
and is more generally useful.

> + at deprecated_function(one_three)
>  def check(branch, verbose):
>      """Run consistency checks on a branch.
>      
>      Results are reported through logging.
>      
> +    Deprecated in 1.3.  Please use check_branch instead.
> +
> +    :raise BzrCheckError: if there's a consistency error.
> +    """
> +    check_branch(branch, verbose)

^^^ We are now up to version 1.5

> +    @needs_read_lock
> +    def check(self):
> +        # bit hacky, check the tree parent is accurate
> +        tree_basis = self.basis_tree()
> +        tree_basis.lock_read()
> +        try:
> +            repo_basis = self.branch.repository.revision_tree(
> +                self.last_revision())
> +            if len(list(repo_basis.iter_changes(tree_basis))):

^^^ If you're going to take the length of the list, you might as well be
completely explicit and do if len() > 0

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

iD8DBQFIFOW60F+nu1YWqI0RAqfYAJ4lb0P/RGa9Of9lSs0+ITjQKeeePACfc01n
KLzH5TvgiODcaovjW2bAtUA=
=pGZ/
-----END PGP SIGNATURE-----