SERVER-99680: Fix legacy_comands_check.py to take inspect line-differences rather than file-differences (#31459)

GitOrigin-RevId: cd71349f51a46c6a8e18606b170fe324f7762fef

Author: davidchen2001

Diff for: buildscripts/legacy_commands_check.py

@@ -4,14 +4,14 @@
 import os
 import re
 import sys
-from typing import Iterable
+from typing import List, Tuple
 
 # Get relative imports to work when the package is not installed on the PYTHONPATH.
 if __name__ == "__main__" and __package__ is None:
     sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(os.path.realpath(__file__)))))
 
 # pylint: disable=wrong-import-position
-from buildscripts.linter.filediff import gather_changed_files_for_lint
+from buildscripts.linter.filediff import gather_changed_files_with_lines
 
 LEGACY_TYPES = [
     "public Command {",
@@ -22,14 +22,15 @@
 ]
 
 
-def check_file_for_legacy_type(file_contents: Iterable[str]) -> bool:
+def check_file_for_legacy_type(modified_lines: List[Tuple[int, str]]) -> bool:
     """Return false if a file defines a legacy command."""
 
     file_has_legacy_type = False
 
-    for i, line in enumerate(file_contents):
-        if any(legacy_type in line for legacy_type in LEGACY_TYPES):
-            print(f"On line {i}: {line.strip()}")
+    for i in range(len(modified_lines)):
+        modified_line = modified_lines[i][1]
+        if any(legacy_type in modified_line for legacy_type in LEGACY_TYPES):
+            print(f"On line {i}: {modified_line.strip()}")
             file_has_legacy_type = True
 
     return file_has_legacy_type
@@ -54,19 +55,18 @@ def main():
     default_dir = os.getenv("BUILD_WORKSPACE_DIRECTORY", ".")
     os.chdir(default_dir)
 
-    files = gather_changed_files_for_lint(is_interesting_file)
+    files = gather_changed_files_with_lines(is_interesting_file)
     found_legacy_command = False
 
     for file in files:
-        with open(file, "r") as search_file:
-            hasLegacyCommandDefinition = check_file_for_legacy_type(search_file)
-
-            if hasLegacyCommandDefinition:
-                found_legacy_command = True
-                print(
-                    file
-                    + " contains a legacy command type definition. Please define a TypedCommand instead."
-                )
+        hasLegacyCommandDefinition = check_file_for_legacy_type(files[file])
+
+        if hasLegacyCommandDefinition:
+            found_legacy_command = True
+            print(
+                file
+                + " contains a legacy command type definition. Please define a TypedCommand instead."
+            )
 
     if found_legacy_command:
         sys.exit(1)

Diff for: buildscripts/linter/filediff.py

@@ -2,7 +2,7 @@
 
 import os
 import sys
-from typing import Callable, List, Tuple
+from typing import Callable, Dict, List, Tuple
 
 import structlog
 from git import Repo
@@ -16,6 +16,7 @@
 from buildscripts.patch_builds.change_data import (
     RevisionMap,
     find_changed_files_in_repos,
+    find_modified_lines_for_files_in_repos,
     generate_revision_map,
 )
 
@@ -66,3 +67,19 @@ def gather_changed_files_for_lint(is_interesting_file: Callable[[str], bool]) ->
 
     LOGGER.info("Found files to lint", files=files)
     return files
+
+
+def gather_changed_files_with_lines(
+    is_interesting_file: Callable[[str], bool],
+) -> Dict[str, List[Tuple[int, str]]]:
+    """
+    Get the files that have changes since the last git commit, along with details of the specific lines that have changed.
+
+    :param is_interesting_file: Filter for whether a file should be returned.
+    :return: Dictionary mapping each changed file to a list of tuples, where each tuple contains the modified line number and its content.
+    """
+    repos, revision_map = _get_repos_and_revisions()
+    changed_files = find_changed_files_in_repos(repos, revision_map)
+
+    filtered_files = [f for f in changed_files if is_interesting_file(f)]
+    return find_modified_lines_for_files_in_repos(repos, filtered_files, revision_map)

Diff for: buildscripts/patch_builds/change_data.py

@@ -2,7 +2,7 @@
 
 import os
 from itertools import chain
-from typing import Any, Dict, Iterable, List, Optional, Set
+from typing import Any, Dict, Iterable, List, Optional, Set, Tuple
 
 import structlog
 from git import DiffIndex, Repo
@@ -115,3 +115,68 @@ def find_changed_files_in_repos(
     :return: Set of changed files.
     """
     return set(chain.from_iterable([find_changed_files(repo, revision_map) for repo in repos]))
+
+
+def find_modified_lines_for_files(
+    repo: Repo, changed_files: List[str], revision_map: Optional[RevisionMap] = None
+) -> Dict[str, List[Tuple[int, str]]]:
+    """
+    For each changed file, determine which lines were modified.
+
+    :param repo: Git repository.
+    :param changed_files: List of file paths that have changed.
+    :param revision_map: Map of revisions to compare against for repos.
+    :return: Dictionary mapping files to lists of modified line numbers.
+    """
+    if not revision_map:
+        revision_map = {}
+
+    modified_lines_and_content = {}
+    compare_commit = revision_map.get(repo.git_dir, repo.head.commit)
+    for file_path in changed_files:
+        diff = repo.git.diff(compare_commit, "--", file_path, unified=0)
+        line_modifications = []
+        # Read file contents
+        with open(file_path, "r") as file:
+            lines = file.readlines()
+
+        # Find line numbers and respective content of lines with modifications in git diff and store in a list of tuples
+        for line in diff.splitlines():
+            if not (line.startswith("@@")):
+                continue
+            parts = line.split(" ")
+            for part in parts:
+                if not (part.startswith("+")):
+                    continue
+                start_line_count = part[1:]
+                if "," in start_line_count:
+                    start_line, count = map(int, start_line_count.split(","))
+                    for i in range(start_line, start_line + count):
+                        if i <= len(lines):
+                            line_modifications.append((i, lines[i - 1].rstrip()))
+                else:
+                    start_line = int(start_line_count)
+                    if start_line <= len(lines):
+                        line_modifications.append((start_line, lines[start_line - 1].rstrip()))
+            modified_lines_and_content[file_path] = line_modifications
+
+    return modified_lines_and_content
+
+
+def find_modified_lines_for_files_in_repos(
+    repos: Iterable[Repo], changed_files: List[str], revision_map: Optional[RevisionMap] = None
+) -> Dict[str, List[Tuple[int, str]]]:
+    """
+    Find the modified lines in files with changes.
+
+    :param repos: List of repos containing changed files.
+    :param revision_map: Map of revisions to compare against for repos.
+    :return: Dictionary mapping each changed file to a list of tuples, where each tuple contains the modified line number and its content.
+    """
+    modified_files_with_lines = {}
+    for repo in repos:
+        modified_files_with_lines.update(
+            find_modified_lines_for_files(repo, changed_files, revision_map)
+        )
+
+    return modified_files_with_lines

Diff for: buildscripts/tests/patch_builds/test_change_data.py

@@ -5,12 +5,14 @@
 import os
 import unittest
 
-from mock import MagicMock, patch
+from mock import MagicMock, mock_open, patch
 
 import buildscripts.patch_builds.change_data as under_test
 
 NS = "buildscripts.patch_builds.change_data"
 
+FILE_CONTENT = "Hello, Universe!\n"
+
 
 def ns(relative_name):  # pylint: disable=invalid-name
     """Return a full name from a name relative to the test module"s name space."""
@@ -71,3 +73,41 @@ def test_missing_repos_are_not_returned(self):
 
         self.assertEqual(revision_map[mock_repo_list[0].git_dir], revision_data["mongo"])
         self.assertEqual(len(revision_map), 1)
+
+
+class TestFindChangedFilesAndLinesInRepos(unittest.TestCase):
+    @patch("builtins.open", new_callable=mock_open, read_data=FILE_CONTENT)
+    def test_detects_file_and_line_modifications_no_change(self, mock_open):
+        repo_mock = MagicMock()
+        repo_mock.git.diff.return_value = ""
+
+        changed_files = ["src/module/test1.cpp"]
+        revision_map = {}
+
+        result = under_test.find_modified_lines_for_files(repo_mock, changed_files, revision_map)
+
+        expected_result = {}
+
+        mock_open.assert_called_once_with("src/module/test1.cpp", "r")
+
+        self.assertEqual(result, expected_result)
+
+    @patch("builtins.open", new_callable=mock_open, read_data=FILE_CONTENT)
+    def test_detects_file_and_line_modifications_one_file(self, mock_open):
+        repo_mock = MagicMock()
+        repo_mock.git.diff.return_value = """\
+@@ -1,2 +1,2 @@
+-Hello, World!
++Hello, Universe!
+ """
+
+        changed_files = ["src/module/test1.cpp"]
+        revision_map = {}
+
+        result = under_test.find_modified_lines_for_files(repo_mock, changed_files, revision_map)
+
+        expected_result = {"src/module/test1.cpp": [(1, "Hello, Universe!")]}
+
+        mock_open.assert_called_once_with("src/module/test1.cpp", "r")
+
+        self.assertEqual(result, expected_result)

Diff for: buildscripts/tests/test_legacy_commands_check.py

@@ -13,80 +13,56 @@ def create_file_iterator(file_contents: str) -> Iterable[str]:
 
 class TestCheckFileForLegacyType(unittest.TestCase):
     def test_typed_command(self):
-        content = """ 
-        line 0
-        class AddShardCmd : public TypedCommand
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), False)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public TypedCommand")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), False)
 
     def test_command(self):
-        content = """ 
-        line 0
-        class AddShardCmd : public Command {
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), True)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public Command {")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), True)
 
     def test_basic_command(self):
-        content = """ 
-        line 0
-        class AddShardCmd : public BasicCommand
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), True)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public BasicCommand")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), True)
 
     def test_basic_command_with_reply_builder_interface(self):
-        content = """ 
-        line 0
-        class AddShardCmd : public BasicCommandWithReplyBuilderInterface
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), True)
+        modified_lines = [
+            (0, ""),
+            (1, "class AddShardCmd : public BasicCommandWithReplyBuilderInterface"),
+        ]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), True)
 
     def test_basic_command_with_request_parser(self):
-        content = """ 
-        line 0
-        class AddShardCmd : public BasicCommandWithRequestParser
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), True)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public BasicCommandWithRequestParser")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), True)
 
     def test_errmsg_command(self):
-        content = """ 
-        line 0
-        class AddShardCmd : public ErrmsgCommandDeprecated
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), True)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public ErrmsgCommandDeprecated")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), True)
 
     def test_generic_command_inherited(self):
         # This might appear in non-legacy command types and should not be mistake for a Command type
 
-        content = """ 
-        line 0
-        public Command
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), False)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public Command")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), False)
 
     def test_kCommand(self):
         # This log statement appears in many Command files for logging purposes and should not be
         # mistaken for a Command type
 
-        content = """ 
-        line 0
-        #define MONGO_LOGV2_DEFAULT_COMPONENT ::mongo::logv2::LogComponent::kCommand
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), False)
+        modified_lines = [
+            (0, ""),
+            (1, "#define MONGO_LOGV2_DEFAULT_COMPONENT ::mongo::logv2::LogComponent::kCommand"),
+        ]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), False)
 
     def test_command_invocation(self):
         # This class is not a Command type and should not be mistaken for a Command type
 
-        content = """ 
-        line 0
-        class AddShardCmd : public CommandInvocation
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), False)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public CommandInvocation")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), False)
 
     def test_command_invocation_hooks(self):
         # This class is not a Command type and should not be mistaken for a Command type
 
-        content = """ 
-        line 0
-        class AddShardCmd : public CommandInvocationHooks
-        """
-        self.assertEqual(check_file_for_legacy_type(create_file_iterator(content)), False)
+        modified_lines = [(0, ""), (1, "class AddShardCmd : public CommandInvocationHooks")]
+        self.assertEqual(check_file_for_legacy_type(modified_lines), False)