Merge lp:~stevenk/launchpad/fixes-bug-387049 into lp:launchpad

[Steve Kowalik]

Forbid copies if the destination archive contains the same files with different contents.

This branch changes the copy package backend so that if the destination archive contains the same files with different contents, CannotCopy is raised and the copy is forbidden.

[Jelmer Vernooij (jelmer)]

Thanks.

One really nitpicky comment; PEP8 requires two empty lines between top-level items.

review: Approve (code*)

[Michael Nelson (michael.nelson)]

> Thanks.
>
> One really nitpicky comment; PEP8 requires two empty lines between top-level
> items.

Cheers Jelmer and Steven,

my only additional comment is that I find:

dfc[file.libraryfile.filename]

hard to read, mainly because of the variable names, but it's just a personal preference. See what you think about the changes below, but feel free to ignore them too :)

> + # Check if files with the same filename already exist in the target
> + destination_file_conflicts = self.archive.getPublishedSources(

s/destination_file_conflicts/destination_source_conflicts

> + name=source.sourcepackagerelease.name)
> + dfc = {}

s/dfc/file_conflicts

> + for file in destination_file_conflicts:

s/file/source_pub

> + for inc_file in file.sourcepackagerelease.files:

s/inc_file/file_alias

> + dfc[inc_file.libraryfile.filename] = inc_file.libraryfile

which would mean this would now be:

                   library_file = file_alias.libraryfile
                   file_conflicts[library_file.filename] = library_file

> + for file in source.sourcepackagerelease.files:
> + if file.libraryfile.filename in dfc.keys():
> + if file.libraryfile != dfc[file.libraryfile.filename]:
> + raise CannotCopy(
> + "%s already exists in destination archive with "
> + "different contents." % file.libraryfile.filename)

review: Approve (code)

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2010-04-09 15:46:09 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2010-04-14 09:58:27 +0000
@@ -339,6 +339,21 @@
                 raise CannotCopy(
                     "binaries conflicting with the existing ones")
 
+        # Check if files with the same filename already exist in the target
+        destination_source_conflicts = self.archive.getPublishedSources(
+            name=source.sourcepackagerelease.name)
+        file_conflicts = {}
+        for source_pub in destination_source_conflicts:
+            for file_alias in source_pub.sourcepackagerelease.files:
+                library_file = file_alias.libraryfile
+                file_conflicts[library_file.filename] = library_file
+        for lf in source.sourcepackagerelease.files:
+            if lf.libraryfile.filename in file_conflicts.keys():
+                if lf.libraryfile != file_conflicts[lf.libraryfile.filename]:
+                    raise CannotCopy(
+                        "%s already exists in destination archive with "
+                        "different contents." % lf.libraryfile.filename)
+
     def checkCopy(self, source, series, pocket):
         """Check if the source can be copied to the given location.
 

=== modified file 'lib/lp/soyuz/scripts/tests/test_copypackage.py'
--- lib/lp/soyuz/scripts/tests/test_copypackage.py	2010-04-09 15:46:09 +0000
+++ lib/lp/soyuz/scripts/tests/test_copypackage.py	2010-04-14 09:58:27 +0000
@@ -2450,6 +2450,45 @@
 
         publish_copies(copied)
 
+    def testCopySourceWithConflictingFiles(self):
+        """We can copy source if the source files match, both in name and
+        contents. We can't if they don't.
+        """
+        ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
+        warty = ubuntu.getSeries('warty')
+        hoary = ubuntu.getSeries('hoary')
+        test_publisher = self.getTestPublisher(warty)
+        test_publisher.addFakeChroots(warty)
+
+        proposed_source = test_publisher.getPubSource(
+            sourcename='test-source', version='1.0-2',
+            distroseries=warty, archive=warty.main_archive,
+            pocket=PackagePublishingPocket.PROPOSED,
+            status=PackagePublishingStatus.PUBLISHED,
+            section='net')
+        proposed_tar = test_publisher.addMockFile(
+            'test-source_1.0.orig.tar.gz', filecontent='aaabbbccc')
+        proposed_source.sourcepackagerelease.addFile(proposed_tar)
+        updates_source = test_publisher.getPubSource(
+            sourcename='test-source', version='1.0-1',
+            distroseries=warty, archive=warty.main_archive,
+            pocket=PackagePublishingPocket.UPDATES,
+            status=PackagePublishingStatus.PUBLISHED,
+            section='misc')
+        updates_tar = test_publisher.addMockFile(
+            'test-source_1.0.orig.tar.gz', filecontent='zzzyyyxxx')
+        updates_source.sourcepackagerelease.addFile(updates_tar)
+        # Commit to ensure librarian files are written.
+        self.layer.txn.commit()
+
+        checker = CopyChecker(warty.main_archive, include_binaries=False)
+        self.assertRaisesWithContent(
+            CannotCopy,
+            "test-source_1.0.orig.tar.gz already exists in destination "
+            "archive with different contents.",
+            checker.checkCopy, proposed_source, warty,
+            PackagePublishingPocket.UPDATES)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)