Merge ~racb/git-ubuntu:commit-authorship into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

Unfortunately the final commit of this branch is quite large. I don't see any easy way of breaking it down since it is all a necessary consequence of the method rewrite, and doing it in smaller pieces would break the other pieces. It doesn't seem worth attempting to incrementally transform the old method first.

There isn't a test for the bug being fixed here because we're removing the broken attribute fetch entirely. It doesn't seem worth injecting a fake Launchpad object to ensure that the attribute isn't being fetched, as there's nothing that uses that attribute in the code base any more.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:887e294f7d0bdfd5e6cec9364a1f74dbdbef864f
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/430/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    FAILED: Unit Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/430//rebuild

review: Needs Fixing (continuous-integration)

[Bryce Harrington (bryce)]

The test failure is because the patch changes the import_unapplied_dsc() signature to drop the fallback_author and fallback_date parameters, and importlocal.py has not been updated. It looks like you can just drop these lines from the call.

I notice fallback_author also appears in build.py and queue.py (which I guess don't show up as test failures simply due to lack of tests), so those will need updated too.

Given the above, would probably be worthwhile to smoke test the CLI before landing it by manually running the commands to do a dummy merge / build / lint / etc.

The code itself looks good. There's some test cases without pydocs but they're straightforward enough not to need them, with the exception that mention should be made in the test cases where stuff from the changelog_tests/ directory are being used. See inline comments for this and a few miscellany.

review: Needs Fixing

[Robie Basak (racb)]

Thanks for the review!

I have fixed and smoke tested build-source (since this is based prior to your collapse into build -S, but the same code is used in both cases), import-ppa and queue. import-local fails, but it failed before for an unrelated reason, so my adjustment to import-local is best-effort only. These fixes are in commit 9642080. Further comments inline.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:9642080cd7779d20ec888fca9cd62cbf18c7b3c0
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/435/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/435//rebuild

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

Looks good.

I've rerun the tests locally (on the eoan lxc container with the StopIteration bug), and looks like the issues have been resolved.

Launchpad appears to have lost the in-line comments (or at least, it's not obvious to me where they've gone), but spot checking the issues I remember flagging before they appear to all be resolved now.

review: Approve

[Robie Basak (racb)]

I have squashed, rebased on master, resolved one merge conflict and force pushed for a final CI check before merging.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:90560ab5cbace79d7a97d46ee447210f1b768c90
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/439/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/439//rebuild

review: Approve (continuous-integration)

diff --git a/gitubuntu/build.py b/gitubuntu/build.py
index f31a2eb..290427b 100644
--- a/gitubuntu/build.py
+++ b/gitubuntu/build.py
@@ -841,13 +841,11 @@ def do_build(
 
         # Did we do a quiltify/changelogify fixup?
         if commitish_tree_hash != tree_hash:
-            fixup_commit_hash = repo.commit_tree_hash(
-                tree_hash=tree_hash,
-                parents=[commitish],
+            fixup_commit_hash = str(repo.commit_source_tree(
+                tree=pygit2.Oid(hex=tree_hash),
+                parents=[pygit2.Oid(hex=commitish)],
                 log_message=b'Automatically generated git-ubuntu fixup.',
-                fallback_author=None,
-                fallback_date=None,
-            )
+            ))
             logging.info(
 """We automatically generated fixup changes relative to
 %s as commit %s.
diff --git a/gitubuntu/changelog_tests/maintainer_name_inner_space b/gitubuntu/changelog_tests/maintainer_name_inner_space
new file mode 100644
index 0000000..0f0b538
--- /dev/null
+++ b/gitubuntu/changelog_tests/maintainer_name_inner_space
@@ -0,0 +1,5 @@
+testpkg (1.0) xenial; urgency=medium
+
+  * Dummy entry.
+
+ -- Test  Maintainer <[e-mail]>  Thu, 01 Jan 1970 00:00:00 +0000
diff --git a/gitubuntu/changelog_tests/maintainer_name_leading_space b/gitubuntu/changelog_tests/maintainer_name_leading_space
new file mode 100644
index 0000000..38b507e
--- /dev/null
+++ b/gitubuntu/changelog_tests/maintainer_name_leading_space
@@ -0,0 +1,5 @@
+testpkg (1.0) xenial; urgency=medium
+
+  * Dummy entry.
+
+ --  Test Maintainer <[e-mail]>  Thu, 01 Jan 1970 00:00:00 +0000
diff --git a/gitubuntu/changelog_tests/maintainer_name_trailing_space b/gitubuntu/changelog_tests/maintainer_name_trailing_space
new file mode 100644
index 0000000..36a3407
--- /dev/null
+++ b/gitubuntu/changelog_tests/maintainer_name_trailing_space
@@ -0,0 +1,5 @@
+testpkg (1.0) xenial; urgency=medium
+
+  * Dummy entry.
+
+ -- Test Maintainer  <[e-mail]>  Thu, 01 Jan 1970 00:00:00 +0000
diff --git a/gitubuntu/changelog_tests/test_maintainer_1 b/gitubuntu/changelog_tests/test_maintainer_1
new file mode 100644
index 0000000..abb45c9
--- /dev/null
+++ b/gitubuntu/changelog_tests/test_maintainer_1
@@ -0,0 +1,5 @@
+testpkg (1.0) xenial; urgency=medium
+
+  * Dummy entry.
+
+ -- Test Maintainer <[e-mail]>  Thu, 01 Jan 1970 00:00:00 +0000
diff --git a/gitubuntu/changelog_tests/test_maintainer_2 b/gitubuntu/changelog_tests/test_maintainer_2
new file mode 100644
index 0000000..921d4e8
--- /dev/null
+++ b/gitubuntu/changelog_tests/test_maintainer_2
@@ -0,0 +1,5 @@
+testpkg (1.0) xenial; urgency=medium
+
+  * Dummy entry.
+
+ --  <[e-mail]>  Thu, 01 Jan 1970 00:00:00 +0000
diff --git a/gitubuntu/changelog_tests/test_maintainer_3 b/gitubuntu/changelog_tests/test_maintainer_3
new file mode 100644
index 0000000..181aa6e
--- /dev/null
+++ b/gitubuntu/changelog_tests/test_maintainer_3
@@ -0,0 +1,5 @@
+testpkg (1.0) xenial; urgency=medium
+
+  * Dummy entry.
+
+ -- <[e-mail]>  Thu, 01 Jan 1970 00:00:00 +0000
diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 20c9b03..bce4573 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -4,6 +4,7 @@
 import collections
 from contextlib import contextmanager
 from copy import copy
+import datetime
 import enum
 from functools import lru_cache
 import itertools
@@ -28,6 +29,7 @@ from gitubuntu.run import (
    run_gbp,
    run_quilt,
 )
+import gitubuntu.spec
 from gitubuntu.test_util import get_test_changelog
 import gitubuntu.versioning
 try:
@@ -574,8 +576,11 @@ class Changelog:
                     'Old (%s) and new (%s) changelog values do not agree' %
                     (self._shell_maintainer, ret)
                 )
-            return ret
-        return self._shell_maintainer
+        else:
+            ret = self._shell_maintainer
+        if not ret:
+            raise ValueError("Unable to parse maintainer from changelog")
+        return ret
 
     @property
     def _shell_date(self):
@@ -677,6 +682,33 @@ class Changelog:
             return ret
         return self._shell_srcpkg
 
+    @property
+    def git_authorship(self):
+        """Extract last changelog entry's maintainer and timestamp
+
+        Parse the first changelog entry's sign-off line into git's commit
+        authorship metadata model according to the import specification.
+
+        :rtype: tuple(str, str, int, int)
+        :returns: tuple of name, email, time (in seconds since epoch) and
+            offset from UTC (in minutes)
+        :raises ValueError: if the changelog sign-off line cannot be parsed
+        """
+        m = re.match(r'(?P<name>.*)<(?P<email>.*)>', self.maintainer)
+        if m is None:
+            raise ValueError('Cannot get authorship')
+
+        time_tuple = time.strptime(self.date, '%a, %d %b %Y %H:%M:%S %z')
+        epoch_seconds = int(time.mktime(time_tuple))
+        tz_offset = (time_tuple.tm_gmtoff or 0) // 60  # seconds -> minutes
+
+        return (
+            m.group('name').strip(),
+            m.group('email'),
+            epoch_seconds,
+            tz_offset,
+        )
+
 
 class GitUbuntuChangelogError(Exception):
     pass
@@ -1763,102 +1795,6 @@ class GitUbuntuRepository:
         _, _, pretty_name = tag.name.partition('refs/tags/')
         return pretty_name
 
-    def get_commit_authorship(
-        self,
-        ref,
-        fallback_author,
-        fallback_date,
-    ):
-        """Extract last debian/changelog entry's maintainer and date
-
-        :param ref str Reference name to lookup the changelog of
-        :param fallback_author str Name to use as commit author if
-             @ref's debian/changelog fails to parse
-        :param fallback_date str Date to use as commit date if @ref's
-             debian/changelog fails to parse
-
-        :rtype tuple(str, str, str)
-        :returns Tuple of name, email and date
-        """
-        try:
-            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)
-
-        m = re.match(r'(?P<name>.*)<(?P<email>.*)>', author)
-        name = ''
-        email = ''
-        if m is None:
-            logging.error('Cannot get commit authorship for %s' % ref)
-            sys.exit(1)
-
-        name = m.group('name')
-        email = m.group('email')
-
-        # fallback to Launchpad's information for the name
-        # cannot do this for the email, as it requires you are logged in
-        if len(name) == 0 and fallback_author:
-            name = fallback_author
-
-        if len(name) == 0 or len(email) == 0:
-            logging.error('Cannot get commit authorship for %s' % ref)
-            sys.exit(1)
-
-        try:
-            # check that date is parseable by git-commit
-            time.strptime(date, '%a, %d %b %Y %H:%M:%S %z')
-        except ValueError:
-            try:
-                # date_published can be None
-                if fallback_date:
-                    date = fallback_date
-            except:
-                logging.error('No LP source package publishing date '
-                              'available. Unable to proceed.'
-                             )
-                sys.exit(1)
-
-        return (name, email, date)
-
-    def get_commit_environment(self, ref, fallback_author, fallback_date):
-        """Get a suitable environment hash to force Git commits to match debian/changelog
-
-        :param ref str Reference name to lookup the changelog of
-        :param fallback_author str Name to use as the commit author if
-             @ref's debian/changelog fails to parse
-        :param fallback_date str Date to use as the commit date if
-             @ref's debian/changelog fails to parse
-
-        :rtype dict
-        :returns A hash suitable for passing as the environment to
-             subprocess.* containing GIT_* variables that will force a
-             commit to match values from debian/changelog.
-        """
-        author_name, author_email, author_date = self.get_commit_authorship(
-            ref,
-            fallback_author,
-            fallback_date,
-        )
-        # committer date is the published date if present
-        # if it is not, fall back to the authorship date
-        if fallback_date:
-            committer_date = fallback_date
-        else:
-            # XXX: is this a good fallback?
-            committer_date = author_date
-
-        return {
-            'GIT_AUTHOR_NAME': author_name,
-            'GIT_AUTHOR_EMAIL': author_email,
-            'GIT_AUTHOR_DATE': author_date,
-            'GIT_COMMITTER_NAME': 'usd-importer',
-            'GIT_COMMITTER_EMAIL': '[e-mail]',
-            'GIT_COMMITTER_DATE': committer_date,
-        }
-
     def create_tracking_branch(self, branch_name, upstream_name, force=False):
         return self.raw_repo.create_branch(
             branch_name,
@@ -1930,51 +1866,60 @@ class GitUbuntuRepository:
                          )
             raise
 
-    def commit_tree_hash(
+    def commit_source_tree(
         self,
-        tree_hash,
+        tree,
         parents,
         log_message,
-        fallback_author,
-        fallback_date,
+        commit_date=None,
     ):
-        """Commit a Git tree with appropriate parents and message
-
-        :param tree_hash str Git tree hash
-        :param parents list(str) Git commit hashes of parents
-        :param log_message bytes Commit message
-        :param fallback_author str Git commit author to use if
-             @tree_hash's debian/changelog does not parse
-        :param fallback_date str Git commit date to use if
-             @tree_hash's debian/changelog does not parse
+        """Commit a git tree with appropriate parents and message
+
+        Given a git tree that contains a source package, create a matching
+        commit using metadata derived from the tree as required according to
+        the import specification.
+
+        Commit metadata elements that are not specified as derived from the
+        tree itself are required as parameters.
+
+        :param pygit2.Oid tree: reference to the git tree in this repository
+            that contains a debian/changelog file
+        :param list(pygit2.Oid) parents: parent commits of the commit to be
+            created
+        :param bytes log_message: commit message
+        :param datetime.datetime commit_date: the commit date to use (any
+            sub-second part of the timestamp is truncated). If None, use the
+            current date.
+        :returns: reference to the created commit
+        :rtype: pygit2.Oid
         """
-        commit_tree = ['git', 'commit-tree', '--no-gpg-sign', tree_hash]
-
-        for parent in parents:
-            commit_tree.extend(['-p', parent])
-
-        commit_env = dict(self.env)  # take a copy for modification
-        if fallback_author or fallback_date:
-            commit_env.update(
-                self.get_commit_environment(
-                    tree_hash,
-                    fallback_author,
-                    fallback_date,
-                )
-            )
-        with tempfile.NamedTemporaryFile() as fp:
-            fp.write(log_message)
-            fp.flush()
-            commit_tree += ['-F', fp.name]
-            try:
-                 stdout, _ = run(commit_tree, env=commit_env)
-                 return stdout.strip()
-            except CalledProcessError:
-                 _, t = tempfile.mkstemp()
-                 shutil.copy(fp.name, t)
-                 logging.exception("Unable to commit tree, saved"
-                                   "temp commit message as %s", t)
-                 sys.exit(1)
+        if commit_date is None:
+            commit_date = datetime.datetime.now()
+
+        changelog = self.get_changelog_from_treeish(str(tree))
+
+        # Divide by 60 for seconds -> minutes
+        commit_date_offset_td = commit_date.utcoffset()
+        commit_date_offset_mins = (
+            int(commit_date_offset_td.total_seconds()) // 60
+            if commit_date_offset_td
+            else 0
+        )
+
+        return self.raw_repo.create_commit(
+            None,  # ref: do not update any ref
+            pygit2.Signature(*changelog.git_authorship),  # author
+            pygit2.Signature(  # committer
+                name=gitubuntu.spec.SYNTHESIZED_COMMITTER_NAME,
+                email=gitubuntu.spec.SYNTHESIZED_COMMITTER_EMAIL,
+                time=int(commit_date.timestamp()),
+                offset=commit_date_offset_mins,
+            ),
+            log_message,  # message
+            tree,  # tree
+            parents,  # parents
+        )
+
 
     @classmethod
     def _create_replacement_tree_builder(cls, repo, treeish, sub_path):
diff --git a/gitubuntu/git_repository_test.py b/gitubuntu/git_repository_test.py
index 01d3525..5c3f95a 100644
--- a/gitubuntu/git_repository_test.py
+++ b/gitubuntu/git_repository_test.py
@@ -1,6 +1,8 @@
 import copy
+import datetime
 import itertools
 import os
+import pkg_resources
 import shutil
 import tempfile
 import unittest
@@ -20,6 +22,7 @@ from gitubuntu.repo_builder import (
     Tree,
 )
 from gitubuntu.source_builder import Source, SourceSpec
+import gitubuntu.spec
 from gitubuntu.test_fixtures import (
     repo,
     pygit2_repo,
@@ -58,6 +61,84 @@ def test_changelog_date():
     assert test_changelog.date == 'Mon, 12 May 2016 08:14:34 -0700'
 
 
[e-mail]('changelog_name, expected', [
+    ('test_maintainer_1', 'Test Maintainer <[e-mail]>'),
+    ('test_maintainer_2', '<[e-mail]>'),
+])
+def test_changelog_maintainer(changelog_name, expected):
+    test_changelog = get_test_changelog(changelog_name)
+    assert test_changelog.maintainer == expected
+
+
+def test_changelog_maintainer_invalid():
+    with pytest.raises(ValueError):
+        test_changelog = get_test_changelog('test_maintainer_3')
+        test_changelog.maintainer
+
+
[e-mail](
+    'changelog_name, name, email, epoch_seconds, offset', [
+        (
+            'test_maintainer_1',
+            'Test Maintainer',
+            '[e-mail]',
+            0,
+            0,
+        ),
+        (
+            'test_maintainer_2',
+            '',
+            '[e-mail]',
+            0,
+            0,
+        ),
+        (
+            'test_date_1',
+            'Test Maintainer',
+            '[e-mail]',
+            1463040874,
+            -420,
+        ),
+        (
+            'test_date_2',
+            'Test Maintainer',
+            '[e-mail]',
+            1463040874,
+            -420,
+        ),
+        (
+            'maintainer_name_leading_space',
+            'Test Maintainer',
+            '[e-mail]',
+            0,
+            0,
+        ),
+        (
+            'maintainer_name_trailing_space',
+            'Test Maintainer',
+            '[e-mail]',
+            0,
+            0,
+        ),
+        (
+            'maintainer_name_inner_space',
+            'Test  Maintainer',
+            '[e-mail]',
+            0,
+            0,
+        ),
+])
+def test_changelog_authorship(
+    changelog_name,
+    name,
+    email,
+    epoch_seconds,
+    offset,
+):
+    result = get_test_changelog(changelog_name).git_authorship
+    assert result == (name, email, epoch_seconds, offset)
+
+
 def test_changelog_utf8():
     test_changelog = get_test_changelog('test_utf8_error')
     assert test_changelog.version == '1.0.3-2'
@@ -697,3 +778,102 @@ def test_repo_does_not_cleanup():
         assert os.path.exists(path)
     finally:
         shutil.rmtree(path, ignore_errors=True)
+
+
+def pygit2_signature_tuple(sig):
+    """Convert a pygit2.Signature object to a tuple for equality comparison
+
+    Support for comparing pygit2.Signature objects directly was added in pygit2
+    0.27.3. Since we are pinned to an earlier version, this function converts a
+    pygit2.Signature to a tuple that can be compared. When we use pygit2 >=
+    0.27.3, this function can be removed and callers changed to compare
+    pygit2.Signature objects directly.
+
+    For interface stability, the returned tuple should be used for equality
+    comparisons only.
+
+    :param pygit2.Signature sig: any Signature object
+    :returns: the given Signature objected converted to an (opaque) tuple that
+        can be compared for equality
+    :rtype: tuple
+    """
+    return tuple(getattr(sig, k) for k in ['name', 'email', 'time', 'offset'])
+
+
+def test_commit_tree(repo):
+    # Construct a repository with an initial commit on the master branch so
+    # that we can verify later parentage and lack of branch movement
+    parent_commit_oid = Repo(
+        commits=[Commit(name='master')],
+        branches={'master': Placeholder('master')}
+    ).write(repo.raw_repo)
+    repo.raw_repo.lookup_reference('HEAD').set_target('refs/heads/master')
+
+    # Construct a tree inside the repository with a debian/changelog in it to
+    # feed to the method under test
+    test_changelog_path = os.path.join(
+        pkg_resources.resource_filename('gitubuntu', 'changelog_tests'),
+        'test_date_1',
+    )
+    with open(test_changelog_path, 'rb') as f:
+        test_changelog_bytes = f.read()
+    test_changelog_blob = Blob(test_changelog_bytes)
+    source_tree = Tree({'debian': Tree({'changelog': test_changelog_blob})})
+    source_tree_oid = source_tree.write(repo.raw_repo)
+
+    # Call the method under test
+    commit_oid = repo.commit_source_tree(
+        tree=source_tree_oid,
+        parents=[parent_commit_oid],
+        log_message='test_commit_msg',
+        commit_date=datetime.datetime(
+            1971,  # year
+            2,  # month
+            3,  # day
+            4,  # hours
+            5,  # minutes
+            6,  # seconds
+            7,  # milliseconds (this should get truncated down to 0)
+            datetime.timezone(datetime.timedelta(hours=-8))
+        ),
+    )
+
+    # Retrieve the commit object created
+    commit = repo.raw_repo[commit_oid]
+
+    # Verify the commit object created is exactly as we expect it
+
+    # Neither HEAD nor the master branch should have changed
+    assert repo.raw_repo.lookup_reference('HEAD').target == 'refs/heads/master'
+    master_ref = repo.raw_repo.lookup_reference('refs/heads/master')
+    assert master_ref.target == parent_commit_oid
+
+    # The tree of the commit should match the tree we requested
+    assert commit.tree_id == source_tree_oid
+
+    # The parent commits should match the parents we requested
+    assert commit.parent_ids == [parent_commit_oid]
+
+    # The commit message should match what we requested
+    assert commit.message == 'test_commit_msg'
+
+    # The commit author should match the changelog entry inside the tree given
+    assert pygit2_signature_tuple(commit.author) == pygit2_signature_tuple(
+        pygit2.Signature(
+            name='Test Maintainer',
+            email='[e-mail]',
+            time=1463040874,
+            offset=-420,
+        )
+    )
+
+    # The commit committer should match the date we requested (except the
+    # milliseconds component) combined with the specification name and email
+    assert pygit2_signature_tuple(commit.committer) == pygit2_signature_tuple(
+        pygit2.Signature(
+            name=gitubuntu.spec.SYNTHESIZED_COMMITTER_NAME,
+            email=gitubuntu.spec.SYNTHESIZED_COMMITTER_EMAIL,
+            time=34430706,
+            offset=-480,
+        )
+    )
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index f7f78a6..c6bfea9 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -578,8 +578,7 @@ def _commit_import(
     changelog_parent_commit,
     upload_parent_commit,
     unapplied_parent_commit,
-    fallback_author,
-    fallback_date,
+    commit_date,
 ):
     """Commit a tree object into the repository with the specified
     parents
@@ -602,10 +601,7 @@ def _commit_import(
          upload parent. Should be None if patches-applied import.
     :param unapplied_parent_commit str Git commit hash of unapplied
          parent. Should be None if patches-unapplied import.
-    :param fallback_author str If the authorship for @version cannot be
-         parsed, use this value. Can be None.
-    :param fallback_date str If the date for @version cannot be parsed,
-         use this value. Can be None.
+    :param str commit_date: committer date to use for the git commit metadata
 
     :rtype str
     :returns Hash of created commit
@@ -619,16 +615,16 @@ def _commit_import(
     parents = []
 
     if changelog_parent_commit is not None:
-        parents.append(changelog_parent_commit)
+        parents.append(pygit2.Oid(hex=changelog_parent_commit))
     if upload_parent_commit is not None:
-        parents.append(upload_parent_commit)
+        parents.append(pygit2.Oid(hex=upload_parent_commit))
     if unapplied_parent_commit is not None:
-        parents.append(unapplied_parent_commit)
+        parents.append(pygit2.Oid(hex=unapplied_parent_commit))
 
-    commit_hash = repo.commit_tree_hash(
-        tree_hash,
-        parents,
-        get_import_commit_msg(
+    commit_hash = repo.commit_source_tree(
+        tree=pygit2.Oid(hex=tree_hash),
+        parents=parents,
+        log_message=get_import_commit_msg(
             repo=repo,
             version=version,
             target_head_name=target_head_name,
@@ -638,11 +634,10 @@ def _commit_import(
             upload_parent_commit=upload_parent_commit,
             unapplied_parent_commit=unapplied_parent_commit,
         ),
-        fallback_author,
-        fallback_date,
+        commit_date=commit_date,
     )
 
-    return commit_hash
+    return str(commit_hash)
 
 def commit_unapplied_patches_import(
     repo,
@@ -652,8 +647,7 @@ def commit_unapplied_patches_import(
     namespace,
     changelog_parent_commit,
     upload_parent_commit,
-    fallback_author,
-    fallback_date,
+    commit_date,
 ):
     return _commit_import(
         repo,
@@ -665,8 +659,7 @@ def commit_unapplied_patches_import(
         upload_parent_commit,
         # unapplied trees do not have a distinct unapplied parent
         None,
-        fallback_author,
-        fallback_date,
+        commit_date,
     )
 
 def commit_applied_patches_import(
@@ -677,8 +670,7 @@ def commit_applied_patches_import(
     namespace,
     changelog_parent_commit,
     unapplied_parent_commit,
-    fallback_author,
-    fallback_date,
+    commit_date,
 ):
     return _commit_import(
         repo,
@@ -691,8 +683,7 @@ def commit_applied_patches_import(
         # can not have them as direct parents
         None,
         unapplied_parent_commit,
-        fallback_author,
-        fallback_date,
+        commit_date,
     )
 
 def import_dsc(repo, dsc, namespace, version, dist):
@@ -1369,8 +1360,7 @@ def import_unapplied_dsc(
     head_name,
     skip_orig,
     parent_overrides,
-    fallback_author,
-    fallback_date,
+    commit_date=None,
 ):
     """Imports a DSC into a Git repository without patches applied
 
@@ -1387,10 +1377,8 @@ def import_unapplied_dsc(
          tarballs. Useful for debug import runs to make them as fast as
          possible, or to workaround pristine-tar issues.
     :param parent_overrides dict See parse_parentfile.
-    :param fallback_author str If the authorship for @version cannot be
-         parsed, use this value. Can be None.
-    :param fallback_date str If the date for @version cannot be parsed,
-         use this value. Can be None.
+    :param str commit_date: committer date to use for the git commit metadata.
+        If None, use the current date.
 
     :rtype None
     """
@@ -1572,8 +1560,7 @@ def import_unapplied_dsc(
         namespace,
         unapplied_changelog_parent_commit,
         upload_parent_commit,
-        fallback_author,
-        fallback_date,
+        commit_date,
     )
     logging.debug(
         "Committed patches-unapplied import of %s as %s in %s",
@@ -1642,13 +1629,6 @@ def import_unapplied_spi(
 
     repo.clean_repository_state()
 
-    fallback_author = None
-    if spi.spphr.package_creator.display_name:
-        fallback_author = str(spi.spphr.package_creator.display_name)
-    fallback_date = None
-    if spi.date_published:
-        fallback_date = str(spi.date_published)
-
     import_unapplied_dsc(
         repo=repo,
         version=str(spi.version),
@@ -1658,8 +1638,7 @@ def import_unapplied_spi(
         head_name=spi.head_name(namespace),
         skip_orig=skip_orig,
         parent_overrides=parent_overrides,
-        fallback_author=fallback_author,
-        fallback_date=fallback_date,
+        commit_date=spi.date_created,
     )
 
 def import_applied_dsc(
@@ -1671,8 +1650,7 @@ def import_applied_dsc(
     head_name,
     allow_applied_failures,
     parent_overrides,
-    fallback_author,
-    fallback_date,
+    commit_date=None,
 ):
     """Imports a DSC into a Git repository with patches applied
 
@@ -1688,10 +1666,8 @@ def import_applied_dsc(
     :param allow_applied_failures bool If True, failure to apply quilt
          patches is not fatal.
     :param parent_overrides dict See parse_parentfile.
-    :param fallback_author str If the authorship for @version cannot be
-         parsed, use this value. Can be None.
-    :param fallback_date str If the date for @version cannot be parsed,
-         use this value. Can be None.
+    :param str commit_date: committer date to use for the git commit metadata.
+        If None, use the current date.
 
     :rtype None
     """
@@ -1699,7 +1675,7 @@ def import_applied_dsc(
     _, _, pretty_head_name = head_name.partition('%s/' % namespace)
 
     unapplied_parent_tag = repo.get_import_tag(version, namespace)
-    unapplied_parent_commit = str(unapplied_parent_tag.peel().id)
+    unapplied_parent_commit = unapplied_parent_tag.peel().id
     unapplied_import_tree_hash = str(unapplied_parent_tag.peel(pygit2.Tree).id)
 
     # this is the result of all patches being applied
@@ -1712,7 +1688,7 @@ def import_applied_dsc(
     logging.debug(
         "Found patches-unapplied version %s as commit %s",
         version,
-        unapplied_parent_commit,
+        str(unapplied_parent_commit),
     )
 
     import_tree_versions = repo.get_all_changelog_versions_from_treeish(
@@ -1812,17 +1788,16 @@ def import_applied_dsc(
                     patch_name.encode()
                 )
 
-            unapplied_parent_commit = repo.commit_tree_hash(
-                applied_import_tree_hash,
-                [unapplied_parent_commit],
-                msg,
-                fallback_author,
-                fallback_date,
+            unapplied_parent_commit = repo.commit_source_tree(
+                tree=pygit2.Oid(hex=applied_import_tree_hash),
+                parents=[unapplied_parent_commit],
+                log_message=msg,
+                commit_date=commit_date,
             )
 
             logging.debug(
                 "Committed patch-application of %s as %s",
-                patch_name, unapplied_parent_commit
+                patch_name, str(unapplied_parent_commit)
             )
     except CalledProcessError as e:
         if allow_applied_failures:
@@ -1836,9 +1811,8 @@ def import_applied_dsc(
         applied_import_tree_hash,
         namespace,
         applied_changelog_parent_commit,
-        unapplied_parent_commit,
-        fallback_author,
-        fallback_date,
+        str(unapplied_parent_commit),
+        commit_date,
     )
     logging.debug(
         "Committed patches-applied import of %s as %s in %s",
@@ -1902,13 +1876,6 @@ def import_applied_spi(
 
     repo.clean_repository_state()
 
-    fallback_author = None
-    if spi.spphr.package_creator.display_name:
-        fallback_author = str(spi.spphr.package_creator.display_name)
-    fallback_date = None
-    if spi.date_published:
-        fallback_date = str(spi.date_published)
-
     import_applied_dsc(
         repo=repo,
         version=str(spi.version),
@@ -1918,8 +1885,7 @@ def import_applied_spi(
         head_name=spi.applied_head_name(namespace),
         allow_applied_failures=allow_applied_failures,
         parent_overrides=parent_overrides,
-        fallback_author=fallback_author,
-        fallback_date=fallback_date,
+        commit_date=spi.date_created,
     )
 
 
diff --git a/gitubuntu/importer_tag_test.py b/gitubuntu/importer_tag_test.py
index 901ab10..c2ae1e2 100644
--- a/gitubuntu/importer_tag_test.py
+++ b/gitubuntu/importer_tag_test.py
@@ -461,15 +461,14 @@ def test_import_unapplied_spi_tags(
         native=False,
     )
 
-    with patch_get_commit_environment(repo):
-        with source_builder.Source(publish_spec) as dsc_path:
-            target.import_unapplied_spi(
-                repo=repo,
-                spi=MockSPI(dsc_path, publish_spec.version),
-                namespace='importer',
-                skip_orig=False,
-                parent_overrides={},
-            )
+    with source_builder.Source(publish_spec) as dsc_path:
+        target.import_unapplied_spi(
+            repo=repo,
+            spi=MockSPI(dsc_path, publish_spec.version),
+            namespace='importer',
+            skip_orig=False,
+            parent_overrides={},
+        )
 
     if reuse:
         # Test that the previous commit was reused. In this case, the
@@ -711,15 +710,14 @@ def test_import_applied_spi_tags(
         has_patches=True,
     )
 
-    with patch_get_commit_environment(repo):
-        with source_builder.Source(publish_spec) as dsc_path:
-            target.import_applied_spi(
-                repo=repo,
-                spi=MockSPI(dsc_path, publish_spec.version),
-                namespace='importer',
-                allow_applied_failures=False,
-                parent_overrides={},
-            )
+    with source_builder.Source(publish_spec) as dsc_path:
+        target.import_applied_spi(
+            repo=repo,
+            spi=MockSPI(dsc_path, publish_spec.version),
+            namespace='importer',
+            allow_applied_failures=False,
+            parent_overrides={},
+        )
 
     if reuse:
         # Test that the previous commit was reused. In this case,
diff --git a/gitubuntu/importer_test.py b/gitubuntu/importer_test.py
index 0ce014a..90412d4 100644
--- a/gitubuntu/importer_test.py
+++ b/gitubuntu/importer_test.py
@@ -4,6 +4,7 @@ from unittest.mock import (
     sentinel,
 )
 
+import datetime
 import os
 import pytest
 import shutil
@@ -617,7 +618,7 @@ def MockSPI(dsc_path, version):
     spi.dsc_pathname = dsc_path
     spi.distribution_name = 'Ubuntu'
     spi.version = version
-    spi.date_published = '1970-01-01T00:00:00Z'
+    spi.date_created = datetime.datetime(1970, 1, 1)
     head_name = Mock(name='head_name')
     head_name.return_value = 'importer/ubuntu/trusty'
     spi.head_name = head_name
@@ -665,18 +666,17 @@ def test_import_unapplied_spi_quilt_patches(
         branches={'importer/ubuntu/trusty': Placeholder('publish')},
     )
 
-    with patch_get_commit_environment(repo):
-        with source_builder.Source(publish_spec) as dsc_path:
-            # import_unapplied_spi currently assumes it is called from the
-            # repository directory (pristine-tar and other commands rely on
-            # this)
-            target.import_unapplied_spi(
-                repo=repo,
-                spi=MockSPI(dsc_path, publish_spec.version),
-                namespace='importer',
-                skip_orig=False,
-                parent_overrides={},
-            )
+    with source_builder.Source(publish_spec) as dsc_path:
+        # import_unapplied_spi currently assumes it is called from the
+        # repository directory (pristine-tar and other commands rely on
+        # this)
+        target.import_unapplied_spi(
+            repo=repo,
+            spi=MockSPI(dsc_path, publish_spec.version),
+            namespace='importer',
+            skip_orig=False,
+            parent_overrides={},
+        )
 
     assert repo_comparator.equals(
         repoA=repo.raw_repo,
@@ -823,18 +823,17 @@ def test_import_unapplied_spi_parenting(
         changelog_versions=changelog_versions,
     )
 
-    with patch_get_commit_environment(repo):
-        with source_builder.Source(publish_spec) as dsc_path:
-            # import_unapplied_spi currently assumes it is called from the
-            # repository directory (pristine-tar and other commands rely on
-            # this)
-            target.import_unapplied_spi(
-                repo=repo,
-                spi=MockSPI(dsc_path, publish_spec.version),
-                namespace='importer',
-                skip_orig=False,
-                parent_overrides={},
-            )
+    with source_builder.Source(publish_spec) as dsc_path:
+        # import_unapplied_spi currently assumes it is called from the
+        # repository directory (pristine-tar and other commands rely on
+        # this)
+        target.import_unapplied_spi(
+            repo=repo,
+            spi=MockSPI(dsc_path, publish_spec.version),
+            namespace='importer',
+            skip_orig=False,
+            parent_overrides={},
+        )
 
     # we would like to check the commit hashes, but we cannot
     # currently, as the commit messages are specific to the importer
@@ -887,18 +886,17 @@ def test_import_unapplied_spi_parent_override(
         changelog_versions=['2-1', '1-1'],
     )
 
-    with patch_get_commit_environment(repo):
-        with source_builder.Source(publish_spec) as dsc_path:
-            # import_unapplied_spi currently assumes it is called from the
-            # repository directory (pristine-tar and other commands rely on
-            # this)
-            target.import_unapplied_spi(
-                repo=repo,
-                spi=MockSPI(dsc_path, publish_spec.version),
-                namespace='importer',
-                skip_orig=False,
-                parent_overrides={'2-1': {'changelog_parent': '1-2'}},
-            )
+    with source_builder.Source(publish_spec) as dsc_path:
+        # import_unapplied_spi currently assumes it is called from the
+        # repository directory (pristine-tar and other commands rely on
+        # this)
+        target.import_unapplied_spi(
+            repo=repo,
+            spi=MockSPI(dsc_path, publish_spec.version),
+            namespace='importer',
+            skip_orig=False,
+            parent_overrides={'2-1': {'changelog_parent': '1-2'}},
+        )
 
     validation_repo = input_repo.copy(
         add_commits=[
@@ -1143,15 +1141,14 @@ def test_import_applied_spi_parenting(
         has_patches=True,
     )
 
-    with patch_get_commit_environment(repo):
-        with source_builder.Source(publish_spec) as dsc_path:
-            target.import_applied_spi(
-                repo=repo,
-                spi=MockSPI(dsc_path, publish_spec.version),
-                namespace='importer',
-                allow_applied_failures=False,
-                parent_overrides={},
-            )
+    with source_builder.Source(publish_spec) as dsc_path:
+        target.import_applied_spi(
+            repo=repo,
+            spi=MockSPI(dsc_path, publish_spec.version),
+            namespace='importer',
+            allow_applied_failures=False,
+            parent_overrides={},
+        )
 
     applied_tag = repo.raw_repo.lookup_reference(
         'refs/tags/importer/applied/1-2',
diff --git a/gitubuntu/importlocal.py b/gitubuntu/importlocal.py
index 778a673..f886ca2 100644
--- a/gitubuntu/importlocal.py
+++ b/gitubuntu/importlocal.py
@@ -101,8 +101,6 @@ def main(
         head_name=None,
         skip_orig=skip_orig,
         parent_overrides=parent_overrides,
-        fallback_author=None,
-        fallback_date=None,
     )
 
     import_applied_dsc(
@@ -114,8 +112,6 @@ def main(
         head_name=None,
         allow_applied_failures=False,
         parent_overrides=parent_overrides,
-        fallback_author=None,
-        fallback_date=None,
     )
 
     repo.clean_repository_state()
diff --git a/gitubuntu/queue.py b/gitubuntu/queue.py
index 19f61fb..c0fc165 100644
--- a/gitubuntu/queue.py
+++ b/gitubuntu/queue.py
@@ -201,15 +201,12 @@ def _commit_upload(repo, upload, parent_commit):
         )
 
     # Ensure parents contains a string if specified
-    parents = [str(parent_commit)] if parent_commit else []
-    commit_hash = repo.commit_tree_hash(
-        tree_hash=tree_hash,
+    parents = [pygit2.Oid(hex=str(parent_commit))] if parent_commit else []
+    commit_hash = str(repo.commit_source_tree(
+        tree=tree_hash,
         parents=parents,
         log_message='Queue import'.encode(),
-        fallback_author=None,
-        fallback_date=None,
-
-    )
+    ))
 
     return commit_hash
 
diff --git a/gitubuntu/repo_builder.py b/gitubuntu/repo_builder.py
index c779c71..6c3aed3 100644
--- a/gitubuntu/repo_builder.py
+++ b/gitubuntu/repo_builder.py
@@ -5,6 +5,7 @@ import pygit2
 
 import gitubuntu.importer
 from gitubuntu.source_builder import Source, SourceSpec
+import gitubuntu.spec
 
 
 """Build test git repositories as data structures
@@ -56,13 +57,21 @@ __all__ = [
 # hashes or something?
 
 
-DEFAULT_SIGNATURE = pygit2.Signature(
-    name='Test Builder',
-    email='[e-mail]',
+DEFAULT_AUTHOR_SIGNATURE = pygit2.Signature(
+    # Match the default from source_builder.CHANGELOG_TEMPLATE
+    name='USD Import Team',
+    email='[e-mail]',
     time=0,  # don't default to current time for hash reproducibility
     offset=0,
 )
 
+DEFAULT_COMMITTER_SIGNATURE = pygit2.Signature(
+    name=gitubuntu.spec.SYNTHESIZED_COMMITTER_NAME,
+    email=gitubuntu.spec.SYNTHESIZED_COMMITTER_EMAIL,
+    time=0,
+    offset=0,
+)
+
 
 class NamedMixin:
     def __init__(self, name=None):
@@ -267,8 +276,8 @@ class Commit(NamedMixin, WriteMixin):
     def _obj_to_oid(self, repo, record=None):
         return repo.create_commit(
             None,
-            DEFAULT_SIGNATURE,
-            DEFAULT_SIGNATURE,
+            DEFAULT_AUTHOR_SIGNATURE,
+            DEFAULT_COMMITTER_SIGNATURE,
             self.message,
             self.tree.write(repo),
             [parent.write(repo, record=record) for parent in self.parents],
@@ -334,7 +343,7 @@ class Repo:
                 name,
                 target.write(repo),
                 pygit2.GIT_OBJ_COMMIT,
-                DEFAULT_SIGNATURE,
+                DEFAULT_COMMITTER_SIGNATURE,
                 'Tag message',
             )
         return written_commits[0] if written_commits else None
diff --git a/gitubuntu/source_information.py b/gitubuntu/source_information.py
index 6bd6d9f..38df33e 100644
--- a/gitubuntu/source_information.py
+++ b/gitubuntu/source_information.py
@@ -276,6 +276,10 @@ class GitUbuntuSourcePackageInformation:
         return self._spphr.date_published
 
     @property
+    def date_created(self):
+        return self._spphr.date_created
+
+    @property
     def pocket(self):
         return self._spphr.pocket
 
diff --git a/gitubuntu/spec.py b/gitubuntu/spec.py
new file mode 100644
index 0000000..305829d
--- /dev/null
+++ b/gitubuntu/spec.py
@@ -0,0 +1,10 @@
+# This file contains constant strings that form part of the importer
+# specification. Changing these strings will mutate commit hashes so must not
+# be changed without careful consideration and declaration of a hash ABI break.
+
+# Changing any importer code or code called from the importer may also cause an
+# ABI break of course, but changing these constants certainly will and so are
+# placed here to try to help avoid accidents.
+
+SYNTHESIZED_COMMITTER_NAME = 'usd-importer'
+SYNTHESIZED_COMMITTER_EMAIL = '[e-mail]'
diff --git a/setup.py b/setup.py
index 7b6b1a0..d9600f9 100644
--- a/setup.py
+++ b/setup.py
@@ -13,12 +13,21 @@ setup(name='gitubuntu',
 	url="https://code.launchpad.net/~usd-import-team/usd-importer/+git/usd-importer",
 	maintainer="Ubuntu Server Team",
 	maintainer_email="[e-mail] ",
+
+    # Pinning notes
+    #
+    # See LP: #1855725 on unnecessary pins.
+    #
+    # pygit2: when >= 0.27.3, git_repository_test.py::pygit2_signature_tuple
+    #     can be removed and callers adjusted. See the docstring in that
+    #     function for details.
+
     install_requires=[
         'argcomplete==1.8.1',
         'astroid<2.3',  # https://github.com/PyCQA/pylint/issues/3137
         'cachetools',
         'python-debian==0.1.31',
-        'pygit2==0.24.2',
+        'pygit2==0.24.2',  # see pinning notes above
         'launchpadlib==1.10.6',
         'petname',
         'pylint<2.4',  # https://github.com/PyCQA/pylint/issues/3137#issuecomment-542286971