fix: recursively remove empty parent dirs after file delete#5
Merged
Conversation
_try_remove_empty_parent previously only checked one level up, leaving ancestor directories (e.g. diagrams/, notes/) as empty shells after all their contents were deleted via pull sync. Now walks up the tree until vault_path, removing each empty directory in turn.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the empty-parent-directory cleanup logic in both file and note syncing so that, after a file is deleted, all ancestor directories up to the vault root are removed if empty rather than only the immediate parent. Sequence diagram for recursive empty-parent directory removalsequenceDiagram
participant FileSync
participant FileSystem
FileSync->>FileSystem: delete file_path
activate FileSync
FileSync->>FileSystem: get parent = file_path.parent
loop while parent != vault_path
FileSync->>FileSystem: exists(parent)?
alt parent exists and is empty
FileSystem-->>FileSync: true, empty
FileSync->>FileSystem: rmdir(parent)
FileSystem-->>FileSync: success or OSError
alt OSError raised
FileSync-->>FileSystem: break loop
else no error
FileSync->>FileSystem: parent = parent.parent
end
else parent nonempty or missing
FileSystem-->>FileSync: false or nonempty
FileSync-->>FileSystem: break loop
end
end
deactivate FileSync
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_try_remove_empty_parentimplementations inFileSyncandNoteSyncare now identical; consider extracting this into a shared helper to avoid duplication and keep the behavior consistent in one place. - You now break on
OSErrorinstead of silently returning; if certain filesystem errors should be ignored (e.g., permission issues vs. real problems), it may be worth distinguishing/logging the exception to make debugging unexpected behavior easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_try_remove_empty_parent` implementations in `FileSync` and `NoteSync` are now identical; consider extracting this into a shared helper to avoid duplication and keep the behavior consistent in one place.
- You now break on `OSError` instead of silently returning; if certain filesystem errors should be ignored (e.g., permission issues vs. real problems), it may be worth distinguishing/logging the exception to make debugging unexpected behavior easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Problem
_try_remove_empty_parentonly walked up one directory level after deleting a file. This left ancestor directories (e.g.diagrams/,notes/,笔记同步助手/) as empty shells on disk even after all their contents were deleted via pull sync.Root cause: if a file at
diagrams/subdir/file.pngwas deleted, the code would clean updiagrams/subdir/but never check whetherdiagrams/itself was now empty.Fix
Changed the single-shot check to a
whileloop that walks up the tree, removing each empty directory in turn, stopping atvault_pathor the first non-empty directory.Affects both
FileSyncandNoteSync.Test plan
.fns_state.json, run pulldiagrams/,notes/,笔记同步助手/) are cleaned up after syncSummary by Sourcery
Bug Fixes: