Build: use actual git identifier for READTHEDOCS_GIT_IDENTIFIER

readthedocs/api/v2/serializers.py

@@ -187,6 +187,7 @@ class Meta(VersionSerializer.Meta):
             "build_data",
             "canonical_url",
             "machine",
+            "git_identifier",
         ]
 
 

readthedocs/builds/models.py

@@ -31,7 +31,6 @@
     PREDEFINED_MATCH_ARGS,
     PREDEFINED_MATCH_ARGS_VALUES,
     STABLE,
-    TAG,
     VERSION_TYPES,
 )
 from readthedocs.builds.managers import (
@@ -330,50 +329,38 @@ def config(self):
         return None
 
     @property
-    def commit_name(self):
+    def git_identifier(self):
         """
-        Return the branch name, the tag name or the revision identifier.
+        Return the branch or tag name of the version.
 
         The result could be used as ref in a git repo, e.g. for linking to
         GitHub, Bitbucket or GitLab.
+
+        - If the version is latest (machine created), we resolve to the default branch of the project.
+        - If the version is stable (machine created), we resolve to the branch that the stable version points to.
+        - If the version is external, we return the identifier, since we don't have access to the name of the branch.
         """
-        # LATEST is special as it is usually a branch but does not contain the
-        # name in verbose_name.
+        # Latest is special as it doesn't contain the actual name in verbose_name.
         if self.slug == LATEST and self.machine:
             return self.project.get_default_branch()
 
+        # Stable is special as it doesn't contain the actual name in verbose_name.
         if self.slug == STABLE and self.machine:
-            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
-
-        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
+            original_stable = self.project.get_original_stable_version()
+            # NOTE: we no longer save branch names with the "origin/" prefix,
+            # but we remove it for old versions.
+            if original_stable:
+                return original_stable.verbose_name.removeprefix("origin/")
+            return self.identifier.removeprefix("origin/")
 
         if self.type == EXTERNAL:
             # If this version is a EXTERNAL version, the identifier will
             # contain the actual commit hash. which we can use to
             # generate url for a given file name
             return self.identifier
 
-        # If we came that far it's not a special version
-        # nor a branch, tag or EXTERNAL version.
-        # Therefore just return the identifier to make a safe guess.
-        log.debug(
-            "TODO: Raise an exception here. Testing what cases it happens",
-        )
-        return self.identifier
+        # For all other cases, verbose_name contains the actual name of the branch/tag.
+        return self.verbose_name
 
     def get_absolute_url(self):
         """
@@ -561,13 +548,18 @@ class APIVersion(Version):
     """
 
     project = None
+    # This is a property in the original model, in order to
+    # be able to assign it a value in the constructor, we need to re-declare it
+    # as an attribute here.
+    git_identifier = None
 
     class Meta:
         proxy = True
 
     def __init__(self, *args, **kwargs):
         self.project = APIProject(**kwargs.pop("project", {}))
         self.canonical_url = kwargs.pop("canonical_url", None)
+        self.git_identifier = kwargs.pop("git_identifier", None)
         # These fields only exist on the API return, not on the model, so we'll
         # remove them to avoid throwing exceptions due to unexpected fields
         for key in ["resource_uri", "absolute_url", "downloads"]:

readthedocs/doc_builder/director.py

@@ -696,7 +696,7 @@ def get_rtd_env_vars(self):
             # TODO: we don't have access to the database from the builder.
             # We need to find a way to expose HTML_URL here as well.
             # "READTHEDOCS_GIT_HTML_URL": self.data.project.remote_repository.html_url,
-            "READTHEDOCS_GIT_IDENTIFIER": self.data.version.identifier,
+            "READTHEDOCS_GIT_IDENTIFIER": self.data.version.git_identifier,
             "READTHEDOCS_GIT_COMMIT_HASH": self.data.build["commit"],
             "READTHEDOCS_PRODUCTION_DOMAIN": settings.PRODUCTION_DOMAIN,
         }

readthedocs/projects/tests/test_build_tasks.py

@@ -372,7 +372,7 @@ def test_get_env_vars(self, load_yaml_config, build_environment, config, externa
                 self.project.checkout_path(self.version.slug), "_readthedocs/"
             ),
             "READTHEDOCS_GIT_CLONE_URL": self.project.repo,
-            "READTHEDOCS_GIT_IDENTIFIER": self.version.identifier,
+            "READTHEDOCS_GIT_IDENTIFIER": self.version.git_identifier,
             "READTHEDOCS_GIT_COMMIT_HASH": self.build.commit,
             "READTHEDOCS_PRODUCTION_DOMAIN": settings.PRODUCTION_DOMAIN,
         }

readthedocs/rtd_tests/tests/test_api.py

@@ -3292,6 +3292,7 @@ def test_get_version_by_id(self):
             "privacy_level": "public",
             "downloads": {},
             "identifier": "2404a34eba4ee9c48cc8bc4055b99a48354f4950",
+            "git_identifier": "0.8",
             "slug": "0.8",
             "has_epub": False,
             "has_htmlzip": False,

readthedocs/rtd_tests/tests/test_project_forms.py

@@ -478,29 +478,6 @@ def test_list_user_created_latest_and_stable_versions_in_default_branch_choices(
             ],
         )
 
-    def test_commit_name_not_in_default_branch_choices(self):
-        form = UpdateProjectForm(instance=self.project)
-        # This version is created by the user
-        latest = self.project.versions.filter(slug=LATEST)
-        # This version is created by the user
-        stable = self.project.versions.filter(slug=STABLE)
-
-        # `commit_name` can not be used as the value for the choices
-        self.assertNotIn(
-            latest.first().commit_name,
-            [
-                identifier
-                for identifier, _ in form.fields["default_branch"].widget.choices
-            ],
-        )
-        self.assertNotIn(
-            stable.first().commit_name,
-            [
-                identifier
-                for identifier, _ in form.fields["default_branch"].widget.choices
-            ],
-        )
-
     def test_external_version_not_in_default_branch_choices(self):
         external_version = get(
             Version,

readthedocs/rtd_tests/tests/test_version.py

@@ -94,15 +94,15 @@ def test_vcs_url_for_manual_stable_version(self):
         stable.save()
         assert "https://github.com/pypa/pip/tree/stable/" == stable.vcs_url
 
-    def test_commit_name_for_stable_version(self):
-        self.assertEqual(self.branch_version.commit_name, "stable")
+    def test_git_identifier_for_stable_version(self):
+        self.assertEqual(self.branch_version.git_identifier, "stable")
 
-    def test_commit_name_for_latest_version(self):
-        self.assertEqual(self.tag_version.commit_name, "master")
+    def test_git_identifier_for_latest_version(self):
+        self.assertEqual(self.tag_version.git_identifier, "master")
 
-    def test_commit_name_for_external_version(self):
+    def test_git_identifier_for_external_version(self):
         self.assertEqual(
-            self.external_version.commit_name, self.external_version.identifier
+            self.external_version.git_identifier, self.external_version.identifier
         )
 
     @override_settings(

readthedocs/rtd_tests/tests/test_version_commit_name.py

@@ -33,7 +33,7 @@ def test_branch_name(self):
             verbose_name="release-2.5.x",
             type=BRANCH,
         )
-        self.assertEqual(version.commit_name, "release-2.5.x")
+        self.assertEqual(version.git_identifier, "release-2.5.x")
 
     def test_tag_name(self):
         version = new(
@@ -43,7 +43,7 @@ def test_tag_name(self):
             verbose_name="release-2.5.0",
             type=TAG,
         )
-        self.assertEqual(version.commit_name, "release-2.5.0")
+        self.assertEqual(version.git_identifier, "release-2.5.0")
 
     def test_branch_with_name_stable(self):
         version = new(
@@ -53,7 +53,7 @@ def test_branch_with_name_stable(self):
             verbose_name="stable",
             type=BRANCH,
         )
-        self.assertEqual(version.commit_name, "stable")
+        self.assertEqual(version.git_identifier, "stable")
 
     def test_stable_version_tag(self):
         version = new(
@@ -65,10 +65,33 @@ def test_stable_version_tag(self):
             machine=True,
         )
         self.assertEqual(
-            version.commit_name,
+            version.git_identifier,
             "3d92b728b7d7b842259ac2020c2fa389f13aff0d",
         )
 
+    def test_stable_version_tag_uses_original_stable(self):
+        project = get(Project)
+        stable = get(
+            Version,
+            project=project,
+            identifier="3d92b728b7d7b842259ac2020c2fa389f13aff0d",
+            slug=STABLE,
+            verbose_name=STABLE,
+            type=TAG,
+            machine=True,
+        )
+        original_stable = get(
+            Version,
+            project=project,
+            identifier="3d92b728b7d7b842259ac2020c2fa389f13aff0d",
+            slug="v2.0",
+            verbose_name="v2.0",
+            type=TAG,
+            machine=True,
+        )
+        self.assertEqual(stable.git_identifier, "v2.0")
+        self.assertEqual(original_stable.git_identifier, "v2.0")
+
     def test_manual_stable_version(self):
         project = get(Project)
         version = get(
@@ -80,7 +103,7 @@ def test_manual_stable_version(self):
             type=BRANCH,
             machine=False,
         )
-        self.assertEqual(version.commit_name, "stable")
+        self.assertEqual(version.git_identifier, "stable")
 
     def test_git_latest_branch(self):
         git_project = get(Project, repo_type=REPO_TYPE_GIT)
@@ -93,14 +116,14 @@ def test_git_latest_branch(self):
             type=BRANCH,
             machine=True,
         )
-        self.assertEqual(version.commit_name, "master")
+        self.assertEqual(version.git_identifier, "master")
 
     def test_manual_latest_version(self):
         project = get(Project)
         version = project.versions.get(slug=LATEST)
         version.machine = False
         version.save()
-        self.assertEqual(version.commit_name, "latest")
+        self.assertEqual(version.git_identifier, "latest")
 
     def test_external_version(self):
         identifier = "ec26de721c3235aad62de7213c562f8c821"
@@ -111,4 +134,4 @@ def test_external_version(self):
             verbose_name="11",
             type=EXTERNAL,
         )
-        self.assertEqual(version.commit_name, identifier)
+        self.assertEqual(version.git_identifier, identifier)