pq.Patch: move support for parsing patch headers into Patch class

and add tests.

4 files changed, 166 insertions, 66 deletions

diff --git a/gbp/pq.py b/gbp/pq.py
index 00127997..ad9af576 100644
--- a/gbp/pq.py
+++ b/gbp/pq.py
@@ -18,7 +18,9 @@
 
 import os
 import re
-from errors import GbpError
+import subprocess
+import tempfile
+from gbp.errors import GbpError
 
 class Patch(object):
     """
@@ -26,15 +28,22 @@ class Patch(object):
 
     @ivar path: path to the patch
     @type path: string
-    @ivar topic: the topic of the patch
+    @ivar topic: the topic of the patch (the directory component)
     @type topic: string
     @ivar strip: path components to strip (think patch -p<strip>)
     @type strip: integer
+    @ivar info: Information retrieved from a RFC822 style patch header
+    @type info: C{dict} with C{str} keys and values
+    @ivar long_desc: the long description of the patch
     """
+    patch_exts = ['diff', 'patch']
+
     def __init__(self, path, topic=None, strip=None):
         self.path = path
         self.topic = topic
         self.strip = strip
+        self.info = None
+        self.long_desc = None
 
     def __repr__(self):
         repr = "<gbp.pq.Patch path='%s' " % self.path
@@ -45,6 +54,104 @@ class Patch(object):
         repr += ">"
         return repr
 
+    def _read_info(self):
+        """
+        Read patch information into a structured form
+
+        using I{git mailinfo}
+        """
+        self.info = {}
+        body = tempfile.NamedTemporaryFile(prefix='gbp_')
+        pipe = subprocess.Popen("git mailinfo '%s' /dev/null < '%s'" %
+                                (body.name, self.path),
+                                shell=True,
+                                stdout=subprocess.PIPE).stdout
+        for line in pipe:
+            if ':' in line:
+                rfc_header, value = line.split(" ", 1)
+                header = rfc_header[:-1].lower()
+                self.info[header] = value.strip()
+        try:
+            self.long_desc = "".join([ line for line in body ])
+            body.close()
+        except IOError as msg:
+            raise GbpError("Failed to read patch header of '%s': %s" %
+                           (self.patch, msg))
+        finally:
+            if os.path.exists(body.name):
+                os.unlink(body.name)
+
+    def _get_subject_from_filename(self):
+        """
+        Determine the patch's subject based on the it's filename
+
+        >>> p = Patch('debian/patches/foo.patch')
+        >>> p._get_subject_from_filename()
+        'foo'
+        >>> Patch('foo.patch')._get_subject_from_filename()
+        'foo'
+        >>> Patch('debian/patches/foo.bar')._get_subject_from_filename()
+        'foo.bar'
+        >>> p = Patch('debian/patches/foo')
+        >>> p._get_subject_from_filename()
+        'foo'
+
+        @return: the patch's subject
+        @rtype: C{str}
+        """
+        subject = os.path.basename(self.path)
+        # Strip of .diff or .patch from patch name
+        try:
+            base, ext = subject.rsplit('.', 1)
+            if ext in self.patch_exts:
+                subject = base
+        except ValueError:
+                pass # No ext so keep subject as is
+        return subject
+
+    def _get_info_field(self, key, get_val=None):
+        """
+        Return the key I{key} from the info C{dict}
+        or use val if I{key} is not a valid key.
+
+        Fill self.info if not already done.
+
+        @param key: key to fetch
+        @type key: C{str}
+        @param get_val: alternate value if key is not in info dict
+        @type get_val: C{str}
+        """
+        if self.info == None:
+            self._read_info()
+
+        if self.info.has_key(key):
+            return self.info[key]
+        else:
+            return get_val() if get_val else None
+
+
+    @property
+    def subject(self):
+        """
+        The patch's subject, either from the patch header or from the filename.
+        """
+        return self._get_info_field('subject', self._get_subject_from_filename)
+
+    @property
+    def author(self):
+        """The patch's author"""
+        return self._get_info_field('author')
+
+    @property
+    def email(self):
+        """The patch author's email address"""
+        return self._get_info_field('email')
+
+    @property
+    def date(self):
+        """The patch's modification time"""
+        return self._get_info_field('date')
+
 
 class PatchSeries(list):
     """
diff --git a/gbp/scripts/pq.py b/gbp/scripts/pq.py
index 4dee07ce..9cc2f0b5 100644
--- a/gbp/scripts/pq.py
+++ b/gbp/scripts/pq.py
@@ -30,7 +30,7 @@ from gbp.command_wrappers import (Command, GitCommand, RunAtCommand,
                                   CommandExecFailed)
 from gbp.errors import GbpError
 import gbp.log
-from gbp.pq import PatchSeries
+from gbp.pq import (PatchSeries, Patch)
 
 PQ_BRANCH_PREFIX = "patch-queue/"
 PATCH_DIR = "debian/patches/"
@@ -233,7 +233,7 @@ def import_quilt_patches(repo, branch, series, tries, force):
         for patch in queue:
             gbp.log.debug("Applying %s" % patch.path)
             try:
-                apply_and_commit_patch(repo, patch.path, patch.topic)
+                apply_and_commit_patch(repo, patch, patch.topic)
             except (GbpError, GitRepositoryError, CommandExecFailed):
                 repo.set_branch(branch)
                 repo.delete_branch(pq_branch)
@@ -249,30 +249,6 @@ def import_quilt_patches(repo, branch, series, tries, force):
         shutil.rmtree(tmpdir)
 
 
-def get_mailinfo(patch):
-    """Read patch information into a structured form"""
-
-    info = {}
-    body = os.path.join('.git', 'gbp_patchinfo')
-    pipe = subprocess.Popen("git mailinfo %s /dev/null < %s" % (body, patch),
-                            shell=True, stdout=subprocess.PIPE).stdout
-    for line in pipe:
-        if ':' in line:
-            rfc_header, value = line.split(" ",1)
-            header = rfc_header[:-1].lower()
-            info[header] = value.strip()
-
-    try:
-        f = file(body)
-        commit_msg = "".join([ line for line in f ])
-        f.close()
-        os.unlink(body)
-    except IOError, msg:
-        raise GbpError, "Failed to read patch header of '%s': %s" % (patch, msg)
-
-    return info, commit_msg
-
-
 def switch_to_pq_branch(repo, branch):
     """Switch to patch-queue branch if not already there, create it if it
        doesn't exist yet"""
@@ -295,55 +271,30 @@ def apply_single_patch(repo, branch, patch, topic=None):
     switch_to_pq_branch(repo, branch)
     apply_and_commit_patch(repo, patch, topic)
 
-def get_patch_subject_from_filename(patch):
-    """
-    Determine the patch's subject based on the patch's filename
-    >>> get_patch_subject('debian/patches/foo.patch')
-    'foo'
-    >>> get_patch_subject('foo.patch')
-    'foo'
-    >>> get_patch_subject('debian/patches/foo.bar')
-    'foo.bar'
-    >>> get_patch_subject('debian/patches/foo')
-    'foo'
-    """
-    subject = os.path.basename(patch)
-    # Strip of .diff or .patch from patch name
-    try:
-        base, ext = subject.rsplit('.', 1)
-        if ext in [ 'diff', 'patch' ]:
-            subject = base
-    except ValueError:
-        pass # No ext so keep subject as is
-    return subject
 
 def apply_and_commit_patch(repo, patch, topic=None):
     """apply a single patch 'patch', add topic 'topic' and commit it"""
-    header, body = get_mailinfo(patch)
-
-    # If we don't find a subject use the patch's name
-    if not header.has_key('subject'):
-        header['subject'] = get_patch_subject_from_filename(patch)
+    author = { 'name': patch.author,
+               'email': patch.email,
+               'date': patch.date }
 
-    if header.has_key('author') and header.has_key('email'):
-        header['name'] = header['author']
-    else:
+    if not (patch.author and patch.email):
         name, email = get_maintainer_from_control()
         if name:
-            gbp.log.warn("Patch '%s' has no authorship information, using '%s <%s>'"
-                         % (patch, name, email))
-            header['name'] = name
-            header['email'] = email
+            gbp.log.warn("Patch '%s' has no authorship information, "
+                         "using '%s <%s>'" % (patch, name, email))
+            author['name'] = name
+            author['email'] = email
         else:
             gbp.log.warn("Patch %s has no authorship information")
 
-    repo.apply_patch(patch)
+    repo.apply_patch(patch.path)
     tree = repo.write_tree()
-    msg = "%s\n\n%s" % (header['subject'], body)
+    msg = "%s\n\n%s" % (patch.subject, patch.long_desc)
     if topic:
         msg += "\nGbp-Pq-Topic: %s" % topic
-    commit = repo.commit_tree(tree, msg, [repo.head], author=header)
-    repo.update_ref('HEAD', commit, msg="gbp-pq import %s" % patch)
+    commit = repo.commit_tree(tree, msg, [repo.head], author=author)
+    repo.update_ref('HEAD', commit, msg="gbp-pq import %s" % patch.path)
 
 
 def drop_pq(repo, branch):
@@ -408,7 +359,7 @@ def main(argv):
             gbp.log.err("No patch name given.")
             return 1
         else:
-            patch = args[2]
+            patchfile = args[2]
     else:
         gbp.log.err("Unknown action '%s'." % args[1])
         return 1
@@ -435,6 +386,7 @@ def main(argv):
         elif action == "rebase":
             rebase_pq(repo, current)
         elif action == "apply":
+            patch = Patch(patchfile)
             apply_single_patch(repo, current, patch, options.topic)
     except CommandExecFailed:
         retval = 1
diff --git a/tests/08_test_patch.py b/tests/08_test_patch.py
new file mode 100644
index 00000000..8c25e04e
--- /dev/null
+++ b/tests/08_test_patch.py
@@ -0,0 +1,34 @@
+import os
+import unittest
+
+from gbp.pq import Patch
+
+class TestPatch(unittest.TestCase):
+    data_dir = "tests/%s_data" % __module__
+
+    def test_filename(self):
+        """Get patch information from the filename"""
+        p = Patch(os.path.join(self.data_dir, "doesnotexist.diff"))
+        self.assertEqual('doesnotexist', p.subject)
+        self.assertEqual({}, p.info)
+        p = Patch(os.path.join(self.data_dir, "doesnotexist.patch"))
+        self.assertEqual('doesnotexist', p.subject)
+        p = Patch(os.path.join(self.data_dir, "doesnotexist"))
+        self.assertEqual('doesnotexist', p.subject)
+        self.assertEqual(None, p.author)
+        self.assertEqual(None, p.email)
+        self.assertEqual(None, p.date)
+
+    def test_header(self):
+        """Get the patch information from a patch header"""
+        patchfile = os.path.join(self.data_dir, "patch1.diff")
+        self.assertTrue(os.path.exists(patchfile))
+        p = Patch(patchfile)
+        self.assertEqual('This is patch1', p.subject)
+        self.assertEqual("foo", p.author)
+        self.assertEqual("foo@example.com", p.email)
+        self.assertEqual("This is the long description.\n"
+                         "It can span several lines.\n",
+                         p.long_desc)
+        self.assertEqual('Sat, 24 Dec 2011 12:05:53 +0100', p.date)
+
diff --git a/tests/08_test_patch_data/patch1.diff b/tests/08_test_patch_data/patch1.diff
new file mode 100644
index 00000000..5521b73e
--- /dev/null
+++ b/tests/08_test_patch_data/patch1.diff
@@ -0,0 +1,7 @@
+From: foo <foo@example.com>
+Date: Sat, 24 Dec 2011 12:05:53 +0100
+Subject: This is
+ patch1
+
+This is the long description.
+It can span several lines.