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

yarikoptic · 2020-02-13T17:25:33Z

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

yarikoptic · 2020-02-13T17:28:35Z

datalad/distribution/create_sibling.py

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


+def _ls_remote_path(ssh, path):
+    try:
+        # yoh tried ls on mac


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

yarikoptic · 2020-02-13T17:28:55Z

datalad/distribution/create_sibling.py

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


+def _ls_remote_path(ssh, path):


candidate for commands abstractions: https://github.com/datalad/datalad/pull/4068/files#r379009046

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

bpoldrack · 2020-02-14T09:35:08Z

datalad/distribution/create_sibling.py

+def _ls_remote_path(ssh, path):
+    try:
+        # yoh tried ls on mac
+        out, err = ssh("ls -a1 {}".format(sh_quote(path)))


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

good idea! will do

bpoldrack · 2020-02-14T09:41:01Z

datalad/distribution/create_sibling.py

@@ -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 \


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.

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

Ah, true. Sorry.

codecov · 2020-02-15T04:11:48Z

Codecov Report

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

@@            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 · 2020-02-15T04:16:43Z

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.

…f there was err output

…tion to warning in non-interactive mode

Decision is based on the files listing

…teractive mode

yarikoptic · 2020-02-17T15:42:34Z

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

yarikoptic · 2020-02-18T15:59:28Z

@mih, this PR awaits your feedback.

kyleam · 2020-02-18T16:03:17Z

datalad/distribution/create_sibling.py

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


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

ok, will move up

kyleam · 2020-02-18T16:04:47Z

datalad/distribution/create_sibling.py

+                            default=False
+                        )
+                    else:
+                        remove = True


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.

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

Apparently was not touched in a big sweep

kyleam

Thanks for the updates.

mih · 2020-02-20T08:57:12Z

Cool! Works for me, thx!

yarikoptic commented Feb 13, 2020

View reviewed changes

bpoldrack reviewed Feb 14, 2020

View reviewed changes

yarikoptic force-pushed the enh-safeguard-create-sibling-replace branch from b522742 to a13963d Compare February 15, 2020 15:01

yarikoptic added 6 commits February 17, 2020 10:26

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

7c6e6b0

RF+ENH: use ls -A1 to not require filtering of ., ..; log a warning i…

a5d4ba2

…f there was err output

RF+ENH(DOC): extend doc for --replace with DANGER ZONE, replace excep…

de2d15c

…tion to warning in non-interactive mode

BF(TST): adjust test - we no longer blindly try rmdir on any directory

3594acd

Decision is based on the files listing

RF: sort imports in test_create_siblings.py

560247b

BF+TST: actually raise an exception if user answers "no" + TST for in…

25d4f41

…teractive mode

yarikoptic force-pushed the enh-safeguard-create-sibling-replace branch from a13963d to 25d4f41 Compare February 17, 2020 15:41

yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Feb 17, 2020

yarikoptic mentioned this pull request Feb 17, 2020

BF: Disable `create-sibling --existing replace' to prevent data loss #4146

Closed

kyleam reviewed Feb 18, 2020

View reviewed changes

yarikoptic added 3 commits February 18, 2020 13:37

RF: --existing=replace just refuse to remove in non-interactive mode

d56949b

RF: move import of ui to the top (delaying has no effect)

d7eed87

RF: create_sibling.py - harmonize imports

718d685

Apparently was not touched in a big sweep

kyleam approved these changes Feb 18, 2020

View reviewed changes

mih merged commit bc05be3 into datalad:master Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yarikoptic commented Feb 13, 2020 •

edited

Loading

yarikoptic Feb 13, 2020

yarikoptic Feb 13, 2020

bpoldrack Feb 14, 2020

bpoldrack Feb 14, 2020 •

edited

Loading

yarikoptic Feb 14, 2020

bpoldrack Feb 14, 2020

yarikoptic Feb 14, 2020

bpoldrack Feb 14, 2020

codecov bot commented Feb 15, 2020 •

edited

Loading

yarikoptic commented Feb 15, 2020

yarikoptic commented Feb 17, 2020

yarikoptic commented Feb 18, 2020

kyleam Feb 18, 2020

yarikoptic Feb 18, 2020

kyleam Feb 18, 2020

yarikoptic Feb 18, 2020

kyleam left a comment

mih commented Feb 20, 2020

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


		def _ls_remote_path(ssh, path):

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

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

Conversation

yarikoptic commented Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpoldrack Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 15, 2020 • edited Loading

Codecov Report

yarikoptic commented Feb 15, 2020

yarikoptic commented Feb 17, 2020

yarikoptic commented Feb 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleam left a comment

Choose a reason for hiding this comment

mih commented Feb 20, 2020

yarikoptic commented Feb 13, 2020 •

edited

Loading

bpoldrack Feb 14, 2020 •

edited

Loading

codecov bot commented Feb 15, 2020 •

edited

Loading