Merge lp:~jelmer/bzr-builddeb/no-force-gz into lp:bzr-builddeb

[Jelmer Vernooij]

Remove the force_gz argument to the repacker; instead, use the target name to determine what to generate.

This prevents us from generating .tar.bz2 files that actually have gz compression and vice versa.

[James Westby (james-w)]

Good idea.

Thanks,

James

review: Approve

=== modified file 'cmds.py'
--- cmds.py	2011-06-28 08:41:44 +0000
+++ cmds.py	2011-07-17 06:08:40 +0000
@@ -578,8 +578,7 @@
             dest_name = tarball_name(package, version, None, format=format)
             tarball_filename = os.path.join(orig_dir, dest_name)
             try:
-                repack_tarball(location, dest_name, target_dir=orig_dir,
-                        force_gz=not v3)
+                repack_tarball(location, dest_name, target_dir=orig_dir)
             except FileExists:
                 raise BzrCommandError("The target file %s already exists, and is either "
                                       "different to the new upstream tarball, or they "
 
=== modified file 'dh_make.py'
--- dh_make.py	2011-06-28 08:41:44 +0000
+++ dh_make.py	2011-07-17 06:08:40 +0000
@@ -66,8 +66,7 @@
             format = "lzma"
     dest_name = util.tarball_name(package_name, version, None, format=format)
     trace.note("Fetching tarball")
-    repack_tarball(tarball, dest_name, target_dir=orig_dir,
-            force_gz=not use_v3)
+    repack_tarball(tarball, dest_name, target_dir=orig_dir)
     provider = upstream.UpstreamProvider(package_name, version,
             orig_dir, [])
     orig_files = provider.provide(os.path.join(tree.basedir, ".."))
 
=== modified file 'repack_tarball.py'
--- repack_tarball.py	2011-06-24 13:23:49 +0000
+++ repack_tarball.py	2011-07-17 06:08:40 +0000
@@ -141,39 +141,37 @@
 
 
 def get_filetype(filename):
-    types = [".tar.gz", ".tgz", ".tar.bz2", ".tar.xz", ".tar.lzma", ".tbz2",
-             ".tar", ".zip"]
-    for filetype in types:
+    types = {
+        ".tar.gz": "gz",
+        ".tgz": "gz",
+        ".tar.bz2": "bz2",
+        ".tar.xz": "xz",
+        ".tar.lzma": "lzma",
+        ".tbz2": "bz2",
+        ".tar": "tar",
+        ".zip": "zip"
+        }
+    for filetype, name in types.iteritems():
         if filename.endswith(filetype):
-            return filetype
-
-
-def get_repacker_class(source_filename, force_gz=True):
+            return name
+
+
+def get_repacker_class(source_format, target_format):
     """Return the appropriate repacker based on the file extension."""
-    filetype = get_filetype(source_filename)
-    if (filetype == ".tar.gz" or filetype == ".tgz"):
-        return CopyRepacker
-    if (filetype == ".tar.bz2" or filetype == ".tbz2"):
-        if force_gz:
-            return Tbz2TgzRepacker
-        return CopyRepacker
-    if filetype == ".tar.lzma":
-        if force_gz:
-            return TarLzma2TgzRepacker
-        return CopyRepacker
-    if filetype == ".tar.xz":
-        return TarLzma2TgzRepacker
-    if filetype == ".tar":
-        return TarTgzRepacker
-    if filetype == ".zip":
-        return ZipTgzRepacker
-    return None
-
-
-def _error_if_exists(target_transport, new_name, source_name, force_gz=True):
+    if source_format == target_format:
+        return CopyRepacker
+    known_formatters = {
+        ("bz2", "gz"): Tbz2TgzRepacker,
+        ("lzma", "gz"): TarLzma2TgzRepacker,
+        ("xz", "gz"): TarLzma2TgzRepacker,
+        ("tar", "gz"): TarTgzRepacker,
+        ("zip", "gz"): ZipTgzRepacker,
+        }
+    return known_formatters.get((source_format, target_format))
+
+
+def _error_if_exists(target_transport, new_name, source_name):
     source_filetype = get_filetype(source_name)
-    if force_gz and source_filetype != ".tar.gz":
-        raise FileExists(new_name)
     source_f = open_file(source_name)
     try:
         source_sha = new_sha(source_f.read()).hexdigest()
@@ -201,8 +199,10 @@
         target_f.close()
 
 
-def _repack_other(target_transport, new_name, source_name, force_gz=True):
-    repacker_cls = get_repacker_class(source_name, force_gz=force_gz)
+def _repack_other(target_transport, new_name, source_name):
+    source_filetype = get_filetype(source_name)
+    target_filetype = get_filetype(new_name)
+    repacker_cls = get_repacker_class(source_filetype, target_filetype)
     if repacker_cls is None:
         raise UnsupportedRepackFormat(source_name)
     target_transport.ensure_base()
@@ -218,7 +218,7 @@
         target_f.close()
 
 
-def repack_tarball(source_name, new_name, target_dir=None, force_gz=True):
+def repack_tarball(source_name, new_name, target_dir=None):
     """Repack the file/dir named to a .tar.gz with the chosen name.
 
     This function takes a named file of either .tar.gz, .tar .tgz .tar.bz2 
@@ -239,7 +239,6 @@
     :keyword target_dir: the directory to consider new_name relative to, and
                          will be created if non-existant.
     :type target_dir: string
-    :param force_gz: whether to repack other .tar files to .tar.gz.
     :return: None
     :throws NoSuchFile: if source_name doesn't exist.
     :throws FileExists: if the target filename (after considering target_dir)
@@ -257,11 +256,13 @@
     extra, new_name = os.path.split(new_name)
     target_transport = get_transport(os.path.join(target_dir, extra))
     if target_transport.has(new_name):
-        _error_if_exists(target_transport, new_name, source_name,
-                force_gz=force_gz)
+        source_format = get_filetype(source_name)
+        target_format = get_filetype(new_name)
+        if source_format != target_format:
+            raise FileExists(new_name)
+        _error_if_exists(target_transport, new_name, source_name)
         return
     if os.path.isdir(source_name):
         _repack_directory(target_transport, new_name, source_name)
     else:
-        _repack_other(target_transport, new_name, source_name,
-                force_gz=force_gz)
+        _repack_other(target_transport, new_name, source_name)
 
=== modified file 'tests/test_repack_tarball.py'
--- tests/test_repack_tarball.py	2011-06-24 13:23:49 +0000
+++ tests/test_repack_tarball.py	2011-07-17 06:08:40 +0000
@@ -27,6 +27,7 @@
 from bzrlib.plugins.builddeb.errors import UnsupportedRepackFormat
 from bzrlib.plugins.builddeb.repack_tarball import (
         repack_tarball,
+        get_filetype,
         get_repacker_class,
         )
 from bzrlib.plugins.builddeb.tests import TestCaseInTempDir
@@ -69,7 +70,9 @@
 
     def test_repack_tarball_non_extant(self):
         error = NoSuchFile
-        if (get_repacker_class(self.old_tarball) is None):
+        old_format = get_filetype(self.old_tarball)
+        new_format = get_filetype(self.new_tarball)
+        if (get_repacker_class(old_format, new_format) is None):
             # directory, can't really be detected remotely, so we have a
             # error that could mean two things
             error = UnsupportedRepackFormat
@@ -132,3 +135,6 @@
         self.assertPathDoesNotExist(self.new_tarball)
         self.assertPathDoesNotExist(os.path.join(target_dir, self.new_tarball))
         self.assertPathExists(target_dir)
+
+
+
 
=== modified file 'tests/test_repack_tarball_extra.py'
--- tests/test_repack_tarball_extra.py	2011-06-24 13:23:49 +0000
+++ tests/test_repack_tarball_extra.py	2011-07-17 06:08:40 +0000
@@ -118,8 +118,7 @@
         bz2_tarball_name = 'package-0.2.tar.bz2'
         create_basedir('package-0.2/', files=['README'])
         make_new_upstream_tarball_bz2(bz2_tarball_name)
-        repack_tarball(bz2_tarball_name, bz2_tarball_name, target_dir=".",
-                force_gz=False)
+        repack_tarball(bz2_tarball_name, bz2_tarball_name, target_dir=".")
         self.assertPathExists(bz2_tarball_name)
 
     def test_exists_different_bz2(self):
 
=== modified file 'upstream/__init__.py'
--- upstream/__init__.py	2011-06-28 08:41:44 +0000
+++ upstream/__init__.py	2011-07-17 06:08:40 +0000
@@ -177,7 +177,7 @@
             fetched_tarball = os.path.join(source_dir, filename)
             if os.path.exists(fetched_tarball):
                 repack_tarball(fetched_tarball, filename,
-                               target_dir=target_dir, force_gz=False)
+                               target_dir=target_dir)
                 filenames.append(os.path.join(target_dir, filename))
         if filenames:
             return filenames
@@ -441,7 +441,7 @@
             return None
         for path in paths:
             repack_tarball(path, os.path.basename(path),
-                    target_dir=target_dir, force_gz=False)
+                    target_dir=target_dir)
         return paths
 
 
@@ -482,7 +482,7 @@
         if version != self.version:
             raise PackageVersionNotPresent(package, version, self)
         dest_name = tarball_name(package, version)
-        repack_tarball(self.path, dest_name, target_dir=target_dir, force_gz=True)
+        repack_tarball(self.path, dest_name, target_dir=target_dir)
         return [os.path.join(target_dir, dest_name)]
 
     def get_latest_version(self, package, version):
@@ -556,8 +556,7 @@
                     outf.close()
             finally:
                 inf.close()
-            repack_tarball(tmppath, dest_name, target_dir=target_dir,
-                force_gz=True)
+            repack_tarball(tmppath, dest_name, target_dir=target_dir)
             return os.path.join(target_dir, dest_name)
         finally:
             shutil.rmtree(tmpdir)