Merge ~racb/git-ubuntu:changelog-parents into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

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

FAILED: Continuous integration, rev:969c7def7266a904c1a306d743503a7083563f5f
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/469/
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/469//rebuild

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:7e2fc4a379cee2d411b880700e8c621e0a86066c
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/470/
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/470//rebuild

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

I have a few very minor suggestions, but looks ok overall.

While this is a big patch, most of the code changes were straightforward to review; there were a few pytest mechanisms I wasn't familiar with, and there were a couple chunks of code that took me some time to untangle, but in the end I didn't spot any logical errors.

Since it's nicely broken up into conceptually distinct patches, I reviewed the code sequentially by patch, so am giving feedback thusly organized, hopefully it'll be clear what lines I'm referring to.

1. importer: drop unused variable
    √ LGTM. Simple cleanup

2. Add test: test_get_existing_import_tags_ordering
    - 'repo' parameter could use a doc, e.g.:
      ":param gitubuntu.git_repository.GitUbuntuRepository repo: the repository to use."
    - Add comment to explain "repo_builder.Repo(...)", e.g.
      "# Construct a synthetic git repository containing tags"
    - Rename 'mock' variable to 'mock_repo'

3. Sort reimport tags before using them
    √ LGTM. Interesting illustration of splitting the test from the fix via xfail.

4. Allow for multiple changelog parents
    √ Previously the code checked that changelog_parent_commit is not None, but
      now it checks len(changelog_parent_commits) != 0. Logically this
      works fine, the one caveat being that this now implies an assumption
      that changelog_parent_commits MUST be a list; if None ever got passed
      in then things will break. I did not spot any places where this is
      likely to happen, though.
    - Given that changelog_parent_commits is always assumed to be a list,
      there is a check around line 607 in _commit_import() that is probably
      unnecessary. It used to be a None check, and now is effectively a
      len()>0 check, however l.extend([]) will be a no-op so no need for a check.

5. test_get_import_commit_msg: multiple parent case
    √ LGTM. I wondered if this could be done as an xfail test case, but it's fine as is.

6. Generalise test_get_changelog_parent_commits
    - Param docs for test_get_changelog_parent_commits() would be nice to add
    √ Otherwise, LGTM

7. Add multiple changelog parent tests
    √ LGTM

8. Support multiple changelog parents
    √ The list comprehension for calculating changelog_parent_commits feels a bit
      dense pythonically but on further review it's just using idioms already
      common in importer.py.

As mentioned in #6, some of the test cases in importer_test.py are documented but not all; would be nice to ensure each of the 3 or 4 test cases added or changed in this patchset also gain docs while we're at it.

review: Needs Fixing

[Robie Basak (racb)]

> 2. Add test: test_get_existing_import_tags_ordering
> - 'repo' parameter could use a doc, e.g.:
> ":param gitubuntu.git_repository.GitUbuntuRepository repo: the
> repository to use."

Done, but note that importantly it's a fixture, so I noted that also.

> - Add comment to explain "repo_builder.Repo(...)", e.g.
> "# Construct a synthetic git repository containing tags"

Done.

> - Rename 'mock' variable to 'mock_repo'

Actually it's not a mock Repo; it's a mocked method on the Repo class.
But I take your point, so I called it mock_get_all_reimport_tags. Since
this is quite long, it busted the line length limit so I also had to
introduce mock_get_all_reimport_tags_patch and wrap a few other lines
too.

> 4. Allow for multiple changelog parents
> √ Previously the code checked that changelog_parent_commit is not
> None, but now it checks len(changelog_parent_commits) != 0.
> Logically this works fine, the one caveat being that this now
> implies an assumption that changelog_parent_commits MUST be a
> list; if None ever got passed in then things will break. I did
> not spot any places where this is likely to happen, though.

Yes - that's the intention. changelog_parent_commits is always a list
now, and we use an empty list instead of None now, in all cases, to
represent that there aren't any.

> - Given that changelog_parent_commits is always assumed to be a list,
> there is a check around line 607 in _commit_import() that is probably
> unnecessary. It used to be a None check, and now is effectively a
> len()>0 check, however l.extend([]) will be a no-op so no need
> for a check.

Good point. Check removed.

> 5. test_get_import_commit_msg: multiple parent case
> √ LGTM. I wondered if this could be done as an xfail test case,
> but it's fine as is.

This was a case that the code was already correct but it lacked a test,
so the xfail process didn't apply. I'm not sure if you already said
that.

> 6. Generalise test_get_changelog_parent_commits
> - Param docs for test_get_changelog_parent_commits() would be nice to add

Done.

> √ Otherwise, LGTM

> As mentioned in #6, some of the test cases in importer_test.py are
> documented but not all; would be nice to ensure each of the 3 or 4
> test cases added or changed in this patchset also gain docs while
> we're at it.

Done. I've ensured that every test case touched has a docstring. I did
this in a new commit at the end of the patchset. Some of this new
documentation exposes some shortcomings in the implementation, but in
the interest of making progress I documented the code as-is, leaving
improvements to the code for the future.

[Robie Basak (racb)]

Here's the range-diff for your reference: http://paste.ubuntu.com/p/bWR7zShPxp/

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

PASSED: Continuous integration, rev:fd197c345d0230cb721e62f62dbf01f0569a4003
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/475/
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/475//rebuild

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

Thanks for the range-diff, all looks good, +1.

review: Approve

diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 98a0997..495a22d 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -488,15 +488,19 @@ def main(
 def get_changelog_for_commit(
     repo,
     tree_hash,
-    changelog_parent_commit,
+    changelog_parent_commits,
 ):
     """Extract changes to debian/changelog in this publish
 
     LP: #1633114 -- favor the changelog parent's diff
     """
     raw_clog_entry = b''
-    if changelog_parent_commit is not None:
-        cmd = ['diff-tree', '-p', changelog_parent_commit,
+    # If there is more than one changelog parent then don't attempt to combine
+    # their entries, and instead return nothing (raw_clog_entry will remain
+    # b''). This can be addressed later when the specification better defines
+    # what the commit message should say in this case, but will do for now.
+    if len(changelog_parent_commits) == 1:
+        cmd = ['diff-tree', '-p', changelog_parent_commits[0],
                tree_hash, '--', 'debian/changelog']
         raw_clog_entry, _ = repo.git_run(cmd, decode=False)
 
@@ -529,10 +533,32 @@ def get_import_commit_msg(
     target_head_name,
     namespace,
     tree_hash,
-    changelog_parent_commit,
+    changelog_parent_commits,
     upload_parent_commit,
     unapplied_parent_commit,
 ):
+    """Compose an appropriate commit message
+
+    When the importer synthesizes a commit, it needs to provide an appropriate
+    commit message. This function centralizes the generation of that message
+    based on the parameters given.
+
+    :param GitUbuntuRepository repo: the repository in which the tree being
+        committed can be found
+    :param str version: the package version string being committed
+    :param str target_head_name: the nominal name of the branch to which the
+        commit is being made
+    :param str namespace: the namespace name to prefix to the nominal branch
+        name to determine the final branch name
+    :param str tree_hash: the hash of the tree object being committed
+    :param list(str) changelog_parent_commits: any changelog parents to use in
+        the commit
+    :param str upload_parent_commit: the upload parent to use in the commit, or
+        None if there isn't one
+    :param str unapplied_parent_commit: the unapplied parent to use in the
+        commit, or None if this is an applied branch commit
+
+    """
     if unapplied_parent_commit:
         import_type = 'patches-applied'
     else:
@@ -543,7 +569,7 @@ def get_import_commit_msg(
     changelog_entry = get_changelog_for_commit(
         repo,
         tree_hash,
-        changelog_parent_commit
+        changelog_parent_commits
     )
 
     msg = (
@@ -554,10 +580,10 @@ def get_import_commit_msg(
         )
     )
 
-    if any([changelog_parent_commit, upload_parent_commit, unapplied_parent_commit]):
+    if any([changelog_parent_commits, upload_parent_commit, unapplied_parent_commit]):
         msg += b'\n'
 
-    if changelog_parent_commit is not None:
+    for changelog_parent_commit in changelog_parent_commits:
         msg += b'\nChangelog parent: %b' % changelog_parent_commit.encode()
     if upload_parent_commit is not None:
         msg += b'\nUpload parent: %b' % upload_parent_commit.encode()
@@ -574,7 +600,7 @@ def _commit_import(
     target_head_name,
     tree_hash,
     namespace,
-    changelog_parent_commit,
+    changelog_parent_commits,
     upload_parent_commit,
     unapplied_parent_commit,
     commit_date,
@@ -594,8 +620,8 @@ def _commit_import(
     :param target_head_name str Git branch name to import to
     :param namespace str Namespace under which relevant refs can be
          found.
-    :param changelog_parent_commit str Git commit hash of
-         changelog parent.
+    :param list(str) changelog_parent_commits Git commit hashes of changelog
+         parents.
     :param upload_parent_commit str Git commit hash of
          upload parent. Should be None if patches-applied import.
     :param unapplied_parent_commit str Git commit hash of unapplied
@@ -605,16 +631,12 @@ def _commit_import(
     :rtype str
     :returns Hash of created commit
     """
-    changelog_entry = get_changelog_for_commit(
-        repo,
-        tree_hash,
-        changelog_parent_commit
-    )
-
     parents = []
 
-    if changelog_parent_commit is not None:
-        parents.append(pygit2.Oid(hex=changelog_parent_commit))
+    parents.extend(
+        pygit2.Oid(hex=changelog_parent_commit)
+        for changelog_parent_commit in changelog_parent_commits
+    )
     if upload_parent_commit is not None:
         parents.append(pygit2.Oid(hex=upload_parent_commit))
     if unapplied_parent_commit is not None:
@@ -629,7 +651,7 @@ def _commit_import(
             target_head_name=target_head_name,
             namespace=namespace,
             tree_hash=tree_hash,
-            changelog_parent_commit=changelog_parent_commit,
+            changelog_parent_commits=changelog_parent_commits,
             upload_parent_commit=upload_parent_commit,
             unapplied_parent_commit=unapplied_parent_commit,
         ),
@@ -644,7 +666,7 @@ def commit_unapplied_patches_import(
     head_name,
     import_tree_hash,
     namespace,
-    changelog_parent_commit,
+    changelog_parent_commits,
     upload_parent_commit,
     commit_date,
 ):
@@ -654,7 +676,7 @@ def commit_unapplied_patches_import(
         head_name,
         import_tree_hash,
         namespace,
-        changelog_parent_commit,
+        changelog_parent_commits,
         upload_parent_commit,
         # unapplied trees do not have a distinct unapplied parent
         None,
@@ -667,7 +689,7 @@ def commit_applied_patches_import(
     head_name,
     import_tree_hash,
     namespace,
-    changelog_parent_commit,
+    changelog_parent_commits,
     unapplied_parent_commit,
     commit_date,
 ):
@@ -677,7 +699,7 @@ def commit_applied_patches_import(
         head_name,
         import_tree_hash,
         namespace,
-        changelog_parent_commit,
+        changelog_parent_commits,
         # uploads will be unapplied trees currently, so applied trees
         # can not have them as direct parents
         None,
@@ -985,16 +1007,35 @@ def get_existing_import_tags(
     namespace,
     patch_state=PatchState.UNAPPLIED,
 ):
+    """Find and return any import tags that already exist for the given version
+
+    :param GitUbuntuRepository repo: repository to search for import tags
+    :param str version: the package version string to find
+    :param str namespace: the name to prefix to the import tag names searched
+    :param PatchState patch_state: whether to look for unapplied or applied
+        tags
+    :rtype: list(pygit2.Reference)
+    :returns: a list of import tags found. If reimport tags exist, this is the
+        list of matching reimport tags in order; the original import tag is not
+        included as it is already represented by the first reimport tag. If
+        reimport tags do not exist, just the sole import tag is returned. If
+        none of these are found, the empty list is returned.
+    """
     existing_import_tag = repo.get_import_tag(version, namespace, patch_state)
     if existing_import_tag:
-        # check if there are reimports
+        # check if there are reimports; if no import tag exists, then by the
+        # specification there can be no reimports
         existing_reimport_tags = repo.get_all_reimport_tags(
             version,
             namespace,
             patch_state,
         )
         if existing_reimport_tags:
-            return existing_reimport_tags
+            return sorted(
+                existing_reimport_tags,
+                # Sort on the integer of the last '/'-separated component
+                key=lambda tag: int(tag.name.split('/')[-1]),
+            )
         else:
             return [existing_import_tag]
     return []
@@ -1002,10 +1043,10 @@ def get_existing_import_tags(
 
 def override_parents(parent_overrides, repo, version, namespace):
     """
-    returns: (unapplied, applied) where both elements of the tuple are commit
-        hash strings of the changelog parent to use, incorporating any defined
-        overrides.
-    rtype: tuple(str, str)
+    returns: (unapplied, applied) where both elements of the tuple are lists of
+        commit hash strings of the changelog parents to use, incorporating any
+        defined overrides.
+    rtype: tuple(list(str), list(str))
     """
     unapplied_changelog_parent_commit = None
     applied_changelog_parent_commit = None
@@ -1057,8 +1098,8 @@ def override_parents(parent_overrides, repo, version, namespace):
                 )
             )
     return (
-        unapplied_changelog_parent_commit,
-        applied_changelog_parent_commit,
+        [unapplied_changelog_parent_commit],
+        [applied_changelog_parent_commit],
     )
 
 def _devel_branch_updates(
@@ -1296,72 +1337,83 @@ def create_import_tag(repo, commit_hash, version, namespace):
         return repo.get_import_tag(version, namespace)
 
 
-def get_changelog_parent_commit(
+def get_changelog_parent_commits(
     repo,
     namespace,
     parent_overrides,
     import_tree_versions,
-    patches_applied,
+    patch_state,
 ):
-    """Walk changelog backwards until we find an imported version,
-    skipping the current version
-
-    :param repo gitubuntu.git_repository.GitUbuntuRepository The
-        repository to use
-    :param parent_overrides dict See parse_parentfile
-    :param import_tree_versions list(str) List of versions to iterate.
-    :param patches_applied bool If true, look for patches-applied
-        imports
-
-    :rtype str
-    :returns Git commit hash of the nearest (chronologically) imported
-        changelog version
+    """Given a changelog history, find the changelog parent commits
+
+    "Changelog parents" are defined in the import specification, which refers
+    to source package data instances. Since this importer runs incrementally in
+    Launchpad publication order, the commits associated with the source package
+    data instances can be located by traversing the matching reimport tags in
+    order. If there are no reimport tags, the sole import tag (if it exists)
+    will match the sole corresponding source package data instance. Otherwise
+    we find no changelog parent and return an empty list.
+
+    This function performs this search and returns a list of commit hashes that
+    are the changelog parents.
+
+    :param gitubuntu.git_repository.GitUbuntuRepository repo: the repository to
+        use
+    :param dict parent_overrides: see parse_parentfile
+    :param list(str) import_tree_versions: list of package version strings as
+        parsed from debian/changelog in the same order
+    :param PatchState patch_state: whether to look for previous unapplied or
+        applied imports
+
+    :rtype: list(str)
+    :returns: a list of commit hashes of any existing imports of the changelog
+        parents as defined by the import specification
     """
     # skip current version
     import_tree_versions = import_tree_versions[1:]
 
     for version in import_tree_versions:
-        if patches_applied:
-            changelog_parent_tag = repo.get_import_tag(
-                version,
-                namespace,
-                PatchState.APPLIED,
-            )
-        else:
-            changelog_parent_tag = repo.get_import_tag(
-                version,
-                namespace,
-                PatchState.UNAPPLIED,
-            ) or repo.get_orphan_tag(
-                version,
-                namespace,
-            )
+        changelog_parent_tags = get_existing_import_tags(
+            repo,
+            version,
+            namespace,
+            patch_state,
+        )
 
-        if not changelog_parent_tag:
+        if not changelog_parent_tags:
             continue
 
         # sanity check that version from d/changelog of the
         # tagged parent matches ours
-        changelog_parent_version, _ = repo.get_changelog_versions_from_treeish(
-            str(changelog_parent_tag.peel(pygit2.Tree).id)
-        )
+        changelog_parent_versions = [
+            repo.get_changelog_versions_from_treeish(
+                str(tag.peel(pygit2.Tree).id)
+            )[0] for tag in changelog_parent_tags
+        ]
 
         # if the parent was imported via a parent override
         # itself, assume it is valid without checking its
         # tree's debian/changelog, as it wasn't used
         if (version in parent_overrides or
-            changelog_parent_version == version
-        ):
-            changelog_parent_commit = str(
-                changelog_parent_tag.peel(pygit2.Commit).id
+            any(
+                [changelog_parent_version == version
+                 for changelog_parent_version in changelog_parent_versions]
             )
-
-            logging.debug("Changelog parent (tag) is %s",
-                repo.tag_to_pretty_name(changelog_parent_tag)
+        ):
+            changelog_parent_commits = [
+                str(tag.peel(pygit2.Commit).id)
+                for tag in changelog_parent_tags
+            ]
+
+            logging.debug("Changelog parent(s) (tag(s)) are %s",
+                [
+                    repo.tag_to_pretty_name(tag)
+                    for tag in changelog_parent_tags
+                ]
             )
-            return changelog_parent_commit
+            return changelog_parent_commits
 
-    return None
+    return []
 
 
 def import_unapplied_dsc(
@@ -1463,7 +1515,6 @@ def import_unapplied_dsc(
 
     logging.debug('Tip version is %s', unapplied_tip_version)
 
-    unapplied_changelog_parent_commit = None
     upload_parent_commit = None
 
     if version in parent_overrides:
@@ -1474,7 +1525,7 @@ def import_unapplied_dsc(
 
         try:
             (
-                unapplied_changelog_parent_commit,
+                unapplied_changelog_parent_commits,
                 _
             ) = override_parents(
                 parent_overrides,
@@ -1494,12 +1545,12 @@ def import_unapplied_dsc(
                  unapplied_tip_version,
             )
 
-        unapplied_changelog_parent_commit = get_changelog_parent_commit(
+        unapplied_changelog_parent_commits = get_changelog_parent_commits(
             repo,
             namespace,
             parent_overrides,
             import_tree_versions,
-            patches_applied=False,
+            patch_state=PatchState.UNAPPLIED,
         )
 
     # check if the version to import has already been uploaded to
@@ -1515,7 +1566,7 @@ def import_unapplied_dsc(
                 "does not match the published version. Will orphan commit.",
                 repo.tag_to_pretty_name(existing_import_tag),
             )
-            unapplied_changelog_parent_commit = None
+            unapplied_changelog_parent_commits = []
         else:
             repo.update_head_to_commit(
                 head_name,
@@ -1549,31 +1600,31 @@ def import_unapplied_dsc(
         else:
             upload_parent_commit = str(existing_upload_tag.peel().id)
 
-            if unapplied_changelog_parent_commit is not None:
+            if unapplied_changelog_parent_commits:
                 try:
                     repo.git_run(
                         [
                             'merge-base',
                             '--is-ancestor',
-                            unapplied_changelog_parent_commit,
+                            unapplied_changelog_parent_commits[0],
                             upload_parent_commit,
                         ],
                         verbose_on_failure=False,
                     )
-                    unapplied_changelog_parent_commit = None
+                    unapplied_changelog_parent_commits = []
                 except CalledProcessError as e:
                     if e.returncode != 1:
                         raise
 
     commit_hash = commit_unapplied_patches_import(
-        repo,
-        version,
-        head_name,
-        unapplied_import_tree_hash,
-        namespace,
-        unapplied_changelog_parent_commit,
-        upload_parent_commit,
-        commit_date,
+        repo=repo,
+        version=version,
+        head_name=head_name,
+        import_tree_hash=unapplied_import_tree_hash,
+        namespace=namespace,
+        changelog_parent_commits=unapplied_changelog_parent_commits,
+        upload_parent_commit=upload_parent_commit,
+        commit_date=commit_date,
     )
     logging.debug(
         "Committed patches-unapplied import of %s as %s in %s",
@@ -1587,7 +1638,7 @@ def import_unapplied_dsc(
         # Not imported before
         tag = import_tag(version, namespace)
     elif (
-        unapplied_changelog_parent_commit is None and
+        not unapplied_changelog_parent_commits and
         upload_parent_commit is None and
         head_name in repo.local_branch_names
     ):
@@ -1726,8 +1777,6 @@ def import_applied_dsc(
 
     logging.debug('Tip version is %s', applied_tip_version)
 
-    applied_changelog_parent_commit = None
-
     if version in parent_overrides:
         logging.debug('%s is specified in the parent override file.' %
                      version
@@ -1735,7 +1784,7 @@ def import_applied_dsc(
 
         (
             _,
-            applied_changelog_parent_commit,
+            applied_changelog_parent_commits,
         ) = override_parents(
             parent_overrides,
             repo,
@@ -1749,12 +1798,12 @@ def import_applied_dsc(
                         )
 
 
-        applied_changelog_parent_commit = get_changelog_parent_commit(
+        applied_changelog_parent_commits = get_changelog_parent_commits(
             repo,
             namespace,
             parent_overrides,
             import_tree_versions,
-            patches_applied=True,
+            patch_state=PatchState.APPLIED,
         )
 
     existing_applied_tag = repo.get_import_tag(
@@ -1774,7 +1823,7 @@ def import_applied_dsc(
         #         "version. Will orphan commit.",
         #        repo.tag_to_pretty_name(applied_tag),
         #    )
-        #    applied_changelog_parent_commit = None
+        #    applied_changelog_parent_commits = []
         #else:
         repo.update_head_to_commit(
             head_name,
@@ -1822,14 +1871,14 @@ def import_applied_dsc(
         raise
 
     commit_hash = commit_applied_patches_import(
-        repo,
-        version,
-        head_name,
-        applied_import_tree_hash,
-        namespace,
-        applied_changelog_parent_commit,
-        str(unapplied_parent_commit),
-        commit_date,
+        repo=repo,
+        version=version,
+        head_name=head_name,
+        import_tree_hash=applied_import_tree_hash,
+        namespace=namespace,
+        changelog_parent_commits=applied_changelog_parent_commits,
+        unapplied_parent_commit=str(unapplied_parent_commit),
+        commit_date=commit_date,
     )
     logging.debug(
         "Committed patches-applied import of %s as %s in %s",
@@ -1843,7 +1892,7 @@ def import_applied_dsc(
         # Not imported before
         tag = import_tag(version, namespace, PatchState.APPLIED)
     elif (
-        applied_changelog_parent_commit is None and
+        not applied_changelog_parent_commits and
         unapplied_parent_commit is None and
         head_name in repo.local_branch_names
     ):
diff --git a/gitubuntu/importer_test.py b/gitubuntu/importer_test.py
index b1dcdc3..95744d9 100644
--- a/gitubuntu/importer_test.py
+++ b/gitubuntu/importer_test.py
@@ -73,39 +73,46 @@ def test_get_import_tag_msg():
 
 
 @pytest.mark.parametrize(
-    'head_name_value, changelog_parent_commit, upload_parent_commit, unapplied_parent_commit, expected',
+    'head_name_value, changelog_parent_commits, upload_parent_commit, unapplied_parent_commit, expected',
     [
         (
             'importer/ubuntu/trusty',
-            None,
+            [],
             None,
             None,
             b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.',
         ),
         (
             'importer/ubuntu/trusty',
-            '123456',
+            ['123456'],
             None,
             None,
             b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nChangelog parent: 123456',
         ),
         (
             'importer/ubuntu/trusty',
-            None,
+            [],
             '123456',
             None,
             b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nUpload parent: 123456',
         ),
         (
             'importer/ubuntu/trusty',
+            ['123456', '234567'],
+            None,
             None,
+            b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nChangelog parent: 123456\nChangelog parent: 234567',
+        ),
+        (
+            'importer/ubuntu/trusty',
+            [],
             None,
             '123456',
             b'Import patches-applied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nUnapplied parent: 123456',
         ),
         (
             'importer/ubuntu/trusty',
-            '123456',
+            ['123456'],
             '789123',
             '456789',
             b'Import patches-applied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nChangelog parent: 123456\nUpload parent: 789123\nUnapplied parent: 456789'
@@ -115,11 +122,30 @@ def test_get_import_tag_msg():
 def test_get_import_commit_msg(
     repo,
     head_name_value,
-    changelog_parent_commit,
+    changelog_parent_commits,
     upload_parent_commit,
     unapplied_parent_commit,
     expected,
 ):
+    """Test that get_import_commit_msg is generally correct
+
+    This is the general parameterised test for the common case uses of
+    target.get_import_commit_msg.
+
+    This test uses a default source_builder.SourceSpec() to construct a test
+    source tree to provide to get_import_commit_msg, so defaults from
+    SourceSpec() will be used such as a package version string of '1-1'.
+
+    :param GitUbuntuRepository repo: fixture providing a temporary
+        GitUbuntuRepository instance to use
+    :param str head_name_value: passed through to get_import_commit_msg as
+        target_head_name
+    :param list(str) changelog_parent_commits: passed through to
+        get_import_commit_msg
+    :param str upload_parent_commit: passed through to get_import_commit_msg
+    :param str unapplied_parent_commit: passed through to get_import_commit_msg
+    :param bytes expected: the expected return value from get_import_commit_msg
+    """
     target.get_changelog_for_commit = Mock()
     target.get_changelog_for_commit.return_value = b''
 
@@ -137,22 +163,24 @@ def test_get_import_commit_msg(
         head_name_value,
         'importer',
         publish_tree_hash,
-        changelog_parent_commit,
+        changelog_parent_commits,
         upload_parent_commit,
         unapplied_parent_commit,
     ) == expected
 
 
 @pytest.mark.parametrize(
-    'input_repo, expected', [
+    'input_repo, patch_state, expected', [
         (
             repo_builder.Repo(),
+            PatchState.UNAPPLIED,
             [],
         ),
         (
             repo_builder.Repo(
                 tags={'importer/import/1-1': repo_builder.Commit()},
             ),
+            PatchState.UNAPPLIED,
             ['refs/tags/importer/import/1-1'],
         ),
         (
@@ -167,32 +195,22 @@ def test_get_import_commit_msg(
                     'importer/reimport/import/1-1/1': repo_builder.Commit(),
                 },
             ),
+            PatchState.UNAPPLIED,
             [
                 'refs/tags/importer/reimport/import/1-1/0',
                 'refs/tags/importer/reimport/import/1-1/1',
             ],
         ),
-    ],
-)
-def test_get_existing_import_tags(repo, input_repo, expected):
-    input_repo.write(repo.raw_repo)
-
-    assert [
-        ref.name for ref in
-        target.get_existing_import_tags(repo, '1-1', 'importer')
-    ] == expected
-
-
[e-mail](
-    'input_repo, expected', [
         (
             repo_builder.Repo(),
+            PatchState.APPLIED,
             [],
         ),
         (
             repo_builder.Repo(
                 tags={'importer/applied/1-1': repo_builder.Commit()},
             ),
+            PatchState.APPLIED,
             ['refs/tags/importer/applied/1-1'],
         ),
         (
@@ -207,6 +225,7 @@ def test_get_existing_import_tags(repo, input_repo, expected):
                     'importer/reimport/applied/1-1/1': repo_builder.Commit(),
                 },
             ),
+            PatchState.APPLIED,
             [
                 'refs/tags/importer/reimport/applied/1-1/0',
                 'refs/tags/importer/reimport/applied/1-1/1',
@@ -214,18 +233,78 @@ def test_get_existing_import_tags(repo, input_repo, expected):
         ),
     ],
 )
-def test_get_existing_applied_tags(repo, input_repo, expected):
+def test_get_existing_import_tags(repo, patch_state, input_repo, expected):
+    """Test that get_existing_import_tags is generally correct
+
+    This is the general parameterised test for the common case uses of
+    target.get_existing_import_tags.
+
+    :param repo gitubuntu.git_repository.GitUbuntuRepository: fixture to hold a
+        temporary output repository
+    :param PatchState patch_state: passed through to get_existing_import_tags
+    :param repo_builder.Repo input_repo: input repository data
+    :param list(str) expected: the names of the references that are expected to
+        be returned, in order.
+    """
     input_repo.write(repo.raw_repo)
 
     assert [
         ref.name for ref in
-        target.get_existing_import_tags(
-            repo,
+        target.get_existing_import_tags(repo, '1-1', 'importer', patch_state)
+    ] == expected
+
+
+def test_get_existing_import_tags_ordering(repo):
+    """Test that get_existing_import_tags returns results in the correct order
+
+    To maintain hash stability, the spec defines that multiple changelog
+    parents must appear in the order that they were published. For this to
+    work, get_existing_import_tags must return the tags in the correct order
+    even if the underlying git repository tags appear in an arbitrary order.
+
+    :param GitUbuntuRepository repo: fixture of a temporary repository to use
+    """
+
+    # Construct a synthetic git repository containing tags
+    repo_builder.Repo(
+        tags={
+            'importer/import/1-1': repo_builder.Commit(),
+            'importer/reimport/import/1-1/0': repo_builder.Commit(),
+            'importer/reimport/import/1-1/1': repo_builder.Commit(),
+        }
+    ).write(repo.raw_repo)
+
+    mock_get_all_reimport_tags_patch = patch.object(
+        repo,
+        'get_all_reimport_tags',
+        autospec=True,
+    )
+    with mock_get_all_reimport_tags_patch as mock_get_all_reimport_tags:
+        # Intentionally arrange for repo.get_all_reimport_tags to return the
+        # expected result but in reverse order
+        mock_get_all_reimport_tags.return_value = [
+            repo.raw_repo.lookup_reference('refs/tags/%s' % name)
+            for name in [
+                'importer/reimport/import/1-1/1',
+                'importer/reimport/import/1-1/0',
+            ]
+        ]
+
+        # Call function under test
+        tags = target.get_existing_import_tags(repo, '1-1', 'importer')
+
+        # Check that our mock was called as expected
+        mock_get_all_reimport_tags.assert_called_once_with(
             '1-1',
             'importer',
-            PatchState.APPLIED,
+            PatchState.UNAPPLIED,
         )
-    ] == expected
+
+        # Result tags should be in numeric order
+        assert [ref.name for ref in tags] == [
+            'refs/tags/importer/reimport/import/1-1/0',
+            'refs/tags/importer/reimport/import/1-1/1',
+        ]
 
 
 @pytest.mark.parametrize(
@@ -442,16 +521,16 @@ def test_create_applied_tag(
         'input_repo',
         'parent_overrides',
         'changelog_versions',
-        'patches_applied',
-        'expected_ref',
+        'patch_state',
+        'expected_refs',
     ],
     [
         (
             repo_builder.Repo(),
             {},
             ['1-2', '1-1',],
-            False,
-            None,
+            PatchState.UNAPPLIED,
+            [],
         ),
         (
             repo_builder.Repo(
@@ -460,8 +539,28 @@ def test_create_applied_tag(
             ),
             {},
             ['1-2', '1-1'],
-            False,
-            'refs/tags/importer/import/1-1',
+            PatchState.UNAPPLIED,
+            ['refs/tags/importer/import/1-1'],
+        ),
+        (
+            repo_builder.Repo(
+                commits=[
+                    repo_builder.Commit.from_spec(name='import'),
+                    repo_builder.Commit.from_spec(name='reimport', mutate=1),
+                ],
+                tags={
+                    'importer/import/1-1': Placeholder('import'),
+                    'importer/reimport/import/1-1/0': Placeholder('import'),
+                    'importer/reimport/import/1-1/1': Placeholder('reimport'),
+                },
+            ),
+            {},
+            ['1-2', '1-1'],
+            PatchState.UNAPPLIED,
+            [
+                'refs/tags/importer/reimport/import/1-1/0',
+                'refs/tags/importer/reimport/import/1-1/1',
+            ],
         ),
         (
             repo_builder.Repo(
@@ -470,15 +569,15 @@ def test_create_applied_tag(
             ),
             {},
             ['1-3', '1-2', '1-1'],
-            False,
-            'refs/tags/importer/import/1-1',
+            PatchState.UNAPPLIED,
+            ['refs/tags/importer/import/1-1'],
         ),
         (
             repo_builder.Repo(),
             {},
             ['1-2', '1-1',],
-            True,
-            None,
+            PatchState.APPLIED,
+            [],
         ),
         (
             repo_builder.Repo(
@@ -487,8 +586,28 @@ def test_create_applied_tag(
             ),
             {},
             ['1-2', '1-1'],
-            True,
-            'refs/tags/importer/applied/1-1',
+            PatchState.APPLIED,
+            ['refs/tags/importer/applied/1-1'],
+        ),
+        (
+            repo_builder.Repo(
+                commits=[
+                    repo_builder.Commit.from_spec(name='import'),
+                    repo_builder.Commit.from_spec(name='reimport', mutate=1),
+                ],
+                tags={
+                    'importer/applied/1-1': Placeholder('import'),
+                    'importer/reimport/applied/1-1/0': Placeholder('import'),
+                    'importer/reimport/applied/1-1/1': Placeholder('reimport'),
+                },
+            ),
+            {},
+            ['1-2', '1-1'],
+            PatchState.APPLIED,
+            [
+                'refs/tags/importer/reimport/applied/1-1/0',
+                'refs/tags/importer/reimport/applied/1-1/1',
+            ],
         ),
         (
             repo_builder.Repo(
@@ -497,30 +616,51 @@ def test_create_applied_tag(
             ),
             {},
             ['1-3', '1-2', '1-1'],
-            True,
-            'refs/tags/importer/applied/1-1',
+            PatchState.APPLIED,
+            ['refs/tags/importer/applied/1-1'],
         ),
     ],
 )
-def test_get_changelog_parent_commit(
+def test_get_changelog_parent_commits(
     repo,
     input_repo,
     parent_overrides,
     changelog_versions,
-    patches_applied,
-    expected_ref,
+    patch_state,
+    expected_refs,
 ):
+    """Test that get_changelog_parent_commits is generally correct
+
+    This is the general parameterised test for the common case uses of
+    target.get_changelog_parent_commits.
+
+    :param GitUbuntuRepository repo: fixture providing a temporary
+        GitUbuntuRepository instance to use
+    :param repo_builder.Repo input_repo: the input repository data to use that
+        will be populated into @repo before @repo is passed through to
+        get_changelog_parent_commits
+    :param dict parent_overrides: passed through to
+        get_changelog_parent_commits.
+    :param PatchState patch_state: passed through to
+        get_changelog_parent_commits
+    :param list(str) expected_refs: the expected return value of
+        get_changelog_parent_commits expressed using a list of reference names.
+        Since get_changelog_parent_commits returns a list of commit hash
+        strings, the reference names will need to be dereferenced before
+        comparison; this way the test parameters don't need to be opaque hash
+        strings.
+    """
     input_repo.write(repo.raw_repo)
-    assert target.get_changelog_parent_commit(
+    assert target.get_changelog_parent_commits(
         repo,
         'importer',
         parent_overrides,
         changelog_versions,
-        patches_applied,
-    ) == (
+        patch_state,
+    ) == ([
         str(repo.raw_repo.lookup_reference(expected_ref).peel(pygit2.Commit).id)
-        if expected_ref else expected_ref
-    )
+        for expected_ref in expected_refs
+    ])
 
 @patch('gitubuntu.git_repository.GitUbuntuRepository.add_base_remotes')
 def test_importer_main_cleanup_on_exception(add_base_remotes_mock):
@@ -703,6 +843,19 @@ def test_import_unapplied_spi_quilt_patches(
     ],
     [
         pytest.param(
+            repo_builder.Repo(),
+            ['1-1'],
+            {
+                'add_commits': [
+                    repo_builder.Commit.from_spec(name='publish'),
+                ],
+                'update_tags': {'importer/import/1-1': Placeholder('publish')},
+            },
+            [
+                'refs/tags/importer/import/1-1',
+            ]
+        ),
+        pytest.param(
             repo_builder.Repo(
                 commits=[repo_builder.Commit.from_spec(name='import')],
                 tags={'importer/import/1-1': Placeholder('import')},
@@ -744,7 +897,7 @@ def test_import_unapplied_spi_quilt_patches(
                 'refs/tags/importer/import/1-3',
             ],
         ),
-        pytest.param(
+        (
             repo_builder.Repo(
                 commits=[
                     repo_builder.Commit.from_spec(name='import'),
@@ -779,7 +932,6 @@ def test_import_unapplied_spi_quilt_patches(
                 'refs/tags/importer/reimport/import/1-1/1',
                 'refs/tags/importer/import/1-2',
             ],
-            marks=pytest.mark.xfail(reason='LP: #1761332'),
         ),
     ]
 )