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

BF: archives upon strip - use rmtree which retries etc instead of rmdir #6064

Merged
merged 1 commit into from
Oct 9, 2021

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Oct 8, 2021

possible side-effect - if code logic would be flawed and leave some files
behind, rmtree would remove them as well (whenever rmdir would fail).
But since tested and never crashed before for that reason I think it should be
safe.

I have tried to troubleshoot/figure out exact problem with
#6058
but it did not reproduce, so decided to just make code robust

edit: the hope is that it closes #4496

possible side-effect - if code logic would be flawed and leave some files
behind, rmtree would remove them as well (whenever rmdir would fail).
But since tested and never crashed before for that reason I think it should be
safe.

I have tried to troubleshoot/figure out exact problem with
datalad#6058
but it did not reproduce, so decided to just make code robust
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Oct 8, 2021
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #6064 (645ce0e) into maint (662b0a5) will decrease coverage by 55.06%.
The diff coverage is 0.00%.

❗ Current head 645ce0e differs from pull request most recent head 6155f3b. Consider uploading reports for the commit 6155f3b to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##            maint    #6064       +/-   ##
===========================================
- Coverage   90.26%   35.19%   -55.07%     
===========================================
  Files         312      312               
  Lines       42189    42171       -18     
===========================================
- Hits        38080    14842    -23238     
- Misses       4109    27329    +23220     
Impacted Files Coverage Δ
datalad/support/archives.py 0.00% <0.00%> (-86.35%) ⬇️
datalad/tests/test_utils.py 0.00% <0.00%> (-97.20%) ⬇️
datalad/plugin/wtf.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/addurls.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_api.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_cmd.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/no_annex.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/support/digests.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_base.py 0.00% <0.00%> (-100.00%) ⬇️
... and 270 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 662b0a5...6155f3b. Read the comment docs.

@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Oct 8, 2021
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

Feels a bit hacky, but I can follow your reasoning. Should not be worse than what it was, and has the potential to help in some cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants