rpm refactor: rewrite set_tag functionality

In addition to refactoring, make sure that we keep the internal _tags
structure in sync.

Signed-off-by: Markus Lehtonen <markus.lehtonen@linux.intel.com>

5 files changed, 194 insertions, 18 deletions

diff --git a/gbp/rpm/__init__.py b/gbp/rpm/__init__.py
index e5ff1484..61cf13be 100644
--- a/gbp/rpm/__init__.py
+++ b/gbp/rpm/__init__.py
@@ -439,24 +439,82 @@ class SpecFile(object):
             if patchnum in self.ignorepatches:
                 self.patches[patchnum]['autoupdate'] = False
 
-    def set_tag(self, tag, value):
+    def _delete_tag(self, tag, num):
+        """Delete a tag"""
+        key = tag.lower()
+        tagname = '%s%s' % (tag, num) if num is not None else tag
+        if key not in self._tags:
+            gbp.log.warn("Trying to delete non-existent tag '%s:'" % tag)
+            return None
+
+        sparedlines = []
+        prev = None
+        for line in self._tags[key]['lines']:
+            if line['num'] == num:
+                gbp.log.debug("Removing '%s:' tag from spec" % tagname)
+                prev = self._content.delete(line['line'])
+            else:
+                sparedlines.append(line)
+        self._tags[key]['lines'] = sparedlines
+        if not self._tags[key]['lines']:
+            self._tags.pop(key)
+        return prev
+
+    def _set_tag(self, tag, num, value, insertafter):
+        """Set a tag value"""
+        key = tag.lower()
+        tagname = '%s%s' % (tag, num) if num is not None else tag
+        value = value.strip()
+        if not value:
+            raise GbpError("Cannot set empty value to '%s:' tag" % tag)
+
+        # Check type of tag, we don't support values for 'multivalue' tags
+        try:
+            header = self._specinfo.packages[0].header
+            tagvalue = header[getattr(rpm, 'RPMTAG_%s' % tagname.upper())]
+        except AttributeError:
+            tagvalue = None
+        tagvalue = None if type(tagvalue) is list else value
+
+        # Try to guess the correct indentation from the previous tag
+        match = re.match(r'^([a-z]+([0-9]+)?\s*:\s*)', str(insertafter), re.I)
+        indent = 12 if not match else len(match.group(1))
+        text = '%-*s%s\n' % (indent, '%s:' % tagname, value)
+        if key in self._tags:
+            self._tags[key]['value'] = tagvalue
+            for line in reversed(self._tags[key]['lines']):
+                if line['num'] == num:
+                    gbp.log.debug("Updating '%s:' tag in spec" % tagname)
+                    line['line'].set_data(text)
+                    line['linevalue'] = value
+                    return line['line']
+
+        gbp.log.debug("Adding '%s:' tag after '%s...' line in spec" %
+                      (tagname, str(insertafter)[0:20]))
+        line = self._content.insert_after(insertafter, text)
+        linerec = {'line': line, 'num': num, 'linevalue': value}
+        if key in self._tags:
+            self._tags[key]['lines'].append(linerec)
+        else:
+            self._tags[key] = {'value': tagvalue, 'lines': [linerec]}
+        return line
+
+    def set_tag(self, tag, num, value, insertafter=None):
         """Update a tag in spec file content"""
         key = tag.lower()
-        if key == 'vcs':
+        tagname = '%s%s' % (tag, num) if num is not None else tag
+        if key in ('patch', 'vcs'):
+            if key in self._tags:
+                insertafter = key
+            elif not insertafter in self._tags:
+                insertafter = 'release'
+            after_line = self._tags[insertafter]['lines'][-1]['line']
             if value:
-                text = '%-12s%s\n' % ('VCS:', value)
-                if key in self._tags:
-                    gbp.log.info("Updating '%s' tag in spec" % tag)
-                    self._tags[key]['lines'][-1]['line'].set_data(text)
-                else:
-                    gbp.log.info("Adding '%s' tag to spec" % tag)
-                    self._content.insert_after(
-                        self._tags['release']['lines'][-1]['line'], text)
+                self._set_tag(tag, num, value, after_line)
             elif key in self._tags:
-                gbp.log.info("Removing '%s' tag from spec" % tag)
-                self._content.delete(self._tags[key]['lines'][-1]['line'])
+                self._delete_tag(tag, num)
         else:
-            raise GbpError("Setting '%s:' tag not supported")
+            raise GbpError("Setting '%s:' tag not supported" % tagname)
 
     def update_patches(self, patchfilenames):
         """Update spec with new patch tags and patch macros"""
diff --git a/gbp/scripts/buildpackage_rpm.py b/gbp/scripts/buildpackage_rpm.py
index 965f3207..0ad5838e 100755
--- a/gbp/scripts/buildpackage_rpm.py
+++ b/gbp/scripts/buildpackage_rpm.py
@@ -596,7 +596,8 @@ def main(argv):
                 tree_name = repo.find_tag('HEAD', longfmt=True, always=True) +\
                             '-dirty'
         # Put 'VCS:' tag to .spec
-        spec.set_tag('vcs', options.spec_vcs_tag % {'tagname': tree_name})
+        spec.set_tag('VCS', None,
+                     options.spec_vcs_tag % {'tagname': tree_name})
         spec.write_spec_file()
 
     except CommandExecFailed:
diff --git a/tests/test_rpm.py b/tests/test_rpm.py
index 55bb4159..88eb571f 100644
--- a/tests/test_rpm.py
+++ b/tests/test_rpm.py
@@ -30,6 +30,14 @@ DATA_DIR = os.path.abspath(os.path.splitext(__file__)[0] + '_data')
 SRPM_DIR = os.path.join(DATA_DIR, 'srpms')
 SPEC_DIR = os.path.join(DATA_DIR, 'specs')
 
+class SpecFileTester(SpecFile):
+    """Helper class for testing"""
+
+    def protected(self, name):
+        """Get a protected member"""
+        return super(SpecFileTester, self).__getattribute__(name)
+
+
 class TestSrcRpmFile(object):
     """Test L{gbp.rpm.SrcRpmFile}"""
 
@@ -77,7 +85,7 @@ class TestSpecFile(object):
     def test_spec(self):
         """Test parsing of a valid spec file"""
         spec_filepath = os.path.join(SPEC_DIR, 'gbp-test.spec')
-        spec = SpecFile(spec_filepath)
+        spec = SpecFileTester(spec_filepath)
 
         # Test basic properties
         assert spec.specfile == spec_filepath
@@ -154,7 +162,7 @@ class TestSpecFile(object):
 
         # Test adding the VCS tag
         reference_spec = os.path.join(SPEC_DIR, 'gbp-test-reference2.spec')
-        spec.set_tag('vcs', 'myvcstag')
+        spec.set_tag('VCS', None, 'myvcstag')
         spec.write_spec_file()
         assert filecmp.cmp(tmp_spec, reference_spec) is True
 
@@ -166,14 +174,41 @@ class TestSpecFile(object):
         reference_spec = os.path.join(SPEC_DIR, 'gbp-test2-reference2.spec')
         spec = SpecFile(tmp_spec)
         spec.update_patches(['1.patch', '2.patch'])
-        spec.set_tag('vcs', 'myvcstag')
+        spec.set_tag('VCS', None, 'myvcstag')
         spec.update_patches(['new.patch'])
         spec.write_spec_file()
         assert filecmp.cmp(tmp_spec, reference_spec) is True
 
         # Test removing the VCS tag
         reference_spec = os.path.join(SPEC_DIR, 'gbp-test2-reference.spec')
-        spec.set_tag('vcs', '')
+        spec.set_tag('VCS', None, '')
+        spec.write_spec_file()
+        assert filecmp.cmp(tmp_spec, reference_spec) is True
+
+    def test_modifying(self):
+        """Test updating/deleting of tags and macros"""
+        tmp_spec = os.path.join(self.tmpdir, 'gbp-test.spec')
+        shutil.copy2(os.path.join(SPEC_DIR, 'gbp-test-updates.spec'), tmp_spec)
+        reference_spec = os.path.join(SPEC_DIR,
+                                      'gbp-test-updates-reference.spec')
+        spec = SpecFileTester(tmp_spec)
+
+        # Mangle tags
+        prev = spec.protected('_delete_tag')('Vendor', None)
+        spec.protected('_set_tag')('License', None, 'new license', prev)
+        spec.protected('_delete_tag')('source', 0)
+        spec.protected('_delete_tag')('patch', 0)
+        spec.protected('_delete_tag')('patch', -1)
+        prev = spec.protected('_delete_tag')('invalidtag', None)
+
+        with assert_raises(GbpError):
+            # Check that setting empty value fails
+            spec.protected('_set_tag')('Version', None, '', prev)
+        with assert_raises(GbpError):
+            # Check that setting invalid tag with public method fails
+            spec.set_tag('invalidtag', None, 'value')
+
+        # Check resulting spec file
         spec.write_spec_file()
         assert filecmp.cmp(tmp_spec, reference_spec) is True
 
diff --git a/tests/test_rpm_data/specs/gbp-test-updates-reference.spec b/tests/test_rpm_data/specs/gbp-test-updates-reference.spec
new file mode 100644
index 00000000..557f8b77
--- /dev/null
+++ b/tests/test_rpm_data/specs/gbp-test-updates-reference.spec
@@ -0,0 +1,39 @@
+#
+# Spec file for testing deleting/adding/updating tags and macros
+#
+
+# Gbp-Undefined-Tag: foobar
+
+# Test that we accept different cases
+Name:           my_name
+Version:        0
+Release:        1
+Summary:        my_summary
+License:        new license
+Distribution:   my_distribution
+Group:          my_group
+Packager:       my_packager
+Url:            my_url
+Vcs:            my_vcs
+BuildRoot:      my_buildroot
+Provides:       my_provides
+Requires:       my_requires
+Conflicts:      my_conflicts
+Obsoletes:      my_obsoletes
+BuildConflicts: my_buildconflicts
+BuildRequires:  my_buildrequires
+AutoReqProv:    No
+AutoReq:        No
+AutoProv:       No
+DistTag:        my_disttag
+BugUrl:         my_bugurl
+Collections:    my_collections
+
+%description
+Package for testing GBP.
+
+%prep
+
+%build
+
+%install
diff --git a/tests/test_rpm_data/specs/gbp-test-updates.spec b/tests/test_rpm_data/specs/gbp-test-updates.spec
new file mode 100644
index 00000000..1cdcb212
--- /dev/null
+++ b/tests/test_rpm_data/specs/gbp-test-updates.spec
@@ -0,0 +1,43 @@
+#
+# Spec file for testing deleting/adding/updating tags and macros
+#
+
+# Gbp-Undefined-Tag: foobar
+
+# Test that we accept different cases
+Name:           my_name
+Version:        0
+Release:        1
+Summary:        my_summary
+License:        my_license
+Distribution:   my_distribution
+Vendor:         my_vendor
+Group:          my_group
+Packager:       my_packager
+Url:            my_url
+Vcs:            my_vcs
+Source:         my_source
+Patch:          my_%patch_fn_base
+Patch0:         my_%{patch_fn_base}0
+BuildRoot:      my_buildroot
+Provides:       my_provides
+Requires:       my_requires
+Conflicts:      my_conflicts
+Obsoletes:      my_obsoletes
+BuildConflicts: my_buildconflicts
+BuildRequires:  my_buildrequires
+AutoReqProv:    No
+AutoReq:        No
+AutoProv:       No
+DistTag:        my_disttag
+BugUrl:         my_bugurl
+Collections:    my_collections
+
+%description
+Package for testing GBP.
+
+%prep
+
+%build
+
+%install