Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delete operation #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheMeinerLP
Copy link

This PR adds the delete operations for changes. The current version only indicates in the debug that files may be deleted but the file operation is never executed because it is not present.
I have added a file walk at the relevant point which does a reverse walk from the base folder and checks if the folder is empty and deletes it. It also deletes the actual file.

Fix file deletion for empty folders
@covers1624
Copy link
Member

I will be investigating this later this week, not entirely sure if this is the correct approach to this specific issue, will need to actually sit down and evaluate this code again.

@TheMeinerLP
Copy link
Author

The general point is that files that do not receive binary code are not deleted. My approach is not perfect and was purely based on my intuition.

@covers1624
Copy link
Member

covers1624 commented May 29, 2024

Okay, so the issue with this approach is that it assumes the output is a directory. DiffPatch can operate on individual files, between zips/tarballs, folders on disk or even stdin/out pipes.

The code here to print the summary for removed files is correct, as you can remove text files with patches (+++ /dev/null), however, there is no standard format for deleting binary files from patches.

The other issue here is that currently DiffPatch assumes your output folder is empty prior to patching.

Code was recently added to detect if your base and output paths are the same and wipe the output directory.

Are you able to provide more information about your use case, perhaps a small cli reproduction?

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