launchpad-reviewers team mailing list archive

[James Westby <jw+debian@xxxxxxxxxxxxxxx>]

James Westby has proposed merging lp:~james-w/launchpad/drop-filecmp into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~james-w/launchpad/drop-filecmp/+merge/108021

= Summary =

When looking at generate_ppa_htaccess we noticed a pessimism
for the common case that we suspect will be adding extra
I/O operations to the run of the script, and so not be helping
with germanium's load.

This branch drops that, and moves to just overwriting the current
.htpasswd file in all cases, even if it wouldn't change the content.

== Proposed fix ==

Do that.

== Pre-implementation notes ==

None.

== LOC Rationale ==

-14 lines.

== Implementation details ==

Deletes the code. Also drops the return value and the removal
of the file, because it will now always be renamed.

== Tests ==

Adjusts the test case for the old behaviour to just test
the new behaviour.

== Demo and Q/A ==

A run of generate_ppa_htaccess with a new subscriber for a
private PPA should suffice.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py
  lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py
-- 
https://code.launchpad.net/~james-w/launchpad/drop-filecmp/+merge/108021
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~james-w/launchpad/drop-filecmp into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2012-05-10 21:44:21 +0000
+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2012-05-30 17:22:22 +0000
@@ -9,7 +9,6 @@
     datetime,
     timedelta,
     )
-import filecmp
 import os
 import tempfile
 
@@ -104,21 +103,17 @@
         :return: True if the file was replaced.
         """
         if self.options.dryrun:
-            return False
+            os.remove(temp_htpasswd_file)
+            return
 
         # The publisher Config object does not have an
         # interface, so we need to remove the security wrapper.
         pub_config = getPubConfig(ppa)
         htpasswd_filename = os.path.join(pub_config.htaccessroot, ".htpasswd")
 
-        if (not os.path.isfile(htpasswd_filename) or
-            not filecmp.cmp(htpasswd_filename, temp_htpasswd_file)):
-            # Atomically replace the old file or create a new file.
-            os.rename(temp_htpasswd_file, htpasswd_filename)
-            self.logger.debug("Replaced htpasswd for %s" % ppa.displayname)
-            return True
-
-        return False
+        # Atomically replace the old file or create a new file.
+        os.rename(temp_htpasswd_file, htpasswd_filename)
+        self.logger.debug("Replaced htpasswd for %s" % ppa.displayname)
 
     def sendCancellationEmail(self, token):
         """Send an email to the person whose subscription was cancelled."""
@@ -323,8 +318,7 @@
 
             self.ensureHtaccess(ppa)
             temp_htpasswd = self.generateHtpasswd(ppa, valid_tokens)
-            if not self.replaceUpdatedHtpasswd(ppa, temp_htpasswd):
-                os.remove(temp_htpasswd)
+            self.replaceUpdatedHtpasswd(ppa, temp_htpasswd)
 
         if self.options.no_deactivation or self.options.dryrun:
             self.logger.info('Dry run, so not committing transaction.')

=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2012-05-10 21:44:21 +0000
+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2012-05-30 17:22:22 +0000
@@ -163,8 +163,7 @@
         os.remove(filename)
 
     def testReplaceUpdatedHtpasswd(self):
-        """Test that the htpasswd file is only replaced if it changes."""
-        FILE_CONTENT = "Kneel before Zod!"
+        """Test that the htpasswd file is replaced."""
         # The publisher Config object does not have an interface, so we
         # need to remove the security wrapper.
         pub_config = getPubConfig(self.ppa)
@@ -174,28 +173,21 @@
         if not os.path.exists(pub_config.htaccessroot):
             os.makedirs(pub_config.htaccessroot)
         file = open(filename, "w")
-        file.write(FILE_CONTENT)
+        file.write("Kneel before Zod!")
         file.close()
 
         # Write the same contents in a temp file.
+        expected_content = "Come to me, son of Jor-El!"
         fd, temp_filename = tempfile.mkstemp(dir=pub_config.htaccessroot)
         file = os.fdopen(fd, "w")
-        file.write(FILE_CONTENT)
+        file.write(expected_content)
         file.close()
 
-        # Replacement should not happen.
         script = self.getScript()
-        self.assertFalse(
-            script.replaceUpdatedHtpasswd(self.ppa, temp_filename))
-
-        # Writing a different .htpasswd should see it get replaced.
-        file = open(filename, "w")
-        file.write("Come to me, son of Jor-El!")
-        file.close()
-
-        self.assertTrue(
-            script.replaceUpdatedHtpasswd(self.ppa, temp_filename))
-
+        script.replaceUpdatedHtpasswd(self.ppa, temp_filename)
+
+        with open(filename, 'r') as f:
+            self.assertEqual(expected_content, f.read())
         os.remove(filename)
 
     def assertDeactivated(self, token):