Replace Version.get_vcs_slug() and Version.remote_slug with Version.commit_name

readthedocs/builds/constants.py

@@ -25,10 +25,14 @@
     ('dash', _('Dash')),
 )
 
+BRANCH = 'branch'
+TAG = 'tag'
+UNKNOWN = 'unknown'
+
 VERSION_TYPES = (
-    ('branch', _('Branch')),
-    ('tag', _('Tag')),
-    ('unknown', _('Unknown')),
+    (BRANCH, _('Branch')),
+    (TAG, _('Tag')),
+    (UNKNOWN, _('Unknown')),
 )
 
 LATEST = 'latest'

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)
+                        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

@@ -10,6 +10,7 @@
 from django.conf import settings
 
 from readthedocs.builds import utils as version_utils
+from readthedocs.builds.constants import BRANCH
 from readthedocs.projects.utils import safe_write
 from readthedocs.projects.exceptions import ProjectImportError
 from readthedocs.restapi.client import api
@@ -72,16 +73,16 @@ 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)
-        github_version_is_editable = (self.version.type == 'branch')
+        github_version_is_editable = (self.version.type == BRANCH)
         display_github = github_user is not None
 
         bitbucket_user, bitbucket_repo = version_utils.get_bitbucket_username_repo(
             url=self.project.repo)
-        bitbucket_version_is_editable = (self.version.type == 'branch')
+        bitbucket_version_is_editable = (self.version.type == BRANCH)
         display_bitbucket = bitbucket_user is not None
 
         rtd_ctx = {

readthedocs/privacy/backend.py

@@ -3,6 +3,8 @@
 
 from guardian.shortcuts import get_objects_for_user
 
+from readthedocs.builds.constants import BRANCH
+from readthedocs.builds.constants import TAG
 from readthedocs.builds.constants import LATEST
 from readthedocs.builds.constants import LATEST_VERBOSE_NAME
 from readthedocs.builds.constants import STABLE
@@ -152,7 +154,7 @@ def create_stable(self, **kwargs):
             'machine': True,
             'active': True,
             'identifier': STABLE,
-            'type': 'tag',
+            'type': TAG,
         }
         defaults.update(kwargs)
         return self.create(**defaults)
@@ -164,7 +166,7 @@ def create_latest(self, **kwargs):
             'machine': True,
             'active': True,
             'identifier': LATEST,
-            'type': 'branch',
+            'type': BRANCH,
         }
         defaults.update(kwargs)
         return self.create(**defaults)

readthedocs/projects/forms.py

@@ -12,6 +12,7 @@
 
 from guardian.shortcuts import assign
 
+from readthedocs.builds.constants import TAG
 from readthedocs.core.utils import trigger_build
 from readthedocs.redirects.models import Redirect
 from readthedocs.projects import constants
@@ -272,7 +273,7 @@ def build_versions_form(project):
     for version in versions_qs:
         field_name = 'version-%s' % version.slug
         privacy_name = 'privacy-%s' % version.slug
-        if version.type == 'tag':
+        if version.type == TAG:
             label = "%s (%s)" % (version.verbose_name, version.identifier[:8])
         else:
             label = version.verbose_name

readthedocs/projects/models.py

@@ -312,7 +312,7 @@ def save(self, *args, **kwargs):
             if not self.versions.filter(slug=LATEST).exists():
                 self.versions.create_latest(identifier=branch)
             # if not self.versions.filter(slug=STABLE).exists():
-            #     self.versions.create_stable(type='branch', identifier=branch)
+            #     self.versions.create_stable(type=BRANCH, identifier=branch)
         except Exception:
             log.error('Error creating default branches', exc_info=True)
 

readthedocs/restapi/views/footer_views.py

@@ -8,6 +8,7 @@
 from rest_framework.response import Response
 
 from readthedocs.builds.constants import LATEST
+from readthedocs.builds.constants import TAG
 from readthedocs.builds.models import Version
 from readthedocs.donate.models import SupporterPromo
 from readthedocs.projects.models import Project
@@ -74,7 +75,7 @@ def footer_html(request):
     else:
         path = ""
 
-    if version.type == 'tag' and version.project.has_pdf(version.slug):
+    if version.type == TAG and version.project.has_pdf(version.slug):
         print_url = (
             'https://keminglabs.com/print-the-docs/quote?project={project}&version={version}'
             .format(

readthedocs/restapi/views/model_views.py

@@ -7,6 +7,8 @@
 from rest_framework.renderers import JSONPRenderer, JSONRenderer, BrowsableAPIRenderer
 from rest_framework.response import Response
 
+from readthedocs.builds.constants import BRANCH
+from readthedocs.builds.constants import TAG
 from readthedocs.builds.filters import VersionFilter
 from readthedocs.builds.models import Build, BuildCommandResult, Version
 from readthedocs.core.utils import trigger_build
@@ -109,11 +111,11 @@ def sync_versions(self, request, **kwargs):
             added_versions = set()
             if 'tags' in data:
                 ret_set = api_utils.sync_versions(
-                    project=project, versions=data['tags'], type='tag')
+                    project=project, versions=data['tags'], type=TAG)
                 added_versions.update(ret_set)
             if 'branches' in data:
                 ret_set = api_utils.sync_versions(
-                    project=project, versions=data['branches'], type='branch')
+                    project=project, versions=data['branches'], type=BRANCH)
                 added_versions.update(ret_set)
             deleted_versions = api_utils.delete_versions(project, data)
         except Exception, e:

readthedocs/rtd_tests/tests/test_supported.py

@@ -2,6 +2,7 @@
 
 from django.test import TestCase
 
+from readthedocs.builds.constants import TAG
 from readthedocs.builds.models import Version
 from readthedocs.projects.models import Project
 
@@ -13,11 +14,11 @@ def setUp(self):
         self.pip = Project.objects.create(name='Pip', slug='pip')
         Version.objects.create(project=self.pip, identifier='0.1',
                                verbose_name='0.1', slug='0.1',
-                               type='tag',
+                               type=TAG,
                                active=True)
         Version.objects.create(project=self.pip, identifier='0.2',
                                verbose_name='0.2', slug='0.2',
-                               type='tag',
+                               type=TAG,
                                active=True)
 
     def test_supported_versions(self):
@@ -55,6 +56,6 @@ def test_adding_version_updates_supported(self):
         self.assertEqual(self.pip.versions.get(slug='0.1').supported, False)
         self.assertEqual(self.pip.versions.get(slug='0.2').supported, True)
         Version.objects.create(project=self.pip, identifier='0.1.1',
-                               verbose_name='0.1.1', type='tag', active=True)
+                               verbose_name='0.1.1', type=TAG, active=True)
         # This gets set to False on creation.
         self.assertEqual(self.pip.versions.get(slug='0.1.1').supported, False)

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')