hooks: added few python3 related fixes to handle bytes vs str on Mercurial hooks

vcsserver/hooks.py

@@ -25,6 +25,8 b' import collections'
 import importlib
 import base64
 import msgpack
+import dataclasses
+import pygit2
 
 from http.client import HTTPConnection
 
@@ -34,7 +36,8 b' import mercurial.node'
 
 from vcsserver.lib.rc_json import json
 from vcsserver import exceptions, subprocessio, settings
-from vcsserver.str_utils import safe_bytes
+from vcsserver.str_utils import ascii_str, safe_str
+from vcsserver.remote.git import Repository
 
 log = logging.getLogger(__name__)
 
@@ -104,7 +107,7 b' class HgMessageWriter(RemoteMessageWrite'
     def __init__(self, ui):
         self.ui = ui
 
-    def write(self, message):
+    def write(self, message: str):
         # TODO: Check why the quiet flag is set by default.
         old = self.ui.quiet
         self.ui.quiet = False
@@ -118,8 +121,8 b' class GitMessageWriter(RemoteMessageWrit'
     def __init__(self, stdout=None):
         self.stdout = stdout or sys.stdout
 
-    def write(self, message):
-        self.stdout.write(safe_bytes(message))
+    def write(self, message: str):
+        self.stdout.write(message)
 
 
 class SvnMessageWriter(RemoteMessageWriter):
@@ -147,8 +150,9 b' def _handle_exception(result):'
     elif exception_class == 'RepositoryError':
         raise exceptions.VcsException()(*result['exception_args'])
     elif exception_class:
-        raise Exception('Got remote exception "%s" with args "%s"' %
-                        (exception_class, result['exception_args']))
+        raise Exception(
+            f"""Got remote exception "{exception_class}" with args "{result['exception_args']}" """
+        )
 
 
 def _get_hooks_client(extras):
@@ -167,7 +171,6 b' def _call_hook(hook_name, extras, writer'
     log.debug('Hooks, using client:%s', hooks_client)
     result = hooks_client(hook_name, extras)
     log.debug('Hooks got result: %s', result)
-
     _handle_exception(result)
     writer.write(result['output'])
 
@@ -198,8 +201,8 b' def _rev_range_hash(repo, node, check_he'
     for rev in range(start, end):
         revs.append(rev)
         ctx = get_ctx(repo, rev)
-        commit_id = mercurial.node.hex(ctx.node())
-        branch = ctx.branch()
+        commit_id = ascii_str(mercurial.node.hex(ctx.node()))
+        branch = safe_str(ctx.branch())
         commits.append((commit_id, branch))
 
     parent_heads = []
@@ -223,8 +226,8 b' def _check_heads(repo, start, end, commi'
     for p in parents:
         branch = get_ctx(repo, p).branch()
         # The heads descending from that parent, on the same branch
-        parent_heads = set([p])
-        reachable = set([p])
+        parent_heads = {p}
+        reachable = {p}
         for x in range(p + 1, end):
             if get_ctx(repo, x).branch() != branch:
                 continue
@@ -301,14 +304,16 b' def pre_push(ui, repo, node=None, **kwar'
     detect_force_push = extras.get('detect_force_push')
 
     rev_data = []
-    if node and kwargs.get('hooktype') == 'pretxnchangegroup':
+    hook_type: str = safe_str(kwargs.get('hooktype'))
+
+    if node and hook_type == 'pretxnchangegroup':
         branches = collections.defaultdict(list)
         commits, _heads = _rev_range_hash(repo, node, check_heads=detect_force_push)
         for commit_id, branch in commits:
             branches[branch].append(commit_id)
 
         for branch, commits in branches.items():
-            old_rev = kwargs.get('node_last') or commits[0]
+            old_rev = ascii_str(kwargs.get('node_last')) or commits[0]
             rev_data.append({
                 'total_commits': len(commits),
                 'old_rev': old_rev,
@@ -325,10 +330,10 b' def pre_push(ui, repo, node=None, **kwar'
                 extras.get('repo_store', ''), extras.get('repository', ''))
             push_ref['hg_env'] = _get_hg_env(
                 old_rev=push_ref['old_rev'],
-                new_rev=push_ref['new_rev'], txnid=kwargs.get('txnid'),
+                new_rev=push_ref['new_rev'], txnid=ascii_str(kwargs.get('txnid')),
                 repo_path=repo_path)
 
-    extras['hook_type'] = kwargs.get('hooktype', 'pre_push')
+    extras['hook_type'] = hook_type or 'pre_push'
     extras['commit_ids'] = rev_data
 
     return _call_hook('pre_push', extras, HgMessageWriter(ui))
@@ -369,6 +374,7 b' def post_push(ui, repo, node, **kwargs):'
     branches = []
     bookmarks = []
     tags = []
+    hook_type: str = safe_str(kwargs.get('hooktype'))
 
     commits, _heads = _rev_range_hash(repo, node)
     for commit_id, branch in commits:
@@ -376,11 +382,12 b' def post_push(ui, repo, node, **kwargs):'
         if branch not in branches:
             branches.append(branch)
 
-    if hasattr(ui, '_rc_pushkey_branches'):
-        bookmarks = ui._rc_pushkey_branches
+    if hasattr(ui, '_rc_pushkey_bookmarks'):
+        bookmarks = ui._rc_pushkey_bookmarks
 
-    extras['hook_type'] = kwargs.get('hooktype', 'post_push')
+    extras['hook_type'] = hook_type or 'post_push'
     extras['commit_ids'] = commit_ids
+
     extras['new_refs'] = {
         'branches': branches,
         'bookmarks': bookmarks,
@@ -401,9 +408,10 b' def post_push_ssh(ui, repo, node, **kwar'
 
 def key_push(ui, repo, **kwargs):
     from vcsserver.hgcompat import get_ctx
-    if kwargs['new'] != '0' and kwargs['namespace'] == 'bookmarks':
+
+    if kwargs['new'] != b'0' and kwargs['namespace'] == b'bookmarks':
         # store new bookmarks in our UI object propagated later to post_push
-        ui._rc_pushkey_branches = get_ctx(repo, kwargs['key']).bookmarks()
+        ui._rc_pushkey_bookmarks = get_ctx(repo, kwargs['key']).bookmarks()
     return
 
 
@@ -432,10 +440,13 b' def handle_git_post_receive(unused_repo_'
     pass
 
 
-HookResponse = collections.namedtuple('HookResponse', ('status', 'output'))
[e-mail]
+class HookResponse:
+    status: int
+    output: str
 
 
-def git_pre_pull(extras):
+def git_pre_pull(extras) -> HookResponse:
     """
     Pre pull hook.
 
@@ -449,19 +460,19 b' def git_pre_pull(extras):'
     if 'pull' not in extras['hooks']:
         return HookResponse(0, '')
 
-    stdout = io.BytesIO()
+    stdout = io.StringIO()
     try:
-        status = _call_hook('pre_pull', extras, GitMessageWriter(stdout))
+        status_code = _call_hook('pre_pull', extras, GitMessageWriter(stdout))
 
     except Exception as error:
         log.exception('Failed to call pre_pull hook')
-        status = 128
-        stdout.write(safe_bytes(f'ERROR: {error}\n'))
+        status_code = 128
+        stdout.write(f'ERROR: {error}\n')
 
-    return HookResponse(status, stdout.getvalue())
+    return HookResponse(status_code, stdout.getvalue())
 
 
-def git_post_pull(extras):
+def git_post_pull(extras) -> HookResponse:
     """
     Post pull hook.
 
@@ -474,12 +485,12 b' def git_post_pull(extras):'
     if 'pull' not in extras['hooks']:
         return HookResponse(0, '')
 
-    stdout = io.BytesIO()
+    stdout = io.StringIO()
     try:
         status = _call_hook('post_pull', extras, GitMessageWriter(stdout))
     except Exception as error:
         status = 128
-        stdout.write(safe_bytes(f'ERROR: {error}\n'))
+        stdout.write(f'ERROR: {error}\n')
 
     return HookResponse(status, stdout.getvalue())
 
@@ -504,15 +515,11 b' def _parse_git_ref_lines(revision_lines)'
     return rev_data
 
 
-def git_pre_receive(unused_repo_path, revision_lines, env):
+def git_pre_receive(unused_repo_path, revision_lines, env) -> int:
     """
     Pre push hook.
 
-    :param extras: dictionary containing the keys defined in simplevcs
-    :type extras: dict
-
     :return: status code of the hook. 0 for success.
-    :rtype: int
     """
     extras = json.loads(env['RC_SCM_DATA'])
     rev_data = _parse_git_ref_lines(revision_lines)
@@ -545,18 +552,18 b' def git_pre_receive(unused_repo_path, re'
 
     extras['hook_type'] = 'pre_receive'
     extras['commit_ids'] = rev_data
-    return _call_hook('pre_push', extras, GitMessageWriter())
+
+    stdout = sys.stdout
+    status_code = _call_hook('pre_push', extras, GitMessageWriter(stdout))
+
+    return status_code
 
 
-def git_post_receive(unused_repo_path, revision_lines, env):
+def git_post_receive(unused_repo_path, revision_lines, env) -> int:
     """
     Post push hook.
 
-    :param extras: dictionary containing the keys defined in simplevcs
-    :type extras: dict
-
     :return: status code of the hook. 0 for success.
-    :rtype: int
     """
     extras = json.loads(env['RC_SCM_DATA'])
     if 'push' not in extras['hooks']:
@@ -576,26 +583,28 b' def git_post_receive(unused_repo_path, r'
         type_ = push_ref['type']
 
         if type_ == 'heads':
+            # starting new branch case
             if push_ref['old_rev'] == empty_commit_id:
-                # starting new branch case
-                if push_ref['name'] not in branches:
-                    branches.append(push_ref['name'])
+                push_ref_name = push_ref['name']
 
-                # Fix up head revision if needed
-                cmd = [settings.GIT_EXECUTABLE, 'show', 'HEAD']
+                if push_ref_name not in branches:
+                    branches.append(push_ref_name)
+
+                need_head_set = ''
+                with Repository(os.getcwd()) as repo:
                 try:
-                    subprocessio.run_command(cmd, env=os.environ.copy())
-                except Exception:
-                    push_ref_name = push_ref['name']
-                    cmd = [settings.GIT_EXECUTABLE, 'symbolic-ref', '"HEAD"', f'"refs/heads/{push_ref_name}"']
+                        repo.head
+                    except pygit2.GitError:
+                        need_head_set = f'refs/heads/{push_ref_name}'
+
+                    if need_head_set:
+                        repo.set_head(need_head_set)
                     print(f"Setting default branch to {push_ref_name}")
-                    subprocessio.run_command(cmd, env=os.environ.copy())
 
-                cmd = [settings.GIT_EXECUTABLE, 'for-each-ref',
-                       '--format=%(refname)', 'refs/heads/*']
+                cmd = [settings.GIT_EXECUTABLE, 'for-each-ref', '--format=%(refname)', 'refs/heads/*']
                 stdout, stderr = subprocessio.run_command(
                     cmd, env=os.environ.copy())
-                heads = stdout
+                heads = safe_str(stdout)
                 heads = heads.replace(push_ref['ref'], '')
                 heads = ' '.join(head for head
                                  in heads.splitlines() if head) or '.'
@@ -604,9 +613,10 b' def git_post_receive(unused_repo_path, r'
                        '--not', heads]
                 stdout, stderr = subprocessio.run_command(
                     cmd, env=os.environ.copy())
-                git_revs.extend(stdout.splitlines())
+                git_revs.extend(list(map(ascii_str, stdout.splitlines())))
+
+            # delete branch case
             elif push_ref['new_rev'] == empty_commit_id:
-                # delete branch case
                 git_revs.append('delete_branch=>%s' % push_ref['name'])
             else:
                 if push_ref['name'] not in branches:
@@ -617,7 +627,25 b' def git_post_receive(unused_repo_path, r'
                        '--reverse', '--pretty=format:%H']
                 stdout, stderr = subprocessio.run_command(
                     cmd, env=os.environ.copy())
-                git_revs.extend(stdout.splitlines())
+                # we get bytes from stdout, we need str to be consistent
+                log_revs = list(map(ascii_str, stdout.splitlines()))
+                git_revs.extend(log_revs)
+
+                # Pure pygit2 impl. but still 2-3x slower :/
+                # results = []
+                #
+                # with Repository(os.getcwd()) as repo:
+                #     repo_new_rev = repo[push_ref['new_rev']]
+                #     repo_old_rev = repo[push_ref['old_rev']]
+                #     walker = repo.walk(repo_new_rev.id, pygit2.GIT_SORT_TOPOLOGICAL)
+                #
+                #     for commit in walker:
+                #         if commit.id == repo_old_rev.id:
+                #             break
+                #         results.append(commit.id.hex)
+                #     # reverse the order, can't use GIT_SORT_REVERSE
+                #     log_revs = results[::-1]
+
         elif type_ == 'tags':
             if push_ref['name'] not in tags:
                 tags.append(push_ref['name'])
@@ -631,13 +659,16 b' def git_post_receive(unused_repo_path, r'
         'tags': tags,
     }
 
+    stdout = sys.stdout
+
     if 'repo_size' in extras['hooks']:
         try:
-            _call_hook('repo_size', extras, GitMessageWriter())
+            _call_hook('repo_size', extras, GitMessageWriter(stdout))
         except Exception:
             pass
 
-    return _call_hook('post_push', extras, GitMessageWriter())
+    status_code = _call_hook('post_push', extras, GitMessageWriter(stdout))
+    return status_code
 
 
 def _get_extras_from_txn_id(path, txn_id):

vcsserver/pygrack.py

@@ -336,8 +336,9 b' class GitRepository(object):'
         pre_pull_messages = ''
         # Upload-pack == clone
         if git_command == 'git-upload-pack':
-            status, pre_pull_messages = hooks.git_pre_pull(self.extras)
-            if status != 0:
+            hook_response = hooks.git_pre_pull(self.extras)
+            if hook_response.status != 0:
+                pre_pull_messages = hook_response.output
                 resp.app_iter = self._build_failed_pre_pull_response(
                     capabilities, pre_pull_messages)
                 return resp
@@ -385,8 +386,8 b' class GitRepository(object):'
 
         # Upload-pack == clone
         if git_command == 'git-upload-pack':
-            unused_status, post_pull_messages = hooks.git_post_pull(self.extras)
-
+            hook_response = hooks.git_post_pull(self.extras)
+            post_pull_messages = hook_response.output
             resp.app_iter = self._build_post_pull_response(out, capabilities, pre_pull_messages, post_pull_messages)
         else:
             resp.app_iter = out

vcsserver/remote/git.py

@@ -22,8 +22,9 b' import posixpath as vcspath'
 import re
 import stat
 import traceback
-import urllib.request, urllib.parse, urllib.error
-import urllib.request, urllib.error, urllib.parse
+import urllib.request
+import urllib.parse
+import urllib.error
 from functools import wraps
 
 import more_itertools
@@ -40,7 +41,7 b' from dulwich.repo import Repo as Dulwich'
 from dulwich.server import update_server_info
 
 from vcsserver import exceptions, settings, subprocessio
-from vcsserver.str_utils import safe_str, safe_int, safe_bytes, ascii_str, ascii_bytes
+from vcsserver.str_utils import safe_str, safe_int, safe_bytes, ascii_bytes
 from vcsserver.base import RepoFactory, obfuscate_qs, ArchiveNode, archive_repo, BinaryEnvelope
 from vcsserver.hgcompat import (
     hg_url as url_parser, httpbasicauthhandler, httpdigestauthhandler)
@@ -69,7 +70,7 b' def reraise_safe_exceptions(func):'
         except (HangupException, UnexpectedCommandError) as e:
             exc = exceptions.VcsException(org_exc=e)
             raise exc(safe_str(e))
-        except Exception as e:
+        except Exception:
             # NOTE(marcink): because of how dulwich handles some exceptions
             # (KeyError on empty repos), we cannot track this and catch all
             # exceptions, it's an exceptions from other handlers
@@ -107,7 +108,7 b' class GitFactory(RepoFactory):'
 
     def _create_repo(self, wire, create, use_libgit2=False):
         if use_libgit2:
-            return Repository(safe_bytes(wire['path']))
+            repo = Repository(safe_bytes(wire['path']))
         else:
             # dulwich mode
             repo_path = safe_str(wire['path'], to_encoding=settings.WIRE_ENCODING)