Handle more local dev version cases

Due to an oversight in testing (the tests for local dev versions were
all running with PBR_VERSION in the environment) we did not handle
enough variety of local development versions.

Specific new cases we now handle:
 - untagged trees - where git describe returns just the sha
 - tags with versions with less than two components (e.g. 1)

Change-Id: Ieaeca592b6ba26c5da50dcad3fb6e7551431d50e
Co-Authored-By: Robert Collins <rbtcollins@hp.com>

7 files changed, 85 insertions, 9 deletions

diff --git a/pbr/packaging.py b/pbr/packaging.py
index 036483e..29817e1 100644
--- a/pbr/packaging.py
+++ b/pbr/packaging.py
@@ -810,8 +810,12 @@ def _get_version_from_git(pre_version):
                     ['log', '-n1', '--pretty=format:%h'], git_dir)
                 return "%s.dev%s.g%s" % (pre_version, _get_revno(git_dir), sha)
         else:
-            return _run_git_command(
+            description = _run_git_command(
                 ['describe', '--always'], git_dir).replace('-', '.')
+            if '.' not in description:
+                # Untagged tree.
+                description = '0.g%s' % description
+            return description
     # If we don't know the version, return an empty string so at least
     # the downstream users of the value always have the same type of
     # object to work with.
diff --git a/pbr/tests/base.py b/pbr/tests/base.py
index ce20de1..0586074 100644
--- a/pbr/tests/base.py
+++ b/pbr/tests/base.py
@@ -98,6 +98,9 @@ class BaseTestCase(testtools.TestCase, testresources.ResourcedTestCase):
         self.useFixture(fixtures.TempHomeDir())
         self.useFixture(fixtures.NestedTempfile())
         self.useFixture(fixtures.FakeLogger())
+        # TODO(lifeless) we should remove PBR_VERSION from the environment.
+        # rather than setting it, because thats not representative - we need to
+        # test non-preversioned codepaths too!
         self.useFixture(fixtures.EnvironmentVariable('PBR_VERSION', '0.0'))
 
         self.temp_dir = self.useFixture(fixtures.TempDir()).path
diff --git a/pbr/tests/test_packaging.py b/pbr/tests/test_packaging.py
index 46dd0d5..16194d2 100644
--- a/pbr/tests/test_packaging.py
+++ b/pbr/tests/test_packaging.py
@@ -43,6 +43,7 @@ import tempfile
 
 import fixtures
 import mock
+import testscenarios
 
 from pbr import packaging
 from pbr.tests import base
@@ -74,10 +75,23 @@ class TestRepo(fixtures.Fixture):
 
 class TestPackagingInGitRepoWithCommit(base.BaseTestCase):
 
+    scenarios = [
+        ('preversioned', dict(preversioned=True)),
+        ('postversioned', dict(preversioned=False)),
+    ]
+
     def setUp(self):
         super(TestPackagingInGitRepoWithCommit, self).setUp()
         repo = self.useFixture(TestRepo(self.package_dir))
         repo.commit()
+        if not self.preversioned:
+            self.useFixture(fixtures.EnvironmentVariable('PBR_VERSION'))
+            setup_cfg_path = os.path.join(self.package_dir, 'setup.cfg')
+            with open(setup_cfg_path, 'rt') as cfg:
+                content = cfg.read()
+            content = content.replace(u'version = 0.1.dev', u'')
+            with open(setup_cfg_path, 'wt') as cfg:
+                cfg.write(content)
         self.run_setup('sdist', allow_fail=False)
 
     def test_authors(self):
@@ -157,3 +171,7 @@ class TestNestedRequirements(base.BaseTestCase):
             f.write('pbr')
         result = packaging.parse_requirements([requirements])
         self.assertEqual(result, ['pbr'])
+
+
+def load_tests(loader, in_tests, pattern):
+    return testscenarios.load_tests_apply_scenarios(loader, in_tests, pattern)
diff --git a/pbr/tests/test_version.py b/pbr/tests/test_version.py
index 315ad78..2a1143a 100644
--- a/pbr/tests/test_version.py
+++ b/pbr/tests/test_version.py
@@ -119,12 +119,31 @@ class TestSemanticVersion(base.BaseTestCase):
         parsed = from_pip_string('0.10.1.3.g83bef74')
         self.assertEqual(expected, parsed)
 
+    def test_from_pip_string_legacy_corner_case_dev(self):
+        # If the last tag is missing, or if the last tag has less than 3
+        # components, we need to 0 extend on parsing.
+        expected = version.SemanticVersion(
+            0, 0, 0, dev_count=1, githash='83bef74')
+        parsed = from_pip_string('0.0.g83bef74')
+        self.assertEqual(expected, parsed)
+
+    def test_from_pip_string_legacy_short_dev(self):
+        # If the last tag is missing, or if the last tag has less than 3
+        # components, we need to 0 extend on parsing.
+        expected = version.SemanticVersion(
+            0, 0, 0, dev_count=1, githash='83bef74')
+        parsed = from_pip_string('0.g83bef74')
+        self.assertEqual(expected, parsed)
+
     def test_from_pip_string_dev_missing_patch_version(self):
         expected = version.SemanticVersion(
             2014, 2, dev_count=21, githash='c4c8d0b')
         parsed = from_pip_string('2014.2.dev21.gc4c8d0b')
         self.assertEqual(expected, parsed)
 
+    def test_from_pip_string_pure_git_hash(self):
+        self.assertRaises(ValueError, from_pip_string, '6eed5ae')
+
     def test_final_version(self):
         semver = version.SemanticVersion(1, 2, 3)
         self.assertEqual((1, 2, 3, 'final', 0), semver.version_tuple())
diff --git a/pbr/tests/testpackage/pbr_testpackage/__init__.py b/pbr/tests/testpackage/pbr_testpackage/__init__.py
index e69de29..aa56dc6 100644
--- a/pbr/tests/testpackage/pbr_testpackage/__init__.py
+++ b/pbr/tests/testpackage/pbr_testpackage/__init__.py
@@ -0,0 +1,3 @@
+import pbr.version
+
+__version__ = pbr.version.VersionInfo('pbr_testpackage').version_string()
diff --git a/pbr/tests/testpackage/setup.cfg b/pbr/tests/testpackage/setup.cfg
index a410e3c..0188bd2 100644
--- a/pbr/tests/testpackage/setup.cfg
+++ b/pbr/tests/testpackage/setup.cfg
@@ -1,5 +1,7 @@
 [metadata]
 name = pbr_testpackage
+# TODO(lifeless) we should inject this as needed otherwise we're not truely
+# testing postversioned codepaths.
 version = 0.1.dev
 author = OpenStack
 author-email = openstack-dev@lists.openstack.org
diff --git a/pbr/version.py b/pbr/version.py
index cb2de9a..748364f 100644
--- a/pbr/version.py
+++ b/pbr/version.py
@@ -140,18 +140,37 @@ class SemanticVersion(object):
 
     @classmethod
     def from_pip_string(klass, version_string):
-        """Create a SemanticVersion from a version string.
+        """Create a SemanticVersion from a pip version string.
 
         This method will parse a version like 1.3.0 into a SemanticVersion.
 
-        For compatibility 1.3.0a1 versions are handled, though SemanticVersion
-        will output them as 1.3.0.0a1 for PEP-440 compatability, and similarly
-        pre-pbr-semver dev versions like 0.10.1.3.g83bef74 will be parsed but
+        This method is responsible for accepting any version string that any
+        older version of pbr ever created.
+
+        Therefore: versions like 1.3.0a1 versions are handled, parsed into a
+        canonical form and then output - resulting in 1.3.0.0a1.
+        Pre pbr-semver dev versions like 0.10.1.3.g83bef74 will be parsed but
         output as 0.10.1.dev3.g83bef74.
+
+        :raises ValueError: Never tagged versions sdisted by old pbr result in
+            just the git hash, e.g. '1234567' which poses a substantial problem
+            since they collide with the semver versions when all the digits are
+            numerals. Such versions will result in a ValueError being thrown if
+            any non-numeric digits are present. They are an exception to the
+            general case of accepting anything we ever output, since they were
+            never intended and would permanently mess up versions on PyPI if
+            ever released - we're treating that as a critical bug that we ever
+            made them and have stopped doing that.
         """
-        components = version_string.split('.')
-        if len(components) < 3:
-            components.extend([0] * (3 - len(components)))
+        input_components = version_string.split('.')
+        # decimals first (keep pre-release and dev/hashes to the right)
+        components = [c for c in input_components if c.isdigit()]
+        digit_len = len(components)
+        if digit_len == 0:
+            raise ValueError("Invalid version %r" % version_string)
+        elif digit_len < 3:
+            components.extend([0] * (3 - digit_len))
+        components.extend(input_components[digit_len:])
         major = int(components[0])
         minor = int(components[1])
         dev_count = None
@@ -191,7 +210,15 @@ class SemanticVersion(object):
                 prerelease_type, prerelease = _parse_type(remainder[0])
                 remainder = remainder[1:]
             if remainder:
-                dev_count = int(remainder[0][3:])
+                component = remainder[0]
+                if component.startswith('dev'):
+                    dev_count = int(component[3:])
+                elif component.startswith('g'):
+                    # git hash - so use a dev_count of 1 as we have to have one
+                    dev_count = 1
+                    githash = component[1:]
+                else:
+                    raise ValueError('Unknown remainder %r' % (remainder,))
         if len(remainder) > 1:
                 githash = remainder[1][1:]
         return SemanticVersion(