Merge ~racb/git-ubuntu:applied-enum into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

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

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

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

Looks good, I see what you mean about how much code this touches, but a solid refactoring.

I don't know if it matters, and probably wouldn't ever in practice, but as python doesn't type check function parameters, the anonymous dict is going to be a bit brittle if incorrect values are passed.

However, I don't think it's worth going out of the way to prevent. If someone does louse up the code and pass a non-enum in, it'll error a bit cryptically but shouldn't be too hard to figure out.

$ python3
Python 3.7.6 (default, Jan 16 2020, 18:23:42)
[GCC 9.2.1 20200110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import enum
>>> PatchState = enum.Enum('PatchState', ['APPLIED', 'UNAPPLIED'])
>>> def test_enum(patch_state=PatchState.UNAPPLIED):
... print({ PatchState.UNAPPLIED: 'import', PatchState.APPLIED: 'applied', }[patch_state])
...
>>> test_enum(PatchState.APPLIED)
applied
>>> test_enum(PatchState.UNAPPLIED)
import
>>> test_enum(PatchState.NOTHING)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.7/enum.py", line 349, in __getattr__
    raise AttributeError(name) from None
AttributeError: NOTHING
>>> test_enum(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in test_enum
KeyError: -1
>>> test_enum(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in test_enum
KeyError: 0
>>> import typing
>>> def test_enum(patch_state: PatchState = PatchState.UNAPPLIED):
... print({ PatchState.UNAPPLIED: 'import', PatchState.APPLIED: 'applied', }[patch_state])
...
>>> test_enum(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in test_enum
KeyError: 0
>>> test_enum(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in test_enum
KeyError: -1
>>> test_enum(PatchState.APPLIED)
applied

review: Approve

[Bryce Harrington (bryce)]

I almost hate to mention it, but I notice the import_tag() and reimport_tag_prefix() functions could be condensed to one routine pretty easily by using a '/'.join() on the parts. IOW:

def import_tag(version, namespace, patch_state=PatchState.UNAPPLIED, reimport=False):
    elements = [ namespace ]
    if reimport:
        elements.append('reimport')
    elements.append('applied' if patch_state==PatchState.APPLIED else 'import')
    elements.append(git_dep14_tag(version))
    return '/'.join(elements)

>>> import_tag('ver', 'ns')
'ns/import/dep14.ver'
>>> import_tag('ver', 'ns', PatchState.APPLIED, True)
'ns/reimport/applied/dep14.ver'

(There's probably simpler ways to build the list than append(), and obviously the reimport bool would be better done as an enum like patch state, but I think that gives the basic idea.)

Frankly, though, I kind of like having two distinct routines here like you've done, because they're conceptually different and will make the code calling them a bit more clear as to what's going on. However, sometimes unifying to a single function can open other refactoring possibilities later, so figure it's worth mentioning as an option.

[Robie Basak (racb)]

> because they're conceptually different and will make the code calling them a bit more clear as to what's going on

Agreed. I prefer leaving this as-is, especially because it's difficult to define the semantics of a combined function from the perspective of callers.

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index bce4573..6dfddd9 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -22,6 +22,7 @@ import gitubuntu.lint
 from gitubuntu.__main__ import top_level_defaults
 import gitubuntu.build
 from gitubuntu.dsc import component_tarball_matches
+from gitubuntu.patch_state import PatchState
 from gitubuntu.run import (
    decode_binary,
    run,
@@ -736,36 +737,37 @@ def git_dep14_tag(version):
         version = pre + '.#lock'
     return 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' % (
+def import_tag(version, namespace, patch_state=PatchState.UNAPPLIED):
+    return '%s/%s/%s' % (
         namespace,
+        {
+            PatchState.UNAPPLIED: 'import',
+            PatchState.APPLIED: 'applied',
+        }[patch_state],
         git_dep14_tag(version),
     )
 
-def applied_reimport_tag_prefix(version, namespace):
-    return '%s/reimport/applied/%s' % (
+def reimport_tag_prefix(version, namespace, patch_state=PatchState.UNAPPLIED):
+    return '%s/reimport/%s/%s' % (
         namespace,
+        {
+            PatchState.UNAPPLIED: 'import',
+            PatchState.APPLIED: 'applied',
+        }[patch_state],
         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):
+def reimport_tag(
+    version,
+    namespace,
+    reimport,
+    patch_state=PatchState.UNAPPLIED,
+):
     return '%s/%s' % (
-        applied_reimport_tag_prefix(version, namespace),
+        reimport_tag_prefix(version, namespace, patch_state=patch_state),
         reimport,
     )
 
-def applied_tag(version, namespace):
-    return '%s/applied/%s' % (namespace, git_dep14_tag(version))
-
 def upload_tag(version, namespace):
     return '%s/upload/%s' % (namespace, git_dep14_tag(version))
 
@@ -1676,32 +1678,41 @@ class GitUbuntuRepository:
         except (KeyError, ValueError):
             return None
 
-    def get_import_tag(self, version, namespace):
-        return self.get_tag_reference(import_tag(version, namespace))
-
-    def get_reimport_tag(self, version, namespace, reimport):
+    def get_import_tag(
+        self,
+        version,
+        namespace,
+        patch_state=PatchState.UNAPPLIED,
+    ):
         return self.get_tag_reference(
-            reimport_tag(version, namespace, reimport)
+            import_tag(version, namespace, patch_state)
         )
 
-    def get_applied_reimport_tag(self, version, namespace, reimport):
+    def get_reimport_tag(
+        self,
+        version,
+        namespace,
+        reimport,
+        patch_state=PatchState.UNAPPLIED,
+    ):
         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)
+            reimport_tag(version, namespace, reimport, patch_state)
         )
 
-    def get_all_applied_reimport_tags(self, version, namespace):
+    def get_all_reimport_tags(
+        self,
+        version,
+        namespace,
+        patch_state=PatchState.UNAPPLIED,
+    ):
         return self.references_with_prefix(
-            'refs/tags/%s' % applied_reimport_tag_prefix(version, namespace)
+            'refs/tags/%s' % reimport_tag_prefix(
+                version,
+                namespace,
+                patch_state,
+            )
         )
 
-    def get_applied_tag(self, version, namespace):
-        return self.get_tag_reference(applied_tag(version, namespace))
-
     def get_upload_tag(self, version, namespace):
         return self.get_tag_reference(upload_tag(version, namespace))
 
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index c6bfea9..98a0997 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -46,14 +46,13 @@ from gitubuntu.git_repository import (
     GitUbuntuRepository,
     GitUbuntuRepositoryFetchError,
     orphan_tag,
-    applied_tag,
     import_tag,
     reimport_tag,
-    applied_reimport_tag,
     upstream_tag,
     PristineTarError,
     is_dir_3_0_quilt,
 )
+from gitubuntu.patch_state import PatchState
 from gitubuntu.run import (
     decode_binary,
     run,
@@ -980,30 +979,24 @@ 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)
+def get_existing_import_tags(
+    repo,
+    version,
+    namespace,
+    patch_state=PatchState.UNAPPLIED,
+):
+    existing_import_tag = repo.get_import_tag(version, namespace, patch_state)
     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(
+        existing_reimport_tags = repo.get_all_reimport_tags(
             version,
             namespace,
+            patch_state,
         )
-        if existing_applied_reimport_tags:
-            return existing_applied_reimport_tags
+        if existing_reimport_tags:
+            return existing_reimport_tags
         else:
-            return [existing_applied_tag]
+            return [existing_import_tag]
     return []
 
 
@@ -1019,11 +1012,13 @@ def override_parents(parent_overrides, repo, version, namespace):
 
     unapplied_changelog_parent_tag = repo.get_import_tag(
         parent_overrides[version]['changelog_parent'],
-        namespace
+        namespace,
+        PatchState.UNAPPLIED,
     )
-    applied_changelog_parent_tag = repo.get_applied_tag(
+    applied_changelog_parent_tag = repo.get_import_tag(
         parent_overrides[version]['changelog_parent'],
-        namespace
+        namespace,
+        PatchState.APPLIED,
     )
     if unapplied_changelog_parent_tag is not None:
         # sanity check that version from d/changelog of the
@@ -1188,12 +1183,17 @@ def get_import_tag_msg():
 def create_applied_tag(repo, commit_hash, version, namespace):
     tag_msg = get_import_tag_msg()
 
-    existing_applied_tag = repo.get_applied_tag(version, namespace)
+    existing_applied_tag = repo.get_import_tag(
+        version,
+        namespace,
+        PatchState.APPLIED,
+    )
     if existing_applied_tag:
-        base_applied_reimport_tag = repo.get_applied_reimport_tag(
+        base_applied_reimport_tag = repo.get_reimport_tag(
             version,
             namespace,
             reimport=0,
+            patch_state=PatchState.APPLIED,
         )
         if base_applied_reimport_tag:
             assert (
@@ -1203,44 +1203,55 @@ def create_applied_tag(repo, commit_hash, version, namespace):
         else:
             repo.create_tag(
                 str(existing_applied_tag.peel(pygit2.Commit).id),
-                applied_reimport_tag(version, namespace, reimport=0),
+                reimport_tag(
+                    version,
+                    namespace,
+                    reimport=0,
+                    patch_state=PatchState.APPLIED,
+                ),
                 tag_msg,
             )
         num_existing_applied_reimports = len(
-            repo.get_all_applied_reimport_tags(version, namespace)
+            repo.get_all_reimport_tags(version, namespace, PatchState.APPLIED)
         )
-        assert not repo.get_applied_reimport_tag(
+        assert not repo.get_reimport_tag(
             version,
             namespace,
             reimport=num_existing_applied_reimports,
+            patch_state=PatchState.APPLIED,
         )
         repo.create_tag(
             commit_hash,
-            applied_reimport_tag(
+            reimport_tag(
                 version,
                 namespace,
                 reimport=num_existing_applied_reimports,
+                patch_state=PatchState.APPLIED,
             ),
             tag_msg,
         )
-        return repo.get_applied_reimport_tag(
+        return repo.get_reimport_tag(
             version,
             namespace,
             reimport=num_existing_applied_reimports,
+            patch_state=PatchState.APPLIED,
         )
     else:
         repo.create_tag(
             commit_hash,
-            applied_tag(version, namespace),
+            import_tag(version, namespace, PatchState.APPLIED),
             tag_msg,
         )
-        return repo.get_applied_tag(version, namespace)
+        return repo.get_import_tag(version, namespace, PatchState.APPLIED)
 
 
 def create_import_tag(repo, commit_hash, version, namespace):
     tag_msg = get_import_tag_msg()
 
-    existing_import_tag = repo.get_import_tag(version, namespace)
+    existing_import_tag = repo.get_import_tag(
+        version,
+        namespace,
+    )
     if existing_import_tag:
         base_reimport_tag = repo.get_reimport_tag(
             version,
@@ -1311,14 +1322,16 @@ def get_changelog_parent_commit(
 
     for version in import_tree_versions:
         if patches_applied:
-            changelog_parent_tag = repo.get_applied_tag(
+            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,
@@ -1744,7 +1757,11 @@ def import_applied_dsc(
             patches_applied=True,
         )
 
-    existing_applied_tag = repo.get_applied_tag(version, namespace)
+    existing_applied_tag = repo.get_import_tag(
+        version,
+        namespace,
+        PatchState.APPLIED,
+    )
     if existing_applied_tag:
         # XXX: What should be checked here? The result of pushing all
         # the patches? How to do it most non-destructively? (Might be
@@ -1822,9 +1839,9 @@ def import_applied_dsc(
     )
 
     tag = None
-    if repo.get_applied_tag(version, namespace) is None:
+    if repo.get_import_tag(version, namespace, PatchState.APPLIED) is None:
         # Not imported before
-        tag = applied_tag(version, namespace)
+        tag = import_tag(version, namespace, PatchState.APPLIED)
     elif (
         applied_changelog_parent_commit is None and
         unapplied_parent_commit is None and
diff --git a/gitubuntu/importer_test.py b/gitubuntu/importer_test.py
index 90412d4..b1dcdc3 100644
--- a/gitubuntu/importer_test.py
+++ b/gitubuntu/importer_test.py
@@ -12,6 +12,7 @@ import tempfile
 
 import pygit2
 
+from gitubuntu.patch_state import PatchState
 import gitubuntu.repo_builder as repo_builder
 from gitubuntu.repo_builder import Placeholder
 import gitubuntu.repo_comparator as repo_comparator
@@ -218,7 +219,12 @@ def test_get_existing_applied_tags(repo, input_repo, expected):
 
     assert [
         ref.name for ref in
-        target.get_existing_applied_tags(repo, '1-1', 'importer')
+        target.get_existing_import_tags(
+            repo,
+            '1-1',
+            'importer',
+            PatchState.APPLIED,
+        )
     ] == expected
 
 
diff --git a/gitubuntu/patch_state.py b/gitubuntu/patch_state.py
new file mode 100644
index 0000000..b2a9cfe
--- /dev/null
+++ b/gitubuntu/patch_state.py
@@ -0,0 +1,3 @@
+import enum
+
+PatchState = enum.Enum('PatchState', ['APPLIED', 'UNAPPLIED'])