Merge ~racb/git-ubuntu:fix-remote-add-changelog-notes into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

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

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

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

Looks good.

I might name the 'changelog_notes' parameter to 'has_changelog_notes' to better communicate that it's a bool rather than e.g. a list of notes or something. But this is internal and well documented so fine if you'd rather keep it as is. Either way, no need for another review round trip.

Otherwise, is a very straightforward fix, and very nice to see the additional code docs.

review: Approve

[Robie Basak (racb)]

Thank you for the review. I agree with the "has_changelog_notes" improvement, but I will avoid that for now to save another manual test cycle (as we have no automatic tests).

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 9bf5a8f..1ea6c6d 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -1307,8 +1307,23 @@ class GitUbuntuRepository:
         self,
         remote_name,
         fetch_url,
-        push_url=None
+        push_url=None,
+        changelog_notes=False,
     ):
+        """Add a remote by URL
+
+        If a remote with the given name doesn't exist, then create it.
+        Otherwise, do nothing.
+
+        :param str remote_name: the name of the remote to create
+        :param str fetch_url: the fetch URL for the remote
+        :param str push_url: the push URL for the remote. If None, then a
+            specific push URL will not be set.
+        :param bool changelog_notes: if True, then a fetch refspec will be
+            added to fetch changelog notes. This only makes sense for an
+            official importer remote such as 'pkg'.
+        :returns: None
+        """
         if not self._fetch_proto:
             raise Exception('Cannot fetch using an object without a protocol')
 
@@ -1325,13 +1340,14 @@ class GitUbuntuRepository:
                 remote_name,
                 '+refs/tags/*:refs/tags/%s/*' % remote_name,
             )
-            # The changelog notes are kept at refs/notes/commits on
-            # Launchpad due to LP: #1871838 even though our standard place for
-            # them is refs/notes/changelog.
-            self.raw_repo.remotes.add_fetch(
-                remote_name,
-                '+refs/notes/commits:refs/notes/changelog',
-            )
+            if changelog_notes:
+                # The changelog notes are kept at refs/notes/commits on
+                # Launchpad due to LP: #1871838 even though our standard place
+                # for them is refs/notes/changelog.
+                self.raw_repo.remotes.add_fetch(
+                    remote_name,
+                    '+refs/notes/commits:refs/notes/changelog',
+                )
             if push_url:
                 self.raw_repo.remotes.set_push_url(
                     remote_name,
@@ -1345,7 +1361,25 @@ class GitUbuntuRepository:
                 ]
             )
 
-    def _add_remote(self, remote_name, remote_url):
+    def _add_remote(self, remote_name, remote_url, changelog_notes=False):
+        """Add a remote by URL location
+
+        URL location means the part of the URL after the proto:// prefix. The
+        protocol to be used will be determined by what was specified by the
+        fetch_proto at class instance construction time. Separate fetch and
+        push URL protocols will be automatically determined.
+
+        If a remote with the given name doesn't exist, then create it.
+        Otherwise, do nothing.
+
+        :param str remote_name: the name of the remote to create
+        :param str remote_url: the URL for the remote but with the proto://
+            prefix missing.
+        :param bool changelog_notes: if True, then a fetch refspec will be
+            added to fetch changelog notes. This only makes sense for an
+            official importer remote such as 'pkg'.
+        :returns: None
+        """
         if not self._fetch_proto:
             raise Exception('Cannot fetch using an object without a protocol')
         if not self._lp_user:
@@ -1353,9 +1387,20 @@ class GitUbuntuRepository:
         fetch_url = '%s://%s' % (self._fetch_proto, remote_url)
         push_url = 'ssh://%s@%s' % (self.lp_user, remote_url)
 
-        self._add_remote_by_fetch_url(remote_name, fetch_url, push_url)
+        self._add_remote_by_fetch_url(
+            remote_name=remote_name,
+            fetch_url=fetch_url,
+            push_url=push_url,
+            changelog_notes=changelog_notes,
+        )
 
-    def add_remote(self, pkgname, repo_owner, remote_name):
+    def add_remote(
+        self,
+        pkgname,
+        repo_owner,
+        remote_name,
+        changelog_notes=False,
+    ):
         """Add a remote to the repository configuration
         :param str pkgname: the name of the source package reflected by this
             repository.
@@ -1364,6 +1409,9 @@ class GitUbuntuRepository:
             If None, the default repository for the source package will be
             used.
         :param str remote_name: the name of the remote to add.
+        :param bool changelog_notes: if True, then a fetch refspec will be
+            added to fetch changelog notes. This only makes sense for an
+            official importer remote such as 'pkg'.
         :returns: None
         """
         if not self._fetch_proto:
@@ -1374,7 +1422,11 @@ class GitUbuntuRepository:
         else:
             remote_url = ('git.launchpad.net/ubuntu/+source/%s' % pkgname)
 
-        self._add_remote(remote_name, remote_url)
+        self._add_remote(
+            remote_name=remote_name,
+            remote_url=remote_url,
+            changelog_notes=changelog_notes,
+        )
 
     def add_remote_by_url(self, remote_name, fetch_url):
         if not self._fetch_proto:
@@ -1393,7 +1445,7 @@ class GitUbuntuRepository:
             used.
         :returns: None
         """
-        self.add_remote(pkgname, repo_owner, 'pkg')
+        self.add_remote(pkgname, repo_owner, 'pkg', changelog_notes=True)
 
     def add_lpuser_remote(self, pkgname):
         if not self._fetch_proto: