[MERGE][bug #6700] diff on branches without working trees

Aaron Bentley aaron.bentley at utoronto.ca
Sun Dec 9 19:58:09 GMT 2007


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

Ian Clatworthy wrote:
> Updated patch attached based on Aaron's feedback. I've introduced two
> new options (--old and --new) that make it more explicit as to what is
> being compared. I think this ought to keep most users happy in that we
> want to expose the functionality now - increased DWIM can come later. It
> also ended up simplifying the code quite a bit.

That seems like a good idea to me.

> +    # Get the old and new revision specs
> +    old_revision_spec = None
> +    new_revision_spec = None
> +    if revision_specs is not None:
> +        if len(revision_specs) > 0:
> +            old_revision_spec = revision_specs[0]
> +        if len(revision_specs) > 1:
> +            new_revision_spec = revision_specs[1]
> +        # If both revision specs include a branch, we can diff them
> +        # without needing to look further for the details

^^^ This doesn't seem to be a true statement.  If one or zero branches
is supplied, extra_trees will include the working tree.  This will
affect the interpretation of file paths.  Why should this behavior
change when there are two branches specified?

Also, should there be any differences in behavior when --new and --old
are supplied, rather than supplying them as part of the revision spec?
I suspect not, so why not just do "old_url =
revision_specs[0].get_branch()" ?

I suggest removing this condition.

> +        if (old_revision_spec is not None and
> +            new_revision_spec is not None and
> +            not old_revision_spec.needs_branch() and
> +            not new_revision_spec.needs_branch()):
> +            old_tree = _get_tree_to_diff(old_revision_spec)
> +            new_tree = _get_tree_to_diff(new_revision_spec)
> +            specific_files = path_list or None
> +            return old_tree, new_tree, specific_files, None

Also, I'm not clear that we should


> +    elif old_url is not None and new_url is not None:
> +        default_location = None  # don't care - not required
> +        other_paths = path_list
> +        check_paths = False
> +    else:
> +        default_location = path_list[0]
> +        other_paths = path_list[1:]
> +        check_paths = True

This also looks like surprising behavior.  Even if there are two URLs
specified, there shouldn't be any harm in having a default location, I
would think.

check_paths seems misnamed, if its only function is to convert paths to
workingtree-relative paths.

> +    if relpath != '':
> +        specific_files.append(relpath)

Is it necessary to avoid appending relpath if blank?  If relpath is
blank, then the user wants a whole-tree diff, and since diff is
recursive, '' will induce a whole-tree diff.


> +    # Get the specific files (all files is None, no files is [])
> +    if check_paths and working_tree is not None:
> +        other_paths = _relative_paths_in_tree(working_tree, other_paths)
> +    specific_files.extend(other_paths)
> +    if len(specific_files) == 0:
> +        specific_files = None
> +
> +    # Get extra trees that ought to be searched for file-ids
> +    extra_trees = None
> +    if new_tree != working_tree and working_tree is not None:

Couldn't old_tree also == working_tree?  Or are you just not worried
about that case?

> +def _get_tree_to_diff(spec, tree=None, branch=None, basis_is_default=True):
> +    if branch is None and tree is not None:
> +        branch = tree.branch
> +    if spec is None or spec.spec is None:
> +        if basis_is_default:
> +            return branch.basis_tree()

Using branch.basis_tree() rather than tree.basis_tree is a behavior
change.  WorkingTree.basis_tree() returns the tree for the
WorkingTree.last_revision(), while branch.basis_tree() returns the tree
for Branch.last_revision().  This will cause very different behavior
when the working tree is out of sync with the branch.

There is also an optimization issue.  If you do tree.basis_tree, you get
a DirStateRevisionTree, which is optimized for comparison against
WorkingTree4.  I believe if you do branch.basis_tree you get a plain
RevisionTree, which is not optimized for comparison against dirstate trees.

> +def _relative_paths_in_tree(tree, paths):
> +    """Get the relative paths within a working tree.
> +
> +    All paths must be existing paths in the working tree.
> +    """

We have never required this before, and I don't think we should start.

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

iD8DBQFHXEjR0F+nu1YWqI0RAuFcAJ9jq34x3NTE024vqtu+6FKzZfNBwQCfbJIZ
ARM4Ac0T4YymTpheZTrze0A=
=nZ6E
-----END PGP SIGNATURE-----



More information about the bazaar mailing list