Merge ~racb/usd-importer:changelog-parsing into usd-importer:master

[Robie Basak]

Native changelog parsing, though still with shell fallback if required. Sharing this early; not fully tested. Needs discussion in relation to regression risk.

I have not yet tested "git ubuntu merge" at all. Imports appear to be working, but I have also not tested this fully yet.

[Nish Aravamudan (nacc)]

Superseded/rebased in new merge.

review: Disapprove

[Nish Aravamudan (nacc)]

Supeseded by: https://code.launchpad.net/~nacc/usd-importer/+git/usd-importer/+merge/328607

Unmerged commits

87b2e97... by Robie Basak on 2017-05-23

Refactor changelog parsing

Replace the shell-based symlink workaround with a proper implementation
that uses pygit2 directly. This results in a significant speedup.

get_changelog_versions_from_treeish() previously returned (None, None)
if 'debian/changelog' was not in the supplied tree-ish. This was not
documented and happened implicitly due to the shell fallback behaviour
in parsing debian/changelog. get_heads_and_versions() appears to rely on
this behaviour, for example when asking for versions for the
'debian/pristine-tar' branch. This behaviour is made explicit in the
replacement function that uses the new changelog parsing.

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 345ff81..7ed57aa 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -2,10 +2,10 @@
 ### XXX: is any of this data in lp already?
 
 from copy import copy
-from enum import Enum, unique
-import functools
+from functools import lru_cache
 import logging
 import os
+import posixpath
 import re
 import shutil
 from subprocess import CalledProcessError
@@ -16,6 +16,7 @@ from gitubuntu.run import run, runq, decode_binary
 try:
     pkg = 'python3-debian'
     import debian
+    import debian.changelog
     pkg = 'python3-pygit2'
     import pygit2
 except ImportError:
@@ -23,33 +24,158 @@ except ImportError:
     sys.exit(1)
 
 
-def memoize(obj):
-    cache = obj.cache = {}
+def _follow_symlinks_to_blob(repo, top_tree, search_path, _rel_tree=None, _rel_path=''):
+    '''Recursively follow a path down a tree, following symlinks, to find blob
+
+    repo: pygit2.Repository object
+    top_tree: pygit2.Tree object of the top of the tree structure
+    search_path: '/'-separated path string of blob to find
+    _rel_tree: (internal) which tree to look further into
+    _rel_path: (internal) the path we are in so far
+    '''
+
+    NORMAL_BLOB_MODES = set([
+        pygit2.GIT_FILEMODE_BLOB,
+        pygit2.GIT_FILEMODE_BLOB_EXECUTABLE,
+    ])
+
+    _rel_tree = _rel_tree or top_tree
+    head, tail = posixpath.split(search_path)
+
+    # A traditional functional split would put a single entry in head with tail
+    # empty, but posixpath.split doesn't necessarily do this. Jiggle it round
+    # to make it appear to have traditional semantics.
+    if not head:
+        head = tail
+        tail = None
+
+    entry = _rel_tree[head]
+    if entry.type == 'tree':
+        return _follow_symlinks_to_blob(
+            repo=repo,
+            top_tree=top_tree,
+            search_path=tail,
+            _rel_tree=repo.get(entry.id),
+            _rel_path=posixpath.join(_rel_path, head),
+        )
+    elif entry.type == 'blob' and entry.filemode == pygit2.GIT_FILEMODE_LINK:
+        # Found a symlink. Start again from the top with adjustment for symlink
+        # following
+        search_path = posixpath.normpath(
+            posixpath.join(_rel_path, repo.get(entry.id).data, tail)
+        )
+        return _follow_symlinks_to_blob(
+            repo=repo,
+            top_tree=top_tree,
+            search_path=search_path,
+        )
+    elif entry.type == 'blob' and entry.filemode in NORMAL_BLOB_MODES:
+        return repo.get(entry.id)
+    else:
+        # Found some special entry such as a "gitlink" (submodule entry)
+        raise ValueError(
+            "Found %r filemode %r looking for %r" %
+                (entry, entry.filemode, posixpath.join(_rel_path, search_path))
+        )
+
+
+def follow_symlinks_to_blob(repo, treeish, path):
+    return _follow_symlinks_to_blob(
+        repo=repo,
+        top_tree=treeish.peel(pygit2.Tree),
+        search_path=posixpath.normpath(path),
+    )
+
+
+class Changelog:
+    '''Representation of a debian/changelog file found inside a git tree-ish
+
+    Uses the debian.changelog module from python3-debian for parsing, but when
+    this fails we fall back to grep/sed-based pattern matching automatically.
+    '''
+    def __init__(self, repo, treeish):
+        '''
+        repo: pygit2.Repository instance
+        treeish: pygit2.Object subclass instance (must peel to pygit2.Tree)
+        '''
 
-    @functools.wraps(obj)
-    def memoizer(*args, **kwargs):
-        key = str(args) + str(kwargs)
-        if key not in cache:
-            cache[key] = obj(*args, **kwargs)
+        blob = follow_symlinks_to_blob(
+            repo=repo,
+            treeish=treeish,
+            path='debian/changelog'
+        )
+        self._contents = blob.data
+        self._changelog = debian.changelog.Changelog(self._contents)
+        if not len(self._changelog.versions):
+            # assume bad read, so fall back to shell later
+            self._changelog = None
+
+    @lru_cache()
+    def _shell(self, cmd):
+        cp = run(
+            cmd,
+            input=self._contents,
+            shell=True,
+            env=self._env,
+            quiet=True,
+        )
+        return decode_binary(cp.stdout).strip()
+
+    @property
+    def version(self):
+        if self._changelog:
+            return str(self._changelog.versions[0])
         else:
-            logging.debug("Cache hit on %s %s = %s", str(args),
-                          str(kwargs),
-                          cache[key][:30].replace("\n", "\\n") + "..")
-        return cache[key]
-    return memoizer
+            shell_cmd = 'grep -m1 \'^\S\' | sed \'s/.*(\(.*\)).*/\\1/\''
+            return self._shell(shell_cmd)
 
+    @property
+    def previous_version(self):
+        if self._changelog:
+            return str(self._changelog.versions[1])
+        else:
+            shell_cmd = 'grep -m1 \'^\S\' | tail -1 | sed \'s/.*(\(.*\)).*/\\1/\''
+            return self._shell(shell_cmd)
+
+    @property
+    def maintainer(self):
+        if self._changelog:
+            return self._changelog.author
+        else:
+            shell_cmd = 'grep -m1 \'^ --\' | sed \'s/ -- \(.*\)  \(.*\)/\\1/\''
+            return self._shell(shell_cmd)
+
+    @property
+    def date(self):
+        if self._changelog:
+            return self._changelog.date
+        else:
+            shell_cmd = 'grep -m1 \'^ --\' | sed \'s/ -- \(.*\)  \(.*\)/\\2/\''
+            return self._shell(shell_cmd)
+
+    @property
+    def all_versions(self):
+        if self._changelog:
+            return [str(v) for v in self._changelog.versions]
+        else:
+            shell_cmd = 'grep \'^\S\' | sed \'s/.*(\(.*\)).*/\\1/\''
+            version_lines = self._shell(shell_cmd)
+            return [
+                v_stripped
+                for v_stripped in (
+                    v.strip() for v in version_lines.splitlines()
+                )
+                if v_stripped
+            ]
+
+    @property
+    def distribution(self):
+        if self._changelog:
+            return self._changelog.distributions
+        else:
+            shell_cmd = 'grep -m1 \'^\S\' | sed \'s/.*\ .*\ \(.*\);.*/\\1/\''
+            return self._shell(shell_cmd)
 
-@unique
-class ChangelogField(Enum):
-    """Trivial enum for specifying fields to extract from
-    debian/changelog
-    """
-    version = 1
-    previous_version = 2
-    maintainer = 3
-    date = 4
-    all_versions = 5
-    distribution = 6
 
 def git_dep14_tag(version):
     """Munge a version string according to taken from
@@ -449,143 +575,64 @@ class GitUbuntuRepository:
     def garbage_collect(self):
         self.git_run(['gc'])
 
-    def extract_file_from_treeish(self, treeish, filename, outfile=None):
+    def extract_file_from_treeish(self, treeish, filename):
         """extract a file from @treeish to a local file
 
         Arguments:
         treeish - SHA1 of treeish
         filename - file to extract from @treeish
-        outfile - name of file to copy @filename contents to, will be overwritten.
-                  If None, will be a NamedTemporaryFile.
-        """
-        if outfile is None:
-            outfile = tempfile.NamedTemporaryFile()
-        else:
-            outfile = open(outfile, mode='w+')
 
-        cat_changelog_cmd = (
-            "echo %s:debian/changelog | "
-            "git cat-file --batch --follow-symlinks | "
-            "sed -n '1{/^[^ ]* blob/!{p;q1}};2,$p'"
-            % treeish
+        Returns a NamedTemporaryFile that is flushed but not rewound.
+        """
+        blob = follow_symlinks_to_blob(
+            self._local_repo,
+            treeish=self._local_repo.get(treeish),
+            path=filename,
         )
-
-        try:
-            cp = run(cat_changelog_cmd, shell=True, env=self._env,
-                     stdout=outfile)
-        except CalledProcessError:
-            raise Exception('Unable to extract file')
-
+        outfile = tempfile.NamedTemporaryFile()
+        outfile.write(blob.data)
         outfile.flush()
         return outfile
 
-    @memoize
-    def parse_changelog_field_in_treeish(self, treeish, field):
-        """Parse debian/changelog for specified field, using
-        dpkg-parsechangelog with fallback to shell munging
-
-        dpkg-parsechangelog is preferentially used, however there have
-        been multiple cases where historical changelogs have not been
-        parseable. Fallback to a relatively dumb shell-based parsing for
-        relevant fields in that case. If that also fails, as a last
-        resort, see if any source patches are applicable.
-
-        Arguments:
-        treeish -- SHA1 of treeish
-        field -- ChangelogField value corresponding to field to extract
-
-        Returns:
-        A subprocess.CompletedProcess instance as returned by
-        subprocess.run
-        """
-        # in each case:
-        # parse_params is what is passed on to dpkg-parsechangelog
-        # shell_cmd is a suitable shell sequence to pipe the changelog
-        # through to achieve the same if dpkg-parsechangelog fails
-        if field is ChangelogField.version:
-            parse_params = '-n1 -SVersion'
-            shell_cmd = 'grep -m1 \'^\S\' | sed \'s/.*(\(.*\)).*/\\1/\''
-        elif field is ChangelogField.previous_version:
-            parse_params = '-n1 -o1 -SVersion'
-            shell_cmd = 'grep -m1 \'^\S\' | tail -1 | sed \'s/.*(\(.*\)).*/\\1/\''
-        elif field is ChangelogField.maintainer:
-            parse_params = '-SMaintainer'
-            shell_cmd = 'grep -m1 \'^ --\' | sed \'s/ -- \(.*\)  \(.*\)/\\1/\''
-        elif field is ChangelogField.date:
-            parse_params = '-SDate'
-            shell_cmd = 'grep -m1 \'^ --\' | sed \'s/ -- \(.*\)  \(.*\)/\\2/\''
-        elif field is ChangelogField.all_versions:
-            parse_params = '--format rfc822 -SVersion --all'
-            shell_cmd = 'grep \'^\S\' | sed \'s/.*(\(.*\)).*/\\1/\''
-        elif field is ChangelogField.distribution:
-            parse_params = '-SDistribution'
-            shell_cmd = 'grep -m1 \'^\S\' | sed \'s/.*\ .*\ \(.*\);.*/\\1/\''
-        else:
-            raise Exception('Unknown changelog field %s' % field)
-
-        # We cannot use "git show" since it does not follow symlinks (LP:
-        # #1661092). Instead, use "git cat-file --batch --follow-symlinks" and
-        # parse the batch output (first line) to ensure that we get a blob
-        # rather than a symlink following failure. If we don't get a blob, then
-        # print what we got and exit 1.
-        cat_changelog_cmd = (
-            "echo %s:debian/changelog | "
-            "git cat-file --batch --follow-symlinks | "
-            "sed -n '1{/^[^ ]* blob/!{p;q1}};2,$p'"
-            % treeish
-        )
-
-        try:
-            cp = run(
-                '%s | dpkg-parsechangelog -l- %s' %
-                (cat_changelog_cmd, parse_params),
-                shell=True, env=self._env, quiet=True)
-        except CalledProcessError:
-            try:
-                cp = run(
-                    '%s | %s' % (cat_changelog_cmd, shell_cmd),
-                    shell=True, env=self._env, quiet=True)
-            except CalledProcessError:
-                raise Exception('Unable to parse changelog')
-
-        return decode_binary(cp.stdout).strip()
+    @lru_cache()
+    def get_changelog_from_treeish(self, treeish):
+        return Changelog(self._local_repo, self._local_repo.get(treeish))
 
     def get_changelog_versions_from_treeish(self, treeish):
         """Extract current and prior versions from debian/changelog in a
         given treeish
-        """
-        current_version = None
-        last_version = None
-        if treeish is not None:
-            try:
-                current_version = self.parse_changelog_field_in_treeish(
-                         treeish, ChangelogField.version
-                                                                       )
-                last_version = self.parse_changelog_field_in_treeish(
-                         treeish, ChangelogField.previous_version
-                                                                    )
 
-            except:
-                logging.exception('Cannot get changelog versions')
-                sys.exit(1)
+        Returns (None, None) if the treeish supplied is None or if
+        'debian/changelog' does not exist in the treeish.
+        """
+        if treeish is None:
+            return None, None
 
-        return (current_version, last_version)
+        try:
+            changelog = self.get_changelog_from_treeish(treeish)
+        except KeyError:
+            # If 'debian/changelog' does
+            # not exist, then (None, None) is returned. KeyError propagates up
+            # from Changelog's __init__.
+            return None, None
+        try:
+            return changelog.version, changelog.previous_version
+        except:
+            logging.exception('Cannot get changelog versions')
+            sys.exit(1)
 
     def get_changelog_distribution_from_treeish(self, treeish):
         """Extract targetted distribution from debian/changelog in a
         given treeish
         """
-        distribution = None
-        if treeish is not None:
-            try:
-                distribution = self.parse_changelog_field_in_treeish(
-                         treeish, ChangelogField.distribution
-                                                                    )
-            except:
-                logging.exception('Cannot get changelog distribution')
-                sys.exit(1)
+        if treeish is None:
+            return None
 
-        return distribution
+        try:
+            return self.get_changelog_from_treeish(treeish).distribution
+        except:
+            logging.exception('Cannot get changelog distribution')
+            sys.exit(1)
 
     def get_heads_and_versions(self, head_prefix, namespace):
         """Extract the last version in debian/changelog of all
@@ -680,12 +727,9 @@ class GitUbuntuRepository:
     def get_commit_authorship(self, ref, spi):
         """Extract last debian/changelog entry's maintainer and date"""
         try:
-            author = self.parse_changelog_field_in_treeish(
-                ref, ChangelogField.maintainer
-                                                          )
-            date = self.parse_changelog_field_in_treeish(
-                ref, ChangelogField.date
-                                                        )
+            changelog = self.get_changelog_from_treeish(ref)
+            author = changelog.maintainer
+            date = changelog.date
         except:
             logging.exception('Cannot get commit authorship for %s' % ref)
             sys.exit(1)
@@ -833,19 +877,12 @@ class GitUbuntuRepository:
         return debian.debian_support.version_compare(a, b)
 
     def get_all_changelog_versions_from_treeish(self, treeish):
+        changelog = self.get_changelog_from_treeish(treeish)
         try:
-            lines = self.parse_changelog_field_in_treeish(
-                     treeish, ChangelogField.all_versions
-                                                         )
+            return changelog.all_versions
         except:
             logging.exception('Cannot get all versions from changelog')
             sys.exit(1)
-        versions = list()
-        for version in lines.splitlines():
-            version = version.strip()
-            if len(version) > 0:
-                versions.append(version)
-        return versions
 
     def annotated_tag(self, tag_name, commitish, force, msg=None):
         try: