launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/turnip:use-subprocess-for-ancestor-force-push into turnip:master.

Commit message:
Use a subprocess git command to check for force push, rather than pygit2

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/turnip/+git/turnip/+merge/357675

pygit2 currently causes CFFI warnings to be output to the git client.
Work around this by using --is-ancestor from https://git-scm.com/docs/git-merge-base to avoid the hook having to import pygit2.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/turnip:use-subprocess-for-ancestor-force-push into turnip:master.

diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
index c20f930..a94eddf 100644
--- a/turnip/pack/hookrpc.py
+++ b/turnip/pack/hookrpc.py
@@ -20,7 +20,6 @@ from __future__ import (
     unicode_literals,
     )
 
-from collections import defaultdict
 import json
 import sys
 
diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index 5901b97..21bdd59 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -12,13 +12,20 @@ from __future__ import (
 import json
 import os
 import socket
+import subprocess
 import sys
 
-import pygit2
 
+# XXX twom 2018-10-23 This should be a pygit2 import, but
+# that currently causes CFFI warnings to be returned to the client.
+GIT_OID_HEX_ZERO = '0'*40
 
-def get_repo():
-    return pygit2.Repository('.')
+
+def check_ancestor(old, new):
+    # https://git-scm.com/docs/git-merge-base#_discussion
+    return_code = subprocess.call(
+        ['git', 'merge-base', '--is-ancestor', old, new])
+    return return_code == 0
 
 
 def determine_permissions_outcome(old, ref, rule_lines):
@@ -27,7 +34,7 @@ def determine_permissions_outcome(old, ref, rule_lines):
     if 'force_push' in rule:
         return
     # We are creating a new ref
-    if old == pygit2.GIT_OID_HEX_ZERO:
+    if old == GIT_OID_HEX_ZERO:
         if 'create' in rule:
             return
         else:
@@ -66,17 +73,13 @@ def match_update_rules(rule_lines, ref_line):
     the rule_lines to confirm that the user has permissions for that operation.
     """
     ref, old, new = ref_line
-    repo = get_repo()
 
     # If it's a create, the old ref doesn't exist
-    if old == pygit2.GIT_OID_HEX_ZERO:
+    if old == GIT_OID_HEX_ZERO:
         return []
 
-    # If there's no common ancestor, or the common ancestor is something
-    # other than the old commit, then this is a non-fast-forward push.
-    base = repo.merge_base(old, new)
-    if base and base.hex == old:
-        # This is a fast-forwardable merge
+    # https://git-scm.com/docs/git-merge-base#_discussion
+    if check_ancestor(old, new):
         return []
 
     # If it's not fast forwardable, check that user has permissions
diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
index e3286fe..093659e 100644
--- a/turnip/pack/tests/test_hooks.py
+++ b/turnip/pack/tests/test_hooks.py
@@ -22,7 +22,6 @@ from twisted.internet import (
 
 from turnip.pack import hookrpc
 from turnip.pack.helpers import ensure_hooks
-import turnip.pack.hooks
 from turnip.pack.hooks import hook
 
 
@@ -196,32 +195,38 @@ class TestPostReceiveHook(HookTestMixin, TestCase):
 class TestUpdateHook(TestCase):
     """Tests for the git update hook"""
 
-    def patch_repo(self, ancestor):
-        hook.get_repo = lambda: MockRepo(ancestor)
+    def setUp(self):
+        super(TestUpdateHook, self).setUp()
+        self.patch_check_ancestor()
+
+    def patch_check_ancestor(self):
+        def check(old, new):
+            # Avoid a subprocess call to execute on a git repository
+            # that we haven't created.
+            if old == 'old':
+                return False
+            return True
+        hook.check_ancestor = check
 
     def test_create(self):
         # Creation is determined by an all 0 base sha
-        self.patch_repo('')
         self.assertEqual(
             [], hook.match_update_rules(
                 [], ['ref', pygit2.GIT_OID_HEX_ZERO, 'new']))
 
     def test_fast_forward(self):
         # If the old sha is a merge ancestor of the new
-        self.patch_repo('somehex')
         self.assertEqual(
             [], hook.match_update_rules([], ['ref', 'somehex', 'new']))
 
     def test_rules_fall_through(self):
         # The default is to deny
-        self.patch_repo('somehex')
         output = hook.match_update_rules({}, ['ref', 'old', 'new'])
         self.assertEqual(
             [b'You do not have permission to force-push to ref.'], output)
 
     def test_no_matching_ref(self):
         # No matches means deny by default
-        self.patch_repo('somehex')
         output = hook.match_update_rules(
             {'notamatch': []},
             ['ref', 'old', 'new'])
@@ -230,7 +235,6 @@ class TestUpdateHook(TestCase):
 
     def test_matching_ref(self):
         # Permission given to force-push
-        self.patch_repo('somehex')
         output = hook.match_update_rules(
             {'ref': ['force_push']},
             ['ref', 'old', 'new'])
@@ -238,7 +242,6 @@ class TestUpdateHook(TestCase):
 
     def test_no_permission(self):
         # User does not have permission to force-push
-        self.patch_repo('somehex')
         output = hook.match_update_rules(
             {'ref': ['create']},
             ['ref', 'old', 'new'])