Build: Simplify and optimize git backend: New clone+fetch pattern (#10430)

* New git clone/fetch/checkout behavior first steps

* Use `identifier` argument for `.update()`, add lots of comments for future readers

* Add missing str() cast

* Remove the `identifier` keyword since we are forced to use verbose_name if tags should work :/

* Revert "Remove the `identifier` keyword since we are forced to use verbose_name if tags should work :/"

This reverts commit 82178a3.

* We need the identifier for branches, but not for tags and PRs

* Update some comments after noticing how `Build.identifier/.verbose_name` fields are used

* lint

* Revert "Revert "Remove the `identifier` keyword since we are forced to use verbose_name if tags should work :/""

This reverts commit a45b42b.

* Use version_identifier as an instance property

* Update readthedocs/vcs_support/backends/git.py

Co-authored-by: Manuel Kaufmann <[email protected]>

* Updates to return values, comments and logging

* Restore common

* Stop fetching --tags and fetch the direct remote reference

* Appease linter: Use raise-from pattern

* Hello darker my old friend

* Restore error handling for clone operation

* Update a few comments, add a bit more raise-from pattern (actually trying to use it for debugging!)

* WIP: refine some comments, log calls and small things while testing locally

* New test class with Feature flag defined

* Adds a tearDown method after discovering that tests were wrongly passing because working directory leaks between test cases

* Log a warning if we aren't specifying the version type when using Git, fetch branches by their exact remote reference and point them to a local branch, rather than FETCH_HEAD

* Update test cases for new git clone+fetch pattern

* Git fetch change: Fetch branches to :refs/remotes/origin/<branch-name> + comment updates

* Overwrite tests to call clone_ng and fetch_ng, and rewrite some entirely

* Update update() docstring

* Always use lsremote for GIT_CLONE_FETCH_CHECKOUT_PATTERN

* Builds need to know about Version.machine=True for tags "stable" so they don't try to get "stable" as a tag - alternatively, we hardcode a check for verbose_name=="stable"

* Conditionally log a warning on stable versions

* linting

* add "machine" to version API test case

* Use "refspec" terminology more consistently

* Add test case for un-named default branches

* Don't have "--no-checkout" when we *need* to checkout the remote HEAD

* Add test case to verify that we can clone+fetch special tag "stable" (machine=True)

* Update readthedocs/vcs_support/backends/git.py

Co-authored-by: Manuel Kaufmann <[email protected]>

* Remove unused logging

* Use `git fetch <commit hash>` for special "stable" versions

* Building an unnamed default branch now skips `git fetch`

* Remove unnecessary logging, add a lot of comments to tests

* Apply suggestions from @humitos code review

Co-authored-by: Manuel Kaufmann <[email protected]>

* Update readthedocs/projects/models.py

Co-authored-by: Manuel Kaufmann <[email protected]>

* Always clean up the presumed repo working dir after a test case

* Move `machine` to `VersionAdminSerializer`

* Add precision to comment

* Use identifier and verbose_name that looks more like actual data would look

* remove unused local reference in git fetch for "stable" tag

* Don't use --no-checkout, just checkout whatever is at the FETCH_HEAD

* Add more logging

* Update comment

* Fix logic condition: Always include `vcs_repository.supports_lsremote`

* Log a warning when we try to generate a pr/mr refspec for an unsupported Git provider

* improve doc strings

* Remove TestGitBackendTwiceInARow

* Use real commit hash in test case

---------

Co-authored-by: Manuel Kaufmann <[email protected]>

Author: benjaoming

readthedocs/api/v2/serializers.py

@@ -130,20 +130,20 @@ class VersionSerializer(serializers.ModelSerializer):
     class Meta:
         model = Version
         fields = [
-            'id',
-            'project',
-            'slug',
-            'identifier',
-            'verbose_name',
-            'privacy_level',
-            'active',
-            'built',
-            'downloads',
-            'type',
-            'has_pdf',
-            'has_epub',
-            'has_htmlzip',
-            'documentation_type',
+            "id",
+            "project",
+            "slug",
+            "identifier",
+            "verbose_name",
+            "privacy_level",
+            "active",
+            "built",
+            "downloads",
+            "type",
+            "has_pdf",
+            "has_epub",
+            "has_htmlzip",
+            "documentation_type",
         ]
 
     def __init__(self, *args, **kwargs):
@@ -192,6 +192,7 @@ class Meta(VersionSerializer.Meta):
             "addons",
             "build_data",
             "canonical_url",
+            "machine",
         ]
 
 

readthedocs/api/v2/views/integrations.py

@@ -15,11 +15,7 @@
 from rest_framework.status import HTTP_400_BAD_REQUEST
 from rest_framework.views import APIView
 
-from readthedocs.builds.constants import (
-    EXTERNAL_VERSION_STATE_CLOSED,
-    EXTERNAL_VERSION_STATE_OPEN,
-    LATEST,
-)
+from readthedocs.builds.constants import LATEST
 from readthedocs.core.signals import webhook_bitbucket, webhook_github, webhook_gitlab
 from readthedocs.core.views.hooks import (
     build_branches,
@@ -97,8 +93,8 @@ def post(self, request, project_slug):
             if not Project.objects.is_active(self.project):
                 resp = {'detail': 'This project is currently disabled'}
                 return Response(resp, status=status.HTTP_406_NOT_ACCEPTABLE)
-        except Project.DoesNotExist:
-            raise NotFound('Project not found')
+        except Project.DoesNotExist as exc:
+            raise NotFound("Project not found") from exc
         if not self.is_payload_valid():
             log.warning('Invalid payload for project and integration.')
             return Response(
@@ -190,7 +186,7 @@ def get_response_push(self, project, branches):
 
         :param project: Project instance
         :type project: Project
-        :param branches: List of branch names to build
+        :param branches: List of branch/tag names to build
         :type branches: list(str)
         """
         to_build, not_building = build_branches(project, branches)
@@ -365,7 +361,7 @@ def get_external_version_data(self):
             return data
         except KeyError as e:
             key = e.args[0]
-            raise ParseError(f"Invalid payload. {key} is required.")
+            raise ParseError(f"Invalid payload. {key} is required.") from e
 
     def is_payload_valid(self):
         """
@@ -500,8 +496,8 @@ def handle_webhook(self):
             try:
                 branch = self._normalize_ref(self.data["ref"])
                 return self.get_response_push(self.project, [branch])
-            except KeyError:
-                raise ParseError('Parameter "ref" is required')
+            except KeyError as exc:
+                raise ParseError('Parameter "ref" is required') from exc
 
         return None
 
@@ -581,7 +577,7 @@ def get_external_version_data(self):
             return data
         except KeyError as e:
             key = e.args[0]
-            raise ParseError(f"Invalid payload. {key} is required.")
+            raise ParseError(f"Invalid payload. {key} is required.") from e
 
     def handle_webhook(self):
         """
@@ -624,8 +620,8 @@ def handle_webhook(self):
             try:
                 branch = self._normalize_ref(data["ref"])
                 return self.get_response_push(self.project, [branch])
-            except KeyError:
-                raise ParseError('Parameter "ref" is required')
+            except KeyError as exc:
+                raise ParseError('Parameter "ref" is required') from exc
 
         if self.project.external_builds_enabled and event == GITLAB_MERGE_REQUEST:
             if (
@@ -726,8 +722,8 @@ def handle_webhook(self):
                     )
                 log.debug("Triggered sync_versions.")
                 return self.sync_versions_response(self.project)
-            except KeyError:
-                raise ParseError('Invalid request')
+            except KeyError as exc:
+                raise ParseError("Invalid request") from exc
         return None
 
     def is_payload_valid(self):
@@ -809,8 +805,8 @@ def handle_webhook(self):
                 self.update_default_branch(default_branch)
 
             return self.get_response_push(self.project, branches)
-        except TypeError:
-            raise ParseError('Invalid request')
+        except TypeError as exc:
+            raise ParseError("Invalid request") from exc
 
     def is_payload_valid(self):
         """

readthedocs/builds/tasks.py

@@ -292,8 +292,16 @@ def sync_versions_task(project_pk, tags_data, branches_data, **kwargs):
     Creates new Version objects for tags/branches that aren't tracked in the database,
     and deletes Version objects for tags/branches that don't exists in the repository.
 
-    :param tags_data: List of dictionaries with ``verbose_name`` and ``identifier``.
-    :param branches_data: Same as ``tags_data`` but for branches.
+    :param tags_data: List of dictionaries with ``verbose_name`` and ``identifier``
+                      Example: [
+                          {"verbose_name": "v1.0.0",
+                           "identifier": "67a9035990f44cb33091026d7453d51606350519"},
+                      ].
+    :param branches_data: Same as ``tags_data`` but for branches (branch name, branch identifier).
+                      Example: [
+                          {"verbose_name": "latest",
+                           "identifier": "main"},
+                      ].
     :returns: `True` or `False` if the task succeeded.
     """
     project = Project.objects.get(pk=project_pk)

readthedocs/doc_builder/director.py

@@ -94,6 +94,8 @@ def setup_vcs(self):
             environment=self.vcs_environment,
             verbose_name=self.data.version.verbose_name,
             version_type=self.data.version.type,
+            version_identifier=self.data.version.identifier,
+            version_machine=self.data.version.machine,
         )
 
         # We can't do too much on ``pre_checkout`` because we haven't
@@ -222,7 +224,7 @@ def build(self):
     def checkout(self):
         """Checkout Git repo and load build config file."""
 
-        log.info("Cloning repository.")
+        log.info("Cloning and fetching.")
         self.vcs_repository.update()
 
         identifier = self.data.build_commit or self.data.version.identifier

readthedocs/projects/models.py

@@ -995,7 +995,13 @@ def has_htmlzip(self, version_slug=LATEST, version_type=None):
         )
 
     def vcs_repo(
-        self, environment, version=LATEST, verbose_name=None, version_type=None
+        self,
+        environment,
+        version=LATEST,
+        verbose_name=None,
+        version_type=None,
+        version_identifier=None,
+        version_machine=None,
     ):
         """
         Return a Backend object for this project able to handle VCS commands.
@@ -1014,8 +1020,13 @@ def vcs_repo(
             repo = None
         else:
             repo = backend(
-                self, version, environment=environment,
-                verbose_name=verbose_name, version_type=version_type
+                self,
+                version,
+                environment=environment,
+                verbose_name=verbose_name,
+                version_type=version_type,
+                version_identifier=version_identifier,
+                version_machine=version_machine,
             )
         return repo
 
@@ -1935,6 +1946,7 @@ def add_features(sender, **kwargs):
     INDEX_FROM_HTML_FILES = 'index_from_html_files'
 
     # Build related features
+    GIT_CLONE_FETCH_CHECKOUT_PATTERN = "git_clone_fetch_checkout_pattern"
     HOSTING_INTEGRATIONS = "hosting_integrations"
     NO_CONFIG_FILE_DEPRECATED = "no_config_file"
     SCALE_IN_PROTECTION = "scale_in_prtection"
@@ -2058,6 +2070,12 @@ def add_features(sender, **kwargs):
                 "sources"
             ),
         ),
+        (
+            GIT_CLONE_FETCH_CHECKOUT_PATTERN,
+            _(
+                "Build: Use simplified and optimized git clone + git fetch + git checkout patterns"
+            ),
+        ),
         (
             HOSTING_INTEGRATIONS,
             _(

readthedocs/projects/tasks/mixins.py

@@ -45,9 +45,13 @@ def sync_versions(self, vcs_repository):
         # Do not use ``ls-remote`` if the VCS does not support it or if we
         # have already cloned the repository locally. The latter happens
         # when triggering a normal build.
-        use_lsremote = (
-            vcs_repository.supports_lsremote
-            and not vcs_repository.repo_exists()
+        # Always use lsremote when we have GIT_CLONE_FETCH_CHECKOUT_PATTERN on
+        # and the repo supports lsremote.
+        # The new pattern does not fetch branch and tag data, so we always
+        # have to do ls-remote.
+        use_lsremote = vcs_repository.supports_lsremote and (
+            self.data.project.has_feature(Feature.GIT_CLONE_FETCH_CHECKOUT_PATTERN)
+            or not vcs_repository.repo_exists()
         )
         sync_tags = vcs_repository.supports_tags and not self.data.project.has_feature(
             Feature.SKIP_SYNC_TAGS
@@ -63,6 +67,10 @@ def sync_versions(self, vcs_repository):
                 include_tags=sync_tags,
                 include_branches=sync_branches,
             )
+
+        # GIT_CLONE_FETCH_CHECKOUT_PATTERN: When the feature flag becomes default, we
+        # can remove this segment since lsremote is always on.
+        # We can even factor out the dependency to gitpython.
         else:
             if sync_tags:
                 tags = vcs_repository.tags

readthedocs/rtd_tests/tests/test_api.py

@@ -3127,14 +3127,15 @@ def test_get_version_by_id(self):
                 "users": [1],
                 "urlconf": None,
             },
-            'privacy_level': 'public',
-            'downloads': {},
-            'identifier': '2404a34eba4ee9c48cc8bc4055b99a48354f4950',
-            'slug': '0.8',
-            'has_epub': False,
-            'has_htmlzip': False,
-            'has_pdf': False,
-            'documentation_type': 'sphinx',
+            "privacy_level": "public",
+            "downloads": {},
+            "identifier": "2404a34eba4ee9c48cc8bc4055b99a48354f4950",
+            "slug": "0.8",
+            "has_epub": False,
+            "has_htmlzip": False,
+            "has_pdf": False,
+            "documentation_type": "sphinx",
+            "machine": False,
         }
 
         self.assertDictEqual(