Build: use actual git identifier for READTHEDOCS_GIT_IDENTIFIER (#11875)

A commit can point to more than one branch/tag, so using git
describe/name-rev or similar doesn't work reliably. Since we already
have the tag/branch name for most of the versions (verbose_name), we can
just use that, there are only three special cases:

- LATEST: this is an alias for the default branch
- STABLE: this is an alias for the stable version
- External versions: we don't save the name of the base branch, just the
commit and PR number, so not sure what to return here, I'm returning the
commit for now. Maybe we can update the docs to mention this.

`commit_name` was doing exactly that, except for stable, so I went ahead
and renamed it and refactored it (we were no longer using it) to use it
in our API, so builders have access to it.

Closes #11662

Author: stsewd

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)