Merge lp:~jelmer/bzr-builddeb/pre-commit-quilt into lp:bzr-builddeb

[Jelmer Vernooij]

Add pre_commit hook which warns about applied quilt patches and can make sure that no quilt patches are applied or that all quilt patches are applied.

[James Westby (james-w)]

Hi,

This looks good, and is nice and small.

I'm not sure about the default behaviour though.

As it stands we expect all patches to be applied in the importer, so
doing anything else is wrong for udd. Perhaps we don't want to enforce
that, but this will warn if someone is doing the right thing, and won't
warn if they have all the patches unapplied.

I'm not sure what the default behaviour should be, but do you agree that
this warning will confuse people?

Thanks,

James

[Jelmer Vernooij (jelmer)]

> This looks good, and is nice and small.
Thanks.

> I'm not sure about the default behaviour though.
>
> As it stands we expect all patches to be applied in the importer, so
> doing anything else is wrong for udd. Perhaps we don't want to enforce
> that, but this will warn if someone is doing the right thing, and won't
> warn if they have all the patches unapplied.
>
> I'm not sure what the default behaviour should be, but do you agree that
> this warning will confuse people?
Hmm, yeah, that's a good question.

Ideally I think we should get to a point where all packages ship with the patches unapplied in the repository, but with hooks that can apply them when you create/update a working tree. Even if everybody agrees about that, it seems pretty far away though.

Perhaps, for now, we can have it warn only if there is a mix of applied and unapplied patches by default? That situation always seems wrong, no matter what your general policy is.

Cheers,

Jelmer

[lp:~jelmer/bzr-builddeb/pre-commit-quilt]

682. By Jelmer Vernooij on 2012-01-05

Merge trunk.

[James Westby (james-w)]

On Thu, 05 Jan 2012 00:27:33 -0000, Jelmer Vernooij <email address hidden> wrote:
> Ideally I think we should get to a point where all packages ship with
> the patches unapplied in the repository, but with hooks that can apply
> them when you create/update a working tree. Even if everybody agrees
> about that, it seems pretty far away though.

True.

> Perhaps, for now, we can have it warn only if there is a mix of applied and unapplied patches by default? That situation always seems wrong, no matter what your general policy is.

That sounds like a good idea to me.

Thanks,

James

[lp:~jelmer/bzr-builddeb/pre-commit-quilt]

683. By Jelmer Vernooij on 2012-01-05

Merge trunk.

684. By Jelmer Vernooij on 2012-01-05

Add tests.

[Jelmer Vernooij (jelmer)]

Now updated to only warn when there is a mix of applied and unapplied patches, and I've added some tests.

Unmerged revisions

=== modified file '__init__.py'
--- __init__.py	2012-01-05 11:38:00 +0000
+++ __init__.py	2012-01-05 17:06:23 +0000
@@ -191,6 +191,16 @@
     return db.tag_name(changelog.version)
 
 
+def start_commit_check_quilt(tree):
+    """start_commit hook which checks the state of quilt patches.
+    """
+    if tree.path2id("debian/patches") is None:
+        # No patches to worry about
+        return
+    from bzrlib.plugins.builddeb.merge_quilt import start_commit_quilt_patches
+    start_commit_quilt_patches(tree)
+
+
 def pre_merge(merger):
     pre_merge_fix_ancestry(merger)
     pre_merge_quilt(merger)
@@ -317,6 +327,10 @@
         "bzrlib.merge", "Merger.hooks",
         'post_merge', post_merge_quilt_cleanup,
         'Cleaning up quilt temporary directories')
+    install_lazy_named_hook(
+        "bzrlib.mutabletree", "MutableTree.hooks",
+        "start_commit", start_commit_check_quilt,
+        "Check for (un)applied quilt patches")
 
 try:
     from bzrlib.revisionspec import revspec_registry
 
=== modified file 'config.py'
--- config.py	2011-11-22 14:05:25 +0000
+++ config.py	2012-01-05 17:06:23 +0000
@@ -295,6 +295,9 @@
   commit_message_from_changelog = _bool_property('commit-message-from-changelog',
        "Whether the commit message should come from debian/changelog", default=False)
 
+  commit_quilt_policy = _opt_property("quilt-commit-policy",
+        "Policy for committing quilt patches (applied / unapplied)")
+
 
 def _test():
   import doctest
 
=== modified file 'debian/changelog'
--- debian/changelog	2012-01-05 16:09:21 +0000
+++ debian/changelog	2012-01-05 17:06:23 +0000
@@ -5,6 +5,8 @@
     commands like 'bzr bash-completion'. LP: #903650
   * Provide merge-package functionality as a hook for 'bzr merge'.
     LP: #486075, LP: #910900
+  * Add pre-commit hook that warns about applied quilt patches, and can
+    automatically unapply/apply all quilt patches. LP: #608670
   * Automatically unapply patches before merge operations.
     LP: #815854
   * Include full changelog paths that were checked in error message.
 
=== modified file 'doc/user_manual/configuration.rst'
--- doc/user_manual/configuration.rst	2011-10-11 21:22:24 +0000
+++ doc/user_manual/configuration.rst	2012-01-05 17:06:23 +0000
@@ -117,11 +117,23 @@
 Committing
 ^^^^^^^^^^
 
-bzr-builddeb will set the commit message from debian/changelog.  If you do not
-want this set::
+bzr-builddeb can set the commit message from debian/changelog.  To enable this,
+set::
 
   * ``commit-message-from-changelog = false``
 
+When there are quilt patches applied in the current tree, ``bzr commit``
+will by default warn::
+
+  $ bzr commit
+  Committing with 5 quilt patches applied.
+  Committing to: /tmp/popt/
+  Committed revision 20.
+
+It is also possible to force it to always make sure that quilt patches
+are unapplied or applied during a commit by setting the
+``quilt-commit-policy`` to either ``applied`` or ``unapplied``.
+
 Builders
 ^^^^^^^^
 
 
=== modified file 'merge_quilt.py'
--- merge_quilt.py	2012-01-05 11:38:00 +0000
+++ merge_quilt.py	2012-01-05 17:06:23 +0000
@@ -27,11 +27,18 @@
 import tempfile
 from bzrlib.revisiontree import RevisionTree
 from bzrlib import (
+    errors,
     merge as _mod_merge,
     trace,
     )
 
-from bzrlib.plugins.builddeb.quilt import quilt_pop_all
+from bzrlib.plugins.builddeb.quilt import (
+    quilt_applied,
+    quilt_unapplied,
+    quilt_pop_all,
+    quilt_push_all,
+    )
+from bzrlib.plugins.builddeb.util import debuild_config
 
 
 class NoUnapplyingMerger(_mod_merge.Merge3Merger):
@@ -86,3 +93,24 @@
     except:
         shutil.rmtree(target_dir)
         raise
+
+
+def start_commit_quilt_patches(tree):
+    config = debuild_config(tree, False)
+    policy = config.commit_quilt_policy
+    applied_patches = quilt_applied(tree.basedir)
+    unapplied_patches = quilt_unapplied(tree.basedir)
+    if policy is None:
+        # No policy set - just warn about having both applied and unapplied
+        # patches.
+        if applied_patches and unapplied_patches:
+            trace.warning(
+                "Committing with %d patches applied and %d patches unapplied.",
+                len(applied_patches), len(unapplied_patches))
+    elif policy == "applied":
+        quilt_push_all(tree.basedir)
+    elif policy == "unapplied":
+        quilt_pop_all(tree.basedir)
+    else:
+        raise errors.BzrError("Invalid setting %r for quilt-commit-policy" %
+                policy)
 
=== modified file 'tests/test_merge_quilt.py'
--- tests/test_merge_quilt.py	2012-01-05 11:38:00 +0000
+++ tests/test_merge_quilt.py	2012-01-05 17:06:23 +0000
@@ -21,9 +21,15 @@
 
 import shutil
 
+from bzrlib import trace
+
 from bzrlib.hooks import install_lazy_named_hook
 
-from bzrlib.plugins.builddeb import pre_merge_quilt, post_merge_quilt_cleanup
+from bzrlib.plugins.builddeb import (
+    pre_merge_quilt,
+    post_merge_quilt_cleanup,
+    start_commit_check_quilt,
+    )
 from bzrlib.plugins.builddeb.quilt import quilt_push_all
 from bzrlib.plugins.builddeb.merge_quilt import tree_unapply_patches
 
@@ -125,3 +131,84 @@
         # "a" should be unapplied again
         self.assertPathDoesNotExist("a/a")
         self.assertEquals(1, conflicts)
+
+
+class StartCommitMergeHookTests(TestCaseWithTransport):
+
+    def enable_hooks(self):
+        install_lazy_named_hook(
+            "bzrlib.mutabletree", "MutableTree.hooks",
+            'start_commit', start_commit_check_quilt,
+            'Check for (un)applied quilt patches')
+
+    def test_applied(self):
+        self.enable_hooks()
+        tree = self.make_branch_and_tree('source')
+        self.build_tree(['source/debian/', 'source/debian/patches/'])
+        self.build_tree_contents([
+            ('source/debian/patches/series', 'patch1\n'),
+            ('source/debian/patches/patch1', TRIVIAL_PATCH),
+            ('source/debian/bzr-builddeb.conf',
+                "[BUILDDEB]\n"
+                "quilt-commit-policy = applied\n")])
+        self.assertPathDoesNotExist("source/.pc/applied-patches")
+        self.assertPathDoesNotExist("source/a")
+        tree.smart_add([tree.basedir])
+        tree.commit("foo")
+        self.assertPathExists("source/.pc/applied-patches")
+        self.assertPathExists("source/a")
+
+    def test_unapplied(self):
+        self.enable_hooks()
+        tree = self.make_branch_and_tree('source')
+        self.build_tree(['source/debian/', 'source/debian/patches/'])
+        self.build_tree_contents([
+            ('source/debian/patches/series', 'patch1\n'),
+            ('source/debian/patches/patch1', TRIVIAL_PATCH),
+            ('source/debian/bzr-builddeb.conf',
+                "[BUILDDEB]\n"
+                "quilt-commit-policy = unapplied\n")])
+        quilt_push_all(tree.basedir)
+        self.assertPathExists("source/.pc/applied-patches")
+        self.assertPathExists("source/a")
+        tree.smart_add([tree.basedir])
+        tree.commit("foo")
+        self.assertPathDoesNotExist("source/.pc/applied-patches")
+        self.assertPathDoesNotExist("source/a")
+
+    def test_warning(self):
+        self.enable_hooks()
+        warnings = []
+        def warning(*args):
+            if len(args) > 1:
+                warnings.append(args[0] % args[1:])
+            else:
+                warnings.append(args[0])
+        _warning = trace.warning
+        trace.warning = warning
+        self.addCleanup(setattr, trace, "warning", _warning)
+        tree = self.make_branch_and_tree('source')
+        self.build_tree(['source/debian/', 'source/debian/patches/'])
+        self.build_tree_contents([
+            ('source/debian/patches/series', 'patch1\n'),
+            ('source/debian/patches/patch1', TRIVIAL_PATCH)])
+        quilt_push_all(tree.basedir)
+        tree.smart_add([tree.basedir])
+        tree.commit("initial")
+        self.assertEquals([], warnings)
+        self.assertPathExists("source/.pc/applied-patches")
+        self.assertPathExists("source/a")
+        self.build_tree_contents([
+            ('source/debian/patches/series', 'patch1\npatch2\n'),
+            ('source/debian/patches/patch2', 
+                """--- /dev/null	2012-01-02 01:09:10.986490031 +0100
++++ base/b	2012-01-02 20:03:59.710666215 +0100
+@@ -0,0 +1 @@
++a
+""")])
+        tree.smart_add([tree.basedir])
+        tree.commit("foo")
+        self.assertEquals(['Committing with 1 patches applied and 1 patches unapplied.'], warnings)
+        self.assertPathExists("source/.pc/applied-patches")
+        self.assertPathExists("source/a")
+        self.assertPathDoesNotExist("source/b")