check_source: Fix revert case for patch detection (fix#2640)

check_source.pl

@@ -144,7 +144,6 @@ if (-d "$old") {
     my $cf = Build::read_config("x86_64", "/usr/lib/build/configs/default.conf");
 
     my %thash = ();
-    my %rhash = ();
     for my $spec (glob("*.spec")) {
         my $ps = Build::Rpm::parse($cf, $spec);
 
@@ -213,8 +212,11 @@ if (-d "$old") {
                 $diff = diff "$old/$changes", "$dir/$changes";
             }
             for my $line (split(/\n/, $diff)) {
-                next unless $line =~ m/^\+/;
-                $line =~ s/^\+//;
+                # Check if the line mentions a patch being added (starts with +)
+                # or removed (starts with -)
+                next unless $line =~ m/^[+-]/;
+                # In any of those cases, remove the patch from the list
+                $line =~ s/^[+-]//;
                 for my $patch (keys %patches) {
                     if (index($line, $patch) != -1) {
                         delete $patches{$patch};

tests/OBSLocal.py

@@ -544,7 +544,7 @@ class StagingWorkflow(ABC):
         self.requests.append(request)
         return request
 
-    def create_submit_request(self, project, package, text=None):
+    def create_submit_request(self, project, package, text=None, add_commit=True):
         """Creates submit request from package in specified project to default project.
 
         It creates project if not exist and also package.
@@ -557,12 +557,16 @@ class StagingWorkflow(ABC):
         :type package: str
         :param text: commit message for initial package creation
         :type text: str
+        :param add_commit: whether add initial package commit. Useful to disable
+               if package already exists
+        :type add_commit: bool
         :return: created request.
         :rtype: Request
         """
         project = self.create_project(project)
         package = Package(name=package, project=project)
-        package.create_commit(text=text)
+        if add_commit:
+            package.create_commit(text=text)
         return self.submit_package(package)
 
     def __del__(self):
@@ -1015,7 +1019,8 @@ class Package(object):
         :param path: path to a directory containing the files that must be added to the package
         """
         for filename in os.listdir(path):
-            with open(os.path.join(path, filename)) as f:
+            # Opening as binary is needed e.g. for compressed tarball sources
+            with open(os.path.join(path, filename), 'rb') as f:
                 self.create_commit(filename=filename, text=f.read())
 
 class Request(object):

tests/check_source_tests.py

@@ -69,7 +69,59 @@ class TestCheckSource(OBSLocal.TestCase):
         """Accepts a request coming from a devel project"""
         self._setup_devel_project()
 
-        req_id = self.wf.create_submit_request(SRC_PROJECT, 'blowfish').reqid
+        req_id = self.wf.create_submit_request(SRC_PROJECT, 'blowfish', add_commit = False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'accepted'))
+        self.assertReview(req_id, by_group=(REVIEW_TEAM, 'new'))
+
+    def test_missing_patch_in_changelog(self):
+        """Reject a request if it adds patch and it is not mentioned in changelog"""
+        # devel files contain patch but not changes
+        self._setup_devel_project(devel_files='blowfish-with-patch')
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+            self.devel_package.name, add_commit = False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        review = self.assertReview(req_id, by_user=(self.bot_user, 'declined'))
+        self.assertIn(
+            'A patch (test.patch) is being added without this addition being mentioned in the changelog.',
+            review.comment
+        )
+
+    def test_patch_in_changelog(self):
+        """Accepts a request if it adds patch and it is mentioned in changelog"""
+        self._setup_devel_project()
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+            self.devel_package.name, add_commit = False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'accepted'))
+        self.assertReview(req_id, by_group=(REVIEW_TEAM, 'new'))
+
+    def test_revert_of_patch(self):
+        """Accepts a request if it reverts addition of patch"""
+        # switch target and devel, so basically do revert of changes done
+        # with patch and changes
+        self._setup_devel_project(devel_files='blowfish',
+            target_files='blowfish-with-patch-changes')
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+            self.devel_package.name, add_commit = False).reqid
 
         self.assertReview(req_id, by_user=(self.bot_user, 'new'))
 
@@ -90,7 +142,7 @@ class TestCheckSource(OBSLocal.TestCase):
         self.wf.create_group(FACTORY_MAINTAINERS.replace('group:', ''))
         self.wf.remote_config_set({ 'required-source-maintainer': FACTORY_MAINTAINERS })
 
-        req = self.wf.create_submit_request(SRC_PROJECT, 'blowfish')
+        req = self.wf.create_submit_request(SRC_PROJECT, 'blowfish', add_commit = False)
 
         self.assertReview(req.reqid, by_user=(self.bot_user, 'new'))
 
@@ -122,7 +174,7 @@ class TestCheckSource(OBSLocal.TestCase):
 
         self._setup_devel_project(maintainer={'groups': [group_name]})
 
-        req_id = self.wf.create_submit_request(SRC_PROJECT, 'blowfish').reqid
+        req_id = self.wf.create_submit_request(SRC_PROJECT, 'blowfish', add_commit = False).reqid
 
         self.assertReview(req_id, by_user=(self.bot_user, 'new'))
 
@@ -144,7 +196,7 @@ class TestCheckSource(OBSLocal.TestCase):
 
         self._setup_devel_project()
 
-        req = self.wf.create_submit_request(SRC_PROJECT, 'blowfish')
+        req = self.wf.create_submit_request(SRC_PROJECT, 'blowfish', add_commit = False)
 
         self.assertReview(req.reqid, by_user=(self.bot_user, 'new'))
 
@@ -160,11 +212,14 @@ class TestCheckSource(OBSLocal.TestCase):
         self.assertEqual(add_role_req.actions[0].tgt_project, SRC_PROJECT)
         self.assertEqual('Created automatically from request %s' % req.reqid, add_role_req.description)
 
-    def _setup_devel_project(self, maintainer={}):
+    def _setup_devel_project(self, maintainer={}, devel_files='blowfish-with-patch-changes',
+            target_files='blowfish'):
         devel_project = self.wf.create_project(SRC_PROJECT, maintainer=maintainer)
-        devel_package = OBSLocal.Package('blowfish', project=devel_project)
+        self.devel_package = OBSLocal.Package('blowfish', project=devel_project)
 
-        fixtures_path = os.path.join(FIXTURES, 'packages', 'blowfish')
-        devel_package.commit_files(fixtures_path)
+        fixtures_path = os.path.join(FIXTURES, 'packages', devel_files)
+        self.devel_package.commit_files(fixtures_path)
 
-        OBSLocal.Package('blowfish', self.project, devel_project=SRC_PROJECT)
+        fixtures_path = os.path.join(FIXTURES, 'packages', target_files)
+        self.target_package = OBSLocal.Package('blowfish', self.project, devel_project=SRC_PROJECT)
+        self.target_package.commit_files(fixtures_path)

tests/fixtures/packages/blowfish-with-patch-changes/blowfish.changes

@@ -0,0 +1,12 @@
+-------------------------------------------------------------------
+Thu Jul  8 08:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Patched version
+-- add test.patch
+- 2
+
+-------------------------------------------------------------------
+Thu Jul  8 07:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Initial version.
+- 1

tests/fixtures/packages/blowfish-with-patch-changes/blowfish.spec

@@ -0,0 +1,20 @@
+#
+# Copyright (c) 2020 SUSE LLC
+#
+# This file is under MIT license
+
+
+Name:           blowfish
+Version:        2
+Release:        0
+Summary:        Blowfish
+License:        GPL-2.0-only
+URL:            https://github.com/openSUSE/cockpit-wicked
+Source:         blowfish-1.tar.gz
+Patch1:        test.patch
+BuildArch:      noarch
+
+%prep
+%patch1
+
+%changelog

tests/fixtures/packages/blowfish-with-patch-changes/test.patch

@@ -0,0 +1 @@
++-

tests/fixtures/packages/blowfish-with-patch/blowfish.changes

@@ -0,0 +1,10 @@
+-------------------------------------------------------------------
+Wed Sep 22 07:30:53 UTC 2021 - Josef Reidinger <jreidinger@suse.com>
+
+- just add patch without explicitly mentioning it
+
+-------------------------------------------------------------------
+Thu Jul  8 07:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Initial version.
+- 1

tests/fixtures/packages/blowfish-with-patch/blowfish.spec

@@ -0,0 +1,20 @@
+#
+# Copyright (c) 2020 SUSE LLC
+#
+# This file is under MIT license
+
+
+Name:           blowfish
+Version:        1
+Release:        0
+Summary:        Blowfish
+License:        GPL-2.0-only
+URL:            https://github.com/openSUSE/cockpit-wicked
+Source:         blowfish-1.tar.gz
+Patch1:        test.patch
+BuildArch:      noarch
+
+%prep
+%patch1
+
+%changelog

tests/fixtures/packages/blowfish-with-patch/test.patch

@@ -0,0 +1 @@
++-