Bazaar

Merge lp:~garyvdm/bzr/get_trees_and_branches_to_diff into lp:bzr

  1. get_trees_and_branches_to_diff
  2. Merge into bzr.dev
Proposed by Gary van der Merwe
Status: Merged
Merged at revision: not available
Proposed branch: lp:~garyvdm/bzr/get_trees_and_branches_to_diff
Merge into: lp:bzr
Diff against target: 146 lines
4 files modified
NEWS (+4/-0)
bzrlib/builtins.py (+5/-3)
bzrlib/diff.py (+4/-2)
bzrlib/tests/test_diff.py (+46/-0)
To merge this branch: bzr merge lp:~garyvdm/bzr/get_trees_and_branches_to_diff
Reviewer Review Type Date Requested Status
Vincent Ladeuil 2nd review Approve
John A Meinel Approve
Review via email: mp+12341@code.launchpad.net

This proposal supersedes a proposal from 2009-09-18.

To post a comment you must log in.
Revision history for this message
Gary van der Merwe (garyvdm) wrote : Posted in a previous version of this proposal

At the moment qdiff uses bzrlib.diff._get_trees_to_diff. I would like for this to be a public api. I would also like for it to return the branches that it opens, so that I can fix Bug 430319.

This patch does the above, rename the relevant method to get_trees_and_branches_to_diff, and adds some basic tests for the method.

Revision history for this message
Gary van der Merwe (garyvdm) wrote :

At the moment qdiff uses bzrlib.diff._get_trees_to_diff. I would like for this to be a public api. I would also like for it to return the branches that it opens, so that I can fix Bug 430319.

This patch does the above, rename the relevant method to get_trees_and_branches_to_diff, and adds some basic tests for the method.

Revision history for this message
John A Meinel (jameinel) wrote :

Seems ok. I was suprised to see that:
            bzrdir.BzrDir.open_containing_tree_or_branch(new_url)
Cannot take a 'possible_transports' parameter, which means that doing:

  bzr diff --old bzr+ssh://foo/branch1 --new bzr+ssh://foo/branch2

Will always connect 2 times.

Anyway, it doesn't specifically relate to promoting this to be a public api, or returning the branches that it does have open.

This was really meant to be a DWIM function based on the command line parameters, more than something that qdiff (or more importantly bzr-explorer) would re-use.

However, as you *are* using it, and find it useful, it seems ok to promote it.

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

ping on a second review of this?

Revision history for this message
Vincent Ladeuil (vila) wrote :

I agree with John about bzrdir.BzrDir.open_containing_tree_or_branch(new_url)
 not accepting a possible_transport parameter.

On the other hand, I'm not convinced that we can blindly share connections
for two potentially concurrent operations and, since we don't have the test
infrastructure for such assertions, that's out of scope for that patch.

I'll merge.

review: Approve (2nd review)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-09-24 09:45:23 +0000
3+++ NEWS 2009-09-24 10:30:31 +0000
4@@ -134,6 +134,10 @@
5
6 * ``bzrlib.tests`` now uses ``stopTestRun`` for its ``TestResult``
7 subclasses - the same as python's unittest module. (Robert Collins)
8+
9+* ``diff._get_trees_to_diff`` has been renamed to
10+ ``diff.get_trees_and_branches_to_diff``. It is now a public API, and it
11+ returns the old and new branches. (Gary van der Merwe)
12
13 * ``bzrlib.trace.log_error``, ``error`` and ``info`` have been deprecated.
14 (Martin Pool)
15
16=== modified file 'bzrlib/builtins.py'
17--- bzrlib/builtins.py 2009-09-16 02:52:14 +0000
18+++ bzrlib/builtins.py 2009-09-24 10:30:31 +0000
19@@ -1887,7 +1887,7 @@
20 @display_command
21 def run(self, revision=None, file_list=None, diff_options=None,
22 prefix=None, old=None, new=None, using=None):
23- from bzrlib.diff import _get_trees_to_diff, show_diff_trees
24+ from bzrlib.diff import get_trees_and_branches_to_diff, show_diff_trees
25
26 if (prefix is None) or (prefix == '0'):
27 # diff -p0 format
28@@ -1907,8 +1907,10 @@
29 raise errors.BzrCommandError('bzr diff --revision takes exactly'
30 ' one or two revision specifiers')
31
32- old_tree, new_tree, specific_files, extra_trees = \
33- _get_trees_to_diff(file_list, revision, old, new,
34+ (old_tree, new_tree,
35+ old_branch, new_branch,
36+ specific_files, extra_trees) = \
37+ get_trees_and_branches_to_diff(file_list, revision, old, new,
38 apply_view=True)
39 return show_diff_trees(old_tree, new_tree, sys.stdout,
40 specific_files=specific_files,
41
42=== modified file 'bzrlib/diff.py'
43--- bzrlib/diff.py 2009-07-29 21:35:05 +0000
44+++ bzrlib/diff.py 2009-09-24 10:30:31 +0000
45@@ -277,7 +277,7 @@
46 new_abspath, e)
47
48
49-def _get_trees_to_diff(path_list, revision_specs, old_url, new_url,
50+def get_trees_and_branches_to_diff(path_list, revision_specs, old_url, new_url,
51 apply_view=True):
52 """Get the trees and specific files to diff given a list of paths.
53
54@@ -341,6 +341,7 @@
55 views.check_path_in_view(working_tree, relpath)
56 specific_files.append(relpath)
57 old_tree = _get_tree_to_diff(old_revision_spec, working_tree, branch)
58+ old_branch = branch
59
60 # Get the new location
61 if new_url is None:
62@@ -354,6 +355,7 @@
63 specific_files.append(relpath)
64 new_tree = _get_tree_to_diff(new_revision_spec, working_tree, branch,
65 basis_is_default=working_tree is None)
66+ new_branch = branch
67
68 # Get the specific files (all files is None, no files is [])
69 if make_paths_wt_relative and working_tree is not None:
70@@ -378,7 +380,7 @@
71 extra_trees = None
72 if working_tree is not None and working_tree not in (old_tree, new_tree):
73 extra_trees = (working_tree,)
74- return old_tree, new_tree, specific_files, extra_trees
75+ return old_tree, new_tree, old_branch, new_branch, specific_files, extra_trees
76
77 def _get_tree_to_diff(spec, tree=None, branch=None, basis_is_default=True):
78 if branch is None and tree is not None:
79
80=== modified file 'bzrlib/tests/test_diff.py'
81--- bzrlib/tests/test_diff.py 2009-03-31 00:12:10 +0000
82+++ bzrlib/tests/test_diff.py 2009-09-24 10:30:31 +0000
83@@ -32,6 +32,7 @@
84 external_diff,
85 internal_diff,
86 show_diff_trees,
87+ get_trees_and_branches_to_diff,
88 )
89 from bzrlib.errors import BinaryFile, NoDiff, ExecutableMissing
90 import bzrlib.osutils as osutils
91@@ -41,6 +42,8 @@
92 import bzrlib._patiencediff_py
93 from bzrlib.tests import (Feature, TestCase, TestCaseWithTransport,
94 TestCaseInTempDir, TestSkipped)
95+from bzrlib.revisiontree import RevisionTree
96+from bzrlib.revisionspec import RevisionSpec
97
98
99 class _AttribFeature(Feature):
100@@ -1382,3 +1385,46 @@
101 self.assertTrue(os.path.samefile('tree/newname', new_path))
102 # make sure we can create files with the same parent directories
103 diff_obj._prepare_files('file2-id', 'oldname2', 'newname2')
104+
105+
106+class TestGetTreesAndBranchesToDiff(TestCaseWithTransport):
107+
108+ def test_basic(self):
109+ tree = self.make_branch_and_tree('tree')
110+ (old_tree, new_tree,
111+ old_branch, new_branch,
112+ specific_files, extra_trees) = \
113+ get_trees_and_branches_to_diff(['tree'], None, None, None)
114+
115+ self.assertIsInstance(old_tree, RevisionTree)
116+ #print dir (old_tree)
117+ self.assertEqual(_mod_revision.NULL_REVISION, old_tree.get_revision_id())
118+ self.assertEqual(tree.basedir, new_tree.basedir)
119+ self.assertEqual(tree.branch.base, old_branch.base)
120+ self.assertEqual(tree.branch.base, new_branch.base)
121+ self.assertIs(None, specific_files)
122+ self.assertIs(None, extra_trees)
123+
124+ def test_with_rev_specs(self):
125+ tree = self.make_branch_and_tree('tree')
126+ self.build_tree_contents([('tree/file', 'oldcontent')])
127+ tree.add('file', 'file-id')
128+ tree.commit('old tree', timestamp=0, rev_id="old-id")
129+ self.build_tree_contents([('tree/file', 'newcontent')])
130+ tree.commit('new tree', timestamp=0, rev_id="new-id")
131+
132+ revisions = [RevisionSpec.from_string('1'),
133+ RevisionSpec.from_string('2')]
134+ (old_tree, new_tree,
135+ old_branch, new_branch,
136+ specific_files, extra_trees) = \
137+ get_trees_and_branches_to_diff(['tree'], revisions, None, None)
138+
139+ self.assertIsInstance(old_tree, RevisionTree)
140+ self.assertEqual("old-id", old_tree.get_revision_id())
141+ self.assertIsInstance(new_tree, RevisionTree)
142+ self.assertEqual("new-id", new_tree.get_revision_id())
143+ self.assertEqual(tree.branch.base, old_branch.base)
144+ self.assertEqual(tree.branch.base, new_branch.base)
145+ self.assertIs(None, specific_files)
146+ self.assertEqual(tree.basedir, extra_trees[0].basedir)