fix: locate pre-commit config edits with exact source marks (#64)

src/sync_pre_commit_lock/pre_commit_config.py

@@ -1,8 +1,8 @@
 from __future__ import annotations
 
-import difflib
 from dataclasses import dataclass, field
 from functools import cached_property
+from importlib.metadata import version
 from typing import TYPE_CHECKING, Any
 
 import strictyaml as yaml
@@ -43,6 +43,27 @@
 )
 
 
+def _round_trip_load(raw: str) -> Any:
+    """Parse ``raw`` with a ruamel round-trip loader, whose nodes carry ``lc`` source marks.
+
+    strictyaml vendors ruamel, so this normally costs no extra dependency. A strictyaml
+    that stops vendoring it still works if ruamel.yaml is installed on its own.
+    """
+    try:
+        from strictyaml.ruamel import YAML  # type: ignore[import-untyped]
+    except ImportError:
+        try:
+            from ruamel.yaml import YAML  # type: ignore[import-not-found]
+        except ImportError as exc:
+            msg = (
+                "Updating .pre-commit-config.yaml needs a ruamel round-trip parser, which"
+                f" strictyaml {version('strictyaml')} does not vendor. Install ruamel.yaml,"
+                " or pin strictyaml<2."
+            )
+            raise RuntimeError(msg) from exc
+    return YAML().load(raw)
+
+
 @dataclass(frozen=True)
 class PreCommitHook:
     id: str
@@ -136,29 +157,23 @@ def repos_normalized(self) -> set[PreCommitRepo]:
         }
 
     @cached_property
-    def document_start_offset(self) -> int:
-        """Return the line number where the YAML document starts."""
-        lines = self.raw_file_contents.split("\n")
-        for i, line in enumerate(lines):
-            # Trim leading/trailing whitespaces
-            line = line.rstrip()
-            # Skip if line is a comment or empty/whitespace
-            if line.startswith("#") or line == "":
-                continue
-            # If line is '---', return line number + 1
-            if line == "---":
-                return i + 1
-        return 0
+    def repo_marks(self) -> Any:
+        """Exact source positions for each repo entry, from a ruamel round-trip parse.
+
+        strictyaml's own ``end_line`` counts logical nodes, so it drifts past any
+        multi-line flow sequence (see #64). ruamel's ``lc`` marks come from the lexer
+        and stay exact, document separator and comments included.
+        """
+        return _round_trip_load(self.raw_file_contents)["repos"]
 
     def update_pre_commit_repo_versions(self, new_versions: dict[PreCommitRepo, PreCommitRepo]) -> None:
         """Fix the pre-commit hooks to match the lockfile. Preserve comments and formatting as much as possible."""
         if len(new_versions) == 0:
             return
 
-        original_lines = self.original_file_lines
-        updated_lines = original_lines[:]
+        edits: list[tuple[int, int, str, str]] = []
 
-        for repo_rev in self.yaml["repos"]:
+        for repo_rev, marks in zip(self.yaml["repos"], self.repo_marks):
             if "rev" not in repo_rev:
                 continue
 
@@ -174,31 +189,36 @@ def update_pre_commit_repo_versions(self, new_versions: dict[PreCommitRepo, PreC
             if not (updated_repo := new_versions.get(normalized_repo)):
                 continue
 
-            rev_line_number: int = rev.end_line + self.document_start_offset
-            rev_line_idx: int = rev_line_number - 1
-            original_rev_line: str = updated_lines[rev_line_idx]
-            updated_lines[rev_line_idx] = original_rev_line.replace(str(rev), updated_repo.rev)
+            if str(rev) != updated_repo.rev:
+                _, _, rev_line, rev_col = marks.lc.data["rev"]
+                edits.append((rev_line, rev_col, str(rev), updated_repo.rev))
 
-            for src_hook, old_hook, new_hook in zip(hooks, normalized_repo.hooks, updated_repo.hooks):
+            for hook_marks, old_hook, new_hook in zip(
+                marks.get("hooks", ()), normalized_repo.hooks, updated_repo.hooks
+            ):
                 if new_hook == old_hook:
                     continue
-                for src_dep, old_dep, new_dep in zip(
-                    src_hook.get("additional_dependencies", []),
-                    old_hook.additional_dependencies,
-                    new_hook.additional_dependencies,
+                for i, (old_dep, new_dep) in enumerate(
+                    zip(old_hook.additional_dependencies, new_hook.additional_dependencies)
                 ):
                     if old_dep == new_dep:
                         continue
-                    dep_line_number: int = src_dep.end_line + self.document_start_offset
-                    dep_line_idx: int = dep_line_number - 1
-                    original_dep_line: str = updated_lines[dep_line_idx]
-                    updated_lines[dep_line_idx] = original_dep_line.replace(str(src_dep), new_dep)
-
-        changes = difflib.ndiff(original_lines, updated_lines)
-        change_count = sum(1 for change in changes if change[0] in ["+", "-"])
+                    dep_line, dep_col = hook_marks["additional_dependencies"].lc.data[i]
+                    edits.append((dep_line, dep_col, old_dep, new_dep))
 
-        if change_count == 0:
+        if not edits:
             msg = "No changes to write, this should not happen"
             raise RuntimeError(msg)
+
+        updated_lines = self.original_file_lines[:]
+        # Rightmost edit first: replacing an earlier scalar on the same line would shift
+        # every column after it, and those columns were measured against the original text.
+        for line_idx, col, old, new in sorted(edits, reverse=True):
+            line = updated_lines[line_idx]
+            if old not in line[col:]:
+                msg = f"Expected {old!r} at line {line_idx + 1}, column {col + 1}, found {line[col:].rstrip()!r}"
+                raise RuntimeError(msg)
+            updated_lines[line_idx] = line[:col] + line[col:].replace(old, new, 1)
+
         with self.pre_commit_config_file_path.open("w") as stream:
             stream.writelines(updated_lines)

tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.expected.yaml

@@ -0,0 +1,30 @@
+
+# Many unused lines before document separator
+
+---
+default_language_version:
+  python: python3.11
+
+repos:
+
+  # This multi-line flow sequence is what strictyaml miscounts, shifting every
+  # line reported below it (see #64).
+  - repo: https://github.com/python-jsonschema/check-jsonschema
+    rev: "0.36.1"
+    hooks:
+      - id: check-jsonschema
+        args: [
+            "--no-cache",
+            "--schemafile",
+            "https://example.invalid/schema.json",
+          ]
+
+  - repo: https://github.com/pre-commit/mirrors-mypy
+    # Some comment
+    rev: v1.5.0
+    hooks:
+      - id: mypy
+        additional_dependencies: [
+          types-PyYAML==1.2.4,
+          types-requests==3.4.5,
+        ]

tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.yaml

@@ -0,0 +1,30 @@
+
+# Many unused lines before document separator
+
+---
+default_language_version:
+  python: python3.11
+
+repos:
+
+  # This multi-line flow sequence is what strictyaml miscounts, shifting every
+  # line reported below it (see #64).
+  - repo: https://github.com/python-jsonschema/check-jsonschema
+    rev: "0.36.1"
+    hooks:
+      - id: check-jsonschema
+        args: [
+            "--no-cache",
+            "--schemafile",
+            "https://example.invalid/schema.json",
+          ]
+
+  - repo: https://github.com/pre-commit/mirrors-mypy
+    # Some comment
+    rev: v1.0.0
+    hooks:
+      - id: mypy
+        additional_dependencies: [
+          types-PyYAML==1.2.4,
+          types-requests,
+        ]

tests/test_pre_commit_config_file.py

@@ -1,11 +1,17 @@
+import builtins
 from pathlib import Path
 from unittest.mock import MagicMock, mock_open
 
 import pytest
 import yaml
 from strictyaml.exceptions import YAMLValidationError
 
-from sync_pre_commit_lock.pre_commit_config import PreCommitHook, PreCommitHookConfig, PreCommitRepo
+from sync_pre_commit_lock.pre_commit_config import (
+    PreCommitHook,
+    PreCommitHookConfig,
+    PreCommitRepo,
+    _round_trip_load,
+)
 
 
 def test_pre_commit_hook_config_initialization() -> None:
@@ -53,21 +59,6 @@ def test_repos_property() -> None:
 FIXTURES = Path(__file__).parent / "fixtures" / "sample_pre_commit_config"
 
 
-@pytest.mark.parametrize(
-    ("path", "offset"),
-    [
-        (FIXTURES / "pre-commit-config-document-separator.yaml", 4),
-        (FIXTURES / "pre-commit-config-start-empty-lines.yaml", 0),
-        (FIXTURES / "pre-commit-config-with-local.yaml", 2),
-        (FIXTURES / "pre-commit-config.yaml", 1),
-        (FIXTURES / "sample-django-stubs.yaml", 0),
-    ],
-)
-def test_files_offset(path: Path, offset: int) -> None:
-    config = PreCommitHookConfig.from_yaml_file(path)
-    assert config.document_start_offset == offset
-
-
 def test_update_versions() -> None:
     config = PreCommitHookConfig.from_yaml_file(FIXTURES / "pre-commit-config-document-separator.yaml")
     config.pre_commit_config_file_path = MagicMock()
@@ -87,13 +78,91 @@ def test_update_versions() -> None:
         assert config.pre_commit_config_file_path.open.call_count == 1
 
 
-@pytest.mark.parametrize("base", ["only-deps", "with-deps", "with-one-liner-deps", "without-new-deps"])
+@pytest.mark.parametrize(
+    "name",
+    [
+        "pre-commit-config.yaml",
+        "pre-commit-config-document-separator.yaml",
+        "pre-commit-config-start-empty-lines.yaml",
+        "pre-commit-config-with-local.yaml",
+        "sample-django-stubs.yaml",
+    ],
+)
+def test_update_versions_rewrites_only_the_rev(name: str) -> None:
+    """Whatever precedes the rev -- blank lines, comments, a `---` separator, a local repo
+    with no rev -- the bump must land on that rev and change nothing else (#64).
+    """
+    path = FIXTURES / name
+    config = PreCommitHookConfig.from_yaml_file(path)
+    mock_file = config.pre_commit_config_file_path = MagicMock()
+    mock_file.open = mock_open()
+
+    initial_repo = config.repos[0]
+    config.update_pre_commit_repo_versions(
+        {initial_repo: PreCommitRepo(initial_repo.repo, "99.99.99", initial_repo.hooks)}
+    )
+
+    written = "".join(mock_file.open().writelines.call_args[0][0])
+    assert "99.99.99" in written
+    assert written.replace("99.99.99", initial_repo.rev) == path.read_text()
+
+
+def test_round_trip_load_explains_itself_without_a_ruamel_parser(monkeypatch: pytest.MonkeyPatch) -> None:
+    """strictyaml vendors ruamel today; if a future one stops, say what to install."""
+    real_import = builtins.__import__
+
+    def no_ruamel(name: str, *args: object, **kwargs: object) -> object:
+        if name in {"strictyaml.ruamel", "ruamel.yaml"}:
+            raise ImportError(name)
+        return real_import(name, *args, **kwargs)  # type: ignore[arg-type]
+
+    monkeypatch.setattr(builtins, "__import__", no_ruamel)
+
+    with pytest.raises(RuntimeError, match="Install ruamel.yaml"):
+        _round_trip_load("repos: []")
+
+
+def test_update_deps_sharing_a_line_when_the_first_one_shrinks() -> None:
+    """A shorter replacement shifts every column after it on that line, so edits must be
+    applied rightmost-first or the later dependency is silently left untouched.
+    """
+    file_content = """\
+repos:
+  - repo: https://github.com/pre-commit/mirrors-mypy
+    rev: v1.0.0
+    hooks:
+      - id: mypy
+        additional_dependencies: [types-PyYAML==6.0.12.20240311, types-requests==2.31.0]
+"""
+    mock_path = MagicMock(spec=Path)
+    mock_path.open = mock_open(read_data=file_content)
+    config = PreCommitHookConfig.from_yaml_file(mock_path)
+
+    initial_repo = config.repos[0]
+    mock_path.open = mock_open()
+    config.update_pre_commit_repo_versions(
+        {
+            initial_repo: PreCommitRepo(
+                initial_repo.repo,
+                initial_repo.rev,
+                [PreCommitHook("mypy", ["types-PyYAML==6.0.12", "types-requests==2.32.0"])],
+            )
+        }
+    )
+
+    written = "".join(mock_path.open().writelines.call_args[0][0])
+    assert "additional_dependencies: [types-PyYAML==6.0.12, types-requests==2.32.0]" in written
+
+
+@pytest.mark.parametrize(
+    "base", ["only-deps", "with-deps", "with-one-liner-deps", "without-new-deps", "flow-multiline-deps"]
+)
 def test_update_additional_dependencies_versions(base: str) -> None:
     config = PreCommitHookConfig.from_yaml_file(FIXTURES / f"pre-commit-config-{base}.yaml")
     mock_file = config.pre_commit_config_file_path = MagicMock()
     mock_file.open = mock_open()
 
-    initial_repo = config.repos[0]
+    initial_repo = next(repo for repo in config.repos if repo.repo.endswith("/mirrors-mypy"))
     updated_repo = PreCommitRepo(
         "https://github.com/pre-commit/mirrors-mypy",
         "v1.5.0",