Merge lp:~spiv/bzr/hardlink-2a-408193 into lp:bzr

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This tries to fix bug 408193, --hardlink not working on recent tree formats. I'm not certain this is a correct fix, as I haven't looked at content filtering code before, so I'd love some careful feedback on this approach.

This patch doesn't seem to break any tests, and it doesn't seem to change performance for the worse. For formats that don't support content filtering, nothing changes. For formats that do, this (assuming I've written this correctly) looks at each file to see if any filtering rules apply to it and if so skips hardlinking of it.

In my testing, if you have no rules configured on a 2a tree, on bzr trees it gives the same performance and hardlinks as --hardlink of a 1.9 format tree. (Both cases with 2a branch and repo, just varying tree format.)

In practice, for me, this makes branching 2a format bzr.dev trees speed up from 3s to 1s. Plus of course uses less disk.

This patch almost certainly deserves new tests. I'd appreciate some suggestions about which tests to add, and where. I guess maybe in test_transform?

[Robert Collins (lifeless)]

On Fri, 2009-11-27 at 00:50 +0000, Andrew Bennetts wrote:

> This patch almost certainly deserves new tests. I'd appreciate some suggestions about which tests to add, and where. I guess maybe in test_transform?

That sounds appropriate to me, and the approach you've taken sounds
plausible.

[Martin Pool (mbp)]

news please.

This looks like a plausible fix to me. I probably wouldn't trust it to go into 2.0 though, at least not without a shakedown in trunk.

There is another test relevant to this in test_checkout so you should at least reenable that.

I guess ideally you want a test that shows that things are not hardlinked when the filters cause them to have different content. However, you could make a case that we should optimistically land this and then see if things actually fail that way.

review: Needs Fixing

[Andrew Bennetts (spiv)]

Thanks very much for the reviews so far. This patch adds a test, as well as the other changes suggested in the earlier reviews.

The test is a bit ugly, but I'm not sure if there's a better way to write it. There are quite a few layers involved in the content filtering APIs. I think it's probably ok, but an extra review won't hurt :)

I agree that this is a bit risky for 2.0, so I wouldn't propose backporting it without a) someone clamouring for it, and b) time for it to be tested in trunk.

[John A Meinel (jameinel)]

46 + self.assertEqual(source_stat, target_stat)

^- It is generally better to use "self.assertEqualStat()".

I realize it isn't strictly required here, since you aren't doing an fstat versus lstat, etc.

118 + os.mkdir(self.test_home_dir + '/.bazaar')
119 + rules_filename = self.test_home_dir + '/.bazaar/rules'
120 + f = open(rules_filename, 'w')
121 + f.write('[name %s]\nrot13=yes\n' % (pattern,))
122 + f.close()

^- Could you use ".build_tree()" and ".build_tree_contents()" here? I'll also note that you should at least use 'wb', since I don't think you want to get \r\n content on Windows. (maybe this doesn't matter as much since it is a config file?)

Would it be easier to use something like "fn = config.get_rules_filename()" ? I don't know how that path is currently determined, but it doesn't seem like you should be going via self.test_home_dir. (If there isn't an api for it, just submit a bug, and leave the test alone.)

Otherwise I think the change is reasonable.

review: Approve

[Andrew Bennetts (spiv)]

John A Meinel wrote:
> Review: Approve
> 46 + self.assertEqual(source_stat, target_stat)
>
> ^- It is generally better to use "self.assertEqualStat()".
>
> I realize it isn't strictly required here, since you aren't doing an fstat
> versus lstat, etc.

Ok, although there's no assertNotEqualStat, so I'll leave the corresponding
assertNotEqual as it is.

> 118 + os.mkdir(self.test_home_dir + '/.bazaar')
> 119 + rules_filename = self.test_home_dir + '/.bazaar/rules'
> 120 + f = open(rules_filename, 'w')
> 121 + f.write('[name %s]\nrot13=yes\n' % (pattern,))
> 122 + f.close()
>
> ^- Could you use ".build_tree()" and ".build_tree_contents()" here? I'll also
> note that you should at least use 'wb', since I don't think you want to get
> \r\n content on Windows. (maybe this doesn't matter as much since it is a
> config file?)

Good point about 'wb', changed (although I don't know the precise rules for
newlines in config files).

I cannot easily use build_tree_contents, because doesn't provide a convenient
way to build a tree relative to a dir other than '.'. Perhaps it should be
updated to take an optional transport and use put_bytes_non_atomic like
TestCase.build_tree? And I still need to remove or rename the file afterwards
before I call reset_rules()... probably that's something that should be handled
by the base TestCase, but it isn't at the moment.

> Would it be easier to use something like "fn = config.get_rules_filename()" ?
> I don't know how that path is currently determined, but it doesn't seem like
> you should be going via self.test_home_dir. (If there isn't an api for it,
> just submit a bug, and leave the test alone.)
>
> Otherwise I think the change is reasonable.

Hmm, there's bzrlib.rules.rules_filename(). I kind of like the safety of using
the test_home_dir explicitly though, it's easy to imagine a naïve optimisation
of rules_filename() caching and returning the wrong value — after all I need to
call reset_rules() because of similar caching. I might be too pessimistic
though. Anyway, I'd still need to makedirs(os.path.dirname(fn)), so it wouldn't
end up being much easier.

You do have a point that ideally I'd have essentially a one-liner:

     write_file(rules_filename(), '''...''')

And instead I have 5 lines, even ignoring the reset_rules call and cleanups.

So this is something to revisit if we write more tests that want to temporarily
install some rules, but I hope it's good enough for now.

-Andrew.

[Martin Pool (mbp)]

If I touched this again I'd probably add assertIsHardLinked. Not needed now. (And this is a case where testtools's assertIs would be nice, so you didn't need to separately write assertIsNotHardLinked.)

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-12-03 02:24:54 +0000
+++ NEWS	2009-12-04 06:17:11 +0000
@@ -48,6 +48,10 @@
 * Terminate ssh subprocesses when no references to them remain, fixing
   subprocess and file descriptor leaks.  (Andrew Bennetts, #426662)
   
+* The ``--hardlink`` option of ``bzr branch`` and ``bzr checkout`` now
+  works for 2a format trees.  Only files unaffected by content filters
+  will be hardlinked.  (Andrew Bennetts, #408193)
+
 * The new glob expansion on Windows would replace all ``\`` characters
   with ``/`` even if it there wasn't a glob to expand, the arg was quoted,
   etc. Now only change slashes if there is something being glob expanded.

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-12-03 20:56:18 +0000
+++ bzrlib/builtins.py	2009-12-04 06:17:11 +0000
@@ -1209,9 +1209,6 @@
         from bzrlib.tag import _merge_tags_if_possible
         accelerator_tree, br_from = bzrdir.BzrDir.open_tree_or_branch(
             from_location)
-        if (accelerator_tree is not None and
-            accelerator_tree.supports_content_filtering()):
-            accelerator_tree = None
         revision = _get_one_revision('branch', revision)
         br_from.lock_read()
         try:

=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- bzrlib/tests/blackbox/test_branch.py	2009-08-20 12:30:49 +0000
+++ bzrlib/tests/blackbox/test_branch.py	2009-12-04 06:17:11 +0000
@@ -171,15 +171,7 @@
         out, err = self.run_bzr(['branch', 'source', 'target', '--hardlink'])
         source_stat = os.stat('source/file1')
         target_stat = os.stat('target/file1')
-        same_file = (source_stat == target_stat)
-        if same_file:
-            pass
-        else:
-            # https://bugs.edge.launchpad.net/bzr/+bug/408193
-            self.assertContainsRe(err, "hardlinking working copy files is "
-                "not currently supported")
-            raise KnownFailure("--hardlink doesn't work in formats "
-                "that support content filtering (#408193)")
+        self.assertEqual(source_stat, target_stat)
 
     def test_branch_standalone(self):
         shared_repo = self.make_repository('repo', shared=True)

=== modified file 'bzrlib/tests/blackbox/test_checkout.py'
--- bzrlib/tests/blackbox/test_checkout.py	2009-08-03 04:19:03 +0000
+++ bzrlib/tests/blackbox/test_checkout.py	2009-12-04 06:17:11 +0000
@@ -160,12 +160,4 @@
             '--hardlink'])
         source_stat = os.stat('source/file1')
         target_stat = os.stat('target/file1')
-        same_file = (source_stat == target_stat)
-        if same_file:
-            pass
-        else:
-            # https://bugs.edge.launchpad.net/bzr/+bug/408193
-            self.assertContainsRe(err, "hardlinking working copy files is "
-                "not currently supported")
-            raise KnownFailure("--hardlink doesn't work in formats "
-                "that support content filtering (#408193)")
+        self.assertEqual(source_stat, target_stat)

=== modified file 'bzrlib/tests/per_tree/test_iter_search_rules.py'
--- bzrlib/tests/per_tree/test_iter_search_rules.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_tree/test_iter_search_rules.py	2009-12-04 06:17:11 +0000
@@ -18,7 +18,6 @@
 
 from bzrlib import (
     rules,
-    tests,
     )
 from bzrlib.tests.per_tree import TestCaseWithTree
 

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2009-11-18 16:10:18 +0000
+++ bzrlib/tests/test_transform.py	2009-12-04 06:17:11 +0000
@@ -15,18 +15,18 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 import os
-import stat
 from StringIO import StringIO
 import sys
 
 from bzrlib import (
     bencode,
     errors,
+    filters,
     generate_ids,
     osutils,
     progress,
     revision as _mod_revision,
-    symbol_versioning,
+    rules,
     tests,
     urlutils,
     )
@@ -1868,6 +1868,49 @@
         self.assertEqual([], list(target.iter_changes(revision_tree)))
         self.assertTrue(source.is_executable('file1-id'))
 
+    def install_rot13_content_filter(self, pattern):
+        original_registry = filters._reset_registry()
+        def restore_registry():
+            filters._reset_registry(original_registry)
+        self.addCleanup(restore_registry)
+        def rot13(chunks, context=None):
+            return [''.join(chunks).encode('rot13')]
+        rot13filter = filters.ContentFilter(rot13, rot13)
+        filters.register_filter_stack_map('rot13', {'yes': [rot13filter]}.get)
+        os.mkdir(self.test_home_dir + '/.bazaar')
+        rules_filename = self.test_home_dir + '/.bazaar/rules'
+        f = open(rules_filename, 'wb')
+        f.write('[name %s]\nrot13=yes\n' % (pattern,))
+        f.close()
+        def uninstall_rules():
+            os.remove(rules_filename)
+            rules.reset_rules()
+        self.addCleanup(uninstall_rules)
+        rules.reset_rules()
+
+    def test_build_tree_content_filtered_files_are_not_hardlinked(self):
+        """build_tree will not hardlink files that have content filtering rules
+        applied to them (but will still hardlink other files from the same tree
+        if it can).
+        """
+        self.requireFeature(HardlinkFeature)
+        self.install_rot13_content_filter('file1')
+        source = self.create_ab_tree()
+        target = self.make_branch_and_tree('target')
+        revision_tree = source.basis_tree()
+        revision_tree.lock_read()
+        self.addCleanup(revision_tree.unlock)
+        build_tree(revision_tree, target, source, hardlink=True)
+        target.lock_read()
+        self.addCleanup(target.unlock)
+        self.assertEqual([], list(target.iter_changes(revision_tree)))
+        source_stat = os.stat('source/file1')
+        target_stat = os.stat('target/file1')
+        self.assertNotEqual(source_stat, target_stat)
+        source_stat = os.stat('source/file2')
+        target_stat = os.stat('target/file2')
+        self.assertEqualStat(source_stat, target_stat)
+
     def test_case_insensitive_build_tree_inventory(self):
         if (tests.CaseInsensitiveFilesystemFeature.available()
             or tests.CaseInsCasePresFilenameFeature.available()):

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2009-12-03 02:24:54 +0000
+++ bzrlib/transform.py	2009-12-04 06:17:11 +0000
@@ -2300,8 +2300,12 @@
         new_desired_files = desired_files
     else:
         iter = accelerator_tree.iter_changes(tree, include_unchanged=True)
-        unchanged = dict((f, p[1]) for (f, p, c, v, d, n, k, e)
-                         in iter if not (c or e[0] != e[1]))
+        unchanged = [(f, p[1]) for (f, p, c, v, d, n, k, e)
+                     in iter if not (c or e[0] != e[1])]
+        if accelerator_tree.supports_content_filtering():
+            unchanged = [(f, p) for (f, p) in unchanged
+                         if not accelerator_tree.iter_search_rules([p]).next()]
+        unchanged = dict(unchanged)
         new_desired_files = []
         count = 0
         for file_id, (trans_id, tree_path) in desired_files:

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2009-11-14 11:06:51 +0000
+++ bzrlib/workingtree_4.py	2009-12-04 06:17:11 +0000
@@ -1447,21 +1447,10 @@
                 if basis_root_id is not None:
                     wt._set_root_id(basis_root_id)
                     wt.flush()
-                # If content filtering is supported, do not use the accelerator
-                # tree - the cost of transforming the content both ways and
-                # checking for changed content can outweight the gains it gives.
-                # Note: do NOT move this logic up higher - using the basis from
-                # the accelerator tree is still desirable because that can save
-                # a minute or more of processing on large trees!
-                # The original tree may not have the same content filters
-                # applied so we can't safely build the inventory delta from
-                # the source tree.
                 if wt.supports_content_filtering():
-                    if hardlink:
-                        # see https://bugs.edge.launchpad.net/bzr/+bug/408193
-                        trace.warning("hardlinking working copy files is not currently "
-                            "supported in %r" % (wt,))
-                    accelerator_tree = None
+                    # The original tree may not have the same content filters
+                    # applied so we can't safely build the inventory delta from
+                    # the source tree.
                     delta_from_tree = False
                 else:
                     delta_from_tree = True