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

[Ian Clatworthy]

Aaron,

Thanks for the review. New patch attached, together with a diff of the
latest changes in case that makes a re-review easier. Some comments
below as well.

Aaron Bentley wrote:

> 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()" ?

OK. I now do that if old_url (and new_url) isn't already set.

> 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

It was there in the original code which is why I retained it. With the
other changes above though, I think it can go.

>> +    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.

I think this statement is required. Consider:

  bzr diff tree/foo --old other_branch

In this case, you only want to diff foo, not ['', foo]. Another
undesirable list for performance reasons is ['', '']. I also feel the
code is cleaner: even if I could always add '', I don't think I'd do it
given None is the "normal" way of saying "all files" (though I
appreciate opinions might vary).

> 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.

Ah. Changed now to use tree.basis_tree instead if tree is not None. Is
that what you expected?

>> +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.

So the code here came from builtins._internal_tree_files() but the
docstring was wrong. I've replaced the docstring with the one from
osutils.relpath (and I've left the code as it was).

I do think conversion to working tree relative paths is generally
required unless both an old branch and a new branch are explicitly
given, either via --old/--new or within the revspecs. I say that because
diff may commonly be run in a directory other that the tree root and the
paths will need to be adjusted accordingly before being passed to the
tree-to-tree diff routine. For example, if I was in bzr.dev/bzrlib and
ran this:

  bzr help.py debug.py --old http://bazaar-vcs.org/bzr/bzr.1.0

then specific_files needs to end up as ['bzrlib/help.py',
'bzrlib/debug.py'].

Ian C.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: better-diff-3.patch
Type: text/x-patch
Size: 28564 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071212/ea418922/attachment-0002.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff-latest-changes.patch
Type: text/x-patch
Size: 4934 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071212/ea418922/attachment-0003.bin