Re: bug in copy-directory

[Stefan Monnier]

> If you have only one function copy-directory that work fine in case 2,
> it will fail in dired because dired already does a part of the job of
> this function (maybe because long time ago dired-create-files has been
> wrote on this bug), and if you make it work in dired it will fail in
> directs calls to copy-directory.

Right, part of the problem seems to be that dired tried to work around
some lack of support in files.el.  Maybe we should revisit dired's code
to clean it up in this respect.

> So to achieve the 3 cases above i propose:

> - Make a copy-directory1 function that work with dired (like ancient
>   code).
> - call copy-directory1 in dired-copy-file-recursive.
> - Make a copy-directory function/command that reuse copy-directory1
>   without extra args (like it was before, last arg is parents).

> This actually works and is IMHO the best solution.(see patch)

Yes, it seems like the best course for now (tho we usually use foo-bar-1
rather than foo-bar1).   Your patch has copy-directory's docstring
mention the non-existent argument `copy-as-subdir', tho.

Also, shouldn't we do the find-file-name-handler dance in copy-directory
rather than in copy-directory1?

Finally, shouldn't the copy-as-subdir dance be done within
copy-directory (sole place where the arg is non-nil), which then lets us
get rid of the argument altogether?

I.e. something like the untested patch below?


        Stefan


=== modified file 'lisp/files.el'
--- lisp/files.el       2011-02-06 04:59:06 +0000
+++ lisp/files.el       2011-02-07 18:54:15 +0000
@@ -4723,8 +4723,7 @@
                 directory 'full directory-files-no-dot-files-regexp)))
       (delete-directory-internal directory)))))
 
-(defun copy-directory (directory newname &optional keep-time
-                                parents copy-as-subdir)
+(defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
 This function always sets the file modes of the output files to match
 the corresponding input file.
@@ -4737,9 +4736,7 @@
 they don't exist.  When called interactively, PARENTS is t.
 
 When NEWNAME is an existing directory, copy DIRECTORY into a
-subdirectory of NEWNAME if optional arg COPY-AS-SUBDIR is
-non-nil, otherwise copy the contents of DIRECTORY into NEWNAME.
-When called interactively, copy into a subdirectory by default."
+subdirectory of NEWNAME."
   (interactive
    (let ((dir (read-directory-name
               "Copy directory: " default-directory default-directory t nil)))
@@ -4762,12 +4759,7 @@
       (unless (file-directory-p directory)
        (error "%s is not a directory" directory))
 
-      (cond
-       ((not (file-directory-p newname))
-       ;; If NEWNAME is not an existing directory, create it;
-       ;; that is where we will copy the files of DIRECTORY.
-       (make-directory newname parents))
-       (copy-as-subdir
+      (when (file-directory-p newname)
        ;; If NEWNAME is an existing directory, and we are copying as
        ;; a subdirectory, the target is NEWNAME/[DIRECTORY-BASENAME].
        (setq newname (expand-file-name
@@ -4777,8 +4769,15 @@
        (and (file-exists-p newname)
             (not (file-directory-p newname))
             (error "Cannot overwrite non-directory %s with a directory"
-                   newname))
-       (make-directory newname t)))
+                   newname)))
+
+      (copy-directory-1 directory newname keep-time parents))))
+
+(defun copy-directory-1 (directory newname &optional keep-time parents)
+  (if (not (file-directory-p newname))
+      ;; If NEWNAME is not an existing directory, create it;
+      ;; that is where we will copy the files of DIRECTORY.
+      (make-directory newname parents))
 
       ;; Copy recursively.
       (dolist (file
@@ -4789,11 +4788,11 @@
                       (file-name-nondirectory file) newname))
              (attrs (file-attributes file)))
          (cond ((file-directory-p file)
-                (copy-directory file target keep-time parents nil))
+             (copy-directory-1 file target keep-time parents nil))
                ((stringp (car attrs)) ; Symbolic link
                 (make-symbolic-link (car attrs) target t))
                (t
-                (copy-file file target t keep-time)))))
+             (copy-file file target t keep-time))))))
 
       ;; Set directory attributes.
       (set-file-modes newname (file-modes directory))