gregwebs/Shelly.hs

cp_r loops

Opened this issue · 3 comments

cp_r "A" "A/B" loops, creating an infinitely deep directory structure.
In contrast, cp -r A A/B on the shell will report an error.

cp: cannot copy a directory, 'A', into itself, 'A/B/A'

The problem with cp_r is that copied directories will be copied again. This can be solved by separating the procedure into two phases: making a list what has to be copied, and then copying.
I just fixed the same problem in Agda, see: agda/agda@443e603 .

Here is the test program for cp_r I used to confirm the bug:

{-# LANGUAGE LambdaCase #-}

import System.Environment (getArgs)
import System.Exit
import Filesystem.Path.CurrentOS (decodeString)

import Shelly (shelly, cp_r)

main :: IO ()
main = getArgs >>= \case
  [src , dest] -> shelly $ cp_r (decodeString src) (decodeString dest)
  _ -> do
   putStrLn "Usage: ShellyCpR src dest\nShould behave like cp -r src dest"
   exitFailure

I can confirm this issue. Although it probably would be considered a corner case (as you can't do it using cp -r, see below), I think it should probably be fixed (in the sense that it should behave equivalently to cp -r, i.e. it should fail).

If you run cp -r A A/B, you get this error message:

'A' -> 'A/B'
cp: cannot copy a directory, 'A', into itself, 'A/B'

I'd like to hear your thoughts on whether you think this should actually copy (by building a list first) or rather fail similarly to cp -r.

The cp man page does not specify the behavior in this case.
I opted for doing the copy in a safe way rather than failing, since it is compatible with what the alternative packages do that I list at agda/agda#2705 (comment) .
Actually ,cp -r does the copying, in addition to giving the error message.

@andreasabel I can confirm that in my manpage doesnt list anything, but I can not confirm that copying is actually done. My cp --version is cp (GNU coreutils) 8.25. Here's a test script:

#!/bin/sh
# Cleanup
rm -rf A
# Create test dir
mkdir -p A/B
echo "foo" > A/bar.txt
# Test copy
cp -r A A/B
# This should not error if copying is actually performed
cat A/B/A/bar.txt # <-- error
# NOTE: A/B/A is created
# Print version
cp --version 

What's your output?

Note that A/B/A is created (but A/B/A/bar.txt is not!). I'm inclined to considering it a bug in cp, but it's probably not very critical for most (if not all) people.

Although I get your point to other Haskell packages, as Shellys function is not called copyDir etc as the examples you pointed to but cp_r I'd expect it to have a behaviour very similar or identical to cp_r (which includes that ininite loops must not happen). Even besides that, I'd consider cp as the standard for file copying on unixoid systems and I believe most people would compare copyDir functions to cp as well. That doesn't prohibit a specific function like copyDirSelfCopy (name TBD) which lists files prior to copying.

Note that there are corner cases where listing the files is not feasible due to RAM usage (i.e. a huge number of files), although I'd say they are pretty rare in reality.

You can find the relevant section in the source code here:
https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c#n2267
It clearly lists goto un_backup;, which I'd consider a sign of aborting the copy considering what un_backup lists: https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c#n2881

Also, due to the comment, I'm pretty sure the current behaviour is actually intended by the coreutils authors.