Merge ~ilasc/launchpad:repack-for-zero-candidates into launchpad:master

[Ioana Lasc]

Handle zero candidates in repack job

This MP attempts to fix 2 issues:

1: When we have zero repository candidates for repacks with the previous code we were getting:

File "lib/lp/code/scripts/repackgitrepository.py", line 103, in __call__
    self.start_at = repackable_repos[-1].id
IndexError: list index out of range

2: We were not logging in the right place the final count of repacks requested per job run. Moved that log statement to "isDone()".

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/code/scripts/repackgitrepository.py b/lib/lp/code/scripts/repackgitrepository.py
index e0ec646..1ae66f5 100644
--- a/lib/lp/code/scripts/repackgitrepository.py
+++ b/lib/lp/code/scripts/repackgitrepository.py
@@ -53,8 +53,14 @@ class RepackTunableLoop(TunableLoop):
     def isDone(self):
         # we stop at maximum 1000 or when we have no repositories
         # that are valid repack candidates
-        return (self.findRepackCandidates().is_empty() or
-                self.num_repacked + self.maximum_chunk_size >= self.targets)
+        result = (self.findRepackCandidates().is_empty() or
+                  self.num_repacked + self.maximum_chunk_size >= self.targets)
+        if result and not self.dry_run:
+            self.logger.info(
+                'Requested a total of %d automatic git repository repacks '
+                'in this run of the Automated Repack Job.'
+                % self.num_repacked)
+        return result
 
     def __call__(self, chunk_size):
         repackable_repos = list(self.findRepackCandidates()[:chunk_size])
@@ -100,13 +106,10 @@ class RepackTunableLoop(TunableLoop):
                 'out of the %d qualifying for repack.'
                 % (counter, len(repackable_repos)))
 
-        self.start_at = repackable_repos[-1].id
+        if repackable_repos:
+            self.start_at = repackable_repos[-1].id
 
         if not self.dry_run:
             transaction.commit()
-            self.logger.info(
-                'Requested a total of %d automatic git repository repacks '
-                'in this run of the Automated Repack Job.'
-                % self.num_repacked)
         else:
             transaction.abort()
diff --git a/lib/lp/code/scripts/tests/test_repack_git_repositories.py b/lib/lp/code/scripts/tests/test_repack_git_repositories.py
index 19f1b2b..53a77c5 100644
--- a/lib/lp/code/scripts/tests/test_repack_git_repositories.py
+++ b/lib/lp/code/scripts/tests/test_repack_git_repositories.py
@@ -156,6 +156,36 @@ class TestRequestGitRepack(TestCaseWithFactory):
             self.assertEqual("/repo/%s/repack" % repo[i].getInternalPath(),
                              self.turnip_server.app.contents[i])
 
+    def test_auto_repack_zero_repackCandidates(self):
+        self.makeTurnipServer()
+        repo = []
+        for i in range(2):
+            repo.append(self.factory.makeGitRepository())
+            repo[i] = removeSecurityProxy(repo[i])
+            repo[i].loose_object_count = 3
+            repo[i].pack_count = 2
+        transaction.commit()
+
+        # zero candidates
+        # assert on the log contents and the content that makes it to Turnip
+        (ret, out, err) = run_script('cronscripts/repack_git_repositories.py')
+        self.assertIn(
+            'Requested a total of 0 automatic git repository repacks in this '
+            'run of the Automated Repack Job.', err)
+        self.assertEqual([],
+                         self.turnip_server.app.contents)
+
+        # exactly one candidate
+        repo[0].loose_object_count = 7000
+        repo[0].pack_count = 43
+        transaction.commit()
+        (ret, out, err) = run_script('cronscripts/repack_git_repositories.py')
+        self.assertIn(
+            'Requested a total of 1 automatic git repository repacks in '
+            'this run of the Automated Repack Job.', err)
+        self.assertEqual("/repo/%s/repack" % repo[0].getInternalPath(),
+                         self.turnip_server.app.contents[0])
+
     def test_auto_repack_loop_throttle(self):
         repacker = RepackTunableLoop(self.log, None)
         # We throttle at 7 for this test, we use a limit