Skip to content

fix: materialize symlinks as links instead of regular files - #91

Closed
vitali87 wants to merge 3 commits into
fix/apply-file-modesfrom
fix/materialize-symlinks
Closed

vitali87 wants to merge 3 commits into
fix/apply-file-modesfrom
fix/materialize-symlinks

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Problem

A symlink in the diff — new file mode 120000, or index a..b 120000 for a retargeted link — was written into the sub-PR worktree as a regular file whose content was the target path plus a newline. The sub-PR changed the entry's type (100644 blob where the feature branch had 120000), so git diff feature <sub-pr> showed link | 2 +-. Dangling symlinks also survived deletion because Path.exists() is false for them.

Fix

  • target_file_modes reads the mode from the index <old>..<new> MODE line as well as new mode/new file mode, and reports symlinks alongside regular files.
  • The worker creates a symlink with symlink_to(content.rstrip("\n")) (git stores the target as the blob), unlinks any existing entry first (regular file being converted, old link), never writes through an existing link, and removes dangling links on deletion.

Verified end to end in real repos: new / retargeted / deleted / dangling symlink, regular-file→symlink and symlink→regular-file conversions, and an ordinary modified file — every sub-PR git ls-tree -r is byte-identical to the feature branch.

Tests

target_file_modes: new symlink, retargeted symlink from the index line, unchanged regular mode from the index line. Worker: new symlink created as a link, regular-file→symlink conversion, plain-file fallback when no mode info.

Local review: 5/5. 456 tests pass, ruff clean. Stacked on #90.

@coderabbitai

coderabbitai Bot commented Aug 30, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 48 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 2b6f05cf-cc6c-4788-b3a6-21be06c22714

📥 Commits

Reviewing files that changed from the base of the PR and between a0d19ba and 8d016ba.

📒 Files selected for processing (4)
  • pr_split/cli.py
  • pr_split/diff_ops/reconstructor.py
  • tests/test_cli_helpers.py
  • tests/test_reconstructor.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

A symlink in the diff (new file mode 120000, or index ... 120000 for a
retargeted link) was written as a regular file containing the target
path plus a newline, so the sub-PR changed the entry type. Read the
mode from the index line too, create symlinks with symlink_to, never
write through an existing link, and unlink dangling links on removal.
@vitali87
vitali87 force-pushed the fix/materialize-symlinks branch from 60ae860 to fb198cc Compare August 30, 2026 14:00

Copy link
Copy Markdown
Owner Author

This change is already in main: it landed via the merge train #243, which includes this PR's head commit. GitHub can't mark a stacked PR merged when it lands that way, so I'm closing it as done.


Generated by Claude Code

@vitali87 vitali87 closed this Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants