Rev 5152: (andrew) Read-lock the trees and branches only once in cmd_diff. in file:///home/pqm/archives/thelove/bzr/%2Btrunk/

Canonical.com Patch Queue Manager pqm at pqm.ubuntu.com
Wed Apr 14 05:35:49 BST 2010


At file:///home/pqm/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 5152 [merge]
revision-id: pqm at pqm.ubuntu.com-20100414043547-j4t4napw7duy07if
parent: pqm at pqm.ubuntu.com-20100413091953-ow6ds0g52xn734v5
parent: andrew.bennetts at canonical.com-20100414031837-k7q2eyughmqxln2f
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Wed 2010-04-14 05:35:47 +0100
message:
  (andrew) Read-lock the trees and branches only once in cmd_diff.
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/builtins.py             builtins.py-20050830033751-fc01482b9ca23183
  bzrlib/diff.py                 diff.py-20050309040759-26944fbbf2ebbf36
  bzrlib/tests/test_diff.py      testdiff.py-20050727164403-d1a3496ebb12e339
=== modified file 'NEWS'
--- a/NEWS	2010-04-13 09:19:53 +0000
+++ b/NEWS	2010-04-14 04:35:47 +0000
@@ -51,6 +51,10 @@
   generated by a template and not edited by the user.
   (Robert Collins, #530265)
 
+* ``bzr diff`` read-locks the trees and branches only once, saving about
+  10-20ms on ``bzr diff`` in a bzr.dev tree.
+  (Andrew Bennetts)
+
 * ``bzr missing`` read-locks the branches only once.
   (Andrew Bennetts)
   
@@ -73,6 +77,10 @@
 API Changes
 ***********
 
+* ``bzrlib.diff.get_trees_and_branches_to_diff`` is deprecated.  Use
+  ``get_trees_and_branches_to_diff_locked`` instead.
+  (Andrew Bennetts)
+  
 Internals
 *********
 

=== modified file 'bzrlib/builtins.py'
--- a/bzrlib/builtins.py	2010-04-13 09:19:53 +0000
+++ b/bzrlib/builtins.py	2010-04-14 04:35:47 +0000
@@ -1955,7 +1955,7 @@
     @display_command
     def run(self, revision=None, file_list=None, diff_options=None,
             prefix=None, old=None, new=None, using=None, format=None):
-        from bzrlib.diff import (get_trees_and_branches_to_diff,
+        from bzrlib.diff import (get_trees_and_branches_to_diff_locked,
             show_diff_trees)
 
         if (prefix is None) or (prefix == '0'):
@@ -1982,8 +1982,8 @@
 
         (old_tree, new_tree,
          old_branch, new_branch,
-         specific_files, extra_trees) = get_trees_and_branches_to_diff(
-            file_list, revision, old, new, apply_view=True)
+         specific_files, extra_trees) = get_trees_and_branches_to_diff_locked(
+            file_list, revision, old, new, self.add_cleanup, apply_view=True)
         return show_diff_trees(old_tree, new_tree, sys.stdout,
                                specific_files=specific_files,
                                external_diff_options=diff_options,

=== modified file 'bzrlib/diff.py'
--- a/bzrlib/diff.py	2010-04-05 21:56:39 +0000
+++ b/bzrlib/diff.py	2010-04-13 13:24:12 +0000
@@ -32,6 +32,7 @@
     branch as _mod_branch,
     bzrdir,
     cmdline,
+    cleanup,
     errors,
     osutils,
     patiencediff,
@@ -48,6 +49,7 @@
     )
 from bzrlib.symbol_versioning import (
     deprecated_function,
+    deprecated_in,
     )
 from bzrlib.trace import mutter, note, warning
 
@@ -289,6 +291,7 @@
                         new_abspath, e)
 
 
+ at deprecated_function(deprecated_in((2, 2, 0)))
 def get_trees_and_branches_to_diff(path_list, revision_specs, old_url, new_url,
                                    apply_view=True):
     """Get the trees and specific files to diff given a list of paths.
@@ -313,7 +316,44 @@
     :returns:
         a tuple of (old_tree, new_tree, old_branch, new_branch,
         specific_files, extra_trees) where extra_trees is a sequence of
-        additional trees to search in for file-ids.
+        additional trees to search in for file-ids.  The trees and branches
+        are not locked.
+    """
+    op = cleanup.OperationWithCleanups(get_trees_and_branches_to_diff_locked)
+    return op.run_simple(path_list, revision_specs, old_url, new_url,
+            op.add_cleanup, apply_view=apply_view)
+    
+
+def get_trees_and_branches_to_diff_locked(
+    path_list, revision_specs, old_url, new_url, add_cleanup, apply_view=True):
+    """Get the trees and specific files to diff given a list of paths.
+
+    This method works out the trees to be diff'ed and the files of
+    interest within those trees.
+
+    :param path_list:
+        the list of arguments passed to the diff command
+    :param revision_specs:
+        Zero, one or two RevisionSpecs from the diff command line,
+        saying what revisions to compare.
+    :param old_url:
+        The url of the old branch or tree. If None, the tree to use is
+        taken from the first path, if any, or the current working tree.
+    :param new_url:
+        The url of the new branch or tree. If None, the tree to use is
+        taken from the first path, if any, or the current working tree.
+    :param add_cleanup:
+        a callable like Command.add_cleanup.  get_trees_and_branches_to_diff
+        will register cleanups that must be run to unlock the trees, etc.
+    :param apply_view:
+        if True and a view is set, apply the view or check that the paths
+        are within it
+    :returns:
+        a tuple of (old_tree, new_tree, old_branch, new_branch,
+        specific_files, extra_trees) where extra_trees is a sequence of
+        additional trees to search in for file-ids.  The trees and branches
+        will be read-locked until the cleanups registered via the add_cleanup
+        param are run.
     """
     # Get the old and new revision specs
     old_revision_spec = None
@@ -342,12 +382,21 @@
         default_location = path_list[0]
         other_paths = path_list[1:]
 
+    def lock_tree_or_branch(wt, br):
+        if wt is not None:
+            wt.lock_read()
+            add_cleanup(wt.unlock)
+        elif br is not None:
+            br.lock_read()
+            add_cleanup(br.unlock)
+
     # Get the old location
     specific_files = []
     if old_url is None:
         old_url = default_location
     working_tree, branch, relpath = \
         bzrdir.BzrDir.open_containing_tree_or_branch(old_url)
+    lock_tree_or_branch(working_tree, branch)
     if consider_relpath and relpath != '':
         if working_tree is not None and apply_view:
             views.check_path_in_view(working_tree, relpath)
@@ -361,6 +410,7 @@
     if new_url != old_url:
         working_tree, branch, relpath = \
             bzrdir.BzrDir.open_containing_tree_or_branch(new_url)
+        lock_tree_or_branch(working_tree, branch)
         if consider_relpath and relpath != '':
             if working_tree is not None and apply_view:
                 views.check_path_in_view(working_tree, relpath)

=== modified file 'bzrlib/tests/test_diff.py'
--- a/bzrlib/tests/test_diff.py	2010-02-23 07:43:11 +0000
+++ b/bzrlib/tests/test_diff.py	2010-04-14 03:18:37 +0000
@@ -17,7 +17,6 @@
 import os
 import os.path
 from cStringIO import StringIO
-import errno
 import subprocess
 import sys
 from tempfile import TemporaryFile
@@ -33,6 +32,7 @@
     internal_diff,
     show_diff_trees,
     get_trees_and_branches_to_diff,
+    get_trees_and_branches_to_diff_locked,
     )
 from bzrlib.errors import BinaryFile, NoDiff, ExecutableMissing
 import bzrlib.osutils as osutils
@@ -44,6 +44,7 @@
                           TestCaseInTempDir, TestSkipped)
 from bzrlib.revisiontree import RevisionTree
 from bzrlib.revisionspec import RevisionSpec
+from bzrlib.symbol_versioning import deprecated_in
 
 from bzrlib.tests.test_win32utils import BackslashDirSeparatorFeature
 
@@ -1394,17 +1395,23 @@
         diff_obj._prepare_files('file2-id', 'oldname2', 'newname2')
 
 
-class TestGetTreesAndBranchesToDiff(TestCaseWithTransport):
+class TestGetTreesAndBranchesToDiffLocked(TestCaseWithTransport):
+
+    def call_gtabtd(self, path_list, revision_specs, old_url, new_url):
+        """Call get_trees_and_branches_to_diff_locked.  Overridden by
+        TestGetTreesAndBranchesToDiff.
+        """
+        return get_trees_and_branches_to_diff_locked(
+            path_list, revision_specs, old_url, new_url, self.addCleanup)
 
     def test_basic(self):
         tree = self.make_branch_and_tree('tree')
         (old_tree, new_tree,
          old_branch, new_branch,
-         specific_files, extra_trees) = \
-            get_trees_and_branches_to_diff(['tree'], None, None, None)
+         specific_files, extra_trees) = self.call_gtabtd(
+             ['tree'], None, None, None)
 
         self.assertIsInstance(old_tree, RevisionTree)
-        #print dir (old_tree)
         self.assertEqual(_mod_revision.NULL_REVISION, old_tree.get_revision_id())
         self.assertEqual(tree.basedir, new_tree.basedir)
         self.assertEqual(tree.branch.base, old_branch.base)
@@ -1424,8 +1431,8 @@
                      RevisionSpec.from_string('2')]
         (old_tree, new_tree,
          old_branch, new_branch,
-         specific_files, extra_trees) = \
-            get_trees_and_branches_to_diff(['tree'], revisions, None, None)
+         specific_files, extra_trees) = self.call_gtabtd(
+            ['tree'], revisions, None, None)
 
         self.assertIsInstance(old_tree, RevisionTree)
         self.assertEqual("old-id", old_tree.get_revision_id())
@@ -1435,3 +1442,15 @@
         self.assertEqual(tree.branch.base, new_branch.base)
         self.assertIs(None, specific_files)
         self.assertEqual(tree.basedir, extra_trees[0].basedir)
+
+
+class TestGetTreesAndBranchesToDiff(TestGetTreesAndBranchesToDiffLocked):
+    """Apply the tests for get_trees_and_branches_to_diff_locked to the
+    deprecated get_trees_and_branches_to_diff function.
+    """
+
+    def call_gtabtd(self, path_list, revision_specs, old_url, new_url):
+        return self.applyDeprecated(
+            deprecated_in((2, 2, 0)), get_trees_and_branches_to_diff,
+            path_list, revision_specs, old_url, new_url)
+




More information about the bazaar-commits mailing list