Merge ~racb/git-ubuntu:unglobal-parent-overrides into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

[Robie Basak (racb)]

I compared the result of importing memcached (something that has a parent override defined) on the merge base (1e67e4c) and on this branch (baf6159). All the branches and tags resulted in identical commit hashes except for the pristine-tar branches. I smoke-checked that the pristine-tar branches visually looked completed in both cases by running git ls-tree on them.

importer/import/1.1.12-1 correctly had no parent in both 1e67e4c and baf6159.

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

PASSED: Continuous integration, rev:baf6159f2646ef0f9d5072e9f9ee4bd9f0dd8144
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/264/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    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/264/rebuild

review: Approve (continuous-integration)

[Nish Aravamudan (nacc)]

I know it's not a performance critical path, but does it make sense to make this @lru_cache()? Operationally, we don't want this to change under us, and it seems like it could during an operation?

[Nish Aravamudan (nacc)]

review: Needs Information

[Nish Aravamudan (nacc)]

Barring that issue, I'm happy to land this and the follow-on.

[Robie Basak (racb)]

Which function do you mean? parse_parentfile is only called from the main function still I think (in the end)?

I did plan @lru_cache, but didn't refactor the code to re-parse every time anyway.

[Nish Aravamudan (nacc)]

Right, parse_parentfile before would not reparse, because _PARENT_OVERRIDES would be defined on second invocation (poor man's @lru_cache :); so if we want to be strictly functionally equivalent, we would keep that behavior. I suppose it does not matter, but we don't assert (nor is it mentioned in the comments) that calling the method will reparse every time.

[Robie Basak (racb)]

I see. Yes - I suppose that's a functional change to parse_parentfile. But I don't think it makes sense to maintain the old behaviour (specifically that protection). A grep for parse_parentfile confirms that we are only ever calling it once from the three main functions (import, importppa and importlocal). If I were to write this from scratch, I wouldn't include this protection, so I think it makes sense to drop it.

I can explicitly call out that change in the commit message if you wish. But I think that not having @lru_cache is the right thing to do.

[Nish Aravamudan (nacc)]

No, that's fine, I just wanted to make sure it was intentional.

[Nish Aravamudan (nacc)]

review: Approve

diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 5e4fe00..cdd34e2 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -317,8 +317,7 @@ def main(
 
     os.makedirs(workdir, exist_ok=True)
 
-    # now sets a global _PARENT_OVERRIDES
-    parse_parentfile(parentfile, pkgname)
+    parent_overrides = parse_parentfile(parentfile, pkgname)
 
     only_debian, history_found = import_publishes(
         repo=repo,
@@ -333,6 +332,7 @@ def main(
         workdir=workdir,
         skip_orig=skip_orig,
         allow_applied_failures=allow_applied_failures,
+        parent_overrides=parent_overrides,
     )
 
     if not history_found:
@@ -354,6 +354,7 @@ def main(
             workdir=workdir,
             skip_orig=skip_orig,
             allow_applied_failures=allow_applied_failures,
+            parent_overrides=parent_overrides,
         )
 
     update_devel_branches(
@@ -846,7 +847,6 @@ def import_patches_applied_tree(repo, dsc_pathname):
         shutil.rmtree(extract_dir)
         repo.clean_repository_state()
 
-_PARENT_OVERRIDES = None
 def parse_parentfile(parentfile, pkgname):
     """Extract parent overrides from a file
 
@@ -862,13 +862,13 @@ def parse_parentfile(parentfile, pkgname):
     The <changelog parent version> is the prior version in the
     child publish's debian/changelog.
 
-    Keyword Arguments:
-    parentfile -- Path to parent overrides file
+    :param str parentfile: path to parent overrides file
+    :param str pkgname: source package name on which to filter
+    :rtype: dict
+    :returns: dictionary keyed by version string of dictionaries keyed by
+        'changelog_parent' of strings of changelog parent versions.
     """
-    global _PARENT_OVERRIDES
-    if _PARENT_OVERRIDES:
-        return
-    _PARENT_OVERRIDES = dict()
+    parent_overrides = dict()
     try:
         with open(parentfile) as f:
             for line in f:
@@ -882,22 +882,30 @@ def parse_parentfile(parentfile, pkgname):
                     continue
                 if m.group('pkgname') != pkgname:
                     continue
-                _PARENT_OVERRIDES[m.group('childversion')] = {
+                parent_overrides[m.group('childversion')] = {
                     'changelog_parent': m.group('parent2version')
                 }
     except FileNotFoundError:
         pass
 
-def override_parents(repo, spi, namespace):
+    return parent_overrides
+
+def override_parents(parent_overrides, repo, spi, 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)
+    """
     unapplied_changelog_parent_commit = None
     applied_changelog_parent_commit = None
 
     unapplied_changelog_parent_tag = repo.get_import_tag(
-        _PARENT_OVERRIDES[spi.version]['changelog_parent'],
+        parent_overrides[spi.version]['changelog_parent'],
         namespace
     )
     applied_changelog_parent_tag = repo.get_applied_tag(
-        _PARENT_OVERRIDES[spi.version]['changelog_parent'],
+        parent_overrides[spi.version]['changelog_parent'],
         namespace
     )
     if unapplied_changelog_parent_tag is not None:
@@ -906,13 +914,13 @@ def override_parents(repo, spi, namespace):
         parent_changelog_version, _ = repo.get_changelog_versions_from_treeish(
             str(unapplied_changelog_parent_tag.peel().id),
         )
-        if parent_changelog_version != _PARENT_OVERRIDES[spi.version]['changelog_parent']:
+        if parent_changelog_version != parent_overrides[spi.version]['changelog_parent']:
             logging.error('Found a tag corresponding to '
                           'changelog parent override '
                           'version %s, but d/changelog '
                           'version (%s) differs. Will '
                           'orphan commit.' % (
-                          _PARENT_OVERRIDES[spi.version]['changelog_parent'],
+                          parent_overrides[spi.version]['changelog_parent'],
                           parent_changelog_version
                           )
                          )
@@ -924,7 +932,7 @@ def override_parents(repo, spi, namespace):
                 repo.tag_to_pretty_name(unapplied_changelog_parent_tag)
             )
     else:
-        if _PARENT_OVERRIDES[spi.version]['changelog_parent'] == '-':
+        if parent_overrides[spi.version]['changelog_parent'] == '-':
             logging.debug('Not setting changelog parent as specified '
                 'in override file.'
             )
@@ -932,7 +940,7 @@ def override_parents(repo, spi, namespace):
             raise ParentOverrideError(
                 "Specified changelog parent override (%s) for version "
                 "(%s) not found in tags. Unable to proceed." % (
-                    _PARENT_OVERRIDES[spi.version]['changelog_parent'],
+                    parent_overrides[spi.version]['changelog_parent'],
                     spi.version,
                 )
             )
@@ -1057,7 +1065,13 @@ def update_devel_branches(
 
 
 # imports a package based upon source package information
-def import_unapplied_spi(repo, spi, namespace, skip_orig):
+def import_unapplied_spi(
+    repo,
+    spi,
+    namespace,
+    skip_orig,
+    parent_overrides,
+):
     """Imports a source package from Launchpad into the git
     repository
 
@@ -1118,7 +1132,7 @@ def import_unapplied_spi(repo, spi, namespace, skip_orig):
     )
 
     # sanity check on spph and d/changelog agreeing on the latest version
-    if spi.version not in _PARENT_OVERRIDES:
+    if spi.version not in parent_overrides:
         assert spi.version == changelog_version, (
             'source pkg version: {} != changelog version: {}'.format(
                 spi.version, changelog_version))
@@ -1144,7 +1158,7 @@ def import_unapplied_spi(repo, spi, namespace, skip_orig):
     upload_parent_commit = None
     unapplied_parent_commit = None
 
-    if spi.version in _PARENT_OVERRIDES:
+    if spi.version in parent_overrides:
         logging.debug(
             '%s is specified in the parent override file.',
             spi.version
@@ -1154,7 +1168,7 @@ def import_unapplied_spi(repo, spi, namespace, skip_orig):
             (
                 unapplied_changelog_parent_commit,
                 _
-            ) = override_parents(repo, spi, namespace)
+            ) = override_parents(parent_overrides, repo, spi, namespace)
         except ParentOverrideError as e:
             logging.error("%s" % e)
             return
@@ -1180,7 +1194,7 @@ def import_unapplied_spi(repo, spi, namespace, skip_orig):
                 # 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 not in _PARENT_OVERRIDES and
+                if (version not in parent_overrides and
                     parent_changelog_version != version
                 ):
                     logging.error(
@@ -1257,7 +1271,8 @@ def import_applied_spi(
     repo,
     spi,
     namespace,
-    allow_applied_failures
+    allow_applied_failures,
+    parent_overrides,
 ):
     """Imports a source package from Launchpad into the git
     repository
@@ -1304,7 +1319,7 @@ def import_applied_spi(
 
     applied_changelog_parent_commit = None
 
-    if spi.version in _PARENT_OVERRIDES:
+    if spi.version in parent_overrides:
         logging.debug('%s is specified in the parent override file.' %
                      spi.version
                     )
@@ -1313,6 +1328,7 @@ def import_applied_spi(
             _,
             applied_changelog_parent_commit,
         ) = override_parents(
+            parent_overrides,
             repo,
             spi,
             namespace,
@@ -1336,7 +1352,7 @@ def import_applied_spi(
                 # 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 not in _PARENT_OVERRIDES and
+                if (version not in parent_overrides and
                     parent_changelog_version != version
                    ):
                     logging.error('Found a tag corresponding to '
@@ -1435,6 +1451,7 @@ def import_publishes(
     workdir,
     skip_orig,
     allow_applied_failures,
+    parent_overrides,
 ):
     history_found = False
     only_debian = False
@@ -1471,6 +1488,7 @@ def import_publishes(
                     repo=repo,
                     spi=srcpkg_information,
                     namespace=_namespace,
+                    parent_overrides=parent_overrides,
                 )
         except NoPublicationHistoryException:
             logging.warning("No publication history found for %s in %s.",
diff --git a/gitubuntu/importlocal.py b/gitubuntu/importlocal.py
index 13f8690..b514edf 100644
--- a/gitubuntu/importlocal.py
+++ b/gitubuntu/importlocal.py
@@ -15,7 +15,6 @@ from gitubuntu.run import run, runq
 from gitubuntu.version import VERSION
 from gitubuntu.importer import (
     parse_parentfile,
-    _PARENT_OVERRIDES,
     import_orig,
     import_dsc,
     get_changelog_for_commit,
@@ -84,7 +83,7 @@ def main(
 
     repo.clean_repository_state()
 
-    parse_parentfile(parentfile, pkgname)
+    parent_overrides = parse_parentfile(parentfile, pkgname)
 
     unapplied_import_tree_hash = import_patches_unapplied_tree(repo, dsc_path)
 
@@ -137,7 +136,7 @@ def main(
             # 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 not in _PARENT_OVERRIDES and
+            if (version not in parent_overrides and
                 parent_changelog_version != version
             ):
                 logging.error('Found a tag corresponding to '
diff --git a/gitubuntu/importppa.py b/gitubuntu/importppa.py
index 835b0aa..a2d27d3 100644
--- a/gitubuntu/importppa.py
+++ b/gitubuntu/importppa.py
@@ -8,26 +8,15 @@ import sys
 from gitubuntu.cache import CACHE_PATH
 from gitubuntu.git_repository import (
     GitUbuntuRepository,
-    GitUbuntuRepositoryFetchError,
 )
 from gitubuntu.importer import (
-    cleanup,
     parse_parentfile,
-    _PARENT_OVERRIDES,
-    import_orig,
-    import_dsc,
-    get_changelog_for_commit,
     import_unapplied_spi,
     import_applied_spi,
-    import_patches_unapplied_tree,
-    import_patches_applied_tree,
 )
 from gitubuntu.source_information import (
     GitUbuntuSourceInformation,
     NoPublicationHistoryException,
-    SourceExtractionException,
-    launchpad_login_auth,
-    GitUbuntuSourceInformation,
 )
 from gitubuntu.version import VERSION
 try:
@@ -110,7 +99,7 @@ def main(
 
     os.makedirs(workdir, exist_ok=True)
 
-    parse_parentfile(pkgname, parentfile)
+    parent_overrides = parse_parentfile(pkgname, parentfile)
 
     history_found = False
     try:
@@ -126,6 +115,7 @@ def main(
                     spi=srcpkg_information,
                     namespace=namespace,
                     skip_orig=skip_orig,
+                    parent_overrides=parent_overrides,
                 )
             except DownloadError:
                 # it is non-fatal for a PPA to not have files for older
@@ -159,6 +149,7 @@ def main(
                     spi=srcpkg_information,
                     namespace=namespace,
                     allow_applied_failures=allow_applied_failures,
+                    parent_overrides=parent_overrides,
                 )
             except DownloadError:
                 # it is non-fatal for a PPA to not have files for older