[MERGE][bug #6700] diff on branches without working trees
Ian Clatworthy
ian.clatworthy at internode.on.net
Wed Dec 12 07:47:34 GMT 2007
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
More information about the bazaar
mailing list