Repository navigation
Keep files deeper than maxdepth when mv deletes the source - #2233
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AbstractFileSystem.mv(..., recursive=True, maxdepth=N)copies only the paths withinmaxdepth, but then callsself.rm(path1, recursive=True)with no depth limit. The whole source tree is removed, so files deeper thanmaxdepthare deleted without ever having been copied.Before / After
Before:
After:
Cause
copyexpands withmaxdepthand skips directories, butmvremoves the source with an unbounded recursiverm. Just passingmaxdepthtormis not enough, because it would then callrmdiron directories that still hold deeper files (on MemoryFileSystem that raisesFileNotFoundErrorfor implicit directories, orENOTEMPTYfor ones created withmkdir).Fix
When
recursive=Trueandmaxdepthis set,mvexpands the source with the samemaxdepthbefore copying. After the copy it removes only those files, then removes a directory within that depth only if it still exists and is empty. Withoutmaxdepth, behaviour is unchanged.Note: with
maxdepth, directories within that depth that are empty after the move (including ones that were already empty) are now removed.copyalready doesn't recreate directories at the destination whenmaxdepthis set.Filesystems that override
mvwith a native rename (local, sftp, ftp, etc.) are not affected and not touched.Tests
test_mv_recursive_maxdepth_keeps_deeper_filesinfsspec/implementations/tests/test_memory.py; it fails on current master (FileNotFoundErrorfor the deeper file) and passes with the fix.pytest fsspec: 1993 passed, 248 skipped, 2 xfailed.ruff check,ruff format --check,codespellclean.Related: #2205 touches nearby lines of
mv(same-path/nesting checks), so whichever lands second needs a small rebase. The destination layout atmaxdepth=1is #2161 and not changed here.