Skip to content

Overwrite existing dest entries when merge_tree copies a symlink - #264

Open
00200200 wants to merge 2 commits into
jaraco:mainfrom
00200200:fix-merge-tree-symlink-overwrite
Open

00200200 wants to merge 2 commits into
jaraco:mainfrom
00200200:fix-merge-tree-symlink-overwrite

Conversation

@00200200

Copy link
Copy Markdown

Summary

merge_tree(..., symlinks=True) is documented to overwrite existing destination contents, and regular files already do that via copy2. Destination symlinks were created with os.symlink and were not replaced, so a second merge of the same tree (or a merge onto a dest that already has that name as a file/directory) raised FileExistsError.

Replace the destination first, then recreate the link.

Fixes #263

Test plan

  • tests/test_path.py::TestMergeTree::test_overwrite_existing_symlink fails on current main (FileExistsError) and passes on this branch
  • Retargeted symlink, file-to-symlink, and directory-to-symlink cases
  • Existing TestMergeTree cases still pass (PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python3 -m pytest tests/test_path.py::TestMergeTree)
  • Full tests/ suite: 112 passed, 8 skipped

merge_tree documents that it overwrites existing contents, but a second
sync with symlinks=True raises FileExistsError. Remove the destination
first so re-merging a tree with links succeeds.

Fixes jaraco#263
@sunxiuguo

Copy link
Copy Markdown

I checked this against base 67319bba and head fec0c684 in an isolated Python 3.12.14 process, using synthetic temporary directories. The intended distinct-directory re-merge works, but the new removal-before-readlink order has a destructive edge case when source and destination resolve to the same directory (including through a directory symlink):

import os
import tempfile
from path import Path

with tempfile.TemporaryDirectory() as temp:
    root = Path(temp)
    src = root / 'src'
    src.mkdir()
    Path('missing-target').symlink(src / 'link')
    alias = root / 'alias'
    src.symlink(alias)
    try:
        src.merge_tree(alias, symlinks=True)
    except OSError as exc:
        print(type(exc).__name__)
    print(os.path.lexists(src / 'link'))

Base prints FileExistsError, True; this head prints FileNotFoundError, False. dest.remove() removes the source link too, so the following source.readlink() cannot read it. The same happens with src.merge_tree(src, symlinks=True). This does not require the link target to exist. An independent check with a valid link also reproduced the source-link removal; the linked target file contents remained unchanged.

Self-copy need not become supported; rejecting source/destination directory identity before any mutation would preserve the current failure-without-source-loss behavior. A regression test should cover a directory alias, not just equal path strings. I have not run Windows tests or the project's complete tox matrix; this is a focused Linux reproduction, not a claim that every merge path is affected.

AI-assisted investigation and draft; independently reproduced in a second isolated check.

@00200200

Copy link
Copy Markdown
Author

Thanks for catching this. Fixed in 4045346: merge_tree now checks directory identity with samefile before creating the destination or changing any entries, and raises SameFileError for self-copies, including directory symlink aliases.

Added four regressions covering direct and aliased destinations with valid and dangling symlinks. All four fail on the previous head and pass with the fix. The 14 merge_tree tests pass, and the full tests plus module doctests pass on macOS with Python 3.10.21 and 3.12.11 (150 passed, 1 skipped each). Formatting and diff checks also pass. The upstream OS/Python CI matrix has not been run locally.

@sunxiuguo

Copy link
Copy Markdown

Verified 4045346 on Linux / Python 3.12.14: the original dangling-link reproduction and the independent valid-link directory-alias / /. cases now raise SameFileError while preserving the source link and target contents. The distinct-directory overwrite control still succeeds.

I also ran your exact TestMergeTree suite: 14 passed. Running the four new parametrized self-merge cases against the previous head fec0c684 gives 4 failures, so the tests catch the reported regression. This was a focused run with plugin autoload disabled, not the full tox or OS/Python matrix. The source-link-loss concern from my earlier comment is resolved in this head.

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.

merge_tree(symlinks=True) cannot overwrite an existing destination symlink

2 participants