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

Fix directory rename hiding files from lower branches #152

Merged
merged 5 commits into from
Jan 6, 2025

Conversation

Tuupertunut
Copy link
Contributor

Fixes #91

This PR fixes the old issue #91 with directory rename. Previously after renaming a directory, only the contents of the highest branch were cow-copied up to the read-write branch, hiding all content the directory had in lower branches. In this PR a new function is introduced that can do a cow-copy recursively for a directory tree in every branch. Now if a file is renamed and it's a non-directory, we do a normal cow-copy, but for directories we do this recursive cow-copy instead. Everything should now work as expected from a proper directory rename.

Doing a cow-copy recursively for a whole directory is significantly more complex that for a single file:

  • We have to copy the directory from every branch because each of them might contain a different part of the directory's contents.
  • We have to recursively copy every file in the directory in every branch.
  • We have to check for each file if it already exists in the destination so we don't overwrite files from higher branches.
  • We have to check for each file that it doesn't have a whiteout in any of the higher branches so we don't accidentally reintroduce it by copying.

Caveat: directory renaming is now much slower than before. That's the price of it working correctly.

@rpodgorny
Copy link
Owner

rpodgorny commented Dec 20, 2024

hi @Tuupertunut ! thank you very much for your contribution! i will take a look at this some time next week. anyway, some tests seem to be failing (see below) - can you look into it meanwhile? thanks!

@Tuupertunut
Copy link
Contributor Author

Yep, that's the case where cow is not enabled. Directory renaming heavily uses cow-copying so I didn't consider that it sometimes might need to work without cow enabled and I'm still not exactly sure how it should behave. I'll figure something out.

@Tuupertunut
Copy link
Contributor Author

Fixed the case with cow disabled by introducing a similar shortcut in the recursive cow-copy function as in the non-recursive one. Now all the tests pass.

However I'm not sure the original shortcut code was working as intended and there was probably a bug already in the non-recursive function too: Rename fails when we have cow enabled, two read-write branches and we rename a file from the lower branch to a name already existing in upper branch. I'm not sure how unionfs is supposed to behave when cow is enabled but there are multiple read-write branches. It's not necessary to fix this right now but just something I discovered.

@rpodgorny
Copy link
Owner

thanks for the updates! i've quick-reviewed the code and it looks reasonable. i especially like the explanatory comments - great work!
...as for the tests, i'm not really sure why you changed the os.path.isdir to os.path.exists in some cases but i've changed it back (and more) in my "pre-merge" branch: https://github.com/rpodgorny/unionfs-fuse/tree/Tuupertunut-master
...so, i'll let this sink in for few more days and will merge it then. thanks again!

@Tuupertunut
Copy link
Contributor Author

Those assertions are all assertFalse, so asserting that a directory does not exist. In that case exists is a stronger assertion than isdir, which might allow there to be a file.

@rpodgorny rpodgorny merged commit 5dd6d28 into rpodgorny:master Jan 6, 2025
6 checks passed
@rpodgorny
Copy link
Owner

Those assertions are all assertFalse, so asserting that a directory does not exist. In that case exists is a stronger assertion than isdir, which might allow there to be a file.

yes, you are correct! i knew i must have overlooked something. ;-) ...so i've "fixed" the other checks and added a comment. other than that, i've merged your branch without changes. thanks again for your contribution!

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.

Directory rename hides files
2 participants