Merge ~ilasc/turnip:add-repack-data-notify into turnip:master

[Ioana Lasc]

Add repack data to notify call

[Ioana Lasc (ilasc)]

This is still work in progress, opened for visibility only.
I working on figuring out how how to 'cd' into the correct directory on the backend before calling git count-objects as that needs to be performed in the actual repo directory.

[Colin Watson (cjwatson)]

Use the cwd= argument to subprocess.check_output.

[Ioana Lasc (ilasc)]

Thanks Colin, the difficulty I'm having is finding the full path in notifyPush to cd into, the path returned by translatedPath is the repo ID, I need to go from that to the full path on disk, or pass it in somehow from somewhere else.

[Colin Watson (cjwatson)]

OK, I saw the commented-out os.chdir and assumed that the problem was at that level.

Rather than calling git count-objects in hookrpc, I think you should call it in the hook itself (turnip/pack/hooks/hook.py) and then send the relevant parts of the answer in the notify_push RPC call. The hook already runs with the repository in question as its current directory.

[Ioana Lasc (ilasc)]

Oh fantastic, out of the 3 avenues I was pursuing this sounds like the best, thanks Colin!

[Ioana Lasc (ilasc)]

Changes implemented, MP now ready for review.

[Ioana Lasc (ilasc)]

End to end local tests now passing, MP ready for review.

[Colin Watson (cjwatson)]

review: Needs Fixing

[Ioana Lasc (ilasc)]

Agreed with all comments, thanks Colin!
Moved the repack_data method to helpers and covered the call to it from processEnded in test_turnip_set_symbolic_ref.

[Colin Watson (cjwatson)]

Looks pretty good now, thanks! Just a few details, but after that feel free to land and see how it goes on qastaging.

review: Approve

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index e871091..a74f1e0 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -42,6 +42,7 @@ from turnip.pack.helpers import (
     ensure_config,
     ensure_hooks,
     INCOMPLETE_PKT,
+    get_repack_data,
     translate_xmlrpc_fault,
     )
 
@@ -679,8 +680,10 @@ class PackBackendProtocol(PackServerProtocol):
         if self.command == b'turnip-set-symbolic-ref':
             if reason.check(error.ProcessDone):
                 try:
+                    loose_object_count, pack_count = get_repack_data()
                     yield self.factory.hookrpc_handler.notify(
-                        self.raw_pathname)
+                        self.raw_pathname, loose_object_count, pack_count,
+                        self.factory.hookrpc_handler.auth_params)
                     self.sendPacket(b'ACK %s\n' % self.symbolic_ref_name)
                 except Exception as e:
                     message = str(e)
diff --git a/turnip/pack/helpers.py b/turnip/pack/helpers.py
index d23f12f..d6fd344 100644
--- a/turnip/pack/helpers.py
+++ b/turnip/pack/helpers.py
@@ -12,6 +12,7 @@ import enum
 import hashlib
 import os.path
 import re
+import subprocess
 import stat
 import sys
 from tempfile import (
@@ -204,6 +205,18 @@ def ensure_hooks(repo_root):
                 pass
 
 
+def get_repack_data():
+    output = subprocess.check_output(
+        ['git', 'count-objects', '-v'], universal_newlines=True)
+    if not output:
+        return None, None
+    match = re.search(r'^packs: (.*)', output, flags=re.M)
+    packs = int(match.group(1)) if match else None
+    match = re.search(r'^count: (.*)', output, flags=re.M)
+    objects = int(match.group(1)) if match else None
+    return objects, packs
+
+
 @enum.unique
 class TurnipFaultCode(enum.Enum):
     """An internal vocabulary of possible faults from the virtinfo service."""
diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
index 15f5187..9ae29cb 100644
--- a/turnip/pack/hookrpc.py
+++ b/turnip/pack/hookrpc.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """RPC server for Git hooks.
@@ -229,9 +229,13 @@ class HookRPCHandler(object):
              for ref in paths})
 
     @defer.inlineCallbacks
-    def notify(self, path):
+    def notify(self, path, loose_object_count, pack_count, auth_params):
         proxy = xmlrpc.Proxy(self.virtinfo_url, allowNone=True)
-        yield proxy.callRemote('notify', six.ensure_str(path)).addTimeout(
+        statistics = dict({("loose_object_count", loose_object_count),
+                           ("pack_count", pack_count)})
+        yield proxy.callRemote('notify', six.ensure_str(path),
+                               statistics,
+                               auth_params).addTimeout(
             self.virtinfo_timeout, self.reactor)
 
     @defer.inlineCallbacks
@@ -239,10 +243,14 @@ class HookRPCHandler(object):
         """Notify the virtinfo service about a push."""
         log_context = HookRPCLogContext(self.auth_params[args['key']])
         path = self.ref_paths[args['key']]
+        auth_params = self.auth_params[args['key']]
+        loose_object_count = args.get('loose_object_count')
+        pack_count = args.get('pack_count')
         log_context.log.info(
             "notifyPush request received: ref_path={path}", path=path)
         try:
-            yield self.notify(path)
+            yield self.notify(path, loose_object_count,
+                              pack_count, auth_params)
         except defer.TimeoutError:
             log_context.log.info(
                 "notifyPush timed out: ref_path={path}", path=path)
diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index e0257b2..983a022 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -19,6 +19,7 @@ import sys
 import six
 
 from turnip.compat.files import fd_buffer
+from turnip.pack.helpers import get_repack_data
 
 # XXX twom 2018-10-23 This should be a pygit2 import, but
 # that currently causes CFFI warnings to be returned to the client.
@@ -212,7 +213,12 @@ if __name__ == '__main__':
         # Details of the changes aren't currently included.
         lines = stdin.readlines()
         if lines:
-            rpc_invoke(sock, 'notify_push', {'key': rpc_key})
+            loose_object_count, pack_count = get_repack_data()
+            rpc_invoke(sock,
+                       'notify_push',
+                       {'key': rpc_key,
+                        'loose_object_count': loose_object_count,
+                        'pack_count': pack_count})
         if len(lines) == 1:
             send_mp_url(lines[0])
         sys.exit(0)
diff --git a/turnip/pack/tests/fake_servers.py b/turnip/pack/tests/fake_servers.py
index 92b77b9..458c317 100644
--- a/turnip/pack/tests/fake_servers.py
+++ b/turnip/pack/tests/fake_servers.py
@@ -114,8 +114,9 @@ class FakeVirtInfoService(xmlrpc.XMLRPC):
         self.authentications.append((username, password))
         return {'user': username}
 
-    def xmlrpc_notify(self, path):
-        self.push_notifications.append(path)
+    def xmlrpc_notify(self, path, statistics, auth_params):
+        self.push_notifications.append((path, statistics,
+                                        auth_params))
 
     def xmlrpc_checkRefPermissions(self, path, ref_paths, auth_params):
         self.ref_permissions_checks.append((path, ref_paths, auth_params))
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 02bf771..be972b2 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -731,8 +731,8 @@ class FrontendFunctionalTestMixin(FunctionalTestMixin):
         yield self.assertCommandSuccess(
             (b'git', b'push', b'origin', b'master'), path=clone1)
         self.assertEqual(
-            [six.ensure_text(self.internal_name)],
-            self.virtinfo.push_notifications)
+            six.ensure_text(self.internal_name),
+            self.virtinfo.push_notifications[0][0])
 
     @defer.inlineCallbacks
     def test_unicode_fault(self):
@@ -849,8 +849,14 @@ class TestSmartHTTPFrontendFunctional(FrontendFunctionalTestMixin, TestCase):
         head_target = yield self.get_symbolic_ref(repo, b'HEAD')
         self.assertEqual(b'refs/heads/new-head', head_target)
         self.assertEqual(
-            [six.ensure_text(self.internal_name)],
-            self.virtinfo.push_notifications)
+            six.ensure_text(self.internal_name),
+            self.virtinfo.push_notifications[0][0])
+        self.assertNotEqual(
+            0,
+            self.virtinfo.push_notifications[0][1].get('pack_count'))
+        self.assertNotEqual(
+            0,
+            self.virtinfo.push_notifications[0][1].get('loose_object_count'))
 
     @defer.inlineCallbacks
     def test_turnip_set_symbolic_ref_error(self):
diff --git a/turnip/pack/tests/test_helpers.py b/turnip/pack/tests/test_helpers.py
index bc66977..2ffba15 100644
--- a/turnip/pack/tests/test_helpers.py
+++ b/turnip/pack/tests/test_helpers.py
@@ -16,10 +16,13 @@ import sys
 import tempfile
 from textwrap import dedent
 import time
+import uuid
 
 from fixtures import TempDir
 from pygit2 import (
     Config,
+    GIT_FILEMODE_BLOB,
+    IndexEntry,
     init_repository,
     )
 import six
@@ -33,6 +36,7 @@ from turnip.pack import helpers
 from turnip.pack.helpers import (
     encode_packet,
     get_capabilities_advertisement,
+    get_repack_data,
     )
 import turnip.pack.hooks
 from turnip.version_info import version_info
@@ -414,3 +418,32 @@ class MockStatsd():
 
     def get_client(self):
         return self
+
+
+class TestGetRepackData(TestCase):
+    """Test get repack indicators for repository."""
+
+    def setUp(self):
+        super(TestGetRepackData, self).setUp()
+        self.repo_dir = self.useFixture(TempDir()).path
+        self.repo = init_repository(self.repo_dir, bare=False)
+
+    def test_get_repack_data(self):
+        curdir = os.getcwd()
+        os.chdir(self.repo_dir)
+        # create a test file
+        blob_content = (
+            b'commit ' + 'file content'.encode('ascii') + b' - '
+            + uuid.uuid1().hex.encode('ascii'))
+        test_file = 'test.txt'
+        # stage the changes
+        self.repo.index.add(IndexEntry(
+            test_file, self.repo.create_blob(blob_content),
+            GIT_FILEMODE_BLOB))
+        self.repo.index.write_tree()
+
+        objects, packs = get_repack_data()
+        os.chdir(curdir)
+
+        self.assertIsNotNone(objects)
+        self.assertIsNotNone(packs)
diff --git a/turnip/pack/tests/test_hookrpc.py b/turnip/pack/tests/test_hookrpc.py
index a4b0d19..eea5d08 100644
--- a/turnip/pack/tests/test_hookrpc.py
+++ b/turnip/pack/tests/test_hookrpc.py
@@ -327,15 +327,33 @@ class TestHookRPCHandler(TestCase):
     @defer.inlineCallbacks
     def test_notifyPush(self):
         with self.registeredKey('/translated') as key:
-            yield self.hookrpc_handler.notifyPush(None, {'key': key})
-        self.assertEqual(['/translated'], self.virtinfo.push_notifications)
+            yield self.hookrpc_handler.notifyPush(
+                None,
+                {'key': key, 'loose_object_count': 19, 'pack_count': 7})
+
+        # notify will now return in this format:
+        # ('/translated', {'loose_object_count': 19, 'pack_count': 7}, {})
+        # with the numbers being different of course for each
+        # repository state
+        self.assertEqual('/translated',
+                         self.virtinfo.push_notifications[0][0])
+        self.assertEqual(
+            19,
+            self.virtinfo.push_notifications[0][1].get(
+                'loose_object_count'))
+        self.assertEqual(
+            7,
+            self.virtinfo.push_notifications[0][1].get(
+                'pack_count'))
 
     def test_notifyPush_timeout(self):
         clock = task.Clock()
         self.hookrpc_handler = hookrpc.HookRPCHandler(
             self.virtinfo_url, 15, reactor=clock)
         with self.registeredKey('/translated') as key:
-            d = self.hookrpc_handler.notifyPush(None, {'key': key})
+            d = self.hookrpc_handler.notifyPush(
+                None,
+                {'key': key, 'loose_object_count': 9, 'pack_count': 7})
             clock.advance(1)
             self.assertFalse(d.called)
             clock.advance(15)