-
Notifications
You must be signed in to change notification settings - Fork 278
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 reference linking as an option for the post processor. #3633
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3633 +/- ##
===========================================
- Coverage 29.36% 29.35% -0.01%
===========================================
Files 274 274
Lines 35395 35419 +24
===========================================
+ Hits 10393 10398 +5
- Misses 25002 25021 +19
Continue to review full report at Codecov.
|
@medariox can you take a look at this? is it ready? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some minor things left to do and it can be merged IMO. Thanks!
medusa/helpers/__init__.py
Outdated
try: | ||
if reflink is None: | ||
raise NotImplementedError() | ||
reflink.reflink(src_file.encode('utf-8'), dest_file.encode('utf-8')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the encoding of src_file
and dest_file
really needed here? I've noticed that the reflink package supports Python 3 as well, so Unicode paths shouldn't be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because otherwise reflink will encode any string as ascii, which will fail on some foreign shows.
See this pull request (which I just opened because I forgot about this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. In this case, can you please add a comment referencing the issue?
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
% for cur_action in ('copy', 'move', 'hardlink', 'symlink'): | ||
<option value="${cur_action}" ${'selected="selected"' if app.PROCESS_METHOD == cur_action else ''}>${process_method_text[cur_action]}</option> | ||
% endfor | ||
% if pkgutil.find_loader("reflink") is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use single quotes here.
Thanks for the great addition! |
No problem, I'm happy it's merged! |
See #3394
This is a new pull request because I messed up my previous git repository.
This adds reflink support for CoW filesystems, currently only BTRFS, upstream is working on adding support for Apple's APFS and I'm looking into Windows' ReFS.