Merge lp:~parthm/bzr/173274-export-hooks into lp:bzr

[Parth Malwankar]

support for pre_export and post_export hooks.

=== Fixes Bug #173274 ===
This patch adds support for pre_export and post_export hooks.

The 'return _exporters[format](...)' call in bzrlib.export.export didn't really return a value. Trivially fixed that also.

[Andrew Bennetts (spiv)]

144 +# FIXME: As export is a directory based module (as against a py file), our
145 +# lazy registering doesn't see 'hooks' in the first tuple. To work around
146 +# this directly list __init__ below. This can possibly be cleaner.
147 +known_hooks.register_lazy(('bzrlib.export.__init__', 'hooks'),
148 + 'bzrlib.export', 'ExportHooks')

I don't understand why this is necessary... oh, I see. There's a bug in _LazyObjectGetter. I've submitted <https://code.edge.launchpad.net/~spiv/bzr/hooks-refactoring/+merge/36101> to fix this, and also make known_hooks registration a little neater. Until that lands this is an ok workaround.

36 - return _exporters[format](tree, dest, root, subdir, filtered=filtered,
37 - per_file_timestamps=per_file_timestamps)
38 + for hook in hooks['pre_export']:
39 + hook(ExportHookParams(tree, dest, format, root, subdir,
40 + filtered, per_file_timestamps))
41 + _exporters[format](tree, dest, root, subdir, filtered=filtered,
42 + per_file_timestamps=per_file_timestamps)
43 + for hook in hooks['post_export']:
44 + hook(ExportHookParams(tree, dest, format, root, subdir,
45 + filtered, per_file_timestamps))

Is this actually sufficient to provide the functionality requested in bug 173274? Specifically, it requests a hook that allows modifying the tree to export, e.g. by running autoconf, and having the new, unversioned (and probably ignored) files that generates be included, and possibly include modifications to versioned files too. I don't think this implementation allows that, as I believe that regardless of the pre_export hook it will still export "the last committed revision", as "bzr help export" says.

Well, the pre-hook could commit a new revision with the changes, and the post-hook could uncommit it... that would be a really ugly hack, and a plugin could already wrap cmd_export to do that sort of hack.

So, two changes I'd like to see:

 * support for somehow allowing selected unversioned files in the tree to be included in the export. Perhaps by changing the Tree object that is being exported?
 * tests that demonstrate that support, so that we can be confident the use case in bug 173274 is satisfied :)

review: Needs Fixing

[Parth Malwankar (parthm)]

> * support for somehow allowing selected unversioned files in the tree to be
> included in the export. Perhaps by changing the Tree object that is being
> exported?
> * tests that demonstrate that support, so that we can be confident the use
> case in bug 173274 is satisfied :)

Taking bug #619478 into account, maybe a --all or --unversioned flag for 'bzr export'? I can create a patch for that before we try to land this in case one of the above seems like a good solution. Thoughts?

Thanks for the review.

[lp:~parthm/bzr/173274-export-hooks]

5436. By Parth Malwankar on 2010-09-21

merged in changes from trunk

[Martin Pool (mbp)]

On 22 September 2010 01:21, Parth Malwankar <email address hidden> wrote:
>
>>  * support for somehow allowing selected unversioned files in the tree to be
>> included in the export.  Perhaps by changing the Tree object that is being
>> exported?
>>  * tests that demonstrate that support, so that we can be confident the use
>> case in bug 173274 is satisfied :)
>
> Taking bug #619478 into account, maybe a --all or --unversioned flag for 'bzr export'? I can create a patch for that before we try to land this in case one of the above seems like a good solution. Thoughts?

That sounds good. I'd used --uncommitted, to be consistent with merge.

--
Martin

[Vincent Ladeuil (vila)]

>>>>> Martin Pool <email address hidden> writes:

<snip/>

    >> Taking bug #619478 into account, maybe a --all or --unversioned
    >> flag for 'bzr export'? I can create a patch for that before we
    >> try to land this in case one of the above seems like a good
    >> solution. Thoughts?

    > That sounds good. I'd used --uncommitted, to be consistent with merge.

+1

Handling 'junk' files may become possible in the near future and using
'--uncommitted' to encompass all the kinds of unversioned file seems to
be the best bet.

   Vincent

[John A Meinel (jameinel)]

Just mentioning that "--uncommitted" sounds to me like "give me the
content of the *versioned* files that hasn't been committed yet". Which
is how merge uses it. Specifically, you don't get junk files when you
'merge --uncomitted'.

[Andrew Bennetts (spiv)]

John A Meinel wrote:
> Just mentioning that "--uncommitted" sounds to me like "give me the
> content of the *versioned* files that hasn't been committed yet". Which
> is how merge uses it. Specifically, you don't get junk files when you
> 'merge --uncomitted'.

Yes, I think uncommitted-but-versioned isn't what the use case is asking for.
It is asking for a way to get some (all?) unversioned files included (and
probably uncommitted changes too).

I'm not sure exactly what would best suit the use case (bzr export of a tree
including 'autoconf' output). Typically you wouldn't want to export all ignored
files (e.g. usually you don't want to ship: tags, *.pyc, *.o, ...). Perhaps the
hypothetical 'precious' designation is good enough, but I'm not sure it's an
exact fit. So I suspect a complete, polished solution may involve some extra
complexity (perhaps just some config in bzrrules to define some files as
unversioned_exportable)...

But that's policy. In terms of mechanism, we need a hook or other API that
gives a plugin a way to:

  * cause selected unversioned files to be included in the export
  * cause uncommitted versions of versioned files to be included in the export

Then we should be able to support whatever exact behaviour is needed.

[Vincent Ladeuil (vila)]

ping, this branch is now a pre-requisite of https://code.edge.launchpad.net/~gz/bzr/transport_post_connect_hook/+merge/38431

What's the status here ?

[Martin Packman (gz)]

Ah, this one isn't a pre-requisite for me, I just used Parth as a reference and inspiration. :)

Unmerged revisions

5436. By Parth Malwankar on 2010-09-21

merged in changes from trunk

5435. By Parth Malwankar on 2010-09-18

added NEWS entry. fixed typo.

5434. By Parth Malwankar on 2010-09-18

added pre-export and post-export hooks + tests.

5433. By Parth Malwankar on 2010-09-18

removed unwanted return from export function

=== modified file 'NEWS'
--- NEWS	2010-09-21 10:48:23 +0000
+++ NEWS	2010-09-21 16:04:46 +0000
@@ -16,6 +16,10 @@
 New Features
 ************
 
+* Added ``pre_export`` and ``post_export`` hooks. This allows plugins
+  to register custom handlers which will be invoked before/after the
+  tree is exported. (Parth Malwankar, #173274)
+
 Bug Fixes
 *********
 
 
=== modified file 'bzrlib/export/__init__.py'
--- bzrlib/export/__init__.py	2010-03-25 09:39:03 +0000
+++ bzrlib/export/__init__.py	2010-09-21 16:04:46 +0000
@@ -20,7 +20,11 @@
 """
 
 import os
-import bzrlib.errors as errors
+
+from bzrlib import (
+    errors,
+    hooks as _mod_hooks,
+    )
 
 # Maps format name => export function
 _exporters = {}
@@ -103,8 +107,14 @@
         raise errors.NoSuchExportFormat(format)
     tree.lock_read()
     try:
-        return _exporters[format](tree, dest, root, subdir, filtered=filtered,
-                                  per_file_timestamps=per_file_timestamps)
+        for hook in hooks['pre_export']:
+            hook(ExportHookParams(tree, dest, format, root, subdir,
+                filtered, per_file_timestamps))
+        _exporters[format](tree, dest, root, subdir, filtered=filtered,
+            per_file_timestamps=per_file_timestamps)
+        for hook in hooks['post_export']:
+            hook(ExportHookParams(tree, dest, format, root, subdir,
+                filtered, per_file_timestamps))
     finally:
         tree.unlock()
 
@@ -176,6 +186,86 @@
         yield entry
 
 
+class ExportHooks(_mod_hooks.Hooks):
+    """A dictionary mapping hook name to a list of callables for export hooks.
+
+    e.g. ['pre_export'] is the list of items to be called before bzr
+    starts exporting files.
+    """
+
+    def __init__(self):
+        """Create the default hooks.
+
+        These are all empty initially, because by default nothing should get
+        notified.
+        """
+        _mod_hooks.Hooks.__init__(self)
+        self.create_hook(_mod_hooks.HookPoint('pre_export',
+            "Called with argument ExportHookParams before Bazaar "
+            "exports the tree. ExportHookParams has the attributes "
+            "(tree, dest, format, root, subdir, filtered, "
+            "per_file_timestamps). The first argument is the tree to be "
+            "exported, other arguments correspond to command "
+            "line options specified by the user for the export command.",
+            (2, 3), None))
+        self.create_hook(_mod_hooks.HookPoint('post_export',
+            "Called with argument ExportHookParams after Bazaar has "
+            "exported the tree. ExportHookParams has the attributes "
+            "(tree, dest, format, root, subdir, filtered, "
+            "per_file_timestamps). The first argument is the tree to be "
+            "exported, other arguments correspond to command "
+            "line options specified by the user for the export command.",
+            (2, 3), None))
+
+
+class ExportHookParams(object):
+    """Object holding parameters passed to post_export hooks.
+
+    :ivar tree: The tree to export
+    :ivar dest: The destination where the files etc. should be put
+    :ivar format: The format (dir, zip, etc)
+    :ivar root: Name of the root directory inside the exported file
+    :ivar subdir: The subdirectory to export.
+    :ivar filtered: If True, content filtering is applied to the
+        files exported.
+    :ivar per_file_timestamps: If True, use the timestamp stored in the
+        tree rather than now().
+    """
+
+    def __init__(self, tree, dest, format=None, root=None, subdir=None,
+            filtered=False, per_file_timestamps=False):
+        """Create a group of post_export hook parameters.
+
+        :param tree: The tree to export
+        :param dest: The destination where the files etc. should be put
+        :param format: The format (dir, zip, etc)
+        :param root: Name of the root directory inside the exported file
+        :param subdir: The subdirectory to export.
+        :param filtered: If True, content filtering is applied to the
+            files exported.
+        :param per_file_timestamps: If True, use the timestamp stored in the
+            tree rather than now().
+        """
+        self.tree = tree
+        self.dest = dest
+        self.format = format
+        self.root = root
+        self.subdir = subdir
+        self.filtered = filtered
+        self.per_file_timestamps = per_file_timestamps
+
+    def __eq__(self, other):
+        return self.__dict__ == other.__dict__
+
+    def __repr__(self):
+        return "<%s(%s, %s, %s, %s, %s, %s, %s)>" % (self.__class__.__name__,
+            self.tree, self.dest, self.format, self.root,
+            self.subdir, self.filtered, self.per_file_timestamps)
+
+
+hooks = ExportHooks()
+
+
 register_lazy_exporter(None, [], 'bzrlib.export.dir_exporter', 'dir_exporter')
 register_lazy_exporter('dir', [], 'bzrlib.export.dir_exporter', 'dir_exporter')
 register_lazy_exporter('tar', ['.tar'], 'bzrlib.export.tar_exporter', 'tar_exporter')
 
=== modified file 'bzrlib/hooks.py'
--- bzrlib/hooks.py	2010-08-28 06:13:48 +0000
+++ bzrlib/hooks.py	2010-09-21 16:04:46 +0000
@@ -40,6 +40,11 @@
     'BzrDirHooks')
 known_hooks.register_lazy(('bzrlib.commands', 'Command.hooks'),
     'bzrlib.commands', 'CommandHooks')
+# FIXME: As export is a directory based module (as against a py file), our
+# lazy registering doesn't see 'hooks' in the first tuple. To work around
+# this directly list __init__ below. This can possibly be cleaner.
+known_hooks.register_lazy(('bzrlib.export.__init__', 'hooks'),
+    'bzrlib.export', 'ExportHooks')
 known_hooks.register_lazy(('bzrlib.info', 'hooks'),
     'bzrlib.info', 'InfoHooks')
 known_hooks.register_lazy(('bzrlib.lock', 'Lock.hooks'), 'bzrlib.lock',
 
=== modified file 'bzrlib/tests/test_export.py'
--- bzrlib/tests/test_export.py	2010-04-14 00:11:32 +0000
+++ bzrlib/tests/test_export.py	2010-09-21 16:04:46 +0000
@@ -121,3 +121,59 @@
         t = self.get_transport('target')
         self.assertEqual(a_time, t.stat('a').st_mtime)
         self.assertEqual(b_time, t.stat('b').st_mtime)
+
+
+class TestHooks(tests.TestCaseWithTransport):
+
+    def build_tree_and_export(self):
+        """Build tree and run export."""
+        self.build_tree(['a/', 'a/b', 'a/c'])
+        wt = self.make_branch_and_tree('.')
+        wt.add(['a', 'a/b', 'a/c'])
+        export.export(wt, 'target', format="dir")
+
+    def assertIsExportHookParams(self, params):
+        """Ensure params is a valid ExportHookParams instance."""
+        self.assertIsInstance(params, export.ExportHookParams)
+        attrs = ['tree', 'dest', 'format', 'root', 'subdir',
+            'filtered', 'per_file_timestamps']
+        for a in attrs:
+            self.assertTrue(hasattr(params, a),
+                'Attribute "%s" not found in ExportHookParam' % a)
+
+    def test_constructor(self):
+        """Check that creating a ExportHooks instance has the right defaults.
+        """
+        hooks = export.ExportHooks()
+        self.assertTrue("pre_export" in hooks, "pre_export not in %s" % hooks)
+        self.assertTrue("post_export" in hooks, "post_export not in %s" % hooks)
+
+    def test_installed_hooks_are_ExportHooks(self):
+        """The installed hooks object should be a ExportHooks.
+        """
+        # The installed hooks are saved in self._preserved_hooks.
+        self.assertIsInstance(self._preserved_hooks[export.__init__][1],
+            export.__init__.ExportHooks)
+
+    def test_post_export_hook(self):
+        """Ensure that post_export hook is invoked with the right args.
+        """
+        calls = []
+        export.hooks.install_named_hook('post_export', calls.append, None)
+        self.assertLength(0, calls)
+        self.build_tree_and_export()
+        self.assertLength(1, calls)
+        params = calls[0]
+        self.assertIsExportHookParams(params)
+
+    def test_pre_export_hook(self):
+        """Ensure that pre_export hook is invoked with the right args.
+        """
+        calls = []
+        export.hooks.install_named_hook('pre_export', calls.append, None)
+        self.assertLength(0, calls)
+        self.build_tree_and_export()
+        self.assertLength(1, calls)
+        params = calls[0]
+        self.assertIsExportHookParams(params)
+