Merge lp:~robru/cupstream2distro/fix-missing-commit-check into lp:cupstream2distro

[Robert Bruce Park]

Do three-way diffing in order to ignore false positive trunk commits.

[Robert Bruce Park (robru)]

Looks good in staging.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1306
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/940/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/940/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1307
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/941/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/941/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1308
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/942/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/942/rebuild

review: Approve (continuous-integration)

=== modified file 'citrain/recipes/merge.py'
--- citrain/recipes/merge.py	2015-12-02 05:35:19 +0000
+++ citrain/recipes/merge.py	2015-12-04 01:43:03 +0000
@@ -226,15 +226,14 @@
     def check_new_commits(self, source=None):
         """Report if this package needs a rebuild, or not."""
         count = 0
-        branches = [self.merges[0].target_branch] + [
+        target = self.merges[0].target_branch
+        branches = [target] + [
             branch.source_branch for branch in self.merges]
         for branch in branches:
-            try:
-                if self.missing_branch_commits(branch.bzr_identity):
-                    logging.error('New commits: %s', branch.web_link)
-                    count += 1
-            except FileNotFoundError:
-                return 'Ready to build'
+            if self.missing_branch_commits(
+                    branch.bzr_identity, target.bzr_identity):
+                logging.error('New commits: %s', branch.web_link)
+                count += 1
         if count:
             return 'Needs rebuild due to new commits'
         return False

=== modified file 'cupstream2distro/branchhandling.py'
--- cupstream2distro/branchhandling.py	2015-12-02 05:35:19 +0000
+++ cupstream2distro/branchhandling.py	2015-12-04 01:43:03 +0000
@@ -36,8 +36,7 @@
 )
 
 
-TRANSLATIONS = {'Launchpad', 'Translations', 'behalf'}
-ME = {'CI', 'Train', 'Bot'}
+REVID = 'revision-id: '
 SEPARATOR = '-' * 60
 IGNORED = ('Automatic PS uploader', 'Launchpad ')
 BUG_REGEX = re.compile(r"""
@@ -61,6 +60,11 @@
     'No handlers could be found for logger "bzr"'.split()
 )
 
+ME = '<bzr-whoami-failed-oh-god-why>'
+with suppress(FileNotFoundError):
+    ME = call('bzr whoami')[1]
+ME = ME[ME.index('<') + 1:ME.index('>')]
+
 
 def parse_bzr_log(log):
     """Generate a list of dicts representing bzr log data."""
@@ -113,11 +117,11 @@
 
 def bzr_missing(*args):
     """Call bzr missing."""
-    cmd = ('bzr', 'missing', '--directory') + args
+    cmd = ('bzr', 'missing', '--show-ids', '--directory') + args
     stdout, stderr = call(cmd)[1:]
     if stderr:
         if 'Not a branch' in stderr:
-            return 'committer: First build'  # Pretend there are new commits
+            return 'revision-id: very-first'  # Pretend there are new commits
         tokens = set(stderr.split())
         err = NoStatusError if len(TRANSIENT & tokens) > 6 else BranchError
         raise err('"bzr missing" failed: {}'.format(stderr))
@@ -128,12 +132,23 @@
 def bzr_cat(branch, filename):
     """Call bzr cat."""
     assert branch.startswith('lp:')
-    log, err = call(['bzr', 'cat', '-d', branch, filename])[1:]
+    log, err = call(['bzr', 'cat', '--directory', branch, filename])[1:]
     if err or not log:
         raise BranchError('{} unreachable: {}'.format(filename, err))
     return log
 
 
+def find_ids_to_ignore(branch, target):
+    """Ignore trunk commits missing from input branches."""
+    ignore = set()
+    if branch != target:
+        for line in bzr_missing(branch, target, '--theirs-only').splitlines():
+            if line.startswith(REVID):
+                ignore.add(line.partition(REVID)[-1])
+    log_value_of.ignore()
+    return ignore
+
+
 @ramdisk
 def get_package_name_from_branch(branch):
     """Get source package name for a particular branch.
@@ -260,19 +275,21 @@
                         authors[author].append(msg + format_bugs(bugs))
         return authors
 
-    def missing_branch_commits(self, branch):
+    def missing_branch_commits(self, branch, target):
         """Report any missing commits between our branch & another."""
-        # Ignore commits by me that only I have
-        ignore = TRANSLATIONS | ME
-        commits = 0
+        ignore = find_ids_to_ignore(branch, target)
+        ignore_me = True  # Ignore commits by me that only I have
         for line in bzr_missing(self.lp_branch, branch).splitlines():
             if line.startswith('You are missing '):
-                # Watch commits by me that I am missing (most likely on trunk)
-                ignore = TRANSLATIONS
-            if line.startswith('committer:'):
-                if len(set(line.split()) & ignore) < 3:
-                    commits += 1
-        return commits
+                ignore_me = False  # Watch commits by me that I'm missing
+            if line.startswith(REVID):
+                if 'launchpad_translations_on_behalf_of_' in line:
+                    continue
+                if ignore_me and ME in line:
+                    continue
+                if not set(line.split()) & ignore:
+                    logging.info('Disputed %s', line)
+                    return True
 
     def get_remote_branch_authors(self, branch):
         """Identify names of unique authors/committers in a lp branch.

=== modified file 'tests/strings.py'
--- tests/strings.py	2015-11-26 02:05:01 +0000
+++ tests/strings.py	2015-12-04 01:43:03 +0000
@@ -269,43 +269,44 @@
 
 BZR_MISSING_OURS = """
 You have 1000000 extra revisions:
-committer: CI Train Bot <[e-mail]>
-committer: CI Train Bot <[e-mail]>
-committer: CI Train Bot <[e-mail]>
+revision-id: [e-mail]-yyyymmdd-deadbeef1
+revision-id: [e-mail]-yyyymmdd-deadbeef2
+revision-id: [e-mail]-yyyymmdd-deadbeef3
+revision-id: [e-mail]-yyyymmdd-deadbeef4
 """
 
 BZR_MISSING_IGNORED = BZR_MISSING_OURS + """
 You are missing 5000 revisions:
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef1
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef2
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef3
 """
 
 BZR_MISSING_ONE = BZR_MISSING_OURS + """
 You are missing 10000 revisions:
-committer: CI Train Bot <[e-mail]>
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
+revision-id: [e-mail]-yyyymmdd-deadbeef5
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef4
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef5
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef6
 """
 
 BZR_MISSING_TWO = BZR_MISSING_OURS + """
 You are missing 5000 revisions:
-committer: CI Train Bot <[e-mail]>
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
-committer: Joe Blow deleted this commit from remote branch
+revision-id: [e-mail]-yyyymmdd-deadbeef6
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef7
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef8
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef9
+revision-id: joe-blow@deleted-this-commit-yyyymmdd-deadbeef6
 """
 
 BZR_MISSING_THREE = BZR_MISSING_OURS + """
 You are missing 10000 revisions:
-committer: CI Train Bot <[e-mail]>
-committer: CI Brain Rot on behalf of zombies
-committer: Lunchpad Translations on behalf of eating
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
-committer: Launchpad Translations on behalf of phablet-team
+revision-id: [e-mail]-yyyymmdd-deadbeef7
+revision-id: [e-mail]-yyyymmdd-deadbeef6
+revision-id: lunchpad_translations_on_behalf_of_foo-20151202054232-deadbeef
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef10
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef11
+revision-id: launchpad_translations_on_behalf_of_foo-20151202054232-deadbeef12
 """
 
 PACKAGING_DIFF_WARNING = """\

=== modified file 'tests/unit/test_branchhandling.py'
--- tests/unit/test_branchhandling.py	2015-12-02 04:35:10 +0000
+++ tests/unit/test_branchhandling.py	2015-12-04 01:43:03 +0000
@@ -27,7 +27,8 @@
 from cupstream2distro.utils import utf8_open
 from cupstream2distro.branchhandling import (
     Branch,
-    get_package_name_from_branch
+    get_package_name_from_branch,
+    find_ids_to_ignore,
 )
 from cupstream2distro.errors import BranchError, MergeError
 
@@ -43,13 +44,26 @@
         self.branch.series = Mock()
         self.branch.silo_ppa = Mock()
 
+    @patch('cupstream2distro.branchhandling.bzr_missing')
+    def test_find_ids_to_ignore(self, missing):
+        """Return a set of revision ids."""
+        missing.return_value = ''
+        self.assertEqual(find_ids_to_ignore('a', 'b'), set())
+        missing.return_value = s.BZR_MISSING_OURS
+        self.assertEqual(find_ids_to_ignore('a', 'b'), {
+            '[e-mail]-yyyymmdd-deadbeef1',
+            '[e-mail]-yyyymmdd-deadbeef4',
+            '[e-mail]-yyyymmdd-deadbeef3',
+            '[e-mail]-yyyymmdd-deadbeef2',
+        })
+
     @patch('cupstream2distro.branchhandling.call')
     def test_get_package_name_from_branch(self, call_mock):
         """Correctly get the package name from the branch."""
         call_mock.return_value = (Mock(returncode=0), 'ex (1.0)', '')
         self.assertEqual(get_package_name_from_branch('lp:example'), 'ex')
         call_mock.assert_called_once_with(
-            ['bzr', 'cat', '-d', 'lp:example', 'debian/changelog'])
+            ['bzr', 'cat', '--directory', 'lp:example', 'debian/changelog'])
 
     @patch('cupstream2distro.branchhandling.call')
     def test_get_package_name_from_branch_cached(self, call_mock):
@@ -58,7 +72,7 @@
         for count in range(10):
             self.assertEqual(get_package_name_from_branch('lp:example'), 'ex')
         call_mock.assert_called_once_with(  # ONCE
-            ['bzr', 'cat', '-d', 'lp:example', 'debian/changelog'])
+            ['bzr', 'cat', '--directory', 'lp:example', 'debian/changelog'])
 
     @patch('cupstream2distro.branchhandling.call')
     def test_get_package_name_from_branch_failed(self, call_mock):
@@ -67,7 +81,7 @@
         with self.assertRaisesRegexp(BranchError, 'unreachable: bark bark'):
             get_package_name_from_branch('lp:example2')
         call_mock.assert_called_once_with(
-            ['bzr', 'cat', '-d', 'lp:example2', 'debian/changelog'])
+            ['bzr', 'cat', '--directory', 'lp:example2', 'debian/changelog'])
 
     def test_branch_method_names(self):
         """Ensure all Branch method names contain "branch" for clarity."""
@@ -404,22 +418,23 @@
         self.assertEquals(self.branch.get_branch_commits(set()), ({}))
 
     @patch('cupstream2distro.branchhandling.bzr_missing')
+    @patch('cupstream2distro.branchhandling.ME', '[e-mail]')
     def test_missing_branch_commits(self, missing):
         """Ensure we can correctly count the number of missing commits."""
         missing.return_value = s.BZR_MISSING_IGNORED
-        self.assertEqual(self.branch.missing_branch_commits('lp:foo'), 0)
+        self.assertIsNone(self.branch.missing_branch_commits('lp:tr', 'lp:tr'))
         missing.return_value = s.BZR_MISSING_ONE
-        self.assertEqual(self.branch.missing_branch_commits('lp:foo'), 1)
+        self.assertTrue(self.branch.missing_branch_commits('lp:tr', 'lp:tr'))
         missing.return_value = s.BZR_MISSING_TWO
-        self.assertEqual(self.branch.missing_branch_commits('lp:foo'), 2)
+        self.assertTrue(self.branch.missing_branch_commits('lp:tr', 'lp:tr'))
         missing.return_value = s.BZR_MISSING_THREE
-        self.assertEqual(self.branch.missing_branch_commits('lp:foo'), 3)
+        self.assertTrue(self.branch.missing_branch_commits('lp:tr', 'lp:tr'))
 
     @patch('cupstream2distro.branchhandling.call')
     def test_missing_branch_commits_notabranch(self, call_mock):
         """Ensure we can correctly count the number of missing commits."""
         call_mock.return_value = None, '', 'Not a branch: /path/to/foo'
-        self.assertEqual(self.branch.missing_branch_commits('lp:foo'), 1)
+        self.assertTrue(self.branch.missing_branch_commits('lp:tr', 'lp:tr'))
 
     @patch('cupstream2distro.branchhandling.call')
     def test_get_remote_branch_authors(self, call_mock):
@@ -432,7 +447,7 @@
                 'CI Train Bot <[e-mail]>',
             ]))
         call_mock.assert_called_once_with(
-            ('bzr', 'missing', '--directory', self.tempdir,
+            ('bzr', 'missing', '--show-ids', '--directory', self.tempdir,
              'lp:~my/foo/features', '--other'))
 
     @patch('cupstream2distro.branchhandling.call')

=== modified file 'tests/unit/test_recipe_merge.py'
--- tests/unit/test_recipe_merge.py	2015-12-02 05:35:19 +0000
+++ tests/unit/test_recipe_merge.py	2015-12-04 01:43:03 +0000
@@ -438,13 +438,6 @@
         merge.missing_branch_commits = Mock(return_value=10)
         self.assertIn('Needs rebuild', merge.check_new_commits())
 
-    def test_merge_check_new_commits_filenotfound(self):
-        """Report if this package is missing local source."""
-        Merge.all_mps = dict(checkered=[Mock()])
-        merge = Merge('checkered', self.series, self.dest, self.ppa)
-        merge.missing_branch_commits = Mock(side_effect=FileNotFoundError)
-        self.assertEqual('Ready to build', merge.check_new_commits())
-
     def test_merge_unbuilt_phase(self):
         """Block publication if merges have new commits."""
         mp_obj = Mock()
@@ -456,8 +449,10 @@
             'foobaz has new, unbuilt commits.'
         })
         self.assertEqual(merge.missing_branch_commits.mock_calls, [
-            call(mp_obj.target_branch.bzr_identity),
-            call(mp_obj.source_branch.bzr_identity),
+            call(mp_obj.target_branch.bzr_identity,
+                 mp_obj.target_branch.bzr_identity),
+            call(mp_obj.source_branch.bzr_identity,
+                 mp_obj.target_branch.bzr_identity),
         ])
 
     def test_merge_unbuilt_phase_none(self):