path: root/dev-util/rbtools/files/rbtools-5.0-scmtool-crash.patch

From 36efa0724e3b4d941aa0554b1d82934319d25ce3 Mon Sep 17 00:00:00 2001
From: David Trowbridge <trowbrds@gmail.com>
Date: Tue, 11 Jun 2024 17:22:15 -0600
Subject: [PATCH] Use SCMTool IDs to look up repositories when possible.

Many versions of Review Board currently have a bug where sending an
unknown value in the tool= parameter to the repository list API would
cause a crash. We'll be shipping a fix for that, but unfortunately there
are a variety of released versions where the Git and Clearcase clients
can trigger this crash on servers that do not have Power Pack installed.

This change fixes the RBTools side to not include the Power Pack SCMTool
names, which will avoid the problem for existing servers. For new
servers, we'll be shipping a server-side fix that fixes the crash, adds
the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs
rather than names to the repository list API. If we see the IDs in the
capability blob, we can assume that we can pass the IDs, including
potentially unknown IDs.

This also fixes a problem where we were sometimes accessing the
repository list API twice with exactly the same parameters.

Testing Done:
- Ran unit tests.
- Verified that the repository list API was accessed using SCMTool names
  that did not include potentially missing ones when running against an
  older server.
- Verified that the repository list API was accessed with SCMTool IDs
  when running against a server with the new API fixes.

Reviewed at https://reviews.reviewboard.org/r/13973/
---
 rbtools/clients/base/scmclient.py | 59 +++++++++++++++++++++++++------
 rbtools/clients/bazaar.py         |  1 +
 rbtools/clients/clearcase.py      | 10 +++++-
 rbtools/clients/cvs.py            |  1 +
 rbtools/clients/git.py            | 10 +++++-
 rbtools/clients/mercurial.py      |  1 +
 rbtools/clients/perforce.py       |  1 +
 rbtools/clients/plastic.py        |  1 +
 rbtools/clients/svn.py            |  1 +
 rbtools/clients/tfs.py            |  1 +
 rbtools/commands/base/commands.py |  3 +-
 rbtools/commands/setup_repo.py    | 25 +++++++++----
 rbtools/utils/repository.py       | 58 ++++++++++++++++++++++--------
 13 files changed, 139 insertions(+), 33 deletions(-)

diff --git a/rbtools/clients/base/scmclient.py b/rbtools/clients/base/scmclient.py
index c053b91b..9f56c084 100644
--- a/rbtools/clients/base/scmclient.py
+++ b/rbtools/clients/base/scmclient.py
@@ -9,24 +9,24 @@
 import argparse
 import logging
 import re
-from typing import (Any, Dict, List, Mapping, Optional, Tuple, Union, cast,
-                    TYPE_CHECKING)
+from typing import (Any, cast, ClassVar, Dict, List, Mapping, Optional,
+                    TYPE_CHECKING, Tuple, Union)
 
 from typing_extensions import NotRequired, TypedDict, final
 
-from rbtools.api.capabilities import Capabilities
-from rbtools.api.resource import (ItemResource,
-                                  ListResource,
-                                  ReviewRequestResource)
 from rbtools.clients.base.patch import PatchAuthor, PatchResult
-from rbtools.clients.base.repository import RepositoryInfo
 from rbtools.clients.errors import SCMClientDependencyError, SCMError
 from rbtools.deprecation import RemovedInRBTools50Warning
-from rbtools.diffs.tools.base import BaseDiffTool
 from rbtools.diffs.tools.registry import diff_tools_registry
 from rbtools.utils.process import execute
 
 if TYPE_CHECKING:
+    from rbtools.api.capabilities import Capabilities
+    from rbtools.api.resource import (ItemResource,
+                                      ListResource,
+                                      ReviewRequestResource)
+    from rbtools.clients.base.repository import RepositoryInfo
+    from rbtools.diffs.tools.base import BaseDiffTool
     from rbtools.config import RBToolsConfig
 
 
@@ -281,14 +281,27 @@ class BaseSCMClient(object):
     #:     str
     name: str = ''
 
-    #: A comma-separated list of SCMClient names on the server
+    #: A comma-separated list of SCMClient names on the server.
     #:
     #: Version Added:
     #:    3.0
     #:
     #: Type:
     #:     str
-    server_tool_names: Optional[str] = None
+    server_tool_names: ClassVar[Optional[str]] = None
+
+    #: A comma-separated list of SCMClient IDs on the server.
+    #:
+    #: This supersedes :py:attr:`server_tool_names` when running on a version
+    #: of Review Board that supports passing tool IDs to the repositories
+    #: list API.
+    #:
+    #: Version Added:
+    #:    5.0.1
+    #:
+    #: Type:
+    #:     str
+    server_tool_ids: ClassVar[Optional[List[str]]] = None
 
     #: Whether this tool requires a command line diff tool.
     #:
@@ -692,6 +705,32 @@ def get_diff_tool(self) -> Optional[BaseDiffTool]:
 
         return diff_tool
 
+    def get_server_tool_names(
+        self,
+        capabilities: Optional[Capabilities],
+    ) -> Optional[str]:
+        """Return the list of supported tool names on the server.
+
+        Version Added:
+            5.0.1
+
+        Args:
+            capabilities (rbtools.api.capabilities.Capabilities):
+                The server capabilities, if present.
+
+        Returns:
+            str:
+            A comma-separated list of server-side tool names to match with.
+        """
+        if (capabilities is not None and
+            capabilities.get_capability('scmtools', 'supported_tools') and
+            self.server_tool_ids is not None):
+            # Versions of Review Board that have this capability allow us to
+            # pass SCMTool IDs rather than names.
+            return ','.join(self.server_tool_ids)
+        else:
+            return self.server_tool_names
+
     def find_matching_server_repository(
         self,
         repositories: ListResource,
diff --git a/rbtools/clients/bazaar.py b/rbtools/clients/bazaar.py
index 0e39315d..4174c010 100644
--- a/rbtools/clients/bazaar.py
+++ b/rbtools/clients/bazaar.py
@@ -37,6 +37,7 @@ class BazaarClient(BaseSCMClient):
     scmclient_id = 'bazaar'
     name = 'Bazaar'
     server_tool_names = 'Bazaar'
+    server_tool_ids = ['bazaar']
     supports_diff_exclude_patterns = True
     supports_parent_diffs = True
     can_branch = True
diff --git a/rbtools/clients/clearcase.py b/rbtools/clients/clearcase.py
index f9c8762e..e2c04633 100644
--- a/rbtools/clients/clearcase.py
+++ b/rbtools/clients/clearcase.py
@@ -392,7 +392,15 @@ class ClearCaseClient(BaseSCMClient):
 
     scmclient_id = 'clearcase'
     name = 'VersionVault / ClearCase'
-    server_tool_names = 'ClearCase,VersionVault / ClearCase'
+
+    # Review Board versions that use the old names-based repositories/?tool=
+    # API parameter also have a bug where a missing name could cause a
+    # server-side crash. This was making it so servers that did not have Power
+    # Pack were failing when we tried to make a query that included the
+    # VersionVault name. We therefore only include it when we know the server
+    # can use server_tool_ids instead.
+    server_tool_names = 'ClearCase'
+    server_tool_ids = ['clearcase', 'versionvault']
 
     requires_diff_tool = True
 
diff --git a/rbtools/clients/cvs.py b/rbtools/clients/cvs.py
index 98354e8b..dd2c72ef 100644
--- a/rbtools/clients/cvs.py
+++ b/rbtools/clients/cvs.py
@@ -28,6 +28,7 @@ class CVSClient(BaseSCMClient):
     scmclient_id = 'cvs'
     name = 'CVS'
     server_tool_names = 'CVS'
+    server_tool_ids = ['cvs']
     supports_diff_exclude_patterns = True
     supports_patch_revert = True
 
diff --git a/rbtools/clients/git.py b/rbtools/clients/git.py
index f4b96cb6..431fd7c6 100644
--- a/rbtools/clients/git.py
+++ b/rbtools/clients/git.py
@@ -76,7 +76,15 @@ class GitClient(BaseSCMClient):
 
     scmclient_id = 'git'
     name = 'Git'
-    server_tool_names = 'Git,Perforce,Subversion,Team Foundation Server (git)'
+
+    # Review Board versions that use the old names-based repositories/?tool=
+    # API parameter also have a bug where a missing name could cause a
+    # server-side crash. This was making it so servers that did not have Power
+    # Pack were failing when we tried to make a query that included the TFS-Git
+    # name. We therefore only include it when we know the server can use
+    # server_tool_ids instead.
+    server_tool_names = 'Git,Perforce,Subversion'
+    server_tool_ids = ['git', 'perforce', 'subversion', 'tfs_git']
 
     supports_commit_history = True
     supports_diff_exclude_patterns = True
diff --git a/rbtools/clients/mercurial.py b/rbtools/clients/mercurial.py
index 2ee473c9..6f1c5ea0 100644
--- a/rbtools/clients/mercurial.py
+++ b/rbtools/clients/mercurial.py
@@ -64,6 +64,7 @@ class MercurialClient(BaseSCMClient):
     scmclient_id = 'mercurial'
     name = 'Mercurial'
     server_tool_names = 'Mercurial,Subversion'
+    server_tool_ids = ['mercurial', 'subversion']
 
     supports_commit_history = True
     supports_diff_exclude_patterns = True
diff --git a/rbtools/clients/perforce.py b/rbtools/clients/perforce.py
index f5892073..4f5d7fb1 100644
--- a/rbtools/clients/perforce.py
+++ b/rbtools/clients/perforce.py
@@ -445,6 +445,7 @@ class PerforceClient(BaseSCMClient):
     scmclient_id = 'perforce'
     name = 'Perforce'
     server_tool_names = 'Perforce'
+    server_tool_ids = ['perforce']
 
     requires_diff_tool = True
 
diff --git a/rbtools/clients/plastic.py b/rbtools/clients/plastic.py
index c45c7abd..3bec215a 100644
--- a/rbtools/clients/plastic.py
+++ b/rbtools/clients/plastic.py
@@ -28,6 +28,7 @@ class PlasticClient(BaseSCMClient):
     scmclient_id = 'plastic'
     name = 'Plastic'
     server_tool_names = 'Plastic SCM'
+    server_tool_ids = ['plastic']
     supports_changesets = True
     supports_patch_revert = True
 
diff --git a/rbtools/clients/svn.py b/rbtools/clients/svn.py
index 34cdffcb..6d0352cc 100644
--- a/rbtools/clients/svn.py
+++ b/rbtools/clients/svn.py
@@ -52,6 +52,7 @@ class SVNClient(BaseSCMClient):
     scmclient_id = 'svn'
     name = 'Subversion'
     server_tool_names = 'Subversion'
+    server_tool_ids = ['subversion']
 
     requires_diff_tool = True
 
diff --git a/rbtools/clients/tfs.py b/rbtools/clients/tfs.py
index 2b70fae3..7b02d3de 100644
--- a/rbtools/clients/tfs.py
+++ b/rbtools/clients/tfs.py
@@ -1321,6 +1321,7 @@ class TFSClient(BaseSCMClient):
     scmclient_id = 'tfs'
     name = 'Team Foundation Server'
     server_tool_names = 'Team Foundation Server'
+    server_tool_ids = ['tfs']
 
     requires_diff_tool = True
 
diff --git a/rbtools/commands/base/commands.py b/rbtools/commands/base/commands.py
index 3313a689..d4e20f61 100644
--- a/rbtools/commands/base/commands.py
+++ b/rbtools/commands/base/commands.py
@@ -939,7 +939,8 @@ def initialize(self) -> None:
                 api_root=self.api_root,
                 tool=tool,
                 repository_name=options.repository_name,
-                repository_paths=repository_info.path)
+                repository_paths=repository_info.path,
+                capabilities=self.capabilities)
             self.repository = repository
 
             if repository:
diff --git a/rbtools/commands/setup_repo.py b/rbtools/commands/setup_repo.py
index 615bc9e3..9905d1b5 100644
--- a/rbtools/commands/setup_repo.py
+++ b/rbtools/commands/setup_repo.py
@@ -1,14 +1,20 @@
 """Implementation of rbt setup-repo."""
 
+from __future__ import annotations
+
 import difflib
 import os
 import textwrap
+from typing import Optional, TYPE_CHECKING, Union
 
 from rbtools.commands.base import BaseCommand, CommandError
 from rbtools.config.loader import CONFIG_FILENAME
 from rbtools.utils.console import confirm, confirm_select
 from rbtools.utils.repository import get_repository_resource
 
+if TYPE_CHECKING:
+    from rbtools.api.resource import ItemResource, RootResource
+
 
 class SetupRepo(BaseCommand):
     """Configure a repository to point to a Review Board server.
@@ -38,22 +44,27 @@ class SetupRepo(BaseCommand):
         BaseCommand.tfs_options,
     ]
 
-    def prompt_rb_repository(self, local_tool_name, server_tool_names,
-                             repository_paths, api_root):
+    def prompt_rb_repository(
+        self,
+        local_tool_name: str,
+        server_tool_names: Optional[str],
+        repository_paths: Optional[Union[str, list[str]]],
+        api_root: RootResource,
+    ) -> Optional[ItemResource]:
         """Interactively prompt to select a matching repository.
 
         The user is prompted to choose a matching repository found on the
         Review Board server.
 
         Args:
-            local_tool_name (unicode):
+            local_tool_name (str):
                 The local name of the detected tool.
 
-            server_tool_names (unicode):
+            server_tool_names (str):
                 A comma-separated list of potentially matching SCMTool names in
                 the Review Board server.
 
-            repository_paths (list or unicode, optional):
+            repository_paths (list or str, optional):
                 A list of potential paths to match for the repository.
 
             api_root (rbtools.api.resource.RootResource):
@@ -192,9 +203,11 @@ def main(self, *args):
         while True:
             self.stdout.new_line()
             self.stdout.write('Current server: %s' % server)
+
+            tool_names = tool.get_server_tool_names(self.capabilities)
             selected_repo = self.prompt_rb_repository(
                 local_tool_name=tool.name,
-                server_tool_names=tool.server_tool_names,
+                server_tool_names=tool_names,
                 repository_paths=repository_info.path,
                 api_root=api_root)
 
diff --git a/rbtools/utils/repository.py b/rbtools/utils/repository.py
index d30d2cc0..d3ec280d 100644
--- a/rbtools/utils/repository.py
+++ b/rbtools/utils/repository.py
@@ -1,17 +1,34 @@
 """Utility functions for working with repositories."""
 
+from __future__ import annotations
+
+from typing import Optional, TYPE_CHECKING, Union
+
 from rbtools.api.errors import APIError
 
+if TYPE_CHECKING:
+    from rbtools.api.capabilities import Capabilities
+    from rbtools.api.resource import ItemResource, RootResource
+    from rbtools.clients.base.repository import RepositoryInfo
+    from rbtools.clients.base.scmclient import BaseSCMClient
+
 
-def get_repository_resource(api_root,
-                            tool=None,
-                            repository_name=None,
-                            repository_paths=None):
+def get_repository_resource(
+    api_root: RootResource,
+    tool: Optional[BaseSCMClient] = None,
+    repository_name: Optional[str] = None,
+    repository_paths: Optional[Union[str, list[str]]] = None,
+    capabilities: Optional[Capabilities] = None,
+) -> tuple[Optional[ItemResource], Optional[ItemResource]]:
     """Return the API resource for the matching repository on the server.
 
     Version Added:
         3.0
 
+    Version Changed:
+        5.0.1:
+        Added the ``capabilities`` argument.
+
     Args:
         api_root (rbtools.api.resource.RootResource):
             The root resource for the API.
@@ -19,12 +36,15 @@ def get_repository_resource(api_root,
         tool (rbtools.clients.base.BaseSCMClient, optional):
             The SCM client corresponding to the local working directory.
 
-        repository_name (unicode, optional):
+        repository_name (str, optional):
             An explicit repository name provided by the local configuration.
 
-        repository_paths (list or unicode, optional):
+        repository_paths (list or str, optional):
             A list of potential paths to match for the repository.
 
+        capabilities (rbtools.api.capabilities.Capabilities, optional):
+            The capabilities fetched from the server.
+
     Returns:
         tuple of rbtools.api.resource.ItemResource:
         A 2-tuple of :py:class:`~rbtools.api.resource.ItemResource`. The first
@@ -47,8 +67,11 @@ def _get_info(repository):
         'only_links': 'info,diff_file_attachments',
     }
 
-    if tool and tool.server_tool_names:
-        query['tool'] = tool.server_tool_names
+    if tool:
+        server_tool_names = tool.get_server_tool_names(capabilities)
+
+        if server_tool_names:
+            query['tool'] = server_tool_names
 
     if repository_name:
         query['name'] = repository_name
@@ -70,9 +93,12 @@ def _get_info(repository):
     # configured path than the client. In that case, we want to try again
     # without filtering by path, and ask each tool to match based on other
     # conditions.
-    query.pop('path', None)
+    if 'path' in query:
+        query.pop('path', None)
 
-    all_repositories = api_root.get_repositories(**query)
+        all_repositories = api_root.get_repositories(**query)
+    else:
+        all_repositories = repositories
 
     if all_repositories.total_results > 0 and tool:
         repository, info = tool.find_matching_server_repository(
@@ -93,7 +119,11 @@ def _get_info(repository):
     return None, None
 
 
-def get_repository_id(repository_info, api_root, repository_name=None):
+def get_repository_id(
+    repository_info: RepositoryInfo,
+    api_root: RootResource,
+    repository_name: Optional[str] = None,
+) -> Optional[int]:
     """Return the ID of a repository from the server.
 
     This will look up all accessible repositories on the server and try to
@@ -106,18 +136,18 @@ def get_repository_id(repository_info, api_root, repository_name=None):
         api_root (rbtools.api.resource.RootResource):
             The root resource for the API.
 
-        repository_name (unicode, optional):
+        repository_name (str, optional):
             An explicit repository name provided by local configuration.
 
     Returns:
         int:
         The ID of the repository, or ``None`` if not found.
     """
-    repository, info = get_repository_resource(
+    repository = get_repository_resource(
         api_root,
         tool=None,
         repository_name=repository_name,
-        repository_paths=repository_info.path)
+        repository_paths=repository_info.path)[0]
 
     if repository:
         return repository.id