Merge ~racb/git-ubuntu:interleave-publication-dates into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

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

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

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

√ python3 setup.py build
√ python3 setup.py check
√ ./snap-wrappers/wrappers/git-ubuntu-self-test
  √ 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
  √ 335 passed, 3 xfailed in 183.72 seconds
  √ Coverage for importer.py at 64%
  √ Overall coverage 54%
√ Per-patch code review
  √ d87a1adecd324cd8fab3bf02c62fbff2352c1f64
    + Makes routine automagically determine target branch
    + LGTM
  √ ade437105ceece98173e13eae60fab431297f9c8
    + importppa.py also has a couple launchpad_versions_published_after()
      calls, however this is slated for removal soon. Otherwise, these
      calls would need adjusted similarly to importer.py.
    + Otherwise LGTM
  √ 371c75514e53ec4acc63c3fd48f3a31b3954e5ea
    + docstrings always welcome
    + LGTM
  √ feb96661189ea0cef2910a6faad1bf918ef57ec9
    + Refactors handling of active_series_only to pre-loop condition as
      a constructed dist_priority dictionary.
    + Adds interleave_launchpad_versions_published_after() to interleave
      the debian and ubuntu versions.
    + Adds testcase for same.
    + This routine is used to build a list of source publications
      (spi's), which are sorted by heapq.merge() using a custom key
      constructed by lookup of date created from launchpad, and dist_priority.
    + Studied dict comprehension for dist_priority creation. Looks
      correct.
    + Studied _get_cached_lp_link() behavior
    + interleave_launchpad_versions_published_after() also needs return
      value documented,
      rtype: iterator
      returns: iterator over the sorted values
    + fake_guspi() doesn't have return documented -- but it's internal to
      a test case, so I don't think it really matters. It's obvious
      enough what it's doing.
    + fake_guspi() documents 'Any' for param type, which appears to mean
      the function's parameters are not type specific. So could specify
      either a datetime object or, as in this case, integers.

review: Approve

[Robie Basak (racb)]

Thank you for the review!

On Tue, Apr 21, 2020 at 01:08:49AM -0000, Bryce Harrington wrote:
> + importppa.py also has a couple launchpad_versions_published_after()
> calls, however this is slated for removal soon. Otherwise, these
> calls would need adjusted similarly to importer.py.

Good spot! Yes - I checked the other callers but as they all turned out
to be slated for removal I ignored them.

> + interleave_launchpad_versions_published_after() also needs return
> value documented,
> rtype: iterator
> returns: iterator over the sorted values

Added, thanks.

> + fake_guspi() doesn't have return documented -- but it's internal to
> a test case, so I don't think it really matters. It's obvious
> enough what it's doing.

I added it anyway, thanks.

> + fake_guspi() documents 'Any' for param type, which appears to mean
> the function's parameters are not type specific. So could specify
> either a datetime object or, as in this case, integers.

Right - to keep things simple I took advantage of duck typing and used
integers instead of datetime objects. It only matters that they are
compared for sorting purposes.

I've forced pushed an update that only adjusts the final commit for
comments and docstrings, including a couple of additional minor
corrections to comments and docstrings that I don't think warrant
further review.

I will merge this branch. Thanks again!

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

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

review: Approve (continuous-integration)

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index ce7ebe8..6673878 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -1669,6 +1669,13 @@ class GitUbuntuRepository:
         """Extract the last version in debian/changelog of all
         '<namespace>/<head_prefix>/debian/*' and
         '<namespace>/<head_prefix>/ubuntu/*' branches.
+
+        :rtype: dict
+        :returns: a dictionary keyed by branch name (without a 'refs/heads/'
+            prefix). The value is a dictionary with two keys: 'head' and
+            'version'. The 'head' value is a pygit2.Branch object. The
+            'version' value is the package version string found at that branch
+            head.
         """
         versions = dict()
         errors = False
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 604b478..12d09cb 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -289,7 +289,7 @@ def _main_with_repo(
 
     parent_overrides = parse_parentfile(parentfile, pkgname)
 
-    only_debian, history_found = import_publishes(
+    history_found = import_publishes(
         repo=repo,
         pkgname=pkgname,
         namespace=namespace,
@@ -377,14 +377,17 @@ def _main_with_repo(
             # Changelog notes go to refs/notes/commits due to LP: #1871838
             'refs/notes/%s/changelog:refs/notes/commits' % namespace,
         ])
-        # update our reference
+        # Update HEAD on LP to point to our desired default; this is
+        # ubuntu/devel if that exists, or debian/sid otherwise.
+        if ('refs/heads/%s/ubuntu/devel' % namespace) in repo.references:
+            desired_lp_default_branch = 'refs/heads/ubuntu/devel'
+        else:
+            desired_lp_default_branch = 'refs/heads/debian/sid'
         lp_git_repo = lp.git_repositories.getByPath(path=repo_path)
         for i in range(retries):
             try:
-                if only_debian:
-                    lp_git_repo.default_branch = 'refs/heads/debian/sid'
-                else:
-                    lp_git_repo.default_branch = 'refs/heads/ubuntu/devel'
+                logging.debug('Setting LP HEAD to %s' % desired_lp_default_branch)
+                lp_git_repo.default_branch = desired_lp_default_branch
                 lp_git_repo.lp_save()
                 break
             except (NotFound, PreconditionFailed) as e:
@@ -1995,7 +1998,6 @@ def import_publishes(
     parent_overrides,
 ):
     history_found = False
-    only_debian = False
     srcpkg_information = None
     if patches_applied:
         _namespace = namespace
@@ -2012,46 +2014,52 @@ def import_publishes(
             import_unapplied_spi,
             skip_orig=skip_orig,
         )
-    for distname, versions, dist_sinfo in (
-            ("debian", debian_head_versions, debian_sinfo),
-            ("ubuntu", ubuntu_head_versions, ubuntu_sinfo)):
-        if active_series_only and distname == "debian":
-            continue
-        try:
-            for srcpkg_information in dist_sinfo.launchpad_versions_published_after(
-                versions,
-                namespace,
-                workdir=workdir,
-                active_series_only=active_series_only
-            ):
-                history_found = True
-                import_func(
-                    repo=repo,
-                    spi=srcpkg_information,
-                    namespace=_namespace,
-                    parent_overrides=parent_overrides,
-                )
-        except NoPublicationHistoryException:
-            logging.warning("No publication history found for %s in %s.",
-                pkgname, distname
+    # Always look in Ubuntu for new publications
+    gusi_head_versions_tuple_list = [
+        (ubuntu_sinfo, ubuntu_head_versions),
+    ]
+    # Unless --active-series-only was specified, also look in Debian for new
+    # publications.
+    if not active_series_only:
+        # For hash stability, we must process new Debian publications with the
+        # same date_created before new Ubuntu publications, so we insert the
+        # Debian entry before the Ubuntu entry.
+        gusi_head_versions_tuple_list.insert(
+            0,
+            (debian_sinfo, debian_head_versions),
+        )
+    try:
+        # We must process new publications in order of date_created interleaved
+        # across both distributions (Debian first) in order to maintain hash
+        # stability according to the spec.
+        for srcpkg_information in GitUbuntuSourceInformation.interleave_launchpad_versions_published_after(
+            gusi_head_versions_tuple_list,
+            namespace=namespace,
+            workdir=workdir,
+            active_series_only=active_series_only,
+        ):
+            history_found = True
+            import_func(
+                repo=repo,
+                spi=srcpkg_information,
+                namespace=_namespace,
+                parent_overrides=parent_overrides,
             )
-            if distname == 'ubuntu':
-                only_debian = True
-        except Exception as e:
-            if srcpkg_information is None:
-                msg = 'Unable to import %s to %s' % (import_type, distname)
-            else:
-                msg = 'Unable to import %s %s to %s' % (import_type,
-                     str(srcpkg_information.version), distname)
-            if not patches_applied:
-                raise GitUbuntuImportError(msg) from e
-            else:
-                logging.error(msg)
-                logging.error(traceback.format_exc())
+    except Exception as e:
+        if srcpkg_information is None:
+            msg = 'Unable to import %s' % import_type
         else:
-            history_found = True
+            msg = 'Unable to import %s %s' % (import_type,
+                 str(srcpkg_information.version))
+        if not patches_applied:
+            raise GitUbuntuImportError(msg) from e
+        else:
+            logging.error(msg)
+            logging.error(traceback.format_exc())
+    else:
+        history_found = True
 
-    return (only_debian, history_found)
+    return history_found
 
 
 def parse_args(subparsers=None, base_subparsers=None):
diff --git a/gitubuntu/source_information.py b/gitubuntu/source_information.py
index 38df33e..cf729b8 100644
--- a/gitubuntu/source_information.py
+++ b/gitubuntu/source_information.py
@@ -1,4 +1,5 @@
 import functools
+import heapq
 import logging
 import os
 import re
@@ -544,12 +545,38 @@ class GitUbuntuSourceInformation(object):
         for srcpkg in spph:
             yield self.get_corrected_spi(srcpkg, workdir)
 
-    def launchpad_versions_published_after(self, head_versions, namespace, workdir=None, active_series_only=False):
+    def launchpad_versions_published_after(
+        self,
+        head_versions,
+        namespace,
+        workdir=None,
+        active_series_only=False,
+    ):
+        """Return a sequence of GitUbuntuSourcePackageInformation instances
+
+        Return a sequence of GitUbuntuSourcePackageInformation instances
+        representing Launchpad publications created in a Launchpad distribution
+        after a particular point, as determined by the head_versions parameter.
+
+        :param dict head_versions: as returned by
+            GitUbuntuRepository.get_heads_and_versions(). This may be empty, in
+            which case all publications are returned. Otherwise, publications
+            are skipped that match a version already found in head_versions, or
+            with a date_created older than what is found in head_versions, is
+            skipped, as well as any publications that have a prior date_created
+            date.
+        :param str namespace: the namespace prefix used in the git repository.
+        :param workdir: passed through to the GitUbuntuSourcePackageInformation
+            constructor.
+        :param bool active_series_only: for Ubuntu, skip any series that is not
+            an active series, as determined by self.active_series_name_list.
+        :rtype: sequence(GitUbuntuSourcePackageInformation)
+        """
         args = {
-                'exact_match':True,
-                'source_name':self.pkgname,
-                'order_by_date':True,
-               }
+            'exact_match': True,
+            'source_name': self.pkgname,
+            'order_by_date': True,
+        }
 
         # we have the date of the commit too, so we can double-check
         # that it matches
@@ -570,8 +597,10 @@ class GitUbuntuSourceInformation(object):
         # Sanity check that the passed in srcpkg name has a publication
         # history
         if len(spph) == 0:
-            raise NoPublicationHistoryException("Is %s published in %s?" %
-                                                (self.pkgname, self.dist_name))
+            logging.warning("No publication history found for %s in %s.",
+                self.pkgname, self.dist_name
+            )
+            return
         if len(head_versions) > 0:
             _spph = list()
             for spphr in spph:
@@ -610,6 +639,87 @@ class GitUbuntuSourceInformation(object):
             yield spi
         raise StopIteration()
 
+    @staticmethod
+    def interleave_launchpad_versions_published_after(
+        gusi_head_versions_tuple_list,
+        namespace,
+        workdir=None,
+        active_series_only=False,
+    ):
+        """Interleave multiple calls to launchpad_versions_published_after
+
+        Interleave the result of multiple calls to the
+        launchpad_versions_published_after() method. The specification requires
+        new publications to be processed across the Debian and Ubuntu
+        distributions in order of date_created to establish
+        hash stability. launchpad_versions_published_after() returns the result
+        of one distribution at a time. This method calls any number
+        simultaneously and interleaves the results so that the caller sees a
+        single combined sequence in ascending order of date_created. If
+        date_created is the same, results are provided in the order that the
+        distributions are specified in gusi_head_versions_tuple_list. In other
+        words, the distribution ordering is the secondary sort key.
+
+        :param list(tuple(GitUbuntuSourceInformation, dict))
+            gusi_head_versions_tuple_list: a list of the parameters to use for
+            the underlying launchpad_versions_published_after() method call.
+            The GitUbuntuSourceInformation instance is the object to call the
+            method against. The second element of the tuple is the
+            head_versions parameter to pass to that method. The other
+            parameters are passed through to the underlying call as-is.
+        :param namespace: passed through to
+            launchpad_versions_published_after().
+        :param workdir: passed through to launchpad_versions_published_after().
+        :param bool active_series_only: passed through to
+            launchpad_versions_published_after().
+        :rtype: sequence(GitUbuntuSourcePackageInformation)
+        :returns: the combined return sequences of the underlying calls to
+            launchpad_versions_published_after() interleaved as specified.
+        """
+        # Create a mapping of distribution to priority, where distribution is
+        # the Launchpad distribution_link string representing the distribution,
+        # and priority is an integer (low number=high priority). Then we will
+        # be able to sort according to priority as a secondary key to ensure
+        # that if timestamps of a publication are the same across
+        # distributions, the distribution mentioned first in
+        # gusi_head_versions_tuple_list will appear in the results first.
+        dist_priority = {
+            gusi.dist.self_link: i
+            for i, (gusi, head_version)
+            in enumerate(gusi_head_versions_tuple_list)
+        }
+
+        # Now that we have a mapping of distribution_link to priority, the sort
+        # key function is simple: sort on date created first, and if they are
+        # equal then use the distribution priority. Python will do this for us
+        # if we provide a tuple. The distribution priority is necessary because
+        # there is no guarantee that heapq.merge(), as used below, provides a
+        # stable sort. If it did, then keying on date_created only would be
+        # sufficient.
+        def key_func(spi):
+            distro_series = _get_cached_lp_link(spi._spphr, 'distro_series')
+            distribution_link = distro_series.distribution_link
+            return spi._spphr.date_created, dist_priority[distribution_link]
+
+        # Create one generator per gusi_head_versions_tuple_list entry.
+        spi_generators_to_interleave = [
+            gusi.launchpad_versions_published_after(
+                head_versions=head_versions,
+                namespace=namespace,
+                workdir=workdir,
+                active_series_only=active_series_only,
+            )
+            for gusi, head_versions in gusi_head_versions_tuple_list
+        ]
+
+        # heapq.merge() will now do the interleaving. This relies on the
+        # results of each of the calls to launchpad_versions_published_after()
+        # already being sorted by date_created.
+        return heapq.merge(
+            *spi_generators_to_interleave,
+            key=key_func,
+        )
+
     def parse_pullfile(self, pullfile):
         """Extract source file overrides from a file
 
diff --git a/gitubuntu/source_information_test.py b/gitubuntu/source_information_test.py
index 7aacb1a..7e88d77 100644
--- a/gitubuntu/source_information_test.py
+++ b/gitubuntu/source_information_test.py
@@ -1,7 +1,10 @@
 import collections
+import functools
+from unittest.mock import Mock, sentinel
 
 import keyring
 import pytest
+import gitubuntu.source_information as target
 from gitubuntu.source_information import derive_codename_from_series
 from gitubuntu import source_information
 
@@ -150,3 +153,124 @@ def test_derive_codename_from_broken_distro_info(monkeypatch, series, codename):
     monkeypatch.setattr(source_information, "_ddi", MockDebianDistroInfo())
 
     assert derive_codename_from_series(series) == codename
+
+
[e-mail](['debian_input', 'ubuntu_input', 'expected'], [
+    (
+        [],
+        [],
+        [],
+    ),
+    (
+        [0],
+        [0],
+        [(0, 'debian'), (0, 'ubuntu')],
+    ),
+    (
+        [1],
+        [0],
+        [(0, 'ubuntu'), (1, 'debian')],
+    ),
+])
+def test_interleave_launchpad_versions_published_after(
+    debian_input,
+    ubuntu_input,
+    expected,
+):
+    """Test that interleaving works correctly
+
+    We will always ask for Debian before Ubuntu. When date_created is the same,
+    Debian should come first. If date_created is different, the earlier
+    date_created should come first.
+
+    :param list(Any) debian_input: the date_created attributes of the fake
+        Launchpad source_package_publishing_history objects of the fake
+        GitUbuntuSourcePackageInformation objects that will be returned by the
+        faked call to launchpad_versions_published_after() call, for the Debian
+        distribution.
+    :param list(Any) debian_input: the date_created attributes of the fake
+        Launchpad source_package_publishing_history objects of the fake
+        GitUbuntuSourcePackageInformation objects that will be returned by the
+        faked call to launchpad_versions_published_after() call, for the Ubuntu
+        distribution.
+    :param list(tuple(date_created, distribution)): the expected ordering in
+        the return sequence of the
+        interleave_launchpad_versions_published_after() call. date_created
+        corresponds to the input data from debian_input and ubuntu_input.
+        distribution is either the 'debian' string or the 'ubuntu' string
+        depending on whether the return value was supposed to come from the
+        Debian or Ubuntu fake GitUbuntuSourceInformation object.
+    """
+    # Store all fake guspis created, since equivalent guspis will be asserted
+    # for equality later, and Mock objects that are otherwise identical do not
+    # compare equal if they are separate instances. For the same call to
+    # fake_guspi() below, we want to return the same Mock object instance so
+    # that they do compare equal.
+    fake_guspis = {}
+    def fake_guspi(dist_link, date_created):
+        """Return a fake target.GitUbuntuSourcePackageInformation object
+
+        Multiple calls to this function with the same parameters return the
+        same fake objects so that they compare equal.
+
+        :param Any dist_link: a mock distribution_link that can be compared
+            against later (for example, use a unittest.mock.sentinel).
+        :param Any date_created: the fake date_created attribute of the
+            underlying Launchpad source_package_publishing_history object.
+        :rtype: Mock
+        :returns: the fake target.GitUbuntuSourcePackageInformation object.
+        """
+        try:
+            return fake_guspis[(dist_link, date_created)]
+        except KeyError:
+            result = Mock(
+                spec=target.GitUbuntuSourcePackageInformation,
+                _spphr=Mock(
+                    distro_series_link=str(hash(dist_link)) + '_distro_series',
+                    distro_series=Mock(distribution_link=dist_link),
+                    date_created=date_created,
+                )
+            )
+            fake_guspis[(dist_link, date_created)] = result
+            return result
+
+    # A fake target.GitUbuntuSourceInformation object to represent the Debian
+    # distribution.
+    debian = Mock(
+        spec=target.GitUbuntuSourceInformation,
+        dist=Mock(self_link=sentinel.debian_dist_link),
+        launchpad_versions_published_after=Mock(return_value=[
+            fake_guspi(sentinel.debian_dist_link, date_created)
+            for date_created in debian_input
+        ])
+    )
+    # A fake target.GitUbuntuSourceInformation object to represent the Ubuntu
+    # distribution.
+    ubuntu = Mock(
+        spec=target.GitUbuntuSourceInformation,
+        dist=Mock(self_link=sentinel.ubuntu_dist_link),
+        launchpad_versions_published_after=Mock(return_value=[
+            fake_guspi(sentinel.ubuntu_dist_link, date_created)
+            for date_created in ubuntu_input
+        ])
+    )
+
+    gusi_head_versions_tuple_list = [
+        (debian, None),
+        (ubuntu, None),
+    ]
+    result = target.GitUbuntuSourceInformation.interleave_launchpad_versions_published_after(
+        gusi_head_versions_tuple_list=gusi_head_versions_tuple_list,
+        namespace=None,
+    )
+    expanded_result = list(result)
+    assert expanded_result == [
+        fake_guspi(
+            dist_link={
+                'debian': sentinel.debian_dist_link,
+                'ubuntu': sentinel.ubuntu_dist_link,
+            }[dist],
+            date_created=date_created,
+        )
+        for date_created, dist in expected
+    ]