Merge ~racb/git-ubuntu:fix-unapplied-commits-and-tags into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

[Bryce Harrington (bryce)]

On cursory first pass look, not spotting much to flag, other than the one.
Will look more deeply and run some tests when this is no longer WIP.

[Robie Basak (racb)]

This branch is now ready for a full review please. Here's the range-diff from the previous version: https://pastebin.ubuntu.com/p/bhPqYsPk9D/

I have built and tested that qemu and bind9 import successfully. I've examined the bind9 result in detail, and the commits, tagging and parenting all look correct to me. I wasn't able to check the upload tags, as the existing upload tags are no longer treated as valid by the importer due to the hash mutations (as expected). I think it's OK to rely on our automated tests on this point however.

I've examined all the bugs being fixed by this branch. I believe that test coverage is adequate for all of them.

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

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

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

Review steps done:

√ python3 setup.py build
√ python3 setup.py check
√ ./snap-wrappers/wrappers/git-ubuntu-self-test [0]
  √ No broken requirements found.
  √ pylint passed!
  √ PASS (syntax) source-package-walker.py
  √ PASS (compilation) source-package-walker.py
  √ PASS (syntax) import-source-packages.py
  √ PASS (compilation) import-source-packages.py
  √ PASS (syntax) update-repository-alias.py
  √ PASS (compilation) update-repository-alias.py
Per-patch code review
  x 7944b5f04631e85110234d58b57fe0111dddca99
    • Doublechecked bug numbers & reports
    • Code docs for new functions validate_upload_tag() and
      find_or_create_unapplied_commit()
    x Test cases for new functions validate_upload_tag() and
      find_or_create_unapplied_commit()
  √ 2d589caba70e5bd95c638113fd69ea9eccd51b81
    • Trivial refactor
  √ f5f4fc6f1bbdf1963eae2b1b2b98bc09f9c4fccb
    • Refactoring with some behavioral adjustments, went through
      refactoring line-by-line and verified no unintended changes
  x 295572f5ea1c42e7eac15645baaee71160694c66
    • Refactoring out a function. Verified code docs, entry and exit
      points, correct return type and behavior. Checked exception
      behavior
    x One whitespace issue, mentioned inline
    x Test case for get_unapplied_import_parents()
  √ 8c01fa3fda0407c11b35f5cf052397f2dccbce57
    • Verified each refactoring step does not leave any variables
      undefined, or introduce any potential behavioral changes

Solid work, I did a pretty thorough code review looking to nitpick but other than one whitespace error found nothing to mention. I especially love seeing the three functions broken out, that helps immensely as some of the importer code is quite lengthy, and this enables more discrete testing to be done. But speaking of which, and I hate to mention it - shouldn't the new functions come with their own test cases?

review: Needs Fixing

[Robie Basak (racb)]

> But speaking of which, and I hate to mention it - shouldn't the new functions come with their own test cases?

I'll consider/discuss this further, but for now, a test coverage report gives me:

gitubuntu/importer.py 609 250 59% 76-78, 118-119, 166, 171-383, 497-528, 641, 726, 754-755, 769-770, 776-780, 786-787, 796, 798, 823-824, 857-858, 869, 884, 892, 953, 981-1001, 1071, 1083, 1089, 1191-1217, 1513-1526, 1665-1666, 1691-1693, 1817-1820, 1828-1832, 1892, 1915-1918, 1941-1947, 2020-2077, 2081-2135, 2138-2161

Looking through these, the areas missed that are relevant to this branch are:

1513-1526 validate_upload_tag when there are changelog_parents
1665-1666 import_unapplied_dsc when ignoring GitUbuntuImportOrigError exception
1691-1693 import_unapplied_dsc when exiting early on ParentOverrideError exception

I'll work on full unit tests for validate_upload_tag first, as that is missing coverage currently so needs doing regardless.

[Robie Basak (racb)]

Thank you for the review! As discussed elsewhere, validate_upload_tag did have inadequate testing and was indeed broken. I have fixed the implementation and added tests for this, giving us full coverage for the area of code affected by this branch except for the "ignoring" and "existing early" cases mentioned in the previous comment. I think these are OK to leave for now - they weren't tested before, haven't changed in behaviour and are minor edge cases anyway. I also squashed in some minor docstring fixes/improvements. I'm leaving unit tests for the remaining added functions as tech-debt for now, on the basis that they do have full test coverage in the integration tests for now, and the right thing to do is probably to move those existing integration tests "down the stack" to become unit tests anyway once that is possible with further refactoring.

To fix the implementation I had to add GitUbuntuRepository.descendant_of which I did in a separate commit.

I would like to review the current test cases to see if I can exercise the previously-untouched areas of validate_upload_tag in an integration test. I'll do this on Monday. Up to you if you're happy to land this branch before I do that (in which case I'll do that in a separate branch if required), or if you'd like to see that in this branch (in which case I'll add a new commit to this branch to make that addition, but the commits already here should be OK to review again now).

Here's the range diff: http://paste.ubuntu.com/p/vcCdtPf5kd/

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

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

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

The new test looks well thought out and a good addition.

I'm fine with seeing these commits land as is, and followup with any additional changes to validate_upload_tag() subsequently. The only consideration on waiting would be if the additional test work reveals issues you'd prefer to squash into the earlier patches. Otherwise, the main thing is getting all the needed bits landed before the release.

I agree that breaking the existing integration tests up into lower level tests would be a good improvement, but can be left as a future followup task for now. Thanks for doublechecking that the existing testsuite covers things enough already.

Btw, I meant to mention when we chatted, that I noticed when I ran scott's nifty snap selftest, it ended with an error. However, the error was unrelated to the tests, I think it was in report generation or code cleanup. Only mentioning it in case you run into it too.

Anyway, this branch has an impressive amount of effort invested in it, and I'm really looking forward to seeing its release. +1 and good luck!

review: Approve

[Robie Basak (racb)]

On Fri, Mar 27, 2020 at 05:59:48PM -0000, Robie Basak wrote:
> I would like to review the current test cases to see if I can exercise
> the previously-untouched areas of validate_upload_tag in an
> integration test. I'll do this on Monday.

I've looked into this further. The current integration tests all use an
import of '1-1' to test the tagging behaviour following an import. So
they do not cover upload tag validation at all, since that generally
relies on changelog parents existing. This is not a mistake.

I think it's sufficient that we now have unit tests for upload tag
validation, and don't see a benefit in integration tests for this
specific case (nothing else affects upload tag validation). I don't
think we need to add additional integration tests to cover upload tag
validation unless specific use cases arise as broken.

So I think this MP is good to merge as-is, with no related further work
apart from the tech-debt already identified.

Thank you Bryce for the reviews!

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index a0ae2e6..49bd9b8 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -1746,6 +1746,7 @@ class GitUbuntuRepository:
         return self.get_tag_reference(orphan_tag(version, namespace))
 
     def create_tag(self, commit_hash, tag_name, tag_msg):
+        logging.debug("Creating tag %s pointing to %s", tag_name, commit_hash)
         self.git_run(
             [
                 'tag',
@@ -2683,3 +2684,46 @@ with other relevant fields (see below for details).
             return ''
 
         return merge_base_commit_hash
+
+    def descendant_of(self, a, b):
+        """Report if one commit is a descendant of another
+
+        This is intended to behave identically to a pygit2.Repository method
+        that exists in pygit2 0.27.2 and newer (since upstream pygit2 commit
+        bbaff28), but it is not present in the current git-ubuntu setup.py
+        install_requires version 0.24.2, so we work around that with this
+        implementation here instead.
+
+        After updating pygit2 to >= 0.27.2, it should be possible to change all
+        callers to call the descendant_of method of our raw_repo attribute
+        instead, and then remove this method.
+
+        :param pygit2.Commit a: one commit
+        :param pygit2.Commit b: another commit
+        :rtype: bool
+        :returns: True if a is a descendant of b, False otherwise. A commit is
+            not treated as a descendant of itself.
+        """
+        if a.id == b.id:
+            # The docstring says we should return False in this edge case, but
+            # --is-ancestor returns True in the same (inverted) edge case, so
+            # we explicitly return False here.
+            return False
+
+        try:
+            self.git_run(
+                [
+                    'merge-base',
+                    '--is-ancestor',
+                    str(b.id),
+                    str(a.id),
+                ],
+                verbose_on_failure=False,
+            )
+        except CalledProcessError as e:
+            if e.returncode == 1:
+                return False
+            else:
+                raise
+        else:
+            return True
diff --git a/gitubuntu/git_repository_test.py b/gitubuntu/git_repository_test.py
index a212080..1cfbc4c 100644
--- a/gitubuntu/git_repository_test.py
+++ b/gitubuntu/git_repository_test.py
@@ -912,3 +912,65 @@ def test_commit_tree(repo):
             offset=-480,
         )
     )
+
+
[e-mail](['a', 'b', 'expected'],
+    [
+        ('root', 'root', False),
+        ('child1', 'root', True),
+        ('root', 'child1', False),
+        ('grandchild1', 'root', True),
+        ('child1', 'child2', False),
+        ('root', 'disjoint', False),
+    ]
+)
+def test_descendant_of(repo, a, b, expected):
+    """
+    General test for GitUbuntuRepository.descendant_of().
+
+    This unit tests validate_upload_tag() for various parameterized cases. The
+    paramater sets assume the repository structure encoded in the Repo() call
+    below.
+
+    :param GitUbuntuRepository repo: fixture providing a temporary
+        GitUbuntuRepository instance to use
+    :param str a: tag name of the first commit to pass to descendant_of()
+    :param str b: tag name of the second commit to pass to descendant_of()
+    :param bool expected: the expected result of descendant_of()
+    """
+    Repo(
+        # Unique message parameters are used in each entry here in order to
+        # ensure that otherwise-identical commits do not end up being the same
+        # commit (with the same hash, etc).
+        commits=[
+            Commit(name='root', message='1'),
+            Commit(name='child1', parents=[Placeholder('root')], message='2'),
+            Commit(name='child2', parents=[Placeholder('root')], message='3'),
+            Commit(
+                name='grandchild1',
+                parents=[Placeholder('child1')],
+                message='4'
+            ),
+            Commit(name='disjoint', message='5'),
+        ],
+        tags={
+            'root': Placeholder('root'),
+            'child1': Placeholder('child1'),
+            'child2': Placeholder('child2'),
+            'grandchild1': Placeholder('grandchild1'),
+            'disjoint': Placeholder('disjoint'),
+        }
+    ).write(repo.raw_repo)
+
+    def find_commit(tag_name):
+        '''
+        Fetch the pygit2.Commit object tagged with the given name
+
+        :param str tag_name: the name of the tag to look up
+        :returns: the commit object tagged with the given name
+        :rtype: pygit2.Commit
+        '''
+        tag = repo.raw_repo.lookup_reference('refs/tags/%s' % tag_name)
+        return tag.peel(pygit2.Commit)
+
+    assert repo.descendant_of(find_commit(a), find_commit(b)) is expected
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 495a22d..6d546bc 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -1416,6 +1416,205 @@ def get_changelog_parent_commits(
     return []
 
 
+def get_unapplied_import_parents(
+    repo,
+    version,
+    namespace,
+    parent_overrides,
+    unapplied_import_tree_hash,
+):
+    """
+    Determine the parents to be used for a synthesized unapplied import commit.
+
+    :param gitubuntu.GitUbuntuRepository repo: the repository that is the
+        target of this import.
+    :param str version: the changelog version of the package being imported.
+    :param str namespace: the namespace under which relevant refs can be found.
+    :param dict parent_overrides: see parse_parentfile()
+    :param str unapplied_import_tree_hash: the hash of tree object in the
+        repository of the source tree being imported.
+    :rtype: list(str)
+    :returns: a list of commit hashes to use as the parents of the unapplied
+        import commit.
+    :raises ParentOverrideError: if there is a problem in finding a parent
+        specified by a parent override.
+    """
+    if version in parent_overrides:
+        logging.debug(
+            '%s is specified in the parent override file.',
+            version
+        )
+
+        return override_parents(
+            parent_overrides,
+            repo,
+            version,
+            namespace,
+        )[0]
+    else:
+        import_tree_versions = repo.get_all_changelog_versions_from_treeish(
+            unapplied_import_tree_hash
+        )
+        changelog_version = import_tree_versions[0]
+
+        logging.debug(
+            "Found changelog version %s in newly imported tree" %
+            changelog_version
+        )
+
+        # sanity check on spph and d/changelog agreeing on the latest version
+        if version not in parent_overrides:
+            assert version == changelog_version, (
+                'source pkg version: {} != changelog version: {}'.format(
+                    version, changelog_version))
+
+        return get_changelog_parent_commits(
+            repo,
+            namespace,
+            parent_overrides,
+            import_tree_versions,
+            patch_state=PatchState.UNAPPLIED,
+        )
+
+
+def validate_upload_tag(
+    repo,
+    upload_tag,
+    import_tree,
+    changelog_parents,
+    version,
+):
+    """Validate an upload tag according to the spec.
+
+    :param gitubuntu.GitUbuntuRepository repo: the repository that is the
+        target of this import.
+    :param pygit2.Reference upload_tag: the upload tag to be validated.
+    :param pygit2.Tree import_tree: the upload published by Launchpad.
+    :param list(str) changelog_parents: the changelog parent commit hash
+        strings of the upload published by Launchpad.
+    :param str version: the package version string of the upload published by
+        Launchpad.
+    :rtype: bool
+    :returns: True if the upload tag validates; False otherwise.
+    """
+    if upload_tag.peel().tree.id != import_tree.id:
+        logging.warn(
+            "Found upload tag %s, but the corresponding tree "
+            "does not match the published version. Will import %s as "
+            "normal, ignoring the upload tag.",
+            repo.tag_to_pretty_name(upload_tag),
+            version,
+        )
+        return False
+
+    if not changelog_parents:
+        return True
+
+    has_changelog_parent_ancestor = any(
+        repo.descendant_of(upload_tag.peel(), repo.raw_repo.get(commit))
+        for commit in changelog_parents
+    )
+    if not has_changelog_parent_ancestor:
+        logging.warn(
+            "Found upload tag %s, but no changelog parent is its ancestor. "
+            "Will import %s as normal, ignoring the upload tag.",
+            repo.tag_to_pretty_name(upload_tag),
+            version,
+        )
+        return False
+
+    return True
+
+
+def find_or_create_unapplied_commit(
+    repo,
+    version,
+    namespace,
+    import_tree,
+    parent_overrides,
+    commit_date,
+    head_name,
+):
+    """Return the commit to be used for an unapplied import
+
+    If an appropriate commit can already be found in the repository, return it.
+    Otherwise, synthesize a new commit and return that.
+
+    An appropriate commit is either the previous import of this package version
+    (if its source tree matches exactly), or a valid rich history commit left
+    by an uploader.
+
+    If this import previously existed and its commit is being returned, then
+    also return the corresponding import tag. Otherwise no tag is returned.
+    Note that this is the import tag, and not related to upload tags. If a
+    previous import didn't exist and an upload tag exists, this function
+    nevertheless returns no tag.
+
+    :param gitubuntu.GitUbuntuRepository repo: the repository that is the
+        target of this import.
+    :param str version: the changelog version of the package being imported.
+    :param str namespace: the namespace under which relevant refs can be found.
+    :param pygit2.Tree import_tree: the tree object in the repository of the
+        source tree being imported.
+    :param dict parent_overrides: see parse_parentfile()
+    :param str commit_date: committer date to use for the git commit metadata.
+        If None, use the current date.
+    :param str head_name: git branch name that is the target of this import.
+        This is currently used in the generated commit message, but will go
+        away when the commit messages are overhauled to the new spec.
+    :rtype: tuple(pygit2.Commit, pygit2.Reference or None)
+    :returns: the commit that has been found or created, and the import tag
+        corresponding to an existing import if it had previously existed.
+    """
+    import_tags = get_existing_import_tags(repo, version, namespace)
+    for candidate_tag in import_tags:
+        if candidate_tag.peel().tree.id == import_tree.id:
+            # An existing import tag matches our imported tree, so we can use
+            # this tag and corresponding commit instead of creating a new
+            # commit
+            return candidate_tag.peel(), candidate_tag
+
+    changelog_parents = get_unapplied_import_parents(
+        repo=repo,
+        version=version,
+        namespace=namespace,
+        parent_overrides=parent_overrides,
+        unapplied_import_tree_hash=str(import_tree.id),
+    )
+
+    # Try looking for a valid upload tag
+    upload_tag = repo.get_upload_tag(version, namespace)
+    if upload_tag:
+        upload_tag_validates = validate_upload_tag(
+            repo=repo,
+            upload_tag=upload_tag,
+            import_tree=import_tree,
+            changelog_parents=changelog_parents,
+            version=version,
+        )
+        if upload_tag_validates:
+            return upload_tag.peel(), None
+
+    # Create a commit
+    commit = repo.raw_repo.get(commit_unapplied_patches_import(
+        repo=repo,
+        version=version,
+        head_name=head_name,
+        import_tree_hash=str(import_tree.id),
+        namespace=namespace,
+        changelog_parent_commits=changelog_parents,
+        upload_parent_commit=None,
+        commit_date=commit_date,
+    ))
+    logging.debug(
+        "Committed patches-unapplied import of %s as %s in %s",
+        version,
+        str(commit.id),
+        head_name.partition('%s/' % namespace)[2],
+    )
+    return commit, None
+
+
 def import_unapplied_dsc(
     repo,
     version,
@@ -1447,9 +1646,6 @@ def import_unapplied_dsc(
 
     :rtype None
     """
-    unapplied_tip_head = repo.get_head_by_name(head_name)
-    _, _, pretty_head_name = head_name.partition('%s/' % namespace)
-
     import_dsc(
         repo,
         GitUbuntuDsc(dsc_pathname),
@@ -1482,180 +1678,31 @@ def import_unapplied_dsc(
         unapplied_import_tree_hash
     )
 
-    import_tree_versions = repo.get_all_changelog_versions_from_treeish(
-        unapplied_import_tree_hash
-    )
-    changelog_version = import_tree_versions[0]
-
-    logging.debug(
-        "Found changelog version %s in newly imported tree" %
-        changelog_version
-    )
-
-    # sanity check on spph and d/changelog agreeing on the latest version
-    if version not in parent_overrides:
-        assert version == changelog_version, (
-            'source pkg version: {} != changelog version: {}'.format(
-                version, changelog_version))
-
-    unapplied_tip_version = None
-    # check if the version to import has already been imported to
-    # this head
-    if unapplied_tip_head is not None:
-        if repo.treeishs_identical(
-            unapplied_import_tree_hash, str(unapplied_tip_head.peel().id)
-        ):
-            logging.warn('%s is identical to %s',
-                         pretty_head_name, version
-                        )
-            return
-        unapplied_tip_version, _ = repo.get_changelog_versions_from_treeish(
-                             str(unapplied_tip_head.peel().id),
-                                                                 )
-
-    logging.debug('Tip version is %s', unapplied_tip_version)
-
-    upload_parent_commit = None
-
-    if version in parent_overrides:
-        logging.debug(
-            '%s is specified in the parent override file.',
-            version
-        )
-
-        try:
-            (
-                unapplied_changelog_parent_commits,
-                _
-            ) = override_parents(
-                parent_overrides,
-                repo,
-                version,
-                namespace,
-            )
-        except ParentOverrideError as e:
-            logging.error("%s" % e)
-            return
-    else:
-        if version_compare(version, unapplied_tip_version) <= 0:
-            logging.warn(
-                 "Version to import (%s) is not after %s tip (%s)",
-                 version,
-                 pretty_head_name,
-                 unapplied_tip_version,
-            )
-
-        unapplied_changelog_parent_commits = get_changelog_parent_commits(
-            repo,
-            namespace,
-            parent_overrides,
-            import_tree_versions,
-            patch_state=PatchState.UNAPPLIED,
+    try:
+        commit, tag = find_or_create_unapplied_commit(
+            repo=repo,
+            version=version,
+            namespace=namespace,
+            import_tree=repo.raw_repo.get(unapplied_import_tree_hash),
+            parent_overrides=parent_overrides,
+            commit_date=commit_date,
+            head_name=head_name,
         )
+    except ParentOverrideError as e:
+        logging.error("%s" % e)
+        return
 
-    # check if the version to import has already been uploaded to
-    # this head -- we leverage the above code to perform a 'dry-run'
-    # of the import tree, to obtain the changelog parent
-    existing_upload_tag = repo.get_upload_tag(version, namespace)
-    existing_import_tag = repo.get_import_tag(version, namespace)
-    existing_orphan_tag = repo.get_orphan_tag(version, namespace)
-    if existing_import_tag:
-        if str(existing_import_tag.peel().tree.id) != unapplied_import_tree_hash:
-            logging.error(
-                "Found import tag %s, but the corresponding tree "
-                "does not match the published version. Will orphan commit.",
-                repo.tag_to_pretty_name(existing_import_tag),
-            )
-            unapplied_changelog_parent_commits = []
-        else:
-            repo.update_head_to_commit(
-                head_name,
-                str(existing_import_tag.peel().id),
-            )
-            return
-    elif existing_orphan_tag:
-        if str(existing_orphan_tag.peel().tree.id) != unapplied_import_tree_hash:
-            raise Exception(
-                "Found orphan tag %s, but the corresponding tree "
-                "does not match the published version. We cannot "
-                "currently support multiple orphan tags for the same "
-                "published version and will now exit.",
-                repo.tag_to_pretty_name(existing_orphan_tag),
-            )
-        else:
-            repo.update_head_to_commit(
-                head_name,
-                str(existing_orphan_tag.peel().id),
-            )
-            return
-    elif existing_upload_tag:
-        if str(existing_upload_tag.peel().tree.id) != unapplied_import_tree_hash:
-            logging.warn(
-                "Found upload tag %s, but the corresponding tree "
-                "does not match the published version. Will import %s as "
-                "normal, ignoring the upload tag.",
-                repo.tag_to_pretty_name(existing_upload_tag),
-                version,
-            )
-        else:
-            upload_parent_commit = str(existing_upload_tag.peel().id)
-
-            if unapplied_changelog_parent_commits:
-                try:
-                    repo.git_run(
-                        [
-                            'merge-base',
-                            '--is-ancestor',
-                            unapplied_changelog_parent_commits[0],
-                            upload_parent_commit,
-                        ],
-                        verbose_on_failure=False,
-                    )
-                    unapplied_changelog_parent_commits = []
-                except CalledProcessError as e:
-                    if e.returncode != 1:
-                        raise
-
-    commit_hash = commit_unapplied_patches_import(
-        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",
-        version,
-        commit_hash,
-        pretty_head_name,
-    )
-
-    tag = None
-    if repo.get_import_tag(version, namespace) is None:
-        # Not imported before
-        tag = import_tag(version, namespace)
-    elif (
-        not unapplied_changelog_parent_commits and
-        upload_parent_commit is None and
-        head_name in repo.local_branch_names
-    ):
-        # No parents and not creating a new branch
-        tag = orphan_tag(version, namespace)
-
-    if tag:
-        logging.debug("Creating tag %s pointing to %s", tag, commit_hash)
-        repo.create_tag(
-            commit_hash=commit_hash,
-            tag_name=tag,
-            tag_msg=get_import_tag_msg(),
+    if not tag:
+        create_import_tag(
+            repo=repo,
+            commit_hash=str(commit.id),
+            version=version,
+            namespace=namespace,
         )
 
     repo.update_head_to_commit(
         head_name,
-        commit_hash,
+        str(commit.id),
     )
 
 # imports a package based upon source package information
diff --git a/gitubuntu/importer_tag_test.py b/gitubuntu/importer_tag_test.py
index c6e824f..a216e73 100644
--- a/gitubuntu/importer_tag_test.py
+++ b/gitubuntu/importer_tag_test.py
@@ -122,8 +122,6 @@ from gitubuntu.importer_test import MockSPI, patch_get_commit_environment
             ],
             # reuse:
             True,
-
-            marks=pytest.mark.xfail(reason='LP: #1761331'),
         ),
 
         # 2) An existing import tag with a different Git tree and an existing
@@ -175,8 +173,6 @@ from gitubuntu.importer_test import MockSPI, patch_get_commit_environment
             ],
             # reuse:
             True,
-
-            marks=pytest.mark.xfail(reason='LP: #1754194'),
         ),
 
         # 3) An existing import tag with a different Git tree and an existing
@@ -235,8 +231,6 @@ from gitubuntu.importer_test import MockSPI, patch_get_commit_environment
             ],
             # reuse:
             False,
-
-            marks=pytest.mark.xfail(reason='LP: #1754507'),
         ),
 
         # 4) An existing import tag with a different Git tree and no upload tag
@@ -286,8 +280,6 @@ from gitubuntu.importer_test import MockSPI, patch_get_commit_environment
             ],
             # reuse:
             False,
-
-            marks=pytest.mark.xfail(reason='LP: #1754706'),
         ),
 
         # 5) No import tag and an existing upload tag with the same Git tree
@@ -322,8 +314,6 @@ from gitubuntu.importer_test import MockSPI, patch_get_commit_environment
             ],
             # reuse:
             True,
-
-            marks=pytest.mark.xfail(reason='LP: #1734883'),
         ),
 
         # 6) No import tag and an existing upload tag with a different Git tree
diff --git a/gitubuntu/importer_test.py b/gitubuntu/importer_test.py
index 95744d9..54f1f2b 100644
--- a/gitubuntu/importer_test.py
+++ b/gitubuntu/importer_test.py
@@ -722,6 +722,146 @@ def test_importer_close_repository_on_exception(main_with_repo_mock):
     repo.close.assert_called()
 
 
[e-mail](
+    [
+        'input_repo',
+        'published_spec',
+        'expected_result',
+    ],
+    [
+        (
+            # Common case: upload tag has a changelog parent as an ancestor
+            repo_builder.Repo(
+                commits=[
+                    repo_builder.Commit.from_spec(name='import'),
+                    repo_builder.Commit.from_spec(
+                        name='upload',
+                        changelog_versions=['1-2', '1-1'],
+                        parents=[Placeholder('import')],
+                    ),
+                ],
+                tags={
+                    'importer/import/1-1': Placeholder('import'),
+                    'importer/upload/1-2': Placeholder('upload'),
+                },
+            ),
+            {'changelog_versions': ['1-2', '1-1']},
+            True,
+        ),
+        (
+            # Upload tag is the first one, with no parents present
+            repo_builder.Repo(
+                commits=[
+                    repo_builder.Commit.from_spec(
+                        name='upload',
+                        version='1-2',
+                    ),
+                ],
+                tags={
+                    'importer/upload/1-2': Placeholder('upload'),
+                },
+            ),
+            {'changelog_versions': ['1-2']},
+            True,
+        ),
+        (
+            # Upload tag mismatches published tree but is otherwise correct
+            repo_builder.Repo(
+                commits=[
+                    repo_builder.Commit.from_spec(name='import'),
+                    repo_builder.Commit.from_spec(
+                        name='upload',
+                        changelog_versions=['1-2', '1-1'],
+                        parents=[Placeholder('import')],
+                        mutate=True,
+                    ),
+                ],
+                tags={
+                    'importer/import/1-1': Placeholder('import'),
+                    'importer/upload/1-2': Placeholder('upload'),
+                },
+            ),
+            {'changelog_versions': ['1-2', '1-1']},
+            False,
+        ),
+        (
+            # Upload tag doesn't have a changelog parent as an ancestor but is
+            # otherwise correct
+            repo_builder.Repo(
+                commits=[
+                    repo_builder.Commit.from_spec(name='import'),
+                    repo_builder.Commit.from_spec(
+                        name='upload',
+                        changelog_versions=['1-2', '1-1'],
+                    ),
+                ],
+                tags={
+                    'importer/import/1-1': Placeholder('import'),
+                    'importer/upload/1-2': Placeholder('upload'),
+                },
+            ),
+            {'changelog_versions': ['1-2', '1-1']},
+            False,
+        ),
+    ],
+)
+def test_validate_upload_tag(
+    repo,
+    input_repo,
+    published_spec,
+    expected_result,
+):
+    """
+    General test for validate_upload_tag().
+
+    This unit tests validate_upload_tag() for various parameterized cases.
+    Given an input repository and the specification of a Launchpad publication
+    of a source package, we check that validate_upload_tag() correctly accepts
+    or rejects the upload tag named 'importer/upload/1-2'. It is assumed that
+    the package being imported is always of version '1-2' for all parameter
+    sets.
+
+    Since the target function requires an upload tag, the case of there not
+    being an upload tag does not need to be tested here, as it wouldn't be
+    called in this case.
+
+    :param GitUbuntuRepository repo: fixture providing a temporary
+        GitUbuntuRepository instance to use
+    :param repo_builder.Repo input_repo: input repository data
+    :param dict published_spec: the package simulated being imported from the
+        archive, specified as a dict to pass as **kwargs to
+        repo_builder.Commit.from_spec()
+    :param bool expected_result: the expected return value of the call to
+        validate_upload_tag()
+    """
+    input_repo.write(repo.raw_repo)
+
+    upload_tag = repo.raw_repo.lookup_reference(
+        'refs/tags/importer/upload/1-2',
+    )
+    upload_tree = upload_tag.peel(pygit2.Tree)
+
+    published_commit_builder = repo_builder.Commit.from_spec(**published_spec)
+    published_commit_oid = published_commit_builder.write(repo.raw_repo)
+    published_tree = repo.raw_repo.get(published_commit_oid).peel(pygit2.Tree)
+
+    changelog_parents = target.get_unapplied_import_parents(
+        repo=repo,
+        version='1-2',
+        namespace='importer',
+        parent_overrides={},
+        unapplied_import_tree_hash=str(published_tree.id),
+    )
+    result = target.validate_upload_tag(
+        repo=repo,
+        upload_tag=upload_tag,
+        changelog_parents=changelog_parents,
+        import_tree=published_tree,
+        version='1-2',
+    )
+    assert result == expected_result
+
+
 def patch_get_commit_environment(repo):
     """Patch a repository to use constant metadata
 
diff --git a/setup.py b/setup.py
index b66dbba..2de1615 100644
--- a/setup.py
+++ b/setup.py
@@ -20,7 +20,9 @@ setup(name='gitubuntu',
     #
     # 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.
+    #     function for details. When >= 0.27.2,
+    #     git_repository.py::GitUbuntuRepository.descendant_of can be removed
+    #     and callers adjusted. See the docstring in that function for details.
 
     install_requires=[
         'argcomplete==1.8.1',