launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/turnip:delete-a-ref-is-noisy into turnip:master.

Commit message:
Deleting a ref outputs irrelevant errors.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Revert some checks for error output where there shouldn't be.
Add two functional tests for deleting a ref (with and without permissions).
Check for the ZERO case in a new ref in the hook, pass through to force-push handling.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/turnip:delete-a-ref-is-noisy into turnip:master.

diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index 21bdd59..1594a4b 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -22,6 +22,9 @@ GIT_OID_HEX_ZERO = '0'*40
 
 
 def check_ancestor(old, new):
+    # This is a delete, setting the new ref.
+    if new == GIT_OID_HEX_ZERO:
+        return False
     # https://git-scm.com/docs/git-merge-base#_discussion
     return_code = subprocess.call(
         ['git', 'merge-base', '--is-ancestor', old, new])
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 6de8489..c0058e4 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -412,6 +412,73 @@ class FunctionalTestMixin(object):
             (b'git', b'log', b'--oneline', b'-n', b'1'), path=clone2)
         self.assertIn(b'Add big file', out)
 
+    @defer.inlineCallbacks
+    def test_delete_ref(self):
+        self.virtinfo.ref_permissions = {
+            'refs/heads/newref': ['create', 'push', 'force_push']}
+        test_root = self.useFixture(TempDir()).path
+        clone1 = os.path.join(test_root, 'clone1')
+
+        # Clone the empty repo from the backend and commit to it.
+        yield self.assertCommandSuccess((b'git', b'clone', self.url, clone1))
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.name', b'Test User'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.email', b'test@xxxxxxxxxxx'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'commit', b'--allow-empty', b'-m', b'Committed test'),
+            path=clone1)
+
+        yield self.assertCommandSuccess(
+            (b'git', b'checkout', b'-b', b'newref'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'commit', b'--allow-empty', b'-m', b'Committed test'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'push', b'origin', b'newref'), path=clone1)
+
+        out, err, code = yield utils.getProcessOutputAndValue(
+            b'git', (b'push', b'origin', b':newref'),
+            env=os.environ, path=clone1)
+        self.assertIn(b'[deleted]', err)
+        self.assertEqual(0, code)
+
+    @defer.inlineCallbacks
+    def test_delete_ref_without_permission(self):
+        self.virtinfo.ref_permissions = {
+            'refs/heads/newref': ['create', 'push']}
+        test_root = self.useFixture(TempDir()).path
+        clone1 = os.path.join(test_root, 'clone1')
+
+        # Clone the empty repo from the backend and commit to it.
+        yield self.assertCommandSuccess((b'git', b'clone', self.url, clone1))
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.name', b'Test User'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.email', b'test@xxxxxxxxxxx'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'commit', b'--allow-empty', b'-m', b'Committed test'),
+            path=clone1)
+
+        yield self.assertCommandSuccess(
+            (b'git', b'checkout', b'-b', b'newref'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'commit', b'--allow-empty', b'-m', b'Committed test'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'push', b'origin', b'newref'), path=clone1)
+
+        out, err, code = yield utils.getProcessOutputAndValue(
+            b'git', (b'push', b'origin', b':newref'),
+            env=os.environ, path=clone1)
+        self.assertIn(
+            b'You do not have permission to force-push to refs/heads/newref',
+            err
+            )
+        self.assertEqual(1, code)
+
 
 class TestBackendFunctional(FunctionalTestMixin, TestCase):
 
diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
index d4b42e5..ebf9c80 100644
--- a/turnip/pack/tests/test_hooks.py
+++ b/turnip/pack/tests/test_hooks.py
@@ -128,17 +128,13 @@ class HookTestMixin(object):
     def assertAccepted(self, updates, permissions):
         code, out, err = yield self.invokeHook(
             self.encodeRefs(updates), permissions)
-        # XXX cjwatson 2018-10-22: We should test that `err` is empty too,
-        # but we need pygit2 >= 0.25.1 to silence some cffi warnings.
-        self.assertEqual((0, b''), (code, out))
+        self.assertEqual((0, b'', b''), (code, out, err))
 
     @defer.inlineCallbacks
     def assertRejected(self, updates, permissions, message):
         code, out, err = yield self.invokeHook(
             self.encodeRefs(updates), permissions)
-        # XXX cjwatson 2018-10-22: We should test that `err` is empty too,
-        # but we need pygit2 >= 0.25.1 to silence some cffi warnings.
-        self.assertEqual((1, message), (code, out))
+        self.assertEqual((1, message, b''), (code, out, err))
 
 
 class TestPreReceiveHook(HookTestMixin, TestCase):