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
=== modified file 'NEWS'
--- NEWS 2009-09-24 09:45:23 +0000
+++ NEWS 2009-09-24 10:30:31 +0000
@@ -134,6 +134,10 @@
134134
135* ``bzrlib.tests`` now uses ``stopTestRun`` for its ``TestResult``135* ``bzrlib.tests`` now uses ``stopTestRun`` for its ``TestResult``
136 subclasses - the same as python's unittest module. (Robert Collins)136 subclasses - the same as python's unittest module. (Robert Collins)
137
138* ``diff._get_trees_to_diff`` has been renamed to
139 ``diff.get_trees_and_branches_to_diff``. It is now a public API, and it
140 returns the old and new branches. (Gary van der Merwe)
137141
138* ``bzrlib.trace.log_error``, ``error`` and ``info`` have been deprecated.142* ``bzrlib.trace.log_error``, ``error`` and ``info`` have been deprecated.
139 (Martin Pool)143 (Martin Pool)
140144
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2009-09-16 02:52:14 +0000
+++ bzrlib/builtins.py 2009-09-24 10:30:31 +0000
@@ -1887,7 +1887,7 @@
1887 @display_command1887 @display_command
1888 def run(self, revision=None, file_list=None, diff_options=None,1888 def run(self, revision=None, file_list=None, diff_options=None,
1889 prefix=None, old=None, new=None, using=None):1889 prefix=None, old=None, new=None, using=None):
1890 from bzrlib.diff import _get_trees_to_diff, show_diff_trees1890 from bzrlib.diff import get_trees_and_branches_to_diff, show_diff_trees
18911891
1892 if (prefix is None) or (prefix == '0'):1892 if (prefix is None) or (prefix == '0'):
1893 # diff -p0 format1893 # diff -p0 format
@@ -1907,8 +1907,10 @@
1907 raise errors.BzrCommandError('bzr diff --revision takes exactly'1907 raise errors.BzrCommandError('bzr diff --revision takes exactly'
1908 ' one or two revision specifiers')1908 ' one or two revision specifiers')
19091909
1910 old_tree, new_tree, specific_files, extra_trees = \1910 (old_tree, new_tree,
1911 _get_trees_to_diff(file_list, revision, old, new,1911 old_branch, new_branch,
1912 specific_files, extra_trees) = \
1913 get_trees_and_branches_to_diff(file_list, revision, old, new,
1912 apply_view=True)1914 apply_view=True)
1913 return show_diff_trees(old_tree, new_tree, sys.stdout,1915 return show_diff_trees(old_tree, new_tree, sys.stdout,
1914 specific_files=specific_files,1916 specific_files=specific_files,
19151917
=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py 2009-07-29 21:35:05 +0000
+++ bzrlib/diff.py 2009-09-24 10:30:31 +0000
@@ -277,7 +277,7 @@
277 new_abspath, e)277 new_abspath, e)
278278
279279
280def _get_trees_to_diff(path_list, revision_specs, old_url, new_url,280def get_trees_and_branches_to_diff(path_list, revision_specs, old_url, new_url,
281 apply_view=True):281 apply_view=True):
282 """Get the trees and specific files to diff given a list of paths.282 """Get the trees and specific files to diff given a list of paths.
283283
@@ -341,6 +341,7 @@
341 views.check_path_in_view(working_tree, relpath)341 views.check_path_in_view(working_tree, relpath)
342 specific_files.append(relpath)342 specific_files.append(relpath)
343 old_tree = _get_tree_to_diff(old_revision_spec, working_tree, branch)343 old_tree = _get_tree_to_diff(old_revision_spec, working_tree, branch)
344 old_branch = branch
344345
345 # Get the new location346 # Get the new location
346 if new_url is None:347 if new_url is None:
@@ -354,6 +355,7 @@
354 specific_files.append(relpath)355 specific_files.append(relpath)
355 new_tree = _get_tree_to_diff(new_revision_spec, working_tree, branch,356 new_tree = _get_tree_to_diff(new_revision_spec, working_tree, branch,
356 basis_is_default=working_tree is None)357 basis_is_default=working_tree is None)
358 new_branch = branch
357359
358 # Get the specific files (all files is None, no files is [])360 # Get the specific files (all files is None, no files is [])
359 if make_paths_wt_relative and working_tree is not None:361 if make_paths_wt_relative and working_tree is not None:
@@ -378,7 +380,7 @@
378 extra_trees = None380 extra_trees = None
379 if working_tree is not None and working_tree not in (old_tree, new_tree):381 if working_tree is not None and working_tree not in (old_tree, new_tree):
380 extra_trees = (working_tree,)382 extra_trees = (working_tree,)
381 return old_tree, new_tree, specific_files, extra_trees383 return old_tree, new_tree, old_branch, new_branch, specific_files, extra_trees
382384
383def _get_tree_to_diff(spec, tree=None, branch=None, basis_is_default=True):385def _get_tree_to_diff(spec, tree=None, branch=None, basis_is_default=True):
384 if branch is None and tree is not None:386 if branch is None and tree is not None:
385387
=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py 2009-03-31 00:12:10 +0000
+++ bzrlib/tests/test_diff.py 2009-09-24 10:30:31 +0000
@@ -32,6 +32,7 @@
32 external_diff,32 external_diff,
33 internal_diff,33 internal_diff,
34 show_diff_trees,34 show_diff_trees,
35 get_trees_and_branches_to_diff,
35 )36 )
36from bzrlib.errors import BinaryFile, NoDiff, ExecutableMissing37from bzrlib.errors import BinaryFile, NoDiff, ExecutableMissing
37import bzrlib.osutils as osutils38import bzrlib.osutils as osutils
@@ -41,6 +42,8 @@
41import bzrlib._patiencediff_py42import bzrlib._patiencediff_py
42from bzrlib.tests import (Feature, TestCase, TestCaseWithTransport,43from bzrlib.tests import (Feature, TestCase, TestCaseWithTransport,
43 TestCaseInTempDir, TestSkipped)44 TestCaseInTempDir, TestSkipped)
45from bzrlib.revisiontree import RevisionTree
46from bzrlib.revisionspec import RevisionSpec
4447
4548
46class _AttribFeature(Feature):49class _AttribFeature(Feature):
@@ -1382,3 +1385,46 @@
1382 self.assertTrue(os.path.samefile('tree/newname', new_path))1385 self.assertTrue(os.path.samefile('tree/newname', new_path))
1383 # make sure we can create files with the same parent directories1386 # make sure we can create files with the same parent directories
1384 diff_obj._prepare_files('file2-id', 'oldname2', 'newname2')1387 diff_obj._prepare_files('file2-id', 'oldname2', 'newname2')
1388
1389
1390class TestGetTreesAndBranchesToDiff(TestCaseWithTransport):
1391
1392 def test_basic(self):
1393 tree = self.make_branch_and_tree('tree')
1394 (old_tree, new_tree,
1395 old_branch, new_branch,
1396 specific_files, extra_trees) = \
1397 get_trees_and_branches_to_diff(['tree'], None, None, None)
1398
1399 self.assertIsInstance(old_tree, RevisionTree)
1400 #print dir (old_tree)
1401 self.assertEqual(_mod_revision.NULL_REVISION, old_tree.get_revision_id())
1402 self.assertEqual(tree.basedir, new_tree.basedir)
1403 self.assertEqual(tree.branch.base, old_branch.base)
1404 self.assertEqual(tree.branch.base, new_branch.base)
1405 self.assertIs(None, specific_files)
1406 self.assertIs(None, extra_trees)
1407
1408 def test_with_rev_specs(self):
1409 tree = self.make_branch_and_tree('tree')
1410 self.build_tree_contents([('tree/file', 'oldcontent')])
1411 tree.add('file', 'file-id')
1412 tree.commit('old tree', timestamp=0, rev_id="old-id")
1413 self.build_tree_contents([('tree/file', 'newcontent')])
1414 tree.commit('new tree', timestamp=0, rev_id="new-id")
1415
1416 revisions = [RevisionSpec.from_string('1'),
1417 RevisionSpec.from_string('2')]
1418 (old_tree, new_tree,
1419 old_branch, new_branch,
1420 specific_files, extra_trees) = \
1421 get_trees_and_branches_to_diff(['tree'], revisions, None, None)
1422
1423 self.assertIsInstance(old_tree, RevisionTree)
1424 self.assertEqual("old-id", old_tree.get_revision_id())
1425 self.assertIsInstance(new_tree, RevisionTree)
1426 self.assertEqual("new-id", new_tree.get_revision_id())
1427 self.assertEqual(tree.branch.base, old_branch.base)
1428 self.assertEqual(tree.branch.base, new_branch.base)
1429 self.assertIs(None, specific_files)
1430 self.assertEqual(tree.basedir, extra_trees[0].basedir)