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

extract_item: do not delete an existing directory if possible #7866

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

Conversation

intelfx
Copy link
Contributor

@intelfx intelfx commented Oct 10, 2023

A pre-existing directory might be a Btrfs subvolume that was created by the user ahead of time when restoring several nested subvolumes from a single archive.


I'm also interested in this feature being backported to Borg 1.2. I have a patch that applies on top of 1.2-maint.

See: #4233

A pre-existing directory might be a Btrfs subvolume that was created by
the user ahead of time when restoring several nested subvolumes from a
single archive.
@codecov-commenter
Copy link

Codecov Report

Merging #7866 (aee25c3) into master (9108039) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #7866      +/-   ##
==========================================
- Coverage   83.33%   83.12%   -0.22%     
==========================================
  Files          66       66              
  Lines       11853    11826      -27     
  Branches     2149     1866     -283     
==========================================
- Hits         9878     9830      -48     
- Misses       1393     1407      +14     
- Partials      582      589       +7     
Files Coverage Δ
src/borg/archive.py 83.45% <100.00%> (-0.23%) ⬇️

... and 19 files with indirect coverage changes

Comment on lines +866 to +867
elif not stat.S_ISDIR(item.mode):
os.rmdir(path)
Copy link
Member

@ThomasWaldmann ThomasWaldmann Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's rather unclear still whether the resulting directory after extraction will be correct (== as in the archive) in all cases:

  • simple attrs
  • xattrs
  • acls
  • (bsd/fs)flags

The existing directory could have some metadata set already, but the resulting directory must be exactly what we have in the archived item, not a mix up of fs item and archived item.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code that makes sure about this could be also useful later if we implement extracting into a non-empty directory (for more than the simplest cases, like the already implemented "continue an extraction").

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, thinking about it...

we currently more or less require extracting into an empty dir (exception: that "continue extraction" feature). we could also require that if there is any pre-existing directory inside the extraction directory, it must not have any special attrs set.

os.unlink(path)
# only remove a directory if it is conflicting
# preserve existing directories because they might be subvolumes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is subvolumes a feature implemented by other fs than btrfs?

if not, maybe we better call it "btrfs subvolumes" here, so it is more clear.

@ThomasWaldmann
Copy link
Member

A small test with an existing fs dir and then extracting "over" that would be good.

@ThomasWaldmann
Copy link
Member

@intelfx can you add tests?

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.

3 participants