Merge ~pappacena/turnip:py3-strings-compat into turnip:master

[Thiago F. Pappacena]

Making some strings compatible both with python2 and python3

This MP should only be merged once we have the new dependencies in place.

It reduces the total amount of failing tests on python3:
From (failures=10, errors=28) to (failures=6, errors=21)

[Colin Watson (cjwatson)]

I'd particularly like to see my comments about get_refs disentangled, but should be fine after that.

review: Approve

[Thiago F. Pappacena (pappacena)]

Pushed the required changes.

[Colin Watson (cjwatson)]

review: Approve

[Thiago F. Pappacena (pappacena)]

Thanks for the review, cjwatson. Pushed the changes. Landing this now.

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/turnip/131/

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/turnip/133/

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/turnip/134/

diff --git a/turnip/api/store.py b/turnip/api/store.py
index 11b4756..934dd71 100644
--- a/turnip/api/store.py
+++ b/turnip/api/store.py
@@ -2,10 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import base64
-from contextlib2 import (
-    contextmanager,
-    ExitStack,
-    )
 import itertools
 import os
 import re
@@ -13,14 +9,18 @@ import shutil
 import subprocess
 import uuid
 
+from contextlib2 import (
+    contextmanager,
+    ExitStack,
+    )
 from pygit2 import (
-    GitError,
     GIT_OBJ_BLOB,
     GIT_OBJ_COMMIT,
-    GIT_OBJ_TREE,
     GIT_OBJ_TAG,
+    GIT_OBJ_TREE,
     GIT_REF_OID,
     GIT_SORT_TOPOLOGICAL,
+    GitError,
     IndexEntry,
     init_repository,
     Oid,
@@ -335,12 +335,14 @@ def get_refs(repo_store, repo_name, exclude_prefixes=None):
     """Return all refs for a git repository."""
     with open_repo(repo_store, repo_name) as repo:
         refs = {}
-        for ref in repo.listall_references():
-            git_object = repo.references[ref].peel()
+        for ref_obj in repo.listall_reference_objects():
             # Filter non-unicode refs, as refs are treated as unicode
             # given json is unable to represent arbitrary byte strings.
             try:
-                ref = ref.decode('utf-8')
+                ref = ref_obj.name
+                if isinstance(ref, bytes):
+                    ref = ref.decode('utf-8')
+                git_object = repo.references[ref].peel()
             except UnicodeDecodeError:
                 pass
             else:
diff --git a/turnip/api/tests/test_api.py b/turnip/api/tests/test_api.py
index 2ff34d0..396369b 100644
--- a/turnip/api/tests/test_api.py
+++ b/turnip/api/tests/test_api.py
@@ -10,16 +10,13 @@ import os
 import subprocess
 from textwrap import dedent
 import unittest
-try:
-    from urllib.parse import quote
-except ImportError:
-    from urllib import quote
 import uuid
 
 from fixtures import (
     EnvironmentVariable,
     TempDir,
     )
+from six.moves.urllib.parse import quote
 from testtools import TestCase
 from testtools.matchers import (
     Equals,
@@ -217,7 +214,7 @@ class ApiTestCase(TestCase):
         """Ensure non-unicode refs are dropped from ref collection."""
         factory = RepoFactory(self.repo_store)
         commit_oid = factory.add_commit('foo', 'foobar.txt')
-        tag = '\xe9\xe9\xe9'  # latin-1
+        tag = b'\xe9\xe9\xe9'  # latin-1
         tag_message = 'tag message'
         factory.add_tag(tag, tag_message, commit_oid)
 
@@ -335,7 +332,7 @@ class ApiTestCase(TestCase):
     def test_repo_diff_non_unicode_commits(self):
         """Ensure non utf-8 chars are handled but stripped from diff."""
         factory = RepoFactory(self.repo_store)
-        message = 'not particularly sensible latin-1: \xe9\xe9\xe9.'
+        message = b'not particularly sensible latin-1: \xe9\xe9\xe9.'
         oid = factory.add_commit(message, 'foo.py')
         oid2 = factory.add_commit('a sensible commit message', 'foo.py', [oid])
 
@@ -406,7 +403,7 @@ class ApiTestCase(TestCase):
             self.repo_path, quote('{}^'.format(c2)), c2)
         resp = self.app.get(path)
         self.assertIn(
-            b'diff --git a/foo.txt b/bar.txt\n', resp.json_body['patch'])
+            'diff --git a/foo.txt b/bar.txt\n', resp.json_body['patch'])
 
     def test_repo_diff_rename_and_change_content_conflict(self):
         # Create repo1 with foo.txt.
@@ -576,7 +573,7 @@ class ApiTestCase(TestCase):
         resp = self.app.get('/repo/{}/compare-merge/{}:{}'.format(
             self.repo_path, c1, c2))
         self.assertIn(
-            b'diff --git a/foo.txt b/bar.txt\n', resp.json_body['patch'])
+            'diff --git a/foo.txt b/bar.txt\n', resp.json_body['patch'])
 
     def test_repo_get_commit(self):
         factory = RepoFactory(self.repo_store)
@@ -671,7 +668,7 @@ class ApiTestCase(TestCase):
     def test_repo_get_non_unicode_log(self):
         """Ensure that non-unicode data is discarded."""
         factory = RepoFactory(self.repo_store)
-        message = '\xe9\xe9\xe9'  # latin-1
+        message = b'\xe9\xe9\xe9'  # latin-1
         oid = factory.add_commit(message, 'foo.py')
         resp = self.app.get('/repo/{}/log/{}'.format(self.repo_path, oid))
         self.assertEqual(message.decode('utf-8', 'replace'),
@@ -809,15 +806,15 @@ class ApiTestCase(TestCase):
         factory.add_commit('b\n', 'dir/file', parents=[c1])
         resp = self.app.get('/repo/{}/blob/dir/file'.format(self.repo_path))
         self.assertEqual(2, resp.json['size'])
-        self.assertEqual('b\n', base64.b64decode(resp.json['data']))
+        self.assertEqual(b'b\n', base64.b64decode(resp.json['data']))
         resp = self.app.get('/repo/{}/blob/dir/file?rev=master'.format(
             self.repo_path))
         self.assertEqual(2, resp.json['size'])
-        self.assertEqual('b\n', base64.b64decode(resp.json['data']))
+        self.assertEqual(b'b\n', base64.b64decode(resp.json['data']))
         resp = self.app.get('/repo/{}/blob/dir/file?rev={}'.format(
             self.repo_path, c1.hex))
         self.assertEqual(2, resp.json['size'])
-        self.assertEqual('a\n', base64.b64decode(resp.json['data']))
+        self.assertEqual(b'a\n', base64.b64decode(resp.json['data']))
 
     def test_repo_blob_missing_commit(self):
         """Trying to get a blob from a non-existent commit returns HTTP 404."""
@@ -861,7 +858,7 @@ class ApiTestCase(TestCase):
         resp = self.app.get('/repo/{}/blob/dir/file?rev=tag-name'.format(
             self.repo_path))
         self.assertEqual(2, resp.json['size'])
-        self.assertEqual('a\n', base64.b64decode(resp.json['data']))
+        self.assertEqual(b'a\n', base64.b64decode(resp.json['data']))
 
     def test_repo_blob_from_non_commit(self):
         """Trying to get a blob from a non-commit returns HTTP 404."""
diff --git a/turnip/api/tests/test_helpers.py b/turnip/api/tests/test_helpers.py
index dac6950..a6035b4 100644
--- a/turnip/api/tests/test_helpers.py
+++ b/turnip/api/tests/test_helpers.py
@@ -4,20 +4,23 @@
 import contextlib
 import fnmatch
 import itertools
+import logging
 import os
+from subprocess import PIPE, Popen, CalledProcessError
 import uuid
 
-from six.moves import urllib
-
 from pygit2 import (
     clone_repository,
-    init_repository,
     GIT_FILEMODE_BLOB,
-    GIT_OBJ_COMMIT,
     IndexEntry,
+    init_repository,
     Repository,
     Signature,
     )
+import six
+from six.moves import urllib
+
+log = logging.getLogger()
 
 
 def get_revlist(repo):
@@ -94,14 +97,34 @@ class RepoFactory(object):
         self.branches.append(branch)
         return branch
 
+    def _get_cmd_line_auth_params(self):
+        return [
+            '-c', 'user.name={}'.format(self.author.name),
+            '-c', 'user.email={}'.format(self.author.email),
+            '-c', 'author.name={}'.format(self.author.name),
+            '-c', 'author.email={}'.format(self.author.email),
+            '-c', 'committer.name={}'.format(self.committer.name),
+            '-c', 'committer.email={}'.format(self.committer.email),
+        ]
+
     def add_tag(self, tag_name, tag_message, oid):
         """Create a tag from tag_name and oid."""
-        repo = self.repo
-        repo.create_tag(tag_name, oid, GIT_OBJ_COMMIT,
-                        self.committer, tag_message)
+        cmd_line = ['git', '-C', self.repo_path]
+        cmd_line += self._get_cmd_line_auth_params()
+        cmd_line += ['tag', '-m', tag_message, tag_name, oid.hex]
+        subproc = Popen(cmd_line, stderr=PIPE, stdout=PIPE)
+        retcode = subproc.wait()
+        if retcode:
+            log.error(
+                "Command %s finished with error code %s. stdout/stderr:\n%s",
+                cmd_line, retcode, subproc.stderr.read())
+            raise CalledProcessError(retcode, cmd_line)
 
     def makeSignature(self, name, email, encoding='utf-8'):
         """Return an author or committer signature."""
+        # email should always be str on python3, but pygit2
+        # doesn't enforce the same for name.
+        email = six.ensure_str(email)
         return Signature(name, email, encoding=encoding)
 
     def stage(self, path, content):
diff --git a/turnip/tests/__init__.py b/turnip/tests/__init__.py
index e69de29..494f3fd 100644
--- a/turnip/tests/__init__.py
+++ b/turnip/tests/__init__.py
@@ -0,0 +1,13 @@
+# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+from turnip.tests.logging import setupLogger
+
+
+setupLogger()
diff --git a/turnip/tests/logging.py b/turnip/tests/logging.py
 new file mode 100644
index 0000000..28b6ef3
--- /dev/null
+++ b/turnip/tests/logging.py
@@ -0,0 +1,16 @@
+# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+import sys
+import logging
+
+
+def setupLogger():
+    """Setup our basic logging for tests."""
+    logging.basicConfig(stream=sys.stdout, level=logging.INFO)