Merge ~nacc/git-ubuntu:importer-reimport-tags-lp1755247 into git-ubuntu:master

[Nish Aravamudan]

Make jenkins happy.

[Robie Basak (racb)]

22:24 <rbasak> nacc: import/reimport creates an edge case in case of a package versioned "import" which is odd but I think may otherwise be valid (for a native package).
22:25 <rbasak> nacc: how about import/<version>, reimport/import/<version>, reimport/applied/<version>?
22:25 <rbasak> IOW, prefix reimport.
22:26 <rbasak> It's a rare edge case that requires it to exist at all, so flagging that it exists at the top level feels reasonable.

Specifically I'd like to avoid ever putting version strings into the same namespace as our specially named stuff, since that could collide. So whenever we have foo/<version>, we should forbid foo/bar where bar is anything else we define.

[Nish Aravamudan (nacc)]

Implemented as above.

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

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

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

review: Approve (continuous-integration)

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

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

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

review: Approve (continuous-integration)

[Robie Basak (racb)]

+1 to the new SPECIFICATION.tags from commit cf7f261.

[Andreas Hasenack (ahasenack)]

An inline comment about the test for creating applied/import tags, which probably falls into the category of test improvements. If you think it's fine to stay as it is, +1.

I would also suggest to rename the get_possible_{import,applied}_tags() method to get_existing_{import,applied}_tags(). Maybe by "possible" you mean "get me tags that could be import/applied" tags? Anyway, as you prefer, it's just a suggegstion.

review: Approve

[Andreas Hasenack (ahasenack)]

Thanks for the changes, +1 again

review: Approve

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

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

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

review: Approve (continuous-integration)

diff --git a/doc/SPECIFICATION.tags b/doc/SPECIFICATION.tags
new file mode 100644
index 0000000..a1e8750
--- /dev/null
+++ b/doc/SPECIFICATION.tags
@@ -0,0 +1,20 @@
+For the original bug reimport requiring this change, please see LP: #1755247.
+
+In the common case, we just need one tag: import/<version>, escaped according
+to DEP-14. In edge cases, we sometimes find that a publication has a different
+tree that must appear at a different place in the commit graph, so we need to
+reimport the same (changelog) "version" multiple times even though they have
+different contents. In this case, we use the following scheme:
+
+- By definition, import/<version> already exists since we only use this scheme
+  when it already exists and need to tag another import with the same version
+  string.
+- reimport/import/<version>/0 is created if it doesn’t already exist and is pointed to
+  the same commit as import/<version>. Assertion: if it does already exist, it
+  must point to import/<version> already; anything else is an importer error.
+- reimport/import/<version>/1 is created and points to the new commit as required. If
+  the /1 suffix already exists, use /2, falling back to /3, /4, etc.
+
+The same naming schema applies (no pun intended) to the patches-applied tags,
+which live under applied/ and reimport/applied/ rather than import/ and
+reimport/import/.
diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 94c8f54..9f1fdea 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -741,6 +741,30 @@ def git_dep14_tag(version):
 def import_tag(version, namespace):
     return '%s/import/%s' % (namespace, git_dep14_tag(version))
 
+def reimport_tag_prefix(version, namespace):
+    return '%s/reimport/import/%s' % (
+        namespace,
+        git_dep14_tag(version),
+    )
+
+def applied_reimport_tag_prefix(version, namespace):
+    return '%s/reimport/applied/%s' % (
+        namespace,
+        git_dep14_tag(version),
+    )
+
+def reimport_tag(version, namespace, reimport):
+    return '%s/%s' % (
+        reimport_tag_prefix(version, namespace),
+        reimport,
+    )
+
+def applied_reimport_tag(version, namespace, reimport):
+    return '%s/%s' % (
+        applied_reimport_tag_prefix(version, namespace),
+        reimport,
+    )
+
 def applied_tag(version, namespace):
     return '%s/applied/%s' % (namespace, git_dep14_tag(version))
 
@@ -1638,6 +1662,26 @@ class GitUbuntuRepository:
     def get_import_tag(self, version, namespace):
         return self.get_tag_reference(import_tag(version, namespace))
 
+    def get_reimport_tag(self, version, namespace, reimport):
+        return self.get_tag_reference(
+            reimport_tag(version, namespace, reimport)
+        )
+
+    def get_applied_reimport_tag(self, version, namespace, reimport):
+        return self.get_tag_reference(
+            applied_reimport_tag(version, namespace, reimport)
+        )
+
+    def get_all_reimport_tags(self, version, namespace):
+        return self.references_with_prefix(
+            'refs/tags/%s' % reimport_tag_prefix(version, namespace)
+        )
+
+    def get_all_applied_reimport_tags(self, version, namespace):
+        return self.references_with_prefix(
+            'refs/tags/%s' % applied_reimport_tag_prefix(version, namespace)
+        )
+
     def get_applied_tag(self, version, namespace):
         return self.get_tag_reference(applied_tag(version, namespace))
 
@@ -1650,6 +1694,17 @@ class GitUbuntuRepository:
     def get_orphan_tag(self, version, namespace):
         return self.get_tag_reference(orphan_tag(version, namespace))
 
+    def create_tag(self, commit_hash, tag_name, tag_msg):
+        self.git_run(
+            [
+                'tag',
+                '-a',
+                '-m', tag_msg,
+                tag_name,
+                commit_hash,
+            ]
+        )
+
     def nearest_remote_branches(self, commit_hash, prefix=None,
         max_commits=100
     ):
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 78fc3dc..a974af8 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -49,6 +49,8 @@ from gitubuntu.git_repository import (
     orphan_tag,
     applied_tag,
     import_tag,
+    reimport_tag,
+    applied_reimport_tag,
     upstream_tag,
     PristineTarError,
     is_dir_3_0_quilt,
@@ -950,6 +952,34 @@ def parse_parentfile(parentfile, pkgname):
 
     return parent_overrides
 
+
+def get_existing_import_tags(repo, version, namespace):
+    existing_import_tag = repo.get_import_tag(version, namespace)
+    if existing_import_tag:
+        # check if there are reimports
+        existing_reimport_tags = repo.get_all_reimport_tags(version, namespace)
+        if existing_reimport_tags:
+            return existing_reimport_tags
+        else:
+            return [existing_import_tag]
+    return []
+
+
+def get_existing_applied_tags(repo, version, namespace):
+    existing_applied_tag = repo.get_applied_tag(version, namespace)
+    if existing_applied_tag:
+        # check if there are reimports
+        existing_applied_reimport_tags = repo.get_all_applied_reimport_tags(
+            version,
+            namespace,
+        )
+        if existing_applied_reimport_tags:
+            return existing_applied_reimport_tags
+        else:
+            return [existing_applied_tag]
+    return []
+
+
 def override_parents(parent_overrides, repo, spi, namespace):
     """
     returns: (unapplied, applied) where both elements of the tuple are commit
@@ -1128,6 +1158,106 @@ def get_import_tag_msg():
     return 'git-ubuntu import v%s' % VERSION
 
 
+def create_applied_tag(repo, commit_hash, version, namespace):
+    tag_msg = get_import_tag_msg()
+
+    existing_applied_tag = repo.get_applied_tag(version, namespace)
+    if existing_applied_tag:
+        base_applied_reimport_tag = repo.get_applied_reimport_tag(
+            version,
+            namespace,
+            reimport=0,
+        )
+        if base_applied_reimport_tag:
+            assert (
+                base_applied_reimport_tag.peel(pygit2.Commit).id ==
+                existing_applied_tag.peel(pygit2.Commit).id
+            )
+        else:
+            repo.create_tag(
+                str(existing_applied_tag.peel(pygit2.Commit).id),
+                applied_reimport_tag(version, namespace, reimport=0),
+                tag_msg,
+            )
+        num_existing_applied_reimports = len(
+            repo.get_all_applied_reimport_tags(version, namespace)
+        )
+        assert not repo.get_applied_reimport_tag(
+            version,
+            namespace,
+            reimport=num_existing_applied_reimports,
+        )
+        repo.create_tag(
+            commit_hash,
+            applied_reimport_tag(
+                version,
+                namespace,
+                reimport=num_existing_applied_reimports,
+            ),
+            tag_msg,
+        )
+        return repo.get_applied_reimport_tag(
+            version,
+            namespace,
+            reimport=num_existing_applied_reimports,
+        )
+    else:
+        repo.create_tag(
+            commit_hash,
+            applied_tag(version, namespace),
+            tag_msg,
+        )
+        return repo.get_applied_tag(version, namespace)
+
+
+def create_import_tag(repo, commit_hash, version, namespace):
+    tag_msg = get_import_tag_msg()
+
+    existing_import_tag = repo.get_import_tag(version, namespace)
+    if existing_import_tag:
+        base_reimport_tag = repo.get_reimport_tag(
+            version,
+            namespace,
+            reimport=0,
+        )
+        if base_reimport_tag:
+            assert (
+                base_reimport_tag.peel(pygit2.Commit).id ==
+                existing_import_tag.peel(pygit2.Commit).id
+            )
+        else:
+            repo.create_tag(
+                str(existing_import_tag.peel(pygit2.Commit).id),
+                reimport_tag(version, namespace, reimport=0),
+                tag_msg,
+            )
+        num_existing_reimports = len(
+            repo.get_all_reimport_tags(version, namespace)
+        )
+        assert not repo.get_reimport_tag(
+            version,
+            namespace,
+            reimport=num_existing_reimports,
+        )
+        repo.create_tag(
+            commit_hash,
+            reimport_tag(version, namespace, reimport=num_existing_reimports),
+            tag_msg,
+        )
+        return repo.get_reimport_tag(
+            version,
+            namespace,
+            reimport=num_existing_reimports,
+        )
+    else:
+        repo.create_tag(
+            commit_hash,
+            import_tag(version, namespace),
+            tag_msg,
+        )
+        return repo.get_import_tag(version, namespace)
+
+
 # imports a package based upon source package information
 def import_unapplied_spi(
     repo,
diff --git a/gitubuntu/repo_comparator.py b/gitubuntu/repo_comparator.py
new file mode 100644
index 0000000..d65b431
--- /dev/null
+++ b/gitubuntu/repo_comparator.py
@@ -0,0 +1,56 @@
+from enum import Enum
+import tempfile
+
+import pygit2
+
+class RepoComparisonType(Enum):
+    Tree = 1
+    Commit = 2
+
+def equals(repoA, repoB, test_refs, comparison_type=RepoComparisonType.Commit):
+    """Compare two pygit2.Repository objects for equality
+
+    :param pygit2.Repository repoA The first repository to compare
+    :param gitubuntu.repo_builder.Repo repoB The second repository to compare
+    :param list(str) test_refs String reference names that should be
+        compared for equality
+    :param RepoComparisonType comparison_type What underlying Git object
+        should be compared for equality
+
+    :rtype bool
+    :returns Whether @repoA and @repoB contain the matching references and
+    reachable Git commits
+    """
+
+    test_refs = set(test_refs)
+
+    with tempfile.TemporaryDirectory() as tmp_dir:
+        pygit2_repoB = pygit2.init_repository(tmp_dir)
+        repoB.write(pygit2_repoB)
+
+        repoA_refs = set(repoA.listall_references())
+        if not test_refs.issubset(repoA_refs):
+            return False
+        repoA_refs = repoA_refs.intersection(test_refs)
+        repoB_refs = set(pygit2_repoB.listall_references())
+        if not test_refs.issubset(repoB_refs):
+            return False
+        repoB_refs = repoB_refs.intersection(test_refs)
+
+        if repoA_refs.symmetric_difference(repoB_refs):
+            return False
+
+        if comparison_type == RepoComparisonType.Tree:
+            peel_type = pygit2.Tree
+        elif comparison_type == RepoComparisonType.Commit:
+            peel_type = pygit2.Commit
+        else:
+            raise TypeError("Unknown RepoComparisonType: %r" % comparison_type)
+
+        for ref in repoA_refs:
+            refA_peeled = repoA.lookup_reference(ref).peel(peel_type)
+            refB_peeled = pygit2_repoB.lookup_reference(ref).peel(peel_type)
+            if str(refA_peeled.id) != str(refB_peeled.id):
+                return False
+
+        return True
diff --git a/gitubuntu/test_importer.py b/gitubuntu/test_importer.py
index 7fd5f45..a4e94d1 100644
--- a/gitubuntu/test_importer.py
+++ b/gitubuntu/test_importer.py
@@ -6,8 +6,8 @@ from unittest.mock import (
 
 import pytest
 
-
 import gitubuntu.repo_builder as repo_builder
+import gitubuntu.repo_comparator as repo_comparator
 import gitubuntu.source_builder as source_builder
 
 import gitubuntu.importer as target
@@ -148,3 +148,349 @@ def test_get_import_commit_msg(
             upload_parent_commit,
             unapplied_parent_commit,
         ) == expected
+
+
[e-mail](
+    'input_data, expected', [
+        (
+            repo_builder.Repo(
+                commit_list=[],
+                branches={},
+                tags={},
+            ),
+            [],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[],
+                branches={},
+                tags={
+                    'importer/import/1-1': repo_builder.Commit(repo_builder.Tree({})),
+                },
+            ),
+            ['refs/tags/importer/import/1-1'],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='import'
+                    ),
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport1'
+                    ),
+                ],
+                branches={},
+                tags={
+                    'importer/import/1-1': repo_builder.Commit(repo_builder.Tree({})),
+                    'importer/reimport/import/1-1/0': repo_builder.Commit(repo_builder.Tree({})),
+                    'importer/reimport/import/1-1/1': repo_builder.Commit(repo_builder.Tree({})),
+                },
+            ),
+            [
+                'refs/tags/importer/reimport/import/1-1/0',
+                'refs/tags/importer/reimport/import/1-1/1',
+            ],
+        ),
+    ],
+)
+def test_get_existing_import_tags(repo, input_data, expected):
+    input_data.write(repo.raw_repo)
+
+    assert [
+        ref.name for ref in
+        target.get_existing_import_tags(repo, '1-1', 'importer')
+    ] == expected
+
+
[e-mail](
+    'input_data, expected', [
+        (
+            repo_builder.Repo(
+                commit_list=[],
+                branches={},
+                tags={},
+            ),
+            [],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[],
+                branches={},
+                tags={
+                    'importer/applied/1-1': repo_builder.Commit(repo_builder.Tree({})),
+                },
+            ),
+            ['refs/tags/importer/applied/1-1'],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='applied'
+                    ),
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport1'
+                    ),
+                ],
+                branches={},
+                tags={
+                    'importer/applied/1-1': repo_builder.Commit(repo_builder.Tree({})),
+                    'importer/reimport/applied/1-1/0': repo_builder.Commit(repo_builder.Tree({})),
+                    'importer/reimport/applied/1-1/1': repo_builder.Commit(repo_builder.Tree({})),
+                },
+            ),
+            [
+                'refs/tags/importer/reimport/applied/1-1/0',
+                'refs/tags/importer/reimport/applied/1-1/1',
+            ],
+        ),
+    ],
+)
+def test_get_existing_applied_tags(repo, input_data, expected):
+    input_data.write(repo.raw_repo)
+
+    assert [
+        ref.name for ref in
+        target.get_existing_applied_tags(repo, '1-1', 'importer')
+    ] == expected
+
+
[e-mail](
+    'input_data, expected_changes, test_refs', [
+        (
+            repo_builder.Repo(
+                commit_list=[],
+                branches={},
+                tags={},
+            ),
+            {
+                'commit_list_to_add': [
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='import'
+                    ),
+                ],
+                'tags_to_add': {
+                    'importer/import/1-1': repo_builder.Placeholder('import'),
+                },
+                'branches_to_update': {
+                },
+            },
+            [
+                'refs/tags/importer/import/1-1',
+            ],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='import'
+                    ),
+                ],
+                branches={},
+                tags={
+                    'importer/import/1-1': repo_builder.Placeholder('import'),
+                },
+            ),
+            {
+                'commit_list_to_add': [
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport'
+                    ),
+                ],
+                'tags_to_add': {
+                    'importer/reimport/import/1-1/0': repo_builder.Placeholder('import'),
+                    'importer/reimport/import/1-1/1': repo_builder.Placeholder('reimport'),
+                },
+                'branches_to_update': {
+                },
+            },
+            [
+                'refs/tags/importer/import/1-1',
+                'refs/tags/importer/reimport/import/1-1/0',
+                'refs/tags/importer/reimport/import/1-1/1',
+            ],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='import'
+                    ),
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport1'
+                    ),
+                ],
+                branches={},
+                tags={
+                    'importer/import/1-1': repo_builder.Placeholder('import'),
+                    'importer/reimport/import/1-1/0': repo_builder.Placeholder('import'),
+                    'importer/reimport/import/1-1/1': repo_builder.Placeholder('reimport1'),
+                },
+            ),
+            {
+                'commit_list_to_add': [
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport2'
+                    ),
+                ],
+                'tags_to_add': {
+                    'importer/reimport/import/1-1/2': repo_builder.Placeholder('reimport2'),
+                },
+                'branches_to_update': {
+                },
+            },
+            [
+                'refs/tags/importer/import/1-1',
+                'refs/tags/importer/reimport/import/1-1/0',
+                'refs/tags/importer/reimport/import/1-1/1',
+                'refs/tags/importer/reimport/import/1-1/2',
+            ],
+        ),
+    ],
+)
+def test_create_import_tag(repo, input_data, expected_changes, test_refs):
+    publish_commit_str = str(repo.raw_repo.get(repo_builder.Commit(
+        repo_builder.Tree({})
+    ).write(repo.raw_repo)).peel(pygit2.Commit).id)
+
+    # copy before write, so that placeholders are still present
+    expected_result = input_data.copy(**expected_changes)
+
+    input_data.write(repo.raw_repo)
+
+    target.create_import_tag(repo, publish_commit_str, '1-1', 'importer')
+
+    assert repo_comparator.equals(
+        repoA=repo.raw_repo,
+        repoB=expected_result,
+        test_refs=test_refs,
+    )
+
+
[e-mail](
+    'input_data, expected_changes, test_refs', [
+        (
+            repo_builder.Repo(
+                commit_list=[],
+                branches={},
+                tags={},
+            ),
+            {
+                'commit_list_to_add': [
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='import'
+                    ),
+                ],
+                'tags_to_add': {
+                    'importer/applied/1-1': repo_builder.Placeholder('import'),
+                },
+                'branches_to_update': {
+                },
+            },
+            [
+                'refs/tags/importer/applied/1-1',
+            ],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='import'
+                    ),
+                ],
+                branches={},
+                tags={
+                    'importer/applied/1-1': repo_builder.Placeholder('import'),
+                },
+            ),
+            {
+                'commit_list_to_add': [
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport'
+                    ),
+                ],
+                'tags_to_add': {
+                    'importer/reimport/applied/1-1/0': repo_builder.Placeholder('import'),
+                    'importer/reimport/applied/1-1/1': repo_builder.Placeholder('reimport'),
+                },
+                'branches_to_update': {
+                },
+            },
+            [
+                'refs/tags/importer/applied/1-1',
+                'refs/tags/importer/reimport/applied/1-1/0',
+                'refs/tags/importer/reimport/applied/1-1/1',
+            ],
+        ),
+        (
+            repo_builder.Repo(
+                commit_list=[
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='import'
+                    ),
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport1'
+                    ),
+                ],
+                branches={},
+                tags={
+                    'importer/applied/1-1': repo_builder.Placeholder('import'),
+                    'importer/reimport/applied/1-1/0': repo_builder.Placeholder('import'),
+                    'importer/reimport/applied/1-1/1': repo_builder.Placeholder('reimport1'),
+                },
+            ),
+            {
+                'commit_list_to_add': [
+                    repo_builder.Commit(
+                        tree=repo_builder.Tree({}),
+                        name='reimport2'
+                    ),
+                ],
+                'tags_to_add': {
+                    'importer/reimport/applied/1-1/2': repo_builder.Placeholder('reimport2'),
+                },
+                'branches_to_update': {
+                },
+            },
+            [
+                'refs/tags/importer/applied/1-1',
+                'refs/tags/importer/reimport/applied/1-1/0',
+                'refs/tags/importer/reimport/applied/1-1/1',
+                'refs/tags/importer/reimport/applied/1-1/2',
+            ],
+        ),
+    ],
+)
+def test_create_applied_tag(repo, input_data, expected_changes, test_refs):
+    publish_commit_str = str(repo.raw_repo.get(repo_builder.Commit(
+        repo_builder.Tree({})
+    ).write(repo.raw_repo)).peel(pygit2.Commit).id)
+
+    # copy before write, so that placeholders are still present
+    expected_result = input_data.copy(**expected_changes)
+
+    input_data.write(repo.raw_repo)
+
+    target.create_applied_tag(repo, publish_commit_str, '1-1', 'importer')
+
+    assert repo_comparator.equals(
+        repoA=repo.raw_repo,
+        repoB=expected_result,
+        test_refs=test_refs,
+    )