diff --git a/pr_split/cli.py b/pr_split/cli.py index dff9a39..394420e 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -2,6 +2,7 @@ import json as json_mod import shutil +import stat import tempfile import time import urllib.request @@ -361,16 +362,30 @@ def _create_single_branch_and_commit( modes = target_file_modes(parsed_diff, group) for file_path, content in materialized.items(): p = Path(worktree_path) / file_path - if content is not None: - p.parent.mkdir(parents=True, exist_ok=True) - # newline="" keeps CRLF from the reconstructed content intact. - p.write_text(content, encoding="utf-8", errors="surrogateescape", newline="") - if file_path in modes: - # Git only tracks the executable bit; apply the diff's - # target mode so chmod changes reach the sub-PR. - p.chmod(modes[file_path] & 0o777) - elif p.exists(): + mode = modes.get(file_path) + if content is None: + # A dangling symlink is not "exists()" but must still go. + if p.is_symlink() or p.exists(): + p.unlink() + continue + p.parent.mkdir(parents=True, exist_ok=True) + if p.is_symlink(): + # Never write through an existing link (it would modify the + # target file); replace the link itself. p.unlink() + if mode is not None and stat.S_ISLNK(mode): + # Git stores a symlink's target as the blob content. A regular + # file being converted to a link is still on disk here. + if p.is_symlink() or p.exists(): + p.unlink() + p.symlink_to(content.rstrip("\n")) + continue + # newline="" keeps CRLF from the reconstructed content intact. + p.write_text(content, encoding="utf-8", errors="surrogateescape", newline="") + if mode is not None: + # Git only tracks the executable bit; apply the diff's target + # mode so chmod changes reach the sub-PR. + p.chmod(mode & 0o777) logger.info(logs.COMMITTING_GROUP.format(group=group.id, title=group.title)) commit_sha = commit_files_in_dir( diff --git a/pr_split/diff_ops/reconstructor.py b/pr_split/diff_ops/reconstructor.py index 607bb12..12e65d2 100644 --- a/pr_split/diff_ops/reconstructor.py +++ b/pr_split/diff_ops/reconstructor.py @@ -1,6 +1,7 @@ from __future__ import annotations import re +import stat import subprocess from loguru import logger @@ -167,15 +168,22 @@ def materialize_group_files( return result -_TARGET_MODE_RE = re.compile(r"^(?:new file mode|new mode) (\d{6})$", re.MULTILINE) +# "new file mode"/"new mode" appear when the mode changes; an unchanged mode +# is only visible on the "index .. " line. +_TARGET_MODE_RE = re.compile( + r"^(?:new file mode|new mode) (\d{6})$|^index [0-9a-f]+\.\.[0-9a-f]+ (\d{6})$", + re.MULTILINE, +) def target_file_modes(parsed_diff: ParsedDiff, group: Group) -> dict[str, int]: """Map each file the group materializes to the mode the diff gives it. - unidiff parses ``old mode``/``new mode``/``new file mode`` headers into - ``patch_info`` only; nothing else applies them, so without this a - ``chmod +x`` that comes with a content change silently loses the bit. + unidiff parses ``old mode``/``new mode``/``new file mode`` and ``index`` + headers into ``patch_info`` only; nothing else applies them, so without + this a ``chmod +x`` that comes with a content change silently loses the + bit and a symlink is written as a regular file. Regular files (100644, + 100755) and symlinks (120000) are reported. """ wanted = {assignment.file_path for assignment in group.assignments} modes: dict[str, int] = {} @@ -188,10 +196,7 @@ def target_file_modes(parsed_diff: ParsedDiff, group: Group) -> dict[str, int]: match = _TARGET_MODE_RE.search(header) if not match: continue - mode = int(match.group(1), 8) - # Only regular files (100644 / 100755): a symlink's 120000 would mask - # to 000 and make the written file unreadable. Symlinks are written as - # plain files by the reconstructor, unchanged from before. - if mode & 0o170000 == 0o100000: + mode = int(match.group(1) or match.group(2), 8) + if stat.S_ISREG(mode) or stat.S_ISLNK(mode): modes[patch_file.path] = mode return modes diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 2607a3d..39feb14 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -923,3 +923,65 @@ def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") assert modes["run.sh"] & stat.S_IXUSR assert not modes["a.py"] & stat.S_IXUSR + + +class TestWorktreeMaterializesSymlinks: + def _run( + self, materialized: dict[str, str | None], modes: dict[str, int] + ) -> dict[str, object]: + import os + from pathlib import Path + + seen: dict[str, object] = {} + + def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> str: + for file_path in file_paths: + p = Path(cwd) / file_path + seen[file_path] = ( + os.readlink(p) if p.is_symlink() else ("missing" if not p.exists() else "file") + ) + return "sha1" + + with ( + patch("pr_split.cli.add_worktree"), + patch("pr_split.cli.remove_worktree"), + patch("pr_split.cli.materialize_group_files", return_value=materialized), + patch("pr_split.cli.target_file_modes", return_value=modes), + patch("pr_split.cli.commit_files_in_dir", side_effect=capture), + ): + _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") + return seen + + def test_new_symlink_is_created_as_a_link(self) -> None: + seen = self._run({"link": "other.py\n", "other.py": "x\n"}, {"link": 0o120000}) + assert seen["link"] == "other.py" + assert seen["other.py"] == "file" + + def test_symlink_without_mode_info_stays_a_plain_file(self) -> None: + seen = self._run({"link": "other.py\n"}, {}) + assert seen["link"] == "file" + + def test_regular_file_converted_to_symlink(self) -> None: + import os + from pathlib import Path + + seen: dict[str, object] = {} + + def pre_populate(path: str, branch: str, start: str) -> None: + Path(path).mkdir(parents=True, exist_ok=True) + (Path(path) / "toref").write_text("old regular content\n") + + def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> str: + p = Path(cwd) / "toref" + seen["toref"] = os.readlink(p) if p.is_symlink() else "file" + return "sha1" + + with ( + patch("pr_split.cli.add_worktree", side_effect=pre_populate), + patch("pr_split.cli.remove_worktree"), + patch("pr_split.cli.materialize_group_files", return_value={"toref": "a.py\n"}), + patch("pr_split.cli.target_file_modes", return_value={"toref": 0o120000}), + patch("pr_split.cli.commit_files_in_dir", side_effect=capture), + ): + _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") + assert seen["toref"] == "a.py" diff --git a/tests/test_reconstructor.py b/tests/test_reconstructor.py index 75343df..fd13eef 100644 --- a/tests/test_reconstructor.py +++ b/tests/test_reconstructor.py @@ -705,12 +705,27 @@ def test_removed_file_is_ignored(self) -> None: ) assert target_file_modes(parsed, self._group("d.sh")) == {} - def test_symlink_mode_is_not_reported(self) -> None: + def test_new_symlink_mode_is_reported(self) -> None: parsed = parse_diff( "diff --git a/link b/link\nnew file mode 120000\n--- /dev/null\n+++ b/link\n" "@@ -0,0 +1 @@\n+other.py\n\\ No newline at end of file\n" ) - assert target_file_modes(parsed, self._group("link")) == {} + assert target_file_modes(parsed, self._group("link")) == {"link": 0o120000} + + def test_retargeted_symlink_mode_comes_from_index_line(self) -> None: + parsed = parse_diff( + "diff --git a/link b/link\nindex d90d485..5f35558 120000\n--- a/link\n+++ b/link\n" + "@@ -1 +1 @@\n-other.py\n\\ No newline at end of file\n" + "+missing.py\n\\ No newline at end of file\n" + ) + assert target_file_modes(parsed, self._group("link")) == {"link": 0o120000} + + def test_unchanged_regular_mode_from_index_line(self) -> None: + parsed = parse_diff( + "diff --git a/a.py b/a.py\nindex 1111111..2222222 100644\n--- a/a.py\n+++ b/a.py\n" + "@@ -1 +1 @@\n-x\n+y\n" + ) + assert target_file_modes(parsed, self._group("a.py")) == {"a.py": 0o100644} def test_mode_change_back_to_non_executable_is_reported(self) -> None: parsed = parse_diff(