Build: Simplify and optimize git backend: New clone+fetch pattern

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,
@@ -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)

readthedocs/builds/tasks.py

@@ -292,8 +292,10 @@ 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: ("v1.0.0", "67a9035990f44cb33091026d7453d51606350519").
+    :param branches_data: Same as ``tags_data`` but for branches (branch name, branch identifier).
+                          Example: ("latest", "main")
     :returns: `True` or `False` if the task succeeded.
     """
     project = Project.objects.get(pk=project_pk)

readthedocs/doc_builder/director.py

@@ -209,10 +209,10 @@ def build(self):
     def checkout(self):
         """Checkout Git repo and load build config file."""
 
-        log.info("Cloning repository.")
-        self.vcs_repository.update()
-
         identifier = self.data.build_commit or self.data.version.identifier
+        log.info("Cloning and fetching.", identifier=identifier)
+        self.vcs_repository.update(identifier=identifier)
+
         log.info("Checking out.", identifier=identifier)
         self.vcs_repository.checkout(identifier)
 

readthedocs/projects/models.py

@@ -995,7 +995,11 @@ 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,
     ):
         """
         Return a Backend object for this project able to handle VCS commands.
@@ -1938,6 +1942,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"
     DONT_CREATE_INDEX = "dont_create_index"
     HOSTING_INTEGRATIONS = "hosting_integrations"
     NO_CONFIG_FILE_DEPRECATED = "no_config_file"
@@ -2066,7 +2071,12 @@ def add_features(sender, **kwargs):
                 "sources"
             ),
         ),
-
+        (
+            GIT_CLONE_FETCH_CHECKOUT_PATTERN,
+            _(
+                "Build: Use simplified and optimized git clone + git fetch + git checkout patterns"
+            ),
+        ),
         (
             DONT_CREATE_INDEX,
             _(

readthedocs/vcs_support/backends/bzr.py

@@ -16,8 +16,8 @@ class Backend(BaseVCS):
     supports_tags = True
     fallback_branch = ''
 
-    def update(self):
-        super().update()
+    def update(self, identifier=None):
+        super().update(identifier=identifier)
         if self.repo_exists():
             return self.up()
         return self.clone()

readthedocs/vcs_support/backends/git.py

@@ -8,7 +8,7 @@
 from git.exc import BadName, InvalidGitRepositoryError, NoSuchPathError
 from gitdb.util import hex_to_bin
 
-from readthedocs.builds.constants import EXTERNAL
+from readthedocs.builds.constants import BRANCH, EXTERNAL, TAG
 from readthedocs.config import ALL
 from readthedocs.projects.constants import (
     GITHUB_BRAND,
@@ -55,18 +55,122 @@ def _get_clone_url(self):
     def set_remote_url(self, url):
         return self.run('git', 'remote', 'set-url', 'origin', url)
 
-    def update(self):
-        """Clone or update the repository."""
-        super().update()
-        if self.repo_exists():
-            self.set_remote_url(self.repo_url)
-            return self.fetch()
-        self.make_clean_working_dir()
-        # A fetch is always required to get external versions properly
+    def update(self, identifier=None):
+        """
+        Clone or update the repository.
+
+        :param identifier: This is the optional identifier for git fetch - a branch or tag name.
+                           PR references are generated automatically for certain Git providers.
+        :return:
+        """
+        super().update(identifier=identifier)
+
+        if self.use_clone_fetch_checkout_pattern():
+
+            # New behavior: Clone is responsible for skipping the operation if the
+            # repo is already cloned.
+            self.clone_ng()
+            # New behavior: No confusing return value. We are not using return values
+            # in the callers.
+            self.fetch_ng(identifier=identifier)
+
+        else:
+            if self.repo_exists():
+                self.set_remote_url(self.repo_url)
+                return self.fetch()
+            self.make_clean_working_dir()
+            # A fetch is always required to get external versions properly
+            if self.version_type == EXTERNAL:
+                self.clone()
+                return self.fetch()
+            return self.clone()
+
+    def get_remote_fetch_reference(self, identifier):
+        """
+        Gets a valid remote reference for the identifier.
+
+        This method is terrible. It decides how to treat the incoming identifier from
+        knowledge of how the caller (the build process) uses build data.
+
+        Build.identifier = a branch name (branches)
+        Build.identifier = commit (tags)
+        Build.identifier = commit (external versions)
+        Build.verbose_name = branch alias, e.g. latest (branches)
+        Build.verbose_name = tag name (tags)
+        Build.verbose_name = PR number (external versions)
+
+        :param identifier: Should be a branch or tag name when building branches or tags.
+        :return: A reference valid for fetch operation
+        """
+        # Branches have the branch identifier set by the caller who instantiated the
+        # Git backend
+        if self.version_type == BRANCH:
+            return identifier
+        # Tags
+        if self.version_type == TAG:
+            return self.verbose_name
         if self.version_type == EXTERNAL:
-            self.clone()
-            return self.fetch()
-        return self.clone()
+
+            # TODO: We should be able to resolve this without looking up in oauth registry
+            git_provider_name = self.project.git_provider_name
+
+            # TODO: Why are these our only patterns?
+            if git_provider_name == GITHUB_BRAND:
+                return GITHUB_PR_PULL_PATTERN.format(id=self.verbose_name)
+            if self.project.git_provider_name == GITLAB_BRAND:
+                return GITLAB_MR_PULL_PATTERN.format(id=self.verbose_name)
+
+            # This seems to be the default behavior when we don't know the remote
+            # reference for a PR/MR: Just fetch everything
+            # TODO: Provide more information about fetch operations without references
+            return None
+
+    def clone_ng(self):
+        # If the repository is already cloned, we don't do anything.
+        # This is likely legacy, but we may want to be able to call .update()
+        # several times in the same build
+        if self.repo_exists():
+            return
+
+        # --no-checkout: Makes it explicit what we are doing here. Nothing is checked out
+        #                until it's explicitly done.
+        # --depth 1: Shallow clone, fetch as little data as possible.
+        cmd = ["git", "clone", "--no-checkout", "--depth", "1", self.repo_url, "."]
+
+        code, stdout, stderr = self.run(*cmd)
+        return code, stdout, stderr
+
+    def fetch_ng(self, identifier):
+        """Implementation for new clone+fetch+checkout pattern."""
+
+        # --force: Likely legacy, it seems to be irrelevant to this usage
+        # --tags: We need to fetch tags in order to resolve these references in the checkout
+        # --prune: Likely legacy, we don't expect a previous fetch command to have run
+        # --prune-tags: Likely legacy, we don't expect a previous fetch command to have run
+        # --tags: This flag was used in the previous approach such that all tags were fetched
+        #         in order to checkout a tag afterwards.
+        # --depth: This flag should be made unnecessary, it's downloading commit data that's
+        #          never used.
+        cmd = [
+            "git",
+            "fetch",
+            "origin",
+            "--force",
+            "--tags",
+            "--prune",
+            "--prune-tags",
+            "--depth",
+            str(self.repo_depth),
+        ]
+        remote_reference = self.get_remote_fetch_reference(identifier)
+
+        if remote_reference:
+            # TODO: We are still fetching the latest 50 commits.
+            # A PR might have another commit added after the build has started...
+            cmd.append(remote_reference)
+
+        code, stdout, stderr = self.run(*cmd)
+        return code, stdout, stderr
 
     def repo_exists(self):
         try:
@@ -150,6 +254,13 @@ def use_shallow_clone(self):
         from readthedocs.projects.models import Feature
         return not self.project.has_feature(Feature.DONT_SHALLOW_CLONE)
 
+    def use_clone_fetch_checkout_pattern(self):
+        """Use the new and optimized clone / fetch / checkout pattern."""
+        from readthedocs.projects.models import Feature
+
+        # TODO: Remove the 'not'
+        return not self.project.has_feature(Feature.GIT_CLONE_FETCH_CHECKOUT_PATTERN)
+
     def fetch(self):
         # --force lets us checkout branches that are not fast-forwarded
         # https://github.com/readthedocs/readthedocs.org/issues/6097

readthedocs/vcs_support/backends/hg.py

@@ -12,8 +12,8 @@ class Backend(BaseVCS):
     supports_branches = True
     fallback_branch = 'default'
 
-    def update(self):
-        super().update()
+    def update(self, identifier=None):
+        super().update(identifier=identifier)
         if self.repo_exists():
             return self.pull()
         return self.clone()

readthedocs/vcs_support/backends/svn.py

@@ -26,8 +26,8 @@ def __init__(self, *args, **kwargs):
         else:
             self.base_url = self.repo_url
 
-    def update(self):
-        super().update()
+    def update(self, identifier=None):
+        super().update(identifier=identifier)
         if self.repo_exists():
             return self.up()
         return self.co()

readthedocs/vcs_support/base.py

@@ -83,7 +83,7 @@ def make_clean_working_dir(self):
         safe_rmtree(self.working_dir, ignore_errors=True)
         self.check_working_dir()
 
-    def update(self):
+    def update(self, identifier=None):
         """
         Update a local copy of the repository in self.working_dir.