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

ENH: safe-guard --existing-replace for cases of populated directories #4147

Merged
merged 9 commits into from Feb 20, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Feb 13, 2020

A safe guard to Close #4145

If remote directory contains some files/directories, we would ask user whenever session is interactive or just raise an exception if not interactive

edit 1: I have been thinking about it and I maintain the opinion that --existing=replace is already an explicit indication from the user of the intent to replace (remove, recreate) a possibly existing remote dataset. Adding the interactive question or raising an exception (if not interactive) goes against the explicitly expressed desire to "replace". So, what if we do that (question or exception) only if remote location does not already have .git and .datalad/config? This way we would safe guard against some possible erroneous invocations, but would allow for automated (no interaction) and explicitly specified behavior.

TODOs (if agreed that it is the way to go)

  • add tests
  • ? add config option (or could be mode "force-replace"?) to be able to "proceed" instead of raising an exception in non-interactive mode or asking in interactive

@@ -287,6 +302,21 @@ def _create_dataset_sibling(
return remoteds_path


def _ls_remote_path(ssh, path):
try:
# yoh tried ls on mac
Copy link
Member Author

Choose a reason for hiding this comment

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

and a dd-wrt router -- both had ls -a1

@@ -287,6 +302,21 @@ def _create_dataset_sibling(
return remoteds_path


def _ls_remote_path(ssh, path):
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

True. Will integrate (I'm on it anyway)

@yarikoptic yarikoptic added UX user experience fix-implemented A fix is available, but has not been merged or released, yet. feedback-desired severity-important major effect on the usability of a package, without rendering it completely unusable to everyone severity-critical makes it unusable, causes data loss, introduces security vulnerability labels Feb 13, 2020
def _ls_remote_path(ssh, path):
try:
# yoh tried ls on mac
out, err = ssh("ls -a1 {}".format(sh_quote(path)))
Copy link
Member

@bpoldrack bpoldrack Feb 14, 2020

Choose a reason for hiding this comment

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

Why not just -A1, eliminating the need to filter out . and .. ourselves?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea! will do

@@ -163,6 +157,27 @@ def _create_dataset_sibling(
lgr.info(_msg + " Skipping")
return
elif existing == 'replace':
if path_children:
has_git = '.git' in path_children
_msg_stats = _msg \
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer a message that states the actual path it is about to remove rather than calling it "it". Particularly with urls (including replacements insteadOf) it might not be clear what's the thing we resolved it to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it, but then realized -- path is provided within _msg, so no need to duplicate, thus "It" is to stay ;)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, true. Sorry.

@codecov
Copy link

codecov bot commented Feb 15, 2020

Codecov Report

Merging #4147 into master will decrease coverage by <.01%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4147      +/-   ##
==========================================
- Coverage   89.54%   89.53%   -0.01%     
==========================================
  Files         276      276              
  Lines       36467    36509      +42     
==========================================
+ Hits        32654    32689      +35     
- Misses       3813     3820       +7
Impacted Files Coverage Δ
datalad/distribution/tests/test_create_sibling.py 98.71% <100%> (+0.09%) ⬆️
datalad/distribution/create_sibling.py 83.64% <94.11%> (-1.53%) ⬇️
datalad/core/local/create.py 94.11% <0%> (-0.66%) ⬇️
datalad/tests/utils.py 89.81% <0%> (-0.19%) ⬇️
datalad/support/annexrepo.py 86.07% <0%> (ø) ⬆️
datalad/support/tests/test_gitrepo.py 99.77% <0%> (ø) ⬆️
datalad/support/gitrepo.py 90.53% <0%> (+0.01%) ⬆️
datalad/tests/test_config.py 99.19% <0%> (+0.03%) ⬆️
datalad/distribution/update.py 97.91% <0%> (+1.04%) ⬆️

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 68f2667...718d685. Read the comment docs.

@yarikoptic
Copy link
Member Author

ok, pushed c55d702 where I extend doc for --existing to describe possible dangers and replacing exception in non-interactive mode with a warning.

This option is indeed as dangerous as rm. Comments in the test (not made by me BTW) equate it to "force"ful operation, which it is. It should be used with care, as rm. For me in zsh (might be my settings), zsh would request confirmation for multi files removal if interactive, and would do it silently in a "batch" mode:

$> mkdir -p dir; touch dir/{1,2,3}; rm dir/*
zsh: sure you want to delete all 3 files in /tmp/dir [yn]? y
$> zsh -c 'mkdir -p dir; touch dir/{1,2,3}; rm dir/*'
$>

Given that so far nobody shot themselves in the foot, dangers would be properly documented/described, $HOME removal so far has not happened I think this should suffice, and breeding double-force option would not really provide an additional safety blanket. Once again -- the --existing=replace shouldn't be used blindly.

@yarikoptic yarikoptic force-pushed the enh-safeguard-create-sibling-replace branch from b522742 to a13963d Compare February 15, 2020 15:01
@yarikoptic yarikoptic force-pushed the enh-safeguard-create-sibling-replace branch from a13963d to 25d4f41 Compare February 17, 2020 15:41
@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Feb 17, 2020
@yarikoptic
Copy link
Member Author

Pushed a fix and a test for interactive mode. AFAIK this is a viable candidate for the merge.

@yarikoptic
Copy link
Member Author

@mih, this PR awaits your feedback.

+ " It is %sa git repository and has %d files/dirs." % (
"" if has_git else "not ", len(path_children)
)
from datalad.ui import ui
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to leave this delayed import, but it doesn't actually help in this case:

#!/bin/sh

python <<\EOF | grep datalad | grep ui
import sys
import datalad.distribution.create_sibling
for k in sorted(sys.modules.keys()):
    print(k)
EOF
datalad.ui
datalad.ui.base
datalad.ui.dialog

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, will move up

default=False
)
else:
remove = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is my main concern with this PR. To me it seems like it'd be better to just make the replace-functionality interactive-only.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, will do. better be safe than sorry they say ;-)

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.

@mih
Copy link
Member

mih commented Feb 20, 2020

Cool! Works for me, thx!

@mih mih merged commit bc05be3 into datalad:master Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback-desired fix-implemented A fix is available, but has not been merged or released, yet. merge-if-ok OP considers this work done, and requests review/merge severity-critical makes it unusable, causes data loss, introduces security vulnerability severity-important major effect on the usability of a package, without rendering it completely unusable to everyone UX user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create-sibling --existing replace is dangerous
4 participants