Feature: use git branches/tags as versions (#31200)

Building on #24639, this allows versions to be prefixed by `git.`. If a version begins `git.`, it is treated as a git ref, and handled as git commits are starting in the referenced PR.

An exception is made for versions that are `git.develop`, `git.main`, `git.master`, `git.head`, or `git.trunk`. Those are assumed to be greater than all other versions, as those prefixed strings are in other contexts.

9 files changed, 305 insertions, 121 deletions

diff --git a/lib/spack/spack/cmd/checksum.py b/lib/spack/spack/cmd/checksum.py
index f88417dd2f..37c06ac7bb 100644
--- a/lib/spack/spack/cmd/checksum.py
+++ b/lib/spack/spack/cmd/checksum.py
@@ -16,7 +16,7 @@ import spack.stage
 import spack.util.crypto
 from spack.package_base import preferred_version
 from spack.util.naming import valid_fully_qualified_module_name
-from spack.version import Version, ver
+from spack.version import VersionBase, ver
 
 description = "checksum available versions of a package"
 section = "packaging"
@@ -65,7 +65,7 @@ def checksum(parser, args):
         remote_versions = None
         for version in versions:
             version = ver(version)
-            if not isinstance(version, Version):
+            if not isinstance(version, VersionBase):
                 tty.die("Cannot generate checksums for version lists or "
                         "version ranges. Use unambiguous versions.")
             url = pkg.find_valid_url_for_version(version)
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 2de6552a82..0801db6146 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -46,7 +46,7 @@ import spack.variant
 from spack.dependency import Dependency, canonical_deptype, default_deptype
 from spack.fetch_strategy import from_kwargs
 from spack.resource import Resource
-from spack.version import Version, VersionChecksumError
+from spack.version import GitVersion, Version, VersionChecksumError, VersionLookupError
 
 __all__ = ['DirectiveError', 'DirectiveMeta', 'version', 'conflicts', 'depends_on',
            'extends', 'provides', 'patch', 'variant', 'resource']
@@ -330,7 +330,17 @@ def version(ver, checksum=None, **kwargs):
             kwargs['checksum'] = checksum
 
         # Store kwargs for the package to later with a fetch_strategy.
-        pkg.versions[Version(ver)] = kwargs
+        version = Version(ver)
+        if isinstance(version, GitVersion):
+            if not hasattr(pkg, 'git') and 'git' not in kwargs:
+                msg = "Spack version directives cannot include git hashes fetched from"
+                msg += " URLs. Error in package '%s'\n" % pkg.name
+                msg += "    version('%s', " % version.string
+                msg += ', '.join("%s='%s'" % (argname, value)
+                                 for argname, value in kwargs.items())
+                msg += ")"
+                raise VersionLookupError(msg)
+        pkg.versions[version] = kwargs
     return _execute_version
 
 
diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 9f907c5da6..ed2e5eeb42 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -1575,16 +1575,30 @@ def for_package_version(pkg, version):
 
     check_pkg_attributes(pkg)
 
-    if not isinstance(version, spack.version.Version):
+    if not isinstance(version, spack.version.VersionBase):
         version = spack.version.Version(version)
 
     # if it's a commit, we must use a GitFetchStrategy
-    if version.is_commit and hasattr(pkg, "git"):
+    if isinstance(version, spack.version.GitVersion):
+        if not hasattr(pkg, "git"):
+            raise FetchError(
+                "Cannot fetch git version for %s. Package has no 'git' attribute" %
+                pkg.name
+            )
         # Populate the version with comparisons to other commits
-        version.generate_commit_lookup(pkg.name)
+        version.generate_git_lookup(pkg.name)
+
+        # For GitVersion, we have no way to determine whether a ref is a branch or tag
+        # Fortunately, we handle branches and tags identically, except tags are
+        # handled slightly more conservatively for older versions of git.
+        # We call all non-commit refs tags in this context, at the cost of a slight
+        # performance hit for branches on older versions of git.
+        # Branches cannot be cached, so we tell the fetcher not to cache tags/branches
+        ref_type = 'commit' if version.is_commit else 'tag'
         kwargs = {
             'git': pkg.git,
-            'commit': str(version)
+            ref_type: version.ref,
+            'no_cache': True,
         }
         kwargs['submodules'] = getattr(pkg, 'submodules', False)
         fetcher = GitFetchStrategy(**kwargs)
diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py
index 9153310bd2..b0c272ed30 100644
--- a/lib/spack/spack/package_base.py
+++ b/lib/spack/spack/package_base.py
@@ -62,7 +62,7 @@ from spack.stage import ResourceStage, Stage, StageComposite, stage_prefix
 from spack.util.executable import ProcessError, which
 from spack.util.package_hash import package_hash
 from spack.util.prefix import Prefix
-from spack.version import Version
+from spack.version import GitVersion, Version, VersionBase
 
 if sys.version_info[0] >= 3:
     FLAG_HANDLER_RETURN_TYPE = Tuple[
@@ -1041,7 +1041,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
         return self._implement_all_urls_for_version(version, uf)
 
     def _implement_all_urls_for_version(self, version, custom_url_for_version=None):
-        if not isinstance(version, Version):
+        if not isinstance(version, VersionBase):
             version = Version(version)
 
         urls = []
@@ -1505,7 +1505,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
         checksum = spack.config.get('config:checksum')
         fetch = self.stage.managed_by_spack
         if checksum and fetch and (self.version not in self.versions) \
-                and (not self.version.is_commit):
+                and (not isinstance(self.version, GitVersion)):
             tty.warn("There is no checksum on file to fetch %s safely." %
                      self.spec.cformat('{name}{@version}'))
 
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 38c03008e2..5a466ab78a 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1429,11 +1429,16 @@ class SpackSolverSetup(object):
                     continue
 
                 known_versions = self.possible_versions[dep.name]
-                if (not dep.version.is_commit and
+                if (not isinstance(dep.version, spack.version.GitVersion) and
                     any(v.satisfies(dep.version) for v in known_versions)):
                     # some version we know about satisfies this constraint, so we
                     # should use that one. e.g, if the user asks for qt@5 and we
-                    # know about qt@5.5.
+                    # know about qt@5.5. This ensures we don't add under-specified
+                    # versions to the solver
+                    #
+                    # For git versions, we know the version is already fully specified
+                    # so we don't have to worry about whether it's an under-specified
+                    # version
                     continue
 
                 # if there is a concrete version on the CLI *that we know nothing
@@ -1678,7 +1683,7 @@ class SpackSolverSetup(object):
 
         # extract all the real versions mentioned in version ranges
         def versions_for(v):
-            if isinstance(v, spack.version.Version):
+            if isinstance(v, spack.version.VersionBase):
                 return [v]
             elif isinstance(v, spack.version.VersionRange):
                 result = [v.start] if v.start else []
@@ -2187,8 +2192,8 @@ class SpecBuilder(object):
         # concretization process)
         for root in self._specs.values():
             for spec in root.traverse():
-                if spec.version.is_commit:
-                    spec.version.generate_commit_lookup(spec.fullname)
+                if isinstance(spec.version, spack.version.GitVersion):
+                    spec.version.generate_git_lookup(spec.fullname)
 
         return self._specs
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 602dcb09e8..7ebb34894d 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -5154,9 +5154,9 @@ class SpecParser(spack.parse.Parser):
             # Note: VersionRange(x, x) is currently concrete, hence isinstance(...).
             if (
                 spec.name and spec.versions.concrete and
-                isinstance(spec.version, vn.Version) and spec.version.is_commit
+                isinstance(spec.version, vn.GitVersion)
             ):
-                spec.version.generate_commit_lookup(spec.fullname)
+                spec.version.generate_git_lookup(spec.fullname)
 
         return specs
 
diff --git a/lib/spack/spack/test/versions.py b/lib/spack/spack/test/versions.py
index c8e7e45196..bcfebcd264 100644
--- a/lib/spack/spack/test/versions.py
+++ b/lib/spack/spack/test/versions.py
@@ -17,7 +17,14 @@ from llnl.util.filesystem import working_dir
 import spack.package_base
 import spack.spec
 from spack.util.executable import which
-from spack.version import Version, VersionList, VersionRange, ver
+from spack.version import (
+    GitVersion,
+    Version,
+    VersionBase,
+    VersionList,
+    VersionRange,
+    ver,
+)
 
 
 def assert_ver_lt(a, b):
@@ -520,7 +527,7 @@ def test_repr_and_str():
 
     def check_repr_and_str(vrs):
         a = Version(vrs)
-        assert repr(a) == "Version('" + vrs + "')"
+        assert repr(a) == "VersionBase('" + vrs + "')"
         b = eval(repr(a))
         assert a == b
         assert str(a) == vrs
@@ -544,19 +551,19 @@ def test_get_item():
     assert isinstance(a[1], int)
     # Test slicing
     b = a[0:2]
-    assert isinstance(b, Version)
+    assert isinstance(b, VersionBase)
     assert b == Version('0.1')
-    assert repr(b) == "Version('0.1')"
+    assert repr(b) == "VersionBase('0.1')"
     assert str(b) == '0.1'
     b = a[0:3]
-    assert isinstance(b, Version)
+    assert isinstance(b, VersionBase)
     assert b == Version('0.1_2')
-    assert repr(b) == "Version('0.1_2')"
+    assert repr(b) == "VersionBase('0.1_2')"
     assert str(b) == '0.1_2'
     b = a[1:]
-    assert isinstance(b, Version)
+    assert isinstance(b, VersionBase)
     assert b == Version('1_2-3')
-    assert repr(b) == "Version('1_2-3')"
+    assert repr(b) == "VersionBase('1_2-3')"
     assert str(b) == '1_2-3'
     # Raise TypeError on tuples
     with pytest.raises(TypeError):
@@ -597,7 +604,7 @@ def test_versions_from_git(mock_git_version_info, monkeypatch, mock_packages):
         spec = spack.spec.Spec('git-test-commit@%s' % commit)
         version = spec.version
         comparator = [str(v) if not isinstance(v, int) else v
-                      for v in version._cmp(version.commit_lookup)]
+                      for v in version._cmp(version.ref_lookup)]
 
         with working_dir(repo_path):
             which('git')('checkout', commit)
@@ -637,6 +644,43 @@ def test_git_hash_comparisons(
     assert spec4.satisfies('@1.0:1.2')
 
 
+@pytest.mark.skipif(sys.platform == 'win32',
+                    reason="Not supported on Windows (yet)")
+def test_git_ref_comparisons(
+        mock_git_version_info, install_mockery, mock_packages, monkeypatch):
+    """Check that hashes compare properly to versions
+    """
+    repo_path, filename, commits = mock_git_version_info
+    monkeypatch.setattr(spack.package_base.PackageBase,
+                        'git', 'file://%s' % repo_path,
+                        raising=False)
+
+    # Spec based on tag v1.0
+    spec_tag = spack.spec.Spec('git-test-commit@git.v1.0')
+    spec_tag.concretize()
+    assert spec_tag.satisfies('@1.0')
+    assert not spec_tag.satisfies('@1.1:')
+    assert str(spec_tag.version) == 'git.v1.0'
+
+    # Spec based on branch 1.x
+    spec_branch = spack.spec.Spec('git-test-commit@git.1.x')
+    spec_branch.concretize()
+    assert spec_branch.satisfies('@1.2')
+    assert spec_branch.satisfies('@1.1:1.3')
+    assert str(spec_branch.version) == 'git.1.x'
+
+
+@pytest.mark.parametrize('string,git', [
+    ('1.2.9', False),
+    ('gitmain', False),
+    ('git.foo', True),
+    ('git.abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd', True),
+    ('abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd', True),
+])
+def test_version_git_vs_base(string, git):
+    assert isinstance(Version(string), GitVersion) == git
+
+
 def test_version_range_nonempty():
     assert Version('1.2.9') in VersionRange('1.2.0', '1.2')
     assert Version('1.1.1') in ver('1.0:1')
diff --git a/lib/spack/spack/version.py b/lib/spack/spack/version.py
index 5fa7793da2..4267d307b1 100644
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@@ -67,11 +67,11 @@ iv_min_len = min(len(s) for s in infinity_versions)
 def coerce_versions(a, b):
     """
     Convert both a and b to the 'greatest' type between them, in this order:
-           Version < VersionRange < VersionList
+           VersionBase < GitVersion < VersionRange < VersionList
     This is used to simplify comparison operations below so that we're always
     comparing things that are of the same type.
     """
-    order = (Version, VersionRange, VersionList)
+    order = (VersionBase, GitVersion, VersionRange, VersionList)
     ta, tb = type(a), type(b)
 
     def check_type(t):
@@ -83,12 +83,16 @@ def coerce_versions(a, b):
     if ta == tb:
         return (a, b)
     elif order.index(ta) > order.index(tb):
-        if ta == VersionRange:
+        if ta == GitVersion:
+            return (a, GitVersion(b))
+        elif ta == VersionRange:
             return (a, VersionRange(b, b))
         else:
             return (a, VersionList([b]))
     else:
-        if tb == VersionRange:
+        if tb == GitVersion:
+            return (GitVersion(a), b)
+        elif tb == VersionRange:
             return (VersionRange(a, a), b)
         else:
             return (VersionList([a]), b)
@@ -165,15 +169,29 @@ class VersionStrComponent(object):
         return not self.__lt__(other)
 
 
-class Version(object):
+def is_git_version(string):
+    if string.startswith('git.'):
+        return True
+    elif len(string) == 40 and COMMIT_VERSION.match(string):
+        return True
+    return False
+
+
+def Version(string):  # capitalized for backwards compatibility
+    if not isinstance(string, str):
+        string = str(string)  # to handle VersionBase and GitVersion types
+
+    if is_git_version(string):
+        return GitVersion(string)
+    return VersionBase(string)
+
+
+class VersionBase(object):
     """Class to represent versions"""
     __slots__ = [
         "version",
         "separators",
         "string",
-        "_commit_lookup",
-        "is_commit",
-        "commit_version",
     ]
 
     def __init__(self, string):
@@ -188,36 +206,12 @@ class Version(object):
         if string and not VALID_VERSION.match(string):
             raise ValueError("Bad characters in version string: %s" % string)
 
-        # An object that can lookup git commits to compare them to versions
-        self._commit_lookup = None
-        self.commit_version = None
         segments = SEGMENT_REGEX.findall(string)
         self.version = tuple(
             int(m[0]) if m[0] else VersionStrComponent(m[1]) for m in segments
         )
         self.separators = tuple(m[2] for m in segments)
 
-        self.is_commit = len(self.string) == 40 and COMMIT_VERSION.match(self.string)
-
-    def _cmp(self, other_lookups=None):
-        commit_lookup = self.commit_lookup or other_lookups
-
-        if self.is_commit and commit_lookup:
-            if self.commit_version is not None:
-                return self.commit_version
-            commit_info = commit_lookup.get(self.string)
-            if commit_info:
-                prev_version, distance = commit_info
-
-                # Extend previous version by empty component and distance
-                # If commit is exactly a known version, no distance suffix
-                prev_tuple = Version(prev_version).version if prev_version else ()
-                dist_suffix = (VersionStrComponent(''), distance) if distance else ()
-                self.commit_version = prev_tuple + dist_suffix
-                return self.commit_version
-
-        return self.version
-
     @property
     def dotted(self):
         """The dotted representation of the version.
@@ -230,7 +224,7 @@ class Version(object):
         Returns:
             Version: The version with separator characters replaced by dots
         """
-        return Version(self.string.replace('-', '.').replace('_', '.'))
+        return type(self)(self.string.replace('-', '.').replace('_', '.'))
 
     @property
     def underscored(self):
@@ -245,7 +239,7 @@ class Version(object):
             Version: The version with separator characters replaced by
                 underscores
         """
-        return Version(self.string.replace('.', '_').replace('-', '_'))
+        return type(self)(self.string.replace('.', '_').replace('-', '_'))
 
     @property
     def dashed(self):
@@ -259,7 +253,7 @@ class Version(object):
         Returns:
             Version: The version with separator characters replaced by dashes
         """
-        return Version(self.string.replace('.', '-').replace('_', '-'))
+        return type(self)(self.string.replace('.', '-').replace('_', '-'))
 
     @property
     def joined(self):
@@ -273,7 +267,7 @@ class Version(object):
         Returns:
             Version: The version with separator characters removed
         """
-        return Version(
+        return type(self)(
             self.string.replace('.', '').replace('-', '').replace('_', ''))
 
     def up_to(self, index):
@@ -323,13 +317,9 @@ class Version(object):
         gcc@4.7 so that when a user asks to build with gcc@4.7, we can find
         a suitable compiler.
         """
-        self_cmp = self._cmp(other.commit_lookup)
-        other_cmp = other._cmp(self.commit_lookup)
-
-        # Do the final comparison
-        nself = len(self_cmp)
-        nother = len(other_cmp)
-        return nother <= nself and self_cmp[:nother] == other_cmp
+        nself = len(self.version)
+        nother = len(other.version)
+        return nother <= nself and self.version[:nother] == other.version
 
     def __iter__(self):
         return iter(self.version)
@@ -356,19 +346,19 @@ class Version(object):
                 string_arg = ''.join(string_arg)
                 return cls(string_arg)
             else:
-                return Version('')
+                return VersionBase('')
 
         message = '{cls.__name__} indices must be integers'
         raise TypeError(message.format(cls=cls))
 
     def __repr__(self):
-        return 'Version(' + repr(self.string) + ')'
+        return 'VersionBase(' + repr(self.string) + ')'
 
     def __str__(self):
         return self.string
 
     def __format__(self, format_spec):
-        return self.string.format(format_spec)
+        return str(self).format(format_spec)
 
     @property
     def concrete(self):
@@ -384,22 +374,16 @@ class Version(object):
         if other is None:
             return False
 
-        # If either is a commit and we haven't indexed yet, can't compare
-        if (other.is_commit or self.is_commit) and not (self.commit_lookup or
-                                                        other.commit_lookup):
-            return False
-
         # Use tuple comparison assisted by VersionStrComponent for performance
-        return self._cmp(other.commit_lookup) < other._cmp(self.commit_lookup)
+        return self.version < other.version
 
     @coerced
     def __eq__(self, other):
-
         # Cut out early if we don't have a version
-        if other is None or type(other) != Version:
+        if other is None or type(other) != VersionBase:
             return False
 
-        return self._cmp(other.commit_lookup) == other._cmp(self.commit_lookup)
+        return self.version == other.version
 
     @coerced
     def __ne__(self, other):
@@ -425,24 +409,23 @@ class Version(object):
         if other is None:
             return False
 
-        self_cmp = self._cmp(other.commit_lookup)
-        return other._cmp(self.commit_lookup)[:len(self_cmp)] == self_cmp
+        return other.version[:len(self.version)] == self.version
 
+    @coerced
     def is_predecessor(self, other):
         """True if the other version is the immediate predecessor of this one.
-           That is, NO non-commit versions v exist such that:
+           That is, NO non-git versions v exist such that:
            (self < v < other and v not in self).
         """
-        self_cmp = self._cmp(self.commit_lookup)
-        other_cmp = other._cmp(other.commit_lookup)
-
-        if self_cmp[:-1] != other_cmp[:-1]:
+        if self.version[:-1] != other.version[:-1]:
             return False
 
-        sl = self_cmp[-1]
-        ol = other_cmp[-1]
+        sl = self.version[-1]
+        ol = other.version[-1]
+        # TODO: extend this to consecutive letters, z/0, and infinity versions
         return type(sl) == int and type(ol) == int and (ol - sl == 1)
 
+    @coerced
     def is_successor(self, other):
         return other.is_predecessor(self)
 
@@ -468,13 +451,135 @@ class Version(object):
         else:
             return VersionList()
 
+
+class GitVersion(VersionBase):
+    """Class to represent versions interpreted from git refs.
+
+    Non-git versions may be coerced to GitVersion for comparison, but no Spec will ever
+    have a GitVersion that is not actually referencing a version from git."""
+    def __init__(self, string):
+        if not isinstance(string, str):
+            string = str(string)  # In case we got a VersionBase or GitVersion object
+
+        git_prefix = string.startswith('git.')
+        self.ref = string[4:] if git_prefix else string
+
+        self.is_commit = len(self.ref) == 40 and COMMIT_VERSION.match(self.ref)
+        self.is_ref = git_prefix  # is_ref False only for comparing to VersionBase
+        self.is_ref |= bool(self.is_commit)
+
+        # ensure git.<hash> and <hash> are treated the same by dropping 'git.'
+        canonical_string = self.ref if self.is_commit else string
+        super(GitVersion, self).__init__(canonical_string)
+
+        # An object that can lookup git refs to compare them to versions
+        self._ref_lookup = None
+        self.ref_version = None
+
+    def _cmp(self, other_lookups=None):
+        # No need to rely on git comparisons for develop-like refs
+        if len(self.version) == 2 and self.isdevelop():
+            return self.version
+
+        # If we've already looked this version up, return cached value
+        if self.ref_version is not None:
+            return self.ref_version
+
+        ref_lookup = self.ref_lookup or other_lookups
+
+        if self.is_ref and ref_lookup:
+            ref_info = ref_lookup.get(self.ref)
+            if ref_info:
+                prev_version, distance = ref_info
+
+                # Extend previous version by empty component and distance
+                # If commit is exactly a known version, no distance suffix
+                prev_tuple = VersionBase(prev_version).version if prev_version else ()
+                dist_suffix = (VersionStrComponent(''), distance) if distance else ()
+                self.ref_version = prev_tuple + dist_suffix
+                return self.ref_version
+
+        return self.version
+
+    @coerced
+    def satisfies(self, other):
+        """A Version 'satisfies' another if it is at least as specific and has
+        a common prefix.  e.g., we want gcc@4.7.3 to satisfy a request for
+        gcc@4.7 so that when a user asks to build with gcc@4.7, we can find
+        a suitable compiler.
+        """
+        self_cmp = self._cmp(other.ref_lookup)
+        other_cmp = other._cmp(self.ref_lookup)
+
+        # Do the final comparison
+        nself = len(self_cmp)
+        nother = len(other_cmp)
+        return nother <= nself and self_cmp[:nother] == other_cmp
+
+    def __repr__(self):
+        return 'GitVersion(' + repr(self.string) + ')'
+
+    @coerced
+    def __lt__(self, other):
+        """Version comparison is designed for consistency with the way RPM
+           does things.  If you need more complicated versions in installed
+           packages, you should override your package's version string to
+           express it more sensibly.
+        """
+        if other is None:
+            return False
+
+        # If we haven't indexed yet, can't compare
+        # If we called this, we know at least one is a git ref
+        if not (self.ref_lookup or other.ref_lookup):
+            return False
+
+        # Use tuple comparison assisted by VersionStrComponent for performance
+        return self._cmp(other.ref_lookup) < other._cmp(self.ref_lookup)
+
+    @coerced
+    def __eq__(self, other):
+        # Cut out early if we don't have a git version
+        if other is None or type(other) != GitVersion:
+            return False
+
+        return self._cmp(other.ref_lookup) == other._cmp(self.ref_lookup)
+
+    def __hash__(self):
+        return hash(str(self))
+
+    @coerced
+    def __contains__(self, other):
+        if other is None:
+            return False
+
+        self_cmp = self._cmp(other.ref_lookup)
+        return other._cmp(self.ref_lookup)[:len(self_cmp)] == self_cmp
+
+    @coerced
+    def is_predecessor(self, other):
+        """True if the other version is the immediate predecessor of this one.
+           That is, NO non-commit versions v exist such that:
+           (self < v < other and v not in self).
+        """
+        self_cmp = self._cmp(self.ref_lookup)
+        other_cmp = other._cmp(other.ref_lookup)
+
+        if self_cmp[:-1] != other_cmp[:-1]:
+            return False
+
+        sl = self_cmp[-1]
+        ol = other_cmp[-1]
+        return type(sl) == int and type(ol) == int and (ol - sl == 1)
+
     @property
-    def commit_lookup(self):
-        if self._commit_lookup:
-            self._commit_lookup.get(self.string)
-            return self._commit_lookup
+    def ref_lookup(self):
+        if self._ref_lookup:
+            # Get operation ensures dict is populated
+            self._ref_lookup.get(self.ref)
+            return self._ref_lookup
 
-    def generate_commit_lookup(self, pkg_name):
+    def generate_git_lookup(self, pkg_name):
         """
         Use the git fetcher to look up a version for a commit.
 
@@ -492,11 +597,11 @@ class Version(object):
         """
 
         # Sanity check we have a commit
-        if not self.is_commit:
-            tty.die("%s is not a commit." % self)
+        if not self.is_ref:
+            tty.die("%s is not a git version." % self)
 
         # Generate a commit looker-upper
-        self._commit_lookup = CommitLookup(pkg_name)
+        self._ref_lookup = CommitLookup(pkg_name)
 
 
 class VersionRange(object):
@@ -715,7 +820,7 @@ class VersionList(object):
                     self.add(ver(v))
 
     def add(self, version):
-        if type(version) in (Version, VersionRange):
+        if type(version) in (VersionBase, GitVersion, VersionRange):
             # This normalizes single-value version ranges.
             if version.concrete:
                 version = version.concrete
@@ -968,7 +1073,7 @@ def ver(obj):
         return _string_to_version(obj)
     elif isinstance(obj, (int, float)):
         return _string_to_version(str(obj))
-    elif type(obj) in (Version, VersionRange, VersionList):
+    elif type(obj) in (VersionBase, GitVersion, VersionRange, VersionList):
         return obj
     else:
         raise TypeError("ver() can't convert %s to version!" % type(obj))
@@ -990,8 +1095,8 @@ class CommitLookup(object):
     """An object for cached lookups of git commits
 
     CommitLookup objects delegate to the misc_cache for locking.
-    CommitLookup objects may be attached to a Version object for which
-    Version.is_commit returns True to allow for comparisons between git commits
+    CommitLookup objects may be attached to a GitVersion object for which
+    Version.is_ref returns True to allow for comparisons between git refs
     and versions as represented by tags in the git repository.
     """
     def __init__(self, pkg_name):
@@ -1074,17 +1179,17 @@ class CommitLookup(object):
             with spack.caches.misc_cache.read_transaction(self.cache_key) as cache_file:
                 self.data = sjson.load(cache_file)
 
-    def get(self, commit):
+    def get(self, ref):
         if not self.data:
             self.load_data()
 
-        if commit not in self.data:
-            self.data[commit] = self.lookup_commit(commit)
+        if ref not in self.data:
+            self.data[ref] = self.lookup_ref(ref)
             self.save()
 
-        return self.data[commit]
+        return self.data[ref]
 
-    def lookup_commit(self, commit):
+    def lookup_ref(self, ref):
         """Lookup the previous version and distance for a given commit.
 
         We use git to compare the known versions from package to the git tags,
@@ -1111,13 +1216,19 @@ class CommitLookup(object):
             # remote instance, simply adding '-f' may not be sufficient
             # (if commits are deleted on the remote, this command alone
             # won't properly update the local rev-list)
-            self.fetcher.git("fetch", '--tags')
+            self.fetcher.git("fetch", '--tags', output=os.devnull, error=os.devnull)
 
-            # Ensure commit is an object known to git
-            # Note the brackets are literals, the commit replaces the format string
-            # This will raise a ProcessError if the commit does not exist
-            # We may later design a custom error to re-raise
-            self.fetcher.git('cat-file', '-e', '%s^{commit}' % commit)
+            # Ensure ref is a commit object known to git
+            # Note the brackets are literals, the ref replaces the format string
+            try:
+                self.fetcher.git(
+                    'cat-file', '-e', '%s^{commit}' % ref,
+                    output=os.devnull, error=os.devnull
+                )
+            except spack.util.executable.ProcessError:
+                raise VersionLookupError(
+                    "%s is not a valid git ref for %s" % (ref, self.pkg_name)
+                )
 
             # List tags (refs) by date, so last reference of a tag is newest
             tag_info = self.fetcher.git(
@@ -1148,11 +1259,11 @@ class CommitLookup(object):
             ancestor_commits = []
             for tag_commit in commit_to_version:
                 self.fetcher.git(
-                    'merge-base', '--is-ancestor', tag_commit, commit,
+                    'merge-base', '--is-ancestor', tag_commit, ref,
                     ignore_errors=[1])
                 if self.fetcher.git.returncode == 0:
                     distance = self.fetcher.git(
-                        'rev-list', '%s..%s' % (tag_commit, commit), '--count',
+                        'rev-list', '%s..%s' % (tag_commit, ref), '--count',
                         output=str, error=str).strip()
                     ancestor_commits.append((tag_commit, int(distance)))
 
@@ -1164,14 +1275,14 @@ class CommitLookup(object):
             else:
                 # Get list of all commits, this is in reverse order
                 # We use this to get the first commit below
-                commit_info = self.fetcher.git("log", "--all", "--pretty=format:%H",
-                                               output=str)
-                commits = [c for c in commit_info.split('\n') if c]
+                ref_info = self.fetcher.git("log", "--all", "--pretty=format:%H",
+                                            output=str)
+                commits = [c for c in ref_info.split('\n') if c]
 
                 # No previous version and distance from first commit
                 prev_version = None
                 distance = int(self.fetcher.git(
-                    'rev-list', '%s..%s' % (commits[-1], commit), '--count',
+                    'rev-list', '%s..%s' % (commits[-1], ref), '--count',
                     output=str, error=str
                 ).strip())
 
diff --git a/var/spack/repos/builtin/packages/libcatalyst/package.py b/var/spack/repos/builtin/packages/libcatalyst/package.py
index 6c68ea5d3d..e81a5157cc 100644
--- a/var/spack/repos/builtin/packages/libcatalyst/package.py
+++ b/var/spack/repos/builtin/packages/libcatalyst/package.py
@@ -16,7 +16,7 @@ class Libcatalyst(CMakePackage):
     maintainers = ['mathstuf']
 
     # master as of 2021-05-12
-    version('8456ccd6015142b5a7705f79471361d4f5644fa7', sha256='5a01f12b271d9d9e9b89f31d45a5f4b8426904483639d38754893adfd3547bab')
+    version('2021-05-12', sha256='5a01f12b271d9d9e9b89f31d45a5f4b8426904483639d38754893adfd3547bab')
 
     variant('mpi', default=False, description='Enable MPI support')
     variant('python3', default=False, description='Enable Python3 support')