`Trasnport` : `ssh` : `copy(recursive=False)` returns `OSError`

[khsrali]

Describe the bug

When using copy(recursive=False), is expected to: :param recursive: if True copy directories recursively, otherwise only copy the specified file(s)

But instead directly raises the OSError of cp command OSError: Error while executing cp. Exit code: 1, stdout: 'b''', stderr: 'b"cp: -r not specified; omitting directory ...''

Suggested solution

Although one can fix this with error handling, etc., I suggest to get rid of this recursive option, and always copy recursively. Since nobody has noticed this sofar, its probably because this option is not really used. If you want to copy everything in a directory except nested directories, just specify filenames one by one.

Your environment

• aiida-core version [2.4.1]: I think copy() in ssh plugin hasn't changed since

[sphuber]

What where the other arguments you specified to the copy call though? Did the remotesource happen to be a directory? Because in that case the behavior report is expected. Compare to the behavior of cp itself (in the following src is a directory):

$ cp src/ tmp
cp: -r not specified; omitting directory 'src/'
$ echo $?
1

you see the same message and the command errors (exit code is 1).

[khsrali]

I understand your desire for consistency with the cp command, but honestly, since we're working within a Python environment, it might be more intuitive to have the function behave more Pythonic. This

copy(`src/`, recursive=False)

either should be permitted or not permitted. Seems like we don't permit that, then what is the point of providing recursive option that never can be False?

The only permitted input for directories is recursive=True and for copying files it's even ignored.

[sphuber]

While

copy('src/', recursive=False)

may raise an error

copy('src', recursive=True)

doesn't, and nor does

copy('some.file', recursive=False)

To me it doesn't seem unreasonable that all possible combinations of argument values for a given function/method should never raise. There are plenty of functions where certain argument value combinations do not make sense.

I am not necessarily bound to keeping the behavior of cp but that was the original design. This was before I even joined AiiDA so @giovannipizzi may give more context for that decision.

I agree that the behavior of cp and the -r flag may not be immediately obvious and the reasons are mostly historical. But since, as you say, it is not currently actually causing problems, I would err on the safe side and not change things, because removing it may actually break things.

TLDR: it is not that I am against the change in principle but in the weighing of pro's and con's I don't see the justification for breaking things for changing something that doesn't cause any problems.

[khsrali]

There are plenty of functions where certain argument value combinations do not make sense.

In general I agree with you but the thing is copy('some.file', recursive=False) = copy('some.file', recursive=Ture) and don't use copy('src/', recursive=False) so technically recursive=Ture for all combinations.

However, as you say I understand the historical reasons, so I would also only err that recursive=False for directory is not permitted to emphasize there is no such thing as only copy files in the root directory and ignoring nested directories/files.. but this is paranoid anyways

[khsrali]

Sorry,... It was a mistake wanted to push to my fork, but since I've changed upstream before, it went directly to aiidateam/aiida-core. Didn't mean to, I can retrieve it now..