diff --git a/pr_split/cli.py b/pr_split/cli.py index ad690a4..dff9a39 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -42,6 +42,7 @@ materialize_group_files, merge_chain_assignments, parse_diff, + target_file_modes, ) from .exceptions import ( ErrorMsg, @@ -357,12 +358,17 @@ def _create_single_branch_and_commit( add_worktree(worktree_path, branch_name, start_point or merge_base_ref) try: materialized = materialize_group_files(parsed_diff, group, merge_base_ref) + 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(): p.unlink() diff --git a/pr_split/diff_ops/__init__.py b/pr_split/diff_ops/__init__.py index d57fc55..42794c1 100644 --- a/pr_split/diff_ops/__init__.py +++ b/pr_split/diff_ops/__init__.py @@ -1,5 +1,5 @@ from .parser import ParsedDiff, extract_diff, parse_diff -from .reconstructor import materialize_group_files, merge_chain_assignments +from .reconstructor import materialize_group_files, merge_chain_assignments, target_file_modes __all__ = [ "ParsedDiff", @@ -7,4 +7,5 @@ "materialize_group_files", "merge_chain_assignments", "parse_diff", + "target_file_modes", ] diff --git a/pr_split/diff_ops/reconstructor.py b/pr_split/diff_ops/reconstructor.py index db37b06..607bb12 100644 --- a/pr_split/diff_ops/reconstructor.py +++ b/pr_split/diff_ops/reconstructor.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re import subprocess from loguru import logger @@ -164,3 +165,33 @@ def materialize_group_files( base_content = _get_base_file_content(file_path, ref) result[file_path] = apply_hunks(base_content, patch_file, indices) return result + + +_TARGET_MODE_RE = re.compile(r"^(?:new file mode|new mode) (\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. + """ + wanted = {assignment.file_path for assignment in group.assignments} + modes: dict[str, int] = {} + for patch_file in parsed_diff.patch_set: + # target_file, not is_removed_file: the latter is also true for a file + # truncated to empty, which still needs its mode applied. + if patch_file.path not in wanted or patch_file.target_file == "/dev/null": + continue + header = "".join(str(line) for line in patch_file.patch_info or []) + 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: + modes[patch_file.path] = mode + return modes diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 69c5820..2607a3d 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -892,3 +892,34 @@ def test_oversized_ids_are_recorded(self) -> None: groups = [self._sized("pr-1", 876), self._sized("pr-2", 400)] assert _oversized_group_ids(groups, 400) == ["pr-1"] + + +class TestWorktreeAppliesFileModes: + @patch("pr_split.cli.commit_files_in_dir", return_value="sha1") + @patch("pr_split.cli.target_file_modes", return_value={"run.sh": 0o100755}) + @patch("pr_split.cli.materialize_group_files", return_value={"run.sh": "x\n", "a.py": "y\n"}) + @patch("pr_split.cli.remove_worktree") + @patch("pr_split.cli.add_worktree") + def test_executable_bit_applied_only_where_diff_says( + self, + mock_add: MagicMock, + mock_remove: MagicMock, + mock_mat: MagicMock, + mock_modes: MagicMock, + mock_commit: MagicMock, + ) -> None: + import os + import stat + from pathlib import Path + + modes: dict[str, int] = {} + + def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> str: + for file_path in file_paths: + modes[file_path] = stat.S_IMODE(os.stat(Path(cwd) / file_path).st_mode) + return "sha1" + + mock_commit.side_effect = capture + _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 diff --git a/tests/test_reconstructor.py b/tests/test_reconstructor.py index b0ac6f4..75343df 100644 --- a/tests/test_reconstructor.py +++ b/tests/test_reconstructor.py @@ -13,6 +13,7 @@ materialize_group_files, merge_chain_assignments, split_git_lines, + target_file_modes, ) from pr_split.exceptions import GitOperationError from pr_split.schemas import Group, GroupAssignment @@ -659,3 +660,68 @@ def test_real_deletion_still_removes(self) -> None: ], ) assert materialize_group_files(parsed, group, "main") == {"a.txt": None} + + +class TestTargetFileModes: + def _group(self, *paths: str) -> Group: + return Group( + id="pr-1", + title="t", + description="d", + assignments=[ + GroupAssignment( + file_path=p, assignment_type=AssignmentType.WHOLE_FILE, hunk_indices=[0] + ) + for p in paths + ], + ) + + def test_mode_change_with_content_is_reported(self) -> None: + parsed = parse_diff( + "diff --git a/run.sh b/run.sh\nold mode 100644\nnew mode 100755\n" + "index 422c2b7..55dce13\n--- a/run.sh\n+++ b/run.sh\n@@ -1,2 +1,2 @@\n a\n-b\n+B\n" + ) + assert target_file_modes(parsed, self._group("run.sh")) == {"run.sh": 0o100755} + + def test_new_executable_file_is_reported(self) -> None: + parsed = parse_diff( + "diff --git a/n.sh b/n.sh\nnew file mode 100755\n--- /dev/null\n+++ b/n.sh\n" + "@@ -0,0 +1 @@\n+x\n" + ) + assert target_file_modes(parsed, self._group("n.sh")) == {"n.sh": 0o100755} + + def test_plain_change_and_unassigned_files_are_ignored(self) -> None: + parsed = parse_diff( + "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -1 +1 @@\n-x\n+y\n" + "diff --git a/b.sh b/b.sh\nold mode 100644\nnew mode 100755\n" + "--- a/b.sh\n+++ b/b.sh\n@@ -1 +1 @@\n-x\n+y\n" + ) + assert target_file_modes(parsed, self._group("a.py")) == {} + + def test_removed_file_is_ignored(self) -> None: + parsed = parse_diff( + "diff --git a/d.sh b/d.sh\ndeleted file mode 100755\n--- a/d.sh\n+++ /dev/null\n" + "@@ -1 +0,0 @@\n-x\n" + ) + assert target_file_modes(parsed, self._group("d.sh")) == {} + + def test_symlink_mode_is_not_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")) == {} + + def test_mode_change_back_to_non_executable_is_reported(self) -> None: + parsed = parse_diff( + "diff --git a/run.sh b/run.sh\nold mode 100755\nnew mode 100644\n" + "--- a/run.sh\n+++ b/run.sh\n@@ -1 +1 @@\n-x\n+y\n" + ) + assert target_file_modes(parsed, self._group("run.sh")) == {"run.sh": 0o100644} + + def test_file_truncated_to_empty_keeps_its_new_mode(self) -> None: + parsed = parse_diff( + "diff --git a/a.sh b/a.sh\nold mode 100644\nnew mode 100755\n" + "--- a/a.sh\n+++ b/a.sh\n@@ -1,2 +0,0 @@\n-x\n-y\n" + ) + assert target_file_modes(parsed, self._group("a.sh")) == {"a.sh": 0o100755}