Replace Version.get_vcs_slug() and Version.remote_slug with Version.c…

…ommit_name

The two places were solving the same problem but with different results. That
is undesired and resulted in invalid behaviour. The commit_name property
unifies those now and has tests to make sure it works the way intended.

The problem with this on-first-sight-easy problem is that we currently do
store different things in "identifier", "slug", "verbose_name" fields
depending if its a branch or tag, if it's a special version (like
stable, latest) etc. The cleanest solution would be to refactor the Version
model to expose those information more easily and store branch_name, tag_name,
explicitly.

readthedocs/builds/models.py

@@ -21,7 +21,7 @@
 
 from .constants import (BUILD_STATE, BUILD_TYPES, VERSION_TYPES,
                         LATEST, NON_REPOSITORY_VERSIONS, STABLE,
-                        BUILD_STATE_FINISHED, BRANCH)
+                        BUILD_STATE_FINISHED, BRANCH, TAG)
 from .version_slug import VersionSlugField
 
 
@@ -99,12 +99,46 @@ def __unicode__(self):
 
     @property
     def commit_name(self):
-        """Return the branch name, the tag name or the revision identifier."""
-        if self.type == BRANCH:
-            return self.identifier
-        if self.verbose_name in NON_REPOSITORY_VERSIONS:
+        """
+        Return the branch name, the tag name or the revision identifier.
+
+        The result could be used as ref in a git repo, e.g. for linking to
+        GitHub or Bitbucket.
+        """
+
+        # LATEST is special as it is usually a branch but does not contain the
+        # name in verbose_name.
+        if self.slug == LATEST:
+            if self.project.default_branch:
+                return self.project.default_branch
+            else:
+                return self.project.vcs_repo().fallback_branch
+
+        if self.slug == STABLE:
+            if self.type == BRANCH:
+                # Special case, as we do not store the original branch name
+                # that the stable version works on. We can only interpolate the
+                # name from the commit identifier, but it's hacky.
+                # TODO: Refactor ``Version`` to store more actual info about
+                # the underlying commits.
+                if self.identifier.startswith('origin/'):
+                    return self.identifier[len('origin/'):]
             return self.identifier
-        return self.verbose_name
+
+        # By now we must have handled all special versions.
+        assert self.slug not in NON_REPOSITORY_VERSIONS
+
+        if self.type in (BRANCH, TAG):
+            # If this version is a branch or a tag, the verbose_name will
+            # contain the actual name. We cannot use identifier as this might
+            # include the "origin/..." part in the case of a branch. A tag
+            # would contain the hash in identifier, which is not as pretty as
+            # the actual tag name.
+            return self.verbose_name
+
+        # If we came that far it's not a special version nor a branch or tag.
+        # Therefore just return the identifier to make a safe guess.
+        return self.identifier
 
     def get_absolute_url(self):
         if not self.built and not self.uploaded:
@@ -124,16 +158,6 @@ def save(self, *args, **kwargs):
         self.project.sync_supported_versions()
         return obj
 
-    @property
-    def remote_slug(self):
-        if self.slug == LATEST:
-            if self.project.default_branch:
-                return self.project.default_branch
-            else:
-                return self.project.vcs_repo().fallback_branch
-        else:
-            return self.slug
-
     @property
     def identifier_friendly(self):
         '''Return display friendly identifier'''
@@ -211,24 +235,6 @@ def clean_build_path(self):
         except OSError:
             log.error('Build path cleanup failed', exc_info=True)
 
-    def get_vcs_slug(self):
-        slug = None
-        if self.slug == LATEST:
-            if self.project.default_branch:
-                slug = self.project.default_branch
-            else:
-                slug = self.project.vcs_repo().fallback_branch
-        elif self.slug == STABLE:
-            return self.identifier
-        else:
-            slug = self.slug
-        # https://github.com/rtfd/readthedocs.org/issues/561
-        # version identifiers with / characters in branch name need to un-slugify
-        # the branch name for remote links to work
-        if slug.replace('-', '/') in self.identifier:
-            slug = slug.replace('-', '/')
-        return slug
-
     def get_github_url(self, docroot, filename, source_suffix='.rst', action='view'):
         repo_url = self.project.repo
         if 'github' not in repo_url:
@@ -259,7 +265,7 @@ def get_github_url(self, docroot, filename, source_suffix='.rst', action='view')
         return GITHUB_URL.format(
             user=user,
             repo=repo,
-            version=self.remote_slug,
+            version=self.commit_name,
             docroot=docroot,
             path=filename,
             source_suffix=source_suffix,
@@ -285,7 +291,7 @@ def get_bitbucket_url(self, docroot, filename, source_suffix='.rst'):
         return BITBUCKET_URL.format(
             user=user,
             repo=repo,
-            version=self.remote_slug,
+            version=self.commit_name,
             docroot=docroot,
             path=filename,
             source_suffix=source_suffix,

readthedocs/doc_builder/backends/sphinx.py

@@ -73,7 +73,7 @@ def append_conf(self, **kwargs):
             raise ProjectImportError('Conf file not found'), None, trace
         outfile.write("\n")
         conf_py_path = self.version.get_conf_py_path()
-        remote_version = self.version.get_vcs_slug()
+        remote_version = self.version.commit_name
 
         github_user, github_repo = version_utils.get_github_username_repo(
             url=self.project.repo)

readthedocs/rtd_tests/tests/test_version_commit_name.py

@@ -0,0 +1,50 @@
+from django.test import TestCase
+from django_dynamic_fixture import get
+from django_dynamic_fixture import new
+
+from readthedocs.builds.constants import BRANCH
+from readthedocs.builds.constants import LATEST
+from readthedocs.builds.constants import STABLE
+from readthedocs.builds.constants import TAG
+from readthedocs.builds.models import Version
+from readthedocs.projects.constants import REPO_TYPE_GIT
+from readthedocs.projects.constants import REPO_TYPE_HG
+from readthedocs.projects.models import Project
+
+
+class VersionCommitNameTests(TestCase):
+    def test_branch_name(self):
+        version = new(Version, identifier='release-2.5.x',
+                      slug='release-2.5.x', verbose_name='release-2.5.x',
+                      type=BRANCH)
+        self.assertEqual(version.commit_name, 'release-2.5.x')
+
+    def test_tag_name(self):
+        version = new(Version, identifier='10f1b29a2bd2', slug='release-2.5.0',
+                      verbose_name='release-2.5.0', type=TAG)
+        self.assertEqual(version.commit_name, 'release-2.5.0')
+
+    def test_branch_with_name_stable(self):
+        version = new(Version, identifier='origin/stable', slug=STABLE,
+                      verbose_name='stable', type=BRANCH)
+        self.assertEqual(version.commit_name, 'stable')
+
+    def test_stable_version_tag(self):
+        version = new(Version,
+                      identifier='3d92b728b7d7b842259ac2020c2fa389f13aff0d',
+                      slug=STABLE, verbose_name=STABLE, type=TAG)
+        self.assertEqual(version.commit_name,
+                         '3d92b728b7d7b842259ac2020c2fa389f13aff0d')
+
+    def test_hg_latest_branch(self):
+        hg_project = get(Project, repo_type=REPO_TYPE_HG)
+        version = new(Version, identifier='default', slug=LATEST,
+                      verbose_name=LATEST, type=BRANCH, project=hg_project)
+        self.assertEqual(version.commit_name, 'default')
+
+    def test_git_latest_branch(self):
+        git_project = get(Project, repo_type=REPO_TYPE_GIT)
+        version = new(Version, project=git_project,
+                      identifier='origin/master', slug=LATEST,
+                      verbose_name=LATEST, type=BRANCH)
+        self.assertEqual(version.commit_name, 'master')